From 1bf0f7af58ce66151771a8b1141ce6830124998f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 19:30:56 +0200 Subject: [PATCH 1/8] test: daemon leak oracle around the real-subprocess daemon lanes (#1781 B1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds test/integration/support/daemon-leak-oracle.ts and calls it at the end of smoke-daemon-clean, smoke-daemon-http and daemon-replace-exit-flush. After shutdown the oracle asserts that no daemon-owned process survives (ownership: PPID descendant, PGID = daemon pid, AGENT_DEVICE_STATE_DIR env, state-dir argv — never global counts) and that the isolated state dir holds only classified artifacts (no *.tmp, no daemon.json/lock without a live daemon, no open capture descriptor). Red-proofs: pre-fix #1324 (a84caa818) leaves simctl recordVideo with ppid 1 / pgid = dead daemon; pre-fix #1109 (be4bd092b) leaves the agent-browser daemon plus its Chrome fleet. Both are clean on main. Refs #1781 #1431 --- .../daemon-replace-exit-flush.test.ts | 13 + test/integration/smoke-daemon-clean.test.ts | 4 + test/integration/smoke-daemon-http.test.ts | 4 + .../integration/support/daemon-leak-oracle.ts | 364 ++++++++++++++++++ 4 files changed, 385 insertions(+) create mode 100644 test/integration/support/daemon-leak-oracle.ts diff --git a/test/integration/daemon-replace-exit-flush.test.ts b/test/integration/daemon-replace-exit-flush.test.ts index 7333068e4d..f3fc94b866 100644 --- a/test/integration/daemon-replace-exit-flush.test.ts +++ b/test/integration/daemon-replace-exit-flush.test.ts @@ -6,6 +6,7 @@ import path from 'node:path'; import { skipWhenLoopbackUnavailable } from '../../src/__tests__/test-utils/loopback.ts'; import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts'; import { isProcessAlive } from '../../src/utils/host-process.ts'; +import { assertNoDaemonLeaks } from './support/daemon-leak-oracle.ts'; import { runCliJson } from './test-helpers.ts'; // #1596: a CLI command that finds its recorded daemon unreachable replaces it @@ -28,6 +29,7 @@ test('daemon replace mid-command returns a structured, parseable error and exits const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replace-exit-flush-')); let info: DaemonInfo | null = null; + const daemonPids: number[] = []; try { // A real daemon, started by this codebase, so its recorded version/code // signature legitimately match — the only way to reach the "unreachable" @@ -36,6 +38,7 @@ test('daemon replace mid-command returns a structured, parseable error and exits assert.equal(started.status, 0, `${started.stderr}\n${started.stdout}`); info = readDaemonInfo(stateDir); + daemonPids.push(info.pid); assert.equal(isProcessAlive(info.pid), true, 'expected the started daemon to be alive'); // Kill it out from under its own metadata: daemon.json stays put and @@ -69,6 +72,16 @@ test('daemon replace mid-command returns a structured, parseable error and exits ); info = readDaemonInfo(stateDir); + daemonPids.push(info.pid); + await stopProcessForTakeover(info.pid, { + termTimeoutMs: 1_500, + killTimeoutMs: 1_500, + expectedStartTime: info.processStartTime, + }); + info = null; + // #1781 B1: neither the SIGKILLed daemon nor its replacement may leave owned + // processes or unclassified state-dir residue once both are gone. + await assertNoDaemonLeaks({ stateDir, daemonPids, phase: 'after-shutdown' }); } finally { if (info) { await stopProcessForTakeover(info.pid, { diff --git a/test/integration/smoke-daemon-clean.test.ts b/test/integration/smoke-daemon-clean.test.ts index 6da6a55a76..2d39ff29b1 100644 --- a/test/integration/smoke-daemon-clean.test.ts +++ b/test/integration/smoke-daemon-clean.test.ts @@ -7,6 +7,7 @@ import { skipWhenLoopbackUnavailable } from '../../src/__tests__/test-utils/loop import { runCmdSync } from '../../src/utils/exec.ts'; import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts'; import { isProcessAlive } from '../../src/utils/host-process.ts'; +import { assertNoDaemonLeaks } from './support/daemon-leak-oracle.ts'; import { runCliJson } from './test-helpers.ts'; type DaemonInfo = { @@ -41,6 +42,9 @@ test('clean daemon script stops a live daemon before removing metadata', async ( assert.equal(isProcessAlive(info.pid), false); assert.equal(fs.existsSync(path.join(stateDir, 'daemon.json')), false); assert.equal(fs.existsSync(path.join(stateDir, 'daemon.lock')), false); + // #1781 B1: the stopped daemon must take every process it owned with it and + // leave only classified artifacts in its state dir. + await assertNoDaemonLeaks({ stateDir, daemonPids: [info.pid], phase: 'after-shutdown' }); } finally { if (info) { await stopProcessForTakeover(info.pid, { diff --git a/test/integration/smoke-daemon-http.test.ts b/test/integration/smoke-daemon-http.test.ts index 3e03998e80..83a07009b0 100644 --- a/test/integration/smoke-daemon-http.test.ts +++ b/test/integration/smoke-daemon-http.test.ts @@ -7,6 +7,7 @@ import { DAEMON_RPC_PROTOCOL_VERSION } from '../../src/daemon/http-health.ts'; import { skipWhenLoopbackUnavailable } from '../../src/__tests__/test-utils/loopback.ts'; import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts'; import { formatResultDebug } from './cli-json.ts'; +import { assertNoDaemonLeaks } from './support/daemon-leak-oracle.ts'; import { runCliJson } from './test-helpers.ts'; type DaemonInfo = { @@ -112,6 +113,9 @@ async function stopDaemonForStateDir(stateDir: string): Promise { killTimeoutMs: 1500, expectedStartTime: info.processStartTime, }); + // #1781 B1: the HTTP-mode daemon must exit without owned processes or + // unclassified state-dir residue. + await assertNoDaemonLeaks({ stateDir, daemonPids: [info.pid], phase: 'after-shutdown' }); } finally { fs.rmSync(stateDir, { recursive: true, force: true }); } diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts new file mode 100644 index 0000000000..b22c420388 --- /dev/null +++ b/test/integration/support/daemon-leak-oracle.ts @@ -0,0 +1,364 @@ +// Daemon leak oracle (#1781 B1, feeds #1431). The real-subprocess daemon lanes +// (smoke-daemon-clean, smoke-daemon-http, daemon-replace-exit-flush) call it at +// their end to answer one question: after `close` and after daemon shutdown, +// did the daemon leave anything it OWNS behind? +// +// Ownership is explicit and conservative. Global process counts are not +// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this +// daemon does not. A host process is daemon-owned when ANY rule holds: +// +// descendant its PPID chain reaches a daemon pid (observable only while +// that daemon lives; orphans reparent to launchd/init). +// process-group its PGID equals a daemon pid. The CLI launches the daemon +// detached (setsid), so the daemon is its own group leader and +// every non-detached runCmdBackground child stays in that group +// after reparenting. This is the #1324 signature: `simctl io … +// recordVideo` with PPID 1 and PGID = the dead daemon's pid. +// POSIX keeps a pgid reserved while any member lives, so it +// cannot be recycled under the check. +// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. +// The CLI starts the daemon with that variable and children +// inherit it, including setsid'd grandchildren the pgid rule +// misses (agent-browser's own daemon → Chrome fleet: #1109). +// macOS hides the environment of Apple platform binaries +// (`ps -E` shows it for node/Chrome, not for /bin/sleep or +// simctl), so Apple tooling is caught by the pgid/argv rules. +// state-dir-argv its command line contains the state dir path (recorders +// writing session artifacts, browsers pinned to a managed home). +// +// The lanes use a fresh mkdtemp state dir per run, so env/argv rules cannot +// match a foreign process. The oracle also excludes itself and its ancestors. +// +// State-dir residue: after shutdown every entry must match +// EXPECTED_STATE_DIR_ENTRIES (unknown ⇒ classify the new artifact, do not widen +// the matcher), `*.tmp` write-then-publish temporaries always fail (torn +// publish), daemon.json/daemon.lock may exist only while a daemon lives, and a +// durable capture descriptor still `lifecycle: "open"` (or a legacy +// `app-log.pid` marker) after shutdown means a capture handle outlived its +// owner — a `completed` descriptor is the finish record ADR 0019 keeps. +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { runCmd } from '../../../src/utils/exec.ts'; +import { isProcessAlive } from '../../../src/utils/host-process.ts'; + +const PS_TIMEOUT_MS = 5_000; +const DEFAULT_SETTLE_MS = 5_000; +const SETTLE_POLL_MS = 250; +const COMMAND_PREVIEW_CHARS = 160; + +export type DaemonLeakPhase = 'after-close' | 'after-shutdown'; + +type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; + +type OwnedProcess = { + pid: number; + ppid: number; + pgid: number; + command: string; + reasons: OwnershipReason[]; +}; + +type DaemonLeakSnapshot = { + stateDir: string; + daemonPids: number[]; + phase: DaemonLeakPhase; + liveDaemonPids: number[]; + ownedProcesses: OwnedProcess[]; + strayStateEntries: string[]; +}; + +export type DaemonLeakOracleOptions = { + stateDir: string; + /** Every daemon pid the lane observed for this state dir (from daemon.json). */ + daemonPids: readonly number[]; + phase: DaemonLeakPhase; + /** How long stragglers may take to exit before they count as leaked. */ + settleMs?: number; +}; + +type HostProcess = { pid: number; ppid: number; pgid: number; command: string; env: string }; + +// Relative-path matchers for everything the daemon may legitimately leave in a +// state dir. `daemon.json`/`daemon.lock` are additionally gated on a live +// daemon; capture descriptors are gated on the phase (see classifyStateEntry). +const EXPECTED_STATE_DIR_ENTRIES: readonly RegExp[] = [ + /^daemon\.json$/, + /^daemon\.lock$/, + /^daemon\.log$/, + /^daemon-shutdown\.json$/, + /^sessions\/[^/]+\/events\.ndjson$/, + /^sessions\/[^/]+\/requests\/[^/]+\.ndjson$/, + /^sessions\/[^/]+\/(?:app|runner)\.log$/, + /^sessions\/[^/]+\/repair-tombstone\.json$/, + /^sessions\/[^/]+\/artifacts\/.+$/, + /^tools\/.+$/, + /^device-claims\/.+$/, +]; +const CAPTURE_DESCRIPTOR_ENTRY = /^sessions\/[^/]+\/[^/]+\.resource\.json$/; +const LEGACY_APP_LOG_MARKER_ENTRY = /^sessions\/[^/]+\/app-log\.pid$/; +const DAEMON_LIVENESS_ENTRY = /^daemon\.(?:json|lock)$/; + +async function captureDaemonLeakSnapshot( + options: DaemonLeakOracleOptions, +): Promise { + const stateDir = path.resolve(options.stateDir); + const daemonPids = uniquePids(options.daemonPids); + const processes = await listHostProcesses(); + const liveDaemonPids = daemonPids.filter((pid) => isProcessAlive(pid)); + return { + stateDir, + daemonPids, + phase: options.phase, + liveDaemonPids, + ownedProcesses: findOwnedProcesses(processes, daemonPids, stateDir), + strayStateEntries: listStateEntries(stateDir).filter( + (entry) => + classifyStateEntry(stateDir, entry, options.phase, liveDaemonPids.length > 0) !== + 'expected', + ), + }; +} + +function findOwnedProcesses( + processes: readonly HostProcess[], + daemonPids: readonly number[], + stateDir: string, +): OwnedProcess[] { + const excluded = new Set([...ancestorsOf(process.pid, processes), ...daemonPids]); + const reasonsByPid = new Map(); + for (const proc of processes) { + if (excluded.has(proc.pid)) continue; + const reasons = directOwnershipReasons(proc, daemonPids, stateDir); + if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); + } + // Ownership propagates down the tree: a child of the daemon, or of any + // process the rules above own, is owned (a detached `(simctl)` grandchild of + // a leaked recorder shim, DTServiceHub under a runner xcodebuild, …). + for (const pid of descendantsOf([...daemonPids, ...reasonsByPid.keys()], processes)) { + if (excluded.has(pid)) continue; + reasonsByPid.set(pid, [...(reasonsByPid.get(pid) ?? []), 'descendant']); + } + return processes.flatMap((proc) => { + const reasons = reasonsByPid.get(proc.pid); + return reasons + ? [{ pid: proc.pid, ppid: proc.ppid, pgid: proc.pgid, command: proc.command, reasons }] + : []; + }); +} + +function directOwnershipReasons( + proc: HostProcess, + daemonPids: readonly number[], + stateDir: string, +): OwnershipReason[] { + // Whole-path matches only: `` as a token or `/…`, never `-sibling`. + const stateDirToken = new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`); + const stateDirEnv = new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`); + const reasons: OwnershipReason[] = []; + if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); + if (stateDirEnv.test(proc.env)) reasons.push('state-dir-env'); + if (stateDirToken.test(proc.command)) reasons.push('state-dir-argv'); + return reasons; +} + +function hasDaemonLeaks(snapshot: DaemonLeakSnapshot): boolean { + return snapshot.ownedProcesses.length > 0 || snapshot.strayStateEntries.length > 0; +} + +/** + * Re-snapshots until clean or `settleMs` elapses. Stragglers that exit on their + * own inside the window are not leaks; #1324/#1109 orphans never exit on their own. + */ +async function settleDaemonLeakSnapshot( + options: DaemonLeakOracleOptions, +): Promise { + const deadline = Date.now() + (options.settleMs ?? DEFAULT_SETTLE_MS); + let snapshot = await captureDaemonLeakSnapshot(options); + while (hasDaemonLeaks(snapshot) && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, SETTLE_POLL_MS)); + snapshot = await captureDaemonLeakSnapshot(options); + } + return snapshot; +} + +/** Throws with the full report if anything daemon-owned survives the settle window. */ +export async function assertNoDaemonLeaks(options: DaemonLeakOracleOptions): Promise { + const snapshot = await settleDaemonLeakSnapshot(options); + if (hasDaemonLeaks(snapshot)) throw new Error(formatDaemonLeakReport(snapshot)); +} + +function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { + const lines = [ + `daemon leak oracle: ${hasDaemonLeaks(snapshot) ? 'LEAK' : 'clean'} (${snapshot.phase})`, + ` state dir: ${snapshot.stateDir}`, + ` daemon pids: ${snapshot.daemonPids.join(', ') || '(none)'}; live: ${ + snapshot.liveDaemonPids.join(', ') || '(none)' + }`, + ` owned processes still alive: ${snapshot.ownedProcesses.length}`, + ...snapshot.ownedProcesses.map((proc) => { + const command = proc.command.replace( + new RegExp(`${escapeRegExp(snapshot.stateDir)}(?=[\\s/]|$)`, 'g'), + '', + ); + const preview = + command.length > COMMAND_PREVIEW_CHARS + ? `${command.slice(0, COMMAND_PREVIEW_CHARS)}…` + : command; + return ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${preview}`; + }), + ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, + ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), + ]; + return lines.join('\n'); +} + +function classifyStateEntry( + stateDir: string, + entry: string, + phase: DaemonLeakPhase, + daemonAlive: boolean, +): 'expected' | 'stray' { + if (entry.endsWith('.tmp')) return 'stray'; + if (DAEMON_LIVENESS_ENTRY.test(entry)) return daemonAlive ? 'expected' : 'stray'; + if (CAPTURE_DESCRIPTOR_ENTRY.test(entry) || LEGACY_APP_LOG_MARKER_ENTRY.test(entry)) { + return classifyCaptureEntry(stateDir, entry, phase); + } + return EXPECTED_STATE_DIR_ENTRIES.some((matcher) => matcher.test(entry)) ? 'expected' : 'stray'; +} + +// Other sessions may still hold live captures while this one closes; after +// shutdown only a completed capture record may remain (never a pid marker). +function classifyCaptureEntry( + stateDir: string, + entry: string, + phase: DaemonLeakPhase, +): 'expected' | 'stray' { + if (phase === 'after-close') return 'expected'; + if (!CAPTURE_DESCRIPTOR_ENTRY.test(entry)) return 'stray'; + return readDescriptorLifecycle(path.join(stateDir, entry)) === 'completed' ? 'expected' : 'stray'; +} + +function readDescriptorLifecycle(filePath: string): string | undefined { + try { + const parsed = JSON.parse(fs.readFileSync(filePath, 'utf8')) as { lifecycle?: unknown }; + return typeof parsed.lifecycle === 'string' ? parsed.lifecycle : undefined; + } catch { + return undefined; + } +} + +function listStateEntries(stateDir: string): string[] { + if (!fs.existsSync(stateDir)) return []; + return fs + .readdirSync(stateDir, { recursive: true, withFileTypes: true }) + .filter((entry) => !entry.isDirectory()) + .map((entry) => + path.relative(stateDir, path.join(entry.parentPath, entry.name)).split(path.sep).join('/'), + ) + .sort(); +} + +// pid/ppid/pgid plus the command line, and separately the environment so the +// argv and env rules stay distinct. macOS `ps -E` appends the environment to +// the command line for same-user processes; Linux exposes it in /proc. +async function listHostProcesses(): Promise { + const darwin = process.platform === 'darwin'; + const [tree, darwinEnv] = await Promise.all([ + runPs(['-axww', '-o', 'pid=,ppid=,pgid=,command=']), + darwin ? runPs(['-E', '-axww', '-o', 'pid=,command=']) : Promise.resolve(''), + ]); + const envByPid = new Map( + parsePsLines(darwinEnv, /^\s*(\d+)\s+(.*)$/).map(([pid, env]) => [Number(pid), env ?? '']), + ); + return parsePsLines(tree, /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/).map( + ([pid, ppid, pgid, command]) => ({ + pid: Number(pid), + ppid: Number(ppid), + pgid: Number(pgid), + command: command ?? '', + env: darwin ? (envByPid.get(Number(pid)) ?? '') : readLinuxEnviron(Number(pid)), + }), + ); +} + +function parsePsLines(stdout: string, shape: RegExp): string[][] { + return stdout.split('\n').flatMap((line) => { + const match = shape.exec(line); + return match ? [match.slice(1)] : []; + }); +} + +async function runPs(args: string[]): Promise { + const result = await runCmd('ps', args, { allowFailure: true, timeoutMs: PS_TIMEOUT_MS }); + if (result.exitCode !== 0) { + throw new Error( + `daemon leak oracle: ps ${args.join(' ')} failed (${result.exitCode}): ${result.stderr}`, + ); + } + return result.stdout; +} + +function readLinuxEnviron(pid: number): string { + try { + return fs.readFileSync(`/proc/${pid}/environ`, 'latin1').replaceAll('\0', ' '); + } catch { + return ''; + } +} + +function descendantsOf( + rootPids: readonly number[], + processes: readonly HostProcess[], +): Set { + const selected = new Set(rootPids); + let changed = true; + while (changed) { + changed = false; + for (const proc of processes) { + if (!selected.has(proc.ppid) || selected.has(proc.pid)) continue; + selected.add(proc.pid); + changed = true; + } + } + for (const pid of rootPids) selected.delete(pid); + return selected; +} + +function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set { + const byPid = new Map(processes.map((proc) => [proc.pid, proc])); + const chain = new Set(); + for (let cursor = pid; cursor > 0 && !chain.has(cursor); cursor = byPid.get(cursor)?.ppid ?? 0) { + chain.add(cursor); + } + return chain; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +function uniquePids(pids: readonly number[]): number[] { + return [...new Set(pids)].filter((pid) => Number.isInteger(pid) && pid > 0); +} + +// Standalone use (red-proofs, manual triage): +// node --experimental-strip-types test/integration/support/daemon-leak-oracle.ts \ +// --state-dir --daemon-pid [--daemon-pid …] \ +// [--phase after-shutdown|after-close] [--settle-ms ] +// Prints the report and exits 1 on a leak. +if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { + const args = process.argv.slice(2); + const readFlag = (name: string): string[] => + args.flatMap((arg, index) => (arg === name && args[index + 1] ? [args[index + 1]!] : [])); + const stateDir = readFlag('--state-dir')[0]; + if (!stateDir) throw new Error('daemon leak oracle: --state-dir is required'); + const snapshot = await settleDaemonLeakSnapshot({ + stateDir, + daemonPids: readFlag('--daemon-pid').map(Number), + phase: (readFlag('--phase')[0] as DaemonLeakPhase | undefined) ?? 'after-shutdown', + settleMs: Number(readFlag('--settle-ms')[0] ?? DEFAULT_SETTLE_MS), + }); + process.stdout.write(`${formatDaemonLeakReport(snapshot)}\n`); + process.exitCode = hasDaemonLeaks(snapshot) ? 1 : 0; +} From a2b0ed8a620b2fe134421874deb5b86313f8da74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 08:33:14 +0200 Subject: [PATCH 2/8] test: fail the daemon leak oracle on a surviving daemon and pin its rules Review of #1859 found the oracle reported "clean" when the daemon itself outlived shutdown: daemon pids were excluded from ownership, liveDaemonPids never reached hasDaemonLeaks, and a live daemon even flipped daemon.json/lock from stray to expected. stopProcessForTakeover is best-effort void, so only smoke-daemon-clean independently asserted death. - a live daemon pid at phase 'after-shutdown' is now itself a leak, and its metadata files stay stray; a live daemon remains legitimate at 'after-close' - split the pure ownership/residue rules into daemon-leak-model.ts and pin them with daemon-leak-model.test.ts, using the real ps rows captured during the #1109 and #1324 red-proofs (the lanes' daemons own no children, so the fixture test is what guards those shapes in CI) - exempt the managed tools/ install tree before the .tmp rule, so agent-browser's own download temporaries are no longer a false LEAK - report empty directories as residue (an unswept session scaffold leaves no file) - reuse src/utils/host-process.ts (expandProcessTree, uniquePositivePids) and its /bin/ps convention instead of re-deriving them - assert in smoke-daemon-http on the success path, not in finally, so the settle window cannot replace a primary assertion's diagnostic Refs #1781 #1431 --- test/integration/smoke-daemon-http.test.ts | 35 +-- .../support/daemon-leak-model.test.ts | 213 +++++++++++++ test/integration/support/daemon-leak-model.ts | 281 +++++++++++++++++ .../integration/support/daemon-leak-oracle.ts | 291 ++++-------------- vitest.config.ts | 6 + 5 files changed, 582 insertions(+), 244 deletions(-) create mode 100644 test/integration/support/daemon-leak-model.test.ts create mode 100644 test/integration/support/daemon-leak-model.ts diff --git a/test/integration/smoke-daemon-http.test.ts b/test/integration/smoke-daemon-http.test.ts index 83a07009b0..53a7e941c6 100644 --- a/test/integration/smoke-daemon-http.test.ts +++ b/test/integration/smoke-daemon-http.test.ts @@ -66,8 +66,17 @@ test('daemon HTTP transport starts from CLI and accepts a command RPC', async (t const unauthorized = await callCommandRpc({ ...info, token: 'wrong-token' }, 'session_list'); assert.equal(unauthorized.status, 401); assert.equal(unauthorized.body.error?.data?.code, 'UNAUTHORIZED'); + // #1781 B1: the HTTP-mode daemon must exit — leaving nothing it owns and no + // unclassified state-dir residue. Asserted on the success path so the + // oracle's settle window can never replace a primary assertion's + // diagnostic; the `finally` below stays best-effort cleanup. + await stopDaemon(info); + await assertNoDaemonLeaks({ stateDir, daemonPids: [info.pid], phase: 'after-shutdown' }); } finally { - await stopDaemonForStateDir(stateDir); + if (fs.existsSync(path.join(stateDir, 'daemon.json'))) { + await stopDaemon(readDaemonInfo(stateDir)); + } + fs.rmSync(stateDir, { recursive: true, force: true }); } }); @@ -102,21 +111,11 @@ async function callCommandRpc( }; } -async function stopDaemonForStateDir(stateDir: string): Promise { - try { - const infoPath = path.join(stateDir, 'daemon.json'); - if (!fs.existsSync(infoPath)) return; - const info = readDaemonInfo(stateDir); - if (!Number.isInteger(info.pid) || info.pid <= 0) return; - await stopProcessForTakeover(info.pid, { - termTimeoutMs: 1500, - killTimeoutMs: 1500, - expectedStartTime: info.processStartTime, - }); - // #1781 B1: the HTTP-mode daemon must exit without owned processes or - // unclassified state-dir residue. - await assertNoDaemonLeaks({ stateDir, daemonPids: [info.pid], phase: 'after-shutdown' }); - } finally { - fs.rmSync(stateDir, { recursive: true, force: true }); - } +async function stopDaemon(info: DaemonInfo): Promise { + if (!Number.isInteger(info.pid) || info.pid <= 0) return; + await stopProcessForTakeover(info.pid, { + termTimeoutMs: 1500, + killTimeoutMs: 1500, + expectedStartTime: info.processStartTime, + }); } diff --git a/test/integration/support/daemon-leak-model.test.ts b/test/integration/support/daemon-leak-model.test.ts new file mode 100644 index 0000000000..e65235c409 --- /dev/null +++ b/test/integration/support/daemon-leak-model.test.ts @@ -0,0 +1,213 @@ +import { describe, expect, test } from 'vitest'; +import { + evaluateDaemonLeaks, + formatDaemonLeakReport, + hasDaemonLeaks, + type DaemonLeakObservation, + type DaemonLeakPhase, + type HostProcess, + type StateEntry, +} from './daemon-leak-model.ts'; + +// The lanes that call the oracle run device-free daemons, so the ownership rules +// would otherwise only ever see an empty process set. These fixtures are the +// real `ps` shapes captured during the #1109 and #1324 red-proofs (SHAs in the +// #1781 B1 PR), so a regex or ordering edit that stops catching either leak +// fails here instead of silently going quiet in CI. +const STATE_DIR = '/tmp/agent-device-lane-abc'; +const DAEMON_PID = 4340; +const OBSERVER_PID = 999; + +function proc(overrides: Partial & Pick): HostProcess { + return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides }; +} + +function observe(overrides: Partial = {}): DaemonLeakObservation { + return { + stateDir: STATE_DIR, + daemonPids: [DAEMON_PID], + livePids: [], + phase: 'after-shutdown', + processes: [], + excludedPids: [OBSERVER_PID], + stateEntries: [], + ...overrides, + }; +} + +function file(entryPath: string, descriptorLifecycle?: string): StateEntry { + return { path: entryPath, kind: 'file', ...(descriptorLifecycle ? { descriptorLifecycle } : {}) }; +} + +describe('owned-process rules', () => { + // #1324: `simctl io … recordVideo` reparents to launchd (ppid 1) but keeps the + // dead daemon's process group, and simctl only finalizes the mp4 on SIGINT. + test('flags a recorder orphaned into the dead daemon process group', () => { + const recorder = proc({ + pid: 52420, + ppid: 1, + pgid: DAEMON_PID, + command: '/…/simctl io 416440AE recordVideo /tmp/out.mp4', + }); + const snapshot = evaluateDaemonLeaks(observe({ processes: [recorder] })); + + expect(snapshot.ownedProcesses).toEqual([ + expect.objectContaining({ pid: 52420, reasons: ['process-group'] }), + ]); + expect(hasDaemonLeaks(snapshot)).toBe(true); + }); + + // #1109: the agent-browser daemon setsids away from the daemon's group, so + // only the inherited state-dir environment and its argv identify the fleet. + test('flags an agent-browser fleet by inherited state dir, including its children', () => { + const browserDaemon = proc({ + pid: 47515, + ppid: 1, + pgid: 47515, + command: `${STATE_DIR}/tools/agent-browser/0.27.1/package/…/agent-browser-darwin-arm64`, + env: `HOME=/tmp AGENT_DEVICE_STATE_DIR=${STATE_DIR}`, + }); + const chrome = proc({ + pid: 47586, + ppid: 47515, + pgid: 47586, + command: 'Google Chrome for Testing', + }); + const renderer = proc({ + pid: 47953, + ppid: 47586, + pgid: 47586, + command: 'Chrome Helper (Renderer)', + }); + const snapshot = evaluateDaemonLeaks(observe({ processes: [browserDaemon, chrome, renderer] })); + + expect(snapshot.ownedProcesses.map((owned) => owned.pid)).toEqual([47515, 47586, 47953]); + expect(snapshot.ownedProcesses[0]?.reasons).toEqual(['state-dir-env', 'state-dir-argv']); + // The fleet below the matched root is owned transitively, not by its own argv. + expect(snapshot.ownedProcesses[1]?.reasons).toEqual(['descendant']); + }); + + test('ignores foreign processes, the observer chain, and the daemons themselves', () => { + const simulator = proc({ pid: 700, command: '/…/CoreSimulator … SimulatorTrampoline' }); + const neighbourStateDir = proc({ + pid: 701, + command: `node --state-dir ${STATE_DIR}-other/daemon.ts`, + env: `AGENT_DEVICE_STATE_DIR=${STATE_DIR}-other`, + }); + const observer = proc({ pid: OBSERVER_PID, pgid: DAEMON_PID }); + const daemon = proc({ pid: DAEMON_PID, pgid: DAEMON_PID }); + const snapshot = evaluateDaemonLeaks( + observe({ processes: [simulator, neighbourStateDir, observer, daemon] }), + ); + + expect(snapshot.ownedProcesses).toEqual([]); + expect(hasDaemonLeaks(snapshot)).toBe(false); + }); +}); + +describe('surviving-daemon rule', () => { + // stopProcessForTakeover is best-effort void: it returns silently on identity + // mismatch, signal failure, or kill timeout, so a daemon can outlive the stop. + test('a daemon still alive after shutdown is itself a leak', () => { + const snapshot = evaluateDaemonLeaks( + observe({ livePids: [DAEMON_PID], stateEntries: [file('daemon.log')] }), + ); + + expect(snapshot.liveDaemonPids).toEqual([DAEMON_PID]); + expect(hasDaemonLeaks(snapshot)).toBe(true); + expect(formatDaemonLeakReport(snapshot)).toContain('daemons that outlived shutdown: 1'); + }); + + test('its metadata files stay stray rather than being excused by its own survival', () => { + const snapshot = evaluateDaemonLeaks( + observe({ livePids: [DAEMON_PID], stateEntries: [file('daemon.json'), file('daemon.lock')] }), + ); + + expect(snapshot.strayStateEntries).toEqual(['daemon.json', 'daemon.lock']); + }); + + test('a live daemon is expected while a session merely closed', () => { + const snapshot = evaluateDaemonLeaks( + observe({ + phase: 'after-close', + livePids: [DAEMON_PID], + stateEntries: [file('daemon.json'), file('daemon.lock')], + }), + ); + + expect(hasDaemonLeaks(snapshot)).toBe(false); + }); +}); + +describe('state-dir residue rules', () => { + test.each<[string, StateEntry, DaemonLeakPhase, 'expected' | 'stray']>([ + ['session event log', file('sessions/default/events.ndjson'), 'after-shutdown', 'expected'], + ['request diagnostics', file('sessions/d/requests/abc.ndjson'), 'after-shutdown', 'expected'], + ['shutdown report', file('daemon-shutdown.json'), 'after-shutdown', 'expected'], + ['torn publish temporary', file('device-claims/a.json.55.tmp'), 'after-shutdown', 'stray'], + [ + 'managed tool download', + file('tools/agent-browser/0.27.1/dl.tmp'), + 'after-shutdown', + 'expected', + ], + [ + 'unswept artifact scaffold', + { path: 'sessions/d/artifacts/pending/', kind: 'empty-directory' }, + 'after-shutdown', + 'stray', + ], + [ + 'unswept session scaffold', + { path: 'sessions/leaked/requests/', kind: 'empty-directory' }, + 'after-shutdown', + 'stray', + ], + ['unknown artifact', file('sessions/d/mystery.bin'), 'after-shutdown', 'stray'], + [ + 'open capture descriptor', + file('sessions/d/screen-recording.resource.json', 'open'), + 'after-shutdown', + 'stray', + ], + [ + 'completed capture descriptor', + file('sessions/d/screen-recording.resource.json', 'completed'), + 'after-shutdown', + 'expected', + ], + [ + 'open capture during close', + file('sessions/d/screen-recording.resource.json', 'open'), + 'after-close', + 'expected', + ], + ['legacy app-log marker', file('sessions/d/app-log.pid'), 'after-shutdown', 'stray'], + ])('%s is %s at %s', (_name, entry, phase, verdict) => { + const snapshot = evaluateDaemonLeaks(observe({ phase, stateEntries: [entry] })); + + expect(snapshot.strayStateEntries).toEqual(verdict === 'stray' ? [entry.path] : []); + }); + + // A managed install tree is third-party output the daemon neither writes nor + // owns, so its own temporaries must not be read as our torn publish. + test('the managed tools exemption does not leak into daemon-written paths', () => { + const snapshot = evaluateDaemonLeaks( + observe({ stateEntries: [file('sessions/d/tools/pending.tmp')] }), + ); + + expect(snapshot.strayStateEntries).toEqual(['sessions/d/tools/pending.tmp']); + }); +}); + +test('a clean shutdown reports no leak', () => { + const snapshot = evaluateDaemonLeaks( + observe({ + processes: [proc({ pid: 700, command: 'unrelated' })], + stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')], + }), + ); + + expect(hasDaemonLeaks(snapshot)).toBe(false); + expect(formatDaemonLeakReport(snapshot)).toContain('daemon leak oracle: clean (after-shutdown)'); +}); diff --git a/test/integration/support/daemon-leak-model.ts b/test/integration/support/daemon-leak-model.ts new file mode 100644 index 0000000000..9d89b939d5 --- /dev/null +++ b/test/integration/support/daemon-leak-model.ts @@ -0,0 +1,281 @@ +// Daemon leak rules (#1781 B1, feeds #1431): given one observation of the host +// process table and of an isolated state dir, decide what the daemon still owns. +// Pure — `daemon-leak-oracle.ts` gathers the observation and asserts on it, and +// `daemon-leak-model.test.ts` pins these rules against fixture observations, +// including the #1109 and #1324 leak shapes. +// +// Ownership is explicit and conservative. Global process counts are not +// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this +// daemon does not. A host process is daemon-owned when ANY rule holds: +// +// descendant its PPID chain reaches a daemon pid, or any process already +// owned below (observable while that ancestor lives; orphans +// reparent to launchd/init). +// process-group its PGID equals a daemon pid. The CLI launches the daemon +// detached (setsid), so the daemon is its own group leader and +// every non-detached runCmdBackground child stays in that group +// after reparenting. This is the #1324 signature: `simctl io … +// recordVideo` with PPID 1 and PGID = the dead daemon's pid. +// A pgid is reserved only while the group has a live member, so +// on a long-lived host pid reuse can eventually hand the number +// to an unrelated process; the lanes' state dirs are fresh per +// run, and every red-proof cross-checks the command line. +// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. +// The CLI starts the daemon with that variable and children +// inherit it, including setsid'd grandchildren the pgid rule +// misses (agent-browser's own daemon → Chrome fleet: #1109). +// macOS hides the environment of Apple platform binaries +// (`ps -E` shows it for node/Chrome, not for /bin/sleep or +// simctl), so Apple tooling is caught by the pgid/argv rules. +// state-dir-argv its command line contains the state dir path as a whole path +// token (recorders writing session artifacts, browsers pinned +// to a managed home). +// +// The lanes use a fresh mkdtemp state dir per run, so the env/argv rules cannot +// match a foreign process. The caller excludes itself and its own ancestors. +// +// Two further leak classes, independent of owned children: +// +// surviving daemon at `after-shutdown` a daemon pid that is still alive IS +// the leak. `stopProcessForTakeover` is best-effort and +// returns silently on identity mismatch, signal failure, or +// kill timeout, so a lane that only stops the daemon never +// learns it survived. +// state-dir residue every entry must match EXPECTED_STATE_DIR_ENTRIES +// (unknown ⇒ classify the new artifact, do not widen the +// matcher): `*.tmp` write-then-publish temporaries are torn +// publishes, an empty directory is an unswept session +// scaffold, daemon.json/daemon.lock may exist only while a +// daemon legitimately lives, and a capture descriptor still +// `lifecycle: "open"` (or a legacy `app-log.pid` marker) +// after shutdown is a capture handle that outlived its +// owner — a `completed` descriptor is the finish record +// ADR 0019 keeps. `tools/` holds managed third-party +// installs (agent-browser and its Chrome), whose own +// download temporaries and scaffolding this daemon neither +// writes nor owns. +import { expandProcessTree, uniquePositivePids } from '../../../src/utils/host-process.ts'; + +const COMMAND_PREVIEW_CHARS = 160; + +export type DaemonLeakPhase = 'after-close' | 'after-shutdown'; + +export type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; + +export type HostProcess = { + pid: number; + ppid: number; + pgid: number; + command: string; + /** Environment as a single string; empty when the host hides it. */ + env: string; +}; + +/** One state-dir path, pre-read so the rules stay free of filesystem access. */ +export type StateEntry = { + /** Relative to the state dir, `/`-separated; directories keep a trailing `/`. */ + path: string; + kind: 'file' | 'empty-directory'; + /** `lifecycle` of a durable capture descriptor, when this entry is one. */ + descriptorLifecycle?: string; +}; + +export type OwnedProcess = { + pid: number; + ppid: number; + pgid: number; + command: string; + reasons: OwnershipReason[]; +}; + +export type DaemonLeakObservation = { + stateDir: string; + daemonPids: readonly number[]; + livePids: readonly number[]; + phase: DaemonLeakPhase; + processes: readonly HostProcess[]; + /** Pids never treated as owned: the observer and its own ancestors. */ + excludedPids: readonly number[]; + stateEntries: readonly StateEntry[]; +}; + +export type DaemonLeakSnapshot = { + stateDir: string; + daemonPids: number[]; + phase: DaemonLeakPhase; + liveDaemonPids: number[]; + ownedProcesses: OwnedProcess[]; + strayStateEntries: string[]; +}; + +// `tools/` is a managed third-party install tree (agent-browser + Chrome), not +// daemon write-then-publish output, so it is exempted before the generic rules. +const MANAGED_TOOLS_ENTRY = /^tools\//; +const EXPECTED_STATE_DIR_ENTRIES: readonly RegExp[] = [ + /^daemon\.json$/, + /^daemon\.lock$/, + /^daemon\.log$/, + /^daemon-shutdown\.json$/, + /^sessions\/[^/]+\/events\.ndjson$/, + /^sessions\/[^/]+\/requests\/[^/]+\.ndjson$/, + /^sessions\/[^/]+\/(?:app|runner)\.log$/, + /^sessions\/[^/]+\/repair-tombstone\.json$/, + /^sessions\/[^/]+\/artifacts\/.+$/, + /^device-claims\/.+$/, +]; +const CAPTURE_DESCRIPTOR_ENTRY = /^sessions\/[^/]+\/[^/]+\.resource\.json$/; +const LEGACY_APP_LOG_MARKER_ENTRY = /^sessions\/[^/]+\/app-log\.pid$/; +const DAEMON_LIVENESS_ENTRY = /^daemon\.(?:json|lock)$/; + +export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonLeakSnapshot { + const daemonPids = uniquePositivePids(observation.daemonPids); + const liveDaemonPids = daemonPids.filter((pid) => observation.livePids.includes(pid)); + const daemonLegitimatelyAlive = observation.phase === 'after-close' && liveDaemonPids.length > 0; + return { + stateDir: observation.stateDir, + daemonPids, + phase: observation.phase, + liveDaemonPids, + ownedProcesses: findOwnedProcesses(observation, daemonPids), + strayStateEntries: observation.stateEntries + .filter( + (entry) => + classifyStateEntry(entry, observation.phase, daemonLegitimatelyAlive) === 'stray', + ) + .map((entry) => entry.path) + .sort(), + }; +} + +/** + * A daemon that outlived its own shutdown is itself the leak — not a detail of + * the report — so it fails alongside owned children and state-dir residue. + */ +export function hasDaemonLeaks(snapshot: DaemonLeakSnapshot): boolean { + return ( + survivingDaemonPids(snapshot).length > 0 || + snapshot.ownedProcesses.length > 0 || + snapshot.strayStateEntries.length > 0 + ); +} + +function survivingDaemonPids(snapshot: DaemonLeakSnapshot): number[] { + return snapshot.phase === 'after-shutdown' ? snapshot.liveDaemonPids : []; +} + +function findOwnedProcesses( + observation: DaemonLeakObservation, + daemonPids: readonly number[], +): OwnedProcess[] { + const excluded = new Set([...observation.excludedPids, ...daemonPids]); + const matchers = stateDirMatchers(observation.stateDir); + const reasonsByPid = new Map(); + for (const proc of observation.processes) { + if (excluded.has(proc.pid)) continue; + const reasons = directOwnershipReasons(proc, daemonPids, matchers); + if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); + } + // Ownership propagates down the tree: a child of the daemon, or of any process + // the rules above own, is owned (a detached `(simctl)` grandchild of a leaked + // recorder shim, DTServiceHub under a runner xcodebuild, …). + const roots = [...daemonPids, ...reasonsByPid.keys()]; + for (const proc of expandProcessTree(roots, observation.processes)) { + if (excluded.has(proc.pid) || roots.includes(proc.pid)) continue; + reasonsByPid.set(proc.pid, [...(reasonsByPid.get(proc.pid) ?? []), 'descendant']); + } + return observation.processes.flatMap((proc) => { + const reasons = reasonsByPid.get(proc.pid); + return reasons + ? [{ pid: proc.pid, ppid: proc.ppid, pgid: proc.pgid, command: proc.command, reasons }] + : []; + }); +} + +type StateDirMatchers = { argv: RegExp; env: RegExp }; + +function stateDirMatchers(stateDir: string): StateDirMatchers { + // Whole-path matches only: `` as a token or `/…`, never `-sibling`. + return { + argv: new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`), + env: new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`), + }; +} + +function directOwnershipReasons( + proc: HostProcess, + daemonPids: readonly number[], + matchers: StateDirMatchers, +): OwnershipReason[] { + const reasons: OwnershipReason[] = []; + if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); + if (matchers.env.test(proc.env)) reasons.push('state-dir-env'); + if (matchers.argv.test(proc.command)) reasons.push('state-dir-argv'); + return reasons; +} + +function classifyStateEntry( + entry: StateEntry, + phase: DaemonLeakPhase, + daemonLegitimatelyAlive: boolean, +): 'expected' | 'stray' { + if (MANAGED_TOOLS_ENTRY.test(entry.path)) return 'expected'; + if (entry.kind === 'empty-directory') return 'stray'; + if (entry.path.endsWith('.tmp')) return 'stray'; + if (DAEMON_LIVENESS_ENTRY.test(entry.path)) { + return daemonLegitimatelyAlive ? 'expected' : 'stray'; + } + if (CAPTURE_DESCRIPTOR_ENTRY.test(entry.path) || LEGACY_APP_LOG_MARKER_ENTRY.test(entry.path)) { + return classifyCaptureEntry(entry, phase); + } + return EXPECTED_STATE_DIR_ENTRIES.some((matcher) => matcher.test(entry.path)) + ? 'expected' + : 'stray'; +} + +// Another session may still hold a live capture while this one closes; after +// shutdown only a completed capture record may remain (never a pid marker). +function classifyCaptureEntry(entry: StateEntry, phase: DaemonLeakPhase): 'expected' | 'stray' { + if (phase === 'after-close') return 'expected'; + if (!CAPTURE_DESCRIPTOR_ENTRY.test(entry.path)) return 'stray'; + return entry.descriptorLifecycle === 'completed' ? 'expected' : 'stray'; +} + +export function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { + const surviving = survivingDaemonPids(snapshot); + return [ + `daemon leak oracle: ${hasDaemonLeaks(snapshot) ? 'LEAK' : 'clean'} (${snapshot.phase})`, + ` state dir: ${snapshot.stateDir}`, + ` daemon pids: ${formatPids(snapshot.daemonPids)}; live: ${formatPids(snapshot.liveDaemonPids)}`, + ` daemons that outlived shutdown: ${surviving.length}${ + surviving.length > 0 ? ` (${formatPids(surviving)})` : '' + }`, + ` owned processes still alive: ${snapshot.ownedProcesses.length}`, + ...snapshot.ownedProcesses.map( + (proc) => + ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${previewCommand( + proc.command, + snapshot.stateDir, + )}`, + ), + ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, + ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), + ].join('\n'); +} + +function previewCommand(command: string, stateDir: string): string { + const masked = command.replace( + new RegExp(`${escapeRegExp(stateDir)}(?=[\\s/]|$)`, 'g'), + '', + ); + return masked.length > COMMAND_PREVIEW_CHARS + ? `${masked.slice(0, COMMAND_PREVIEW_CHARS)}…` + : masked; +} + +function formatPids(pids: readonly number[]): string { + return pids.join(', ') || '(none)'; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts index b22c420388..da80ff4f00 100644 --- a/test/integration/support/daemon-leak-oracle.ts +++ b/test/integration/support/daemon-leak-oracle.ts @@ -1,72 +1,36 @@ -// Daemon leak oracle (#1781 B1, feeds #1431). The real-subprocess daemon lanes -// (smoke-daemon-clean, smoke-daemon-http, daemon-replace-exit-flush) call it at -// their end to answer one question: after `close` and after daemon shutdown, -// did the daemon leave anything it OWNS behind? +// Daemon leak oracle (#1781 B1, feeds #1431): observes the host process table +// and an isolated state dir, then applies the ownership/residue rules in +// `daemon-leak-model.ts` (which documents them and is where they are tested). // -// Ownership is explicit and conservative. Global process counts are not -// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this -// daemon does not. A host process is daemon-owned when ANY rule holds: -// -// descendant its PPID chain reaches a daemon pid (observable only while -// that daemon lives; orphans reparent to launchd/init). -// process-group its PGID equals a daemon pid. The CLI launches the daemon -// detached (setsid), so the daemon is its own group leader and -// every non-detached runCmdBackground child stays in that group -// after reparenting. This is the #1324 signature: `simctl io … -// recordVideo` with PPID 1 and PGID = the dead daemon's pid. -// POSIX keeps a pgid reserved while any member lives, so it -// cannot be recycled under the check. -// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. -// The CLI starts the daemon with that variable and children -// inherit it, including setsid'd grandchildren the pgid rule -// misses (agent-browser's own daemon → Chrome fleet: #1109). -// macOS hides the environment of Apple platform binaries -// (`ps -E` shows it for node/Chrome, not for /bin/sleep or -// simctl), so Apple tooling is caught by the pgid/argv rules. -// state-dir-argv its command line contains the state dir path (recorders -// writing session artifacts, browsers pinned to a managed home). -// -// The lanes use a fresh mkdtemp state dir per run, so env/argv rules cannot -// match a foreign process. The oracle also excludes itself and its ancestors. -// -// State-dir residue: after shutdown every entry must match -// EXPECTED_STATE_DIR_ENTRIES (unknown ⇒ classify the new artifact, do not widen -// the matcher), `*.tmp` write-then-publish temporaries always fail (torn -// publish), daemon.json/daemon.lock may exist only while a daemon lives, and a -// durable capture descriptor still `lifecycle: "open"` (or a legacy -// `app-log.pid` marker) after shutdown means a capture handle outlived its -// owner — a `completed` descriptor is the finish record ADR 0019 keeps. +// What each caller actually exercises depends on what its daemon did. The three +// real-subprocess daemon lanes run `session list`/`close` with no device, +// simulator or browser, so their daemons own no children and the process arm +// asserts an empty set; what they guard is the surviving-daemon check and the +// state-dir residue. The #1109/#1324 leak shapes are guarded by the model's +// fixture test and reproduced by hand against the pre-fix commits — see the PR. import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { runCmd } from '../../../src/utils/exec.ts'; import { isProcessAlive } from '../../../src/utils/host-process.ts'; +import { + evaluateDaemonLeaks, + formatDaemonLeakReport, + hasDaemonLeaks, + type DaemonLeakPhase, + type DaemonLeakSnapshot, + type HostProcess, + type StateEntry, +} from './daemon-leak-model.ts'; const PS_TIMEOUT_MS = 5_000; const DEFAULT_SETTLE_MS = 5_000; const SETTLE_POLL_MS = 250; -const COMMAND_PREVIEW_CHARS = 160; - -export type DaemonLeakPhase = 'after-close' | 'after-shutdown'; - -type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; +// Matches src/utils/host-process.ts: an absolute path so a PATH-resolved +// third-party `ps` (Homebrew, procps) cannot change the flag semantics. +const HOST_PS_COMMAND = process.platform === 'win32' ? 'ps' : '/bin/ps'; -type OwnedProcess = { - pid: number; - ppid: number; - pgid: number; - command: string; - reasons: OwnershipReason[]; -}; - -type DaemonLeakSnapshot = { - stateDir: string; - daemonPids: number[]; - phase: DaemonLeakPhase; - liveDaemonPids: number[]; - ownedProcesses: OwnedProcess[]; - strayStateEntries: string[]; -}; +export type { DaemonLeakPhase } from './daemon-leak-model.ts'; export type DaemonLeakOracleOptions = { stateDir: string; @@ -77,95 +41,22 @@ export type DaemonLeakOracleOptions = { settleMs?: number; }; -type HostProcess = { pid: number; ppid: number; pgid: number; command: string; env: string }; - -// Relative-path matchers for everything the daemon may legitimately leave in a -// state dir. `daemon.json`/`daemon.lock` are additionally gated on a live -// daemon; capture descriptors are gated on the phase (see classifyStateEntry). -const EXPECTED_STATE_DIR_ENTRIES: readonly RegExp[] = [ - /^daemon\.json$/, - /^daemon\.lock$/, - /^daemon\.log$/, - /^daemon-shutdown\.json$/, - /^sessions\/[^/]+\/events\.ndjson$/, - /^sessions\/[^/]+\/requests\/[^/]+\.ndjson$/, - /^sessions\/[^/]+\/(?:app|runner)\.log$/, - /^sessions\/[^/]+\/repair-tombstone\.json$/, - /^sessions\/[^/]+\/artifacts\/.+$/, - /^tools\/.+$/, - /^device-claims\/.+$/, -]; -const CAPTURE_DESCRIPTOR_ENTRY = /^sessions\/[^/]+\/[^/]+\.resource\.json$/; -const LEGACY_APP_LOG_MARKER_ENTRY = /^sessions\/[^/]+\/app-log\.pid$/; -const DAEMON_LIVENESS_ENTRY = /^daemon\.(?:json|lock)$/; - async function captureDaemonLeakSnapshot( options: DaemonLeakOracleOptions, ): Promise { const stateDir = path.resolve(options.stateDir); - const daemonPids = uniquePids(options.daemonPids); - const processes = await listHostProcesses(); - const liveDaemonPids = daemonPids.filter((pid) => isProcessAlive(pid)); - return { + const processes = await listHostProcessesWithGroups(); + return evaluateDaemonLeaks({ stateDir, - daemonPids, + daemonPids: options.daemonPids, + livePids: options.daemonPids.filter((pid) => isProcessAlive(pid)), phase: options.phase, - liveDaemonPids, - ownedProcesses: findOwnedProcesses(processes, daemonPids, stateDir), - strayStateEntries: listStateEntries(stateDir).filter( - (entry) => - classifyStateEntry(stateDir, entry, options.phase, liveDaemonPids.length > 0) !== - 'expected', - ), - }; -} - -function findOwnedProcesses( - processes: readonly HostProcess[], - daemonPids: readonly number[], - stateDir: string, -): OwnedProcess[] { - const excluded = new Set([...ancestorsOf(process.pid, processes), ...daemonPids]); - const reasonsByPid = new Map(); - for (const proc of processes) { - if (excluded.has(proc.pid)) continue; - const reasons = directOwnershipReasons(proc, daemonPids, stateDir); - if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); - } - // Ownership propagates down the tree: a child of the daemon, or of any - // process the rules above own, is owned (a detached `(simctl)` grandchild of - // a leaked recorder shim, DTServiceHub under a runner xcodebuild, …). - for (const pid of descendantsOf([...daemonPids, ...reasonsByPid.keys()], processes)) { - if (excluded.has(pid)) continue; - reasonsByPid.set(pid, [...(reasonsByPid.get(pid) ?? []), 'descendant']); - } - return processes.flatMap((proc) => { - const reasons = reasonsByPid.get(proc.pid); - return reasons - ? [{ pid: proc.pid, ppid: proc.ppid, pgid: proc.pgid, command: proc.command, reasons }] - : []; + processes, + excludedPids: [...ancestorsOf(process.pid, processes)], + stateEntries: readStateEntries(stateDir), }); } -function directOwnershipReasons( - proc: HostProcess, - daemonPids: readonly number[], - stateDir: string, -): OwnershipReason[] { - // Whole-path matches only: `` as a token or `/…`, never `-sibling`. - const stateDirToken = new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`); - const stateDirEnv = new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`); - const reasons: OwnershipReason[] = []; - if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); - if (stateDirEnv.test(proc.env)) reasons.push('state-dir-env'); - if (stateDirToken.test(proc.command)) reasons.push('state-dir-argv'); - return reasons; -} - -function hasDaemonLeaks(snapshot: DaemonLeakSnapshot): boolean { - return snapshot.ownedProcesses.length > 0 || snapshot.strayStateEntries.length > 0; -} - /** * Re-snapshots until clean or `settleMs` elapses. Stragglers that exit on their * own inside the window are not leaks; #1324/#1109 orphans never exit on their own. @@ -188,55 +79,36 @@ export async function assertNoDaemonLeaks(options: DaemonLeakOracleOptions): Pro if (hasDaemonLeaks(snapshot)) throw new Error(formatDaemonLeakReport(snapshot)); } -function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { - const lines = [ - `daemon leak oracle: ${hasDaemonLeaks(snapshot) ? 'LEAK' : 'clean'} (${snapshot.phase})`, - ` state dir: ${snapshot.stateDir}`, - ` daemon pids: ${snapshot.daemonPids.join(', ') || '(none)'}; live: ${ - snapshot.liveDaemonPids.join(', ') || '(none)' - }`, - ` owned processes still alive: ${snapshot.ownedProcesses.length}`, - ...snapshot.ownedProcesses.map((proc) => { - const command = proc.command.replace( - new RegExp(`${escapeRegExp(snapshot.stateDir)}(?=[\\s/]|$)`, 'g'), - '', - ); - const preview = - command.length > COMMAND_PREVIEW_CHARS - ? `${command.slice(0, COMMAND_PREVIEW_CHARS)}…` - : command; - return ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${preview}`; - }), - ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, - ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), - ]; - return lines.join('\n'); -} - -function classifyStateEntry( - stateDir: string, - entry: string, - phase: DaemonLeakPhase, - daemonAlive: boolean, -): 'expected' | 'stray' { - if (entry.endsWith('.tmp')) return 'stray'; - if (DAEMON_LIVENESS_ENTRY.test(entry)) return daemonAlive ? 'expected' : 'stray'; - if (CAPTURE_DESCRIPTOR_ENTRY.test(entry) || LEGACY_APP_LOG_MARKER_ENTRY.test(entry)) { - return classifyCaptureEntry(stateDir, entry, phase); +// Files, plus empty directories as their own entries (an unswept session +// scaffold leaves no file behind), plus the `lifecycle` of durable capture +// descriptors so the rules stay free of filesystem access. +function readStateEntries(stateDir: string): StateEntry[] { + if (!fs.existsSync(stateDir)) return []; + const entries: StateEntry[] = []; + for (const dirent of fs.readdirSync(stateDir, { recursive: true, withFileTypes: true })) { + const absolute = path.join(dirent.parentPath, dirent.name); + const relative = path.relative(stateDir, absolute).split(path.sep).join('/'); + if (!dirent.isDirectory()) { + entries.push({ + path: relative, + kind: 'file', + ...(relative.endsWith('.resource.json') + ? { descriptorLifecycle: readDescriptorLifecycle(absolute) } + : {}), + }); + } else if (isEmptyDirectory(absolute)) { + entries.push({ path: `${relative}/`, kind: 'empty-directory' }); + } } - return EXPECTED_STATE_DIR_ENTRIES.some((matcher) => matcher.test(entry)) ? 'expected' : 'stray'; + return entries; } -// Other sessions may still hold live captures while this one closes; after -// shutdown only a completed capture record may remain (never a pid marker). -function classifyCaptureEntry( - stateDir: string, - entry: string, - phase: DaemonLeakPhase, -): 'expected' | 'stray' { - if (phase === 'after-close') return 'expected'; - if (!CAPTURE_DESCRIPTOR_ENTRY.test(entry)) return 'stray'; - return readDescriptorLifecycle(path.join(stateDir, entry)) === 'completed' ? 'expected' : 'stray'; +function isEmptyDirectory(directoryPath: string): boolean { + try { + return fs.readdirSync(directoryPath).length === 0; + } catch { + return false; + } } function readDescriptorLifecycle(filePath: string): string | undefined { @@ -248,21 +120,11 @@ function readDescriptorLifecycle(filePath: string): string | undefined { } } -function listStateEntries(stateDir: string): string[] { - if (!fs.existsSync(stateDir)) return []; - return fs - .readdirSync(stateDir, { recursive: true, withFileTypes: true }) - .filter((entry) => !entry.isDirectory()) - .map((entry) => - path.relative(stateDir, path.join(entry.parentPath, entry.name)).split(path.sep).join('/'), - ) - .sort(); -} - -// pid/ppid/pgid plus the command line, and separately the environment so the -// argv and env rules stay distinct. macOS `ps -E` appends the environment to -// the command line for same-user processes; Linux exposes it in /proc. -async function listHostProcesses(): Promise { +// src/utils/host-process.ts's listHostProcesses covers pid/ppid/command; the +// ownership rules also need the process group and the environment, which need a +// second `ps` on macOS (`-E` appends the environment to the command column) and +// /proc on Linux. +async function listHostProcessesWithGroups(): Promise { const darwin = process.platform === 'darwin'; const [tree, darwinEnv] = await Promise.all([ runPs(['-axww', '-o', 'pid=,ppid=,pgid=,command=']), @@ -290,7 +152,10 @@ function parsePsLines(stdout: string, shape: RegExp): string[][] { } async function runPs(args: string[]): Promise { - const result = await runCmd('ps', args, { allowFailure: true, timeoutMs: PS_TIMEOUT_MS }); + const result = await runCmd(HOST_PS_COMMAND, args, { + allowFailure: true, + timeoutMs: PS_TIMEOUT_MS, + }); if (result.exitCode !== 0) { throw new Error( `daemon leak oracle: ps ${args.join(' ')} failed (${result.exitCode}): ${result.stderr}`, @@ -307,24 +172,6 @@ function readLinuxEnviron(pid: number): string { } } -function descendantsOf( - rootPids: readonly number[], - processes: readonly HostProcess[], -): Set { - const selected = new Set(rootPids); - let changed = true; - while (changed) { - changed = false; - for (const proc of processes) { - if (!selected.has(proc.ppid) || selected.has(proc.pid)) continue; - selected.add(proc.pid); - changed = true; - } - } - for (const pid of rootPids) selected.delete(pid); - return selected; -} - function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set { const byPid = new Map(processes.map((proc) => [proc.pid, proc])); const chain = new Set(); @@ -334,14 +181,6 @@ function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set Number.isInteger(pid) && pid > 0); -} - // Standalone use (red-proofs, manual triage): // node --experimental-strip-types test/integration/support/daemon-leak-oracle.ts \ // --state-dir --daemon-pid [--daemon-pid …] \ diff --git a/vitest.config.ts b/vitest.config.ts index e15533161a..f80dc46c5d 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -89,6 +89,12 @@ export default defineConfig({ // #1781 A9: pins the root-doc paths-ignore entries directly against the // real workflow YAML, parse-only like its sibling above. 'test/ci/root-docs-paths-ignore.test.ts', + // The daemon leak oracle's ownership/residue rules (#1781 B1): pure + // decisions over fixture `ps` rows and state-dir listings, so they + // need no daemon, device, or subprocess. The lanes that call the + // oracle are device-free, so this is where the #1109/#1324 leak + // shapes are actually guarded. + 'test/integration/support/daemon-leak-model.test.ts', // The frozen replay-compat corpus (#1417): parse-only, no device or // subprocess work, so it belongs in the fast lane next to the // grammar it guards. From a1cf0c9879b29227017ceb94e9974824b69807a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 13:30:21 +0200 Subject: [PATCH 3/8] test: make the leak oracle's after-close phase name the session that closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of #1859: `after-close` accepted every capture descriptor and legacy marker, so the closed session's unfinalized handle was indistinguishable from another session's legitimately live one — the phase could not fail, the same shape as the surviving-daemon blocker, and it left half of B1's stated scope undelivered. - the observation carries the session directories closed at the checkpoint; a capture handle must be finalized once its owning session is gone (every session after shutdown, only the closed ones after close), while another session's live handle stays expected and a legacy pid marker is never a finish record - the oracle accepts `closedSessions` and a `sessionsDir` override, normalizing entries to the canonical `sessions//…` shape so an in-process harness rooted directly at its own sessions dir is classified the same way - add a real route regression: a provider-backed session with a live screen recording is closed through the daemon route, and the oracle refuses a descriptor left `lifecycle: "open"`. Reverting the close-route finalization (session-close-lifecycle-teardown.ts, the #1325 fix) turns it red naming sessions/default/screen-recording.resource.json; with the pre-fix model it stays green, which is the P1 in one line Refs #1781 #1431 --- .../session-close-leak-oracle.test.ts | 76 +++++++++++++++++++ .../support/daemon-leak-model.test.ts | 51 ++++++++++++- test/integration/support/daemon-leak-model.ts | 46 ++++++++--- .../integration/support/daemon-leak-oracle.ts | 36 ++++++++- 4 files changed, 191 insertions(+), 18 deletions(-) create mode 100644 test/integration/provider-scenarios/session-close-leak-oracle.test.ts diff --git a/test/integration/provider-scenarios/session-close-leak-oracle.test.ts b/test/integration/provider-scenarios/session-close-leak-oracle.test.ts new file mode 100644 index 0000000000..f9ebe0c196 --- /dev/null +++ b/test/integration/provider-scenarios/session-close-leak-oracle.test.ts @@ -0,0 +1,76 @@ +import assert from 'node:assert/strict'; +import path from 'node:path'; +import { expect, test } from 'vitest'; +import { assertRpcOk } from './assertions.ts'; +import { + createAndroidRecordingScenarioHarness, + withAndroidRecordingScenario, +} from './android-recording-fixtures.ts'; +import { createAndroidRecordingProvider } from './android-recording-provider-fixtures.ts'; +import { PROVIDER_SCENARIO_ANDROID } from './fixtures.ts'; +import { PARALLEL_PROVIDER_SCENARIO_TIMEOUT_MS } from './test-timeouts.ts'; +import { screenRecordingResourceStore } from '../../../src/daemon/screen-recording-resource-store.ts'; +import { assertNoDaemonLeaks } from '../support/daemon-leak-oracle.ts'; + +// The `after-close` half of the #1781 B1 leak oracle, on a real daemon route. +// The three CLI daemon lanes only ever shut a daemon down, so this is the one +// place a closing session actually owns a resource: `record start` opens a +// record-only session with a live screen-recording handle, and the session-close +// teardown that #1325 added must finalize it. Removing that teardown step leaves +// the descriptor `lifecycle: "open"` and turns this test red. +test( + 'Provider-backed integration closing a recording session leaves no unfinalized capture handle', + async () => { + await withAndroidRecordingScenario( + 'agent-device-provider-scenario-close-leak-', + async (tmpDir) => { + const calls: string[][] = []; + const outputPath = path.join(tmpDir, 'close-leak.mp4'); + const daemon = await createAndroidRecordingScenarioHarness({ + androidAdbProvider: () => createAndroidRecordingProvider({ calls }), + deviceInventoryProvider: async () => [PROVIDER_SCENARIO_ANDROID], + }); + try { + const started = await daemon.callCommand('record', ['start', outputPath], { + platform: 'android', + serial: PROVIDER_SCENARIO_ANDROID.id, + recordingScope: 'device', + }); + assert.equal(assertRpcOk<{ recording?: unknown }>(started).recording, 'started'); + + const sessionDir = daemon.sessionDir('default'); + // The live handle is durable state: the oracle must be able to see the + // unfinalized descriptor it will later refuse. + assert.equal( + screenRecordingResourceStore.read(screenRecordingResourceStore.resolvePath(sessionDir)) + .status, + 'decoded', + ); + + await daemon.callCommand('close', [], { + platform: 'android', + serial: PROVIDER_SCENARIO_ANDROID.id, + }); + + // `daemonPids: []` on purpose: this harness runs the daemon route + // in-process, so there is no daemon process to own children and the + // state-dir arm is the whole check here. The process arm is exercised by + // the CLI lanes and pinned by daemon-leak-model.test.ts. + await expect( + assertNoDaemonLeaks({ + stateDir: path.dirname(sessionDir), + sessionsDir: path.dirname(sessionDir), + daemonPids: [], + phase: 'after-close', + closedSessions: [path.basename(sessionDir)], + settleMs: 0, + }), + ).resolves.toBeUndefined(); + } finally { + await daemon.close(); + } + }, + ); + }, + PARALLEL_PROVIDER_SCENARIO_TIMEOUT_MS, +); diff --git a/test/integration/support/daemon-leak-model.test.ts b/test/integration/support/daemon-leak-model.test.ts index e65235c409..7e149da201 100644 --- a/test/integration/support/daemon-leak-model.test.ts +++ b/test/integration/support/daemon-leak-model.test.ts @@ -30,6 +30,7 @@ function observe(overrides: Partial = {}): DaemonLeakObse phase: 'after-shutdown', processes: [], excludedPids: [OBSERVER_PID], + closedSessions: [], stateEntries: [], ...overrides, }; @@ -177,8 +178,8 @@ describe('state-dir residue rules', () => { 'expected', ], [ - 'open capture during close', - file('sessions/d/screen-recording.resource.json', 'open'), + "another session's open capture during close", + file('sessions/other/screen-recording.resource.json', 'open'), 'after-close', 'expected', ], @@ -200,6 +201,52 @@ describe('state-dir residue rules', () => { }); }); +// The phase only means something if it can name the session that closed: without +// that, the closed session's unfinalized capture handle is indistinguishable +// from another session's legitimately live one, and `after-close` certifies +// nothing. Wired through by the session-close route regression in +// test/integration/provider-scenarios/session-close-leak-oracle.test.ts. +describe('closed-session capture handles', () => { + const closedSessionCapture = (lifecycle: string) => + file('sessions/closed-one/screen-recording.resource.json', lifecycle); + const afterClose = (stateEntries: StateEntry[]): DaemonLeakObservation => + observe({ + phase: 'after-close', + livePids: [DAEMON_PID], + closedSessions: ['closed-one'], + stateEntries, + }); + + test('the closed session must have finalized its capture handle', () => { + const snapshot = evaluateDaemonLeaks(afterClose([closedSessionCapture('open')])); + + expect(snapshot.strayStateEntries).toEqual([ + 'sessions/closed-one/screen-recording.resource.json', + ]); + expect(hasDaemonLeaks(snapshot)).toBe(true); + }); + + test('a session that did not close may still hold a live capture handle', () => { + const snapshot = evaluateDaemonLeaks( + afterClose([file('sessions/still-open/screen-recording.resource.json', 'open')]), + ); + + expect(hasDaemonLeaks(snapshot)).toBe(false); + }); + + test('a finalized handle from the closed session is its finish record', () => { + const snapshot = evaluateDaemonLeaks(afterClose([closedSessionCapture('completed')])); + + expect(hasDaemonLeaks(snapshot)).toBe(false); + }); + + test('a legacy pid marker is never a finish record for the closed session', () => { + const snapshot = evaluateDaemonLeaks(afterClose([file('sessions/closed-one/app-log.pid')])); + + expect(snapshot.strayStateEntries).toEqual(['sessions/closed-one/app-log.pid']); + }); +}); + test('a clean shutdown reports no leak', () => { const snapshot = evaluateDaemonLeaks( observe({ diff --git a/test/integration/support/daemon-leak-model.ts b/test/integration/support/daemon-leak-model.ts index 9d89b939d5..b032abf468 100644 --- a/test/integration/support/daemon-leak-model.ts +++ b/test/integration/support/daemon-leak-model.ts @@ -96,6 +96,13 @@ export type DaemonLeakObservation = { processes: readonly HostProcess[]; /** Pids never treated as owned: the observer and its own ancestors. */ excludedPids: readonly number[]; + /** + * Session directory names closed at this checkpoint. Their capture handles + * must be finalized; other sessions may still hold live ones. A checkpoint + * that cannot say which session closed cannot tell an unfinalized handle from + * a legitimately live one, so `after-close` callers always pass them. + */ + closedSessions: readonly string[]; stateEntries: readonly StateEntry[]; }; @@ -140,7 +147,11 @@ export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonL strayStateEntries: observation.stateEntries .filter( (entry) => - classifyStateEntry(entry, observation.phase, daemonLegitimatelyAlive) === 'stray', + classifyStateEntry(entry, { + phase: observation.phase, + daemonLegitimatelyAlive, + closedSessions: observation.closedSessions, + }) === 'stray', ) .map((entry) => entry.path) .sort(), @@ -213,33 +224,44 @@ function directOwnershipReasons( return reasons; } -function classifyStateEntry( - entry: StateEntry, - phase: DaemonLeakPhase, - daemonLegitimatelyAlive: boolean, -): 'expected' | 'stray' { +type StateEntryContext = { + phase: DaemonLeakPhase; + daemonLegitimatelyAlive: boolean; + closedSessions: readonly string[]; +}; + +function classifyStateEntry(entry: StateEntry, context: StateEntryContext): 'expected' | 'stray' { if (MANAGED_TOOLS_ENTRY.test(entry.path)) return 'expected'; if (entry.kind === 'empty-directory') return 'stray'; if (entry.path.endsWith('.tmp')) return 'stray'; if (DAEMON_LIVENESS_ENTRY.test(entry.path)) { - return daemonLegitimatelyAlive ? 'expected' : 'stray'; + return context.daemonLegitimatelyAlive ? 'expected' : 'stray'; } if (CAPTURE_DESCRIPTOR_ENTRY.test(entry.path) || LEGACY_APP_LOG_MARKER_ENTRY.test(entry.path)) { - return classifyCaptureEntry(entry, phase); + return classifyCaptureEntry(entry, context); } return EXPECTED_STATE_DIR_ENTRIES.some((matcher) => matcher.test(entry.path)) ? 'expected' : 'stray'; } -// Another session may still hold a live capture while this one closes; after -// shutdown only a completed capture record may remain (never a pid marker). -function classifyCaptureEntry(entry: StateEntry, phase: DaemonLeakPhase): 'expected' | 'stray' { - if (phase === 'after-close') return 'expected'; +// A capture handle must be finalized once its owning session is gone: after +// shutdown that is every session, after close only the sessions that closed. +// Another session's live capture stays expected, and a legacy pid marker is +// never a finish record, so it is stray whenever its session is gone. +function classifyCaptureEntry(entry: StateEntry, context: StateEntryContext): 'expected' | 'stray' { + const owningSessionGone = + context.phase === 'after-shutdown' || + context.closedSessions.includes(sessionDirectoryOf(entry.path) ?? ''); + if (!owningSessionGone) return 'expected'; if (!CAPTURE_DESCRIPTOR_ENTRY.test(entry.path)) return 'stray'; return entry.descriptorLifecycle === 'completed' ? 'expected' : 'stray'; } +function sessionDirectoryOf(entryPath: string): string | undefined { + return /^sessions\/([^/]+)\//.exec(entryPath)?.[1]; +} + export function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { const surviving = survivingDaemonPids(snapshot); return [ diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts index da80ff4f00..4a546bd3b5 100644 --- a/test/integration/support/daemon-leak-oracle.ts +++ b/test/integration/support/daemon-leak-oracle.ts @@ -37,6 +37,20 @@ export type DaemonLeakOracleOptions = { /** Every daemon pid the lane observed for this state dir (from daemon.json). */ daemonPids: readonly number[]; phase: DaemonLeakPhase; + /** + * Session directory names (not paths) closed at this checkpoint — required at + * `after-close`, where only they identify whose capture handle must be + * finalized. `sessionStore.resolveSessionDir(name)`'s basename, or the + * `sessionStateDir` an `open`/`close` response reports. + */ + closedSessions?: readonly string[]; + /** + * Where sessions live, when that is not `/sessions` — an in-process + * scenario harness roots its SessionStore directly at its own temp dir. + * Entries below it are normalized to the canonical `sessions//…` shape + * so the rules stay layout-independent. + */ + sessionsDir?: string; /** How long stragglers may take to exit before they count as leaked. */ settleMs?: number; }; @@ -53,7 +67,8 @@ async function captureDaemonLeakSnapshot( phase: options.phase, processes, excludedPids: [...ancestorsOf(process.pid, processes)], - stateEntries: readStateEntries(stateDir), + closedSessions: options.closedSessions ?? [], + stateEntries: readStateEntries(stateDir, options.sessionsDir), }); } @@ -82,12 +97,13 @@ export async function assertNoDaemonLeaks(options: DaemonLeakOracleOptions): Pro // Files, plus empty directories as their own entries (an unswept session // scaffold leaves no file behind), plus the `lifecycle` of durable capture // descriptors so the rules stay free of filesystem access. -function readStateEntries(stateDir: string): StateEntry[] { +function readStateEntries(stateDir: string, sessionsDir?: string): StateEntry[] { if (!fs.existsSync(stateDir)) return []; + const sessionsRoot = path.resolve(sessionsDir ?? path.join(stateDir, 'sessions')); const entries: StateEntry[] = []; for (const dirent of fs.readdirSync(stateDir, { recursive: true, withFileTypes: true })) { const absolute = path.join(dirent.parentPath, dirent.name); - const relative = path.relative(stateDir, absolute).split(path.sep).join('/'); + const relative = toCanonicalEntryPath(absolute, stateDir, sessionsRoot); if (!dirent.isDirectory()) { entries.push({ path: relative, @@ -103,6 +119,16 @@ function readStateEntries(stateDir: string): StateEntry[] { return entries; } +// `sessions//…` regardless of where the SessionStore is rooted. +function toCanonicalEntryPath(absolute: string, stateDir: string, sessionsRoot: string): string { + const withinSessions = path.relative(sessionsRoot, absolute); + const relative = + withinSessions.startsWith('..') || path.isAbsolute(withinSessions) + ? path.relative(stateDir, absolute) + : path.join('sessions', withinSessions); + return relative.split(path.sep).join('/'); +} + function isEmptyDirectory(directoryPath: string): boolean { try { return fs.readdirSync(directoryPath).length === 0; @@ -184,7 +210,8 @@ function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set --daemon-pid [--daemon-pid …] \ -// [--phase after-shutdown|after-close] [--settle-ms ] +// [--phase after-shutdown|after-close] [--closed-session …] +// [--settle-ms ] // Prints the report and exits 1 on a leak. if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { const args = process.argv.slice(2); @@ -196,6 +223,7 @@ if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.me stateDir, daemonPids: readFlag('--daemon-pid').map(Number), phase: (readFlag('--phase')[0] as DaemonLeakPhase | undefined) ?? 'after-shutdown', + closedSessions: readFlag('--closed-session'), settleMs: Number(readFlag('--settle-ms')[0] ?? DEFAULT_SETTLE_MS), }); process.stdout.write(`${formatDaemonLeakReport(snapshot)}\n`); From e2c1139a2b8c5f17849fa4c67175571bd863189d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 14:35:11 +0200 Subject: [PATCH 4/8] test: require the closed-session identity at the leak oracle's owning interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of #1859: `closedSessions` was optional and defaulted to empty, so any caller — or the standalone CLI invoked with `--phase after-close` and no `--closed-session` — silently restored the vacuous checkpoint that accepts every unfinalized capture handle and reports clean. - phase and the identity that phase needs are now one discriminated shape: { phase: 'after-close'; closedSessions: NonEmpty } | { phase: 'after-shutdown' }, so the empty case is not expressible and 'after-shutdown' is unchanged - the CLI refuses the same invocation with a typed INVALID_ARGS error and a hint naming what the missing identity would have cost, instead of degrading - daemon-leak-oracle-cli.test.ts drives the real CLI: the unnamed after-close invocation must fail without reporting, the named one finds the planted unfinalized handle, and after-shutdown is unaffected. Reverting the guard makes it print 'clean (after-close)' over that same handle and turns the test red Refs #1781 #1431 --- .../daemon-leak-oracle-cli.test.ts | 61 ++++++++++++++ .../support/daemon-leak-model.test.ts | 74 +++++++++------- test/integration/support/daemon-leak-model.ts | 32 ++++--- .../integration/support/daemon-leak-oracle.ts | 84 ++++++++++++------- 4 files changed, 185 insertions(+), 66 deletions(-) create mode 100644 test/integration/daemon-leak-oracle-cli.test.ts diff --git a/test/integration/daemon-leak-oracle-cli.test.ts b/test/integration/daemon-leak-oracle-cli.test.ts new file mode 100644 index 0000000000..6a78105698 --- /dev/null +++ b/test/integration/daemon-leak-oracle-cli.test.ts @@ -0,0 +1,61 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { runCmdSync } from '../../src/utils/exec.ts'; + +// #1781 B1: the oracle's `after-close` checkpoint is only meaningful when it can +// name the sessions that closed — without them it would accept every unfinalized +// capture handle and report clean, which is the vacuous mode the type system now +// refuses in code. The standalone CLI is the one caller that builds options from +// strings rather than types, so it must refuse the same invocation at runtime +// instead of silently degrading to that mode. + +const ORACLE_PATH = 'test/integration/support/daemon-leak-oracle.ts'; + +function runOracle(args: string[]) { + return runCmdSync(process.execPath, ['--experimental-strip-types', ORACLE_PATH, ...args], { + allowFailure: true, + timeoutMs: 60_000, + }); +} + +test('the leak oracle CLI refuses an after-close checkpoint that names no session', (t) => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-leak-oracle-cli-')); + // A closed session that left an unfinalized capture handle: the exact residue + // the refused invocation would have reported clean. + const sessionDir = path.join(stateDir, 'sessions', 'closed-one'); + fs.mkdirSync(sessionDir, { recursive: true }); + fs.writeFileSync( + path.join(sessionDir, 'screen-recording.resource.json'), + `${JSON.stringify({ lifecycle: 'open' })}\n`, + ); + t.after(() => fs.rmSync(stateDir, { recursive: true, force: true })); + + const refused = runOracle(['--state-dir', stateDir, '--phase', 'after-close']); + assert.notEqual(refused.exitCode, 0, `expected a refusal, got:\n${refused.stdout}`); + assert.match(refused.stderr, /--phase after-close requires at least one --closed-session/); + // A refusal, not a leak report: the checkpoint never ran. + assert.doesNotMatch(refused.stdout, /daemon leak oracle:/); + + // The same invocation, once it names the session, runs and finds the handle. + const named = runOracle([ + '--state-dir', + stateDir, + '--phase', + 'after-close', + '--closed-session', + 'closed-one', + '--settle-ms', + '0', + ]); + assert.equal(named.exitCode, 1, `expected a leak report, got:\n${named.stdout}${named.stderr}`); + assert.match(named.stdout, /LEAK \(after-close\)/); + assert.match(named.stdout, /sessions\/closed-one\/screen-recording\.resource\.json/); + + // `after-shutdown` needs no session identity and is unaffected by the guard. + const shutdown = runOracle(['--state-dir', stateDir, '--settle-ms', '0']); + assert.equal(shutdown.exitCode, 1, shutdown.stderr); + assert.match(shutdown.stdout, /LEAK \(after-shutdown\)/); +}); diff --git a/test/integration/support/daemon-leak-model.test.ts b/test/integration/support/daemon-leak-model.test.ts index 7e149da201..a3bdf236f4 100644 --- a/test/integration/support/daemon-leak-model.test.ts +++ b/test/integration/support/daemon-leak-model.test.ts @@ -4,8 +4,10 @@ import { formatDaemonLeakReport, hasDaemonLeaks, type DaemonLeakObservation, - type DaemonLeakPhase, + type DaemonLeakObservationBase, + type DaemonLeakPhaseSelection, type HostProcess, + type NonEmpty, type StateEntry, } from './daemon-leak-model.ts'; @@ -22,20 +24,35 @@ function proc(overrides: Partial & Pick): HostP return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides }; } -function observe(overrides: Partial = {}): DaemonLeakObservation { +// The phase and its identity arrive together: `after-close` cannot be requested +// without naming the sessions that closed, so these helpers cannot construct the +// vacuous checkpoint either. +function observe( + overrides: Partial = {}, + selection: DaemonLeakPhaseSelection = { phase: 'after-shutdown' }, +): DaemonLeakObservation { return { stateDir: STATE_DIR, daemonPids: [DAEMON_PID], livePids: [], - phase: 'after-shutdown', processes: [], excludedPids: [OBSERVER_PID], - closedSessions: [], stateEntries: [], ...overrides, + ...selection, }; } +function observeAfterClose( + closedSessions: NonEmpty, + overrides: Partial = {}, +): DaemonLeakObservation { + return observe( + { livePids: [DAEMON_PID], ...overrides }, + { phase: 'after-close', closedSessions }, + ); +} + function file(entryPath: string, descriptorLifecycle?: string): StateEntry { return { path: entryPath, kind: 'file', ...(descriptorLifecycle ? { descriptorLifecycle } : {}) }; } @@ -129,9 +146,7 @@ describe('surviving-daemon rule', () => { test('a live daemon is expected while a session merely closed', () => { const snapshot = evaluateDaemonLeaks( - observe({ - phase: 'after-close', - livePids: [DAEMON_PID], + observeAfterClose(['closed-one'], { stateEntries: [file('daemon.json'), file('daemon.lock')], }), ); @@ -141,51 +156,59 @@ describe('surviving-daemon rule', () => { }); describe('state-dir residue rules', () => { - test.each<[string, StateEntry, DaemonLeakPhase, 'expected' | 'stray']>([ - ['session event log', file('sessions/default/events.ndjson'), 'after-shutdown', 'expected'], - ['request diagnostics', file('sessions/d/requests/abc.ndjson'), 'after-shutdown', 'expected'], - ['shutdown report', file('daemon-shutdown.json'), 'after-shutdown', 'expected'], - ['torn publish temporary', file('device-claims/a.json.55.tmp'), 'after-shutdown', 'stray'], + const AFTER_SHUTDOWN: DaemonLeakPhaseSelection = { phase: 'after-shutdown' }; + // A different session from the one this entry belongs to, so the row proves the + // phase rule rather than the closed-session rule. + const AFTER_CLOSE: DaemonLeakPhaseSelection = { + phase: 'after-close', + closedSessions: ['closed-one'], + }; + + test.each<[string, StateEntry, DaemonLeakPhaseSelection, 'expected' | 'stray']>([ + ['session event log', file('sessions/default/events.ndjson'), AFTER_SHUTDOWN, 'expected'], + ['request diagnostics', file('sessions/d/requests/abc.ndjson'), AFTER_SHUTDOWN, 'expected'], + ['shutdown report', file('daemon-shutdown.json'), AFTER_SHUTDOWN, 'expected'], + ['torn publish temporary', file('device-claims/a.json.55.tmp'), AFTER_SHUTDOWN, 'stray'], [ 'managed tool download', file('tools/agent-browser/0.27.1/dl.tmp'), - 'after-shutdown', + AFTER_SHUTDOWN, 'expected', ], [ 'unswept artifact scaffold', { path: 'sessions/d/artifacts/pending/', kind: 'empty-directory' }, - 'after-shutdown', + AFTER_SHUTDOWN, 'stray', ], [ 'unswept session scaffold', { path: 'sessions/leaked/requests/', kind: 'empty-directory' }, - 'after-shutdown', + AFTER_SHUTDOWN, 'stray', ], - ['unknown artifact', file('sessions/d/mystery.bin'), 'after-shutdown', 'stray'], + ['unknown artifact', file('sessions/d/mystery.bin'), AFTER_SHUTDOWN, 'stray'], [ 'open capture descriptor', file('sessions/d/screen-recording.resource.json', 'open'), - 'after-shutdown', + AFTER_SHUTDOWN, 'stray', ], [ 'completed capture descriptor', file('sessions/d/screen-recording.resource.json', 'completed'), - 'after-shutdown', + AFTER_SHUTDOWN, 'expected', ], [ "another session's open capture during close", file('sessions/other/screen-recording.resource.json', 'open'), - 'after-close', + AFTER_CLOSE, 'expected', ], - ['legacy app-log marker', file('sessions/d/app-log.pid'), 'after-shutdown', 'stray'], - ])('%s is %s at %s', (_name, entry, phase, verdict) => { - const snapshot = evaluateDaemonLeaks(observe({ phase, stateEntries: [entry] })); + ['legacy app-log marker', file('sessions/d/app-log.pid'), AFTER_SHUTDOWN, 'stray'], + ])('%s is %s', (_name, entry, selection, verdict) => { + const snapshot = evaluateDaemonLeaks(observe({ stateEntries: [entry] }, selection)); expect(snapshot.strayStateEntries).toEqual(verdict === 'stray' ? [entry.path] : []); }); @@ -210,12 +233,7 @@ describe('closed-session capture handles', () => { const closedSessionCapture = (lifecycle: string) => file('sessions/closed-one/screen-recording.resource.json', lifecycle); const afterClose = (stateEntries: StateEntry[]): DaemonLeakObservation => - observe({ - phase: 'after-close', - livePids: [DAEMON_PID], - closedSessions: ['closed-one'], - stateEntries, - }); + observeAfterClose(['closed-one'], { stateEntries }); test('the closed session must have finalized its capture handle', () => { const snapshot = evaluateDaemonLeaks(afterClose([closedSessionCapture('open')])); diff --git a/test/integration/support/daemon-leak-model.ts b/test/integration/support/daemon-leak-model.ts index b032abf468..a74c17bda9 100644 --- a/test/integration/support/daemon-leak-model.ts +++ b/test/integration/support/daemon-leak-model.ts @@ -88,24 +88,32 @@ export type OwnedProcess = { reasons: OwnershipReason[]; }; -export type DaemonLeakObservation = { +export type NonEmpty = readonly [T, ...T[]]; + +/** + * The phase and the identity it needs, as one shape. An `after-close` + * checkpoint that cannot name the sessions that closed cannot tell their + * unfinalized capture handles from another session's legitimately live one, so + * it would accept every open handle and certify nothing. The identity is + * therefore part of the phase rather than an option a caller may omit, and the + * typechecker refuses the empty case. + */ +export type DaemonLeakPhaseSelection = + | { phase: 'after-shutdown' } + | { phase: 'after-close'; closedSessions: NonEmpty }; + +export type DaemonLeakObservationBase = { stateDir: string; daemonPids: readonly number[]; livePids: readonly number[]; - phase: DaemonLeakPhase; processes: readonly HostProcess[]; /** Pids never treated as owned: the observer and its own ancestors. */ excludedPids: readonly number[]; - /** - * Session directory names closed at this checkpoint. Their capture handles - * must be finalized; other sessions may still hold live ones. A checkpoint - * that cannot say which session closed cannot tell an unfinalized handle from - * a legitimately live one, so `after-close` callers always pass them. - */ - closedSessions: readonly string[]; stateEntries: readonly StateEntry[]; }; +export type DaemonLeakObservation = DaemonLeakPhaseSelection & DaemonLeakObservationBase; + export type DaemonLeakSnapshot = { stateDir: string; daemonPids: number[]; @@ -150,7 +158,7 @@ export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonL classifyStateEntry(entry, { phase: observation.phase, daemonLegitimatelyAlive, - closedSessions: observation.closedSessions, + closedSessions: closedSessionsOf(observation), }) === 'stray', ) .map((entry) => entry.path) @@ -258,6 +266,10 @@ function classifyCaptureEntry(entry: StateEntry, context: StateEntryContext): 'e return entry.descriptorLifecycle === 'completed' ? 'expected' : 'stray'; } +function closedSessionsOf(observation: DaemonLeakObservation): readonly string[] { + return observation.phase === 'after-close' ? observation.closedSessions : []; +} + function sessionDirectoryOf(entryPath: string): string | undefined { return /^sessions\/([^/]+)\//.exec(entryPath)?.[1]; } diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts index 4a546bd3b5..ce028d2dac 100644 --- a/test/integration/support/daemon-leak-oracle.ts +++ b/test/integration/support/daemon-leak-oracle.ts @@ -13,11 +13,12 @@ import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { runCmd } from '../../../src/utils/exec.ts'; import { isProcessAlive } from '../../../src/utils/host-process.ts'; +import { AppError } from '@agent-device/kernel/errors'; import { evaluateDaemonLeaks, formatDaemonLeakReport, hasDaemonLeaks, - type DaemonLeakPhase, + type DaemonLeakPhaseSelection, type DaemonLeakSnapshot, type HostProcess, type StateEntry, @@ -30,20 +31,20 @@ const SETTLE_POLL_MS = 250; // third-party `ps` (Homebrew, procps) cannot change the flag semantics. const HOST_PS_COMMAND = process.platform === 'win32' ? 'ps' : '/bin/ps'; -export type { DaemonLeakPhase } from './daemon-leak-model.ts'; - -export type DaemonLeakOracleOptions = { +/** + * `phase` carries the identity that phase needs: `after-close` cannot be + * requested without naming at least one closed session (see + * DaemonLeakPhaseSelection), so the vacuous checkpoint that accepts every open + * capture handle is not expressible. `after-shutdown` takes nothing extra. + * + * Closed sessions are directory names, not paths — the basename of + * `sessionStore.resolveSessionDir(name)`, or of the `sessionStateDir` an + * `open`/`close` response reports. + */ +export type DaemonLeakOracleOptions = DaemonLeakPhaseSelection & { stateDir: string; /** Every daemon pid the lane observed for this state dir (from daemon.json). */ daemonPids: readonly number[]; - phase: DaemonLeakPhase; - /** - * Session directory names (not paths) closed at this checkpoint — required at - * `after-close`, where only they identify whose capture handle must be - * finalized. `sessionStore.resolveSessionDir(name)`'s basename, or the - * `sessionStateDir` an `open`/`close` response reports. - */ - closedSessions?: readonly string[]; /** * Where sessions live, when that is not `/sessions` — an in-process * scenario harness roots its SessionStore directly at its own temp dir. @@ -61,13 +62,12 @@ async function captureDaemonLeakSnapshot( const stateDir = path.resolve(options.stateDir); const processes = await listHostProcessesWithGroups(); return evaluateDaemonLeaks({ + ...phaseSelectionOf(options), stateDir, daemonPids: options.daemonPids, livePids: options.daemonPids.filter((pid) => isProcessAlive(pid)), - phase: options.phase, processes, excludedPids: [...ancestorsOf(process.pid, processes)], - closedSessions: options.closedSessions ?? [], stateEntries: readStateEntries(stateDir, options.sessionsDir), }); } @@ -94,6 +94,12 @@ export async function assertNoDaemonLeaks(options: DaemonLeakOracleOptions): Pro if (hasDaemonLeaks(snapshot)) throw new Error(formatDaemonLeakReport(snapshot)); } +function phaseSelectionOf(options: DaemonLeakOracleOptions): DaemonLeakPhaseSelection { + return options.phase === 'after-close' + ? { phase: 'after-close', closedSessions: options.closedSessions } + : { phase: 'after-shutdown' }; +} + // Files, plus empty directories as their own entries (an unswept session // scaffold leaves no file behind), plus the `lifecycle` of durable capture // descriptors so the rules stay free of filesystem access. @@ -207,25 +213,47 @@ function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set --daemon-pid [--daemon-pid …] \ -// [--phase after-shutdown|after-close] [--closed-session …] -// [--settle-ms ] -// Prints the report and exits 1 on a leak. -if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { - const args = process.argv.slice(2); +/** + * Parses the standalone CLI's argv into oracle options, refusing the invocation + * the type system already refuses in code: `--phase after-close` without a + * `--closed-session` would accept every open capture handle and report clean. + */ +function parseDaemonLeakOracleArgs(argv: readonly string[]): DaemonLeakOracleOptions { const readFlag = (name: string): string[] => - args.flatMap((arg, index) => (arg === name && args[index + 1] ? [args[index + 1]!] : [])); + argv.flatMap((arg, index) => (arg === name && argv[index + 1] ? [argv[index + 1]!] : [])); const stateDir = readFlag('--state-dir')[0]; - if (!stateDir) throw new Error('daemon leak oracle: --state-dir is required'); - const snapshot = await settleDaemonLeakSnapshot({ + if (!stateDir) { + throw new AppError('INVALID_ARGS', 'daemon leak oracle: --state-dir is required'); + } + const common = { stateDir, daemonPids: readFlag('--daemon-pid').map(Number), - phase: (readFlag('--phase')[0] as DaemonLeakPhase | undefined) ?? 'after-shutdown', - closedSessions: readFlag('--closed-session'), + ...(readFlag('--sessions-dir')[0] ? { sessionsDir: readFlag('--sessions-dir')[0] } : {}), settleMs: Number(readFlag('--settle-ms')[0] ?? DEFAULT_SETTLE_MS), - }); + }; + const phase = readFlag('--phase')[0] ?? 'after-shutdown'; + if (phase !== 'after-close') return { ...common, phase: 'after-shutdown' }; + const [first, ...rest] = readFlag('--closed-session'); + if (!first) { + throw new AppError( + 'INVALID_ARGS', + 'daemon leak oracle: --phase after-close requires at least one --closed-session', + { + hint: 'Pass the closed session directory name (the basename of the session state dir). Without it the checkpoint would accept every unfinalized capture handle and report clean.', + }, + ); + } + return { ...common, phase: 'after-close', closedSessions: [first, ...rest] }; +} + +// Standalone use (red-proofs, manual triage): +// node --experimental-strip-types test/integration/support/daemon-leak-oracle.ts \ +// --state-dir --daemon-pid [--daemon-pid …] \ +// [--phase after-shutdown | --phase after-close --closed-session …] +// [--sessions-dir ] [--settle-ms ] +// Prints the report and exits 1 on a leak. +if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { + const snapshot = await settleDaemonLeakSnapshot(parseDaemonLeakOracleArgs(process.argv.slice(2))); process.stdout.write(`${formatDaemonLeakReport(snapshot)}\n`); process.exitCode = hasDaemonLeaks(snapshot) ? 1 : 0; } From 068992d21703f6fec5c8a09115d4ca394239a2ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 15:19:34 +0200 Subject: [PATCH 5/8] test: split the leak oracle's unrecorded process arm out of the lane path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer asked whether the harness can be trimmed. The two arms differ in kind: the daemon already records captures and state-dir artifacts, so those rules are short and fire on every lane run; nothing records what a daemon spawned, so the process arm reconstructs ownership from the OS four ways — and the three CLI daemon lanes are device-free, so it only ever compared an empty set to an empty set. Move that arm to test/integration/support/daemon-owned-process-probe.ts, the manual script that produced the #1109/#1324 evidence, and keep its rules pinned by a fixture test (no lane can run the probe, so its fixtures are the only CI guard on those shapes). The shipped oracle keeps every guarantee it had: surviving daemon at after-shutdown, the closed-session finalization rule, the state-dir allowlist, the CLI refusal, and the phase-discriminated types. Lane-path harness 852 → 548 LOC (-36%); repo total roughly flat, because the ownership reconstruction can be relocated but not deleted. That is the argument for the follow-up: once the daemon records owned child pids the way it records captures, the arm collapses to "read the record, assert they are dead" and moves back into the oracle. Re-proved after the move: #1324 against pre-fix a84caa818 on an iPhone 16 simulator still goes red through the probe (simctl recordVideo, ppid 1, pgid = the dead daemon, 0-byte mp4), and clean once the orphan is reaped. Refs #1781 #1431 --- .fallowrc.json | 4 + .../support/daemon-leak-model.test.ts | 88 +------ test/integration/support/daemon-leak-model.ts | 229 ++++-------------- .../integration/support/daemon-leak-oracle.ts | 103 +------- .../daemon-owned-process-probe.test.ts | 74 ++++++ .../support/daemon-owned-process-probe.ts | 218 +++++++++++++++++ vitest.config.ts | 4 + 7 files changed, 358 insertions(+), 362 deletions(-) create mode 100644 test/integration/support/daemon-owned-process-probe.test.ts create mode 100644 test/integration/support/daemon-owned-process-probe.ts diff --git a/.fallowrc.json b/.fallowrc.json index ed1e6fbff3..955ceff188 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -24,6 +24,10 @@ // subprocess (test/integration/daemon-replace-exit-flush.test.ts), so // dependency analysis cannot follow the runCmdSync string path to it. "test/integration/support/exit-after-flush.ts", + // #1781 B1: the manual red-proof probe for daemon-owned child processes. It + // is run by hand (and produced the #1109/#1324 evidence), so nothing imports + // it; see its header for why that arm cannot assert from a device-free lane. + "test/integration/support/daemon-owned-process-probe.ts", "src/utils/update-check-entry.ts", "examples/sdk/client-session.ts", "examples/sdk/metro-runtime.ts", diff --git a/test/integration/support/daemon-leak-model.test.ts b/test/integration/support/daemon-leak-model.test.ts index a3bdf236f4..e838d41af4 100644 --- a/test/integration/support/daemon-leak-model.test.ts +++ b/test/integration/support/daemon-leak-model.test.ts @@ -6,23 +6,12 @@ import { type DaemonLeakObservation, type DaemonLeakObservationBase, type DaemonLeakPhaseSelection, - type HostProcess, type NonEmpty, type StateEntry, } from './daemon-leak-model.ts'; -// The lanes that call the oracle run device-free daemons, so the ownership rules -// would otherwise only ever see an empty process set. These fixtures are the -// real `ps` shapes captured during the #1109 and #1324 red-proofs (SHAs in the -// #1781 B1 PR), so a regex or ordering edit that stops catching either leak -// fails here instead of silently going quiet in CI. const STATE_DIR = '/tmp/agent-device-lane-abc'; const DAEMON_PID = 4340; -const OBSERVER_PID = 999; - -function proc(overrides: Partial & Pick): HostProcess { - return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides }; -} // The phase and its identity arrive together: `after-close` cannot be requested // without naming the sessions that closed, so these helpers cannot construct the @@ -35,8 +24,6 @@ function observe( stateDir: STATE_DIR, daemonPids: [DAEMON_PID], livePids: [], - processes: [], - excludedPids: [OBSERVER_PID], stateEntries: [], ...overrides, ...selection, @@ -57,72 +44,6 @@ function file(entryPath: string, descriptorLifecycle?: string): StateEntry { return { path: entryPath, kind: 'file', ...(descriptorLifecycle ? { descriptorLifecycle } : {}) }; } -describe('owned-process rules', () => { - // #1324: `simctl io … recordVideo` reparents to launchd (ppid 1) but keeps the - // dead daemon's process group, and simctl only finalizes the mp4 on SIGINT. - test('flags a recorder orphaned into the dead daemon process group', () => { - const recorder = proc({ - pid: 52420, - ppid: 1, - pgid: DAEMON_PID, - command: '/…/simctl io 416440AE recordVideo /tmp/out.mp4', - }); - const snapshot = evaluateDaemonLeaks(observe({ processes: [recorder] })); - - expect(snapshot.ownedProcesses).toEqual([ - expect.objectContaining({ pid: 52420, reasons: ['process-group'] }), - ]); - expect(hasDaemonLeaks(snapshot)).toBe(true); - }); - - // #1109: the agent-browser daemon setsids away from the daemon's group, so - // only the inherited state-dir environment and its argv identify the fleet. - test('flags an agent-browser fleet by inherited state dir, including its children', () => { - const browserDaemon = proc({ - pid: 47515, - ppid: 1, - pgid: 47515, - command: `${STATE_DIR}/tools/agent-browser/0.27.1/package/…/agent-browser-darwin-arm64`, - env: `HOME=/tmp AGENT_DEVICE_STATE_DIR=${STATE_DIR}`, - }); - const chrome = proc({ - pid: 47586, - ppid: 47515, - pgid: 47586, - command: 'Google Chrome for Testing', - }); - const renderer = proc({ - pid: 47953, - ppid: 47586, - pgid: 47586, - command: 'Chrome Helper (Renderer)', - }); - const snapshot = evaluateDaemonLeaks(observe({ processes: [browserDaemon, chrome, renderer] })); - - expect(snapshot.ownedProcesses.map((owned) => owned.pid)).toEqual([47515, 47586, 47953]); - expect(snapshot.ownedProcesses[0]?.reasons).toEqual(['state-dir-env', 'state-dir-argv']); - // The fleet below the matched root is owned transitively, not by its own argv. - expect(snapshot.ownedProcesses[1]?.reasons).toEqual(['descendant']); - }); - - test('ignores foreign processes, the observer chain, and the daemons themselves', () => { - const simulator = proc({ pid: 700, command: '/…/CoreSimulator … SimulatorTrampoline' }); - const neighbourStateDir = proc({ - pid: 701, - command: `node --state-dir ${STATE_DIR}-other/daemon.ts`, - env: `AGENT_DEVICE_STATE_DIR=${STATE_DIR}-other`, - }); - const observer = proc({ pid: OBSERVER_PID, pgid: DAEMON_PID }); - const daemon = proc({ pid: DAEMON_PID, pgid: DAEMON_PID }); - const snapshot = evaluateDaemonLeaks( - observe({ processes: [simulator, neighbourStateDir, observer, daemon] }), - ); - - expect(snapshot.ownedProcesses).toEqual([]); - expect(hasDaemonLeaks(snapshot)).toBe(false); - }); -}); - describe('surviving-daemon rule', () => { // stopProcessForTakeover is best-effort void: it returns silently on identity // mismatch, signal failure, or kill timeout, so a daemon can outlive the stop. @@ -133,7 +54,9 @@ describe('surviving-daemon rule', () => { expect(snapshot.liveDaemonPids).toEqual([DAEMON_PID]); expect(hasDaemonLeaks(snapshot)).toBe(true); - expect(formatDaemonLeakReport(snapshot)).toContain('daemons that outlived shutdown: 1'); + expect(formatDaemonLeakReport(snapshot)).toContain( + `daemons that outlived shutdown: ${DAEMON_PID}`, + ); }); test('its metadata files stay stray rather than being excused by its own survival', () => { @@ -267,10 +190,7 @@ describe('closed-session capture handles', () => { test('a clean shutdown reports no leak', () => { const snapshot = evaluateDaemonLeaks( - observe({ - processes: [proc({ pid: 700, command: 'unrelated' })], - stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')], - }), + observe({ stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')] }), ); expect(hasDaemonLeaks(snapshot)).toBe(false); diff --git a/test/integration/support/daemon-leak-model.ts b/test/integration/support/daemon-leak-model.ts index a74c17bda9..6a94bb2e15 100644 --- a/test/integration/support/daemon-leak-model.ts +++ b/test/integration/support/daemon-leak-model.ts @@ -1,40 +1,10 @@ -// Daemon leak rules (#1781 B1, feeds #1431): given one observation of the host -// process table and of an isolated state dir, decide what the daemon still owns. -// Pure — `daemon-leak-oracle.ts` gathers the observation and asserts on it, and -// `daemon-leak-model.test.ts` pins these rules against fixture observations, -// including the #1109 and #1324 leak shapes. +// Daemon leak rules (#1781 B1, feeds #1431): given one observation of an +// isolated state dir and the liveness of the daemons that owned it, decide +// whether the daemon left behind anything it had recorded as its own. Pure — +// `daemon-leak-oracle.ts` gathers the observation and asserts on it, and +// `daemon-leak-model.test.ts` pins these rules. // -// Ownership is explicit and conservative. Global process counts are not -// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this -// daemon does not. A host process is daemon-owned when ANY rule holds: -// -// descendant its PPID chain reaches a daemon pid, or any process already -// owned below (observable while that ancestor lives; orphans -// reparent to launchd/init). -// process-group its PGID equals a daemon pid. The CLI launches the daemon -// detached (setsid), so the daemon is its own group leader and -// every non-detached runCmdBackground child stays in that group -// after reparenting. This is the #1324 signature: `simctl io … -// recordVideo` with PPID 1 and PGID = the dead daemon's pid. -// A pgid is reserved only while the group has a live member, so -// on a long-lived host pid reuse can eventually hand the number -// to an unrelated process; the lanes' state dirs are fresh per -// run, and every red-proof cross-checks the command line. -// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. -// The CLI starts the daemon with that variable and children -// inherit it, including setsid'd grandchildren the pgid rule -// misses (agent-browser's own daemon → Chrome fleet: #1109). -// macOS hides the environment of Apple platform binaries -// (`ps -E` shows it for node/Chrome, not for /bin/sleep or -// simctl), so Apple tooling is caught by the pgid/argv rules. -// state-dir-argv its command line contains the state dir path as a whole path -// token (recorders writing session artifacts, browsers pinned -// to a managed home). -// -// The lanes use a fresh mkdtemp state dir per run, so the env/argv rules cannot -// match a foreign process. The caller excludes itself and its own ancestors. -// -// Two further leak classes, independent of owned children: +// Two leak classes, both keyed on a record the daemon itself writes: // // surviving daemon at `after-shutdown` a daemon pid that is still alive IS // the leak. `stopProcessForTakeover` is best-effort and @@ -46,31 +16,25 @@ // matcher): `*.tmp` write-then-publish temporaries are torn // publishes, an empty directory is an unswept session // scaffold, daemon.json/daemon.lock may exist only while a -// daemon legitimately lives, and a capture descriptor still -// `lifecycle: "open"` (or a legacy `app-log.pid` marker) -// after shutdown is a capture handle that outlived its -// owner — a `completed` descriptor is the finish record -// ADR 0019 keeps. `tools/` holds managed third-party -// installs (agent-browser and its Chrome), whose own -// download temporaries and scaffolding this daemon neither -// writes nor owns. -import { expandProcessTree, uniquePositivePids } from '../../../src/utils/host-process.ts'; - -const COMMAND_PREVIEW_CHARS = 160; +// daemon legitimately lives, and a capture handle whose +// owning session is gone must be finalized — a descriptor +// still `lifecycle: "open"`, or a legacy `app-log.pid` +// marker, outlived its owner, while a `completed` +// descriptor is the finish record ADR 0019 keeps. +// `tools/` holds managed third-party installs +// (agent-browser and its Chrome), whose own download +// temporaries and scaffolding this daemon neither writes +// nor owns. +// +// Daemon-owned child processes are the third leak class, and the only one with +// no such record: nothing persists what a daemon spawned, so ownership has to be +// reconstructed from the OS. That arm lives in the manual +// `daemon-owned-process-probe.ts` — it is what produced the #1109 and #1324 +// evidence, and it returns here once the daemon records owned pids. +import { uniquePositivePids } from '../../../src/utils/host-process.ts'; export type DaemonLeakPhase = 'after-close' | 'after-shutdown'; -export type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; - -export type HostProcess = { - pid: number; - ppid: number; - pgid: number; - command: string; - /** Environment as a single string; empty when the host hides it. */ - env: string; -}; - /** One state-dir path, pre-read so the rules stay free of filesystem access. */ export type StateEntry = { /** Relative to the state dir, `/`-separated; directories keep a trailing `/`. */ @@ -80,14 +44,6 @@ export type StateEntry = { descriptorLifecycle?: string; }; -export type OwnedProcess = { - pid: number; - ppid: number; - pgid: number; - command: string; - reasons: OwnershipReason[]; -}; - export type NonEmpty = readonly [T, ...T[]]; /** @@ -106,9 +62,6 @@ export type DaemonLeakObservationBase = { stateDir: string; daemonPids: readonly number[]; livePids: readonly number[]; - processes: readonly HostProcess[]; - /** Pids never treated as owned: the observer and its own ancestors. */ - excludedPids: readonly number[]; stateEntries: readonly StateEntry[]; }; @@ -119,7 +72,6 @@ export type DaemonLeakSnapshot = { daemonPids: number[]; phase: DaemonLeakPhase; liveDaemonPids: number[]; - ownedProcesses: OwnedProcess[]; strayStateEntries: string[]; }; @@ -141,26 +93,24 @@ const EXPECTED_STATE_DIR_ENTRIES: readonly RegExp[] = [ const CAPTURE_DESCRIPTOR_ENTRY = /^sessions\/[^/]+\/[^/]+\.resource\.json$/; const LEGACY_APP_LOG_MARKER_ENTRY = /^sessions\/[^/]+\/app-log\.pid$/; const DAEMON_LIVENESS_ENTRY = /^daemon\.(?:json|lock)$/; +const SESSION_DIRECTORY = /^sessions\/([^/]+)\//; export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonLeakSnapshot { const daemonPids = uniquePositivePids(observation.daemonPids); const liveDaemonPids = daemonPids.filter((pid) => observation.livePids.includes(pid)); - const daemonLegitimatelyAlive = observation.phase === 'after-close' && liveDaemonPids.length > 0; + const context: StateEntryContext = { + phase: observation.phase, + // Only a session close leaves a daemon legitimately running. + daemonLegitimatelyAlive: observation.phase === 'after-close' && liveDaemonPids.length > 0, + closedSessions: observation.phase === 'after-close' ? observation.closedSessions : [], + }; return { stateDir: observation.stateDir, daemonPids, phase: observation.phase, liveDaemonPids, - ownedProcesses: findOwnedProcesses(observation, daemonPids), strayStateEntries: observation.stateEntries - .filter( - (entry) => - classifyStateEntry(entry, { - phase: observation.phase, - daemonLegitimatelyAlive, - closedSessions: closedSessionsOf(observation), - }) === 'stray', - ) + .filter((entry) => classifyStateEntry(entry, context) === 'stray') .map((entry) => entry.path) .sort(), }; @@ -168,68 +118,21 @@ export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonL /** * A daemon that outlived its own shutdown is itself the leak — not a detail of - * the report — so it fails alongside owned children and state-dir residue. + * the report — so it fails alongside state-dir residue. */ export function hasDaemonLeaks(snapshot: DaemonLeakSnapshot): boolean { - return ( - survivingDaemonPids(snapshot).length > 0 || - snapshot.ownedProcesses.length > 0 || - snapshot.strayStateEntries.length > 0 - ); -} - -function survivingDaemonPids(snapshot: DaemonLeakSnapshot): number[] { - return snapshot.phase === 'after-shutdown' ? snapshot.liveDaemonPids : []; -} - -function findOwnedProcesses( - observation: DaemonLeakObservation, - daemonPids: readonly number[], -): OwnedProcess[] { - const excluded = new Set([...observation.excludedPids, ...daemonPids]); - const matchers = stateDirMatchers(observation.stateDir); - const reasonsByPid = new Map(); - for (const proc of observation.processes) { - if (excluded.has(proc.pid)) continue; - const reasons = directOwnershipReasons(proc, daemonPids, matchers); - if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); - } - // Ownership propagates down the tree: a child of the daemon, or of any process - // the rules above own, is owned (a detached `(simctl)` grandchild of a leaked - // recorder shim, DTServiceHub under a runner xcodebuild, …). - const roots = [...daemonPids, ...reasonsByPid.keys()]; - for (const proc of expandProcessTree(roots, observation.processes)) { - if (excluded.has(proc.pid) || roots.includes(proc.pid)) continue; - reasonsByPid.set(proc.pid, [...(reasonsByPid.get(proc.pid) ?? []), 'descendant']); - } - return observation.processes.flatMap((proc) => { - const reasons = reasonsByPid.get(proc.pid); - return reasons - ? [{ pid: proc.pid, ppid: proc.ppid, pgid: proc.pgid, command: proc.command, reasons }] - : []; - }); -} - -type StateDirMatchers = { argv: RegExp; env: RegExp }; - -function stateDirMatchers(stateDir: string): StateDirMatchers { - // Whole-path matches only: `` as a token or `/…`, never `-sibling`. - return { - argv: new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`), - env: new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`), - }; + return survivingDaemonPids(snapshot).length > 0 || snapshot.strayStateEntries.length > 0; } -function directOwnershipReasons( - proc: HostProcess, - daemonPids: readonly number[], - matchers: StateDirMatchers, -): OwnershipReason[] { - const reasons: OwnershipReason[] = []; - if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); - if (matchers.env.test(proc.env)) reasons.push('state-dir-env'); - if (matchers.argv.test(proc.command)) reasons.push('state-dir-argv'); - return reasons; +export function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { + return [ + `daemon leak oracle: ${hasDaemonLeaks(snapshot) ? 'LEAK' : 'clean'} (${snapshot.phase})`, + ` state dir: ${snapshot.stateDir}`, + ` daemon pids: ${formatPids(snapshot.daemonPids)}; live: ${formatPids(snapshot.liveDaemonPids)}`, + ` daemons that outlived shutdown: ${formatPids(survivingDaemonPids(snapshot))}`, + ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, + ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), + ].join('\n'); } type StateEntryContext = { @@ -238,6 +141,10 @@ type StateEntryContext = { closedSessions: readonly string[]; }; +function survivingDaemonPids(snapshot: DaemonLeakSnapshot): number[] { + return snapshot.phase === 'after-shutdown' ? snapshot.liveDaemonPids : []; +} + function classifyStateEntry(entry: StateEntry, context: StateEntryContext): 'expected' | 'stray' { if (MANAGED_TOOLS_ENTRY.test(entry.path)) return 'expected'; if (entry.kind === 'empty-directory') return 'stray'; @@ -260,56 +167,12 @@ function classifyStateEntry(entry: StateEntry, context: StateEntryContext): 'exp function classifyCaptureEntry(entry: StateEntry, context: StateEntryContext): 'expected' | 'stray' { const owningSessionGone = context.phase === 'after-shutdown' || - context.closedSessions.includes(sessionDirectoryOf(entry.path) ?? ''); + context.closedSessions.includes(SESSION_DIRECTORY.exec(entry.path)?.[1] ?? ''); if (!owningSessionGone) return 'expected'; if (!CAPTURE_DESCRIPTOR_ENTRY.test(entry.path)) return 'stray'; return entry.descriptorLifecycle === 'completed' ? 'expected' : 'stray'; } -function closedSessionsOf(observation: DaemonLeakObservation): readonly string[] { - return observation.phase === 'after-close' ? observation.closedSessions : []; -} - -function sessionDirectoryOf(entryPath: string): string | undefined { - return /^sessions\/([^/]+)\//.exec(entryPath)?.[1]; -} - -export function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { - const surviving = survivingDaemonPids(snapshot); - return [ - `daemon leak oracle: ${hasDaemonLeaks(snapshot) ? 'LEAK' : 'clean'} (${snapshot.phase})`, - ` state dir: ${snapshot.stateDir}`, - ` daemon pids: ${formatPids(snapshot.daemonPids)}; live: ${formatPids(snapshot.liveDaemonPids)}`, - ` daemons that outlived shutdown: ${surviving.length}${ - surviving.length > 0 ? ` (${formatPids(surviving)})` : '' - }`, - ` owned processes still alive: ${snapshot.ownedProcesses.length}`, - ...snapshot.ownedProcesses.map( - (proc) => - ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${previewCommand( - proc.command, - snapshot.stateDir, - )}`, - ), - ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, - ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), - ].join('\n'); -} - -function previewCommand(command: string, stateDir: string): string { - const masked = command.replace( - new RegExp(`${escapeRegExp(stateDir)}(?=[\\s/]|$)`, 'g'), - '', - ); - return masked.length > COMMAND_PREVIEW_CHARS - ? `${masked.slice(0, COMMAND_PREVIEW_CHARS)}…` - : masked; -} - function formatPids(pids: readonly number[]): string { return pids.join(', ') || '(none)'; } - -function escapeRegExp(value: string): string { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts index ce028d2dac..f0d6efa280 100644 --- a/test/integration/support/daemon-leak-oracle.ts +++ b/test/integration/support/daemon-leak-oracle.ts @@ -1,17 +1,12 @@ -// Daemon leak oracle (#1781 B1, feeds #1431): observes the host process table -// and an isolated state dir, then applies the ownership/residue rules in +// Daemon leak oracle (#1781 B1, feeds #1431): observes an isolated state dir and +// the liveness of the daemons that owned it, then applies the rules in // `daemon-leak-model.ts` (which documents them and is where they are tested). -// -// What each caller actually exercises depends on what its daemon did. The three -// real-subprocess daemon lanes run `session list`/`close` with no device, -// simulator or browser, so their daemons own no children and the process arm -// asserts an empty set; what they guard is the surviving-daemon check and the -// state-dir residue. The #1109/#1324 leak shapes are guarded by the model's -// fixture test and reproduced by hand against the pre-fix commits — see the PR. +// Daemon-owned child processes are reconstructed by the manual +// `daemon-owned-process-probe.ts` instead — see its header for why that arm +// cannot assert anything from a device-free lane. import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { runCmd } from '../../../src/utils/exec.ts'; import { isProcessAlive } from '../../../src/utils/host-process.ts'; import { AppError } from '@agent-device/kernel/errors'; import { @@ -20,27 +15,11 @@ import { hasDaemonLeaks, type DaemonLeakPhaseSelection, type DaemonLeakSnapshot, - type HostProcess, type StateEntry, } from './daemon-leak-model.ts'; -const PS_TIMEOUT_MS = 5_000; const DEFAULT_SETTLE_MS = 5_000; const SETTLE_POLL_MS = 250; -// Matches src/utils/host-process.ts: an absolute path so a PATH-resolved -// third-party `ps` (Homebrew, procps) cannot change the flag semantics. -const HOST_PS_COMMAND = process.platform === 'win32' ? 'ps' : '/bin/ps'; - -/** - * `phase` carries the identity that phase needs: `after-close` cannot be - * requested without naming at least one closed session (see - * DaemonLeakPhaseSelection), so the vacuous checkpoint that accepts every open - * capture handle is not expressible. `after-shutdown` takes nothing extra. - * - * Closed sessions are directory names, not paths — the basename of - * `sessionStore.resolveSessionDir(name)`, or of the `sessionStateDir` an - * `open`/`close` response reports. - */ export type DaemonLeakOracleOptions = DaemonLeakPhaseSelection & { stateDir: string; /** Every daemon pid the lane observed for this state dir (from daemon.json). */ @@ -56,18 +35,13 @@ export type DaemonLeakOracleOptions = DaemonLeakPhaseSelection & { settleMs?: number; }; -async function captureDaemonLeakSnapshot( - options: DaemonLeakOracleOptions, -): Promise { +function captureDaemonLeakSnapshot(options: DaemonLeakOracleOptions): DaemonLeakSnapshot { const stateDir = path.resolve(options.stateDir); - const processes = await listHostProcessesWithGroups(); return evaluateDaemonLeaks({ ...phaseSelectionOf(options), stateDir, daemonPids: options.daemonPids, livePids: options.daemonPids.filter((pid) => isProcessAlive(pid)), - processes, - excludedPids: [...ancestorsOf(process.pid, processes)], stateEntries: readStateEntries(stateDir, options.sessionsDir), }); } @@ -80,10 +54,10 @@ async function settleDaemonLeakSnapshot( options: DaemonLeakOracleOptions, ): Promise { const deadline = Date.now() + (options.settleMs ?? DEFAULT_SETTLE_MS); - let snapshot = await captureDaemonLeakSnapshot(options); + let snapshot = captureDaemonLeakSnapshot(options); while (hasDaemonLeaks(snapshot) && Date.now() < deadline) { await new Promise((resolve) => setTimeout(resolve, SETTLE_POLL_MS)); - snapshot = await captureDaemonLeakSnapshot(options); + snapshot = captureDaemonLeakSnapshot(options); } return snapshot; } @@ -152,67 +126,6 @@ function readDescriptorLifecycle(filePath: string): string | undefined { } } -// src/utils/host-process.ts's listHostProcesses covers pid/ppid/command; the -// ownership rules also need the process group and the environment, which need a -// second `ps` on macOS (`-E` appends the environment to the command column) and -// /proc on Linux. -async function listHostProcessesWithGroups(): Promise { - const darwin = process.platform === 'darwin'; - const [tree, darwinEnv] = await Promise.all([ - runPs(['-axww', '-o', 'pid=,ppid=,pgid=,command=']), - darwin ? runPs(['-E', '-axww', '-o', 'pid=,command=']) : Promise.resolve(''), - ]); - const envByPid = new Map( - parsePsLines(darwinEnv, /^\s*(\d+)\s+(.*)$/).map(([pid, env]) => [Number(pid), env ?? '']), - ); - return parsePsLines(tree, /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/).map( - ([pid, ppid, pgid, command]) => ({ - pid: Number(pid), - ppid: Number(ppid), - pgid: Number(pgid), - command: command ?? '', - env: darwin ? (envByPid.get(Number(pid)) ?? '') : readLinuxEnviron(Number(pid)), - }), - ); -} - -function parsePsLines(stdout: string, shape: RegExp): string[][] { - return stdout.split('\n').flatMap((line) => { - const match = shape.exec(line); - return match ? [match.slice(1)] : []; - }); -} - -async function runPs(args: string[]): Promise { - const result = await runCmd(HOST_PS_COMMAND, args, { - allowFailure: true, - timeoutMs: PS_TIMEOUT_MS, - }); - if (result.exitCode !== 0) { - throw new Error( - `daemon leak oracle: ps ${args.join(' ')} failed (${result.exitCode}): ${result.stderr}`, - ); - } - return result.stdout; -} - -function readLinuxEnviron(pid: number): string { - try { - return fs.readFileSync(`/proc/${pid}/environ`, 'latin1').replaceAll('\0', ' '); - } catch { - return ''; - } -} - -function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set { - const byPid = new Map(processes.map((proc) => [proc.pid, proc])); - const chain = new Set(); - for (let cursor = pid; cursor > 0 && !chain.has(cursor); cursor = byPid.get(cursor)?.ppid ?? 0) { - chain.add(cursor); - } - return chain; -} - /** * Parses the standalone CLI's argv into oracle options, refusing the invocation * the type system already refuses in code: `--phase after-close` without a diff --git a/test/integration/support/daemon-owned-process-probe.test.ts b/test/integration/support/daemon-owned-process-probe.test.ts new file mode 100644 index 0000000000..077618fea2 --- /dev/null +++ b/test/integration/support/daemon-owned-process-probe.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, test } from 'vitest'; +import { findOwnedProcesses, type HostProcess } from './daemon-owned-process-probe.ts'; + +// The probe is manual — no lane can run it, because a device-free daemon owns no +// children (see its header). These fixtures are the real `ps` rows captured +// during the #1109 and #1324 red-proofs (SHAs in the #1781 B1 PR), so a regex or +// ordering edit that stops catching either leak fails here rather than silently +// weakening the next hand-run proof. +const STATE_DIR = '/tmp/agent-device-lane-abc'; +const DAEMON_PID = 4340; + +function proc(overrides: Partial & Pick): HostProcess { + return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides }; +} + +const owned = (processes: HostProcess[]) => findOwnedProcesses(processes, [DAEMON_PID], STATE_DIR); + +describe('daemon-owned process rules', () => { + // #1324: `simctl io … recordVideo` reparents to launchd (ppid 1) but keeps the + // dead daemon's process group, and simctl only finalizes the mp4 on SIGINT. + test('flags a recorder orphaned into the dead daemon process group', () => { + const recorder = proc({ + pid: 52420, + ppid: 1, + pgid: DAEMON_PID, + command: '/…/simctl io 416440AE recordVideo /tmp/out.mp4', + }); + + expect(owned([recorder])).toEqual([ + expect.objectContaining({ pid: 52420, reasons: ['process-group'] }), + ]); + }); + + // #1109: the agent-browser daemon setsids away from the daemon's group, so + // only the inherited state-dir environment and its argv identify the fleet. + test('flags an agent-browser fleet by inherited state dir, including its children', () => { + const browserDaemon = proc({ + pid: 47515, + pgid: 47515, + command: `${STATE_DIR}/tools/agent-browser/0.27.1/package/…/agent-browser-darwin-arm64`, + env: `HOME=/tmp AGENT_DEVICE_STATE_DIR=${STATE_DIR}`, + }); + const chrome = proc({ + pid: 47586, + ppid: 47515, + pgid: 47586, + command: 'Google Chrome for Testing', + }); + const renderer = proc({ + pid: 47953, + ppid: 47586, + pgid: 47586, + command: 'Chrome Helper (Renderer)', + }); + const matches = owned([browserDaemon, chrome, renderer]); + + expect(matches.map((match) => match.pid)).toEqual([47515, 47586, 47953]); + expect(matches[0]?.reasons).toEqual(['state-dir-env', 'state-dir-argv']); + // The fleet below the matched root is owned transitively, not by its own argv. + expect(matches[1]?.reasons).toEqual(['descendant']); + }); + + test('ignores foreign processes, a neighbouring state dir, and the daemon itself', () => { + const simulator = proc({ pid: 700, command: '/…/CoreSimulator … SimulatorTrampoline' }); + const neighbour = proc({ + pid: 701, + command: `node --state-dir ${STATE_DIR}-other/daemon.ts`, + env: `AGENT_DEVICE_STATE_DIR=${STATE_DIR}-other`, + }); + const daemon = proc({ pid: DAEMON_PID, pgid: DAEMON_PID }); + + expect(owned([simulator, neighbour, daemon])).toEqual([]); + }); +}); diff --git a/test/integration/support/daemon-owned-process-probe.ts b/test/integration/support/daemon-owned-process-probe.ts new file mode 100644 index 0000000000..c1895b9c5e --- /dev/null +++ b/test/integration/support/daemon-owned-process-probe.ts @@ -0,0 +1,218 @@ +// Manual red-proof probe for daemon-owned child processes (#1781 B1, feeds +// #1431). Run by hand against a daemon that has just been stopped; it is what +// produced the #1109 and #1324 evidence in the PR. +// +// It is deliberately NOT wired into a lane. Nothing records what a daemon +// spawned, so ownership has to be reconstructed from the OS, and the three +// real-subprocess daemon lanes run `session list`/`close` with no device, +// simulator, or browser — their daemons own no children, so a lane assertion +// here would only ever compare an empty set to an empty set. The shipped +// oracle (`daemon-leak-oracle.ts`) asserts what the daemon does record: an +// unfinalized capture handle, state-dir residue, and its own survival. Once the +// daemon records owned child pids the way it records captures, this probe +// collapses into "read the record, assert they are dead" and moves back into +// the oracle. +// +// Ownership is explicit and conservative. Global process counts are not +// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this +// daemon does not. A host process is daemon-owned when ANY rule holds: +// +// descendant its PPID chain reaches a daemon pid, or any process already +// owned below (observable while that ancestor lives; orphans +// reparent to launchd/init). +// process-group its PGID equals a daemon pid. The CLI launches the daemon +// detached (setsid), so the daemon is its own group leader and +// every non-detached runCmdBackground child stays in that group +// after reparenting. This is the #1324 signature: `simctl io … +// recordVideo` with PPID 1 and PGID = the dead daemon's pid. +// A pgid is reserved only while the group has a live member, so +// on a long-lived host pid reuse can eventually hand the number +// to an unrelated process; cross-check the command line. +// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. +// The CLI starts the daemon with that variable and children +// inherit it, including setsid'd grandchildren the pgid rule +// misses (agent-browser's own daemon → Chrome fleet: #1109). +// macOS hides the environment of Apple platform binaries +// (`ps -E` shows it for node/Chrome, not for /bin/sleep or +// simctl), so Apple tooling is caught by the pgid/argv rules. +// state-dir-argv its command line contains the state dir path as a whole path +// token (recorders writing session artifacts, browsers pinned +// to a managed home). +// +// Use a fresh state dir per run, so the env/argv rules cannot match a foreign +// process. The probe excludes itself and its own ancestors. +// +// The rules are pinned by `daemon-owned-process-probe.test.ts` against the real +// `ps` rows captured during those proofs, so a regex or ordering edit that stops +// catching either leak fails in CI even though no lane can run this probe. +// +// Usage (prints the report; exits 1 when anything owned is still alive): +// node --experimental-strip-types test/integration/support/daemon-owned-process-probe.ts \ +// --state-dir --daemon-pid [--daemon-pid …] +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { runCmd } from '../../../src/utils/exec.ts'; +import { expandProcessTree, uniquePositivePids } from '../../../src/utils/host-process.ts'; + +const PS_TIMEOUT_MS = 5_000; +const COMMAND_PREVIEW_CHARS = 160; +// Matches src/utils/host-process.ts: an absolute path so a PATH-resolved +// third-party `ps` (Homebrew, procps) cannot change the flag semantics. +const HOST_PS_COMMAND = process.platform === 'win32' ? 'ps' : '/bin/ps'; + +export type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; + +export type HostProcess = { + pid: number; + ppid: number; + pgid: number; + command: string; + /** Environment as a single string; empty when the host hides it. */ + env: string; +}; + +export type OwnedProcess = HostProcess & { reasons: OwnershipReason[] }; + +export function findOwnedProcesses( + processes: readonly HostProcess[], + daemonPids: readonly number[], + stateDir: string, +): OwnedProcess[] { + const excluded = new Set([...ancestorsOf(process.pid, processes), ...daemonPids]); + const matchers = stateDirMatchers(stateDir); + const reasonsByPid = new Map(); + for (const proc of processes) { + if (excluded.has(proc.pid)) continue; + const reasons = directOwnershipReasons(proc, daemonPids, matchers); + if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); + } + // Ownership propagates down the tree: a child of the daemon, or of any process + // the rules above own, is owned (a detached `(simctl)` grandchild of a leaked + // recorder shim, DTServiceHub under a runner xcodebuild, …). + const roots = [...daemonPids, ...reasonsByPid.keys()]; + for (const proc of expandProcessTree(roots, processes)) { + if (excluded.has(proc.pid) || roots.includes(proc.pid)) continue; + reasonsByPid.set(proc.pid, [...(reasonsByPid.get(proc.pid) ?? []), 'descendant']); + } + return processes.flatMap((proc) => { + const reasons = reasonsByPid.get(proc.pid); + return reasons ? [{ ...proc, reasons }] : []; + }); +} + +function stateDirMatchers(stateDir: string): { argv: RegExp; env: RegExp } { + // Whole-path matches only: `` as a token or `/…`, never `-sibling`. + return { + argv: new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`), + env: new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`), + }; +} + +function directOwnershipReasons( + proc: HostProcess, + daemonPids: readonly number[], + matchers: { argv: RegExp; env: RegExp }, +): OwnershipReason[] { + const reasons: OwnershipReason[] = []; + if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); + if (matchers.env.test(proc.env)) reasons.push('state-dir-env'); + if (matchers.argv.test(proc.command)) reasons.push('state-dir-argv'); + return reasons; +} + +function formatReport(owned: readonly OwnedProcess[], stateDir: string, pids: number[]): string { + const mask = new RegExp(`${escapeRegExp(stateDir)}(?=[\\s/]|$)`, 'g'); + return [ + `daemon-owned processes: ${owned.length > 0 ? 'LEAK' : 'clean'}`, + ` state dir: ${stateDir}`, + ` daemon pids: ${pids.join(', ') || '(none)'}`, + ...owned.map((proc) => { + const command = proc.command.replace(mask, ''); + return ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${ + command.length > COMMAND_PREVIEW_CHARS + ? `${command.slice(0, COMMAND_PREVIEW_CHARS)}…` + : command + }`; + }), + ].join('\n'); +} + +// pid/ppid/pgid plus the command line, and separately the environment so the +// argv and env rules stay distinct. macOS `ps -E` appends the environment to +// the command column for same-user processes; Linux exposes it in /proc. +async function listHostProcesses(): Promise { + const darwin = process.platform === 'darwin'; + const [tree, darwinEnv] = await Promise.all([ + runPs(['-axww', '-o', 'pid=,ppid=,pgid=,command=']), + darwin ? runPs(['-E', '-axww', '-o', 'pid=,command=']) : Promise.resolve(''), + ]); + const envByPid = new Map( + parsePsLines(darwinEnv, /^\s*(\d+)\s+(.*)$/).map(([pid, env]) => [Number(pid), env ?? '']), + ); + return parsePsLines(tree, /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/).map( + ([pid, ppid, pgid, command]) => ({ + pid: Number(pid), + ppid: Number(ppid), + pgid: Number(pgid), + command: command ?? '', + env: darwin ? (envByPid.get(Number(pid)) ?? '') : readLinuxEnviron(Number(pid)), + }), + ); +} + +function parsePsLines(stdout: string, shape: RegExp): string[][] { + return stdout.split('\n').flatMap((line) => { + const match = shape.exec(line); + return match ? [match.slice(1)] : []; + }); +} + +async function runPs(args: string[]): Promise { + const result = await runCmd(HOST_PS_COMMAND, args, { + allowFailure: true, + timeoutMs: PS_TIMEOUT_MS, + }); + if (result.exitCode !== 0) { + throw new Error(`ps ${args.join(' ')} failed (${result.exitCode}): ${result.stderr}`); + } + return result.stdout; +} + +function readLinuxEnviron(pid: number): string { + try { + return fs.readFileSync(`/proc/${pid}/environ`, 'latin1').replaceAll('\0', ' '); + } catch { + return ''; + } +} + +function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set { + const parentByPid = new Map(processes.map((proc) => [proc.pid, proc.ppid])); + const chain = new Set(); + let cursor: number | undefined = pid; + while (cursor && !chain.has(cursor)) { + chain.add(cursor); + cursor = parentByPid.get(cursor); + } + return chain; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +// Only when run directly: importing this module (its fixture test) must not +// execute the probe. +if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { + const args = process.argv.slice(2); + const readFlag = (name: string): string[] => + args.flatMap((arg, index) => (arg === name && args[index + 1] ? [args[index + 1]!] : [])); + const stateDir = readFlag('--state-dir')[0]; + if (!stateDir) throw new Error('daemon-owned process probe: --state-dir is required'); + const daemonPids = uniquePositivePids(readFlag('--daemon-pid').map(Number)); + const resolvedStateDir = path.resolve(stateDir); + const owned = findOwnedProcesses(await listHostProcesses(), daemonPids, resolvedStateDir); + process.stdout.write(`${formatReport(owned, resolvedStateDir, daemonPids)}\n`); + process.exitCode = owned.length > 0 ? 1 : 0; +} diff --git a/vitest.config.ts b/vitest.config.ts index f80dc46c5d..293cf26877 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -95,6 +95,10 @@ export default defineConfig({ // oracle are device-free, so this is where the #1109/#1324 leak // shapes are actually guarded. 'test/integration/support/daemon-leak-model.test.ts', + // Same for the manual owned-process probe's rules: no lane can run the + // probe (a device-free daemon owns no children), so its fixtures are + // the only CI guard on the #1109/#1324 ownership shapes. + 'test/integration/support/daemon-owned-process-probe.test.ts', // The frozen replay-compat corpus (#1417): parse-only, no device or // subprocess work, so it belongs in the fast lane next to the // grammar it guards. From 6a8f84baf1cbeea1252cc8a7e518efd370b8a6a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 17:08:15 +0200 Subject: [PATCH 6/8] test: guard the daemon-owned-process arm in CI on the live web lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of ce9f0eeb: 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 3a5b9bef3) 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 --- .fallowrc.json | 4 - .../daemon-replace-exit-flush.test.ts | 7 +- test/integration/smoke-web-platform.test.ts | 56 ++++- .../support/daemon-leak-model.test.ts | 88 ++++++- test/integration/support/daemon-leak-model.ts | 229 ++++++++++++++---- .../integration/support/daemon-leak-oracle.ts | 103 +++++++- .../daemon-owned-process-probe.test.ts | 74 ------ .../support/daemon-owned-process-probe.ts | 218 ----------------- vitest.config.ts | 4 - 9 files changed, 422 insertions(+), 361 deletions(-) delete mode 100644 test/integration/support/daemon-owned-process-probe.test.ts delete mode 100644 test/integration/support/daemon-owned-process-probe.ts diff --git a/.fallowrc.json b/.fallowrc.json index 955ceff188..ed1e6fbff3 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -24,10 +24,6 @@ // subprocess (test/integration/daemon-replace-exit-flush.test.ts), so // dependency analysis cannot follow the runCmdSync string path to it. "test/integration/support/exit-after-flush.ts", - // #1781 B1: the manual red-proof probe for daemon-owned child processes. It - // is run by hand (and produced the #1109/#1324 evidence), so nothing imports - // it; see its header for why that arm cannot assert from a device-free lane. - "test/integration/support/daemon-owned-process-probe.ts", "src/utils/update-check-entry.ts", "examples/sdk/client-session.ts", "examples/sdk/metro-runtime.ts", diff --git a/test/integration/daemon-replace-exit-flush.test.ts b/test/integration/daemon-replace-exit-flush.test.ts index f3fc94b866..b3009a4085 100644 --- a/test/integration/daemon-replace-exit-flush.test.ts +++ b/test/integration/daemon-replace-exit-flush.test.ts @@ -78,10 +78,13 @@ test('daemon replace mid-command returns a structured, parseable error and exits killTimeoutMs: 1_500, expectedStartTime: info.processStartTime, }); - info = null; // #1781 B1: neither the SIGKILLed daemon nor its replacement may leave owned - // processes or unclassified state-dir residue once both are gone. + // processes or unclassified state-dir residue once both are gone. `info` + // stays set until this passes: `stopProcessForTakeover` is best-effort, so a + // failed stop must still reach the `finally` retry below rather than have + // the state dir removed out from under a daemon that is still running. await assertNoDaemonLeaks({ stateDir, daemonPids, phase: 'after-shutdown' }); + info = null; } finally { if (info) { await stopProcessForTakeover(info.pid, { diff --git a/test/integration/smoke-web-platform.test.ts b/test/integration/smoke-web-platform.test.ts index aabaccf030..a59288af3e 100644 --- a/test/integration/smoke-web-platform.test.ts +++ b/test/integration/smoke-web-platform.test.ts @@ -1,10 +1,12 @@ import assert from 'node:assert/strict'; -import { mkdirSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { createServer, type Server } from 'node:http'; import path from 'node:path'; import test from 'node:test'; +import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts'; import { type CliJsonResult, formatResultDebug, runBuiltCliJson } from './cli-json.ts'; import { assertPngDimensions, assertPngFile } from './provider-scenarios/assertions.ts'; +import { assertNoDaemonLeaks } from './support/daemon-leak-oracle.ts'; const TEST_NAME = 'live web platform e2e smoke'; const WEB_E2E_ENABLED = process.env.AGENT_DEVICE_WEB_E2E === '1'; @@ -25,10 +27,21 @@ type WebSmokeContext = { lastSnapshot?: any; screenshotPath: string; server: Server; + stateDir: string; stepHistory: StepRecord[]; url: string; }; +// #1781 B1 / #1109: this is the one lane whose daemon owns real child processes +// — the managed agent-browser daemon and its Chrome fleet — so it is where the +// leak oracle's owned-process arm can actually fail. #1109's acceptance is that +// a stopped daemon leaves zero agent-browser processes behind within the idle +// window; the lane pins AGENT_BROWSER_IDLE_TIMEOUT_MS at 30s, so the settle +// window here has to outlast it. Graceful daemon shutdown does not close web +// sessions (#1868), so the fleet legitimately lives until that window elapses — +// which is why the checkpoint waits it out rather than asserting immediately. +const WEB_LEAK_SETTLE_MS = 90_000; + test( TEST_NAME, { @@ -90,6 +103,7 @@ async function createWebSmokeContext(): Promise { env, screenshotPath: path.join(artifactDir, 'web-smoke.png'), server: fixture.server, + stateDir, stepHistory: [], url: fixture.url, }; @@ -237,6 +251,11 @@ async function cleanupWebSmoke(context: WebSmokeContext, opened: boolean): Promi errors.push(error); } } + try { + await assertOrphanedWebSessionLeavesNothingOwned(context); + } catch (error) { + errors.push(error); + } try { await closeServer(context.server); } catch (error) { @@ -250,6 +269,41 @@ async function cleanupWebSmoke(context: WebSmokeContext, opened: boolean): Promi } } +// #1109's acceptance, as a lane assertion: a daemon that dies while a web +// session is still open must leave zero agent-browser processes behind within +// the idle window. That is the leak's real shape — an ordinary `close` reaps the +// fleet, so only an unclosed session can strand it — and it is the one route in +// CI where the oracle's owned-process arm has real children to find. +async function assertOrphanedWebSessionLeavesNothingOwned(context: WebSmokeContext): Promise { + await runStep(context, 'reopen for the orphan checkpoint', [ + 'open', + context.url, + ...context.common, + ]); + const infoPath = path.join(context.stateDir, 'daemon.json'); + // Never skip silently: no daemon metadata after a live web session means the + // checkpoint would certify a daemon it never observed. + assert.ok(existsSync(infoPath), `expected daemon metadata at ${infoPath}`); + const info = JSON.parse(readFileSync(infoPath, 'utf8')) as { + pid: number; + processStartTime?: string; + }; + assert.ok(Number.isInteger(info.pid) && info.pid > 0, `expected a daemon pid in ${infoPath}`); + // Stop the daemon with the session still open: the browser fleet is orphaned + // exactly as in #1109, and only its idle lifecycle can still reap it. + await stopProcessForTakeover(info.pid, { + termTimeoutMs: 5_000, + killTimeoutMs: 5_000, + expectedStartTime: info.processStartTime, + }); + await assertNoDaemonLeaks({ + stateDir: context.stateDir, + daemonPids: [info.pid], + phase: 'after-shutdown', + settleMs: WEB_LEAK_SETTLE_MS, + }); +} + function recordStep( context: WebSmokeContext, step: string, diff --git a/test/integration/support/daemon-leak-model.test.ts b/test/integration/support/daemon-leak-model.test.ts index e838d41af4..a3bdf236f4 100644 --- a/test/integration/support/daemon-leak-model.test.ts +++ b/test/integration/support/daemon-leak-model.test.ts @@ -6,12 +6,23 @@ import { type DaemonLeakObservation, type DaemonLeakObservationBase, type DaemonLeakPhaseSelection, + type HostProcess, type NonEmpty, type StateEntry, } from './daemon-leak-model.ts'; +// The lanes that call the oracle run device-free daemons, so the ownership rules +// would otherwise only ever see an empty process set. These fixtures are the +// real `ps` shapes captured during the #1109 and #1324 red-proofs (SHAs in the +// #1781 B1 PR), so a regex or ordering edit that stops catching either leak +// fails here instead of silently going quiet in CI. const STATE_DIR = '/tmp/agent-device-lane-abc'; const DAEMON_PID = 4340; +const OBSERVER_PID = 999; + +function proc(overrides: Partial & Pick): HostProcess { + return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides }; +} // The phase and its identity arrive together: `after-close` cannot be requested // without naming the sessions that closed, so these helpers cannot construct the @@ -24,6 +35,8 @@ function observe( stateDir: STATE_DIR, daemonPids: [DAEMON_PID], livePids: [], + processes: [], + excludedPids: [OBSERVER_PID], stateEntries: [], ...overrides, ...selection, @@ -44,6 +57,72 @@ function file(entryPath: string, descriptorLifecycle?: string): StateEntry { return { path: entryPath, kind: 'file', ...(descriptorLifecycle ? { descriptorLifecycle } : {}) }; } +describe('owned-process rules', () => { + // #1324: `simctl io … recordVideo` reparents to launchd (ppid 1) but keeps the + // dead daemon's process group, and simctl only finalizes the mp4 on SIGINT. + test('flags a recorder orphaned into the dead daemon process group', () => { + const recorder = proc({ + pid: 52420, + ppid: 1, + pgid: DAEMON_PID, + command: '/…/simctl io 416440AE recordVideo /tmp/out.mp4', + }); + const snapshot = evaluateDaemonLeaks(observe({ processes: [recorder] })); + + expect(snapshot.ownedProcesses).toEqual([ + expect.objectContaining({ pid: 52420, reasons: ['process-group'] }), + ]); + expect(hasDaemonLeaks(snapshot)).toBe(true); + }); + + // #1109: the agent-browser daemon setsids away from the daemon's group, so + // only the inherited state-dir environment and its argv identify the fleet. + test('flags an agent-browser fleet by inherited state dir, including its children', () => { + const browserDaemon = proc({ + pid: 47515, + ppid: 1, + pgid: 47515, + command: `${STATE_DIR}/tools/agent-browser/0.27.1/package/…/agent-browser-darwin-arm64`, + env: `HOME=/tmp AGENT_DEVICE_STATE_DIR=${STATE_DIR}`, + }); + const chrome = proc({ + pid: 47586, + ppid: 47515, + pgid: 47586, + command: 'Google Chrome for Testing', + }); + const renderer = proc({ + pid: 47953, + ppid: 47586, + pgid: 47586, + command: 'Chrome Helper (Renderer)', + }); + const snapshot = evaluateDaemonLeaks(observe({ processes: [browserDaemon, chrome, renderer] })); + + expect(snapshot.ownedProcesses.map((owned) => owned.pid)).toEqual([47515, 47586, 47953]); + expect(snapshot.ownedProcesses[0]?.reasons).toEqual(['state-dir-env', 'state-dir-argv']); + // The fleet below the matched root is owned transitively, not by its own argv. + expect(snapshot.ownedProcesses[1]?.reasons).toEqual(['descendant']); + }); + + test('ignores foreign processes, the observer chain, and the daemons themselves', () => { + const simulator = proc({ pid: 700, command: '/…/CoreSimulator … SimulatorTrampoline' }); + const neighbourStateDir = proc({ + pid: 701, + command: `node --state-dir ${STATE_DIR}-other/daemon.ts`, + env: `AGENT_DEVICE_STATE_DIR=${STATE_DIR}-other`, + }); + const observer = proc({ pid: OBSERVER_PID, pgid: DAEMON_PID }); + const daemon = proc({ pid: DAEMON_PID, pgid: DAEMON_PID }); + const snapshot = evaluateDaemonLeaks( + observe({ processes: [simulator, neighbourStateDir, observer, daemon] }), + ); + + expect(snapshot.ownedProcesses).toEqual([]); + expect(hasDaemonLeaks(snapshot)).toBe(false); + }); +}); + describe('surviving-daemon rule', () => { // stopProcessForTakeover is best-effort void: it returns silently on identity // mismatch, signal failure, or kill timeout, so a daemon can outlive the stop. @@ -54,9 +133,7 @@ describe('surviving-daemon rule', () => { expect(snapshot.liveDaemonPids).toEqual([DAEMON_PID]); expect(hasDaemonLeaks(snapshot)).toBe(true); - expect(formatDaemonLeakReport(snapshot)).toContain( - `daemons that outlived shutdown: ${DAEMON_PID}`, - ); + expect(formatDaemonLeakReport(snapshot)).toContain('daemons that outlived shutdown: 1'); }); test('its metadata files stay stray rather than being excused by its own survival', () => { @@ -190,7 +267,10 @@ describe('closed-session capture handles', () => { test('a clean shutdown reports no leak', () => { const snapshot = evaluateDaemonLeaks( - observe({ stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')] }), + observe({ + processes: [proc({ pid: 700, command: 'unrelated' })], + stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')], + }), ); expect(hasDaemonLeaks(snapshot)).toBe(false); diff --git a/test/integration/support/daemon-leak-model.ts b/test/integration/support/daemon-leak-model.ts index 6a94bb2e15..a74c17bda9 100644 --- a/test/integration/support/daemon-leak-model.ts +++ b/test/integration/support/daemon-leak-model.ts @@ -1,10 +1,40 @@ -// Daemon leak rules (#1781 B1, feeds #1431): given one observation of an -// isolated state dir and the liveness of the daemons that owned it, decide -// whether the daemon left behind anything it had recorded as its own. Pure — -// `daemon-leak-oracle.ts` gathers the observation and asserts on it, and -// `daemon-leak-model.test.ts` pins these rules. +// Daemon leak rules (#1781 B1, feeds #1431): given one observation of the host +// process table and of an isolated state dir, decide what the daemon still owns. +// Pure — `daemon-leak-oracle.ts` gathers the observation and asserts on it, and +// `daemon-leak-model.test.ts` pins these rules against fixture observations, +// including the #1109 and #1324 leak shapes. // -// Two leak classes, both keyed on a record the daemon itself writes: +// Ownership is explicit and conservative. Global process counts are not +// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this +// daemon does not. A host process is daemon-owned when ANY rule holds: +// +// descendant its PPID chain reaches a daemon pid, or any process already +// owned below (observable while that ancestor lives; orphans +// reparent to launchd/init). +// process-group its PGID equals a daemon pid. The CLI launches the daemon +// detached (setsid), so the daemon is its own group leader and +// every non-detached runCmdBackground child stays in that group +// after reparenting. This is the #1324 signature: `simctl io … +// recordVideo` with PPID 1 and PGID = the dead daemon's pid. +// A pgid is reserved only while the group has a live member, so +// on a long-lived host pid reuse can eventually hand the number +// to an unrelated process; the lanes' state dirs are fresh per +// run, and every red-proof cross-checks the command line. +// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. +// The CLI starts the daemon with that variable and children +// inherit it, including setsid'd grandchildren the pgid rule +// misses (agent-browser's own daemon → Chrome fleet: #1109). +// macOS hides the environment of Apple platform binaries +// (`ps -E` shows it for node/Chrome, not for /bin/sleep or +// simctl), so Apple tooling is caught by the pgid/argv rules. +// state-dir-argv its command line contains the state dir path as a whole path +// token (recorders writing session artifacts, browsers pinned +// to a managed home). +// +// The lanes use a fresh mkdtemp state dir per run, so the env/argv rules cannot +// match a foreign process. The caller excludes itself and its own ancestors. +// +// Two further leak classes, independent of owned children: // // surviving daemon at `after-shutdown` a daemon pid that is still alive IS // the leak. `stopProcessForTakeover` is best-effort and @@ -16,25 +46,31 @@ // matcher): `*.tmp` write-then-publish temporaries are torn // publishes, an empty directory is an unswept session // scaffold, daemon.json/daemon.lock may exist only while a -// daemon legitimately lives, and a capture handle whose -// owning session is gone must be finalized — a descriptor -// still `lifecycle: "open"`, or a legacy `app-log.pid` -// marker, outlived its owner, while a `completed` -// descriptor is the finish record ADR 0019 keeps. -// `tools/` holds managed third-party installs -// (agent-browser and its Chrome), whose own download -// temporaries and scaffolding this daemon neither writes -// nor owns. -// -// Daemon-owned child processes are the third leak class, and the only one with -// no such record: nothing persists what a daemon spawned, so ownership has to be -// reconstructed from the OS. That arm lives in the manual -// `daemon-owned-process-probe.ts` — it is what produced the #1109 and #1324 -// evidence, and it returns here once the daemon records owned pids. -import { uniquePositivePids } from '../../../src/utils/host-process.ts'; +// daemon legitimately lives, and a capture descriptor still +// `lifecycle: "open"` (or a legacy `app-log.pid` marker) +// after shutdown is a capture handle that outlived its +// owner — a `completed` descriptor is the finish record +// ADR 0019 keeps. `tools/` holds managed third-party +// installs (agent-browser and its Chrome), whose own +// download temporaries and scaffolding this daemon neither +// writes nor owns. +import { expandProcessTree, uniquePositivePids } from '../../../src/utils/host-process.ts'; + +const COMMAND_PREVIEW_CHARS = 160; export type DaemonLeakPhase = 'after-close' | 'after-shutdown'; +export type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; + +export type HostProcess = { + pid: number; + ppid: number; + pgid: number; + command: string; + /** Environment as a single string; empty when the host hides it. */ + env: string; +}; + /** One state-dir path, pre-read so the rules stay free of filesystem access. */ export type StateEntry = { /** Relative to the state dir, `/`-separated; directories keep a trailing `/`. */ @@ -44,6 +80,14 @@ export type StateEntry = { descriptorLifecycle?: string; }; +export type OwnedProcess = { + pid: number; + ppid: number; + pgid: number; + command: string; + reasons: OwnershipReason[]; +}; + export type NonEmpty = readonly [T, ...T[]]; /** @@ -62,6 +106,9 @@ export type DaemonLeakObservationBase = { stateDir: string; daemonPids: readonly number[]; livePids: readonly number[]; + processes: readonly HostProcess[]; + /** Pids never treated as owned: the observer and its own ancestors. */ + excludedPids: readonly number[]; stateEntries: readonly StateEntry[]; }; @@ -72,6 +119,7 @@ export type DaemonLeakSnapshot = { daemonPids: number[]; phase: DaemonLeakPhase; liveDaemonPids: number[]; + ownedProcesses: OwnedProcess[]; strayStateEntries: string[]; }; @@ -93,24 +141,26 @@ const EXPECTED_STATE_DIR_ENTRIES: readonly RegExp[] = [ const CAPTURE_DESCRIPTOR_ENTRY = /^sessions\/[^/]+\/[^/]+\.resource\.json$/; const LEGACY_APP_LOG_MARKER_ENTRY = /^sessions\/[^/]+\/app-log\.pid$/; const DAEMON_LIVENESS_ENTRY = /^daemon\.(?:json|lock)$/; -const SESSION_DIRECTORY = /^sessions\/([^/]+)\//; export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonLeakSnapshot { const daemonPids = uniquePositivePids(observation.daemonPids); const liveDaemonPids = daemonPids.filter((pid) => observation.livePids.includes(pid)); - const context: StateEntryContext = { - phase: observation.phase, - // Only a session close leaves a daemon legitimately running. - daemonLegitimatelyAlive: observation.phase === 'after-close' && liveDaemonPids.length > 0, - closedSessions: observation.phase === 'after-close' ? observation.closedSessions : [], - }; + const daemonLegitimatelyAlive = observation.phase === 'after-close' && liveDaemonPids.length > 0; return { stateDir: observation.stateDir, daemonPids, phase: observation.phase, liveDaemonPids, + ownedProcesses: findOwnedProcesses(observation, daemonPids), strayStateEntries: observation.stateEntries - .filter((entry) => classifyStateEntry(entry, context) === 'stray') + .filter( + (entry) => + classifyStateEntry(entry, { + phase: observation.phase, + daemonLegitimatelyAlive, + closedSessions: closedSessionsOf(observation), + }) === 'stray', + ) .map((entry) => entry.path) .sort(), }; @@ -118,21 +168,68 @@ export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonL /** * A daemon that outlived its own shutdown is itself the leak — not a detail of - * the report — so it fails alongside state-dir residue. + * the report — so it fails alongside owned children and state-dir residue. */ export function hasDaemonLeaks(snapshot: DaemonLeakSnapshot): boolean { - return survivingDaemonPids(snapshot).length > 0 || snapshot.strayStateEntries.length > 0; + return ( + survivingDaemonPids(snapshot).length > 0 || + snapshot.ownedProcesses.length > 0 || + snapshot.strayStateEntries.length > 0 + ); } -export function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { - return [ - `daemon leak oracle: ${hasDaemonLeaks(snapshot) ? 'LEAK' : 'clean'} (${snapshot.phase})`, - ` state dir: ${snapshot.stateDir}`, - ` daemon pids: ${formatPids(snapshot.daemonPids)}; live: ${formatPids(snapshot.liveDaemonPids)}`, - ` daemons that outlived shutdown: ${formatPids(survivingDaemonPids(snapshot))}`, - ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, - ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), - ].join('\n'); +function survivingDaemonPids(snapshot: DaemonLeakSnapshot): number[] { + return snapshot.phase === 'after-shutdown' ? snapshot.liveDaemonPids : []; +} + +function findOwnedProcesses( + observation: DaemonLeakObservation, + daemonPids: readonly number[], +): OwnedProcess[] { + const excluded = new Set([...observation.excludedPids, ...daemonPids]); + const matchers = stateDirMatchers(observation.stateDir); + const reasonsByPid = new Map(); + for (const proc of observation.processes) { + if (excluded.has(proc.pid)) continue; + const reasons = directOwnershipReasons(proc, daemonPids, matchers); + if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); + } + // Ownership propagates down the tree: a child of the daemon, or of any process + // the rules above own, is owned (a detached `(simctl)` grandchild of a leaked + // recorder shim, DTServiceHub under a runner xcodebuild, …). + const roots = [...daemonPids, ...reasonsByPid.keys()]; + for (const proc of expandProcessTree(roots, observation.processes)) { + if (excluded.has(proc.pid) || roots.includes(proc.pid)) continue; + reasonsByPid.set(proc.pid, [...(reasonsByPid.get(proc.pid) ?? []), 'descendant']); + } + return observation.processes.flatMap((proc) => { + const reasons = reasonsByPid.get(proc.pid); + return reasons + ? [{ pid: proc.pid, ppid: proc.ppid, pgid: proc.pgid, command: proc.command, reasons }] + : []; + }); +} + +type StateDirMatchers = { argv: RegExp; env: RegExp }; + +function stateDirMatchers(stateDir: string): StateDirMatchers { + // Whole-path matches only: `` as a token or `/…`, never `-sibling`. + return { + argv: new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`), + env: new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`), + }; +} + +function directOwnershipReasons( + proc: HostProcess, + daemonPids: readonly number[], + matchers: StateDirMatchers, +): OwnershipReason[] { + const reasons: OwnershipReason[] = []; + if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); + if (matchers.env.test(proc.env)) reasons.push('state-dir-env'); + if (matchers.argv.test(proc.command)) reasons.push('state-dir-argv'); + return reasons; } type StateEntryContext = { @@ -141,10 +238,6 @@ type StateEntryContext = { closedSessions: readonly string[]; }; -function survivingDaemonPids(snapshot: DaemonLeakSnapshot): number[] { - return snapshot.phase === 'after-shutdown' ? snapshot.liveDaemonPids : []; -} - function classifyStateEntry(entry: StateEntry, context: StateEntryContext): 'expected' | 'stray' { if (MANAGED_TOOLS_ENTRY.test(entry.path)) return 'expected'; if (entry.kind === 'empty-directory') return 'stray'; @@ -167,12 +260,56 @@ function classifyStateEntry(entry: StateEntry, context: StateEntryContext): 'exp function classifyCaptureEntry(entry: StateEntry, context: StateEntryContext): 'expected' | 'stray' { const owningSessionGone = context.phase === 'after-shutdown' || - context.closedSessions.includes(SESSION_DIRECTORY.exec(entry.path)?.[1] ?? ''); + context.closedSessions.includes(sessionDirectoryOf(entry.path) ?? ''); if (!owningSessionGone) return 'expected'; if (!CAPTURE_DESCRIPTOR_ENTRY.test(entry.path)) return 'stray'; return entry.descriptorLifecycle === 'completed' ? 'expected' : 'stray'; } +function closedSessionsOf(observation: DaemonLeakObservation): readonly string[] { + return observation.phase === 'after-close' ? observation.closedSessions : []; +} + +function sessionDirectoryOf(entryPath: string): string | undefined { + return /^sessions\/([^/]+)\//.exec(entryPath)?.[1]; +} + +export function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { + const surviving = survivingDaemonPids(snapshot); + return [ + `daemon leak oracle: ${hasDaemonLeaks(snapshot) ? 'LEAK' : 'clean'} (${snapshot.phase})`, + ` state dir: ${snapshot.stateDir}`, + ` daemon pids: ${formatPids(snapshot.daemonPids)}; live: ${formatPids(snapshot.liveDaemonPids)}`, + ` daemons that outlived shutdown: ${surviving.length}${ + surviving.length > 0 ? ` (${formatPids(surviving)})` : '' + }`, + ` owned processes still alive: ${snapshot.ownedProcesses.length}`, + ...snapshot.ownedProcesses.map( + (proc) => + ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${previewCommand( + proc.command, + snapshot.stateDir, + )}`, + ), + ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, + ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), + ].join('\n'); +} + +function previewCommand(command: string, stateDir: string): string { + const masked = command.replace( + new RegExp(`${escapeRegExp(stateDir)}(?=[\\s/]|$)`, 'g'), + '', + ); + return masked.length > COMMAND_PREVIEW_CHARS + ? `${masked.slice(0, COMMAND_PREVIEW_CHARS)}…` + : masked; +} + function formatPids(pids: readonly number[]): string { return pids.join(', ') || '(none)'; } + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts index f0d6efa280..ce028d2dac 100644 --- a/test/integration/support/daemon-leak-oracle.ts +++ b/test/integration/support/daemon-leak-oracle.ts @@ -1,12 +1,17 @@ -// Daemon leak oracle (#1781 B1, feeds #1431): observes an isolated state dir and -// the liveness of the daemons that owned it, then applies the rules in +// Daemon leak oracle (#1781 B1, feeds #1431): observes the host process table +// and an isolated state dir, then applies the ownership/residue rules in // `daemon-leak-model.ts` (which documents them and is where they are tested). -// Daemon-owned child processes are reconstructed by the manual -// `daemon-owned-process-probe.ts` instead — see its header for why that arm -// cannot assert anything from a device-free lane. +// +// What each caller actually exercises depends on what its daemon did. The three +// real-subprocess daemon lanes run `session list`/`close` with no device, +// simulator or browser, so their daemons own no children and the process arm +// asserts an empty set; what they guard is the surviving-daemon check and the +// state-dir residue. The #1109/#1324 leak shapes are guarded by the model's +// fixture test and reproduced by hand against the pre-fix commits — see the PR. import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; +import { runCmd } from '../../../src/utils/exec.ts'; import { isProcessAlive } from '../../../src/utils/host-process.ts'; import { AppError } from '@agent-device/kernel/errors'; import { @@ -15,11 +20,27 @@ import { hasDaemonLeaks, type DaemonLeakPhaseSelection, type DaemonLeakSnapshot, + type HostProcess, type StateEntry, } from './daemon-leak-model.ts'; +const PS_TIMEOUT_MS = 5_000; const DEFAULT_SETTLE_MS = 5_000; const SETTLE_POLL_MS = 250; +// Matches src/utils/host-process.ts: an absolute path so a PATH-resolved +// third-party `ps` (Homebrew, procps) cannot change the flag semantics. +const HOST_PS_COMMAND = process.platform === 'win32' ? 'ps' : '/bin/ps'; + +/** + * `phase` carries the identity that phase needs: `after-close` cannot be + * requested without naming at least one closed session (see + * DaemonLeakPhaseSelection), so the vacuous checkpoint that accepts every open + * capture handle is not expressible. `after-shutdown` takes nothing extra. + * + * Closed sessions are directory names, not paths — the basename of + * `sessionStore.resolveSessionDir(name)`, or of the `sessionStateDir` an + * `open`/`close` response reports. + */ export type DaemonLeakOracleOptions = DaemonLeakPhaseSelection & { stateDir: string; /** Every daemon pid the lane observed for this state dir (from daemon.json). */ @@ -35,13 +56,18 @@ export type DaemonLeakOracleOptions = DaemonLeakPhaseSelection & { settleMs?: number; }; -function captureDaemonLeakSnapshot(options: DaemonLeakOracleOptions): DaemonLeakSnapshot { +async function captureDaemonLeakSnapshot( + options: DaemonLeakOracleOptions, +): Promise { const stateDir = path.resolve(options.stateDir); + const processes = await listHostProcessesWithGroups(); return evaluateDaemonLeaks({ ...phaseSelectionOf(options), stateDir, daemonPids: options.daemonPids, livePids: options.daemonPids.filter((pid) => isProcessAlive(pid)), + processes, + excludedPids: [...ancestorsOf(process.pid, processes)], stateEntries: readStateEntries(stateDir, options.sessionsDir), }); } @@ -54,10 +80,10 @@ async function settleDaemonLeakSnapshot( options: DaemonLeakOracleOptions, ): Promise { const deadline = Date.now() + (options.settleMs ?? DEFAULT_SETTLE_MS); - let snapshot = captureDaemonLeakSnapshot(options); + let snapshot = await captureDaemonLeakSnapshot(options); while (hasDaemonLeaks(snapshot) && Date.now() < deadline) { await new Promise((resolve) => setTimeout(resolve, SETTLE_POLL_MS)); - snapshot = captureDaemonLeakSnapshot(options); + snapshot = await captureDaemonLeakSnapshot(options); } return snapshot; } @@ -126,6 +152,67 @@ function readDescriptorLifecycle(filePath: string): string | undefined { } } +// src/utils/host-process.ts's listHostProcesses covers pid/ppid/command; the +// ownership rules also need the process group and the environment, which need a +// second `ps` on macOS (`-E` appends the environment to the command column) and +// /proc on Linux. +async function listHostProcessesWithGroups(): Promise { + const darwin = process.platform === 'darwin'; + const [tree, darwinEnv] = await Promise.all([ + runPs(['-axww', '-o', 'pid=,ppid=,pgid=,command=']), + darwin ? runPs(['-E', '-axww', '-o', 'pid=,command=']) : Promise.resolve(''), + ]); + const envByPid = new Map( + parsePsLines(darwinEnv, /^\s*(\d+)\s+(.*)$/).map(([pid, env]) => [Number(pid), env ?? '']), + ); + return parsePsLines(tree, /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/).map( + ([pid, ppid, pgid, command]) => ({ + pid: Number(pid), + ppid: Number(ppid), + pgid: Number(pgid), + command: command ?? '', + env: darwin ? (envByPid.get(Number(pid)) ?? '') : readLinuxEnviron(Number(pid)), + }), + ); +} + +function parsePsLines(stdout: string, shape: RegExp): string[][] { + return stdout.split('\n').flatMap((line) => { + const match = shape.exec(line); + return match ? [match.slice(1)] : []; + }); +} + +async function runPs(args: string[]): Promise { + const result = await runCmd(HOST_PS_COMMAND, args, { + allowFailure: true, + timeoutMs: PS_TIMEOUT_MS, + }); + if (result.exitCode !== 0) { + throw new Error( + `daemon leak oracle: ps ${args.join(' ')} failed (${result.exitCode}): ${result.stderr}`, + ); + } + return result.stdout; +} + +function readLinuxEnviron(pid: number): string { + try { + return fs.readFileSync(`/proc/${pid}/environ`, 'latin1').replaceAll('\0', ' '); + } catch { + return ''; + } +} + +function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set { + const byPid = new Map(processes.map((proc) => [proc.pid, proc])); + const chain = new Set(); + for (let cursor = pid; cursor > 0 && !chain.has(cursor); cursor = byPid.get(cursor)?.ppid ?? 0) { + chain.add(cursor); + } + return chain; +} + /** * Parses the standalone CLI's argv into oracle options, refusing the invocation * the type system already refuses in code: `--phase after-close` without a diff --git a/test/integration/support/daemon-owned-process-probe.test.ts b/test/integration/support/daemon-owned-process-probe.test.ts deleted file mode 100644 index 077618fea2..0000000000 --- a/test/integration/support/daemon-owned-process-probe.test.ts +++ /dev/null @@ -1,74 +0,0 @@ -import { describe, expect, test } from 'vitest'; -import { findOwnedProcesses, type HostProcess } from './daemon-owned-process-probe.ts'; - -// The probe is manual — no lane can run it, because a device-free daemon owns no -// children (see its header). These fixtures are the real `ps` rows captured -// during the #1109 and #1324 red-proofs (SHAs in the #1781 B1 PR), so a regex or -// ordering edit that stops catching either leak fails here rather than silently -// weakening the next hand-run proof. -const STATE_DIR = '/tmp/agent-device-lane-abc'; -const DAEMON_PID = 4340; - -function proc(overrides: Partial & Pick): HostProcess { - return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides }; -} - -const owned = (processes: HostProcess[]) => findOwnedProcesses(processes, [DAEMON_PID], STATE_DIR); - -describe('daemon-owned process rules', () => { - // #1324: `simctl io … recordVideo` reparents to launchd (ppid 1) but keeps the - // dead daemon's process group, and simctl only finalizes the mp4 on SIGINT. - test('flags a recorder orphaned into the dead daemon process group', () => { - const recorder = proc({ - pid: 52420, - ppid: 1, - pgid: DAEMON_PID, - command: '/…/simctl io 416440AE recordVideo /tmp/out.mp4', - }); - - expect(owned([recorder])).toEqual([ - expect.objectContaining({ pid: 52420, reasons: ['process-group'] }), - ]); - }); - - // #1109: the agent-browser daemon setsids away from the daemon's group, so - // only the inherited state-dir environment and its argv identify the fleet. - test('flags an agent-browser fleet by inherited state dir, including its children', () => { - const browserDaemon = proc({ - pid: 47515, - pgid: 47515, - command: `${STATE_DIR}/tools/agent-browser/0.27.1/package/…/agent-browser-darwin-arm64`, - env: `HOME=/tmp AGENT_DEVICE_STATE_DIR=${STATE_DIR}`, - }); - const chrome = proc({ - pid: 47586, - ppid: 47515, - pgid: 47586, - command: 'Google Chrome for Testing', - }); - const renderer = proc({ - pid: 47953, - ppid: 47586, - pgid: 47586, - command: 'Chrome Helper (Renderer)', - }); - const matches = owned([browserDaemon, chrome, renderer]); - - expect(matches.map((match) => match.pid)).toEqual([47515, 47586, 47953]); - expect(matches[0]?.reasons).toEqual(['state-dir-env', 'state-dir-argv']); - // The fleet below the matched root is owned transitively, not by its own argv. - expect(matches[1]?.reasons).toEqual(['descendant']); - }); - - test('ignores foreign processes, a neighbouring state dir, and the daemon itself', () => { - const simulator = proc({ pid: 700, command: '/…/CoreSimulator … SimulatorTrampoline' }); - const neighbour = proc({ - pid: 701, - command: `node --state-dir ${STATE_DIR}-other/daemon.ts`, - env: `AGENT_DEVICE_STATE_DIR=${STATE_DIR}-other`, - }); - const daemon = proc({ pid: DAEMON_PID, pgid: DAEMON_PID }); - - expect(owned([simulator, neighbour, daemon])).toEqual([]); - }); -}); diff --git a/test/integration/support/daemon-owned-process-probe.ts b/test/integration/support/daemon-owned-process-probe.ts deleted file mode 100644 index c1895b9c5e..0000000000 --- a/test/integration/support/daemon-owned-process-probe.ts +++ /dev/null @@ -1,218 +0,0 @@ -// Manual red-proof probe for daemon-owned child processes (#1781 B1, feeds -// #1431). Run by hand against a daemon that has just been stopped; it is what -// produced the #1109 and #1324 evidence in the PR. -// -// It is deliberately NOT wired into a lane. Nothing records what a daemon -// spawned, so ownership has to be reconstructed from the OS, and the three -// real-subprocess daemon lanes run `session list`/`close` with no device, -// simulator, or browser — their daemons own no children, so a lane assertion -// here would only ever compare an empty set to an empty set. The shipped -// oracle (`daemon-leak-oracle.ts`) asserts what the daemon does record: an -// unfinalized capture handle, state-dir residue, and its own survival. Once the -// daemon records owned child pids the way it records captures, this probe -// collapses into "read the record, assert they are dead" and moves back into -// the oracle. -// -// Ownership is explicit and conservative. Global process counts are not -// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this -// daemon does not. A host process is daemon-owned when ANY rule holds: -// -// descendant its PPID chain reaches a daemon pid, or any process already -// owned below (observable while that ancestor lives; orphans -// reparent to launchd/init). -// process-group its PGID equals a daemon pid. The CLI launches the daemon -// detached (setsid), so the daemon is its own group leader and -// every non-detached runCmdBackground child stays in that group -// after reparenting. This is the #1324 signature: `simctl io … -// recordVideo` with PPID 1 and PGID = the dead daemon's pid. -// A pgid is reserved only while the group has a live member, so -// on a long-lived host pid reuse can eventually hand the number -// to an unrelated process; cross-check the command line. -// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. -// The CLI starts the daemon with that variable and children -// inherit it, including setsid'd grandchildren the pgid rule -// misses (agent-browser's own daemon → Chrome fleet: #1109). -// macOS hides the environment of Apple platform binaries -// (`ps -E` shows it for node/Chrome, not for /bin/sleep or -// simctl), so Apple tooling is caught by the pgid/argv rules. -// state-dir-argv its command line contains the state dir path as a whole path -// token (recorders writing session artifacts, browsers pinned -// to a managed home). -// -// Use a fresh state dir per run, so the env/argv rules cannot match a foreign -// process. The probe excludes itself and its own ancestors. -// -// The rules are pinned by `daemon-owned-process-probe.test.ts` against the real -// `ps` rows captured during those proofs, so a regex or ordering edit that stops -// catching either leak fails in CI even though no lane can run this probe. -// -// Usage (prints the report; exits 1 when anything owned is still alive): -// node --experimental-strip-types test/integration/support/daemon-owned-process-probe.ts \ -// --state-dir --daemon-pid [--daemon-pid …] -import fs from 'node:fs'; -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; -import { runCmd } from '../../../src/utils/exec.ts'; -import { expandProcessTree, uniquePositivePids } from '../../../src/utils/host-process.ts'; - -const PS_TIMEOUT_MS = 5_000; -const COMMAND_PREVIEW_CHARS = 160; -// Matches src/utils/host-process.ts: an absolute path so a PATH-resolved -// third-party `ps` (Homebrew, procps) cannot change the flag semantics. -const HOST_PS_COMMAND = process.platform === 'win32' ? 'ps' : '/bin/ps'; - -export type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; - -export type HostProcess = { - pid: number; - ppid: number; - pgid: number; - command: string; - /** Environment as a single string; empty when the host hides it. */ - env: string; -}; - -export type OwnedProcess = HostProcess & { reasons: OwnershipReason[] }; - -export function findOwnedProcesses( - processes: readonly HostProcess[], - daemonPids: readonly number[], - stateDir: string, -): OwnedProcess[] { - const excluded = new Set([...ancestorsOf(process.pid, processes), ...daemonPids]); - const matchers = stateDirMatchers(stateDir); - const reasonsByPid = new Map(); - for (const proc of processes) { - if (excluded.has(proc.pid)) continue; - const reasons = directOwnershipReasons(proc, daemonPids, matchers); - if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); - } - // Ownership propagates down the tree: a child of the daemon, or of any process - // the rules above own, is owned (a detached `(simctl)` grandchild of a leaked - // recorder shim, DTServiceHub under a runner xcodebuild, …). - const roots = [...daemonPids, ...reasonsByPid.keys()]; - for (const proc of expandProcessTree(roots, processes)) { - if (excluded.has(proc.pid) || roots.includes(proc.pid)) continue; - reasonsByPid.set(proc.pid, [...(reasonsByPid.get(proc.pid) ?? []), 'descendant']); - } - return processes.flatMap((proc) => { - const reasons = reasonsByPid.get(proc.pid); - return reasons ? [{ ...proc, reasons }] : []; - }); -} - -function stateDirMatchers(stateDir: string): { argv: RegExp; env: RegExp } { - // Whole-path matches only: `` as a token or `/…`, never `-sibling`. - return { - argv: new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`), - env: new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`), - }; -} - -function directOwnershipReasons( - proc: HostProcess, - daemonPids: readonly number[], - matchers: { argv: RegExp; env: RegExp }, -): OwnershipReason[] { - const reasons: OwnershipReason[] = []; - if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); - if (matchers.env.test(proc.env)) reasons.push('state-dir-env'); - if (matchers.argv.test(proc.command)) reasons.push('state-dir-argv'); - return reasons; -} - -function formatReport(owned: readonly OwnedProcess[], stateDir: string, pids: number[]): string { - const mask = new RegExp(`${escapeRegExp(stateDir)}(?=[\\s/]|$)`, 'g'); - return [ - `daemon-owned processes: ${owned.length > 0 ? 'LEAK' : 'clean'}`, - ` state dir: ${stateDir}`, - ` daemon pids: ${pids.join(', ') || '(none)'}`, - ...owned.map((proc) => { - const command = proc.command.replace(mask, ''); - return ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${ - command.length > COMMAND_PREVIEW_CHARS - ? `${command.slice(0, COMMAND_PREVIEW_CHARS)}…` - : command - }`; - }), - ].join('\n'); -} - -// pid/ppid/pgid plus the command line, and separately the environment so the -// argv and env rules stay distinct. macOS `ps -E` appends the environment to -// the command column for same-user processes; Linux exposes it in /proc. -async function listHostProcesses(): Promise { - const darwin = process.platform === 'darwin'; - const [tree, darwinEnv] = await Promise.all([ - runPs(['-axww', '-o', 'pid=,ppid=,pgid=,command=']), - darwin ? runPs(['-E', '-axww', '-o', 'pid=,command=']) : Promise.resolve(''), - ]); - const envByPid = new Map( - parsePsLines(darwinEnv, /^\s*(\d+)\s+(.*)$/).map(([pid, env]) => [Number(pid), env ?? '']), - ); - return parsePsLines(tree, /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/).map( - ([pid, ppid, pgid, command]) => ({ - pid: Number(pid), - ppid: Number(ppid), - pgid: Number(pgid), - command: command ?? '', - env: darwin ? (envByPid.get(Number(pid)) ?? '') : readLinuxEnviron(Number(pid)), - }), - ); -} - -function parsePsLines(stdout: string, shape: RegExp): string[][] { - return stdout.split('\n').flatMap((line) => { - const match = shape.exec(line); - return match ? [match.slice(1)] : []; - }); -} - -async function runPs(args: string[]): Promise { - const result = await runCmd(HOST_PS_COMMAND, args, { - allowFailure: true, - timeoutMs: PS_TIMEOUT_MS, - }); - if (result.exitCode !== 0) { - throw new Error(`ps ${args.join(' ')} failed (${result.exitCode}): ${result.stderr}`); - } - return result.stdout; -} - -function readLinuxEnviron(pid: number): string { - try { - return fs.readFileSync(`/proc/${pid}/environ`, 'latin1').replaceAll('\0', ' '); - } catch { - return ''; - } -} - -function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set { - const parentByPid = new Map(processes.map((proc) => [proc.pid, proc.ppid])); - const chain = new Set(); - let cursor: number | undefined = pid; - while (cursor && !chain.has(cursor)) { - chain.add(cursor); - cursor = parentByPid.get(cursor); - } - return chain; -} - -function escapeRegExp(value: string): string { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - -// Only when run directly: importing this module (its fixture test) must not -// execute the probe. -if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { - const args = process.argv.slice(2); - const readFlag = (name: string): string[] => - args.flatMap((arg, index) => (arg === name && args[index + 1] ? [args[index + 1]!] : [])); - const stateDir = readFlag('--state-dir')[0]; - if (!stateDir) throw new Error('daemon-owned process probe: --state-dir is required'); - const daemonPids = uniquePositivePids(readFlag('--daemon-pid').map(Number)); - const resolvedStateDir = path.resolve(stateDir); - const owned = findOwnedProcesses(await listHostProcesses(), daemonPids, resolvedStateDir); - process.stdout.write(`${formatReport(owned, resolvedStateDir, daemonPids)}\n`); - process.exitCode = owned.length > 0 ? 1 : 0; -} diff --git a/vitest.config.ts b/vitest.config.ts index 293cf26877..f80dc46c5d 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -95,10 +95,6 @@ export default defineConfig({ // oracle are device-free, so this is where the #1109/#1324 leak // shapes are actually guarded. 'test/integration/support/daemon-leak-model.test.ts', - // Same for the manual owned-process probe's rules: no lane can run the - // probe (a device-free daemon owns no children), so its fixtures are - // the only CI guard on the #1109/#1324 ownership shapes. - 'test/integration/support/daemon-owned-process-probe.test.ts', // The frozen replay-compat corpus (#1417): parse-only, no device or // subprocess work, so it belongs in the fast lane next to the // grammar it guards. From c717e560d3d9a1d29f55e373e093c170480722d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 18:00:49 +0200 Subject: [PATCH 7/8] docs: align daemon leak coverage rationale --- test/integration/support/daemon-leak-oracle.ts | 10 +++++----- vitest.config.ts | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts index ce028d2dac..843935b857 100644 --- a/test/integration/support/daemon-leak-oracle.ts +++ b/test/integration/support/daemon-leak-oracle.ts @@ -3,11 +3,11 @@ // `daemon-leak-model.ts` (which documents them and is where they are tested). // // What each caller actually exercises depends on what its daemon did. The three -// real-subprocess daemon lanes run `session list`/`close` with no device, -// simulator or browser, so their daemons own no children and the process arm -// asserts an empty set; what they guard is the surviving-daemon check and the -// state-dir residue. The #1109/#1324 leak shapes are guarded by the model's -// fixture test and reproduced by hand against the pre-fix commits — see the PR. +// device-free daemon lanes guard surviving-daemon and state-dir residue rules. +// The Web smoke owns a real managed agent-browser + Chrome fleet, so its +// post-shutdown checkpoint makes the #1109 process arm fail on the shipped +// route. The model fixtures pin every ownership shape, including #1324's +// reparented recorder; both historical failures also have manual red-proofs. import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; diff --git a/vitest.config.ts b/vitest.config.ts index f80dc46c5d..8231200b6b 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -91,9 +91,9 @@ export default defineConfig({ 'test/ci/root-docs-paths-ignore.test.ts', // The daemon leak oracle's ownership/residue rules (#1781 B1): pure // decisions over fixture `ps` rows and state-dir listings, so they - // need no daemon, device, or subprocess. The lanes that call the - // oracle are device-free, so this is where the #1109/#1324 leak - // shapes are actually guarded. + // need no daemon, device, or subprocess. The Web smoke exercises + // #1109 with a real managed browser fleet; these fixtures pin every + // ownership rule, including #1324's reparented recorder shape. 'test/integration/support/daemon-leak-model.test.ts', // The frozen replay-compat corpus (#1417): parse-only, no device or // subprocess work, so it belongs in the fast lane next to the From 919da440a90608602388265bdb049f1afc09bc51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 18:19:08 +0200 Subject: [PATCH 8/8] refactor(test): narrow daemon oracle to durable state leaks --- test/integration/smoke-web-platform.test.ts | 56 +------ .../support/daemon-leak-model.test.ts | 80 ---------- test/integration/support/daemon-leak-model.ts | 147 +----------------- .../integration/support/daemon-leak-oracle.ts | 83 +--------- vitest.config.ts | 8 +- 5 files changed, 14 insertions(+), 360 deletions(-) diff --git a/test/integration/smoke-web-platform.test.ts b/test/integration/smoke-web-platform.test.ts index a59288af3e..aabaccf030 100644 --- a/test/integration/smoke-web-platform.test.ts +++ b/test/integration/smoke-web-platform.test.ts @@ -1,12 +1,10 @@ import assert from 'node:assert/strict'; -import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { mkdirSync, writeFileSync } from 'node:fs'; import { createServer, type Server } from 'node:http'; import path from 'node:path'; import test from 'node:test'; -import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts'; import { type CliJsonResult, formatResultDebug, runBuiltCliJson } from './cli-json.ts'; import { assertPngDimensions, assertPngFile } from './provider-scenarios/assertions.ts'; -import { assertNoDaemonLeaks } from './support/daemon-leak-oracle.ts'; const TEST_NAME = 'live web platform e2e smoke'; const WEB_E2E_ENABLED = process.env.AGENT_DEVICE_WEB_E2E === '1'; @@ -27,21 +25,10 @@ type WebSmokeContext = { lastSnapshot?: any; screenshotPath: string; server: Server; - stateDir: string; stepHistory: StepRecord[]; url: string; }; -// #1781 B1 / #1109: this is the one lane whose daemon owns real child processes -// — the managed agent-browser daemon and its Chrome fleet — so it is where the -// leak oracle's owned-process arm can actually fail. #1109's acceptance is that -// a stopped daemon leaves zero agent-browser processes behind within the idle -// window; the lane pins AGENT_BROWSER_IDLE_TIMEOUT_MS at 30s, so the settle -// window here has to outlast it. Graceful daemon shutdown does not close web -// sessions (#1868), so the fleet legitimately lives until that window elapses — -// which is why the checkpoint waits it out rather than asserting immediately. -const WEB_LEAK_SETTLE_MS = 90_000; - test( TEST_NAME, { @@ -103,7 +90,6 @@ async function createWebSmokeContext(): Promise { env, screenshotPath: path.join(artifactDir, 'web-smoke.png'), server: fixture.server, - stateDir, stepHistory: [], url: fixture.url, }; @@ -251,11 +237,6 @@ async function cleanupWebSmoke(context: WebSmokeContext, opened: boolean): Promi errors.push(error); } } - try { - await assertOrphanedWebSessionLeavesNothingOwned(context); - } catch (error) { - errors.push(error); - } try { await closeServer(context.server); } catch (error) { @@ -269,41 +250,6 @@ async function cleanupWebSmoke(context: WebSmokeContext, opened: boolean): Promi } } -// #1109's acceptance, as a lane assertion: a daemon that dies while a web -// session is still open must leave zero agent-browser processes behind within -// the idle window. That is the leak's real shape — an ordinary `close` reaps the -// fleet, so only an unclosed session can strand it — and it is the one route in -// CI where the oracle's owned-process arm has real children to find. -async function assertOrphanedWebSessionLeavesNothingOwned(context: WebSmokeContext): Promise { - await runStep(context, 'reopen for the orphan checkpoint', [ - 'open', - context.url, - ...context.common, - ]); - const infoPath = path.join(context.stateDir, 'daemon.json'); - // Never skip silently: no daemon metadata after a live web session means the - // checkpoint would certify a daemon it never observed. - assert.ok(existsSync(infoPath), `expected daemon metadata at ${infoPath}`); - const info = JSON.parse(readFileSync(infoPath, 'utf8')) as { - pid: number; - processStartTime?: string; - }; - assert.ok(Number.isInteger(info.pid) && info.pid > 0, `expected a daemon pid in ${infoPath}`); - // Stop the daemon with the session still open: the browser fleet is orphaned - // exactly as in #1109, and only its idle lifecycle can still reap it. - await stopProcessForTakeover(info.pid, { - termTimeoutMs: 5_000, - killTimeoutMs: 5_000, - expectedStartTime: info.processStartTime, - }); - await assertNoDaemonLeaks({ - stateDir: context.stateDir, - daemonPids: [info.pid], - phase: 'after-shutdown', - settleMs: WEB_LEAK_SETTLE_MS, - }); -} - function recordStep( context: WebSmokeContext, step: string, diff --git a/test/integration/support/daemon-leak-model.test.ts b/test/integration/support/daemon-leak-model.test.ts index a3bdf236f4..c4c511ca2a 100644 --- a/test/integration/support/daemon-leak-model.test.ts +++ b/test/integration/support/daemon-leak-model.test.ts @@ -6,23 +6,12 @@ import { type DaemonLeakObservation, type DaemonLeakObservationBase, type DaemonLeakPhaseSelection, - type HostProcess, type NonEmpty, type StateEntry, } from './daemon-leak-model.ts'; -// The lanes that call the oracle run device-free daemons, so the ownership rules -// would otherwise only ever see an empty process set. These fixtures are the -// real `ps` shapes captured during the #1109 and #1324 red-proofs (SHAs in the -// #1781 B1 PR), so a regex or ordering edit that stops catching either leak -// fails here instead of silently going quiet in CI. const STATE_DIR = '/tmp/agent-device-lane-abc'; const DAEMON_PID = 4340; -const OBSERVER_PID = 999; - -function proc(overrides: Partial & Pick): HostProcess { - return { ppid: 1, pgid: overrides.pid, command: 'unrelated', env: '', ...overrides }; -} // The phase and its identity arrive together: `after-close` cannot be requested // without naming the sessions that closed, so these helpers cannot construct the @@ -35,8 +24,6 @@ function observe( stateDir: STATE_DIR, daemonPids: [DAEMON_PID], livePids: [], - processes: [], - excludedPids: [OBSERVER_PID], stateEntries: [], ...overrides, ...selection, @@ -57,72 +44,6 @@ function file(entryPath: string, descriptorLifecycle?: string): StateEntry { return { path: entryPath, kind: 'file', ...(descriptorLifecycle ? { descriptorLifecycle } : {}) }; } -describe('owned-process rules', () => { - // #1324: `simctl io … recordVideo` reparents to launchd (ppid 1) but keeps the - // dead daemon's process group, and simctl only finalizes the mp4 on SIGINT. - test('flags a recorder orphaned into the dead daemon process group', () => { - const recorder = proc({ - pid: 52420, - ppid: 1, - pgid: DAEMON_PID, - command: '/…/simctl io 416440AE recordVideo /tmp/out.mp4', - }); - const snapshot = evaluateDaemonLeaks(observe({ processes: [recorder] })); - - expect(snapshot.ownedProcesses).toEqual([ - expect.objectContaining({ pid: 52420, reasons: ['process-group'] }), - ]); - expect(hasDaemonLeaks(snapshot)).toBe(true); - }); - - // #1109: the agent-browser daemon setsids away from the daemon's group, so - // only the inherited state-dir environment and its argv identify the fleet. - test('flags an agent-browser fleet by inherited state dir, including its children', () => { - const browserDaemon = proc({ - pid: 47515, - ppid: 1, - pgid: 47515, - command: `${STATE_DIR}/tools/agent-browser/0.27.1/package/…/agent-browser-darwin-arm64`, - env: `HOME=/tmp AGENT_DEVICE_STATE_DIR=${STATE_DIR}`, - }); - const chrome = proc({ - pid: 47586, - ppid: 47515, - pgid: 47586, - command: 'Google Chrome for Testing', - }); - const renderer = proc({ - pid: 47953, - ppid: 47586, - pgid: 47586, - command: 'Chrome Helper (Renderer)', - }); - const snapshot = evaluateDaemonLeaks(observe({ processes: [browserDaemon, chrome, renderer] })); - - expect(snapshot.ownedProcesses.map((owned) => owned.pid)).toEqual([47515, 47586, 47953]); - expect(snapshot.ownedProcesses[0]?.reasons).toEqual(['state-dir-env', 'state-dir-argv']); - // The fleet below the matched root is owned transitively, not by its own argv. - expect(snapshot.ownedProcesses[1]?.reasons).toEqual(['descendant']); - }); - - test('ignores foreign processes, the observer chain, and the daemons themselves', () => { - const simulator = proc({ pid: 700, command: '/…/CoreSimulator … SimulatorTrampoline' }); - const neighbourStateDir = proc({ - pid: 701, - command: `node --state-dir ${STATE_DIR}-other/daemon.ts`, - env: `AGENT_DEVICE_STATE_DIR=${STATE_DIR}-other`, - }); - const observer = proc({ pid: OBSERVER_PID, pgid: DAEMON_PID }); - const daemon = proc({ pid: DAEMON_PID, pgid: DAEMON_PID }); - const snapshot = evaluateDaemonLeaks( - observe({ processes: [simulator, neighbourStateDir, observer, daemon] }), - ); - - expect(snapshot.ownedProcesses).toEqual([]); - expect(hasDaemonLeaks(snapshot)).toBe(false); - }); -}); - describe('surviving-daemon rule', () => { // stopProcessForTakeover is best-effort void: it returns silently on identity // mismatch, signal failure, or kill timeout, so a daemon can outlive the stop. @@ -268,7 +189,6 @@ describe('closed-session capture handles', () => { test('a clean shutdown reports no leak', () => { const snapshot = evaluateDaemonLeaks( observe({ - processes: [proc({ pid: 700, command: 'unrelated' })], stateEntries: [file('daemon.log'), file('sessions/default/events.ndjson')], }), ); diff --git a/test/integration/support/daemon-leak-model.ts b/test/integration/support/daemon-leak-model.ts index a74c17bda9..7fdda099d5 100644 --- a/test/integration/support/daemon-leak-model.ts +++ b/test/integration/support/daemon-leak-model.ts @@ -1,40 +1,9 @@ -// Daemon leak rules (#1781 B1, feeds #1431): given one observation of the host -// process table and of an isolated state dir, decide what the daemon still owns. -// Pure — `daemon-leak-oracle.ts` gathers the observation and asserts on it, and -// `daemon-leak-model.test.ts` pins these rules against fixture observations, -// including the #1109 and #1324 leak shapes. +// Daemon leak rules (#1781 B1, feeds #1431): given one observation of an +// isolated state dir and the daemon pids a lane observed, decide whether daemon +// lifecycle or durable state leaked. Pure — `daemon-leak-oracle.ts` gathers the +// observation and asserts on it; `daemon-leak-model.test.ts` pins the rules. // -// Ownership is explicit and conservative. Global process counts are not -// evidence — the simulator, XCTest, and Chrome-for-Testing own processes this -// daemon does not. A host process is daemon-owned when ANY rule holds: -// -// descendant its PPID chain reaches a daemon pid, or any process already -// owned below (observable while that ancestor lives; orphans -// reparent to launchd/init). -// process-group its PGID equals a daemon pid. The CLI launches the daemon -// detached (setsid), so the daemon is its own group leader and -// every non-detached runCmdBackground child stays in that group -// after reparenting. This is the #1324 signature: `simctl io … -// recordVideo` with PPID 1 and PGID = the dead daemon's pid. -// A pgid is reserved only while the group has a live member, so -// on a long-lived host pid reuse can eventually hand the number -// to an unrelated process; the lanes' state dirs are fresh per -// run, and every red-proof cross-checks the command line. -// state-dir-env its environment carries AGENT_DEVICE_STATE_DIR=. -// The CLI starts the daemon with that variable and children -// inherit it, including setsid'd grandchildren the pgid rule -// misses (agent-browser's own daemon → Chrome fleet: #1109). -// macOS hides the environment of Apple platform binaries -// (`ps -E` shows it for node/Chrome, not for /bin/sleep or -// simctl), so Apple tooling is caught by the pgid/argv rules. -// state-dir-argv its command line contains the state dir path as a whole path -// token (recorders writing session artifacts, browsers pinned -// to a managed home). -// -// The lanes use a fresh mkdtemp state dir per run, so the env/argv rules cannot -// match a foreign process. The caller excludes itself and its own ancestors. -// -// Two further leak classes, independent of owned children: +// Two leak classes: // // surviving daemon at `after-shutdown` a daemon pid that is still alive IS // the leak. `stopProcessForTakeover` is best-effort and @@ -54,23 +23,10 @@ // installs (agent-browser and its Chrome), whose own // download temporaries and scaffolding this daemon neither // writes nor owns. -import { expandProcessTree, uniquePositivePids } from '../../../src/utils/host-process.ts'; - -const COMMAND_PREVIEW_CHARS = 160; +import { uniquePositivePids } from '../../../src/utils/host-process.ts'; export type DaemonLeakPhase = 'after-close' | 'after-shutdown'; -export type OwnershipReason = 'descendant' | 'process-group' | 'state-dir-env' | 'state-dir-argv'; - -export type HostProcess = { - pid: number; - ppid: number; - pgid: number; - command: string; - /** Environment as a single string; empty when the host hides it. */ - env: string; -}; - /** One state-dir path, pre-read so the rules stay free of filesystem access. */ export type StateEntry = { /** Relative to the state dir, `/`-separated; directories keep a trailing `/`. */ @@ -80,14 +36,6 @@ export type StateEntry = { descriptorLifecycle?: string; }; -export type OwnedProcess = { - pid: number; - ppid: number; - pgid: number; - command: string; - reasons: OwnershipReason[]; -}; - export type NonEmpty = readonly [T, ...T[]]; /** @@ -106,9 +54,6 @@ export type DaemonLeakObservationBase = { stateDir: string; daemonPids: readonly number[]; livePids: readonly number[]; - processes: readonly HostProcess[]; - /** Pids never treated as owned: the observer and its own ancestors. */ - excludedPids: readonly number[]; stateEntries: readonly StateEntry[]; }; @@ -119,7 +64,6 @@ export type DaemonLeakSnapshot = { daemonPids: number[]; phase: DaemonLeakPhase; liveDaemonPids: number[]; - ownedProcesses: OwnedProcess[]; strayStateEntries: string[]; }; @@ -151,7 +95,6 @@ export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonL daemonPids, phase: observation.phase, liveDaemonPids, - ownedProcesses: findOwnedProcesses(observation, daemonPids), strayStateEntries: observation.stateEntries .filter( (entry) => @@ -171,67 +114,13 @@ export function evaluateDaemonLeaks(observation: DaemonLeakObservation): DaemonL * the report — so it fails alongside owned children and state-dir residue. */ export function hasDaemonLeaks(snapshot: DaemonLeakSnapshot): boolean { - return ( - survivingDaemonPids(snapshot).length > 0 || - snapshot.ownedProcesses.length > 0 || - snapshot.strayStateEntries.length > 0 - ); + return survivingDaemonPids(snapshot).length > 0 || snapshot.strayStateEntries.length > 0; } function survivingDaemonPids(snapshot: DaemonLeakSnapshot): number[] { return snapshot.phase === 'after-shutdown' ? snapshot.liveDaemonPids : []; } -function findOwnedProcesses( - observation: DaemonLeakObservation, - daemonPids: readonly number[], -): OwnedProcess[] { - const excluded = new Set([...observation.excludedPids, ...daemonPids]); - const matchers = stateDirMatchers(observation.stateDir); - const reasonsByPid = new Map(); - for (const proc of observation.processes) { - if (excluded.has(proc.pid)) continue; - const reasons = directOwnershipReasons(proc, daemonPids, matchers); - if (reasons.length > 0) reasonsByPid.set(proc.pid, reasons); - } - // Ownership propagates down the tree: a child of the daemon, or of any process - // the rules above own, is owned (a detached `(simctl)` grandchild of a leaked - // recorder shim, DTServiceHub under a runner xcodebuild, …). - const roots = [...daemonPids, ...reasonsByPid.keys()]; - for (const proc of expandProcessTree(roots, observation.processes)) { - if (excluded.has(proc.pid) || roots.includes(proc.pid)) continue; - reasonsByPid.set(proc.pid, [...(reasonsByPid.get(proc.pid) ?? []), 'descendant']); - } - return observation.processes.flatMap((proc) => { - const reasons = reasonsByPid.get(proc.pid); - return reasons - ? [{ pid: proc.pid, ppid: proc.ppid, pgid: proc.pgid, command: proc.command, reasons }] - : []; - }); -} - -type StateDirMatchers = { argv: RegExp; env: RegExp }; - -function stateDirMatchers(stateDir: string): StateDirMatchers { - // Whole-path matches only: `` as a token or `/…`, never `-sibling`. - return { - argv: new RegExp(`(?:^|[\\s=])${escapeRegExp(stateDir)}(?:[\\s/]|$)`), - env: new RegExp(`AGENT_DEVICE_STATE_DIR=${escapeRegExp(stateDir)}(?:\\s|$)`), - }; -} - -function directOwnershipReasons( - proc: HostProcess, - daemonPids: readonly number[], - matchers: StateDirMatchers, -): OwnershipReason[] { - const reasons: OwnershipReason[] = []; - if (daemonPids.includes(proc.pgid)) reasons.push('process-group'); - if (matchers.env.test(proc.env)) reasons.push('state-dir-env'); - if (matchers.argv.test(proc.command)) reasons.push('state-dir-argv'); - return reasons; -} - type StateEntryContext = { phase: DaemonLeakPhase; daemonLegitimatelyAlive: boolean; @@ -283,33 +172,11 @@ export function formatDaemonLeakReport(snapshot: DaemonLeakSnapshot): string { ` daemons that outlived shutdown: ${surviving.length}${ surviving.length > 0 ? ` (${formatPids(surviving)})` : '' }`, - ` owned processes still alive: ${snapshot.ownedProcesses.length}`, - ...snapshot.ownedProcesses.map( - (proc) => - ` pid ${proc.pid} ppid ${proc.ppid} pgid ${proc.pgid} [${proc.reasons.join(',')}] ${previewCommand( - proc.command, - snapshot.stateDir, - )}`, - ), ` stray state-dir entries: ${snapshot.strayStateEntries.length}`, ...snapshot.strayStateEntries.map((entry) => ` ${entry}`), ].join('\n'); } -function previewCommand(command: string, stateDir: string): string { - const masked = command.replace( - new RegExp(`${escapeRegExp(stateDir)}(?=[\\s/]|$)`, 'g'), - '', - ); - return masked.length > COMMAND_PREVIEW_CHARS - ? `${masked.slice(0, COMMAND_PREVIEW_CHARS)}…` - : masked; -} - function formatPids(pids: readonly number[]): string { return pids.join(', ') || '(none)'; } - -function escapeRegExp(value: string): string { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} diff --git a/test/integration/support/daemon-leak-oracle.ts b/test/integration/support/daemon-leak-oracle.ts index 843935b857..75758b9f92 100644 --- a/test/integration/support/daemon-leak-oracle.ts +++ b/test/integration/support/daemon-leak-oracle.ts @@ -1,17 +1,9 @@ -// Daemon leak oracle (#1781 B1, feeds #1431): observes the host process table -// and an isolated state dir, then applies the ownership/residue rules in +// Daemon leak oracle (#1781 B1, feeds #1431): observes daemon liveness and an +// isolated state dir, then applies the lifecycle/residue rules in // `daemon-leak-model.ts` (which documents them and is where they are tested). -// -// What each caller actually exercises depends on what its daemon did. The three -// device-free daemon lanes guard surviving-daemon and state-dir residue rules. -// The Web smoke owns a real managed agent-browser + Chrome fleet, so its -// post-shutdown checkpoint makes the #1109 process arm fail on the shipped -// route. The model fixtures pin every ownership shape, including #1324's -// reparented recorder; both historical failures also have manual red-proofs. import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { runCmd } from '../../../src/utils/exec.ts'; import { isProcessAlive } from '../../../src/utils/host-process.ts'; import { AppError } from '@agent-device/kernel/errors'; import { @@ -20,16 +12,11 @@ import { hasDaemonLeaks, type DaemonLeakPhaseSelection, type DaemonLeakSnapshot, - type HostProcess, type StateEntry, } from './daemon-leak-model.ts'; -const PS_TIMEOUT_MS = 5_000; const DEFAULT_SETTLE_MS = 5_000; const SETTLE_POLL_MS = 250; -// Matches src/utils/host-process.ts: an absolute path so a PATH-resolved -// third-party `ps` (Homebrew, procps) cannot change the flag semantics. -const HOST_PS_COMMAND = process.platform === 'win32' ? 'ps' : '/bin/ps'; /** * `phase` carries the identity that phase needs: `after-close` cannot be @@ -60,21 +47,18 @@ async function captureDaemonLeakSnapshot( options: DaemonLeakOracleOptions, ): Promise { const stateDir = path.resolve(options.stateDir); - const processes = await listHostProcessesWithGroups(); return evaluateDaemonLeaks({ ...phaseSelectionOf(options), stateDir, daemonPids: options.daemonPids, livePids: options.daemonPids.filter((pid) => isProcessAlive(pid)), - processes, - excludedPids: [...ancestorsOf(process.pid, processes)], stateEntries: readStateEntries(stateDir, options.sessionsDir), }); } /** * Re-snapshots until clean or `settleMs` elapses. Stragglers that exit on their - * own inside the window are not leaks; #1324/#1109 orphans never exit on their own. + * own inside the window are not leaks. */ async function settleDaemonLeakSnapshot( options: DaemonLeakOracleOptions, @@ -152,67 +136,6 @@ function readDescriptorLifecycle(filePath: string): string | undefined { } } -// src/utils/host-process.ts's listHostProcesses covers pid/ppid/command; the -// ownership rules also need the process group and the environment, which need a -// second `ps` on macOS (`-E` appends the environment to the command column) and -// /proc on Linux. -async function listHostProcessesWithGroups(): Promise { - const darwin = process.platform === 'darwin'; - const [tree, darwinEnv] = await Promise.all([ - runPs(['-axww', '-o', 'pid=,ppid=,pgid=,command=']), - darwin ? runPs(['-E', '-axww', '-o', 'pid=,command=']) : Promise.resolve(''), - ]); - const envByPid = new Map( - parsePsLines(darwinEnv, /^\s*(\d+)\s+(.*)$/).map(([pid, env]) => [Number(pid), env ?? '']), - ); - return parsePsLines(tree, /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/).map( - ([pid, ppid, pgid, command]) => ({ - pid: Number(pid), - ppid: Number(ppid), - pgid: Number(pgid), - command: command ?? '', - env: darwin ? (envByPid.get(Number(pid)) ?? '') : readLinuxEnviron(Number(pid)), - }), - ); -} - -function parsePsLines(stdout: string, shape: RegExp): string[][] { - return stdout.split('\n').flatMap((line) => { - const match = shape.exec(line); - return match ? [match.slice(1)] : []; - }); -} - -async function runPs(args: string[]): Promise { - const result = await runCmd(HOST_PS_COMMAND, args, { - allowFailure: true, - timeoutMs: PS_TIMEOUT_MS, - }); - if (result.exitCode !== 0) { - throw new Error( - `daemon leak oracle: ps ${args.join(' ')} failed (${result.exitCode}): ${result.stderr}`, - ); - } - return result.stdout; -} - -function readLinuxEnviron(pid: number): string { - try { - return fs.readFileSync(`/proc/${pid}/environ`, 'latin1').replaceAll('\0', ' '); - } catch { - return ''; - } -} - -function ancestorsOf(pid: number, processes: readonly HostProcess[]): Set { - const byPid = new Map(processes.map((proc) => [proc.pid, proc])); - const chain = new Set(); - for (let cursor = pid; cursor > 0 && !chain.has(cursor); cursor = byPid.get(cursor)?.ppid ?? 0) { - chain.add(cursor); - } - return chain; -} - /** * Parses the standalone CLI's argv into oracle options, refusing the invocation * the type system already refuses in code: `--phase after-close` without a diff --git a/vitest.config.ts b/vitest.config.ts index 8231200b6b..1645984c59 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -89,11 +89,9 @@ export default defineConfig({ // #1781 A9: pins the root-doc paths-ignore entries directly against the // real workflow YAML, parse-only like its sibling above. 'test/ci/root-docs-paths-ignore.test.ts', - // The daemon leak oracle's ownership/residue rules (#1781 B1): pure - // decisions over fixture `ps` rows and state-dir listings, so they - // need no daemon, device, or subprocess. The Web smoke exercises - // #1109 with a real managed browser fleet; these fixtures pin every - // ownership rule, including #1324's reparented recorder shape. + // The daemon leak oracle's lifecycle/residue rules (#1781 B1): pure + // decisions over fixture state-dir listings, so they need no daemon, + // device, or subprocess. 'test/integration/support/daemon-leak-model.test.ts', // The frozen replay-compat corpus (#1417): parse-only, no device or // subprocess work, so it belongs in the fast lane next to the