Skip to content

fix(checkpoints): recover from stale git locks in the shadow store - #92127

Open
33hodl wants to merge 1 commit into
NousResearch:mainfrom
33hodl:pr-checkpoint-stale-lock-repair
Open

fix(checkpoints): recover from stale git locks in the shadow store#92127
33hodl wants to merge 1 commit into
NousResearch:mainfrom
33hodl:pr-checkpoint-stale-lock-repair

Conversation

@33hodl

@33hodl 33hodl commented Aug 22, 2026

Copy link
Copy Markdown

Problem

A git process killed mid-write (gateway restart with KillMode=mixed, crash, OOM) leaves its <target>.lock file behind in the checkpoint shadow store. Every later git command needing that target then fails with:

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

(rc=128) until a human removes the file by hand. The checkpoint manager has no recovery for this — it silently returns False from _take() and the affected project never gets snapshots again.

Reproduction (production, Aug 2026)

Two abandoned locks sat in the store since 2026-08-04 and 2026-08-13 (killed git processes — no live process can hold a lock for that long). From that point on, every checkpoint snapshot of the affected project failed identically, 18+ times in a 24h window:

ERROR tools.checkpoint_manager: Git command failed: git add -A (rc=128)
  stderr=fatal: Unable to create '.../indexes/e9671acd244849c5.lock': File exists.

Manual repro of the same failure:

$ GIT_DIR=.../checkpoints/store GIT_INDEX_FILE=.../indexes/e9671acd244849c5 git add -A
fatal: Unable to create '.../indexes/e9671acd244849c5.lock': File exists.

Git itself suggests "remove the file manually to continue" — exactly what the manager should do, safely, on its own.

Fix

  • _repair_stale_locks(store) — sweeps the store for .lock files older than _STALE_LOCK_MAX_AGE_S (600s) and unlinks them. Git holds a lock for the duration of a single command bounded by _GIT_TIMEOUT (30s), so anything older is abandoned by definition; a live concurrent git process is never disturbed (10-minute margin).
  • _run_git retry — on the exact failure signature (rc=128 + Unable to create + File exists), run the sweep and retry the command once. This covers every call site (snapshot, diff, restore, prune) in one place.
  • _init_store proactive sweep — the shared entry point of every snapshot path clears stale locks before any git command runs, so the first operation never fails.

Why a bounded retry instead of always sweeping before every git call: sweeping is only needed when a lock is actually present, and the retry makes the repair self-healing at the exact point of failure while the proactive sweep keeps the common path clean.

Validation

  • 3 new regression tests in tests/tools/test_checkpoint_manager.py:
    • test_repair_only_removes_old_locks — fresh lock kept, stale lock removed, count correct
    • test_run_git_retries_once_after_clearing_stale_lock — the exact production failure (stale lock on the per-project index) now succeeds via the retry path
    • test_snapshot_succeeds_after_stale_lock_repair — end-to-end through ensure_checkpoint
  • Full file suite: 47 passed (3 failures in TestSafeRestore are pre-existing on main — verified by stashing the change and re-running; unrelated safe-restore user-edit tests)
  • ruff check clean on both changed files
  • Production observation: after the fix removed the two abandoned locks, the previously failing git add -A for the affected project succeeds

Killed git processes (gateway restart, crash, OOM) leave <target>.lock
files behind in the checkpoint store. Every later command needing that
target fails with rc=128 'Unable to create ...: File exists' until a
human removes the lock by hand, silently disabling checkpoints for the
affected project (observed in production: 18+ failures over 24h from
locks abandoned 18 days earlier).

Sweep locks older than _STALE_LOCK_MAX_AGE_S (600s; git holds a lock
for seconds at most) from _init_store so the common path never fails,
and retry once in _run_git on the exact lock-exists failure signature
so every call site self-heals. Adds 3 regression tests.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #13232, #52887, and #74737 address the same stale-checkpoint-lock family. This PR uses a broader store sweep plus error-triggered retry; maintainers should choose or consolidate the recovery policy.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.
Sound recovery design with the safety reasoning spelled out where it matters:

  1. The 600s staleness cutoff is justified against _GIT_TIMEOUT (≤60s): a lock older than ten minutes cannot belong to a git process this manager spawned, so removal can't disturb a live operation. That argument — documented next to the constant — is what makes age-based cleanup defensible.
  2. The retry is tightly gated (rc=128 + "Unable to create" + "File exists", repair found something, retry once), so genuine git failures aren't masked by blind retries.
  3. Sweeping at _init_store before any operation prevents the first snapshot after a crash from hitting the wedge, and restricting the objects/ walk to pack/ avoids touching loose-object dirs.

Tests cover all three layers: repair-only-removes-old, retry-succeeds-after-clear, and end-to-end snapshot-after-repair with mtime backdating.

Minor notes:

  1. Clock skew / coarse-mtime filesystems could make a recent lock look old (or vice versa). Given the store is Hermes-owned and the 10× margin over the max command timeout, the exposure is negligible — but if you ever want belt-and-braces, comparing against the lock file's ctime too costs two lines.
  2. logger.warning per removed lock is the right level: rare, actionable, and greppable in errors.log when someone asks why checkpoints resumed working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants