Skip to content

fix: return settled batch and replay deadline results - #3858

Merged
thymikee merged 12 commits into
mainfrom
t3/daemon-deadline-options
Oct 21, 2026
Merged

thymikee merged 12 commits into
mainfrom
t3/daemon-deadline-options

Conversation

@thymikee

@thymikee thymikee commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Summary

Give batch and replay a 60-second daemon work budget inside the 90-second client envelope; replay --timeout X uses X/X+30 seconds. Settled failures retain typed timeout, completed-step progress, and replay source. Existing cancellation handlers run; completed success, earlier explicit cancellation, and direct test retain their behavior. Refs #3325.

This follow-up changes one Apple test file: the deadline fires after the fake runner admits longPress, then the test requires two status polls for its exact command ID and a cleared charge. Production code is unchanged from 9c61f5a9. Cumulative diff from the PR merge base: 37 files, +1,590/-203. No physical retirement or resource-operation follow-up is included.

Validation

Published head 70b86fa: exact-head pnpm check:affected --run --head passed (run-08GQma, exactCommit=true, 7,347 related tests). The focused test passed under coverage instrumentation. The same test on disposable old production 70a57a7c9a failed at the intended missing journal polls. Fresh independent Sol review found no issues. Receipt, negative, review, and prior live evidence.

Coverage at 9c61f5a9 failed in job 119238930459; new-head CI completed: 17 successful checks, one skipped, one neutral. Earlier iPhone simulator and Android live results remain attributed to 9c61f5a9, whose production code is unchanged here. Physical native stop and universal settlement remain unproven.

@thymikee

Copy link
Copy Markdown
Member Author

Validation evidence at 1522466baa43c2b2581b95aab808e3a3b7a5b5be:

pnpm check:affected --run --head 1522466baa43c2b2581b95aab808e3a3b7a5b5be
receipt: run-6S1OVs
status: passed
exactCommit: true
Test Files 899 passed (899)
Tests      7326 passed (7326)

Format, lint, typecheck, layering, Fallow, build, related tests, documentation links, and prose checks passed. Unit/provider selections were covered by related tests. Coverage, platform smoke, and remaining GitHub-owned checks are still CI obligations; parked replay lanes were skipped.

Regression comparisons used disposable checkouts with frozen installs and builds, then removed them:

  • pnpm exec vitest run --project provider-integration test/integration/provider-scenarios/ios-fold.test.ts -t 'worst-case fold returns': base f8c60ca188 + test patch: 5 failed / 2 filtered; 30969b4c2d: 5 passed / 2 filtered. Defaults consumed the old 90-second client envelope; explicit replay budgets lacked the 30-second margin.
  • pnpm exec vitest run --project provider-integration test/integration/provider-scenarios/composite-deadline.test.ts: 30969b4c2d + test patch: 2 failed (missing request signal/typed deadline result); 209c7e52a9: 2 passed. All nine Android/fold provider scenarios also pass at 1522466baa.

Live changed-route evidence at 209c7e52a9, Android emulator-5554, helper 0.21.30-dev, persistent session transport:

batch: longpress @e7, durationMs=120000
exit=1; elapsed=60724ms
error.code=COMMAND_FAILED
error.details.reason=daemon_request_timeout
error.details.timeoutMs=60000

The same daemon (PID 12454) and session answered a subsequent snapshot; helper retirement was confirmed by a fresh helper session. The exact session was closed, claims were empty, and the isolated daemon was stopped. Device ownership was released.

Independent Sol review found the Android cancellation P1 in the first round; it was fixed. Subsequent reviews found no issues, including final signal-identity equivalence with the live-tested version. The standalone longpress envelope ledger remains #3326.

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-21 14:47 UTC

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.49 MB 5.50 MB +4.6 kB
Package (unpacked) 5.49 MB 5.50 MB +4.6 kB
Package (download) 1.67 MB 1.67 MB +1.5 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 39.3 ms 39.8 ms +0.5 ms
CLI --help 81.3 ms 79.7 ms -1.5 ms

