diff --git a/tests/tools/test_checkpoint_manager.py b/tests/tools/test_checkpoint_manager.py index 5c6db10c0119..4d8fe31546c7 100644 --- a/tests/tools/test_checkpoint_manager.py +++ b/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() diff --git a/tools/checkpoint_manager.py b/tools/checkpoint_manager.py index f0b47734cea5..d73207d39d51 100644 --- a/tools/checkpoint_manager.py +++ b/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 ``.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