From c74030a11d531fe88a3c07d63c3113a7c2943ad7 Mon Sep 17 00:00:00 2001 From: Teknium Date: Sun, 19 Apr 2026 05:46:01 -0700 Subject: [PATCH] =?UTF-8?q?refactor(process):=20extend=20preexec=5Ffn?= =?UTF-8?q?=E2=86=92start=5Fnew=5Fsession=20swap=20to=20remaining=20Popen?= =?UTF-8?q?=20sites?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #8399 replaced preexec_fn=os.setsid with start_new_session= in tools/environments/local.py to use CPython's thread-safe POSIX fastpath instead of a between-fork-and-exec callback. Apply the same swap to the other three files covered by tests/tools/test_windows_compat.py so all Popen call sites use a consistent idiom. Also: - Strip a trailing-whitespace line introduced by the original PR. - Update CONTRIBUTING.md + website/docs/developer-guide/contributing.md to recommend start_new_session in the cross-platform process management section. - Fix a stale docstring reference in tools/environments/base.py (_popen_bash). - Extend test_windows_compat.py with a parallel regression guard that rejects bare start_new_session=True (must be gated on _IS_WINDOWS). Scope note: this is code hygiene, not a fix for #8340 — the two forms invoke the same setsid() syscall, so this swap alone does not change behavior for the 'setsid ... & disown' hang scenario in that issue. #8340 remains open. --- CONTRIBUTING.md | 3 +-- gateway/platforms/whatsapp.py | 2 +- tests/tools/test_windows_compat.py | 28 +++++++++++++++++--- tools/code_execution_tool.py | 2 +- tools/environments/base.py | 2 +- tools/environments/local.py | 2 +- tools/process_registry.py | 2 +- website/docs/developer-guide/contributing.md | 3 +-- 8 files changed, 31 insertions(+), 13 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4577454e4..e14efcff4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -543,8 +543,7 @@ Hermes runs on Linux, macOS, and Windows. When writing code that touches the OS: 3. **Process management.** `os.setsid()`, `os.killpg()`, and signal handling differ on Windows. Use platform checks: ```python import platform - if platform.system() != "Windows": - kwargs["preexec_fn"] = os.setsid + kwargs["start_new_session"] = platform.system() != "Windows" ``` 4. **Path separators.** Use `pathlib.Path` instead of string concatenation with `/`. diff --git a/gateway/platforms/whatsapp.py b/gateway/platforms/whatsapp.py index d1de5b856..04b7657c6 100644 --- a/gateway/platforms/whatsapp.py +++ b/gateway/platforms/whatsapp.py @@ -372,7 +372,7 @@ class WhatsAppAdapter(BasePlatformAdapter): ], stdout=bridge_log_fh, stderr=bridge_log_fh, - preexec_fn=None if _IS_WINDOWS else os.setsid, + start_new_session=not _IS_WINDOWS, env=bridge_env, ) diff --git a/tests/tools/test_windows_compat.py b/tests/tools/test_windows_compat.py index ec04d2095..dce51ef38 100644 --- a/tests/tools/test_windows_compat.py +++ b/tests/tools/test_windows_compat.py @@ -19,13 +19,13 @@ GUARDED_FILES = [ PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent -def _get_preexec_fn_values(filepath: Path) -> list: - """Find all preexec_fn= keyword arguments in Popen calls.""" +def _get_kwarg_values(filepath: Path, kwarg_name: str) -> list: + """Find all ``kwarg_name=...`` keyword arguments in function calls.""" source = filepath.read_text(encoding="utf-8") tree = ast.parse(source, filename=str(filepath)) values = [] for node in ast.walk(tree): - if isinstance(node, ast.keyword) and node.arg == "preexec_fn": + if isinstance(node, ast.keyword) and node.arg == kwarg_name: values.append(ast.dump(node.value)) return values @@ -38,13 +38,33 @@ class TestNoUnconditionalSetsid: filepath = PROJECT_ROOT / relpath if not filepath.exists(): pytest.skip(f"{relpath} not found") - values = _get_preexec_fn_values(filepath) + values = _get_kwarg_values(filepath, "preexec_fn") for val in values: # A bare os.setsid would be: Attribute(value=Name(id='os'), attr='setsid') assert "attr='setsid'" not in val or "IfExp" in val or "None" in val, ( f"{relpath} has unconditional preexec_fn=os.setsid" ) + @pytest.mark.parametrize("relpath", GUARDED_FILES) + def test_start_new_session_is_guarded(self, relpath): + """start_new_session must not be an unconditional True. + + The modern thread-safe replacement for ``preexec_fn=os.setsid`` is + ``start_new_session=``, but Windows' subprocess backend doesn't + support it — so it must always be gated on a platform check (typically + ``not _IS_WINDOWS``), never a bare ``True``. + """ + filepath = PROJECT_ROOT / relpath + if not filepath.exists(): + pytest.skip(f"{relpath} not found") + values = _get_kwarg_values(filepath, "start_new_session") + for val in values: + # A bare True literal would be: Constant(value=True) + assert val != "Constant(value=True)", ( + f"{relpath} has unconditional start_new_session=True " + f"(must be guarded, e.g. `not _IS_WINDOWS`)" + ) + class TestIsWindowsConstant: """Each guarded file must define _IS_WINDOWS.""" diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index c5a89488a..de6084d30 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -1065,7 +1065,7 @@ def execute_code( stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.DEVNULL, - preexec_fn=None if _IS_WINDOWS else os.setsid, + start_new_session=not _IS_WINDOWS, ) # --- Poll loop: watch for exit, timeout, and interrupt --- diff --git a/tools/environments/base.py b/tools/environments/base.py index 1bc08449e..4d8f6b0d4 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -115,7 +115,7 @@ def _popen_bash( """Spawn a subprocess with standard stdout/stderr/stdin setup. If *stdin_data* is provided, writes it asynchronously via :func:`_pipe_stdin`. - Backends with special Popen needs (e.g. local's ``preexec_fn``) can bypass + Backends with special Popen needs (e.g. local's ``start_new_session``) can bypass this and call :func:`_pipe_stdin` directly. """ proc = subprocess.Popen( diff --git a/tools/environments/local.py b/tools/environments/local.py index 37abf8e0c..5b0b89bcc 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -269,7 +269,7 @@ class LocalEnvironment(BaseEnvironment): stdin=subprocess.PIPE if stdin_data is not None else subprocess.DEVNULL, start_new_session=not _IS_WINDOWS, ) - + if stdin_data is not None: _pipe_stdin(proc, stdin_data) diff --git a/tools/process_registry.py b/tools/process_registry.py index 92f3db2a1..e60aeff5b 100644 --- a/tools/process_registry.py +++ b/tools/process_registry.py @@ -395,7 +395,7 @@ class ProcessRegistry: stdout=subprocess.PIPE, stderr=subprocess.STDOUT, stdin=subprocess.PIPE, - preexec_fn=None if _IS_WINDOWS else os.setsid, + start_new_session=not _IS_WINDOWS, ) session.process = proc diff --git a/website/docs/developer-guide/contributing.md b/website/docs/developer-guide/contributing.md index f9b9e0ec5..107d0ba79 100644 --- a/website/docs/developer-guide/contributing.md +++ b/website/docs/developer-guide/contributing.md @@ -129,8 +129,7 @@ except UnicodeDecodeError: ```python import platform -if platform.system() != "Windows": - kwargs["preexec_fn"] = os.setsid +kwargs["start_new_session"] = platform.system() != "Windows" ``` ### 4. Path separators