Skip to content

Commit fa93137

Browse files
aprylewuaprylewu
committed
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>
1 parent dcb6b14 commit fa93137

2 files changed

Lines changed: 78 additions & 10 deletions

File tree

‎git/cmd.py‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1303,9 +1303,9 @@ def execute(
13031303
carefully considered, due to the following limitations:
13041304
13051305
1. This feature is not supported at all on Windows.
1306-
2. Effectiveness may vary by operating system. ``ps --ppid`` is used to
1307-
enumerate child processes, which is available on most GNU/Linux systems
1308-
but not most others.
1306+
2. Enumerating child processes requires ``pgrep -P``, or a ``ps`` command
1307+
supporting the POSIX ``-A`` and ``-o`` options if ``pgrep`` is not
1308+
installed. Effectiveness may vary on systems without these commands.
13091309
3. Deeper descendants do not receive signals, though they may sometimes
13101310
terminate as a consequence of their parent processes being killed.
13111311
4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side
@@ -1465,14 +1465,24 @@ def kill_process(pid: int) -> None:
14651465
14661466
This callback implementation would be ineffective and unsafe on Windows.
14671467
"""
1468-
p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE)
14691468
child_pids = []
1470-
if p.stdout is not None:
1471-
for line in p.stdout:
1472-
if len(line.split()) > 0:
1473-
local_pid = (line.split())[0]
1474-
if local_pid.isdigit():
1475-
child_pids.append(int(local_pid))
1469+
try:
1470+
p = Popen(["pgrep", "-P", str(pid)], stdout=PIPE)
1471+
except FileNotFoundError:
1472+
# POSIX ps does not support selecting by parent PID.
1473+
with Popen(["ps", "-A", "-o", "pid=", "-o", "ppid="], stdout=PIPE) as p:
1474+
if p.stdout is not None:
1475+
for line in p.stdout:
1476+
fields = line.split()
1477+
if len(fields) == 2 and all(field.isdigit() for field in fields):
1478+
if int(fields[1]) == pid:
1479+
child_pids.append(int(fields[0]))
1480+
else:
1481+
with p:
1482+
if p.stdout is not None:
1483+
for line in p.stdout:
1484+
if line.strip().isdigit():
1485+
child_pids.append(int(line))
14761486
try:
14771487
os.kill(pid, signal.SIGKILL)
14781488
for child_pid in child_pids:

‎test/test_git.py‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import pickle
1515
import re
1616
import shutil
17+
import signal
1718
import subprocess
1819
import sys
1920
import tempfile
@@ -332,6 +333,63 @@ def test_it_honors_kill_after_timeout_with_output_stream(self):
332333
self.assertEqual(output_stream.getvalue(), b"started\n")
333334
self.assertIn("Timeout: the command", stderr)
334335

336+
@skipUnless(
337+
sys.platform not in ("win32", "cygwin"),
338+
"child process lookup requires pgrep or POSIX ps",
339+
)
340+
@ddt.data(False, True)
341+
def test_timeout_kills_direct_child(self, without_pgrep):
342+
with tempfile.TemporaryDirectory() as directory:
343+
marker = Path(directory, "child-survived")
344+
child_code = (
345+
"import pathlib, sys, time; time.sleep(2); "
346+
"pathlib.Path(sys.argv[1]).write_text('survived', encoding='utf-8')"
347+
)
348+
parent_code = (
349+
"import subprocess, sys, time; "
350+
"subprocess.Popen([sys.executable, '-c', sys.argv[1], sys.argv[2]]); "
351+
"time.sleep(30)"
352+
)
353+
popen = cmd.Popen
354+
355+
def portable_popen(args, **kwargs):
356+
if without_pgrep and args[0] == "pgrep":
357+
raise FileNotFoundError("pgrep is not installed")
358+
return popen(args, **kwargs)
359+
360+
with mock.patch.object(cmd, "Popen", side_effect=portable_popen):
361+
status, _, stderr = self.git.execute(
362+
[sys.executable, "-c", parent_code, child_code, str(marker)],
363+
kill_after_timeout=1,
364+
with_exceptions=False,
365+
with_extended_output=True,
366+
)
367+
368+
self.assertNotEqual(status, 0)
369+
self.assertIn("Timeout: the command", stderr)
370+
self.assertFalse(marker.exists(), "the direct child survived the timeout")
371+
372+
@skipUnless(sys.platform != "win32", "kill_after_timeout is not supported on Windows")
373+
def test_timeout_ps_fallback_selects_only_direct_children(self):
374+
process = mock.MagicMock()
375+
process.pid = 1234
376+
process.communicate.return_value = (b"", b"")
377+
process.returncode = -signal.SIGKILL
378+
ps = mock.MagicMock()
379+
ps.__enter__.return_value = ps
380+
ps.stdout = io.BytesIO(b"PID PPID\n 321 1\n 5678 1234\n 9012 5678\n\n")
381+
382+
with contextlib.ExitStack() as stack:
383+
stack.enter_context(mock.patch.object(cmd, "safer_popen", return_value=process))
384+
stack.enter_context(mock.patch.object(cmd, "Popen", side_effect=[FileNotFoundError, ps]))
385+
kill = stack.enter_context(mock.patch.object(cmd.os, "kill"))
386+
timer = stack.enter_context(mock.patch.object(cmd.threading, "Timer"))
387+
# Run the timeout callback synchronously, with no real processes or signals.
388+
timer.return_value.start.side_effect = lambda: timer.call_args.args[1](1234)
389+
self.git.execute(["git", "version"], kill_after_timeout=1, with_exceptions=False)
390+
391+
self.assertEqual(kill.call_args_list, [mock.call(1234, signal.SIGKILL), mock.call(5678, signal.SIGKILL)])
392+
335393
def test_it_executes_git_without_stdout_redirect(self):
336394
returncode, stdout, stderr = self.git.execute(
337395
["git", "version"],

0 commit comments

Comments
 (0)