Fix timeout child-process lookup on non-GNU systems - #2236
Open
aprylewu wants to merge 1 commit into
Open
Conversation
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 gitpython-developers#1756 Signed-off-by: Mingyang Wu <129849514+aprylewu@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Git.execute(..., kill_after_timeout=...)currently finds direct child processes withps --ppid. On macOS, that command fails and only the parent receivesSIGKILL; a child can continue running and keep the captured output pipes open after the timeout.Use
pgrep -Pto enumerate direct children, falling back to POSIXps -A -o pid= -o ppid=whenpgrepis not installed. The fallback filters by the target parent's PID. Both enumeration subprocesses are waited for and their pipes closed. The existing parent-first signal order and direct-child scope are preserved.The regression runs a real parent/child pair through
Git.execute. Before the change, the child survives the timeout and writes a marker; after the change, it is terminated, both with nativepgrepand withpgrepunavailable. A separate test checks that the fallback excludes unrelated processes and grandchildren.Fixes #1756.
Validation on macOS 27.0, Python 3.13.5:
python -m pytest test/test_git.py -q --no-cov: 105 passed, 1 skipped.ruff check .andruff format --check .(0.16.5).codespell git/cmd.py test/test_git.py(2.4.3).mypy(1.18.2): 45 source files.basedpyright --warnings(1.39.9): no errors or warnings.git diff --check.Linux and Cygwin were not run locally. The real-process regression skips Cygwin, whose default
psdoes not support these POSIX options; the documented tool requirements still apply. This does not add native Windows support or change the separate timeout implementation used byRemote.fetch,pull, andpush.I am an AI agent (OpenAI Codex) acting on behalf of Mingyang Wu (
aprylewu). I prepared this implementation and ran the checks above.