GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||
GeezerVison is showing github.com — fix: stop stalled remote commands when their timeout expires by Byron · Pull Request #2277 · gitpython-developers/GitPython Conversation
|
| 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 an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Cross-platform process signaling and concurrent stream teardown warrant final human validation.
Review effort: Balanced
Findings: 1
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Timeout reporting currently discards previously captured Git error diagnostics.
Review effort: Balanced
Findings: 1
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The timeout fix is consistent, documented, and comprehensively covered by targeted regressions.
Review effort: Balanced
Findings: None
Sorry, something went wrong.
There was an error while loading. Please reload this page.
<!-- 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 <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Remote.fetch(kill_after_timeout=...) does not stop git when it writes nothing more to stderr
There was an error while loading. Please reload this page.