The OS lanes are marker-driven: list_os_marked_tests.py picks the files
a lane imports from their platforms() specs and the lane selects with
-m platforms. A test gated with skipif(sys.platform != "win32") is
therefore never imported on the Windows lane and skipped everywhere else
— it runs on no host. skipif(sys.platform == "win32") tests were merely
invisible to the lane bookkeeping, but the rule the tree now follows is
one host marker, never a bare skipif.
Mechanical mapping, semantics preserved: skip-on-Windows → "posix",
skip-off-Windows → "windows", skip-off-Linux → "linux", skip-on-macOS →
"not macos". The former skip reasons stay as trailing comments. A
non-host condition (os.geteuid() == 0) stays a separate skipif beside
the marker, spelled getattr(os, "geteuid", ...) so the decorator still
imports on Windows.
Where the conversion would stack two platforms() marks on one test (the
conftest rejects that at collection) the narrower mark wins:
- test_update_wedged_gateway: the class is already platforms("linux");
its per-test "needs UNIX sockets" marks were redundant and are gone.
- test_process_registry.TestSystemdCgroupIsolation: the class-level
skip-on-Windows moves onto the 11 methods that had no host mark; the
11 platforms("linux") methods keep theirs.
- test_file_ops_single_roundtrip: the two fifo tests drop their
platforms("linux") in favour of the module's "posix" (mkfifo exists on
macOS; both tests already skip when it does not).
- test_linux_desktop_entry / test_gateway_job_teardown_live: duplicate
or wider marks removed.
134 lines
4.5 KiB
Python
134 lines
4.5 KiB
Python
"""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_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 bounded_git_probe, kill_process_tree
|
|
|
|
pytestmark = pytest.mark.platforms("posix") # 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"""\
|
|
#!/usr/bin/env 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("#!/usr/bin/env 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_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_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_process_tree(_Dead()) # must not raise
|