fix: stop stalled remote commands when their timeout expires (#2276)

`Remote.fetch(kill_after_timeout=...)` can hang indefinitely when Git stops
writing to stderr: `AutoInterrupt._terminate()` closes the buffered stream
while its pump thread holds the read lock, before signalling the process.
`Remote.pull()` and `Remote.push()` share the same path. Killing only Git or
its direct children also leaves HTTP(S) helpers holding stderr open; Apple
Git launches the network helper through an intermediate Git process.

Move output stream closure after process termination, preserving stdin EOF
before waiting for exit. Extract the existing POSIX
watchdog process lookup into `_kill_process()` and collect descendants before
sending `SIGKILL`, reusing it for both command and remote timeouts. Store one timeout diagnostic for `AutoInterrupt.wait()` without synchronously
re-entering user callbacks. Share one
monotonic deadline between stdout and stderr rather than allowing each join
the full timeout. Ordinary `AutoInterrupt` cleanup retains `SIGTERM`.

Add a bounded loopback-server regression for fetch, pull, and push over
`git://`, HTTP, and HTTPS. All nine cases failed before the fix, taking about
five seconds until the server released the stalled connection, and now pass
in 6.66 seconds total with a 0.5-second command timeout. Extend the existing
`ps` fallback test to cover grandchildren and exclude unrelated processes.

Git behavior reference: the local Git source baseline is `v2.56.0-rc1`.
`transport-helper.c:get_helper()` sets `helper->err = 0`, inheriting stderr;
`connect.c:git_connect()` starts SSH transport children; `run-command.c`
handles inherited descriptors and only signals children marked for cleanup.
Runtime reproduction used Apple Git `2.54.0 (Apple Git-157)`.

Validation: command, cleanup, and command-deprecation tests: 119 passed,
1 skipped. Remote tests: 39 passed, 1 failed; `TestRemote.test_base` fails
identically with the unchanged `git/cmd.py` on this machine because
`refs/remotes/daemon_origin/new_branch` is missing. Ruff lint/format and
`git diff --check` pass. Pinned mypy and basedpyright pass with no issues.
Python 3.8 focused timeout/cleanup regressions: 16 passed.

Codex commit review identified that post-termination `stdin.close()` can flush
buffered writes into a broken pipe, raising `BrokenPipeError` and skipping
stdout/stderr cleanup. Reproduced with `git hash-object --stdin` and added a
regression. Suppress that expected broken pipe while closing stdin so output
streams are still closed. Both cleanup tests pass on Python 3.12; Python 3.8
cleanup plus all stalled-remote regressions: 11 passed. Ruff, mypy, and
basedpyright pass after this correction.

A further commit review reproduced ordinary cleanup hanging when a child
ignores `SIGTERM` and waits for stdin EOF. Preserve input closure before
termination, suppressing a broken pipe if timeout signalling already killed
the reader, and retain delayed output closure. A bounded subprocess regression
failed before this correction and now exits normally without its safety kill.
Python 3.8 cleanup plus stalled-remote regressions: 12 passed; mypy,
basedpyright, Ruff, and `git diff --check` pass.

Commit review also reproduced waiting on undrained output after stdin EOF,
and an unconditional post-timeout pump join waiting for a blocked callback.
Signal before output closure but reap afterward, retaining the original
nonblocking pipe cleanup order. Remove the added unconditional pump joins.
Treat `ValueError` from an already closed stream as expected cleanup, while
preserving failures from handlers. Both bounded regressions failed before
these corrections and pass afterward. Python 3.8 focused timeout/cleanup
coverage: 19 passed. Type checks and Ruff pass.

Further review reproduced timeout aborting before termination when both process
lookup utilities are unavailable, and regression setup inheriting global
commit signing. Make descendant enumeration best-effort on `OSError`, still
sending `SIGKILL` to known processes, and disable signing for the synthetic
regression commit. The missing-tools regression failed before the correction
and passes afterward. Python 3.8 focused tests: 20 passed; type checks and
Ruff pass.

PR CI's Windows Python 3.14/3.15 jobs found `signal.SIGKILL` unavailable in
Windows stubs after extracting the helper. Reproduced with
`mypy --platform win32 --python-version 3.14`; guard the POSIX signalling loop
with the same platform condition used by its callers. Windows mypy, native
mypy, basedpyright, Ruff, and `git diff --check` pass afterward.

Copilot review noted that synchronously emitting a timeout to a blocked stderr
handler still hangs, and that `Git.execute()` documented only direct-child
signalling. Store the timeout diagnostic on `AutoInterrupt` for `wait()` rather
than invoking either callback, and extend callback coverage to stderr. Both
stdout/stderr cases failed before this correction and pass afterward with the
timeout message intact. Update the public descendant limitation to describe
detachment and processes spawned after enumeration. Python 3.8 focused timeout
and cleanup coverage: 21 passed. Native and Windows mypy, basedpyright, Ruff,
and `git diff --check` pass.

Commit review reproduced a partial push losing its timeout after at least one
porcelain result when progress stderr contained no other error. Preserve the
stored timeout in `Remote._get_push_info()` when assigning `PushInfoList.error`,
so `raise_if_error()` still raises. The partial-result regression failed before
this correction and passes afterward. Python 3.8 focused timeout/cleanup tests:
22 passed. Native/Windows mypy, basedpyright, Ruff lint/format and diff checks
pass.

Cygwin CI ran all new stalled-remote cases and exposed that its `ps` rejects
POSIX `-A`/`-o` options, leaving fetch/pull HTTP(S) helpers alive. Use Cygwin's
supported `ps -ef` and select the UID-prefixed PID/PPID columns. Extend the
existing descendant parser test with representative Cygwin rows; it failed
before this fix and now passes. Cygwin upstream `winsup/utils/ps.cc` defines
that format and supports `-e`/`-f`. Native/Windows type checks and Ruff pass.

Cygwin rerun: all nine stalled-remote regressions pass. The POSIX parser
parameter alone failed because it inherited `sys.platform == "cygwin"`;
explicitly simulate Linux or Cygwin for each parser parameter. Both parser
cases, basedpyright, Ruff, and diff checks pass. Cygwin reported 1581 passed
with that single test failure before this fixture correction.

Final Copilot feedback found partial push timeouts emitting an empty warning
labelled as fetching. Warn only when stderr is captured and label it as
pushing, preserving the stored timeout in `PushInfoList.error`. The enhanced
partial-push regression failed before the correction and now passes; Ruff,
mypy, and diff checks pass. The preceding head passed all 50 CI checks.

Copilot review identified that output handling can time out after the child has
already exited successfully. In that case `AutoInterrupt.wait()` previously
returned zero despite its stored timeout marker. Normalize a successful or
unknown status to failure when that marker is present, preserving any existing
nonzero child status and reporting the timeout diagnostic. Extend the blocked
stdout/stderr regression to children that print and exit immediately; both new
cases failed before this correction and now raise with a nonzero status.
Python 3.8 focused timeout/cleanup suite: 24 passed. Native and Windows-targeted
mypy, basedpyright, Ruff lint/format, and `git diff --check` pass.

Further Copilot feedback identified that a blocked output handler can outlive an
already-reaped child, leaving an obsolete PID in its `Popen` object. Check
`poll()` before enumerating or signalling the process tree so handler timeouts
still fail without attempting to kill an exited child's potentially recycled
PID. Both exited-child callback regressions now assert that `_kill_process()`
is never called; both failed before the guard and pass afterward. Python 3.8
focused timeout/cleanup suite: 24 passed. Native and Windows-targeted mypy,
basedpyright, Ruff lint/format, and `git diff --check` pass.

Author: codex

git/cmd.py

@@ -19,6 +19,7 @@
 import sys
 from textwrap import dedent
 import threading
+import time
 import warnings
 
 from git.compat import defenc, force_bytes, safe_decode
@@ -99,6 +100,45 @@
 ## @{
 
 
+def _kill_process(pid: int) -> bool:
+    """Kill a POSIX process and its descendants, returning whether it was killed."""
+    # Collect descendants before signalling, while their parent PIDs still identify them.
+    pids = [pid]
+    for parent_pid in pids:
+        try:
+            try:
+                p = Popen(["pgrep", "-P", str(parent_pid)], stdout=PIPE)
+            except FileNotFoundError:
+                # POSIX ps does not support selecting by parent PID.
+                ps_args = ["ps", "-ef"] if sys.platform == "cygwin" else ["ps", "-A", "-o", "pid=", "-o", "ppid="]
+                with Popen(ps_args, stdout=PIPE) as p:
+                    if p.stdout is not None:
+                        for line in p.stdout:
+                            fields = line.split()
+                            if sys.platform == "cygwin":
+                                fields = fields[1:3]  # ps -ef starts with UID, PID, PPID.
+                            if len(fields) == 2 and all(field.isdigit() for field in fields):
+                                if int(fields[1]) == parent_pid:
+                                    pids.append(int(fields[0]))
+            else:
+                with p:
+                    if p.stdout is not None:
+                        for line in p.stdout:
+                            if line.strip().isdigit():
+                                pids.append(int(line))
+        except OSError as ex:
+            _logger.info("Unable to enumerate child processes: %r", ex)
+    killed = False
+    if sys.platform != "win32":
+        for process_pid in pids:
+            try:
+                os.kill(process_pid, signal.SIGKILL)
+                killed = killed or process_pid == pid
+            except OSError:
+                pass
+    return killed
+
+
 def handle_process_output(
     process: Union["Git.AutoInterrupt", Popen],
     stdout_handler: Union[
@@ -165,7 +205,7 @@ def pump_stream(
 
         except Exception as ex:
             _logger.error(f"Pumping {name!r} of cmd({remove_password_if_present(cmdline)}) failed due to: {ex!r}")
-            if "I/O operation on closed file" not in str(ex):
+            if not (isinstance(ex, ValueError) and stream.closed):
                 # Only reraise if the error was not due to the stream closing.
                 raise CommandError([f"<{name}-pump>"] + remove_password_if_present(cmdline), ex) from ex
         finally:
@@ -200,27 +240,23 @@ def pump_stream(
         threads.append(t)
 
     # FIXME: Why join? Will block if stdin needs feeding...
+    deadline = None if kill_after_timeout is None else time.monotonic() + kill_after_timeout
     for t in threads:
-        t.join(timeout=kill_after_timeout)
+        t.join(timeout=None if deadline is None else max(0, deadline - time.monotonic()))
         if t.is_alive():
             if isinstance(process, Git.AutoInterrupt):
+                if sys.platform != "win32" and process.proc is not None and process.proc.poll() is None:
+                    _kill_process(process.proc.pid)
                 process._terminate()
             else:  # Don't want to deal with the other case.
                 raise RuntimeError(
                     "Thread join() timed out in cmd.handle_process_output()."
                     f" kill_after_timeout={kill_after_timeout} seconds"
                 )
-            if stderr_handler:
-                error_str: Union[str, bytes] = (
-                    f"error: process killed because it timed out. kill_after_timeout={kill_after_timeout} seconds"
-                )
-                if not decode_streams and isinstance(p_stderr, BinaryIO):
-                    # Assume stderr_handler needs binary input.
-                    error_str = cast(str, error_str)
-                    error_str = error_str.encode()
-                # We ignore typing on the next line because mypy does not like the way
-                # we inferred that stderr takes str or bytes.
-                stderr_handler(error_str)  # type: ignore[arg-type]
+            process._timeout_error = (
+                f"error: process killed because it timed out. kill_after_timeout={kill_after_timeout} seconds"
+            )
+            break
 
     if finalizer:
         finalizer(process)
@@ -326,7 +362,7 @@ class _AutoInterrupt:
     raise.
     """
 
-    __slots__ = ("proc", "args", "status")
+    __slots__ = ("proc", "args", "status", "_timeout_error")
 
     # If this is non-zero it will override any status code during _terminate, used
     # to prevent race conditions in testing.
@@ -336,6 +372,7 @@ def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None:
         self.proc = proc
         self.args = args
         self.status: Union[int, None] = None
+        self._timeout_error: Optional[str] = None
 
     def _terminate(self) -> None:
         """Terminate the underlying process."""
@@ -344,37 +381,45 @@ def _terminate(self) -> None:
 
         proc = self.proc
         self.proc = None
-        if proc.stdin:
-            proc.stdin.close()
-        if proc.stdout:
-            proc.stdout.close()
-        if proc.stderr:
-            proc.stderr.close()
-        # Did the process finish already so we have a return code?
         try:
-            if proc.poll() is not None:
-                self.status = self._status_code_if_terminate or proc.poll()
+            if proc.stdin:
+                # A timed-out process may already have exited before input is flushed.
+                with contextlib.suppress(BrokenPipeError):
+                    proc.stdin.close()
+            # Did the process finish already so we have a return code?
+            try:
+                if proc.poll() is not None:
+                    self.status = self._status_code_if_terminate or proc.poll()
+                    return
+            except OSError as ex:
+                _logger.info("Ignored error after process had died: %r", ex)
+
+            # It can be that nothing really exists anymore...
+            if os is None or getattr(os, "kill", None) is None:
                 return
-        except OSError as ex:
-            _logger.info("Ignored error after process had died: %r", ex)
 
-        # It can be that nothing really exists anymore...
-        if os is None or getattr(os, "kill", None) is None:
-            return
+            # Try to kill it.
+            try:
+                proc.terminate()
+            except (OSError, AttributeError) as ex:
+                # On interpreter shutdown (notably on Windows), parts of the stdlib used by
+                # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None),
+                # which can cause AttributeError during terminate(). In that case, we prefer
+                # to silently ignore to avoid noisy "Exception ignored in: __del__" messages.
+                _logger.info("Ignored error while terminating process: %r", ex)
+                return
+            # END exception handling
+        finally:
+            if proc.stdout:
+                proc.stdout.close()
+            if proc.stderr:
+                proc.stderr.close()
 
-        # Try to kill it.
         try:
-            proc.terminate()
-            status = proc.wait()  # Ensure the process goes away.
-
+            status = proc.wait()
             self.status = self._status_code_if_terminate or status
         except (OSError, AttributeError) as ex:
-            # On interpreter shutdown (notably on Windows), parts of the stdlib used by
-            # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None),
-            # which can cause AttributeError during terminate(). In that case, we prefer
-            # to silently ignore to avoid noisy "Exception ignored in: __del__" messages.
-            _logger.info("Ignored error while terminating process: %r", ex)
-        # END exception handling
+            _logger.info("Ignored error while waiting for terminated process: %r", ex)
 
     def __del__(self) -> None:
         self._terminate()
@@ -393,9 +438,8 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
             May deadlock if output or error pipes are used and not handled separately.
 
         :raise git.exc.GitCommandError:
-            If the return status is not 0.
+            If the return status is not 0 or output handling timed out.
         """
-        stderr_b = force_bytes(data=stderr, encoding="utf-8") or b""
         status: Union[int, None]
         if self.proc is not None:
             status = self.proc.wait()
@@ -404,6 +448,11 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
             status = self.status
             p_stderr = None
 
+        if self._timeout_error is not None:
+            stderr = self._timeout_error
+            status = status or 1
+        stderr_b = force_bytes(data=stderr, encoding="utf-8") or b""
+
         def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes:
             if stream:
                 try:
@@ -1381,9 +1430,10 @@ def execute(
             1. This feature is not supported at all on Windows.
             2. Enumerating child processes requires ``pgrep -P``, or a ``ps`` command
                supporting the POSIX ``-A`` and ``-o`` options if ``pgrep`` is not
-               installed. Effectiveness may vary on systems without these commands.
-            3. Deeper descendants do not receive signals, though they may sometimes
-               terminate as a consequence of their parent processes being killed.
+               installed (``ps -ef`` on Cygwin). Effectiveness may vary on systems
+               without these commands.
+            3. Descendants are enumerated before signalling. Processes that detach
+               or spawn after enumeration may not receive signals.
             4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side
                effects on a repository. For example, stale locks in case of
                :manpage:`git-gc(1)` could render the repository incapable of accepting
@@ -1537,43 +1587,9 @@ def execute(
             timeout = kill_after_timeout
 
             def kill_process(pid: int) -> None:
-                """Callback to kill a process.
-
-                This callback implementation would be ineffective and unsafe on Windows.
-                """
-                child_pids = []
-                try:
-                    p = Popen(["pgrep", "-P", str(pid)], stdout=PIPE)
-                except FileNotFoundError:
-                    # POSIX ps does not support selecting by parent PID.
-                    with Popen(["ps", "-A", "-o", "pid=", "-o", "ppid="], stdout=PIPE) as p:
-                        if p.stdout is not None:
-                            for line in p.stdout:
-                                fields = line.split()
-                                if len(fields) == 2 and all(field.isdigit() for field in fields):
-                                    if int(fields[1]) == pid:
-                                        child_pids.append(int(fields[0]))
-                else:
-                    with p:
-                        if p.stdout is not None:
-                            for line in p.stdout:
-                                if line.strip().isdigit():
-                                    child_pids.append(int(line))
-                try:
-                    os.kill(pid, signal.SIGKILL)
-                    for child_pid in child_pids:
-                        try:
-                            os.kill(child_pid, signal.SIGKILL)
-                        except OSError:
-                            pass
-                    # Tell the main routine that the process was killed.
+                if _kill_process(pid):
                     assert kill_check is not None
                     kill_check.set()
-                except OSError:
-                    # It is possible that the process gets completed in the duration
-                    # after timeout happens and before we try to kill the process.
-                    pass
-                return
 
             def make_timeout_error() -> Union[str, bytes]:
                 err = f'Timeout: the command "{" ".join(redacted_command)}" did not complete in {timeout:g} secs.'

git/remote.py

@@ -988,8 +988,9 @@ def stdout_handler(line: str) -> None:
             # even if there is an output).
             if not output:
                 raise
-            elif stderr_text:
-                _logger.warning("Error lines received while fetching: %s", stderr_text)
+            elif stderr_text or proc._timeout_error:
+                if stderr_text:
+                    _logger.warning("Error lines received while pushing: %s", stderr_text)
                 output.error = e
 
         return output

test/test_autointerrupt.py

@@ -1,3 +1,9 @@
+from subprocess import PIPE
+import sys
+import threading
+
+import pytest
+
 from git.cmd import Git
 
 
@@ -31,3 +37,36 @@ def test_autointerrupt_terminate_ignores_attributeerror():
 
     # Ensure the reference is cleared to avoid repeated attempts.
     assert ai.proc is None
+
+
+def test_autointerrupt_terminate_closes_buffered_stdin():
+    ai = Git().hash_object("--stdin", as_process=True, istream=PIPE)
+    proc = ai.proc
+    proc.stdin.write(b"buffered input")
+    try:
+        ai._terminate()
+        assert proc.stdin.closed
+        assert proc.stdout.closed
+        assert proc.stderr.closed
+    finally:
+        proc.stdout.close()
+        proc.stderr.close()
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="requires POSIX SIGTERM handling")
+def test_autointerrupt_terminate_closes_pipes_before_waiting():
+    script = (
+        "import signal, sys; signal.signal(signal.SIGTERM, signal.SIG_IGN); "
+        "print('ready', flush=True); sys.stdin.read(); sys.stdout.buffer.write(b'x' * 1000000)"
+    )
+    ai = Git().execute([sys.executable, "-c", script], as_process=True, istream=PIPE)
+    proc = ai.proc
+    assert proc.stdout.readline() == b"ready\n"
+    watchdog = threading.Timer(5, proc.kill)
+    watchdog.start()
+    try:
+        ai._terminate()
+        assert proc.returncode != -9, "cleanup waited for exit with undrained output pipes"
+    finally:
+        watchdog.cancel()
+        watchdog.join()