join_pipelines: drain base_cmd stdout in a background thread#5
Merged
Conversation
When `base_cmd`'s output exceeds the pipe buffer (~64 KB on Linux) it blocks writing to its captured `stdout=PIPE`, which stops it from draining the FIFOs the pipelines write to, which blocks the `p.wait()` calls → deadlock. Drain the captured stdout in a daemon thread so the writer never blocks. Reproduces on any `git-diff-x '<cmd>' <file>` where the piped output per side runs to hundreds of KB (e.g. `gdxc 'jq .' <big-json-file>`). Regression test in `TestJoinPipelinesLargeOutput`: `seq 1 30000` vs `seq 30001 60000` (~340 KB diff output); guarded by `SIGALRM` so a broken fix fails loud instead of hanging.
There was a problem hiding this comment.
Pull request overview
Fixes a deadlock in gdxc/join_pipelines when base_cmd produces more output than the OS pipe buffer by draining base_cmd’s captured stdout concurrently, and adds a regression test to ensure large output no longer hangs.
Changes:
- Drain
base_cmdstdout in a background thread to prevent pipe-buffer backpressure deadlocking the pipelines. - Add a regression test that generates ~340 KB of diff output and fails fast via
SIGALRMif a hang reappears.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
dffs/utils.py |
Drain base_cmd stdout asynchronously to prevent pipe-buffer deadlocks during join_pipelines. |
tests/test_join_pipelines.py |
Add a large-output regression test guarded by an alarm to catch hangs reliably. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…test Follow-ups on the deadlock fix: - `dffs/utils.py`: the stdout-drain thread previously swallowed any exception from `proc.stdout.read()`, silently returning empty output with a success code. Capture the exception and re-raise it in the main thread after `join()` instead. - `tests/test_join_pipelines.py`: assert the complete diff output (hunk header, all 30000 `<` lines, `---`, all 30000 `>` lines), verifying the drain captured every byte rather than only that it didn't deadlock. Capture via `monkeypatch` on `dffs.utils.stdout` since the module's import-time `from sys import stdout` binding defeats `capsys`/`capfd`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Summary
git-diff-x '<cmd>' <file>(a.k.a.gdxc <cmd> <file>) deadlocks whenever the piped output per side runs past the pipe buffer (~64 KB on Linux) — e.g.gdxc 'jq .' <big-json-file>.join_pipelinescapturesbase_cmd's stdout viastdout=PIPEand reads it only afterp.wait()returns for each pipeline. Whenbase_cmd's output fills the pipe, it blocks writing → can't drain the FIFOs the pipelines write to →p.wait()never returns.base_cmdnever blocks on its own writes.Test plan
TestJoinPipelinesLargeOutput::test_large_diff_output_does_not_deadlock—seq 1 30000vsseq 30001 60000(~340 KB diff output), guarded bySIGALRMso a broken fix fails loud instead of hanging. Passes with fix; caught deadlock on the previous behavior.gdxc 'jq .' www/public/assets/station-luc.jsonon a 461 KB JSON file: was hanging 2+ min, now completes in ~0.5 s with 8,653 diff lines of output.pytest: 65 pass (was 64; +1 new test). The 3 pre-existingtest_shell_integration.pyfailures are unrelated.🤖 Generated with Claude Code