fix(bot-screen): process-group cleanup waits for the whole group, not the leader
_kill_group_then_wait (runtime) and _kill_group (install) returned once the leader had exited, so a TERM-ignoring descendant in the same group (a stuck dpkg, a display client) survived the KILL round with the display or the dpkg lock. Both now poll killpg(pgid, 0) until nobody is left. The installer also kills the group when the drain raises (a failing output sink), since the slot is released either way.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user