@thymikee

Copy link
Copy Markdown
Member Author

Checks are green (19 of 19) and there are no conflicts, but I found one problem at 1522466 that should be fixed before merge. When the deadline fires, withRequestDeadline calls signal.throwIfAborted() at request-cancel.ts#L324. That replaces the response that runBatch already built (batch.ts#L146-L175) with a bare AppError whose details are only {reason, timeoutMs, requestId} (see request-router.ts:263). Replay failures lose the failing line the same way. The same lines also turn a batch that finished every step just after the deadline into a failure with no results.

For example, with the batch [press A, fill B, longpress 120000], the caller gets a generic COMMAND_FAILED with no executed count, no partialResults, and no dispatched: 'unknown'. It cannot tell that press and fill already reached the app, so a re-run repeats them. A late but complete success is also reported as a failure.

The rule should be that the deadline may add reason: daemon_request_timeout and timeoutMs to the composite's result, but must never replace it. When the task settles with a DaemonResponse, keep that response. If it failed, merge {reason, timeoutMs} into its existing error.details. If it succeeded, return it unchanged. Use the thrown deadline error only when the task throws without a response. Please add assertions on details.executed, details.partialResults, and details.dispatched to the multi-step batch provider scenario.

Not blocking, and you can take or leave these: AndroidTouchInjector (adb-transport.ts:159) has no signal parameter, so a provider-native touch at touch-executor.ts:31 keeps running after the abort, and an optional signal argument (or a narrower documented guarantee) would fix that; the daemonDeadlineMs early return in timeout-policy.ts:42 means batch envelopeMs and replay budget.envelope: 'margin' are never read, so a budget variant that carries the deadline would stop the two declarations from drifting; isRequestCanceled now also returns true for a deadline, which changes readers such as request-execution-scope.ts:360 and lease.ts:111 without a test, so naming that case or keeping a separate predicate would be clearer; the router uses req.meta!.requestId! and resolves the timeout twice, so resolving it once in handleRequest is simpler; and the deleted replay registry comment about delegated re-entry looks unrelated to this fix.

I did not run the regression tests on base or head. I checked their regression validity by reading the pre-change code paths. The live Android evidence is your report at 209c7e5, and the later commits change only lock wiring and tests. I did not check that every batchable or replayable step (install, type, scroll, iOS runner calls) honors the request signal within the 30 s margin. I also found no in-repo AndroidTouchInjector implementer, so I cannot tell whether the provider gap is reachable.

@thymikee

Copy link
Copy Markdown
Member Author

Follow-up to #3858 (comment) and the explicit resource decision #3325 (comment).

At dedab1360527b42accd07647ef253ffbd21eee46, withRequestDeadline preserves a settled DaemonResponse. Successful responses return unchanged. Failed responses retain existing diagnostics, batch progress and replay source; timeout details are merged. A deadline exception is used when work throws without a response, including when synchronous work crosses the cutoff before the timer can run. Earlier explicit cancellation retains its reason. Batch remains 60s inside 90s; replay remains X/X+30s; direct test stays unbounded.

The original requested multi-step progress assertions, late-success identity, failed-response merging, and replay source tests are retained. Their negative comparisons keep the original base SHA attribution. The follow-up resource, IME, source-header, Apple lifecycle and late-throw comparisons are separately attributed in the evidence.

The selected resource policy refuses unsupported operation shapes before runtime admission or native/resource effects. It uses existing domain plans and resource ownership. Existing resources and recovery manifests retain standalone cleanup authority. Observations are assessed separately. Close/reopen, pre-handler expired leases, and every owned stale manifest preflight before cleanup. Android test-IME assessment reads its existing owner map; standalone restoration remains unchanged. Audio status with an existing host probe is refused because it can dispose that probe. All nested test source manifests preflight attempt timeouts; admitted nested tests share the original cutoff and restoration allowance.

