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>
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>
Thirty-four tests covering every branch in the reaping path that no test
reached, which was all of the fail-closed ones. The function has always
accepted an injectable dependency bag; nothing used it. Every existing test
drove real git and injected only the clock and the liveness probe, so each
guard that exists for a failure — an unreadable git dir, a null directory
listing, an unresolvable remote ref, a missing pointer file, an unlocked
sibling, an ambiguous remote — had never executed. They are now driven by
injecting exactly the fault that selects them, and each asserts its specific
status and reason rather than that something happened.
The last one needed no new mechanism, only the right one. It was reported as
unreachable without a cross-user PID, but the default liveness helper is
reachable by not injecting over it and patching process.kill, which is the
deterministic injection this repo requires over real OS conditions. Its three
outcomes — EPERM, ESRCH, and a clean return — now assert their verdicts.
The assertions were kill-tested rather than assumed. Against mutated copies of
the built module, renaming the six reason strings fails eighteen tests,
neutralising the fail-closed returns fails seven more, dropping the
ambiguous-remote guard fails one, and removing the prune catch and its timeout
guard fails both prune tests. Flipping the EPERM arm to false turns a skip into
a reap and fails that test.
Four places where production folds distinct causes into one verdict are
recorded in the tests rather than papered over. A lock is too fresh whether its
mtime is unreadable or merely recent; a branch tip fails to resolve for three
different reasons; a PID reads as alive whether the owner lives or the probe
threw. Where the return value cannot separate them the tests assert the git
call sequence instead, and where even that cannot, the test says so.
Six weak assertions already in the older file are replaced rather than left
beside the new ones: five guarded their assertions behind `if (entry)`, so a
missing entry skipped the check and passed, and one asserted only that the
reaper returned a non-empty array. Three JSON parses wrapped in doesNotThrow
now parse directly, so a malformed payload reports its own syntax error
instead of a generic message.
Refs #3057
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
