From fc0a6f0bb8e08be902d4cfdd3f74538a30780984 Mon Sep 17 00:00:00 2001 From: Gabriel <123674083+VerbalChainsaw@users.noreply.github.com> Date: Sun, 19 Jul 2026 02:12:54 -0500 Subject: [PATCH] fix(environments): Windows job-breakaway parity for LocalEnvironment foreground commands Route LocalEnvironment._run_bash's foreground spawn through windows_detach_popen_kwargs() so a Windows foreground child breaks out of the parent Hermes job object where that job permits breakaway, giving best-effort process-survival parity with the POSIX start_new_session=True path. On POSIX the helper supplies exactly one start_new_session=True (replacing the literal keyword this call site passed), so there is no duplicated Popen keyword and no preexec_fn. Add a narrow fallback for an error consistent with a construction-time breakaway refusal: retry once without CREATE_BREAKAWAY_FROM_JOB only when exc.winerror == 5 AND IsProcessInJob confirms this process is in a job. Fail closed when membership is unknown or the query fails, and propagate every other OSError after a single attempt. A successful Popen is never respawned. DETACHED_PROCESS does not sever explicitly redirected stdio (only an inherited console), so stdout=PIPE / stderr=STDOUT / stdin redirection at this call site are unaffected; explicitly redirected stdin, stdout, and merged stderr were confirmed functional under DETACHED_PROCESS in native testing. This is process-survival parity only. It is not universal job escape (a restrictive job may silently retain the child), not durable execution, and not durable output: after abrupt Hermes death a surviving process has no result owner, its output is lost, and side effects may complete unrecorded. Controlled ownership (timeout, interrupt, shutdown, explicit tree-kill) is retained. Tests: new tests/tools/test_local_windows_breakaway_parity.py drives the real _run_bash with subprocess.Popen mocked (both platform branches forced), and a native job-object experiment demonstrates permitting-job escape and restrictive-job retention at the exact patched call site. --- .../test_local_windows_breakaway_parity.py | 299 ++++++++++++++++++ tools/environments/local.py | 124 +++++++- 2 files changed, 408 insertions(+), 15 deletions(-) create mode 100644 tests/tools/test_local_windows_breakaway_parity.py diff --git a/tests/tools/test_local_windows_breakaway_parity.py b/tests/tools/test_local_windows_breakaway_parity.py new file mode 100644 index 000000000000..0fe287f60bec --- /dev/null +++ b/tests/tools/test_local_windows_breakaway_parity.py @@ -0,0 +1,299 @@ +"""Behavioral tests for LocalEnvironment._run_bash Windows job-breakaway parity. + +These drive the REAL patched ``_run_bash`` implementation with +``subprocess.Popen`` monkey-patched to a fake, so we assert the exact +kwargs/argv/env/cwd it constructs and how it reacts to constructor failures. +No real subprocesses and no real Windows APIs are required — flag values come +from the same helpers the production code uses, and both platform branches are +forced explicitly, so every test runs on any host OS. + +Contract under test: + +* Windows primary call receives the full detach kwargs (no start_new_session). +* A qualifying ERROR_ACCESS_DENIED (winerror 5) while the parent is confirmed + in a job retries exactly once without CREATE_BREAKAWAY_FROM_JOB. +* winerror 5 with the parent NOT in a job propagates with exactly one attempt. +* winerror != 5 propagates with exactly one attempt. +* A job-membership query failure fails closed: original error propagates, + no retry. +* POSIX receives exactly one start_new_session=True and no creationflags. +* No preexec_fn is introduced on either platform. +* The retry preserves argv/cwd/env/stdout/stderr/stdin/text/encoding/errors. +* The returned process remains explicitly killable. +* A successful Popen return is never respawned. +""" +import subprocess + +import pytest + +from hermes_cli._subprocess_compat import ( + windows_detach_flags_without_breakaway, + windows_detach_popen_kwargs, +) +from tools.environments.local import LocalEnvironment + + +class _FakeStream: + def write(self, data): + return len(data) + + def read(self, n=-1): + return "" + + def readline(self): + return "" + + def close(self): + pass + + +class _FakeProc: + """Minimal stand-in for a spawned subprocess.Popen.""" + + def __init__(self, args, kw): + self.args = args + self._kw = kw + self.pid = 4242 + self.returncode = None + self.stdout = _FakeStream() + self.stderr = None + self.stdin = _FakeStream() + + def poll(self): + return self.returncode + + def wait(self, timeout=None): + return self.returncode + + def kill(self): + self.returncode = -9 + + def terminate(self): + self.returncode = -15 + + +@pytest.fixture +def spawn_calls(monkeypatch): + """Record every subprocess.Popen invocation _run_bash makes. + + Patches the heavy environment helpers so _run_bash exercises only the + spawn path, and suppresses the init_session bootstrap so the command + under test is the sole Popen caller. + """ + import tools.environments.local as local_mod + + monkeypatch.setattr(local_mod, "_find_bash", lambda: "bash") + monkeypatch.setattr(local_mod, "_make_run_env", lambda env: dict(env or {})) + monkeypatch.setattr(local_mod, "_resolve_safe_cwd", lambda cwd: cwd) + monkeypatch.setattr(local_mod.BaseEnvironment, "init_session", lambda self: None) + + calls = [] + + def _fake_popen(args, *a, **kw): + calls.append((args, dict(kw))) + return _FakeProc(args, kw) + + monkeypatch.setattr(subprocess, "Popen", _fake_popen) + return calls + + +def _force_windows(monkeypatch, *, in_job=True): + import hermes_cli._subprocess_compat as compat_mod + import tools.environments.local as local_mod + + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + monkeypatch.setattr(compat_mod, "IS_WINDOWS", True) + monkeypatch.setattr(local_mod, "_process_in_job", lambda: in_job) + + +def _force_posix(monkeypatch): + import os + + import hermes_cli._subprocess_compat as compat_mod + import tools.environments.local as local_mod + + monkeypatch.setattr(local_mod, "_IS_WINDOWS", False) + monkeypatch.setattr(compat_mod, "IS_WINDOWS", False) + monkeypatch.setattr(os, "getpgid", lambda pid: 1000, raising=False) + + +def _fail_first_popen_with(monkeypatch, calls, exc): + """Install a Popen fake whose first call raises *exc* and later calls + succeed, recording every attempt into *calls*.""" + state = {"n": 0} + + def _popen(args, *a, **kw): + calls.append((args, dict(kw))) + state["n"] += 1 + if state["n"] == 1: + raise exc + return _FakeProc(args, kw) + + monkeypatch.setattr(subprocess, "Popen", _popen) + + +def _winerror(code, msg="spawn failed"): + exc = OSError(msg) + exc.winerror = code + return exc + + +def _make_env(): + env = LocalEnvironment(cwd="/tmp") + env.env = {"PATH": "/usr/bin"} + return env + + +def test_windows_primary_full_detach_kwargs(monkeypatch, spawn_calls): + _force_windows(monkeypatch) + _make_env()._run_bash("echo hi") + + assert len(spawn_calls) == 1 + kw = spawn_calls[0][1] + assert kw.get("creationflags") == windows_detach_popen_kwargs()["creationflags"] + assert "start_new_session" not in kw + assert "preexec_fn" not in kw + assert kw["stdout"] is subprocess.PIPE + assert kw["stderr"] is subprocess.STDOUT + assert kw["stdin"] is subprocess.DEVNULL + assert kw["text"] is True + assert kw["encoding"] == "utf-8" + assert kw["errors"] == "replace" + + +def test_windows_breakaway_refusal_retries_once(monkeypatch, spawn_calls): + _force_windows(monkeypatch, in_job=True) + _fail_first_popen_with(monkeypatch, spawn_calls, _winerror(5)) + + proc = _make_env()._run_bash("echo hi") + + assert len(spawn_calls) == 2 + first, second = spawn_calls[0][1], spawn_calls[1][1] + assert second.get("creationflags") == windows_detach_flags_without_breakaway() + assert spawn_calls[0][0] == spawn_calls[1][0] + for key in ("cwd", "env", "stdout", "stderr", "stdin", "text", "encoding", "errors"): + assert second.get(key) == first.get(key), key + assert isinstance(proc, _FakeProc) + + +def test_winerror_5_not_in_job_propagates(monkeypatch, spawn_calls): + _force_windows(monkeypatch, in_job=False) + + def _always_fail(args, *a, **kw): + spawn_calls.append((args, dict(kw))) + raise _winerror(5) + + monkeypatch.setattr(subprocess, "Popen", _always_fail) + + with pytest.raises(OSError) as excinfo: + _make_env()._run_bash("echo hi") + + assert excinfo.value.winerror == 5 + assert len(spawn_calls) == 1 + + +def test_winerror_not_5_propagates(monkeypatch, spawn_calls): + _force_windows(monkeypatch, in_job=True) + + def _always_fail(args, *a, **kw): + spawn_calls.append((args, dict(kw))) + raise _winerror(2, "file not found") + + monkeypatch.setattr(subprocess, "Popen", _always_fail) + + with pytest.raises(OSError) as excinfo: + _make_env()._run_bash("echo hi") + + assert excinfo.value.winerror == 2 + assert len(spawn_calls) == 1 + + +def test_job_membership_query_failure_fails_closed(monkeypatch, spawn_calls): + """Force the real _process_in_job's ctypes boundary to fail: the helper + must return False (never raise), the original winerror-5 error must + propagate unchanged, and no fallback spawn may occur.""" + import ctypes + + import hermes_cli._subprocess_compat as compat_mod + import tools.environments.local as local_mod + + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + monkeypatch.setattr(compat_mod, "IS_WINDOWS", True) + # No _process_in_job stub here — exercise the real helper with a broken + # Win32 boundary underneath it. + monkeypatch.setattr( + ctypes, "WinDLL", + lambda *a, **kw: (_ for _ in ()).throw(OSError("kernel32 unavailable")), + raising=False, + ) + + def _always_fail(args, *a, **kw): + spawn_calls.append((args, dict(kw))) + raise _winerror(5) + + monkeypatch.setattr(subprocess, "Popen", _always_fail) + + with pytest.raises(OSError) as excinfo: + _make_env()._run_bash("echo hi") + + assert excinfo.value.winerror == 5 + assert len(spawn_calls) == 1 + + +def test_second_constructor_failure_propagates(monkeypatch, spawn_calls): + _force_windows(monkeypatch, in_job=True) + + def _always_fail(args, *a, **kw): + spawn_calls.append((args, dict(kw))) + raise _winerror(5) + + monkeypatch.setattr(subprocess, "Popen", _always_fail) + + with pytest.raises(OSError): + _make_env()._run_bash("echo hi") + + assert len(spawn_calls) == 2 # primary + exactly one fallback, no third + + +def test_posix_single_start_new_session(monkeypatch, spawn_calls): + _force_posix(monkeypatch) + _make_env()._run_bash("echo hi") + + assert len(spawn_calls) == 1 + kw = spawn_calls[0][1] + assert kw.get("start_new_session") is True + assert "creationflags" not in kw + assert "preexec_fn" not in kw + + +def test_posix_oserror_propagates_without_retry(monkeypatch, spawn_calls): + _force_posix(monkeypatch) + + def _always_fail(args, *a, **kw): + spawn_calls.append((args, dict(kw))) + raise OSError("fork failed") + + monkeypatch.setattr(subprocess, "Popen", _always_fail) + + with pytest.raises(OSError): + _make_env()._run_bash("echo hi") + + assert len(spawn_calls) == 1 + + +def test_returned_process_killable(monkeypatch, spawn_calls): + _force_windows(monkeypatch) + proc = _make_env()._run_bash("sleep 30") + + assert proc.returncode is None + proc.kill() + assert proc.returncode == -9 + + +def test_no_respawn_after_success(monkeypatch, spawn_calls): + _force_windows(monkeypatch) + env = _make_env() + proc = env._run_bash("echo hi") + + assert len(spawn_calls) == 1 + assert isinstance(proc, _FakeProc) diff --git a/tools/environments/local.py b/tools/environments/local.py index 8b4450c72010..a1dd4b6f8292 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -14,13 +14,55 @@ from pathlib import Path from tools.environments.base import BaseEnvironment, _pipe_stdin -from hermes_cli._subprocess_compat import windows_hide_flags +from hermes_cli._subprocess_compat import ( + windows_detach_flags_without_breakaway, + windows_detach_popen_kwargs, + windows_hide_flags, +) _IS_WINDOWS = platform.system() == "Windows" logger = logging.getLogger(__name__) +def _process_in_job() -> bool: + """Return True if the current process belongs to a Windows job object. + + Used to disambiguate a ``CREATE_BREAKAWAY_FROM_JOB`` refusal. That flag is + refused with ERROR_ACCESS_DENIED (winerror 5) only when a job object + exists that disallows breakaway; a process not in any job cannot receive + that refusal, so winerror 5 there is an unrelated access-denied condition. + + Fails closed: returns False on non-Windows and on any query failure — + callers must never retry on an unknown membership state. + """ + if not _IS_WINDOWS: + return False + try: + import ctypes + from ctypes import wintypes + + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.GetCurrentProcess.argtypes = [] + kernel32.GetCurrentProcess.restype = wintypes.HANDLE + # Win32 BOOL is a 32-bit int (wintypes.BOOL) — NOT ctypes.c_bool, + # whose 1-byte buffer would be too small for IsProcessInJob's + # 4-byte PBOOL out-parameter write. + kernel32.IsProcessInJob.argtypes = [ + wintypes.HANDLE, + wintypes.HANDLE, + ctypes.POINTER(wintypes.BOOL), + ] + kernel32.IsProcessInJob.restype = wintypes.BOOL + in_job = wintypes.BOOL(0) + ok = kernel32.IsProcessInJob( + kernel32.GetCurrentProcess(), None, ctypes.byref(in_job) + ) + return bool(ok) and bool(in_job.value) + except Exception: + return False + + def _msys_to_windows_path(cwd: str) -> str: """Translate a Git Bash / MSYS-style POSIX path (``/c/Users/x``) to the native Windows form (``C:\\Users\\x``) so ``os.path.isdir`` and @@ -1374,21 +1416,73 @@ def _run_bash(self, cmd_string: str, *, login: bool = False, _popen_cwd = self.cwd - _popen_kwargs = {"creationflags": windows_hide_flags()} if _IS_WINDOWS else {} + # Platform-aware detach with a narrow breakaway-refusal fallback. + # On POSIX the helper supplies the same single ``start_new_session=True`` + # this call site already used (os.setsid): the child is + # session-isolated — explicitly killable via its pgid during + # interrupt/timeout/shutdown, yet not torn down by an uncatchable + # parent death. On Windows it sets the full detach flag bundle + # (CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_NO_WINDOW | + # CREATE_BREAKAWAY_FROM_JOB) so a foreground child aligns with that + # same best-effort survival behavior where the parent's job object + # permits breakaway. DETACHED_PROCESS does not affect the explicit + # stdout/stderr/stdin redirections below — only inherited-console + # stdio, which this call site never uses. + _popen_kwargs = windows_detach_popen_kwargs() - proc = subprocess.Popen( - args, - text=True, - env=run_env, - encoding="utf-8", - errors="replace", - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - stdin=subprocess.PIPE if stdin_data is not None else subprocess.DEVNULL, - start_new_session=True, - cwd=_popen_cwd, - **_popen_kwargs, - ) + try: + proc = subprocess.Popen( + args, + text=True, + env=run_env, + encoding="utf-8", + errors="replace", + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + stdin=subprocess.PIPE if stdin_data is not None else subprocess.DEVNULL, + cwd=_popen_cwd, + **_popen_kwargs, + ) + except OSError as _spawn_err: + if not _IS_WINDOWS: + raise + # Narrow retry, only for an error consistent with a breakaway + # refusal: CREATE_BREAKAWAY_FROM_JOB is refused with + # ERROR_ACCESS_DENIED (winerror 5) when the parent belongs to a job + # that disallows breakaway. Two conditions must both hold before we + # retry, so an unrelated access-denied error is not mistaken for a + # breakaway refusal: + # (a) exc.winerror == 5 — the breakaway-refusal error code; + # (b) the current process is in a job object. Without (b) an + # ERROR_ACCESS_DENIED cannot be a breakaway refusal (there is + # no job to refuse from), so it is an unrelated access-denied + # condition and must propagate rather than trigger a second + # spawn with different creationflags. + # _process_in_job() fails closed (False on query failure), so an + # unknown membership state also propagates the original error. + # At most one retry; a successful Popen return is never respawned. + # Note: on modern nested-job Windows builds a forbidden breakaway + # can instead succeed silently with the child retained in the job + # — no error is raised, this fallback never runs, and the current + # (in-job) behavior simply continues for that configuration. + if getattr(_spawn_err, "winerror", None) == 5 and _process_in_job(): + _popen_kwargs = { + "creationflags": windows_detach_flags_without_breakaway() + } + proc = subprocess.Popen( + args, + text=True, + env=run_env, + encoding="utf-8", + errors="replace", + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + stdin=subprocess.PIPE if stdin_data is not None else subprocess.DEVNULL, + cwd=_popen_cwd, + **_popen_kwargs, + ) + else: + raise if not _IS_WINDOWS: try: proc._hermes_pgid = os.getpgid(proc.pid)