Repository navigation
Commit fa93137
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
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1303,9 +1303,9 @@ def execute( | |||
| 1303 | 1303 | carefully considered, due to the following limitations: | |
| 1304 | 1304 | ||
| 1305 | 1305 | 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. | ||
| 1309 | 1309 | 3. Deeper descendants do not receive signals, though they may sometimes | |
| 1310 | 1310 | terminate as a consequence of their parent processes being killed. | |
| 1311 | 1311 | 4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side | |
@@ -1465,14 +1465,24 @@ def kill_process(pid: int) -> None: | |||
| 1465 | 1465 | ||
| 1466 | 1466 | This callback implementation would be ineffective and unsafe on Windows. | |
| 1467 | 1467 | """ | |
| 1468 | - p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE) | ||
| 1469 | 1468 | 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)) | ||
| 1476 | 1486 | try: | |
| 1477 | 1487 | os.kill(pid, signal.SIGKILL) | |
| 1478 | 1488 | for child_pid in child_pids: | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -14,6 +14,7 @@ | |||
| 14 | 14 | import pickle | |
| 15 | 15 | import re | |
| 16 | 16 | import shutil | |
| 17 | + import signal | ||
| 17 | 18 | import subprocess | |
| 18 | 19 | import sys | |
| 19 | 20 | import tempfile | |
@@ -332,6 +333,63 @@ def test_it_honors_kill_after_timeout_with_output_stream(self): | |||
| 332 | 333 | self.assertEqual(output_stream.getvalue(), b"started\n") | |
| 333 | 334 | self.assertIn("Timeout: the command", stderr) | |
| 334 | 335 | ||
| 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 | + | ||
| 335 | 393 | def test_it_executes_git_without_stdout_redirect(self): | |
| 336 | 394 | returncode, stdout, stderr = self.git.execute( | |
| 337 | 395 | ["git", "version"], | |
0 commit comments