feat(cargo-each): add portable execution inputs - #176
martin-kolinek wants to merge 25 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate issues remain in execution cleanup, timeout handling, parsing, and placeholder expansion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds portable package-file selection, workspace Rust-version substitution, bounded parallel execution, buffered output, timeouts, and process-tree cleanup to cargo-each.
Changes:
- Adds repeatable
--package-fileinputs and lazy Rust-version validation. - Adds
--jobs, deterministic buffered output, and per-invocation timeouts. - Updates tests, documentation, dependencies, and lockfile.
File summaries
| File | Summary |
|---|---|
crates/cargo-each/tests/cli.rs |
Adds integration coverage. |
crates/cargo-each/src/workspace.rs |
Resolves and validates workspace Rust versions. |
crates/cargo-each/src/substitute.rs |
Adds workspace placeholder substitution; moderate issue: inserted values can be rescanned. |
crates/cargo-each/src/select.rs |
Parses package-file selections; critical issue: malformed validation results are not propagated correctly. |
crates/cargo-each/src/run.rs |
Implements execution and cleanup; critical issues remain around unbounded output draining and synchronous timeout termination, plus moderate issues with interrupted reads, descendant lifecycle, and placeholder ordering. |
crates/cargo-each/src/plan.rs |
Propagates expanded invocation options. |
crates/cargo-each/src/main.rs |
Updates CLI documentation. |
crates/cargo-each/src/filter.rs |
Updates test fixtures. |
crates/cargo-each/src/error.rs |
Adds typed input and configuration diagnostics. |
crates/cargo-each/src/cli.rs |
Defines new selection and execution options. |
crates/cargo-each/README.md |
Updates generated documentation; nit: differs from the source rustdoc artifact. |
crates/cargo-each/docs/design/README.md |
Documents the expanded contract. |
crates/cargo-each/Cargo.toml |
Adds required dependencies. |
Cargo.lock |
Records dependency updates. |
Review details
Suppressed comments (5)
crates/cargo-each/README.md:121
- This generated README disagrees with its source rustdoc:
crates/cargo-each/src/main.rs:111usesmember's, while this line usesmember’s. Since the repository'sanvil-readme-checkvalidates generated crate READMEs, regenerate this file from the source rather than committing an artifact-only mismatch.
uses `{workspace-rust-version}`, then requires every member’s resolved
crates/cargo-each/src/run.rs:350
run_capturedusesspawn_treeeven whentimeoutisNone, so the no-timeout parallel path reachesProcessTree::observe, whose normal-exit contract sweeps descendants (crates/cargo-gamma-process/src/process_tree.rs:1120-1125). A command such assh -c 'sleep 10 & exit 0'therefore has its background child killed under--jobs > 1, unlike the existing sequentialCommand::statuspath; the new--jobscontract only documents buffering. Please preserve the no-timeout child lifecycle or document this behavior explicitly.
let mut tree = match spawn_tree(command) {
Ok(tree) => tree,
Err(error) => {
crates/cargo-each/src/run.rs:189
- Workspace placeholder expansion happens after
{manifest}is substituted. If the workspace/member path legally contains the literal{workspace-rust-version}text, this second pass rewrites the inserted manifest path as well, so a valid{manifest}becomes wrong. Expand the workspace token against the original command argument before inserting member-derived values; target mode needs the same ordering.
let mut next_index = 0;
crates/cargo-each/src/run.rs:209
- Target mode repeats the same replacement ordering:
{manifest}is inserted before the workspace-token pass. A legal member path containing{workspace-rust-version}is therefore rewritten into an invalid path whenever both placeholders are used. Apply the workspace-token replacement to the original argument before inserting target/member values here as well.
if !keep_going && outcome.outcome.result.failed() {
crates/cargo-each/src/substitute.rs:97
- This helper is called after
{manifest}and other per-package substitutions, so it also rewrites the replacement text. A workspace path such as/tmp/{workspace-rust-version}/Cargo.tomlmakes an originalecho {manifest}invocation either fail as unresolved (when the token was not in the command) or silently alter the path. Replace this workspace token on the original argument before inserting other placeholder values, or otherwise keep inserted values from being rescanned.
fn replace_workspace_rust_version(arg: String, placeholders: &Placeholders) -> Result<String, EachError> {
if !arg.contains(WORKSPACE_RUST_VERSION_TOKEN) {
return Ok(arg);
}
let version = placeholders
.workspace_rust_version()
.ok_or_else(|| WorkspaceRustVersionError::new("the command uses the placeholder but its root value was not resolved".to_owned()))?;
Ok(arg.replace(WORKSPACE_RUST_VERSION_TOKEN, version))
- Files reviewed: 13/14 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (97.6%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
=======================================
Coverage 97.6% 97.6%
=======================================
Files 304 306 +2
Lines 69683 71310 +1627
=======================================
+ Hits 68016 69634 +1618
- Misses 1667 1676 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved process-execution and resource-handling issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
crates/cargo-each/src/run.rs:361
- The parallel runner unconditionally sets child stdin to
Stdio::null(), whereas the existing sequentialCommand::status()path inherits the caller's stdin. Consequently, opting into--jobs > 1changes commands that consume input to see EOF immediately, which can silently break otherwise valid commands. Preserve the prior stdin behavior or make this non-interactive contract explicit and reject/document input-consuming commands.
let _ = command.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped());
crates/cargo-each/src/run.rs:213
- The scheduler retains every
BufferedOutcomeinoutcomesuntil the entire plan has finished, and only emits them afterward. With--keep-goingthis makes captured stdout/stderr grow with the total output of every selected package rather than with--jobs; a workspace-wide test run can therefore exhaust cargo-each's memory even though concurrency is bounded. Spool completed output to per-invocation temporary storage, or otherwise add a bounded retention strategy that still preserves plan-order emission.
let mut outcomes = Vec::with_capacity(invocations.len());
while let Some(outcome) = wait_for_worker(&mut workers) {
if failure_stops_launching(keep_going, outcome.outcome.result.failed()) {
stop_launching = true;
}
outcomes.push(outcome);
crates/cargo-each/src/run.rs:574
- The captured reader treats every
readerror as fatal, includingErrorKind::Interrupted. A signal can transiently interrupt a pipe read, and the repository's sibling process-tree reader explicitly retries this condition (crates/cargo-gamma-process/src/process_tree.rs:1359-1362); here it instead turns a successful invocation into an infrastructure failure and drops the remaining output. Retry interrupted reads before returning other errors.
let read = match stream.read(&mut chunk)? {
0 => return Ok(()),
read => read,
};
crates/cargo-each/src/run.rs:414
- This no-timeout branch still waits through
ProcessTree::observe, whose contract kills surviving descendants when the leader exits. Consequently, requesting--jobs > 1changes a successful command's child-process semantics: a command that intentionally leaves a background child running is terminated, whereas the default sequential path usesCommand::status()and leaves it running. The documented process-tree termination is scoped to timed-out commands; either preserve the no-timeout behavior or document and test this additional ownership of descendants.
let tree_outcome = match timeout {
Some(timeout) => wait_for_tree(&mut tree, timeout),
None => wait_for_tree_without_timeout(&mut tree),
};
crates/cargo-each/src/substitute.rs:213
- This comment is no longer accurate:
{workspace-rust-version}is a valid placeholder in this branch and is substituted on line 217. Narrow the statement to the{packages}token so it describes the actual validation guarantee.
Placeholders::Once { packages, .. } => {
// Validation above guarantees each arg is either exactly
// `{packages}` or contains no placeholder token at all.
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved process cleanup, scheduling, reader-lifecycle, and capacity-handling issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
crates/cargo-each/src/run.rs:228
- With
keep_goingenabled, a worker-thread creation error setsstop_launching, so pending invocations are never attempted and the function returns anAppErrorinstead of an invocation outcome. That contradicts the documented complete-plan/exit-1 contract and differs from ordinary command-spawn failures, which are recorded per invocation; record this index as an infrastructure outcome and continue scheduling underkeep_going(while retaining the fail-fast abort).
Err(error) => {
launch_error = Some(error);
stop_launching = true;
}
crates/cargo-each/src/run.rs:759
- When the drain deadline expires,
retainingis set false, but the reader may already be blocked instream.read; dropping theJoinHandleonly detaches that thread and leaves its pipe handle alive. A descendant that keeps the pipe open indefinitely therefore leaks one blocked thread and read handle per stream, so repeated--jobs/--keep-goinginvocations can eventually exhaust process resources. Close/interrupt the stream before detaching, or use a reader ownership design with a cancellable read.
drop(thread);
crates/cargo-each/src/run.rs:191
cargo_gamma_process::capacity()is the Unix limit for watched process-tree slots, but this branch applies it even whentimeoutisNone; in that moderun_capturedcreates an ordinaryChildand never allocates aProcessTree. On Unix, requesting--jobsabove that unrelated watcher capacity is therefore silently throttled. Apply this cap only when timed containment is actually selected.
let worker_count = jobs.get().min(invocations.len()).min(cargo_gamma_process::capacity().max(1));
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
Parallel output capture and process cleanup require fixes, and stdin behavior needs a defined contract.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
crates/cargo-each/src/run.rs:559
- This error branch drops the local
Childimmediately and returns; unlike the setup-failure paths above, it never callsterminate_ordinary_child. If a wait error occurs before the leader is reaped, the no-timeout parallel path can report an infrastructure failure while the invocation is still running. Run the same bounded cleanup here and retain the wait error, plus any cleanup error, in the result.
Err(error) => TreeOutcome::new(InvocationResult::Infrastructure(format!(
"failed to wait for child process: {error}"
))),
crates/cargo-each/src/run.rs:191
cargo_gamma_process::capacity()is the Unix interrupt-watch limit, but thetimeout == Nonebranch launches ordinaryChilds directly at lines 364-369 and never consumes those slots. Capping every parallel run by this value unnecessarily throttles ordinary--jobs Nexecutions (and limits them to the registry capacity on Unix); apply the cap only when timeout-basedProcessTrees are used.
let worker_count = jobs.get().min(invocations.len()).min(cargo_gamma_process::capacity().max(1));
crates/cargo-each/src/run.rs:774
- On the drain deadline this only flips a flag and drops the
JoinHandle; it cannot interrupt the reader's already-blockedstream.read, because the stream is owned by that detached thread. A descendant that keeps a pipe open indefinitely therefore leaves one live thread and file descriptor per timed-out stream, so repeated--keep-goinginvocations can accumulate resources despite the one-second bound. The reader needs a cancellable/closable ownership path, or the resource lifetime must be bounded another way.
Err(mpsc::RecvTimeoutError::Timeout) => {
retaining.store(false, Ordering::Release);
Some(format!(
"child {stream} remained open for more than {} ms after the {boundary} completed; partial output was retained",
grace.as_millis()
crates/cargo-each/src/run.rs:363
- Parallel workers unconditionally replace stdin with
Stdio::null(), while the sequentialrun_streamedpath leaves stdin inherited. Consequently the same arbitrary command receives caller input with--jobs 1but EOF with--jobs > 1, and the new CLI/docs do not state this change in input semantics. Please define/document the stdin contract or provide an explicit handling policy for commands that read stdin.
let _ = command.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped());
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🔵 Needs a closer look
Four unresolved moderate issues in run.rs affect stdin behavior, parallelism limits, child cleanup, and reader-thread resources.
Review details
Suppressed comments (4)
crates/cargo-each/src/run.rs:371
- The parallel runner unconditionally replaces the caller's stdin with
Stdio::null(). A command run with--jobs > 1therefore receives EOF even when the plan contains only one invocation, whereas the sequential path leaves stdin inherited (run_streamedatcrates/cargo-each/src/run.rs:327-335). This is an observable behavior change for commands that read stdin; either preserve stdin for the single-invocation case and define/reject it for multiple workers, or document and test the new contract.
let _ = command.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped());
crates/cargo-each/src/run.rs:191
- This clamps every parallel run to the process-tree interrupt registry capacity, even when
timeoutisNone. In the no-timeout branch below, workers use ordinaryCommand::spawn(crates/cargo-each/src/run.rs:372-377) and do not consume process-tree watch slots, so an explicit--jobslarger than that capacity is silently serialized to the platform ceiling for no reason. Apply the capacity limit only to timed/contained workers so ordinary parallel runs can use the requested bound.
let worker_count = jobs.get().min(invocations.len()).min(cargo_gamma_process::capacity().max(1));
crates/cargo-each/src/run.rs:567
- If
Child::wait()returns an error, this arm has already taken the child out ofCapturedProcessand returns without terminating it. The subsequentdrop(process)therefore seesOrdinary(None), so a still-running child (and its descendants) can outlive cargo-each; the injectedCaptureFault::WaitFailureexercises this path. Attempt bounded termination here and preserve any cleanup error alongside the wait error.
match waited {
Ok(status) => TreeOutcome::new(InvocationResult::Exited(status)),
Err(error) => TreeOutcome::new(InvocationResult::Infrastructure(format!(
"failed to wait for child process: {error}"
))),
crates/cargo-each/src/run.rs:783
- This only flips
retaining; if a background or escaped descendant keeps the pipe open, the reader can remain blocked instream.read, and theJoinHandleis then dropped below without stopping that thread. Each such invocation can therefore leave a live reader thread and pipe handle behind even though the scheduler returns, so repeated parallel runs can exhaust process resources. Please make the pipe read cancellable/closeable from the owning side, or use a bounded reusable reader strategy, before detaching.
Err(mpsc::RecvTimeoutError::Timeout) => {
retaining.store(false, Ordering::Release);
Some(format!(
"child {stream} remained open for more than {} ms after the {boundary} completed; partial output was retained",
grace.as_millis()
))
- Files reviewed: 17/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
One critical and four moderate unresolved findings affect process reaping, timeout semantics, memory bounds, stdin behavior, and reader cleanup.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
crates/cargo-each/src/run.rs:215
- The 1 MiB limit is only per stream, while every
BufferedOutcomeis retained inoutcomesuntil all workers finish and deterministic emission begins. A large plan whose commands each produce less than 1 MiB can therefore accumulate output proportional to the entire plan, defeating the bounded-memory expectation in the design (crates/cargo-each/docs/design/README.md:305-320) and potentially exhausting the runner. Please drain completed outcomes in plan order as soon as the next contiguous result is available, or enforce a global output budget/spill policy.
let mut outcomes = Vec::with_capacity(invocations.len());
while let Some(outcome) = wait_for_worker(&mut workers) {
if failure_stops_launching(keep_going, outcome.outcome.result.failed()) {
stop_launching = true;
}
crates/cargo-each/src/run.rs:372
- Parallel execution unconditionally replaces the caller's stdin with
Stdio::null(), so a command that reads stdin receives EOF when--jobs > 1; the sequential path still inherits stdin throughCommand::status(). That makes execution depend on the concurrency setting, while the documented parallel behavior says commands retain ordinary direct-child semantics (src/main.rs:117-124). Preserve an explicit stdin policy across modes or document/reject this incompatible behavior.
let _ = command.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped());
crates/cargo-gamma-process/src/process_tree.rs:1357
- At the deadline, a failed kill returns an error with
error.kind()(for example,PermissionDenied) instead ofio::ErrorKind::TimedOut, even though this method documents deadline results as timed-out errors. PreserveTimedOutas the error kind while retaining the kill failure in the message so callers can distinguish a bounded deadline from an immediate cleanup failure.
return Err(kill_error.take().map_or_else(
|| io::Error::new(io::ErrorKind::TimedOut, deadline_error.clone()),
|error| io::Error::new(error.kind(), format!("{error}; {deadline_error}")),
));
- Files reviewed: 17/18 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
A critical unbounded wait and a moderate --keep-going test mismatch remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 2
- Review effort level: Lite
Exercise the timed streamed runner through an injected test spawner so Linux hosts without delegated cgroups validate the logic without weakening production timeout refusal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🔵 Needs a closer look
Reaper startup performs potentially blocking work inside the Unix interrupt spawn window, which can defer terminal-signal handling under resource contention.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-gamma-process/src/process_tree.rs:450
ensure_reaper()now runs while the UnixSpawnGuard::spawningwindow is held. That window's contract explicitly forbids covering work that cannot create a child because an interrupt is deferred (crates/cargo-gamma-unsafe/src/interrupt.rs:35-42); creating the reaper thread can block or fail beforeCommand::spawn, so resource contention can delay terminal-signal handling. Move this preflight beforewindow()is opened (or close and reopen the guard around it).
- Files reviewed: 21/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
Keep children queued when try_wait is interrupted while continuing to warn and stop tracking on permanent observation failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🔵 Needs a closer look
The new public cargo-gamma-unsafe pipe surface is not documented in that crate’s design or implementation docs.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-gamma-unsafe/src/lib.rs:78
- This adds a new public OS-specific interface for interruptible child-pipe reads, but
cargo-gamma-unsafe/docs/DESIGN.mdstill describes the crate only in terms of process-subtree termination and cgroup/job containment (:8-15,:21-30,:37-41). The repository rule inAGENTS.md:13-17and:73-76requires externally observable behavior to be documented with the implementation; add the pipe's portability and readiness/ownership contract to the unsafe crate's design/implementation docs.
- Files reviewed: 21/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
Also give the sequential timeout integration probe enough startup time under instrumented CI while preserving its five-second timeout behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🔵 Needs a closer look
It changes cross-platform process containment, concurrent scheduling, pipe I/O, and child reaping, requiring final human validation.
Review details
- Files reviewed: 21/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
martinhavelka (wukchung)
left a comment
There was a problem hiding this comment.
🤖 Independent multi-model review with per-finding verification against the current branch. Four findings survived; two are reproduced against a build of this branch. Everything else raised in earlier rounds verified as correctly and completely addressed.
Use effective concurrency for stream mode, defer workspace-version resolution for empty plans, keep cancelled output recovery nonblocking, and make detached reaper diagnostics restart-safe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🔵 Needs a closer look
It spans cross-platform unsafe pipe handling, process-tree lifecycle guarantees, concurrent scheduling, and timeout cleanup.
Review details
- Files reviewed: 21/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
Restore the Unix test Write import and use repository-approved standard-input terminology in public documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
Reaper handoff failures can discard child ownership, and one bounded-termination failure path can fall into blocking ProcessTree::Drop cleanup.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 21/23 changed files
- Comments generated: 2
- Review effort level: Lite
|
Move recovered children into a process-wide retry owner so ordinary and contained cleanup paths remain bounded without dropping the last wait handle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fc919b-99f7-4134-aad1-2116321b4e0c
There was a problem hiding this comment.
🟡 Changes recommended
A reaper unwind race can strand children in the retry queue, and the new pipe backend lacks corresponding unsafe-crate design documentation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 21/23 changed files
- Comments generated: 2
- Review effort level: Lite
| let mut reaper = CHILD_REAPER.lock().unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| let mut active = std::mem::take(&mut reaper.children); | ||
| reaper.retry_children.append(&mut active); | ||
| reaper.running = false; | ||
| CHILD_REAPER_READY.notify_all(); |
| pub mod interrupt; | ||
| #[cfg(windows)] | ||
| pub mod job; | ||
| pub mod pipe; |
🤖 Prepares cargo-each to replace shell-heavy workspace orchestration in generated tooling.
--package-fileselections, including Windows-style leading UTF-8 BOMs{workspace-rust-version}resolution and validation--jobs Nexecution plus machine-relative--jobs auto, with deterministic spill-backed output--timeoutwith sealed process-tree containment and durable background reapingThis implements the cargo-each portion of #173. It remains independent of cargo-delta #32; generated impact producers must first adopt the documented one-
name@version-per-line package-file format before switching recipes to the planned examples.