diff --git a/CHANGELOG.md b/CHANGELOG.md index 941c61ea..7539fc3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- **PR #2582** by @Michaelyklam (refs #2577) — Harden embedded workspace-terminal shell cleanup so graceful WebUI shutdowns close/reap every active PTY shell and the spawned shell receives a Linux parent-death signal if the WebUI process dies. The terminal close path now waits again after SIGKILL so timed-out shells do not remain unreaped. + ## [v0.51.92] — 2026-05-19 — Release BP (stage-385 — 7-PR full sweep batch — RFC Slice 3c clarification + workspace tree icon alignment + project move cache refresh + auto-compression handoff metadata + Grok OAuth provider catalog + anonymous custom endpoint picker fallback + PWA standalone reload + pull-to-refresh) diff --git a/api/terminal.py b/api/terminal.py index 47d88abb..5ac2c741 100644 --- a/api/terminal.py +++ b/api/terminal.py @@ -9,6 +9,7 @@ in the agent execution layer. from __future__ import annotations import errno +import atexit import codecs import fcntl import os @@ -69,6 +70,20 @@ _TERMINALS: dict[str, TerminalSession] = {} _LOCK = threading.RLock() +def _terminal_shell_preexec_fn() -> None: + """Ask Linux to terminate the PTY shell when the WebUI parent dies.""" + try: + import ctypes + + libc = ctypes.CDLL(None) + libc.prctl(1, signal.SIGTERM) # PR_SET_PDEATHSIG=1, SIGTERM=15 + except Exception: + # Non-Linux platforms or restricted runtimes should still be able to + # open an embedded terminal; they just do not get the Linux pdeathsig + # hardening. + pass + + def _decode_terminal_output(decoder, data: bytes) -> str: """Decode PTY bytes without stripping terminal control sequences.""" return decoder.decode(data) @@ -178,6 +193,7 @@ def start_terminal(session_id: str, workspace: Path, rows: int = 24, cols: int = stdout=slave_fd, stderr=slave_fd, close_fds=True, + preexec_fn=_terminal_shell_preexec_fn, start_new_session=True, ) os.close(slave_fd) @@ -240,9 +256,24 @@ def close_terminal(session_id: str) -> bool: os.killpg(term.proc.pid, signal.SIGKILL) except ProcessLookupError: pass + try: + term.proc.wait(timeout=1.0) + except (subprocess.TimeoutExpired, ProcessLookupError): + pass finally: try: os.close(term.master_fd) except OSError: pass return True + + +def close_all_terminals() -> None: + """Best-effort reap of embedded shells during graceful WebUI shutdown.""" + with _LOCK: + session_ids = list(_TERMINALS) + for session_id in session_ids: + close_terminal(session_id) + + +atexit.register(close_all_terminals) diff --git a/tests/test_terminal_process_cleanup.py b/tests/test_terminal_process_cleanup.py new file mode 100644 index 00000000..51607935 --- /dev/null +++ b/tests/test_terminal_process_cleanup.py @@ -0,0 +1,103 @@ +import subprocess + +import api.terminal as terminal + + +class _DummyThread: + def __init__(self, *args, **kwargs): + self.args = args + self.kwargs = kwargs + self.started = False + + def start(self): + self.started = True + + +class _FakeProc: + pid = 999_999_999 + + def __init__(self): + self.wait_calls = [] + + def poll(self): + return None + + def wait(self, timeout=None): + self.wait_calls.append(timeout) + return 0 + + +def test_terminal_shell_uses_parent_death_signal_preexec(monkeypatch, tmp_path): + captured = {} + proc = _FakeProc() + + def fake_popen(*args, **kwargs): + captured["args"] = args + captured["kwargs"] = kwargs + return proc + + monkeypatch.setattr(terminal.subprocess, "Popen", fake_popen) + monkeypatch.setattr(terminal.threading, "Thread", _DummyThread) + monkeypatch.setattr(terminal, "_set_size", lambda *args, **kwargs: None) + + term = terminal.start_terminal("term-preexec", tmp_path) + + try: + assert term.proc is proc + assert captured["kwargs"]["preexec_fn"] is terminal._terminal_shell_preexec_fn + assert captured["kwargs"]["start_new_session"] is True + assert captured["kwargs"]["stdin"] == captured["kwargs"]["stdout"] == captured["kwargs"]["stderr"] + finally: + terminal.close_terminal("term-preexec") + + +def test_close_terminal_waits_again_after_sigkill(monkeypatch): + class TimeoutThenReapedProc(_FakeProc): + def wait(self, timeout=None): + self.wait_calls.append(timeout) + if len(self.wait_calls) == 1: + raise subprocess.TimeoutExpired(cmd="shell", timeout=timeout) + return -9 + + proc = TimeoutThenReapedProc() + term = terminal.TerminalSession( + session_id="term-timeout", + workspace="/tmp", + proc=proc, + master_fd=12345, + ) + terminal._TERMINALS["term-timeout"] = term + kills = [] + monkeypatch.setattr(terminal.os, "killpg", lambda pid, sig: kills.append((pid, sig))) + monkeypatch.setattr(terminal.os, "close", lambda fd: None) + + assert terminal.close_terminal("term-timeout") is True + + assert proc.wait_calls == [1.5, 1.0] + assert kills == [(proc.pid, terminal.signal.SIGHUP), (proc.pid, terminal.signal.SIGKILL)] + + +def test_close_all_terminals_closes_snapshot(monkeypatch): + terminal._TERMINALS.clear() + terminal._TERMINALS.update({"a": object(), "b": object()}) + closed = [] + + def fake_close(session_id): + closed.append(session_id) + terminal._TERMINALS.pop(session_id, None) + return True + + monkeypatch.setattr(terminal, "close_terminal", fake_close) + + terminal.close_all_terminals() + + assert closed == ["a", "b"] + assert terminal._TERMINALS == {} + + +def test_terminal_module_registers_graceful_shutdown_reaper(): + src = terminal.Path(terminal.__file__).read_text() + + assert "atexit.register(close_all_terminals)" in src + assert "preexec_fn=_terminal_shell_preexec_fn" in src + assert "libc.prctl(1, signal.SIGTERM)" in src