diff --git a/plugins/_code_execution/AGENTS.md b/plugins/_code_execution/AGENTS.md index 2de8457f1..1f79ed4ee 100644 --- a/plugins/_code_execution/AGENTS.md +++ b/plugins/_code_execution/AGENTS.md @@ -14,6 +14,7 @@ ## Local Contracts - Keep session concurrency, timeout, streaming, and reset behavior predictable. +- Terminal reset/close must not hang on foreground commands or shells that ignore SIGTERM. - Explicitly target local versus SSH execution runtimes. - Do not hardcode secrets, SSH credentials, or local user paths. diff --git a/plugins/_code_execution/helpers/tty_session.py b/plugins/_code_execution/helpers/tty_session.py index 986e63ecd..5d3e3d018 100644 --- a/plugins/_code_execution/helpers/tty_session.py +++ b/plugins/_code_execution/helpers/tty_session.py @@ -1,10 +1,12 @@ -import asyncio, os, sys, platform, errno +import asyncio, os, sys, platform, errno, signal _IS_WIN = platform.system() == "Windows" if _IS_WIN: import winpty # pip install pywinpty # type: ignore import msvcrt +_CLOSE_TIMEOUT_SECONDS = 2 + def _reconfigure_stream_errors(stream) -> None: reconfigure = getattr(stream, "reconfigure", None) @@ -77,15 +79,16 @@ class TTYSession: # Terminate the process if it exists if self._proc: + if getattr(self._proc, "returncode", None) is None: + self._signal_process(signal.SIGTERM) try: - if getattr(self._proc, "returncode", None) is None: - self._proc.terminate() - except ProcessLookupError: - pass - except Exception: - pass - try: - await self._proc.wait() + await asyncio.wait_for(self._proc.wait(), _CLOSE_TIMEOUT_SECONDS) + except asyncio.TimeoutError: + self._signal_process(signal.SIGKILL) + try: + await asyncio.wait_for(self._proc.wait(), _CLOSE_TIMEOUT_SECONDS) + except Exception: + pass except Exception: pass @@ -93,6 +96,28 @@ class TTYSession: self._proc = None self._pump_task = None + def _signal_process(self, sig): + if self._proc is None: + return + try: + if _IS_WIN: + if sig == signal.SIGKILL: + self._proc.kill() + else: + self._proc.terminate() + return + os.killpg(self._proc.pid, sig) + except ProcessLookupError: + pass + except Exception: + try: + if sig == signal.SIGKILL: + self._proc.kill() + else: + self._proc.terminate() + except Exception: + pass + def _release_pty_master(self): """Release the POSIX PTY master exactly once. @@ -166,11 +191,7 @@ class TTYSession: # Only attempt to kill if the process is still running if getattr(self._proc, "returncode", None) is None: - try: - self._proc.kill() - except ProcessLookupError: - # Child already gone – treat as successfully killed - pass + self._signal_process(signal.SIGKILL) self._release_pty_master() async def read(self, timeout=None): @@ -241,6 +262,7 @@ async def _spawn_posix_pty(cmd, cwd, env, echo): cwd=cwd, env=env, close_fds=True, + start_new_session=True, ) os.close(slave) diff --git a/tests/test_code_execution_pager.py b/tests/test_code_execution_pager.py index 3e80d457d..a4f2d2325 100644 --- a/tests/test_code_execution_pager.py +++ b/tests/test_code_execution_pager.py @@ -1,10 +1,13 @@ -"""Regression tests for issue #1697. +"""Regression tests for code execution shell lifecycle behavior. Pagers (more/less) must be disabled in the non-interactive shells created by the code execution tool: without user input they block forever and spin at 100% CPU. """ +import asyncio + from plugins._code_execution.helpers import shell_local, shell_ssh +from plugins._code_execution.helpers.tty_session import TTYSession def test_local_env_disables_pagers_and_preserves_existing(): @@ -30,3 +33,13 @@ def test_local_env_does_not_mutate_input(): def test_ssh_command_disables_pagers(): assert "GIT_PAGER=cat" in shell_ssh.PAGER_DISABLE_COMMAND assert "PAGER=cat" in shell_ssh.PAGER_DISABLE_COMMAND + + +def test_tty_close_kills_term_resistant_process(): + async def run(): + session = TTYSession("bash -lc 'trap \"\" TERM; sleep 30'") + await session.start() + await asyncio.wait_for(session.close(), timeout=6) + assert session._proc is None + + asyncio.run(run())