github/langchain
1
0
Fork 0
mirror of https://github.com/hwchase17/langchain.git synced 2026-07-11 09:26:29 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(langchain): avoid shared process-group kill in shell middleware (#36359)

Browse Source 
Fixes #36358

---

**Summary**
This PR fixes a process termination safety bug in ShellToolMiddleware
where cleanup could kill the caller process group when
create_process_group is False.

**Why this change**
When the shell child is started without a dedicated process group, it
can share the parent group. The existing cleanup path used group kill
unconditionally, which could terminate unrelated processes including the
caller. This is a high-impact availability risk.

**What changed**

Updated shell session kill logic to use group kill only when the child
is in a different process group than the caller.
Added safe fallback to child-only kill when both share the same process
group.
Added regression tests for both scenarios:
Shared process group: no group kill, child-only kill.
Dedicated process group: existing group kill behavior is preserved.

**Validation**

Targeted new tests passed.
Full shell tool unit test file passed.

AI assistance disclosure
This contribution was prepared with assistance from an AI coding agent.
I reviewed, validated, and finalized the proposed changes and test
coverage before submission.

**Areas for careful review**

Process-group detection behavior on Linux and other POSIX environments.
Any implications for existing timeout and shutdown flows in shell
middleware.
Whether additional integration-level coverage is desirable for process
cleanup behavior.

---------

Co-authored-by: Mason Daugherty <github@mdrxy.com>
This commit is contained in:
Yashodip More
2026-07-06 09:21:57 +05:30
committed by GitHub
parent fd822b07c0
commit a586e9044c
4 changed files with 222 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
libs/langchain_v1/langchain/agents/middleware/_execution.py
View File

@@ -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(

39
libs/langchain_v1/langchain/agents/middleware/shell_tool.py
View File

@@ -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, ""):

40
libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_execution_policies.py
View File

@@ -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"
)

148
libs/langchain_v1/tests/unit_tests/agents/middleware/implementations/test_shell_tool.py
View File

@@ -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()