diff --git a/hermes_cli/update_custody.py b/hermes_cli/update_custody.py index aab25efdfa..975f83f84f 100644 --- a/hermes_cli/update_custody.py +++ b/hermes_cli/update_custody.py @@ -262,8 +262,14 @@ def run(argv: Sequence[str], *, inherit_lock: bool = False, **kwargs) -> subproc try: stdout, stderr = proc.communicate(input, timeout=timeout) except subprocess.TimeoutExpired as exc: + _kill_tree(proc) proc.kill() - exc.stdout, exc.stderr = proc.communicate() + try: + exc.stdout, exc.stderr = proc.communicate(timeout=_DRAIN_SECONDS) + except subprocess.TimeoutExpired: + # A descendant the tree kill missed still holds the pipes: leave them to the + # reader threads (closing a pipe a thread is reading blocks) and give up on them. + proc.stdin = proc.stdout = proc.stderr = None raise except BaseException: proc.kill() @@ -274,6 +280,20 @@ def run(argv: Sequence[str], *, inherit_lock: bool = False, **kwargs) -> subproc return subprocess.CompletedProcess(proc.args, code, stdout, stderr) +# How long a timed-out child's output may still drain after its tree was killed. +_DRAIN_SECONDS = 5 + + +def _kill_tree(proc: subprocess.Popen) -> None: + """Windows: kill the child AND what it started (git.exe's git-remote-https inherits the + stdout/stderr pipes; killing git.exe alone leaves communicate() waiting on the helper). Not + TerminateJobObject: the job also holds the update's other children.""" + with contextlib.suppress(OSError, subprocess.SubprocessError): + subprocess.run(["taskkill", "/T", "/F", "/PID", str(proc.pid)], stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=10, + creationflags=getattr(subprocess, "CREATE_NO_WINDOW", 0)) + + def run_git(git_cmd: Sequence[str], args: Sequence[str], **kwargs) -> subprocess.CompletedProcess: """THE updater git runner: custody config in argv, the lock fd only into local mutators, Windows job binding inside an update. ``kwargs`` are ``subprocess.run``'s.""" diff --git a/tests/hermes_cli/test_update_custody.py b/tests/hermes_cli/test_update_custody.py index 05da483d86..3a3bfaa9bb 100644 --- a/tests/hermes_cli/test_update_custody.py +++ b/tests/hermes_cli/test_update_custody.py @@ -300,3 +300,19 @@ def test_run_git_hands_the_lock_fd_to_local_mutators_only(repo, tmp_path, monkey lock.release() assert ran.returncode == 0, ran.stderr assert out.read_text(encoding="utf-8-sig").split() == (["yes"] if holds else ["no"]), args + + +@pytest.mark.platforms("posix") +def test_a_timed_out_child_never_waits_on_a_grandchild_holding_its_pipes(monkeypatch): + """N14: the in-update (Windows) run() killed only the direct child on a timeout, then waited + unbounded for the pipes a grandchild still held (git-remote-https under a stalled fetch). + Driven here through that branch with a POSIX grandchild the tree kill (taskkill) cannot reach.""" + from hermes_cli import update_custody + + monkeypatch.setattr(update_custody.sys, "platform", "win32") + monkeypatch.setattr(update_custody, "_held", lambda: {"fd": None}) + monkeypatch.setattr(update_custody, "popen", lambda argv, **kw: subprocess.Popen(list(argv), **kw)) + started = time.monotonic() + with pytest.raises(subprocess.TimeoutExpired): + update_custody.run(["sh", "-c", "sleep 20 & sleep 20"], capture_output=True, timeout=0.5) + assert time.monotonic() - started < 15, "run() waited on a grandchild after the timeout"