diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index cf5fae60d..2b48498d5 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -1724,8 +1724,21 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes } // 4a. Stale-lock guard: skip if lock is too fresh (PID recycling / race). + // + // The two causes are reported SEPARATELY (#3057). A lock whose mtime could + // not be read is not "fresh" in any sense: `lock_too_fresh` tells an + // operator that waiting will resolve the skip, and waiting never resolves a + // stat failure — the lock could be seconds or months old and the sweep has + // no way to tell. Conflating them is the same defect this module already + // fixed for `parse_failed` vs `no_worktrees` in planWorktreePrune: a + // decision made on unread data must not be indistinguishable from one made + // on real data. const lockMtime = mtimeSafe(lockedFile); - if (!lockMtime || nowMs - lockMtime.getTime() < reapMtimeGuardMs) { + if (!lockMtime) { + results.push({ path: worktreePath, status: 'skipped', reason: 'lock_age_unknown' }); + continue; + } + if (nowMs - lockMtime.getTime() < reapMtimeGuardMs) { results.push({ path: worktreePath, status: 'skipped', reason: 'lock_too_fresh' }); continue; } @@ -1738,11 +1751,18 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes } const pid = parseInt(pidStr, 10); // Number.isFinite, not Number.isNaN: pidStr is captured by /^\d+/ above, so - // pid can never be NaN. But a >309-digit string parses to Infinity, and - // process.kill(Infinity, 0) throws a TypeError that defaultIsPidAlive's - // errno-only catch maps to "dead" (no `.code` on a TypeError) — which - // would reap a worktree whose owner may well be alive. Fail closed here, - // before that value is ever trusted. + // pid can never be NaN. A 309-or-more-digit string parses to Infinity. + // + // NOT LOAD-BEARING FOR SAFETY — do not delete it as redundant. Fail-closed + // liveness now lives in defaultIsPidAlive, which treats every non-ESRCH + // outcome (including the TypeError process.kill throws for Infinity) as + // ALIVE. This guard survives because it produces a more ACCURATE verdict + // for garbage input: `lock_owner_unknown` says "the lock names a PID this + // parse could not represent", whereas falling through would report + // `pid_alive` — an assertion about an owner that was never probed. + // Note this is an EARLIER, DIFFERENT gate than the process.kill range + // limit: process.kill accepts up to 2147483647 and rejects 2147483648 + // (measured), far below the parse cliff this guard catches. if (!Number.isFinite(pid)) { results.push({ path: worktreePath, status: 'skipped', reason: 'lock_owner_unknown' }); continue; @@ -1813,13 +1833,28 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes // ─── reapOrphanWorktrees deps helpers ───────────────────────────────────────── +/** + * Liveness probe for a lock-owner PID — FAILS CLOSED (#3057). + * + * `ESRCH` ("no such process") is the ONLY outcome that proves the owner is + * gone. Every other failure means the probe could not determine liveness: + * - `EPERM` — the process exists, we just may not signal it; + * - `TypeError` / `ERR_INVALID_ARG_TYPE` — `process.kill` accepts a pid up + * to 2147483647 and REJECTS 2147483648 and above (measured), so a finite + * but out-of-range pid never reaches the OS at all; + * - anything else — an outcome this helper does not recognise. + * + * The return value feeds a DESTRUCTIVE decision (`git worktree remove + * --force`), so an unrecognised failure must never read as "dead". Hence the + * inversion: only ESRCH returns false; everything else returns true (alive, + * do not reap). + */ function defaultIsPidAlive(pid: number): boolean { try { process.kill(pid, 0); return true; } catch (err) { - if (err && (err as NodeJS.ErrnoException).code === 'EPERM') return true; - return false; + return (err as NodeJS.ErrnoException | null | undefined)?.code !== 'ESRCH'; } } @@ -1890,6 +1925,12 @@ function resolveWorktreeRoot(cwd: string, deps: WorktreeDeps = {}): { root: stri function pruneOrphanedWorktrees(repoRoot: string, deps: WorktreeDeps & { writeErr?: (s: string) => void } = {}): string[] { const writeErr = deps.writeErr || ((s: string) => process.stderr.write(s)); try { + // `...deps` comes LAST deliberately: `parseWorktreePorcelain` is a declared + // member of WorktreeDeps and planWorktreePrune already reads + // `deps.parseWorktreePorcelain` before falling back to the module function, + // so a caller-supplied parser is an intended override, not an accident. + // The hard-coded key is only a restatement of that same default. Do not + // reorder the two — `tests/worktree-safety-reap.test.cjs` pins the override. const plan = planWorktreePrune( repoRoot, { allowDestructive: false }, diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index 7a0936407..1fd9f7978 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -446,6 +446,17 @@ describe('#3057 W3: readConfigBaseBranch — config present but unusable', () => }); test('config.json parses to a non-object → null for null / string / number / array', () => { + // NOTE on the `[]` case: this documents observed behaviour only. It does + // NOT pin the `Array.isArray(cfg)` guard in readConfigBaseBranch — that + // guard is unreachable (and therefore unkillable) through this readFile + // entry point. `cfg` is always the result of `JSON.parse(raw)` on a + // string, and a JSON array can never carry a `.git` or `.base_branch` + // own-property the way a hand-built JS array could; with the guard + // deleted entirely, `top.git`/`top.base_branch` on an array are still + // `undefined`, so the result is `null` either way. Verified by mutation: + // deleting `|| Array.isArray(cfg)` from the built lib does not change any + // output for any JSON-string input. The guard is real defense-in-depth + // for a future non-JSON-string caller, not something this suite can pin. assert.strictEqual(readWith('null'), null, 'JSON null must not be treated as a config'); assert.strictEqual(readWith('"master"'), null, 'a bare JSON string must not be treated as a config'); assert.strictEqual(readWith('42'), null, 'a bare JSON number must not be treated as a config'); @@ -461,7 +472,14 @@ describe('#3057 W3: readConfigBaseBranch — config present but unusable', () => 'a whitespace-only override must not win the precedence ladder'); }); - test('"git" key present but not an object → skipped, flat legacy key still consulted', () => { + test('"git" key present but not a usable object (string/array/null) → nested lookup finds nothing, flat legacy key still consulted', () => { + // NOTE on the `"git":[]` case: like the sibling note above, this does NOT + // pin `!Array.isArray(gitSection)`. `gitSection` here is a JSON-parsed + // array with no `.base_branch` own-property, so `gitSection.base_branch` + // is `undefined` whether or not the guard runs — the flat key is + // consulted either way. Verified by mutation: deleting + // `&& !Array.isArray(gitSection)` from the built lib does not change this + // output for any JSON-string input. assert.strictEqual(readWith('{"git":"main","base_branch":"release"}'), 'release'); assert.strictEqual(readWith('{"git":[],"base_branch":"release"}'), 'release'); assert.strictEqual(readWith('{"git":null,"base_branch":"release"}'), 'release'); @@ -580,45 +598,18 @@ describe('#3057 W3: tryRemoteShow — tier-3 output that is present but not auth }); describe('#3057 W3: tryLocalBranch — non-empty stdout that names neither main nor master', () => { - /** - * An `execGit` stand-in whose result counts how many times `stdout` is read. - * `tryLocalBranch` reads it once in the `!r.stdout` guard and a second time to - * split it into lines. The read count therefore identifies WHICH null the - * function returned: 1 read = the early guard fired; 2 reads = the guard was - * passed and the final `return null` after the main/master checks ran. - */ - function countingGit(stdoutValue) { - const state = { reads: 0 }; - const git = () => ({ - exitCode: 0, - stderr: '', - signal: null, - error: null, - timedOut: false, - get stdout() { state.reads += 1; return stdoutValue; }, - }); - git.state = state; - return git; - } - test('stdout is exactly "\\n" → null, reached PAST the empty-stdout guard', () => { // This is the branch that was once deleted as "unreachable". The guard is // `if (r.exitCode !== 0 || !r.stdout) return null` — `"\n"` is a truthy // string, so the guard does NOT fire; `split('\n')` yields ["", ""], both // main/master checks are false, and the FINAL `return null` executes. // Deleting that line makes this function return `undefined`, which - // strictEqual(null) catches. The read count proves which null we got. - const git = countingGit('\n'); - assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', git), null); - assert.strictEqual(git.state.reads, 2, - 'stdout must be read twice: once by the guard (which passes) and once to split into lines'); + // strictEqual(null) catches. + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '\n' })), null); }); test('stdout is exactly "" → null via the EARLY guard (a different arm)', () => { - const git = countingGit(''); - assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', git), null); - assert.strictEqual(git.state.reads, 1, - 'an empty stdout must short-circuit in the guard, never reaching the line split'); + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '' })), null); }); // Boundary trio over the number of branch lines `git branch --list main master` diff --git a/tests/worktree-safety-reap.test.cjs b/tests/worktree-safety-reap.test.cjs index eeed3a899..93ff4a124 100644 --- a/tests/worktree-safety-reap.test.cjs +++ b/tests/worktree-safety-reap.test.cjs @@ -451,7 +451,7 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { assert.ok(fs.existsSync(f.wtDir)); }); - test('reports lock_too_fresh when the real mtime helper cannot stat the lock file', () => { + test('reports lock_age_unknown when the real mtime helper cannot stat the lock file', () => { const f = makeFixture(tmpBase, 'statfails'); // No `mtimeSafe` injection: this drives the module's own default helper and @@ -463,8 +463,12 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { ); const row = onlyRow(result); - assert.strictEqual(row.status, 'skipped'); - assert.strictEqual(row.reason, 'lock_too_fresh'); + // NOT `lock_too_fresh` (#3057): an unreadable mtime is not an age at all. + // Freshness tells an operator to wait; waiting never clears an EIO. + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'skipped', reason: 'lock_age_unknown' } + ); assert.ok(fs.existsSync(f.wtDir)); }); @@ -488,7 +492,7 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { assert.ok(fs.existsSync(f.wtDir)); }); - test('reports lock_too_fresh when the lock file cannot be stat-ed', () => { + test('reports lock_age_unknown, not lock_too_fresh, when mtimeSafe returns null', () => { const f = makeFixture(tmpBase, 'nomtime'); // nowMs is the far future, so a REAL mtime would read as stale and the @@ -499,23 +503,30 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { nowMs: 8640000000000000, })); - assert.strictEqual(row.status, 'skipped'); - assert.strictEqual(row.reason, 'lock_too_fresh'); + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'skipped', reason: 'lock_age_unknown' } + ); assert.ok(fs.existsSync(f.wtDir)); }); - test('reports lock_too_fresh for a zero-age lock under the default guard', () => { + test('reports lock_too_fresh, not lock_age_unknown, for a readable zero-age lock under the default guard', () => { const f = makeFixture(tmpBase, 'defaultguard'); const now = 1000000; + // The other half of the split: the mtime IS readable, the lock genuinely is + // recent, and waiting out the guard genuinely would change the outcome. const row = onlyRow(reapOrphanWorktrees(f.repoDir, { isPidAlive: () => false, mtimeSafe: () => new Date(now), nowMs: now, })); - assert.strictEqual(row.status, 'skipped'); - assert.strictEqual(row.reason, 'lock_too_fresh'); + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'skipped', reason: 'lock_too_fresh' } + ); + assert.ok(fs.existsSync(f.wtDir)); }); test('reaps the same zero-age lock when an injected reapMtimeGuardMs of 0 retires the guard', () => { @@ -702,7 +713,14 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.ok(fs.existsSync(f.wtDir), 'unmerged work must survive the sweep'); }); - test('reports lock_owner_unknown and leaves the worktree on disk for a 400-digit lock PID', () => { + // ── The Number.isFinite PARSE gate ──────────────────────────────────────── + // This gate is NOT the process.kill range limit (pinned in the next block). + // It fires far later, where `parseInt('9'.repeat(N), 10)` stops being + // representable: finite through N=308, Infinity from N=309 (measured). + // Reaching it means the reaper never learned a usable PID at all, so the + // verdict is `lock_owner_unknown`, not a liveness claim. + + test('reports lock_owner_unknown for a 400-digit lock PID (parse overflows past the Number.isFinite gate)', () => { const f = makeFixture(tmpBase, 'giantpid', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(400)); @@ -712,10 +730,7 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.ok(fs.existsSync(f.wtDir), 'a lock PID that overflows to Infinity must never be reaped'); }); - // Parse-cliff boundary, measured empirically: `parseInt('9'.repeat(N), 10)` - // is finite through N=308 and becomes Infinity at N=309 (probed via - // `node -e`, reported in the task return). - test('reaches the liveness check for a 308-digit lock PID (last finite length)', () => { + test('passes a 308-digit lock PID through the Number.isFinite gate (last representable length)', () => { const f = makeFixture(tmpBase, 'cliffminus1', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(308)); let seenPid; @@ -729,7 +744,7 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.strictEqual(fs.existsSync(f.wtDir), false); }); - test('reports lock_owner_unknown for a 309-digit lock PID (first Infinity length)', () => { + test('stops a 309-digit lock PID at the Number.isFinite gate (first unrepresentable length)', () => { const f = makeFixture(tmpBase, 'cliffexact', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(309)); @@ -739,6 +754,87 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.ok(fs.existsSync(f.wtDir)); }); + // ── The process.kill RANGE cliff — the one that actually decides a reap ─── + // Measured with the real `process.kill(pid, 0)` on this platform: + // 2147483647 → Error, code ESRCH (accepted; asks the OS) + // 2147483648 → TypeError ERR_INVALID_ARG_TYPE (rejected before the OS) + // Both tests drive the module's OWN `defaultIsPidAlive` (no `isPidAlive` + // injection) so the verdict is produced by the real errno classification. + // Each asserts the throw shape first: if a future Node moved the cliff, the + // probe fails loudly instead of the verdict flipping silently. + + const PID_KILL_MAX = 2147483647; + + test('treats the largest PID process.kill accepts as dead when the OS answers ESRCH', () => { + const f = makeFixture(tmpBase, 'killmax', { lock: false }); + fs.writeFileSync(path.join(f.adminDir, 'locked'), String(PID_KILL_MAX)); + + // Measured cliff, lower side: this value reaches the OS, which has no such + // process (every platform's max PID is orders of magnitude below it). + assert.throws( + () => process.kill(PID_KILL_MAX, 0), + (err) => err.code === 'ESRCH', + `process.kill(${PID_KILL_MAX}, 0) must reach the OS and report ESRCH` + ); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'reaped', reason: 'pid_dead_and_merged' } + ); + assert.strictEqual(fs.existsSync(f.wtDir), false); + }); + + test('treats the first PID process.kill rejects as ALIVE and leaves the worktree on disk', () => { + const f = makeFixture(tmpBase, 'killmaxplus1', { lock: false }); + const overRange = PID_KILL_MAX + 1; + fs.writeFileSync(path.join(f.adminDir, 'locked'), String(overRange)); + + // Measured cliff, upper side: one past the accepted range, `process.kill` + // throws a TypeError with NO errno. That is "could not determine", not + // "dead" — the old errno-only catch read it as dead and REAPED here. + assert.throws( + () => process.kill(overRange, 0), + (err) => err instanceof TypeError && err.code === 'ERR_INVALID_ARG_TYPE', + `process.kill(${overRange}, 0) must throw TypeError ERR_INVALID_ARG_TYPE` + ); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir), 'an unclassifiable liveness probe must never reap'); + }); + + test('treats an unrecognised errno from process.kill as ALIVE (only ESRCH means dead)', (t) => { + // EPERM has its own test above; this pins the GENERAL rule for a code the + // helper has never heard of, which an `=== EPERM ? true : false` catch + // would classify as dead. + const f = makeFixture(tmpBase, 'defaultkill-einval'); + const originalKill = process.kill; + t.after(() => { process.kill = originalKill; }); + process.kill = () => { throw Object.assign(new Error('EINVAL'), { code: 'EINVAL' }); }; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('treats a codeless throw from process.kill as ALIVE', (t) => { + // A thrown value with no `.code` at all (the TypeError case in the + // abstract): `undefined !== 'ESRCH'`, so it must still read as alive. + const f = makeFixture(tmpBase, 'defaultkill-bare'); + const originalKill = process.kill; + t.after(() => { process.kill = originalKill; }); + process.kill = () => { throw new Error('no errno on this one'); }; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir)); + }); + test('reaches the liveness check for an ordinary small lock PID', () => { const f = makeFixture(tmpBase, 'ordinarypid', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '4242'); @@ -857,6 +953,30 @@ describe('#3057 cmdWorktreeReapOrphans / pruneOrphanedWorktrees output verdicts' ]); }); + test('pruneOrphanedWorktrees hands the porcelain to a caller-supplied parseWorktreePorcelain', () => { + const f = makeFixture(tmpBase, 'pruneparser'); + const realPorcelain = git(['worktree', 'list', '--porcelain'], f.repoDir); + const seen = []; + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + // `parseWorktreePorcelain` is a declared member of the deps bag and + // `planWorktreePrune` reads `deps.parseWorktreePorcelain` first, defaulting + // to the module function only when absent. pruneOrphanedWorktrees therefore + // spreads `...deps` AFTER its own hard-coded default so the caller's parser + // wins. Ordering the two the other way round is invisible to every other + // test in the tree; this one fails if the spread moves. + const removed = pruneOrphanedWorktrees(f.repoDir, { + execGit: faultyGit, + parseWorktreePorcelain: (porcelain) => { seen.push(porcelain); return []; }, + writeErr: () => { throw new Error('no degradation warning expected'); }, + }); + + assert.deepStrictEqual(removed, []); + assert.strictEqual(seen.length, 1, 'the injected parser must be the one that ran, exactly once'); + assert.strictEqual(seen[0], realPorcelain, 'it must receive the porcelain readWorktreeList obtained'); + assert.strictEqual(calledWith(faultyGit, ['worktree', 'prune']), true, 'the metadata prune still runs'); + }); + test('pruneOrphanedWorktrees returns an empty list and warns nothing when git throws', () => { const f = makeFixture(tmpBase, 'prunethrows'); const err = [];