From 2429805a450dd22d4e88fed6a224be866973889c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o?= Date: Thu, 1 Oct 2026 18:46:19 +0200 Subject: [PATCH] fix(shell): only treat ESRCH as proof a PTY session is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _session_alive collapsed every OSError from killpg(pgid, 0) into "the group is gone". EPERM means the opposite — the group answered the probe but holds a process we may not signal — so a session we could not touch was reported as contained, and a timed-out command that left children running said it had terminated cleanly. Resolving PTY_KILL_ESCALATION also named signal.SIGKILL unconditionally, which does not exist on native Windows. app.py imports this module at start-up, so that turned a POSIX-only teardown detail into the whole app failing to import there. --- routes/shell_routes.py | 20 ++++++++++-- tests/test_shell_routes.py | 66 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/routes/shell_routes.py b/routes/shell_routes.py index 900ac4125..73e055c18 100644 --- a/routes/shell_routes.py +++ b/routes/shell_routes.py @@ -564,7 +564,14 @@ PTY_UNSUPPORTED_ERROR = "pty_unsupported" # PTY teardown. The PTY child leads its own session (os.setsid), so killing it # has to signal the whole process group and then confirm the group is gone — # see _terminate_pty_session. -PTY_KILL_ESCALATION = (signal.SIGTERM, signal.SIGKILL) +# ``signal.SIGKILL`` does not exist on native Windows, and this module is +# imported unconditionally by app.py, so resolve the escalation defensively +# rather than at the cost of the whole app failing to start there. +PTY_KILL_ESCALATION = tuple( + sig + for sig in (getattr(signal, "SIGTERM", None), getattr(signal, "SIGKILL", None)) + if sig is not None +) PTY_KILL_GRACE = 1.0 # seconds a signalled session gets to exit PTY_KILL_POLL_INTERVAL = 0.05 # re-check interval while waiting for it PTY_KILL_FAILED_HINT = "; processes it started survived the kill and are still running" @@ -722,9 +729,16 @@ def _session_alive(pgid: int | None, pid: int) -> bool: if pgid is not None and killpg is not None: try: killpg(pgid, 0) - return True + except ProcessLookupError: + return False # ESRCH — no member of the group is left except OSError: - return False + # Anything else (EPERM when the group holds a process we may not + # signal, EINVAL) answers the probe without proving the group is + # gone. Only ESRCH does that, so treat the rest as still running: + # reporting a surviving session as contained is the one outcome + # teardown must never produce. + return True + return True return pid_alive(pid) diff --git a/tests/test_shell_routes.py b/tests/test_shell_routes.py index 281cc8ea6..69e7b6ab1 100644 --- a/tests/test_shell_routes.py +++ b/tests/test_shell_routes.py @@ -2,6 +2,7 @@ import asyncio import builtins +import errno import importlib import importlib.util import json @@ -64,6 +65,29 @@ def test_shell_routes_import_without_posix_pty_modules(monkeypatch): assert module._find_line_break(b"ok\n") == (2, 1) +def test_shell_routes_import_without_sigkill(monkeypatch): + """Native Windows has no signal.SIGKILL; app.py imports this module anyway. + + The teardown escalation is resolved at import time, so naming SIGKILL + unconditionally would stop the whole app from starting on Windows rather + than only degrading PTY teardown there. + """ + monkeypatch.delattr(signal, "SIGKILL", raising=False) + + module_path = Path(__file__).resolve().parents[1] / "routes" / "shell_routes.py" + spec = importlib.util.spec_from_file_location( + "_shell_routes_without_sigkill", module_path + ) + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + try: + spec.loader.exec_module(module) + finally: + sys.modules.pop(spec.name, None) + + assert module.PTY_KILL_ESCALATION == (signal.SIGTERM,) + + async def test_generate_pty_reports_explicit_unsupported_error(monkeypatch): """Clients can distinguish unsupported PTY mode from process failures.""" import routes.shell_routes as shell_routes @@ -314,6 +338,48 @@ async def test_generate_pty_timeout_says_so_when_the_session_survives( ) +def test_session_alive_treats_a_refused_probe_as_alive(monkeypatch): + """EPERM says the group exists but we may not signal it, not that it died. + + Only ESRCH proves a process group is gone. Collapsing every OSError into + "gone" is the one error that makes teardown report a surviving session as + contained. + """ + import routes.shell_routes as shell_routes + + def refuse(_pgid, _sig): + raise PermissionError(errno.EPERM, "Operation not permitted") + + monkeypatch.setattr(shell_routes.os, "killpg", refuse) + assert shell_routes._session_alive(4242, 4242) is True + + def gone(_pgid, _sig): + raise ProcessLookupError(errno.ESRCH, "No such process") + + monkeypatch.setattr(shell_routes.os, "killpg", gone) + assert shell_routes._session_alive(4242, 4242) is False + + +async def test_terminate_pty_session_reports_a_group_it_may_not_signal(monkeypatch): + """A session we cannot signal at all is reported as not contained. + + Both the signal and the liveness probe are refused, so teardown has done + nothing and must say so rather than infer death from its own failure. + """ + import routes.shell_routes as shell_routes + + def refuse(*_args): + raise PermissionError(errno.EPERM, "Operation not permitted") + + monkeypatch.setattr(shell_routes, "PTY_KILL_GRACE", 0.01) + monkeypatch.setattr(shell_routes, "_session_pgid", lambda _: 4242) + monkeypatch.setattr(shell_routes.os, "killpg", refuse) + monkeypatch.setattr(shell_routes.os, "kill", refuse) + + proc = SimpleNamespace(pid=4242, returncode=0, wait=None) + assert await shell_routes._terminate_pty_session(proc) is False + + class TestFindLineBreak: """Test line-break detection in byte buffers."""