Repository navigation
fix: stop stalled remote commands when their timeout expires - #2277
Byron merged 1 commit into
Conversation
Sorry, something went wrong.
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 · 1
Avoid synchronous re-entry of blocked stderr handler on timeout · New
Update Git.execute documentation for recursive signal handling · New
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.
Sorry, something went wrong.
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
Sorry, something went wrong.
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
Sorry, something went wrong.
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
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A timeout can still return success when the child exits normally while an output handler remains blocked.
Review effort: Balanced
Findings: 1
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The timeout path can signal a recycled PID after the child has already exited.
Review effort: Balanced
Findings: 1
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Timeout reporting currently discards previously captured Git error diagnostics.
Review effort: Balanced
Findings: 1
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The timeout fix is consistent, documented, and comprehensively covered by targeted regressions.
Review effort: Balanced
Findings: None
Sorry, something went wrong.
<!-- 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.
Sorry, something went wrong.
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 ps and Cygwin ps -ef when pgrep is unavailable.
Both output streams share one deadline. Append timeout diagnostics to captured stderr for
AutoInterrupt.wait() instead of synchronously re-entering callbacks, and preserve
those 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 the
server'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:
Alpine, dependency compatibility, lint, types, tests, and documentation.
the final callback/parser additions; subsequent focused checks cover those changes.
parser-fixture platform mismatch is corrected by explicitly selecting each modeled platform.
were addressed with regressions or documentation and resolved.
due to a missing remote-tracking ref; this limitation was verified against the baseline.
Git reference: local source baseline v2.56.0-rc1, especially
transport-helper.c:get_helper() (inherited stderr), connect.c:git_connect()
(SSH transport children), and run-command.c (descriptor handling and opt-in
child 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.