From 485f6cb95c209625c7456fafc35129dcd07946a2 Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Sun, 8 Nov 2026 09:46:05 +0000 Subject: [PATCH] fix(change-window): bound every Git read to one budget Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- .../repository_change_window/repository.py | 65 ++++++- .../test_change_window_git_budget.py | 177 ++++++++++++++++++ 2 files changed, 236 insertions(+), 6 deletions(-) create mode 100644 tests/capabilities/test_change_window_git_budget.py diff --git a/loopx/capabilities/repository_change_window/repository.py b/loopx/capabilities/repository_change_window/repository.py index 4e1dc5ff8c..120945407d 100644 --- a/loopx/capabilities/repository_change_window/repository.py +++ b/loopx/capabilities/repository_change_window/repository.py @@ -1,13 +1,20 @@ from __future__ import annotations -from dataclasses import dataclass import hashlib import os -from pathlib import Path import re import subprocess +from dataclasses import dataclass +from pathlib import Path from urllib.parse import unquote, urlsplit +from ...extensions.process_runtime import retire_owned_process + +# A managed commit or push hook must not wait forever on a Git helper. An +# fsmonitor hook, a stale index lock or a checkout on a network mount can +# otherwise hold the hook -- and the operation Git asked it to admit -- open. +GIT_READ_TIMEOUT_SECONDS = 10.0 + class RepositoryChangeWindowError(ValueError): """A fail-closed repository or pending-change contract error.""" @@ -24,19 +31,65 @@ class RepositoryContext: head_oid: str +def _retire_owned_read(process: subprocess.Popen[bytes]) -> None: + """Retire the read's own process group and drop its pipes within a deadline. + + The shared runtime bounds termination, Windows ``taskkill`` and pipe + closure under one budget. Git runs helpers -- fsmonitor hooks, pagers -- + inside the session it starts in, so killing the leader alone leaves them + behind; an inherited pipe that a descendant still holds must not extend + the caller's wait either. + """ + + try: + retire_owned_process(process, process_tree_owned=True) + except (OSError, RuntimeError, ValueError, subprocess.SubprocessError): + # The answer is already unavailable; cleanup must not replace it with + # a traceback from a read-only or admission command. The read's own + # timeout or cancellation remains the caller's result. + pass + + def git( repo: Path, *args: str, check: bool = True, input_bytes: bytes | None = None, ) -> subprocess.CompletedProcess[bytes]: - result = subprocess.run( - ["git", "-C", str(repo), *args], - check=False, - input=input_bytes, + """Run one Git read, or refuse when it cannot answer inside its budget. + + An unanswered read is not an empty result. Callers fingerprint a pending + change, evaluate provider state and install or remove hooks from this one + entry point; raising here makes those paths fail closed instead of + reporting a clean worktree they never observed. + """ + + argv = ["git", "-C", str(repo), *args] + process = subprocess.Popen( + argv, + stdin=subprocess.PIPE if input_bytes is not None else None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + start_new_session=os.name == "posix", ) + try: + stdout, stderr = process.communicate( + input=input_bytes, timeout=GIT_READ_TIMEOUT_SECONDS + ) + except subprocess.TimeoutExpired: + _retire_owned_read(process) + raise RepositoryChangeWindowError( + "git " + + " ".join(args) + + f" did not answer within {GIT_READ_TIMEOUT_SECONDS:g} seconds" + ) from None + except BaseException: + # A terminal cancellation reaches this process, not the session the + # read owns. Retire that session before letting the cancellation + # continue to its handler. + _retire_owned_read(process) + raise + result = subprocess.CompletedProcess(argv, process.returncode, stdout, stderr) if check and result.returncode != 0: detail = ( result.stderr.decode("utf-8", errors="replace").strip() diff --git a/tests/capabilities/test_change_window_git_budget.py b/tests/capabilities/test_change_window_git_budget.py new file mode 100644 index 0000000000..037e0c64f3 --- /dev/null +++ b/tests/capabilities/test_change_window_git_budget.py @@ -0,0 +1,177 @@ +from __future__ import annotations + +import os +import shlex +import subprocess +import time +from pathlib import Path + +import pytest + +from loopx.capabilities.repository_change_window import repository +from loopx.extensions import process_runtime + + +def _repository(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main", str(repo)], check=True) + subprocess.run( + ["git", "-C", str(repo), "config", "user.email", "fixture@example.invalid"], + check=True, + ) + subprocess.run( + ["git", "-C", str(repo), "config", "user.name", "Fixture"], check=True + ) + (repo / "tracked.txt").write_text("base\n", encoding="utf-8") + subprocess.run(["git", "-C", str(repo), "add", "tracked.txt"], check=True) + subprocess.run(["git", "-C", str(repo), "commit", "-qm", "base"], check=True) + return repo + + +def test_a_finished_read_keeps_bytes_exit_status_and_stdin(tmp_path: Path) -> None: + repo = _repository(tmp_path) + + result = repository.git(repo, "rev-parse", "--verify", "HEAD") + assert result.returncode == 0 + assert len(result.stdout.strip()) == 40 + assert isinstance(result.stdout, bytes) + + missing = repository.git( + repo, "rev-parse", "--verify", "definitely-missing", check=False + ) + assert missing.returncode != 0 + + hashed = repository.git(repo, "hash-object", "--stdin", input_bytes=b"stdin\n") + assert hashed.returncode == 0 + assert len(hashed.stdout.strip()) == 40 + + assert repository.git_text(repo, "symbolic-ref", "--short", "HEAD") == "main" + + +@pytest.mark.skipif(os.name != "posix", reason="the fake git is a POSIX script") +def test_an_unanswered_read_refuses_instead_of_reporting_clean( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + repo = _repository(tmp_path) + heartbeats = tmp_path / "heartbeats" + heartbeats.write_text("", encoding="utf-8") + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + fake = bin_dir / "git" + fake.write_text( + "#!/bin/sh\n" + f"while :; do echo tick >> {shlex.quote(str(heartbeats))}; sleep 0.05; done\n", + encoding="utf-8", + ) + fake.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setattr(repository, "GIT_READ_TIMEOUT_SECONDS", 1.0) + + started = time.monotonic() + with pytest.raises( + repository.RepositoryChangeWindowError, match="did not answer within" + ): + repository.repository_has_changes(repo) + elapsed = time.monotonic() - started + assert elapsed < 6, f"an unanswered read held the caller for {elapsed:.1f}s" + + retired = heartbeats.stat().st_size + assert retired > 0, "the fixture never ran, so this proves nothing" + time.sleep(0.5) + assert heartbeats.stat().st_size == retired, ( + "the helper and its child outlived the read budget" + ) + + +def test_a_read_that_cannot_start_still_raises_oserror( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + repo = _repository(tmp_path) + + def cannot_start(command, **kwargs): + raise OSError("git is not installed") + + monkeypatch.setattr(repository.subprocess, "Popen", cannot_start) + + with pytest.raises(OSError, match="git is not installed"): + repository.repository_has_changes(repo) + +class _WindowsNameProxy: + """Report Windows only to the runtime under test, keeping pathlib host-native.""" + + def __init__(self, module): + self._module = module + + def __getattr__(self, name): + return getattr(self._module, name) + + @property + def name(self): + return "nt" + + +_TASKKILL_FAILURES = ( + "#!/bin/sh\nwhile :; do :; done\n", + "#!/bin/sh\nexit 1\n", +) + + +@pytest.mark.skipif(os.name != "posix", reason="the fake binaries are POSIX scripts") +@pytest.mark.parametrize("taskkill_script", _TASKKILL_FAILURES) +def test_a_failed_windows_cleanup_still_returns_the_read_timeout( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + taskkill_script: str, +) -> None: + """A stuck or refused taskkill must not replace the read's own timeout.""" + + repo = _repository(tmp_path) + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + (bin_dir / "git").write_text("#!/bin/sh\nwhile :; do :; done\n", encoding="utf-8") + (bin_dir / "git").chmod(0o755) + taskkill = bin_dir / "taskkill" + taskkill.write_text(taskkill_script, encoding="utf-8") + taskkill.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setattr(process_runtime, "os", _WindowsNameProxy(os)) + monkeypatch.setattr(repository, "GIT_READ_TIMEOUT_SECONDS", 0.25) + + started = time.monotonic() + with pytest.raises( + repository.RepositoryChangeWindowError, match="did not answer within" + ): + repository.repository_has_changes(repo) + assert time.monotonic() - started < 3 + + +@pytest.mark.skipif(os.name != "posix", reason="the fake binaries are POSIX scripts") +@pytest.mark.parametrize("taskkill_script", _TASKKILL_FAILURES) +def test_a_failed_windows_cleanup_still_propagates_cancellation( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + taskkill_script: str, +) -> None: + """Ctrl-C keeps travelling even when Windows termination is unconfirmed.""" + + repo = _repository(tmp_path) + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + (bin_dir / "git").write_text("#!/bin/sh\nwhile :; do :; done\n", encoding="utf-8") + (bin_dir / "git").chmod(0o755) + taskkill = bin_dir / "taskkill" + taskkill.write_text(taskkill_script, encoding="utf-8") + taskkill.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setattr(process_runtime, "os", _WindowsNameProxy(os)) + + def interrupted(self, timeout=None, input=None): + raise KeyboardInterrupt + + monkeypatch.setattr(subprocess.Popen, "communicate", interrupted) + + started = time.monotonic() + with pytest.raises(KeyboardInterrupt): + repository.repository_has_changes(repo) + assert time.monotonic() - started < 3