Typed replay refusal details retain shape, standalone alternative, dispatched=no, and source diagnostics without post-failure native capture. Earlier completed batch results remain present. The optional review comments were assessed at their owners: declared budgets now carry the daemon cutoff, the router resolves it once, and request cancellation preserves typed reasons. Provider-native operations with an unproven deadline contract are refused or explicitly outside the guarantee. No pending-resource API, detached deadline race, global ADB cancellation, increased envelope, repeated restoration allocation, or forced durable cancellation was added.

Full evidence: full exact-head receipt, fresh independent FULL R14 assessment, original brief and all R1–R13 dispositions, regression logs with SHA attribution, final native builds, changed-route live checks and cleanup.

The independent reviewer found no concrete findings and no materially smaller design preserving the required behavior and coverage across 134 files / 6,871 gross lines. Injected facets, 52 named composite cases and simulated ADB do not prove every command or native backend. Physical Apple, Mac, Linux, web, Harmony, Vega, remote and arbitrary custom-provider native settlement remain unproven. #3325 remains open pending merge/acceptance; #3326 is separate. CI is pending at publication; the parent owns subsequent checks/events.

@thymikee

Copy link
Copy Markdown
Member Author

Independent package-growth assessment at dedab1360527b42accd07647ef253ffbd21eee46: no actionable smaller-design finding.

The exact-head Size run reports +19,748 unpacked bytes and +5,910 download bytes. Its pack manifests place the unpacked increase entirely in generated dist files; tests, docs, assets, and dependencies add no measured package bytes. Three new chunks total 4,273 bytes; existing chunks also grew, including mechanics.js (+3,616) and runner-client.js (+1,704). Startup median: --version unchanged at 37.6 ms; --help 79.1 → 76.1 ms.

The fresh read-only Sol reviewer assessed the complete diff: 2,690 gross production lines (+1,416 net), 4,109 test/fixture lines, 63 documentation lines, and 9 metadata lines. Resource admission reuses existing runtime plans and ownership; deadline/restoration use existing request registration; native-stop confirmation remains with the runner owner. No dependencies were added. No concrete smaller alternative preserved the required cancellation, indirect-disposal refusal, all-source preflight, and cleanup authority.

Chunk sizes locate growth but do not establish exact source attribution or savings from an unbuilt alternative. The checkout remained clean; no new builds, tests, or device runs were performed for this assessment. R14 and live evidence retain their original attribution.

@thymikee

Copy link
Copy Markdown
Member Author

Current-head CI at 755df85a02aa83d42f76c2a93bbdea8cb9f27816 has completed: 19 successful checks, two skipped, and one neutral; no failed or pending checks.

The previously failing Android API36 Smoke lane now passes after restoring signaled Back to the ADB route. Coverage and Integration Tests also passed, along with Apple/Linux smoke and the remaining selected checks.

The PR is clean and mergeable, with no unresolved review threads. No checks were manually rerun and no validation limits changed. The evidence retains the original failed-head attribution and the fixed-head regression, review, receipt, and live proof. #3325 remains open until merge and acceptance verification.

@thymikee

Copy link
Copy Markdown
Member Author

The earlier findings from 1522466 are fixed at 755df85, but one new problem remains. The composite-deadline assertions now cover the 1522466 code path, and the Android Back route is restored. CI shows 22 checks with none failing, and the earlier Android API36 smoke failure now passes. There are no conflicts.

