From 1e844b942ce9591cbab9446b1a9fe1f390e55b40 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 01:26:15 -0400 Subject: [PATCH] fix(#3103): only "no such process" means the owner is dead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the previous commit fixed the wrong cliff. process.kill accepts a pid up to 2147483647 and throws a TypeError above it, so 2147483648 — an ordinary finite number — sailed past the finite check, threw, and was classified dead. Measured here: 2147483646 and 2147483647 raise ESRCH, 2147483648 and above raise ERR_INVALID_ARG_TYPE. The digit-length boundary the last commit pinned was a different, earlier gate, and its tests implied it was the meaningful one. The liveness helper now treats only ESRCH as dead. EPERM, a type error from an out-of-range pid, an error with no code at all — every outcome it does not recognise returns alive, because the value feeds a forced worktree removal and an unrecognised failure must never read as permission to delete. Against a build with the old catch, a lock holding 2147483648 reaps the worktree; with this one it is skipped and the directory survives. The finite check stays. It no longer carries the safety, but it still names garbage input accurately rather than reporting it as a live owner, and its comment now says that so it is not removed as redundant. The freshness verdict is split. An unreadable lock mtime and a genuinely recent lock both reported lock_too_fresh, which tells an operator to wait when waiting cannot help — the same conflation this module already separated for parse failures. The unreadable case is now lock_age_unknown. Only this module and its tests read these strings; no workflow or command consumes them. The dependency spread in the prune path is kept as it is, with a test that kills it. A caller-supplied porcelain parser displacing the default is the intended seam, not an accident: the parser is a declared member of the dependency type and the planner already prefers an injected one, so the hard-coded key restates the same default rather than guarding against override. Previously nothing exercised that line at all. Refs #3057 Co-Authored-By: Claude Opus 5 --- src/worktree-safety.cts | 57 +++++++++-- tests/git-base-branch.test.cjs | 53 ++++------ tests/worktree-safety-reap.test.cjs | 150 +++++++++++++++++++++++++--- 3 files changed, 206 insertions(+), 54 deletions(-) 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 = [];