Skip to content

Commit ddc4307

Browse files
committed
test: guard the daemon-owned-process arm in CI on the live web lane
Re-review of ce9f0ee: moving that arm to a manual probe left the two failures B1 exists to prevent detectable only by hand, and a fixture test proves regexes recognize synthetic rows, not that the shipped route reaps what it spawned. Restore the arm to the shipped oracle (the three files return byte-identical to 3a5b9be) and give it a lane that can execute it: smoke-web-platform is the one CI route whose daemon owns real children — the managed agent-browser daemon and its Chrome fleet. After the normal smoke it reopens a session, stops the daemon with that session still open (the #1109 shape: an ordinary close reaps the fleet, so only an unclosed session can strand it), and requires that nothing owned outlives the browser idle window. Proven both ways locally: green in 59s, and red when the fleet is stranded (browser idle window raised past the settle budget) with the oracle naming 15 owned processes — the #1109 signature, in a lane that runs on every PR. Also: daemon-replace-exit-flush cleared `info` before the oracle ran, so a failed stop would skip the `finally` retry and remove the state dir while the daemon was still alive. Clear it only once the checkpoint passes. Refs #1781 #1431 #1882
1 parent ce9f0ee commit ddc4307

9 files changed

Lines changed: 422 additions & 361 deletions

.fallowrc.json

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,6 @@
2424
// subprocess (test/integration/daemon-replace-exit-flush.test.ts), so
2525
// dependency analysis cannot follow the runCmdSync string path to it.
2626
"test/integration/support/exit-after-flush.ts",
27-
// #1781 B1: the manual red-proof probe for daemon-owned child processes. It
28-
// is run by hand (and produced the #1109/#1324 evidence), so nothing imports
29-
// it; see its header for why that arm cannot assert from a device-free lane.
30-
"test/integration/support/daemon-owned-process-probe.ts",
3127
"src/utils/update-check-entry.ts",
3228
"examples/sdk/client-session.ts",
3329
"examples/sdk/metro-runtime.ts",

test/integration/daemon-replace-exit-flush.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,13 @@ test('daemon replace mid-command returns a structured, parseable error and exits
7878
killTimeoutMs: 1_500,
7979
expectedStartTime: info.processStartTime,
8080
});
81-
info = null;
8281
// #1781 B1: neither the SIGKILLed daemon nor its replacement may leave owned
83-
// processes or unclassified state-dir residue once both are gone.
82+
// processes or unclassified state-dir residue once both are gone. `info`
83+
// stays set until this passes: `stopProcessForTakeover` is best-effort, so a
84+
// failed stop must still reach the `finally` retry below rather than have
85+
// the state dir removed out from under a daemon that is still running.
8486
await assertNoDaemonLeaks({ stateDir, daemonPids, phase: 'after-shutdown' });
87+
info = null;
8588
} finally {
8689
if (info) {
8790
await stopProcessForTakeover(info.pid, {

test/integration/smoke-web-platform.test.ts

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import assert from 'node:assert/strict';
2-
import { mkdirSync, writeFileSync } from 'node:fs';
2+
import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs';
33
import { createServer, type Server } from 'node:http';
44
import path from 'node:path';
55
import test from 'node:test';
6+
import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts';
67
import { type CliJsonResult, formatResultDebug, runBuiltCliJson } from './cli-json.ts';
78
import { assertPngFile } from './provider-scenarios/assertions.ts';
9+
import { assertNoDaemonLeaks } from './support/daemon-leak-oracle.ts';
810

911
const TEST_NAME = 'live web platform e2e smoke';
1012
const WEB_E2E_ENABLED = process.env.AGENT_DEVICE_WEB_E2E === '1';
@@ -25,10 +27,21 @@ type WebSmokeContext = {
2527
lastSnapshot?: any;
2628
screenshotPath: string;
2729
server: Server;
30+
stateDir: string;
2831
stepHistory: StepRecord[];
2932
url: string;
3033
};
3134

35+
// #1781 B1 / #1109: this is the one lane whose daemon owns real child processes
36+
// — the managed agent-browser daemon and its Chrome fleet — so it is where the
37+
// leak oracle's owned-process arm can actually fail. #1109's acceptance is that
38+
// a stopped daemon leaves zero agent-browser processes behind within the idle
39+
// window; the lane pins AGENT_BROWSER_IDLE_TIMEOUT_MS at 30s, so the settle
40+
// window here has to outlast it. Graceful daemon shutdown does not close web
41+
// sessions (#1868), so the fleet legitimately lives until that window elapses —
42+
// which is why the checkpoint waits it out rather than asserting immediately.
43+
const WEB_LEAK_SETTLE_MS = 90_000;
44+
3245
test(
3346
TEST_NAME,
3447
{
@@ -82,6 +95,7 @@ async function createWebSmokeContext(): Promise<WebSmokeContext> {
8295
env,
8396
screenshotPath: path.join(artifactDir, 'web-smoke.png'),
8497
server: fixture.server,
98+
stateDir,
8599
stepHistory: [],
86100
url: fixture.url,
87101
};
@@ -228,6 +242,11 @@ async function cleanupWebSmoke(context: WebSmokeContext, opened: boolean): Promi
228242
errors.push(error);
229243
}
230244
}
245+
try {
246+
await assertOrphanedWebSessionLeavesNothingOwned(context);
247+
} catch (error) {
248+
errors.push(error);
249+
}
231250
try {
232251
await closeServer(context.server);
233252
} catch (error) {
@@ -241,6 +260,41 @@ async function cleanupWebSmoke(context: WebSmokeContext, opened: boolean): Promi
241260
}
242261
}
243262

263+
// #1109's acceptance, as a lane assertion: a daemon that dies while a web
264+
// session is still open must leave zero agent-browser processes behind within
265+
// the idle window. That is the leak's real shape — an ordinary `close` reaps the
266+
// fleet, so only an unclosed session can strand it — and it is the one route in
267+
// CI where the oracle's owned-process arm has real children to find.
268+
async function assertOrphanedWebSessionLeavesNothingOwned(context: WebSmokeContext): Promise<void> {
269+
await runStep(context, 'reopen for the orphan checkpoint', [
270+
'open',
271+
context.url,
272+
...context.common,
273+
]);
274+
const infoPath = path.join(context.stateDir, 'daemon.json');
275+
// Never skip silently: no daemon metadata after a live web session means the
276+
// checkpoint would certify a daemon it never observed.
277+
assert.ok(existsSync(infoPath), `expected daemon metadata at ${infoPath}`);
278+
const info = JSON.parse(readFileSync(infoPath, 'utf8')) as {
279+
pid: number;
280+
processStartTime?: string;
281+
};
282+
assert.ok(Number.isInteger(info.pid) && info.pid > 0, `expected a daemon pid in ${infoPath}`);
283+
// Stop the daemon with the session still open: the browser fleet is orphaned
284+
// exactly as in #1109, and only its idle lifecycle can still reap it.
285+
await stopProcessForTakeover(info.pid, {
286+
termTimeoutMs: 5_000,
287+
killTimeoutMs: 5_000,
288+
expectedStartTime: info.processStartTime,
289+
});
290+
await assertNoDaemonLeaks({
291+
stateDir: context.stateDir,
292+
daemonPids: [info.pid],
293+
phase: 'after-shutdown',
294+
settleMs: WEB_LEAK_SETTLE_MS,
295+
});
296+
}
297+
244298
function recordStep(
245299
context: WebSmokeContext,
246300
step: string,

test/integration/support/daemon-leak-model.test.ts

Lines changed: 84 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,23 @@ import {
66
type DaemonLeakObservation,
77
type DaemonLeakObservationBase,
88
type DaemonLeakPhaseSelection,
9+
type HostProcess,
910
type NonEmpty,
1011
type StateEntry,
1112
} from './daemon-leak-model.ts';
1213

14+
// The lanes that call the oracle run device-free daemons, so the ownership rules
15+
// would otherwise only ever see an empty process set. These fixtures are the
16+
// real `ps` shapes captured during the #1109 and #1324 red-proofs (SHAs in the
17+
// #1781 B1 PR), so a regex or ordering edit that stops catching either leak
18+
// fails here instead of silently going quiet in CI.
1319
const STATE_DIR = '/tmp/agent-device-lane-abc';
1420
const DAEMON_PID = 4340;
21+
const OBSERVER_PID = 999;
22+
23+
function proc(overrides: Partial<HostProcess> & Pick<HostProcess, 'pid'>): HostProcess {
24+
return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides };
25+
}
1526

1627
// The phase and its identity arrive together: `after-close` cannot be requested
1728
// without naming the sessions that closed, so these helpers cannot construct the
@@ -24,6 +35,8 @@ function observe(
2435
stateDir: STATE_DIR,
2536
daemonPids: [DAEMON_PID],
2637
livePids: [],
38+
processes: [],
39+
excludedPids: [OBSERVER_PID],
2740
stateEntries: [],
2841
...overrides,
2942
...selection,
@@ -44,6 +57,72 @@ function file(entryPath: string, descriptorLifecycle?: string): StateEntry {
4457
return { path: entryPath, kind: 'file', ...(descriptorLifecycle ? { descriptorLifecycle } : {}) };
4558
}
4659

60+
describe('owned-process rules', () => {
61+
// #1324: `simctl io … recordVideo` reparents to launchd (ppid 1) but keeps the
62+
// dead daemon's process group, and simctl only finalizes the mp4 on SIGINT.
63+
test('flags a recorder orphaned into the dead daemon process group', () => {
64+
const recorder = proc({
65+
pid: 52420,
66+
ppid: 1,
67+
pgid: DAEMON_PID,
68+
command: '/…/simctl io 416440AE recordVideo /tmp/out.mp4',
69+
});
70+
const snapshot = evaluateDaemonLeaks(observe({ processes: [recorder] }));
71+
72+
expect(snapshot.ownedProcesses).toEqual([
73+
expect.objectContaining({ pid: 52420, reasons: ['process-group'] }),
74+
]);
75+
expect(hasDaemonLeaks(snapshot)).toBe(true);
76+
});
77+
78+
// #1109: the agent-browser daemon setsids away from the daemon's group, so
79+
// only the inherited state-dir environment and its argv identify the fleet.
80+
test('flags an agent-browser fleet by inherited state dir, including its children', () => {
81+
const browserDaemon = proc({
82+
pid: 47515,
83+
ppid: 1,
84+
pgid: 47515,
85+
command: `${STATE_DIR}/tools/agent-browser/0.27.1/package/…/agent-browser-darwin-arm64`,
86+
env: `HOME=/tmp AGENT_DEVICE_STATE_DIR=${STATE_DIR}`,
87+
});
88+
const chrome = proc({
89+
pid: 47586,
90+
ppid: 47515,
91+
pgid: 47586,
92+
command: 'Google Chrome for Testing',
93+
});
94+
const renderer = proc({
95+
pid: 47953,
96+
ppid: 47586,
97+
pgid: 47586,
98+
command: 'Chrome Helper (Renderer)',
99+
});
100+
const snapshot = evaluateDaemonLeaks(observe({ processes: [browserDaemon, chrome, renderer] }));
101+
102+
expect(snapshot.ownedProcesses.map((owned) => owned.pid)).toEqual([47515, 47586, 47953]);
103+
expect(snapshot.ownedProcesses[0]?.reasons).toEqual(['state-dir-env', 'state-dir-argv']);
104+
// The fleet below the matched root is owned transitively, not by its own argv.
105+
expect(snapshot.ownedProcesses[1]?.reasons).toEqual(['descendant']);
106+
});
107+
108+
test('ignores foreign processes, the observer chain, and the daemons themselves', () => {
109+
const simulator = proc({ pid: 700, command: '/…/CoreSimulator … SimulatorTrampoline' });
110+
const neighbourStateDir = proc({
111+
pid: 701,
112+
command: `node --state-dir ${STATE_DIR}-other/daemon.ts`,
113+
env: `AGENT_DEVICE_STATE_DIR=${STATE_DIR}-other`,
114+
});
115+
const observer = proc({ pid: OBSERVER_PID, pgid: DAEMON_PID });
116+
const daemon = proc({ pid: DAEMON_PID, pgid: DAEMON_PID });
117+
const snapshot = evaluateDaemonLeaks(
118+
observe({ processes: [simulator, neighbourStateDir, observer, daemon] }),
119+
);
120+
121+
expect(snapshot.ownedProcesses).toEqual([]);
122+
expect(hasDaemonLeaks(snapshot)).toBe(false);
123+
});
124+
});
125+
47126
describe('surviving-daemon rule', () => {
48127
// stopProcessForTakeover is best-effort void: it returns silently on identity
49128
// mismatch, signal failure, or kill timeout, so a daemon can outlive the stop.
@@ -54,9 +133,7 @@ describe('surviving-daemon rule', () => {
54133

55134
expect(snapshot.liveDaemonPids).toEqual([DAEMON_PID]);
56135
expect(hasDaemonLeaks(snapshot)).toBe(true);
57-
expect(formatDaemonLeakReport(snapshot)).toContain(
58-
`daemons that outlived shutdown: ${DAEMON_PID}`,
59-
);
136+
expect(formatDaemonLeakReport(snapshot)).toContain('daemons that outlived shutdown: 1');
60137
});
61138

62139
test('its metadata files stay stray rather than being excused by its own survival', () => {
@@ -190,7 +267,10 @@ describe('closed-session capture handles', () => {
190267

191268
test('a clean shutdown reports no leak', () => {
192269
const snapshot = evaluateDaemonLeaks(
193-
observe({ stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')] }),
270+
observe({
271+
processes: [proc({ pid: 700, command: 'unrelated' })],
272+
stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')],
273+
}),
194274
);
195275

196276
expect(hasDaemonLeaks(snapshot)).toBe(false);

0 commit comments

Comments
 (0)