fix(checkpoint): self-heal stale index.lock from crashed git process

[thapecroth]

Summary

tools/checkpoint_manager._run_git now removes any index.lock older than 60s in the shadow repo before invoking git. Checkpoint ops per shadow are strictly serial (one gateway agent per session), so a lock file that old is unambiguously orphaned from a crashed or killed prior git subprocess.

The bug

A single crashed git add leaves index.lock behind. Every subsequent checkpoint for that shadow then fails with:

fatal: Unable to create '/home/<user>/.hermes/checkpoints/<hash>/index.lock': File exists.
Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again.

Observed in prod logs: 56 identical errors over 6 days on a single shadow repo until manual cleanup. No recovery path — the agent just keeps retrying and failing forever.

Why 60s

Real git ops on a shadow repo finish in milliseconds. 60s is ~1000x the realistic upper bound for an in-flight op, so any lock older than that is a zombie. Conservative enough to never race against a legitimate concurrent op, tight enough to recover within one retry cycle.

Scope

• _clear_stale_lock(shadow_repo) — new helper, called from _run_git entry.
• Logs a WARNING with the lock age when it removes one, so the underlying crash pattern stays visible.
• No behavior change when no lock exists or the lock is fresh.

Test plan

4 new tests in tests/tools/test_checkpoint_manager.py::TestStaleLockCleanup:

• Removes a stale lock (backdated 90s)
• Preserves a fresh lock (might be live op)
• No-op when no lock exists
• End-to-end: git add -A succeeds after stale-lock cleanup

Full tests/tools/test_checkpoint_manager.py + tests/test_batch_runner_checkpoint.py: 69/69 pass.

🤖 Generated with Claude Code

[teknium1]

Thanks for the targeted recovery work. The stale-lock bug class remains relevant, but the original patch predates the checkpoint v2 storage redesign.

Problems

• tools/checkpoint_manager.py:175 removes shadow_repo / "index.lock". On current main, staging uses a per-project index (_index_path() at tools/checkpoint_manager.py:223-224) passed as GIT_INDEX_FILE (tools/checkpoint_manager.py:266-269 and _take() at tools/checkpoint_manager.py:889-925). The proposed target is therefore not the active index used by checkpoint git add.
• The E2E test at tests/tools/test_checkpoint_manager.py:743-752 creates that retired root-level lock shape, so it does not cover the current v2 path.

Suggested changes

• Rework the cleanup to derive the lock from the actual index_file used for index-mutating calls, and add a v2 regression test that plants that lock before a real checkpoint/add operation.
• The linked salvage PR #52887 carries the same root-store target, so it needs the same correction.

This is an automated hermes-sweeper review.

[teknium1]

Current main no longer stages through a per-shadow-repo root index: _take() passes store/indexes/<project-hash> as GIT_INDEX_FILE. This should target the lock associated with the actual index_file, otherwise a stale lock from the active git add path remains untouched.

[GottZ]

This was generated by AI during triage.

Summary

Three PRs address stale Git index locks that can permanently wedge checkpoint creation. #13232 and #52887 clean the retired root/store-level index.lock rather than the active v2 per-project GIT_INDEX_FILE lock, while closed #45871 targets the correct per-project lock and adds replacement-race checks but was withdrawn over a remaining unlink race.

Related pull requests

• #13232 related — (+90/-0) — rework: The diff adds age-based cleanup and tests for shadow_repo/index.lock, but checkpoint v2 git add uses a per-project GIT_INDEX_FILE and therefore creates <index_file>.lock instead. Consistent with the keep_open review on #13232, retain it only as the consolidation vehicle and retarget both implementation and regression coverage to the actual v2 checkpoint path.
• #45871 [closed] duplicate — (+136/-0) — reference implementation, not merge-ready: Although closed, this diff remains relevant because it derives the lock from the active per-project index_file and attempts to preserve a lock replaced between stat checks. It was withdrawn after a local review identified a remaining race between the final identity check and unlink, so that concurrency objection must be resolved before its approach is reused.
• #52887 duplicate — (+89/-5) — duplicate requiring the same rework: This salvage largely reproduces #13232 and still calls cleanup on store/index.lock, while its end-to-end test omits index_file and therefore does not exercise the runtime snapshot route. Despite the keep_open review on #52887, the diff provides no distinct working fix; its useful salvage context can be consolidated into #13232.

Duplicates

#13232 and #52887 are substantially the same root/store-level stale-lock cleanup and test the same inactive lock shape. #45871 addresses the same bug class but is a technically distinct, closer reference because it targets the v2 per-project index lock and adds replacement-race checks.

Suggested consolidation

