Skip to content

Commit e8b779c

Browse files
thymikeethymikee
authored
fix(daemon): keep close-time script-save failures from leaking the session/device claim (#1392)
* fix(daemon): keep close-time script-save failures from leaking the session/device claim A close-time script write (implicit from `open --save-script`, or this close's own `--save-script`) that refuses to publish (e.g. a no-clobber target-exists AppError) threw uncaught out of `handleCloseCommand`, skipping lease release, device-claim release, and `sessionStore.delete` entirely — while the `close` action had already been recorded with no rollback. Live-repro'd over the real CLI against an Android emulator: this single gap explained both symptoms split out of #1384 into #1391 — a lingering `DEVICE_IN_USE` claim after a failed `close`, and a published `.ad` rewritten with duplicated trailing `close` lines when the same close was retried (each attempt re-recorded a `close` action on top of the one never rolled back from the prior failure). Catch the write failure, roll back the just-recorded `close` action (mirroring the existing repair-armed commit-failure pattern), and let teardown (lease release, device-claim clear, session delete) complete regardless — exactly as an ordinary platform-close failure already doesn't block them. The failure is still surfaced to the caller, but after teardown, with a corrected hint: retrying the same close is no longer meaningful since the session is now gone. Fixes #1391 * refactor(daemon): shrink handleCloseCommand/runSessionCloseTeardown under fallow's complexity gate CI's fallow code-quality check flagged handleCloseCommand (126 lines, 19 cyclomatic / 16 cognitive) and runSessionCloseTeardown (73 lines) as exceeding the large-function/high-complexity thresholds after the prior commit's fix. Extract runCloseTeardownAndRelease (teardown + lease release + claim clear + delete + ordered error surfacing) and buildCloseSuccessResponse (final response shaping) out of handleCloseCommand, and finalizeOrdinaryCloseScript out of runSessionCloseTeardown. No behavior change — same control flow, split into named, independently-readable steps; fallow now reports 0 complexity findings for this diff. * fix(daemon): preserve the write error's structured details in the close-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path. * refactor(daemon): drop the vestigial close-time rollback, add router-level #1391 coverage Review feedback on #1392 (thymikee), P2 items: - The close-time save-script failure's session.actions rollback (finalizeOrdinaryCloseScript) was left over from an earlier design where a failed save could keep the session alive for retry. It never does now — runCloseTeardownAndRelease always tears the session down regardless of the outcome — so there is no surviving session for a later write to duplicate the close action on. Drop the rollback; the durable events.ndjson entry (which the rollback never touched anyway) and the in-memory action now agree, both accurately recording that the close happened. - Add a request-router-level regression (request-router-typed-error.test.ts, alongside the existing repair-close BLOCKER 2 test it mirrors) proving the normalized JSON error shape a real client sees: top-level retriable:false, details.reason/path preserved, and the session torn down — not just that handleCloseCommand throws the right AppError when called directly. * test(daemon): assert the durable close event survives a failed close-time save Review feedback on #1392 (thymikee), final P2 item: the previous commit removed the actions rollback because there's no surviving session to duplicate the close action on, but nothing actually asserted the durable events.ndjson action.recorded:close event stays put. Flush and read it back so a future rollback or event-order change can't silently recreate the in-memory/durable mismatch the removed rollback used to paper over asymmetrically. * test(daemon): assert the retained session's in-memory close action, not just the durable event Review feedback on #1392 (thymikee): the durable-event assertion alone doesn't catch a reintroduced session.actions.length = actionsBeforeClose rollback, because that event is queued (and durable) before the write even attempts — a regression there would leave the assertion passing while silently reintroducing the in-memory/durable mismatch. Retain the session object past handleCloseCommand (store.delete only drops the map entry, not the object a local variable still points at) and assert its actions array contains exactly one close entry, matching the durable event count. Verified by temporarily reintroducing the old rollback locally: this assertion fails (0 !== 1) where the prior durable-only check did not, then reverted. * refactor(daemon): model repair close retry as receipt * refactor(daemon): merge blockingError to state, not explain, the save-script exclusion Following up on the comment-trimming pass already on this branch: the device-claim condition (!platformCloseError && !cleanupAggregate) and the two-line throw sequence right below it both needed a paragraph explaining why saveScriptError is excluded from one but not the other. Merge platformCloseError and cleanupAggregate into a single named blockingError — its name now states the exclusion the comment used to argue for, and the throw sequence collapses from two ifs to one. Trimmed the remaining long docblocks in this file the same way: state what's non-obvious in 1-3 lines instead of re-deriving it in prose. * refactor(daemon): clarify close script finalization
1 parent a798976 commit e8b779c

10 files changed

Lines changed: 474 additions & 279 deletions

‎scripts/help-conformance-cases.mjs‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,41 @@ export const CASES = [
155155
{ id: 'noRawAdbKeyevent', pattern: /\badb\s+shell\s+input\b/i },
156156
],
157157
},
158+
{
159+
id: 'ios-system-ui-widget-flow',
160+
docs: ['--help:first30', 'ios-system-ui'],
161+
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.',
162+
expectations: ['validPlanCommands', 'fullPrefix', 'usesSnapshotI', 'opensAndCloses'],
163+
matchers: [
164+
{
165+
id: 'bindsSessionToSpringBoard',
166+
pattern: /\bagent-device\s+open\s+com\.apple\.springboard\b[^\n]*--platform\s+ios\b/i,
167+
},
168+
{
169+
id: 'entersEditModeWithCoordinateLongpress',
170+
pattern: /\bagent-device\s+longpress\s+-?\d+(?:\.\d+)?\s+-?\d+(?:\.\d+)?\b/i,
171+
},
172+
{
173+
id: 'refreshesSnapshotAfterEnteringEditMode',
174+
pattern: /\blongpress\b[\s\S]*\n[^\n]*\bsnapshot\s+-i\b/i,
175+
},
176+
{
177+
id: 'usesScreenshotForSparseGalleryResult',
178+
pattern:
179+
/\bagent-device\s+screenshot\b[\s\S]*\n[^\n]*\bagent-device\s+press\s+-?\d+(?:\.\d+)?\s+-?\d+(?:\.\d+)?\b/i,
180+
},
181+
{
182+
id: 'returnsToAppUnderTest',
183+
pattern: /\bagent-device\s+open\s+com\.example\.calendar\b[^\n]*--platform\s+ios\b/i,
184+
},
185+
],
186+
forbidden: [
187+
{
188+
id: 'noRawSimulatorControl',
189+
pattern: /(?:^|\n)(?:xcrun\s+simctl|idb|maestro)\b/i,
190+
},
191+
],
192+
},
158193
{
159194
id: 'web-managed-backend-loop',
160195
docs: ['--help:first30', 'web'],

‎scripts/layering/session-state.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ export const SESSION_STATE_FIELD_OWNERS: Readonly<Record<string, readonly string
5757
'src/daemon/handlers/session-script-publication.ts',
5858
],
5959
recordSession: [
60-
'src/daemon/handlers/session-close.ts',
60+
'src/daemon/handlers/session-close-script.ts',
6161
'src/daemon/handlers/session-open.ts',
6262
'src/daemon/handlers/session-replay-runtime.ts',
6363
'src/daemon/handlers/session-script-publication.ts',
@@ -81,8 +81,7 @@ export const SESSION_STATE_FIELD_OWNERS: Readonly<Record<string, readonly string
8181
saveScriptCommitted: ['src/daemon/session-script-writer.ts'],
8282
repairSourcePath: ['src/daemon/handlers/session-replay-runtime.ts'],
8383
pendingRecordAndHeal: ['src/daemon/handlers/session-replay-resume.ts'],
84-
repairPlatformCloseSucceeded: ['src/daemon/handlers/session-close.ts'],
85-
repairPlatformCloseIdentity: ['src/daemon/handlers/session-close.ts'],
84+
repairPlatformCloseReceipt: ['src/daemon/handlers/session-close.ts'],
8685

8786
trace: ['src/daemon/handlers/record-trace.ts'],
8887
recording: ['src/daemon/handlers/record-trace-recording.ts'],

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { test, expect, vi, beforeEach } from 'vitest';
2+
import fs from 'node:fs';
23
import os from 'node:os';
34
import path from 'node:path';
45

@@ -167,3 +168,40 @@ test('BLOCKER 2 (second follow-up): a repair-close platform-close failure surfac
167168
// The session is retained (not torn down), addressable for the retry.
168169
expect(sessionStore.get('typed-error')).toBeDefined();
169170
});
171+
172+
// Unlike the handler-level test in session-device-claims.test.ts, this goes
173+
// through the real router boundary, so it also covers normalizeError's
174+
// details.retriable hoisting.
175+
test('#1391: an ordinary close-time script-save failure surfaces details.reason/path and retriable:false through the router, and the session is torn down', async () => {
176+
const { sessionStore, handler } = makeHandler();
177+
const session = makeIosSession('typed-error');
178+
session.recordSession = true;
179+
const targetPath = path.join(
180+
os.tmpdir(),
181+
`agent-device-router-typed-error-${Date.now()}-${Math.random().toString(36).slice(2)}.ad`,
182+
);
183+
fs.writeFileSync(targetPath, 'pre-existing\n');
184+
session.saveScriptPath = targetPath;
185+
sessionStore.set('typed-error', session);
186+
187+
try {
188+
// Untargeted close: no positionals, so no platform close is dispatched
189+
// (`shouldDispatchPlatformClose`) — isolates the script-save failure from
190+
// any platform-close error, matching BLOCKER 2's own targeted-vs-untargeted
191+
// distinction above.
192+
const response = await handler(request('close'));
193+
194+
expect(response.ok).toBe(false);
195+
if (response.ok) return;
196+
expect(response.error.code).toBe('COMMAND_FAILED');
197+
expect(response.error.retriable).toBe(false);
198+
expect(response.error.details?.reason).toBe('script_target_exists');
199+
expect(response.error.details?.path).toBe(targetPath);
200+
// Unlike the repair-armed case above, an ordinary session's teardown
201+
// never withholds on a failed script save — it is always torn down.
202+
expect(sessionStore.get('typed-error')).toBeUndefined();
203+
expect(mockDispatch).not.toHaveBeenCalled();
204+
} finally {
205+
fs.rmSync(targetPath, { force: true });
206+
}
207+
});

‎src/daemon/handlers/__tests__/session-close-script.test.ts‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
import fs from 'node:fs';
2+
import os from 'node:os';
3+
import path from 'node:path';
4+
import { afterEach, expect, test, vi } from 'vitest';
5+
import { AppError } from '../../../kernel/errors.ts';
6+
import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts';
7+
import { SessionStore } from '../../session-store.ts';
8+
import type { DaemonRequest } from '../../types.ts';
9+
import {
10+
buildRetriableRepairCloseFailureResponse,
11+
commitRepairScriptBeforeClose,
12+
finalizeOrdinaryCloseScript,
13+
} from '../session-close-script.ts';
14+
15+
const roots: string[] = [];
16+
17+
afterEach(() => {
18+
vi.restoreAllMocks();
19+
for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true });
20+
});
21+
22+
function setup(name: string) {
23+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-session-close-script-'));
24+
roots.push(root);
25+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
26+
const session = makeIosSession(name, { appBundleId: 'com.example.app' });
27+
sessionStore.set(name, session);
28+
const req: DaemonRequest = {
29+
token: 'token',
30+
session: name,
31+
command: 'close',
32+
positionals: [],
33+
flags: {},
34+
};
35+
return { req, session, sessionStore };
36+
}
37+
38+
test('failed repair publication removes only its synthetic close before retry', () => {
39+
const { req, session, sessionStore } = setup('repair');
40+
session.saveScriptBoundary = 0;
41+
session.saveScriptComplete = true;
42+
session.recordSession = true;
43+
const failure = new AppError('COMMAND_FAILED', 'publish failed');
44+
vi.spyOn(sessionStore, 'writeSessionLog').mockReturnValue({ written: false, error: failure });
45+
46+
expect(commitRepairScriptBeforeClose(sessionStore, session, req)).toEqual({
47+
kind: 'failed',
48+
error: failure,
49+
});
50+
expect(session.actions).toEqual([]);
51+
});
52+
53+
test('repair close failure keeps normalized metadata and is explicitly retriable', () => {
54+
const { session } = setup('repair-error');
55+
session.saveScriptPath = '/tmp/repaired.ad';
56+
const failure = new AppError('COMMAND_FAILED', 'publish failed', {
57+
reason: 'target-exists',
58+
hint: 'Choose another path.',
59+
logPath: '/tmp/daemon.log',
60+
});
61+
62+
expect(buildRetriableRepairCloseFailureResponse(session, failure)).toEqual({
63+
ok: false,
64+
error: {
65+
code: 'COMMAND_FAILED',
66+
message: 'publish failed',
67+
hint: 'Choose another path.',
68+
logPath: '/tmp/daemon.log',
69+
details: {
70+
reason: 'target-exists',
71+
session: 'repair-error',
72+
savedScript: '/tmp/repaired.ad',
73+
},
74+
retriable: true,
75+
},
76+
});
77+
});
78+
79+
test('ordinary publication failure retains its close action after making the error non-retriable', () => {
80+
const { req, session, sessionStore } = setup('ordinary');
81+
const failure = new AppError('COMMAND_FAILED', 'target exists', {
82+
reason: 'target-exists',
83+
hint: 'Retry close.',
84+
});
85+
vi.spyOn(sessionStore, 'writeSessionLog').mockImplementation(() => {
86+
throw failure;
87+
});
88+
89+
const result = finalizeOrdinaryCloseScript({
90+
req: { ...req, flags: { saveScript: true } },
91+
session,
92+
sessionStore,
93+
platformCloseError: undefined,
94+
});
95+
96+
expect(session.actions.map(({ command }) => command)).toEqual(['close']);
97+
expect(result).toMatchObject({
98+
code: 'COMMAND_FAILED',
99+
message: 'The session was closed, but its script was not saved: target exists',
100+
details: {
101+
reason: 'target-exists',
102+
retriable: false,
103+
},
104+
});
105+
});

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ import { SessionStore } from '../../session-store.ts';
3535
import { handleCloseCommand } from '../session-close.ts';
3636
import { handleOpenCommand } from '../session-open.ts';
3737
import type { DeviceInfo } from '../../../kernel/device.ts';
38+
import type { SessionState } from '../../types.ts';
39+
import { AppError } from '../../../kernel/errors.ts';
3840

