diff --git a/tests/tools/test_checkpoint_manager.py b/tests/tools/test_checkpoint_manager.py index db96b0eafaf4..f772aa18ed4a 100644 --- a/tests/tools/test_checkpoint_manager.py +++ b/tests/tools/test_checkpoint_manager.py @@ -1044,3 +1044,466 @@ def test_includes_newly_added_files(self, mgr, work_dir): assert result["success"] is True assert "feature.py" in result["diff"] assert "+x = 1" in result["diff"] + + +# ========================================================================= +# Abandoned index lock recovery (#74108) +# ========================================================================= + +class TestAbandonedIndexLockRecovery: + """A killed git leaves ``.lock`` behind and never reclaims it. + + Git creates the lock with O_EXCL and renames it into place on success. + The file records no owner, so after a kill (readily triggered by our + subprocess timeout on a WSL2 drvfs/9p work tree such as /mnt/c) every + later ``git add`` against that index fails instantly with + "Unable to create ... File exists" — for the rest of the session and + every future session, until a human deletes it. + + These exercise the real ``_run_git`` against real git, not mocks. + """ + + @staticmethod + def _prepare_store(work_dir, checkpoint_base, monkeypatch): + monkeypatch.setattr( + "tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base, + ) + store = _store_path(checkpoint_base) + assert _init_store(store, str(work_dir)) is None + dir_hash = _project_hash(str(work_dir)) + from tools.checkpoint_manager import _index_path + + index_file = _index_path(store, dir_hash) + index_file.parent.mkdir(parents=True, exist_ok=True) + return store, index_file + + def test_stale_lock_is_reclaimed_and_the_call_retried( + self, work_dir, checkpoint_base, monkeypatch, + ): + """The healing path: an install already wedged by an earlier timeout.""" + from tools.checkpoint_manager import ( + _MAX_GIT_CALL_SECONDS, _index_lock_path, + ) + + store, index_file = self._prepare_store( + work_dir, checkpoint_base, monkeypatch, + ) + lock = _index_lock_path(index_file, store) + lock.write_text("") + # Age it past the widest possible git-call window so it is provably + # owned by nobody. + old = time.time() - (_MAX_GIT_CALL_SECONDS + 60) + os.utime(lock, (old, old)) + + ok, _out, err = _run_git( + ["add", "-A"], store, str(work_dir), index_file=index_file, + ) + + assert ok, f"git add should succeed after reclaiming the lock: {err}" + assert not lock.exists() + + def test_fresh_lock_is_left_alone( + self, work_dir, checkpoint_base, monkeypatch, + ): + """A lock young enough to belong to a live call must not be stolen.""" + from tools.checkpoint_manager import _index_lock_path + + store, index_file = self._prepare_store( + work_dir, checkpoint_base, monkeypatch, + ) + lock = _index_lock_path(index_file, store) + lock.write_text("") # mtime = now + + ok, _out, err = _run_git( + ["add", "-A"], store, str(work_dir), index_file=index_file, + ) + + assert not ok + assert lock.exists(), "a concurrent call's lock must survive" + assert "unable to create" in err.lower() + + def test_timeout_reclaims_its_own_lock_without_an_age_check( + self, work_dir, checkpoint_base, monkeypatch, + ): + """The prevention path: our child was killed, so its lock is ours. + + The lock is brand new here — an age check would refuse it — but the + timeout handler knows subprocess.run already reaped the owner. + """ + from tools.checkpoint_manager import _index_lock_path + + store, index_file = self._prepare_store( + work_dir, checkpoint_base, monkeypatch, + ) + lock = _index_lock_path(index_file, store) + + real_run = subprocess.run + + def fake_run(cmd, **kwargs): + # Simulate git creating its lock and then being killed. + lock.write_text("") + raise subprocess.TimeoutExpired(cmd, kwargs.get("timeout", 30)) + + monkeypatch.setattr(subprocess, "run", fake_run) + ok, _out, err = _run_git( + ["add", "-A"], store, str(work_dir), index_file=index_file, + ) + monkeypatch.setattr(subprocess, "run", real_run) + + assert not ok + assert "timed out" in err + assert not lock.exists(), ( + "the timeout handler must reclaim the lock its own child " + "abandoned, or every later checkpoint fails with 'File exists'" + ) + + def test_recovery_retries_only_once( + self, work_dir, checkpoint_base, monkeypatch, + ): + """A permanently failing call must not recurse.""" + from tools.checkpoint_manager import ( + _MAX_GIT_CALL_SECONDS, _index_lock_path, + ) + + store, index_file = self._prepare_store( + work_dir, checkpoint_base, monkeypatch, + ) + lock = _index_lock_path(index_file, store) + calls = [] + real_run = subprocess.run + + def fake_run(cmd, **kwargs): + calls.append(cmd) + # Always re-plant a stale lock and always fail with git's message. + lock.write_text("") + old = time.time() - (_MAX_GIT_CALL_SECONDS + 60) + os.utime(lock, (old, old)) + return subprocess.CompletedProcess( + cmd, 128, "", + f"fatal: Unable to create '{lock}': File exists.", + ) + + monkeypatch.setattr(subprocess, "run", fake_run) + ok, _out, _err = _run_git( + ["add", "-A"], store, str(work_dir), index_file=index_file, + ) + monkeypatch.setattr(subprocess, "run", real_run) + + assert not ok + assert len(calls) == 2, f"expected one retry, got {len(calls)} calls" + + def test_unrelated_failures_do_not_touch_the_lock( + self, work_dir, checkpoint_base, monkeypatch, + ): + from tools.checkpoint_manager import ( + _MAX_GIT_CALL_SECONDS, _index_lock_path, + ) + + store, index_file = self._prepare_store( + work_dir, checkpoint_base, monkeypatch, + ) + lock = _index_lock_path(index_file, store) + lock.write_text("") + old = time.time() - (_MAX_GIT_CALL_SECONDS + 60) + os.utime(lock, (old, old)) + + # A failure whose stderr is not the lock message leaves it in place. + ok, _out, _err = _run_git( + ["rev-parse", "--verify", "refs/heads/does-not-exist"], + store, str(work_dir), index_file=index_file, + ) + + assert not ok + assert lock.exists() + + +class TestLockReclaimCoversEveryLockClass: + """The wedge is not specific to the per-project index (#74108). + + Any git lock left by a killed process blocks its operation forever, and + the checkpoint store takes several: the per-project index for ``add``, + the store-root index for calls that pass no ``index_file``, and ref locks + for ``update-ref``/maintenance. Reclaiming the path git *itself* names + covers all of them with one mechanism and no guessing. + """ + + @staticmethod + def _prepare_store(work_dir, checkpoint_base, monkeypatch): + monkeypatch.setattr( + "tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base, + ) + store = _store_path(checkpoint_base) + assert _init_store(store, str(work_dir)) is None + return store + + def test_store_root_index_lock_is_the_target_without_index_file( + self, work_dir, checkpoint_base, monkeypatch, + ): + """#52887 cleans this one; it is real, but it is not the add path.""" + from tools.checkpoint_manager import _index_lock_path + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + assert _index_lock_path(None, store) == store / "index.lock" + + def test_per_project_index_lock_is_the_target_for_add( + self, work_dir, checkpoint_base, monkeypatch, + ): + """...and this is the one `git add -A` actually takes.""" + from tools.checkpoint_manager import _index_path, _index_lock_path + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + index_file = _index_path(store, _project_hash(str(work_dir))) + assert _index_lock_path(index_file, store) == index_file.with_name( + index_file.name + ".lock" + ) + assert (store / "index.lock") != _index_lock_path(index_file, store) + + def test_ref_lock_named_by_git_is_reclaimed( + self, work_dir, checkpoint_base, monkeypatch, + ): + """A killed update-ref wedges that ref, not the index.""" + from tools.checkpoint_manager import ( + _MAX_GIT_CALL_SECONDS, _reclaimable_locks_from_stderr, + ) + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + ref_lock = store / "refs" / "heads" / "hermes-checkpoint.lock" + ref_lock.parent.mkdir(parents=True, exist_ok=True) + ref_lock.write_text("") + old = time.time() - (_MAX_GIT_CALL_SECONDS + 60) + os.utime(ref_lock, (old, old)) + + stderr = ( + "error: cannot lock ref 'refs/heads/hermes-checkpoint': " + f"Unable to create '{ref_lock}': File exists." + ) + assert _reclaimable_locks_from_stderr(stderr, store) == [ref_lock.resolve()] + + def test_detection_survives_a_localized_git( + self, work_dir, checkpoint_base, monkeypatch, + ): + """git translates the prose but never the path. + + Matching on the English "unable to create" would silently stop + recovering for every non-English locale. + """ + from tools.checkpoint_manager import _reclaimable_locks_from_stderr + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + lock = store / "index.lock" + lock.write_text("") + turkish = f"onulmaz: '{lock}' oluşturulamıyor: File exists." + + assert _reclaimable_locks_from_stderr(turkish, store) == [lock.resolve()] + + def test_paths_outside_the_store_are_never_reclaimed( + self, work_dir, checkpoint_base, monkeypatch, tmp_path, + ): + """A message must not be able to point the cleanup at someone else.""" + from tools.checkpoint_manager import _reclaimable_locks_from_stderr + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + outsider = tmp_path / "not-ours.lock" + outsider.write_text("") + stderr = f"fatal: Unable to create '{outsider}': File exists." + + assert _reclaimable_locks_from_stderr(stderr, store) == [] + assert outsider.exists() + + def test_relative_paths_in_stderr_are_ignored( + self, work_dir, checkpoint_base, monkeypatch, + ): + from tools.checkpoint_manager import _reclaimable_locks_from_stderr + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + assert _reclaimable_locks_from_stderr( + "fatal: Unable to create 'index.lock': File exists.", store, + ) == [] + + def test_end_to_end_ref_lock_recovery_through_run_git( + self, work_dir, checkpoint_base, monkeypatch, + ): + """Real git, real ref lock: the retry clears it and the call succeeds.""" + from tools.checkpoint_manager import _MAX_GIT_CALL_SECONDS, _index_path + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + index_file = _index_path(store, _project_hash(str(work_dir))) + index_file.parent.mkdir(parents=True, exist_ok=True) + assert _run_git(["add", "-A"], store, str(work_dir), index_file=index_file)[0] + ok, sha, err = _run_git( + ["commit", "-m", "snap"], store, str(work_dir), index_file=index_file, + ) + assert ok, err + + ref = "refs/heads/hermes-test-ref" + assert _run_git(["update-ref", ref, "HEAD"], store, str(work_dir))[0] + + ref_lock = store / (ref + ".lock") + ref_lock.parent.mkdir(parents=True, exist_ok=True) + ref_lock.write_text("") + old = time.time() - (_MAX_GIT_CALL_SECONDS + 60) + os.utime(ref_lock, (old, old)) + + ok, _out, err = _run_git(["update-ref", ref, "HEAD"], store, str(work_dir)) + + assert ok, f"update-ref should succeed after reclaiming the ref lock: {err}" + assert not ref_lock.exists() + + +class TestTimeoutCleanupIsCommandSpecific: + """The timeout handler must not guess which lock a command held (#74737 review). + + ``update-ref`` takes a ref lock and passes no ``index_file``. Assuming the + index there would both miss the ref lock the killed call actually left and + put an unrelated, possibly *live*, root-index lock at risk. + """ + + @staticmethod + def _prepare_store(work_dir, checkpoint_base, monkeypatch): + monkeypatch.setattr( + "tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base, + ) + store = _store_path(checkpoint_base) + assert _init_store(store, str(work_dir)) is None + return store + + def test_update_ref_timeout_clears_its_ref_lock_not_the_index( + self, work_dir, checkpoint_base, monkeypatch, + ): + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + ref = "refs/hermes-checkpoints/abc123" + ref_lock = store / f"{ref}.lock" + ref_lock.parent.mkdir(parents=True, exist_ok=True) + root_index_lock = store / "index.lock" + root_index_lock.write_text("") # an unrelated, live lock + + real_run = subprocess.run + + def fake_run(cmd, **kwargs): + ref_lock.write_text("") + raise subprocess.TimeoutExpired(cmd, kwargs.get("timeout", 30)) + + monkeypatch.setattr(subprocess, "run", fake_run) + ok, _out, err = _run_git(["update-ref", ref, "HEAD"], store, str(work_dir)) + monkeypatch.setattr(subprocess, "run", real_run) + + assert not ok and "timed out" in err + assert not ref_lock.exists(), "the ref lock this call held must be cleared" + assert root_index_lock.exists(), ( + "an unrelated root-index lock must never be collateral damage" + ) + + def test_unmappable_command_clears_nothing_on_timeout( + self, work_dir, checkpoint_base, monkeypatch, + ): + """gc/reflog take locks we do not predict — defer to recovery.""" + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + root_index_lock = store / "index.lock" + root_index_lock.write_text("") + + real_run = subprocess.run + + def fake_run(cmd, **kwargs): + raise subprocess.TimeoutExpired(cmd, kwargs.get("timeout", 30)) + + monkeypatch.setattr(subprocess, "run", fake_run) + _run_git(["gc", "--prune=now", "--quiet"], store, str(work_dir)) + monkeypatch.setattr(subprocess, "run", real_run) + + assert root_index_lock.exists() + + def test_lock_predating_the_call_is_not_ours( + self, work_dir, checkpoint_base, monkeypatch, + ): + """Our child died, but this lock was already there — somebody else's.""" + from tools.checkpoint_manager import _index_path, _index_lock_path + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + index_file = _index_path(store, _project_hash(str(work_dir))) + index_file.parent.mkdir(parents=True, exist_ok=True) + lock = _index_lock_path(index_file, store) + lock.write_text("") + old = time.time() - 5 # before this call starts, but recent = live + os.utime(lock, (old, old)) + + real_run = subprocess.run + + def fake_run(cmd, **kwargs): + raise subprocess.TimeoutExpired(cmd, kwargs.get("timeout", 30)) + + monkeypatch.setattr(subprocess, "run", fake_run) + _run_git(["add", "-A"], store, str(work_dir), index_file=index_file) + monkeypatch.setattr(subprocess, "run", real_run) + + assert lock.exists(), "a lock older than our launch belongs to another call" + + +class TestReclaimSurvivesTheReplacementRace: + """Judging and deleting a lock by name is not atomic (#74737 review). + + Between the staleness check and the unlink, another session can finish its + own recovery and a fresh git can create a new lock at the same pathname. + Unlinking by name would delete that live lock, so the removal claims the + lock with an atomic rename and verifies it got the inode it judged. + """ + + @staticmethod + def _prepare_store(work_dir, checkpoint_base, monkeypatch): + monkeypatch.setattr( + "tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base, + ) + store = _store_path(checkpoint_base) + assert _init_store(store, str(work_dir)) is None + return store + + def test_a_lock_replaced_mid_reclaim_is_put_back( + self, work_dir, checkpoint_base, monkeypatch, + ): + from tools.checkpoint_manager import ( + _MAX_GIT_CALL_SECONDS, _clear_abandoned_lock, + ) + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + lock = store / "index.lock" + lock.write_text("stale") + old = time.time() - (_MAX_GIT_CALL_SECONDS + 60) + os.utime(lock, (old, old)) + + real_rename = os.rename + + def racing_rename(src, dst, *a, **k): + # Fire inside the window between our staleness judgement and our + # claim: another session reclaims the stale inode and a fresh git + # creates a NEW lock at the same pathname. Our rename must then + # claim the replacement, and the identity check must catch it. + if str(src) == str(lock): + lock.unlink() + lock.write_text("fresh-and-live") + return real_rename(src, dst, *a, **k) + + monkeypatch.setattr(os, "rename", racing_rename) + removed = _clear_abandoned_lock(lock, "test") + monkeypatch.setattr(os, "rename", real_rename) + + assert removed is False + assert lock.exists(), "the replacement lock must survive" + assert lock.read_text() == "fresh-and-live" + leftovers = list(store.glob("index.lock.reclaim-*")) + assert not leftovers, f"claim files must not be left behind: {leftovers}" + + def test_an_unreplaced_stale_lock_is_still_removed( + self, work_dir, checkpoint_base, monkeypatch, + ): + from tools.checkpoint_manager import ( + _MAX_GIT_CALL_SECONDS, _clear_abandoned_lock, + ) + + store = self._prepare_store(work_dir, checkpoint_base, monkeypatch) + lock = store / "index.lock" + lock.write_text("stale") + old = time.time() - (_MAX_GIT_CALL_SECONDS + 60) + os.utime(lock, (old, old)) + + assert _clear_abandoned_lock(lock, "test") is True + assert not lock.exists() + assert not list(store.glob("index.lock.reclaim-*")) diff --git a/tools/checkpoint_manager.py b/tools/checkpoint_manager.py index df3666f134b3..e273f064a9c8 100644 --- a/tools/checkpoint_manager.py +++ b/tools/checkpoint_manager.py @@ -56,10 +56,11 @@ import shutil import subprocess import time +import uuid from pathlib import Path from hermes_constants import get_hermes_home from hermes_cli._subprocess_compat import windows_hide_flags -from typing import Dict, List, Optional, Set, Tuple +from typing import Dict, List, Optional, Sequence, Set, Tuple from utils import env_int @@ -224,6 +225,11 @@ def _index_path(store: Path, dir_hash: str) -> Path: return store / _INDEXES_DIRNAME / dir_hash +# Widest window any single checkpoint git call can occupy: the longest caller +# multiplier applied to _GIT_TIMEOUT. A lock older than this cannot belong to +# a still-running call, which is what makes reclaiming it safe. +_MAX_GIT_CALL_SECONDS: int = _GIT_TIMEOUT * 3 + def _ref_name(dir_hash: str) -> str: return f"{_REFS_PREFIX}/{dir_hash}" @@ -232,9 +238,206 @@ def _project_meta_path(store: Path, dir_hash: str) -> Path: return store / _PROJECTS_DIRNAME / f"{dir_hash}.json" -# --------------------------------------------------------------------------- -# Git env -# --------------------------------------------------------------------------- +# Git names the exact lock file it could not create, in single quotes, for +# every lock class — the index ("Unable to create '/index.lock': File +# exists") and refs ("cannot lock ref 'refs/...': Unable to create +# '/refs/heads/x.lock'"). Matching the quoted *path* rather than the +# English prose keeps this working under a localized git: the surrounding +# message is translated, the path is not. +_LOCK_PATH_RE = re.compile(r"'([^']+\.lock)'") + + +def _index_lock_path(index_file: Optional[Path], store: Path) -> Path: + """Path of the ``.lock`` git creates while writing this call's index. + + Calls that pass ``index_file`` use the per-project index + (``store/indexes/``); the rest fall back to git's default + ``$GIT_DIR/index``. Both are real: ``git add -A`` takes the former, while + maintenance calls that pass no index take the latter. + """ + if index_file is not None: + return index_file.with_name(index_file.name + ".lock") + return store / "index.lock" + + +# Git subcommands that take the index lock. Anything not listed (gc, reflog, +# for-each-ref, rev-list, log, cat-file, ...) either takes no lock or takes one +# we do not try to predict — those heal through the stderr-driven recovery +# path instead of a guess. +_INDEX_LOCKING_GIT_COMMANDS = frozenset({ + "add", "read-tree", "commit", "checkout", "reset", "rm", "mv", "apply", +}) + + +def _lock_for_timed_out_command( + args: Sequence[str], store: Path, index_file: Optional[Path], +) -> Optional[Path]: + """The lock *this* command would have been holding, or None if unknown. + + The timeout handler must not guess. ``update-ref`` takes a ref lock and + passes no ``index_file``, so assuming the index would both miss its real + lock and put an unrelated root-index lock at risk. Returning None simply + defers to the recovery path, which reads the lock's path out of git's own + error on the next call. + """ + if not args: + return None + command = args[0] + if command in _INDEX_LOCKING_GIT_COMMANDS: + return _index_lock_path(index_file, store) + if command == "update-ref": + for arg in args[1:]: + if arg.startswith("-"): + continue + return store / f"{arg}.lock" if arg.startswith("refs/") else None + return None + + +def _reclaimable_locks_from_stderr(stderr: str, store: Path) -> List[Path]: + """Lock files git named in its own error, restricted to our own store. + + Only paths that resolve **inside** the checkpoint store are returned, so a + surprising or hostile message can never point the cleanup at a file we do + not own. + """ + try: + store_resolved = store.resolve() + except OSError: + return [] + found: List[Path] = [] + for raw in _LOCK_PATH_RE.findall(stderr or ""): + candidate = Path(raw) + if not candidate.is_absolute(): + continue + try: + resolved = candidate.resolve() + except OSError: + continue + if store_resolved not in resolved.parents: + continue + if resolved not in found: + found.append(resolved) + return found + + +def _restore_claimed_lock(claim: Path, lock_path: Path) -> None: + """Put a wrongly-claimed lock back without clobbering a newer one.""" + try: + os.link(claim, lock_path) + except FileExistsError: + pass # the slot was retaken; the newer lock wins + except OSError: + try: + os.rename(claim, lock_path) + return + except OSError: + pass + try: + claim.unlink() + except OSError: + pass + + +def _clear_abandoned_lock( + lock_path: Optional[Path], + reason: str, + *, + require_stale: bool = True, +) -> bool: + """Reclaim a git ``.lock`` left behind by a process that is gone. + + Git creates its lock files with ``O_EXCL`` and renames them into place on + success. A git that is killed — notably by our own subprocess timeout, + which fires readily when the work tree is a WSL2 ``drvfs``/9p mount such + as ``/mnt/c`` — leaves the lock behind. The file records no owner, so git + can never reclaim it on its own: every later call needing that lock fails + instantly with "File exists", for the rest of the session *and every + future session*, until a human deletes it (#74108). + + Two independent guards decide whether a lock may be taken, and the + caller picks which one applies: + + * ``require_stale`` — the lock's mtime is older than + ``_MAX_GIT_CALL_SECONDS``. Every checkpoint git call is bounded by + ``_GIT_TIMEOUT`` times the largest caller multiplier, so nothing older + than that window can belong to a running call. Used by the recovery + path, which knows nothing about who created the lock. + The timeout path passes ``require_stale=False`` because it has already + reaped the owning child, and it decides ownership by *observing the lock's + absence before launching* rather than by comparing an mtime to the wall + clock — filesystem timestamp granularity makes that comparison unreliable + (locally the margin is tens of microseconds). + + Whichever guard applies, the removal itself is done by *claiming* the + lock with an atomic rename and then verifying identity. Between judging a + lock and unlinking it by name, another session can finish its own + recovery and a fresh git can create a new lock at the same pathname; + unlinking by name would delete that live lock. ``rename()`` moves one + specific directory entry in a single syscall, and comparing + ``(st_dev, st_ino, st_mtime_ns)`` afterwards proves we claimed the inode + we judged. A mismatch means we caught a replacement, which is put back. + + Returns True when a lock was removed. + """ + if lock_path is None: + return False + try: + judged = lock_path.stat() + except FileNotFoundError: + return False + except OSError as exc: + logger.debug("Cannot stat checkpoint lock %s: %s", lock_path, exc) + return False + + age = time.time() - judged.st_mtime + if require_stale and age < _MAX_GIT_CALL_SECONDS: + logger.debug( + "Checkpoint lock %s is %.1fs old — younger than the %ds bound on " + "a git call, so another call may still own it; leaving it.", + lock_path, age, _MAX_GIT_CALL_SECONDS, + ) + return False + claim = lock_path.with_name(f"{lock_path.name}.reclaim-{uuid.uuid4().hex[:8]}") + try: + os.rename(lock_path, claim) + except FileNotFoundError: + return False + except OSError as exc: + logger.warning( + "Could not claim abandoned checkpoint lock %s: %s", lock_path, exc, + ) + return False + + try: + claimed = claim.stat() + except OSError: + _restore_claimed_lock(claim, lock_path) + return False + if (claimed.st_dev, claimed.st_ino, claimed.st_mtime_ns) != ( + judged.st_dev, judged.st_ino, judged.st_mtime_ns + ): + logger.debug( + "Checkpoint lock %s was replaced while being reclaimed — putting " + "the live lock back.", lock_path, + ) + _restore_claimed_lock(claim, lock_path) + return False + + try: + claim.unlink() + except OSError as exc: + logger.warning( + "Could not remove claimed checkpoint lock %s: %s", claim, exc, + ) + return False + + logger.warning( + "Removed abandoned checkpoint lock %s (%.1fs old, %s). Without this " + "every later call needing that lock would fail with 'File exists'.", + lock_path, age, reason, + ) + return True + def _git_env( store: Path, @@ -305,12 +508,17 @@ def _run_git( timeout: int = _GIT_TIMEOUT, allowed_returncodes: Optional[Set[int]] = None, index_file: Optional[Path] = None, + _retry_after_lock_recovery: bool = True, ) -> Tuple[bool, str, str]: """Run a git command against the shared store. Returns (ok, stdout, stderr). ``allowed_returncodes`` suppresses error logging for known/expected non-zero exits while preserving the normal ``ok = (returncode == 0)`` contract. Example: ``git diff --cached --quiet`` returns 1 when changes exist. + + ``_retry_after_lock_recovery`` is internal: when a call fails because a + stale ``.lock`` is present, the lock is reclaimed and the call is + retried exactly once (see ``_clear_abandoned_lock``). """ normalized_working_dir = _normalize_path(working_dir) if not normalized_working_dir.exists(): @@ -326,6 +534,13 @@ def _run_git( cmd = ["git"] + list(args) allowed_returncodes = allowed_returncodes or set() + # Ownership signal for the timeout path below: the lock this command would + # take, and whether it was already there before we launched. Observing + # absence is exact; comparing an mtime against the wall clock is not, + # because filesystem timestamp granularity can round a just-created lock + # to before the moment we started. + _timeout_lock = _lock_for_timed_out_command(args, store, index_file) + _timeout_lock_pre_existed = bool(_timeout_lock and _timeout_lock.exists()) try: result = subprocess.run( cmd, @@ -344,6 +559,21 @@ def _run_git( stdout = result.stdout.strip() stderr = result.stderr.strip() if not ok and result.returncode not in allowed_returncodes: + # A leftover index lock from a previously killed git blocks every + # call against this index forever. Reclaim it once (only when it + # is provably stale) and retry, so an install already wedged by an + # earlier timeout heals itself instead of needing manual cleanup. + if _retry_after_lock_recovery and any( + _clear_abandoned_lock(lock, "blocking a new git call") + for lock in _reclaimable_locks_from_stderr(stderr, store) + ): + return _run_git( + args, store, working_dir, + timeout=timeout, + allowed_returncodes=allowed_returncodes, + index_file=index_file, + _retry_after_lock_recovery=False, + ) logger.error( "Git command failed: %s (rc=%d) stderr=%s", " ".join(cmd), result.returncode, stderr, @@ -352,6 +582,17 @@ def _run_git( except subprocess.TimeoutExpired: msg = f"git timed out after {timeout}s: {' '.join(cmd)}" logger.error(msg, exc_info=True) + # subprocess.run has already killed and reaped the child, so a lock + # *this* command created is abandoned by definition. Only the lock + # this specific command takes is eligible, and only if it was absent + # before we launched — a lock that was already there belongs to + # somebody else and must survive. + if not _timeout_lock_pre_existed: + _clear_abandoned_lock( + _timeout_lock, + f"owning git timed out after {timeout}s", + require_stale=False, + ) return False, "", msg except FileNotFoundError as exc: missing_target = getattr(exc, "filename", None)