Skip to content

fix(checkpoint): self-heal stale index.lock from crashed git process (salvage of #13232 by @thapecroth) - #52887

Open
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/13232-checkpoint-stale-lock
Open

fix(checkpoint): self-heal stale index.lock from crashed git process (salvage of #13232 by @thapecroth)#52887
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/13232-checkpoint-stale-lock

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Salvage of #13232 by @thapecroth — rebased onto current origin/main with regression tests.

A crashed/killed git process leaves an orphaned index.lock in the checkpoint shadow repo. Because nothing cleans it up, every subsequent checkpoint git op for that repo fails ("Unable to create '.../index.lock': File exists") — checkpointing stays wedged until manual cleanup (observed in production: 56+ errors over 6 days on one shadow repo).

Root Cause

Symptom — Checkpointing silently stops working for a session/repo and the log fills with index.lock: File exists errors until someone manually deletes the lock.

Root cause_run_git (tools/checkpoint_manager.py) shells out to git against the shadow repo but never accounts for a zombie index.lock left by a previously crashed/killed git invocation. Git refuses to acquire the index lock while the file exists, so every later git add/commit fails. There's no self-healing path.

Evidence — The end-to-end test plants a backdated index.lock in a real shadow repo, then runs git add -A; without the fix the function doesn't exist / the lock wedges the op, with the fix the lock is removed and the add succeeds.

Fix + why this level — Add _clear_stale_lock(shadow_repo), called at the top of every _run_git. It removes index.lock only when it's older than _STALE_LOCK_SECONDS (60s). This is safe because checkpoint ops against a shadow repo are strictly serial (one gateway agent per session), so a lock that old is unambiguously orphaned. This is the correct level — _run_git is the single choke point for all checkpoint git calls, so one guard protects add/commit/diff uniformly; a fresh lock (possible live op) is deliberately left alone.

Scope / risk — One helper + one call at the top of _run_git. Fresh locks (<60s) are preserved (test test_preserves_fresh_lock); absent lock is a no-op; unlink failures are logged, not raised. Non-checkpoint git usage is unaffected.

Changes from original

  • Rebased onto current main (clean single commit); wired _clear_stale_lock into the current _run_git (call before subprocess.run, keyed on the store shadow path).
  • Carried the original's 4 regression tests (TestStaleLockCleanup) — all fail without the fix (function absent / lock wedges the op).

Verification

python3 -m pytest tests/tools/test_checkpoint_manager.py -q
81 passed

# without the fix (stashed):
ImportError: cannot import name '_clear_stale_lock'  (+ end-to-end wedge)
4 failed

Real behavior proof

# Shadow repo with a stale index.lock (backdated >60s), then real `git add -A`:
# With fix:    git add succeeds; "Removed stale index.lock (age=90s) at .../index.lock"
# Without fix: every checkpoint git op fails with "index.lock: File exists" until manual cleanup

Credit: salvage of #13232 by @thapecroth — rebased onto current main with guardrail tests.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have labels Jun 26, 2026
@Bartok9

Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Plate-clear rebase (2026-07-11)

  • Rebased onto upstream/main (6142203bd).
  • Conflicts resolved: 1 file(s) (feature side preferred for intentional patches when conflicted).
  • Force-with-lease push to Bartok9 branch salvage/13232-checkpoint-stale-lock.
  • Intent preserved; still awaiting CI green before merge.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for salvaging the stale-lock report and preserving the Windows subprocess behavior. The current implementation needs targeted rework before it covers the checkpoint path.

Problems

  • tools/checkpoint_manager.py:310 checks store/index.lock, and the new call at :357 always passes store. Current snapshots instead select store/indexes/<hash> at current main tools/checkpoint_manager.py:889-890 and pass it to git add at :923-926; Git therefore locks <index_file>.lock, not the root-store path.
  • The regression test at tests/tools/test_checkpoint_manager.py:1100-1105 creates the same root lock and calls _run_git without index_file, so it does not exercise the runtime snapshot route.

Suggested changes

  • Derive cleanup from the supplied index_file and remove its sibling .lock only when stale; keep root-index cleanup only for a verified caller that uses it.
  • Test a stale lock beside _index_path(...) through _take or _run_git(..., index_file=...), including the fresh-lock guard.

The v2 shared-store/per-project-index model was introduced by a0fedfbb1; this is a focused salvage correction rather than a clean cherry-pick. Automated hermes-sweeper review.


# Clear any orphaned index.lock from a previously crashed git op so this
# serial checkpoint call isn't wedged by a zombie lock.
_clear_stale_lock(store)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clears store/index.lock, but production checkpoint staging uses GIT_INDEX_FILE=store/indexes/<hash> (_take passes that index_file to git add). Its stale lock is <index_file>.lock, so this does not recover the runtime wedge. Please derive the cleanup target from index_file and test that path.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
mehmetkr-31 added a commit to mehmetkr-31/hermes-agent that referenced this pull request Jul 31, 2026
Extends the recovery from the per-project index lock to whatever lock git
itself reports, after two findings while comparing against NousResearch#13232/NousResearch#52887.

1. Locale. Detection previously matched the English "unable to create" in
   stderr. git localizes that prose — under a Turkish locale the same
   failure reads "onulmaz: '<path>' oluşturulamıyor: File exists." — so
   recovery would have silently stopped working for every non-English
   install. Detection now matches the quoted *path* ending in .lock, which
   git does not translate.

2. Coverage. The store takes more than one lock. `git add -A` passes
   index_file and so takes store/indexes/<hash>.lock, but the 43 calls that
   pass no index_file use git's default $GIT_DIR/index, whose lock is
   store/index.lock, and update-ref/maintenance calls take ref locks under
   store/refs/. A killed process wedges whichever one it held. Reclaiming
   the path git names in its error covers all of them with one mechanism and
   no guessing, so _index_lock_path() is now total (per-project index when
   index_file is given, store root otherwise) and the retry path reads its
   target out of stderr.

Reclaimed paths are validated to resolve *inside* the checkpoint store, so a
surprising or hostile message can never point the cleanup at a file we do
not own; relative paths are ignored. The staleness proof is unchanged.

Adds 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.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(salvage of NousResearch#13232 by @thapecroth)

Rebuilt on latest main (Bartok9 hygiene 2026-08-01).
Original: NousResearch#52887
@Bartok9

Bartok9 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebuilt onto latest main via patch re-apply (force-push). Please re-run CI.

— Bartok9 public PR hygiene 2026-08-01

@Bartok9
Bartok9 force-pushed the salvage/13232-checkpoint-stale-lock branch from 66e3c5b to d52e042 Compare August 1, 2026 17:36
…tok9

Per-PR attribution so check-attribution passes on this branch (Teknium).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants