Bound foreground subprocess output and wait time - #627
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens WFL foreground subprocess execution (execute command) by bounding captured stdout/stderr memory usage and ensuring cancellation/deadlines can interrupt both process execution and pipe draining, closing a reliability/security gap where output could grow unbounded or the interpreter could stall indefinitely.
Changes:
- Replaces
Command::output()with concurrent, bounded stdout/stderr draining plus an interrupt monitor (budget cancellation + deadline / per-operation timeout insidemain loop). - Adds cleanup guards to abort pipe collectors and terminate/reap the direct child on all failure/drop paths.
- Extends regression coverage with cross-platform helper subprocess fixtures and tests for flooding, stalls, cancellation, deadlines, and inherited-pipe hangs; fixes zero-sized bounded-buffer behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/interpreter/mod.rs | Implements bounded foreground capture with interruption + cleanup logic and adds focused subprocess regressions. |
| src/interpreter/bounded_buffer.rs | Fixes zero-sized buffers (discard all output) and makes stats counters overflow-safe. |
| Docs/reference/configuration-reference.md | Clarifies max_buffer_size_bytes semantics (raw-byte ceiling, truncation behavior, UTF-8 expansion bounds). |
| Docs/04-advanced-features/subprocess-execution.md | Documents bounded capture + truncation warnings and deadline/cancellation behavior for foreground commands (including main loop semantics). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn foreground_command_deadline( | ||
| budget: &ExecutionBudget, | ||
| configured_timeout: Duration, | ||
| ) -> Result<ForegroundCommandDeadline, ExecuteCommandError> { | ||
| budget | ||
| .check_cancelled() | ||
| .map_err(ExecuteCommandError::Budget)?; | ||
|
|
||
| if budget.is_deadline_exempt() { | ||
| return Ok(ForegroundCommandDeadline::MainLoop { | ||
| started: Instant::now(), | ||
| timeout: configured_timeout, | ||
| }); | ||
| } | ||
|
|
||
| budget | ||
| .check_deadline() | ||
| .map_err(ExecuteCommandError::Budget)?; | ||
| if budget.limits().max_duration.is_some() { | ||
| Ok(ForegroundCommandDeadline::Execution) | ||
| } else { | ||
| Ok(ForegroundCommandDeadline::None) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Foreground commands inside a main loop now gain a finite per-command timeout (behavior change)
Previously a foreground execute command inside a main loop inherited the loop's deadline exemption and could run indefinitely (cmd.output().await had no timeout). Now foreground_command_deadline (src/interpreter/mod.rs:1245-1268) gives each such command a fresh timeout_seconds window (default 60s, foreground_command_interrupt src/interpreter/mod.rs:1289-1295). A WFL server main loop that legitimately shells out to a command taking longer than timeout_seconds (e.g. a long build) will now be killed and reaped at that limit, whereas before it would run to completion. This is the PR's stated intent and is documented, but it is a real backward-compatibility behavior change for existing programs that run long foreground commands inside a main loop; worth confirming the default (60s) is acceptable and that affected users can raise timeout_seconds.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by #632, which preserves this security fix in the consolidated Rust-source hardening PR. The combined head is mergeable and all required CI checks are green. |
Summary
Command::outputbuffering with concurrent bounded stdout/stderr drainsmax_buffer_size_bytesper stream, preserve recent output, and fix zero-sized buffers retaining a bytemain loopa fresh configured timeoutSecurity impact
A child could previously emit unlimited stdout/stderr into memory or stall forever, including after its direct process exited while a descendant retained inherited pipes. Foreground capture is now bounded and the complete operation is interruptible. Malformed UTF-8 can expand the retained raw bytes only by a fixed factor.
Validation
git diff --checkpassedPart of the Rust-source production-readiness work tracked in #610.