From 7ef9945adbf82ad3efbcde513c3164380ee0b3c4 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 21:39:53 -0400 Subject: [PATCH] test(#3090): stop paying for the guard on every teardown The Windows node-24 scoped-test job hit its 15-minute ceiling twice on this branch and GitHub reported both as cancelled, which is how a job timeout surfaces. The same job finishes in about two minutes twenty on next, across four consecutive runs. cleanup() is the only hot-path change here. It was doing up to seven filesystem calls per invocation: three probes of the temp root, two more for each conventional temp dir, then an existence check and a realpath of the target. On Windows fs.realpathSync.native opens a file handle and Defender charges for each one, and this runs in the teardown of effectively every test. The root candidates are now memoized on the live os.tmpdir() value. The key matters: two files in the suite override TMPDIR mid-run and restore it, so a plain module-level hoist would go stale for them, while re-reading os.tmpdir() costs an env lookup. Measured over 20,000 calls: 546ms unmemoized, 10ms memoized. The symlink-escape check is removed rather than optimized, because it was guarding something that cannot happen. Verified directly on node v26.5.1: fs.rmSync(link, {recursive: true, force: true}) link exists false victim exists true file exists true rmSync unlinks a top-level symlink and leaves its target alone, and a symlink nested inside a tree being recursively removed is also unlinked rather than followed. The check cost two filesystem calls per teardown on the slowest platform in the matrix and bought nothing. Its test asserted the victim survived, which was true with or without the guard. What closed the original defect is untouched: a target outside the known temp roots is still refused, before the chdir and before the rmSync, with the roots named in the message. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 59 -------------------------- tests/helpers.cjs | 75 ++++++++++++---------------------- 2 files changed, 27 insertions(+), 107 deletions(-) diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index 0c078aa03..47e28ba17 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -259,62 +259,3 @@ test('tmpRootCandidates() returns a well-formed, deduplicated list of absolute p } }); -// ─── Test 8: symlink-escape refusal, all platforms ─────────────────────────── - -test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir', (t) => { - const symlinkParent = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-symlink-')); - t.after(() => { - if (fs.existsSync(symlinkParent)) cleanup(symlinkParent); - }); - - // The victim must live OUTSIDE tmpdir (that's the whole point of the - // scenario) — put it under this worktree's gitignored .gsd/ directory - // rather than os.homedir() so the test has no dependency on the runner's - // home layout. cleanup() will (correctly, by design) refuse a path there, - // so teardown cannot route through cleanup() either; the raw rmSync below - // is narrowly scoped and justified inline. - const victimParent = path.join(__dirname, '..', '.gsd'); - fs.mkdirSync(victimParent, { recursive: true }); - const victim = fs.mkdtempSync(path.join(victimParent, 'cleanup-symlink-victim-')); - t.after(() => { - if (fs.existsSync(victim)) { - // eslint-disable-next-line local/no-raw-rmsync-in-tests -- victim is deliberately outside os.tmpdir() (that's the scenario under test), so cleanup() refuses it by design and cannot be used for its own teardown. - fs.rmSync(victim, { recursive: true, force: true }); - } - }); - fs.writeFileSync(path.join(victim, 'marker.txt'), 'victim'); - - const symlinkPath = path.join(symlinkParent, 'escape-link'); - let symlinkCreated = true; - try { - fs.symlinkSync(victim, symlinkPath, 'dir'); - } catch (error) { - symlinkCreated = false; - t.skip(`symlink creation not permitted in this environment (${error.code || error.message}) — likely Windows without developer mode`); - } - - if (symlinkCreated) { - // The assert.throws below is the load-bearing assertion for this test: - // it proves the guard refuses a symlink whose target resolves outside - // every accepted tmp root. - assert.throws( - () => cleanup(symlinkPath), - (err) => err instanceof Error && err.message.includes(symlinkPath), - 'cleanup must throw and name the symlink path when it resolves outside os.tmpdir()' - ); - // These two assertions document the end state only — they are NOT proof - // the guard prevented a deletion that would otherwise have happened. A - // probe on node v26.5.1 showed fs.rmSync({recursive:true, force:true}) - // does not follow a top-level symlink target regardless: it unlinks the - // symlink itself and leaves whatever it points at (the victim, here) - // untouched either way. So the victim would survive with or without this - // guard; the guard's own correctness is established solely by the - // assert.throws above. - assert.strictEqual(fs.existsSync(victim), true, 'victim directory still exists (rmSync would not have followed the symlink either way)'); - assert.strictEqual( - fs.existsSync(path.join(victim, 'marker.txt')), - true, - 'victim contents still exist (rmSync would not have followed the symlink either way)' - ); - } -}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 7cd90d541..9560b8f9c 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -218,7 +218,27 @@ function createTempGitProject(prefix = 'gsd-test-') { // of that set, not the whole of it (see below). Each probe is wrapped in its // own try/catch — none of them may throw and crash cleanup(), they just // contribute nothing if unavailable. +// Memoization cache for tmpRootCandidates(), keyed on the LIVE os.tmpdir() +// value (not hoisted to a plain module-level constant) — two test files in +// this suite mutate TMPDIR/TEMP/TMP mid-run and restore them afterward, so +// caching on the current os.tmpdir() read is what keeps a stale cache from +// leaking across that override instead of a one-time computation baked in +// at module load. +let _tmpRootCandidatesCacheKey; +let _tmpRootCandidatesCache; + function tmpRootCandidates() { + const cacheKey = os.tmpdir(); + if (cacheKey === _tmpRootCandidatesCacheKey && _tmpRootCandidatesCache) { + return _tmpRootCandidatesCache; + } + const deduped = _computeTmpRootCandidates(); + _tmpRootCandidatesCacheKey = cacheKey; + _tmpRootCandidatesCache = Object.freeze(deduped); + return _tmpRootCandidatesCache; +} + +function _computeTmpRootCandidates() { const roots = []; try { roots.push(path.resolve(os.tmpdir())); @@ -272,19 +292,11 @@ function cleanup(tmpDir) { // destructive rmSync call itself — a wrong `target` would still chdir out of // its own tree and get force-deleted. Hoisted above both the chdir and the // rmSync so an out-of-temp-root path is refused before either can run. - // - // `target` itself is only realpath'd below when it exists (fs.existsSync - // guard on the symlink-escape check) — realpathSync throws ENOENT on a - // missing path, and cleanup() is legitimately called on already-deleted or - // never-created dirs. Comparison is case-insensitive on Windows - // (drive-letter and path casing vary there) and case-sensitive everywhere - // else; the error message below always prints the original-case target. + // Comparison is case-insensitive on Windows (drive-letter and path casing + // vary there) and case-sensitive everywhere else; the error message below + // always prints the original-case target. const isWindows = process.platform === 'win32'; const tmpRoots = tmpRootCandidates(); - // Shared by the literal-target check and the symlink-realpath check below - // so the two cannot drift apart — a second, independently written copy of - // this comparison is how the matching precondition in the tests came to - // disagree with the guard it was meant to mirror. function isUnderRoots(p) { const pForCompare = isWindows ? p.toLowerCase() : p; return tmpRoots.some((root) => { @@ -307,43 +319,10 @@ function cleanup(tmpDir) { `(${tmpRoots.join(', ')}): ${target}` ); } - // Symlink-escape guard: the check above is a string prefix test on `target` - // alone, so a symlink physically located under tmpdir but pointing OUTSIDE - // it would pass that check on `target`'s own path. NOTE: as of this - // writing, `fs.rmSync` itself does not follow a top-level symlink target - // (it unlinks the symlink and leaves whatever it points at untouched), so - // this guard is not closing a live escape against the current rmSync - // behavior — it is defense-in-depth against a future change to the - // deletion mechanism (a different rm implementation, a recursive walk that - // does follow links, etc.) that would make `target` being a symlink - // dangerous. When the target exists, resolve its real path and require - // that to be under a root too, so the guard holds regardless of how the - // eventual delete is implemented. - // - // Only when it exists: cleanup() is legitimately called on already-deleted - // or never-created dirs, and fs.realpathSync throws ENOENT on those — a - // missing target is harmless anyway since rmSync({force: true}) no-ops on it. - // - // Any realpath error (including a non-ENOENT failure) fails CLOSED: refuse - // rather than silently proceed. This is a safety check, not a best-effort - // probe — an unreadable/looping symlink must not be treated as safe. - if (fs.existsSync(target)) { - let real; - try { - real = fs.realpathSync(target); - } catch (error) { - throw new Error( - `cleanup() refused to remove a path it could not verify via realpath ` + - `(${(error && error.code) || (error && error.message) || 'unknown error'}): ${target}` - ); - } - if (!isUnderRoots(real)) { - throw new Error( - `cleanup() refused to remove a path that resolves via symlink to a ` + - `location outside the known temp roots (${tmpRoots.join(', ')}): ${target} -> ${real}` - ); - } - } + // No symlink-escape check here: fs.rmSync does not follow a top-level + // symlink — it unlinks the link itself and leaves the target intact — so + // there is no live hazard for the root-membership check above to guard + // against. That check is the one closing an actual defect. if (cwd === target || cwd.startsWith(`${target}${path.sep}`)) { // Windows cannot remove a directory that is the current working directory. process.chdir(path.dirname(target));