Skip to content

fix(checkpoints): race-safe stale index lock cleanup via atomic rename - #45883

Closed
Veritas-7 wants to merge 1 commit into
NousResearch:mainfrom
Veritas-7:veritas/checkpoint-stale-lock-racesafe-v2
Closed

fix(checkpoints): race-safe stale index lock cleanup via atomic rename#45883
Veritas-7 wants to merge 1 commit into
NousResearch:mainfrom
Veritas-7:veritas/checkpoint-stale-lock-racesafe-v2

Conversation

@Veritas-7

Copy link
Copy Markdown
Contributor

Summary

  • Adds _cleanup_stale_index_lock() which uses os.rename (POSIX-atomic on a single filesystem) to take exclusive ownership of a stale checkpoint index lock before removing it.
  • Called before git add -A in the checkpoint creation path, preventing a single crashed worker from permanently blocking checkpoints for a project.

Problem

When a Hermes worker is killed mid-checkpoint, git's zero-byte <GIT_INDEX_FILE>.lock can linger forever. Every subsequent checkpoint for that project fails with Unable to create ...index.lock: File exists.

A previous attempt (#45871) used stat-then-unlink with a second stat-compare to narrow the race window. This PR eliminates the race entirely by using rename -- an atomic operation -- to take ownership of the stale lock before unlinking.

Race-safety design

  1. Check age: lock mtime must be older than max(_GIT_TIMEOUT * 3, 300s).
  2. Atomic rename: os.rename(lock, .stale-lock-PID-mtime) -- either we move the exact stale file, or it is already gone.
  3. Verify: confirm the renamed file st_dev + st_ino match the original stat.
  4. Cleanup: unlink the renamed file.

Tests

  • 5 new focused tests: no-lock, stale removal, recent-lock skip, temp-file cleanup, atomicity verification
  • Full suite: 82 passed
  • gitleaks: no leaks found

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have labels Jun 14, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verified clean — no issues found.

Reviewed the full diff (2 files, +156/−0):

  • Atomic stale-lock cleanup: uses os.rename() (POSIX-atomic on same filesystem) to take exclusive ownership of a stale .lock file before unlinking. This eliminates the TOCTOU window in a naive stat-then-unlink approach.
  • Three-phase verification: (1) stat + age check, (2) atomic rename, (3) inode comparison to confirm the renamed file is the same one that was stat'd. If the inode doesn't match, the renamed copy is cleaned up and the lock is left alone.
  • Staleness threshold: max(_GIT_TIMEOUT * 3, 300) — generous enough that no in-flight git command could still be using the lock.
  • Called before every _take(): ensures a crashed worker's abandoned lock doesn't permanently block checkpoints for a project.
  • Test coverage: 5 tests covering no-lock, stale lock removal, recent lock preservation, no temp files left behind, and atomic rename verification.
  • CI note: test (6) fails on test_notification_poller_emits_distinct_watch_matches_once — a pre-existing flaky test in tui_gateway/server.py unrelated to this change.

LGTM.

Use os.rename to take exclusive ownership of a stale checkpoint index lock
before removing it, eliminating the TOCTOU race in stat-then-unlink.
Adds 5 focused tests covering no-lock, stale removal, recent-lock skip,
temp-file cleanup, and atomicity verification.
@Veritas-7
Veritas-7 force-pushed the veritas/checkpoint-stale-lock-racesafe-v2 branch from af6624f to c2adfc4 Compare June 14, 2026 11:26
AIalliAI added a commit to AIalliAI/Hermes that referenced this pull request Jun 14, 2026
@Veritas-7 Veritas-7 closed this Jun 16, 2026
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 P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants