fix(daemon): keep close-time script-save failures from leaking the session/device claim - #1392
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head P2 — preserve the structured save failure when wrapping it after teardown. The teardown/rollback path itself looks sound. The claimed Android live repro is still not independently verifiable: it provides no exact commands/output, claim state, or before/after artifact hashes, which are the evidence requested in #1391. |
…se-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path.
d9bfa71 to
b695224
Compare
|
Addressed both points at P2 (structured details dropped) — Live-repro verifiability — re-ran end-to-end against a real Android emulator ( $ echo "already-published-content" > $BASE/published.ad
$ shasum -a 256 $BASE/published.ad
985ab9364b52aab6b0b92c77281203ad858b141fe68bf9f088abe1334060f112 published.ad
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts open com.android.settings \
--device "Pixel 9 Pro XL" --save-script=$BASE/published.ad --json
{"success":true,"data":{"session":"default", ... "message":"Opened: com.android.settings"}}
$ cat $BASE/claims/*.json
{"schemaVersion":1,"deviceKey":"local:android:none:emulator-5556", ...,
"session":"cwd:90ba66552d63190e:default", ...}
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts close --json
{"success":false,"error":{"code":"COMMAND_FAILED",
"message":"The session was closed, but its script was not saved: A file already exists at .../published.ad; ...",
"hint":"Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.",
"retriable":false,
"details":{"reason":"script_target_exists","path":".../published.ad"}}}
$ ls $BASE/claims/ # empty — device claim released despite the failed save
$ node ... session list --json
{"success":true,"data":{"sessions":[]}} # session deleted
$ node ... open com.android.settings --device "Pixel 9 Pro XL" --session other --json
{"success":true, ... "message":"Opened: com.android.settings"} # no DEVICE_IN_USE
$ shasum -a 256 $BASE/published.ad
985ab9364b52aab6b0b92c77281203ad858b141fe68bf9f088abe1334060f112 published.ad # byte-identical, untouchedConfirmed on 🤖 Addressed by Claude Code |
|
Review at |
|
Fair catch — re-ran the whole sequence with $ BASE=/tmp/repro1391-verify2
$ echo "already-published-content" > $BASE/published.ad
$ shasum -a 256 $BASE/published.ad
985ab9364b52aab6b0b92c77281203ad858b141fe68bf9f088abe1334060f112 published.ad
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts open com.android.settings \
--device "Pixel 9 Pro XL" --save-script=$BASE/published.ad --json
{"success":true,"data":{"session":"default","sessionStateDir":"...verify2/state/sessions/cwd_90ba66552d63190e_default", ... "id":"emulator-5556", "message":"Opened: com.android.settings"}}
$ cat $BASE/claims/*.json
{"schemaVersion":1,"deviceKey":"local:android:none:emulator-5556", ...,
"session":"cwd:90ba66552d63190e:default", "stateDir":"...verify2/state/sessions/cwd_90ba66552d63190e_default", ...}
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts session list --json
{"success":true,"data":{"sessions":[{"name":"default", ... "id":"emulator-5556", ...}]}} # session exists in the isolated daemon, before close
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts close --json
{"success":false,"error":{"code":"COMMAND_FAILED",
"message":"The session was closed, but its script was not saved: A file already exists at .../published.ad; ...",
"hint":"Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.",
"retriable":false,
"details":{"reason":"script_target_exists","path":".../published.ad"}}}
$ ls $BASE/claims/
(empty — device claim released despite the failed save)
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts session list --json
{"success":true,"data":{"sessions":[]}} # session deleted, same isolated daemon
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts open com.android.settings \
--device "Pixel 9 Pro XL" --session other --json
{"success":true,"data":{"session":"other","sessionStateDir":"...verify2/state/sessions/other", ... "message":"Opened: com.android.settings"}} # no DEVICE_IN_USE; sessionStateDir confirms it landed on the same isolated daemon, not a fresh/default one
$ shasum -a 256 $BASE/published.ad
985ab9364b52aab6b0b92c77281203ad858b141fe68bf9f088abe1334060f112 published.ad # unchanged from step 1
🤖 Addressed by Claude Code |
|
Re-review at |
…se-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path.
b695224 to
66ea4b7
Compare
|
Rebased onto latest Re-ran the full sequence on the rebased worktree, this time including the missing $ BASE=/tmp/repro1391-verify3
$ echo "already-published-content" > $BASE/published.ad
$ shasum -a 256 $BASE/published.ad
985ab9364b52aab6b0b92c77281203ad858b141fe68bf9f088abe1334060f112 published.ad
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts open com.android.settings \
--device "Pixel 9 Pro XL" --save-script=$BASE/published.ad --json
{"success":true,"data":{"session":"default", ..."id":"emulator-5556", "message":"Opened: com.android.settings"}}
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts session list --json
{"success":true,"data":{"sessions":[{"name":"default", ...}]}} # exists before close
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts close --json
{"success":false,"error":{"code":"COMMAND_FAILED",
"message":"The session was closed, but its script was not saved: A file already exists at .../published.ad; ...",
"hint":"Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.",
"retriable":false,
"details":{"reason":"script_target_exists","path":".../published.ad"}}}
$ ls $BASE/claims/ # empty
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts session list --json
{"success":true,"data":{"sessions":[]}} # deleted
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts open com.android.settings \
--device "Pixel 9 Pro XL" --session other --json
{"success":true,"data":{"session":"other","sessionStateDir":"...verify3/state/sessions/other", ...}} # no DEVICE_IN_USE
$ shasum -a 256 $BASE/published.ad
985ab9364b52aab6b0b92c77281203ad858b141fe68bf9f088abe1334060f112 published.ad # unchanged
# --- cleanup (previously missing) ---
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts close --session other --json
{"success":true,"data":{"session":"other","message":"Closed: other"}}
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts session list --json
{"success":true,"data":{"sessions":[]}}
$ ls $BASE/claims/ # empty
$ AGENT_DEVICE_CLAIMS_DIR=$BASE/claims AGENT_DEVICE_STATE_DIR=$BASE/state \
node --experimental-strip-types src/cli.ts daemon stop --json
{"success":true,"data":{"stopped":true,"mode":"graceful","cleanupConfidence":"known", ...}}
$ ps aux | grep "agent-device-1216-a79985/src/daemon.ts"
(no process bound to $BASE/state — confirmed by absence from the process list and a
daemon-shutdown.json left in $BASE/state as the graceful-shutdown record)No open sessions or daemon processes remain from this run. 🤖 Addressed by Claude Code |
|
Reviewed exact head
Keep |
66ea4b7 to
a5b5a7d
Compare
…se-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path.
…level #1391 coverage Review feedback on #1392 (thymikee), P2 items: - The close-time save-script failure's session.actions rollback (finalizeOrdinaryCloseScript) was left over from an earlier design where a failed save could keep the session alive for retry. It never does now — runCloseTeardownAndRelease always tears the session down regardless of the outcome — so there is no surviving session for a later write to duplicate the close action on. Drop the rollback; the durable events.ndjson entry (which the rollback never touched anyway) and the in-memory action now agree, both accurately recording that the close happened. - Add a request-router-level regression (request-router-typed-error.test.ts, alongside the existing repair-close BLOCKER 2 test it mirrors) proving the normalized JSON error shape a real client sees: top-level retriable:false, details.reason/path preserved, and the session torn down — not just that handleCloseCommand throws the right AppError when called directly.
|
Addressed at P2 (event-log durability) — realized the rollback ( P2 (router-level coverage) — added P1 (PUBLISHED variant not covered) — you're right, and I want to be precise rather than paper over it. Investigated three candidate mechanisms for how a PUBLISHED session's artifact could get rewritten, all ruled out:
I don't have the original #1381 validation session/event logs the item-2 report was based on, and per #1391's own bar ("both need their own live-repro verification... before a fix"), I'm not willing to write a fix for a mechanism I haven't reproduced. Narrowed the PR body accordingly — it no longer claims 🤖 Addressed by Claude Code |
|
Re-reviewed exact head P2 test gap: the behavior changed specifically to retain the durable The PR body now correctly narrows its claim: it covers the lingering claim and ARMED ordinary-session duplicate mechanism, not #1391's unreproduced PUBLISHED + stale-replay/interleaved-close rewrite. Keep #1391 open for that residual symptom. |
a5b5a7d to
c22a4a7
Compare
…se-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path.
…level #1391 coverage Review feedback on #1392 (thymikee), P2 items: - The close-time save-script failure's session.actions rollback (finalizeOrdinaryCloseScript) was left over from an earlier design where a failed save could keep the session alive for retry. It never does now — runCloseTeardownAndRelease always tears the session down regardless of the outcome — so there is no surviving session for a later write to duplicate the close action on. Drop the rollback; the durable events.ndjson entry (which the rollback never touched anyway) and the in-memory action now agree, both accurately recording that the close happened. - Add a request-router-level regression (request-router-typed-error.test.ts, alongside the existing repair-close BLOCKER 2 test it mirrors) proving the normalized JSON error shape a real client sees: top-level retriable:false, details.reason/path preserved, and the session torn down — not just that handleCloseCommand throws the right AppError when called directly.
…time save Review feedback on #1392 (thymikee), final P2 item: the previous commit removed the actions rollback because there's no surviving session to duplicate the close action on, but nothing actually asserted the durable events.ndjson action.recorded:close event stays put. Flush and read it back so a future rollback or event-order change can't silently recreate the in-memory/durable mismatch the removed rollback used to paper over asymmetrically.
|
Addressed at Extended the existing Thanks for confirming the P1 scoping — agreed on keeping #1391 open for the unreproduced PUBLISHED variant. 🤖 Addressed by Claude Code |
|
Re-reviewed exact head No rebase semantic drift found; router JSON coverage remains sound, all checks pass, and the PR body correctly leaves #1391's unreproduced PUBLISHED/interleaved variant open. |
…se-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path.
5a0701f to
050237e
Compare
…level #1391 coverage Review feedback on #1392 (thymikee), P2 items: - The close-time save-script failure's session.actions rollback (finalizeOrdinaryCloseScript) was left over from an earlier design where a failed save could keep the session alive for retry. It never does now — runCloseTeardownAndRelease always tears the session down regardless of the outcome — so there is no surviving session for a later write to duplicate the close action on. Drop the rollback; the durable events.ndjson entry (which the rollback never touched anyway) and the in-memory action now agree, both accurately recording that the close happened. - Add a request-router-level regression (request-router-typed-error.test.ts, alongside the existing repair-close BLOCKER 2 test it mirrors) proving the normalized JSON error shape a real client sees: top-level retriable:false, details.reason/path preserved, and the session torn down — not just that handleCloseCommand throws the right AppError when called directly.
…time save Review feedback on #1392 (thymikee), final P2 item: the previous commit removed the actions rollback because there's no surviving session to duplicate the close action on, but nothing actually asserted the durable events.ndjson action.recorded:close event stays put. Flush and read it back so a future rollback or event-order change can't silently recreate the in-memory/durable mismatch the removed rollback used to paper over asymmetrically.
…ot just the durable event Review feedback on #1392 (thymikee): the durable-event assertion alone doesn't catch a reintroduced session.actions.length = actionsBeforeClose rollback, because that event is queued (and durable) before the write even attempts — a regression there would leave the assertion passing while silently reintroducing the in-memory/durable mismatch. Retain the session object past handleCloseCommand (store.delete only drops the map entry, not the object a local variable still points at) and assert its actions array contains exactly one close entry, matching the durable event count. Verified by temporarily reintroducing the old rollback locally: this assertion fails (0 !== 1) where the prior durable-only check did not, then reverted.
|
Sharp catch — you're right the durable-event assertion alone couldn't detect that specific regression, since Retained the session object directly ( Verified this actually closes the gap you described: temporarily reintroduced 🤖 Addressed by Claude Code |
|
Ready at |
thymikee
added
the
ready-for-human
…ssion/device claim A close-time script write (implicit from `open --save-script`, or this close's own `--save-script`) that refuses to publish (e.g. a no-clobber target-exists AppError) threw uncaught out of `handleCloseCommand`, skipping lease release, device-claim release, and `sessionStore.delete` entirely — while the `close` action had already been recorded with no rollback. Live-repro'd over the real CLI against an Android emulator: this single gap explained both symptoms split out of #1384 into #1391 — a lingering `DEVICE_IN_USE` claim after a failed `close`, and a published `.ad` rewritten with duplicated trailing `close` lines when the same close was retried (each attempt re-recorded a `close` action on top of the one never rolled back from the prior failure). Catch the write failure, roll back the just-recorded `close` action (mirroring the existing repair-armed commit-failure pattern), and let teardown (lease release, device-claim clear, session delete) complete regardless — exactly as an ordinary platform-close failure already doesn't block them. The failure is still surfaced to the caller, but after teardown, with a corrected hint: retrying the same close is no longer meaningful since the session is now gone. Fixes #1391
…nder fallow's complexity gate CI's fallow code-quality check flagged handleCloseCommand (126 lines, 19 cyclomatic / 16 cognitive) and runSessionCloseTeardown (73 lines) as exceeding the large-function/high-complexity thresholds after the prior commit's fix. Extract runCloseTeardownAndRelease (teardown + lease release + claim clear + delete + ordered error surfacing) and buildCloseSuccessResponse (final response shaping) out of handleCloseCommand, and finalizeOrdinaryCloseScript out of runSessionCloseTeardown. No behavior change — same control flow, split into named, independently-readable steps; fallow now reports 0 complexity findings for this diff.
…se-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path.
…level #1391 coverage Review feedback on #1392 (thymikee), P2 items: - The close-time save-script failure's session.actions rollback (finalizeOrdinaryCloseScript) was left over from an earlier design where a failed save could keep the session alive for retry. It never does now — runCloseTeardownAndRelease always tears the session down regardless of the outcome — so there is no surviving session for a later write to duplicate the close action on. Drop the rollback; the durable events.ndjson entry (which the rollback never touched anyway) and the in-memory action now agree, both accurately recording that the close happened. - Add a request-router-level regression (request-router-typed-error.test.ts, alongside the existing repair-close BLOCKER 2 test it mirrors) proving the normalized JSON error shape a real client sees: top-level retriable:false, details.reason/path preserved, and the session torn down — not just that handleCloseCommand throws the right AppError when called directly.
…time save Review feedback on #1392 (thymikee), final P2 item: the previous commit removed the actions rollback because there's no surviving session to duplicate the close action on, but nothing actually asserted the durable events.ndjson action.recorded:close event stays put. Flush and read it back so a future rollback or event-order change can't silently recreate the in-memory/durable mismatch the removed rollback used to paper over asymmetrically.
…ot just the durable event Review feedback on #1392 (thymikee): the durable-event assertion alone doesn't catch a reintroduced session.actions.length = actionsBeforeClose rollback, because that event is queued (and durable) before the write even attempts — a regression there would leave the assertion passing while silently reintroducing the in-memory/durable mismatch. Retain the session object past handleCloseCommand (store.delete only drops the map entry, not the object a local variable still points at) and assert its actions array contains exactly one close entry, matching the durable event count. Verified by temporarily reintroducing the old rollback locally: this assertion fails (0 !== 1) where the prior durable-only check did not, then reverted.
…-script exclusion Following up on the comment-trimming pass already on this branch: the device-claim condition (!platformCloseError && !cleanupAggregate) and the two-line throw sequence right below it both needed a paragraph explaining why saveScriptError is excluded from one but not the other. Merge platformCloseError and cleanupAggregate into a single named blockingError — its name now states the exclusion the comment used to argue for, and the throw sequence collapses from two ifs to one. Trimmed the remaining long docblocks in this file the same way: state what's non-obvious in 1-3 lines instead of re-deriving it in prose.
08d5bc8 to
d5fb3f7
Compare
|
Coverage is failing on this push, but it's unrelated to this PR — confirmed reproducible on a clean Flagged as its own task rather than fixing inline here (out of scope for #1391): 🤖 Addressed by Claude Code |
|
1 participant
Summary
Fix ordinary session teardown when close-time script persistence fails.
closeaction.ios-system-uihelp-conformance case that was failing the inherited Coverage gate.This covers the lingering
DEVICE_IN_USEfailure and duplicate-close mechanism for an ARMED ordinary recording. It does not claim the unreproduced PUBLISHED + interleaved stale-replay variant; #1391 remains open for that evidence-dependent case.Validation
pnpm check:toolingpnpm check:fallowpnpm test:smoke: 14 passed, 1 opt-in web smoke skippedAddresses #1391 for item 1 and the ARMED-session duplicate-close mechanism.