Skip to content

Commit cf6a5f1

Browse files
authored
fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish (#1235)
* fix(replay): repair-transaction lifecycle (ADR 0012 decision 6 / #1234) Agent-supervised re-record repair lifecycle, rebased onto #1225's failure-isolated close teardown. Consolidated from the earlier iterative rounds into the final teardown-commits model: - R7 keep-alive keyed off PERSISTED transaction state (repairSessionHeld signal), so a `replay --from` continuation without --save-script is still held on divergence. - Commit gated on transaction COMPLETION (saveScriptComplete/saveScriptCommitted), never on `close` alone — no prefix is ever published. - Single commit path: `commitRepairBeforeClose` runs before #1225's `runSessionCloseTeardown` destructive steps; a repair-armed session skips the teardown's ordinary writeSessionLog, non-repair keeps it. Idle-reap/shutdown commit-on-completion or tombstone via `finalizeRepairTeardown`. - BLOCKER fixes: reaped `replay --from` -> REPAIR_SESSION_EXPIRED; commit failures surfaced (not swallowed) and keep the session for retry (with healed-path reporting on success); race-safe atomic no-clobber publish; minimal `[open, close]` arms the transaction. Integrated with #1225: keeps runSessionCloseTeardown's failure-isolated cleanup + preserved platform-close error; repair commit happens first so a failed commit keeps the session addressable. * fix(replay): make the atomic publish primitive decide the no-clobber race winner BLOCKER 1 (coordinator re-review): after linkSync saw an existing target, the no-clobber publish fell back to an unconditional renameSync once the target was classified "incomplete" — two concurrent writers could both read the SAME pre-existing partial as overwritable and both renameSync over it, each returning success with no signal to the loser. A silent, undetectable clobber. publishNoClobberAtomically now makes every winner decision an atomic primitive: - linkSync is the only way to win outright (EEXIST iff a file is at the target at that instant). - On EEXIST, the existing file is grabbed via an atomic renameSync into a private, uniquely-named quarantine path *before* it is inspected, so the completeness check never races the shared path. A competing writer's own grab racing ours surfaces as ENOENT, and we re-evaluate from the top instead of trusting a stale read. - A COMPLETE quarantined file is restored (best effort) and the publish is refused; a genuinely partial one is discarded and the exclusive linkSync is retried. Every interleaving converges on exactly one winning linkSync and every other writer observing a definitive, thrown "already exists" — never two silent successes, never a torn file. Adds a regression (session-script-writer.test.ts) with both writers starting against the SAME pre-existing partial target, using a renameSync spy to force a genuine interleaving (writer B's whole publish runs inside writer A's grab step) instead of the existing competing-writer test's sequential complete-vs-complete scenario, which never exercised this race. * fix(daemon): preserve failed COMPLETE-transaction commits instead of a generic expiry BLOCKER 2 (coordinator re-review): finalizeRepairTeardown ignored the writer's { written: false, error } outcome, so a COMPLETE transaction whose commit failed at idle-reap/daemon-shutdown teardown (no-clobber refusal, bare-@ref, or a filesystem error) was silently swallowed. Daemon teardown then deleted the session and left a generic "reaped before it was finalized" REPAIR_SESSION_EXPIRED tombstone — losing the only record that a commit was even attempted, let alone why it failed. finalizeRepairTeardown now captures the writer's result. On a real commit failure it writes a distinct commit-failure tombstone (RepairSessionTombstone gains an optional commitFailure: { code, message }); request-router's repairExpiredIfTombstoned surfaces that as a new REPAIR_COMMIT_FAILED error carrying the real cause instead of folding it into REPAIR_SESSION_EXPIRED. Adds a new AppErrorCode REPAIR_COMMIT_FAILED (kernel/errors.ts) with its own hint, a session-store regression proving finalizeRepairTeardown preserves the failure (and leaves the prior complete artifact untouched), and a request-router regression proving the router translates a commit-failure tombstone to REPAIR_COMMIT_FAILED rather than the generic expiry. * fix(daemon): record the skipped terminal close in idle-reap/shutdown auto-commit BLOCKER 3 (coordinator re-review): the source plan's terminal `close` is skipped-while-armed (Fix 3), so it never lands in session.actions. The explicit `close --save-script` path accounts for this by recording a synthetic finalize close (commitRepairBeforeClose) before committing, but finalizeRepairTeardown's auto-commit at idle-reap/daemon-shutdown never runs that handler — its committed healed .ad was missing its own terminal close, so the ADR's "self-contained, fresh-replayable artifact" requirement didn't hold for this path even though the existing auto-commit test only checked existence + the completeness sentinel. finalizeRepairTeardown now calls a new recordRepairFinalizeCloseIfCommitting before writeSessionLog, mirroring commitRepairBeforeClose's recording exactly (same command/positionals/flags shape), but only when the transaction is actually about to be committed (COMPLETE, not yet COMMITTED) — an aborted transaction's write is a no-op regardless. Strengthens the existing auto-commit test (session-replay-repair-transaction .test.ts) to use a source plan with a real terminal close, and to parse the committed script and assert it ends with ['open', 'click', 'close'] with no bare @ref — not just sentinel/existence. Adds a session-store.ts unit regression exercising finalizeRepairTeardown directly for the same self-contained-artifact assertion. * fix(daemon): serialize the no-clobber publish decision behind an exclusive lock BLOCKER 1 (review follow-up): publishNoClobberAtomically's inspect/restore/ publish sequence was atomic per-step but not exclusive as a whole. Writer A could quarantine an existing COMPLETE target, and — before A restored it — writer B could linkSync its own COMPLETE artifact into the now-empty target and return success, only for A's restore (renameSync, which replaces an existing destination per POSIX) to silently stomp B's freshly published bytes. Wrap the whole decide-and-act sequence in an exclusive publish lock (acquireNoClobberLock/releaseNoClobberLock, an atomic linkSync claim over a PID-stamped lock file) so a competing writer for the same scriptPath cannot begin its own decision until the lock holder's sequence has finished and released it. A lock whose PID is provably dead is reclaimed immediately; a lock held by a live process is never stolen, only waited on with a bounded backoff before failing loudly. Adds a deterministic regression that forces the exact reported interleaving via a renameSync spy (mirroring the existing BLOCKER 1 test's technique) and confirms the pre-existing COMPLETE artifact is never clobbered. Also relaxes the older PARTIAL-race test's loser-message assertion, since a losing writer may now fail via lock contention instead of the no-clobber-specific message, depending on interleaving timing. * fix(daemon): report retriable:true for a preserved repair-close failure BLOCKER 3: buildRepairCloseFailureResponse preserves the session specifically so the agent can retry close/close --save-script, but reported details.retriable: false — machine-consistent recovery guidance requires retriable: true whenever the session was kept addressable for a retry. Extends the existing BLOCKER 2b/2c no-clobber-failure test with an assertion on this contract. * fix(daemon): run the repair close's platform close before committing BLOCKER 2: commitRepairBeforeClose recorded a successful terminal `close` and published the healed artifact BEFORE dispatchTargetedPlatformClose ran. If the platform close then failed, the session was torn down and the committed .ad falsely contained a successful close — contradicting the existing failed-close lifecycle contract (a failed close is never recorded as Closed). For a repair-armed session, dispatch the targeted platform close first; only on success does the commit (record + publish) proceed. On failure, return without touching the session at all — same as the existing commit-failure contract, so the agent can fix the cause and retry. runSessionCloseTeardown gains a skipPlatformClose flag so the already-confirmed-successful close is never dispatched a second time during teardown. Adds a regression: a COMPLETE repair whose targeted platform close rejects must not commit a healed .ad, must not record a close action, and must keep the session addressable for retry; a subsequent successful retry then commits cleanly with exactly one terminal close. * fix(daemon): close the no-clobber lock's dead-writer TOCTOU with rename-CAS Two waiters could both observe the same dead-PID publish lock and both decide to reclaim it. If one waiter's reclaim (remove + re-acquire with its own LIVE lock) completed inside the other's decision window, the first waiter's stale rmSync(lockPath) deleted the SECOND waiter's live lock by pathname (not the dead one it actually inspected), letting both enter the exclusive publish section at once. Reclaim is now a rename-based compare-and-swap: renameSync(lockPath, uniquePath) is the atomic claim (only one racer's rename of a given source ever succeeds; the loser gets ENOENT and retries). Only the winner inspects what it actually grabbed at the private claim path — if genuinely dead, discard it; if the claim raced with someone else's fresh reclaim and grabbed their live lock instead, restore it untouched and back off. The live holder's lock is never stolen. Regression drives the exact two-reclaimer interleaving deterministically via a readFileSync spy (writer B reclaims+re-acquires live, inside writer A's reclaim window) and confirms it fails against the prior rmSync-based reclaim. * fix(daemon): surface repair-close retriable/diagnosticId/logPath at the wire top level buildRepairCloseFailureResponse hand-rolled its response shape instead of going through normalizeError, so it put retriable under error.details.retriable — a location neither the router's enrichDaemonError nor the client reads (both read the top-level DaemonError.retriable) — and silently dropped the underlying platform/ commit error's details, diagnosticId, and logPath entirely. Now routes through normalizeError like every other AppError -> DaemonResponse conversion in this codebase, preserving the underlying error's details/diagnosticId/logPath, with retriable forced true at the top level (the session is retained specifically for retry, which must never be contradicted by the underlying error's own classification). Also fixes a companion gap in finalizeDaemonResponse: it rebuilds every handler-RETURNED (non-thrown) failure response into a fresh AppError before re-normalizing, but only carried hint/diagnosticId/logPath through that reconstruction, not retriable/supportedOn — so even a handler setting them correctly at the top level still lost them at this step. Both are now carried through the same way, discovered only by verifying the close fix through the actual router boundary as requested. Regression: an updated handler-level test confirms diagnosticId/ logPath/details survive a repair-close failure, and a new router-level test (through createRequestHandler, not just the raw builder) confirms retriable:true and the platform error's diagnosticId/logPath/details all survive to the client. Both fail against the pre-fix code. * fix(daemon): never re-dispatch an already-succeeded repair-close platform close When a repair-armed close's targeted platform close SUCCEEDED but the subsequent script commit FAILED (no-clobber refusal, a bare-@ref failure, or an fs error), the session was correctly retained for retry -- but nothing recorded that the platform close had already happened. A retry (close --save-script=<other>) dispatched dispatchTargetedPlatformClose again, so a non-idempotent backend could fail or wedge recovery on a second close of an already-closed target. SessionState now carries repairPlatformCloseSucceeded, set the moment the platform close returns success. A subsequent repair close consumes it and skips straight to the commit instead of re-dispatching; it is cleared once the transaction's outcome (commit or abort) is settled, so it never lingers past a single close attempt. Regression: platform close succeeds, commit fails (no-clobber), session is retained; a retry does not re-invoke dispatchTargetedPlatformClose (asserted via call count) and still commits cleanly to the retry path. Fails against the pre-fix code (dispatch called twice). * fix(daemon): enforce complete-artifact protection on explicit --save-script targets The explicit --save-script=<path> publish path bypassed the no-clobber completeness guard entirely (protectComplete only gated on the DEFAULT healed-sibling marker), so it silently overwrote even a sentinel-marked COMPLETE healed artifact at a caller-directed path. An explicit target is caller-DIRECTED (which path to write to), never caller-AUTHORIZED to destroy an unreviewed prior healed diff sitting there. Gate protectComplete on repairArmed instead of the defaulted-path marker, so every repair-armed publish (default sibling or explicit target alike) refuses to clobber a COMPLETE artifact. Ordinary (non-repair) recordings are unaffected: they never carry the completeness sentinel, so the guard never actually engages for them. * fix(daemon): replace PID-liveness lock reclaim with a TTL publish lease reclaimDeadLock (grab lock away -> inspect PID -> restore if live) was structurally race-prone: a three-writer interleaving let waiter A rename waiter B's now-LIVE lock away (to inspect it), waiter C linkSync its own lock into the momentarily-empty path, then A's "restore" (renameSync, which replaces an existing destination) silently clobbered C's freshly acquired lock -- and the pathname-based release could then remove a successor's lock, not the caller's own. Replace it with a TTL lease (LEASE_TTL_MS = 30s). The lock file's content is now a unique owner token plus its own creation timestamp (pid:random:createdAtMs); staleness is judged purely from that embedded timestamp, never by asking the OS whether a PID is alive. A stale lease is stolen via a single atomic renameSync(lockPath, <lockPath>.expired.<id>) -- exactly one caller can ever win that rename for a still-existing source -- and the grabbed content is always discarded outright: there is no restore path at all. verifyOwnership re-checks the lease immediately before the publish critical section, so a writer whose lease gets displaced underneath it (by a stale steal decision racing a concurrent re-acquire) is never fooled into publishing unprotected -- it safely aborts instead. releaseLease only unlinks the lock file when its current token still matches the caller's own, so release can never delete a successor's lock. Regression coverage (session-script-writer.test.ts): a fresh lease is never stolen; an expired lease is stolen and reclaimed cleanly; and the reviewer's exact three-writer interleaving (a stale steal decision grabbing a concurrently-re-acquired fresh lease) is driven deterministically via spies on renameSync/linkSync, asserting exactly one holder ever enters the critical section, no live claim is silently clobbered by a restore, and release never deletes a successor's lock. Confirmed the new tests fail against the old reclaimDeadLock implementation and pass with the lease. * fix(daemon): bind the repair-close platform-close marker to request identity 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 flag was still set as though a real close had run; a retry with a DIFFERENT identity -- a target newly added, or a changed target -- then wrongly skipped the platform close entirely and committed as though it had run. Bind the marker to the request's identity: repairPlatformCloseIdentity records the target (positionals) of the close whose platform close last succeeded -- the only thing that changes what dispatchTargetedPlatformClose actually does (close's other flags, shutdown and saveScript, feed the post-teardown shutdown and the commit path respectively, never the platform close dispatch itself). A retry only skips the platform close when BOTH repairPlatformCloseSucceeded is true AND the identity matches; otherwise it re-runs. Regressions (session-replay-repair-transaction.test.ts): an untargeted-then-targeted retry and a changed-target retry both must re-dispatch the platform close (asserted via the dispatch mock call count/args), not skip it. Confirmed both fail against the prior session-wide boolean and pass with the identity-bound marker. * fix(daemon): surface a shutdown-time repair-commit failure before client cleanup A successful owned one-shot replay --save-script marks the transaction COMPLETE and returns success BEFORE publication -- the actual commit is deferred to daemon teardown (finalizeRepairTeardown), which runs inside the daemon process's own shutdown handler and, on failure, writes a REPAIR_COMMIT_FAILED tombstone. cleanupDaemonAfterRequest then removed the owned ephemeral state dir REGARDLESS of that tombstone, so the caller received success while the failure and its only recovery evidence were deleted in the same breath. session-store.ts exports findUnrecoveredRepairCommitFailure(sessionsDir), scanning every session subdirectory for a non-expired tombstone carrying commitFailure -- the client has no live SessionStore/session name to key off of, only the owned state dir's filesystem path. cleanupDaemonAfterRequest checks for it (after stopDaemonProcessForTakeover, which waits for the daemon to actually exit -- by then any tombstone the daemon's own shutdown handler would write is already on disk) before rmSync'ing the state dir: if found, the state dir is preserved and the response is overridden to a REPAIR_COMMIT_FAILED error instead of the raw success. daemon-client.ts's sendToDaemon now returns cleanup's result rather than the raw request result (restructured as a caught-and-rethrown error rather than a `return` inside `finally`, which oxlint's no-unsafe-finally rejects and which would also swallow a thrown request failure). Regression (daemon-client-lifecycle.test.ts): forces a shutdown-time commit failure by pre-seeding the tombstone in the owned state dir before the client's cleanup runs, and asserts the REPAIR_COMMIT_FAILED response is surfaced and the state dir (with the tombstone) survives. Confirmed it fails without the fix (raw success returned, state dir removed). * fix(daemon): replace the no-clobber publish lock with refuse-on-exist The TTL-lease/reclaim machinery only existed to auto-overwrite a partial healed artifact while never clobbering a complete one — but a concurrent complete-vs-complete race was already correct with a plain exclusive linkSync (first wins, second sees EEXIST), and a leftover partial is a degenerate state, not something to silently replace. Publish is now a single exclusive linkSync: absent target succeeds, ANY pre-existing target (complete or partial, default sibling or explicit --save-script path) is refused. Removes the whole lock/lease/reclaim race class. * docs(daemon): scope refuse-on-exist contract/comments to ordinary recording too PR #1235 review blocker: SessionScriptWriter.write's refuse-on-exist publish is uniform across repair-armed heals AND ordinary (non-repair) open/close --save-script recording, but the ADR contract and several comments still read as if only healed repair publication is refused and ordinary recording keeps the old rename-replace overwrite. Maintainer decision is to keep the behavior uniform and fix the docs/comments/coverage instead of re-scoping. - session-script-writer.ts: clarify isRepairArmedWriteBlocked only gates whether a publish is attempted, not what refuse-on-exist does once it is; fix write()'s catch-block comment, which claimed no AppError was ever raised on the ordinary path (now false since refuse-on-exist is uniform); broaden publishHealedScriptAtomically's doc to state it is write()'s only publish primitive for every target, referencing the removed publishOverwriteAtomically and the future --force/--overwrite (#1258). - session-action-recorder.ts / session-replay-runtime.ts / types.ts: fix comments claiming an explicit --save-script=<path> (or the saveScriptDefaultedHealedPath marker) is exempt from the clobber guard; the guard is uniform regardless of path origin or repair-armed status. - docs/adr/0012-interactive-replay.md: add a "Scope" paragraph making the refusal explicitly uniform across repair and ordinary recording, and extend the decision-6 acceptance-test bullet and migration-plan step 9 bullet to require ordinary-recording no-clobber coverage too. - session-script-writer.test.ts: add the missing existing-target coverage for the ORDINARY (non-repair, no saveScriptBoundary) path — refused with bytes unchanged when the target exists (thrown, since ordinary writes rethrow AppErrors rather than returning them), and confirmed to still succeed against an absent target. No runtime behavior change: publish is still a single uniform exclusive linkSync for every --save-script target.
1 parent 139153c commit cf6a5f1

25 files changed

Lines changed: 3247 additions & 131 deletions

‎docs/adr/0012-interactive-replay.md‎

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -576,10 +576,12 @@ Implementation is not accepted on benchmark evidence alone. Required automated c
576576
`COMPLETE` transaction committing (not tombstoning) — and is idempotent with no re-publish once
577577
`COMMITTED`; a terminal-close test proving an armed replay reaching the terminal source `close`
578578
**skips** it without deleting the session (C4); an atomic-publication test proving the temp file is
579-
created in the target's own directory and the no-clobber is race-safe (create-exclusive/rename-if-absent);
580-
and a no-clobber test proving an existing *complete* (sentinel-marked) healed artifact is protected
581-
while an incomplete/partial one is
582-
overwritable.
579+
created in the target's own directory and published via a single exclusive `linkSync`
580+
(create-if-absent, first writer wins); and a no-clobber test proving publication refuses ANY
581+
pre-existing target — complete or partial alike, byte-for-byte unchanged — for both the default healed
582+
sibling and an explicit `--save-script=<path>`, **and for an ordinary (non-repair) `open`/`close
583+
--save-script` recording whose target already exists** — the writer entry point and publish primitive
584+
are shared, so the refusal is uniform, not repair-only (see "Scope" below).
583585

584586
Extend the settle benchmark (`~/.agent-device-bench/rnnav-matrix.py` pattern, external harness) with a
585587
replay arm only after these contracts pass: measure clean replay and one induced divergence repaired
@@ -791,19 +793,37 @@ double-commit nor trip the no-clobber guard).
791793

792794
**Atomic, race-safe publication.** The commit serializes the healed slice (ending with a serialized
793795
`close` line so the artifact is self-contained) to a temp file created **in the same directory as the
794-
final target** — atomic `rename` requires the same filesystem, and an explicit `--save-script=<path>`
795-
target may live on a different mount than any process-wide temp dir — then atomically renames it into
796-
place. A reader therefore never observes a half-written healed script, and an aborted repair leaves no
797-
partial behind. The no-clobber guard is enforced **race-safely** by the atomic primitive itself
798-
(create-exclusive / rename-if-absent), not by a check-then-write.
799-
800-
**No-clobber applies only to a COMPLETE healed artifact.** The guard protects a *complete, review-worthy*
801-
artifact only. An incomplete or partial file is always overwritable by a fresh repair that reaches
802-
`COMMITTED`. Completeness is established by a sentinel written **only on commit** — a trailing
803-
`# agent-device:heal-complete` marker — or an equivalent structural check; the atomic temp→publish flow is
804-
what makes that sentinel trustworthy (a partial never reaches the published path carrying the sentinel).
805-
Auto-versioned output names (e.g. `.healed.2.ad`) are explicitly **out of scope** here — a separate
806-
naming change, not part of this decision.
796+
final target** — an explicit `--save-script=<path>` target may live on a different mount than any
797+
process-wide temp dir, and `linkSync` requires the same filesystem — then publishes with a single
798+
exclusive `linkSync(temp, target)`: create-if-absent, first writer wins. A reader therefore never
799+
observes a half-written healed script, and an aborted repair leaves no partial behind.
800+
801+
**Publication refuses ANY pre-existing target — complete or partial, default or explicit path alike.**
802+
An earlier design distinguished a *complete, review-worthy* artifact (protected) from an incomplete or
803+
partial one (silently overwritable), enforced through a publish lock. That distinction added a whole
804+
class of lock/lease/reclaim races for a case that is, in practice, a degenerate state: a partial healed
805+
file left behind by an aborted or reaped repair. The simpler, adopted design collapses this: the atomic
806+
`linkSync` itself decides the winner (`EEXIST` iff a file already sits at the target, regardless of its
807+
contents), and the caller must explicitly clear the way — remove the existing file, or pass a different
808+
`replay --save-script=<path>` — rather than have it silently replaced. No lock, no lease, no steal, no
809+
overwrite. The `# agent-device:heal-complete` trailing sentinel remains — it still marks a healed `.ad` as
810+
a complete, review-worthy repair artifact for any other reader — but it no longer participates in the
811+
publish decision. Auto-versioned output names (e.g. `.healed.2.ad`) are explicitly **out of scope** here —
812+
a separate naming change, not part of this decision.
813+
814+
**Scope: this refusal is uniform across repair AND ordinary recording, not repair-only.** Everything
815+
above is written in the context of decision 6's repair-armed heal, but `publishHealedScriptAtomically`
816+
(`src/daemon/session-script-writer.ts`) is the single publish primitive `SessionScriptWriter.write` calls
817+
for every `--save-script` target, with no repair-armed-vs-ordinary branch — a repair-armed heal
818+
(`saveScriptBoundary` set) and an ordinary, non-repair `open --save-script`/`close --save-script`
819+
recording (`saveScriptBoundary` absent) publish through the exact same exclusive `linkSync`. Before this
820+
decision, ordinary recording published via a separate atomic rename-replace path
821+
(`publishOverwriteAtomically`, since removed), silently overwriting an existing target; that overwrite
822+
path is gone. An ordinary recording whose target already exists is now refused exactly like a healed
823+
repair publish would be: `EEXIST` surfaces as the same `AppError`, the existing file is left
824+
byte-for-byte unchanged, and the caller must remove it or choose a different `--save-script=<path>`.
825+
There is no `--force`/`--overwrite` escape hatch for either path today; that is tracked as a future
826+
follow-up (#1258), not part of this decision.
807827

808828
**Repair-session tombstone (R7 ownership).** When a bounded-expiry escape hatch idle-reaps (or a daemon
809829
shutdown tears down) a repair-armed transaction that has **not** reached `COMPLETE`, the teardown aborts
@@ -924,8 +944,10 @@ each states its dependencies explicitly.
924944
regression proving the session is NOT deleted when the terminal `close` is reached; the
925945
`REPAIR_SESSION_EXPIRED` tombstone for an incomplete-transaction reap/shutdown (keyed by session key,
926946
owner + bounded expiry, cleared by a fresh `replay --save-script`); and race-safe atomic publication
927-
(temp file in the target's own directory, create-exclusive/rename-if-absent no-clobber against the
928-
`# agent-device:heal-complete` sentinel).
947+
(temp file in the target's own directory, published via a single exclusive `linkSync` that refuses ANY
948+
pre-existing target — complete or partial, default or explicit path alike, and uniformly for an
949+
ordinary non-repair recording's target too, since the publish primitive is shared — never overwriting
950+
one; see "Scope" under Decision 6 above).
929951

930952
## Migration progress
931953

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
/**
2+
* ADR 0012 decision 6, R7 (C5a): when a repair session was reaped before it was
3+
* finalized, the request router rewrites the resulting `SESSION_NOT_FOUND` into
4+
* a `REPAIR_SESSION_EXPIRED` recovery error with actionable re-run guidance,
5+
* rather than leaking a bare SESSION_NOT_FOUND. Any other error, or the absence
6+
* of a live tombstone, passes through unchanged.
7+
*/
8+
import { test, expect, vi } from 'vitest';
9+
import fs from 'node:fs';
10+
import os from 'node:os';
11+
import path from 'node:path';
12+
import { getResolveTargetDeviceMock } from './request-router-dispatch-mocks.ts';
13+
14+
vi.mock('../device-ready.ts', () => ({ ensureDeviceReady: vi.fn(async () => {}) }));
15+
16+
import { createRequestHandler } from '../request-router.ts';
17+
import type { DaemonRequest, SessionState } from '../types.ts';
18+
import type { DeviceInfo } from '../../kernel/device.ts';
19+
import { LeaseRegistry } from '../lease-registry.ts';
20+
import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts';
21+
import { parseReplayInput } from '../../compat/replay-input.ts';
22+
import { computeReplayPlanDigest } from '../../replay/plan-digest.ts';
23+
import { readEffectiveReplayPlanDigestMetadata } from '../handlers/session-replay-runtime-plan.ts';
24+
25+
const mockResolveTargetDevice = vi.mocked(getResolveTargetDeviceMock());
26+
27+
function makeHandler(prefix: string) {
28+
const sessionStore = makeSessionStore(prefix);
29+
const handler = createRequestHandler({
30+
logPath: path.join(os.tmpdir(), 'daemon.log'),
31+
token: 'test-token',
32+
sessionStore,
33+
leaseRegistry: new LeaseRegistry(),
34+
trackDownloadableArtifact: () => 'artifact-id',
35+
});
36+
return { sessionStore, handler };
37+
}
38+
39+
function tombstonedSession(name: string): SessionState {
40+
return {
41+
name,
42+
device: { platform: 'apple', id: 'sim-1', name: 'iPhone', kind: 'simulator', booted: true },
43+
createdAt: Date.now(),
44+
actions: [],
45+
saveScriptBoundary: 0,
46+
repairSourcePath: '/flows/login.ad',
47+
};
48+
}
49+
50+
function closeRequest(session: string): DaemonRequest {
51+
return { token: 'test-token', session, command: 'close', positionals: [], flags: {} };
52+
}
53+
54+
test('a command that finds no session but hits a live repair tombstone gets REPAIR_SESSION_EXPIRED', async () => {
55+
const { sessionStore, handler } = makeHandler('agent-device-router-repair-expired-');
56+
// The repair session was reaped (idle-reap) leaving a tombstone; the store
57+
// has no live session by that name.
58+
sessionStore.writeRepairTombstone(tombstonedSession('repair-x'));
59+
60+
const response = await handler(closeRequest('repair-x'));
61+
62+
expect(response.ok).toBe(false);
63+
if (response.ok) return;
64+
expect(response.error.code).toBe('REPAIR_SESSION_EXPIRED');
65+
// Re-run guidance carries the original script path from the tombstone.
66+
expect(response.error.message).toMatch(/replay \/flows\/login\.ad --save-script/);
67+
});
68+
69+
test('without a tombstone, a missing session still returns a plain SESSION_NOT_FOUND', async () => {
70+
const { handler } = makeHandler('agent-device-router-no-tombstone-');
71+
const response = await handler(closeRequest('never-existed'));
72+
73+
expect(response.ok).toBe(false);
74+
if (response.ok) return;
75+
expect(response.error.code).toBe('SESSION_NOT_FOUND');
76+
});
77+
78+
// ADR 0012 decision 6 (BLOCKER 2): a tombstone left by a COMPLETE transaction
79+
// whose commit FAILED at teardown must surface a distinct, actionable
80+
// REPAIR_COMMIT_FAILED — never the generic "reaped before it was finalized"
81+
// REPAIR_SESSION_EXPIRED, which would misleadingly suggest the transaction
82+
// never completed at all.
83+
test('a command hitting a commit-failure tombstone gets REPAIR_COMMIT_FAILED with the real cause, not a generic REPAIR_SESSION_EXPIRED', async () => {
84+
const { sessionStore, handler } = makeHandler('agent-device-router-commit-failed-');
85+
sessionStore.writeRepairTombstone(tombstonedSession('repair-commit-fail'), undefined, {
86+
code: 'COMMAND_FAILED',
87+
message: 'A prior healed script already exists at /flows/login.healed.ad; ...',
88+
});
89+
90+
const response = await handler(closeRequest('repair-commit-fail'));
91+
92+
expect(response.ok).toBe(false);
93+
if (response.ok) return;
94+
expect(response.error.code).toBe('REPAIR_COMMIT_FAILED');
95+
expect(response.error.code).not.toBe('REPAIR_SESSION_EXPIRED');
96+
expect(response.error.message).toMatch(/already exists/);
97+
// Still carries the actionable re-run guidance.
98+
expect(response.error.message).toMatch(/replay \/flows\/login\.ad --save-script/);
99+
});
100+
101+
test('an expired tombstone does not shadow a missing session', async () => {
102+
const { sessionStore, handler } = makeHandler('agent-device-router-expired-tombstone-');
103+
// TTL 0 => already stale.
104+
sessionStore.writeRepairTombstone(tombstonedSession('repair-y'), 0);
105+
106+
const response = await handler(closeRequest('repair-y'));
107+
108+
expect(response.ok).toBe(false);
109+
if (response.ok) return;
110+
expect(response.error.code).toBe('SESSION_NOT_FOUND');
111+
});
112+
113+
// ADR 0012 decision 6, R7 (BLOCKER 1): the tombstone translation also covers the
114+
// `replay --from` continuation path — a reaped repair session's continuation
115+
// gets REPAIR_SESSION_EXPIRED, not a REPLAY_DIVERGENCE that slipped past it.
116+
test('a replay --from continuation on a reaped repair session gets REPAIR_SESSION_EXPIRED (not REPLAY_DIVERGENCE)', async () => {
117+
// Device resolution/readiness is mocked so the router reaches the replay
118+
// handler's missing-session preflight fast and deterministically.
119+
const iosDevice: DeviceInfo = {
120+
platform: 'apple',
121+
id: 'sim-1',
122+
name: 'iPhone',
123+
kind: 'simulator',
124+
booted: true,
125+
};
126+
mockResolveTargetDevice.mockResolvedValue(iosDevice);
127+
const { sessionStore, handler } = makeHandler('agent-device-router-from-expired-');
128+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-router-from-script-'));
129+
const scriptPath = path.join(root, 'flow.ad');
130+
fs.writeFileSync(scriptPath, 'open "Demo"\nclick id="a"\n');
131+
132+
// Compute the plan digest exactly as runReplayScriptFile does (a real agent
133+
// takes it from the divergence report's resume.planDigest).
134+
const flags = { platform: 'ios' as const };
135+
const parsed = parseReplayInput(fs.readFileSync(scriptPath, 'utf8'), flags, {
136+
sourcePath: scriptPath,
137+
});
138+
const digest = computeReplayPlanDigest({
139+
actions: parsed.actions,
140+
actionLines: parsed.actionLines,
141+
actionSourcePaths: parsed.actionSourcePaths,
142+
metadata: readEffectiveReplayPlanDigestMetadata(flags),
143+
});
144+
145+
// The repair session was reaped, leaving a tombstone; no live session exists.
146+
sessionStore.writeRepairTombstone(tombstonedSession('repair-from'));
147+
148+
const response = await handler({
149+
token: 'test-token',
150+
session: 'repair-from',
151+
command: 'replay',
152+
positionals: [scriptPath],
153+
flags: { ...flags, replayFrom: 2, replayPlanDigest: digest },
154+
});
155+
156+
expect(response.ok).toBe(false);
157+
if (response.ok) return;
158+
expect(response.error.code).toBe('REPAIR_SESSION_EXPIRED');
159+
expect(response.error.code).not.toBe('REPLAY_DIVERGENCE');
160+
expect(response.error.message).toMatch(/replay \/flows\/login\.ad --save-script/);
161+
162+
fs.rmSync(root, { recursive: true, force: true });
163+
});

‎src/daemon/__tests__/request-router-typed-error.test.ts‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,3 +122,48 @@ test('deterministic errors (INVALID_ARGS) are returned with the default shape
122122
expect('supportedOn' in response.error).toBe(false);
123123
expect(mockDispatch).not.toHaveBeenCalled();
124124
});
125+
126+
// ADR 0012 decision 6, BLOCKER 2 (second follow-up): a repair-armed `close`
127+
// whose targeted platform close fails must surface `retriable: true` at the
128+
// TOP level of the wire error — the location `enrichDaemonError` below and
129+
// the client actually read (`DaemonError.retriable` in kernel/contracts.ts) —
130+
// and must preserve the underlying platform error's own diagnosticId/logPath/
131+
// details rather than discarding them. Exercised through the REAL router
132+
// boundary (`createRequestHandler`), not just the raw response builder, so a
133+
// regression in either the handler OR `enrichDaemonError`'s own
134+
// `error.retriable ?? retriableForErrorCode(error.code)` fallback is caught.
135+
test('BLOCKER 2 (second follow-up): a repair-close platform-close failure surfaces retriable:true and diagnosticId/logPath/details at the TOP level through the router', async () => {
136+
const { sessionStore, handler } = makeHandler();
137+
const session = makeIosSession('typed-error');
138+
session.recordSession = true;
139+
session.saveScriptBoundary = 0;
140+
session.saveScriptComplete = true;
141+
session.actions = [{ ts: 1, command: 'open', positionals: ['Demo'], flags: {} }];
142+
sessionStore.set('typed-error', session);
143+
144+
// DEVICE_NOT_FOUND is not in `retriableForErrorCode`'s conservative allow
145+
// list — if the handler ever regressed to relying on that code-level
146+
// fallback instead of forcing `retriable: true` itself, this would catch it.
147+
mockDispatch.mockRejectedValueOnce(
148+
new AppError('DEVICE_NOT_FOUND', 'device vanished', {
149+
diagnosticId: 'diag-router-close-1',
150+
logPath: '/tmp/router-close-1.log',
151+
someExtra: 'x',
152+
}),
153+
);
154+
155+
// A targeted close (an explicit positional app target) is what makes the
156+
// repair-armed platform close actually dispatch instead of no-op.
157+
const response = await handler(request('close', { positionals: ['com.example.app'] }));
158+
159+
expect(response.ok).toBe(false);
160+
if (response.ok) return;
161+
expect(response.error.retriable).toBe(true);
162+
expect('retriable' in (response.error.details ?? {})).toBe(false);
163+
expect(response.error.diagnosticId).toBe('diag-router-close-1');
164+
expect(response.error.logPath).toBe('/tmp/router-close-1.log');
165+
expect(response.error.details?.someExtra).toBe('x');
166+
expect(response.error.code).toBe('DEVICE_NOT_FOUND');
167+
// The session is retained (not torn down), addressable for the retry.
168+
expect(sessionStore.get('typed-error')).toBeDefined();
169+
});

0 commit comments

Comments
 (0)