From 4db8702fd1285dd81fe6f79e39a529f8882fc10e Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Fri, 6 Nov 2026 07:28:49 +0800 Subject: [PATCH 1/3] fix(doctor): hold every Git read to one budget `doctor` inspects a checkout the operator may not control, and none of its six Git reads had a budget: `rev-parse` twice, the metadata runner behind `git_metadata_for_root`, `merge-base --is-ancestor`, `remote`, `remote get-url` and `rev-parse --verify`. A repository blocked on a stale index lock, a network mount or a very large history therefore held the diagnostic open with no output at all. Reproduced with a fake `git` on PATH that only sleeps: `_is_git_worktree_root` was still running after 5s. All six reads now go through `_run_git`, bounded by `GIT_PROBE_TIMEOUT_SECONDS`. The helper answers `None` when the budget runs out, which is the answer every caller already treats as "unknown": root detection fails closed, metadata fields stay `None`, revision relation stays `UNKNOWN`, and the trusted release ref is refused. No new failure mode, and the diagnostic still returns instead of raising. The same class in `loopx/contract.py` was bounded in #8152. Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/doctor_git.py | 137 ++++++++++---------------- tests/test_doctor_git_probe_budget.py | 85 ++++++++++++++++ 2 files changed, 137 insertions(+), 85 deletions(-) create mode 100644 tests/test_doctor_git_probe_budget.py diff --git a/loopx/doctor_git.py b/loopx/doctor_git.py index c8101f8628..1a5c04e953 100644 --- a/loopx/doctor_git.py +++ b/loopx/doctor_git.py @@ -11,6 +11,12 @@ from urllib.parse import urlsplit +# `doctor` inspects a checkout the operator may not control, so every Git read +# below runs under one budget. A repository blocked on a stale index lock, a +# network mount or a very large history must not hold the diagnostic open. +GIT_PROBE_TIMEOUT_SECONDS = 10.0 + + class GitRevisionRelation(str, Enum): SAME = "same" INSTALLED_AHEAD = "installed_ahead" @@ -27,24 +33,42 @@ def _source_git_environment() -> dict[str, str]: return env +def _run_git( + source_root: Path, *arguments: str +) -> subprocess.CompletedProcess[str] | None: + """Answer one local Git query, or None when Git cannot answer in budget. + + Every caller already treats a missing answer as "unknown" rather than as + evidence, so an exhausted budget degrades along that existing path instead + of raising out of a read-only diagnostic. + """ + + try: + return subprocess.run( + ["git", "-C", str(source_root), *arguments], + check=False, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + env=_source_git_environment(), + timeout=GIT_PROBE_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.SubprocessError): + return None + + def _is_git_worktree_root(source_root: Path) -> bool: """Reject Git's upward discovery while accepting linked worktree gitfiles.""" if not (source_root / ".git").exists(): return False - try: - result = subprocess.run( - ["git", "-C", str(source_root), "rev-parse", "--show-toplevel"], - check=False, capture_output=True, text=True, - encoding="utf-8", errors="replace", - env=_source_git_environment(), - ) - return ( - result.returncode == 0 - and bool(result.stdout.strip()) - and Path(result.stdout.strip()).resolve() == source_root - ) - except OSError: + result = _run_git(source_root, "rev-parse", "--show-toplevel") + if result is None or result.returncode != 0: + return False + reported = result.stdout.strip() + if not reported: return False + return Path(reported).resolve() == source_root def git_metadata_for_root(root: Path | None) -> dict[str, Any]: @@ -68,19 +92,8 @@ def git_metadata_for_root(root: Path | None) -> dict[str, Any]: } def _run(args: list[str]) -> str | None: - try: - result = subprocess.run( - ["git", "-C", str(source_root), *args], - check=False, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - env=_source_git_environment(), - ) - except OSError: - return None - if result.returncode != 0: + result = _run_git(source_root, *args) + if result is None or result.returncode != 0: return None return result.stdout.strip() @@ -124,25 +137,10 @@ def git_revision_relation( return GitRevisionRelation.UNKNOWN def _is_ancestor(ancestor: str, descendant: str) -> bool | None: - try: - result = subprocess.run( - [ - "git", - "-C", - str(source_root), - "merge-base", - "--is-ancestor", - ancestor, - descendant, - ], - check=False, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - env=_source_git_environment(), - ) - except OSError: + result = _run_git( + source_root, "merge-base", "--is-ancestor", ancestor, descendant + ) + if result is None: return None if result.returncode == 0: return True @@ -208,36 +206,16 @@ def trusted_release_ref_for_root( if not _is_git_worktree_root(source_root): return None - try: - remotes = subprocess.run( - ["git", "-C", str(source_root), "remote"], - check=False, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - env=_source_git_environment(), - ) - except OSError: - return None - if remotes.returncode != 0: + remotes = _run_git(source_root, "remote") + if remotes is None or remotes.returncode != 0: return None for remote in remotes.stdout.splitlines(): remote = remote.strip() if not remote: continue - try: - remote_url = subprocess.run( - ["git", "-C", str(source_root), "remote", "get-url", remote], - check=False, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - env=_source_git_environment(), - ) - except OSError: + remote_url = _run_git(source_root, "remote", "get-url", remote) + if remote_url is None: continue if ( remote_url.returncode != 0 @@ -246,23 +224,12 @@ def trusted_release_ref_for_root( ): continue trusted_ref = f"refs/remotes/{remote}/{expected_ref}" - resolved = subprocess.run( - [ - "git", - "-C", - str(source_root), - "rev-parse", - "--verify", - f"{trusted_ref}^{{commit}}", - ], - check=False, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - env=_source_git_environment(), + resolved = _run_git(source_root, "rev-parse", "--verify", f"{trusted_ref}^{{commit}}") + commit = ( + resolved.stdout.strip() + if resolved is not None and resolved.returncode == 0 + else "" ) - commit = resolved.stdout.strip() if resolved.returncode == 0 else "" if commit: return { "label": f"{expected_repository}@{expected_ref}", diff --git a/tests/test_doctor_git_probe_budget.py b/tests/test_doctor_git_probe_budget.py new file mode 100644 index 0000000000..722368c8a5 --- /dev/null +++ b/tests/test_doctor_git_probe_budget.py @@ -0,0 +1,85 @@ +from __future__ import annotations + +import os +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +from loopx import doctor_git + + +def _repo(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + (repo / ".git").mkdir(parents=True) + return repo + + +def test_git_answers_degrade_when_the_budget_runs_out( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A Git that never answers leaves the diagnostic saying "unknown".""" + + repo = _repo(tmp_path) + + def always_timeout(command, **kwargs): + raise subprocess.TimeoutExpired(command, kwargs.get("timeout")) + + monkeypatch.setattr(doctor_git.subprocess, "run", always_timeout) + + assert doctor_git._is_git_worktree_root(repo) is False + assert doctor_git.git_metadata_for_root(repo) == { + "root": str(repo.resolve()), + "git_commit": None, + "git_ref": None, + "git_dirty": None, + } + assert ( + doctor_git.git_revision_relation( + repo, installed_commit="a" * 40, comparison_commit="b" * 40 + ) + is doctor_git.GitRevisionRelation.UNKNOWN + ) + assert ( + doctor_git.trusted_release_ref_for_root( + repo, repository="example/project", ref="main" + ) + is None + ) + + +@pytest.mark.skipif(os.name != "posix", reason="the fake git is a POSIX script") +def test_doctor_finishes_when_git_blocks( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A blocked Git must not hold doctor open: every entry point returns.""" + + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + fake = bin_dir / "git" + fake.write_text(f"#!{sys.executable}\nimport time\ntime.sleep(30)\n", encoding="utf-8") + fake.chmod(0o755) + repo = _repo(tmp_path) + + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setattr(doctor_git, "GIT_PROBE_TIMEOUT_SECONDS", 0.3) + + started = time.monotonic() + assert doctor_git._is_git_worktree_root(repo) is False + assert doctor_git.git_metadata_for_root(repo)["git_dirty"] is None + assert ( + doctor_git.git_revision_relation( + repo, installed_commit="a" * 40, comparison_commit="b" * 40 + ) + is doctor_git.GitRevisionRelation.UNKNOWN + ) + assert ( + doctor_git.trusted_release_ref_for_root( + repo, repository="example/project", ref="main" + ) + is None + ) + elapsed = time.monotonic() - started + assert elapsed < 20, f"a blocked Git held doctor for {elapsed:.1f}s" From 4e8a992abc8ca2e8198c863442bb042f83d4d800 Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Fri, 6 Nov 2026 09:19:56 +0800 Subject: [PATCH 2/3] fix(doctor): retire the probe's own group and keep a stale manifest out Two gaps the review found in the ten-second budget. `subprocess.run(timeout=...)` retires the Git leader only. Git starts its helpers -- fsmonitor hooks, pagers, credential helpers -- inside the group it runs in, so a blocked `status` left the hook and its child behind and repeated diagnostics accumulated them. The query now launches in a session of its own, and an exhausted budget retires that group through the existing `terminate_process_tree` before dropping the pipes instead of draining them: an inherited pipe outlives the leader, so a drain would wait on work this call no longer owns. Reproduced with a `core.fsmonitor` hook that never answers and keeps a child writing heartbeats -- leader-only kill left it growing, the group retirement stops it. `_is_git_worktree_root` answered False both for "there is no Git here" and for "there is Git here and it did not answer". `release_manifest._source_metadata` stood an older `release.json` in for the second case, so a checkout at commit B that carried a manifest claiming A wrote A back as its own source. The two cases are separate states now, and only a checkout with no `.git` of its own falls back to the recorded manifest; an unanswered read stays unknown. Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/doctor_git.py | 145 ++++++++++++++---- loopx/release_manifest.py | 10 +- tests/test_distribution_runtime_identity.py | 4 + tests/test_doctor_git_encoding.py | 37 ++++- tests/test_doctor_git_probe_budget.py | 81 +++++++++- tests/test_release_manifest_unanswered_git.py | 98 ++++++++++++ 6 files changed, 330 insertions(+), 45 deletions(-) create mode 100644 tests/test_release_manifest_unanswered_git.py diff --git a/loopx/doctor_git.py b/loopx/doctor_git.py index 1a5c04e953..1a74333992 100644 --- a/loopx/doctor_git.py +++ b/loopx/doctor_git.py @@ -5,11 +5,14 @@ import os import re import subprocess +from dataclasses import dataclass from enum import Enum from pathlib import Path from typing import Any from urllib.parse import urlsplit +from .extensions.process_runtime import terminate_process_tree + # `doctor` inspects a checkout the operator may not control, so every Git read # below runs under one budget. A repository blocked on a stale index lock, a @@ -17,6 +20,23 @@ GIT_PROBE_TIMEOUT_SECONDS = 10.0 +@dataclass(frozen=True) +class GitAnswer: + """One finished Git query: the only fields the callers below read.""" + + returncode: int + stdout: str + stderr: str + + +class GitSourceState(str, Enum): + """What one checkout says about owning a Git identity of its own.""" + + WORKTREE = "worktree" + ABSENT = "absent" + UNCONFIRMED = "unconfirmed" + + class GitRevisionRelation(str, Enum): SAME = "same" INSTALLED_AHEAD = "installed_ahead" @@ -33,63 +53,113 @@ def _source_git_environment() -> dict[str, str]: return env +def _retire_owned_probe(process: subprocess.Popen[str]) -> None: + """Stop the query's own process group, then drop its pipes unread. + + Git runs helpers -- fsmonitor hooks, pagers, credential helpers -- inside + the group it starts in, so killing the leader alone leaves them behind and + repeated diagnostics accumulate them. An inherited pipe also survives that + kill, so a drained ``communicate`` would wait on work nobody owns any more; + the pipes are closed instead. + """ + + try: + terminate_process_tree(process, grace_seconds=0.2) + except (OSError, RuntimeError): + # The answer is already unknown; an unconfirmed cleanup must not turn a + # read-only diagnostic into a traceback. + pass + finally: + for stream in (process.stdout, process.stderr): + if stream is not None: + stream.close() + try: + process.wait(timeout=1.0) + except subprocess.TimeoutExpired: + pass + + def _run_git( source_root: Path, *arguments: str -) -> subprocess.CompletedProcess[str] | None: +) -> GitAnswer | None: """Answer one local Git query, or None when Git cannot answer in budget. Every caller already treats a missing answer as "unknown" rather than as evidence, so an exhausted budget degrades along that existing path instead - of raising out of a read-only diagnostic. + of raising out of a read-only diagnostic. The query is launched in a + session of its own so the budget can retire the helpers it started, not + just the Git process this call happens to hold. """ + argv = ["git", "-C", str(source_root), *arguments] try: - return subprocess.run( - ["git", "-C", str(source_root), *arguments], - check=False, - capture_output=True, + process = subprocess.Popen( + argv, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, encoding="utf-8", errors="replace", env=_source_git_environment(), - timeout=GIT_PROBE_TIMEOUT_SECONDS, + start_new_session=os.name == "posix", ) - except (OSError, subprocess.SubprocessError): + except OSError: return None + try: + stdout, stderr = process.communicate(timeout=GIT_PROBE_TIMEOUT_SECONDS) + except subprocess.TimeoutExpired: + _retire_owned_probe(process) + return None + return GitAnswer( + returncode=process.returncode, stdout=stdout, stderr=stderr + ) -def _is_git_worktree_root(source_root: Path) -> bool: - """Reject Git's upward discovery while accepting linked worktree gitfiles.""" +def _git_source_state(source_root: Path) -> GitSourceState: + """Reject Git's upward discovery while accepting linked worktree gitfiles. + + A checkout that carries its own ``.git`` is a Git source even when this run + cannot read it. Callers that stand something else in for a missing answer + have to tell that case apart from "there is no Git here at all". + """ + if not (source_root / ".git").exists(): - return False + return GitSourceState.ABSENT result = _run_git(source_root, "rev-parse", "--show-toplevel") if result is None or result.returncode != 0: - return False + return GitSourceState.UNCONFIRMED reported = result.stdout.strip() if not reported: - return False - return Path(reported).resolve() == source_root + return GitSourceState.UNCONFIRMED + if Path(reported).resolve() != source_root: + return GitSourceState.ABSENT + return GitSourceState.WORKTREE -def git_metadata_for_root(root: Path | None) -> dict[str, Any]: +def _is_git_worktree_root(source_root: Path) -> bool: + return _git_source_state(source_root) is GitSourceState.WORKTREE + + +def git_source_metadata( + root: Path | None, +) -> tuple[dict[str, Any], GitSourceState]: + """Return the identity a root claims and how far this run could read it. + + The pair matters because "this checkout has no Git identity" and "this + checkout has one that did not answer in time" call for opposite behaviour + from callers that have something else to fall back on. + """ + + unknown = {"root": None, "git_commit": None, "git_ref": None, "git_dirty": None} if root is None: - return { - "root": None, - "git_commit": None, - "git_ref": None, - "git_dirty": None, - } + return unknown, GitSourceState.ABSENT try: source_root = root.expanduser().resolve() except OSError: source_root = root.expanduser() - if not _is_git_worktree_root(source_root): - return { - "root": str(source_root), - "git_commit": None, - "git_ref": None, - "git_dirty": None, - } + state = _git_source_state(source_root) + if state is not GitSourceState.WORKTREE: + return {**unknown, "root": str(source_root)}, state def _run(args: list[str]) -> str | None: result = _run_git(source_root, *args) @@ -103,12 +173,19 @@ def _run(args: list[str]) -> str | None: status = _run(["status", "--porcelain"]) # Empty successful output proves clean; a failed status leaves it unknown. dirty = None if status is None else bool(status) - return { - "root": str(source_root), - "git_commit": commit, - "git_ref": branch or tag, - "git_dirty": dirty, - } + return ( + { + "root": str(source_root), + "git_commit": commit, + "git_ref": branch or tag, + "git_dirty": dirty, + }, + state, + ) + + +def git_metadata_for_root(root: Path | None) -> dict[str, Any]: + return git_source_metadata(root)[0] def git_revision_relation( diff --git a/loopx/release_manifest.py b/loopx/release_manifest.py index b212a39579..dc9d7688ca 100644 --- a/loopx/release_manifest.py +++ b/loopx/release_manifest.py @@ -9,7 +9,7 @@ from typing import Any from . import __version__ -from .doctor_git import git_metadata_for_root +from .doctor_git import GitSourceState, git_source_metadata RELEASE_MANIFEST_SCHEMA_VERSION = "loopx_release_manifest_v0" @@ -126,9 +126,13 @@ def _manifest_source_metadata(source_root: Path | None) -> dict[str, Any]: def _source_metadata(source_root: Path | None) -> dict[str, Any]: - git_metadata = git_metadata_for_root(source_root) + git_metadata, git_state = git_source_metadata(source_root) git_metadata.pop("root") - if any(git_metadata.get(key) is not None for key in ("git_commit", "git_ref", "git_dirty")): + if git_state is not GitSourceState.ABSENT: + # A checkout with a `.git` of its own is its own source of truth. When + # Git cannot answer this run, the `release.json` next to it describes an + # earlier install, so it must not be written back as this one: an + # unanswered read stays unknown instead of claiming the old commit. return { **git_metadata, "kind": None, diff --git a/tests/test_distribution_runtime_identity.py b/tests/test_distribution_runtime_identity.py index 54b7131043..5aeec6a38c 100644 --- a/tests/test_distribution_runtime_identity.py +++ b/tests/test_distribution_runtime_identity.py @@ -63,6 +63,10 @@ def test_real_pip_compiled_and_uncompiled_wheel_have_the_same_identity(tmp_path: "loopx/__init__.py": Path(manifest.__file__).with_name("__init__.py").read_text(), "loopx/release_manifest.py": Path(manifest.__file__).read_text(), "loopx/doctor_git.py": Path(manifest.__file__).with_name("doctor_git.py").read_text(), + "loopx/extensions/__init__.py": "", + "loopx/extensions/process_runtime.py": Path(manifest.__file__).with_name( + "extensions" + ).joinpath("process_runtime.py").read_text(), f"{metadata}/METADATA": f"Metadata-Version: 2.1\nName: loopx\nVersion: {version}\n", f"{metadata}/WHEEL": "Wheel-Version: 1.0\nGenerator: test\nRoot-Is-Purelib: true\nTag: py3-none-any\n", } diff --git a/tests/test_doctor_git_encoding.py b/tests/test_doctor_git_encoding.py index d8a20c4602..4ae3af15dc 100644 --- a/tests/test_doctor_git_encoding.py +++ b/tests/test_doctor_git_encoding.py @@ -85,7 +85,9 @@ def install(*, returncode=0, fail_at=None): def run(command, **kwargs): args = command[3:] if args == ["rev-parse", "--show-toplevel"]: - return SimpleNamespace(returncode=0, stdout=str(tmp_path) + "\n") + return SimpleNamespace( + returncode=0, stdout=str(tmp_path) + "\n", stderr="" + ) calls.append(args) code = ( returncode[len(calls) - 1] @@ -111,10 +113,39 @@ def run(command, **kwargs): f"sys.exit({code})" ) return subprocess.run( - [sys.executable, "-c", script], check=kwargs.pop("check"), **kwargs + [sys.executable, "-c", script], + check=kwargs.pop("check", False), + **kwargs, ) - monkeypatch.setattr(doctor_git, "subprocess", SimpleNamespace(run=run)) + class FakeProcess: + """The fake ``run`` above already finished the query synchronously.""" + + def __init__(self, command, **kwargs): + kwargs.pop("start_new_session", None) + result = run(command, **kwargs) + self.args = command + self.returncode = result.returncode + self.stdout = getattr(result, "stdout", "") + self.stderr = getattr(result, "stderr", "") + + def communicate(self, timeout=None): + return self.stdout, self.stderr + + def wait(self, timeout=None): + return self.returncode + + def poll(self): + return self.returncode + + def kill(self): + return None + + monkeypatch.setattr( + doctor_git, + "subprocess", + SimpleNamespace(run=run, Popen=FakeProcess, PIPE=subprocess.PIPE), + ) return calls return install diff --git a/tests/test_doctor_git_probe_budget.py b/tests/test_doctor_git_probe_budget.py index 722368c8a5..9602bc092c 100644 --- a/tests/test_doctor_git_probe_budget.py +++ b/tests/test_doctor_git_probe_budget.py @@ -1,6 +1,7 @@ from __future__ import annotations import os +import shlex import subprocess import sys import time @@ -11,23 +12,37 @@ from loopx import doctor_git +def _worktree(tmp_path: Path) -> Path: + """A real single-commit repository, so Git really reads an index.""" + + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", str(repo)], check=True) + subprocess.run(["git", "-C", str(repo), "config", "user.email", "f@e.invalid"], check=True) + subprocess.run(["git", "-C", str(repo), "config", "user.name", "Fixture"], check=True) + (repo / "tracked.txt").write_text("original\n", encoding="utf-8") + subprocess.run(["git", "-C", str(repo), "add", "tracked.txt"], check=True) + subprocess.run(["git", "-C", str(repo), "commit", "-qm", "fixture"], check=True) + return repo + + def _repo(tmp_path: Path) -> Path: repo = tmp_path / "repo" (repo / ".git").mkdir(parents=True) return repo -def test_git_answers_degrade_when_the_budget_runs_out( +def test_a_git_that_cannot_start_degrades_to_unknown( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """A Git that never answers leaves the diagnostic saying "unknown".""" + """A missing Git leaves the diagnostic saying "unknown", not raising.""" repo = _repo(tmp_path) - def always_timeout(command, **kwargs): - raise subprocess.TimeoutExpired(command, kwargs.get("timeout")) + def cannot_start(command, **kwargs): + raise OSError("git is not installed") - monkeypatch.setattr(doctor_git.subprocess, "run", always_timeout) + monkeypatch.setattr(doctor_git.subprocess, "Popen", cannot_start) assert doctor_git._is_git_worktree_root(repo) is False assert doctor_git.git_metadata_for_root(repo) == { @@ -83,3 +98,59 @@ def test_doctor_finishes_when_git_blocks( ) elapsed = time.monotonic() - started assert elapsed < 20, f"a blocked Git held doctor for {elapsed:.1f}s" + + +@pytest.mark.skipif(os.name != "posix", reason="core.fsmonitor needs a POSIX hook") +def test_a_blocked_query_takes_the_helpers_it_started( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Git's own helpers are in its group, and the budget has to retire them. + + The hook here is an fsmonitor that never answers: Git waits on it, and the + hook keeps a child writing heartbeats. Killing only the Git process leaves + both alive, so the heartbeat is what tells the two outcomes apart. + """ + + repo = _worktree(tmp_path) + heartbeats = tmp_path / "heartbeats" + heartbeats.write_text("", encoding="utf-8") + hook = repo / ".git" / "blocking-fsmonitor" + hook.write_text( + "#!/bin/sh\n" + f"while :; do echo tick >> {shlex.quote(str(heartbeats))}; sleep 0.1; done\n", + encoding="utf-8", + ) + hook.chmod(0o755) + subprocess.run( + ["git", "-C", str(repo), "config", "core.fsmonitor", str(hook)], check=True + ) + + monkeypatch.setattr(doctor_git, "GIT_PROBE_TIMEOUT_SECONDS", 0.8) + started = time.monotonic() + assert doctor_git._run_git(repo, "status", "--porcelain") is None + assert time.monotonic() - started < 6 + + retired = heartbeats.stat().st_size + assert retired > 0, "the fixture hook never ran, so this proves nothing" + time.sleep(0.6) + assert heartbeats.stat().st_size == retired, ( + "the fsmonitor hook and its child outlived the query budget" + ) + + +def test_an_unanswered_root_probe_is_not_reported_as_absent( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A checkout with its own `.git` stays a Git source when Git is silent.""" + + repo = _repo(tmp_path) + monkeypatch.setattr(doctor_git, "GIT_PROBE_TIMEOUT_SECONDS", 0.3) + monkeypatch.setattr(doctor_git, "_run_git", lambda *args, **kwargs: None) + + metadata, state = doctor_git.git_source_metadata(repo) + + assert state is doctor_git.GitSourceState.UNCONFIRMED + assert metadata["git_commit"] is None + plain = tmp_path / "plain" + plain.mkdir() + assert doctor_git.git_source_metadata(plain)[1] is doctor_git.GitSourceState.ABSENT diff --git a/tests/test_release_manifest_unanswered_git.py b/tests/test_release_manifest_unanswered_git.py new file mode 100644 index 0000000000..2fce505dae --- /dev/null +++ b/tests/test_release_manifest_unanswered_git.py @@ -0,0 +1,98 @@ +"""An unanswered Git read keeps its own answer; an older release.json is another install.""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +from loopx import doctor_git +from loopx.release_manifest import write_release_manifest + +STALE_COMMIT = "a" * 40 + + +def _checkout(tmp_path: Path) -> Path: + repo = tmp_path / "checkout" + repo.mkdir() + for command in ( + ["git", "init", "-q", str(repo)], + ["git", "-C", str(repo), "config", "user.email", "fixture@example.invalid"], + ["git", "-C", str(repo), "config", "user.name", "Source Fixture"], + ): + subprocess.run(command, check=True) + (repo / "tracked.txt").write_text("current\n", encoding="utf-8") + subprocess.run(["git", "-C", str(repo), "add", "tracked.txt"], check=True) + subprocess.run(["git", "-C", str(repo), "commit", "-qm", "current"], check=True) + return repo + + +def _head(repo: Path) -> str: + return subprocess.run( + ["git", "-C", str(repo), "rev-parse", "HEAD"], + check=True, capture_output=True, encoding="utf-8", + ).stdout.strip() + + +def _source_manifest(release_root: Path, source_root: Path) -> dict[str, object]: + release_root.mkdir() + path = write_release_manifest( + release_root=release_root, release_id="fixture", source_root=source_root, env={}, + ) + manifest = json.loads(path.read_text(encoding="utf-8")) + return manifest["source"] + + +def test_a_stale_manifest_does_not_stand_in_for_an_unanswered_root_probe( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + repo = _checkout(tmp_path) + head = _head(repo) + (repo / "release.json").write_text( + json.dumps({ + "source": { + "kind": "github_archive", + "git_commit": STALE_COMMIT, + "git_ref": "v0.9.0", + "git_dirty": False, + "ref": "v0.9.0", + } + }), + encoding="utf-8", + ) + + healthy = _source_manifest(tmp_path / "release-healthy", repo) + assert healthy["git_commit"] == head + assert healthy["git_ref"] is not None + + real_git = shutil.which("git") + assert real_git is not None + slow_bin = tmp_path / "slow-bin" + slow_bin.mkdir() + wrapper = slow_bin / "git" + wrapper.write_text( + "#!/bin/sh\n" + 'for argument in "$@"; do\n' + ' if [ "$argument" = "--show-toplevel" ]; then sleep 30; fi\n' + "done\n" + f'exec {shlex_quote(real_git)} "$@"\n', + encoding="utf-8", + ) + wrapper.chmod(0o755) + monkeypatch.setenv("PATH", f"{slow_bin}{os.pathsep}{os.environ['PATH']}") + monkeypatch.setattr(doctor_git, "GIT_PROBE_TIMEOUT_SECONDS", 0.5) + + delayed = _source_manifest(tmp_path / "release-delayed", repo) + + assert delayed["git_commit"] != STALE_COMMIT + assert delayed["git_commit"] is None + assert delayed["git_dirty"] is None + assert delayed["kind"] == "local_checkout" + + +def shlex_quote(value: str) -> str: + return "'" + value.replace("'", "'\"'\"'") + "'" From 28ded5091c3f1c0ba127d2d4e927e08d5f97ef2d Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:39:35 +0800 Subject: [PATCH 3/3] fix(doctor): retire the probe's session when the diagnostic is cancelled The budget covered an exhausted wait, not an interrupted one. Git runs in a session of its own so the budget can retire its helpers, which also means a terminal Ctrl-C -- a SIGINT to the process group of the running `doctor` -- never reaches Git. `communicate` raised `KeyboardInterrupt` out of the only place that owns that session, the diagnostic exited, and the fsmonitor hook and its child kept running. Repeated cancellations accumulate the same background work the timeout fix removed. The wait now retires the owned session on any interrupted exit and re-raises the original interruption, so an exhausted budget keeps answering "unknown" while Ctrl-C stays Ctrl-C. Reproduced with the manifest writer CLI, real Git and a real `core.fsmonitor` hook that keeps a child writing heartbeats: interrupting the writer's group left the heartbeat growing on the previous head and stops it now. Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/doctor_git.py | 11 +++- tests/test_doctor_git_probe_budget.py | 77 +++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/loopx/doctor_git.py b/loopx/doctor_git.py index 1a74333992..df09510587 100644 --- a/loopx/doctor_git.py +++ b/loopx/doctor_git.py @@ -88,7 +88,9 @@ def _run_git( evidence, so an exhausted budget degrades along that existing path instead of raising out of a read-only diagnostic. The query is launched in a session of its own so the budget can retire the helpers it started, not - just the Git process this call happens to hold. + just the Git process this call happens to hold. Cancelling the diagnostic + (a terminal Ctrl-C) reaches this process's group and not that session, so + an interrupted wait retires the same helpers and then keeps travelling. """ argv = ["git", "-C", str(source_root), *arguments] @@ -110,6 +112,13 @@ def _run_git( except subprocess.TimeoutExpired: _retire_owned_probe(process) return None + except BaseException: + # The wait is over without Git answering: an operator cancelled the + # diagnostic. Retiring here is the only owner this session has, and the + # original cancellation still has to reach its own handler, so the + # timeout keeps answering "unknown" while Ctrl-C keeps being Ctrl-C. + _retire_owned_probe(process) + raise return GitAnswer( returncode=process.returncode, stdout=stdout, stderr=stderr ) diff --git a/tests/test_doctor_git_probe_budget.py b/tests/test_doctor_git_probe_budget.py index 9602bc092c..c9f5ed8618 100644 --- a/tests/test_doctor_git_probe_budget.py +++ b/tests/test_doctor_git_probe_budget.py @@ -2,6 +2,7 @@ import os import shlex +import signal import subprocess import sys import time @@ -154,3 +155,79 @@ def test_an_unanswered_root_probe_is_not_reported_as_absent( plain = tmp_path / "plain" plain.mkdir() assert doctor_git.git_source_metadata(plain)[1] is doctor_git.GitSourceState.ABSENT + + +@pytest.mark.skipif(os.name != "posix", reason="a terminal Ctrl-C is a POSIX group signal") +def test_a_cancelled_query_takes_the_helpers_it_started(tmp_path: Path) -> None: + """Ctrl-C during a query retires the session that query owns. + + The manifest writer is the terminal entry point that reads a source root's + identity, and it is what an install run calls. Start it as a terminal + would, let real Git block on a real ``core.fsmonitor`` hook, then send the + interrupt to the writer's own process group. Git runs in a session of its + own, so the interrupt never reaches it: only the writer can retire it. + """ + + repo = _worktree(tmp_path) + heartbeats = tmp_path / "heartbeats" + heartbeats.write_text("", encoding="utf-8") + stop = tmp_path / "stop" + hook = repo / ".git" / "blocking-fsmonitor" + hook.write_text( + "#!/bin/sh\n" + f"while [ ! -f {shlex.quote(str(stop))} ]; do\n" + f" echo tick >> {shlex.quote(str(heartbeats))}\n" + " sleep 0.1\n" + "done\n", + encoding="utf-8", + ) + hook.chmod(0o755) + subprocess.run( + ["git", "-C", str(repo), "config", "core.fsmonitor", str(hook)], check=True + ) + release_root = tmp_path / "release" + release_root.mkdir() + + environment = { + key: value + for key, value in os.environ.items() + if not key.startswith(("PYTEST", "COVERAGE", "COV_CORE")) + } + environment.update({ + "PYTHONPATH": str(Path(__file__).resolve().parents[1]), + "LOOPX_USAGE_PING": "0", + }) + writer = subprocess.Popen( + [ + sys.executable, "-m", "loopx.release_manifest", str(release_root), + "--release-id", "cancelled", "--source-root", str(repo), + ], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + env=environment, + start_new_session=True, + ) + try: + deadline = time.monotonic() + 20 + while heartbeats.stat().st_size == 0 and time.monotonic() < deadline: + time.sleep(0.05) + running = heartbeats.stat().st_size + assert running > 0, "the fixture hook never ran, so this proves nothing" + assert writer.poll() is None, "the writer had already returned" + os.killpg(os.getpgid(writer.pid), signal.SIGINT) + writer.communicate(timeout=20) + assert writer.returncode != 0 + assert not (release_root / "release.json").exists(), ( + "a cancelled run still claimed a manifest" + ) + retired = heartbeats.stat().st_size + time.sleep(0.6) + assert heartbeats.stat().st_size == retired, ( + "the fsmonitor hook and its child outlived the cancellation" + ) + finally: + stop.write_text("stop\n", encoding="utf-8") + if writer.poll() is None: + os.killpg(os.getpgid(writer.pid), signal.SIGKILL) + writer.communicate(timeout=20)