From 42e92c9c09ea0490dcd20c9ac8144333efc6f008 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:18:56 -0700 Subject: [PATCH] fix(git): kill the whole probe process tree on timeout (port of openai/codex#36793) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Timing out a bounded git probe must not leave helper descendants (credential helpers, git-remote-https, hook children) running after the probe fails open. bounded_git_probe now spawns the child in its own process group on POSIX (process_group=0), and _kill_git_process_tree signals the whole group with os.killpg — gated on the child actually leading its own group (pgid == pid), so a shared-group spawn can never take down unrelated processes. Windows keeps the existing taskkill /T /F tree kill. Proven live on main: a fake git that forks a 300s descendant left the descendant running after the probe timeout; with the fix the descendant dies with the launcher. Fast path and fail-open contract unchanged. Port of openai/codex#36793 (Terminate timed-out Git process trees). --- hermes_cli/_subprocess_compat.py | 35 ++++- tests/hermes_cli/test_git_probe_tree_kill.py | 135 +++++++++++++++++++ 2 files changed, 165 insertions(+), 5 deletions(-) create mode 100644 tests/hermes_cli/test_git_probe_tree_kill.py diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index b6b6406660..2477aa29be 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -350,13 +350,21 @@ def noninteractive_git_env( def _kill_git_process_tree(proc: "subprocess.Popen") -> None: - """Best-effort terminate *proc* and, on Windows, its descendants. + """Best-effort terminate *proc* and its descendants on both platforms. - ``proc.kill()`` alone only terminates the PATH-resolved ``git`` launcher; a + ``proc.kill()`` alone only terminates the direct child. On Windows a suspended descendant ``git.exe`` can survive holding duplicates of the captured pipe handles, which keeps the pipes from reaching EOF and leaks two - reader threads + the process per fired timeout. ``taskkill /T /F`` takes the + reader threads + the process per fired timeout — ``taskkill /T /F`` takes the whole tree down so the bounded drain that follows can actually reach EOF. + On POSIX the same class exists: killing the launcher leaves descendants + (credential helpers, ``git-remote-https``, hook children) running and + holding the pipe write ends. The probe is spawned in its own process group + (``process_group=0`` in :func:`bounded_git_probe`), so when — and only + when — the child leads its own group (``pgid == pid``), the entire group is + signalled with ``os.killpg``. The ownership check means a fallback spawn + that shares our group can never cause us to kill unrelated processes. + Ported from openai/codex#36793 ("Terminate timed-out Git process trees"). All failures are swallowed — this is cleanup on an already-failing path, and the caller's contract is to fail open. ``kill()`` can raise (access denied, @@ -365,6 +373,17 @@ def _kill_git_process_tree(proc: "subprocess.Popen") -> None: re-enter the deadlock class it fixes: it captures no pipes (DEVNULL), so its own timeout cleanup has no reader threads to join. """ + if not IS_WINDOWS: + # Group-kill first: verify the child actually leads its own process + # group before signalling it, so we never blast a shared group. + try: + import signal as _signal + + pgid = os.getpgid(proc.pid) + if pgid == proc.pid: + os.killpg(pgid, _signal.SIGKILL) + except Exception: + pass try: proc.kill() except OSError: @@ -408,9 +427,15 @@ def bounded_git_probe(argv: Sequence[str], *, timeout: float) -> str: The normal-path spawn contract mirrors the previous ``run`` call byte-for-byte: PIPE/PIPE/DEVNULL, ``text`` with UTF-8 ``errors="replace"`` decoding, and the - hidden-window ``creationflags`` on Windows only. + hidden-window ``creationflags`` on Windows only. On POSIX the probe is + additionally placed in its own process group (``process_group=0``, + Python ≥3.11) so timeout cleanup can take down descendants — credential + helpers, ``git-remote-https``, hook children — with the launcher instead of + orphaning them (see :func:`_kill_git_process_tree`; port of + openai/codex#36793). ``process_group`` only changes which group the child + belongs to; it does not detach the terminal or alter the fast path. """ - _popen_kwargs = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {} + _popen_kwargs: dict = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {"process_group": 0} try: proc = subprocess.Popen( list(argv), diff --git a/tests/hermes_cli/test_git_probe_tree_kill.py b/tests/hermes_cli/test_git_probe_tree_kill.py new file mode 100644 index 0000000000..05fec64007 --- /dev/null +++ b/tests/hermes_cli/test_git_probe_tree_kill.py @@ -0,0 +1,135 @@ +"""POSIX process-tree cleanup for ``bounded_git_probe`` (port of openai/codex#36793). + +Timing out a git probe must not leave helper descendants (credential helpers, +``git-remote-https``, hook children) running after the probe returns. The +probe spawns the child in its own process group (``process_group=0``) and +``_kill_git_process_tree`` signals the whole group with ``os.killpg`` — but +only when the child actually leads its own group, so a shared-group spawn can +never take down unrelated processes. + +These tests use REAL subprocesses (no mocks): a mock cannot reproduce group +membership or survival semantics. +""" + +import os +import subprocess +import sys +import textwrap +import time + +import pytest + +from hermes_cli import _subprocess_compat +from hermes_cli._subprocess_compat import _kill_git_process_tree, bounded_git_probe + +pytestmark = pytest.mark.skipif( + sys.platform == "win32", reason="POSIX process-group semantics" +) + + +def _write_forking_script(tmp_path, marker_name="child.pid"): + """A fake ``git`` that forks a long-lived descendant, then stalls.""" + marker = tmp_path / marker_name + script = tmp_path / "fakegit.sh" + script.write_text( + textwrap.dedent( + f"""\ + #!/bin/bash + sleep 300 & + echo $! > {marker} + sleep 300 + """ + ) + ) + script.chmod(0o755) + return script, marker + + +def _pid_alive(pid: int) -> bool: + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + return True + + +def _wait_marker(marker, timeout=5.0) -> int: + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if marker.exists() and marker.read_text().strip(): + return int(marker.read_text().strip()) + time.sleep(0.05) + raise AssertionError("forking script never wrote its descendant pid") + + +def test_timeout_kills_descendants(tmp_path): + """A probe timeout must take the descendant down with the launcher.""" + script, marker = _write_forking_script(tmp_path) + + out = bounded_git_probe([str(script)], timeout=1.0) + assert out == "" + + child_pid = _wait_marker(marker) + # Precondition sanity: the descendant existed (marker written) — now it + # must be gone shortly after the probe returned. + deadline = time.monotonic() + 3.0 + while time.monotonic() < deadline and _pid_alive(child_pid): + time.sleep(0.05) + alive = _pid_alive(child_pid) + if alive: # cleanup so a failure doesn't leak a 300s sleeper + os.kill(child_pid, 9) + assert not alive, f"descendant {child_pid} survived probe timeout" + + +def test_posix_spawn_uses_own_process_group(tmp_path): + """The probe child must lead its own process group (killpg precondition).""" + script = tmp_path / "pgid.sh" + script.write_text("#!/bin/bash\necho \"$$ $(ps -o pgid= -p $$ | tr -d ' ')\"\n") + script.chmod(0o755) + + out = bounded_git_probe([str(script)], timeout=5.0) + pid, pgid = out.split() + assert pid == pgid, f"probe child pid={pid} does not lead its group pgid={pgid}" + assert int(pgid) != os.getpgid(0), "probe child must not share our group" + + +def test_group_kill_skipped_when_child_shares_our_group(): + """_kill_git_process_tree must never killpg a group the child doesn't lead. + + Spawn WITHOUT process_group=0 (child inherits OUR group): the ownership + check (pgid == pid) must skip the group signal, or the test process itself + would die here. + """ + proc = subprocess.Popen( + ["sleep", "60"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + stdin=subprocess.DEVNULL, + ) + assert os.getpgid(proc.pid) == os.getpgid(0) # shared group precondition + _kill_git_process_tree(proc) + proc.wait(timeout=5) + # We are alive to make this assertion — killpg on our own group would have + # taken the test runner down. The direct child is still killed. + assert proc.returncode is not None + + +def test_fast_path_unaffected(tmp_path): + """Successful probes behave exactly as before the group-kill port.""" + subprocess.run(["git", "init", "-q", str(tmp_path / "repo")], check=True) + out = bounded_git_probe( + ["git", "-C", str(tmp_path / "repo"), "rev-parse", "--is-inside-work-tree"], + timeout=10, + ) + assert out == "true" + + +def test_kill_helper_swallow_all_failures(): + """Cleanup on the fail-open path must never raise, even for a reaped pid.""" + + class _Dead: + pid = 2**22 # extremely unlikely to exist + def kill(self): + raise OSError("already reaped") + + _kill_git_process_tree(_Dead()) # must not raise