fix(update): reclaim a recovery lock whose owner PID is gone

[greqone]

What does this PR do?

_claim_recovery_lock decides whether a held .update-incomplete.lock may be broken purely by age: older than one hour, take it; otherwise give up. It never asks whether the process that wrote the lock still exists.

That gets the priority exactly backwards. A crashed or force-killed owner is the precise state the marker system exists to recover from, so its abandoned lock is the one that should be reclaimed soonest. Instead it is honoured for a full hour, and honoured silently: the claim fails, recover_if_needed returns without printing anything, and a pending dependency install sits unfinished long after the process holding it died. From the outside the CLI looks healthy and simply never repairs itself.

On Windows this compounds with the "update" in argv exclusion a few lines up. hermes update deliberately never runs the recovery path, so the only code that can finish a deferred install is the early pass, which is the code being told to wait. I hit this on a real install: the lock was owned by PID 59912, dead for twenty minutes, and three consecutive hermes doctor runs each no-opped without a word while the pending install stayed pending.

The fix reuses this module's own _pid_is_running helper, which already exists a few hundred lines above and already handles the Win32 OpenProcess case. Read the PID out of the lock body, and if that process is gone, reclaim the lock immediately.

Deliberately preserved:

• The age fallback. Still there for bodies this cannot reason about (empty, truncated, written by an older layout) and for an owner that is alive but wedged. _lock_owner_is_live is conservative: anything unreadable or non-numeric counts as live, so an unidentifiable lock is never stolen on liveness grounds.
• Single-flight. A living owner still keeps its lock.

Two behaviour changes beyond the probe, both small:

• A reclaimed lock is retried on the same launch. The old age path unlinked the file but still returned False, so even a successful break cost an extra launch.
• Exactly one retry. If a racer wins the same reclaim and recreates the lock between the discard and the retry, we yield to it rather than spinning.

recover_if_needed carried an inline copy of the claim logic with the identical bug. It now calls the shared helper, so both paths are fixed by construction rather than by remembering to patch two places.

Related Issue

Related to #86943 — that issue's headline symptom is the pre-#86735 preflight, fixed by #86781. This is the other reason a deferral fails to resume, and it survives that fix: once the preflight correctly defers, an orphaned lock can still keep the marker recovery from running for an hour.

Not a duplicate of #86782, #86826 or #86857, none of which touch the lock.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• hermes_cli/_early_recovery.py
  • new _lock_owner_is_live() — parses the bare PID from the lock body and defers to the existing _pid_is_running(); conservative on anything it cannot parse
  • new _discard_stale_recovery_lock() — removes a lock whose owner is gone, or which has aged out
  • _RECOVERY_LOCK_MAX_AGE_SECONDS replaces the bare 3600 literal
  • _claim_recovery_lock() retries once after a successful discard
  • recover_if_needed() drops its inline duplicate of the claim and calls the helper
• tests/hermes_cli/test_early_recovery.py — ten new tests (below)

How to Test

1. Create a project root with a .lazy-refresh-incomplete marker and write .update-incomplete.lock containing the PID of a process that has already exited.
2. Before this change: _claim_recovery_lock(root) returns False, and recover_if_needed performs no repair, for one hour, printing nothing.
3. After: the lock is reclaimed on the first launch and the repair runs.

Automated:

pytest tests/hermes_cli/test_early_recovery.py -q     # 27 passed

Nine of the ten new tests fail on main without the source change; the tenth (test_recovery_lock_yields_to_a_live_owner) passes both before and after by design, as it guards the single-flight behaviour this must not break.

New coverage:

Test	Asserts
..._reclaimed_when_owner_pid_is_dead	dead owner is reclaimed, lock rewritten with our PID
..._reclaimed_from_a_real_dead_pid	same via a genuinely spawned-and-reaped PID, no mocking, so _pid_is_running is exercised for real on each OS
..._yields_to_a_live_owner	single-flight preserved
..._age_fallback_breaks_a_wedged_live_owner	the old hour-long escape hatch survives
..._unparseable_body_waits_for_the_age_fallback	parametrised over empty / whitespace / non-numeric / newlines
..._does_not_loop_when_a_racer_recreates_it	returns False instead of spinning
test_stale_lock_no_longer_blocks_the_early_repair	end to end: the user-visible bug

Adjacent suites run clean: test_early_recovery.py, test_update_self_lock.py, test_update_interrupted_recovery.py, test_checkout_mutation_guards.py — 56 passed.

ruff check . passes. ruff format is not applied: main does not satisfy it on these files either, and CI enforces only ruff check, so reformatting would have added unrelated churn.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix
• I've run the affected suites and all tests pass
• I've added tests for my changes
• I've tested on my platform: Windows 11 Pro 26200, Python 3.11

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — docstrings only, no user-facing doc change
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact — the probe routes through the existing _pid_is_running, which already branches Win32 OpenProcess vs POSIX os.kill(pid, 0); the real-dead-PID test covers both. Module stays stdlib-only, which is load-bearing here.
• I've updated tool descriptions/schemas if I changed tool behavior — N/A

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[greqone]

@gemini-code-assist @devin-ai-integration @chatgpt-codex-connector please review.

Focus areas, since this touches a lock:

1. Reclaim safety. _lock_owner_is_live treats anything unreadable or non-numeric as live so the age fallback stays in charge. Is that the right default, and is PID reuse a concern worth more than the single-retry guard?
2. The single retry. _claim_recovery_lock retries the O_CREAT|O_EXCL claim exactly once after a successful discard. Racer recreates the lock in between: we yield. Any interleaving where that still double-claims?
3. Same-launch reclaim. Old code unlinked on the age path but still returned False, costing an extra launch. Returning True now is a deliberate behaviour change; sanity-check it against the single-flight contract main.py's full recovery relies on.
4. Cross-platform. The probe reuses the module's existing _pid_is_running (Win32 OpenProcess vs POSIX os.kill(pid, 0)). _early_recovery must stay stdlib-only, that property is load-bearing.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(update): reclaim a recovery lock whose owner PID is gone

The liveness-probe + age-fallback design is sound, and the conservative direction (unreadable/non-numeric body → treat as live → wait for age fallback) is the right call for a lock that must never be stolen from a real owner. Observations:

1. hermes_cli/_early_recovery.py _claim_recovery_lock — the retry loop for retry in (False, True) is correct: only the first miss may reclaim, and a lock that reappears after the discard belongs to a racer, so the second pass returns False. The test test_recovery_lock_does_not_loop_when_a_racer_recreates_it pins exactly this. No issue.
2. PID-reuse race: _pid_is_running(pid) may report a recycled PID as live, keeping the lock until the age fallback (1h). That's a safe failure mode (recovery is delayed, never unsafe). If shorter recovery latency matters, the age fallback could be reduced for the unparseable/ambiguous cases only — but current behavior is defensible.
3. _discard_stale_recovery_lock — lock_path.stat().st_mtime is read after the liveness check; there's a small race where the owner dies between the check and the stat, but the outcome (age check then unlink) is still safe because the lock content is what matters. No action needed.
4. recover_if_needed — the old code's except OSError: pass # proceed unlocked behavior is preserved via _claim_recovery_lock returning True on OSError. One subtle difference: previously a readable lock with a dead owner was reclaimed only after age; now it's reclaimed immediately. That's the intended fix. Good.
5. Test quality is high (real dead PID via reaped child, live-owner yield, age fallback, racer-recreate loop). Minor: test_recovery_lock_unparseable_body_waits_for_the_age_fallback reuses body with two different ages but writes the file twice — the second write resets mtime to now before utime is applied, which the helper handles. No issue.