test(tool-server): scope vega-cli-timeout sentinels to the running process - #711
Draft
latekvo wants to merge 1 commit into
Draft
test(tool-server): scope vega-cli-timeout sentinels to the running process#711latekvo wants to merge 1 commit into
latekvo wants to merge 1 commit into
Conversation
…ocess
The fake `vega` launcher's workers were marked with fixed sentinels (`sleep
69101`…`69107`), and `strayCount`/`sweep` match those against the command line of
every process on the machine. Two concurrent runs of this file therefore shared one
sentinel namespace: each run's `afterEach` sweep SIGKILLed the other run's live
workers, so the `hang` launcher's blocking `sleep` returned, the launcher exited 0,
and `runVega` resolved `{ stdout: "", stderr: "" }` at ~100ms instead of rejecting on
its 400ms deadline. That failed both `hang` tests — "rejects on its own deadline"
(`expected { stdout: '', stderr: '' } to be an instance of Error`) and "reaps the
ENTIRE worker tree on timeout" (`promise resolved instead of rejecting`).
Measured 0/20 in isolation but 2/15 with the tool-server suite running in parallel
(the suite runs this file too). Pinning the whole run to one CPU did not reproduce it
in 8 runs, so the 400ms budget is not the problem and the reap itself is sound — it
was purely the shared sentinel namespace.
Derive the sentinels from a per-process tag (pid plus three random digits, so a
worker leaked by an earlier run whose pid has been recycled is not mistaken for
ours). The tag rides in the fraction of the sleep duration rather than being the
duration, so a run killed before `afterAll` leaks its workers for ten minutes instead
of the previous nineteen hours - which matters more now that no later run shares the
namespace that would have cleaned them up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
vega-cli-timeout.test.tsfails intermittently, always the same two tests, always the same way:Both at ~60-130 ms against a 400 ms deadline —
runVegaresolved rather than timing out. It never fails in isolation.Not a tight budget, and not a reaping race
taskset -c 0), 8 runsThe single-CPU pin is the discriminator: if the 400 ms budget were too tight, or the reap raced under contention, starving it to one core would make it worse. It passes 8/8.
packages/tool-server/src/utils/vega-cli.tsis not implicated and is unchanged here.Cause: the sentinel namespace is machine-global
The test's worker sentinels were fixed constants (
sleep 69101…sleep 69107), and cleanup matches them withpkill -f/pgrep -f— which are global to the machine, not scoped to the process.Two concurrent runs of this file therefore share one namespace, and each run's
afterEachSIGKILLs the other run's live workers. The fakevegalauncher's blockingspawnSync("sleep", …)returns early, the launcher exits 0, andrunVegaresolves cleanly well inside its deadline — exactly the observed signature.The existing comment shows the pattern was already narrowed once ("Match only this suite's sentinels (6910[0-9]) rather than a loose
sleep 691"). That tightened it against other suites but not against a second copy of this suite, which is the case that actually occurs: CI matrices,vitest --no-file-parallelismreruns, and any machine running more than one checkout.Deterministic reproduction
A neighbour issuing this file's own sweep (
pkill -f '^sleep 6910[0-9]$') on a 20 Hz loop reproduces it every time:Fix
Derive the sentinels from a per-process tag (pid plus three random digits, so a recycled pid cannot collide) and scope every
pgrep/pkillto it. A concurrent run computes a different tag and its sweeps no longer reach these workers.The tag rides in the fractional part of the sleep duration, which also shortens a leaked worker's life from ~19 h to 10 min. That matters more than it looks: previously a later run of this file would clean up an abandoned worker as a side effect of the shared namespace, and scoping the namespace deliberately removes that accident.
Verification
^sleep 600\.[0-9]+$) still kills the fixed code 4/4. The workers remain real and killable; only the tag protects them, so the fix is not passing by making them unmatchable.collectDescendantPids→[]) makesstrayCountreport a live stray (expected 1 to be +0), proving the new escapedpgreppattern genuinely matches.RUN_TAGto a constant, with a neighbour using the pattern a concurrent run of the mutated file computes: 4/4 fail, killing precisely the two named tests.eslint .0,prettier --check .0,tsc --build0 — unpiped, exit codes read directly.Sibling audit
This is the only member of the class. Every candidate checked:
adb-terminal-error-format(mockschild_process),await-ui-element/await-screen-idle/mjpeg-stream(in-process timers, no subprocess),artifacts(realtar, no deadline assertion),list-devices(psmocked),vega-process(realps, read-only, owns no workers),launcher-helpers(TCP, no subprocess). None spawns real workers into a global namespace under a deadline.