3941
const mockDispatch = vi.mocked(dispatchCommand);
4042
const mockResolveTargetDevice = vi.mocked(resolveTargetDevice);
@@ -245,3 +247,80 @@ test('local close clears its matching advisory claim after teardown', async () =
245247
assert.equal(response.ok, true);
246248
assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []);
247249
});
250+
251+
test('#1391: a close-time script save failure still clears the advisory claim and deletes the session', async () => {
252+
const { store, stateDir } = setup();
253+
const acquired = await acquireAdvisoryDeviceClaim({
254+
device: android,
255+
session: 'close-save-script-failure',
256+
workspace: process.cwd(),
257+
stateDir,
258+
});
259+
assert.ok(acquired.ownership);
260+
const targetPath = path.join(stateDir, 'already-published.ad');
261+
fs.writeFileSync(targetPath, 'pre-existing\n');
262+
// Retained directly (not just looked up via `store`) so it stays inspectable
263+
// after `handleCloseCommand` deletes it from the store — `store.delete` only
264+
// drops the map entry, it doesn't touch the object this variable still
265+
// points at.
266+
const session: SessionState = {
267+
name: 'close-save-script-failure',
268+
device: android,
269+
deviceClaim: acquired.ownership,
270+
createdAt: Date.now(),
271+
actions: [],
272+
recordSession: true,
273+
saveScriptPath: targetPath,
274+
};
275+
store.set('close-save-script-failure', session);
276+
mockDispatch.mockResolvedValue({});
277+
278+
// Like the platform-close-error tests above, a failed close-time save is
279+
// thrown (not returned as `{ok:false}`) — `handleCloseCommand` never
280+
// converts its own errors; the request-router does that for real requests.
281+
let thrown: unknown;
282+
try {
283+
await handleCloseCommand({
284+
req: {
285+
command: 'close',
286+
token: 'test',
287+
session: 'close-save-script-failure',
288+
positionals: [],
289+
flags: {},
290+
},
291+
sessionName: 'close-save-script-failure',
292+
logPath: path.join(stateDir, 'daemon.log'),
293+
sessionStore: store,
294+
leaseRegistry: new LeaseRegistry(),
295+
});
296+
} catch (error) {
297+
thrown = error;
298+
}
299+
300+
// The failed save is surfaced, but never at the cost of leaking the session
301+
// or the device claim (#1391 item 1) — and the target is left untouched
302+
// rather than rewritten (#1391 item 2).
303+
assert.ok(thrown instanceof AppError, 'expected close to throw an AppError');
304+
const error = thrown as AppError;
305+
assert.equal(error.code, 'COMMAND_FAILED');
306+
assert.match(error.message, /session was closed, but its script was not saved/);
307+
assert.equal(error.details?.retriable, false);
308+
// The original write failure's machine-readable details survive the
309+
// close-specific hint/retriable override, so a caller dispatching on them
310+
// (e.g. `reason === 'script_target_exists'`) still can.
311+
assert.equal(error.details?.reason, 'script_target_exists');
312+
assert.equal(error.details?.path, targetPath);
313+
assert.equal(store.get('close-save-script-failure'), undefined);
314+
assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []);
315+
assert.equal(fs.readFileSync(targetPath, 'utf8'), 'pre-existing\n');
316+
317+
assert.deepEqual(
318+
session.actions.map((action) => action.command),
319+
['close'],
320+
);
321+
await store.flushEvents('close-save-script-failure');
322+
const durableCloseEvents = store
323+
.readEvents('close-save-script-failure')
324+
.events.filter((event) => event.kind === 'action.recorded' && event.command === 'close');
325+
assert.equal(durableCloseEvents.length, session.actions.length);
326+
});

