feat: std/process start, children with live output streaming and kill#863
Merged
Conversation
Add the streaming half of std/process. `start(program, args)` spawns a child without waiting (`spawn` is the goroutine keyword, hence start) and returns a Child handle: read_stdout and read_stderr drain whatever arrived since the last read, poll try-waits without blocking (running is its Bool shorthand), wait blocks with a scheduler-friendly poll loop, kill terminates the child, and free drops the handle without killing. The child's stdin is null so it does not inherit the parent's terminal. Runtime side: a second registry sharing the process id sequence holds the live process, two output buffers pumped by plain reader threads outside the collector, and the exit code once observed (cached so poll stays cheap). Poll reports -2 over the FFI while running; killing an already-exited child reports success. Spawn failures use the existing process last-error slot, and the argv crosses in the same NUL-joined encoding as run. The smoke test drives the whole surface end to end through compiled binaries: output drained while the child is provably still running, the exit code after wait, killing a long-idling child, and the spawn-failure Err path. The std-process spec documents the new model. Fixes #862
Strix Security Review1 open security finding on this PR:
Review summaryThe latest commit’s process-lifecycle changes improve spawned-child cleanup, but the new Updated for Reviewed by Strix |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a streaming subprocess API to std/process with Changesstd/process spawn/streaming feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StdProcess as std/process.rv
participant Runtime as raven-runtime FFI
participant PumpThreads as spawn_pump threads
participant OSChild as child process
StdProcess->>Runtime: raven_process_spawn(program, args)
Runtime->>OSChild: spawn with null stdin
Runtime->>PumpThreads: start stdout/stderr pumps
PumpThreads->>OSChild: read stdout/stderr pipes
PumpThreads->>Runtime: append to shared buffers
StdProcess->>Runtime: raven_process_read_stdout(id)
Runtime-->>StdProcess: drained bytes since last read
StdProcess->>Runtime: raven_process_poll(id)
Runtime-->>StdProcess: -2 while running, exit code after exit
StdProcess->>Runtime: raven_process_kill(id)
Runtime->>OSChild: terminate
StdProcess->>Runtime: raven_process_spawn_free(id)
Runtime-->>Runtime: remove registry entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
raven-runtime/src/lib.rs (1)
2546-2553: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded buffer growth if output isn't drained.
spawn_pumpappends indefinitely intoArc<Mutex<Vec<u8>>>with no size cap. For the stated background-shell use case, a child that produces large or continuous output before the caller reads it (or if the caller never reads stderr, say) can grow memory without bound.Consider capping buffer size (e.g., drop oldest bytes past a limit, or track/report an overflow flag) to bound worst-case memory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raven-runtime/src/lib.rs` around lines 2546 - 2553, The stdout/stderr capture in spawn_pump currently appends into unbounded Vec buffers, which can grow without limit if the caller does not drain them. Update the child output handling in the background-shell path to enforce a fixed maximum buffer size for the Arc<Mutex<Vec<u8>>> collectors, and make spawn_pump or its callers drop the oldest bytes or set an overflow indicator once the limit is reached. Ensure the change is applied consistently to both stdout and stderr capture so memory usage stays bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@raven-runtime/src/lib.rs`:
- Around line 2599-2653: raven_process_spawn_free currently removes the registry
entry without ensuring an exited child is reaped, so update the spawn lifecycle
in raven_process_poll, raven_process_kill, and raven_process_spawn_free to reap
any child that has already exited before dropping ownership. Add a wait/reap
path when freeing an entry whose Child may have terminated, or transfer cleanup
to a background waiter so exited children are always collected before the
registry entry disappears.
---
Nitpick comments:
In `@raven-runtime/src/lib.rs`:
- Around line 2546-2553: The stdout/stderr capture in spawn_pump currently
appends into unbounded Vec buffers, which can grow without limit if the caller
does not drain them. Update the child output handling in the background-shell
path to enforce a fixed maximum buffer size for the Arc<Mutex<Vec<u8>>>
collectors, and make spawn_pump or its callers drop the oldest bytes or set an
overflow indicator once the limit is reached. Ensure the change is applied
consistently to both stdout and stderr capture so memory usage stays bounded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce63b42e-6e72-480e-9f2b-e097123255a9
📒 Files selected for processing (6)
docs/v2/specs/std-process.mdexamples/v2/spawn_child.rvexamples/v2/spawn_parent.rvraven-runtime/src/lib.rsstdlib/std/process.rvtests/codegen_smoke.rs
Address the review findings on the std/process start surface. Each output buffer is now capped at 8 MB: a child that outruns its reader keeps only the most recent bytes, so an undrained pipe cannot grow memory without bound. And spawn_free now guarantees reaping: a child whose exit was already observed was reaped by that poll, and any other child is handed to a background waiter thread that collects it when it exits, so a freed child never lingers as a zombie. The spec and the Child.free doc note both behaviors, and a unit test pins the cap keeping the tail.
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
Comment on lines
+2667
to
+2674
| let entry = spawn_registry().lock().unwrap().remove(&id); | ||
| if let Some(mut entry) = entry { | ||
| if entry.code.is_some() { | ||
| return; | ||
| } | ||
| std::thread::spawn(move || { | ||
| let _ = entry.child.wait(); | ||
| }); |
There was a problem hiding this comment.
🔵 Unbounded waiter thread creation when freeing running child processes
Severity: MEDIUM · CWE-400
The new streaming process API lets untrusted Raven code detach a still-running child via Child.free(). The runtime responds by spawning a dedicated OS thread that blocks until that child exits, so repeated start() + free() calls can accumulate unbounded threads and subprocesses and exhaust host resources.
Suggested change
| let entry = spawn_registry().lock().unwrap().remove(&id); | |
| if let Some(mut entry) = entry { | |
| if entry.code.is_some() { | |
| return; | |
| } | |
| std::thread::spawn(move || { | |
| let _ = entry.child.wait(); | |
| }); | |
| let mut registry = spawn_registry().lock().unwrap(); | |
| let Some(mut entry) = registry.get_mut(&id) else { | |
| return; | |
| }; | |
| if entry.code.is_some() { | |
| registry.remove(&id); | |
| return; | |
| } | |
| // Refuse to detach a still-running child with a dedicated waiter thread: | |
| // the caller must kill or wait first so one program cannot accumulate | |
| // unbounded blocked OS threads by repeatedly calling free(). | |
| return; |
Prompt to fix with AI
This is a security vulnerability found during a code review.
Vulnerability: Unbounded waiter thread creation when freeing running child processes
Severity: MEDIUM
CWE: CWE-400
The new streaming process API lets untrusted Raven code detach a still-running child via `Child.free()`. The runtime responds by spawning a dedicated OS thread that blocks until that child exits, so repeated `start()` + `free()` calls can accumulate unbounded threads and subprocesses and exhaust host resources.
Location: raven-runtime/src/lib.rs:2667-2674
Context: Freeing a live child spawns an unbounded blocking waiter thread
```
// Before:
let entry = spawn_registry().lock().unwrap().remove(&id);
if let Some(mut entry) = entry {
if entry.code.is_some() {
return;
}
std::thread::spawn(move || {
let _ = entry.child.wait();
});
// After:
let mut registry = spawn_registry().lock().unwrap();
let Some(mut entry) = registry.get_mut(&id) else {
return;
};
if entry.code.is_some() {
registry.remove(&id);
return;
}
// Refuse to detach a still-running child with a dedicated waiter thread:
// the caller must kill or wait first so one program cannot accumulate
// unbounded blocked OS threads by repeatedly calling free().
return;
```
Location: stdlib/std/process.rv:109-113
Context: Public wrapper exposes the detach-on-free behavior to untrusted Raven code
```
// Before:
// Drop the runtime entry for this child. Does not kill a running child;
// the runtime still reaps it when it exits.
fun free(self) {
raven_process_spawn_free(self.id)
}
// After:
// Drop the runtime entry for this child only after it has exited.
// Call kill() or wait() first for a running child.
fun free(self) {
raven_process_spawn_free(self.id)
}
```
How to fix:
Do not create one background waiter thread per freed running child. Keep detached children in a shared structure that can be reaped without per-child blocked threads, or change `free()` so it only succeeds for already-exited children (or explicitly kills before freeing) to prevent attacker-controlled thread accumulation.
Please fix this vulnerability. If you propose a fix, make it concise and minimal.React 👍 / 👎 to tune Strix for this repo. A repo collaborator (or the PR author) can resolve this thread to dismiss the finding.
Comment on lines
+109
to
+113
| // Drop the runtime entry for this child. Does not kill a running child; | ||
| // the runtime still reaps it when it exits. | ||
| fun free(self) { | ||
| raven_process_spawn_free(self.id) | ||
| } |
There was a problem hiding this comment.
Public wrapper exposes the detach-on-free behavior to untrusted Raven code
Suggested change
| // Drop the runtime entry for this child. Does not kill a running child; | |
| // the runtime still reaps it when it exits. | |
| fun free(self) { | |
| raven_process_spawn_free(self.id) | |
| } | |
| // Drop the runtime entry for this child only after it has exited. | |
| // Call kill() or wait() first for a running child. | |
| fun free(self) { | |
| raven_process_spawn_free(self.id) | |
| } |
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
1 participant
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.
Closes #862.
std/process gains its streaming half: start a child, watch its output while it runs, and stop it. This is the runtime capability background shells in rook build on (martian56/rook#25), the same shape as the std/http streaming work.
Surface
spawnis the goroutine keyword, hencestart.Design
Child, two output buffers, and the observed exit code. Plain OS reader threads pump each pipe into its buffer until end of stream; they never touch a GC object. Reads copy out before allocating, following the existing lock discipline.None; signal deaths and unknown ids stay -1). Kill treats an already-exited child as success. Free drops the entry without killing, documented. wait polls with a short sleep so other goroutines keep running.run.Testing
cargo test: 789 pass, 0 fail (fmt and clippy clean; the one clippy warning is pre-existing in the compiler lib).process_spawn_streams_and_killsdrives compiled binaries end to end: the parent drains "early" while the child is provably still running, waits for exit code 3, sees the late output, kills a 20s-idling child, and takes the Err path on a nonexistent program. Deterministic expected stdout, same harness as the other process tests.docs/v2/specs/std-process.mdupdated with the two capture models and the extended registry.Summary by CodeRabbit
std/process::start(program, args) -> Result<Child, Error>for asynchronous subprocesses with a newChildhandle.Childsupports incrementalread_stdout/read_stderr, non-blockingpoll/running,wait,kill, andfree.