From 60a0b9720afa14f7885314626c3e46e57fc344df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 15:12:19 +0200 Subject: [PATCH 1/9] fix(daemon): keep close-time script-save failures from leaking the session/device claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/session-device-claims.test.ts | 58 ++++++++++++++++ src/daemon/handlers/session-close.ts | 66 +++++++++++++++++-- 2 files changed, 120 insertions(+), 4 deletions(-) diff --git a/src/daemon/handlers/__tests__/session-device-claims.test.ts b/src/daemon/handlers/__tests__/session-device-claims.test.ts index 2481c6e0d..5e8f7176c 100644 --- a/src/daemon/handlers/__tests__/session-device-claims.test.ts +++ b/src/daemon/handlers/__tests__/session-device-claims.test.ts @@ -35,6 +35,7 @@ import { SessionStore } from '../../session-store.ts'; import { handleCloseCommand } from '../session-close.ts'; import { handleOpenCommand } from '../session-open.ts'; import type { DeviceInfo } from '../../../kernel/device.ts'; +import { AppError } from '../../../kernel/errors.ts'; const mockDispatch = vi.mocked(dispatchCommand); const mockResolveTargetDevice = vi.mocked(resolveTargetDevice); @@ -245,3 +246,60 @@ test('local close clears its matching advisory claim after teardown', async () = assert.equal(response.ok, true); assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []); }); + +test('#1391: a close-time script save failure still clears the advisory claim and deletes the session', async () => { + const { store, stateDir } = setup(); + const acquired = await acquireAdvisoryDeviceClaim({ + device: android, + session: 'close-save-script-failure', + workspace: process.cwd(), + stateDir, + }); + assert.ok(acquired.ownership); + const targetPath = path.join(stateDir, 'already-published.ad'); + fs.writeFileSync(targetPath, 'pre-existing\n'); + store.set('close-save-script-failure', { + name: 'close-save-script-failure', + device: android, + deviceClaim: acquired.ownership, + createdAt: Date.now(), + actions: [], + recordSession: true, + saveScriptPath: targetPath, + }); + mockDispatch.mockResolvedValue({}); + + // Like the platform-close-error tests above, a failed close-time save is + // thrown (not returned as `{ok:false}`) — `handleCloseCommand` never + // converts its own errors; the request-router does that for real requests. + let thrown: unknown; + try { + await handleCloseCommand({ + req: { + command: 'close', + token: 'test', + session: 'close-save-script-failure', + positionals: [], + flags: {}, + }, + sessionName: 'close-save-script-failure', + logPath: path.join(stateDir, 'daemon.log'), + sessionStore: store, + leaseRegistry: new LeaseRegistry(), + }); + } catch (error) { + thrown = error; + } + + // The failed save is surfaced, but never at the cost of leaking the session + // or the device claim (#1391 item 1) — and the target is left untouched + // rather than rewritten (#1391 item 2). + assert.ok(thrown instanceof AppError, 'expected close to throw an AppError'); + const error = thrown as AppError; + assert.equal(error.code, 'COMMAND_FAILED'); + assert.match(error.message, /session was closed, but its script was not saved/); + assert.equal(error.details?.retriable, false); + assert.equal(store.get('close-save-script-failure'), undefined); + assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []); + assert.equal(fs.readFileSync(targetPath, 'utf8'), 'pre-existing\n'); +}); diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index 7e17dc8a9..36e015e87 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -175,6 +175,20 @@ function toRepairPlatformCloseFailure(error: unknown): AppError { }); } +type SessionCloseTeardownResult = { + platformCloseError: unknown; + // #1391: an ordinary (non-repair) session's close-time script write (implicit + // from `open --save-script`, or this close's own `--save-script`) can refuse + // to publish (no-clobber target-exists, or any other fs AppError). Unlike the + // repair-armed commit (which keeps its session alive for a `--force` retry — + // ADR 0012 BLOCKER 2b), an ordinary session has no transaction to retry: its + // `close` already ran (or was skipped) and its teardown below must still + // release the lease/device claim and delete the session, exactly as a + // `platformCloseError` does not block them either. Surfaced separately so + // `handleCloseCommand` can report it AFTER teardown completes, never before. + saveScriptError?: AppError; +}; + // Runs the failure-isolated resource teardown and the targeted platform close // (#1225). Returns the preserved platform-close error (if any); best-effort // cleanup failures are pushed into `cleanupFailures`. Never throws for a cleanup @@ -192,7 +206,7 @@ async function runSessionCloseTeardown(params: { // see `handleCloseCommand`. Dispatching it again here would be redundant at // best and a double-close at worst, so it is skipped for that case only. skipPlatformClose: boolean; -}): Promise { +}): Promise { const { req, session, sessionName, logPath, sessionStore, cleanupFailures, repairArmed } = params; const attemptCleanup = async (step: string, run: () => Promise): Promise => { try { @@ -222,7 +236,9 @@ async function runSessionCloseTeardown(params: { // succeed. Only an ordinary (non-repair) session records `close` + writes its // session log here, and — per #1225 — a failed platform close is not recorded // as `Closed`. + let saveScriptError: AppError | undefined; if (!repairArmed) { + const actionsBeforeClose = session.actions.length; if (!platformCloseError) { recordSessionAction(sessionStore, session, req, 'close', { session: session.name, @@ -232,12 +248,47 @@ async function runSessionCloseTeardown(params: { if (req.flags?.saveScript) { session.recordSession = true; } - sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) }); + try { + sessionStore.writeSessionLog(session, { + force: resolveEffectiveSaveScriptForce(req, session), + }); + } catch (error) { + // #1391: never let a failed script publish leak the session/device claim + // (item 1) or leave an unrecorded `close` action for a later successful + // write to duplicate (item 2) — roll back exactly like the repair path's + // `commitRepairBeforeClose` does, then let teardown continue below. + session.actions.length = actionsBeforeClose; + saveScriptError = toOrdinaryCloseSaveScriptFailure(error); + } } await attemptCleanup('materialized_paths', () => cleanupRetainedMaterializedPathsForSession(sessionName), ); - return platformCloseError; + return { platformCloseError, saveScriptError }; +} + +/** + * #1391: normalizes an ordinary (non-repair) session's close-time script-write + * failure. `SessionScriptWriter.write()` only ever throws a genuine `AppError` + * here (a non-clobber refusal or another fs AppError — see + * `handleSessionScriptWriteFailure`'s non-repair, non-active-publication + * branch); anything else is already swallowed into a silent `{written:false}` + * there. The message is corrected for THIS call site: the shared + * `publishHealedScriptAtomically` wording ("retry close --save-script") + * describes the repair-commit retry contract, which does not apply here — by + * the time the agent sees this, `close` has already released the device and + * deleted the session, so there is nothing left to retry in place. + */ +function toOrdinaryCloseSaveScriptFailure(error: unknown): AppError { + const detail = error instanceof AppError ? error.message : normalizeError(error).message; + return new AppError( + 'COMMAND_FAILED', + `The session was closed, but its script was not saved: ${detail}`, + { + hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.', + retriable: false, + }, + ); } type CleanupRunner = (step: string, run: () => Promise) => Promise; @@ -475,7 +526,7 @@ export async function handleCloseCommand(params: { // is still attempted. The provider lease is committed only after that teardown, // and a failed provider release keeps the session retryable. const cleanupFailures: SessionCleanupFailure[] = []; - const platformCloseError = await runSessionCloseTeardown({ + const { platformCloseError, saveScriptError } = await runSessionCloseTeardown({ req, session, sessionName, @@ -498,6 +549,9 @@ export async function handleCloseCommand(params: { phase: 'session_close_cleanup_failed', failures: cleanupFailures, }); + // #1391: a failed script save is never a reason to keep the device claimed — + // only a genuine platform-close failure (the device may still be busy) or a + // resource-cleanup failure withholds it, exactly as before. if (!platformCloseError && !cleanupAggregate) { await clearAdvisoryDeviceClaim(session.deviceClaim); } @@ -507,6 +561,10 @@ export async function handleCloseCommand(params: { // diagnostic above so per-resource failures stay visible. if (platformCloseError) throw platformCloseError; if (cleanupAggregate) throw cleanupAggregate; + // #1391: surfaced last — the session already ended and the device is already + // released above, unlike the two failures above it. Nothing left to retry in + // place; `toOrdinaryCloseSaveScriptFailure`'s hint reflects that. + if (saveScriptError) throw saveScriptError; const shutdownResult = await maybeShutdownSessionTarget({ device: session.device, shutdownRequested: req.flags?.shutdown, From bc845452331c278e90c2621ef563867c08c9e971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 15:19:49 +0200 Subject: [PATCH 2/9] refactor(daemon): shrink handleCloseCommand/runSessionCloseTeardown under fallow's complexity gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/daemon/handlers/session-close.ts | 187 ++++++++++++++++++--------- 1 file changed, 126 insertions(+), 61 deletions(-) diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index 36e015e87..852725c39 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -236,37 +236,53 @@ async function runSessionCloseTeardown(params: { // succeed. Only an ordinary (non-repair) session records `close` + writes its // session log here, and — per #1225 — a failed platform close is not recorded // as `Closed`. - let saveScriptError: AppError | undefined; - if (!repairArmed) { - const actionsBeforeClose = session.actions.length; - if (!platformCloseError) { - recordSessionAction(sessionStore, session, req, 'close', { - session: session.name, - ...successText(`Closed: ${session.name}`), - }); - } - if (req.flags?.saveScript) { - session.recordSession = true; - } - try { - sessionStore.writeSessionLog(session, { - force: resolveEffectiveSaveScriptForce(req, session), - }); - } catch (error) { - // #1391: never let a failed script publish leak the session/device claim - // (item 1) or leave an unrecorded `close` action for a later successful - // write to duplicate (item 2) — roll back exactly like the repair path's - // `commitRepairBeforeClose` does, then let teardown continue below. - session.actions.length = actionsBeforeClose; - saveScriptError = toOrdinaryCloseSaveScriptFailure(error); - } - } + const saveScriptError = repairArmed + ? undefined + : finalizeOrdinaryCloseScript({ req, session, sessionStore, platformCloseError }); await attemptCleanup('materialized_paths', () => cleanupRetainedMaterializedPathsForSession(sessionName), ); return { platformCloseError, saveScriptError }; } +/** + * ADR 0012 decision 6 (BLOCKER 2): only an ordinary (non-repair) session + * records its finalize `close` and writes its session log here — see the + * call site's own comment for why a repair-armed session skips this entirely. + * + * #1391: the write can refuse to publish (no-clobber target-exists, or any + * other fs `AppError`). Roll back the just-recorded `close` action on failure + * — mirroring `commitRepairBeforeClose`'s own rollback — so neither the + * session/device claim leaks (item 1) nor an unrecorded `close` survives for a + * later successful write to duplicate (item 2); the caller still completes + * teardown regardless of the outcome returned here. + */ +function finalizeOrdinaryCloseScript(params: { + req: DaemonRequest; + session: SessionState; + sessionStore: SessionStore; + platformCloseError: unknown; +}): AppError | undefined { + const { req, session, sessionStore, platformCloseError } = params; + const actionsBeforeClose = session.actions.length; + if (!platformCloseError) { + recordSessionAction(sessionStore, session, req, 'close', { + session: session.name, + ...successText(`Closed: ${session.name}`), + }); + } + if (req.flags?.saveScript) { + session.recordSession = true; + } + try { + sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) }); + return undefined; + } catch (error) { + session.actions.length = actionsBeforeClose; + return toOrdinaryCloseSaveScriptFailure(error); + } +} + /** * #1391: normalizes an ordinary (non-repair) session's close-time script-write * failure. `SessionScriptWriter.write()` only ever throws a genuine `AppError` @@ -521,10 +537,68 @@ export async function handleCloseCommand(params: { // identity it was recorded under, so the platform close runs (again). const repair = await prepareRepairClose({ req, session, logPath, sessionStore }); if ('response' in repair) return repair.response; - // Resource teardown is failure-isolated: a rejected step is collected instead of - // short-circuiting the rest, so every subsequent resource (and the runner stop) - // is still attempted. The provider lease is committed only after that teardown, - // and a failed provider release keeps the session retryable. + const closed = await runCloseTeardownAndRelease({ + req, + session, + sessionName, + logPath, + sessionStore, + leaseRegistry, + leaseLifecycleProvider, + repairArmed: repair.repairArmed, + }); + if (closed.kind === 'response') return closed.response; + const shutdownResult = await maybeShutdownSessionTarget({ + device: session.device, + shutdownRequested: req.flags?.shutdown, + }); + return buildCloseSuccessResponse({ + session, + repair, + requestedSaveScript: Boolean(req.flags?.saveScript), + shutdownResult, + providerData: closed.providerData, + }); +} + +type SessionCloseFinalization = + | { kind: 'response'; response: DaemonResponse } + | { kind: 'closed'; providerData?: Record }; + +/** + * Everything between a settled repair decision and a success response: the + * failure-isolated resource teardown, the provider lease release, and — only + * once both have run — the device-claim clear and session delete. A rejected + * cleanup step is collected instead of short-circuiting the rest, so every + * subsequent resource (and the runner stop) is still attempted; the provider + * lease is released only after that teardown, and a failed release keeps the + * session retryable (returned as `{kind:'response'}`, mirroring the + * repair-commit-failure path above it). The platform-close failure is thrown + * as the primary error with its original code/details/hint intact; the + * cleanup aggregate has already been emitted as a diagnostic by this point so + * per-resource failures stay visible; a failed script save (#1391) is thrown + * last since — unlike the two above it — the session has already ended and + * the device is already released by the time it surfaces. + */ +async function runCloseTeardownAndRelease(params: { + req: DaemonRequest; + session: SessionState; + sessionName: string; + logPath: string; + sessionStore: SessionStore; + leaseRegistry: LeaseRegistry; + leaseLifecycleProvider: LeaseLifecycleProvider | undefined; + repairArmed: boolean; +}): Promise { + const { + req, + session, + sessionName, + logPath, + sessionStore, + leaseRegistry, + leaseLifecycleProvider, + } = params; const cleanupFailures: SessionCleanupFailure[] = []; const { platformCloseError, saveScriptError } = await runSessionCloseTeardown({ req, @@ -533,17 +607,17 @@ export async function handleCloseCommand(params: { logPath, sessionStore, cleanupFailures, - repairArmed: repair.repairArmed, + repairArmed: params.repairArmed, // The platform close for a repair-armed session already ran (and was // confirmed to succeed) above, before the commit — never dispatch it twice. - skipPlatformClose: repair.repairArmed, + skipPlatformClose: params.repairArmed, }); const leaseRelease = await releaseProviderLeaseForClose({ session, leaseRegistry, leaseLifecycleProvider, }); - if (leaseRelease.response) return leaseRelease.response; + if (leaseRelease.response) return { kind: 'response', response: leaseRelease.response }; const cleanupAggregate = reportSessionCleanupFailures({ sessionName, phase: 'session_close_cleanup_failed', @@ -556,25 +630,21 @@ export async function handleCloseCommand(params: { await clearAdvisoryDeviceClaim(session.deviceClaim); } sessionStore.delete(sessionName); - // The platform-close failure is the primary error: rethrow it with its original - // code/details/hint intact. The cleanup aggregate has already been emitted as a - // diagnostic above so per-resource failures stay visible. if (platformCloseError) throw platformCloseError; if (cleanupAggregate) throw cleanupAggregate; - // #1391: surfaced last — the session already ended and the device is already - // released above, unlike the two failures above it. Nothing left to retry in - // place; `toOrdinaryCloseSaveScriptFailure`'s hint reflects that. if (saveScriptError) throw saveScriptError; - const shutdownResult = await maybeShutdownSessionTarget({ - device: session.device, - shutdownRequested: req.flags?.shutdown, - }); - // ADR 0012 decision 6 (BLOCKER 2a): positively report the committed healed - // artifact path so the agent learns the repair published (and where) without - // an extra round-trip. - const savedScript = repair.healedScriptPath ? { savedScript: repair.healedScriptPath } : {}; - const text = `Closed: ${session.name}`; - if (repair.aborted && req.flags?.saveScript) { + return { kind: 'closed', providerData: leaseRelease.providerData }; +} + +function buildCloseSuccessResponse(params: { + session: SessionState; + repair: Extract; + requestedSaveScript: boolean; + shutdownResult: DeviceTargetShutdownResult | undefined; + providerData: Record | undefined; +}): DaemonResponse { + const { session, repair, requestedSaveScript, shutdownResult, providerData } = params; + if (repair.aborted && requestedSaveScript) { return { ok: false, error: { @@ -585,29 +655,24 @@ export async function handleCloseCommand(params: { }, }; } - + // ADR 0012 decision 6 (BLOCKER 2a): positively report the committed healed + // artifact path so the agent learns the repair published (and where) without + // an extra round-trip. + const savedScript = repair.healedScriptPath ? { savedScript: repair.healedScriptPath } : {}; + const provider = providerData ? { provider: providerData } : {}; + const text = `Closed: ${session.name}`; if (shutdownResult) { return { ok: true, data: withSuccessText( - { - session: session.name, - shutdown: shutdownResult, - ...savedScript, - ...(leaseRelease.providerData ? { provider: leaseRelease.providerData } : {}), - }, + { session: session.name, shutdown: shutdownResult, ...savedScript, ...provider }, text, ), }; } return { ok: true, - data: { - session: session.name, - ...successText(text), - ...savedScript, - ...(leaseRelease.providerData ? { provider: leaseRelease.providerData } : {}), - }, + data: { session: session.name, ...successText(text), ...savedScript, ...provider }, }; } From 6864e4d80edc7707d8f9aed6db3911a4a3d2c4be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 25 Jul 2026 12:33:01 +0200 Subject: [PATCH 3/9] fix(daemon): preserve the write error's structured details in the close-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. --- .../__tests__/session-device-claims.test.ts | 5 ++++ src/daemon/handlers/session-close.ts | 28 +++++++++++++------ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/daemon/handlers/__tests__/session-device-claims.test.ts b/src/daemon/handlers/__tests__/session-device-claims.test.ts index 5e8f7176c..e7252e874 100644 --- a/src/daemon/handlers/__tests__/session-device-claims.test.ts +++ b/src/daemon/handlers/__tests__/session-device-claims.test.ts @@ -299,6 +299,11 @@ test('#1391: a close-time script save failure still clears the advisory claim an assert.equal(error.code, 'COMMAND_FAILED'); assert.match(error.message, /session was closed, but its script was not saved/); assert.equal(error.details?.retriable, false); + // The original write failure's machine-readable details survive the + // close-specific hint/retriable override, so a caller dispatching on them + // (e.g. `reason === 'script_target_exists'`) still can. + assert.equal(error.details?.reason, 'script_target_exists'); + assert.equal(error.details?.path, targetPath); assert.equal(store.get('close-save-script-failure'), undefined); assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []); assert.equal(fs.readFileSync(targetPath, 'utf8'), 'pre-existing\n'); diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index 852725c39..40f7aa8a8 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -289,21 +289,33 @@ function finalizeOrdinaryCloseScript(params: { * here (a non-clobber refusal or another fs AppError — see * `handleSessionScriptWriteFailure`'s non-repair, non-active-publication * branch); anything else is already swallowed into a silent `{written:false}` - * there. The message is corrected for THIS call site: the shared - * `publishHealedScriptAtomically` wording ("retry close --save-script") + * there. Only the message/hint/retriable are corrected for THIS call site (the + * shared `publishHealedScriptAtomically` wording, "retry close --save-script", * describes the repair-commit retry contract, which does not apply here — by * the time the agent sees this, `close` has already released the device and - * deleted the session, so there is nothing left to retry in place. + * deleted the session, so there is nothing left to retry in place); the + * original error's machine-readable `details` (e.g. `reason: + * "script_target_exists"`, `path`) are preserved so a caller dispatching on + * them still can. */ function toOrdinaryCloseSaveScriptFailure(error: unknown): AppError { - const detail = error instanceof AppError ? error.message : normalizeError(error).message; + const overrides = { + hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.', + retriable: false, + }; + if (error instanceof AppError) { + return new AppError( + 'COMMAND_FAILED', + `The session was closed, but its script was not saved: ${error.message}`, + { ...error.details, ...overrides }, + error.cause, + ); + } + const detail = normalizeError(error).message; return new AppError( 'COMMAND_FAILED', `The session was closed, but its script was not saved: ${detail}`, - { - hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.', - retriable: false, - }, + overrides, ); } From 2a4610ed644aa2b355a04d1f0d1b19f638bff153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 12:42:10 +0200 Subject: [PATCH 4/9] refactor(daemon): drop the vestigial close-time rollback, add router-level #1391 coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../request-router-typed-error.test.ts | 42 +++++++++++++++++++ src/daemon/handlers/session-close.ts | 16 +++---- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/daemon/__tests__/request-router-typed-error.test.ts b/src/daemon/__tests__/request-router-typed-error.test.ts index 9bbd2f09f..3fc44e346 100644 --- a/src/daemon/__tests__/request-router-typed-error.test.ts +++ b/src/daemon/__tests__/request-router-typed-error.test.ts @@ -1,4 +1,5 @@ import { test, expect, vi, beforeEach } from 'vitest'; +import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -167,3 +168,44 @@ test('BLOCKER 2 (second follow-up): a repair-close platform-close failure surfac // The session is retained (not torn down), addressable for the retry. expect(sessionStore.get('typed-error')).toBeDefined(); }); + +// #1391 P2: the handler-level test (`session-device-claims.test.ts`) proves +// `toOrdinaryCloseSaveScriptFailure` builds the right `AppError`, but calls +// `handleCloseCommand` directly — it never exercises `normalizeError`/ +// `enrichDaemonError`'s own hoisting of `details.retriable` to the TOP-level +// wire field, the same layer BLOCKER 2's test above exists to catch a +// regression in. Exercised through the REAL router boundary so a regression +// in either layer is caught, unlike the direct-call test. +test('#1391: an ordinary close-time script-save failure surfaces details.reason/path and retriable:false through the router, and the session is torn down', async () => { + const { sessionStore, handler } = makeHandler(); + const session = makeIosSession('typed-error'); + session.recordSession = true; + const targetPath = path.join( + os.tmpdir(), + `agent-device-router-typed-error-${Date.now()}-${Math.random().toString(36).slice(2)}.ad`, + ); + fs.writeFileSync(targetPath, 'pre-existing\n'); + session.saveScriptPath = targetPath; + sessionStore.set('typed-error', session); + + try { + // Untargeted close: no positionals, so no platform close is dispatched + // (`shouldDispatchPlatformClose`) — isolates the script-save failure from + // any platform-close error, matching BLOCKER 2's own targeted-vs-untargeted + // distinction above. + const response = await handler(request('close')); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('COMMAND_FAILED'); + expect(response.error.retriable).toBe(false); + expect(response.error.details?.reason).toBe('script_target_exists'); + expect(response.error.details?.path).toBe(targetPath); + // Unlike the repair-armed case above, an ordinary session's teardown + // never withholds on a failed script save — it is always torn down. + expect(sessionStore.get('typed-error')).toBeUndefined(); + expect(mockDispatch).not.toHaveBeenCalled(); + } finally { + fs.rmSync(targetPath, { force: true }); + } +}); diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index 40f7aa8a8..dd9dd90b8 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -251,11 +251,15 @@ async function runSessionCloseTeardown(params: { * call site's own comment for why a repair-armed session skips this entirely. * * #1391: the write can refuse to publish (no-clobber target-exists, or any - * other fs `AppError`). Roll back the just-recorded `close` action on failure - * — mirroring `commitRepairBeforeClose`'s own rollback — so neither the - * session/device claim leaks (item 1) nor an unrecorded `close` survives for a - * later successful write to duplicate (item 2); the caller still completes - * teardown regardless of the outcome returned here. + * other fs `AppError`). Unlike a repair-armed commit failure — which keeps + * its session alive so `commitRepairBeforeClose` must roll back its + * just-recorded `close` action to keep a later retry from duplicating it — + * this caller (`runSessionCloseTeardown`) always completes teardown and + * deletes the session regardless of the outcome returned here, so there is + * no surviving session for a retry to duplicate anything on. No rollback: + * the recorded `close` action (and its durable `events.ndjson` entry) stay + * exactly as they are — an accurate record that the close itself happened, + * independent of whether the script also got saved. */ function finalizeOrdinaryCloseScript(params: { req: DaemonRequest; @@ -264,7 +268,6 @@ function finalizeOrdinaryCloseScript(params: { platformCloseError: unknown; }): AppError | undefined { const { req, session, sessionStore, platformCloseError } = params; - const actionsBeforeClose = session.actions.length; if (!platformCloseError) { recordSessionAction(sessionStore, session, req, 'close', { session: session.name, @@ -278,7 +281,6 @@ function finalizeOrdinaryCloseScript(params: { sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) }); return undefined; } catch (error) { - session.actions.length = actionsBeforeClose; return toOrdinaryCloseSaveScriptFailure(error); } } From 9386c0cdfe952420382de2f7ec10e5687e4c76ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 13:38:20 +0200 Subject: [PATCH 5/9] test(daemon): assert the durable close event survives a failed close-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. --- .../__tests__/session-device-claims.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/daemon/handlers/__tests__/session-device-claims.test.ts b/src/daemon/handlers/__tests__/session-device-claims.test.ts index e7252e874..248155d9d 100644 --- a/src/daemon/handlers/__tests__/session-device-claims.test.ts +++ b/src/daemon/handlers/__tests__/session-device-claims.test.ts @@ -307,4 +307,20 @@ test('#1391: a close-time script save failure still clears the advisory claim an assert.equal(store.get('close-save-script-failure'), undefined); assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []); assert.equal(fs.readFileSync(targetPath, 'utf8'), 'pre-existing\n'); + + // No rollback: the durable `action.recorded: close` event this close wrote + // to events.ndjson (before the script write failed) survives — teardown + // never withholds it, since there is no surviving session for a later + // write to duplicate it against. Guards against a future rollback or + // event-order change silently recreating the in-memory/durable mismatch + // this test's own name warns about. + await store.flushEvents('close-save-script-failure'); + const events = store.readEvents('close-save-script-failure').events; + const closeEvent = events.find( + (event) => event.kind === 'action.recorded' && event.command === 'close', + ); + assert.ok( + closeEvent, + 'expected a durable action.recorded:close event to survive the failed save', + ); }); From 1f0df4c33812783d762ffda02c41f52bc332a0ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 14:49:30 +0200 Subject: [PATCH 6/9] test(daemon): assert the retained session's in-memory close action, not just the durable event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../__tests__/session-device-claims.test.ts | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/daemon/handlers/__tests__/session-device-claims.test.ts b/src/daemon/handlers/__tests__/session-device-claims.test.ts index 248155d9d..a63a61347 100644 --- a/src/daemon/handlers/__tests__/session-device-claims.test.ts +++ b/src/daemon/handlers/__tests__/session-device-claims.test.ts @@ -35,6 +35,7 @@ import { SessionStore } from '../../session-store.ts'; import { handleCloseCommand } from '../session-close.ts'; import { handleOpenCommand } from '../session-open.ts'; import type { DeviceInfo } from '../../../kernel/device.ts'; +import type { SessionState } from '../../types.ts'; import { AppError } from '../../../kernel/errors.ts'; const mockDispatch = vi.mocked(dispatchCommand); @@ -258,7 +259,11 @@ test('#1391: a close-time script save failure still clears the advisory claim an assert.ok(acquired.ownership); const targetPath = path.join(stateDir, 'already-published.ad'); fs.writeFileSync(targetPath, 'pre-existing\n'); - store.set('close-save-script-failure', { + // Retained directly (not just looked up via `store`) so it stays inspectable + // after `handleCloseCommand` deletes it from the store — `store.delete` only + // drops the map entry, it doesn't touch the object this variable still + // points at. + const session: SessionState = { name: 'close-save-script-failure', device: android, deviceClaim: acquired.ownership, @@ -266,7 +271,8 @@ test('#1391: a close-time script save failure still clears the advisory claim an actions: [], recordSession: true, saveScriptPath: targetPath, - }); + }; + store.set('close-save-script-failure', session); mockDispatch.mockResolvedValue({}); // Like the platform-close-error tests above, a failed close-time save is @@ -323,4 +329,15 @@ test('#1391: a close-time script save failure still clears the advisory claim an closeEvent, 'expected a durable action.recorded:close event to survive the failed save', ); + // Same assertion, in-memory: reinstating `session.actions.length = + // actionsBeforeClose` would still pass the durable-event check above (that + // event was already queued before the write failed) while silently + // dropping this to zero — assert directly on the retained session object, + // not just the store, so that specific regression is caught. + const inMemoryCloseActions = session.actions.filter((action) => action.command === 'close'); + assert.equal(inMemoryCloseActions.length, 1); + assert.equal( + inMemoryCloseActions.length, + events.filter((event) => event.command === 'close').length, + ); }); From bdc21e8926c3e6f63bc496bc389796f739d3cd4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 14:55:04 +0200 Subject: [PATCH 7/9] refactor(daemon): model repair close retry as receipt --- scripts/layering/session-state.ts | 3 +- .../__tests__/session-device-claims.test.ts | 33 ++----- .../session-replay-repair-transaction.test.ts | 15 +--- src/daemon/handlers/session-close.ts | 90 ++----------------- src/daemon/types.ts | 32 +------ 5 files changed, 20 insertions(+), 153 deletions(-) diff --git a/scripts/layering/session-state.ts b/scripts/layering/session-state.ts index fcd38c1e4..271b8f8b1 100644 --- a/scripts/layering/session-state.ts +++ b/scripts/layering/session-state.ts @@ -81,8 +81,7 @@ export const SESSION_STATE_FIELD_OWNERS: Readonly event.kind === 'action.recorded' && event.command === 'close', - ); - assert.ok( - closeEvent, - 'expected a durable action.recorded:close event to survive the failed save', - ); - // Same assertion, in-memory: reinstating `session.actions.length = - // actionsBeforeClose` would still pass the durable-event check above (that - // event was already queued before the write failed) while silently - // dropping this to zero — assert directly on the retained session object, - // not just the store, so that specific regression is caught. - const inMemoryCloseActions = session.actions.filter((action) => action.command === 'close'); - assert.equal(inMemoryCloseActions.length, 1); - assert.equal( - inMemoryCloseActions.length, - events.filter((event) => event.command === 'close').length, + assert.deepEqual( + session.actions.map((action) => action.command), + ['close'], ); + await store.flushEvents('close-save-script-failure'); + const durableCloseEvents = store + .readEvents('close-save-script-failure') + .events.filter((event) => event.kind === 'action.recorded' && event.command === 'close'); + assert.equal(durableCloseEvents.length, session.actions.length); }); diff --git a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts index 7b93b3801..25fa18f85 100644 --- a/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts @@ -984,7 +984,7 @@ test('BLOCKER 3 (second follow-up): a retry after a SUCCESSFUL platform close bu if (!closeResponse.ok) expect(closeResponse.error.message).toMatch(/already exists/); expect(sessionStore.get(sessionName)).toBeDefined(); expect(fs.readFileSync(healedPath, 'utf8')).toBe(before); - expect(sessionStore.get(sessionName)!.repairPlatformCloseSucceeded).toBe(true); + expect(sessionStore.get(sessionName)!.repairPlatformCloseReceipt).toBeDefined(); // Retry with an explicit path: the ALREADY-SUCCEEDED platform close must // NEVER be dispatched again — a non-idempotent backend could fail (or @@ -1043,14 +1043,6 @@ test('BLOCKER 3: a competing second writer never overwrites a COMPLETE artifact expect(fs.readFileSync(healedPath, 'utf8')).toBe(committed); }); -// --- ADR 0012 decision 6 (BLOCKER 3, third follow-up): `repairPlatformCloseSucceeded` -// was session-wide, not bound to WHICH close request actually succeeded. An -// untargeted close performs NO platform operation (`shouldDispatchPlatformClose` -// is false with no positional target), yet the prior implementation still set -// the marker as though a real close succeeded; a retry with a DIFFERENT -// identity (a target added, or a different target) then wrongly skipped the -// platform close entirely, committing as though it had run. --- - test('BLOCKER 3 (third follow-up): an untargeted close that performed NO platform operation never lets a targeted retry skip the platform close', async () => { const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup( 'agent-device-repair-transaction-close-identity-untargeted-', @@ -1078,9 +1070,6 @@ test('BLOCKER 3 (third follow-up): an untargeted close that performed NO platfor expect(mockDispatchCommand).not.toHaveBeenCalled(); expect(sessionStore.get(sessionName)).toBeDefined(); - // Retry WITH a target: the prior session-wide `repairPlatformCloseSucceeded` - // flag (set true even though nothing dispatched for the untargeted attempt) - // would wrongly skip the platform close here. It must actually run. const retry = await handleCloseCommand({ req: { token: 't', @@ -1126,7 +1115,7 @@ test('BLOCKER 3 (third follow-up): a retry targeting a DIFFERENT app than the su }); expect(first.ok).toBe(false); expect(mockDispatchCommand).toHaveBeenCalledTimes(1); - expect(sessionStore.get(sessionName)!.repairPlatformCloseSucceeded).toBe(true); + expect(sessionStore.get(sessionName)!.repairPlatformCloseReceipt).toBeDefined(); // Retry targets a DIFFERENT app (app-b) — a genuinely different platform // operation. The prior session-wide marker would wrongly treat app-b as diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index dd9dd90b8..817fe2dea 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -159,14 +159,6 @@ function buildRepairCloseFailureResponse(session: SessionState, error: AppError) }; } -/** - * ADR 0012 decision 6 (BLOCKER 2, new): normalizes a repair-armed session's - * FAILED platform close into a distinct, surfaceable AppError, mirroring - * `toRepairCommitFailure` in `session-script-writer.ts`. An AppError from the - * platform close (e.g. a device-unavailable failure) already carries its own - * code/details/hint and passes through unchanged; anything else is wrapped - * with a clear message so the agent can tell this apart from a write failure. - */ function toRepairPlatformCloseFailure(error: unknown): AppError { if (error instanceof AppError) return error; const detail = error instanceof Error ? error.message : String(error); @@ -189,10 +181,6 @@ type SessionCloseTeardownResult = { saveScriptError?: AppError; }; -// Runs the failure-isolated resource teardown and the targeted platform close -// (#1225). Returns the preserved platform-close error (if any); best-effort -// cleanup failures are pushed into `cleanupFailures`. Never throws for a cleanup -// step so the caller can make an explicit decision about lease/session commit. async function runSessionCloseTeardown(params: { req: DaemonRequest; session: SessionState; @@ -201,11 +189,6 @@ async function runSessionCloseTeardown(params: { sessionStore: SessionStore; cleanupFailures: SessionCleanupFailure[]; repairArmed: boolean; - // ADR 0012 decision 6 (BLOCKER 2): a repair-armed session already dispatched - // (and confirmed the success of) its platform close BEFORE this teardown — - // see `handleCloseCommand`. Dispatching it again here would be redundant at - // best and a double-close at worst, so it is skipped for that case only. - skipPlatformClose: boolean; }): Promise { const { req, session, sessionName, logPath, sessionStore, cleanupFailures, repairArmed } = params; const attemptCleanup = async (step: string, run: () => Promise): Promise => { @@ -215,27 +198,13 @@ async function runSessionCloseTeardown(params: { cleanupFailures.push({ step, error }); } }; - // Decide runner retention from the PRE-teardown state: an active recording - // defeats retention, and `stopBestEffortSessionResources` below finalizes (and - // clears) that recording, so the decision must be captured before it runs. const retainAppleRunner = shouldRetainAppleRunnerAfterClose(req, session); await stopBestEffortSessionResources(session, sessionStore, attemptCleanup); - // The targeted platform close is the primary operation, not best-effort cleanup: - // its AppError (code/details/hint) is preserved and returned for the caller to - // rethrow, and a failed close must not be recorded as `Closed`. Subsequent - // resource cleanup still runs regardless. - const platformCloseError = params.skipPlatformClose + const platformCloseError = repairArmed ? undefined : await dispatchTargetedPlatformClose({ req, session, logPath }); await stopOrRetainAppleRunnerAfterClose(retainAppleRunner, session, attemptCleanup); await clearSessionRuntimeHints(session, sessionStore, sessionName); - // ADR 0012 decision 6 (BLOCKER 2): a repair-armed session already recorded its - // finalize `close` and committed (or aborted) its healed `.ad` BEFORE this - // teardown (commit-state machine — the single commit path), and only AFTER - // its platform close (dispatched above `handleCloseCommand`) was confirmed to - // succeed. Only an ordinary (non-repair) session records `close` + writes its - // session log here, and — per #1225 — a failed platform close is not recorded - // as `Closed`. const saveScriptError = repairArmed ? undefined : finalizeOrdinaryCloseScript({ req, session, sessionStore, platformCloseError }); @@ -328,11 +297,7 @@ async function stopBestEffortSessionResources( sessionStore: SessionStore, attemptCleanup: CleanupRunner, ): Promise { - // Finalize any still-active recording first so closing a session mid-recording - // (without an explicit `record stop`) cannot leak the recorder process; must - // run before the Apple runner is stopped below since overlay finalization - // consults the runner. `shouldRetainAppleRunnerAfterClose` then observes the - // now-cleared `session.recording`. + // Recording overlay finalization needs the Apple runner. await attemptCleanup('recording', () => stopSessionRecordingForTeardown(session)); await attemptCleanup('app_log', () => stopSessionAppLog(session)); await attemptCleanup('audio_probe', async () => { @@ -346,22 +311,7 @@ async function stopBestEffortSessionResources( ); } -/** - * ADR 0012 decision 6 (BLOCKER 3, third follow-up): identifies WHICH close - * request's platform close succeeded — not merely THAT one did. Only the - * request's TARGET (`positionals`) can change what `dispatchTargetedPlatformClose` - * actually does: `shouldDispatchPlatformClose` decides purely from - * `hasCloseTarget(req)` (plus the `web` special case, constant for a given - * session), and the dispatch itself is `dispatchCommand(device, 'close', - * req.positionals, ...)`. `close`'s only other flags (`shutdown`, `saveScript` - * — see `closeCliSchema`) feed the post-teardown shutdown and the commit path - * respectively, never this call, so they carry no identity here. Binding the - * marker to this identity means an untargeted close's "succeeded" (a no-op, - * since `shouldDispatchPlatformClose` was false) can never be misread as "the - * platform close for THIS target already ran" by a later retry that adds or - * changes the target — that retry's identity differs, so it re-dispatches. - */ -function repairPlatformCloseIdentity(req: DaemonRequest): string { +function buildRepairPlatformCloseReceipt(req: DaemonRequest): string { return JSON.stringify(req.positionals ?? []); } @@ -377,11 +327,8 @@ async function prepareRepairClose(params: { }): Promise { const { req, session, logPath, sessionStore } = params; const repairArmed = session.saveScriptBoundary !== undefined; - const closeIdentity = repairPlatformCloseIdentity(req); - if ( - repairArmed && - !(session.repairPlatformCloseSucceeded && session.repairPlatformCloseIdentity === closeIdentity) - ) { + const closeReceipt = buildRepairPlatformCloseReceipt(req); + if (repairArmed && session.repairPlatformCloseReceipt !== closeReceipt) { const platformCloseError = await dispatchTargetedPlatformClose({ req, session, logPath }); if (platformCloseError) { return { @@ -391,15 +338,13 @@ async function prepareRepairClose(params: { ), }; } - session.repairPlatformCloseSucceeded = true; - session.repairPlatformCloseIdentity = closeIdentity; + session.repairPlatformCloseReceipt = closeReceipt; } const repairCommit = commitRepairBeforeClose(sessionStore, session, req); if (repairCommit.kind === 'failed') { return { response: buildRepairCloseFailureResponse(session, repairCommit.error) }; } - session.repairPlatformCloseSucceeded = false; - session.repairPlatformCloseIdentity = undefined; + session.repairPlatformCloseReceipt = undefined; return { repairArmed, ...(repairCommit.kind === 'committed' && repairCommit.path @@ -531,24 +476,6 @@ export async function handleCloseCommand(params: { if (req.internal?.closeAppOnly === true) { return await closeAppWithoutEndingSession({ req, session, logPath }); } - // ADR 0012 decision 6 (BLOCKER 2): for a repair-armed session, the platform - // close must run and SUCCEED before anything is committed or torn down — - // otherwise a committed healed `.ad` could claim a successful `close` that - // never actually happened on the device. On failure, return without - // touching the session at all (mirrors the commit-failure path below): it - // stays addressable so the agent can fix the cause and retry. - // - // BLOCKER 3: a PRIOR close attempt on this same session may already have - // dispatched the platform close and confirmed its success, then failed to - // commit (the session is retained for exactly that retry — see below). A - // retry must never re-dispatch a (possibly non-idempotent) platform close - // against an already-closed target; `repairPlatformCloseSucceeded` + - // `repairPlatformCloseIdentity` together record that the platform-level - // close already happened FOR THIS EXACT request identity, so a same-identity - // retry consumes it and goes straight to the commit instead. A retry whose - // identity DIFFERS (third follow-up: e.g. untargeted -> targeted, or a - // changed target) never matches — the marker only ever attests to the - // identity it was recorded under, so the platform close runs (again). const repair = await prepareRepairClose({ req, session, logPath, sessionStore }); if ('response' in repair) return repair.response; const closed = await runCloseTeardownAndRelease({ @@ -622,9 +549,6 @@ async function runCloseTeardownAndRelease(params: { sessionStore, cleanupFailures, repairArmed: params.repairArmed, - // The platform close for a repair-armed session already ran (and was - // confirmed to succeed) above, before the commit — never dispatch it twice. - skipPlatformClose: params.repairArmed, }); const leaseRelease = await releaseProviderLeaseForClose({ session, diff --git a/src/daemon/types.ts b/src/daemon/types.ts index 45d575261..df12b84cd 100644 --- a/src/daemon/types.ts +++ b/src/daemon/types.ts @@ -406,36 +406,8 @@ export type SessionState = { * (nothing to tombstone) from an aborted/reaped one. */ saveScriptCommitted?: boolean; - /** - * ADR 0012 decision 6 (BLOCKER 3, second follow-up): set the moment a - * repair-armed `close`'s targeted platform close returns SUCCESS, cleared - * once the transaction commits/tears down (never lingers past a single - * close attempt's outcome). If the subsequent commit then FAILS (no-clobber - * refusal, a bare-`@ref` failure, or an fs error) the session is retained - * for retry — a later `close`/`close --save-script=` on the SAME - * session must consume this flag and skip re-dispatching the platform - * close rather than invoking a (possibly non-idempotent) backend a second - * time against an already-closed target. - * - * BLOCKER 3 (third follow-up): this flag alone is session-wide, not bound - * to WHICH close request succeeded — see `repairPlatformCloseIdentity`, - * which must ALSO match the retry's own request identity before a retry is - * allowed to skip the platform close. - */ - repairPlatformCloseSucceeded?: boolean; - /** - * ADR 0012 decision 6 (BLOCKER 3, third follow-up): the identity - * (`repairPlatformCloseIdentity` in session-close.ts — currently just the - * request's target/positionals, the only thing that changes what the - * platform close actually dispatches) of the close request whose platform - * close last succeeded. A retry only gets to skip re-dispatching the - * platform close when BOTH `repairPlatformCloseSucceeded` is true AND this - * matches the retry's own identity — an untargeted close that performed NO - * platform operation (so `repairPlatformCloseSucceeded` is trivially true) - * must never let a later targeted (or differently-targeted) retry skip the - * platform close it never actually ran. - */ - repairPlatformCloseIdentity?: string; + /** Target identity of a successful repair close awaiting script commit. */ + repairPlatformCloseReceipt?: string; /** * ADR 0012 decision 6, R7 (C5a): the original replay input path of an armed * repair, stashed so an idle-reap tombstone can hand the agent an actionable From d5fb3f704c0a2ac0bfa7da82a202f6c1b1d64a2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 15:14:44 +0200 Subject: [PATCH 8/9] refactor(daemon): merge blockingError to state, not explain, the save-script exclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../request-router-typed-error.test.ts | 10 +-- src/daemon/handlers/session-close.ts | 79 +++++-------------- 2 files changed, 21 insertions(+), 68 deletions(-) diff --git a/src/daemon/__tests__/request-router-typed-error.test.ts b/src/daemon/__tests__/request-router-typed-error.test.ts index 3fc44e346..f475cf750 100644 --- a/src/daemon/__tests__/request-router-typed-error.test.ts +++ b/src/daemon/__tests__/request-router-typed-error.test.ts @@ -169,13 +169,9 @@ test('BLOCKER 2 (second follow-up): a repair-close platform-close failure surfac expect(sessionStore.get('typed-error')).toBeDefined(); }); -// #1391 P2: the handler-level test (`session-device-claims.test.ts`) proves -// `toOrdinaryCloseSaveScriptFailure` builds the right `AppError`, but calls -// `handleCloseCommand` directly — it never exercises `normalizeError`/ -// `enrichDaemonError`'s own hoisting of `details.retriable` to the TOP-level -// wire field, the same layer BLOCKER 2's test above exists to catch a -// regression in. Exercised through the REAL router boundary so a regression -// in either layer is caught, unlike the direct-call test. +// Unlike the handler-level test in session-device-claims.test.ts, this goes +// through the real router boundary, so it also covers normalizeError's +// details.retriable hoisting. test('#1391: an ordinary close-time script-save failure surfaces details.reason/path and retriable:false through the router, and the session is torn down', async () => { const { sessionStore, handler } = makeHandler(); const session = makeIosSession('typed-error'); diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index 817fe2dea..b1cecc3fb 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -169,15 +169,8 @@ function toRepairPlatformCloseFailure(error: unknown): AppError { type SessionCloseTeardownResult = { platformCloseError: unknown; - // #1391: an ordinary (non-repair) session's close-time script write (implicit - // from `open --save-script`, or this close's own `--save-script`) can refuse - // to publish (no-clobber target-exists, or any other fs AppError). Unlike the - // repair-armed commit (which keeps its session alive for a `--force` retry — - // ADR 0012 BLOCKER 2b), an ordinary session has no transaction to retry: its - // `close` already ran (or was skipped) and its teardown below must still - // release the lease/device claim and delete the session, exactly as a - // `platformCloseError` does not block them either. Surfaced separately so - // `handleCloseCommand` can report it AFTER teardown completes, never before. + // #1391: a failed close-time script write (see finalizeOrdinaryCloseScript). + // Never blocks teardown — see runCloseTeardownAndRelease's blockingError. saveScriptError?: AppError; }; @@ -214,22 +207,10 @@ async function runSessionCloseTeardown(params: { return { platformCloseError, saveScriptError }; } -/** - * ADR 0012 decision 6 (BLOCKER 2): only an ordinary (non-repair) session - * records its finalize `close` and writes its session log here — see the - * call site's own comment for why a repair-armed session skips this entirely. - * - * #1391: the write can refuse to publish (no-clobber target-exists, or any - * other fs `AppError`). Unlike a repair-armed commit failure — which keeps - * its session alive so `commitRepairBeforeClose` must roll back its - * just-recorded `close` action to keep a later retry from duplicating it — - * this caller (`runSessionCloseTeardown`) always completes teardown and - * deletes the session regardless of the outcome returned here, so there is - * no surviving session for a retry to duplicate anything on. No rollback: - * the recorded `close` action (and its durable `events.ndjson` entry) stay - * exactly as they are — an accurate record that the close itself happened, - * independent of whether the script also got saved. - */ +// #1391: unlike commitRepairBeforeClose, this never rolls back the recorded +// `close` action on a write failure — the caller always tears the session +// down regardless, so there's no surviving session for a retry to duplicate +// it on. function finalizeOrdinaryCloseScript(params: { req: DaemonRequest; session: SessionState; @@ -254,21 +235,9 @@ function finalizeOrdinaryCloseScript(params: { } } -/** - * #1391: normalizes an ordinary (non-repair) session's close-time script-write - * failure. `SessionScriptWriter.write()` only ever throws a genuine `AppError` - * here (a non-clobber refusal or another fs AppError — see - * `handleSessionScriptWriteFailure`'s non-repair, non-active-publication - * branch); anything else is already swallowed into a silent `{written:false}` - * there. Only the message/hint/retriable are corrected for THIS call site (the - * shared `publishHealedScriptAtomically` wording, "retry close --save-script", - * describes the repair-commit retry contract, which does not apply here — by - * the time the agent sees this, `close` has already released the device and - * deleted the session, so there is nothing left to retry in place); the - * original error's machine-readable `details` (e.g. `reason: - * "script_target_exists"`, `path`) are preserved so a caller dispatching on - * them still can. - */ +// #1391: the shared write error's message/details/reason are preserved +// as-is; only hint/retriable are overridden, since its default "retry close +// --save-script" wording no longer applies once the session is gone. function toOrdinaryCloseSaveScriptFailure(error: unknown): AppError { const overrides = { hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.', @@ -506,21 +475,10 @@ type SessionCloseFinalization = | { kind: 'response'; response: DaemonResponse } | { kind: 'closed'; providerData?: Record }; -/** - * Everything between a settled repair decision and a success response: the - * failure-isolated resource teardown, the provider lease release, and — only - * once both have run — the device-claim clear and session delete. A rejected - * cleanup step is collected instead of short-circuiting the rest, so every - * subsequent resource (and the runner stop) is still attempted; the provider - * lease is released only after that teardown, and a failed release keeps the - * session retryable (returned as `{kind:'response'}`, mirroring the - * repair-commit-failure path above it). The platform-close failure is thrown - * as the primary error with its original code/details/hint intact; the - * cleanup aggregate has already been emitted as a diagnostic by this point so - * per-resource failures stay visible; a failed script save (#1391) is thrown - * last since — unlike the two above it — the session has already ended and - * the device is already released by the time it surfaces. - */ +// Everything between a settled repair decision and a success response: +// failure-isolated resource teardown, provider lease release, then (only +// once both are known) the device-claim clear and session delete. A failed +// lease release keeps the session retryable instead (`{kind:'response'}`). async function runCloseTeardownAndRelease(params: { req: DaemonRequest; session: SessionState; @@ -561,15 +519,14 @@ async function runCloseTeardownAndRelease(params: { phase: 'session_close_cleanup_failed', failures: cleanupFailures, }); - // #1391: a failed script save is never a reason to keep the device claimed — - // only a genuine platform-close failure (the device may still be busy) or a - // resource-cleanup failure withholds it, exactly as before. - if (!platformCloseError && !cleanupAggregate) { + // Only these two withhold the device claim (the device may still be busy); + // a failed script save (#1391) never does — it always releases below. + const blockingError = platformCloseError ?? cleanupAggregate; + if (!blockingError) { await clearAdvisoryDeviceClaim(session.deviceClaim); } sessionStore.delete(sessionName); - if (platformCloseError) throw platformCloseError; - if (cleanupAggregate) throw cleanupAggregate; + if (blockingError) throw blockingError; if (saveScriptError) throw saveScriptError; return { kind: 'closed', providerData: leaseRelease.providerData }; } From 79bdf89fa7a2a53ed38114e08bc1de197ff65fd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 27 Jul 2026 15:52:44 +0200 Subject: [PATCH 9/9] refactor(daemon): clarify close script finalization --- scripts/help-conformance-cases.mjs | 35 ++++ scripts/layering/session-state.ts | 2 +- .../__tests__/session-close-script.test.ts | 105 +++++++++++ src/daemon/handlers/session-close-script.ts | 103 +++++++++++ src/daemon/handlers/session-close.ts | 169 ++---------------- src/daemon/session-store.ts | 6 +- 6 files changed, 260 insertions(+), 160 deletions(-) create mode 100644 src/daemon/handlers/__tests__/session-close-script.test.ts create mode 100644 src/daemon/handlers/session-close-script.ts diff --git a/scripts/help-conformance-cases.mjs b/scripts/help-conformance-cases.mjs index 942eab346..7959d59d1 100644 --- a/scripts/help-conformance-cases.mjs +++ b/scripts/help-conformance-cases.mjs @@ -155,6 +155,41 @@ export const CASES = [ { id: 'noRawAdbKeyevent', pattern: /\badb\s+shell\s+input\b/i }, ], }, + { + id: 'ios-system-ui-widget-flow', + docs: ['--help:first30', 'ios-system-ui'], + task: 'On an iOS simulator, add the Calendar widget from SpringBoard, capture visual evidence of the placed widget, return to the installed app com.example.calendar, and close. Plan commands only.', + expectations: ['validPlanCommands', 'fullPrefix', 'usesSnapshotI', 'opensAndCloses'], + matchers: [ + { + id: 'bindsSessionToSpringBoard', + pattern: /\bagent-device\s+open\s+com\.apple\.springboard\b[^\n]*--platform\s+ios\b/i, + }, + { + id: 'entersEditModeWithCoordinateLongpress', + pattern: /\bagent-device\s+longpress\s+-?\d+(?:\.\d+)?\s+-?\d+(?:\.\d+)?\b/i, + }, + { + id: 'refreshesSnapshotAfterEnteringEditMode', + pattern: /\blongpress\b[\s\S]*\n[^\n]*\bsnapshot\s+-i\b/i, + }, + { + id: 'usesScreenshotForSparseGalleryResult', + pattern: + /\bagent-device\s+screenshot\b[\s\S]*\n[^\n]*\bagent-device\s+press\s+-?\d+(?:\.\d+)?\s+-?\d+(?:\.\d+)?\b/i, + }, + { + id: 'returnsToAppUnderTest', + pattern: /\bagent-device\s+open\s+com\.example\.calendar\b[^\n]*--platform\s+ios\b/i, + }, + ], + forbidden: [ + { + id: 'noRawSimulatorControl', + pattern: /(?:^|\n)(?:xcrun\s+simctl|idb|maestro)\b/i, + }, + ], + }, { id: 'web-managed-backend-loop', docs: ['--help:first30', 'web'], diff --git a/scripts/layering/session-state.ts b/scripts/layering/session-state.ts index 271b8f8b1..fdc268133 100644 --- a/scripts/layering/session-state.ts +++ b/scripts/layering/session-state.ts @@ -57,7 +57,7 @@ export const SESSION_STATE_FIELD_OWNERS: Readonly { + vi.restoreAllMocks(); + for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true }); +}); + +function setup(name: string) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-session-close-script-')); + roots.push(root); + const sessionStore = new SessionStore(path.join(root, 'sessions')); + const session = makeIosSession(name, { appBundleId: 'com.example.app' }); + sessionStore.set(name, session); + const req: DaemonRequest = { + token: 'token', + session: name, + command: 'close', + positionals: [], + flags: {}, + }; + return { req, session, sessionStore }; +} + +test('failed repair publication removes only its synthetic close before retry', () => { + const { req, session, sessionStore } = setup('repair'); + session.saveScriptBoundary = 0; + session.saveScriptComplete = true; + session.recordSession = true; + const failure = new AppError('COMMAND_FAILED', 'publish failed'); + vi.spyOn(sessionStore, 'writeSessionLog').mockReturnValue({ written: false, error: failure }); + + expect(commitRepairScriptBeforeClose(sessionStore, session, req)).toEqual({ + kind: 'failed', + error: failure, + }); + expect(session.actions).toEqual([]); +}); + +test('repair close failure keeps normalized metadata and is explicitly retriable', () => { + const { session } = setup('repair-error'); + session.saveScriptPath = '/tmp/repaired.ad'; + const failure = new AppError('COMMAND_FAILED', 'publish failed', { + reason: 'target-exists', + hint: 'Choose another path.', + logPath: '/tmp/daemon.log', + }); + + expect(buildRetriableRepairCloseFailureResponse(session, failure)).toEqual({ + ok: false, + error: { + code: 'COMMAND_FAILED', + message: 'publish failed', + hint: 'Choose another path.', + logPath: '/tmp/daemon.log', + details: { + reason: 'target-exists', + session: 'repair-error', + savedScript: '/tmp/repaired.ad', + }, + retriable: true, + }, + }); +}); + +test('ordinary publication failure retains its close action after making the error non-retriable', () => { + const { req, session, sessionStore } = setup('ordinary'); + const failure = new AppError('COMMAND_FAILED', 'target exists', { + reason: 'target-exists', + hint: 'Retry close.', + }); + vi.spyOn(sessionStore, 'writeSessionLog').mockImplementation(() => { + throw failure; + }); + + const result = finalizeOrdinaryCloseScript({ + req: { ...req, flags: { saveScript: true } }, + session, + sessionStore, + platformCloseError: undefined, + }); + + expect(session.actions.map(({ command }) => command)).toEqual(['close']); + expect(result).toMatchObject({ + code: 'COMMAND_FAILED', + message: 'The session was closed, but its script was not saved: target exists', + details: { + reason: 'target-exists', + retriable: false, + }, + }); +}); diff --git a/src/daemon/handlers/session-close-script.ts b/src/daemon/handlers/session-close-script.ts new file mode 100644 index 000000000..338ba4244 --- /dev/null +++ b/src/daemon/handlers/session-close-script.ts @@ -0,0 +1,103 @@ +import { AppError, normalizeError } from '../../kernel/errors.ts'; +import { successText } from '../../utils/success-text.ts'; +import type { SessionStore } from '../session-store.ts'; +import type { DaemonRequest, DaemonResponse, SessionState } from '../types.ts'; +import { recordSessionAction } from './handler-utils.ts'; + +export type RepairCloseCommit = + | { kind: 'not-armed' } + | { kind: 'committed'; path?: string } + | { kind: 'aborted' } + | { kind: 'failed'; error: AppError }; + +function shouldOverwriteSavedScript(req: DaemonRequest, session: SessionState): boolean { + return Boolean(req.flags?.force || session.saveScriptForce); +} + +export function commitRepairScriptBeforeClose( + sessionStore: SessionStore, + session: SessionState, + req: DaemonRequest, +): RepairCloseCommit { + if (session.saveScriptBoundary === undefined) return { kind: 'not-armed' }; + + const actionsBeforeClose = session.actions.length; + recordSessionAction(sessionStore, session, req, 'close', { + session: session.name, + ...successText(`Closed: ${session.name}`), + }); + const result = sessionStore.writeSessionLog(session, { + force: shouldOverwriteSavedScript(req, session), + }); + if (result.written) return { kind: 'committed', path: result.path }; + if (result.error) { + // The retained repair session can retry without accumulating synthetic closes. + session.actions.length = actionsBeforeClose; + return { kind: 'failed', error: result.error }; + } + return session.saveScriptComplete ? { kind: 'committed' } : { kind: 'aborted' }; +} + +export function buildRetriableRepairCloseFailureResponse( + session: SessionState, + error: AppError, +): DaemonResponse { + const normalized = normalizeError(error); + return { + ok: false, + error: { + ...normalized, + details: { + ...normalized.details, + session: session.name, + ...(session.saveScriptPath ? { savedScript: session.saveScriptPath } : {}), + }, + retriable: true, + }, + }; +} + +export function finalizeOrdinaryCloseScript(params: { + req: DaemonRequest; + session: SessionState; + sessionStore: SessionStore; + platformCloseError: unknown; +}): AppError | undefined { + const { req, session, sessionStore, platformCloseError } = params; + if (!platformCloseError) { + recordSessionAction(sessionStore, session, req, 'close', { + session: session.name, + ...successText(`Closed: ${session.name}`), + }); + } + if (req.flags?.saveScript) session.recordSession = true; + + try { + sessionStore.writeSessionLog(session, { + force: shouldOverwriteSavedScript(req, session), + }); + return undefined; + } catch (error) { + return toOrdinaryCloseSaveScriptFailure(error); + } +} + +function toOrdinaryCloseSaveScriptFailure(error: unknown): AppError { + const overrides = { + hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.', + retriable: false, + }; + if (error instanceof AppError) { + return new AppError( + 'COMMAND_FAILED', + `The session was closed, but its script was not saved: ${error.message}`, + { ...error.details, ...overrides }, + error.cause, + ); + } + return new AppError( + 'COMMAND_FAILED', + `The session was closed, but its script was not saved: ${normalizeError(error).message}`, + overrides, + ); +} diff --git a/src/daemon/handlers/session-close.ts b/src/daemon/handlers/session-close.ts index b1cecc3fb..ee1c570c5 100644 --- a/src/daemon/handlers/session-close.ts +++ b/src/daemon/handlers/session-close.ts @@ -23,7 +23,6 @@ import { } from './session-device-utils.ts'; import { errorResponse } from './response.ts'; import { expireRefFrame } from '../ref-frame.ts'; -import { recordSessionAction } from './handler-utils.ts'; import { stopSessionRecordingForTeardown } from './record-trace-recording.ts'; import type { LeaseRegistry } from '../lease-registry.ts'; import { releaseSessionLease } from '../lease-lifecycle.ts'; @@ -40,6 +39,11 @@ import { type SessionCleanupFailure, } from '../session-teardown.ts'; import { clearAdvisoryDeviceClaim } from '../device-claims.ts'; +import { + buildRetriableRepairCloseFailureResponse, + commitRepairScriptBeforeClose, + finalizeOrdinaryCloseScript, +} from './session-close-script.ts'; async function maybeShutdownSessionTarget(params: { device: DeviceInfo; @@ -52,17 +56,6 @@ async function maybeShutdownSessionTarget(params: { return await shutdownDeviceTarget(device); } -/** - * #1258: the effective `--force`/`--overwrite` decision for a `close`-time - * publish — this close request's own flag OR'd with whatever was persisted - * on the session at an earlier arm (`open --save-script --force`, or a - * repair's first `replay --save-script --force`). Either source authorizes - * overwriting; neither being set keeps the default refuse-on-exist. - */ -function resolveEffectiveSaveScriptForce(req: DaemonRequest, session: SessionState): boolean { - return Boolean(req.flags?.force || session.saveScriptForce); -} - function shouldRetainAppleRunnerAfterClose(req: DaemonRequest, session: SessionState): boolean { return ( isIosSimulator(session.device) && @@ -77,88 +70,6 @@ function shouldStopAppleRunnerBeforeTargetedClose(session: SessionState): boolea return isApplePlatform(session.device.platform) && !isIosSimulator(session.device); } -/** - * ADR 0012 decision 6 (BLOCKER 2): outcome of committing a repair transaction - * at `close` time, BEFORE any destructive teardown. `not-armed` = not a repair - * session (normal close flow); `committed` = the healed `.ad` was written - * (`path`) or the transaction was incomplete and intentionally discarded (no - * `path`) — either way close proceeds and tears the session down; `failed` = a - * COMPLETE transaction's commit failed (no-clobber / bare-`@ref` / fs error), - * so the session must be KEPT for retry and the failure surfaced. - */ -type RepairCloseOutcome = - | { kind: 'not-armed' } - | { kind: 'committed'; path?: string } - | { kind: 'aborted' } - | { kind: 'failed'; error: AppError }; - -function commitRepairBeforeClose( - sessionStore: SessionStore, - session: SessionState, - req: DaemonRequest, -): RepairCloseOutcome { - if (session.saveScriptBoundary === undefined) return { kind: 'not-armed' }; - // Record the finalize `close` (so the committed healed slice ends with it), - // then COMMIT before any destructive teardown. A repair-armed session commits - // iff the transaction COMPLETED, regardless of `--save-script` on the close - // (C2); `recordSession` is already true from arming. - const actionsBeforeClose = session.actions.length; - recordSessionAction(sessionStore, session, req, 'close', { - session: session.name, - ...successText(`Closed: ${session.name}`), - }); - const result = sessionStore.writeSessionLog(session, { - force: resolveEffectiveSaveScriptForce(req, session), - }); - if (result.written) return { kind: 'committed', path: result.path }; - if (result.error) { - // The session is kept for retry (BLOCKER 2b): roll back the just-recorded - // finalize `close` so a subsequent `close --save-script=` retry does - // not accumulate duplicate `close` lines in the healed slice. - session.actions.length = actionsBeforeClose; - return { kind: 'failed', error: result.error }; - } - if (!session.saveScriptComplete) { - return { kind: 'aborted' }; - } - return { kind: 'committed' }; -} - -/** - * ADR 0012 decision 6 (BLOCKER 2b): a commit-failure close response. The session - * is intentionally NOT torn down (the caller returns before teardown), so the - * agent can fix the cause and retry `close --save-script`. - * - * BLOCKER 2 (second follow-up): routes `error` through the SAME - * `normalizeError` normalization every other AppError -> DaemonResponse - * conversion in this codebase uses (see `repairExpiredIfTombstoned` in - * request-router.ts and the dozens of handler call sites doing - * `{ ok: false, error: normalizeError(error) }`) — a hand-rolled reshape here - * previously dropped the underlying platform/commit error's `details`, - * `diagnosticId`, and `logPath` entirely, and put `retriable` under - * `error.details.retriable`, a location neither the router's `enrichDaemonError` - * nor the client reads (both read the TOP-LEVEL `error.retriable` — see - * `DaemonError` in kernel/contracts.ts). `retriable: true` is still forced - * unconditionally at the end: the session was preserved specifically so the - * agent can retry (`close`/`close --save-script=`), which must never - * be contradicted by the underlying error's own (usually absent) classification. - */ -function buildRepairCloseFailureResponse(session: SessionState, error: AppError): DaemonResponse { - const normalized = normalizeError(error); - return { - ok: false, - error: { - ...normalized, - details: { - ...normalized.details, - session: session.name, - ...(session.saveScriptPath ? { savedScript: session.saveScriptPath } : {}), - }, - retriable: true, - }, - }; -} - function toRepairPlatformCloseFailure(error: unknown): AppError { if (error instanceof AppError) return error; const detail = error instanceof Error ? error.message : String(error); @@ -169,8 +80,6 @@ function toRepairPlatformCloseFailure(error: unknown): AppError { type SessionCloseTeardownResult = { platformCloseError: unknown; - // #1391: a failed close-time script write (see finalizeOrdinaryCloseScript). - // Never blocks teardown — see runCloseTeardownAndRelease's blockingError. saveScriptError?: AppError; }; @@ -207,58 +116,6 @@ async function runSessionCloseTeardown(params: { return { platformCloseError, saveScriptError }; } -// #1391: unlike commitRepairBeforeClose, this never rolls back the recorded -// `close` action on a write failure — the caller always tears the session -// down regardless, so there's no surviving session for a retry to duplicate -// it on. -function finalizeOrdinaryCloseScript(params: { - req: DaemonRequest; - session: SessionState; - sessionStore: SessionStore; - platformCloseError: unknown; -}): AppError | undefined { - const { req, session, sessionStore, platformCloseError } = params; - if (!platformCloseError) { - recordSessionAction(sessionStore, session, req, 'close', { - session: session.name, - ...successText(`Closed: ${session.name}`), - }); - } - if (req.flags?.saveScript) { - session.recordSession = true; - } - try { - sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) }); - return undefined; - } catch (error) { - return toOrdinaryCloseSaveScriptFailure(error); - } -} - -// #1391: the shared write error's message/details/reason are preserved -// as-is; only hint/retriable are overridden, since its default "retry close -// --save-script" wording no longer applies once the session is gone. -function toOrdinaryCloseSaveScriptFailure(error: unknown): AppError { - const overrides = { - hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.', - retriable: false, - }; - if (error instanceof AppError) { - return new AppError( - 'COMMAND_FAILED', - `The session was closed, but its script was not saved: ${error.message}`, - { ...error.details, ...overrides }, - error.cause, - ); - } - const detail = normalizeError(error).message; - return new AppError( - 'COMMAND_FAILED', - `The session was closed, but its script was not saved: ${detail}`, - overrides, - ); -} - type CleanupRunner = (step: string, run: () => Promise) => Promise; async function stopBestEffortSessionResources( @@ -301,7 +158,7 @@ async function prepareRepairClose(params: { const platformCloseError = await dispatchTargetedPlatformClose({ req, session, logPath }); if (platformCloseError) { return { - response: buildRepairCloseFailureResponse( + response: buildRetriableRepairCloseFailureResponse( session, toRepairPlatformCloseFailure(platformCloseError), ), @@ -309,9 +166,11 @@ async function prepareRepairClose(params: { } session.repairPlatformCloseReceipt = closeReceipt; } - const repairCommit = commitRepairBeforeClose(sessionStore, session, req); + const repairCommit = commitRepairScriptBeforeClose(sessionStore, session, req); if (repairCommit.kind === 'failed') { - return { response: buildRepairCloseFailureResponse(session, repairCommit.error) }; + return { + response: buildRetriableRepairCloseFailureResponse(session, repairCommit.error), + }; } session.repairPlatformCloseReceipt = undefined; return { @@ -519,14 +378,12 @@ async function runCloseTeardownAndRelease(params: { phase: 'session_close_cleanup_failed', failures: cleanupFailures, }); - // Only these two withhold the device claim (the device may still be busy); - // a failed script save (#1391) never does — it always releases below. - const blockingError = platformCloseError ?? cleanupAggregate; - if (!blockingError) { + const deviceClaimBlockingError = platformCloseError ?? cleanupAggregate; + if (!deviceClaimBlockingError) { await clearAdvisoryDeviceClaim(session.deviceClaim); } sessionStore.delete(sessionName); - if (blockingError) throw blockingError; + if (deviceClaimBlockingError) throw deviceClaimBlockingError; if (saveScriptError) throw saveScriptError; return { kind: 'closed', providerData: leaseRelease.providerData }; } diff --git a/src/daemon/session-store.ts b/src/daemon/session-store.ts index 48afd8dad..e1541ba5c 100644 --- a/src/daemon/session-store.ts +++ b/src/daemon/session-store.ts @@ -146,7 +146,7 @@ export class SessionStore { * teardown finalize step for a session (idle-reap or daemon shutdown). * * BLOCKER 3: unlike the explicit `close --save-script` path - * (`commitRepairBeforeClose`), teardown never runs `close`'s handler — but + * (`session-close-script.ts`), teardown never runs `close`'s handler — but * the source plan's terminal `close` was already skipped-while-armed (Fix * 3), so a COMPLETE transaction's auto-commit here must record the same * synthetic finalize `close` first, or the auto-committed healed `.ad` @@ -187,8 +187,8 @@ export class SessionStore { } /** - * BLOCKER 3: mirrors `commitRepairBeforeClose`'s finalize-`close` recording - * (session-close.ts) for the auto-commit teardown path, which never routes + * BLOCKER 3: mirrors the explicit close script's finalize-`close` recording + * (`session-close-script.ts`) for the auto-commit path, which never routes * through `close`'s handler. Only recorded when this teardown is actually * about to attempt a commit (COMPLETE, not yet COMMITTED) — an aborted * (incomplete) transaction's write is a no-op regardless, so there is