refactor(daemon): model repair close retry as receipt

Author: thymikee

scripts/layering/session-state.ts

@@ -81,8 +81,7 @@ export const SESSION_STATE_FIELD_OWNERS: Readonly<Record<string, readonly string
   saveScriptCommitted: ['src/daemon/session-script-writer.ts'],
   repairSourcePath: ['src/daemon/handlers/session-replay-runtime.ts'],
   pendingRecordAndHeal: ['src/daemon/handlers/session-replay-resume.ts'],
-  repairPlatformCloseSucceeded: ['src/daemon/handlers/session-close.ts'],
-  repairPlatformCloseIdentity: ['src/daemon/handlers/session-close.ts'],
+  repairPlatformCloseReceipt: ['src/daemon/handlers/session-close.ts'],
 
   trace: ['src/daemon/handlers/record-trace.ts'],
   recording: ['src/daemon/handlers/record-trace-recording.ts'],

src/daemon/handlers/__tests__/session-device-claims.test.ts

@@ -314,30 +314,13 @@ test('#1391: a close-time script save failure still clears the advisory claim an
   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',
-  );
-  // 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);
 });

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

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<SessionCloseTeardownResult> {
   const { req, session, sessionName, logPath, sessionStore, cleanupFailures, repairArmed } = params;
   const attemptCleanup = async (step: string, run: () => Promise<void>): Promise<void> => {
@@ -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<void> {
-  // 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<RepairClosePreparation> {
   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,

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=<other>` 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