Merge #13232 only after reworking it to clean the lock adjacent to the actual per-project GIT_INDEX_FILE, adding a real _take or _run_git(..., index_file=...) regression test with fresh-lock coverage, and resolving the remaining check-to-unlink race documented when #45871 was closed. Then #52887 can be closed as a duplicate; keep closed #45871 as the reference for correct v2 lock-path derivation rather than reopening or merging it with its documented race unresolved.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup13232 ["PRs duplicating each other"]
        P13232["PR #13232 (open)"]
        P45871["PR #45871 (closed)"]
        P52887["PR #52887 (open)"]
    end
    class P13232 open
    class P45871 closed
    class P52887 open
    class P13232 target
    click P13232 "https://github.com/NousResearch/hermes-agent/pull/13232"
    click P45871 "https://github.com/NousResearch/hermes-agent/pull/45871"
    click P52887 "https://github.com/NousResearch/hermes-agent/pull/52887"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 17 kB of PR diffs, 6 kB of issue/PR text, 3 kB of discussion (4 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[0xdfi]

Production reproduction against checkpoint v2, confirming the active lock path discussed in review:

Observed failure

• Profile-scoped shared checkpoint store on macOS.

• Checkpoint target was a broad scratch/workspace container: approximately 1.4 GiB with 270 top-level entries.

• _take() invoked _run_git(["add", "-A"], ..., index_file=<per-project-index>).

• The Git subprocess exceeded its 60-second timeout and was killed.

• _run_git() caught subprocess.TimeoutExpired and returned failure, but did not clean up the lock created by the timed-out process.

• The lock left behind was the v2 per-project lock:

  $HERMES_HOME/checkpoints/store/indexes/<project-hash>.lock
  

  It was not store/index.lock and was unrelated to the source repository's .git/index.lock.

• The file was zero bytes, no process owned it (lsof returned no owner), and there were no active checkpoint Git processes. Later checkpoint attempts remained wedged until the orphan was quarantined.

This directly corroborates the review finding: recovery needs to derive the lock from the actual index_file / GIT_INDEX_FILE, not the root store path.

Locally validated recovery behavior

We tested timeout handling with both cases:

1. A lock absent before the Git invocation but created before TimeoutExpired is handled: the orphan is removed after the timed-out child has been killed and reaped.
2. A lock that existed before the invocation is preserved.

We also added a pre-stage worktree-size guard because the triggering checkpoint attempted to inventory a workspace container substantially larger than the checkpoint store's configured capacity. With the guard, the 1.4 GiB worktree is skipped in about 0.6 seconds with no lock and no checkpoint-store growth.

Relevant checkpoint tests plus CLI/path/config coverage passed locally (111 tests).

Concurrency caveat / suggested upstream shape

A simple exists-before / unlink-after-timeout implementation still has a narrow replacement race if another process creates a new lock between timeout handling and unlink. For an upstream implementation, I agree with using the actual per-project index_file path plus an atomic ownership step such as the rename-and-inode-verification approach explored in #45883.

Suggested split:

• This PR: correct v2 per-project stale/timeout lock recovery with race-safe ownership and regression tests through _run_git(..., index_file=...) or _take().
• Separate hardening change: configurable worktree/input-size preflight so a broad scratch container cannot repeatedly trigger expensive git add -A operations.

[0xdfi]

Follow-up evidence from the same host, in a second profile-scoped checkpoint store:

• Found another zero-byte v2 per-project lock at $HERMES_HOME/checkpoints/store/indexes/<project-hash>.lock.
• The lock was approximately three days old.
• lsof showed no owner, and no related checkpoint Git process existed.
• Project metadata mapped the hash to a real workdir.
• This lock predated the current invocation, so a timeout-only cleanup path would preserve it. Git instead fails immediately with File exists, meaning TimeoutExpired is never reached.
• After confirming no owner/process, the lock was atomically moved into the profile store's quarantine directory and the active path was verified absent.

This is a separate concrete example of why the repair must include aged pre-existing lock recovery, not only cleanup after the direct child times out. That recovery should run only while holding a crash-releasing cross-process coordination lock for the relevant per-project index/transaction.

[GottZ]

This was generated by AI during triage.

Delta since our previous triage comment

@0xdfi added two production observations that corroborate the previous review: checkpoint v2 leaves the orphan at the per-project <GIT_INDEX_FILE>.lock, not at the root/store-level path targeted by #13232. The second observation also extends the analysis by showing that recovery must handle aged locks predating the current invocation, while using crash-releasing cross-process coordination to avoid racing an active writer.

Changed pull requests

• #13232 related — (+90/-0) — rework, with stronger production evidence: The new reports confirm that the current diff still cleans the wrong lock path and that timeout-only cleanup is insufficient; #13232 should derive the lock from the supplied per-project index_file and coordinate aged pre-existing-lock recovery across processes.

Suggested consolidation

The previous consolidation recommendation is unchanged: retain #13232 as the rework vehicle and do not merge it until the v2 lock path and documented concurrency race are addressed.

Complex graph unchanged since our previous triage comment.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 17 kB of PR diffs, 6 kB of issue/PR text, 7 kB of discussion (6 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[GottZ]

This was generated by AI during triage.

Delta since our previous triage comment

@Bartok9 rebuilt #52887 onto the latest main, changing its current diff to +34/-5. The refreshed diff confirms rather than corrects the prior finding: _clear_stale_lock(store) still targets store/index.lock instead of the active per-project <GIT_INDEX_FILE>.lock.

Changed pull requests

• #52887 duplicate — (+34/-5) — rework unchanged after rebuild: Despite the keep_open review on #52887, the refreshed diff still calls _clear_stale_lock(store) and therefore misses the per-project lock used by checkpoint v2; the unrelated formatting/comment edits do not address that root cause.

Suggested consolidation

The consolidation recommendation is unchanged, consistent with the backlog lane: retain #13232 as the rework vehicle and close #52887 as its duplicate once the corrected v2 fix is consolidated.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup13232 ["PRs duplicating each other"]
        P13232["PR #13232 (open)"]
        P45871["PR #45871 (closed)"]
        P52887["PR #52887 (open)"]
    end
    class P13232 open
    class P45871 closed
    class P52887 open
    class P13232 target
    click P13232 "https://github.com/NousResearch/hermes-agent/pull/13232"
    click P45871 "https://github.com/NousResearch/hermes-agent/pull/45871"
    click P52887 "https://github.com/NousResearch/hermes-agent/pull/52887"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 14 kB of PR diffs, 6 kB of issue/PR text, 7 kB of discussion (7 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.