diff --git a/libs/langchain_v1/langchain/agents/middleware/_execution.py b/libs/langchain_v1/langchain/agents/middleware/_execution.py index aa55b30185f..2e90a3332a7 100644 --- a/libs/langchain_v1/langchain/agents/middleware/_execution.py +++ b/libs/langchain_v1/langchain/agents/middleware/_execution.py @@ -220,7 +220,7 @@ class CodexSandboxExecutionPolicy(BaseExecutionPolicy): env=env, cwd=workspace, preexec_fn=None, - start_new_session=False, + start_new_session=True, ) def _build_command(self, command: Sequence[str]) -> list[str]: @@ -325,7 +325,7 @@ class DockerExecutionPolicy(BaseExecutionPolicy): env=host_env, cwd=workspace, preexec_fn=None, - start_new_session=False, + start_new_session=True, ) def _build_command( diff --git a/libs/langchain_v1/langchain/agents/middleware/shell_tool.py b/libs/langchain_v1/langchain/agents/middleware/shell_tool.py index 1b5029538f5..3003d419337 100644 --- a/libs/langchain_v1/langchain/agents/middleware/shell_tool.py +++ b/libs/langchain_v1/langchain/agents/middleware/shell_tool.py @@ -403,11 +403,40 @@ class ShellSession: return if hasattr(os, "killpg"): - with contextlib.suppress(ProcessLookupError): - os.killpg(os.getpgid(self._process.pid), signal.SIGKILL) - else: # pragma: no cover - with contextlib.suppress(ProcessLookupError): - self._process.kill() + try: + child_pgid = os.getpgid(self._process.pid) + # Only send a group kill when the child has a dedicated process group. + # If the child shares our group, killpg would terminate the caller too, + # so fall through to the direct kill below. That direct kill reaps only + # the immediate child, so any descendants it spawned may be orphaned. + # This applies to HostExecutionPolicy(create_process_group=False), the + # only policy that runs the shell in the caller's process group. + if child_pgid != os.getpgrp(): + os.killpg(child_pgid, signal.SIGKILL) + return + except ProcessLookupError: + # Process already gone; nothing left to kill. + return + except OSError: + # e.g. EPERM while querying or signaling the group. Don't leak the + # child silently; fall through to a direct kill. + LOGGER.warning( + "Group kill failed; falling back to direct kill.", + exc_info=True, + ) + + try: + self._process.kill() + except ProcessLookupError: + # Process exited between the check above and this kill; nothing to do. + pass + except OSError: + # The fallback kill can hit the same condition (e.g. EPERM) that routed us + # here. Log rather than let it escape the session shutdown path. + LOGGER.warning( + "Direct kill failed.", + exc_info=True, + ) def _enqueue_stream(self, stream: Any, label: str) -> None: for line in iter(stream.readline, ""): diff --git a/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_execution_policies.py b/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_execution_policies.py index 0e2fe001b64..81ea3cc4da3 100644 --- a/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_execution_policies.py +++ b/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_execution_policies.py @@ -204,7 +204,7 @@ def test_codex_policy_spawns_codex_cli(monkeypatch: pytest.MonkeyPatch, tmp_path assert cwd == tmp_path assert env["TEST_VAR"] == "1" assert preexec_fn is None - assert not start_new_session + assert start_new_session is True return Mock() monkeypatch.setattr( @@ -298,7 +298,7 @@ def test_docker_policy_spawns_docker_run(monkeypatch: pytest.MonkeyPatch, tmp_pa recorded["command"] = list(command) assert cwd == tmp_path assert "PATH" in env # host environment should retain system PATH - assert not start_new_session + assert start_new_session is True return Mock() monkeypatch.setattr( @@ -407,3 +407,39 @@ def test_docker_policy_resolve_missing_binary(monkeypatch: pytest.MonkeyPatch) - policy = DockerExecutionPolicy() with pytest.raises(RuntimeError): policy._resolve_binary() + + +def test_all_policies_use_dedicated_process_group( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Policies without a process-group override must spawn a new session. + + They must use start_new_session=True so that ShellSession._kill_process can safely + killpg the entire subprocess tree on timeout without risking the caller's + process group. HostExecutionPolicy(create_process_group=False) is the only + case where a shared group is intentional, and _kill_process guards against + that via its getpgid/getpgrp check. + """ + monkeypatch.setattr(shutil, "which", lambda _: "/usr/bin/fake") + + for policy in [ + HostExecutionPolicy(), + CodexSandboxExecutionPolicy(platform="linux"), + DockerExecutionPolicy(), + ]: + captured: dict[str, Any] = {} + + def fake_launch( + *_args: Any, + start_new_session: bool, + _captured: dict[str, Any] = captured, + **_kwargs: Any, + ) -> subprocess.Popen[str]: + _captured["start_new_session"] = start_new_session + return Mock(spec=subprocess.Popen, pid=9999) + + monkeypatch.setattr(_execution, "_launch_subprocess", fake_launch) + policy.spawn(workspace=tmp_path, env={"PATH": "/bin"}, command=("/bin/sh",)) + assert captured["start_new_session"] is True, ( + f"{type(policy).__name__} must use start_new_session=True" + ) diff --git a/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_tool.py b/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_tool.py index b3b2a22125c..e1cb50fbb46 100644 --- a/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_tool.py +++ b/libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_tool.py @@ -1,10 +1,14 @@ from __future__ import annotations import gc +import logging +import os +import signal import tempfile import time from pathlib import Path from typing import Any, cast +from unittest.mock import Mock import pytest from langchain_core.messages import ToolMessage @@ -14,6 +18,7 @@ from langgraph.runtime import Runtime from langchain.agents.middleware.shell_tool import ( HostExecutionPolicy, RedactionRule, + ShellSession, ShellToolMiddleware, ShellToolState, _SessionResources, @@ -546,3 +551,146 @@ def test_get_or_create_resources_reuses_existing(tmp_path: Path) -> None: # Clean up resources1.finalizer() + + +def test_kill_process_avoids_group_kill_for_shared_process_group( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Avoid `killpg` when child shares the caller process group.""" + session = ShellSession(tmp_path, HostExecutionPolicy(), ("/bin/bash",), {}) + process = Mock() + process.pid = 1234 + session._process = process + + killpg_mock = Mock() + + def fake_getpgid(pid: int) -> int: + assert pid == process.pid, "getpgid must be queried with the child's pid" + return 1000 + + monkeypatch.setattr(os, "killpg", killpg_mock, raising=False) + monkeypatch.setattr(os, "getpgid", fake_getpgid, raising=False) + monkeypatch.setattr(os, "getpgrp", lambda: 1000, raising=False) + + session._kill_process() + + killpg_mock.assert_not_called() + process.kill.assert_called_once_with() + + +def test_kill_process_uses_group_kill_for_dedicated_process_group( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Keep process-group kill behavior when child runs in a separate group.""" + session = ShellSession(tmp_path, HostExecutionPolicy(), ("/bin/bash",), {}) + process = Mock() + process.pid = 5678 + session._process = process + + killpg_mock = Mock() + + def fake_getpgid(pid: int) -> int: + assert pid == process.pid, "getpgid must be queried with the child's pid" + return 2000 + + monkeypatch.setattr(os, "killpg", killpg_mock, raising=False) + monkeypatch.setattr(os, "getpgid", fake_getpgid, raising=False) + monkeypatch.setattr(os, "getpgrp", lambda: 1000, raising=False) + + session._kill_process() + + killpg_mock.assert_called_once_with(2000, signal.SIGKILL) + process.kill.assert_not_called() + + +def test_kill_process_returns_early_when_process_already_gone( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Return without a direct kill when the process is already gone.""" + session = ShellSession(tmp_path, HostExecutionPolicy(), ("/bin/bash",), {}) + process = Mock() + process.pid = 1111 + session._process = process + + killpg_mock = Mock() + monkeypatch.setattr(os, "killpg", killpg_mock, raising=False) + monkeypatch.setattr(os, "getpgid", Mock(side_effect=ProcessLookupError), raising=False) + monkeypatch.setattr(os, "getpgrp", lambda: 1000, raising=False) + + session._kill_process() + + killpg_mock.assert_not_called() + process.kill.assert_not_called() + + +def test_kill_process_falls_back_when_group_kill_raises_oserror( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Fall back to a direct kill (and log) when the group kill raises an OSError.""" + session = ShellSession(tmp_path, HostExecutionPolicy(), ("/bin/bash",), {}) + process = Mock() + process.pid = 4321 + session._process = process + + killpg_mock = Mock(side_effect=PermissionError("operation not permitted")) + + def fake_getpgid(pid: int) -> int: + assert pid == process.pid, "getpgid must be queried with the child's pid" + return 2000 + + monkeypatch.setattr(os, "killpg", killpg_mock, raising=False) + monkeypatch.setattr(os, "getpgid", fake_getpgid, raising=False) + monkeypatch.setattr(os, "getpgrp", lambda: 1000, raising=False) + + with caplog.at_level(logging.WARNING): + session._kill_process() + + killpg_mock.assert_called_once_with(2000, signal.SIGKILL) + process.kill.assert_called_once_with() + assert "falling back to direct kill" in caplog.text.lower() + + +def test_kill_process_swallows_and_logs_direct_kill_oserror( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Log, don't raise, when the fallback direct kill itself fails with an OSError.""" + session = ShellSession(tmp_path, HostExecutionPolicy(), ("/bin/bash",), {}) + process = Mock() + process.pid = 7777 + process.kill.side_effect = PermissionError("operation not permitted") + session._process = process + + # Shared process group -> skip killpg and go straight to the direct kill. + monkeypatch.setattr(os, "killpg", Mock(), raising=False) + monkeypatch.setattr(os, "getpgid", lambda _pid: 1000, raising=False) + monkeypatch.setattr(os, "getpgrp", lambda: 1000, raising=False) + + with caplog.at_level(logging.WARNING): + session._kill_process() # must not propagate the PermissionError + + process.kill.assert_called_once_with() + assert "direct kill failed" in caplog.text.lower() + + +def test_kill_process_uses_direct_kill_without_killpg( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Use a direct kill on platforms without `os.killpg` (e.g. Windows).""" + session = ShellSession(tmp_path, HostExecutionPolicy(), ("/bin/bash",), {}) + process = Mock() + process.pid = 2222 + session._process = process + + monkeypatch.delattr(os, "killpg", raising=False) + + session._kill_process() + + process.kill.assert_called_once_with() + + +def test_kill_process_noop_without_active_process(tmp_path: Path) -> None: + """Do nothing when there is no active process to kill.""" + session = ShellSession(tmp_path, HostExecutionPolicy(), ("/bin/bash",), {}) + + # No process has been started; this must not raise. + session._kill_process()