fix(setup): preserve trailing command output while bounding inherited-pipe waits - #1207
Draft
karkarl wants to merge 1 commit into
Draft
fix(setup): preserve trailing command output while bounding inherited-pipe waits#1207karkarl wants to merge 1 commit into
karkarl wants to merge 1 commit into
Conversation
…-pipe waits CommandRunner previously always applied a short bounded grace to the stdout/stderr drain after the child exited, which could truncate trailing output on high-volume commands. Await full output-close on the normal path so complete output is preserved, and apply the bounded grace only on timeout cleanup or when a caller opts in via allowInheritedPipeHandleEscape for WSL inspection commands whose descendants can hold inherited pipe handles open. Opt the wsl.exe --list/--verbose inspection callers into the bounded escape (CleanupStaleDistroStep, CreateWslInstanceStep, ExistingConfigDetector, StartGatewayStep). Add regression tests: high-volume stdout/stderr drain with trailing markers on the default path, a bounded-timeout test, and update the pipe-holder test to use the opt-in. Fixes #1194 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 availability
|
Codex review: needs maintainer review before merge. Reviewed August 22, 2026, 11:19 AM ET / 15:19 UTC. ClawSweeper reviewWhat this changesThe branch waits for complete stdout and stderr draining after ordinary setup commands, while retaining a short bounded wait for timeouts and selected WSL inspection commands. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readinessKeep open. This focused collaborator-authored draft addresses a real current-main truncation path and is not already implemented; it needs the repository-required Windows setup validation before merge. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherThe setup engine runs Windows and WSL commands and captures their output for provisioning decisions. Its results feed WSL installation, preflight diagnostics, gateway startup, and setup completion checks. flowchart LR
A[Setup steps] --> B[Command runner]
B --> C[Windows or WSL command]
C --> D[Standard output and error drains]
D --> E{Timeout or known inherited handle?}
E -->|No| F[Wait for stream close]
E -->|Yes| G[Use bounded grace]
F --> H[Captured setup result]
G --> H
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve the scoped normal-drain behavior, then complete the repository build, Shared and Tray suites, plus the product WSL gateway validation before landing. Do we have a high-confidence way to reproduce the issue? Yes. Current main has an unconditional 250 ms output-drain race, and the branch adds a focused high-volume marker test for the resulting truncation path; this read-only review did not execute it. Is this the best way to solve the issue? Yes, with the remaining WSL validation. Restricting the bounded escape to timeout cleanup and known inherited-handle callers is a narrower maintainable fix than retaining truncation for every command. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ccd64bbb7d68. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
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
CommandRunner.RunAsyncalways applied a short bounded grace (s_outputDrainGrace) to the stdout/stderr drain after the child process exited. On high-volume commands whose output was still draining when that grace elapsed, trailing output could be truncated even though the process exited cleanly.This change preserves complete output on the normal path and keeps the bounded wait only where it is actually needed:
RunAsyncnow awaits full stdout/stderr close so all trailing output is captured.allowInheritedPipeHandleEscapeparameter for WSL inspection commands whose descendants can keep inherited pipe handles open after the direct child exits.wsl.exe --list/--verbose/--version/--statusinspection callers (CleanupStaleDistroStep,CreateWslInstanceStep,ExistingConfigDetector,StartGatewayStep,PreflightWslStep) opt into the bounded escape, since those are the calls that can leave a descendant holding inherited handles.Timeout and cancellation behavior are unchanged.
Fixes #1194
Validation
dotnet test tests/OpenClaw.SetupEngine.Tests(subsystem touched by this change): full suite 949/949 passed after restoring against a local NuGet feed.CommandRunnerTests: 7/7 passed, including the new high-volume drain and bounded-timeout tests../build.ps1and the Shared/Tray closeout suites are Not verified / blocked: NuGet cannot reachhttps://api.nuget.org/v3/index.jsonon this host (NU1900 "Unable to load the service index"; underlying TLS/service-index handshake failure), and the global cache lacksMicrosoft.ML.OnnxRuntime 1.29.0. A maintainer on a network-enabled host should run./build.ps1,dotnet test tests/OpenClaw.Shared.Tests, anddotnet test tests/OpenClaw.Tray.Teststo complete closeout before merge.Real behavior proof
RunAsync_DrainsHighVolumeStdoutAndStderrThroughTrailingMarkersruns a command emitting 8,000 lines on each of stdout and stderr followed bySTDOUT_MARKER/STDERR_MARKER, on the default (non-opt-in) path. It asserts all 8,000 lines on each stream are present and that the output ends with the trailing marker, directly proving trailing output is no longer truncated.RunAsync_ProcessTimeoutRemainsBoundedproves the timeout path still returns bounded (TimedOut, exit-1) within the grace window.RunAsync_ExitsLeavingPipeHolder...now passesallowInheritedPipeHandleEscape: true, proving the opt-in path stays bounded when a descendant holds an inherited pipe handle open.This PR is intentionally left as a draft for a human to complete full-repo
./build.ps1+ Shared/Tray validation on a network-enabled host before merge.