fix(#3103): only "no such process" means the owner is dead
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 },
|
||||
|
||||
Reference in New Issue
Block a user