fix(git): kill the whole probe process tree on timeout (port of openai/codex#36793)
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).
This commit is contained in:
Teknium
@@ -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),
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user