mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
refactor(process): extend preexec_fn→start_new_session swap to remaining Popen sites
PR #8399 replaced preexec_fn=os.setsid with start_new_session=<bool> 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.
This commit is contained in:
parent
94a1990404
commit
c74030a11d
8 changed files with 31 additions and 13 deletions
|
|
@ -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 `/`.
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -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=<bool>``, 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."""
|
||||
|
|
|
|||
|
|
@ -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 ---
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue