diff --git a/hermes_cli/update_lock.py b/hermes_cli/update_lock.py index fbbf263947..a5218cb5d4 100644 --- a/hermes_cli/update_lock.py +++ b/hermes_cli/update_lock.py @@ -983,9 +983,15 @@ def _inherited_lock_fd(path: Path) -> int | None: def _lock_holder(fd_or_path) -> UpdateHolder: + """Who holds the checkout lock, from its holder record. A recorded updater that is gone (its + incarnation is dead) while the lock stays held means a process it started holds it: that is + the ``held`` holder, never the dead pid (the marker reader's wording, R6).""" raw = _read_bytes(fd_or_path) or b"" parsed = _parse_marker(raw) - return UpdateHolder(pid=max(parsed.pid, 0), age_seconds=parsed.age() if parsed.started_at else 0.0) + age = parsed.age() if parsed.started_at else 0.0 + if parsed.pid > 0 and incarnation_live(parsed.pid, parsed.create_time) is False: + return UpdateHolder(pid=0, age_seconds=age, held=True) + return UpdateHolder(pid=max(parsed.pid, 0), age_seconds=age) def _open_lock_file(path: Path) -> tuple[int | None, object]: @@ -1260,18 +1266,28 @@ class UpdateLock: pid, ancestor, or an outer claim of this same process) succeeds with ``acquired`` False. ``acquire`` returns False (and sets ``holder``) when another live update owns either lock or the lock cannot be created at all — never "proceed unlocked". + + ``install_root`` names the checkout whose kernel lock guards the claim (default: this + checkout). With ``checkout_first`` (the default, when ``install_root`` is given) ``acquire`` + takes that lock before the marker. ``checkout_first=False`` claims the marker alone — a + launch that may run under a live update's claim (venv_sync) and takes the checkout lock later, + only for its own mutation. Either way a dead marker over a checkout lock held by another + process tree is refused as ``held`` and kept, never reclaimed (R6). """ - def __init__(self, *, path: Path | None = None, install_root: Path | str | None = None) -> None: + def __init__(self, *, path: Path | None = None, install_root: Path | str | None = None, + checkout_first: bool = True) -> None: self.path = path or update_marker_path() self.install_root = None if install_root is None else Path(install_root) + self.checkout_first = checkout_first self.acquired = False self.holder: UpdateHolder | None = None self._claimed = False self._checkout = False def acquire(self) -> bool: - if self.install_root is not None and not self.acquire_checkout(self.install_root): + if self.install_root is not None and self.checkout_first \ + and not self.acquire_checkout(self.install_root): return False try: ok = self._claim_marker() @@ -1324,6 +1340,14 @@ class UpdateLock: continue # vanished between publish and read: retry if _live_partners(existing): return self._adopt_or_refuse(existing) + if self._checkout_held_elsewhere(): + # R6: the marker's owner is dead but a process it started (completion, + # build, git) still holds the checkout. The update is not over: keep + # its marker and refuse as ``held``. + self.holder = UpdateHolder( + pid=0, held=True, + age_seconds=max(existing.age(), 0.0) if existing.started_at is not None else 0.0) + return False # Dead or malformed, judged under the mutex: reclaim and claim in one hold. with suppress(FileNotFoundError): self.path.unlink() @@ -1336,9 +1360,21 @@ class UpdateLock: except OSError as exc: self.holder = UpdateHolder(pid=0, age_seconds=0.0, reason=f"{self.path} is not writable ({exc})") return False - self.holder = read_live_update(path=self.path) or UpdateHolder(pid=0, age_seconds=0.0) + self.holder = read_live_update(path=self.path, install_root=self.install_root) \ + or UpdateHolder(pid=0, age_seconds=0.0) return False + def _checkout_held_elsewhere(self) -> bool: + """The checkout lock is held, and not by this process's own update tree (our hold, or + a lock fd inherited from the update that spawned us — that tree may reclaim).""" + if self._checkout: + return False + path = checkout_lock_path(self.install_root) + if _HELD is not None and _HELD["path"] == str(path): + return False + # Only when held: a free lock is not taken by the inherited-fd lookup below. + return checkout_lock_held(self.install_root) and _inherited_lock_fd(path) is None + @staticmethod def _is_partner(pid: int) -> bool: return pid == os.getpid() or (pid and (pid == _handoff_pid() or _is_ancestor_pid(pid))) diff --git a/hermes_cli/venv_sync.py b/hermes_cli/venv_sync.py index 00a5ecceea..4aac2f5684 100644 --- a/hermes_cli/venv_sync.py +++ b/hermes_cli/venv_sync.py @@ -401,13 +401,16 @@ def prepare_launch(project_root: Path, argv: list[str]) -> Path | None: # noise (the record's age tracks the wait), leaving the marker armed. pass elif not owed_to_cli and (not current or pending.is_file()): - lock = UpdateLock() + # The marker alone first (a process the live update spawned runs under its claim and + # inherits no checkout lock); install_root still names the checkout whose held lock + # keeps a dead update's marker (R6: refused as held, never reclaimed). + lock = UpdateLock(install_root=root, checkout_first=False) if not lock.acquire(): raise RuntimeError("an update is still running; wait for it to exit, then relaunch Hermes") try: # Under the launching update's own claim (its pid is our ancestor) a process it # spawned owes no tail: that obligation is the updater's. - if not lock.acquired and read_live_update() is not None: + if not lock.acquired and read_live_update(install_root=root) is not None: if current: return None # A process the update spawns before its dependencies are current (a restarted @@ -464,7 +467,7 @@ def _prepare_borrowed_launch(root: Path, owner: Path, *, current: bool) -> Path from hermes_cli.update_lock import UpdateLock if not current: - lock = UpdateLock() + lock = UpdateLock(install_root=root, checkout_first=False) # R6, as in prepare_launch if not lock.acquire(): raise RuntimeError("an update is still running; wait for it to exit, then relaunch Hermes") try: diff --git a/tests/hermes_cli/test_update_lock_protocol.py b/tests/hermes_cli/test_update_lock_protocol.py index 104ab591ff..ea960edbcb 100644 --- a/tests/hermes_cli/test_update_lock_protocol.py +++ b/tests/hermes_cli/test_update_lock_protocol.py @@ -400,6 +400,83 @@ def test_dead_marker_is_kept_and_reported_held_while_the_checkout_lock_is_held(t assert not marker.exists(), "with the tree gone the dead marker is reclaimed" +def _dead_marker(marker: Path) -> None: + dead = subprocess.Popen([sys.executable, "-c", "pass"]) + dead.wait() + marker.write_text(f"{dead.pid}\n{int(time.time())}\nct:1.5\n", encoding="utf-8") + + +def test_a_claimer_never_reclaims_a_dead_marker_while_the_checkout_lock_is_held(tmp_path): + """M1 (round 7): the launch path claims the marker before it takes the checkout lock + (venv_sync). Its claim step deleted the dead marker over a held lock, then dropped its own + claim when the checkout refused: the marker was gone while the killed update's tree ran.""" + from hermes_cli import update_lock + + install = tmp_path / "install" + install.mkdir() + marker = tmp_path / ".hermes-update-in-progress" + _dead_marker(marker) + dead_bytes = marker.read_bytes() + holder = _python(tmp_path, _LOCK_HOLDER, install, tmp_path / "ready") + try: + _wait_for(tmp_path / "ready", holder) + claim = UpdateLock(path=marker, install_root=install) + assert claim._claim_marker() is False, "claimed over a dead marker while the checkout lock is held" + assert marker.read_bytes() == dead_bytes, "the claim step replaced the held update's marker" + assert claim.holder is not None and claim.holder.held + launch = UpdateLock(path=marker, install_root=install, checkout_first=False) # venv_sync + assert launch.acquire() is False + launch.release() + assert launch.holder is not None and launch.holder.held and launch.holder.pid == 0 + assert marker.read_bytes() == dead_bytes + finally: + holder.kill() + holder.wait() + with UpdateLock(path=marker, install_root=install, checkout_first=False) as launch: + assert launch.acquired, "with the tree gone the dead marker is reclaimed and claimed" + assert not marker.exists() + assert update_lock.checkout_lock_held(install) is False + + +_ORPHANING_HOLDER = """ +import subprocess, sys +from pathlib import Path +sys.path.insert(0, sys.argv[1]) +from hermes_cli import update_lock +root = Path(sys.argv[2]) +assert update_lock._acquire_checkout(root) is None # the record names this (soon dead) updater +release = Path(sys.argv[3]).with_name("release") +wait = "import sys, time, pathlib\\nwhile not pathlib.Path(sys.argv[1]).exists(): time.sleep(0.05)" +child = subprocess.Popen([sys.executable, "-c", wait, str(release)], + pass_fds=update_lock.checkout_lock_fds(root)) +Path(sys.argv[3]).write_text(str(child.pid), encoding="utf-8") +""" + + +@posix_only +def test_a_refusal_over_a_dead_updaters_lock_uses_the_held_wording(tmp_path): + """m6: `hermes update`, launch repair and source completion refuse through the lock's + holder record. Its updater is dead and a child it started holds the lock: say so, never + name the dead process.""" + from hermes_cli import update_lock + + install = tmp_path / "install" + install.mkdir() + updater = _python(tmp_path, _ORPHANING_HOLDER, install, tmp_path / "child") + updater.wait(timeout=30) + dead_pid = updater.pid + _wait_for(tmp_path / "child") + try: + lock = UpdateLock(path=tmp_path / ".hermes-update-in-progress", install_root=install) + assert lock.acquire() is False + assert lock.holder is not None and lock.holder.held and lock.holder.pid == 0 + text = update_lock.describe_holder(lock.holder) + assert f"process {dead_pid}" not in text + assert "still holds the checkout" in text + finally: + (tmp_path / "release").touch() # the orphaned child is not ours to signal + + # --- D17: a "held?" probe never makes a concurrent update fail -------------------------------- _PROBER = """