‎src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -984,7 +984,7 @@ test('BLOCKER 3 (second follow-up): a retry after a SUCCESSFUL platform close bu
984984
if (!closeResponse.ok) expect(closeResponse.error.message).toMatch(/already exists/);
985985
expect(sessionStore.get(sessionName)).toBeDefined();
986986
expect(fs.readFileSync(healedPath, 'utf8')).toBe(before);
987-
expect(sessionStore.get(sessionName)!.repairPlatformCloseSucceeded).toBe(true);
987+
expect(sessionStore.get(sessionName)!.repairPlatformCloseReceipt).toBeDefined();
988988

989989
// Retry with an explicit path: the ALREADY-SUCCEEDED platform close must
990990
// 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
10431043
expect(fs.readFileSync(healedPath, 'utf8')).toBe(committed);
10441044
});
10451045

1046-
// --- ADR 0012 decision 6 (BLOCKER 3, third follow-up): `repairPlatformCloseSucceeded`
1047-
// was session-wide, not bound to WHICH close request actually succeeded. An
1048-
// untargeted close performs NO platform operation (`shouldDispatchPlatformClose`
1049-
// is false with no positional target), yet the prior implementation still set
1050-
// the marker as though a real close succeeded; a retry with a DIFFERENT
1051-
// identity (a target added, or a different target) then wrongly skipped the
1052-
// platform close entirely, committing as though it had run. ---
1053-
10541046
test('BLOCKER 3 (third follow-up): an untargeted close that performed NO platform operation never lets a targeted retry skip the platform close', async () => {
10551047
const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup(
10561048
'agent-device-repair-transaction-close-identity-untargeted-',
@@ -1078,9 +1070,6 @@ test('BLOCKER 3 (third follow-up): an untargeted close that performed NO platfor
10781070
expect(mockDispatchCommand).not.toHaveBeenCalled();
10791071
expect(sessionStore.get(sessionName)).toBeDefined();
10801072

1081-
// Retry WITH a target: the prior session-wide `repairPlatformCloseSucceeded`
1082-
// flag (set true even though nothing dispatched for the untargeted attempt)
1083-
// would wrongly skip the platform close here. It must actually run.
10841073
const retry = await handleCloseCommand({
10851074
req: {
10861075
token: 't',
@@ -1126,7 +1115,7 @@ test('BLOCKER 3 (third follow-up): a retry targeting a DIFFERENT app than the su
11261115
});
11271116
expect(first.ok).toBe(false);
11281117
expect(mockDispatchCommand).toHaveBeenCalledTimes(1);
1129-
expect(sessionStore.get(sessionName)!.repairPlatformCloseSucceeded).toBe(true);
1118+
expect(sessionStore.get(sessionName)!.repairPlatformCloseReceipt).toBeDefined();
11301119

11311120
// Retry targets a DIFFERENT app (app-b) — a genuinely different platform
11321121
// operation. The prior session-wide marker would wrongly treat app-b as

0 commit comments

Comments
 (0)