fix(update): a timed-out update child never hangs on a grandchild's pipes (review N14)

What: update_custody.run's in-update (Windows) branch kills the child's tree (taskkill /T /F) on a
timeout, then drains its output for at most 5 s and detaches pipes a survivor still holds.
Why: it killed only git.exe and waited unbounded on git-remote-https, which inherits the pipes.
Red/green: pre-fix run(sh -c 'sleep 20 & sleep 20', timeout=0.5) -> AssertionError (waited); fixed -> passed.
This commit is contained in:
Teknium
2026-10-04 23:06:17 -07:00
parent d23ddec6d5
commit 1057d2111e
2 changed files with 37 additions and 1 deletions

View File

@@ -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."""

View File

@@ -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"