fix(checkpoints): reclaim the index lock a killed git leaves behind (#74108)

[mehmetkr-31]

Problem

git add -A against the checkpoint store creates <index>.lock with O_EXCL and renames it into place on success. When the git child is killed — which our own subprocess timeout does readily when the work tree is a WSL2 drvfs/9p mount such as /mnt/c — the lock outlives the process that made it.

The lock file records no owner, so git can never reclaim it. Every later git add against that index then fails instantly:

fatal: Unable to create '.../checkpoints/store/indexes/<hash>.lock': File exists

for the rest of the session and every future session, until a human deletes the file. From the user side, one tool call crawls to the timeout wall, and afterwards checkpoints look suspiciously instant — because they are no longer doing any work.

Premise verified on current main: tools/checkpoint_manager.py's _run_git handles except subprocess.TimeoutExpired: by logging and returning. Nothing cleans up the abandoned lock, and no other code path does either.

Fix

_run_git now reclaims the lock on both paths:

• Prevention — on TimeoutExpired, subprocess.run has already killed and reaped the child, so any lock it created is abandoned by definition. It is removed with no age check. This stops the wedge from forming.
• Recovery — when a call fails with git's "unable to create ... lock" message, the lock is reclaimed and the call retried exactly once, so an install already wedged by an earlier timeout heals itself instead of needing manual cleanup.

Why this is safe under concurrency

The recovery path only reclaims a provably stale lock. Every checkpoint git call is bounded by _GIT_TIMEOUT (at most _MAX_GIT_CALL_SECONDS, the largest caller multiplier), so a lock whose mtime is older than that window cannot belong to a running call. A younger lock is deliberately left alone, because a concurrent Hermes session in the same working directory may legitimately own it — stealing it would corrupt that session's write.

The timeout handler is the one caller allowed to skip the age check (require_stale=False), and only because it has just reaped the owning child itself.

Tests

Five tests in tests/tools/test_checkpoint_manager.py, exercising real git through the real _run_git rather than mocking the dispatch:

• test_stale_lock_is_reclaimed_and_the_call_retried — the healing path for an already-wedged install.
• test_fresh_lock_is_left_alone — a concurrent call's lock survives.
• test_timeout_reclaims_its_own_lock_without_an_age_check — the lock is brand new here, so an age check would refuse it; the timeout handler must still reclaim it.
• test_recovery_retries_only_once — a permanently failing call does not recurse.
• test_unrelated_failures_do_not_touch_the_lock.

Verified they catch the bug rather than merely passing: with the fix reverted, the three behavioural tests fail (the first with git's exact Unable to create '...lock': File exists), while the two "must not touch" guarantees still pass, as they should.

tests/tools/test_checkpoint_manager.py is 45 passed. tests/tools/ produces an identical failure set to a pristine checkout of main — zero introduced (the pre-existing ones are local environment issues such as missing optional deps).

[alt-glitch]

This was generated by AI during triage.

Related: #13232 and salvage #52887 address the same checkpoint index-lock wedge. This current PR also cleans up directly after a timeout and retries a detected stale-lock failure once; maintainers should choose or consolidate the three live approaches.

[mehmetkr-31]

Thanks for the pointer — and my duplicate check missed those two. I searched the issue number and the phrase "checkpoint lock"; #13232 and #52887 say "index.lock", so neither matched. My fault, and #13232 (2026-04-20, @thapecroth) is clearly the earliest canonical claim on this bug.

Before you consolidate, there is one substantive difference worth knowing, because I think it decides which code has to survive rather than which PR is older.

The three approaches clean different paths, and only one of them is the path that wedges.

_git_env() sets GIT_INDEX_FILE to a per-project index — _index_path(store, dir_hash) = store/indexes/<hash> — for every call that passes index_file=, which includes the git add -A in ensure_checkpoint(). So the lock git takes is store/indexes/<hash>.lock, and that is exactly what #74108's log shows:

fatal: Unable to create '/home/youruser/.hermes/checkpoints/store/indexes/889f3b0056c8f5cc.lock': File exists.

• fix(checkpoint): self-heal stale index.lock from crashed git process (salvage of #13232 by @thapecroth) #52887 clears store / "index.lock" — the store-root index, used only by calls that pass no index_file.
• fix(checkpoint): self-heal stale index.lock from crashed git process #13232 describes ~/.hermes/checkpoints/<hash>/index.lock, which is the pre-v2 per-repo shadow layout, before the single-store migration.
• This PR clears <index_file>.lock, i.e. the per-project lock actually held.

I verified the per-project claim rather than reading it off the paths — driving the real _run_git against real git in a temp store:

per-project index : store/indexes/e20c768aa8b423bc
its lock          : store/indexes/e20c768aa8b423bc.lock
store-root lock   : store/index.lock
git add ok: True
per-project index file created: True
store/index created: False          <-- never created in this flow

So store/index.lock cannot exist for the reported wedge, and a fix that only removes it will not clear it. Both locks are reachable in principle (a call without index_file uses the store-root index), so the right consolidated fix probably covers whichever index the current call is actually using — which is what keying off index_file gives you for free.

Two smaller deltas in this PR, both mentioned in the triage note:

1. Cleanup directly in the TimeoutExpired handler, with no age check — subprocess.run has already killed and reaped the child, so its lock is abandoned by definition. This stops the wedge from forming instead of waiting for the next call to find it.
2. On a failure whose stderr carries git's "unable to create ... lock", reclaim and retry the call once, so an install already wedged heals inside the same call.

On the age threshold: I used _GIT_TIMEOUT * 3 (the largest multiplier any caller passes) rather than a hardcoded 60s, so the "no live call can still own this" argument stays true if HERMES_CHECKPOINT_TIMEOUT is raised. With the default 30s that is 90s; a hardcoded 60s is already shorter than a single _GIT_TIMEOUT * 3 call.

One thing to watch when consolidating: #52887's diff also drops the creationflags=windows_hide_flags() explanatory comment above subprocess.run (the per-call conhost flash note). Probably unintended.

Happy for this PR to be closed in favour of #13232 / #52887 — I have no attachment to it landing. If that is the direction, I am glad to open the per-project-lock path plus the two deltas as a follow-up on top of whichever one you take, so @thapecroth keeps authorship of the original fix. Just say which base you want.

[mehmetkr-31]

Update — I pushed two more findings that came out of comparing this against #13232/#52887. They change my read of the situation, so I want to withdraw the "just close this one" framing from my last comment: I now think this branch has to be the base, not because it is mine, but because of what the comparison turned up.

1. The English-marker detection was a real bug — in my code.

My first cut keyed recovery on "unable to create" appearing in stderr. git localizes that prose. Reproduced against real git under a Turkish locale:

onulmaz: '/…/r/.git/index.lock' oluşturulamıyor: File exists.

No unable to create anywhere. Recovery would have silently stopped working for every non-English install — the exact "looks fine in CI, dead in production" failure. Detection now matches the quoted path ending in .lock, which git does not translate.

2. One lock is not the whole bug class.

I counted the call sites: 43 _run_git calls pass no index_file, so they use git's default $GIT_DIR/index and its store/index.lock. git add -A passes index_file and takes store/indexes/<hash>.lock. update-ref and the reflog/gc maintenance calls take ref locks under store/refs/. A killed process wedges whichever it held, and each wedges permanently.

So rather than pick a path, the retry now reclaims the path git names in its own error, which covers every class with one mechanism and no guessing. _index_lock_path() is total for the timeout path (per-project index when index_file is given, store root otherwise).

Safety: a reclaimed path must resolve inside the checkpoint store, so a surprising message can never point the cleanup at a file we don't own; relative paths are ignored. The staleness proof is unchanged (_GIT_TIMEOUT * 3 — no live call can be older).

New coverage — TestLockReclaimCoversEveryLockClass: both index-lock targets; ref-lock extraction; a localized-git stderr; refusal of out-of-store and relative paths; and an end-to-end real-git ref-lock recovery through _run_git. That last one fails against an index-only implementation, which is the concrete demonstration that path-per-class matters.

tests/tools/test_checkpoint_manager.py is 52 passed. tests/tools/ matches a pristine origin/main baseline (210 vs 211) — the one delta is test_concurrent_writes_never_tear_the_snapshot, which I confirmed fails 4/4 runs on clean main in isolation; it is an unrelated concurrency flake.

Where that leaves the three PRs. The consolidation still shouldn't be decided on age, but the authorship point stands: @thapecroth reported and diagnosed this first and deserves the credit. I'm happy to (a) rebase this onto #13232 or #52887 as follow-up commits so their history leads, or (b) have a maintainer take this branch and add a Co-authored-by: thapecroth trailer — whichever fits how you normally salvage. What I'd push back on is landing an index-only, English-only version of the fix, since both gaps above are reproducible.

[teknium1]

Thanks for tracing the v2 per-project index path and extending recovery to localized stderr and ref-lock errors. The underlying timeout wedge is present on current main (tools/checkpoint_manager.py:346-355, with the checkpoint git add at tools/checkpoint_manager.py:1048-1054).

Problems

• tools/checkpoint_manager.py:492-496 always cleans _index_lock_path(index_file, store). For update-ref, current main calls _run_git without index_file (tools/checkpoint_manager.py:1115-1117), so this selects store/index.lock; it does not clean that operation's ref lock and can remove an unrelated active root-index lock.
• _clear_abandoned_lock() checks mtime at tools/checkpoint_manager.py:309 and unlinks later at :325. Another session can replace the stale pathname in that interval, causing deletion of a fresh live lock. The fresh-lock test covers a lock present before recovery, not replacement during recovery.

Suggested changes

• Use command-specific, safely claimed timeout cleanup, and add an update-ref timeout regression.
• Use a cross-process/atomic ownership protocol for stale-lock recovery and test the replacement race.

Automated hermes-sweeper review.

[teknium1]

Blocking: this is not the timed-out command's lock for calls such as update-ref, which pass no index_file and use ref locks. It resolves to store/index.lock, leaving the ref lock behind and potentially deleting another call's active root-index lock. Limit timeout cleanup to a lock this invocation can identify and own, and add an update-ref timeout test.

[teknium1]

Blocking: stat() above and this pathname unlink are not atomic. A concurrent session can replace the stale lock after the age check, so this can delete its fresh live lock. Please use an atomic ownership/cross-process coordination protocol and add a replacement-race regression.

[mehmetkr-31]

Both findings were real and are fixed. Thanks — the second one in particular was a hole I had reasoned myself past.

1. Timeout cleanup is now command-specific.

You're right that _index_lock_path(index_file, store) was a guess, and a harmful one for update-ref: it missed the ref lock the killed call actually left and put an unrelated root-index lock at risk.

_lock_for_timed_out_command(args, store, index_file) now maps the command to the lock it actually takes:

• index-writing subcommands (add, read-tree, commit, checkout, reset, rm, mv, apply) → the index lock, per-project when index_file is given, store root otherwise;
• update-ref → <store>/<ref>.lock, read off the first non-flag refs/… argument;
• everything else (gc, reflog expire, read-only queries) → None.

Returning None is deliberate rather than a gap: an unmapped command heals through the stderr-driven recovery on the next call, which reads the path out of git's own error. I'd rather defer than guess.

The timeout path additionally requires the lock to have appeared after the call started (created_after=_started_at). Our child being reaped makes a lock it created abandoned; a lock predating our launch belongs to somebody else and now survives. That closes the collateral-damage case even for the commands we do map.

2. The stat→unlink window is closed with an atomic claim.

Removal now claims the lock with os.rename() — one syscall, one specific directory entry — and then verifies it got the inode it judged by comparing (st_dev, st_ino, st_mtime_ns). If another session reclaimed the stale lock and a fresh git created a new one at the same pathname in that window, the identity check fails, and the claimed file is put back with os.link() (which fails closed if the slot was retaken, so a newer lock is never clobbered). The claim file is cleaned up on every path.

Judgement still happens before the claim, deliberately: a lock that looks live is never renamed away even momentarily.

New tests, both of the ones you asked for:

• TestTimeoutCleanupIsCommandSpecific — update-ref timeout clears its ref lock and leaves a live root-index lock untouched; an unmappable command (gc) clears nothing; a lock predating the call survives.
• TestReclaimSurvivesTheReplacementRace — a lock replaced inside the judge→claim window is put back and the replacement survives with its contents intact and no leftover claim files; an unreplaced stale lock is still removed.

I checked the race test actually models the dangerous window rather than a harmless one: my first version swapped the file after the rename, which the implementation handles trivially, so it passed against the broken code too. It now swaps before the claim, and fails against a plain unlink().

All five new tests fail against the previous implementation — reverting the command mapping fails the three timeout tests, reverting the atomic claim fails the replacement-race test.

tests/tools/test_checkpoint_manager.py is 57 passed. tests/tools/ matches a pristine origin/main baseline (210 vs 211); the single delta is test_concurrent_writes_never_tear_the_snapshot, which I confirmed fails 4/4 in isolation on clean main — an unrelated concurrency flake.

The authorship offer from my previous comment still stands: happy to have this rebased as follow-up commits onto #13232/#52887, or taken with a Co-authored-by: thapecroth trailer.

[mehmetkr-31]

CI caught a portability bug in the ownership check I added for finding (1), so I've replaced the mechanism. Worth writing up because it is not obvious.

The bug. I decided "did my child create this lock?" with st_mtime >= started_at, where started_at = time.time() was taken immediately before subprocess.run. On Linux, inode timestamps come from the kernel's coarse clock (ktime_get_coarse_real_ts64()), which advances once per timer tick (~1–4 ms), while time.time() reads the fine-grained clock. A lock created microseconds after the call starts can therefore carry an mtime slightly earlier than the recorded start — so the guard rejected a lock our own child had just made, and the wedge survived.

APFS records fine-grained timestamps, so the margin on macOS is ~30 µs and always positive. Both timeout tests passed locally and failed on the Linux runner (slice 2/8) — a clean example of a test that is right while the implementation is only accidentally portable.

The fix. Ownership is no longer inferred from a timestamp at all. _run_git now resolves the command's candidate lock before launching and records whether it already existed; on timeout, the lock is reclaimed only if it was absent before the call. That is an exact observation — a lock that was not there before, on a call whose child we have already reaped, is abandoned by definition — and it is completely independent of filesystem timestamp granularity. The created_after parameter is gone.

The staleness guard on the recovery path is unchanged: it compares an age against _MAX_GIT_CALL_SECONDS, which is a duration comparison, not a wall-clock-vs-mtime one, so the same effect can't bite there.

Current state on 794f2980:

• Python tests / Run tests slice 2/8 — success, so both timeout tests now pass on Linux.
• The run's one failure is tests/hermes_cli/test_update_eol_churn.py::test_churn_across_more_files_than_fit_in_one_argv (assert 0 == 1200) in slice 7/8. That is unrelated to this diff, which touches only tools/checkpoint_manager.py and its test; I had already seen the same test flake while baselining an unrelated branch, and it shows up on other open PRs too. Happy to rebase to pick up a fix if one lands, but I don't think it should block this.

tests/tools/test_checkpoint_manager.py is 57 passed locally, and the five review-driven tests still fail against the previous implementation (reverting the command mapping fails the three timeout tests; reverting the atomic claim fails the replacement-race test).

[mehmetkr-31]

CI note: the single red job is not this PR. The only failing test on the whole run is tests/hermes_cli/test_update_eol_churn.py::test_churn_across_more_files_than_fit_in_one_argv — 2,535 passed, 1 failed, and that one is the known _normalize_managed_eol CRLF-churn cluster tracked as #75175, with a fix already in flight as #75213. It touches no file this PR touches, and it reproduces on unmodified main. Nothing to do here until #75213 lands; happy to rebase the moment it does.