subagent: make interrupt cancel a goal run, not just its turn
`interrupt` killed the child and trusted that to end the run. It didn't: the default signal is SIGINT, and `claude --print` handles SIGINT by writing its terminal `result` event and exiting **zero**. A zero exit is `TurnEnd::Complete`, so `spawn_and_track`'s killed-turn early return — the code that was supposed to stop the run — never fired, and the loop went straight on to spawn the next goal turn. An interrupted run carried on to completion; the interrupt changed nothing about the outcome. Measured, not inferred: `claude --print --verbose --output-format stream-json`, SIGINT'd mid-turn, exits 0 on every run. So record the reason instead of inferring it. `interrupt` writes `StopReason::Cancelled` — the same path `goal_reached`/`need_help` already use — while it still holds the `running` lock, so there is no instant in which the name has lost its cancel handle but not yet gained its stop reason. `plan_after_turn` checks stop reasons ahead of the goal, so the run stops whichever way the child ends up exiting. `force`'s SIGKILL still produces a `TurnEnd::Killed`, and `status` still prefers the kill record it already had. `continue`'s budget reset is untouched: it clears the stop reason and hands back a fresh allowance, which is what makes a cancel a pause an operator can undo. The test spawns the real continuation loop against a fake claude that exits 0 on SIGINT, interrupts it, and asserts turn two never starts — it reports `left: 2` without the fix.
This commit is contained in:
atlas
mara
parent
11ec050ca8
commit
33da51382e
3 changed files with 217 additions and 24 deletions
|
|
@ -268,11 +268,13 @@ impl SubagentMcp {
|
|||
}
|
||||
|
||||
#[tool(
|
||||
description = "Signal a currently-running subagent session to stop. Only works once \
|
||||
it's actually running — a `start`/`continue` still in its brief window before the \
|
||||
process is confirmed spawned refuses interrupt too (nothing to signal yet; retry \
|
||||
shortly), same as a name with nothing tracked at all. `force: true` for SIGKILL, \
|
||||
otherwise SIGINT."
|
||||
description = "Cancel a currently-running subagent session. This stops the whole run, \
|
||||
not just the turn in flight: a session started with a `goal` will not be \
|
||||
re-prompted toward it afterwards, and `continue` is what restarts it. Only works \
|
||||
once it's actually running — a `start`/`continue` still in its brief window before \
|
||||
the process is confirmed spawned refuses interrupt too (nothing to signal yet; \
|
||||
retry shortly), same as a name with nothing tracked at all. `force: true` for \
|
||||
SIGKILL, otherwise SIGINT."
|
||||
)]
|
||||
fn interrupt(&self, Parameters(args): Parameters<InterruptArgs>) -> String {
|
||||
match session::interrupt(&self.state, &args.name, args.force) {
|
||||
|
|
|
|||
|
|
@ -1,8 +1,8 @@
|
|||
//! The claude-facing half of this daemon: spawn a subagent turn, keep giving
|
||||
//! it turns until it says it's done or runs out of them, track it only while
|
||||
//! it's alive, and push exactly one todo when the whole run stops — saying
|
||||
//! whether it finished, was killed, or stopped for one of the four reasons
|
||||
//! the continuation loop records.
|
||||
//! whether it finished, was killed, or stopped for one of the reasons the
|
||||
//! continuation loop records.
|
||||
//!
|
||||
//! **No task files, no restart recovery.** The daemon's only state is an
|
||||
//! in-memory `name -> Option<Cancel>` map (see `State`'s own doc for the
|
||||
|
|
@ -110,11 +110,11 @@ const DEFAULT_MAX_TURNS: u32 = 5;
|
|||
/// by `status`, appended to the end-of-turn todo, and written into the
|
||||
/// session's report file when it has one.
|
||||
///
|
||||
/// Only the four ends of the *continuation loop* live here. A turn that was
|
||||
/// killed or that failed outright never reaches the loop's decision at all:
|
||||
/// those keep the records they already had (`State::killed`, the todo's own
|
||||
/// killed wording), and giving them a second home here would have handed
|
||||
/// `status` two rival answers for one fact.
|
||||
/// Only ends of the *continuation loop* live here. A turn that was killed or
|
||||
/// that failed outright never reaches the loop's decision at all: those keep
|
||||
/// the records they already had (`State::killed`, the todo's own killed
|
||||
/// wording), and giving them a second home here would have handed `status`
|
||||
/// two rival answers for one fact.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
enum StopReason {
|
||||
/// The turn ended and there was no goal to continue toward — the
|
||||
|
|
@ -127,6 +127,19 @@ enum StopReason {
|
|||
NeedHelp(String),
|
||||
/// `turns` turns ran and the goal was never reported reached.
|
||||
TurnCap { turns: u32 },
|
||||
/// An operator called `interrupt`. Unlike the four above, this is
|
||||
/// recorded from *outside* the run, by `interrupt` itself, before the
|
||||
/// signal is sent.
|
||||
///
|
||||
/// It has to be, and that is the whole of it: `interrupt`'s
|
||||
/// default is SIGINT, and claude handles SIGINT by writing its terminal
|
||||
/// `result` event and **exiting 0**. An exit of zero is
|
||||
/// `TurnEnd::Complete`, so the killed-turn early return in
|
||||
/// `spawn_and_track` never fires and the loop re-prompts a run the
|
||||
/// operator had just asked to stop. Recording the reason makes the
|
||||
/// cancel a fact about the *run* rather than an inference from how its
|
||||
/// child happened to die.
|
||||
Cancelled,
|
||||
}
|
||||
|
||||
impl StopReason {
|
||||
|
|
@ -156,6 +169,9 @@ impl StopReason {
|
|||
"the harness turn limit was reached ({turns} turns) without the goal ever being \
|
||||
reported reached, so the work stopped where it had got to"
|
||||
),
|
||||
Self::Cancelled => "it was interrupted, which cancels the whole run and not just the \
|
||||
turn that was in flight — its work stopped wherever it had got to"
|
||||
.to_owned(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1889,7 +1905,7 @@ fn describe_pending_signal(stop: Option<&StopReason>) -> String {
|
|||
/// The answer for a session whose run has stopped, one per [`StopReason`].
|
||||
/// Each names the state, why the continuation stopped, and what `continue`
|
||||
/// would do about it — and the `GoalReached` one is deliberately the least
|
||||
/// reassuring of the four, because it is the one a caller is most likely to
|
||||
/// reassuring of them, because it is the one a caller is most likely to
|
||||
/// read as "finished successfully" when it means "said so".
|
||||
fn describe_stopped(name: &str, stop: &StopReason, turns: &str) -> String {
|
||||
match stop {
|
||||
|
|
@ -1919,6 +1935,12 @@ fn describe_stopped(name: &str, stop: &StopReason, turns: &str) -> String {
|
|||
than finishing.{turns} Check what it actually left behind; `continue` gives it a \
|
||||
fresh allowance if carrying on is worth it."
|
||||
),
|
||||
StopReason::Cancelled => format!(
|
||||
"subagent `{name}` was CANCELLED — someone called `interrupt` on it, which stopped \
|
||||
the run and not merely the turn that was in flight, so nothing further will start \
|
||||
on its own.{turns} Its work stopped wherever it had got to: check what it left \
|
||||
behind before acting on it. `continue` restarts it, with a fresh turn allowance."
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1928,12 +1950,25 @@ fn describe_stopped(name: &str, stop: &StopReason, turns: &str) -> String {
|
|||
/// to signal yet — the reservation is put back so a concurrent
|
||||
/// `start`/`continue` for the same name still gets refused).
|
||||
///
|
||||
/// This stops a goal run, not just the turn in it: the signalled child ends
|
||||
/// as `TurnEnd::Killed`, which the continuation loop treats as the end of
|
||||
/// the whole run rather than something to re-prompt past. That falls out of
|
||||
/// there being no child left to continue, and it is the answer you want —
|
||||
/// `interrupt` would be useless if the harness immediately started turn
|
||||
/// three of five.
|
||||
/// This stops a goal run, not just the turn in it — and it does so by
|
||||
/// **recording `StopReason::Cancelled`**, not by relying on how the child
|
||||
/// dies.
|
||||
///
|
||||
/// That distinction is the entire fix. The signal alone is not enough: the
|
||||
/// default is SIGINT, claude handles SIGINT by emitting its terminal
|
||||
/// `result` event and exiting 0, and a zero exit is `TurnEnd::Complete` —
|
||||
/// so `spawn_and_track`'s killed-turn early return never fires and the loop
|
||||
/// goes straight on to spawn turn three of five. The recorded reason is what
|
||||
/// `plan_after_turn` reads, and it is checked before the goal, so the run
|
||||
/// stops whichever way the child ends up exiting. `force`'s SIGKILL does
|
||||
/// produce a `TurnEnd::Killed`; recording the reason anyway costs nothing
|
||||
/// and keeps one path, and `status` still prefers the kill record it
|
||||
/// already had.
|
||||
///
|
||||
/// The reason is written while the `running` lock is still held, so there is
|
||||
/// no instant in which the name has lost its cancel handle but not yet
|
||||
/// gained its stop reason — a turn ending of its own accord in that gap
|
||||
/// would find nothing to stop it and start the next one.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
|
|
@ -1952,9 +1987,14 @@ pub fn interrupt(state: &State, name: &str, force: bool) -> anyhow::Result<Strin
|
|||
);
|
||||
}
|
||||
Some(Some(cancel)) => {
|
||||
state.record_stop(name, StopReason::Cancelled);
|
||||
drop(running);
|
||||
cancel.cancel(force);
|
||||
Ok(format!("interrupt sent to subagent `{name}`"))
|
||||
Ok(format!(
|
||||
"interrupt sent to subagent `{name}` — the run is cancelled, not just the turn \
|
||||
that was in flight, so no further goal turn will start. `continue` is what \
|
||||
restarts it, with a fresh turn allowance."
|
||||
))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -3181,6 +3221,146 @@ mod tests {
|
|||
}
|
||||
}
|
||||
|
||||
/// A stand-in for claude that reproduces the one behaviour the cancel
|
||||
/// turns on: on SIGINT it exits **zero**. The real `claude --print` does
|
||||
/// exactly this — it writes its terminal `result` event and shuts down
|
||||
/// cleanly — which is why an interrupted turn is indistinguishable from
|
||||
/// a completed one by exit status alone, and why `interrupt` cannot rely
|
||||
/// on the child's death to stop the run.
|
||||
///
|
||||
/// It appends a line to `log` as its very first act, so the file is a
|
||||
/// count of how many turns the continuation loop actually spawned. Every
|
||||
/// argument is ignored; the driver's own flags mean nothing here.
|
||||
///
|
||||
/// The wait is `sleep &` + `wait` rather than a plain foreground
|
||||
/// `sleep`: a POSIX shell runs a pending trap as soon as `wait` returns,
|
||||
/// but defers it until a *foreground* child has finished — which for a
|
||||
/// 30-second sleep is long after the test has given up. The background
|
||||
/// sleep gets its own stdout/stderr for the mirror-image reason: it
|
||||
/// would otherwise inherit and keep open the pipes the driver reads to
|
||||
/// EOF, so `RunningClaude::wait` would not return until the orphaned
|
||||
/// sleep expired, and the turn would look like it was still running.
|
||||
fn fake_claude(dir: &Path, log: &Path) -> PathBuf {
|
||||
use std::os::unix::fs::PermissionsExt as _;
|
||||
let script = dir.join("fake-claude");
|
||||
std::fs::write(
|
||||
&script,
|
||||
format!(
|
||||
"#!/bin/sh\necho turn >> '{}'\ntrap 'exit 0' INT\nsleep 30 >/dev/null 2>&1 &\nwait\n",
|
||||
log.display()
|
||||
),
|
||||
)
|
||||
.expect("write the fake claude");
|
||||
std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755))
|
||||
.expect("make the fake claude executable");
|
||||
script
|
||||
}
|
||||
|
||||
/// How many turns the fake has been spawned for so far.
|
||||
fn turns_spawned(log: &Path) -> usize {
|
||||
std::fs::read_to_string(log).map_or(0, |body| body.lines().count())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn an_interrupt_cancels_the_whole_goal_run_and_not_merely_its_turn() {
|
||||
// The regression, end to end through the real
|
||||
// continuation loop: a goal run with four turns left is interrupted,
|
||||
// its child exits cleanly the way claude actually does, and the loop
|
||||
// must not spawn turn two.
|
||||
let dir = std::env::temp_dir().join(format!(
|
||||
"hive-subagent-cancel-{}-{:?}",
|
||||
std::process::id(),
|
||||
std::thread::current().id()
|
||||
));
|
||||
std::fs::create_dir_all(&dir).expect("scratch dir");
|
||||
let log = dir.join("turns");
|
||||
let program = fake_claude(&dir, &log);
|
||||
|
||||
let state = Arc::new(mid_run("n", "carry on indefinitely", 5));
|
||||
let config = Config {
|
||||
program: Some(program),
|
||||
..Config::default()
|
||||
};
|
||||
spawn_and_track(
|
||||
&state,
|
||||
"n",
|
||||
&config,
|
||||
&Attach::Create("n".to_owned()),
|
||||
"turn one".to_owned(),
|
||||
None,
|
||||
)
|
||||
.expect("the fake claude spawns");
|
||||
|
||||
// Wait for the first turn to be genuinely under way before
|
||||
// interrupting: signalling a child that hasn't run yet would prove
|
||||
// nothing about the loop.
|
||||
for _ in 0..100 {
|
||||
if turns_spawned(&log) == 1 {
|
||||
break;
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(50)).await;
|
||||
}
|
||||
assert_eq!(turns_spawned(&log), 1, "the first turn never started");
|
||||
|
||||
// An operator's plain `interrupt`, not `force` — SIGINT is the
|
||||
// default and the case that was broken.
|
||||
interrupt(&state, "n", false).expect("a running turn is interruptible");
|
||||
|
||||
// Generous: the fake logs its invocation as its first act, so a
|
||||
// re-prompt shows up here almost immediately once the loop decides
|
||||
// to make one.
|
||||
tokio::time::sleep(Duration::from_millis(1500)).await;
|
||||
assert_eq!(
|
||||
turns_spawned(&log),
|
||||
1,
|
||||
"an interrupted goal run must not be re-prompted — that is the whole of the cancel"
|
||||
);
|
||||
assert_eq!(
|
||||
state.stop_reason("n"),
|
||||
Some(StopReason::Cancelled),
|
||||
"and the run must carry a reason saying why it stopped"
|
||||
);
|
||||
std::fs::remove_dir_all(&dir).ok();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_cancelled_run_stops_with_turns_still_on_its_clock() {
|
||||
// The loop half of the same fact, asserted directly: `Cancelled` is
|
||||
// read where the other stop signals are, ahead of the goal, so it
|
||||
// stops a run that has four turns left rather than being overridden
|
||||
// by them.
|
||||
let state = mid_run("n", "a goal", 5);
|
||||
state.record_stop("n", StopReason::Cancelled);
|
||||
assert_eq!(
|
||||
state.plan_after_turn("n"),
|
||||
Continuation::Stop(StopReason::Cancelled)
|
||||
);
|
||||
assert_eq!(
|
||||
state.turns("n"),
|
||||
Some((1, 5)),
|
||||
"and must not spend one on the way out"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_continue_after_a_cancel_starts_the_run_over() {
|
||||
// The deliberate other half: a cancel is not a permanent
|
||||
// state. `continue` clears the reason and hands back a fresh
|
||||
// allowance, which is what makes `interrupt` a pause an operator can
|
||||
// undo rather than a session they have to abandon.
|
||||
let state = mid_run("n", "a goal", 5);
|
||||
state.record_stop("n", StopReason::Cancelled);
|
||||
state.clear_stop("n");
|
||||
state.restart_turns("n");
|
||||
assert_eq!(
|
||||
state.plan_after_turn("n"),
|
||||
Continuation::Continue {
|
||||
prompt: continuation_prompt("n", "a goal", 2, 5)
|
||||
},
|
||||
"a continued session resumes toward its goal"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_signal_on_the_last_allowed_turn_outranks_the_cap() {
|
||||
// Both are true at once, and which one is reported is the difference
|
||||
|
|
|
|||
Loading…
Reference in a new issue