test: refuse foreign-pid signals from unit-test workers (Coverage fork death, #1824) - #1854
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact |
…n the Coverage job Temporary instrumentation for #1824. Every vitest fork logs each real process.kill it sends to a foreign pid (and every kill/pkill it spawns); the Coverage job parks sentinel processes on the pids the Apple runner tests fabricate (4141/4242/4343/4444) and reports which of them survive the run. Reverted before this PR leaves draft.
A vitest worker may signal only itself and the processes it spawned. src/__tests__/hermetic-signal-setup.ts records any other process.kill, answers it with ESRCH (so best-effort kill paths proceed as if the pid were dead), and fails the sending test by name in afterEach. The senders this catches today are the Apple runner tests, which fabricate runner child pids (4242, 4141, 4343, 4444) and mocked the liveness reads in host-process.ts but not the signal writes: killRunnerProcessTree delivered real SIGINT/SIGTERM/SIGKILL to those pids and their process groups — 146 signals per run of runner-session.test.ts. On the CI runner the sibling vitest forks live in that pid band, so the Coverage job periodically lost one fork mid-file with no test attributed (issue #1824, 6 of the last 40 red CI runs). The group-signal write moves behind signalProcessGroupBestEffort in host-process.ts, next to signalPidsBestEffort, so the runner tests mock the signal seam in the same place they already mock the liveness reads. Refs #1824
2711b11 to
84ffcc7
Compare
…tinels in the Coverage job" This reverts commit a8a0200.
|
CI note (head The one outstanding check is Smoke Tests (Linux), which is stuck in Leaving this in draft until that lane is green rather than undrafting on a partial signal. It needs one more rerun once the mirror recovers; no code change is pending. |
…the mutation lane Review of #1854 found three gaps in the first pass: - The guard intercepted process.kill only, so the spawned half of the same function family was unguarded: runner-disposal spawns `pkill -P <pid>` and `pkill -f 'xcodebuild.*AgentDeviceRunner.env.session-...'`, and request-router-open.test.ts fired that pattern kill twice per suite run. On a developer machine with a live Apple runner, `pnpm test` could reach it. The setup file now refuses kill/pkill/killall spawns with ENOENT — which the best-effort callers already tolerate — and records them the same way; that test stubs the Apple tool seam. - vitest.mutation.config.ts hard-coded its own setupFiles list, so the Stryker lane ran without the guard. SETUP_FILES is now exported from vitest.config.ts and imported there, next to the SUBPROCESS_STUB_TESTS import that already crossed the same boundary. - 'processes it spawned' meant direct children only; a grandchild started through a shell wrapper was refused with advice that did not fit. The docs and the failure message now say direct children and name the remedy. Synchronous spawns are no longer remembered as own pids: spawnSync and execFileSync have already exited when they return, so keeping their pids would license a signal to whatever inherits them next. Refs #1824
|
Re-reviewed exact
Fresh exact-head CI is still pending. The body also needs updating: effective scope is 13 files, not 11, and the setup is 140 LOC, not ~85. |
|
CI note: the two iOS reds on this PR are not from this diff — filed as #1874. Run |
|
Undrafted on green CI. 28/28 checks pass on |
…xecFile path Re-review of #1854 found two holes in the guard itself, both the class it exists to close: - An async child's pid stayed authorized for the worker's lifetime after it was reaped. A pid is a claim on a process-table slot, and the kernel reissues that slot once it is free, so 'I spawned this pid once' licensed a signal to whatever holds it now. Authority now ends on exit/close. Cleanup that signals a child must gate on isProcessAlive, which is what the existing cleanup paths already do. - wrapSpawner copied execFile's original util.promisify.custom onto the wrapper, and promisify() resolves through that symbol instead of calling the function — so every promisified caller got an unguarded execFile, bypassing both the kill-binary refusal and child tracking. That path is now wrapped too. hermetic-signal-setup.test.ts covers both, plus the allowed cases they could regress into: a reaped child's pid is refused, a promisified execFile cannot smuggle a pkill, a promisified child is still tracked, a live foreign pid is refused, and signal 0 stays free. Reverting either fix reds three of them. Refs #1824
|
Both P1s were real, and both were the guard failing at its own premise. Fixed in 1. Authority now ends at child exit. You're right and my earlier reasoning was backwards: a pid is a claim on a process-table slot, not on a process, and the kernel reissues that slot once it's free — so "I spawned this pid once" was licensing exactly the reuse hazard the file exists to close. 2. The promisified path is wrapped. Regressions — new Body facts corrected: 14 files (not 11 — I'd counted only the first commit; 13 was right before this commit added the test file), and the setup file is 170 LOC with 94 LOC of tests, not ~85. CI: |
thymikee
added
the
ready-for-human
|
Re-reviewed exact Code review is clean. Exact-head CI is 27/28 green with iOS Smoke still pending; no device evidence applies to this test-harness/host-process change. |
|
1 participant
Summary
Refs #1824 — this closes one of two causes behind that signature; see "A second, independent cause" below. The issue stays open on its 20-run criterion.
The Coverage job's "Worker exited unexpectedly" was not OOM.
runner-session.test.tsandrunner-request-cancellation.test.tsfabricate runner child pids (makeBackgroundRunner(4242), plus4141/4343/4444) and mock the liveness reads insrc/utils/host-process.tsbut not the signal writes, sokillRunnerProcessTreedelivered realSIGINT/SIGTERM/SIGKILLto those pids and to their process groups (process.kill(-4242, …)) — 146 signals per run ofrunner-session.test.ts. On the ubuntu runner the sibling vitest forks live in exactly that pid band, so the job periodically killed one of its own forks: one file's results vanish, no test fails, and the pool reports an unhandled error 30–40 s later when the run settles.The negative-pid half is the sharper edge and the reason this is fixed at the seam rather than by renumbering the fake pids:
-4242addresses a whole process group, so a localpnpm testcan deliver SIGKILL to an unrelated group leader's children on a developer machine. Any constant a test invents is someone's live pid somewhere.Two changes, both at the owning interface:
signalProcessGroupBestEffortjoinssignalPidsBestEffortinsrc/utils/host-process.ts, andrunner-disposal.ts's rawprocess.kill(-pid, …)moves behind it. Signal writes now live in the same module as the liveness reads, so a test mocks both in one place — whichrunner-session.test.ts,runner-request-cancellation.test.ts, andrunner-disposal.test.tsnow do.src/__tests__/hermetic-signal-setup.ts(new, in the sharedSETUP_FILES— now exported fromvitest.config.tsand imported byvitest.mutation.config.ts, so the Stryker lane cannot drift off it) makes the class impossible rather than trusting each test: a worker may signal only itself and its own direct children. It closes both exits, because the runner-disposal family uses both —process.kill(answeredESRCH) and a spawnedkill/pkill/killall(answeredENOENT), whose-Pand-fforms reach processes the worker never spawned at all. Best-effort kill paths therefore proceed as if nothing matched, no write leaves the worker, and the sending test fails by name inafterEach/afterAll. Signal 0, the liveness probe, stays free; killing a daemon or Metro fixture the test itself spawned stays allowed.The spawned half is not hypothetical:
request-router-open.test.tsran a realpkill -f 'xcodebuild.*AgentDeviceRunner\.env\.session-…'twice per suite run, so on a developer's Macpnpm testcould reach a live runner. It now stubs the Apple tool seam, the way the runner tests stub the signal seam.runner-session.test.tswas over the 1,000-line tripwire, so its fabricated session/lease/child fixtures moved torunner-session-fixtures.ts(the sibling-fixtures pattern AGENTS.md prescribes) instead of growing the file; the size-ratchet pin ratchets down, 2083 → 2001.docs/agents/testing.mddocuments the invariant.Validation
Cause confirmed on CI, before the fix — run 32165517507, Coverage job 95804064013, instrumentation-only head
2711b116f. Sentinel processes were parked on 4141/4242/4343/4444 beforepnpm test:coverage:ci:The split is itself corroboration: 4242 dies 223 ms after the first
unit-corefile starts —runner-session.test.ts, which signals only 4242 — and the other three die together 69 s later, which is whererunner-request-cancellation.test.ts(4141/4242/4343/4444) runs.Fix verified with the same probe — run 32222448997, Coverage job 95975387817. Note this is a different base from the before-run (
2711b116f→84ffcc744, 71 files and +3272 lines of unrelatedmainbetween them), so it is the probe that is held constant, not the tree:Neither runner test appears in the trace at all any more; the only signals left in the whole run are tests killing daemon/Metro children they spawned themselves. (The report line
signals aimed at a pid that was also a vitest fork: 0printed in both runs and proves nothing — planting the sentinels burns ~1977 throwaway pids, which pushes every fork in the instrumented run above 4444. I am not offering it as a contrast.)What is observed vs inferred. Observed: the tests deliver real signals to those pids, the signals kill whatever holds them, and the fix stops both. Inferred: that the victim in the six catalogued deaths was a sibling fork. The arithmetic behind the inference — in an uninstrumented run the ~1977 planting pids do not exist, so the first
src/file's worker (pid 6128 before, 6121 after) would land near 6128 − 1977 ≈ 4151, inside 4141–4444 — plus the victims being exactly the files alive in that window. No run captures a fork's death certificate, so the mechanism evidence carries this, not a single smoking-gun frame.Vanished-file forensics — in all six recent deaths (runs 32041468051, 32044136488, 32047734673, 32126085337, 32151259761, 31783654175) the file whose results disappear is one of the three largest
unit-corefiles (snapshot-handler×3,runner-session×2,daemon-client×1) — exactly the files alive in the same scheduling window asrunner-session.test.ts, ~50–60 s into the run. The issue's "last ~40 s" was the report-print time, not the death time.Red proof, direct writes — guard on, tests unfixed:
runner-session.test.ts+runner-request-cancellation.test.tsfail 28 of 76, each naming the refused signal, the pid, and the delivery path:With the test-side mocks restored: 76/76 pass.
Red proof, the guard's own holes —
hermetic-signal-setup.test.tscovers the two ownership gaps re-review found. Reverting either fix reds three of its seven cases:signalProcessGroupBestEffortalso carries its own behavioral tests (delivery to-pid, ESRCH →false, and a refusal to touchprocess.killat all for pid ≤ 0, which would otherwise address the caller's own group).Green —
pnpm check:affected --rungreen on the final head (unit lane 2807/2807 with the guard active). Local full-suite runs under 100–280 host load flaked only on the documented contention files (ios-lifecycle,interaction-response-shape,fuzz/harness), which fail identically on plainorigin/mainunder the same load, and pass in isolation.Four lines
[vitest-pool]: Worker forks emitted error) behind 6 of the last ~40 red CI runs — and any future test that signals a pid it did not spawn, which now fails by name instead of killing an unrelated process or process group on the host.A second, independent cause (not fixed here)
PR #1866 hit the identical pool error on run 32223038279 after making
scripts/fuzz/corpus-replay.test.tsheavier (a large module graph pulled into the V8-instrumented run), and fixed it by shrinking that graph. I checked whether it was my mechanism with the timing shifted. It is not:corpus-replay.test.ts, asubprocess-stubmember. That project runsfileParallelism: false, maxWorkers: 1in a group that starts only afterunit-coredrains: lastunit-corefile ✓ 06:27:32, stub files 06:27:38 → 06:27:49, death reported 06:28:43. It was the only file executing.runner-session.test.ts✓ 17:27:02 vscorpus-replay.test.ts17:30:33. Their forks are gone, and late forks carry pids ~14 000, nowhere near 4141–4444.[vitest-pool]: Timeout terminating forks worker for … corpus-replay.test.tsand noJavaScript heap out of memory— a silent death of a bloated fork, i.e. an external SIGKILL such as the kernel OOM killer (which prints only todmesg), not an in-process V8 heap abort.So there are two death shapes behind #1824: (A) an early
unit-corefile dies while other files run and the fabricated-pid signals are in flight — that is what the sentinels proved, dead before / alive after, and what this PR fixes; (B) a single heavy instrumented file dies alone in the tail under memory pressure — #1866's, unaffected by this PR. Five of the six deaths I catalogued have shape (A); #1866's has shape (B). I am not claiming this PR closes (B), and the 20-run criterion is what distinguishes a real recurrence from a fluke either way.Residual risk
The guard covers
process.killand spawnedkill/pkill/killallfrom a test worker. What it cannot see is a signal sent from inside a subprocess a test spawned (that process's ownprocess.killruns in another runtime); no such sender exists in the repo today. And shape (B) above remains open: if the tail deaths continue, the follow-up is a fork heap ceiling or a coverage shard, not this seam.Touched files: 14 across the three commits (the earlier "11" counted only the first commit, not the review follow-ups). Scope stayed inside the test harness plus the Apple runner disposal seam.