From 456136659d723c2c6ddb4c92c54b2c50ab8c2d27 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 3 Sep 2026 13:42:14 -0400 Subject: [PATCH] fix(#4220): terminate the Windows temp-sweep ancestor walk (and two bugs it unmasked) (#4245) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4220): terminate the temp-sweep ancestor walk with a fixed-point check scripts/run-tests.cjs's sweepProtectSet block walked each selected test file's ancestor directories, stopping on `cur !== runTempRoot && cur.length > 1` — a POSIX-only sentinel. path.posix.dirname('/') === '/' (length 1) correctly stops, but path.win32.dirname('C:\\') === 'C:\\' (length 3) never satisfies the length check, so the walk spun forever on Windows whenever a selected file lived outside runTempRoot (the common case). This has hung every Windows CI shard since #4207. Extract the walk into a pure, exported computeSweepProtectSet(selected, runTempRoot, dirnameImpl) helper and replace the length sentinel with a fixed-point check (stop when dirnameImpl(cur) === cur), which terminates correctly on POSIX, Windows drive roots, and UNC roots alike with no platform branch. * fix(#4220): repoint TEMP/TMP alongside TMPDIR in run-tests-temp-root test child env Node's os.tmpdir() on Windows never reads TMPDIR, only TEMP/TMP. The test's runNode child-process env override only set TMPDIR, so on a real Windows runner nested inside a run-tests invocation the child inherited the outer process's already-repointed TEMP/TMP and its mkdtempSync(os.tmpdir()) landed under the outer run's temp root instead of the test's intended `outer` directory. This was masked on gsd-test's benches and locally because Windows CI always died in the #4220 infinite loop before reaching this test. * fix(#4220): stop the ancestor walk from protecting the filesystem root itself computeSweepProtectSet added `cur` to the protect set before checking whether dirname(cur) === cur, so on the terminating iteration it protected the filesystem root (posix `/`, and analogously a win32 drive root) instead of stopping before adding it. Caught by the existing posix-parity regression assertion (`!protectSet.has('/')`) on the linux-node24 gsd-test bench. Reorder to compute the parent and check the fixed point before adding. * fix(#4220): backfill changeset pr number to 4245 --------- Co-authored-by: sim --- .changeset/eager-geese-squeak.md | 5 ++ scripts/run-tests.cjs | 45 ++++++++--- tests/run-tests-temp-root.test.cjs | 124 ++++++++++++++++++++++++++++- 3 files changed, 160 insertions(+), 14 deletions(-) create mode 100644 .changeset/eager-geese-squeak.md diff --git a/.changeset/eager-geese-squeak.md b/.changeset/eager-geese-squeak.md new file mode 100644 index 000000000..1e79bc0b1 --- /dev/null +++ b/.changeset/eager-geese-squeak.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4245 +--- +**Windows CI test runs no longer hang indefinitely.** The test runner's temp-sweep protection walk used a POSIX-only termination check that never fired on a Windows drive root, spinning forever and timing out every Windows CI shard. A second, previously-masked bug in the temp-root regression test's own child-process env override (only `TMPDIR`, not `TEMP`/`TMP`) is also fixed, since Windows never reads `TMPDIR`. diff --git a/scripts/run-tests.cjs b/scripts/run-tests.cjs index 42ff7f377..bfdadf75c 100644 --- a/scripts/run-tests.cjs +++ b/scripts/run-tests.cjs @@ -957,6 +957,34 @@ function rankChunkFilesByWeight(files, weightOf, timingsTable) { }); } +// #4020 / #4220: pure helper extracted from main()'s temp-sweep protection +// block. Walks each selected file's ancestor directories, protecting every +// one from a later temp-sweep, stopping either at runTempRoot or at the +// filesystem root. Termination uses a FIXED-POINT check (stop when +// dirnameImpl(cur) === cur) instead of a POSIX-only length sentinel +// (`cur.length > 1`): path.posix.dirname('/') === '/' (length 1) correctly +// stops, but path.win32.dirname('C:\\') === 'C:\\' (length 3) never +// satisfies a length check, so the old logic spun forever on Windows +// whenever a selected file lived outside runTempRoot — the common case, +// since most selected test files live in the repo checkout, not the temp +// root (root-caused every Windows CI shard timing out since #4207). The +// injectable dirnameImpl lets tests exercise path.win32/path.posix behavior +// directly without a real Windows/POSIX runner. +function computeSweepProtectSet(selectedFiles, runTempRoot, dirnameImpl = require('path').dirname) { + const protectSet = new Set(); + for (const f of selectedFiles) { + let cur = f; + while (cur && cur !== runTempRoot) { + const parent = dirnameImpl(cur); + if (parent === cur) break; // cur IS the filesystem root — never protect it, just stop + protectSet.add(cur); + cur = parent; + } + if (cur === runTempRoot) protectSet.add(f); // exact-file case + } + return protectSet; +} + function main() { const args = process.argv.slice(2); const parsed = parseArgs(args); @@ -1135,18 +1163,7 @@ function main() { // chunk still runs — a harness may stage synthetic test files under the run // root (tests/run-tests-harness.test.cjs's 30-file chunking fixture). Protect // every ancestor of every selected file that lies INSIDE the run root. - const sweepProtectSet = new Set(); - { - const { dirname } = require('path'); - for (const f of selected) { - let cur = f; - while (cur && cur !== runTempRoot && cur.length > 1) { - sweepProtectSet.add(cur); - cur = dirname(cur); - } - if (cur === runTempRoot) sweepProtectSet.add(f); // exact-file case - } - } + const sweepProtectSet = computeSweepProtectSet(selected, runTempRoot); // Sandbox the overlay home so the loader's global scan ($GSD_HOME/.gsd/capabilities) // cannot read a developer's real installed capabilities during tests (ADR-1244 D2). @@ -1643,4 +1660,8 @@ module.exports = { setupRunTempRoot, sweepRunTempRoot, assertTempRootBounded, + // #4220: extracted for in-process unit coverage of the ancestor-walk + // termination logic (Windows drive-root hang fix) without a real + // Windows/POSIX runner. + computeSweepProtectSet, }; diff --git a/tests/run-tests-temp-root.test.cjs b/tests/run-tests-temp-root.test.cjs index d7bc97a80..4725d01c8 100644 --- a/tests/run-tests-temp-root.test.cjs +++ b/tests/run-tests-temp-root.test.cjs @@ -42,7 +42,10 @@ describe('#4020 — run-tests temp root', () => { const outer = createTempDir('gsd-4020-outer-'); t.after(() => cleanup(outer)); - const r = runNode(['-e', setupProbe], { timeoutMs: 30_000, env: { ...process.env, TMPDIR: outer } }); + const r = runNode(['-e', setupProbe], { + timeoutMs: 30_000, + env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer }, + }); assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`); const out = JSON.parse(r.stdout.trim().split(/\n/).pop()); assert.ok(out.base.startsWith('gsd-test-run-'), @@ -64,7 +67,10 @@ describe('#4020 — run-tests temp root', () => { const outer = createTempDir('gsd-4020-idem-'); t.after(() => cleanup(outer)); - const r = runNode(['-e', probe], { timeoutMs: 30_000, env: { ...process.env, TMPDIR: outer } }); + const r = runNode(['-e', probe], { + timeoutMs: 30_000, + env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer }, + }); assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`); assert.ok(JSON.parse(r.stderr.trim().split('\n').pop()).reuse === true, 'a second invocation with the root active reuses it'); @@ -175,4 +181,118 @@ describe('#4020 — run-tests temp root', () => { assert.ok(fs.existsSync(sibling), 'a nested runner never sweeps the shared root — sibling fixtures survive'); }); + + describe('#4220 — computeSweepProtectSet ancestor-walk termination', () => { + // The original inline block stopped the ancestor walk on + // `cur !== runTempRoot && cur.length > 1`, a POSIX-only sentinel: + // path.posix.dirname('/') === '/' (length 1) correctly stops, but + // path.win32.dirname('C:\\') === 'C:\\' (length 3) never satisfies a + // length check — dirname returns the SAME string forever, so the loop + // never terminates on Windows whenever a selected file lives outside + // runTempRoot (the common case: most selected test files are in the repo + // checkout, not the temp root). This bounds every assertion below with a + // hard iteration cap so a genuinely-hanging implementation fails FAST in + // this test run rather than wedging the suite. + const HARD_ITERATION_CAP = 10_000; + + // A dirnameImpl wrapper that throws once it has been called more times + // than a real ancestor walk could ever need, turning an infinite loop + // into a fast, loud test failure instead of a hang. + function boundedDirname(realDirnameImpl) { + let calls = 0; + return (p) => { + calls++; + if (calls > HARD_ITERATION_CAP) { + throw new Error( + `computeSweepProtectSet: dirname called >${HARD_ITERATION_CAP} times — ` + + `the ancestor walk did not terminate (regression of #4220)`, + ); + } + return realDirnameImpl(p); + }; + } + + test('terminates and protects ancestors on win32 paths outside runTempRoot', () => { + const runner = require('../scripts/run-tests.cjs'); + assert.equal(typeof runner.computeSweepProtectSet, 'function', + 'the runner must export computeSweepProtectSet for in-process verification'); + const win32 = require('node:path').win32; + // A file living entirely outside runTempRoot (the common case — most + // selected test files are in the repo checkout), walked with the + // Windows dirname implementation whose drive-root is NOT length 1. + const selected = ['C:\\repo\\tests\\a\\b.test.cjs']; + const runTempRoot = 'C:\\Users\\ci\\AppData\\Local\\Temp\\gsd-test-run-xyz'; + + const protectSet = runner.computeSweepProtectSet( + selected, runTempRoot, boundedDirname(win32.dirname), + ); + + assert.ok(protectSet.has('C:\\repo\\tests\\a\\b.test.cjs'), 'the file itself is protected'); + assert.ok(protectSet.has('C:\\repo\\tests\\a'), 'immediate parent is protected'); + assert.ok(protectSet.has('C:\\repo\\tests'), 'grandparent is protected'); + assert.ok(protectSet.has('C:\\repo'), 'walk reaches up to the drive-relative root'); + // The walk must have stopped — it must NOT contain the drive root + // itself looping forever, and the protect set must be finite/small. + assert.ok(protectSet.size < 20, 'the walk terminates with a small, bounded protect set'); + }); + + test('terminates and protects ancestors on win32 paths INSIDE runTempRoot (exact-file case)', () => { + const runner = require('../scripts/run-tests.cjs'); + const win32 = require('node:path').win32; + const runTempRoot = 'C:\\Users\\ci\\AppData\\Local\\Temp\\gsd-test-run-xyz'; + const selected = [`${runTempRoot}\\fixture\\nested\\c.test.cjs`]; + + const protectSet = runner.computeSweepProtectSet( + selected, runTempRoot, boundedDirname(win32.dirname), + ); + + assert.ok(protectSet.has(selected[0]), 'the file itself is protected'); + assert.ok(protectSet.has(`${runTempRoot}\\fixture\\nested`), 'immediate parent is protected'); + assert.ok(protectSet.has(`${runTempRoot}\\fixture`), 'grandparent is protected'); + assert.ok(!protectSet.has(win32.dirname(runTempRoot)), + 'the walk stops at runTempRoot and never protects its parent'); + }); + + test('terminates and protects ancestors on posix paths (parity with win32)', () => { + const runner = require('../scripts/run-tests.cjs'); + const posix = require('node:path').posix; + const selected = ['/repo/tests/a/b.test.cjs']; + const runTempRoot = '/synthetic-root/gsd-test-run-xyz'; + + const protectSet = runner.computeSweepProtectSet( + selected, runTempRoot, boundedDirname(posix.dirname), + ); + + assert.ok(protectSet.has('/repo/tests/a/b.test.cjs'), 'the file itself is protected'); + assert.ok(protectSet.has('/repo/tests/a'), 'immediate parent is protected'); + assert.ok(protectSet.has('/repo/tests'), 'grandparent is protected'); + assert.ok(protectSet.has('/repo'), 'walk reaches up toward the root'); + assert.ok(!protectSet.has('/'), + 'the walk stops before adding the filesystem root itself, mirroring win32 behavior'); + }); + + test('exact-file case on posix: selected file lies directly under runTempRoot', () => { + const runner = require('../scripts/run-tests.cjs'); + const posix = require('node:path').posix; + const runTempRoot = '/synthetic-root/gsd-test-run-xyz'; + const selected = [runTempRoot]; + + const protectSet = runner.computeSweepProtectSet( + selected, runTempRoot, boundedDirname(posix.dirname), + ); + assert.ok(protectSet.has(runTempRoot), 'the exact-file case protects runTempRoot itself'); + }); + + test('defaults dirnameImpl to the real platform path.dirname when not injected', () => { + const runner = require('../scripts/run-tests.cjs'); + const path = require('node:path'); + const os = require('node:os'); + const runTempRoot = path.join(os.tmpdir(), 'gsd-test-run-default'); + const selected = [path.join(runTempRoot, 'fixture', 'd.test.cjs')]; + + const protectSet = runner.computeSweepProtectSet(selected, runTempRoot); + assert.ok(protectSet.has(selected[0]), 'default dirnameImpl still protects the selected file'); + assert.ok(protectSet.has(path.join(runTempRoot, 'fixture')), 'default dirnameImpl walks ancestors'); + }); + }); });