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 fcd38c1e4..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 { + 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/__tests__/session-close-script.test.ts b/src/daemon/handlers/__tests__/session-close-script.test.ts new file mode 100644 index 000000000..221fb67be --- /dev/null +++ b/src/daemon/handlers/__tests__/session-close-script.test.ts @@ -0,0 +1,105 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, expect, test, vi } from 'vitest'; +import { AppError } from '../../../kernel/errors.ts'; +import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; +import { SessionStore } from '../../session-store.ts'; +import type { DaemonRequest } from '../../types.ts'; +import { + buildRetriableRepairCloseFailureResponse, + commitRepairScriptBeforeClose, + finalizeOrdinaryCloseScript, +} from '../session-close-script.ts'; + +const roots: string[] = []; + +afterEach(() => { + 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/__tests__/session-device-claims.test.ts b/src/daemon/handlers/__tests__/session-device-claims.test.ts index 2481c6e0d..9a5763874 100644 --- a/src/daemon/handlers/__tests__/session-device-claims.test.ts +++ b/src/daemon/handlers/__tests__/session-device-claims.test.ts @@ -35,6 +35,8 @@ 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); const mockResolveTargetDevice = vi.mocked(resolveTargetDevice); @@ -245,3 +247,80 @@ 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'); + // 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, + createdAt: Date.now(), + 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 + // 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); + // 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'); + + 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-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 7e17dc8a9..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,96 +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, - }, - }; -} - -/** - * 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); @@ -175,10 +78,11 @@ function toRepairPlatformCloseFailure(error: unknown): 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. +type SessionCloseTeardownResult = { + platformCloseError: unknown; + saveScriptError?: AppError; +}; + async function runSessionCloseTeardown(params: { req: DaemonRequest; session: SessionState; @@ -187,12 +91,7 @@ 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 { +}): Promise { const { req, session, sessionName, logPath, sessionStore, cleanupFailures, repairArmed } = params; const attemptCleanup = async (step: string, run: () => Promise): Promise => { try { @@ -201,43 +100,20 @@ 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`. - if (!repairArmed) { - if (!platformCloseError) { - recordSessionAction(sessionStore, session, req, 'close', { - session: session.name, - ...successText(`Closed: ${session.name}`), - }); - } - if (req.flags?.saveScript) { - session.recordSession = true; - } - sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) }); - } + const saveScriptError = repairArmed + ? undefined + : finalizeOrdinaryCloseScript({ req, session, sessionStore, platformCloseError }); await attemptCleanup('materialized_paths', () => cleanupRetainedMaterializedPathsForSession(sessionName), ); - return platformCloseError; + return { platformCloseError, saveScriptError }; } type CleanupRunner = (step: string, run: () => Promise) => Promise; @@ -247,11 +123,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 () => { @@ -265,22 +137,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 ?? []); } @@ -296,29 +153,26 @@ 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 { - response: buildRepairCloseFailureResponse( + response: buildRetriableRepairCloseFailureResponse( session, toRepairPlatformCloseFailure(platformCloseError), ), }; } - session.repairPlatformCloseSucceeded = true; - session.repairPlatformCloseIdentity = closeIdentity; + 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.repairPlatformCloseSucceeded = false; - session.repairPlatformCloseIdentity = undefined; + session.repairPlatformCloseReceipt = undefined; return { repairArmed, ...(repairCommit.kind === 'committed' && repairCommit.path @@ -450,73 +304,99 @@ 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; - // 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: +// 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; + 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 = await runSessionCloseTeardown({ + const { platformCloseError, saveScriptError } = await runSessionCloseTeardown({ req, session, sessionName, logPath, sessionStore, cleanupFailures, - repairArmed: repair.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, + repairArmed: 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', failures: cleanupFailures, }); - if (!platformCloseError && !cleanupAggregate) { + const deviceClaimBlockingError = platformCloseError ?? cleanupAggregate; + if (!deviceClaimBlockingError) { 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; - 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) { + if (deviceClaimBlockingError) throw deviceClaimBlockingError; + if (saveScriptError) throw saveScriptError; + 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: { @@ -527,29 +407,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 }, }; } 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 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