fix(daemon): keep close-time script-save failures from leaking the session/device claim#1392
fix(daemon): keep close-time script-save failures from leaking the session/device claim#1392thymikee wants to merge 3 commits into
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. |
…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.
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 |
1 participant
Summary
DEVICE_IN_USEclaim right afterclose, and a published.adrewritten with duplicated trailingcloselines under interleaved closes — needed their own live-repro verification (real daemon over CLI, not the in-process provider harness) before a fix. This does that verification and fixes the shared root cause.handleCloseCommand's ordinary (non-repair) teardown path, the close-time script write (sessionStore.writeSessionLog, triggered implicitly byopen --save-script, or by this close's own--save-script) can throw an uncaughtAppError— most commonly a no-clobber refusal when the target file already exists. That throw propagated straight out ofhandleCloseCommand, skipping lease release, device-claim release, andsessionStore.delete()entirely, while thecloseaction had already been appended to the session's history with no rollback.DEVICE_IN_USE: the session and its device claim survive the failed close, so the next command targeting that device sees it as still owned by the stale session.closelines / rewritten artifact: retrying the close (e.g. with--force) re-records anothercloseaction on top of the one never rolled back from the failed attempt, then successfully overwrites the target with both — a script with two trailingcloselines.open --save-script=<existing-file>thenclose(orclose --save-script) reliably reproduced both symptoms onmain; verified they're gone after the fix.Fix
runSessionCloseTeardownnow catches the ordinary-session script-write failure, rolls back the just-recordedcloseaction (mirroring the existing repair-armedcommitRepairBeforeCloserollback-on-failure pattern), and returns it separately fromplatformCloseErrorinstead of letting it escape uncaught.handleCloseCommandlets teardown (lease release, device-claim clear, session delete) complete unconditionally on a script-save failure — exactly as an ordinary platform-close failure already doesn't block them — then surfaces the failure to the caller after teardown, via a newtoOrdinaryCloseSaveScriptFailurewrapper with a corrected message/hint: the sharedpublishHealedScriptAtomicallywording ("retry close --save-script") describes the repair-commit retry contract, which no longer applies here since the session is already gone by the time the agent sees the error.Test plan
src/daemon/handlers/__tests__/session-device-claims.test.ts(#1391: a close-time script save failure still clears the advisory claim and deletes the session): a plaincloseagainst a session armed with a save-script path that already exists on disk throws the correctedAppError, but still clears the advisory device claim, deletes the session, and leaves the pre-existing target file untouched.main(session/claim lingers after a failedclose --save-script; a retried close duplicates the trailingcloseline), then confirmed both are fixed.npx vitest run --project unit-core).tsc --noEmit,oxlint, andoxfmt --checkall clean.Fixes #1391