From fa931374b841fd01f11114b42ca0fdb4d87633dc Mon Sep 17 00:00:00 2001 From: Mingyang Wu <129849514+aprylewu@users.noreply.github.com> Date: Sat, 12 Sep 2026 23:02:32 +0800 Subject: [PATCH] Fix timeout child-process lookup on non-GNU systems Git.execute used ps --ppid to find direct children before enforcing kill_after_timeout. On macOS this option is rejected: the parent is killed, but a child can continue running and hold captured output pipes open. Use pgrep -P for the child lookup, with POSIX ps PID/PPID output as a fallback when pgrep is absent. Filter the fallback by the original parent PID and reap the lookup subprocess in both paths. Keep the existing parent-first SIGKILL order, direct-child scope, and Windows guard, and update the documented command requirements. Systems without either lookup facility and the existing PID-reuse race remain limitations. Add real-process regressions for native pgrep and the ps fallback, plus a test that excludes unrelated processes and grandchildren from the fallback. Both real-process cases failed on the original code on macOS. The command module now passes 105 tests with 1 skip on macOS 27.0 / Python 3.13.5. Ruff check and format, codespell, mypy (45 files), basedpyright, and diff whitespace checks pass. Linux and Cygwin were not run locally; Cygwin's default ps lacks the required options, so the real-process cases skip it. Fixes #1756 Signed-off-by: Mingyang Wu <129849514+aprylewu@users.noreply.github.com> --- git/cmd.py | 30 ++++++++++++++++--------- test/test_git.py | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 193dfd4f6..9de2f6e8a 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1303,9 +1303,9 @@ def execute( carefully considered, due to the following limitations: 1. This feature is not supported at all on Windows. - 2. Effectiveness may vary by operating system. ``ps --ppid`` is used to - enumerate child processes, which is available on most GNU/Linux systems - but not most others. + 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. 4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side @@ -1465,14 +1465,24 @@ def kill_process(pid: int) -> None: This callback implementation would be ineffective and unsafe on Windows. """ - p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE) child_pids = [] - if p.stdout is not None: - for line in p.stdout: - if len(line.split()) > 0: - local_pid = (line.split())[0] - if local_pid.isdigit(): - child_pids.append(int(local_pid)) + 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: diff --git a/test/test_git.py b/test/test_git.py index a88d980fb..b19652363 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -14,6 +14,7 @@ import pickle import re import shutil +import signal import subprocess import sys import tempfile @@ -332,6 +333,63 @@ def test_it_honors_kill_after_timeout_with_output_stream(self): self.assertEqual(output_stream.getvalue(), b"started\n") self.assertIn("Timeout: the command", stderr) + @skipUnless( + sys.platform not in ("win32", "cygwin"), + "child process lookup requires pgrep or POSIX ps", + ) + @ddt.data(False, True) + def test_timeout_kills_direct_child(self, without_pgrep): + with tempfile.TemporaryDirectory() as directory: + marker = Path(directory, "child-survived") + child_code = ( + "import pathlib, sys, time; time.sleep(2); " + "pathlib.Path(sys.argv[1]).write_text('survived', encoding='utf-8')" + ) + parent_code = ( + "import subprocess, sys, time; " + "subprocess.Popen([sys.executable, '-c', sys.argv[1], sys.argv[2]]); " + "time.sleep(30)" + ) + popen = cmd.Popen + + def portable_popen(args, **kwargs): + if without_pgrep and args[0] == "pgrep": + raise FileNotFoundError("pgrep is not installed") + return popen(args, **kwargs) + + with mock.patch.object(cmd, "Popen", side_effect=portable_popen): + status, _, stderr = self.git.execute( + [sys.executable, "-c", parent_code, child_code, str(marker)], + kill_after_timeout=1, + with_exceptions=False, + with_extended_output=True, + ) + + self.assertNotEqual(status, 0) + self.assertIn("Timeout: the command", stderr) + self.assertFalse(marker.exists(), "the direct child survived the timeout") + + @skipUnless(sys.platform != "win32", "kill_after_timeout is not supported on Windows") + def test_timeout_ps_fallback_selects_only_direct_children(self): + process = mock.MagicMock() + process.pid = 1234 + process.communicate.return_value = (b"", b"") + process.returncode = -signal.SIGKILL + ps = mock.MagicMock() + ps.__enter__.return_value = ps + ps.stdout = io.BytesIO(b"PID PPID\n 321 1\n 5678 1234\n 9012 5678\n\n") + + with contextlib.ExitStack() as stack: + stack.enter_context(mock.patch.object(cmd, "safer_popen", return_value=process)) + stack.enter_context(mock.patch.object(cmd, "Popen", side_effect=[FileNotFoundError, ps])) + kill = stack.enter_context(mock.patch.object(cmd.os, "kill")) + timer = stack.enter_context(mock.patch.object(cmd.threading, "Timer")) + # Run the timeout callback synchronously, with no real processes or signals. + timer.return_value.start.side_effect = lambda: timer.call_args.args[1](1234) + self.git.execute(["git", "version"], kill_after_timeout=1, with_exceptions=False) + + self.assertEqual(kill.call_args_list, [mock.call(1234, signal.SIGKILL), mock.call(5678, signal.SIGKILL)]) + def test_it_executes_git_without_stdout_redirect(self): returncode, stdout, stderr = self.git.execute( ["git", "version"],