diff --git a/hermes_cli/source_build.py b/hermes_cli/source_build.py index 8170bc1f0a..427acb1021 100644 --- a/hermes_cli/source_build.py +++ b/hermes_cli/source_build.py @@ -48,10 +48,10 @@ def source_build_env(base_env: dict | None = None, *, explicit: bool = False) -> def run_in_custody(project_root: Path, command: list, label: str, **kwargs): """``pm.progress.run_contained`` for a build command that writes the checkout (node, npm): - it and everything it starts stay in the update's custody (POSIX: the checkout lock fd and - its own process group, killed when it exits; Windows: the owner's kill-on-close job), so the - checkout is never handed to a contender while one of them still writes. Outside an update it - is ``run_contained`` as is.""" + it and everything it starts stay in the update's custody (POSIX: the checkout lock fd, the + caller's process group, and every descendant killed when it exits; Windows: the owner's + kill-on-close job), so the checkout is never handed to a contender while one of them still + writes. Outside an update it is ``run_contained`` as is.""" from pm.progress import run_contained from hermes_cli.update_custody import contained_command diff --git a/hermes_cli/update_custody.py b/hermes_cli/update_custody.py index dff1668f5a..291d4b0dbd 100644 --- a/hermes_cli/update_custody.py +++ b/hermes_cli/update_custody.py @@ -477,30 +477,133 @@ def _join_launcher_python() -> str: return sys.executable -# POSIX twin of the launcher: Node marks the donated lock fd close-on-exec, so nothing node -# starts (npm, esbuild, sh) keeps the checkout locked. The command runs as its own process group -# under this stdlib parent, which holds the fd too and, once the command exits (or this parent is -# interrupted), kills the whole group: no descendant outlives the custodial command (N13). -_OWN_GROUP = ( - "import os, signal, subprocess, sys\n" - "fds = tuple(int(fd) for fd in sys.argv[1].split(','))\n" - "p = subprocess.Popen(sys.argv[2:], pass_fds=fds, start_new_session=True)\n" - "try:\n" - " code = p.wait()\n" - "finally:\n" - " try:\n" - " os.killpg(p.pid, signal.SIGKILL)\n" # windows-footgun: ok — POSIX-only launcher - " except OSError:\n" - " pass\n" - "sys.exit(code if code >= 0 else 128 - code)\n" -) +# POSIX twin of the launcher (N13, review L1/L6). Node marks the donated lock fd close-on-exec, +# so nothing node starts (npm, esbuild, sh) keeps the checkout locked: custody of them rests on +# this stdlib parent, which holds the fd until no descendant of the command is left. +# * The command stays in the CALLER's process group: every group kill that stops a build (a +# Ctrl-C'd completion child's ``killpg``, Desktop's ``kill(-pid)``) reaches this launcher, +# node and everything under it that did not start a session of its own. +# * Linux: the launcher is a child subreaper, so a descendant orphaned by node's exit (or by any +# intermediate's) is re-parented to it, never to init. Once node exits it SIGKILLs and reaps +# its children until none is left; it only signals its own unreaped children, so no pid it +# kills can have been reused (never a ``killpg`` of an already reaped leader's group). +# * Elsewhere (macOS has no subreaper) it records node's descendant tree from ``ps`` while node +# runs and, once node exits, kills every recorded process whose start time still matches, +# plus what they started. Best effort: a process born and orphaned between two samples escapes. +# * SIGINT/SIGTERM/SIGHUP are forwarded to node (a node still running 10 s later is killed); the +# descendants are then killed as above. +_REAP_TREE = r""" +import os, signal, subprocess, sys, time +KILL = signal.SIGKILL # windows-footgun: ok - POSIX-only launcher +fds = tuple(int(fd) for fd in sys.argv[1].split(',')) +me = os.getpid() +reaper = False +if sys.platform.startswith('linux'): + try: + import ctypes + reaper = ctypes.CDLL(None, use_errno=True).prctl(36, 1, 0, 0, 0) == 0 # PR_SET_CHILD_SUBREAPER + except (OSError, AttributeError): + reaper = False + +def children(): + found = [] + for entry in os.listdir('/proc'): + if not entry.isdigit(): + continue + try: + with open('/proc/%s/stat' % entry, 'rb') as fh: + stat = fh.read() + if int(stat[stat.rindex(b')') + 2:].split()[1]) == me: + found.append(int(entry)) + except (OSError, ValueError, IndexError): + continue + return found + +def table(): + try: + out = subprocess.run(['ps', '-A', '-o', 'pid=,ppid=,stat=,lstart='], stdin=subprocess.DEVNULL, + capture_output=True, text=True, errors='replace', timeout=10).stdout + except (OSError, subprocess.SubprocessError): + return {} + rows = {} + for line in out.splitlines(): + parts = line.split(None, 3) + if len(parts) == 4 and parts[0].isdigit() and parts[1].isdigit() and not parts[2].startswith('Z'): + rows[int(parts[0])] = (int(parts[1]), parts[3].strip()) + return rows + +seen = {} + +def sample(root): + rows = table() + tree = {pid for pid, start in seen.items() if rows.get(pid, (0, None))[1] == start} + if root is not None: + tree.add(root) + grew = True + while grew: + grew = False + for pid, (ppid, start) in rows.items(): + if ppid in tree and pid not in tree: + tree.add(pid) + seen[pid] = start + grew = True + return [pid for pid, start in seen.items() if rows.get(pid, (0, None))[1] == start] + +def kill(pids): + for pid in pids: + try: + os.kill(pid, KILL) + except OSError: + pass + +p = subprocess.Popen(sys.argv[2:], pass_fds=fds) +got = [] + +def forward(signum, frame): + got.append(signum) + try: + p.send_signal(signum) + except OSError: + pass + +for name in ('SIGINT', 'SIGTERM', 'SIGHUP'): + signal.signal(getattr(signal, name), forward) +deadline = None +while True: + try: + code = p.wait(timeout=0.5) + break + except subprocess.TimeoutExpired: + pass + if not reaper: + sample(p.pid) + if got: + deadline = deadline or time.monotonic() + 10 + if time.monotonic() > deadline: + p.kill() +end = time.monotonic() + 10 +while time.monotonic() < end: + live = children() if reaper else sample(None) + if not live: + break + kill(live) + while reaper: + try: + if not os.waitpid(-1, os.WNOHANG)[0]: + break + except ChildProcessError: + break + time.sleep(0.02) +sys.exit(code if code >= 0 else 128 - code) +""" @contextlib.contextmanager def contained_command(argv: Sequence[str], *, inherit_lock: bool = True, root=None): """``(argv, kwargs)`` for a checkout writer started by a runner that hides its Popen (the Node build in ``pm.progress.run_contained``). POSIX: the lock fd (this process's, or one it - inherited for checkout ``root``), with the command's process group killed when it exits. + inherited for checkout ``root``), under a launcher that keeps the command in the caller's + process group and kills every descendant left when it exits (:data:`_REAP_TREE`). Windows inside an update: the command runs under a launcher that joins the update's kill-on-close job first; when the job cannot be handed over or the join is refused, the command never runs and :class:`CustodyRefused` is raised (D2).""" @@ -510,7 +613,7 @@ def contained_command(argv: Sequence[str], *, inherit_lock: bool = True, root=No fds = tuple(checkout_lock_fds(root)) if inherit_lock and root is not None else _lock_fds(inherit_lock) if fds: - argv = [sys.executable, "-I", "-S", "-c", _OWN_GROUP, ",".join(map(str, fds)), *argv] + argv = [sys.executable, "-I", "-S", "-c", _REAP_TREE, ",".join(map(str, fds)), *argv] yield argv, ({"pass_fds": fds} if fds else {}) return try: diff --git a/tests/hermes_cli/test_update_custody.py b/tests/hermes_cli/test_update_custody.py index 54d8df06ec..592d716b63 100644 --- a/tests/hermes_cli/test_update_custody.py +++ b/tests/hermes_cli/test_update_custody.py @@ -362,6 +362,88 @@ def test_no_build_descendant_outlives_the_build_call(repo, tmp_path, monkeypatch assert not late.exists(), "a build descendant kept writing after the build call returned" +def _late_writer(late: Path, *, detach: bool = False, delay: float = 2.0) -> str: + """Python source for a build descendant that writes ``late`` after ``delay`` seconds, holding + no inherited fd and no pipe (so nothing but a kill stops it); ``detach`` starts its own session.""" + detach_code = "os.setsid(); " if detach else "" # windows-footgun: ok - the callers are POSIX-only + return (f"import os, pathlib, time; {detach_code}os.closerange(3, 4096); " + f"time.sleep({delay}); pathlib.Path({str(late)!r}).touch()") + + +def _node_starting(writer: str, *, then: str) -> list[str]: + """A stand-in for node: starts ``writer`` (stdio detached), then runs ``then``.""" + return [sys.executable, "-c", + "import subprocess, sys, time; d = subprocess.DEVNULL; " + f"subprocess.Popen([sys.executable, '-c', {writer!r}], stdin=d, stdout=d, stderr=d); {then}"] + + +def _launch_in_custody(repo: Path, tmp_path: Path, command: list[str], *, platform: str | None = None, + **popen) -> subprocess.Popen: + """Start ``command`` the way the build runner does (contained_command under a held checkout + lock); ``platform`` makes the launcher see another ``sys.platform`` (its no-subreaper path).""" + from hermes_cli.update_custody import contained_command + + lock = ul.UpdateLock(path=tmp_path / "marker", install_root=repo) + assert lock.acquire() + try: + with contained_command(command, root=repo) as (argv, custody): + assert argv[3] == "-c", argv + if platform is not None: + argv[4] = f"import sys; sys.platform = {platform!r}\n" + argv[4] + return subprocess.Popen(argv, stdin=subprocess.DEVNULL, **custody, **popen) + finally: + lock.release() + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="POSIX process groups + /proc") +def test_a_group_kill_of_the_caller_stops_the_whole_build(repo, tmp_path): + """L1: the completion child's Ctrl-C path ``killpg``s its own group and Desktop kills the + backend's group; the build (node and what it starts) must be inside that group. A build + moved into a session of its own outlived the kill and kept writing after the lock was free.""" + import signal + + late_node, late_child = tmp_path / "late-node", tmp_path / "late-child" + node = _node_starting(_late_writer(late_child), then=f"time.sleep(2); __import__('pathlib').Path({str(late_node)!r}).touch()") + # The launcher leads a fresh group here, as the completion child does for its build. + launcher = _launch_in_custody(repo, tmp_path, node, start_new_session=True) + time.sleep(1) + os.killpg(launcher.pid, signal.SIGKILL) # windows-footgun: ok - Linux-only test + launcher.wait(timeout=10) + time.sleep(3) + assert not late_node.exists(), "node survived a group kill of its caller" + assert not late_child.exists(), "a build descendant survived a group kill of its caller" + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="POSIX sessions + /proc") +@pytest.mark.parametrize("platform", [None, "darwin"], ids=["subreaper", "ps-walk"]) +def test_a_detached_grandchild_outliving_node_dies_before_the_build_returns(repo, tmp_path, platform): + """N13/L1: a descendant that left node's process group (its own session) and outlives node + is still killed before the build call returns: Linux through the subreaper, elsewhere + through the recorded descendant tree.""" + late = tmp_path / "late" + node = _node_starting(_late_writer(late, detach=True), then="time.sleep(1.2)") + launcher = _launch_in_custody(repo, tmp_path, node, platform=platform) + assert launcher.wait(timeout=30) == 0 + time.sleep(3) + assert not late.exists(), "a detached build descendant kept writing after the build call returned" + + +@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="POSIX signals + /proc") +def test_a_terminated_launcher_takes_node_and_its_descendants_with_it(repo, tmp_path): + """L1: SIGTERM to the launcher (a service manager, the caller's polite stop) is forwarded to + node, and what node started is killed too, before the launcher exits.""" + import signal + + late = tmp_path / "late" + node = _node_starting(_late_writer(late), then="time.sleep(30)") + launcher = _launch_in_custody(repo, tmp_path, node, start_new_session=True) + time.sleep(1) + launcher.send_signal(signal.SIGTERM) + assert launcher.wait(timeout=20) == 128 + signal.SIGTERM + time.sleep(3) + assert not late.exists(), "a build descendant kept writing after its launcher was terminated" + + @pytest.mark.skipif(not Path("/proc/self/fd").is_dir(), reason="needs /proc fd listing") def test_the_desktop_build_runs_in_checkout_custody(repo, tmp_path, monkeypatch): """F04: the desktop app build (npm run build / builder) writes the checkout like the other