diff --git a/tests/tools/test_bot_desktop_install.py b/tests/tools/test_bot_desktop_install.py index 820f92d16c..afef3220eb 100644 --- a/tests/tools/test_bot_desktop_install.py +++ b/tests/tools/test_bot_desktop_install.py @@ -126,6 +126,9 @@ def test_passwordless_sudo_runs_the_install_without_asking_for_a_password(monkey def wait(self, timeout=None): return 0 + def poll(self): + return 0 + monkeypatch.setattr(install.subprocess, "Popen", _Proc) monkeypatch.setattr(install.os, "killpg", lambda *a: None) lines: list[str] = [] @@ -204,3 +207,41 @@ def test_no_sudo_binary_returns_the_host_command_instead_of_a_password_card(monk on_line=lines.append) assert code == install.NO_SUDO assert any("apt-get install" in line for line in lines), lines + + +def test_timeout_finishes_the_group_when_only_the_leader_dies_on_term(monkeypatch): + """TERM ends the leader shell at once, but a descendant in the same group that ignores TERM used to be + left alive holding the dpkg lock: the kill helper returned as soon as the leader was reaped. Cleanup is + done only when the whole process group is gone.""" + import subprocess + import time + + monkeypatch.setattr(install, "_sudo_nopasswd", lambda: True) + monkeypatch.setattr(install, "_TERM_GRACE_SECONDS", 0.3, raising=False) + real_popen = subprocess.Popen + + def popen(argv, **kw): + if argv[:1] != ["sudo"]: + return real_popen(argv, **kw) + return real_popen(["sh", "-c", "sh -c 'trap \"\" TERM; echo child $$; sleep 30' & wait"], **kw) + + monkeypatch.setattr(install.subprocess, "Popen", popen) + lines: list[str] = [] + install.install_packages(ask_password=lambda: "", on_line=lines.append, timeout_seconds=0.5) + child = next(int(line.split()[1]) for line in lines if line.startswith("child ")) + deadline = time.monotonic() + 2.0 + while time.monotonic() < deadline and _alive(child): + time.sleep(0.05) + try: + assert not _alive(child), "the TERM-ignoring descendant survived the timeout cleanup" + finally: + subprocess.run(["kill", "-9", str(child)], check=False) + + +def _alive(pid: int) -> bool: + import os + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + return True diff --git a/tools/bot_desktop/install.py b/tools/bot_desktop/install.py index 1f66d9ce6b..58765c1c97 100644 --- a/tools/bot_desktop/install.py +++ b/tools/bot_desktop/install.py @@ -129,6 +129,8 @@ def _run(cmd: str, *, ask_password: Callable[[], str], on_line: Callable[[str], return proc.returncode if proc.returncode is not None else -9 finally: proc.stdout.close() # type: ignore[union-attr] + if proc.poll() is None: # the drain raised (a failing on_line sink): the slot is released, so no orphan + _kill_group(proc) _TERM_GRACE_SECONDS = 5.0 @@ -166,9 +168,24 @@ def _kill_group(proc: subprocess.Popen) -> None: apt/dnf running as root with the dpkg lock while the slot is released, so the whole group goes: TERM first so dpkg can finish its transaction, KILL after the grace. Best effort — as non-root neither signal reaches a root-owned child, which is why the caller never waits on EOF.""" + def _group_gone() -> bool: + try: + os.killpg(proc.pid, 0) # windows-footgun: ok — Linux-only (is_supported_host) + except ProcessLookupError: + return True + except PermissionError: + return False # a root-owned child is still there + return False + for sig, grace in ((signal.SIGTERM, _TERM_GRACE_SECONDS), (signal.SIGKILL, 1.0)): # windows-footgun: ok — Linux-only (is_supported_host) with contextlib.suppress(ProcessLookupError, PermissionError): os.killpg(proc.pid, sig) # windows-footgun: ok — Linux-only (is_supported_host) with contextlib.suppress(subprocess.TimeoutExpired): proc.wait(timeout=grace) + # The leader (sudo / the package manager) going away is not the end: wait for the whole group so a + # TERM-ignoring descendant gets the KILL round instead of surviving with the dpkg lock. + deadline = time.monotonic() + grace + while time.monotonic() < deadline and not _group_gone(): + time.sleep(0.05) + if _group_gone(): return diff --git a/tools/bot_desktop/runtime.py b/tools/bot_desktop/runtime.py index 1360eb7205..fadeef6479 100644 --- a/tools/bot_desktop/runtime.py +++ b/tools/bot_desktop/runtime.py @@ -231,10 +231,23 @@ def _kill_group_then_wait(pgid: Optional[int], pid: int, grace: float = 2.0) -> os.killpg(pgid, sig) # windows-footgun: ok — Linux-only runtime (is_supported_host gates start/stop) else: os.kill(pid, sig) + def _anything_left() -> bool: + # The leader dying first is the common case (bash exits on TERM, Xvnc traps it); the group is + # done only when killpg(0) finds nobody, else a TERM-ignoring descendant keeps the display. + if pgid is None: + return _pid_alive(pid) + try: + os.killpg(pgid, 0) # windows-footgun: ok — Linux-only runtime (is_supported_host gates start/stop) + except ProcessLookupError: + return False + except PermissionError: + return True + return True + _signal(signal.SIGTERM) deadline = time.monotonic() + grace while time.monotonic() < deadline: - if not _pid_alive(pid): + if not _anything_left(): return time.sleep(0.05) _signal(signal.SIGKILL) # windows-footgun: ok — Linux-only runtime (is_supported_host gates start/stop)