diff --git a/loopx/registry.py b/loopx/registry.py index a8826530fd..d944d9f3e3 100644 --- a/loopx/registry.py +++ b/loopx/registry.py @@ -95,6 +95,10 @@ def registry_goals(registry: dict[str, Any]) -> list[dict[str, Any]]: ) +_GIT_PROBE_TIMEOUT_SECONDS = 1.5 +_GIT_PROBE_DRAIN_SECONDS = 1.0 + + def _definitely_outside_git_worktree(probe_dir: Path) -> bool: if os.environ.get("GIT_DIR") or os.environ.get("GIT_WORK_TREE"): return False @@ -105,6 +109,27 @@ def _definitely_outside_git_worktree(probe_dir: Path) -> bool: return not any(os.path.lexists(parent / ".git") for parent in (current, *current.parents)) +def _release_probe_pipes(process: subprocess.Popen[str]) -> None: + """Drop a timed-out probe's pipes instead of draining them. + + A descendant that opened its own session keeps the inherited write end, so + reading to EOF waits for a process this probe no longer owns. The deadline + already discards the output, so close the parent's ends and let the one + reaped leader decide how long the release may take. + """ + + for stream in (process.stdout, process.stderr): + try: + if stream is not None: + stream.close() + except (OSError, ValueError): # pragma: no cover - already closed. + pass + try: + process.wait(timeout=_GIT_PROBE_DRAIN_SECONDS) + except subprocess.TimeoutExpired: # pragma: no cover - the group kill already landed. + process.kill() + + def _registry_git_probe(path: Path) -> dict[str, Any]: probe_dir = path if path.is_dir() else path.parent @@ -119,8 +144,11 @@ def unavailable() -> dict[str, Any]: } def run_git(*args: str, cwd: Path = probe_dir) -> subprocess.CompletedProcess[str] | None: + # A probe that times out must retire everything it started. `run` kills + # only the leader, so a Git that fanned out into a helper, a hook or a + # remote would outlive the deadline on every later status read. try: - return subprocess.run( + process = subprocess.Popen( ["git", *args], cwd=cwd, text=True, @@ -128,11 +156,26 @@ def run_git(*args: str, cwd: Path = probe_dir) -> subprocess.CompletedProcess[st errors="replace", stdout=subprocess.PIPE, stderr=subprocess.PIPE, - check=False, - timeout=1.5, + start_new_session=os.name == "posix", + creationflags=( + int(getattr(subprocess, "CREATE_NEW_PROCESS_GROUP")) + if os.name == "nt" else 0 + ), ) - except (OSError, subprocess.TimeoutExpired): + except OSError: + return None + try: + stdout, stderr = process.communicate(timeout=_GIT_PROBE_TIMEOUT_SECONDS) + except subprocess.TimeoutExpired: + from .extensions.process_runtime import terminate_process_tree + + try: + terminate_process_tree(process, 0) + except (OSError, RuntimeError, ValueError): + process.kill() + _release_probe_pipes(process) return None + return subprocess.CompletedProcess(["git", *args], process.returncode, stdout, stderr) # Git discovers a normal repository only through an ancestor `.git` # marker. Non-Git LoopX projects are common, and status can inspect the diff --git a/tests/test_registry_git_probe.py b/tests/test_registry_git_probe.py index f87e646711..24721dd692 100644 --- a/tests/test_registry_git_probe.py +++ b/tests/test_registry_git_probe.py @@ -1,6 +1,10 @@ from __future__ import annotations +import os +import signal import subprocess +import sys +import time from pathlib import Path import pytest @@ -35,7 +39,7 @@ def unexpected_git( calls.append(tuple(command)) return subprocess.CompletedProcess(command, 128, "", "not a git repository") - monkeypatch.setattr(registry_module.subprocess, "run", unexpected_git) + monkeypatch.setattr(registry_module.subprocess, "Popen", unexpected_git) assert registry_module._registry_git_probe(registry_path) == { "available": True, @@ -102,7 +106,7 @@ def test_registry_git_probe_reports_unavailable_without_git( def unexpected_git(*_: object, **__: object) -> None: raise AssertionError("non-Git fast path must not launch Git") - monkeypatch.setattr(registry_module.subprocess, "run", unexpected_git) + monkeypatch.setattr(registry_module.subprocess, "Popen", unexpected_git) assert registry_module._registry_git_probe(registry_path) == { "available": False, @@ -112,3 +116,83 @@ def unexpected_git(*_: object, **__: object) -> None: "ignored": False, "worktree_root_recorded": False, } + + +@pytest.mark.skipif(os.name != "posix", reason="the probe's fake git is a POSIX shell script") +def test_registry_git_probe_retires_the_process_tree_on_timeout( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A Git that fans out must not outlive the deadline that ended the probe.""" + + probe = tmp_path / "project" + probe.mkdir() + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + grandchild = tmp_path / "grandchild.pid" + fake_git = bin_dir / "git" + fake_git.write_text( + "#!/bin/sh\n" + "sleep 30 &\n" + f"echo $! > {grandchild}\n" + "sleep 30\n", + encoding="utf-8", + ) + fake_git.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setenv("GIT_DIR", str(tmp_path / "fake.git")) + + observed = registry_module._registry_git_probe(probe / "registry.json") + assert observed["probe_status"] == "git_unavailable" + + pid = int(grandchild.read_text(encoding="utf-8").strip()) + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + try: + os.kill(pid, 0) + except ProcessLookupError: + return + time.sleep(0.05) + os.kill(pid, signal.SIGKILL) + raise AssertionError("the probe timed out but left its grandchild running") + + +@pytest.mark.skipif(os.name != "posix", reason="the probe's fake git is a POSIX script") +def test_registry_git_probe_bounds_the_drain_when_a_descendant_keeps_the_pipe( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A descendant in another session can hold stdout open past the deadline.""" + + probe = tmp_path / "project" + probe.mkdir() + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + grandchild = tmp_path / "grandchild.pid" + fake_git = bin_dir / "git" + fake_git.write_text( + f"#!{sys.executable}\n" + "import subprocess, sys, time\n" + "child = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(12)'],\n" + " start_new_session=True)\n" + f"open({str(grandchild)!r}, 'w', encoding='utf-8').write(str(child.pid))\n" + "time.sleep(12)\n", + encoding="utf-8", + ) + fake_git.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setenv("GIT_DIR", str(tmp_path / "fake.git")) + + started = time.monotonic() + try: + observed = registry_module._registry_git_probe(probe / "registry.json") + elapsed = time.monotonic() - started + finally: + if grandchild.exists(): + try: + os.kill(int(grandchild.read_text(encoding="utf-8").strip() or 0), signal.SIGKILL) + except (OSError, ValueError): + pass + + assert observed["probe_status"] == "git_unavailable" + assert elapsed < 6, f"the probe waited {elapsed:.2f}s on a pipe it no longer owns"