test: give each launcher-spawning file its own TMPDIR - #347
Conversation
d53d23d to
cd40180
Compare
The launcher writes cache-fix-proxy-<port>.sha256 under os.tmpdir() on every
spawn, and the tests spawn it with `{ ...process.env }`. So each run left those
records in the shared /tmp, and 2,145 had accumulated on this host.
Set once per file rather than at each spawn site, because the env is inherited —
this also covers sites added later, which is what went wrong the first time.
proxy-held-port already had a private TMPDIR at one of its eleven spawn sites,
with a comment explaining exactly why ("Asserting a global path can only ever
measure the machine's history"); the other ten never got it.
Measured with a private TMPDIR per run, before -> after:
proxy-held-port 24 -> 0 (34/34)
proxy-holder-handover 10 -> 0 (11/11)
proxy-wrapper 7 -> 0 (45/45)
stdio-epipe-survival 2 -> 0 (4/4)
proxy-shutdown-once 1 -> 0 (3/3)
proxy-server 0 -> 0 (32/32) consistency, not a fix
The counts include the ccf-runsvc-, ccf-wild- and ccf-v6- directories these
files also left behind, not only the .sha256 records.
proxy-wrapper is a different family from the same cause: runWrapper forks the
wrapper with the inherited env and the launcher writes its scratch CA under
os.tmpdir(), so seven cache-fix-ca-scratch-* dirs survived each run for the seven
days its reaper waits. It goes through that file's own tempDir() registrar rather
than a raw mkdtempSync — the file has a meta-assertion requiring exactly that,
and it caught the first attempt.
In five of the six the cleanup hook is appended at EOF, so it cannot delete the
dir out from under a sweep that reaps ports after the cases: node runs root hooks
in registration order. proxy-wrapper is the exception — its removal rides the
file's existing tempDirs hook near the top, which is harmless there because that
is the file's only root hook, but the property does not hold uniformly.
Co-Authored-By: Claude <noreply@anthropic.com>
cd40180 to
6b1be11
Compare
|
Field data from running this branch, in case it is useful for review. Before (a tree without this PR or #345): three consecutive full-suite runs left 102 orphaned launcher fingerprint records in the shared temp directory — one per launcher the suite spawned, on ephemeral ports, all with no surviving process. They accumulate silently: nothing in the suite output mentions them, and each run adds to the pile left by the last. After (a tree carrying this PR and #345): a full-suite run left zero records in the shared temp directory. Same suite, same machine, same shell, back to back. What this does and does not attribute. Giving each launcher-spawning file its own Why the shared directory matters beyond tidiness. Happy to re-run with any variation that would make the attribution sharper. 🤖 Generated with Claude Code — Proxy Builder |
The launcher publishes `cache-fix-proxy-<port>.sha256` and its scratch CA under its own os.tmpdir(), so a test run wrote both into the shared /tmp. On a box that also runs the product, that record is named after a port a live holder may be serving. Measured at the parent commit, one run of proxy-shutdown-once.test.mjs into a private TMPDIR: one cache-fix-proxy-<port>.sha256 left behind. After this change the same run leaves the directory empty, and a full suite run leaves no cache-fix-* entry at all. The mechanism is an env var because the launcher is a child process and reads its OWN os.tmpdir(). node:test gives each file its own process, so one directory per file falls out of that, and later spawn sites are covered without being edited. Cleanup hangs off `exit` rather than an after() hook: it runs after the port sweeps, so there is no hook-registration order to get right, and it is armed before the importing file's body instead of at the end of it. Measured, a module-level failure between the mkdtemp and an after() registration leaves the directory behind; against this module the same failure leaves nothing. test/tmp-isolation.test.mjs measures the property through a real process boundary rather than reading the source: the probe and a grandchild both write markers and check them, and the tmpdir the probe inherited must be empty once it exits. Red both ways - dropping the assignment leaves the markers there, dropping the cleanup leaves the private dir there. Co-Authored-By: Claude <noreply@anthropic.com>
|
The failing caseIt is not the TMPDIR isolationThree arms, node 20, the exact case, whole file. Round 2 interleaved the arms
45/45 green, so there is no rate difference to attribute. Green runs alone
Where it does live
srv.on("error", (e) => {
const held = srv.listening;
process.stderr.write(...);
if (!held) process.exit(1);The test asserts the standby is already on the port before the kill, so the I could not reproduce it locally in 45 runs, so that is where the evidence Why it cannot be proven from a CI log, and why the obvious fix is wrongThat handler writes "the listen failed, so there is no descriptor left to Giving it an inherited stderr is not the fix, and the code says why in its The comment's answer is that what it would have said is on No code change on this branch: adding a retry or widening the assertion would 🤖 Generated with Claude Code |
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex review: PR #347 round 1
Review: PR #347 TMPDIR-per-launcher-test isolation
Date: 2026-08-26
Reviewed: PR #347 (a2834ba0a0e4e7ae124c72bab5c2f1fc75f68237) against base 78ca94837129636de543df412edcea71fe0da6c8
Round: 1
Label applied: changes-requested
What Is Correct
Measured: applying the PR diff to a fresh clone at base 78ca94837129636de543df412edcea71fe0da6c8 succeeded cleanly.
Measured: the affected launcher-spawning tests and new smoke test pass under parallel execution on Node v24.11.1:
node --test --test-concurrency=6 test/proxy-held-port.test.mjs test/proxy-holder-handover.test.mjs test/proxy-server.test.mjs test/proxy-shutdown-once.test.mjs test/proxy-wrapper.test.mjs test/stdio-epipe-survival.test.mjs test/tmp-isolation.test.mjs -> tests 130, pass 130, fail 0.
Measured: the same focused command under Node v20.20.2 also passed locally (tests 130, fail 0, skipped 2). The GitHub Node 20 job is still red on the PR head, but the logged failure is a holder handover race in test/proxy-holder-handover.test.mjs:551, not a direct tmpdir assertion: actual: 'ECONNREFUSED', expected: 'pong'.
Measured: a forced-failure probe importing test/file-tmpdir.mjs, writing under os.tmpdir(), and then throwing exited nonzero and left the inherited outer temp directory empty: {"status":1,"entries":[],"stderrHasForced":true}. The diff commits to deterministic cleanup on process exit rather than preserving failure scratch for debug.
Read: test/file-tmpdir.mjs:16 uses mkdtempSync(join(tmpdir(), "ccf-test-")), so the directory name is atomically unique; there is no PID or timestamp collision window. test/file-tmpdir.mjs:18-19 registers cleanup on process exit, so normal pass and assertion-failure exits both run the cleanup path.
Read: test/tmp-isolation.test.mjs:23-44 exercises the helper through a child and grandchild process and asserts the inherited outer tmpdir is empty after the child exits, which is the right shape for guarding an env-inheritance helper instead of relying only on side effects.
Read: PR #346 uses per-test mc- scratch directories inside test/proxy-microcompact-stability.test.mjs; PR #347 uses a file-level ccf-test- temp root in launcher-spawning test files. They do not conflict on the same TMPDIR keys or scratch prefixes.
Blockers
- Windows still writes to the shared temp root, and the smoke test can pass while proving the wrong path. [Read]
test/file-tmpdir.mjs:17 only assigns process.env.TMPDIR = FILE_TMP. Node's documented os.tmpdir() behavior is platform-specific: on Windows it is overridden by TEMP and TMP; TMPDIR is the non-Windows override path (Node docs: https://nodejs.org/api/os.html#ostmpdir). That means a Windows launcher child that calls os.tmpdir() will ignore this helper's TMPDIR assignment and continue using the process's inherited TEMP/TMP shared scratch directory.
The new smoke test has the same blind spot. test/tmp-isolation.test.mjs:38-44 spawns the probe with only TMPDIR: outer and then asserts outer is empty. On Windows, outer is empty because os.tmpdir() never used it, not because scratch was isolated and cleaned. This is exactly the load-bearing risk in the commissioning brief: green suite, wrong assertion. Set all relevant temp env keys for the child path (TMPDIR, TMP, and TEMP, with care for existing semantics) and make the smoke assertion prove the key that the current platform's os.tmpdir() actually reads.
What Needs Attention
Measured: CI status at review time is not green. PR head a2834ba0a0e4e7ae124c72bab5c2f1fc75f68237 has test (18) success, test (22) success, security checks success, and test (20) failure. Because the local Node 20 focused rerun passed, this may be a pre-existing/flaky holder-handover test race rather than introduced by this PR, but it remains a red supported-runtime check on a touched test file and should be resolved or rerun green before approval.
Measured: residual grep over the touched tests for TMPDIR, /tmp, tmpdir(), mkdtemp, cache-fix-proxy-, and cache-fix-ca-scratch- found no un-isolated launcher scratch path introduced by this PR. Remaining tmpdir() uses are inside files importing file-tmpdir.mjs first, or explicit per-case temp overrides such as the existing unwritable-TMPDIR test.
Bloat / Non-Functional
None. Production LOC is 0. This is test-only infrastructure. The new helper plus one smoke test is proportionate to the repeated launcher-spawn sites it covers, and there are no new production exports, config keys, or runtime paths.
Recommendations
Update test/file-tmpdir.mjs to set the platform-relevant temp environment variables inherited by child launchers, not just TMPDIR. The smoke test should also validate the actual os.tmpdir() value used inside the probe and grandchild is under the private root on the current platform, so a Windows run cannot pass merely because the asserted outer directory was never used.
After that change, rerun the focused test set under parallelism and get the PR's Node 20 CI check green or explicitly document a rerun showing the CI failure was unrelated flake.
Bottom Line
Request changes. The POSIX/Linux behavior measured cleanly, and the implementation is close, but the helper is load-bearing test infrastructure and currently misses Windows because it only sets TMPDIR. That defeats the stated cross-platform isolation goal silently.
— Codex, cross-LLM review, round 1
vsits-codex-review-agent
Bot
added
changes-requested
…eads `os.tmpdir()` is platform-specific: TMPDIR on POSIX, TEMP then TMP on Windows. The helper set TMPDIR alone, so a Windows run kept writing launcher records and scratch CAs to the shared temp root -- the exact state this exists to prevent. The smoke case could not report it. It spawns its probe with `TMPDIR: outer` and asserts `outer` is empty; on Windows `outer` is empty because os.tmpdir() never looked at it, not because anything was isolated. A green suite proving the wrong path. Both halves fixed: the helper sets all three keys, and the probe spawn points all three at the inherited directory so the redirect is actually exercised whatever the platform reads. The new case asserts the property directly -- every key os.tmpdir() can read names the private dir -- which is measurable on POSIX and is the thing Windows depends on. RED: still on the inherited root: TMP,TEMP GREEN: 2/2 in the file; 83/83 across the suites that import the helper mutation: reverting to TMPDIR alone kills exactly the new case Co-Authored-By: Claude <noreply@anthropic.com>
…t see a miss Two corrections to the commit before this one, both mine. The comment said "TMPDIR on POSIX, TEMP then TMP on Windows". Measured from node's own `os.tmpdir`: POSIX reads TMPDIR || TMP || TEMP, so `TMP` alone and `TEMP` alone each work there too, and Windows reads TEMP || TMP. The reason to set all three is that a DIFFERENT one is read FIRST per platform, not that the others are ignored. The separate case asserting all three keys agree was green against a helper that redirects nothing: the spawn hands the child all three pointing at the inherited directory, so with no redirect they still agree. Agreement was never the property -- movement is. Folded into the probe that already runs inside the redirected child, where the existing "the inherited directory is empty" assertion supplies the movement half. One case now, thirty lines shorter, and strictly stronger: redirect removed -> dies on the inherited directory not being empty TMPDIR alone -> dies naming TMP,TEMP cleanup removed -> dies on the inherited directory not being empty Three for three against the previous shape's two. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — the Windows blind spot was real and both halves are fixed at a later head. Reviewed: The helper sets every key, not the one this platform happens to read first: // ALL THREE, because os.tmpdir() reads a DIFFERENT one first per platform:
// TMPDIR || TMP || TEMP on POSIX, TEMP || TMP on Windows.
for (const key of ["TMPDIR", "TMP", "TEMP"]) process.env[key] = FILE_TMP;The smoke test no longer passes by proving the wrong path. It was the { env: { ...process.env, TMPDIR: outer, TMP: outer, TEMP: outer } }
The grandchild is deliberate: the launcher is a CHILD reading its own Measured at head: Could you re-review at 🤖 Generated with Claude Code |
1 participant
The shared /tmp is not the launcher's
Eight test files spawn the launcher, and the launcher writes under its own
os.tmpdir()—cache-fix-proxy-<port>.sha256and a scratch CA. On a box that is also running the product, that record is named after a port a live holder may be serving, and the suite's copies land beside the real ones.Each spawning file now gets a private temp directory, set at module scope before its body runs:
The env var rather than a path threaded through each spawn: the launcher is a child and reads its own
os.tmpdir(), so the redirection only reaches it by being inherited.node:testgives each file its own process, so one directory per file falls out of that.Every key
os.tmpdir()reads, not just this platform'sMeasured from node's own
os.tmpdirsource, and confirmed empirically on 18 / 20 / 24:TMPDIR→TMP→TEMP→/tmpTEMP→TMP→SystemRoot\tempSetting only
TMPDIRleft a Windows run writing to the shared root — and the smoke case could not report it, because it spawned its probe withTMPDIR: outerand assertedouterwas empty. On Windowsouteris empty becauseos.tmpdir()never looked at it, not because anything was isolated. A green suite proving the wrong path.All three keys are set now, and the probe spawn points all three at the inherited directory so the redirect is actually exercised whatever the platform reads.
Cleanup on
exit, notafter()process.on("exit")runs after every hook, so it cannot delete the directory out from under a sweep that reaps ports once the cases are done, and it is armed before the importing file's body runs rather than at the end of it.Verification
The case drives a real child and checks both markers — its own and its grandchild's — before asserting the inherited directory is empty, because an empty directory proves nothing if neither was ever written.
the process left scratch in the tmpdir it inheritedTMPDIRalone, as it wasstill on the inherited root: TMP,TEMPthe process left scratch in the tmpdir it inheritedThree for three, from one case. An earlier revision split this across two and the second was green against a helper that redirects nothing — it asserted the three keys agreed, and with all three pointed at the inherited directory they agree whether or not anything moved. Agreement was never the property; movement is.
8 files changed, +104. New files: 2 (
test/file-tmpdir.mjs,test/tmp-isolation.test.mjs). New exports, env vars: 0 —TMPDIR/TMP/TEMPare set, not introduced.Known, and not fixed here
ubuntu-latest×3; the one macOS job is path-filtered to a shim and never runsnpm test). The Windows half is reasoned from node's source and a resolution-order simulation, not from a Windows run.TMPDIRoverrides elsewhere in the suite stay Windows-blind (proxy-wrapper.test.mjs:1143,:1338,:1343;proxy-held-port.test.mjs:1994), and two launcher-spawning files do not import the helper (proxy-probe-bounded.test.mjs:86,install-service.test.mjs:830). Both measured leaving nothing behind today.