Fix Process pipe-buffer deadlock in CommandExecutor#49
Merged
Conversation
- Drain stdout and stderr concurrently via readabilityHandler + DispatchGroup before waiting for process exit (the macOS pipe buffer is ~64KB; the old waitUntilExit-then-readDataToEndOfFile order deadlocked any command whose combined output exceeded that — xcodebuild, agvtool, large git/find runs) - Consolidate execute, executeReadOnly, executeWithArguments onto a single runCollecting helper using the SwiftPM-style drain pattern - Switch availableData → read(upToCount:) to avoid SR-14669 spurious zero-byte callbacks - Remove the racy 'waitUntilExit then continuation.finish()' fallback in the async streaming paths — the readabilityHandler EOF callback is now the sole termination signal, so tail bytes are no longer dropped between the final pipe read and process termination - Tee stdout/stderr to the parent process in real time when verbose: true - Add four regression tests covering >64KB output on each sync path plus an EOF-sentinel test on the async streaming path - Bump version to 1.0.6
There was a problem hiding this comment.
Pull request overview
This PR fixes intermittent Process deadlocks in CommandExecutor caused by waiting for process exit before draining stdout/stderr (pipe-buffer fill). It refactors command execution to drain both pipes concurrently and adds regression tests that previously would hang with large output.
Changes:
- Refactors synchronous execution to a shared
runCollectingimplementation that drains stdout/stderr concurrently. - Updates async streaming execution to rely on EOF from
readabilityHandler(removing a race-prone fallback finisher). - Adds regression tests producing 200KB on stdout/stderr to ensure no deadlocks and no EOF tail-byte loss.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
VERSION |
Bumps package version to reflect the fix. |
Tests/XprojectTests/Utilities/CommandExecutorTests.swift |
Adds large-output regression tests to prevent deadlock regressions and detect EOF truncation. |
Sources/Xproject/Utilities/CommandExecutor.swift |
Implements concurrent pipe draining for sync execution and adjusts async streaming drain/termination behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Cap read(upToCount:) at 64KB instead of Int.max — matches the macOS pipe buffer; readabilityHandler is re-invoked for any remaining bytes - Log pipe read errors to stderr instead of silently treating them as EOF, so a non-EOF failure during drain is at least visible in CI logs - Dispatch tee writes onto a serial DispatchQueue and flush it before runCollecting returns. Previously the synchronous FileHandle.write inside the readabilityHandler could block on a slow stdout consumer (e.g. 'xp test --verbose | slow-consumer'), stalling the drain and recreating the pipe-buffer deadlock this PR set out to fix
- Add swiftlint:disable for type_body_length (struct is 314 lines; the process-execution surface area is cohesive and splitting would be artificial) - Use thousand separator in readChunkSize: 64 * 1_024 - Wrap single-line guard return per conditional_returns_on_newline
Node.js 20 actions are deprecated on GitHub Actions runners (forced to Node 24 from June 2nd, 2026; Node 20 removed September 16th, 2026). Bumps every first-party action across workflows to the latest major: - actions/checkout v4 → v6 (Node 24, separates creds storage) - actions/cache v4 → v5 (Node 24 runtime only) - actions/upload-artifact v4 → v7 (Node 24 + optional direct-file upload) No API surface used by these workflows changed across the bumps.
diogot
force-pushed
the
fix/command-executor-pipe-buffer-deadlock
branch
from
73855e5 to
d2b9fd2
Compare
2 tasks
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
Intermittent deadlocks in
CommandExecutortraced to the classic macOS pipe-buffer anti-pattern:waitUntilExit()was called beforereadDataToEndOfFile(), so any command whose combined stdout+stderr exceeded the ~64KB pipe buffer would hang the child (blocked writing) and the parent (blocked waiting). This was hittingxcodebuild,agvtool, largefind/gitruns, andaltooluploads.What changed
runCollectinghelper drains stdout and stderr in parallel viareadabilityHandler+DispatchGroup, thenwaitUntilExit, thengroup.wait(). Modelled on the SwiftPMTSCBasic.Processpattern.execute,executeReadOnly,executeWithArgumentsnow sharerunCollectinginstead of triplicating the unsafe sequential-read code.availableData→read(upToCount:)to avoid the spurious zero-byte callbacks documented in SR-14669.continuation.finish()afterwaitUntilExit— this raced the finalreadabilityHandlerdispatch and could drop tail bytes. The handler's empty-data callback is now the sole termination signal;waitUntilExitonly collects the exit status after both consumer tasks complete.verbose: true, sync execution now tees stdout/stderr to the parent process in real time (previously silent until completion).Mutex<Data>(Swift 6Synchronization) wrapped in a smallLockedDataclass — avoids@unchecked Sendablewhich requires approval perCLAUDE.md.Verification
Four regression tests added in
CommandExecutorTests.swiftcovering 200KB each on stdout and stderr (well past 64KB):END_SENTINELassertion in the streaming test confirms no data is dropped at EOF.Test plan
swift test— 468 tests passxp build/xp testagainst a real Xcode project to confirm the verbose tee behavior