On a physical iPhone or Apple TV, a daemon-deadline abort of a runner command leaves the device unusable until the daemon restarts. confirmRunnerNativeStop throws runner_stop_unconfirmed for any device that is not a simulator or macOS (runner-disposal.ts#L357). The session then stays in runnerSessions in the draining state. isRunnerSessionServing throws for every later request (runner-session.ts#L429). stopRunnerSessionInternal returns early, so close never deletes the entry (runner-session.ts#L627). A batch such as wait 55000; longpress @e 10000 on a physical device crosses the 60 s deadline and hits this. Close and reopen cannot recover it. That is the daemon restart #3325 removes, and it contradicts "daemon and its sessions remain available". Please give an unconfirmed deadline stop one exit that works without a restart. Close and the next ensureRunnerSession should re-run confirmation and delete the entry once it succeeds. Physical devices also need a confirmation they can meet, or runner-bound steps should be refused before dispatch inside bounded composites. A test should cover unconfirmed stop, then close, then a fresh open that starts a runner.

Not blocking, and you can take or leave these: the diff is 6,149 added and 864 removed lines across 134 files, over the 2,500-line cap in docs/agents/pull-requests.md; confirmStopped is derived from the log reason daemon_request_timeout in invalidateRunnerSession, so a typed retirement mode from settleFailedRunnerSend would be safer; and the failed-response merge in request-cancel.ts overwrites any existing error.details.reason, so a step's own typed reason such as runner_stop_unconfirmed may be lost at batch level (I did not read batch.ts, so I am not sure of this one).

Could this be stacked as four independently mergeable layers? The first would be the deadline core: withRequestDeadline with response preservation, the timeout-policy margin, between-step checks in the batch and replay loops, and signal coverage for the long steps named in the issue (fold, longpress, install). That closes the Done-when of #3325. The second would be bounded-composite resource refusal, declared as a trait on the command descriptors rather than a command-name switch in composite-resource-admission.ts. The third would be the Android per-op signal threading. The fourth would be Apple confirmed-stop retirement with the recovery path above. Layers 2 to 4 only narrow the window where a step ignores the signal, and none is needed to stop the deadline from resetting the daemon. Layer 2 needs its descriptor trait first, which would amend ADR 0008. Layer 4 needs a runner retirement state with an exit first, under ADR 0019.

I did not run tests. I judged the regression coverage by reading the 1522466 code path against the new assertions. The physical iOS finding comes from reading the runner code at 755df85, not from a device run. The simulator and Android live evidence in the gist is attributed to dedab13, plus an Android Back check at 755df85, and I did not re-run it. I also did not audit every batchable or replayable step for signal coverage within the 30 s margin.

Before merge, the Apple unconfirmed stop needs an exit, then the stacking question needs an answer.

@thymikee

Copy link
Copy Markdown
Member Author

Confirmed the draining-state problem at 755df85a: an unconfirmed retirement has no retry through close or ensureRunnerSession, and the current confirmation function rejects physical devices unconditionally. Green simulator CI does not cover that path.

I’m addressing recovery at the runner owner while preserving the lease and replacement fence until native stop is actually confirmed. Physical-device admission also needs an honest capability boundary if no supported stop confirmation exists. The regression will cover unconfirmed stop, a subsequent confirmation attempt, close, and a fresh runner; simulator evidence will remain distinct from physical-device evidence.

I’m separately assessing the proposed four-layer split against the actual deadline dependencies and the repository’s gross-diff rule. I’ll answer with concrete layer ownership and prerequisites before restructuring the published branch. The typed retirement-mode suggestion and preservation of a step’s own typed failure details are included in the assessment.

@thymikee

Copy link
Copy Markdown
Member Author

The independent split assessment recommends smaller review layers, with deadline activation last. The proposed core-first layer cannot yet close #3325: withRequestDeadline aborts the signal but still awaits the operation, while the client retains the 30-second margin and reset policy. An operation that ignores cancellation or an unbounded resource teardown can still overrun that margin. The admission and native-retirement work therefore supplies prerequisites for the guarantee.

I’m preparing this dependency order locally:

Layer Owning concern
1 Neutral request registration, inherited cutoff, and restoration allowance
2 Descriptor-owned bounded-admission category, existing domain-plan projection, read-only disposal preflight, and ADR 0008
3 Android input/text/IME/helper/viewport/keyboard cancellation
4 Android capture/restoration/lifecycle/settings/runtime-fact cancellation
5 Apple launch cancellation and confirmed-stop retirement with a recovery exit, under ADR 0019
6 Activate batch/replay deadlines, response/progress preservation, queued-lock handling, and public help/docs

The Android changes alone currently account for roughly 3,789 gross lines, so one Android layer would still exceed the review cap. Tests and fixtures will follow their owners. Each layer must build and remain safe when merged after its prerequisites; the #3325 completion claim belongs to the final activation layer.

The admission trait will select the existing domain-plan owner rather than duplicate its operation parsing or resource state. I’ll report both parent-relative review size and cumulative origin/main size; stacking is not a silent cap waiver. The published branch remains intact while the layers and Apple recovery fix are prepared and validated.

@thymikee

Copy link
Copy Markdown
Member Author

Following the user's concern about scope and complexity, the unpublished six-layer reconstruction is on hold. Its branches and unfinished work are preserved; no stack branch has been pushed.

I reread #3325's completion criteria: worst-case fold provider scenarios in batch and replay must return typed daemon results rather than client timeout/reset, with direct test remaining unbounded. A fresh isolated candidate is extracting the deadline, result/progress preservation, and necessary named long-step cancellation from the base. It must state its actual backend limits rather than claim universal native settlement.

The blanket physical iOS/tvOS runner refusal has not been accepted and will not be published as a workaround. The new permanent physical draining fence is distinct from the preexisting limitations of proving physical native stop. The local safety candidates and the source finding that terminal journal status also needs an idle/busy check remain preserved for review; neither is physical-device proof.

The published 755df85a branch is unchanged. Its green CI does not resolve the new review blocker or authorize merging it.

@thymikee
thymikee force-pushed the t3/daemon-deadline-options branch from 755df85 to 9c61f5a Compare October 21, 2026 13:06
@thymikee thymikee changed the title fix: enforce daemon deadlines for batch and replay fix: return settled batch and replay deadline results Oct 21, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Replaced the superseded 755df85 branch with the smaller, reviewed 9c61f5a9b1f6c4f44afbe1cf0c40dc9b36d2c490, based on current main 23fcd9ebf4. The old head remains preserved locally and its published evidence stays attributed to that SHA. The replacement was pushed once with an exact old-head lease and tags disabled.

Claude’s scope consultation confirmed that #3193 already preserves sessions when the daemon answers its liveness probe. The remaining core gap is a typed settled timeout with progress/replay source and a client response margin. This patch uses ordinary typed request cancellation internally, so existing Apple journal drain and Android cleanup run. It does not add forced physical-stop confirmation, a permanent draining fence, or a blanket physical-device refusal. This addresses the new physical recovery regression raised in review 6313914386 by excluding that introduced retirement design, rather than clearing an uncertain old lease.

The diff is now 37 files / 1,764 gross lines; production +288/-55. Fresh full R3 found no findings. Exact-head run-4ZYbSM passed 7,347 related tests with exactCommit=true and checkoutChanged=false. Both assigned source-daemon live batches (iPhone simulator and Android emulator) crossed the 60-second budget during longpress, retained completed wait progress, skipped step3, and then accepted native input and snapshots in the same session. Sessions, daemons, panels and claims were cleaned up.

New exact-head receipt, independent review, negative proofs, builds and live logs. Physical devices were unavailable. Existing cancellation may exceed the margin; transport-timeout/liveness fallback remains. No native-stop/no-overlap guarantee is claimed. Accepted resource-operation-shape refusal remains a separate follow-up, so #3325 stays open. Actual new-head CI must complete; prior green 755 checks do not validate this replacement.

@thymikee

Copy link
Copy Markdown
Member Author

The actual-head CI Size report at 9c61f5a9 crossed the repository’s 3 kB review threshold. A fresh independent focused assessment found no actionable smaller design.

The CI artifact measures +4,569 B unpacked and +1,551 B downloaded. All unpacked growth is in 14 generated JavaScript chunks; package file counts and dependency bytes are unchanged. Command metadata/help (+1,633 B) and request deadline handling (+1,150 B) account for most growth. No accidentally packaged tests/assets or added dependency was found.

Full assessment and raw CI JSON are now in the replacement evidence bundle. Startup sample ranges overlap, so the lower measured medians are not a claimed speed improvement. No source changes, builds, tests, or devices were needed for this assessment. Remaining CI is still running; prior-head results are not transferred.

@thymikee

Copy link
Copy Markdown
Member Author

Coverage follow-up is published at 70b86fa1800d40b0fd4b891be0d385f164252be3 (fast-forward from 9c61f5a9). The previous Coverage job failed because its real 25 ms test deadline expired before the instrumented fake runner admitted longPress. The one-file test fix waits for that HTTP admission, then fires the real host-kit deadline callback on a controlled clock. It still requires the typed external timeout, two status polls for the same command ID, and no outstanding charge.

The synchronized test fails on disposable old 70a57a7c9a production with zero journal polls and passes at this head under focused coverage instrumentation. Exact-head run-08GQma passed 7,347 related tests with exactCommit=true; a fresh independent Sol review found no issues. New receipt, negative, and review. Production code is unchanged from 9c61f5a9, so its simulator/Android live evidence remains attributed there. New-head CI is running; the failed old Coverage job is not a pass.

@thymikee

Copy link
Copy Markdown
Member Author

The code at 9c61f5a looks ready. The earlier evidence request is now met: the live runs on the iPhone 17e simulator and the Android emulator are in the author's gist at this commit. No physical iPhone or Apple TV run exists. I accepted that because this head changes no Apple runner production code, and the deadline uses the same canceled-journal drain as an explicit cancel. I did not observe a physical-device drain at this head. I read the code and the new tests but did not run them.

Not blocking, and you can take or leave these: resolveDaemonRequestTimeoutMs(req) runs once in handleRequest and again in handleRequestOnLedger, which then needs the req.meta!.requestId! assertions (request-router.ts#L192), so resolving it once and passing the id and timeout on together would remove them. replay --timeout now sets a daemon limit with the client waiting 30s longer, and batch gains a 60s daemon budget, so a .changeset entry would record that change for CLI, MCP and Node callers (replay/index.ts#L69).

A newer head, 70b86fa, came in after this review. It changes only runner-host-deadline-journal.test.ts, to fix the Coverage failure you traced to that test's deadline timing. I will check it and the rerun CI on the next pass. I know of no conflicts.

@thymikee

Copy link
Copy Markdown
Member Author

CI is complete at exact head 70b86fa1800d40b0fd4b891be0d385f164252be3: 17 successful checks, one skipped, one neutral, with no failed or pending check. Coverage, Integration Tests and all three Smoke checks passed. The PR is CLEAN/MERGEABLE.

The old Coverage failure at 9c61f5a9 remains attributed to that head; this is a new automatic CI run after the deterministic test fix, not a rerun of the failed job. Current-head CI status and review/regression evidence. Production is unchanged from the attributed simulator/Android live runs at 9c61f5a9; the physical-device and native-settlement limits remain.

No further core source change is needed from these results. The separately accepted resource-operation refusal is still being prepared, so #3325 stays open.

@thymikee

Copy link
Copy Markdown
Member Author

The PR is ready for human review at 70b86fa. The earlier review at 9c61f5a was clean, and this update changes only one test. CI is green. The live evidence from 9c61f5a still covers the behavior because the production code has not changed. I read the test change but did not run it. No conflicts.

Not blocking, and you can take it or leave it: in runner-host-deadline-journal.test.ts the timer spy treats the first setTimeout call after install as the deadline timer and asserts a 25 ms delay (L24). That cannot misfire today, but would matching on the 25 ms delay instead of call order make a future failure easier to read?

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 21, 2026
@thymikee
thymikee merged commit 6ac6611 into main Oct 21, 2026
19 checks passed
@thymikee
thymikee deleted the t3/daemon-deadline-options branch October 21, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant