Skip to content

Commit bffd99a

Browse files
committed
fix(daemon): let record stop reach the durable manifest a named session addresses (#3674)
The named-session guard covered every record action, so after a daemon restart — the exact state #3674 serves — `record stop --session qa` was refused before the on-disk `screen-recording.resource.json` was read, killing the manifest recovery that `help record` and commands.md promise. That recovery is only reachable through a named session: the manifest path derives from the session address, and the lane proves the manifest's session and device before serving. Start is different: it can only be answered on a chosen device, so it keeps the pre-selection refusal. Order the guard on the resolved plan (`plan.kind === 'start'`, still before the app-scope refusal) and pin both directions: the replayed completed export survives store loss, and a start after the same loss still refuses on the session. Reverting the guard fails both start pins; keeping it stop-wide fails the replay pin. Also complete the refusal taxonomy in commands.md: the session-required family answers the same event as a reasonless SESSION_NOT_FOUND, so document that branch.
1 parent e18e053 commit bffd99a

3 files changed

Lines changed: 69 additions & 6 deletions

File tree

‎packages/daemon/src/sessions/handlers/__tests__/record-runtime-stop-recovery.test.ts‎

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1+
import { expect, test, vi } from 'vitest';
12
import fs from 'node:fs';
23
import path from 'node:path';
3-
import { expect, test } from 'vitest';
44
import { mkdtempForTestSync } from '@agent-device/test-kit';
55
import { screenRecordingResourceStore } from '@agent-device/capture-kit/screen-recording-resource-store';
66
import {
@@ -9,6 +9,65 @@ import {
99
recordingResourcePath,
1010
} from './record-runtime.fixtures.ts';
1111

12+
// The stop lane that runs WITHOUT a held session resolves its device through the selection
13+
// dispatch. The daemon-restart tests below pin that lane, so the dispatch answers with the
14+
// harness device here; the live-handle tests never reach it because their session is held.
15+
const RESTART_SELECTED_DEVICE = {
16+
platform: 'android',
17+
id: 'emulator-5554',
18+
name: 'Pixel',
19+
kind: 'emulator',
20+
} as const;
21+
vi.mock('@agent-device/device-selection/dispatch-resolve', () => ({
22+
resolveTargetDevice: vi.fn(async () => ({ ...RESTART_SELECTED_DEVICE })),
23+
}));
24+
25+
test('record stop after a daemon restart replays the completed export for the named session', async () => {
26+
// The #3674 state itself: sessions are memory-only, so a restart drops `recording`, while the
27+
// on-disk manifest survives. The caller still names the session (the only address that finds
28+
// the manifest at all), and `help record` promises the completed recording on that stop.
29+
const harness = makeRecordRuntimeHarness('record-runtime-restart-stop-');
30+
const outPath = path.join(
31+
mkdtempForTestSync('record-runtime-restart-stop-output-'),
32+
'capture.mp4',
33+
);
34+
fs.writeFileSync(outPath, 'mp4');
35+
await harness.run(['start', outPath]);
36+
await harness.run(['stop']);
37+
const ref = harness.sessionStore.lookup(harness.sessionName);
38+
if (!ref) throw new Error('Expected the stopped recording session');
39+
harness.sessionStore.retire(ref);
40+
41+
const recovered = await harness.run(['stop'], { sessionExplicit: true });
42+
43+
expect(recovered).toMatchObject({
44+
ok: true,
45+
data: { recording: 'stopped', outPath, recordingBackend: 'adb screenrecord' },
46+
});
47+
});
48+
49+
test('record start after a daemon restart still refuses the named missing session', async () => {
50+
// The sibling direction of the restart pair: start has no manifest to recover, so the
51+
// named-session refusal keeps its priority for it.
52+
const harness = makeRecordRuntimeHarness('record-runtime-restart-start-');
53+
const outPath = path.join(
54+
mkdtempForTestSync('record-runtime-restart-start-output-'),
55+
'capture.mp4',
56+
);
57+
await harness.run(['start', outPath]);
58+
const ref = harness.sessionStore.lookup(harness.sessionName);
59+
if (!ref) throw new Error('Expected the live recording session');
60+
harness.sessionStore.retire(ref);
61+
62+
const response = await harness.run(['start', outPath], { sessionExplicit: true });
63+
64+
expect(response).toMatchObject({
65+
ok: false,
66+
error: { code: 'SESSION_NOT_FOUND', details: { reason: 'named_session_not_found' } },
67+
});
68+
expect(harness.runtime.start).toHaveBeenCalledOnce();
69+
});
70+
1271
test('record stop with a live handle binds no runtime and terminalizes the durable record', async () => {
1372
const harness = makeRecordRuntimeHarness('record-runtime-live-stop-');
1473
const outPath = path.join(mkdtempForTestSync('record-runtime-live-stop-output-'), 'capture.mp4');

‎packages/daemon/src/sessions/handlers/record-runtime.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,14 @@ async function handleRecordCommandUnsafe(
9999
const { req, sessionName, sessionStore } = params;
100100
const existingRef = sessionStore.lookup(sessionName);
101101
const existingSession = existingRef?.session;
102-
// Ordered before every record-specific refusal: a named missing session answers on the
103-
// session. Only that branch pays the function-scoped import (ADR-0019 loading shape).
104-
if (!existingSession) {
102+
const { plan, scope } = resolveRecordPlan(req, existingSession);
103+
// Only the start half can be answered on a device, so only it answers on the session: a named
104+
// missing session refuses before any device is chosen, ahead of the record-specific refusals.
105+
// The stop half must reach the durable manifest first — after a daemon restart the memory-only
106+
// session is gone for exactly the callers whose manifest the named session addresses, and the
107+
// recovery lane proves the manifest's session and device before serving anything.
108+
// Only that branch pays the function-scoped import (ADR-0019 loading shape).
109+
if (!existingSession && plan.kind === 'start') {
105110
const { requireNamedSessionHoldsDevice } =
106111
await import('../../core/explicit-session-resolution.ts');
107112
requireNamedSessionHoldsDevice({
@@ -110,7 +115,6 @@ async function handleRecordCommandUnsafe(
110115
session: existingSession,
111116
});
112117
}
113-
const { plan, scope } = resolveRecordPlan(req, existingSession);
114118
if (plan.kind === 'start' && !isWholeScreenRecordingScope(scope) && !existingSession) {
115119
return missingAppSessionResponse(req);
116120
}

‎website/docs/docs/commands.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,7 @@ Read-only commands such as `get`, `snapshot`, and `wait` report `no`, and so do
485485
These refusals carry `dispatched: no` and a typed `error.details.reason`, so branch on the reason, not the message:
486486
487487
- `session_or_device_selector_required`: the request had neither an active session nor a device selector. Open a session or pass `--platform`. Commands that require a session or a device selector before they read the request's target (`boot`, `clipboard`, `push`, `apps`, and siblings) answer this even when the request named a session the daemon does not hold.
488-
- `named_session_not_found`: the request named a session this daemon does not hold, with no device selector to fall back on (or no selector it can answer without the session — `appstate` on an Apple target refuses this even with an explicit device). The daemon lost its sessions (replaced or restarted), or the name is wrong; `agent-device session list` shows what it holds.
488+
- `named_session_not_found`: the request named a session this daemon does not hold, with no device selector to fall back on (or no selector it can answer without the session — `appstate` on an Apple target refuses this even with an explicit device). The daemon lost its sessions (replaced or restarted), or the name is wrong; `agent-device session list` shows what it holds. Commands that require a session before they read anything else (interactions, `find` clicks, RN tooling) answer the same event as `SESSION_NOT_FOUND` with no reason; treat a reasonless `SESSION_NOT_FOUND` on a named session as this one.
489489
- `session_app_required`: a per-app setting was requested but the session binds no app and none was named. Run `open <app>`, or name the app on the request for a setting that takes one (`settings clear-app-state com.example.app`).
490490
- `app_not_installed`: the named app resolves to no installed app on the target. Run `apps --all` for the exact bundle id or package, or install the app, then retry.
491491
- `webdriver_route_unsupported`: on a WebDriver-backed device cloud, the driver does not implement the route; the limit is the cloud's driver, not the device. A command with a second route (`orientation` has two) tries it before failing. After an ambiguous failure on that transport, only read-only requests are resent.

0 commit comments

Comments
 (0)