From 2af82b17807fdc2bf743003d81d44fd33552746f Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 20:13:38 -0400 Subject: [PATCH 1/7] Share Command execution ownership for capture and streaming (#466) --- src/harness.rs | 1 - src/harness/adapter.rs | 2 +- src/harness/process.rs | 287 +----- src/harness/process/tests.rs | 418 --------- src/main.rs | 1 + src/process.rs | 422 +++++++++ src/process/faults.rs | 49 + src/process/tests.rs | 840 ++++++++++++++++++ .../process_tree.rs => process/tree.rs} | 4 +- 9 files changed, 1336 insertions(+), 688 deletions(-) delete mode 100644 src/harness/process/tests.rs create mode 100644 src/process.rs create mode 100644 src/process/faults.rs create mode 100644 src/process/tests.rs rename src/{harness/process_tree.rs => process/tree.rs} (96%) diff --git a/src/harness.rs b/src/harness.rs index 69de9d129..dc7c81c0f 100644 --- a/src/harness.rs +++ b/src/harness.rs @@ -21,7 +21,6 @@ pub(crate) mod interpretation_tests; pub(crate) mod muse; pub(crate) mod opencode; pub(crate) mod process; -mod process_tree; pub(crate) mod settings; mod skill_load; diff --git a/src/harness/adapter.rs b/src/harness/adapter.rs index 7bcea19fb..04468b604 100644 --- a/src/harness/adapter.rs +++ b/src/harness/adapter.rs @@ -34,7 +34,7 @@ pub trait Adapter: Sync { /// Stop the session and its descendants, including commands that made /// their own process group or session, with this Harness's stop signals. fn stop(&self, child: &mut Child) { - super::process_tree::stop(child, self.stop_signals()); + crate::process::stop(child, self.stop_signals()); } /// Fixed overrides, also used by the adapter's Model and Effort check. diff --git a/src/harness/process.rs b/src/harness/process.rs index 888f20bab..8fbb43fca 100644 --- a/src/harness/process.rs +++ b/src/harness/process.rs @@ -1,285 +1,40 @@ -//! Own a Harness child and all its I/O until completion or whole-tree cleanup. +//! Prepare Harness environment and stop behavior for the shared execution owner. -use std::io::{Read, Write}; -use std::os::unix::process::CommandExt; -use std::process::{Child, ChildStdout, Command, ExitStatus, Output, Stdio}; -use std::sync::mpsc; -use std::thread::{self, JoinHandle}; -use std::time::Duration; +use std::process::{ChildStdout, Command, Output}; -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::Result; use super::Adapter; -use crate::interrupt; +use crate::process::{self, Interruption, Streamed}; -#[cfg(test)] -mod tests; - -/// Capture both output pipes without interpreting the child's exit status. pub(super) fn output( adapter: &dyn Adapter, command: &mut Command, input: Option<&str>, ) -> Result { - let cli = adapter.name(); - let (status, stdout, stderr) = execute(adapter, command, input, true, move |pipe| { - read(pipe).with_context(|| format!("could not read {cli}'s stdout")) - })?; - Ok(Output { - status, - stdout, - stderr, - }) -} - -/// Recovered consumer state accompanies an independent execution result. -#[derive(Debug)] -pub(crate) struct Streamed { - pub execution: Result, - pub state: T, + process::output( + command.envs(adapter.environment().iter().copied()), + input.map(str::as_bytes), + adapter.name(), + Interruption::Ordinary, + &|child| adapter.stop(child), + ) } -/// Consume stdout concurrently with prompt I/O, inheriting stderr. Preserve -/// state after any normal consumer return, even when execution fails. Return -/// only after completion or whole-tree cleanup and every worker is joined. -/// A consumer panic or failure before it starts returns no state. Recorded -/// interruption suppresses recovery even after a normal consumer return. pub(crate) fn streaming( adapter: &dyn Adapter, command: &mut Command, input: Option<&str>, - mut state: T, + state: T, consume: impl FnOnce(ChildStdout, &mut T) -> Result<()> + Send + 'static, ) -> Result> { - // One send fits without waiting for the caller, which must first finish - // supervising and joining all workers, including on transport failure. - let (completed, recovered) = mpsc::sync_channel(1); - let execution = execute(adapter, command, input, false, move |pipe| { - let result = consume(pipe, &mut state); - let _ = completed.send(state); - result - }) - .map(|(status, (), _)| status); - interrupt::check()?; - match recovered.try_recv() { - Ok(state) => Ok(Streamed { execution, state }), - Err(_) => Err(execution.expect_err("a successful consumer always returns its state")), - } -} - -fn read(mut pipe: impl Read) -> std::io::Result> { - let mut bytes = Vec::new(); - pipe.read_to_end(&mut bytes)?; - Ok(bytes) -} - -fn execute( - adapter: &dyn Adapter, - command: &mut Command, - input: Option<&str>, - capture_stderr: bool, - consume: impl FnOnce(ChildStdout) -> Result + Send + 'static, -) -> Result<(ExitStatus, T, Vec)> { - if interrupt::requested() { - bail!("interrupted"); - } - let cli = adapter.name(); - let child = command - .envs(adapter.environment().iter().copied()) - .process_group(0) - .stdin(if input.is_some() { - Stdio::piped() - } else { - Stdio::null() - }) - .stdout(Stdio::piped()) - .stderr(if capture_stderr { - Stdio::piped() - } else { - Stdio::inherit() - }) - .spawn() - .with_context(|| format!("could not run {cli}"))?; - // Install ownership immediately: every fallible step after spawn is - // covered, including pipe extraction and starting workers. - let mut owned = Execution { - adapter, - child, - stdout: None, - stderr: None, - stdin: None, - completed: false, - }; - let result = (|| { - let pipe = owned - .child - .stdout - .take() - .context("no stdout from Harness")?; - let reader = if capture_stderr { - format!("{cli}'s stdout reader") - } else { - "the session stream reader".to_string() - }; - owned.stdout = Some(Worker::start(reader, move || consume(pipe))?); - if capture_stderr { - let pipe = owned.child.stderr.take().context("no captured stderr")?; - owned.stderr = Some(Worker::start( - format!("{cli}'s stderr reader"), - move || read(pipe).with_context(|| format!("could not read {cli}'s stderr")), - )?); - } - if let Some(input) = input { - let mut pipe = owned.child.stdin.take().context("no prompt stdin")?; - let input = input.to_string(); - owned.stdin = Some(Worker::start( - format!("{cli}'s prompt writer"), - move || { - pipe.write_all(input.as_bytes()) - .with_context(|| format!("could not write the prompt to {cli}")) - // Dropping the pipe closes stdin, including for an empty prompt. - }, - )?); - } - let status = owned.wait()?; - let stdout = owned.stdout.as_mut().unwrap().value.take().unwrap(); - let stderr = owned - .stderr - .as_mut() - .and_then(|worker| worker.value.take()) - .unwrap_or_default(); - Ok((status, stdout, stderr)) - })(); - if result.is_err() || interrupt::requested() { - owned.cleanup(); - // Interruption can also arrive during stopping or joining. Preserve - // the original transport failure unless an interruption was observed. - if interrupt::requested() { - bail!("interrupted"); - } - } else { - owned.completed = true; - } - result -} - -struct Execution<'a, T> { - adapter: &'a dyn Adapter, - child: Child, - stdout: Option>, - stderr: Option>>, - stdin: Option>, - completed: bool, -} - -impl Execution<'_, T> { - fn wait(&mut self) -> Result { - let mut status = None; - loop { - if interrupt::requested() { - bail!("interrupted"); - } - if status.is_none() { - status = self - .child - .try_wait() - .with_context(|| format!("could not wait for {}", self.adapter.name()))?; - } - let stdin_done = self - .stdin - .as_mut() - .map(Worker::poll) - .transpose()? - .unwrap_or(true); - let stdout_done = self.stdout.as_mut().unwrap().poll()?; - let stderr_done = self - .stderr - .as_mut() - .map(Worker::poll) - .transpose()? - .unwrap_or(true); - if let Some(status) = status - && stdin_done - && stdout_done - && stderr_done - { - if interrupt::requested() { - bail!("interrupted"); - } - return Ok(status); - } - thread::sleep(Duration::from_millis(100)); - } - } - - fn cleanup(&mut self) { - if self.completed { - return; - } - self.adapter.stop(&mut self.child); - // Pipes may still be attached if worker startup failed. - drop(self.child.stdin.take()); - drop(self.child.stdout.take()); - drop(self.child.stderr.take()); - if let Some(worker) = &mut self.stdin { - worker.join(); - } - if let Some(worker) = &mut self.stdout { - worker.join(); - } - if let Some(worker) = &mut self.stderr { - worker.join(); - } - self.completed = true; - } -} - -impl Drop for Execution<'_, T> { - fn drop(&mut self) { - self.cleanup(); - } -} - -struct Worker { - handle: Option>>, - value: Option, - name: String, -} - -impl Worker { - fn start(name: String, work: impl FnOnce() -> Result + Send + 'static) -> Result - where - T: Send + 'static, - { - let handle = thread::Builder::new() - .spawn(work) - .with_context(|| format!("could not start {name}"))?; - Ok(Self { - handle: Some(handle), - value: None, - name, - }) - } - - /// Join only a completed worker while supervising the child. Its failure - /// or panic must initiate cleanup even if the child has not exited yet. - fn poll(&mut self) -> Result { - if self.handle.as_ref().is_some_and(JoinHandle::is_finished) { - self.value = Some(self.handle.take().unwrap().join().map_err(|panic| { - let cause = panic - .downcast_ref::() - .map(String::as_str) - .or_else(|| panic.downcast_ref::<&str>().copied()) - .unwrap_or("unknown panic"); - anyhow!("{} panicked: {cause}", self.name) - })??); - } - Ok(self.handle.is_none()) - } - - fn join(&mut self) { - if let Some(handle) = self.handle.take() { - let _ = handle.join(); - } - } + process::streaming( + command.envs(adapter.environment().iter().copied()), + input.map(str::as_bytes), + adapter.name(), + Interruption::Ordinary, + &|child| adapter.stop(child), + state, + consume, + ) } diff --git a/src/harness/process/tests.rs b/src/harness/process/tests.rs deleted file mode 100644 index aafa97cb5..000000000 --- a/src/harness/process/tests.rs +++ /dev/null @@ -1,418 +0,0 @@ -//! Behavior through the same two operations used by sessions and checks. - -use super::*; -use crate::harness::Harness; - -use std::fs; -use std::path::Path; -use std::time::Instant; - -use tempfile::TempDir; - -fn shell(script: &str) -> Command { - let mut command = Command::new("bash"); - command.args(["-c", script]); - command -} - -#[test] -fn captured_completion_preserves_both_byte_streams_and_nonzero_status() { - let result = output( - Harness::OpenCode.adapter(), - &mut shell("printf '\\377out\\000\\n'; printf '\\376err\\000\\n' >&2; exit 7"), - None, - ) - .unwrap(); - - assert_eq!(result.status.code(), Some(7)); - assert_eq!(result.stdout, b"\xffout\0\n"); - assert_eq!(result.stderr, b"\xfeerr\0\n"); -} - -#[test] -fn streamed_completion_preserves_raw_bytes_and_nonzero_status_in_consumer_state() { - let consumed = streaming( - Harness::Grok.adapter(), - &mut shell("printf 'unknown\\n\\377malformed\\n'; exit 9"), - None, - (Vec::new(), "initial"), - |pipe, state| { - state.0 = read(pipe)?; - state.1 = "finished"; - Ok(()) - }, - ) - .unwrap(); - - assert_eq!(consumed.execution.unwrap().code(), Some(9)); - assert_eq!( - consumed.state, - (b"unknown\n\xffmalformed\n".to_vec(), "finished") - ); -} - -#[test] -fn captured_prompt_is_null_when_absent_and_written_once_then_closed_when_present() { - for (input, expected) in [ - (None, "nullclosed"), - (Some(""), "closed"), - (Some("once"), "onceclosed"), - ] { - let result = output( - Harness::OpenCode.adapter(), - &mut shell("if [ /dev/fd/0 -ef /dev/null ]; then printf null; fi; cat; printf closed"), - input, - ) - .unwrap(); - assert!(result.status.success()); - assert_eq!(result.stdout, expected.as_bytes()); - } -} - -/// Cleanup uses only this fixture's recorded PID, including on assertion failure. -struct RecordedProcess(TempDir); - -impl RecordedProcess { - fn new() -> Self { - Self(tempfile::tempdir().unwrap()) - } - - fn command(&self, script: &str) -> Command { - let mut command = shell(script); - command.env("PID_FILE", self.0.path().join("pid")); - command - } - - fn pid(&self) -> libc::pid_t { - fs::read_to_string(self.0.path().join("pid")) - .unwrap() - .trim() - .parse() - .unwrap() - } - - fn assert_stopped(&self) { - assert!(!exists(self.pid()), "the owned child survived cleanup"); - } -} - -fn exists(pid: libc::pid_t) -> bool { - // SAFETY: signal 0 probes only the exact PID recorded by this test. - unsafe { libc::kill(pid, 0) == 0 } -} - -impl Drop for RecordedProcess { - fn drop(&mut self) { - if let Ok(pid) = fs::read_to_string(self.0.path().join("pid")) - && let Ok(pid) = pid.trim().parse() - && exists(pid) - { - // SAFETY: this exact PID belongs to the test's finite command. - unsafe { libc::kill(pid, libc::SIGKILL) }; - } - } -} - -#[test] -fn broken_prompt_stops_and_reaps_the_child_with_the_original_stdin_error() { - let child = RecordedProcess::new(); - let started = Instant::now(); - let result = output( - Harness::OpenCode.adapter(), - &mut child.command("echo $$ > \"$PID_FILE\"; exec 0<&-; exec sleep 5"), - Some(&"p".repeat(1024 * 1024)), - ); - - let error = result.unwrap_err(); - assert!( - format!("{error:#}").contains("could not write the prompt to opencode"), - "{error:#}" - ); - assert_eq!( - error.downcast_ref::().unwrap().kind(), - std::io::ErrorKind::BrokenPipe - ); - assert!(started.elapsed() < Duration::from_secs(2)); - child.assert_stopped(); -} - -struct FailingLog; - -impl Write for FailingLog { - fn write(&mut self, _bytes: &[u8]) -> std::io::Result { - Err(std::io::Error::other("log device failed")) - } - - fn flush(&mut self) -> std::io::Result<()> { - Ok(()) - } -} - -#[test] -fn consumer_log_failure_cleans_up_even_with_a_blocked_prompt_writer() { - let child = RecordedProcess::new(); - let escaped = RecordedProcess::new(); - let mut command = child.command( - "set -m; trap 'wait; exit' TERM; echo $$ > \"$PID_FILE\"; \ - sleep 5 /dev/null 2>&1 & echo $! > \"$DESCENDANT_PID_FILE\"; \ - printf ready; wait", - ); - command.env("DESCENDANT_PID_FILE", escaped.0.path().join("pid")); - let started = Instant::now(); - let result = streaming( - Harness::OpenCode.adapter(), - &mut command, - Some(&"p".repeat(1024 * 1024)), - b"initial".to_vec(), - |mut pipe, state| { - let mut bytes = [0; 5]; - pipe.read_exact(&mut bytes)?; - state.extend_from_slice(&bytes); - FailingLog - .write_all(&bytes) - .context("could not write Session log") - }, - ) - .unwrap(); - - assert_eq!(result.state, b"initialready"); - assert_eq!( - format!("{:#}", result.execution.unwrap_err()), - "could not write Session log: log device failed" - ); - assert!(started.elapsed() < Duration::from_secs(2)); - child.assert_stopped(); - escaped.assert_stopped(); -} - -#[test] -fn prompt_failure_retains_state_and_precedes_a_consumer_failure_during_cleanup() { - let child = RecordedProcess::new(); - let result = streaming( - Harness::OpenCode.adapter(), - &mut child.command("echo $$ > \"$PID_FILE\"; printf ready; exec 0<&-; exec sleep 5"), - Some(&"p".repeat(1024 * 1024)), - b"initial".to_vec(), - |mut pipe, state| { - pipe.read_to_end(state)?; - bail!("later consumer failure") - }, - ) - .unwrap(); - - assert_eq!(result.state, b"initialready"); - let error = result.execution.unwrap_err(); - assert_eq!(error.to_string(), "could not write the prompt to opencode"); - assert_eq!( - error.downcast_ref::().unwrap().kind(), - std::io::ErrorKind::BrokenPipe - ); - child.assert_stopped(); -} - -#[test] -fn consumer_panic_stops_and_reaps_the_child_and_reports_its_cause() { - let child = RecordedProcess::new(); - let started = Instant::now(); - let result = streaming::<()>( - Harness::OpenCode.adapter(), - &mut child.command("echo $$ > \"$PID_FILE\"; printf ready; exec sleep 5"), - None, - (), - |mut pipe, _state| { - pipe.read_exact(&mut [0; 5])?; - panic!("consumer exploded"); - }, - ); - - assert_eq!( - format!("{:#}", result.unwrap_err()), - "the session stream reader panicked: consumer exploded" - ); - assert!(started.elapsed() < Duration::from_secs(2)); - child.assert_stopped(); -} - -#[test] -fn unread_captured_prompt_remains_interruptible() { - assert_unread_prompt_interruptible( - "harness::process::tests::unread_captured_prompt_remains_interruptible", - true, - ); -} - -#[test] -fn unread_streamed_prompt_remains_interruptible() { - assert_unread_prompt_interruptible( - "harness::process::tests::unread_streamed_prompt_remains_interruptible", - false, - ); -} - -#[test] -fn eof_does_not_finish_either_operation_before_the_child_exits() { - for captured in [true, false] { - let mut command = shell("exec 1>&- 2>&-; sleep 0.3; exit 7"); - let started = Instant::now(); - let status = if captured { - output(Harness::OpenCode.adapter(), &mut command, None) - .unwrap() - .status - } else { - streaming( - Harness::OpenCode.adapter(), - &mut command, - None, - Vec::new(), - |pipe, state| { - *state = read(pipe)?; - Ok(()) - }, - ) - .unwrap() - .execution - .unwrap() - }; - assert_eq!(status.code(), Some(7)); - assert!(started.elapsed() >= Duration::from_millis(250)); - } -} - -#[test] -fn both_operations_wait_for_output_held_after_child_exit() { - for captured in [true, false] { - let mut command = shell("(sleep 0.3; printf late) & exit 7"); - let (status, bytes) = if captured { - let result = output(Harness::OpenCode.adapter(), &mut command, None).unwrap(); - (result.status, result.stdout) - } else { - let result = streaming( - Harness::OpenCode.adapter(), - &mut command, - None, - Vec::new(), - |pipe, state| { - *state = read(pipe)?; - Ok(()) - }, - ) - .unwrap(); - (result.execution.unwrap(), result.state) - }; - assert_eq!(status.code(), Some(7)); - assert_eq!(bytes, b"late"); - } -} - -#[test] -fn normal_completion_leaves_background_work_without_owned_io_running() { - for captured in [true, false] { - let work = RecordedProcess::new(); - let mut command = - work.command("sleep 5 /dev/null 2>&1 & echo $! > \"$PID_FILE\"; exit 0"); - let status = if captured { - output(Harness::OpenCode.adapter(), &mut command, None) - .unwrap() - .status - } else { - streaming( - Harness::OpenCode.adapter(), - &mut command, - None, - (), - |pipe, _state| { - read(pipe)?; - Ok(()) - }, - ) - .unwrap() - .execution - .unwrap() - }; - - assert!(status.success()); - assert!( - exists(work.pid()), - "normal completion killed background work" - ); - } -} - -#[test] -fn captured_completion_waits_for_stderr_after_stdout_and_child_exit() { - let result = output( - Harness::OpenCode.adapter(), - &mut shell("(exec 1>&-; sleep 0.3; printf late-diagnostic >&2) & exit 7"), - None, - ) - .unwrap(); - - assert_eq!(result.status.code(), Some(7)); - assert!(result.stdout.is_empty()); - assert_eq!(result.stderr, b"late-diagnostic"); -} - -fn assert_unread_prompt_interruptible(test_name: &str, captured: bool) { - const SIGNAL_DIR: &str = "THIRDSHIFT_PROCESS_TEST_SIGNAL_DIR"; - // Run this interface test alone in another process: the interruption - // predicate is process-wide and must not affect concurrent unit tests. - if let Some(dir) = std::env::var_os(SIGNAL_DIR) { - interrupt::install().unwrap(); - let owned = RecordedProcess::new(); - let mut command = - owned.command("echo $$ > \"$PID_FILE\"; touch \"$READY_FILE\"; exec sleep 5"); - command.env("READY_FILE", Path::new(&dir).join("ready")); - let prompt = "p".repeat(1024 * 1024); - let result = if captured { - output(Harness::OpenCode.adapter(), &mut command, Some(&prompt)).map(|_| ()) - } else { - streaming( - Harness::OpenCode.adapter(), - &mut command, - Some(&prompt), - Vec::new(), - |pipe, state| { - *state = read(pipe)?; - Ok(()) - }, - ) - .map(|_| ()) - }; - assert_eq!(format!("{:#}", result.unwrap_err()), "interrupted"); - owned.assert_stopped(); - return; - } - - let dir = tempfile::tempdir().unwrap(); - let mut child = Command::new(std::env::current_exe().unwrap()) - .args(["--exact", test_name, "--nocapture"]) - .env(SIGNAL_DIR, dir.path()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .unwrap(); - let deadline = Instant::now() + Duration::from_secs(10); - while !dir.path().join("ready").exists() && Instant::now() < deadline { - if child.try_wait().unwrap().is_some() { - break; - } - thread::sleep(Duration::from_millis(10)); - } - if !dir.path().join("ready").exists() { - let _ = child.kill(); - let result = child.wait_with_output().unwrap(); - panic!("the isolated test never started: {result:?}"); - } - let interrupted_at = Instant::now(); - // SAFETY: this is the exact PID of our isolated test process. - assert_eq!( - unsafe { libc::kill(child.id() as libc::pid_t, libc::SIGINT) }, - 0 - ); - let result = child.wait_with_output().unwrap(); - assert!(result.status.success(), "{result:?}"); - assert!( - interrupted_at.elapsed() < Duration::from_secs(2), - "the unread prompt prevented interruption" - ); -} diff --git a/src/main.rs b/src/main.rs index 0c4d1a145..cb524ae4a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -24,6 +24,7 @@ mod notification; mod pass; mod pickup; mod poll; +mod process; mod progress; mod prompt; #[cfg(test)] diff --git a/src/process.rs b/src/process.rs new file mode 100644 index 000000000..238f473a8 --- /dev/null +++ b/src/process.rs @@ -0,0 +1,422 @@ +//! Own a Command CLI child and all its I/O until completion or whole-tree cleanup. + +use std::io::{Read, Write}; +use std::os::unix::process::CommandExt; +use std::process::{Child, ChildStdout, Command, ExitStatus, Output, Stdio}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, mpsc}; +use std::thread::{self, JoinHandle}; +use std::time::Duration; + +use anyhow::{Context, Result, anyhow, bail}; + +use crate::interrupt; + +mod tree; +pub(crate) use tree::stop; + +#[cfg(test)] +mod faults; +#[cfg(test)] +mod tests; + +/// Ordinary work stops for a recorded interrupt; completion work finishes +/// independently without clearing the Command's recorded request. +#[derive(Clone, Copy)] +pub(crate) enum Interruption { + Ordinary, + // The following Git/GitHub migration tickets supply production callers. + #[cfg_attr(not(test), allow(dead_code))] + Completion, +} + +impl Interruption { + fn requested(self) -> bool { + matches!(self, Self::Ordinary) && interrupt::requested() + } + + fn check(self) -> Result<()> { + if self.requested() { + bail!("interrupted"); + } + Ok(()) + } +} + +/// Capture both output pipes without interpreting the child's exit status. +pub(crate) fn output( + command: &mut Command, + input: Option<&[u8]>, + name: &str, + interruption: Interruption, + stop: &dyn Fn(&mut Child), +) -> Result { + let cli = name.to_owned(); + let (execution, _) = execute( + command, + input, + name, + interruption, + stop, + true, + move |pipe| read(pipe).with_context(|| format!("could not read {cli}'s stdout")), + ); + let (status, stdout, stderr) = execution?; + Ok(Output { + status, + stdout, + stderr, + }) +} + +/// Recovered consumer state accompanies an independent execution result. +#[derive(Debug)] +pub(crate) struct Streamed { + pub execution: Result, + pub state: T, +} + +/// Consume stdout concurrently with prompt I/O, inheriting stderr. Preserve +/// state after any normal consumer return, even when execution fails. Return +/// only after completion or whole-tree cleanup and every worker is joined. +/// A consumer panic or failure before it starts returns no state. Recorded +/// interruption suppresses recovery even after a normal consumer return. +pub(crate) fn streaming( + command: &mut Command, + input: Option<&[u8]>, + name: &str, + interruption: Interruption, + stop: &dyn Fn(&mut Child), + mut state: T, + consume: impl FnOnce(ChildStdout, &mut T) -> Result<()> + Send + 'static, +) -> Result> { + // One send fits without waiting for the caller, which must first finish + // supervising and joining all workers, including on transport failure. + let (completed, recovered) = mpsc::sync_channel(1); + let (execution, started) = execute( + command, + input, + name, + interruption, + stop, + false, + move |pipe| { + let result = consume(pipe, &mut state); + let _ = completed.send(state); + result + }, + ); + let execution = execution.map(|(status, (), _)| status); + interruption.check()?; + if !started { + return Err(execution.expect_err("startup failure cannot execute successfully")); + } + match recovered.try_recv() { + Ok(state) => Ok(Streamed { execution, state }), + Err(_) => Err(execution.expect_err("a successful consumer always returns its state")), + } +} + +fn read(mut pipe: impl Read) -> std::io::Result> { + let mut bytes = Vec::new(); + pipe.read_to_end(&mut bytes)?; + Ok(bytes) +} + +fn execute( + command: &mut Command, + input: Option<&[u8]>, + name: &str, + interruption: Interruption, + stop: &dyn Fn(&mut Child), + capture_stderr: bool, + consume: impl FnOnce(ChildStdout) -> Result + Send + 'static, +) -> (Result<(ExitStatus, T, Vec)>, bool) { + let mut started = false; + let result = (|| { + if interruption.requested() { + bail!("interrupted"); + } + let cli = name.to_owned(); + let (wake, awakened) = mpsc::channel(); + #[cfg(test)] + let fault = faults::Fault::from(command); + let child = command + .process_group(0) + .stdin(if input.is_some() { + Stdio::piped() + } else { + Stdio::null() + }) + .stdout(Stdio::piped()) + .stderr(if capture_stderr { + Stdio::piped() + } else { + Stdio::inherit() + }) + .spawn() + .with_context(|| format!("could not run {cli}"))?; + // Install ownership immediately: every fallible step after spawn is + // covered, including pipe extraction and starting workers. + let mut owned = Execution { + name, + interruption, + stop, + child, + awakened, + stdout: None, + stderr: None, + stdin: None, + completed: false, + }; + let result = (|| { + #[cfg(test)] + fault.fire("stdout-pipe")?; + let pipe = owned + .child + .stdout + .take() + .with_context(|| format!("no stdout from {name}"))?; + let reader = if capture_stderr { + format!("{cli}'s stdout reader") + } else { + "the session stream reader".to_string() + }; + #[cfg(test)] + fault.fire("stdout-start")?; + #[cfg(test)] + let stdout_fault = fault.clone(); + owned.stdout = Some(Worker::start(reader, wake.clone(), move || { + #[cfg(test)] + stdout_fault.fire("stdout-read")?; + consume(pipe) + })?); + if capture_stderr { + #[cfg(test)] + fault.fire("stderr-pipe")?; + let pipe = owned.child.stderr.take().context("no captured stderr")?; + #[cfg(test)] + fault.fire("stderr-start")?; + #[cfg(test)] + let stderr_fault = fault.clone(); + owned.stderr = Some(Worker::start( + format!("{cli}'s stderr reader"), + wake.clone(), + { + let cli = cli.clone(); + move || { + #[cfg(test)] + stderr_fault.fire("stderr-read")?; + read(pipe).with_context(|| format!("could not read {cli}'s stderr")) + } + }, + )?); + } + if let Some(input) = input { + #[cfg(test)] + fault.fire("stdin-pipe")?; + let mut pipe = owned.child.stdin.take().context("no prompt stdin")?; + #[cfg(test)] + fault.fire("stdin-start")?; + let input = input.to_vec(); + #[cfg(test)] + let stdin_fault = fault.clone(); + owned.stdin = Some(Worker::start( + format!("{cli}'s prompt writer"), + wake.clone(), + move || { + #[cfg(test)] + stdin_fault.fire("stdin-write")?; + pipe.write_all(&input) + .with_context(|| format!("could not write the prompt to {cli}")) + // Dropping the pipe closes stdin, including for an empty prompt. + }, + )?); + } + started = true; + #[cfg(test)] + fault.fire("wait")?; + let status = owned.wait()?; + let stdout = owned.stdout.as_mut().unwrap().value.take().unwrap(); + let stderr = owned + .stderr + .as_mut() + .and_then(|worker| worker.value.take()) + .unwrap_or_default(); + Ok((status, stdout, stderr)) + })(); + if result.is_err() || interruption.requested() { + owned.cleanup(); + // Interruption can also arrive during stopping or joining. Preserve + // the original transport failure unless an interruption was observed. + if interruption.requested() { + bail!("interrupted"); + } + } else { + owned.completed = true; + } + result + })(); + (interruption.check().and(result), started) +} + +struct Execution<'a, T> { + name: &'a str, + interruption: Interruption, + stop: &'a dyn Fn(&mut Child), + child: Child, + awakened: mpsc::Receiver<()>, + stdout: Option>, + stderr: Option>>, + stdin: Option>, + completed: bool, +} + +impl Execution<'_, T> { + fn wait(&mut self) -> Result { + let mut status = None; + loop { + if self.interruption.requested() { + bail!("interrupted"); + } + if status.is_none() { + status = self + .child + .try_wait() + .with_context(|| format!("could not wait for {}", self.name))?; + } + let stdin_done = self + .stdin + .as_mut() + .map(Worker::poll) + .transpose()? + .unwrap_or(true); + let stdout_done = self.stdout.as_mut().unwrap().poll()?; + let stderr_done = self + .stderr + .as_mut() + .map(Worker::poll) + .transpose()? + .unwrap_or(true); + if let Some(status) = status + && stdin_done + && stdout_done + && stderr_done + { + if self.interruption.requested() { + bail!("interrupted"); + } + return Ok(status); + } + // Worker completion (including unwind) wakes us immediately. + // EOF can precede child exit: when all I/O is joined, recheck + // the child promptly without treating EOF as completion. + let wait = if stdin_done && stdout_done && stderr_done { + 1 + } else { + 100 + }; + let _ = self.awakened.recv_timeout(Duration::from_millis(wait)); + } + } + + fn cleanup(&mut self) { + if self.completed { + return; + } + (self.stop)(&mut self.child); + let _ = self.child.wait(); + // Pipes may still be attached if worker startup failed. + drop(self.child.stdin.take()); + drop(self.child.stdout.take()); + drop(self.child.stderr.take()); + if let Some(worker) = &mut self.stdin { + worker.join(); + } + if let Some(worker) = &mut self.stdout { + worker.join(); + } + if let Some(worker) = &mut self.stderr { + worker.join(); + } + self.completed = true; + } +} + +impl Drop for Execution<'_, T> { + fn drop(&mut self) { + self.cleanup(); + } +} + +struct Worker { + handle: Option>>, + completed: Arc, + value: Option, + name: String, +} + +impl Worker { + fn start( + name: String, + wake: mpsc::Sender<()>, + work: impl FnOnce() -> Result + Send + 'static, + ) -> Result + where + T: Send + 'static, + { + let completed = Arc::new(AtomicBool::new(false)); + let notification = Completion { + completed: Arc::clone(&completed), + wake, + }; + let handle = thread::Builder::new() + .spawn(move || { + let _notification = notification; + work() + }) + .with_context(|| format!("could not start {name}"))?; + Ok(Self { + handle: Some(handle), + completed, + value: None, + name, + }) + } + + /// Join only a completed worker while supervising the child. Its failure + /// or panic must initiate cleanup even if the child has not exited yet. + fn poll(&mut self) -> Result { + if self.handle.is_some() && self.completed.load(Ordering::Acquire) { + self.value = Some(self.handle.take().unwrap().join().map_err(|panic| { + let cause = panic + .downcast_ref::() + .map(String::as_str) + .or_else(|| panic.downcast_ref::<&str>().copied()) + .unwrap_or("unknown panic"); + anyhow!("{} panicked: {cause}", self.name) + })??); + } + Ok(self.handle.is_none()) + } + + fn join(&mut self) { + if let Some(handle) = self.handle.take() { + let _ = handle.join(); + } + } +} + +/// Dropped both on a normal worker return and while unwinding. Set readiness +/// before waking so the supervisor can join without racing is_finished(). +struct Completion { + completed: Arc, + wake: mpsc::Sender<()>, +} + +impl Drop for Completion { + fn drop(&mut self) { + self.completed.store(true, Ordering::Release); + let _ = self.wake.send(()); + } +} diff --git a/src/process/faults.rs b/src/process/faults.rs new file mode 100644 index 000000000..aac0bf656 --- /dev/null +++ b/src/process/faults.rs @@ -0,0 +1,49 @@ +//! Per-command faults, only compiled into the unit-test executable. Fixtures +//! still call the owning interface rather than private workers or handles. + +use std::path::PathBuf; +use std::process::Command; +use std::thread; +use std::time::{Duration, Instant}; + +use anyhow::{Result, bail}; + +#[derive(Clone)] +pub(super) struct Fault { + point: Option, + gate: Option, +} + +impl Fault { + pub fn from(command: &Command) -> Self { + let env = |key| { + command + .get_envs() + .find(|(name, _)| *name == key) + .and_then(|(_, value)| value) + }; + Self { + point: env("THIRDSHIFT_EXECUTION_TEST_FAULT") + .map(|value| value.to_string_lossy().into_owned()), + gate: env("THIRDSHIFT_EXECUTION_TEST_GATE").map(PathBuf::from), + } + } + + pub fn fire(&self, at: &str) -> Result<()> { + let panic = self.point.as_deref() == Some(&format!("{at}-panic")); + if self.point.as_deref() != Some(at) && !panic { + return Ok(()); + } + if let Some(gate) = &self.gate { + let deadline = Instant::now() + Duration::from_secs(2); + while !gate.exists() && Instant::now() < deadline { + thread::sleep(Duration::from_millis(1)); + } + assert!(gate.exists(), "the fault fixture never reached its gate"); + } + if panic { + panic!("injected {at} panic"); + } + bail!(std::io::Error::other(format!("injected {at} failure"))); + } +} diff --git a/src/process/tests.rs b/src/process/tests.rs new file mode 100644 index 000000000..7911fe88a --- /dev/null +++ b/src/process/tests.rs @@ -0,0 +1,840 @@ +//! Behavior through the same two operations used by sessions and checks. + +use super::*; + +use std::fs; +use std::path::Path; +use std::time::Instant; + +use tempfile::TempDir; + +fn shell(script: &str) -> Command { + let mut command = Command::new("bash"); + command.args(["-c", script]); + command +} + +#[test] +fn captured_completion_preserves_both_byte_streams_and_nonzero_status() { + let result = output( + &mut shell("printf '\\377out\\000\\n'; printf '\\376err\\000\\n' >&2; exit 7"), + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + ) + .unwrap(); + + assert_eq!(result.status.code(), Some(7)); + assert_eq!(result.stdout, b"\xffout\0\n"); + assert_eq!(result.stderr, b"\xfeerr\0\n"); +} + +#[test] +fn streamed_completion_preserves_raw_bytes_and_nonzero_status_in_consumer_state() { + let consumed = streaming( + &mut shell("printf 'unknown\\n\\377malformed\\n'; exit 9"), + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + (Vec::new(), "initial"), + |pipe, state| { + state.0 = read(pipe)?; + state.1 = "finished"; + Ok(()) + }, + ) + .unwrap(); + + assert_eq!(consumed.execution.unwrap().code(), Some(9)); + assert_eq!( + consumed.state, + (b"unknown\n\xffmalformed\n".to_vec(), "finished") + ); +} + +#[test] +fn captured_prompt_is_null_when_absent_and_written_once_then_closed_when_present() { + for (input, expected) in [ + (None, "nullclosed"), + (Some(""), "closed"), + (Some("once"), "onceclosed"), + ] { + let result = output( + &mut shell("if [ /dev/fd/0 -ef /dev/null ]; then printf null; fi; cat; printf closed"), + input.map(str::as_bytes), + "fixture", + Interruption::Ordinary, + &stop_fixture, + ) + .unwrap(); + assert!(result.status.success()); + assert_eq!(result.stdout, expected.as_bytes()); + } +} + +/// Cleanup uses only this fixture's recorded PID, including on assertion failure. +struct RecordedProcess(TempDir); + +impl RecordedProcess { + fn new() -> Self { + Self(tempfile::tempdir().unwrap()) + } + + fn command(&self, script: &str) -> Command { + let mut command = shell(script); + command.env("PID_FILE", self.0.path().join("pid")); + command + } + + fn pid(&self) -> libc::pid_t { + fs::read_to_string(self.0.path().join("pid")) + .unwrap() + .trim() + .parse() + .unwrap() + } + + fn assert_stopped(&self) { + assert!(!exists(self.pid()), "the owned child survived cleanup"); + } +} + +fn exists(pid: libc::pid_t) -> bool { + // SAFETY: signal 0 probes only the exact PID recorded by this test. + unsafe { libc::kill(pid, 0) == 0 } +} + +impl Drop for RecordedProcess { + fn drop(&mut self) { + if let Ok(pid) = fs::read_to_string(self.0.path().join("pid")) + && let Ok(pid) = pid.trim().parse() + && exists(pid) + { + // SAFETY: this exact PID belongs to the test's finite command. + unsafe { libc::kill(pid, libc::SIGKILL) }; + } + } +} + +#[test] +fn broken_prompt_stops_and_reaps_the_child_with_the_original_stdin_error() { + let child = RecordedProcess::new(); + let started = Instant::now(); + let result = output( + &mut child.command("echo $$ > \"$PID_FILE\"; exec 0<&-; exec sleep 5"), + Some("p".repeat(1024 * 1024).as_bytes()), + "fixture", + Interruption::Ordinary, + &stop_fixture, + ); + + let error = result.unwrap_err(); + assert!( + format!("{error:#}").contains("could not write the prompt to fixture"), + "{error:#}" + ); + assert_eq!( + error.downcast_ref::().unwrap().kind(), + std::io::ErrorKind::BrokenPipe + ); + assert!(started.elapsed() < Duration::from_secs(2)); + child.assert_stopped(); +} + +struct FailingLog; + +impl Write for FailingLog { + fn write(&mut self, _bytes: &[u8]) -> std::io::Result { + Err(std::io::Error::other("log device failed")) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +#[test] +fn consumer_log_failure_cleans_up_even_with_a_blocked_prompt_writer() { + let child = RecordedProcess::new(); + let escaped = RecordedProcess::new(); + let mut command = child.command( + "set -m; trap 'wait; exit' TERM; echo $$ > \"$PID_FILE\"; \ + sleep 5 /dev/null 2>&1 & echo $! > \"$DESCENDANT_PID_FILE\"; \ + printf ready; wait", + ); + command.env("DESCENDANT_PID_FILE", escaped.0.path().join("pid")); + let started = Instant::now(); + let result = streaming( + &mut command, + Some("p".repeat(1024 * 1024).as_bytes()), + "fixture", + Interruption::Ordinary, + &stop_fixture, + b"initial".to_vec(), + |mut pipe, state| { + let mut bytes = [0; 5]; + pipe.read_exact(&mut bytes)?; + state.extend_from_slice(&bytes); + FailingLog + .write_all(&bytes) + .context("could not write Session log") + }, + ) + .unwrap(); + + assert_eq!(result.state, b"initialready"); + assert_eq!( + format!("{:#}", result.execution.unwrap_err()), + "could not write Session log: log device failed" + ); + assert!(started.elapsed() < Duration::from_secs(2)); + child.assert_stopped(); + escaped.assert_stopped(); +} + +#[test] +fn prompt_failure_retains_state_and_precedes_a_consumer_failure_during_cleanup() { + let child = RecordedProcess::new(); + let result = streaming( + &mut child.command("echo $$ > \"$PID_FILE\"; printf ready; exec 0<&-; exec sleep 5"), + Some("p".repeat(1024 * 1024).as_bytes()), + "fixture", + Interruption::Ordinary, + &stop_fixture, + b"initial".to_vec(), + |mut pipe, state| { + pipe.read_to_end(state)?; + bail!("later consumer failure") + }, + ) + .unwrap(); + + assert_eq!(result.state, b"initialready"); + let error = result.execution.unwrap_err(); + assert_eq!(error.to_string(), "could not write the prompt to fixture"); + assert_eq!( + error.downcast_ref::().unwrap().kind(), + std::io::ErrorKind::BrokenPipe + ); + child.assert_stopped(); +} + +#[test] +fn consumer_panic_stops_and_reaps_the_child_and_reports_its_cause() { + let child = RecordedProcess::new(); + let started = Instant::now(); + let result = streaming::<()>( + &mut child.command("echo $$ > \"$PID_FILE\"; printf ready; exec sleep 5"), + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + (), + |mut pipe, _state| { + pipe.read_exact(&mut [0; 5])?; + panic!("consumer exploded"); + }, + ); + + assert_eq!( + format!("{:#}", result.unwrap_err()), + "the session stream reader panicked: consumer exploded" + ); + assert!(started.elapsed() < Duration::from_secs(2)); + child.assert_stopped(); +} + +#[test] +fn unread_captured_prompt_remains_interruptible() { + assert_unread_prompt_interruptible( + "process::tests::unread_captured_prompt_remains_interruptible", + true, + ); +} + +#[test] +fn unread_streamed_prompt_remains_interruptible() { + assert_unread_prompt_interruptible( + "process::tests::unread_streamed_prompt_remains_interruptible", + false, + ); +} + +#[test] +fn eof_does_not_finish_either_operation_before_the_child_exits() { + for captured in [true, false] { + let mut command = shell("exec 1>&- 2>&-; sleep 0.3; exit 7"); + let started = Instant::now(); + let status = if captured { + output( + &mut command, + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + ) + .unwrap() + .status + } else { + streaming( + &mut command, + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + Vec::new(), + |pipe, state| { + *state = read(pipe)?; + Ok(()) + }, + ) + .unwrap() + .execution + .unwrap() + }; + assert_eq!(status.code(), Some(7)); + assert!(started.elapsed() >= Duration::from_millis(250)); + } +} + +#[test] +fn both_operations_wait_for_output_held_after_child_exit() { + for captured in [true, false] { + let mut command = shell("(sleep 0.3; printf late) & exit 7"); + let (status, bytes) = if captured { + let result = output( + &mut command, + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + ) + .unwrap(); + (result.status, result.stdout) + } else { + let result = streaming( + &mut command, + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + Vec::new(), + |pipe, state| { + *state = read(pipe)?; + Ok(()) + }, + ) + .unwrap(); + (result.execution.unwrap(), result.state) + }; + assert_eq!(status.code(), Some(7)); + assert_eq!(bytes, b"late"); + } +} + +#[test] +fn normal_completion_leaves_background_work_without_owned_io_running() { + for captured in [true, false] { + let work = RecordedProcess::new(); + let mut command = + work.command("sleep 5 /dev/null 2>&1 & echo $! > \"$PID_FILE\"; exit 0"); + let status = if captured { + output( + &mut command, + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + ) + .unwrap() + .status + } else { + streaming( + &mut command, + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + (), + |pipe, _state| { + read(pipe)?; + Ok(()) + }, + ) + .unwrap() + .execution + .unwrap() + }; + + assert!(status.success()); + assert!( + exists(work.pid()), + "normal completion killed background work" + ); + } +} + +#[test] +fn captured_completion_waits_for_stderr_after_stdout_and_child_exit() { + let result = output( + &mut shell("(exec 1>&-; sleep 0.3; printf late-diagnostic >&2) & exit 7"), + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + ) + .unwrap(); + + assert_eq!(result.status.code(), Some(7)); + assert!(result.stdout.is_empty()); + assert_eq!(result.stderr, b"late-diagnostic"); +} + +fn assert_unread_prompt_interruptible(test_name: &str, captured: bool) { + const SIGNAL_DIR: &str = "THIRDSHIFT_PROCESS_TEST_SIGNAL_DIR"; + // Run this interface test alone in another process: the interruption + // predicate is process-wide and must not affect concurrent unit tests. + if let Some(dir) = std::env::var_os(SIGNAL_DIR) { + interrupt::install().unwrap(); + let owned = RecordedProcess::new(); + let mut command = + owned.command("echo $$ > \"$PID_FILE\"; touch \"$READY_FILE\"; exec sleep 5"); + command.env("READY_FILE", Path::new(&dir).join("ready")); + let prompt = "p".repeat(1024 * 1024); + let result = if captured { + output( + &mut command, + Some(prompt.as_bytes()), + "fixture", + Interruption::Ordinary, + &stop_fixture, + ) + .map(|_| ()) + } else { + streaming( + &mut command, + Some(prompt.as_bytes()), + "fixture", + Interruption::Ordinary, + &stop_fixture, + Vec::new(), + |pipe, state| { + *state = read(pipe)?; + Ok(()) + }, + ) + .map(|_| ()) + }; + assert_eq!(format!("{:#}", result.unwrap_err()), "interrupted"); + owned.assert_stopped(); + return; + } + + let dir = tempfile::tempdir().unwrap(); + let mut child = Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test_name, "--nocapture"]) + .env(SIGNAL_DIR, dir.path()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let deadline = Instant::now() + Duration::from_secs(10); + while !dir.path().join("ready").exists() && Instant::now() < deadline { + if child.try_wait().unwrap().is_some() { + break; + } + thread::sleep(Duration::from_millis(10)); + } + if !dir.path().join("ready").exists() { + let _ = child.kill(); + let result = child.wait_with_output().unwrap(); + panic!("the isolated test never started: {result:?}"); + } + let interrupted_at = Instant::now(); + // SAFETY: this is the exact PID of our isolated test process. + assert_eq!( + unsafe { libc::kill(child.id() as libc::pid_t, libc::SIGINT) }, + 0 + ); + let result = child.wait_with_output().unwrap(); + assert!(result.status.success(), "{result:?}"); + assert!( + interrupted_at.elapsed() < Duration::from_secs(2), + "the unread prompt prevented interruption" + ); +} + +fn stop_fixture(child: &mut Child) { + stop(child, &[libc::SIGTERM]); +} + +#[test] +fn completion_ignores_recorded_signals_without_permitting_ordinary_work() { + const SIGNAL: &str = "THIRDSHIFT_EXECUTION_RECORDED_SIGNAL"; + if let Ok(signal) = std::env::var(SIGNAL) { + interrupt::install().unwrap(); + signal_hook::low_level::raise(signal.parse().unwrap()).unwrap(); + let result = output( + &mut shell("cat; printf diagnostic >&2; exit 7"), + Some(b"\xffonce\0"), + "fixture", + Interruption::Completion, + &stop_fixture, + ) + .unwrap(); + assert_eq!(result.status.code(), Some(7)); + assert_eq!(result.stdout, b"\xffonce\0"); + assert_eq!(result.stderr, b"diagnostic"); + assert!(interrupt::requested()); + + let streamed = streaming( + &mut shell("printf retained; exec 0<&-; exec sleep 0.2"), + Some(&vec![b'p'; 1024 * 1024]), + "fixture", + Interruption::Completion, + &stop_fixture, + Vec::new(), + |mut pipe, state| { + pipe.read_to_end(state)?; + Ok(()) + }, + ) + .unwrap(); + assert_eq!(streamed.state, b"retained"); + assert_eq!( + streamed + .execution + .unwrap_err() + .downcast_ref::() + .unwrap() + .kind(), + std::io::ErrorKind::BrokenPipe + ); + let panicked = streaming( + &mut shell("printf ready; exec sleep 0.2"), + None, + "fixture", + Interruption::Completion, + &stop_fixture, + (), + |mut pipe, _| { + pipe.read_exact(&mut [0; 5])?; + panic!("completion consumer exploded") + }, + ) + .unwrap_err(); + assert!( + panicked + .to_string() + .contains("completion consumer exploded") + ); + assert!(interrupt::requested()); + + let refused = RecordedProcess::new(); + let result = output( + &mut refused.command("echo $$ > \"$PID_FILE\""), + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + ); + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + assert!(!refused.0.path().join("pid").exists()); + return; + } + for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { + let result = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "process::tests::completion_ignores_recorded_signals_without_permitting_ordinary_work", + "--nocapture", + ]) + .env(SIGNAL, signal.to_string()) + .output() + .unwrap(); + assert!(result.status.success(), "{result:?}"); + } +} + +#[test] +#[ignore = "local short-capture latency probe; no timing assertion for CI"] +fn representative_short_captures() { + for executable in ["true", "bash"] { + let started = Instant::now(); + for _ in 0..25 { + let mut command = Command::new(executable); + if executable == "bash" { + command.args(["-c", "printf short"]); + } + let result = output( + &mut command, + None, + executable, + Interruption::Ordinary, + &stop_fixture, + ) + .unwrap(); + assert!(result.status.success()); + if executable == "bash" { + assert_eq!(result.stdout, b"short"); + } + } + eprintln!( + "{executable}: 25 captures in {:?} (mean {:?})", + started.elapsed(), + started.elapsed() / 25 + ); + } +} + +#[test] +fn startup_failure_returns_no_state_even_when_the_consumer_has_returned() { + let owned = RecordedProcess::new(); + let gate = owned.0.path().join("consumer-returned"); + let mut command = shell("printf ready; exec sleep 0.2"); + command.env("THIRDSHIFT_EXECUTION_TEST_FAULT", "stdin-start"); + command.env("THIRDSHIFT_EXECUTION_TEST_GATE", &gate); + let stop = |child: &mut Child| { + fs::write(owned.0.path().join("pid"), child.id().to_string()).unwrap(); + stop_fixture(child); + }; + let result = streaming( + &mut command, + Some(b"prompt"), + "fixture", + Interruption::Ordinary, + &stop, + Vec::new(), + move |mut pipe, state| { + let mut bytes = [0; 5]; + pipe.read_exact(&mut bytes)?; + state.extend_from_slice(&bytes); + fs::write(gate, "returned")?; + Ok(()) + }, + ); + assert!( + result.is_err(), + "startup failure returned recovered state: {result:?}" + ); + owned.assert_stopped(); +} + +fn with_recorded_signal(test_name: &str, exercise: impl FnOnce(libc::c_int)) { + const SIGNAL: &str = "THIRDSHIFT_EXECUTION_ACTIVE_SIGNAL"; + if let Ok(signal) = std::env::var(SIGNAL) { + interrupt::install().unwrap(); + exercise(signal.parse().unwrap()); + return; + } + for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { + let result = Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test_name, "--nocapture"]) + .env(SIGNAL, signal.to_string()) + .output() + .unwrap(); + assert!(result.status.success(), "{result:?}"); + } +} + +#[test] +fn active_ordinary_cancellation_does_not_cancel_concurrent_completion() { + with_recorded_signal( + "process::tests::active_ordinary_cancellation_does_not_cancel_concurrent_completion", + |signal| { + let owned = RecordedProcess::new(); + let ready = owned.0.path().join("completion-ready"); + let mut completion = shell("touch \"$READY\"; sleep 0.3; printf completed; exit 7"); + completion.env("READY", &ready); + let finishing = thread::spawn(move || { + output( + &mut completion, + None, + "completion fixture", + Interruption::Completion, + &stop_fixture, + ) + }); + let mut ordinary = owned.command("while ! test -f \"$READY\"; do sleep 0.01; done; echo $$ > \"$PID_FILE\"; kill -\"$SIGNAL\" \"$SUPERVISOR\"; exec sleep 5"); + ordinary + .env("READY", &ready) + .env("SIGNAL", signal.to_string()) + .env("SUPERVISOR", std::process::id().to_string()); + let started = Instant::now(); + let result = output( + &mut ordinary, + Some(&vec![b'p'; 1024 * 1024]), + "ordinary fixture", + Interruption::Ordinary, + &stop_fixture, + ); + // Join this exact concurrent operation before any assertion can unwind. + let completed = finishing.join().unwrap().unwrap(); + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + owned.assert_stopped(); + assert_eq!(completed.status.code(), Some(7)); + assert_eq!(completed.stdout, b"completed"); + assert!(interrupt::requested()); + assert!(started.elapsed() < Duration::from_secs(2)); + }, + ); +} + +#[test] +fn interruption_after_cli_exit_stops_ordinary_pipe_holders_but_completion_drains_them() { + with_recorded_signal( + "process::tests::interruption_after_cli_exit_stops_ordinary_pipe_holders_but_completion_drains_them", + |signal| { + // Completion receives the new signal while its direct CLI has exited. + // The following ordinary call observes that same unchanged flag. + for policy in [Interruption::Completion, Interruption::Ordinary] { + let holder = RecordedProcess::new(); + let mut command = holder.command("bash -c 'while kill -0 \"$1\" 2>/dev/null; do sleep 0.01; done; echo $$ > \"$PID_FILE\"; kill -\"$SIGNAL\" \"$SUPERVISOR\"; sleep 0.2; printf late' holder $$ & exit 7"); + command + .env("SIGNAL", signal.to_string()) + .env("SUPERVISOR", std::process::id().to_string()); + let result = output(&mut command, None, "fixture", policy, &stop_fixture); + if matches!(policy, Interruption::Completion) { + let result = result.unwrap(); + assert_eq!(result.status.code(), Some(7)); + assert_eq!(result.stdout, b"late"); + } else { + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + assert!(!holder.0.path().join("pid").exists()); + } + assert!(interrupt::requested()); + } + }, + ); +} + +#[test] +fn active_ordinary_cancellation_stops_output_held_after_cli_exit() { + with_recorded_signal( + "process::tests::active_ordinary_cancellation_stops_output_held_after_cli_exit", + |signal| { + let holder = RecordedProcess::new(); + let mut command = holder.command("bash -c 'while kill -0 \"$1\" 2>/dev/null; do sleep 0.01; done; echo $$ > \"$PID_FILE\"; kill -\"$SIGNAL\" \"$SUPERVISOR\"; exec sleep 5' holder $$ & exit 7"); + command + .env("SIGNAL", signal.to_string()) + .env("SUPERVISOR", std::process::id().to_string()); + let started = Instant::now(); + let result = output( + &mut command, + None, + "fixture", + Interruption::Ordinary, + &stop_fixture, + ); + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + assert!(started.elapsed() < Duration::from_secs(2)); + holder.assert_stopped(); + }, + ); +} + +#[test] +fn captured_execution_drains_full_pipes_concurrently_with_binary_stdin() { + let result = output( + &mut shell("head -c 262144 /dev/zero & head -c 262144 /dev/zero >&2 & cat >/dev/null; wait; printf drained"), + Some(&vec![0xff; 262144]), + "fixture", + Interruption::Ordinary, + &stop_fixture, + ).unwrap(); + assert!(result.status.success()); + let mut stdout = vec![0; 262144]; + stdout.extend_from_slice(b"drained"); + assert_eq!(result.stdout, stdout); + assert_eq!(result.stderr, vec![0; 262144]); +} + +#[test] +fn captured_setup_and_worker_faults_stop_reap_and_join_before_return() { + for fault in [ + "stdout-pipe", + "stdout-start", + "stderr-pipe", + "stderr-start", + "stdin-pipe", + "stdin-start", + "stdout-read", + "stderr-read", + "stdin-write", + "stdout-read-panic", + "stderr-read-panic", + "stdin-write-panic", + "stderr-start-panic", + "wait-panic", + ] { + let owned = RecordedProcess::new(); + let mut command = shell("exec sleep 0.2"); + command.env("THIRDSHIFT_EXECUTION_TEST_FAULT", fault); + let stop = |child: &mut Child| { + fs::write(owned.0.path().join("pid"), child.id().to_string()).unwrap(); + stop_fixture(child); + }; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + output( + &mut command, + Some(b"prompt"), + "fixture", + Interruption::Ordinary, + &stop, + ) + })); + assert!( + matches!(result, Err(_) | Ok(Err(_))), + "{fault} returned success" + ); + owned.assert_stopped(); + } +} + +#[test] +fn interruption_during_cleanup_suppresses_retained_state_and_transport_failure() { + with_recorded_signal( + "process::tests::interruption_during_cleanup_suppresses_retained_state_and_transport_failure", + |signal| { + let owned = RecordedProcess::new(); + let stop = |child: &mut Child| { + signal_hook::low_level::raise(signal).unwrap(); + stop_fixture(child); + }; + let result = streaming( + &mut owned + .command("echo $$ > \"$PID_FILE\"; printf ready; exec 0<&-; exec sleep 5"), + Some(&vec![b'p'; 1024 * 1024]), + "fixture", + Interruption::Ordinary, + &stop, + Vec::new(), + |mut pipe, state| { + pipe.read_to_end(state)?; + Ok(()) + }, + ); + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + owned.assert_stopped(); + }, + ); +} + +#[test] +fn spawn_failure_returns_no_streamed_state_and_names_the_command() { + let result = streaming( + &mut Command::new("/thirdshift-no-such-command-fixture"), + None, + "missing fixture", + Interruption::Ordinary, + &stop_fixture, + "initial", + |_, _| panic!("consumer must not run after spawn failure"), + ); + assert_eq!( + result.unwrap_err().to_string(), + "could not run missing fixture" + ); +} diff --git a/src/harness/process_tree.rs b/src/process/tree.rs similarity index 96% rename from src/harness/process_tree.rs rename to src/process/tree.rs index 5039e5131..dfaba912f 100644 --- a/src/harness/process_tree.rs +++ b/src/process/tree.rs @@ -1,4 +1,4 @@ -//! The shared stop path for every Harness, including escaped process groups. +//! The shared CLI stop path, including escaped process groups. use std::collections::{BTreeSet, HashMap}; use std::process::{Child, Command}; @@ -14,7 +14,7 @@ const STOP_GRACE: Duration = Duration::from_secs(10); /// Give each of the Harness's stop signals the existing grace period. Once /// the CLI exits, skip the remaining grace periods but still kill its work. -pub(super) fn stop(child: &mut Child, signals: &[libc::c_int]) { +pub(crate) fn stop(child: &mut Child, signals: &[libc::c_int]) { let pid = child.id() as libc::pid_t; let mut tree = ProcessTree { pids: BTreeSet::from([pid]), From 102d60195681432f1096a5cb9d450e0ada535a47 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 20:16:14 -0400 Subject: [PATCH 2/7] Bundle execution controls after Standards review (#466) --- src/harness/process.rs | 18 ++-- src/process.rs | 79 ++++++-------- src/process/tests.rs | 238 ++++++++++++++++++++++++++--------------- 3 files changed, 196 insertions(+), 139 deletions(-) diff --git a/src/harness/process.rs b/src/harness/process.rs index 8fbb43fca..6158747b5 100644 --- a/src/harness/process.rs +++ b/src/harness/process.rs @@ -5,7 +5,7 @@ use std::process::{ChildStdout, Command, Output}; use anyhow::Result; use super::Adapter; -use crate::process::{self, Interruption, Streamed}; +use crate::process::{self, Control, Interruption, Streamed}; pub(super) fn output( adapter: &dyn Adapter, @@ -15,9 +15,11 @@ pub(super) fn output( process::output( command.envs(adapter.environment().iter().copied()), input.map(str::as_bytes), - adapter.name(), - Interruption::Ordinary, - &|child| adapter.stop(child), + Control { + name: adapter.name(), + interruption: Interruption::Ordinary, + stop: &|child| adapter.stop(child), + }, ) } @@ -31,9 +33,11 @@ pub(crate) fn streaming( process::streaming( command.envs(adapter.environment().iter().copied()), input.map(str::as_bytes), - adapter.name(), - Interruption::Ordinary, - &|child| adapter.stop(child), + Control { + name: adapter.name(), + interruption: Interruption::Ordinary, + stop: &|child| adapter.stop(child), + }, state, consume, ) diff --git a/src/process.rs b/src/process.rs index 238f473a8..7f89a9c39 100644 --- a/src/process.rs +++ b/src/process.rs @@ -43,24 +43,26 @@ impl Interruption { } } +/// Caller-supplied diagnostics and interruption behavior for one execution. +/// This immutable value owns no child or worker and grants no other execution +/// permission to ignore the Command's recorded interruption. +#[derive(Clone, Copy)] +pub(crate) struct Control<'a> { + pub name: &'a str, + pub interruption: Interruption, + pub stop: &'a dyn Fn(&mut Child), +} + /// Capture both output pipes without interpreting the child's exit status. pub(crate) fn output( command: &mut Command, input: Option<&[u8]>, - name: &str, - interruption: Interruption, - stop: &dyn Fn(&mut Child), + control: Control<'_>, ) -> Result { - let cli = name.to_owned(); - let (execution, _) = execute( - command, - input, - name, - interruption, - stop, - true, - move |pipe| read(pipe).with_context(|| format!("could not read {cli}'s stdout")), - ); + let cli = control.name.to_owned(); + let (execution, _) = execute(command, input, control, true, move |pipe| { + read(pipe).with_context(|| format!("could not read {cli}'s stdout")) + }); let (status, stdout, stderr) = execution?; Ok(Output { status, @@ -79,35 +81,25 @@ pub(crate) struct Streamed { /// Consume stdout concurrently with prompt I/O, inheriting stderr. Preserve /// state after any normal consumer return, even when execution fails. Return /// only after completion or whole-tree cleanup and every worker is joined. -/// A consumer panic or failure before it starts returns no state. Recorded +/// A consumer panic or any startup failure returns no state. Recorded ordinary /// interruption suppresses recovery even after a normal consumer return. pub(crate) fn streaming( command: &mut Command, input: Option<&[u8]>, - name: &str, - interruption: Interruption, - stop: &dyn Fn(&mut Child), + control: Control<'_>, mut state: T, consume: impl FnOnce(ChildStdout, &mut T) -> Result<()> + Send + 'static, ) -> Result> { // One send fits without waiting for the caller, which must first finish // supervising and joining all workers, including on transport failure. let (completed, recovered) = mpsc::sync_channel(1); - let (execution, started) = execute( - command, - input, - name, - interruption, - stop, - false, - move |pipe| { - let result = consume(pipe, &mut state); - let _ = completed.send(state); - result - }, - ); + let (execution, started) = execute(command, input, control, false, move |pipe| { + let result = consume(pipe, &mut state); + let _ = completed.send(state); + result + }); let execution = execution.map(|(status, (), _)| status); - interruption.check()?; + control.interruption.check()?; if !started { return Err(execution.expect_err("startup failure cannot execute successfully")); } @@ -126,12 +118,15 @@ fn read(mut pipe: impl Read) -> std::io::Result> { fn execute( command: &mut Command, input: Option<&[u8]>, - name: &str, - interruption: Interruption, - stop: &dyn Fn(&mut Child), + control: Control<'_>, capture_stderr: bool, consume: impl FnOnce(ChildStdout) -> Result + Send + 'static, ) -> (Result<(ExitStatus, T, Vec)>, bool) { + let Control { + name, + interruption, + stop: _, + } = control; let mut started = false; let result = (|| { if interruption.requested() { @@ -159,9 +154,7 @@ fn execute( // Install ownership immediately: every fallible step after spawn is // covered, including pipe extraction and starting workers. let mut owned = Execution { - name, - interruption, - stop, + control, child, awakened, stdout: None, @@ -261,9 +254,7 @@ fn execute( } struct Execution<'a, T> { - name: &'a str, - interruption: Interruption, - stop: &'a dyn Fn(&mut Child), + control: Control<'a>, child: Child, awakened: mpsc::Receiver<()>, stdout: Option>, @@ -276,14 +267,14 @@ impl Execution<'_, T> { fn wait(&mut self) -> Result { let mut status = None; loop { - if self.interruption.requested() { + if self.control.interruption.requested() { bail!("interrupted"); } if status.is_none() { status = self .child .try_wait() - .with_context(|| format!("could not wait for {}", self.name))?; + .with_context(|| format!("could not wait for {}", self.control.name))?; } let stdin_done = self .stdin @@ -303,7 +294,7 @@ impl Execution<'_, T> { && stdout_done && stderr_done { - if self.interruption.requested() { + if self.control.interruption.requested() { bail!("interrupted"); } return Ok(status); @@ -324,7 +315,7 @@ impl Execution<'_, T> { if self.completed { return; } - (self.stop)(&mut self.child); + (self.control.stop)(&mut self.child); let _ = self.child.wait(); // Pipes may still be attached if worker startup failed. drop(self.child.stdin.take()); diff --git a/src/process/tests.rs b/src/process/tests.rs index 7911fe88a..432279106 100644 --- a/src/process/tests.rs +++ b/src/process/tests.rs @@ -19,9 +19,11 @@ fn captured_completion_preserves_both_byte_streams_and_nonzero_status() { let result = output( &mut shell("printf '\\377out\\000\\n'; printf '\\376err\\000\\n' >&2; exit 7"), None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .unwrap(); @@ -35,9 +37,11 @@ fn streamed_completion_preserves_raw_bytes_and_nonzero_status_in_consumer_state( let consumed = streaming( &mut shell("printf 'unknown\\n\\377malformed\\n'; exit 9"), None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, (Vec::new(), "initial"), |pipe, state| { state.0 = read(pipe)?; @@ -64,9 +68,11 @@ fn captured_prompt_is_null_when_absent_and_written_once_then_closed_when_present let result = output( &mut shell("if [ /dev/fd/0 -ef /dev/null ]; then printf null; fi; cat; printf closed"), input.map(str::as_bytes), - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .unwrap(); assert!(result.status.success()); @@ -125,9 +131,11 @@ fn broken_prompt_stops_and_reaps_the_child_with_the_original_stdin_error() { let result = output( &mut child.command("echo $$ > \"$PID_FILE\"; exec 0<&-; exec sleep 5"), Some("p".repeat(1024 * 1024).as_bytes()), - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ); let error = result.unwrap_err(); @@ -169,9 +177,11 @@ fn consumer_log_failure_cleans_up_even_with_a_blocked_prompt_writer() { let result = streaming( &mut command, Some("p".repeat(1024 * 1024).as_bytes()), - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, b"initial".to_vec(), |mut pipe, state| { let mut bytes = [0; 5]; @@ -200,9 +210,11 @@ fn prompt_failure_retains_state_and_precedes_a_consumer_failure_during_cleanup() let result = streaming( &mut child.command("echo $$ > \"$PID_FILE\"; printf ready; exec 0<&-; exec sleep 5"), Some("p".repeat(1024 * 1024).as_bytes()), - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, b"initial".to_vec(), |mut pipe, state| { pipe.read_to_end(state)?; @@ -228,9 +240,11 @@ fn consumer_panic_stops_and_reaps_the_child_and_reports_its_cause() { let result = streaming::<()>( &mut child.command("echo $$ > \"$PID_FILE\"; printf ready; exec sleep 5"), None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, (), |mut pipe, _state| { pipe.read_exact(&mut [0; 5])?; @@ -271,9 +285,11 @@ fn eof_does_not_finish_either_operation_before_the_child_exits() { output( &mut command, None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .unwrap() .status @@ -281,9 +297,11 @@ fn eof_does_not_finish_either_operation_before_the_child_exits() { streaming( &mut command, None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, Vec::new(), |pipe, state| { *state = read(pipe)?; @@ -307,9 +325,11 @@ fn both_operations_wait_for_output_held_after_child_exit() { let result = output( &mut command, None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .unwrap(); (result.status, result.stdout) @@ -317,9 +337,11 @@ fn both_operations_wait_for_output_held_after_child_exit() { let result = streaming( &mut command, None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, Vec::new(), |pipe, state| { *state = read(pipe)?; @@ -344,9 +366,11 @@ fn normal_completion_leaves_background_work_without_owned_io_running() { output( &mut command, None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .unwrap() .status @@ -354,9 +378,11 @@ fn normal_completion_leaves_background_work_without_owned_io_running() { streaming( &mut command, None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, (), |pipe, _state| { read(pipe)?; @@ -381,9 +407,11 @@ fn captured_completion_waits_for_stderr_after_stdout_and_child_exit() { let result = output( &mut shell("(exec 1>&-; sleep 0.3; printf late-diagnostic >&2) & exit 7"), None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .unwrap(); @@ -407,18 +435,22 @@ fn assert_unread_prompt_interruptible(test_name: &str, captured: bool) { output( &mut command, Some(prompt.as_bytes()), - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .map(|_| ()) } else { streaming( &mut command, Some(prompt.as_bytes()), - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, Vec::new(), |pipe, state| { *state = read(pipe)?; @@ -479,9 +511,11 @@ fn completion_ignores_recorded_signals_without_permitting_ordinary_work() { let result = output( &mut shell("cat; printf diagnostic >&2; exit 7"), Some(b"\xffonce\0"), - "fixture", - Interruption::Completion, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Completion, + stop: &stop_fixture, + }, ) .unwrap(); assert_eq!(result.status.code(), Some(7)); @@ -492,9 +526,11 @@ fn completion_ignores_recorded_signals_without_permitting_ordinary_work() { let streamed = streaming( &mut shell("printf retained; exec 0<&-; exec sleep 0.2"), Some(&vec![b'p'; 1024 * 1024]), - "fixture", - Interruption::Completion, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Completion, + stop: &stop_fixture, + }, Vec::new(), |mut pipe, state| { pipe.read_to_end(state)?; @@ -515,9 +551,11 @@ fn completion_ignores_recorded_signals_without_permitting_ordinary_work() { let panicked = streaming( &mut shell("printf ready; exec sleep 0.2"), None, - "fixture", - Interruption::Completion, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Completion, + stop: &stop_fixture, + }, (), |mut pipe, _| { pipe.read_exact(&mut [0; 5])?; @@ -536,9 +574,11 @@ fn completion_ignores_recorded_signals_without_permitting_ordinary_work() { let result = output( &mut refused.command("echo $$ > \"$PID_FILE\""), None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ); assert_eq!(result.unwrap_err().to_string(), "interrupted"); assert!(!refused.0.path().join("pid").exists()); @@ -571,9 +611,11 @@ fn representative_short_captures() { let result = output( &mut command, None, - executable, - Interruption::Ordinary, - &stop_fixture, + Control { + name: executable, + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ) .unwrap(); assert!(result.status.success()); @@ -603,9 +645,11 @@ fn startup_failure_returns_no_state_even_when_the_consumer_has_returned() { let result = streaming( &mut command, Some(b"prompt"), - "fixture", - Interruption::Ordinary, - &stop, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop, + }, Vec::new(), move |mut pipe, state| { let mut bytes = [0; 5]; @@ -652,9 +696,11 @@ fn active_ordinary_cancellation_does_not_cancel_concurrent_completion() { output( &mut completion, None, - "completion fixture", - Interruption::Completion, - &stop_fixture, + Control { + name: "completion fixture", + interruption: Interruption::Completion, + stop: &stop_fixture, + }, ) }); let mut ordinary = owned.command("while ! test -f \"$READY\"; do sleep 0.01; done; echo $$ > \"$PID_FILE\"; kill -\"$SIGNAL\" \"$SUPERVISOR\"; exec sleep 5"); @@ -666,9 +712,11 @@ fn active_ordinary_cancellation_does_not_cancel_concurrent_completion() { let result = output( &mut ordinary, Some(&vec![b'p'; 1024 * 1024]), - "ordinary fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "ordinary fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ); // Join this exact concurrent operation before any assertion can unwind. let completed = finishing.join().unwrap().unwrap(); @@ -695,7 +743,15 @@ fn interruption_after_cli_exit_stops_ordinary_pipe_holders_but_completion_drains command .env("SIGNAL", signal.to_string()) .env("SUPERVISOR", std::process::id().to_string()); - let result = output(&mut command, None, "fixture", policy, &stop_fixture); + let result = output( + &mut command, + None, + Control { + name: "fixture", + interruption: policy, + stop: &stop_fixture, + }, + ); if matches!(policy, Interruption::Completion) { let result = result.unwrap(); assert_eq!(result.status.code(), Some(7)); @@ -724,9 +780,11 @@ fn active_ordinary_cancellation_stops_output_held_after_cli_exit() { let result = output( &mut command, None, - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, ); assert_eq!(result.unwrap_err().to_string(), "interrupted"); assert!(started.elapsed() < Duration::from_secs(2)); @@ -740,9 +798,7 @@ fn captured_execution_drains_full_pipes_concurrently_with_binary_stdin() { let result = output( &mut shell("head -c 262144 /dev/zero & head -c 262144 /dev/zero >&2 & cat >/dev/null; wait; printf drained"), Some(&vec![0xff; 262144]), - "fixture", - Interruption::Ordinary, - &stop_fixture, + Control { name: "fixture", interruption: Interruption::Ordinary, stop: &stop_fixture }, ).unwrap(); assert!(result.status.success()); let mut stdout = vec![0; 262144]; @@ -780,9 +836,11 @@ fn captured_setup_and_worker_faults_stop_reap_and_join_before_return() { output( &mut command, Some(b"prompt"), - "fixture", - Interruption::Ordinary, - &stop, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop, + }, ) })); assert!( @@ -807,9 +865,11 @@ fn interruption_during_cleanup_suppresses_retained_state_and_transport_failure() &mut owned .command("echo $$ > \"$PID_FILE\"; printf ready; exec 0<&-; exec sleep 5"), Some(&vec![b'p'; 1024 * 1024]), - "fixture", - Interruption::Ordinary, - &stop, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop, + }, Vec::new(), |mut pipe, state| { pipe.read_to_end(state)?; @@ -827,9 +887,11 @@ fn spawn_failure_returns_no_streamed_state_and_names_the_command() { let result = streaming( &mut Command::new("/thirdshift-no-such-command-fixture"), None, - "missing fixture", - Interruption::Ordinary, - &stop_fixture, + Control { + name: "missing fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, "initial", |_, _| panic!("consumer must not run after spawn failure"), ); From b56615143ad663803bf9fb2a4728065cbfbc8a57 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 20:39:06 -0400 Subject: [PATCH 3/7] Make Git execution interruptible with completion views (#467) --- src/branch.rs | 27 ++-- src/claim.rs | 8 +- src/delivery.rs | 10 +- src/git.rs | 66 ++++++-- src/git/execution_tests.rs | 298 +++++++++++++++++++++++++++++++++++++ src/launch.rs | 19 ++- src/process.rs | 2 - src/spec_run.rs | 2 +- src/worktree.rs | 38 ++--- tests/failed_run.rs | 31 +++- 10 files changed, 430 insertions(+), 71 deletions(-) create mode 100644 src/git/execution_tests.rs diff --git a/src/branch.rs b/src/branch.rs index 3048bbd30..64c3e3842 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -97,7 +97,7 @@ trait Reads { /// `first_branch`, from `issue`'s repository only. fn pull_requests(&self, issue: &IssueUrl, first_branch: &str) -> Result>; /// The head of the local `branch` in the Launch directory, if it has one. - fn local_head(&self, branch: &str) -> Option; + fn local_head(&self, branch: &str) -> Result>; } /// A branch on origin. @@ -138,15 +138,13 @@ impl Reads for GitHubAndOrigin<'_> { github::pull_requests_with_head_prefix(issue, first_branch) } - fn local_head(&self, branch: &str) -> Option { - self.launch - .run(&[ - "rev-parse", - "--verify", - "--quiet", - &format!("refs/heads/{branch}"), - ]) - .ok() + fn local_head(&self, branch: &str) -> Result> { + self.launch.run_optional(&[ + "rev-parse", + "--verify", + "--quiet", + &format!("refs/heads/{branch}"), + ]) } } @@ -230,7 +228,7 @@ fn used(reads: &impl Reads, issue: &IssueUrl) -> Result { /// A local `branch` in the launch repository must be at `origin_sha`, or not /// exist when origin has no copy, so no local-only commits are destroyed. fn check_local_branch(reads: &impl Reads, branch: &str, origin_sha: Option<&str>) -> Result<()> { - let Some(local_sha) = reads.local_head(branch) else { + let Some(local_sha) = reads.local_head(branch)? else { return Ok(()); }; match origin_sha { @@ -335,14 +333,15 @@ mod tests { .collect()) } - fn local_head(&self, branch: &str) -> Option { + fn local_head(&self, branch: &str) -> Result> { self.seen .borrow_mut() .push(Read::LocalHead(branch.to_string())); - self.local + Ok(self + .local .iter() .find(|(name, _)| *name == branch) - .map(|(_, head)| head.to_string()) + .map(|(_, head)| head.to_string())) } } diff --git a/src/claim.rs b/src/claim.rs index 5e6cd044e..0255809e2 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -62,12 +62,8 @@ impl Claim<'_> { /// The run has ended as it has, so this never fails: a failure is a /// warning naming what to run by hand, and an interrupt doesn't stop it. pub fn end(self, reached: Option) { - self.claimed.end( - &mut OnGitHub { - launch: self.launch, - }, - reached, - ); + let launch = self.launch.completion(); + self.claimed.end(&mut OnGitHub { launch: &launch }, reached); } } diff --git a/src/delivery.rs b/src/delivery.rs index 2b4348eb9..34b1edfcd 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -429,7 +429,7 @@ impl Outside for InWorktree<'_> { } fn push(&mut self) -> Result<()> { - self.worktree.push() + self.worktree.push(self.worktree.git()) } fn pull_request(&mut self) -> Result> { @@ -501,8 +501,8 @@ impl FailedOutside for OnFailure<'_> { /// against the Base branch, so no empty Issue branch appears on origin. fn commit_and_push(&mut self, reason: &str) -> Result<()> { let worktree = &self.worktree; - let git = worktree.git(); - if worktree.merge_in_progress()? { + let git = worktree.git().completion(); + if worktree.merge_in_progress(&git)? { git.run(&["merge", "--abort"])?; } git.run(&["add", "-A"])?; @@ -525,7 +525,7 @@ impl FailedOutside for OnFailure<'_> { "-m", &message, ])?; - worktree.push() + worktree.push(&git) } fn pull_request(&mut self) -> Result> { @@ -576,7 +576,7 @@ impl repair_loop::Outside for RunOutside<'_> { } fn push(&mut self) -> Result<()> { - self.worktree.push() + self.worktree.push(self.worktree.git()) } fn head(&mut self) -> Result { diff --git a/src/git.rs b/src/git.rs index e80c116f7..d57d9befa 100644 --- a/src/git.rs +++ b/src/git.rs @@ -7,21 +7,39 @@ use std::time::{Duration, Instant}; use anyhow::{Context, Result, bail}; +use crate::process::{self, Control, Interruption}; + +#[cfg(test)] +mod execution_tests; + /// `git` bound to a working directory. Output is captured, never passed /// through, so stdout stays reserved for the PR URL. pub struct Git { dir: PathBuf, + interruption: Interruption, } impl Git { pub fn new(dir: impl Into) -> Self { - Git { dir: dir.into() } + Git { + dir: dir.into(), + interruption: Interruption::Ordinary, + } } pub fn dir(&self) -> &Path { &self.dir } + /// An independent view for finishing work in the same directory. It + /// ignores recorded interruption without changing this or other views. + pub fn completion(&self) -> Self { + Git { + dir: self.dir.clone(), + interruption: Interruption::Completion, + } + } + /// Run `git ` and return its trimmed stdout. If it exits non-zero, /// fail with git's own `error:` or `fatal:` line, if it wrote one, then /// the last lines of its stderr and of its stdout, where a hook's @@ -30,12 +48,7 @@ impl Git { /// Tickets fetch or create worktrees from one Launch directory at once, /// it is run again, for up to [`LOCK_WAIT`]. pub fn run(&self, args: &[&str]) -> Result { - let deadline = Instant::now() + LOCK_WAIT; - let mut output = self.output(args)?; - while !output.status.success() && held_lock(&output.stderr) && Instant::now() < deadline { - std::thread::sleep(Duration::from_millis(100)); - output = self.output(args)?; - } + let output = self.retried_output(args)?; if !output.status.success() { let tail = [error_first(&output.stderr), last_lines(&output.stdout)].concat(); let mut message = format!("git {} failed", args.join(" ")); @@ -47,6 +60,26 @@ impl Git { Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) } + /// A probe's trimmed stdout, or no answer for a completed nonzero exit. + /// Interruption and transport failures remain errors, never absence. + pub fn run_optional(&self, args: &[&str]) -> Result> { + let output = self.retried_output(args)?; + Ok(output + .status + .success() + .then(|| String::from_utf8_lossy(&output.stdout).trim().to_string())) + } + + fn retried_output(&self, args: &[&str]) -> Result { + let deadline = Instant::now() + LOCK_WAIT; + let mut output = self.output(args)?; + while !output.status.success() && held_lock(&output.stderr) && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(100)); + output = self.output(args)?; + } + Ok(output) + } + /// The repository's common git directory, the one its worktrees share. pub fn common_dir(&self) -> Result { Ok(self.dir.join(self.run(&["rev-parse", "--git-common-dir"])?)) @@ -71,17 +104,22 @@ impl Git { } /// Run `git ` and report whether it exited zero, for commands whose - /// exit status is the answer. + /// exit status is the answer. Only a completed nonzero exit is false; + /// interruption and transport failures remain errors. pub fn succeeds(&self, args: &[&str]) -> Result { Ok(self.output(args)?.status.success()) } fn output(&self, args: &[&str]) -> Result { - Command::new("git") - .args(args) - .current_dir(&self.dir) - .output() - .with_context(|| format!("could not run git {}", args.join(" "))) + process::output( + Command::new("git").args(args).current_dir(&self.dir), + None, + Control { + name: &format!("git {}", args.join(" ")), + interruption: self.interruption, + stop: &|child| process::stop(child, &[libc::SIGTERM]), + }, + ) } } @@ -146,7 +184,7 @@ mod tests { /// A repository in a temp directory, with an identity and one commit, and /// a bare repository beside it as its `origin`. No global or system /// config is read. - fn repo_with_origin() -> (tempfile::TempDir, Git) { + pub(super) fn repo_with_origin() -> (tempfile::TempDir, Git) { let temp = tempfile::TempDir::new().unwrap(); let isolated = |dir: &Path, args: &[&str]| { let status = Command::new("git") diff --git a/src/git/execution_tests.rs b/src/git/execution_tests.rs new file mode 100644 index 000000000..9af98c571 --- /dev/null +++ b/src/git/execution_tests.rs @@ -0,0 +1,298 @@ +//! Git behavior in isolated Commands, so recorded interruption cannot leak +//! into another test running in the same process. + +use super::*; +use crate::interrupt; +use std::fs; +use std::os::unix::fs::PermissionsExt; + +fn isolated(name: &str, test: impl FnOnce(libc::c_int)) { + if std::env::var("THIRDSHIFT_TEST_GIT").as_deref() == Ok(name) { + interrupt::install().unwrap(); + test( + std::env::var("THIRDSHIFT_TEST_GIT_SIGNAL") + .unwrap() + .parse() + .unwrap(), + ); + return; + } + for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { + let output = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + &format!("git::execution_tests::{name}"), + "--nocapture", + ]) + .env("THIRDSHIFT_TEST_GIT", name) + .env("THIRDSHIFT_TEST_GIT_SIGNAL", signal.to_string()) + .output() + .unwrap(); + assert!(output.status.success(), "signal {signal}: {output:?}"); + } +} + +#[test] +fn ordinary_git_refuses_to_spawn_after_interruption() { + isolated( + "ordinary_git_refuses_to_spawn_after_interruption", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + signal_hook::low_level::raise(signal).unwrap(); + let error = git + .run(&["-c", "alias.start=!touch started", "start"]) + .unwrap_err(); + assert_eq!(error.to_string(), "interrupted"); + assert!(!git.dir().join("started").exists()); + }, + ); +} + +#[test] +fn completion_git_finishes_without_permitting_the_original_view() { + isolated( + "completion_git_finishes_without_permitting_the_original_view", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + let completion = git.completion(); + signal_hook::low_level::raise(signal).unwrap(); + completion + .run(&["config", "thirdshift.finished", "yes"]) + .unwrap(); + assert_eq!( + completion.run(&["config", "thirdshift.finished"]).unwrap(), + "yes" + ); + assert_eq!(completion.dir(), git.dir()); + assert!(interrupt::requested()); + assert_eq!( + git.run(&["config", "thirdshift.finished"]) + .unwrap_err() + .to_string(), + "interrupted" + ); + assert_eq!( + Git::new(git.dir()) + .succeeds(&["rev-parse", "HEAD"]) + .unwrap_err() + .to_string(), + "interrupted" + ); + }, + ); +} + +#[test] +fn optional_answers_distinguish_absence_from_execution_failure() { + isolated( + "optional_answers_distinguish_absence_from_execution_failure", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + assert_eq!( + git.run_optional(&["symbolic-ref", "--short", "HEAD"]) + .unwrap() + .as_deref(), + Some("main") + ); + assert_eq!( + git.run_optional(&["rev-parse", "--verify", "--quiet", "refs/heads/absent"]) + .unwrap(), + None + ); + assert!( + !git.succeeds(&["rev-parse", "--verify", "--quiet", "refs/heads/absent"]) + .unwrap() + ); + assert!(git.succeeds(&["rev-parse", "HEAD"]).unwrap()); + let missing = Git::new(git.dir().join("absent-directory")); + assert!(missing.run_optional(&["rev-parse", "HEAD"]).is_err()); + assert!(missing.succeeds(&["rev-parse", "HEAD"]).is_err()); + signal_hook::low_level::raise(signal).unwrap(); + assert_eq!( + git.run_optional(&["rev-parse", "--verify", "--quiet", "refs/heads/absent"]) + .unwrap_err() + .to_string(), + "interrupted" + ); + assert_eq!( + git.succeeds(&["rev-parse", "--verify", "--quiet", "refs/heads/absent"]) + .unwrap_err() + .to_string(), + "interrupted" + ); + }, + ); +} + +/// The executable is a real Git alias. Only its recorded PID is used for +/// fallback cleanup, including when an assertion fails. +struct Fixture { + pid: PathBuf, +} + +impl Fixture { + fn new(git: &Git, script: &str) -> Self { + let pid = git.dir().join("fixture.pid"); + let path = git.dir().join("fixture.sh"); + fs::write( + &path, + format!("#!/bin/sh\necho $$ > fixture.pid\n{script}\n"), + ) + .unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap(); + Self { pid } + } + + fn pid(&self) -> Option { + fs::read_to_string(&self.pid).ok()?.trim().parse().ok() + } +} + +fn exists(pid: libc::pid_t) -> bool { + // SAFETY: signal 0 only probes the PID recorded by this fixture. + unsafe { libc::kill(pid, 0) == 0 } +} + +impl Drop for Fixture { + fn drop(&mut self) { + if let Some(pid) = self.pid() + && exists(pid) + { + // SAFETY: this PID belongs to the test's finite executable. + unsafe { libc::kill(pid, libc::SIGKILL) }; + } + } +} + +const FIXTURE: &[&str] = &["-c", "alias.fixture=!./fixture.sh", "fixture"]; + +#[test] +fn a_parent_only_signal_stops_active_git_and_propagates_through_probes() { + isolated( + "a_parent_only_signal_stops_active_git_and_propagates_through_probes", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + let fixture = Fixture::new( + &git, + &format!("kill -{signal} {}\nexec sleep 3", std::process::id()), + ); + let started = Instant::now(); + assert_eq!( + git.succeeds(FIXTURE).unwrap_err().to_string(), + "interrupted" + ); + assert!( + started.elapsed() < Duration::from_secs(2), + "Git waited for natural expiry" + ); + let pid = fixture.pid().expect("the executable never started"); + let deadline = Instant::now() + Duration::from_secs(2); + while exists(pid) && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(10)); + } + assert!(!exists(pid), "Git left its owned executable alive"); + assert_eq!( + git.run_optional(FIXTURE).unwrap_err().to_string(), + "interrupted" + ); + assert_eq!(git.run(FIXTURE).unwrap_err().to_string(), "interrupted"); + }, + ); +} + +#[test] +fn completion_git_finishes_with_a_newly_arriving_signal() { + isolated( + "completion_git_finishes_with_a_newly_arriving_signal", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + let _fixture = Fixture::new( + &git, + &format!( + "kill -{signal} {}\nsleep 0.1\nprintf 'completed\\n'", + std::process::id() + ), + ); + assert_eq!(git.completion().run(FIXTURE).unwrap(), "completed"); + assert!(interrupt::requested()); + assert_eq!(git.run(FIXTURE).unwrap_err().to_string(), "interrupted"); + }, + ); +} + +#[test] +fn an_optional_probe_propagates_active_cancellation() { + isolated( + "an_optional_probe_propagates_active_cancellation", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + let _fixture = Fixture::new( + &git, + &format!("kill -{signal} {}\nexec sleep 3", std::process::id()), + ); + assert_eq!( + git.run_optional(FIXTURE).unwrap_err().to_string(), + "interrupted" + ); + }, + ); +} + +fn interrupt_lock_retry(git: &Git, signal: libc::c_int) -> Result { + let held = git.dir().join("held"); + let tried = git.dir().join("tried"); + fs::write(&held, "").unwrap(); + let _fixture = Fixture::new( + git, + "if test -f held; then\n touch tried\n echo \"fatal: Unable to create 'config.lock': File exists\" >&2\n exit 1\nfi\nprintf 'finished\\n'", + ); + let release = std::thread::spawn(move || { + let deadline = Instant::now() + Duration::from_secs(5); + while !tried.exists() && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(10)); + } + let attempted = tried.exists(); + std::thread::sleep(Duration::from_millis(50)); + signal_hook::low_level::raise(signal).unwrap(); + std::thread::sleep(Duration::from_millis(200)); + fs::remove_file(held).unwrap(); + assert!(attempted, "Git never reached the held lock"); + }); + let result = git.run(FIXTURE); + release.join().unwrap(); + result +} + +#[test] +fn ordinary_lock_retries_observe_interruption() { + isolated("ordinary_lock_retries_observe_interruption", |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + assert_eq!( + interrupt_lock_retry(&git, signal).unwrap_err().to_string(), + "interrupted" + ); + }); +} + +#[test] +fn completion_lock_retries_ignore_existing_and_arriving_interruption() { + isolated( + "completion_lock_retries_ignore_existing_and_arriving_interruption", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + let completion = git.completion(); + assert_eq!( + interrupt_lock_retry(&completion, signal).unwrap(), + "finished" + ); + // The same held-lock path also works with the flag already recorded. + fs::remove_file(git.dir().join("tried")).unwrap(); + assert_eq!( + interrupt_lock_retry(&completion, signal).unwrap(), + "finished" + ); + assert!(interrupt::requested()); + assert_eq!(git.run(FIXTURE).unwrap_err().to_string(), "interrupted"); + }, + ); +} diff --git a/src/launch.rs b/src/launch.rs index e80f26e38..9fe1b74fb 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -73,9 +73,7 @@ impl LaunchDirectory { fn open(dir: PathBuf, opening: Opening) -> Result { let git = Git::new(dir); let origin = git.run(&["config", "remote.origin.url"])?; - let checked_out = git - .run(&["symbolic-ref", "--quiet", "--short", "HEAD"]) - .ok(); + let checked_out = git.run_optional(&["symbolic-ref", "--quiet", "--short", "HEAD"])?; Ok(LaunchDirectory { git, origin, @@ -145,8 +143,7 @@ impl LaunchDirectory { let local = format!("refs/heads/{base}"); if self .git - .run(&["rev-parse", "--verify", "--quiet", &local]) - .is_ok() + .succeeds(&["rev-parse", "--verify", "--quiet", &local])? { let ahead = self.git @@ -172,8 +169,16 @@ impl LaunchDirectory { let up_to_date = self .git .succeeds(&["merge-base", "--is-ancestor", &origin_base, "HEAD"]); - if up_to_date.unwrap_or(false) { - return; + match up_to_date { + Ok(true) => return, + Ok(false) => {} + Err(error) => { + progress::warn( + &error, + format_args!("could not check whether {base} is up to date"), + ); + return; + } } progress::step(format_args!( "updating {base} in the Launch directory from {origin_base}" diff --git a/src/process.rs b/src/process.rs index 7f89a9c39..1d1ecd023 100644 --- a/src/process.rs +++ b/src/process.rs @@ -25,8 +25,6 @@ mod tests; #[derive(Clone, Copy)] pub(crate) enum Interruption { Ordinary, - // The following Git/GitHub migration tickets supply production callers. - #[cfg_attr(not(test), allow(dead_code))] Completion, } diff --git a/src/spec_run.rs b/src/spec_run.rs index cee2283f4..eb90a0cf6 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -227,7 +227,7 @@ impl<'a> ChildRunsAndGitHub<'a> { impl Outside for ChildRunsAndGitHub<'_> { fn push(&mut self) -> Result<()> { - self.worktree().push() + self.worktree().push(self.worktree().git()) } /// Retain the child Run's ownership until polling delivers its ending. diff --git a/src/worktree.rs b/src/worktree.rs index db79325df..7844e91c8 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -111,25 +111,24 @@ impl Worktree { /// The target repo's hooks are skipped: the session runs the tests and CI /// gates the PR, so a local hook doesn't decide whether work reaches /// origin. - pub fn push(&self) -> Result<()> { + pub fn push(&self, git: &Git) -> Result<()> { progress::step(format_args!("pushing {}", self.branch)); - self.git - .run(&["push", "--no-verify", "origin", &self.branch])?; + git.run(&["push", "--no-verify", "origin", &self.branch])?; Ok(()) } /// Delete the Issue branch on origin, or do nothing if it is already gone /// there, e.g. deleted by GitHub after a merge. Hooks are skipped, as for - /// [`Worktree::push`]. + /// [`Worktree::push`]. This finishes a confirmed Self-merge, including + /// the origin read that reconciles a failed deletion. pub fn delete_from_origin(&self) -> Result<()> { progress::step(format_args!("deleting {} on origin", self.branch)); - let Err(error) = self - .git - .run(&["push", "--no-verify", "origin", "--delete", &self.branch]) + let git = self.git.completion(); + let Err(error) = git.run(&["push", "--no-verify", "origin", "--delete", &self.branch]) else { return Ok(()); }; - match self.git.on_origin(&self.branch) { + match git.on_origin(&self.branch) { Ok(false) => Ok(()), _ => Err(error), } @@ -199,7 +198,7 @@ impl Worktree { fn merge(&self, upstream: &str) -> Result { match self.git.run(&["merge", "--no-edit", "--ff", upstream]) { Ok(_) => Ok(Merge::Clean), - Err(_) if self.merge_in_progress()? => Ok(Merge::Conflicted(PendingMerge { + Err(_) if self.merge_in_progress(&self.git)? => Ok(Merge::Conflicted(PendingMerge { upstream: upstream.to_string(), commit: self.git.run(&["rev-parse", "MERGE_HEAD"])?, })), @@ -213,7 +212,7 @@ impl Worktree { /// next round's work. pub fn ensure_merged(&self, pending: &PendingMerge) -> Result<()> { let upstream = &pending.upstream; - if self.merge_in_progress()? { + if self.merge_in_progress(&self.git)? { bail!("the merge of {upstream} is still in progress"); } if !self.merged(&pending.commit)? { @@ -236,9 +235,8 @@ impl Worktree { } /// Whether a merge is in progress in the worktree. - pub fn merge_in_progress(&self) -> Result { - self.git - .succeeds(&["rev-parse", "-q", "--verify", "MERGE_HEAD"]) + pub fn merge_in_progress(&self, git: &Git) -> Result { + git.succeeds(&["rev-parse", "-q", "--verify", "MERGE_HEAD"]) } } @@ -247,7 +245,9 @@ impl Drop for Worktree { let path = self.path().to_string_lossy().into_owned(); if self.kept { let head = self - .head() + .git + .completion() + .run(&["rev-parse", "HEAD"]) .unwrap_or_else(|error| format!("an unknown commit ({error:#})")); progress::step(format_args!( "keeping the worktree {path} and local branch {} at {head}", @@ -259,7 +259,8 @@ impl Drop for Worktree { "cleaning up the worktree and local branch {}", self.branch )); - let _lock = lock_launch(&self.launch).inspect_err(|error| { + let launch = self.launch.completion(); + let _lock = lock_launch(&launch).inspect_err(|error| { progress::step(format_args!("cleaning up without the lock: {error:#}")) }); // Each step is attempted even if the one before it failed. @@ -268,7 +269,7 @@ impl Drop for Worktree { &["branch", "-D", &self.branch], ]; for step in steps { - if let Err(error) = self.launch.run(step) { + if let Err(error) = launch.run(step) { progress::step(format_args!("cleanup incomplete: {error:#}")); } } @@ -321,11 +322,12 @@ impl ReviewWorktree { impl Drop for ReviewWorktree { fn drop(&mut self) { progress::step("cleaning up the worktree"); - let _lock = lock_launch(&self.launch).inspect_err(|error| { + let launch = self.launch.completion(); + let _lock = lock_launch(&launch).inspect_err(|error| { progress::step(format_args!("cleaning up without the lock: {error:#}")) }); let path = self.path.to_string_lossy(); - if let Err(error) = self.launch.run(&["worktree", "remove", "--force", &path]) { + if let Err(error) = launch.run(&["worktree", "remove", "--force", &path]) { progress::step(format_args!("cleanup incomplete: {error:#}")); } } diff --git a/tests/failed_run.rs b/tests/failed_run.rs index 43185b35a..7dc976056 100644 --- a/tests/failed_run.rs +++ b/tests/failed_run.rs @@ -185,7 +185,7 @@ fn when_origin_rejects_the_failure_push_the_worktree_and_local_branch_are_kept() "stderr: {}", result.stderr ); - assert_kept(&scenario, &result); + assert_kept(&scenario, &result, "claude exited 3"); } #[test] @@ -198,13 +198,36 @@ fn when_origin_is_unreachable_for_the_failure_push_the_worktree_and_local_branch let result = scenario.run(&[&scenario.issue_url(7)]); - assert_kept(&scenario, &result); + assert_kept(&scenario, &result, "claude exited 3"); +} + +#[test] +fn an_interrupted_failure_push_that_is_rejected_keeps_the_work_and_reports_its_head() { + let scenario = Scenario::new(); + scenario.repo_has_hook( + &scenario.origin_dir(), + "pre-receive", + "#!/bin/sh\necho 'origin says no' >&2\nexit 1\n", + ); + scenario.agent_does(&format!( + "echo 'half done' > wip.txt\ntouch {}\nsleep 30\n", + scenario.path("agent-started").display() + )); + + let result = scenario.run_and_signal(&[&scenario.issue_url(7)], "agent-started", "TERM"); + + assert!( + result.stderr.contains("origin says no"), + "{}", + result.stderr + ); + assert_kept(&scenario, &result, "interrupted"); } /// What a Failed run whose work didn't reach origin shares: the failure /// commit is on the local Issue branch in the worktree, which are both kept, /// and a line on stderr says where. -fn assert_kept(scenario: &Scenario, result: &RunResult) { +fn assert_kept(scenario: &Scenario, result: &RunResult, reason: &str) { assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); assert_eq!(scenario.origin_log("issue-7"), None); let worktree = scenario.path("work/widgets-issue-7"); @@ -218,7 +241,7 @@ fn assert_kept(scenario: &Scenario, result: &RunResult) { scenario .launch_git(&["log", "-1", "--format=%s", head]) .trim(), - "thirdshift: failed run (claude exited 3)" + format!("thirdshift: failed run ({reason})") ); let kept = result .stderr From b06420e45e2227034e116d63fc00b74539dae81d Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 20:41:48 -0400 Subject: [PATCH 4/7] Simplify bound Git helpers and reuse signal tests (#467) --- src/delivery.rs | 8 +-- src/git.rs | 14 ++++ src/process/tests.rs | 168 ++++++++++++++++++++----------------------- src/spec_run.rs | 2 +- src/worktree.rs | 15 ++-- 5 files changed, 101 insertions(+), 106 deletions(-) diff --git a/src/delivery.rs b/src/delivery.rs index 34b1edfcd..774991160 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -429,7 +429,7 @@ impl Outside for InWorktree<'_> { } fn push(&mut self) -> Result<()> { - self.worktree.push(self.worktree.git()) + self.worktree.push() } fn pull_request(&mut self) -> Result> { @@ -502,7 +502,7 @@ impl FailedOutside for OnFailure<'_> { fn commit_and_push(&mut self, reason: &str) -> Result<()> { let worktree = &self.worktree; let git = worktree.git().completion(); - if worktree.merge_in_progress(&git)? { + if git.merge_in_progress()? { git.run(&["merge", "--abort"])?; } git.run(&["add", "-A"])?; @@ -525,7 +525,7 @@ impl FailedOutside for OnFailure<'_> { "-m", &message, ])?; - worktree.push(&git) + git.push(worktree.branch()) } fn pull_request(&mut self) -> Result> { @@ -576,7 +576,7 @@ impl repair_loop::Outside for RunOutside<'_> { } fn push(&mut self) -> Result<()> { - self.worktree.push(self.worktree.git()) + self.worktree.push() } fn head(&mut self) -> Result { diff --git a/src/git.rs b/src/git.rs index d57d9befa..97c446a18 100644 --- a/src/git.rs +++ b/src/git.rs @@ -8,6 +8,7 @@ use std::time::{Duration, Instant}; use anyhow::{Context, Result, bail}; use crate::process::{self, Control, Interruption}; +use crate::progress; #[cfg(test)] mod execution_tests; @@ -103,6 +104,19 @@ impl Git { Ok(!found.is_empty()) } + /// Push `branch` to origin without local hooks: sessions and CI check + /// the work, and a rejecting hook must not strand Failed run salvage. + pub fn push(&self, branch: &str) -> Result<()> { + progress::step(format_args!("pushing {branch}")); + self.run(&["push", "--no-verify", "origin", branch])?; + Ok(()) + } + + /// Whether a merge is in progress in this directory. + pub fn merge_in_progress(&self) -> Result { + self.succeeds(&["rev-parse", "-q", "--verify", "MERGE_HEAD"]) + } + /// Run `git ` and report whether it exited zero, for commands whose /// exit status is the answer. Only a completed nonzero exit is false; /// interruption and transport failures remain errors. diff --git a/src/process/tests.rs b/src/process/tests.rs index 432279106..52b58e3b6 100644 --- a/src/process/tests.rs +++ b/src/process/tests.rs @@ -504,98 +504,86 @@ fn stop_fixture(child: &mut Child) { #[test] fn completion_ignores_recorded_signals_without_permitting_ordinary_work() { - const SIGNAL: &str = "THIRDSHIFT_EXECUTION_RECORDED_SIGNAL"; - if let Ok(signal) = std::env::var(SIGNAL) { - interrupt::install().unwrap(); - signal_hook::low_level::raise(signal.parse().unwrap()).unwrap(); - let result = output( - &mut shell("cat; printf diagnostic >&2; exit 7"), - Some(b"\xffonce\0"), - Control { - name: "fixture", - interruption: Interruption::Completion, - stop: &stop_fixture, - }, - ) - .unwrap(); - assert_eq!(result.status.code(), Some(7)); - assert_eq!(result.stdout, b"\xffonce\0"); - assert_eq!(result.stderr, b"diagnostic"); - assert!(interrupt::requested()); - - let streamed = streaming( - &mut shell("printf retained; exec 0<&-; exec sleep 0.2"), - Some(&vec![b'p'; 1024 * 1024]), - Control { - name: "fixture", - interruption: Interruption::Completion, - stop: &stop_fixture, - }, - Vec::new(), - |mut pipe, state| { - pipe.read_to_end(state)?; - Ok(()) - }, - ) - .unwrap(); - assert_eq!(streamed.state, b"retained"); - assert_eq!( - streamed - .execution - .unwrap_err() - .downcast_ref::() - .unwrap() - .kind(), - std::io::ErrorKind::BrokenPipe - ); - let panicked = streaming( - &mut shell("printf ready; exec sleep 0.2"), - None, - Control { - name: "fixture", - interruption: Interruption::Completion, - stop: &stop_fixture, - }, - (), - |mut pipe, _| { - pipe.read_exact(&mut [0; 5])?; - panic!("completion consumer exploded") - }, - ) - .unwrap_err(); - assert!( - panicked - .to_string() - .contains("completion consumer exploded") - ); - assert!(interrupt::requested()); + with_recorded_signal( + "process::tests::completion_ignores_recorded_signals_without_permitting_ordinary_work", + |signal| { + signal_hook::low_level::raise(signal).unwrap(); + let result = output( + &mut shell("cat; printf diagnostic >&2; exit 7"), + Some(b"\xffonce\0"), + Control { + name: "fixture", + interruption: Interruption::Completion, + stop: &stop_fixture, + }, + ) + .unwrap(); + assert_eq!(result.status.code(), Some(7)); + assert_eq!(result.stdout, b"\xffonce\0"); + assert_eq!(result.stderr, b"diagnostic"); + assert!(interrupt::requested()); - let refused = RecordedProcess::new(); - let result = output( - &mut refused.command("echo $$ > \"$PID_FILE\""), - None, - Control { - name: "fixture", - interruption: Interruption::Ordinary, - stop: &stop_fixture, - }, - ); - assert_eq!(result.unwrap_err().to_string(), "interrupted"); - assert!(!refused.0.path().join("pid").exists()); - return; - } - for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { - let result = Command::new(std::env::current_exe().unwrap()) - .args([ - "--exact", - "process::tests::completion_ignores_recorded_signals_without_permitting_ordinary_work", - "--nocapture", - ]) - .env(SIGNAL, signal.to_string()) - .output() + let streamed = streaming( + &mut shell("printf retained; exec 0<&-; exec sleep 0.2"), + Some(&vec![b'p'; 1024 * 1024]), + Control { + name: "fixture", + interruption: Interruption::Completion, + stop: &stop_fixture, + }, + Vec::new(), + |mut pipe, state| { + pipe.read_to_end(state)?; + Ok(()) + }, + ) .unwrap(); - assert!(result.status.success(), "{result:?}"); - } + assert_eq!(streamed.state, b"retained"); + assert_eq!( + streamed + .execution + .unwrap_err() + .downcast_ref::() + .unwrap() + .kind(), + std::io::ErrorKind::BrokenPipe + ); + let panicked = streaming( + &mut shell("printf ready; exec sleep 0.2"), + None, + Control { + name: "fixture", + interruption: Interruption::Completion, + stop: &stop_fixture, + }, + (), + |mut pipe, _| { + pipe.read_exact(&mut [0; 5])?; + panic!("completion consumer exploded") + }, + ) + .unwrap_err(); + assert!( + panicked + .to_string() + .contains("completion consumer exploded") + ); + assert!(interrupt::requested()); + + let refused = RecordedProcess::new(); + let result = output( + &mut refused.command("echo $$ > \"$PID_FILE\""), + None, + Control { + name: "fixture", + interruption: Interruption::Ordinary, + stop: &stop_fixture, + }, + ); + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + assert!(!refused.0.path().join("pid").exists()); + }, + ); } #[test] diff --git a/src/spec_run.rs b/src/spec_run.rs index eb90a0cf6..cee2283f4 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -227,7 +227,7 @@ impl<'a> ChildRunsAndGitHub<'a> { impl Outside for ChildRunsAndGitHub<'_> { fn push(&mut self) -> Result<()> { - self.worktree().push(self.worktree().git()) + self.worktree().push() } /// Retain the child Run's ownership until polling delivers its ending. diff --git a/src/worktree.rs b/src/worktree.rs index 7844e91c8..586cf6291 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -111,10 +111,8 @@ impl Worktree { /// The target repo's hooks are skipped: the session runs the tests and CI /// gates the PR, so a local hook doesn't decide whether work reaches /// origin. - pub fn push(&self, git: &Git) -> Result<()> { - progress::step(format_args!("pushing {}", self.branch)); - git.run(&["push", "--no-verify", "origin", &self.branch])?; - Ok(()) + pub fn push(&self) -> Result<()> { + self.git.push(&self.branch) } /// Delete the Issue branch on origin, or do nothing if it is already gone @@ -198,7 +196,7 @@ impl Worktree { fn merge(&self, upstream: &str) -> Result { match self.git.run(&["merge", "--no-edit", "--ff", upstream]) { Ok(_) => Ok(Merge::Clean), - Err(_) if self.merge_in_progress(&self.git)? => Ok(Merge::Conflicted(PendingMerge { + Err(_) if self.git.merge_in_progress()? => Ok(Merge::Conflicted(PendingMerge { upstream: upstream.to_string(), commit: self.git.run(&["rev-parse", "MERGE_HEAD"])?, })), @@ -212,7 +210,7 @@ impl Worktree { /// next round's work. pub fn ensure_merged(&self, pending: &PendingMerge) -> Result<()> { let upstream = &pending.upstream; - if self.merge_in_progress(&self.git)? { + if self.git.merge_in_progress()? { bail!("the merge of {upstream} is still in progress"); } if !self.merged(&pending.commit)? { @@ -233,11 +231,6 @@ impl Worktree { self.git .succeeds(&["merge-base", "--is-ancestor", rev, "HEAD"]) } - - /// Whether a merge is in progress in the worktree. - pub fn merge_in_progress(&self, git: &Git) -> Result { - git.succeeds(&["rev-parse", "-q", "--verify", "MERGE_HEAD"]) - } } impl Drop for Worktree { From e2f4f2a864a1a3250b2bd0e338a71c343875386d Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 21:00:41 -0400 Subject: [PATCH 5/7] Make GitHub execution interruptible and finish interrupted workflows (#468) --- CONTEXT.md | 10 +- src/base_fix.rs | 8 +- src/branch.rs | 14 +- src/ci.rs | 6 +- src/claim.rs | 27 +- src/delivery.rs | 29 +- src/github.rs | 1205 +++++++++++++++++---------------- src/github/execution_tests.rs | 379 +++++++++++ src/labels.rs | 10 +- src/launch.rs | 4 +- src/notification.rs | 4 +- src/pass.rs | 10 +- src/ready.rs | 16 +- src/run.rs | 6 +- src/setup.rs | 5 +- src/spec_run.rs | 11 +- src/spec_run/spec_pr.rs | 24 +- tests/spec_run.rs | 61 ++ 18 files changed, 1173 insertions(+), 656 deletions(-) create mode 100644 src/github/execution_tests.rs diff --git a/CONTEXT.md b/CONTEXT.md index bc699c467..5fc0aef88 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -62,7 +62,7 @@ _Avoid_: task, sub-task One invocation of the factory on a single issue that is not a **Spec**, from launch to cleanup, ending in one pull request. Started directly on an **Issue URL**, by a **Spec run** for one of its **Tickets**, by an **Architect run** for the Ticket its **Architecture review** published, or by a **Pickup run** for the **Ready issue** it took. **Claim**: -The mark that the factory has taken an issue: thirdshift labels the issue `in-progress`, in place of `ready-for-agent` if it has that, when a **Run** or a **Spec run** starts on it, whether started on its **Issue URL** or by an **Architect run** or a **Pickup run**. A **Ticket**'s Run in a Spec run and a **Base fix** make none. A Claim is released, `ready-for-agent` put back, only when the Run ends with nothing on `origin` to take over: no **Issue branch** and no pull request. Otherwise it stays while the issue is open, whether its pull request is ready for review or the Run failed, until the **Day shift** relabels it, and thirdshift takes the label off once the issue is closed. +The mark that the factory has taken an issue: thirdshift labels the issue `in-progress`, in place of `ready-for-agent` if it has that, when a **Run** or a **Spec run** starts on it, whether started on its **Issue URL** or by an **Architect run** or a **Pickup run**. A **Ticket**'s Run in a Spec run and a **Base fix** make none. Ending a Claim finishes even after Command interruption. A Claim is released, `ready-for-agent` put back, only when the Run ends with nothing on `origin` to take over: no **Issue branch** and no pull request. Otherwise it stays while the issue is open, whether its pull request is ready for review or the Run failed, until the **Day shift** relabels it, and thirdshift takes the label off once the issue is closed. _Avoid_: lock (a lock is per machine and ends with its process), assignment **Spec run**: @@ -126,7 +126,7 @@ An open **Ticket** labelled with a triage role that says it is not agent work: ` The agent session a **Spec run** starts once every **Ticket** is done: it reviews the whole **Spec branch** against the **Base branch** and the **Spec**, fixes what it agrees with, and writes the **Spec PR**'s description, listing the rest as **Unaddressed findings**. **Failed spec run**: -A **Spec run** that ends, including by interruption, with any of its **Tickets** not done, or with its **Spec PR** not ready, mergeable and green (or, when asked to merge, not merged). It still takes every Ticket it can reach, and a failed Ticket stops only the Tickets it blocks. +A **Spec run** that ends, including by interruption, with any of its **Tickets** not done, or with its **Spec PR** not ready, mergeable and green (or, when asked to merge, not merged). It still takes every Ticket it can reach, and a failed Ticket stops only the Tickets it blocks. After Command interruption it starts no new Ticket or Spec review, drains its active Tickets, and finishes publishing their final Tickets checklist and outcome lines. **Merge run**: A **Run** asked to end with its pull request merged rather than left for review, by the command it was started with or by the **User config**. It does everything a **Run** does, then a **Self-merge**. A **Ticket**'s Run in a **Spec run** is always a Merge run into the **Spec branch**; the command's ask applies to the **Spec PR**. @@ -148,7 +148,7 @@ Writing the **User config** by answering a few questions, one per setting that m _Avoid_: init, onboarding, configure **Self-merge**: -The step at the end of a **Merge run** in which thirdshift itself merges the pull request into the **Base branch** with a merge commit, once it is open, ready for review, mergeable and green on the head commit it merges. No human reviews it first. It ends with the **Issue branch** deleted, the issue closed by thirdshift, unless it is already closed, and the issue's **Claim** removed. +The step at the end of a **Merge run** in which thirdshift itself merges the pull request into the **Base branch** with a merge commit, once it is open, ready for review, mergeable and green on the head commit it merges. No human reviews it first. Once the merge is confirmed at the requested head, completion continues even after Command interruption. It ends with the **Issue branch** deleted, the issue closed by thirdshift, unless it is already closed, and the issue's **Claim** removed. **Policy refusal**: A merge the **Self-merge** tried that failed, where the round of the Repair loop that followed found nothing to fix: the **Base branch** unchanged, no conflict, CI green or absent, the pull request mergeable. The cause is a repository setting or rule, such as merge commits disallowed or a review required. thirdshift never reads GitHub's error text to decide it. The **Merge run** is a **Failed run** that leaves the pull request ready for review. @@ -169,7 +169,7 @@ A **Merge run** into a **Run**'s **Base branch** that thirdshift starts when tha _Avoid_: hotfix, base repair (a **Repair** works on the Issue branch) **Failed run**: -A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI. For a **Merge run**, it is also a Run that ends without its pull request merged. Its work is still pushed so nothing is lost (or, if the push fails, its worktree and local **Issue branch** are kept), and its open pull request, if any, is converted back to a draft, unless the pull request is ready, mergeable and green and only the **Self-merge** could not happen. +A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI. For a **Merge run**, it is also a Run that ends without its pull request merged. Even after Command interruption, any unfinished merge is aborted and its work is still pushed so nothing is lost (or, if the push fails, its worktree and local **Issue branch** are kept), and its open pull request, if any, is converted back to a draft through the same finishing path, unless the pull request is ready, mergeable and green and only the **Self-merge** could not happen. **Issue branch**: A branch a **Run** works on for one issue: `issue-` (the first), then `issue--branch-` for k ≥ 2. A number counts as used once its pull request is merged or closed, even if the branch itself was deleted. @@ -197,7 +197,7 @@ The full transcript of one agent session, one per session, a **Resume** and each _Avoid_: transcript, session file **Command**: -One invocation of thirdshift that does factory work: a **Run** or **Spec run** started on an **Issue URL**, an **Architect run**, or a **Pickup run**. A child Run, a **Ticket**'s Run or a **Base fix**, is part of the command that started it, not one of its own. +One invocation of thirdshift that does factory work: a **Run** or **Spec run** started on an **Issue URL**, an **Architect run**, or a **Pickup run**. A child Run, a **Ticket**'s Run or a **Base fix**, is part of the command that started it, not one of its own. Interruption stops its ordinary CLI work and prevents more from starting, while Failed run salvage, confirmed Self-merge completion, Claim ending and Spec accounting still finish; finishing never clears the recorded interruption. _Avoid_: invocation, job **Command log**: diff --git a/src/base_fix.rs b/src/base_fix.rs index d3716d53b..61e9d801f 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -17,7 +17,7 @@ use anyhow::{Result, bail}; use crate::child_run::{self, Ended, Kind}; use crate::ci::{self, FailedChecks}; use crate::git::Git; -use crate::github::{self, Check, ListedIssue}; +use crate::github::{Check, GitHub, ListedIssue}; use crate::harness::Choice; use crate::issue::IssueUrl; use crate::labels::{Label, Labels, READY_FOR_AGENT}; @@ -484,7 +484,7 @@ impl Outside for LaunchAndGitHub<'_> { /// One `gh issue list`. fn open_fixes(&mut self) -> Result> { - github::open_issues_labelled(&self.issue.repo_slug(), BASE_FIX) + GitHub::new().open_issues_labelled(&self.issue.repo_slug(), BASE_FIX) } /// Left if the mark file is there and its lock free when first opened. @@ -508,11 +508,11 @@ impl Outside for LaunchAndGitHub<'_> { } fn issue_is_open(&mut self, issue: &IssueUrl) -> Result { - github::issue_is_open(issue) + GitHub::new().issue_is_open(issue) } fn create_issue(&mut self, title: &str, body: &str, labels: &[Label]) -> Result { - github::create_issue(self.issue, title, body, labels) + GitHub::new().create_issue(self.issue, title, body, labels) } fn mark_running(&mut self, number: u64) -> Result { diff --git a/src/branch.rs b/src/branch.rs index 64c3e3842..8eb1af8ca 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -4,7 +4,7 @@ use anyhow::{Result, bail}; use crate::git::Git; -use crate::github::{self, PrState, PullRequest}; +use crate::github::{GitHub, PrState, PullRequest}; use crate::issue::IssueUrl; pub enum Selection { @@ -66,8 +66,8 @@ impl Selection { /// their PRs in any state: the highest number seen on either decides. Fails /// if a local copy of the chosen branch in the Launch directory differs from /// origin's: the Run replaces it and deletes it at cleanup. -pub fn select(launch: &Git, issue: &IssueUrl) -> Result { - select_through(&GitHubAndOrigin { launch }, issue) +pub fn select(launch: &Git, github: &GitHub, issue: &IssueUrl) -> Result { + select_through(&GitHubAndOrigin { launch, github }, issue) } /// What shows an issue was started. @@ -82,8 +82,8 @@ pub enum Started { /// What shows `issue` was ever started, if it was: the first Issue branch for /// it on origin, or else the newest pull request from one, open, merged or /// closed. -pub fn started(launch: &Git, issue: &IssueUrl) -> Result> { - started_through(&GitHubAndOrigin { launch }, issue) +pub fn started(launch: &Git, github: &GitHub, issue: &IssueUrl) -> Result> { + started_through(&GitHubAndOrigin { launch, github }, issue) } /// What selection reads of origin, GitHub and the Launch directory. Each @@ -109,6 +109,7 @@ struct OnOrigin { /// The reads of GitHub, of the Launch directory `launch` and of its origin. struct GitHubAndOrigin<'a> { launch: &'a Git, + github: &'a GitHub, } impl Reads for GitHubAndOrigin<'_> { @@ -135,7 +136,8 @@ impl Reads for GitHubAndOrigin<'_> { /// One `gh pr list`. fn pull_requests(&self, issue: &IssueUrl, first_branch: &str) -> Result> { - github::pull_requests_with_head_prefix(issue, first_branch) + self.github + .pull_requests_with_head_prefix(issue, first_branch) } fn local_head(&self, branch: &str) -> Result> { diff --git a/src/ci.rs b/src/ci.rs index 617ecd254..5f5a8426d 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -9,7 +9,7 @@ use std::time::{Duration, Instant}; use anyhow::Result; -use crate::github::{self, ActionsJob, Check, CheckState}; +use crate::github::{ActionsJob, Check, CheckState, GitHub}; use crate::issue::IssueUrl; use crate::poll; use crate::progress; @@ -109,11 +109,11 @@ impl Outside for OnGitHub<'_> { type Deadline = Instant; fn checks_on(&mut self, sha: &str) -> Result> { - github::checks_on(self.issue, sha) + GitHub::new().checks_on(self.issue, sha) } fn rerun_failed_jobs(&mut self, workflow_run: u64) -> Result<()> { - github::rerun_failed_jobs(self.issue, workflow_run) + GitHub::new().rerun_failed_jobs(self.issue, workflow_run) } fn pause(&mut self) -> Result<()> { diff --git a/src/claim.rs b/src/claim.rs index 0255809e2..351eb0edd 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -14,7 +14,7 @@ use anyhow::{Context, Result}; use crate::branch; use crate::git::Git; -use crate::github; +use crate::github::GitHub; use crate::interrupt; use crate::issue::IssueUrl; use crate::labels::{self, Edit, Label, Labels, READY_FOR_AGENT}; @@ -45,7 +45,13 @@ pub struct Claim<'a> { /// `ready-for-agent`, is left as it is, with no request made. A failure names /// the Claim as what could not be made. pub fn make<'a>(issue: &'a IssueUrl, launch: &'a Git) -> Result> { - let claimed = Claimed::make(&mut OnGitHub { launch }, issue)?; + let claimed = Claimed::make( + &mut OnGitHub { + launch, + github: GitHub::new(), + }, + issue, + )?; Ok(Claim { launch, claimed }) } @@ -63,7 +69,13 @@ impl Claim<'_> { /// warning naming what to run by hand, and an interrupt doesn't stop it. pub fn end(self, reached: Option) { let launch = self.launch.completion(); - self.claimed.end(&mut OnGitHub { launch: &launch }, reached); + self.claimed.end( + &mut OnGitHub { + launch: &launch, + github: GitHub::new().completion(), + }, + reached, + ); } } @@ -98,15 +110,16 @@ struct State { /// The issue on GitHub, and origin as the Launch directory `launch` sees it. struct OnGitHub<'a> { launch: &'a Git, + github: GitHub, } impl Outside for OnGitHub<'_> { fn labels(&mut self, issue: &IssueUrl) -> Result { - github::issue_labels(issue) + self.github.issue_labels(issue) } fn state(&mut self, issue: &IssueUrl) -> Result { - let issue = github::issue(issue)?; + let issue = self.github.issue(issue)?; Ok(State { is_open: issue.is_open, labels: issue.labels, @@ -114,11 +127,11 @@ impl Outside for OnGitHub<'_> { } fn started(&mut self, issue: &IssueUrl) -> Result { - Ok(branch::started(self.launch, issue)?.is_some()) + Ok(branch::started(self.launch, &self.github, issue)?.is_some()) } fn apply(&mut self, edit: &Edit) -> Result<()> { - edit.apply() + edit.apply(&self.github) } fn interrupted(&mut self) -> bool { diff --git a/src/delivery.rs b/src/delivery.rs index 774991160..dea30caec 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -20,7 +20,7 @@ mod repair_loop; use crate::base_fix::BaseFix; use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::{FailedRun, PolicyRefusal, interrupted_or}; -use crate::github::{self, Mergeable, PullRequest}; +use crate::github::{GitHub, Mergeable, PullRequest}; use crate::harness::Choice; use crate::host; use crate::interrupt; @@ -433,19 +433,19 @@ impl Outside for InWorktree<'_> { } fn pull_request(&mut self) -> Result> { - github::pull_request_for(self.issue, self.worktree.branch()) + GitHub::new().pull_request_for(self.issue, self.worktree.branch()) } fn pr_body(&mut self, number: u64) -> Result { - github::pr_body(self.issue, number) + GitHub::new().pr_body(self.issue, number) } fn set_pr_body(&mut self, number: u64, body: &str) -> Result<()> { - github::set_pr_body(self.issue, number, body) + GitHub::new().set_pr_body(self.issue, number, body) } fn mark_ready(&mut self) -> Result<()> { - github::mark_ready(self.issue, self.worktree.branch()) + GitHub::new().mark_ready(self.issue, self.worktree.branch()) } fn take_to_goal(&mut self, pr_url: &str, goal: Goal) -> Result<()> { @@ -465,11 +465,11 @@ impl Outside for InWorktree<'_> { } fn issue_is_open(&mut self) -> Result { - github::issue_is_open(self.issue) + GitHub::new().completion().issue_is_open(self.issue) } fn close_issue(&mut self, comment: &str) -> Result<()> { - github::close_issue(self.issue, comment) + GitHub::new().completion().close_issue(self.issue, comment) } fn interrupted(&mut self) -> bool { @@ -529,11 +529,15 @@ impl FailedOutside for OnFailure<'_> { } fn pull_request(&mut self) -> Result> { - github::pull_request_for(self.issue, self.worktree.branch()) + GitHub::new() + .completion() + .pull_request_for(self.issue, self.worktree.branch()) } fn convert_to_draft(&mut self) -> Result<()> { - github::convert_to_draft(self.issue, self.worktree.branch()) + GitHub::new() + .completion() + .convert_to_draft(self.issue, self.worktree.branch()) } fn keep(&mut self) { @@ -648,7 +652,7 @@ impl repair_loop::Outside for RunOutside<'_> { } fn merge(&mut self, head: &str) -> Result<()> { - github::merge(self.issue, self.worktree.branch(), head) + GitHub::new().merge(self.issue, self.worktree.branch(), head) } fn interrupt_requested(&mut self) -> bool { @@ -672,12 +676,13 @@ fn pending(merge: Merge) -> Option { /// mergeable, waiting up to the grace period for GitHub to work out the last. fn ensure_pr_ready_and_mergeable(issue: &IssueUrl, branch: &str) -> Result<()> { progress::step("checking the PR is open, ready and mergeable"); - let pr = open(github::pull_request_for(issue, branch)?)?; + let github = GitHub::new(); + let pr = open(github.pull_request_for(issue, branch)?)?; if pr.is_draft { bail!("PR {} is a draft", pr.url); } let mergeable = poll::within(poll::grace_period(), || { - Ok(Some(github::mergeable(issue, branch)?).filter(|m| *m != Mergeable::Unknown)) + Ok(Some(github.mergeable(issue, branch)?).filter(|m| *m != Mergeable::Unknown)) })?; match mergeable { Some(Mergeable::Yes) => Ok(()), diff --git a/src/github.rs b/src/github.rs index f7b320aeb..44793362a 100644 --- a/src/github.rs +++ b/src/github.rs @@ -2,7 +2,7 @@ //! user. use std::fmt; -use std::process::Command; +use std::process::{Command, Output}; use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; @@ -10,37 +10,629 @@ use serde_json::Value; use crate::issue::IssueUrl; use crate::labels::{Label, Labels}; +use crate::process::{self, Control, Interruption}; -/// The `fields` of `issue`, as JSON. -fn issue_view(issue: &IssueUrl, fields: &str) -> Result { - gh_json(&[ - "issue", - "view", - &issue.number.to_string(), - "--repo", - &issue.repo_slug(), - "--json", - fields, - ]) -} +#[cfg(test)] +mod execution_tests; -/// Whether the issue `json` describes, with its `state`, is open. -fn state_is_open(json: &Value) -> Result { - Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") +/// GitHub CLI operations bound to an immutable interruption policy. +/// Ordinary views stop for Command interruption; completion views finish +/// independently without changing the recorded request or other views. +pub struct GitHub { + interruption: Interruption, } -/// Whether `issue` is open. -pub fn issue_is_open(issue: &IssueUrl) -> Result { - state_is_open(&issue_view(issue, "state")?) +impl GitHub { + pub fn new() -> Self { + Self { + interruption: Interruption::Ordinary, + } + } + + pub fn completion(&self) -> Self { + Self { + interruption: Interruption::Completion, + } + } + + /// The `fields` of `issue`, as JSON. + fn issue_view(&self, issue: &IssueUrl, fields: &str) -> Result { + self.gh_json(&[ + "issue", + "view", + &issue.number.to_string(), + "--repo", + &issue.repo_slug(), + "--json", + fields, + ]) + } + + /// Whether `issue` is open. + pub fn issue_is_open(&self, issue: &IssueUrl) -> Result { + state_is_open(&self.issue_view(issue, "state")?) + } + + /// The title of `issue`. + pub fn issue_title(&self, issue: &IssueUrl) -> Result { + let json = self.issue_view(issue, "title")?; + Ok(json["title"] + .as_str() + .context("gh output has no title")? + .to_string()) + } + + /// `issue`'s state, labels and when it was created. + pub fn issue(&self, issue: &IssueUrl) -> Result { + let json = self.issue_view(issue, "state,labels,createdAt")?; + let created = json["createdAt"] + .as_str() + .context("gh output has no createdAt")?; + Ok(Issue { + is_open: state_is_open(&json)?, + labels: labels_of(&json)?, + created: DateTime::parse_from_rfc3339(created) + .with_context(|| format!("gh output has an unreadable createdAt {created}"))? + .to_utc(), + }) + } + + /// The labels of `issue`. + pub fn issue_labels(&self, issue: &IssueUrl) -> Result { + labels_of(&self.issue_view(issue, "labels")?) + } + + /// Set `issue`'s labels to exactly `labels`, in one request, so a swap of + /// one label for another can't stop halfway. Through the REST API: `gh issue + /// edit` fails on the GitHub Projects (classic) sunset in older `gh`. + pub fn set_labels(&self, issue: &IssueUrl, labels: &Labels) -> Result<()> { + let path = labels_path(issue); + let fields: Vec = labels + .names() + .map(|label| format!("labels[]={label}")) + .collect(); + let mut args = vec!["api", "--method", "PUT", &path, "--silent"]; + for field in &fields { + args.extend(["-f", field]); + } + self.gh(&args) + } + + /// Take `label` off `issue`, in one request that keeps its other labels. + /// Through the REST API, as [`GitHub::set_labels`] is. GitHub refuses it if the issue + /// does not have the label. + pub fn remove_label(&self, issue: &IssueUrl, label: &str) -> Result<()> { + self.gh(&[ + "api", + "--method", + "DELETE", + &label_path(issue, label), + "--silent", + ]) + } + + /// The sub-issues of `issue`, its Tickets if it has any, in one query. + pub fn tickets(&self, issue: &IssueUrl) -> Result> { + let json = self.issue_query(issue, TICKETS_QUERY)?; + nodes( + &json["data"]["repository"]["issue"]["subIssues"], + "subIssues", + )? + .iter() + .map(|node| { + let mut blockers = Vec::new(); + let mut open_blockers = Vec::new(); + for blocker in nodes(&node["blockedBy"], "blockedBy")? { + blockers.push(node_number(blocker)?); + if node_is_open(blocker)? { + open_blockers.push(node_number(blocker)?); + } + } + Ok(Ticket { + number: node_number(node)?, + is_open: node_is_open(node)?, + labels: named(nodes(&node["labels"], "labels")?), + has_sub_issues: sub_issue_count(node)? > 0, + blockers, + open_blockers, + }) + }) + .collect() + } + + /// The answer to `query`, a GraphQL query about `issue` that takes its + /// `$owner`, `$repo` and `$number`. + fn issue_query(&self, issue: &IssueUrl, query: &str) -> Result { + self.gh_json(&[ + "api", + "graphql", + "-f", + &format!("query={query}"), + "-f", + &format!("owner={}", issue.owner), + "-f", + &format!("repo={}", issue.repo), + "-F", + &format!("number={}", issue.number), + ]) + } + + /// Close `issue` with `comment`. + pub fn close_issue(&self, issue: &IssueUrl, comment: &str) -> Result<()> { + self.gh(&[ + "issue", + "close", + &issue.number.to_string(), + "--repo", + &issue.repo_slug(), + "--comment", + comment, + ]) + } + + /// Every open issue labelled `label` in the repository `repo`, an + /// `owner/repo`, newest first. They come from GitHub's issue list, not its + /// search, whose index can be a while behind an issue just opened. + pub fn open_issues_labelled(&self, repo: &str, label: Label) -> Result> { + self.issues_labelled(repo, label, "open") + } + + /// Every closed issue labelled `label` in the repository `repo`, an + /// `owner/repo`, newest first. + pub fn closed_issues_labelled(&self, repo: &str, label: Label) -> Result> { + self.issues_labelled(repo, label, "closed") + } + + /// Every issue labelled `label` in the repository `repo` whose state is + /// `state`, `open` or `closed`, newest first. + fn issues_labelled(&self, repo: &str, label: Label, state: &str) -> Result> { + let json = self.gh_json(&[ + "issue", + "list", + "--state", + state, + "--repo", + repo, + "--label", + label.name(), + "--json", + "url,title,labels", + "--limit", + "1000", + ])?; + json.as_array() + .context("gh issue list did not return a list")? + .iter() + .map(|listed| { + let field = |name: &str| { + listed[name] + .as_str() + .with_context(|| format!("gh issue list output has no {name}")) + }; + Ok(ListedIssue { + issue: IssueUrl::parse(field("url")?)?, + title: field("title")?.to_string(), + labels: labels_of(listed)?, + }) + }) + .collect() + } + + /// `issue` as a Pickup run that lists issues by `label` reads it, in one + /// query. + pub fn candidate(&self, issue: &IssueUrl, label: Label) -> Result { + let json = self.issue_query(issue, CANDIDATE_QUERY)?; + let issue = &json["data"]["repository"]["issue"]; + let parent = issue["parent"]["url"] + .as_str() + .map(IssueUrl::parse) + .transpose()?; + let mut open_blockers = Vec::new(); + for blocker in nodes(&issue["blockedBy"], "blockedBy")? { + if node_is_open(blocker)? { + open_blockers.push(node_number(blocker)?); + } + } + let at = |event: &Value| -> Result> { + let at = event["createdAt"] + .as_str() + .context("gh api graphql output has an event with no createdAt")?; + Ok(DateTime::parse_from_rfc3339(at) + .with_context(|| format!("gh api graphql output has an unreadable createdAt {at}"))? + .to_utc()) + }; + let mut shaped = Vec::new(); + for event in nodes(&issue["labelled"], "labelled")? { + let name = event["label"]["name"].as_str(); + if name.is_some_and(|name| label.is_named(name)) { + shaped.push(Shaped { + by: Shaping::Labelled, + at: at(event)?, + }); + } + } + for event in nodes(&issue["linked"], "linked")? { + let by = match event["__typename"].as_str() { + Some("SubIssueAddedEvent" | "SubIssueRemovedEvent") => Shaping::SubIssues, + Some("BlockedByAddedEvent" | "BlockedByRemovedEvent") => Shaping::Blockers, + other => bail!("gh api graphql output has an unknown event {other:?}"), + }; + shaped.push(Shaped { by, at: at(event)? }); + } + Ok(Candidate { + parent, + sub_issue_is_open: nodes(&issue["subIssues"], "subIssues")? + .iter() + .map(node_is_open) + .collect::>()?, + open_blockers, + last_shaped: shaped.into_iter().max_by_key(|shaped| shaped.at), + }) + } + + /// Add each of `labels` to the repository `repo`, an `owner/repo`, with its + /// description, unless the repository has it already. + pub fn ensure_labels(&self, repo: &str, labels: &[Label]) -> Result<()> { + let known = self.gh_json(&[ + "label", "list", "--repo", repo, "--json", "name", "--limit", "1000", + ])?; + let known = named( + known + .as_array() + .context("gh label list did not return a list")?, + ); + for label in labels { + if !known.has(*label) { + self.gh(&[ + "label", + "create", + label.name(), + "--repo", + repo, + "--description", + label.description(), + ])?; + } + } + Ok(()) + } + + /// Open an issue titled `title`, with `body` and `labels`, in the repository + /// of `issue`, and return it. Each label is first added to the repository, + /// with its description, if the repository lacks it: `gh` refuses a label it + /// doesn't know. + pub fn create_issue( + &self, + issue: &IssueUrl, + title: &str, + body: &str, + labels: &[Label], + ) -> Result { + let repo = issue.repo_slug(); + self.ensure_labels(&repo, labels)?; + let names: Vec<&str> = labels.iter().map(|label| label.name()).collect(); + let url = self.gh_stdout(&[ + "issue", + "create", + "--repo", + &repo, + "--title", + title, + "--body", + body, + "--label", + &names.join(","), + ])?; + IssueUrl::parse(&url).context("gh issue create did not print the issue's URL") + } + + /// The public email of the signed-in user's GitHub profile, or `None` if it + /// is private. Private addresses need the `user` scope, which a default + /// `gh auth login` token lacks, so thirdshift never asks for them. + pub fn profile_email(&self) -> Result> { + let json = self.gh_json(&["api", "user"])?; + Ok(json["email"].as_str().map(str::to_string)) + } + + /// The `fields` of the pull request `pr`, its head branch or its number, as + /// JSON. + fn pr_view(&self, issue: &IssueUrl, pr: &str, fields: &str) -> Result { + self.gh_json(&[ + "pr", + "view", + pr, + "--repo", + &issue.repo_slug(), + "--json", + fields, + ]) + } + + /// The pull request whose head is `branch`, if `gh` finds one. + pub fn pull_request_for(&self, issue: &IssueUrl, branch: &str) -> Result> { + let json = match self.pr_view(issue, branch, PR_FIELDS) { + Ok(json) => json, + Err(error) if format!("{error:#}").contains("no pull requests found") => { + return Ok(None); + } + Err(error) => return Err(error), + }; + PullRequest::from_json(&json).map(Some) + } + + /// Every pull request in this repository, in any state, whose head branch + /// starts with `prefix`. PRs from forks are left out: their heads are not + /// this repository's branches. + pub fn pull_requests_with_head_prefix( + &self, + issue: &IssueUrl, + prefix: &str, + ) -> Result> { + let json = self.gh_json(&[ + "pr", + "list", + "--repo", + &issue.repo_slug(), + "--state", + "all", + "--search", + &format!("head:{prefix}"), + "--limit", + "1000", + "--json", + &format!("{PR_FIELDS},isCrossRepository"), + ])?; + json.as_array() + .context("gh pr list did not return a list")? + .iter() + .filter(|pr| pr["isCrossRepository"] != Value::Bool(true)) + .map(PullRequest::from_json) + .collect() + } + + /// Open a draft pull request from `head` into `base`, and return its URL. + pub fn create_draft_pr( + &self, + issue: &IssueUrl, + head: &str, + base: &str, + title: &str, + body: &str, + ) -> Result { + self.gh_stdout(&[ + "pr", + "create", + "--repo", + &issue.repo_slug(), + "--head", + head, + "--base", + base, + "--title", + title, + "--body", + body, + "--draft", + ]) + } + + /// The body of pull request `number`. + pub fn pr_body(&self, issue: &IssueUrl, number: u64) -> Result { + let json = self.pr_view(issue, &number.to_string(), "body")?; + Ok(json["body"] + .as_str() + .context("gh output has no body")? + .to_string()) + } + + /// Set the body of pull request `number` to `body`, through the REST API: + /// `gh pr edit` fails on the GitHub Projects (classic) sunset in older `gh`. + pub fn set_pr_body(&self, issue: &IssueUrl, number: u64, body: &str) -> Result<()> { + self.gh(&[ + "api", + "--method", + "PATCH", + &format!("repos/{}/pulls/{number}", issue.repo_slug()), + "-f", + &format!("body={body}"), + "--silent", + ]) + } + + /// Mark the pull request whose head is `branch` ready for review. + pub fn mark_ready(&self, issue: &IssueUrl, branch: &str) -> Result<()> { + self.gh(&["pr", "ready", branch, "--repo", &issue.repo_slug()]) + } + + /// Convert the pull request whose head is `branch` back to a draft. + pub fn convert_to_draft(&self, issue: &IssueUrl, branch: &str) -> Result<()> { + self.gh(&[ + "pr", + "ready", + branch, + "--undo", + "--repo", + &issue.repo_slug(), + ]) + } + + /// Merge the pull request whose head is `branch` into its base with a merge + /// commit, but only if its head is still `head`. Never GitHub's auto-merge + /// (ADR-0004), and never `--delete-branch`, whose local deletion would + /// interfere with the worktree. If `gh` fails but the pull request merged at + /// `head` anyway, e.g. because `gh` was interrupted once GitHub had merged, + /// that is a merge. + pub fn merge(&self, issue: &IssueUrl, branch: &str, head: &str) -> Result<()> { + let Err(error) = self.gh(&[ + "pr", + "merge", + branch, + "--repo", + &issue.repo_slug(), + "--merge", + "--match-head-commit", + head, + ]) else { + return Ok(()); + }; + match self.completion().merged_head(issue, branch) { + Ok(Some(merged)) if merged == head => Ok(()), + _ => Err(error), + } + } + + /// The head commit the pull request whose head is `branch` was merged at, if + /// it is merged. + fn merged_head(&self, issue: &IssueUrl, branch: &str) -> Result> { + let json = self.pr_view(issue, branch, "state,headRefOid")?; + if json["state"] != "MERGED" { + return Ok(None); + } + let head = json["headRefOid"] + .as_str() + .context("gh output has no headRefOid")?; + Ok(Some(head.to_string())) + } + + /// Whether the pull request whose head is `branch` can be merged. + pub fn mergeable(&self, issue: &IssueUrl, branch: &str) -> Result { + let json = self.pr_view(issue, branch, "mergeable")?; + match json["mergeable"].as_str() { + Some("MERGEABLE") => Ok(Mergeable::Yes), + Some("CONFLICTING") => Ok(Mergeable::No), + Some("UNKNOWN") => Ok(Mergeable::Unknown), + other => bail!("gh output has an unknown mergeable state {other:?}"), + } + } + + /// Every check run and commit status on commit `sha`. Of the check runs + /// sharing a name, as the attempts of a re-run job do, only the latest is + /// here. + pub fn checks_on(&self, issue: &IssueUrl, sha: &str) -> Result> { + let commit = format!("repos/{}/commits/{sha}", issue.repo_slug()); + let runs = self.gh_api_items(&format!("{commit}/check-runs?per_page=100"), "check_runs")?; + let statuses = self.gh_api_items(&format!("{commit}/status?per_page=100"), "statuses")?; + let url = |value: &Value| { + value + .as_str() + .filter(|url| !url.is_empty()) + .map(String::from) + }; + + let mut checks = Vec::new(); + for run in &runs { + let state = match (run["status"].as_str(), run["conclusion"].as_str()) { + (Some("completed"), Some("success" | "neutral" | "skipped")) => CheckState::Passed, + (Some("completed"), _) => CheckState::Failed, + _ => CheckState::Pending, + }; + checks.push(Check { + name: run["name"] + .as_str() + .context("a check run has no name")? + .to_string(), + state, + job: ActionsJob::of(run["id"].as_u64(), run["details_url"].as_str()), + url: url(&run["details_url"]).or_else(|| url(&run["html_url"])), + }); + } + for status in &statuses { + let state = match status["state"].as_str() { + Some("success") => CheckState::Passed, + Some("pending") => CheckState::Pending, + _ => CheckState::Failed, + }; + checks.push(Check { + name: status["context"] + .as_str() + .context("a commit status has no context")? + .to_string(), + state, + url: url(&status["target_url"]), + job: None, + }); + } + Ok(checks) + } + + /// Ask GitHub to re-run the failed jobs of the GitHub Actions workflow run + /// `workflow_run`, as a new attempt of it. + pub fn rerun_failed_jobs(&self, issue: &IssueUrl, workflow_run: u64) -> Result<()> { + self.gh(&[ + "run", + "rerun", + &workflow_run.to_string(), + "--failed", + "--repo", + &issue.repo_slug(), + ]) + } + + /// Run `gh `, failing with its stderr if it exits non-zero. + fn gh(&self, args: &[&str]) -> Result<()> { + self.gh_stdout(args).map(drop) + } + + /// Run `gh ` and return its trimmed stdout, failing with its stderr if + /// it exits non-zero. + fn gh_stdout(&self, args: &[&str]) -> Result { + let output = self.output(args)?; + if !output.status.success() { + bail!( + "gh {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) + } + + /// Every item of the `field` list of the GitHub API's `path`, across all its + /// pages. + fn gh_api_items(&self, path: &str, field: &str) -> Result> { + let output = self.output(&["api", "--paginate", "--jq", &format!(".{field}[]"), path])?; + if !output.status.success() { + bail!( + "gh api {path} failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + serde_json::Deserializer::from_slice(&output.stdout) + .into_iter() + .collect::>() + .with_context(|| format!("gh api {path} returned invalid JSON")) + } + + /// Run `gh ` and parse its stdout as JSON, failing with gh's stderr if + /// it exits non-zero. + fn gh_json(&self, args: &[&str]) -> Result { + let command = format!("gh {}", args[..2].join(" ")); + let output = self.output(args)?; + if !output.status.success() { + bail!( + "{command} failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + serde_json::from_slice(&output.stdout) + .with_context(|| format!("{command} returned invalid JSON")) + } + fn output(&self, args: &[&str]) -> Result { + process::output( + Command::new("gh").args(args), + None, + Control { + name: "gh", + interruption: self.interruption, + stop: &|child| process::stop(child, &[libc::SIGTERM]), + }, + ) + } } -/// The title of `issue`. -pub fn issue_title(issue: &IssueUrl) -> Result { - let json = issue_view(issue, "title")?; - Ok(json["title"] - .as_str() - .context("gh output has no title")? - .to_string()) +/// Whether the issue `json` describes, with its `state`, is open. +fn state_is_open(json: &Value) -> Result { + Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") } /// An issue as an Architect run reads the issue its Architecture review @@ -52,21 +644,6 @@ pub struct Issue { pub created: DateTime, } -/// `issue`'s state, labels and when it was created. -pub fn issue(issue: &IssueUrl) -> Result { - let json = issue_view(issue, "state,labels,createdAt")?; - let created = json["createdAt"] - .as_str() - .context("gh output has no createdAt")?; - Ok(Issue { - is_open: state_is_open(&json)?, - labels: labels_of(&json)?, - created: DateTime::parse_from_rfc3339(created) - .with_context(|| format!("gh output has an unreadable createdAt {created}"))? - .to_utc(), - }) -} - /// The labels of the issue `json` describes, with its `labels`. fn labels_of(json: &Value) -> Result { Ok(named( @@ -84,57 +661,23 @@ fn named(labels: &[Value]) -> Labels { .collect() } -/// The labels of `issue`. -pub fn issue_labels(issue: &IssueUrl) -> Result { - labels_of(&issue_view(issue, "labels")?) -} - /// The REST API's path for the labels of `issue`. fn labels_path(issue: &IssueUrl) -> String { format!("repos/{}/issues/{}/labels", issue.repo_slug(), issue.number) } -/// Set `issue`'s labels to exactly `labels`, in one request, so a swap of -/// one label for another can't stop halfway. Through the REST API: `gh issue -/// edit` fails on the GitHub Projects (classic) sunset in older `gh`. -pub fn set_labels(issue: &IssueUrl, labels: &Labels) -> Result<()> { - let path = labels_path(issue); - let fields: Vec = labels - .names() - .map(|label| format!("labels[]={label}")) - .collect(); - let mut args = vec!["api", "--method", "PUT", &path, "--silent"]; - for field in &fields { - args.extend(["-f", field]); - } - gh(&args) -} - /// The REST API's path for `label` on `issue`. fn label_path(issue: &IssueUrl, label: &str) -> String { format!("{}/{label}", labels_path(issue)) } -/// Take `label` off `issue`, in one request that keeps its other labels. -/// Through the REST API, as [`set_labels`] is. GitHub refuses it if the issue -/// does not have the label. -pub fn remove_label(issue: &IssueUrl, label: &str) -> Result<()> { - gh(&[ - "api", - "--method", - "DELETE", - &label_path(issue, label), - "--silent", - ]) -} - -/// The command that does what [`remove_label`] does, to run by hand. +/// The command that does what [`GitHub::remove_label`] does, to run by hand. pub fn remove_label_command(issue: &IssueUrl, label: Label) -> String { format!("gh api --method DELETE {}", label_path(issue, label.name())) } /// The command that adds `label` to `issue`, keeping its other labels, to -/// run by hand: unlike [`set_labels`], it needs none of the others named. +/// run by hand: unlike [`GitHub::set_labels`], it needs none of the others named. pub fn add_label_command(issue: &IssueUrl, label: Label) -> String { format!( "gh api --method POST {} -f 'labels[]={label}'", @@ -175,52 +718,6 @@ query($owner: String!, $repo: String!, $number: Int!) { } }"; -/// The sub-issues of `issue`, its Tickets if it has any, in one query. -pub fn tickets(issue: &IssueUrl) -> Result> { - let json = issue_query(issue, TICKETS_QUERY)?; - nodes( - &json["data"]["repository"]["issue"]["subIssues"], - "subIssues", - )? - .iter() - .map(|node| { - let mut blockers = Vec::new(); - let mut open_blockers = Vec::new(); - for blocker in nodes(&node["blockedBy"], "blockedBy")? { - blockers.push(node_number(blocker)?); - if node_is_open(blocker)? { - open_blockers.push(node_number(blocker)?); - } - } - Ok(Ticket { - number: node_number(node)?, - is_open: node_is_open(node)?, - labels: named(nodes(&node["labels"], "labels")?), - has_sub_issues: sub_issue_count(node)? > 0, - blockers, - open_blockers, - }) - }) - .collect() -} - -/// The answer to `query`, a GraphQL query about `issue` that takes its -/// `$owner`, `$repo` and `$number`. -fn issue_query(issue: &IssueUrl, query: &str) -> Result { - gh_json(&[ - "api", - "graphql", - "-f", - &format!("query={query}"), - "-f", - &format!("owner={}", issue.owner), - "-f", - &format!("repo={}", issue.repo), - "-F", - &format!("number={}", issue.number), - ]) -} - /// The `nodes` of `connection`, the `what` of a GraphQL answer. fn nodes<'a>(connection: &'a Value, what: &str) -> Result<&'a Vec> { connection["nodes"] @@ -250,19 +747,6 @@ fn sub_issue_count(node: &Value) -> Result { .context("gh api graphql output has an issue with no subIssues count") } -/// Close `issue` with `comment`. -pub fn close_issue(issue: &IssueUrl, comment: &str) -> Result<()> { - gh(&[ - "issue", - "close", - &issue.number.to_string(), - "--repo", - &issue.repo_slug(), - "--comment", - comment, - ]) -} - /// An issue as a listing of its repository's issues gives it. #[derive(Debug, Clone)] pub struct ListedIssue { @@ -271,54 +755,6 @@ pub struct ListedIssue { pub labels: Labels, } -/// Every open issue labelled `label` in the repository `repo`, an -/// `owner/repo`, newest first. They come from GitHub's issue list, not its -/// search, whose index can be a while behind an issue just opened. -pub fn open_issues_labelled(repo: &str, label: Label) -> Result> { - issues_labelled(repo, label, "open") -} - -/// Every closed issue labelled `label` in the repository `repo`, an -/// `owner/repo`, newest first. -pub fn closed_issues_labelled(repo: &str, label: Label) -> Result> { - issues_labelled(repo, label, "closed") -} - -/// Every issue labelled `label` in the repository `repo` whose state is -/// `state`, `open` or `closed`, newest first. -fn issues_labelled(repo: &str, label: Label, state: &str) -> Result> { - let json = gh_json(&[ - "issue", - "list", - "--state", - state, - "--repo", - repo, - "--label", - label.name(), - "--json", - "url,title,labels", - "--limit", - "1000", - ])?; - json.as_array() - .context("gh issue list did not return a list")? - .iter() - .map(|listed| { - let field = |name: &str| { - listed[name] - .as_str() - .with_context(|| format!("gh issue list output has no {name}")) - }; - Ok(ListedIssue { - issue: IssueUrl::parse(field("url")?)?, - title: field("title")?.to_string(), - labels: labels_of(listed)?, - }) - }) - .collect() -} - /// An open issue labelled for a Pickup run to take, as the Pickup run reads /// what its listing doesn't give. pub struct Candidate { @@ -396,121 +832,6 @@ query($owner: String!, $repo: String!, $number: Int!) { } }"; -/// `issue` as a Pickup run that lists issues by `label` reads it, in one -/// query. -pub fn candidate(issue: &IssueUrl, label: Label) -> Result { - let json = issue_query(issue, CANDIDATE_QUERY)?; - let issue = &json["data"]["repository"]["issue"]; - let parent = issue["parent"]["url"] - .as_str() - .map(IssueUrl::parse) - .transpose()?; - let mut open_blockers = Vec::new(); - for blocker in nodes(&issue["blockedBy"], "blockedBy")? { - if node_is_open(blocker)? { - open_blockers.push(node_number(blocker)?); - } - } - let at = |event: &Value| -> Result> { - let at = event["createdAt"] - .as_str() - .context("gh api graphql output has an event with no createdAt")?; - Ok(DateTime::parse_from_rfc3339(at) - .with_context(|| format!("gh api graphql output has an unreadable createdAt {at}"))? - .to_utc()) - }; - let mut shaped = Vec::new(); - for event in nodes(&issue["labelled"], "labelled")? { - let name = event["label"]["name"].as_str(); - if name.is_some_and(|name| label.is_named(name)) { - shaped.push(Shaped { - by: Shaping::Labelled, - at: at(event)?, - }); - } - } - for event in nodes(&issue["linked"], "linked")? { - let by = match event["__typename"].as_str() { - Some("SubIssueAddedEvent" | "SubIssueRemovedEvent") => Shaping::SubIssues, - Some("BlockedByAddedEvent" | "BlockedByRemovedEvent") => Shaping::Blockers, - other => bail!("gh api graphql output has an unknown event {other:?}"), - }; - shaped.push(Shaped { by, at: at(event)? }); - } - Ok(Candidate { - parent, - sub_issue_is_open: nodes(&issue["subIssues"], "subIssues")? - .iter() - .map(node_is_open) - .collect::>()?, - open_blockers, - last_shaped: shaped.into_iter().max_by_key(|shaped| shaped.at), - }) -} - -/// Add each of `labels` to the repository `repo`, an `owner/repo`, with its -/// description, unless the repository has it already. -pub fn ensure_labels(repo: &str, labels: &[Label]) -> Result<()> { - let known = gh_json(&[ - "label", "list", "--repo", repo, "--json", "name", "--limit", "1000", - ])?; - let known = named( - known - .as_array() - .context("gh label list did not return a list")?, - ); - for label in labels { - if !known.has(*label) { - gh(&[ - "label", - "create", - label.name(), - "--repo", - repo, - "--description", - label.description(), - ])?; - } - } - Ok(()) -} - -/// Open an issue titled `title`, with `body` and `labels`, in the repository -/// of `issue`, and return it. Each label is first added to the repository, -/// with its description, if the repository lacks it: `gh` refuses a label it -/// doesn't know. -pub fn create_issue( - issue: &IssueUrl, - title: &str, - body: &str, - labels: &[Label], -) -> Result { - let repo = issue.repo_slug(); - ensure_labels(&repo, labels)?; - let names: Vec<&str> = labels.iter().map(|label| label.name()).collect(); - let url = gh_stdout(&[ - "issue", - "create", - "--repo", - &repo, - "--title", - title, - "--body", - body, - "--label", - &names.join(","), - ])?; - IssueUrl::parse(&url).context("gh issue create did not print the issue's URL") -} - -/// The public email of the signed-in user's GitHub profile, or `None` if it -/// is private. Private addresses need the `user` scope, which a default -/// `gh auth login` token lacks, so thirdshift never asks for them. -pub fn profile_email() -> Result> { - let json = gh_json(&["api", "user"])?; - Ok(json["email"].as_str().map(str::to_string)) -} - const PR_FIELDS: &str = "number,url,state,headRefName,baseRefName,isDraft"; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -570,159 +891,6 @@ impl PullRequest { } } -/// The `fields` of the pull request `pr`, its head branch or its number, as -/// JSON. -fn pr_view(issue: &IssueUrl, pr: &str, fields: &str) -> Result { - gh_json(&[ - "pr", - "view", - pr, - "--repo", - &issue.repo_slug(), - "--json", - fields, - ]) -} - -/// The pull request whose head is `branch`, if `gh` finds one. -pub fn pull_request_for(issue: &IssueUrl, branch: &str) -> Result> { - let json = match pr_view(issue, branch, PR_FIELDS) { - Ok(json) => json, - Err(error) if format!("{error:#}").contains("no pull requests found") => return Ok(None), - Err(error) => return Err(error), - }; - PullRequest::from_json(&json).map(Some) -} - -/// Every pull request in this repository, in any state, whose head branch -/// starts with `prefix`. PRs from forks are left out: their heads are not -/// this repository's branches. -pub fn pull_requests_with_head_prefix(issue: &IssueUrl, prefix: &str) -> Result> { - let json = gh_json(&[ - "pr", - "list", - "--repo", - &issue.repo_slug(), - "--state", - "all", - "--search", - &format!("head:{prefix}"), - "--limit", - "1000", - "--json", - &format!("{PR_FIELDS},isCrossRepository"), - ])?; - json.as_array() - .context("gh pr list did not return a list")? - .iter() - .filter(|pr| pr["isCrossRepository"] != Value::Bool(true)) - .map(PullRequest::from_json) - .collect() -} - -/// Open a draft pull request from `head` into `base`, and return its URL. -pub fn create_draft_pr( - issue: &IssueUrl, - head: &str, - base: &str, - title: &str, - body: &str, -) -> Result { - gh_stdout(&[ - "pr", - "create", - "--repo", - &issue.repo_slug(), - "--head", - head, - "--base", - base, - "--title", - title, - "--body", - body, - "--draft", - ]) -} - -/// The body of pull request `number`. -pub fn pr_body(issue: &IssueUrl, number: u64) -> Result { - let json = pr_view(issue, &number.to_string(), "body")?; - Ok(json["body"] - .as_str() - .context("gh output has no body")? - .to_string()) -} - -/// Set the body of pull request `number` to `body`, through the REST API: -/// `gh pr edit` fails on the GitHub Projects (classic) sunset in older `gh`. -pub fn set_pr_body(issue: &IssueUrl, number: u64, body: &str) -> Result<()> { - gh(&[ - "api", - "--method", - "PATCH", - &format!("repos/{}/pulls/{number}", issue.repo_slug()), - "-f", - &format!("body={body}"), - "--silent", - ]) -} - -/// Mark the pull request whose head is `branch` ready for review. -pub fn mark_ready(issue: &IssueUrl, branch: &str) -> Result<()> { - gh(&["pr", "ready", branch, "--repo", &issue.repo_slug()]) -} - -/// Convert the pull request whose head is `branch` back to a draft. -pub fn convert_to_draft(issue: &IssueUrl, branch: &str) -> Result<()> { - gh(&[ - "pr", - "ready", - branch, - "--undo", - "--repo", - &issue.repo_slug(), - ]) -} - -/// Merge the pull request whose head is `branch` into its base with a merge -/// commit, but only if its head is still `head`. Never GitHub's auto-merge -/// (ADR-0004), and never `--delete-branch`, whose local deletion would -/// interfere with the worktree. If `gh` fails but the pull request merged at -/// `head` anyway, e.g. because `gh` was interrupted once GitHub had merged, -/// that is a merge. -pub fn merge(issue: &IssueUrl, branch: &str, head: &str) -> Result<()> { - let Err(error) = gh(&[ - "pr", - "merge", - branch, - "--repo", - &issue.repo_slug(), - "--merge", - "--match-head-commit", - head, - ]) else { - return Ok(()); - }; - match merged_head(issue, branch) { - Ok(Some(merged)) if merged == head => Ok(()), - _ => Err(error), - } -} - -/// The head commit the pull request whose head is `branch` was merged at, if -/// it is merged. -fn merged_head(issue: &IssueUrl, branch: &str) -> Result> { - let json = pr_view(issue, branch, "state,headRefOid")?; - if json["state"] != "MERGED" { - return Ok(None); - } - let head = json["headRefOid"] - .as_str() - .context("gh output has no headRefOid")?; - Ok(Some(head.to_string())) -} - /// Whether a pull request can be merged into its base. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Mergeable { @@ -733,17 +901,6 @@ pub enum Mergeable { Unknown, } -/// Whether the pull request whose head is `branch` can be merged. -pub fn mergeable(issue: &IssueUrl, branch: &str) -> Result { - let json = pr_view(issue, branch, "mergeable")?; - match json["mergeable"].as_str() { - Some("MERGEABLE") => Ok(Mergeable::Yes), - Some("CONFLICTING") => Ok(Mergeable::No), - Some("UNKNOWN") => Ok(Mergeable::Unknown), - other => bail!("gh output has an unknown mergeable state {other:?}"), - } -} - /// Where a check or status stands. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CheckState { @@ -785,128 +942,6 @@ impl ActionsJob { } } -/// Every check run and commit status on commit `sha`. Of the check runs -/// sharing a name, as the attempts of a re-run job do, only the latest is -/// here. -pub fn checks_on(issue: &IssueUrl, sha: &str) -> Result> { - let commit = format!("repos/{}/commits/{sha}", issue.repo_slug()); - let runs = gh_api_items(&format!("{commit}/check-runs?per_page=100"), "check_runs")?; - let statuses = gh_api_items(&format!("{commit}/status?per_page=100"), "statuses")?; - let url = |value: &Value| { - value - .as_str() - .filter(|url| !url.is_empty()) - .map(String::from) - }; - - let mut checks = Vec::new(); - for run in &runs { - let state = match (run["status"].as_str(), run["conclusion"].as_str()) { - (Some("completed"), Some("success" | "neutral" | "skipped")) => CheckState::Passed, - (Some("completed"), _) => CheckState::Failed, - _ => CheckState::Pending, - }; - checks.push(Check { - name: run["name"] - .as_str() - .context("a check run has no name")? - .to_string(), - state, - job: ActionsJob::of(run["id"].as_u64(), run["details_url"].as_str()), - url: url(&run["details_url"]).or_else(|| url(&run["html_url"])), - }); - } - for status in &statuses { - let state = match status["state"].as_str() { - Some("success") => CheckState::Passed, - Some("pending") => CheckState::Pending, - _ => CheckState::Failed, - }; - checks.push(Check { - name: status["context"] - .as_str() - .context("a commit status has no context")? - .to_string(), - state, - url: url(&status["target_url"]), - job: None, - }); - } - Ok(checks) -} - -/// Ask GitHub to re-run the failed jobs of the GitHub Actions workflow run -/// `workflow_run`, as a new attempt of it. -pub fn rerun_failed_jobs(issue: &IssueUrl, workflow_run: u64) -> Result<()> { - gh(&[ - "run", - "rerun", - &workflow_run.to_string(), - "--failed", - "--repo", - &issue.repo_slug(), - ]) -} - -/// Run `gh `, failing with its stderr if it exits non-zero. -fn gh(args: &[&str]) -> Result<()> { - gh_stdout(args).map(drop) -} - -/// Run `gh ` and return its trimmed stdout, failing with its stderr if -/// it exits non-zero. -fn gh_stdout(args: &[&str]) -> Result { - let output = Command::new("gh") - .args(args) - .output() - .context("could not run gh")?; - if !output.status.success() { - bail!( - "gh {} failed: {}", - args.join(" "), - String::from_utf8_lossy(&output.stderr).trim() - ); - } - Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) -} - -/// Every item of the `field` list of the GitHub API's `path`, across all its -/// pages. -fn gh_api_items(path: &str, field: &str) -> Result> { - let output = Command::new("gh") - .args(["api", "--paginate", "--jq", &format!(".{field}[]"), path]) - .output() - .context("could not run gh")?; - if !output.status.success() { - bail!( - "gh api {path} failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); - } - serde_json::Deserializer::from_slice(&output.stdout) - .into_iter() - .collect::>() - .with_context(|| format!("gh api {path} returned invalid JSON")) -} - -/// Run `gh ` and parse its stdout as JSON, failing with gh's stderr if -/// it exits non-zero. -fn gh_json(args: &[&str]) -> Result { - let command = format!("gh {}", args[..2].join(" ")); - let output = Command::new("gh") - .args(args) - .output() - .context("could not run gh")?; - if !output.status.success() { - bail!( - "{command} failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); - } - serde_json::from_slice(&output.stdout) - .with_context(|| format!("{command} returned invalid JSON")) -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs new file mode 100644 index 000000000..79559e4a2 --- /dev/null +++ b/src/github/execution_tests.rs @@ -0,0 +1,379 @@ +use super::*; +use crate::interrupt; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::PathBuf; +use std::time::{Duration, Instant}; + +fn isolated(name: &str, test: impl FnOnce(libc::c_int)) { + if std::env::var("THIRDSHIFT_TEST_GITHUB").as_deref() == Ok(name) { + interrupt::install().unwrap(); + test( + std::env::var("THIRDSHIFT_TEST_GITHUB_SIGNAL") + .unwrap() + .parse() + .unwrap(), + ); + return; + } + for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { + let output = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + &format!("github::execution_tests::{name}"), + "--nocapture", + ]) + .env("THIRDSHIFT_TEST_GITHUB", name) + .env("THIRDSHIFT_TEST_GITHUB_SIGNAL", signal.to_string()) + .output() + .unwrap(); + assert!(output.status.success(), "signal {signal}: {output:?}"); + } +} + +/// PATH changes happen only in the isolated process. Cleanup uses the +/// executable's recorded PID, including after a failed assertion. +struct Fixture { + temp: tempfile::TempDir, +} + +impl Fixture { + fn new(script: &str) -> Self { + let fixture = Self { + temp: tempfile::tempdir().unwrap(), + }; + fixture.script(script); + let path = format!( + "{}:{}", + fixture.temp.path().display(), + std::env::var("PATH").unwrap() + ); + // SAFETY: this isolated process runs one test; no environment readers + // or execution workers have started. + unsafe { std::env::set_var("PATH", path) }; + fixture + } + + fn path(&self, name: &str) -> PathBuf { + self.temp.path().join(name) + } + + fn script(&self, script: &str) { + let gh = self.path("gh"); + fs::write( + &gh, + format!( + "#!/bin/sh\necho $$ > '{}'\n{script}\n", + self.path("pid").display() + ), + ) + .unwrap(); + fs::set_permissions(gh, fs::Permissions::from_mode(0o755)).unwrap(); + } + + fn pid(&self) -> Option { + fs::read_to_string(self.path("pid")) + .ok()? + .trim() + .parse() + .ok() + } +} + +fn exists(pid: libc::pid_t) -> bool { + // SAFETY: signal 0 only probes the fixture's recorded PID. + unsafe { libc::kill(pid, 0) == 0 } +} + +impl Drop for Fixture { + fn drop(&mut self) { + if let Some(pid) = self.pid() + && exists(pid) + { + // SAFETY: this PID belongs to the fixture executable. + unsafe { libc::kill(pid, libc::SIGKILL) }; + } + } +} + +fn issue() -> IssueUrl { + IssueUrl::parse("https://github.com/acme/widgets/issues/7").unwrap() +} + +#[test] +fn parent_only_interruption_stops_active_github() { + isolated("parent_only_interruption_stops_active_github", |signal| { + let fixture = Fixture::new(&format!( + "kill -{signal} {}\nexec sleep 3", + std::process::id() + )); + let started = Instant::now(); + assert_eq!( + GitHub::new().issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + assert!( + started.elapsed() < Duration::from_secs(2), + "GitHub waited for natural expiry" + ); + assert!( + !exists(fixture.pid().expect("the executable never started")), + "GitHub left its executable alive" + ); + }); +} + +#[test] +fn ordinary_github_refuses_reads_and_writes_before_spawn() { + isolated( + "ordinary_github_refuses_reads_and_writes_before_spawn", + |signal| { + let fixture = Fixture::new("echo 'no pull requests found' >&2; exit 1"); + let github = GitHub::new(); + assert!( + github + .pull_request_for(&issue(), "issue-7") + .unwrap() + .is_none() + ); + fs::remove_file(fixture.path("pid")).unwrap(); + signal_hook::low_level::raise(signal).unwrap(); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + assert_eq!( + github + .mark_ready(&issue(), "issue-7") + .unwrap_err() + .to_string(), + "interrupted" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "interrupted" + ); + assert!(github.pull_request_for(&issue(), "issue-7").is_err()); + assert!( + !fixture.path("pid").exists(), + "an ordinary operation spawned after interruption" + ); + }, + ); +} + +fn finish_issue_creation(name: &str, already_interrupted: bool) { + isolated(name, |signal| { + let github = GitHub::new(); + let completion = github.completion(); + let fixture = Fixture::new(""); + fixture.script(&format!( + r#" +case "$1 $2" in + 'label list') + kill -{signal} {parent} + sleep 0.1 + printf '[]' + ;; + 'label create') touch '{label}' ;; + 'issue create') printf ' https://github.com/acme/widgets/issues/8\n\n' ;; + *) exit 1 ;; +esac +"#, + parent = std::process::id(), + label = fixture.path("label-created").display() + )); + if already_interrupted { + signal_hook::low_level::raise(signal).unwrap(); + } + let created = completion + .create_issue( + &issue(), + "Finish", + "Work", + &[crate::labels::READY_FOR_AGENT], + ) + .unwrap(); + assert_eq!(created.url, "https://github.com/acme/widgets/issues/8"); + assert!(fixture.path("label-created").exists()); + assert!(interrupt::requested()); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + assert_eq!( + GitHub::new().issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + }); +} + +#[test] +fn completion_finishes_nested_label_reads_and_writes_without_permitting_ordinary_views() { + finish_issue_creation( + "completion_finishes_nested_label_reads_and_writes_without_permitting_ordinary_views", + true, + ); +} + +#[test] +fn completion_finishes_with_a_newly_arriving_interrupt() { + finish_issue_creation("completion_finishes_with_a_newly_arriving_interrupt", false); +} + +#[test] +fn an_optional_pr_probe_propagates_active_cancellation_instead_of_absence() { + isolated( + "an_optional_pr_probe_propagates_active_cancellation_instead_of_absence", + |signal| { + let fixture = Fixture::new(&format!( + "echo 'no pull requests found' >&2\nkill -{signal} {}\nexec sleep 3", + std::process::id() + )); + let error = GitHub::new() + .pull_request_for(&issue(), "issue-7") + .err() + .expect("cancellation became absence"); + assert_eq!(error.to_string(), "interrupted"); + assert!(!exists(fixture.pid().unwrap())); + }, + ); +} + +fn cancelled_merge(name: &str, state: &str, head: &str, confirmed: bool) { + isolated(name, |signal| { + let fixture = Fixture::new(&format!( + r#" +case "$1 $2" in + 'pr merge') kill -{signal} {parent}; exec sleep 3 ;; + 'pr view') printf '%s' '{{"state":"{state}","headRefOid":"{head}"}}' ;; + *) exit 1 ;; +esac +"#, + parent = std::process::id() + )); + let github = GitHub::new(); + let result = github.merge(&issue(), "issue-7", "requested-head"); + if confirmed { + result.unwrap(); + } else { + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + } + assert!(interrupt::requested()); + assert_eq!( + github + .mark_ready(&issue(), "issue-7") + .unwrap_err() + .to_string(), + "interrupted" + ); + assert!(!exists(fixture.pid().unwrap())); + }); +} + +#[test] +fn a_cancelled_merge_is_confirmed_only_at_the_requested_head() { + cancelled_merge( + "a_cancelled_merge_is_confirmed_only_at_the_requested_head", + "MERGED", + "requested-head", + true, + ); +} + +#[test] +fn a_cancelled_merge_at_another_head_remains_a_failure() { + cancelled_merge( + "a_cancelled_merge_at_another_head_remains_a_failure", + "MERGED", + "foreign-head", + false, + ); +} + +#[test] +fn an_unconfirmed_cancelled_merge_remains_a_failure() { + cancelled_merge( + "an_unconfirmed_cancelled_merge_remains_a_failure", + "OPEN", + "requested-head", + false, + ); +} + +#[test] +fn github_preserves_trimmed_text_json_and_paginated_items() { + isolated( + "github_preserves_trimmed_text_json_and_paginated_items", + |_| { + let _fixture = Fixture::new( + r#" +case "$1 $2" in + 'issue view') printf ' {"title":"Widgets"}\n' ;; + 'pr create') printf ' https://github.com/acme/widgets/pull/1\n\n' ;; + 'api --paginate') + if test "$4" = '.check_runs[]'; then + printf '%s\n' '{"name":"build","status":"completed","conclusion":"success"}' '{"name":"lint","status":"completed","conclusion":"failure"}' + else + printf '%s\n' '{"context":"external","state":"pending"}' + fi ;; + *) exit 1 ;; +esac +"#, + ); + let github = GitHub::new(); + assert_eq!(github.issue_title(&issue()).unwrap(), "Widgets"); + assert_eq!( + github + .create_draft_pr(&issue(), "issue-7", "main", "Widgets", "Work") + .unwrap(), + "https://github.com/acme/widgets/pull/1" + ); + let checks = github.checks_on(&issue(), "abc").unwrap(); + let answers: Vec<_> = checks + .iter() + .map(|check| (check.name.as_str(), check.state)) + .collect(); + assert_eq!( + answers, + [ + ("build", CheckState::Passed), + ("lint", CheckState::Failed), + ("external", CheckState::Pending) + ] + ); + }, + ); +} + +#[test] +fn github_keeps_its_exit_and_parse_diagnostics() { + isolated("github_keeps_its_exit_and_parse_diagnostics", |_| { + let fixture = Fixture::new("printf ' refused \\n' >&2; exit 1"); + let github = GitHub::new(); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "gh issue view failed: refused" + ); + assert_eq!( + github + .mark_ready(&issue(), "issue-7") + .unwrap_err() + .to_string(), + "gh pr ready issue-7 --repo acme/widgets failed: refused" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 failed: refused" + ); + fixture.script("printf 'invalid'"); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "gh issue view returned invalid JSON" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 returned invalid JSON" + ); + }); +} diff --git a/src/labels.rs b/src/labels.rs index effbaa166..6aaf91c1c 100644 --- a/src/labels.rs +++ b/src/labels.rs @@ -8,7 +8,7 @@ use std::fmt; use anyhow::Result; -use crate::github; +use crate::github::{self, GitHub}; use crate::issue::IssueUrl; /// A label thirdshift knows. It prints as its name. @@ -202,15 +202,15 @@ impl<'a> Edit<'a> { /// puts on to the repository, with its description, if the repository /// lacks it. A change of no label makes no request, and taking a single /// label off, putting none on, keeps the labels added since the read. - pub fn apply(&self) -> Result<()> { + pub fn apply(&self, github: &GitHub) -> Result<()> { match request(&self.labels, &self.off, &self.on) { Request::None => Ok(()), - Request::Delete(label) => github::remove_label(self.issue, &label), + Request::Delete(label) => github.remove_label(self.issue, &label), Request::Put { missing, labels } => { if !missing.is_empty() { - github::ensure_labels(&self.issue.repo_slug(), &missing)?; + github.ensure_labels(&self.issue.repo_slug(), &missing)?; } - github::set_labels(self.issue, &labels) + github.set_labels(self.issue, &labels) } } } diff --git a/src/launch.rs b/src/launch.rs index 9fe1b74fb..3f1276b70 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -19,7 +19,7 @@ use anyhow::{Context, Result, bail}; use crate::config; use crate::git::Git; -use crate::github; +use crate::github::GitHub; use crate::issue::{IssueUrl, Repo}; use crate::progress; @@ -61,7 +61,7 @@ impl LaunchDirectory { directory.origin ); } - if !github::issue_is_open(issue)? { + if !GitHub::new().issue_is_open(issue)? { bail!("issue #{} is closed", issue.number); } directory.check_identity()?; diff --git a/src/notification.rs b/src/notification.rs index 6a612d702..2971e4590 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -9,7 +9,7 @@ use anyhow::Result; use crate::config::EmailSettings; use crate::email::Resend; -use crate::github; +use crate::github::GitHub; use crate::harness::Choice; use crate::host; use crate::issue::{IssueUrl, Repo}; @@ -96,7 +96,7 @@ impl RunNotification { issue: issue.clone(), // Left out of the subject if GitHub can't be asked; the Run's // own preflight reports why. - title: github::issue_title(issue).ok(), + title: GitHub::new().issue_title(issue).ok(), }, // Left out of the subject if origin names none; the Architect // run's own preflight reports why. diff --git a/src/pass.rs b/src/pass.rs index aa7db3ea3..0256f9889 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -18,7 +18,7 @@ use anyhow::Result; use crate::asks::{Asks, Flags}; use crate::config::UserConfig; -use crate::github::{self, Issue, ListedIssue}; +use crate::github::{GitHub, Issue, ListedIssue}; use crate::harness::Choice; use crate::interrupt; use crate::issue::{IssueUrl, Repo}; @@ -114,19 +114,19 @@ pub struct LaunchAndGitHub<'a> { impl Outside for LaunchAndGitHub<'_> { fn open_issues(&mut self, label: Label) -> Result> { - github::open_issues_labelled(&self.repo.slug(), label) + GitHub::new().open_issues_labelled(&self.repo.slug(), label) } fn closed_issues(&mut self, label: Label) -> Result> { - github::closed_issues_labelled(&self.repo.slug(), label) + GitHub::new().closed_issues_labelled(&self.repo.slug(), label) } fn issue(&mut self, issue: &IssueUrl) -> Result { - github::issue(issue) + GitHub::new().issue(issue) } fn apply(&mut self, edit: &Edit) -> Result<()> { - edit.apply() + edit.apply(&GitHub::new()) } fn interrupted(&mut self) -> bool { diff --git a/src/ready.rs b/src/ready.rs index 812606bc6..fbfe51647 100644 --- a/src/ready.rs +++ b/src/ready.rs @@ -10,7 +10,7 @@ use crate::base_fix; use crate::branch::{self, Started}; use crate::claim; use crate::git::Git; -use crate::github::{self, Candidate, ListedIssue, Shaping}; +use crate::github::{Candidate, GitHub, ListedIssue, Shaping}; use crate::issue::{IssueUrl, Repo}; use crate::labels::{Label, READY_FOR_AGENT}; use crate::progress; @@ -29,8 +29,13 @@ pub struct ReadyIssue { /// issue labelled `ready-for-agent` passed over before it, with why. /// Nothing is changed, on GitHub or in `launch`. pub fn first(launch: &Git, repo: &Repo) -> Result> { - let candidates = github::open_issues_labelled(&repo.slug(), READY_FOR_AGENT)?; - let reads = GitHubAndOrigin { launch }; + let reads = GitHubAndOrigin { + launch, + github: GitHub::new(), + }; + let candidates = reads + .github + .open_issues_labelled(&repo.slug(), READY_FOR_AGENT)?; Search::new(&reads, candidates, Utc::now(), progress::step).first_ready() } @@ -47,15 +52,16 @@ trait Reads { /// The reads of GitHub and of the Launch directory `launch`'s origin. struct GitHubAndOrigin<'a> { launch: &'a Git, + github: GitHub, } impl Reads for GitHubAndOrigin<'_> { fn candidate(&self, issue: &IssueUrl) -> Result { - github::candidate(issue, READY_FOR_AGENT) + self.github.candidate(issue, READY_FOR_AGENT) } fn started(&self, issue: &IssueUrl) -> Result> { - branch::started(self.launch, issue) + branch::started(self.launch, &self.github, issue) } } diff --git a/src/run.rs b/src/run.rs index fab341e20..140d6416a 100644 --- a/src/run.rs +++ b/src/run.rs @@ -13,7 +13,7 @@ use crate::child_run::Kind; use crate::claim::{self, Claim}; use crate::delivery::{Delivery, Opening}; use crate::failed_run::FailedRun; -use crate::github::{self, Ticket}; +use crate::github::{GitHub, Ticket}; use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; @@ -385,7 +385,7 @@ impl<'a> Outside for LaunchAndGitHub<'a> { type Worktree = Worktree; fn tickets(&mut self) -> Result> { - github::tickets(self.issue) + GitHub::new().tickets(self.issue) } fn started(&mut self, work: Work) { @@ -393,7 +393,7 @@ impl<'a> Outside for LaunchAndGitHub<'a> { } fn select(&mut self) -> Result { - branch::select(self.directory.git(), self.issue) + branch::select(self.directory.git(), &GitHub::new(), self.issue) } fn base_branch(&mut self, named: Option<&str>) -> Result { diff --git a/src/setup.rs b/src/setup.rs index cf7f39e0d..10b420490 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -19,12 +19,13 @@ use anyhow::{Context, Result}; use crate::config::{self, EmailSettings, UserConfig, UserConfigDocument}; use crate::git::Git; +use crate::github::GitHub; use crate::harness::{ Harness, ModelAndEffort, Settings, settings::{self, Terminal}, }; use crate::resend_key::{Credentials, Source}; -use crate::{email, github, interrupt, progress}; +use crate::{email, interrupt, progress}; use questions::Answers; @@ -326,7 +327,7 @@ impl Outside for OnMachine<'_> { } fn github_email(&mut self) -> Result> { - github::profile_email() + GitHub::new().profile_email() } fn git_email(&mut self) -> Result { diff --git a/src/spec_run.rs b/src/spec_run.rs index cee2283f4..5b3abb77f 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -23,7 +23,7 @@ use crate::base_fix::BaseFixAsk; use crate::child_run::{self, Ended, Handle, Kind}; use crate::delivery::{Delivery, Opening}; use crate::failed_run::{FailedRun, interrupted_or}; -use crate::github::{self, Ticket}; +use crate::github::{GitHub, Ticket}; use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; @@ -133,6 +133,13 @@ fn review_and_deliver( checklist: &str, spec_pr_url: &str, ) -> Result { + if outside.interrupted() { + return Err(FailedRun { + interrupted: true, + pr_url: Some(spec_pr_url.to_string()), + ..FailedRun::from(anyhow::anyhow!("interrupted")) + }); + } let opening = Opening { kind: SPEC_REVIEW, prompt: prompt::spec_review(spec, base, branch, spec_pr_url), @@ -270,7 +277,7 @@ impl Outside for ChildRunsAndGitHub<'_> { } fn tickets(&mut self) -> Result> { - github::tickets(self.spec) + GitHub::new().completion().tickets(self.spec) } fn show(&mut self, checklist: &str) { diff --git a/src/spec_run/spec_pr.rs b/src/spec_run/spec_pr.rs index 541ee5987..9621c0afa 100644 --- a/src/spec_run/spec_pr.rs +++ b/src/spec_run/spec_pr.rs @@ -10,7 +10,7 @@ use anyhow::{Context, Result}; -use crate::github::{self, PullRequest}; +use crate::github::{GitHub, PullRequest}; use crate::issue::IssueUrl; use crate::progress; @@ -39,8 +39,13 @@ impl<'a> SpecPr<'a> { spec, branch: branch.to_string(), base, + github: GitHub::new(), }; - Self::resume_through(on_github, spec, branch, base) + let mut resumed = Self::resume_through(on_github, spec, branch, base)?; + // Resume is ordinary; subsequent writes account for child endings, + // even when interruption prevents more implementation work. + resumed.outside.github = resumed.outside.github.completion(); + Ok(resumed) } } @@ -168,6 +173,7 @@ pub(super) trait Outside { /// The Spec PR of `spec`, from `branch` into `base`, on GitHub, and progress /// lines printed on stderr. pub(super) struct OnGitHub<'a> { + github: GitHub, spec: &'a IssueUrl, branch: String, base: &'a str, @@ -175,27 +181,29 @@ pub(super) struct OnGitHub<'a> { impl Outside for OnGitHub<'_> { fn pull_request(&mut self) -> Result> { - github::pull_request_for(self.spec, &self.branch) + self.github.pull_request_for(self.spec, &self.branch) } fn convert_to_draft(&mut self) -> Result<()> { - github::convert_to_draft(self.spec, &self.branch) + self.github.convert_to_draft(self.spec, &self.branch) } fn spec_title(&mut self) -> Result { - github::issue_title(self.spec) + self.github.issue_title(self.spec) } fn create_draft(&mut self, title: &str, body: &str) -> Result<()> { - github::create_draft_pr(self.spec, &self.branch, self.base, title, body).map(|_| ()) + GitHub::new() + .create_draft_pr(self.spec, &self.branch, self.base, title, body) + .map(|_| ()) } fn pr_body(&mut self, number: u64) -> Result { - github::pr_body(self.spec, number) + self.github.pr_body(self.spec, number) } fn set_pr_body(&mut self, number: u64, body: &str) -> Result<()> { - github::set_pr_body(self.spec, number, body) + self.github.set_pr_body(self.spec, number, body) } fn step(&mut self, line: String) { diff --git a/tests/spec_run.rs b/tests/spec_run.rs index d76d2deb2..041e17757 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1574,6 +1574,67 @@ fn a_signal_to_the_spec_run_alone_fails_the_ticket_run_then_ends_the_spec_run_as ); } +#[test] +fn an_existing_spec_pr_accounts_for_every_drained_ticket_after_interruption() { + let scenario = diamond_spec(); + scenario.origin_has_branch("issue-20", "main", &[]); + let pr_url = scenario.github_has_pr("issue-20", "main", "OPEN"); + for (ticket, other) in [(21, 22), (22, 21)] { + scenario.agent_does_for( + ticket, + &format!( + "echo 'half done {ticket}' > wip.txt\n{}touch {root}/both-started\nsleep 30\n", + waits_for_other_session(&scenario, ticket, other), + root = scenario.path("").display() + ), + ); + } + + let result = scenario.run_and_signal( + &[&spec_url(&scenario), "parallel", "2"], + "both-started", + "TERM", + ); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.stdout, format!("{pr_url}\n")); + let mut sessions = sessions_by_issue(&scenario); + sessions.sort(); + assert_eq!( + sessions, + ["21", "22"], + "no new Ticket or Spec review may start" + ); + let pr = spec_pr(&scenario); + assert_eq!(pr["url"], pr_url); + assert_eq!(pr["isDraft"], true); + assert_eq!( + checklist_in(pr["body"].as_str().unwrap()), + "\n## Tickets\n\n- [ ] #21 interrupted\n- [ ] #22 interrupted\n- [ ] #23 blocked by #21, #22\n" + ); + for ticket in [21, 22] { + assert_contains( + &result.stderr, + &format!("thirdshift: #{ticket} interrupted\n"), + ); + assert_eq!( + scenario.origin_log(&format!("issue-{ticket}")).unwrap()[0], + "thirdshift: failed run (interrupted)" + ); + scenario.assert_cleaned_up(&format!("issue-{ticket}")); + } + assert_eq!( + support::before_command_log(&result.stderr).last(), + Some(&"thirdshift: interrupted") + ); + assert!( + !result.stderr.contains("could not update the Spec PR"), + "{}", + result.stderr + ); + scenario.assert_cleaned_up("issue-20"); +} + #[test] fn parent_only_interruption_reaches_all_active_tickets_before_waiting_and_saves_their_work() { let scenario = diamond_spec(); From d12588b9ca8fbb8c4e9024b1555c3008a97bc925 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 21:02:57 -0400 Subject: [PATCH 6/7] Keep Spec PR opening on the completion view after review (#468) --- src/spec_run/spec_pr.rs | 2 +- tests/spec_run.rs | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/spec_run/spec_pr.rs b/src/spec_run/spec_pr.rs index 9621c0afa..10a9abcea 100644 --- a/src/spec_run/spec_pr.rs +++ b/src/spec_run/spec_pr.rs @@ -193,7 +193,7 @@ impl Outside for OnGitHub<'_> { } fn create_draft(&mut self, title: &str, body: &str) -> Result<()> { - GitHub::new() + self.github .create_draft_pr(self.spec, &self.branch, self.base, title, body) .map(|_| ()) } diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 041e17757..83eff4f3e 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1574,6 +1574,42 @@ fn a_signal_to_the_spec_run_alone_fails_the_ticket_run_then_ends_the_spec_run_as ); } +#[test] +fn the_first_landing_opens_its_spec_pr_when_accounting_is_interrupted() { + let scenario = linear_spec(); + scenario.agent_does_for( + 21, + &format!( + "{}gh fake on-issue-view 20 'touch {}; sleep 1'\n", + agent_lands(21, "first.txt"), + scenario.path("reading-spec-title").display() + ), + ); + + // The Ticket has landed; interruption arrives while accounting reads + // the title, before opening the Spec PR. No subsequent work may start. + let result = scenario.run_and_signal(&[&spec_url(&scenario)], "reading-spec-title", "TERM"); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(sessions_by_issue(&scenario), ["21"]); + let pr = spec_pr(&scenario); + let pr_url = pr["url"].as_str().unwrap(); + assert_eq!(result.stdout, format!("{pr_url}\n")); + assert_eq!(pr["isDraft"], true); + let checklist = checklist_in(pr["body"].as_str().unwrap()); + assert_contains( + checklist, + "- [x] #21 landed with https://github.com/acme/widgets/pull/1\n", + ); + assert_contains(checklist, "- [ ] #22 not started\n"); + assert_contains(&result.stderr, "thirdshift: #21 landed\n"); + assert_eq!( + support::before_command_log(&result.stderr).last(), + Some(&"thirdshift: interrupted") + ); + scenario.assert_cleaned_up("issue-20"); +} + #[test] fn an_existing_spec_pr_accounts_for_every_drained_ticket_after_interruption() { let scenario = diamond_spec(); From d6253458ba68106be0504478ebf5d91593fc31b2 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 21:23:28 -0400 Subject: [PATCH 7/7] Share signal-test isolation after Spec review (#465) --- src/git/execution_tests.rs | 72 ++++++---------- src/github/execution_tests.rs | 156 +++++++++++++++------------------- src/main.rs | 2 + src/process/tests.rs | 18 +--- src/test_support.rs | 30 +++++++ 5 files changed, 128 insertions(+), 150 deletions(-) create mode 100644 src/test_support.rs diff --git a/src/git/execution_tests.rs b/src/git/execution_tests.rs index 9af98c571..af491966b 100644 --- a/src/git/execution_tests.rs +++ b/src/git/execution_tests.rs @@ -3,39 +3,14 @@ use super::*; use crate::interrupt; +use crate::test_support::with_recorded_signal; use std::fs; use std::os::unix::fs::PermissionsExt; -fn isolated(name: &str, test: impl FnOnce(libc::c_int)) { - if std::env::var("THIRDSHIFT_TEST_GIT").as_deref() == Ok(name) { - interrupt::install().unwrap(); - test( - std::env::var("THIRDSHIFT_TEST_GIT_SIGNAL") - .unwrap() - .parse() - .unwrap(), - ); - return; - } - for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { - let output = Command::new(std::env::current_exe().unwrap()) - .args([ - "--exact", - &format!("git::execution_tests::{name}"), - "--nocapture", - ]) - .env("THIRDSHIFT_TEST_GIT", name) - .env("THIRDSHIFT_TEST_GIT_SIGNAL", signal.to_string()) - .output() - .unwrap(); - assert!(output.status.success(), "signal {signal}: {output:?}"); - } -} - #[test] fn ordinary_git_refuses_to_spawn_after_interruption() { - isolated( - "ordinary_git_refuses_to_spawn_after_interruption", + with_recorded_signal( + "git::execution_tests::ordinary_git_refuses_to_spawn_after_interruption", |signal| { let (_temp, git) = super::tests::repo_with_origin(); signal_hook::low_level::raise(signal).unwrap(); @@ -50,8 +25,8 @@ fn ordinary_git_refuses_to_spawn_after_interruption() { #[test] fn completion_git_finishes_without_permitting_the_original_view() { - isolated( - "completion_git_finishes_without_permitting_the_original_view", + with_recorded_signal( + "git::execution_tests::completion_git_finishes_without_permitting_the_original_view", |signal| { let (_temp, git) = super::tests::repo_with_origin(); let completion = git.completion(); @@ -84,8 +59,8 @@ fn completion_git_finishes_without_permitting_the_original_view() { #[test] fn optional_answers_distinguish_absence_from_execution_failure() { - isolated( - "optional_answers_distinguish_absence_from_execution_failure", + with_recorded_signal( + "git::execution_tests::optional_answers_distinguish_absence_from_execution_failure", |signal| { let (_temp, git) = super::tests::repo_with_origin(); assert_eq!( @@ -168,8 +143,8 @@ const FIXTURE: &[&str] = &["-c", "alias.fixture=!./fixture.sh", "fixture"]; #[test] fn a_parent_only_signal_stops_active_git_and_propagates_through_probes() { - isolated( - "a_parent_only_signal_stops_active_git_and_propagates_through_probes", + with_recorded_signal( + "git::execution_tests::a_parent_only_signal_stops_active_git_and_propagates_through_probes", |signal| { let (_temp, git) = super::tests::repo_with_origin(); let fixture = Fixture::new( @@ -202,8 +177,8 @@ fn a_parent_only_signal_stops_active_git_and_propagates_through_probes() { #[test] fn completion_git_finishes_with_a_newly_arriving_signal() { - isolated( - "completion_git_finishes_with_a_newly_arriving_signal", + with_recorded_signal( + "git::execution_tests::completion_git_finishes_with_a_newly_arriving_signal", |signal| { let (_temp, git) = super::tests::repo_with_origin(); let _fixture = Fixture::new( @@ -222,8 +197,8 @@ fn completion_git_finishes_with_a_newly_arriving_signal() { #[test] fn an_optional_probe_propagates_active_cancellation() { - isolated( - "an_optional_probe_propagates_active_cancellation", + with_recorded_signal( + "git::execution_tests::an_optional_probe_propagates_active_cancellation", |signal| { let (_temp, git) = super::tests::repo_with_origin(); let _fixture = Fixture::new( @@ -265,19 +240,22 @@ fn interrupt_lock_retry(git: &Git, signal: libc::c_int) -> Result { #[test] fn ordinary_lock_retries_observe_interruption() { - isolated("ordinary_lock_retries_observe_interruption", |signal| { - let (_temp, git) = super::tests::repo_with_origin(); - assert_eq!( - interrupt_lock_retry(&git, signal).unwrap_err().to_string(), - "interrupted" - ); - }); + with_recorded_signal( + "git::execution_tests::ordinary_lock_retries_observe_interruption", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + assert_eq!( + interrupt_lock_retry(&git, signal).unwrap_err().to_string(), + "interrupted" + ); + }, + ); } #[test] fn completion_lock_retries_ignore_existing_and_arriving_interruption() { - isolated( - "completion_lock_retries_ignore_existing_and_arriving_interruption", + with_recorded_signal( + "git::execution_tests::completion_lock_retries_ignore_existing_and_arriving_interruption", |signal| { let (_temp, git) = super::tests::repo_with_origin(); let completion = git.completion(); diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index 79559e4a2..7461a7fc8 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -1,36 +1,11 @@ use super::*; use crate::interrupt; +use crate::test_support::with_recorded_signal; use std::fs; use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; use std::time::{Duration, Instant}; -fn isolated(name: &str, test: impl FnOnce(libc::c_int)) { - if std::env::var("THIRDSHIFT_TEST_GITHUB").as_deref() == Ok(name) { - interrupt::install().unwrap(); - test( - std::env::var("THIRDSHIFT_TEST_GITHUB_SIGNAL") - .unwrap() - .parse() - .unwrap(), - ); - return; - } - for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { - let output = Command::new(std::env::current_exe().unwrap()) - .args([ - "--exact", - &format!("github::execution_tests::{name}"), - "--nocapture", - ]) - .env("THIRDSHIFT_TEST_GITHUB", name) - .env("THIRDSHIFT_TEST_GITHUB_SIGNAL", signal.to_string()) - .output() - .unwrap(); - assert!(output.status.success(), "signal {signal}: {output:?}"); - } -} - /// PATH changes happen only in the isolated process. Cleanup uses the /// executable's recorded PID, including after a failed assertion. struct Fixture { @@ -102,31 +77,34 @@ fn issue() -> IssueUrl { #[test] fn parent_only_interruption_stops_active_github() { - isolated("parent_only_interruption_stops_active_github", |signal| { - let fixture = Fixture::new(&format!( - "kill -{signal} {}\nexec sleep 3", - std::process::id() - )); - let started = Instant::now(); - assert_eq!( - GitHub::new().issue_title(&issue()).unwrap_err().to_string(), - "interrupted" - ); - assert!( - started.elapsed() < Duration::from_secs(2), - "GitHub waited for natural expiry" - ); - assert!( - !exists(fixture.pid().expect("the executable never started")), - "GitHub left its executable alive" - ); - }); + with_recorded_signal( + "github::execution_tests::parent_only_interruption_stops_active_github", + |signal| { + let fixture = Fixture::new(&format!( + "kill -{signal} {}\nexec sleep 3", + std::process::id() + )); + let started = Instant::now(); + assert_eq!( + GitHub::new().issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + assert!( + started.elapsed() < Duration::from_secs(2), + "GitHub waited for natural expiry" + ); + assert!( + !exists(fixture.pid().expect("the executable never started")), + "GitHub left its executable alive" + ); + }, + ); } #[test] fn ordinary_github_refuses_reads_and_writes_before_spawn() { - isolated( - "ordinary_github_refuses_reads_and_writes_before_spawn", + with_recorded_signal( + "github::execution_tests::ordinary_github_refuses_reads_and_writes_before_spawn", |signal| { let fixture = Fixture::new("echo 'no pull requests found' >&2; exit 1"); let github = GitHub::new(); @@ -163,7 +141,7 @@ fn ordinary_github_refuses_reads_and_writes_before_spawn() { } fn finish_issue_creation(name: &str, already_interrupted: bool) { - isolated(name, |signal| { + with_recorded_signal(name, |signal| { let github = GitHub::new(); let completion = github.completion(); let fixture = Fixture::new(""); @@ -211,20 +189,23 @@ esac #[test] fn completion_finishes_nested_label_reads_and_writes_without_permitting_ordinary_views() { finish_issue_creation( - "completion_finishes_nested_label_reads_and_writes_without_permitting_ordinary_views", + "github::execution_tests::completion_finishes_nested_label_reads_and_writes_without_permitting_ordinary_views", true, ); } #[test] fn completion_finishes_with_a_newly_arriving_interrupt() { - finish_issue_creation("completion_finishes_with_a_newly_arriving_interrupt", false); + finish_issue_creation( + "github::execution_tests::completion_finishes_with_a_newly_arriving_interrupt", + false, + ); } #[test] fn an_optional_pr_probe_propagates_active_cancellation_instead_of_absence() { - isolated( - "an_optional_pr_probe_propagates_active_cancellation_instead_of_absence", + with_recorded_signal( + "github::execution_tests::an_optional_pr_probe_propagates_active_cancellation_instead_of_absence", |signal| { let fixture = Fixture::new(&format!( "echo 'no pull requests found' >&2\nkill -{signal} {}\nexec sleep 3", @@ -241,7 +222,7 @@ fn an_optional_pr_probe_propagates_active_cancellation_instead_of_absence() { } fn cancelled_merge(name: &str, state: &str, head: &str, confirmed: bool) { - isolated(name, |signal| { + with_recorded_signal(name, |signal| { let fixture = Fixture::new(&format!( r#" case "$1 $2" in @@ -274,7 +255,7 @@ esac #[test] fn a_cancelled_merge_is_confirmed_only_at_the_requested_head() { cancelled_merge( - "a_cancelled_merge_is_confirmed_only_at_the_requested_head", + "github::execution_tests::a_cancelled_merge_is_confirmed_only_at_the_requested_head", "MERGED", "requested-head", true, @@ -284,7 +265,7 @@ fn a_cancelled_merge_is_confirmed_only_at_the_requested_head() { #[test] fn a_cancelled_merge_at_another_head_remains_a_failure() { cancelled_merge( - "a_cancelled_merge_at_another_head_remains_a_failure", + "github::execution_tests::a_cancelled_merge_at_another_head_remains_a_failure", "MERGED", "foreign-head", false, @@ -294,7 +275,7 @@ fn a_cancelled_merge_at_another_head_remains_a_failure() { #[test] fn an_unconfirmed_cancelled_merge_remains_a_failure() { cancelled_merge( - "an_unconfirmed_cancelled_merge_remains_a_failure", + "github::execution_tests::an_unconfirmed_cancelled_merge_remains_a_failure", "OPEN", "requested-head", false, @@ -303,8 +284,8 @@ fn an_unconfirmed_cancelled_merge_remains_a_failure() { #[test] fn github_preserves_trimmed_text_json_and_paginated_items() { - isolated( - "github_preserves_trimmed_text_json_and_paginated_items", + with_recorded_signal( + "github::execution_tests::github_preserves_trimmed_text_json_and_paginated_items", |_| { let _fixture = Fixture::new( r#" @@ -348,32 +329,35 @@ esac #[test] fn github_keeps_its_exit_and_parse_diagnostics() { - isolated("github_keeps_its_exit_and_parse_diagnostics", |_| { - let fixture = Fixture::new("printf ' refused \\n' >&2; exit 1"); - let github = GitHub::new(); - assert_eq!( - github.issue_title(&issue()).unwrap_err().to_string(), - "gh issue view failed: refused" - ); - assert_eq!( - github - .mark_ready(&issue(), "issue-7") - .unwrap_err() - .to_string(), - "gh pr ready issue-7 --repo acme/widgets failed: refused" - ); - assert_eq!( - github.checks_on(&issue(), "abc").err().unwrap().to_string(), - "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 failed: refused" - ); - fixture.script("printf 'invalid'"); - assert_eq!( - github.issue_title(&issue()).unwrap_err().to_string(), - "gh issue view returned invalid JSON" - ); - assert_eq!( - github.checks_on(&issue(), "abc").err().unwrap().to_string(), - "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 returned invalid JSON" - ); - }); + with_recorded_signal( + "github::execution_tests::github_keeps_its_exit_and_parse_diagnostics", + |_| { + let fixture = Fixture::new("printf ' refused \\n' >&2; exit 1"); + let github = GitHub::new(); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "gh issue view failed: refused" + ); + assert_eq!( + github + .mark_ready(&issue(), "issue-7") + .unwrap_err() + .to_string(), + "gh pr ready issue-7 --repo acme/widgets failed: refused" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 failed: refused" + ); + fixture.script("printf 'invalid'"); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "gh issue view returned invalid JSON" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 returned invalid JSON" + ); + }, + ); } diff --git a/src/main.rs b/src/main.rs index cb524ae4a..7257b725d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -37,6 +37,8 @@ mod session; mod setup; mod skills; mod spec_run; +#[cfg(test)] +mod test_support; mod update; mod worktree; diff --git a/src/process/tests.rs b/src/process/tests.rs index 52b58e3b6..e5e419ff9 100644 --- a/src/process/tests.rs +++ b/src/process/tests.rs @@ -1,6 +1,7 @@ //! Behavior through the same two operations used by sessions and checks. use super::*; +use crate::test_support::with_recorded_signal; use std::fs; use std::path::Path; @@ -654,23 +655,6 @@ fn startup_failure_returns_no_state_even_when_the_consumer_has_returned() { owned.assert_stopped(); } -fn with_recorded_signal(test_name: &str, exercise: impl FnOnce(libc::c_int)) { - const SIGNAL: &str = "THIRDSHIFT_EXECUTION_ACTIVE_SIGNAL"; - if let Ok(signal) = std::env::var(SIGNAL) { - interrupt::install().unwrap(); - exercise(signal.parse().unwrap()); - return; - } - for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { - let result = Command::new(std::env::current_exe().unwrap()) - .args(["--exact", test_name, "--nocapture"]) - .env(SIGNAL, signal.to_string()) - .output() - .unwrap(); - assert!(result.status.success(), "{result:?}"); - } -} - #[test] fn active_ordinary_cancellation_does_not_cancel_concurrent_completion() { with_recorded_signal( diff --git a/src/test_support.rs b/src/test_support.rs new file mode 100644 index 000000000..edbd0a6e5 --- /dev/null +++ b/src/test_support.rs @@ -0,0 +1,30 @@ +//! Run signal-sensitive tests in isolated processes so interruption and +//! fixture environment changes cannot leak into another test. + +use std::process::Command; + +use crate::interrupt; + +/// Exercise the fully qualified test once per recorded signal, installing +/// the handler only in the subprocess selected for that test. +pub(crate) fn with_recorded_signal(test_name: &str, exercise: impl FnOnce(libc::c_int)) { + const TEST: &str = "THIRDSHIFT_TEST_SIGNAL_NAME"; + const SIGNAL: &str = "THIRDSHIFT_TEST_SIGNAL"; + if std::env::var(TEST).as_deref() == Ok(test_name) { + interrupt::install().unwrap(); + exercise(std::env::var(SIGNAL).unwrap().parse().unwrap()); + return; + } + for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { + let result = Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test_name, "--nocapture"]) + .env(TEST, test_name) + .env(SIGNAL, signal.to_string()) + .output() + .unwrap(); + assert!( + result.status.success(), + "{test_name}, signal {signal}: {result:?}" + ); + } +}