fix(#3103): refuse to reap on a PID the parse could not represent
A lock file whose PID is a digit string longer than 308 characters parses to Infinity, not NaN. The default liveness helper then calls process.kill with it, which throws a TypeError rather than an errno error, and that helper's catch only recognises EPERM — so it returns false, meaning "the owner is dead", and the worktree becomes eligible to be removed. The reaper already fails closed for this exact situation. It wraps the liveness call in a catch that sets alive and does not reap, with a comment saying liveness could not be determined. That protection never fires here, because the inner catch swallowed the error first and answered confidently instead of admitting it did not know. A guard that cannot verify safety reporting success is the defect this whole epic is named for, and it was sitting inside the one function in the tree that deletes things. The guard deleted earlier on this branch tested for NaN. That test really was dead — a digits-only capture cannot parseInt to NaN — but the reachable failure is non-finite, so removing it without correcting the predicate left the hole open. The check is now for a finite value, and a malformed PID reports the same lock_owner_unknown skip as an unparseable one, since both mean the same thing: the owner is unknown, so nothing is removed. Measured, not assumed: 308 nines still parse finite, 309 are Infinity. All three of that boundary are covered, along with a 400-digit case that asserts the worktree is still on disk afterwards — the consequence, not just the verdict. Against a build with the guard removed, that case reports pid_dead_and_merged and the worktree is gone. The liveness helper's own catch still maps every non-EPERM error to "dead". The fix belongs where the value stops being trustworthy rather than at the far end of it, but that helper is worth revisiting on its own terms. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1737,9 +1737,18 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes
|
||||
continue;
|
||||
}
|
||||
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.
|
||||
if (!Number.isFinite(pid)) {
|
||||
results.push({ path: worktreePath, status: 'skipped', reason: 'lock_owner_unknown' });
|
||||
continue;
|
||||
}
|
||||
let pidIsAlive: boolean;
|
||||
try {
|
||||
// No `Number.isNaN(pid) ||` guard: pidStr is captured by /^\d+/ above, so pid is never NaN.
|
||||
pidIsAlive = isPidAliveCheck(pid);
|
||||
} catch {
|
||||
pidIsAlive = true; // Cannot determine liveness — treat as alive, do not reap.
|
||||
|
||||
@@ -702,6 +702,57 @@ 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', () => {
|
||||
const f = makeFixture(tmpBase, 'giantpid', { lock: false });
|
||||
fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(400));
|
||||
|
||||
const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps()));
|
||||
|
||||
assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'lock_owner_unknown' });
|
||||
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)', () => {
|
||||
const f = makeFixture(tmpBase, 'cliffminus1', { lock: false });
|
||||
fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(308));
|
||||
let seenPid;
|
||||
|
||||
const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({
|
||||
isPidAlive: (pid) => { seenPid = pid; return false; },
|
||||
})));
|
||||
|
||||
assert.strictEqual(seenPid, Number('9'.repeat(308)), 'a finite 308-digit PID must reach isPidAlive unchanged');
|
||||
assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'reaped', reason: 'pid_dead_and_merged' });
|
||||
assert.strictEqual(fs.existsSync(f.wtDir), false);
|
||||
});
|
||||
|
||||
test('reports lock_owner_unknown for a 309-digit lock PID (first Infinity length)', () => {
|
||||
const f = makeFixture(tmpBase, 'cliffexact', { lock: false });
|
||||
fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(309));
|
||||
|
||||
const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps()));
|
||||
|
||||
assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'lock_owner_unknown' });
|
||||
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');
|
||||
let seenPid;
|
||||
|
||||
const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({
|
||||
isPidAlive: (pid) => { seenPid = pid; return false; },
|
||||
})));
|
||||
|
||||
assert.strictEqual(seenPid, 4242, 'an ordinary PID must reach isPidAlive unchanged');
|
||||
assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'reaped', reason: 'pid_dead_and_merged' });
|
||||
assert.strictEqual(fs.existsSync(f.wtDir), false);
|
||||
});
|
||||
|
||||
test('reports remove_failed and leaves the worktree on disk when git worktree remove fails', () => {
|
||||
const f = makeFixture(tmpBase, 'removefails');
|
||||
const faultyGit = makeFaultyGit({
|
||||
|
||||
Reference in New Issue
Block a user