Repository navigation
fix: stop stalled remote commands when their timeout expires - #2277
Conversation
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.
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)
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)
<!-- Byron --> While I looked at the production code changes with some care, I only rubber-stamped the tests. <!-- agent --> `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)`. Assisted-by: GPT 6.1 Sol Co-authored-by: GPT 6.1 Sol <codex@openai.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-platform process-tree signaling and stream cleanup warrant the planned final human review.
0 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
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.