fix(shell): only treat ESRCH as proof a PTY session is gone

_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.
This commit is contained in:
Léo
2026-10-01 18:46:19 +02:00
parent f49e09e59a
commit 2429805a45
2 changed files with 83 additions and 3 deletions
+17 -3
View File
@@ -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)
+66
View File
@@ -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."""