fix(update): a launch claimer never reclaims a dead marker while the checkout lock is held (R6/M1)
venv_sync.prepare_launch and _prepare_borrowed_launch claimed the marker with UpdateLock() (no install_root), and UpdateLock._claim_marker unlinked a dead marker without asking whether the checkout lock was held. The claim then failed acquire_checkout and released its own claim: the killed update's marker was gone while its completion child still ran, and the Desktop gate reopened over it. - _claim_marker refuses as held (pid 0) and keeps the marker when the checkout lock is held by another process tree (our own hold or an inherited lock fd may still reclaim). - UpdateLock(checkout_first=False): install_root names the guarding checkout without taking its lock first; both venv_sync sites pass install_root=root. - _lock_holder reports a dead recorded updater over a held lock as held, so hermes update / launch repair / source completion refusals use the held wording instead of naming the dead pid (m6).
This commit is contained in:
@@ -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)))
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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 = """
|
||||
|
||||
Reference in New Issue
Block a user