diff --git a/apodex/sandbox.py b/apodex/sandbox.py index 4b0a933..eb6e402 100644 --- a/apodex/sandbox.py +++ b/apodex/sandbox.py @@ -41,9 +41,11 @@ from __future__ import annotations import asyncio +import contextlib import logging import os import shlex +import signal import sys from dataclasses import dataclass from pathlib import Path @@ -258,8 +260,20 @@ async def run_shell( cwd=cwd, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + start_new_session=True, ) - out, err = await asyncio.wait_for(proc.communicate(), timeout=timeout) + try: + out, err = await asyncio.wait_for(proc.communicate(), timeout=timeout) + finally: + if proc.returncode is None: + # Timed out or cancelled: kill the whole session, not just the + # shell, or its children keep running and holding the pipes. Same + # contract as ``_CurrentCommands.run`` in plugins.tools._sandbox, + # including the bounded wait for a setsid escapee killpg misses. + with contextlib.suppress(OSError): + os.killpg(proc.pid, signal.SIGKILL) + with contextlib.suppress(TimeoutError): + await asyncio.wait_for(proc.wait(), timeout=5) return ( proc.returncode or 0, out.decode("utf-8", "replace"), diff --git a/apodex/tests/test_native.py b/apodex/tests/test_native.py index 20c1a88..51ae6e0 100644 --- a/apodex/tests/test_native.py +++ b/apodex/tests/test_native.py @@ -2,8 +2,11 @@ import asyncio import os +import time from pathlib import Path +import pytest + from apodex import cli, docker, sandbox from apodex.native import prepare_native_runtime from apodex.sandbox import BWRAP, CONTAINER, NATIVE, Strategy, resolve_strategy @@ -199,6 +202,22 @@ def kill(self): assert second.binds == ((str(second_workspace.resolve()),) * 2 + (False,),) +def test_run_shell_kills_the_whole_command_on_timeout(tmp_path) -> None: + """A timed-out command must not keep writing to the workspace. + + The subshell is a grandchild holding the output pipes, so killing only the + shell would still leave it alive to write the marker. + """ + with pytest.raises(TimeoutError): + asyncio.run(sandbox.run_shell( + "(sleep 2; touch marker) & wait", str(tmp_path), 1, + Strategy(NATIVE, "test"), + )) + time.sleep(2) + + assert not (tmp_path / "marker").exists() + + def test_macos_falls_back_to_native_when_docker_is_unavailable( tmp_path, monkeypatch, capsys, ) -> None: