Skip to content

Commit 5ccd980

Browse files
thymikeethymikee
committed
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
1 parent 14be01b commit 5ccd980

2 files changed

Lines changed: 120 additions & 4 deletions

File tree

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ 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 { AppError } from '../../../kernel/errors.ts';
3839

3940
const mockDispatch = vi.mocked(dispatchCommand);
4041
const mockResolveTargetDevice = vi.mocked(resolveTargetDevice);
@@ -245,3 +246,60 @@ test('local close clears its matching advisory claim after teardown', async () =
245246
assert.equal(response.ok, true);
246247
assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []);
247248
});
249+
250+
test('#1391: a close-time script save failure still clears the advisory claim and deletes the session', async () => {
251+
const { store, stateDir } = setup();
252+
const acquired = await acquireAdvisoryDeviceClaim({
253+
device: android,
254+
session: 'close-save-script-failure',
255+
workspace: process.cwd(),
256+
stateDir,
257+
});
258+
assert.ok(acquired.ownership);
259+
const targetPath = path.join(stateDir, 'already-published.ad');
260+
fs.writeFileSync(targetPath, 'pre-existing\n');
261+
store.set('close-save-script-failure', {
262+
name: 'close-save-script-failure',
263+
device: android,
264+
deviceClaim: acquired.ownership,
265+
createdAt: Date.now(),
266+
actions: [],
267+
recordSession: true,
268+
saveScriptPath: targetPath,
269+
});
270+
mockDispatch.mockResolvedValue({});
271+
272+
// Like the platform-close-error tests above, a failed close-time save is
273+
// thrown (not returned as `{ok:false}`) — `handleCloseCommand` never
274+
// converts its own errors; the request-router does that for real requests.
275+
let thrown: unknown;
276+
try {
277+
await handleCloseCommand({
278+
req: {
279+
command: 'close',
280+
token: 'test',
281+
session: 'close-save-script-failure',
282+
positionals: [],
283+
flags: {},
284+
},
285+
sessionName: 'close-save-script-failure',
286+
logPath: path.join(stateDir, 'daemon.log'),
287+
sessionStore: store,
288+
leaseRegistry: new LeaseRegistry(),
289+
});
290+
} catch (error) {
291+
thrown = error;
292+
}
293+
294+
// The failed save is surfaced, but never at the cost of leaking the session
295+
// or the device claim (#1391 item 1) — and the target is left untouched
296+
// rather than rewritten (#1391 item 2).
297+
assert.ok(thrown instanceof AppError, 'expected close to throw an AppError');
298+
const error = thrown as AppError;
299+
assert.equal(error.code, 'COMMAND_FAILED');
300+
assert.match(error.message, /session was closed, but its script was not saved/);
301+
assert.equal(error.details?.retriable, false);
302+
assert.equal(store.get('close-save-script-failure'), undefined);
303+
assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []);
304+
assert.equal(fs.readFileSync(targetPath, 'utf8'), 'pre-existing\n');
305+
});

‎src/daemon/handlers/session-close.ts‎

Lines changed: 62 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,20 @@ function toRepairPlatformCloseFailure(error: unknown): AppError {
175175
});
176176
}
177177

