GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: stop stalled remote commands when their timeout expires by Byron · Pull Request #2277 · gitpython-developers/GitPython

Conversation

@Byron

@Byron Byron commented Oct 7, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Tasks

This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.

  • refackiew

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:

  • All 50 CI checks pass on 30b741c, including Linux, macOS, Windows, Cygwin,
    Alpine, dependency compatibility, lint, types, tests, and documentation.
  • Python 3.8 focused timeout and cleanup suite: 24 passed.
  • Command, cleanup, and command-deprecation modules: 122 passed, 1 skipped before
    the final callback/parser additions; subsequent focused checks cover those changes.
  • All nine loopback cases pass with pgrep disabled, exercising the POSIX ps fallback.
  • Cygwin CI passed all nine stalled-remote cases and 1581 tests overall; its sole
    parser-fixture platform mismatch is corrected by explicitly selecting each modeled platform.
  • Ruff lint/format, native and Windows-targeted mypy, basedpyright, and diff checks pass.
  • Final Codex commit review found no actionable regressions. All six Copilot threads
    were addressed with regressions or documentation and resolved.
  • Local TestRemote.test_base fails identically with the unchanged implementation
    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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Copilot AI balanced review requested due to automatic review settings October 7, 2026 08:57
Copilot started reviewing on behalf of Byron October 7, 2026 08:59
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Medium severity · 1 Low severity

Open (2)
  • Medium severity Avoid synchronous re-entry of blocked stderr handler on timeout · New
  • Low severity Update Git.execute documentation for recursive signal handling · New
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread git/cmd.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread git/cmd.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Byron
Byron force-pushed the kill-all-proper branch from 7475939 to a720bd8 Compare October 7, 2026 09:10
Copilot AI balanced review requested due to automatic review settings October 7, 2026 09:10
Copilot started reviewing on behalf of Byron October 7, 2026 09:11
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
  • Medium severity Avoid synchronous re-entry of blocked stderr handler on timeout
  • Low severity Update Git.execute documentation for recursive signal handling

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Byron
Byron force-pushed the kill-all-proper branch from a720bd8 to 56d0713 Compare October 7, 2026 09:13
Copilot AI balanced review requested due to automatic review settings October 7, 2026 09:13
Copilot started reviewing on behalf of Byron October 7, 2026 09:17
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
  • Medium severity Avoid synchronous re-entry of blocked stderr handler on timeout
  • Low severity Update Git.execute documentation for recursive signal handling

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Copilot AI balanced review requested due to automatic review settings October 7, 2026 09:41
@Byron
Byron force-pushed the kill-all-proper branch from 56d0713 to 7823f6b Compare October 7, 2026 09:41
Copilot started reviewing on behalf of Byron October 7, 2026 09:43
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Byron
Byron force-pushed the kill-all-proper branch from 7823f6b to f1d9f61 Compare October 7, 2026 10:02
Copilot AI balanced review requested due to automatic review settings October 7, 2026 10:02
Copilot started reviewing on behalf of Byron October 7, 2026 10:08
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Cross-platform process signaling and concurrent stream teardown warrant final human validation.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
  • Medium severity Avoid empty fetch error logs when push times out without stderr · New

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread git/remote.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 11:00
@Byron
Byron force-pushed the kill-all-proper branch from f1d9f61 to 4c0b78e Compare October 7, 2026 11:00
Copilot started reviewing on behalf of Byron October 7, 2026 11:01
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 High severity

Open (1)
  • High severity Timeout incorrectly reported as success when handler remains blocked · New
Resolved since last review (1)
  • Medium severity Avoid empty fetch error logs when push times out without stderr

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread git/cmd.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Byron
Byron force-pushed the kill-all-proper branch from 4c0b78e to e5822e3 Compare October 7, 2026 11:29
Copilot AI balanced review requested due to automatic review settings October 7, 2026 11:29
Copilot started reviewing on behalf of Byron October 7, 2026 11:29
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The timeout path can signal a recycled PID after the child has already exited.

Review effort: Balanced
Findings: 1 High severity

Open (1)
  • High severity Avoid killing recycled PID after child exits during handler timeout · New
Resolved since last review (1)
  • High severity Timeout incorrectly reported as success when handler remains blocked

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread git/cmd.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 11:45
@Byron
Byron force-pushed the kill-all-proper branch from e5822e3 to 3d1cfe1 Compare October 7, 2026 11:45
Copilot started reviewing on behalf of Byron October 7, 2026 11:49
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Timeout reporting currently discards previously captured Git error diagnostics.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
  • Medium severity Preserve captured stderr when appending timeout marker · New
Resolved since last review (1)
  • High severity Avoid killing recycled PID after child exits during handler timeout

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread git/cmd.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 12:00
@Byron
Byron force-pushed the kill-all-proper branch from 3d1cfe1 to 30b741c Compare October 7, 2026 12:00
Copilot started reviewing on behalf of Byron October 7, 2026 12:01
View session
Copilot AI reviewed Oct 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The timeout fix is consistent, documented, and comprehensively covered by targeted regressions.

Review effort: Balanced
Findings: None

Resolved since last review (1)
  • Medium severity Preserve captured stderr when appending timeout marker

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Byron @codex
fix: stop stalled remote commands when their timeout expires (#2276)
5ba236e
<!-- 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]>
@Byron
Byron force-pushed the kill-all-proper branch from 30b741c to 5ba236e Compare October 8, 2026 07:06
Copilot AI balanced review requested due to automatic review settings October 8, 2026 07:06
Copilot started reviewing on behalf of Byron October 8, 2026 07:08
View session
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Byron
Byron merged commit 1af7ce6 into main Oct 8, 2026
51 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@Byron
Byron deleted the kill-all-proper branch October 29, 2026 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

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

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@Byron
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.