Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A blocked stderr callback can still stall timeout cleanup, and the public timeout documentation contradicts the new descendant handling.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Fixes stalled remote-command timeout cleanup by terminating Git’s process tree before closing streams.
Changes:
- Adds recursive descendant termination and shared timeout deadlines.
- Reorders process and stream cleanup to avoid pipe deadlocks.
- Adds timeout and cleanup regression tests.
| File | Description |
|---|---|
git/cmd.py |
Implements descendant termination and revised timeout cleanup. |
test/test_git.py |
Tests descendant lookup and blocked callbacks. |
test/test_autointerrupt.py |
Tests stream cleanup and termination ordering. |
test/test_remote.py |
Tests stalled fetch, pull, and push operations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Cross-platform process-tree signalling and concurrent stream cleanup warrant final human review despite comprehensive regression coverage.
Review effort: Balanced
Findings: None
Resolved since last review (2)
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The timeout behavior, cleanup ordering, error propagation, documentation, and relevant edge cases are consistently implemented and covered.
Review effort: Balanced
Findings: None
Resolved since last review (2)
`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. Copilot also identified that assigning the timeout diagnostic discarded stderr already captured by fetch or push. Encode caller-supplied stderr first and append the timeout marker, preserving actionable Git diagnostics and the failure status. Extend all blocked-handler cases to supply both text and byte stderr and assert that the original error survives alongside the timeout. All four cases failed before the correction and now pass. Python 3.8 focused timeout and cleanup tests: 24 passed. Native/Windows mypy, basedpyright, Ruff lint/format, and `git diff --check` pass.
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
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.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by
Codex GPT-6.Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Fixes #2276.
Stalled fetch, pull, and push commands can deadlock when timeout cleanup closes
stderr while its pump holds the buffered reader lock. Kill Git and its descendants
before closing output streams, then reap the process. Reuse the command watchdog's
process lookup, extending it to grandchildren (including Apple Git's HTTP helper
launcher). Support both POSIX
psand Cygwinps -efwhenpgrepis unavailable.Both output streams share one deadline. Append timeout diagnostics to captured stderr for
AutoInterrupt.wait()instead of synchronously re-entering callbacks, and preservethose errors in partial push results, including when Git exits successfully while
a callback remains blocked. Skip process-tree signalling after the child exits.
Ordinary cleanup keeps stdin EOF delivery,
SIGTERM, and output closure before waiting, including buffered/broken input pipes.Update the public descendant limitations to match the implementation.
The loopback regression covers all nine combinations of fetch/pull/push and
git:///HTTP/HTTPS. All nine failed before the fix, returning only after theserver's five-second safety release; all now pass with 0.5-second timeouts,
including in Cygwin CI. Additional regressions cover buffered stdin, undrained
output, blocked stdout/stderr callbacks, partial pushes, missing process lookup
tools, and POSIX/Cygwin descendant parsing.
Validation:
30b741c, including Linux, macOS, Windows, Cygwin,Alpine, dependency compatibility, lint, types, tests, and documentation.
the final callback/parser additions; subsequent focused checks cover those changes.
pgrepdisabled, exercising the POSIXpsfallback.parser-fixture platform mismatch is corrected by explicitly selecting each modeled platform.
were addressed with regressions or documentation and resolved.
TestRemote.test_basefails identically with the unchanged implementationdue to a missing remote-tracking ref; this limitation was verified against the baseline.
Git reference: local source baseline
v2.56.0-rc1, especiallytransport-helper.c:get_helper()(inherited stderr),connect.c:git_connect()(SSH transport children), and
run-command.c(descriptor handling and opt-inchild cleanup). Runtime reproduction used Apple Git 2.54.0. Cygwin process
format was checked against upstream
winsup/utils/ps.cc.Process lookup is best-effort if tools are unavailable. Descendants that already
detached or spawn after enumeration may escape signalling. Windows command
timeouts remain unsupported.