From 6113d1196ea90166bee58769d927e296613e10bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 09:21:12 +0200 Subject: [PATCH 1/3] fix(test): serialize the lane whose files sweep the shared run-directory root `check:tmpdir-leaks:test` runs four files in one `node --test` invocation, which parallelizes per file by default. Two of them sweep the real shared root: node-test-tmpdir.test.ts asserts `pruneAbandonedRunDirectories(TEST_RUN_TMP_ROOT)` both keeps and removes a planted directory, and vitest-tmpdir-global-setup.test.ts drives a real `vitest` whose global setup performs that same sweep. One file's sweep can therefore delete the run directory the other is mid-flight on, and the orphan test's "the next run prunes it" assertion observes `[]` instead of the directory. It passed 5/5 locally and failed on a CI runner with exactly that `actual: []`; the outcome depends on process interleaving, not the code. Forward `--test-concurrency=1` (cost measured at +2s: 3.4s -> 5.5s) and pin it. The pin checks the flag sits among the args the wrapper forwards to its child, not merely anywhere in the command: the wrapper spawns `node --test`, so a flag written before the wrapper path is parsed by the wrapper's own node process and never reaches the runner, leaving the lane silently parallel. Verified that misplaced form fails the new test. --- package.json | 2 +- scripts/node-test-tmpdir.test.ts | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 6b36917657..52f9fa0e6c 100644 --- a/package.json +++ b/package.json @@ -161,7 +161,7 @@ "check:xctest-selection": "node --experimental-strip-types scripts/check-xctest-selection.ts", "check:packaged-runner-swift": "node --experimental-strip-types scripts/check-packaged-runner-swift.ts", "check:tmpdir-leaks": "node --experimental-strip-types scripts/check-tmpdir-leaks.ts", - "check:tmpdir-leaks:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/check-tmpdir-leaks-model.test.ts scripts/vitest-tmpdir-global-setup.test.ts scripts/node-test-tmpdir.test.ts scripts/swift-toolchain-tmpdir.test.ts", + "check:tmpdir-leaks:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test --test-concurrency=1 scripts/check-tmpdir-leaks-model.test.ts scripts/vitest-tmpdir-global-setup.test.ts scripts/node-test-tmpdir.test.ts scripts/swift-toolchain-tmpdir.test.ts", "check:freerange": "fr", "check:quick": "pnpm lint && pnpm typecheck", "sync:mcp-metadata": "node scripts/sync-mcp-metadata.mjs", diff --git a/scripts/node-test-tmpdir.test.ts b/scripts/node-test-tmpdir.test.ts index cd490d2207..cdb23cfbb3 100644 --- a/scripts/node-test-tmpdir.test.ts +++ b/scripts/node-test-tmpdir.test.ts @@ -256,6 +256,53 @@ test('every node --test package.json script routes through scripts/node-test-tmp ); }); +// These two files each sweep the real shared root: this file asserts +// `pruneAbandonedRunDirectories(TEST_RUN_TMP_ROOT)` both keeps and removes a +// planted directory, and the Vitest lifecycle file drives a real `vitest` whose +// global setup sweeps the same root. `node --test` runs files in parallel by +// default, so one file's sweep can remove the directory the other is mid-flight +// on: the orphan test's "the next run prunes it" assertion then observes `[]` +// and fails. Whether it does depends on process interleaving, not the code — +// it passed 5/5 locally and failed on a CI runner with `actual: []`. +// Serialization is the fix; this keeps it an invariant rather than a flag +// someone can drop while "cleaning up" the lane. +const GLOBAL_SWEEP_TEST_FILES = [ + 'scripts/node-test-tmpdir.test.ts', + 'scripts/vitest-tmpdir-global-setup.test.ts', +]; + +// The flag must sit among the args the wrapper FORWARDS to its child: the +// wrapper spawns `node --test`, so a flag written before the +// wrapper path is parsed by the wrapper's own node process and never reaches +// the runner — the lane would silently go parallel while still containing the +// string. Everything after the wrapper path is what the child receives. +function forwardedNodeArgs(command: string): string { + const wrapperIndex = command.indexOf('scripts/node-test-tmpdir.ts'); + return wrapperIndex === -1 + ? '' + : command.slice(wrapperIndex + 'scripts/node-test-tmpdir.ts'.length); +} + +test('lanes that sweep the shared run-directory root serialize their files', () => { + const manifest = JSON.parse( + fs.readFileSync(path.join(REPOSITORY_ROOT, 'package.json'), 'utf8'), + ) as { scripts?: Record }; + + const unsynchronized = Object.entries(manifest.scripts ?? {}) + .filter(([, command]) => GLOBAL_SWEEP_TEST_FILES.every((file) => command.includes(file))) + .filter(([, command]) => !forwardedNodeArgs(command).includes('--test-concurrency=1')) + .map(([name]) => name); + + assert.deepEqual( + unsynchronized, + [], + `these scripts run the global run-directory sweep from several test files in parallel, so a ` + + `sweep from one file can remove the directory another file is asserting on: ` + + `${unsynchronized.join(', ')}. Add --test-concurrency=1 after scripts/node-test-tmpdir.ts ` + + `so the child runner receives it.`, + ); +}); + // INT32_MAX exceeds every platform's pid range (Linux pid_max caps at 2^22, // macOS at 99999), so kill(pid, 0) is ESRCH by construction — an owner that // is dead and can never be reused mid-test, unlike a freshly exited child's pid. From 9b9d18499c11f0d83cdb4dadf189dfc2db6b9385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 09:21:12 +0200 Subject: [PATCH 2/3] fix(test): stop reading a live host process as the recorded browser owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The orphan-cleanup test recorded PID 101 and asserted the reaper cleared the record. 101 is only reliably dead on a fresh CI runner: on a Mac with a simulator runtime mounted it is `appleaccountd`, so the reaper inspects a live process, reads it as ownership-lost, retains the record rather than clearing it, and the test fails on that machine while staying green on CI. Use the `NEVER_A_PID` idiom the tmpdir lanes already use — INT32_MAX is outside every platform's pid range, so the owner is dead by construction on every host and cannot be recycled mid-test. That is also the branch the test means to exercise: a recorded owner that already exited, hence a stale record that must be cleared. --- .../src/agent-browser-lifecycle.test.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/platform-web/src/agent-browser-lifecycle.test.ts b/packages/platform-web/src/agent-browser-lifecycle.test.ts index bc3ef2888a..35a04f5497 100644 --- a/packages/platform-web/src/agent-browser-lifecycle.test.ts +++ b/packages/platform-web/src/agent-browser-lifecycle.test.ts @@ -204,6 +204,14 @@ test('cleanup does not treat the shared socket directory mtime as browser activi } }); +// INT32_MAX exceeds every platform's pid range, so kill(pid, 0) is ESRCH by +// construction: a recorded owner that is dead on every host and cannot be +// recycled mid-test. A literal like 101 is not — on a Mac with a simulator +// runtime mounted, PID 101 is a live `appleaccountd`, the reaper reads it as +// ownership-lost, retains the record instead of clearing it, and this test goes +// red on that machine while staying green on CI. +const NEVER_A_PID = 2_147_483_647; + test('cleanup reads recorded browser identities without reconstructing a process tree', async () => { const stateDir = mkdtempForTestSync('agent-device-web-life-'); const originalIdleTimeout = process.env.AGENT_BROWSER_IDLE_TIMEOUT_MS; @@ -216,9 +224,9 @@ test('cleanup reads recorded browser identities without reconstructing a process status: 'decoded' as const, records: [ { - pid: 101, - startTime: 'start-101', - command: 'command-101', + pid: NEVER_A_PID, + startTime: `start-${NEVER_A_PID}`, + command: `command-${NEVER_A_PID}`, purpose: 'managed-web-browser', }, ], @@ -235,7 +243,7 @@ test('cleanup reads recorded browser identities without reconstructing a process ownedProcessRecords, }); - assert.deepEqual(result.pids, [101]); + assert.deepEqual(result.pids, [NEVER_A_PID]); assert.deepEqual(result.signalPids, []); assert.equal(mockRunCmd.mock.calls.length, 0); expect(ownedProcessRecords.clear).toHaveBeenCalledOnce(); From 338e8079bad79668652ad0319a1cfeb9c8772dc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 14:45:18 +0200 Subject: [PATCH 3/3] test: tighten the serialization pin to the forwarded token, not a substring Review found the pin's `includes('--test-concurrency=1')` accepted two shapes that still run files in parallel: `--test-concurrency=10`, and the flag sitting in a later `&&` segment of a command whose sweep segment never serializes. Split the command the way the wrapper ratchet above already does, read the tokens the wrapper forwards to its child, and require exactly one that equals `--test-concurrency=1`. A wrapper path that is not a standalone token reports the lane unsynchronized rather than guessing. Mutations verified to fail the pin: concurrency `10`, flag removed, flag moved before the wrapper path, a `--test-concurrency=1` decoy in a later segment, a duplicated flag, and a `./`-prefixed wrapper path. Soften the NEVER_A_PID comment to the claim the code can support. Windows pids are 32-bit, so INT32_MAX does not sit outside every platform's pid range by construction; it sits above any pid a real host mints in practice, which is what makes the recorded owner absent here. --- .../src/agent-browser-lifecycle.test.ts | 14 +++--- scripts/node-test-tmpdir.test.ts | 46 ++++++++++++++----- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/packages/platform-web/src/agent-browser-lifecycle.test.ts b/packages/platform-web/src/agent-browser-lifecycle.test.ts index 35a04f5497..325d3d5fbd 100644 --- a/packages/platform-web/src/agent-browser-lifecycle.test.ts +++ b/packages/platform-web/src/agent-browser-lifecycle.test.ts @@ -204,12 +204,14 @@ test('cleanup does not treat the shared socket directory mtime as browser activi } }); -// INT32_MAX exceeds every platform's pid range, so kill(pid, 0) is ESRCH by -// construction: a recorded owner that is dead on every host and cannot be -// recycled mid-test. A literal like 101 is not — on a Mac with a simulator -// runtime mounted, PID 101 is a live `appleaccountd`, the reaper reads it as -// ownership-lost, retains the record instead of clearing it, and this test goes -// red on that machine while staying green on CI. +// INT32_MAX is above any pid a real host mints in practice — Linux pid_max caps +// at 2^22, macOS at 99999, and a Windows pid counter would need hundreds of +// millions of process creations to get near 2^31 — so on every host this suite +// runs on the owner is absent and cannot be recycled mid-test. A literal like +// 101 is not: on a Mac with a simulator runtime mounted, PID 101 is a live +// `appleaccountd`, the reaper reads it as ownership-lost, retains the record +// instead of clearing it, and this test goes red on that machine while staying +// green on CI. const NEVER_A_PID = 2_147_483_647; test('cleanup reads recorded browser identities without reconstructing a process tree', async () => { diff --git a/scripts/node-test-tmpdir.test.ts b/scripts/node-test-tmpdir.test.ts index cdb23cfbb3..72afb2d967 100644 --- a/scripts/node-test-tmpdir.test.ts +++ b/scripts/node-test-tmpdir.test.ts @@ -271,16 +271,40 @@ const GLOBAL_SWEEP_TEST_FILES = [ 'scripts/vitest-tmpdir-global-setup.test.ts', ]; -// The flag must sit among the args the wrapper FORWARDS to its child: the -// wrapper spawns `node --test`, so a flag written before the -// wrapper path is parsed by the wrapper's own node process and never reaches -// the runner — the lane would silently go parallel while still containing the -// string. Everything after the wrapper path is what the child receives. -function forwardedNodeArgs(command: string): string { - const wrapperIndex = command.indexOf('scripts/node-test-tmpdir.ts'); - return wrapperIndex === -1 - ? '' - : command.slice(wrapperIndex + 'scripts/node-test-tmpdir.ts'.length); +// Serialization must reach the CHILD runner, and must mean one file at a time. +// The wrapper spawns `node --test`, so a flag written +// before the wrapper path is parsed by the wrapper's own node process and never +// gets there; a bare substring test would also accept `--test-concurrency=10`, +// which is ten files in flight and the same race. Read the forwarded tokens of +// the one segment that runs both sweep files, the way the wrapper test above +// segments `&&` chains. +const TEST_CONCURRENCY_ONE = '--test-concurrency=1'; +const WRAPPER_SCRIPT_TOKEN = 'scripts/node-test-tmpdir.ts'; + +function sweepSegmentRunsParallel(command: string): boolean { + const segment = command + .split('&&') + .find( + (part) => + part.includes(WRAPPER_SCRIPT_TOKEN) && + GLOBAL_SWEEP_TEST_FILES.every((file) => part.includes(file)), + ); + if (segment === undefined) { + return false; + } + + const tokens = segment.trim().split(/\s+/); + const wrapperIndex = tokens.indexOf(WRAPPER_SCRIPT_TOKEN); + if (wrapperIndex === -1) { + // The wrapper is only ever invoked with its path as a standalone argument; + // any other shape is one this check cannot reason about, so report it as + // unsynchronized rather than guessing. + return true; + } + + const forwarded = tokens.slice(wrapperIndex + 1); + const declared = forwarded.filter((token) => token.startsWith('--test-concurrency=')); + return declared.length !== 1 || declared[0] !== TEST_CONCURRENCY_ONE; } test('lanes that sweep the shared run-directory root serialize their files', () => { @@ -290,7 +314,7 @@ test('lanes that sweep the shared run-directory root serialize their files', () const unsynchronized = Object.entries(manifest.scripts ?? {}) .filter(([, command]) => GLOBAL_SWEEP_TEST_FILES.every((file) => command.includes(file))) - .filter(([, command]) => !forwardedNodeArgs(command).includes('--test-concurrency=1')) + .filter(([, command]) => sweepSegmentRunsParallel(command)) .map(([name]) => name); assert.deepEqual(