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

tests/tools/test_checkpoint_manager.py

@@ -1049,3 +1049,63 @@ def test_clear_all_on_missing_base_is_noop(self, tmp_path, monkeypatch):
         result = clear_all()
         assert result["deleted"] is False
         assert result["bytes_freed"] == 0
+
+
+# ---------------------------------------------------------------------------
+# Race-safe stale index lock cleanup
+# ---------------------------------------------------------------------------
+
+class TestStaleIndexLockCleanup:
+    """Tests for _cleanup_stale_index_lock — the race-safe stale lock remover."""
+
+    def test_no_lock_no_error(self, tmp_path):
+        """When no lock exists, cleanup is a silent no-op."""
+        from tools.checkpoint_manager import _cleanup_stale_index_lock
+        index_file = tmp_path / "index"
+        # Must not raise
+        _cleanup_stale_index_lock(index_file)
+
+    def test_removes_old_stale_lock(self, tmp_path):
+        """A lock file older than the threshold is removed."""
+        from tools.checkpoint_manager import _cleanup_stale_index_lock
+        index_file = tmp_path / "index"
+        lock_file = tmp_path / "index.lock"
+        lock_file.touch()
+        # Set mtime to 1 hour ago — well beyond the staleness threshold
+        old_time = time.time() - 3600
+        os.utime(lock_file, (old_time, old_time))
+        _cleanup_stale_index_lock(index_file)
+        assert not lock_file.exists(), "stale lock should have been removed"
+
+    def test_leaves_recent_lock_alone(self, tmp_path):
+        """A recently-created lock must not be removed."""
+        from tools.checkpoint_manager import _cleanup_stale_index_lock
+        index_file = tmp_path / "index"
+        lock_file = tmp_path / "index.lock"
+        lock_file.touch()
+        # mtime is now — too recent
+        _cleanup_stale_index_lock(index_file)
+        assert lock_file.exists(), "recent lock should be left alone"
+
+    def test_no_temp_file_left_behind(self, tmp_path):
+        """After cleanup, no temporary renamed file should remain."""
+        from tools.checkpoint_manager import _cleanup_stale_index_lock
+        index_file = tmp_path / "index"
+        lock_file = tmp_path / "index.lock"
+        lock_file.touch()
+        old_time = time.time() - 3600
+        os.utime(lock_file, (old_time, old_time))
+        _cleanup_stale_index_lock(index_file)
+        temps = list(tmp_path.glob(".stale-lock-*"))
+        assert temps == [], f"temp files left behind: {temps}"
+
+    def test_lock_removed_atomically(self, tmp_path):
+        """Verify the rename-based approach: original lock path disappears."""
+        from tools.checkpoint_manager import _cleanup_stale_index_lock
+        index_file = tmp_path / "index"
+        lock_file = tmp_path / "index.lock"
+        lock_file.write_bytes(b"")  # zero-byte like git
+        old_time = time.time() - 600
+        os.utime(lock_file, (old_time, old_time))
+        _cleanup_stale_index_lock(index_file)
+        assert not lock_file.exists()

tools/checkpoint_manager.py

@@ -223,6 +223,97 @@ def _index_path(store: Path, dir_hash: str) -> Path:
     return store / _INDEXES_DIRNAME / dir_hash
 
 
+def _cleanup_stale_index_lock(index_file: Path) -> None:
+    """Best-effort, race-safe cleanup for abandoned checkpoint index locks.
+
+    Git creates ``<GIT_INDEX_FILE>.lock`` with O_EXCL and removes it on normal
+    exit.  If a Hermes worker is killed mid-checkpoint, that zero-byte lock can
+    linger forever and every later checkpoint for the same project fails with
+    "Unable to create ...index.lock: File exists".
+
+    Race-safety: we use *rename*, which is atomic on a single filesystem, to
+    take exclusive ownership of the stale lock *before* unlinking it.  This
+    eliminates the TOCTOU window present in a naive stat-then-unlink: between
+    the stat and the unlink another process could replace the file, and we
+    would delete a live lock.  With rename, either we move the exact stale
+    file (and now own it), or we fail because the file is already gone.
+
+    A lock is considered stale only if its mtime is older than three times the
+    git timeout — generous enough that no in-flight git command (bounded by
+    ``_GIT_TIMEOUT``) could still be using it.
+    """
+    lock_path = index_file.with_name(index_file.name + ".lock")
+    min_age = max(_GIT_TIMEOUT * 3, 300)
+
+    # --- Check 1: is the lock old enough to be stale? ---
+    try:
+        st = lock_path.stat()
+    except FileNotFoundError:
+        return  # already cleaned up
+    except OSError as exc:
+        logger.debug("Could not stat checkpoint index lock %s: %s", lock_path, exc)
+        return
+
+    age = time.time() - st.st_mtime
+    if age < min_age:
+        logger.debug(
+            "Checkpoint index lock is recent; leaving in place: %s (age %.1fs)",
+            lock_path, age,
+        )
+        return
+
+    # --- Check 2: atomically take ownership via rename ---
+    # rename is POSIX-atomic on the same filesystem.  If it succeeds, we
+    # exclusively own the stale lock and no other process can touch the
+    # original path until a new git process creates a fresh one.
+    pid_suffix = os.getpid()
+    tmp_name = f".stale-lock-{pid_suffix}-{int(st.st_mtime)}"
+    stale_copy = index_file.with_name(tmp_name)
+    try:
+        os.rename(lock_path, stale_copy)
+    except FileNotFoundError:
+        return  # someone else already cleaned it up
+    except OSError as exc:
+        # On cross-filesystem rename would fail with EXDEV, but that can't
+        # happen here because the lock and temp are in the same directory.
+        # Any other error is unexpected — log and leave the lock alone.
+        logger.debug("Could not rename stale checkpoint index lock %s: %s", lock_path, exc)
+        return
+
+    # --- Check 3: verify the renamed file matches the original stat ---
+    try:
+        moved = stale_copy.stat()
+    except OSError:
+        moved = None
+    same_lock = (
+        moved is not None
+        and moved.st_dev == st.st_dev
+        and moved.st_ino == st.st_ino
+    )
+    if not same_lock:
+        logger.debug(
+            "Checkpoint index lock changed during cleanup; leaving renamed copy: %s",
+            stale_copy,
+        )
+        try:
+            stale_copy.unlink()
+        except OSError:
+            pass
+        return
+
+    # --- Cleanup: remove the renamed stale lock ---
+    try:
+        stale_copy.unlink()
+        logger.warning(
+            "Removed stale checkpoint index lock: %s (age %.0fs)",
+            lock_path, age,
+        )
+    except FileNotFoundError:
+        pass  # already removed
+    except OSError as exc:
+        logger.debug("Could not remove renamed stale lock %s: %s", stale_copy, exc)
+
+
 def _ref_name(dir_hash: str) -> str:
     return f"{_REFS_PREFIX}/{dir_hash}"
 
@@ -887,6 +978,11 @@ def _take(self, working_dir: str, reason: str) -> bool:
             # First snapshot for this project.
             index_file.parent.mkdir(parents=True, exist_ok=True)
 
+        # Best-effort cleanup of any stale index lock left by a killed
+        # checkpoint attempt.  This prevents a single crashed worker from
+        # permanently blocking checkpoints for a project.
+        _cleanup_stale_index_lock(index_file)
+
         # Stage with per-project index.  Include a per-stage file-size filter
         # via ``core.bigFileThreshold`` is not what we want — instead, we
         # rely on the exclude file for broad patterns and post-stage prune