diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f2a4f5e..2156502 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -246,8 +246,10 @@ jobs: # antivirus quarantines every freshly built test binary that drives a # JSON-RPC loop. This step is the only place they run, so it is the # gate. New feature plugins under crates/plugins/ go on this list. + # --no-fail-fast: a failure in one crate otherwise leaves every crate + # after it untested, and a release run should name all of them. run: >- - cargo test --locked + cargo test --locked --no-fail-fast -p rebon-core -p rebon-harness -p rebon-tool -p rebon-acp -p rebon-agent-core -p rebon-slash-commands -p rebon-acp-client -p rebon-config -p rebon-proto diff --git a/crates/plugins/sandbox/src/container.rs b/crates/plugins/sandbox/src/container.rs index 63a2c7b..f2354bf 100644 --- a/crates/plugins/sandbox/src/container.rs +++ b/crates/plugins/sandbox/src/container.rs @@ -454,6 +454,9 @@ mod tests { } } + // Only a Windows `Path` splits `C:\node\node.exe` at its backslash; on + // macOS Node's folder would read as empty, and only Windows runs this. + #[cfg(windows)] #[test] fn on_windows_the_host_runs_under_the_helper_with_a_pipe_its_reads_and_no_network() { let features = crate::runtime::windows::SandboxWinFeatures { diff --git a/crates/rebon-tool/src/shell_process.rs b/crates/rebon-tool/src/shell_process.rs index 6ef40ae..5f25286 100644 --- a/crates/rebon-tool/src/shell_process.rs +++ b/crates/rebon-tool/src/shell_process.rs @@ -31,6 +31,12 @@ const OUTPUT_RESPONSE_BYTES: usize = 64 * 1024; const MONITOR_LINE_BUFFER_BYTES: usize = 1024 * 1024; const MAX_RUNNING_SHELLS_PER_OWNER: usize = 16; const MAX_COMPLETED_SHELLS_PER_OWNER: usize = 64; +/// How long the output readers get to reach end of file once the process +/// group is gone. They normally reach it at once; they do not while a +/// process outside the group holds a pipe's write end. On macOS that can be +/// any process another thread spawned while the pipe was being made: without +/// `pipe2`, std marks a new pipe close-on-exec in a second step. +const READER_DRAIN_GRACE: Duration = Duration::from_secs(2); #[derive(Debug, Clone, Copy)] pub(crate) enum ShellOutputEncoding { @@ -1416,15 +1422,36 @@ async fn supervise_process( if let Err(err) = entry.process_tree.terminate() { entry.record_error(format!("failed to clean up process tree: {err}")); } - if let Err(err) = stdout_task.await { - entry.record_error(format!("background stdout reader failed: {err}")); - } - if let Err(err) = stderr_task.await { - entry.record_error(format!("background stderr reader failed: {err}")); - } + let drain_deadline = Instant::now() + READER_DRAIN_GRACE; + drain_reader(stdout_task, drain_deadline, &entry, "stdout").await; + drain_reader(stderr_task, drain_deadline, &entry, "stderr").await; entry.finish(wait_result); } +/// Let a reader finish what the pipe still holds, but no longer than +/// `deadline`: whatever keeps the pipe open past it is not the shell's. +/// Cutting it off is not an error, so a command that exited cleanly still +/// reads as exited. +async fn drain_reader( + mut reader: JoinHandle<()>, + deadline: Instant, + entry: &ShellEntry, + stream: &str, +) { + match tokio::time::timeout_at(deadline, &mut reader).await { + Ok(Ok(())) => {} + Ok(Err(err)) => entry.record_error(format!("background {stream} reader failed: {err}")), + Err(_) => { + tracing::debug!( + shell_id = %entry.shell_id, + stream, + "output cut off: pipe held open outside the process group" + ); + reader.abort(); + } + } +} + fn running_count(entries: &HashMap>, owner: &ShellOwner) -> usize { entries .values() @@ -2853,6 +2880,35 @@ mod tests { assert_eq!(lock(&controller.monitor_events).len(), 1); } + /// A process outside the group can hold stdout open after the group is + /// gone; on macOS any process spawned while the pipe was made can. Perl + /// stands in for one here: `setpgrp` leaves the group the stop reaps, + /// and it keeps the pipe for longer than the test waits. + #[cfg(unix)] + #[tokio::test] + async fn command_monitor_auto_stop_does_not_wait_for_a_pipe_held_outside_the_group() { + let _env = crate::test_env::hold_env(); + let script = "perl -e 'setpgrp(0, 0); sleep 10' & sleep 0.5; printf 'event\\n'; sleep 30"; + let registry = ShellProcessRegistry::new(); + let controller = Arc::new(RecordingTaskController::default()); + *lock(&controller.monitor_disposition) = Some(MonitorEventDisposition::AutoStop); + let context = context("session-a").with_task_runtime_controller(controller.clone()); + registry + .spawn_monitor( + &context, + configured_command(script), + script.into(), + "noisy events".into(), + "shell command (redacted)".into(), + None, + ) + .await + .unwrap(); + + let completion = wait_for_monitor_completion(&controller).await; + assert_eq!(completion.status, MonitorTaskCompletionStatus::AutoStopped); + } + #[test] fn output_ring_reports_stale_cursors() { let process_tree = Arc::new(ProcessTreeControl::finished_without_a_tree());