178+
type SessionCloseTeardownResult = {
179+
platformCloseError: unknown;
180+
// #1391: an ordinary (non-repair) session's close-time script write (implicit
181+
// from `open --save-script`, or this close's own `--save-script`) can refuse
182+
// to publish (no-clobber target-exists, or any other fs AppError). Unlike the
183+
// repair-armed commit (which keeps its session alive for a `--force` retry —
184+
// ADR 0012 BLOCKER 2b), an ordinary session has no transaction to retry: its
185+
// `close` already ran (or was skipped) and its teardown below must still
186+
// release the lease/device claim and delete the session, exactly as a
187+
// `platformCloseError` does not block them either. Surfaced separately so
188+
// `handleCloseCommand` can report it AFTER teardown completes, never before.
189+
saveScriptError?: AppError;
190+
};
191+
178192
// Runs the failure-isolated resource teardown and the targeted platform close
179193
// (#1225). Returns the preserved platform-close error (if any); best-effort
180194
// cleanup failures are pushed into `cleanupFailures`. Never throws for a cleanup
@@ -192,7 +206,7 @@ async function runSessionCloseTeardown(params: {
192206
// see `handleCloseCommand`. Dispatching it again here would be redundant at
193207
// best and a double-close at worst, so it is skipped for that case only.
194208
skipPlatformClose: boolean;
195-
}): Promise<unknown> {
209+
}): Promise<SessionCloseTeardownResult> {
196210
const { req, session, sessionName, logPath, sessionStore, cleanupFailures, repairArmed } = params;
197211
const attemptCleanup = async (step: string, run: () => Promise<void>): Promise<void> => {
198212
try {
@@ -222,7 +236,9 @@ async function runSessionCloseTeardown(params: {
222236
// succeed. Only an ordinary (non-repair) session records `close` + writes its
223237
// session log here, and — per #1225 — a failed platform close is not recorded
224238
// as `Closed`.
239+
let saveScriptError: AppError | undefined;
225240
if (!repairArmed) {
241+
const actionsBeforeClose = session.actions.length;
226242
if (!platformCloseError) {
227243
recordSessionAction(sessionStore, session, req, 'close', {
228244
session: session.name,
@@ -232,12 +248,47 @@ async function runSessionCloseTeardown(params: {
232248
if (req.flags?.saveScript) {
233249
session.recordSession = true;
234250
}
235-
sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) });
251+
try {
252+
sessionStore.writeSessionLog(session, {
253+
force: resolveEffectiveSaveScriptForce(req, session),
254+
});
255+
} catch (error) {
256+
// #1391: never let a failed script publish leak the session/device claim
257+
// (item 1) or leave an unrecorded `close` action for a later successful
258+
// write to duplicate (item 2) — roll back exactly like the repair path's
259+
// `commitRepairBeforeClose` does, then let teardown continue below.
260+
session.actions.length = actionsBeforeClose;
261+
saveScriptError = toOrdinaryCloseSaveScriptFailure(error);
262+
}
236263
}
237264
await attemptCleanup('materialized_paths', () =>
238265
cleanupRetainedMaterializedPathsForSession(sessionName),
239266
);
240-
return platformCloseError;
267+
return { platformCloseError, saveScriptError };
268+
}
269+
270+
/**
271+
* #1391: normalizes an ordinary (non-repair) session's close-time script-write
272+
* failure. `SessionScriptWriter.write()` only ever throws a genuine `AppError`
273+
* here (a non-clobber refusal or another fs AppError — see
274+
* `handleSessionScriptWriteFailure`'s non-repair, non-active-publication
275+
* branch); anything else is already swallowed into a silent `{written:false}`
276+
* there. The message is corrected for THIS call site: the shared
277+
* `publishHealedScriptAtomically` wording ("retry close --save-script")
278+
* describes the repair-commit retry contract, which does not apply here — by
279+
* the time the agent sees this, `close` has already released the device and
280+
* deleted the session, so there is nothing left to retry in place.
281+
*/
282+
function toOrdinaryCloseSaveScriptFailure(error: unknown): AppError {
283+
const detail = error instanceof AppError ? error.message : normalizeError(error).message;
284+
return new AppError(
285+
'COMMAND_FAILED',
286+
`The session was closed, but its script was not saved: ${detail}`,
287+
{
288+
hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.',
289+
retriable: false,
290+
},
291+
);
241292
}
242293

243294
type CleanupRunner = (step: string, run: () => Promise<void>) => Promise<void>;
@@ -475,7 +526,7 @@ export async function handleCloseCommand(params: {
475526
// is still attempted. The provider lease is committed only after that teardown,
476527
// and a failed provider release keeps the session retryable.
477528
const cleanupFailures: SessionCleanupFailure[] = [];
478-
const platformCloseError = await runSessionCloseTeardown({
529+
const { platformCloseError, saveScriptError } = await runSessionCloseTeardown({
479530
req,
480531
session,
481532
sessionName,
@@ -498,6 +549,9 @@ export async function handleCloseCommand(params: {
498549
phase: 'session_close_cleanup_failed',
499550
failures: cleanupFailures,
500551
});
552+
// #1391: a failed script save is never a reason to keep the device claimed —
553+
// only a genuine platform-close failure (the device may still be busy) or a
554+
// resource-cleanup failure withholds it, exactly as before.
501555
if (!platformCloseError && !cleanupAggregate) {
502556
await clearAdvisoryDeviceClaim(session.deviceClaim);
503557
}
@@ -507,6 +561,10 @@ export async function handleCloseCommand(params: {
507561
// diagnostic above so per-resource failures stay visible.
508562
if (platformCloseError) throw platformCloseError;
509563
if (cleanupAggregate) throw cleanupAggregate;
564+
// #1391: surfaced last — the session already ended and the device is already
565+
// released above, unlike the two failures above it. Nothing left to retry in
566+
// place; `toOrdinaryCloseSaveScriptFailure`'s hint reflects that.
567+
if (saveScriptError) throw saveScriptError;
510568
const shutdownResult = await maybeShutdownSessionTarget({
511569
device: session.device,
512570
shutdownRequested: req.flags?.shutdown,

0 commit comments

Comments
 (0)