From 0dc42af4e4db1c0c8b30ddfd434c67ecd68b7040 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 12:32:29 -0400 Subject: [PATCH 1/2] Stop every Harness session process tree on interruption (#435) --- README.md | 2 + src/harness.rs | 1 + src/harness/adapter.rs | 7 +++ src/harness/process_tree.rs | 110 ++++++++++++++++++++++++++++++++++ src/session.rs | 32 ++-------- tests/fakes.rs | 20 +++++++ tests/process_tree.rs | 116 ++++++++++++++++++++++++++++++++++++ tests/support/fakes.rs | 2 +- 8 files changed, 261 insertions(+), 29 deletions(-) create mode 100644 src/harness/process_tree.rs create mode 100644 tests/process_tree.rs diff --git a/README.md b/README.md index a5ffc0e51..fdc837fc9 100644 --- a/README.md +++ b/README.md @@ -299,6 +299,8 @@ For each of the three, the command wins, then the [User config](#user-config), t With `harness codex`, every session runs `codex exec --json --dangerously-bypass-approvals-and-sandbox` in the Run's worktree, with stdin set to null, and with `-c project_doc_fallback_filenames=["CLAUDE.md"]`, so Codex reads `CLAUDE.md` where a repository has no `AGENTS.md`. Codex's sandbox is off: on a stock Ubuntu machine it can't start, and commits and pushes from a worktree write outside it, so the worktree is the only boundary, as with Claude's auto mode ([ADR-0012](docs/adr/0012-factory-skills-linked-into-the-worktree-codex-unsandboxed.md)). The Factory skills are linked into the worktree's `.agents/skills/`, kept out of git with `/.agents/skills/thirdshift-*` in `.git/info/exclude`, and each prompt's first line loads its skill as `$thirdshift-`. `codex exec` kills what the agent left running as it exits, so a session whose turn ends with a command or a sub-agent call still running (started, never completed) gets a Resume, as on Claude: `codex exec … resume `, with the Model, Effort, bypass flag and `CLAUDE.md` fallback given again, as Codex keeps none of them. An interrupt sends a Codex session SIGINT, the only signal Codex stops cleanly on, before the SIGTERM and SIGKILL every session gets, each 10 seconds apart. Progress lines come from Codex's `item.*` events, the session-ended line gives its token totals, as Codex reports no turns or cost, and the final message is the last `agent_message`. A session whose turn fails (`turn.failed`), or that exits non-zero, fails the Run with Codex's error in the cause; an `error` event Codex retries does not. Each Codex session adds a `trust_level = "trusted"` entry for the repository to `~/.codex/config.toml`, and your `~/.claude/` instructions and hooks don't reach it. +Interrupting a Command stops every session's process tree on every Harness, including commands that started their own process group or session. thirdshift snapshots the descendants with `ps` on Linux and macOS, sends each process group the Harness's stop signals with a 10-second grace period each, then takes another snapshot before SIGKILL to catch commands started in the meantime. If the CLI exits on an earlier signal, thirdshift kills the remaining commands immediately without another grace period. + Before any work, where an Origin match failure would stop it, and so before the Claim, the worktree and any Command log, a command checks what it chose: the Harness's CLI must be on `PATH`, else it fails naming the CLI and the setting that chose it; and on Claude a named Model gets a minimal test call, `claude -p --model ` with the Effort, if any, which must succeed, else the command fails with what Claude said. No test call is made when no Model is named. On Codex a named Model and Effort are checked against `codex debug models`, which costs no tokens: each matches regardless of case, a Model by its slug or its display name, and becomes the name Codex takes, so `model GPT-6.1-Sol effort Max` reaches Codex as `gpt-6.1-sol` and `max`. An unknown Model, or an Effort that Model doesn't support (with no Model, one no Model supports), fails naming the valid choices. What the command records is what the check settled on. A Pickup run or an Architect run makes these checks only once it has decided not to skip, and a Spec run makes them once for all its Tickets. The Command log's opening lines (`sessions run on claude · claude-opus-5-5 · high`), the Activity log's start line, the pull request's body and the Run notification each name the Harness, Model and Effort, `default model` or `default effort` where the Harness's own is used. In the pull request's body, the Spec PR's included, thirdshift writes the line itself once the opening session has ended, such as `Built with claude · claude-opus-5-5 · high`, replacing any it wrote there before. diff --git a/src/harness.rs b/src/harness.rs index fc047bb8d..d20d5074c 100644 --- a/src/harness.rs +++ b/src/harness.rs @@ -12,6 +12,7 @@ use anyhow::{Result, bail}; mod adapter; pub(crate) mod claude; pub(crate) mod codex; +mod process_tree; pub use adapter::{Adapter, Invocation}; pub use codex::Catalog; diff --git a/src/harness/adapter.rs b/src/harness/adapter.rs index 93ba94b36..10a03c07c 100644 --- a/src/harness/adapter.rs +++ b/src/harness/adapter.rs @@ -1,6 +1,7 @@ //! Everything that varies between Harnesses, behind one interface. use std::path::Path; +use std::process::Child; use anyhow::Result; @@ -30,6 +31,12 @@ pub trait Adapter: Sync { ) -> Result>; fn stream(&self, worktree: &Path) -> Box; + /// 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()); + } + /// Fixed overrides, also used by the adapter's Model and Effort check. fn environment(&self) -> &'static [(&'static str, &'static str)] { &[] diff --git a/src/harness/process_tree.rs b/src/harness/process_tree.rs new file mode 100644 index 000000000..5039e5131 --- /dev/null +++ b/src/harness/process_tree.rs @@ -0,0 +1,110 @@ +//! The shared stop path for every Harness, including escaped process groups. + +use std::collections::{BTreeSet, HashMap}; +use std::process::{Child, Command}; +use std::thread; +use std::time::{Duration, Instant}; + +use anyhow::{Context, Result, bail}; + +use crate::progress; + +const POLL: Duration = Duration::from_millis(100); +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]) { + let pid = child.id() as libc::pid_t; + let mut tree = ProcessTree { + pids: BTreeSet::from([pid]), + groups: BTreeSet::from([pid]), + }; + tree.refresh(); + 'signals: for &signal in signals { + tree.signal(signal); + let deadline = Instant::now() + STOP_GRACE; + while Instant::now() < deadline { + if let Ok(Some(_)) = child.try_wait() { + break 'signals; + } + thread::sleep(POLL); + } + } + // Retain the first snapshot: the CLI may already have exited, leaving + // its descendants reparented. Known descendants also seed this lookup + // so their new children are found even when the CLI is gone. + tree.refresh(); + tree.signal(libc::SIGKILL); + let _ = child.wait(); +} + +struct ProcessTree { + pids: BTreeSet, + groups: BTreeSet, +} + +impl ProcessTree { + fn refresh(&mut self) { + if let Err(error) = self.snapshot() { + progress::step(format_args!( + "could not snapshot the session's process tree: {error:#}; \ + stopping its known process groups" + )); + } + } + + fn snapshot(&mut self) -> Result<()> { + // Separate -o flags and empty headers work on Linux and macOS. + let output = Command::new("ps") + .args(["-A", "-o", "pid=", "-o", "ppid=", "-o", "pgid="]) + .output() + .context("could not run ps")?; + if !output.status.success() { + bail!( + "ps exited {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ); + } + let mut children = HashMap::<_, Vec<_>>::new(); + let mut groups = HashMap::new(); + for line in String::from_utf8_lossy(&output.stdout).lines() { + let mut fields = line.split_whitespace(); + let mut next = || -> Result { + fields + .next() + .context("missing ps field")? + .parse() + .context("invalid ps field") + }; + let (pid, parent, group) = (next()?, next()?, next()?); + children.entry(parent).or_default().push(pid); + groups.insert(pid, group); + } + let mut pending: Vec<_> = self.pids.iter().copied().collect(); + let mut seen = BTreeSet::new(); + while let Some(pid) = pending.pop() { + if !seen.insert(pid) { + continue; + } + self.pids.insert(pid); + if let Some(&group) = groups.get(&pid) + && group > 0 + { + self.groups.insert(group); + } + if let Some(children) = children.get(&pid) { + pending.extend(children); + } + } + Ok(()) + } + + fn signal(&self, signal: libc::c_int) { + for &group in &self.groups { + // SAFETY: kill has no memory-safety preconditions; groups are positive. + unsafe { libc::kill(-group, signal) }; + } + } +} diff --git a/src/session.rs b/src/session.rs index 779fa1a28..174af6d2f 100644 --- a/src/session.rs +++ b/src/session.rs @@ -5,7 +5,7 @@ use std::fs::{self, File}; use std::io::{BufRead, BufReader, Read, Write}; use std::os::unix::process::CommandExt; use std::path::{Path, PathBuf}; -use std::process::{Child, Command, Stdio}; +use std::process::{Command, Stdio}; use std::thread; use std::time::{Duration, Instant}; @@ -272,7 +272,7 @@ fn ending_with(killed: &[&str]) -> String { /// `log` and condensing it through `stream` to progress lines on stderr, /// each labelled `kind`. Returns what the stream showed once the CLI has /// exited cleanly, its turn not failed. Otherwise fails with the error the -/// stream gave, if any. An interrupt stops the session, as [`stop`] does, +/// stream gave, if any. An interrupt stops the session, as [`Adapter::stop`] does, /// and fails with `interrupted`. fn run( kind: &str, @@ -332,13 +332,13 @@ fn run( && let Some(mut stdin) = child.stdin.take() && let Err(error) = stdin.write_all(input.as_bytes()) { - stop(&mut child, adapter); + adapter.stop(&mut child); let _ = follower.join(); return Err(error).with_context(|| format!("could not write the prompt to {cli}")); } let status = loop { if interrupt::requested() { - stop(&mut child, adapter); + adapter.stop(&mut child); bail!("interrupted"); } if let Some(status) = child @@ -383,30 +383,6 @@ fn run( const POLL: Duration = Duration::from_millis(100); -/// How long a session gets to exit after each signal that asks it to stop. -const STOP_GRACE: Duration = Duration::from_secs(10); - -/// Stop `child`, a session on `adapter`, by its process group: with the -/// signals that ask it to stop, in turn, each given `STOP_GRACE`, -/// then SIGKILL. -fn stop(child: &mut Child, adapter: &dyn Adapter) { - let group = -(child.id() as libc::pid_t); - for &signal in adapter.stop_signals() { - // SAFETY: kill has no memory-safety preconditions. - unsafe { libc::kill(group, signal) }; - let deadline = Instant::now() + STOP_GRACE; - while Instant::now() < deadline { - if let Ok(Some(_)) = child.try_wait() { - return; - } - thread::sleep(POLL); - } - } - // SAFETY: as above. - unsafe { libc::kill(group, libc::SIGKILL) }; - let _ = child.wait(); -} - /// Copy every line of `stream` to `log_file` and print the progress lines it /// condenses to, until the stream ends. fn follow( diff --git a/tests/fakes.rs b/tests/fakes.rs index 0b197d18a..496dfef4d 100644 --- a/tests/fakes.rs +++ b/tests/fakes.rs @@ -24,6 +24,7 @@ mod json; use std::fs::{self, File}; use std::io::Write; +use std::os::unix::process::CommandExt; use std::path::{Path, PathBuf}; use std::process::{Command, Output}; @@ -110,6 +111,25 @@ fn main() { "gh" => gh::main(args.collect()), "claude" => claude::main(args.collect()), "codex" => codex::main(args.collect()), + "detached-command" => { + let command = args.next().expect("no detached command"); + let mut command = Command::new(command); + command.args(args); + // SAFETY: setsid is async-signal-safe and accesses no Rust state. + unsafe { + command.pre_exec(|| { + unsafe extern "C" { + fn setsid() -> i32; + } + if setsid() == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + let error = command.exec(); + die(&format!("could not start detached command: {error}"), 1); + } _ => die(&format!("fake: no fake is called {name}"), 2), } exit(0); diff --git a/tests/process_tree.rs b/tests/process_tree.rs new file mode 100644 index 000000000..473c39b3c --- /dev/null +++ b/tests/process_tree.rs @@ -0,0 +1,116 @@ +//! Interrupting a Run stops commands that escaped the session's process group. + +mod support; + +use std::fs; +use std::thread; +use std::time::{Duration, Instant}; + +use support::Scenario; + +/// Clean up by the recorded pid even when the regression assertion fails. +struct DetachedCommand(libc::pid_t); + +impl DetachedCommand { + fn exists(&self) -> bool { + // SAFETY: kill has no memory-safety preconditions; signal 0 only probes. + unsafe { libc::kill(self.0, 0) == 0 } + } +} + +impl Drop for DetachedCommand { + fn drop(&mut self) { + if self.exists() { + // SAFETY: the recorded pid is this command's process group leader. + unsafe { libc::kill(-self.0, libc::SIGKILL) }; + } + } +} + +#[test] +fn interrupting_a_claude_run_stops_a_command_in_its_own_session() { + assert_detached_command_stopped("claude"); +} + +#[test] +fn interrupting_a_codex_run_stops_a_command_in_its_own_session() { + assert_detached_command_stopped("codex"); +} + +fn assert_detached_command_stopped(harness: &str) { + let scenario = Scenario::new(); + let pid = scenario.path("detached-pid"); + scenario.agent_does(&format!( + r#"detached-command bash -c 'trap "" INT TERM; echo $$ > "{pid}"; exec sleep 120' >/dev/null 2>&1 & +while ! test -s "{pid}"; do sleep 0.01; done +touch "{started}" +while :; do sleep 0.1; done +"#, + pid = pid.display(), + started = scenario.path("agent-started").display(), + )); + + let mut held = scenario.run_until( + &["harness", harness, &scenario.issue_url(7)], + &[], + "agent-started", + ); + let command = DetachedCommand(fs::read_to_string(pid).unwrap().trim().parse().unwrap()); + assert!(command.exists(), "the detached command never started"); + let interrupted_at = Instant::now(); + held.signal("INT"); + let result = held.finish(); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert!( + interrupted_at.elapsed() < Duration::from_secs(8), + "a session that exited on its first signal incurred another grace period" + ); + let deadline = Instant::now() + Duration::from_secs(2); + while command.exists() && Instant::now() < deadline { + thread::sleep(Duration::from_millis(10)); + } + assert!( + !command.exists(), + "the detached command survived interruption" + ); +} + +#[test] +fn a_command_started_during_the_stop_grace_period_is_killed_too() { + let scenario = Scenario::new(); + let pid = scenario.path("late-pid"); + let signals = scenario.path("signals"); + scenario.agent_does(&format!( + r#"on_int() {{ + echo INT >> "{signals}" + detached-command bash -c 'trap "" INT TERM; echo $$ > "{pid}"; exec sleep 120' >/dev/null 2>&1 & +}} +trap on_int INT +trap 'echo TERM >> "{signals}"' TERM +touch "{started}" +while :; do sleep 0.1 || :; done +"#, + pid = pid.display(), + signals = signals.display(), + started = scenario.path("agent-started").display(), + )); + + let result = scenario.run_and_signal( + &["harness", "codex", &scenario.issue_url(7)], + "agent-started", + "INT", + ); + let command = DetachedCommand(fs::read_to_string(pid).unwrap().trim().parse().unwrap()); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(fs::read_to_string(signals).unwrap(), "INT\nTERM\n"); + let deadline = Instant::now() + Duration::from_secs(2); + while command.exists() && Instant::now() < deadline { + thread::sleep(Duration::from_millis(10)); + } + assert!( + !command.exists(), + "the command started during the grace period survived" + ); +} diff --git a/tests/support/fakes.rs b/tests/support/fakes.rs index b851c6b75..2de6a51e2 100644 --- a/tests/support/fakes.rs +++ b/tests/support/fakes.rs @@ -21,7 +21,7 @@ const SOURCES: [&str; 5] = [ /// Put the fake `gh`, `claude` and `codex` in `bin`. pub fn install(bin: &Path) { - for name in ["gh", "claude", "codex"] { + for name in ["gh", "claude", "codex", "detached-command"] { std::os::unix::fs::symlink(executable(), bin.join(name)).unwrap(); } } From 3778911b511e70cfb82365e7166b40a2a639531e Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 12:38:54 -0400 Subject: [PATCH 2/2] Share detached-command exit assertions after Standards review --- tests/process_tree.rs | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/tests/process_tree.rs b/tests/process_tree.rs index 473c39b3c..2c898e7dc 100644 --- a/tests/process_tree.rs +++ b/tests/process_tree.rs @@ -16,6 +16,14 @@ impl DetachedCommand { // SAFETY: kill has no memory-safety preconditions; signal 0 only probes. unsafe { libc::kill(self.0, 0) == 0 } } + + fn assert_stopped(&self, message: &str) { + let deadline = Instant::now() + Duration::from_secs(2); + while self.exists() && Instant::now() < deadline { + thread::sleep(Duration::from_millis(10)); + } + assert!(!self.exists(), "{message}"); + } } impl Drop for DetachedCommand { @@ -66,14 +74,7 @@ while :; do sleep 0.1; done interrupted_at.elapsed() < Duration::from_secs(8), "a session that exited on its first signal incurred another grace period" ); - let deadline = Instant::now() + Duration::from_secs(2); - while command.exists() && Instant::now() < deadline { - thread::sleep(Duration::from_millis(10)); - } - assert!( - !command.exists(), - "the detached command survived interruption" - ); + command.assert_stopped("the detached command survived interruption"); } #[test] @@ -105,12 +106,5 @@ while :; do sleep 0.1 || :; done assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); assert_eq!(fs::read_to_string(signals).unwrap(), "INT\nTERM\n"); - let deadline = Instant::now() + Duration::from_secs(2); - while command.exists() && Instant::now() < deadline { - thread::sleep(Duration::from_millis(10)); - } - assert!( - !command.exists(), - "the command started during the grace period survived" - ); + command.assert_stopped("the command started during the grace period survived"); }