subagent daemon: report a killed session as killed, not idle
A subagent whose claude process died on a signal — the kernel's OOM
killer, a stopped unit, an `interrupt` — was indistinguishable from one
that finished its turn: its entry left the `running` map, `status` fell
through to "a session exists on disk" and answered `idle`, and the
end-of-turn todo said the subagent had "finished". The usual next move
on that reading is `continue`, which resumes work that was cut mid-turn
with nothing having recorded that it was cut.
The driver already preserves how the child ended — `RunningClaude::wait`
returns `Error::Exit` carrying the `ExitStatus`, whose `signal()` is the
whole answer — so this reads it rather than having to recover it:
`classify_end` turns the outcome into `Complete` / `Killed { signal }` /
`Failed`, and `State::finish_turn` remembers a kill against the name
(cleared by the next confirmed spawn under it).
What an agent sees as a result:
- `status` reports the session killed, naming the signal, instead of idle;
- the todo the daemon pushes without being asked says the subagent was
KILLED mid-turn rather than that it finished;
- `continue` still resumes such a session, but its reply says the
previous turn was killed, so no caller carries on from cut-off work
believing it was complete.
Refs #4326
This commit is contained in:
parent
fde4a36b93
commit
f817e27d4c
5 changed files with 383 additions and 29 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -1996,6 +1996,7 @@ dependencies = [
|
||||||
"hive-claude",
|
"hive-claude",
|
||||||
"hive-sock-client",
|
"hive-sock-client",
|
||||||
"hive-types",
|
"hive-types",
|
||||||
|
"libc",
|
||||||
"rmcp",
|
"rmcp",
|
||||||
"schemars",
|
"schemars",
|
||||||
"serde",
|
"serde",
|
||||||
|
|
|
||||||
|
|
@ -33,6 +33,26 @@ claude's own on-disk session (`hive_claude::SessionStore`), which
|
||||||
`continue` reattaches to independent of the daemon's own lifetime — a
|
`continue` reattaches to independent of the daemon's own lifetime — a
|
||||||
restart loses the _in-flight turn_, not the subagent's history.
|
restart loses the _in-flight turn_, not the subagent's history.
|
||||||
|
|
||||||
|
## A killed turn
|
||||||
|
|
||||||
|
A subagent whose `claude` process dies on a signal — the kernel's OOM
|
||||||
|
killer under memory pressure, a stopped unit, an `interrupt` from the
|
||||||
|
owning agent — did not finish its turn, and the daemon says so rather
|
||||||
|
than letting it settle back into `idle`:
|
||||||
|
|
||||||
|
- `status` reports the session **killed**, naming the signal, instead of
|
||||||
|
the `idle` it reports for a turn that ended on its own;
|
||||||
|
- the end-of-turn todo the daemon pushes without being asked says the
|
||||||
|
subagent was killed mid-turn, not that it finished;
|
||||||
|
- `continue` still resumes such a session — often what you want — but
|
||||||
|
its reply says the previous turn was killed, so nobody carries on from
|
||||||
|
cut-off work believing it was complete.
|
||||||
|
|
||||||
|
The record is per-name, in memory with the rest of this daemon's state,
|
||||||
|
and the next confirmed spawn under that name clears it. A daemon restart
|
||||||
|
loses it along with everything else — the daemon has to outlive the kill
|
||||||
|
to report it, which is what `OOMPolicy=continue` on the unit is for.
|
||||||
|
|
||||||
## Compaction trade-off
|
## Compaction trade-off
|
||||||
|
|
||||||
Built on `hive_claude::Claude::spawn` + `RunningClaude::wait` directly
|
Built on `hive_claude::Claude::spawn` + `RunningClaude::wait` directly
|
||||||
|
|
|
||||||
|
|
@ -15,6 +15,7 @@ hive-agent-sock.workspace = true
|
||||||
hive-claude.workspace = true
|
hive-claude.workspace = true
|
||||||
hive-sock-client.workspace = true
|
hive-sock-client.workspace = true
|
||||||
hive-types.workspace = true
|
hive-types.workspace = true
|
||||||
|
libc.workspace = true
|
||||||
rmcp.workspace = true
|
rmcp.workspace = true
|
||||||
schemars.workspace = true
|
schemars.workspace = true
|
||||||
serde.workspace = true
|
serde.workspace = true
|
||||||
|
|
|
||||||
|
|
@ -131,7 +131,9 @@ impl SubagentMcp {
|
||||||
because its previous turn finished and you have a follow-up instruction, or you're \
|
because its previous turn finished and you have a follow-up instruction, or you're \
|
||||||
reattaching after this daemon restarted (the session itself survives independently \
|
reattaching after this daemon restarted (the session itself survives independently \
|
||||||
of the daemon that spawned it). Returns as soon as confirmed running, same as \
|
of the daemon that spawned it). Returns as soon as confirmed running, same as \
|
||||||
`start`. Refuses a name with no session on disk at all, or one already running."
|
`start`. Refuses a name with no session on disk at all, or one already running. \
|
||||||
|
Resuming a session whose last turn was killed is allowed — the reply says so, since \
|
||||||
|
that turn's work stopped wherever it had got to."
|
||||||
)]
|
)]
|
||||||
fn r#continue(&self, Parameters(args): Parameters<ContinueArgs>) -> String {
|
fn r#continue(&self, Parameters(args): Parameters<ContinueArgs>) -> String {
|
||||||
match session::continue_(
|
match session::continue_(
|
||||||
|
|
@ -164,8 +166,10 @@ impl SubagentMcp {
|
||||||
description = "Report whether a subagent is currently running — a zero-cost check that \
|
description = "Report whether a subagent is currently running — a zero-cost check that \
|
||||||
never launches a process, unlike `continue`. Distinguishes running, starting (a \
|
never launches a process, unlike `continue`. Distinguishes running, starting (a \
|
||||||
`start`/`continue` is in flight but not yet a confirmed spawn — this is normally \
|
`start`/`continue` is in flight but not yet a confirmed spawn — this is normally \
|
||||||
over in well under a second), idle (a session exists but nothing is in flight — \
|
over in well under a second), killed (its last turn died on a signal — the kernel's \
|
||||||
`continue` to give it another turn), and no such session at all."
|
OOM killer, a stopped unit, an `interrupt` — so its work was cut off mid-turn rather \
|
||||||
|
than finished), idle (a session exists, its last turn finished, nothing is in flight \
|
||||||
|
— `continue` to give it another turn), and no such session at all."
|
||||||
)]
|
)]
|
||||||
fn status(&self, Parameters(args): Parameters<StatusArgs>) -> String {
|
fn status(&self, Parameters(args): Parameters<StatusArgs>) -> String {
|
||||||
match session::status(&self.state, &args.name, args.dir.as_deref()) {
|
match session::status(&self.state, &args.name, args.dir.as_deref()) {
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,6 @@
|
||||||
//! The claude-facing half of this daemon: spawn a subagent turn, track it
|
//! The claude-facing half of this daemon: spawn a subagent turn, track it
|
||||||
//! only while it's alive, and push exactly one todo when it finishes.
|
//! only while it's alive, and push exactly one todo when it ends — saying
|
||||||
|
//! whether it finished or was killed.
|
||||||
//!
|
//!
|
||||||
//! **No task files, no restart recovery.** The daemon's only state is an
|
//! **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 what
|
//! in-memory `name -> Option<Cancel>` map (see `State`'s own doc for what
|
||||||
|
|
@ -15,6 +16,15 @@
|
||||||
//! restarted and I want to pick this back up" — after a restart, re-supply
|
//! restarted and I want to pick this back up" — after a restart, re-supply
|
||||||
//! `dir` once if the session isn't in the daemon's own default directory.
|
//! `dir` once if the session isn't in the daemon's own default directory.
|
||||||
//!
|
//!
|
||||||
|
|
||||||
|
//! **A killed turn is not a finished turn.** A child that died on a signal
|
||||||
|
//! arrives as a `hive_claude::Error::Exit` carrying its `ExitStatus`, so the
|
||||||
|
//! "how" is there to be read: `classify_end` takes the signal out of it and
|
||||||
|
//! `State::finish_turn` remembers it against the name, which is what lets
|
||||||
|
//! `status`, the end-of-turn todo and `continue`'s own reply all say the
|
||||||
|
//! session was killed rather than let it read as finished. See
|
||||||
|
//! `docs/tools/subagent.md`.
|
||||||
|
//!
|
||||||
//! **No mid-turn compaction.** Building on `hive_claude::Claude::spawn` +
|
//! **No mid-turn compaction.** Building on `hive_claude::Claude::spawn` +
|
||||||
//! `RunningClaude::wait` directly (not `InfiniteSession::run`) is what makes
|
//! `RunningClaude::wait` directly (not `InfiniteSession::run`) is what makes
|
||||||
//! `interrupt` possible at all — `InfiniteSession` has no cancel handle to
|
//! `interrupt` possible at all — `InfiniteSession` has no cancel handle to
|
||||||
|
|
@ -28,11 +38,61 @@
|
||||||
//! assumption stops holding, not shipped here.
|
//! assumption stops holding, not shipped here.
|
||||||
|
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
|
use std::os::unix::process::ExitStatusExt as _;
|
||||||
use std::path::PathBuf;
|
use std::path::PathBuf;
|
||||||
use std::sync::{Arc, Mutex, PoisonError};
|
use std::sync::{Arc, Mutex, PoisonError};
|
||||||
|
|
||||||
use hive_claude::{Attach, Cancel, Claude, Config, NoopSink, SessionStore};
|
use hive_claude::{Attach, Cancel, Claude, Config, NoopSink, SessionStore};
|
||||||
|
|
||||||
|
/// How a subagent's turn ended, as far as the daemon can tell from what
|
||||||
|
/// [`hive_claude::RunningClaude::wait`] returned.
|
||||||
|
///
|
||||||
|
/// The distinction that matters is `Killed` vs everything else: a child that
|
||||||
|
/// died on a signal was cut off mid-turn by something outside this daemon
|
||||||
|
/// (the kernel's OOM killer, a `systemctl stop`, an operator's `kill`) or by
|
||||||
|
/// an `interrupt` call here — either way its work stopped wherever it got
|
||||||
|
/// to, which is not what `Complete` means.
|
||||||
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||||
|
enum TurnEnd {
|
||||||
|
/// The turn ran to completion.
|
||||||
|
Complete,
|
||||||
|
/// The child was terminated by `signal` instead of exiting on its own.
|
||||||
|
Killed { signal: i32 },
|
||||||
|
/// claude exited on its own but did not complete the turn — a
|
||||||
|
/// recognized sentinel (rate limit, prompt too long, …) or a plain
|
||||||
|
/// non-zero exit. Carries the message shown to the caller.
|
||||||
|
Failed(String),
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Name a signal number for a human: `9` reads as `SIGKILL (signal 9)`. Only
|
||||||
|
/// the three that end a subagent in practice are named — anything else keeps
|
||||||
|
/// the number, which is still enough to look up.
|
||||||
|
fn describe_signal(signal: i32) -> String {
|
||||||
|
match signal {
|
||||||
|
libc::SIGKILL => "SIGKILL (signal 9)".to_owned(),
|
||||||
|
libc::SIGTERM => "SIGTERM (signal 15)".to_owned(),
|
||||||
|
libc::SIGINT => "SIGINT (signal 2)".to_owned(),
|
||||||
|
other => format!("signal {other}"),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Read how the turn ended out of what the driver returned. A signalled
|
||||||
|
/// child surfaces as [`hive_claude::Error::Exit`] whose `ExitStatus` has a
|
||||||
|
/// `signal()` — the driver's own docs point at exactly this check — so the
|
||||||
|
/// "how" is preserved by the time it reaches here rather than having to be
|
||||||
|
/// recovered.
|
||||||
|
fn classify_end(outcome: hive_claude::Result<()>) -> TurnEnd {
|
||||||
|
let Err(error) = outcome else {
|
||||||
|
return TurnEnd::Complete;
|
||||||
|
};
|
||||||
|
if let hive_claude::Error::Exit { status, .. } = &error
|
||||||
|
&& let Some(signal) = status.signal()
|
||||||
|
{
|
||||||
|
return TurnEnd::Killed { signal };
|
||||||
|
}
|
||||||
|
TurnEnd::Failed(format!("claude error: {error}"))
|
||||||
|
}
|
||||||
|
|
||||||
/// This daemon's whole state: which names currently have a live process or
|
/// This daemon's whole state: which names currently have a live process or
|
||||||
/// a reservation in flight, and where to push the completion todo.
|
/// a reservation in flight, and where to push the completion todo.
|
||||||
/// `Arc`-wrapped so the background task that drives a turn to completion
|
/// `Arc`-wrapped so the background task that drives a turn to completion
|
||||||
|
|
@ -46,6 +106,12 @@ use hive_claude::{Attach, Cancel, Claude, Config, NoopSink, SessionStore};
|
||||||
/// `name` free" and committing to it are two different lock acquisitions
|
/// `name` free" and committing to it are two different lock acquisitions
|
||||||
/// unless the check *is* the reservation — see `reserve`.
|
/// unless the check *is* the reservation — see `reserve`.
|
||||||
///
|
///
|
||||||
|
/// `killed` is the other half of that map's story: `running` says what is in
|
||||||
|
/// flight *now*, `killed` remembers the names whose last turn ended on a
|
||||||
|
/// signal rather than on its own, keyed to the signal number. Without it a
|
||||||
|
/// killed session is indistinguishable from a finished one the moment its
|
||||||
|
/// entry leaves `running` — the whole point of this record.
|
||||||
|
///
|
||||||
/// ⚠️ This concurrency guard is keyed by `name` alone — a `start`/`continue`
|
/// ⚠️ This concurrency guard is keyed by `name` alone — a `start`/`continue`
|
||||||
/// for `name` with a *different* `dir` than one already in flight under
|
/// for `name` with a *different* `dir` than one already in flight under
|
||||||
/// that name is refused as "already running," even though the two would
|
/// that name is refused as "already running," even though the two would
|
||||||
|
|
@ -56,6 +122,7 @@ use hive_claude::{Attach, Cancel, Claude, Config, NoopSink, SessionStore};
|
||||||
pub struct State {
|
pub struct State {
|
||||||
running: Mutex<HashMap<String, Option<Cancel>>>,
|
running: Mutex<HashMap<String, Option<Cancel>>>,
|
||||||
dirs: Mutex<HashMap<String, String>>,
|
dirs: Mutex<HashMap<String, String>>,
|
||||||
|
killed: Mutex<HashMap<String, i32>>,
|
||||||
socket: PathBuf,
|
socket: PathBuf,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -65,6 +132,7 @@ impl State {
|
||||||
Self {
|
Self {
|
||||||
running: Mutex::new(HashMap::new()),
|
running: Mutex::new(HashMap::new()),
|
||||||
dirs: Mutex::new(HashMap::new()),
|
dirs: Mutex::new(HashMap::new()),
|
||||||
|
killed: Mutex::new(HashMap::new()),
|
||||||
socket,
|
socket,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -107,6 +175,48 @@ impl State {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Retire the finished turn's tracking for `name` and remember how it
|
||||||
|
/// ended: a `Killed` end is recorded so `status`, the end-of-turn todo
|
||||||
|
/// and a later `continue` can all say so; any other end clears a stale
|
||||||
|
/// record from an earlier turn.
|
||||||
|
///
|
||||||
|
/// The kill is recorded *before* the `running` entry goes away, so there
|
||||||
|
/// is no instant in which `name` is neither running nor known-killed —
|
||||||
|
/// a `status` landing between the two would otherwise read the session
|
||||||
|
/// as plainly idle, which is the exact confusion this record exists to
|
||||||
|
/// remove.
|
||||||
|
fn finish_turn(&self, name: &str, end: &TurnEnd) {
|
||||||
|
let mut killed = self.killed.lock().unwrap_or_else(PoisonError::into_inner);
|
||||||
|
match end {
|
||||||
|
TurnEnd::Killed { signal } => killed.insert(name.to_owned(), *signal),
|
||||||
|
TurnEnd::Complete | TurnEnd::Failed(_) => killed.remove(name),
|
||||||
|
};
|
||||||
|
drop(killed);
|
||||||
|
self.running
|
||||||
|
.lock()
|
||||||
|
.unwrap_or_else(PoisonError::into_inner)
|
||||||
|
.remove(name);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The signal `name`'s last turn died on, if it died on one.
|
||||||
|
fn killed_by(&self, name: &str) -> Option<i32> {
|
||||||
|
self.killed
|
||||||
|
.lock()
|
||||||
|
.unwrap_or_else(PoisonError::into_inner)
|
||||||
|
.get(name)
|
||||||
|
.copied()
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Forget that `name`'s last turn was killed — called once a new turn is
|
||||||
|
/// confirmed spawned, since the record describes the turn before it and
|
||||||
|
/// would otherwise keep flagging a session that has since run again.
|
||||||
|
fn clear_kill(&self, name: &str) {
|
||||||
|
self.killed
|
||||||
|
.lock()
|
||||||
|
.unwrap_or_else(PoisonError::into_inner)
|
||||||
|
.remove(name);
|
||||||
|
}
|
||||||
|
|
||||||
/// An explicit `dir` is remembered against `name` and returned as-is; an
|
/// An explicit `dir` is remembered against `name` and returned as-is; an
|
||||||
/// omitted one (`None`) falls back to whatever was last remembered for
|
/// omitted one (`None`) falls back to whatever was last remembered for
|
||||||
/// `name`, so a `start` that gave a `dir` doesn't force every later
|
/// `name`, so a `start` that gave a `dir` doesn't force every later
|
||||||
|
|
@ -301,6 +411,11 @@ fn start_reserved(
|
||||||
/// at all (nothing to continue) or one already running (same
|
/// at all (nothing to continue) or one already running (same
|
||||||
/// concurrent-run hazard as `start`).
|
/// concurrent-run hazard as `start`).
|
||||||
///
|
///
|
||||||
|
/// Resuming a session whose last turn was *killed* is allowed — that is
|
||||||
|
/// often exactly what the caller wants — but never silent: the reply says
|
||||||
|
/// so, since a caller that never ran `status` and missed the todo would
|
||||||
|
/// otherwise carry on from cut-off work believing it was finished work.
|
||||||
|
///
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
/// No session under `name`, an invalid name, one already running, or
|
/// No session under `name`, an invalid name, one already running, or
|
||||||
|
|
@ -318,6 +433,9 @@ pub fn continue_(
|
||||||
"subagent `{name}` is already running — use `interrupt` first if you meant to redirect it"
|
"subagent `{name}` is already running — use `interrupt` first if you meant to redirect it"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
// Read before the spawn, which clears the record as soon as the new turn
|
||||||
|
// is confirmed — this reply is the last chance to mention it.
|
||||||
|
let killed = state.killed_by(name);
|
||||||
// Only commit the remembered `dir` now that `reserve` has actually
|
// Only commit the remembered `dir` now that `reserve` has actually
|
||||||
// claimed `name` — see `resolve_dir`'s doc for why the order matters.
|
// claimed `name` — see `resolve_dir`'s doc for why the order matters.
|
||||||
let dir = state.resolve_dir(name, dir);
|
let dir = state.resolve_dir(name, dir);
|
||||||
|
|
@ -325,7 +443,21 @@ pub fn continue_(
|
||||||
if result.is_err() {
|
if result.is_err() {
|
||||||
state.release_reservation(name);
|
state.release_reservation(name);
|
||||||
}
|
}
|
||||||
result
|
result.map(|msg| note_resumed_after_kill(&msg, killed))
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Append the "you are resuming a killed session" note to `continue`'s reply
|
||||||
|
/// when its previous turn was signalled; pass the reply through unchanged
|
||||||
|
/// otherwise.
|
||||||
|
fn note_resumed_after_kill(msg: &str, killed: Option<i32>) -> String {
|
||||||
|
match killed {
|
||||||
|
None => msg.to_owned(),
|
||||||
|
Some(signal) => format!(
|
||||||
|
"{msg} — note: its previous turn was killed ({}) rather than finishing, so this turn \
|
||||||
|
resumes from work that was cut off mid-way",
|
||||||
|
describe_signal(signal)
|
||||||
|
),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The slow, fallible part of `continue_`, run only after `reserve` has
|
/// The slow, fallible part of `continue_`, run only after `reserve` has
|
||||||
|
|
@ -379,45 +511,79 @@ fn spawn_and_track(
|
||||||
.lock()
|
.lock()
|
||||||
.unwrap_or_else(PoisonError::into_inner)
|
.unwrap_or_else(PoisonError::into_inner)
|
||||||
.insert(name.to_owned(), Some(running.cancel_handle()));
|
.insert(name.to_owned(), Some(running.cancel_handle()));
|
||||||
|
// This turn supersedes whatever the previous one did, including having
|
||||||
|
// been killed — the record is about the turn before this one.
|
||||||
|
state.clear_kill(name);
|
||||||
|
|
||||||
let state = Arc::clone(state);
|
let state = Arc::clone(state);
|
||||||
let task_name = name.to_owned();
|
let task_name = name.to_owned();
|
||||||
tokio::spawn(async move {
|
tokio::spawn(async move {
|
||||||
let outcome = running.wait(&prompt, &NoopSink).await;
|
let end = classify_end(running.wait(&prompt, &NoopSink).await);
|
||||||
state
|
match &end {
|
||||||
.running
|
TurnEnd::Complete => {}
|
||||||
.lock()
|
TurnEnd::Killed { signal } => {
|
||||||
.unwrap_or_else(PoisonError::into_inner)
|
tracing::warn!(
|
||||||
.remove(&task_name);
|
name = %task_name,
|
||||||
let summary = match outcome {
|
signal,
|
||||||
Ok(()) => "turn complete".to_owned(),
|
"subagent: turn killed — the child died on a signal, it did not finish"
|
||||||
Err(e) => {
|
);
|
||||||
tracing::warn!(name = %task_name, error = %e, "subagent: turn failed");
|
|
||||||
format!("claude error: {e}")
|
|
||||||
}
|
}
|
||||||
};
|
TurnEnd::Failed(e) => {
|
||||||
push_completion_todo(&state.socket, &task_name, &summary).await;
|
tracing::warn!(name = %task_name, error = %e, "subagent: turn failed");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
state.finish_turn(&task_name, &end);
|
||||||
|
push_turn_end_todo(&state.socket, &task_name, &end).await;
|
||||||
});
|
});
|
||||||
|
|
||||||
Ok(format!("subagent `{name}` started"))
|
Ok(format!("subagent `{name}` started"))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Report whether `name` is currently running — a zero-cost check that
|
/// Report whether `name` is currently running — a zero-cost check that
|
||||||
/// never launches a process, unlike `continue`. Distinguishes four states:
|
/// never launches a process, unlike `continue`. Distinguishes five states:
|
||||||
/// running, starting (reserved, not yet a confirmed spawn — see `State`'s
|
/// running, starting (reserved, not yet a confirmed spawn — see `State`'s
|
||||||
/// doc), idle (a session exists but nothing is in flight), and no such
|
/// doc), killed (its last turn died on a signal), idle (a session exists,
|
||||||
/// session at all.
|
/// its last turn finished, nothing is in flight), and no such session at
|
||||||
|
/// all.
|
||||||
///
|
///
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
/// An invalid name, or no session — running or on disk — under `name`.
|
/// An invalid name, or no session — running, killed or on disk — under
|
||||||
|
/// `name`.
|
||||||
pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result<String> {
|
pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result<String> {
|
||||||
validate_name(name)?;
|
validate_name(name)?;
|
||||||
// Read-only: an explicit `dir` here is a one-off "check this other
|
// Read-only: an explicit `dir` here is a one-off "check this other
|
||||||
// directory's session" per this fn's own doc, not a new remembered
|
// directory's session" per this fn's own doc, not a new remembered
|
||||||
// default — `peek_dir` resolves the same way but never writes.
|
// default — `peek_dir` resolves the same way but never writes.
|
||||||
let dir = state.peek_dir(name, dir);
|
let dir = state.peek_dir(name, dir);
|
||||||
match state.occupancy(name) {
|
let occupancy = state.occupancy(name);
|
||||||
|
let killed = state.killed_by(name);
|
||||||
|
// Nothing on disk to look for while something is tracked in memory —
|
||||||
|
// those states answer on their own, and the store read is the only
|
||||||
|
// expensive part of this call.
|
||||||
|
let session_exists = if occupancy.is_none() && killed.is_none() {
|
||||||
|
let config = build_config(name, None, None, dir.as_deref());
|
||||||
|
build_store(&config)?.find_by_title(name).is_some()
|
||||||
|
} else {
|
||||||
|
false
|
||||||
|
};
|
||||||
|
describe_status(name, occupancy, killed, session_exists)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Render `status`'s answer from the three facts it gathers. Split out from
|
||||||
|
/// the gathering so the killed-versus-idle distinction is exercisable
|
||||||
|
/// without a real spawn, a real signal and a real on-disk claude session.
|
||||||
|
///
|
||||||
|
/// A recorded kill outranks the on-disk session: the session file exists
|
||||||
|
/// either way, so "a session is there" is precisely the fact that cannot
|
||||||
|
/// tell the two apart.
|
||||||
|
fn describe_status(
|
||||||
|
name: &str,
|
||||||
|
occupancy: Option<bool>,
|
||||||
|
killed: Option<i32>,
|
||||||
|
session_exists: bool,
|
||||||
|
) -> anyhow::Result<String> {
|
||||||
|
match occupancy {
|
||||||
Some(true) => return Ok(format!("subagent `{name}` is running")),
|
Some(true) => return Ok(format!("subagent `{name}` is running")),
|
||||||
Some(false) => {
|
Some(false) => {
|
||||||
return Ok(format!(
|
return Ok(format!(
|
||||||
|
|
@ -426,9 +592,16 @@ pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result<St
|
||||||
}
|
}
|
||||||
None => {}
|
None => {}
|
||||||
}
|
}
|
||||||
let config = build_config(name, None, None, dir.as_deref());
|
if let Some(signal) = killed {
|
||||||
let store = build_store(&config)?;
|
return Ok(format!(
|
||||||
if store.find_by_title(name).is_some() {
|
"subagent `{name}` was killed — its last turn died on {}, so its work stopped \
|
||||||
|
wherever it had got to rather than finishing. `continue` still resumes it, but \
|
||||||
|
whatever it was told to do is unfinished: check what it actually left behind before \
|
||||||
|
trusting it.",
|
||||||
|
describe_signal(signal)
|
||||||
|
));
|
||||||
|
}
|
||||||
|
if session_exists {
|
||||||
Ok(format!(
|
Ok(format!(
|
||||||
"subagent `{name}` is idle — its last turn finished; `continue` to give it another"
|
"subagent `{name}` is idle — its last turn finished; `continue` to give it another"
|
||||||
))
|
))
|
||||||
|
|
@ -467,20 +640,37 @@ pub fn interrupt(state: &State, name: &str, force: bool) -> anyhow::Result<Strin
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Push `name`'s one-shot completion todo. Best-effort: a connect/write
|
/// Push `name`'s one-shot end-of-turn todo. Best-effort: a connect/write
|
||||||
/// failure is logged and swallowed, matching every other in-agent-socket
|
/// failure is logged and swallowed, matching every other in-agent-socket
|
||||||
/// producer in this codebase — there's no retry queue to fall back to, and
|
/// producer in this codebase — there's no retry queue to fall back to, and
|
||||||
/// the caller has already moved on by the time this fires.
|
/// the caller has already moved on by the time this fires.
|
||||||
async fn push_completion_todo(socket: &std::path::Path, name: &str, summary: &str) {
|
async fn push_turn_end_todo(socket: &std::path::Path, name: &str, end: &TurnEnd) {
|
||||||
let req = hive_agent_sock::Request::UpsertTodo {
|
let req = hive_agent_sock::Request::UpsertTodo {
|
||||||
subsystem: "subagent".to_owned(),
|
subsystem: "subagent".to_owned(),
|
||||||
key: Some(name.to_owned()),
|
key: Some(name.to_owned()),
|
||||||
summary: format!("subagent `{name}` finished: {summary}"),
|
summary: turn_end_summary(name, end),
|
||||||
source: None,
|
source: None,
|
||||||
reopen_if_acked: false,
|
reopen_if_acked: false,
|
||||||
};
|
};
|
||||||
if let Err(e) = hive_sock_client::notify(socket, &req, hive_sock_client::Retry::None).await {
|
if let Err(e) = hive_sock_client::notify(socket, &req, hive_sock_client::Retry::None).await {
|
||||||
tracing::warn!(name, error = ?e, "subagent: completion todo push failed");
|
tracing::warn!(name, error = ?e, "subagent: end-of-turn todo push failed");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The todo text for a turn that has ended. A killed turn deliberately does
|
||||||
|
/// not use the "finished" wording the other two share: this todo is the only
|
||||||
|
/// thing the owning agent is shown without asking, so it has to read as the
|
||||||
|
/// interruption it is rather than as one more completed subagent.
|
||||||
|
fn turn_end_summary(name: &str, end: &TurnEnd) -> String {
|
||||||
|
match end {
|
||||||
|
TurnEnd::Complete => format!("subagent `{name}` finished: turn complete"),
|
||||||
|
TurnEnd::Failed(e) => format!("subagent `{name}` finished: {e}"),
|
||||||
|
TurnEnd::Killed { signal } => format!(
|
||||||
|
"subagent `{name}` was KILLED mid-turn ({}) — it did not finish, and its work stopped \
|
||||||
|
wherever it had got to. Check what it left behind before you act on it; `continue` \
|
||||||
|
resumes the session if you want it carried on.",
|
||||||
|
describe_signal(*signal)
|
||||||
|
),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -671,6 +861,144 @@ mod tests {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The error the driver hands back for a child that died on `signal` —
|
||||||
|
/// `ExitStatus::from_raw` takes a raw `wait(2)` status, whose low seven
|
||||||
|
/// bits are the terminating signal, so this is the same value
|
||||||
|
/// `RunningClaude::wait` would have produced from a real kill.
|
||||||
|
fn signalled_exit(signal: i32) -> hive_claude::Error {
|
||||||
|
hive_claude::Error::Exit {
|
||||||
|
status: std::process::ExitStatus::from_raw(signal),
|
||||||
|
stderr_tail: String::new(),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn classify_end_separates_a_signalled_child_from_a_clean_one() {
|
||||||
|
assert_eq!(classify_end(Ok(())), TurnEnd::Complete);
|
||||||
|
assert_eq!(
|
||||||
|
classify_end(Err(signalled_exit(libc::SIGKILL))),
|
||||||
|
TurnEnd::Killed { signal: 9 },
|
||||||
|
"a SIGKILLed child must not read as a turn that ended on its own"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
classify_end(Err(signalled_exit(libc::SIGTERM))),
|
||||||
|
TurnEnd::Killed { signal: 15 }
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
matches!(
|
||||||
|
classify_end(Err(hive_claude::Error::PromptTooLong)),
|
||||||
|
TurnEnd::Failed(_)
|
||||||
|
),
|
||||||
|
"claude exiting on its own is a failure, not a kill"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn status_reports_killed_not_idle_for_a_signalled_session() {
|
||||||
|
// Both sessions exist on disk and neither is running: the only thing
|
||||||
|
// that can tell them apart is the recorded signal.
|
||||||
|
let killed = describe_status("n", None, Some(libc::SIGKILL), true).expect("killed status");
|
||||||
|
let idle = describe_status("n", None, None, true).expect("idle status");
|
||||||
|
assert!(
|
||||||
|
killed.contains("killed") && killed.contains("SIGKILL (signal 9)"),
|
||||||
|
"a killed session must say so, and name the signal: {killed}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
!killed.contains("idle"),
|
||||||
|
"a killed session must not also read as idle: {killed}"
|
||||||
|
);
|
||||||
|
assert!(idle.contains("idle"), "a finished turn still reads idle");
|
||||||
|
assert_ne!(
|
||||||
|
killed, idle,
|
||||||
|
"collapsing the two back together is the bug this reports"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_killed_turn_and_a_clean_one_do_not_land_in_the_same_state() {
|
||||||
|
// The whole path a real turn takes, minus the process: what the
|
||||||
|
// driver returned -> what the daemon records -> what `status` says.
|
||||||
|
let state = State::new(PathBuf::from("/dev/null"));
|
||||||
|
for (name, outcome) in [
|
||||||
|
("gone", Err(signalled_exit(libc::SIGKILL))),
|
||||||
|
("done", Ok(())),
|
||||||
|
] {
|
||||||
|
assert!(state.reserve(name));
|
||||||
|
state.finish_turn(name, &classify_end(outcome));
|
||||||
|
assert_eq!(
|
||||||
|
state.occupancy(name),
|
||||||
|
None,
|
||||||
|
"the turn is over either way — nothing stays tracked as running"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
assert_eq!(state.killed_by("gone"), Some(9));
|
||||||
|
assert_eq!(state.killed_by("done"), None);
|
||||||
|
|
||||||
|
let gone = describe_status("gone", None, state.killed_by("gone"), true).expect("status");
|
||||||
|
let done = describe_status("done", None, state.killed_by("done"), true).expect("status");
|
||||||
|
assert!(gone.contains("killed"), "{gone}");
|
||||||
|
assert!(done.contains("idle"), "{done}");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_new_turn_clears_the_previous_turn_s_kill() {
|
||||||
|
let state = State::new(PathBuf::from("/dev/null"));
|
||||||
|
state.finish_turn("n", &TurnEnd::Killed { signal: 9 });
|
||||||
|
assert_eq!(state.killed_by("n"), Some(9));
|
||||||
|
// What `spawn_and_track` does once the next turn is confirmed
|
||||||
|
// spawned, and then what its own clean end does.
|
||||||
|
state.clear_kill("n");
|
||||||
|
assert_eq!(state.killed_by("n"), None);
|
||||||
|
state.finish_turn("n", &TurnEnd::Complete);
|
||||||
|
assert_eq!(
|
||||||
|
state.killed_by("n"),
|
||||||
|
None,
|
||||||
|
"a turn that finished must not leave the old kill standing"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn the_killed_todo_does_not_read_like_a_completion() {
|
||||||
|
let complete = turn_end_summary("n", &TurnEnd::Complete);
|
||||||
|
let killed = turn_end_summary("n", &TurnEnd::Killed { signal: 9 });
|
||||||
|
assert_eq!(
|
||||||
|
complete, "subagent `n` finished: turn complete",
|
||||||
|
"the ordinary completion todo is unchanged"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
killed.contains("KILLED mid-turn") && killed.contains("SIGKILL (signal 9)"),
|
||||||
|
"the killed todo must state the kill and the signal: {killed}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
!killed.contains("finished"),
|
||||||
|
"the one notification pushed without being asked must not read as a finished turn: \
|
||||||
|
{killed}"
|
||||||
|
);
|
||||||
|
assert_ne!(complete, killed);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn continue_tells_the_caller_it_is_resuming_a_killed_session() {
|
||||||
|
let plain = note_resumed_after_kill("subagent `n` started", None);
|
||||||
|
assert_eq!(
|
||||||
|
plain, "subagent `n` started",
|
||||||
|
"an ordinary resume is unchanged"
|
||||||
|
);
|
||||||
|
let after_kill = note_resumed_after_kill("subagent `n` started", Some(libc::SIGKILL));
|
||||||
|
assert!(
|
||||||
|
after_kill.contains("killed") && after_kill.contains("SIGKILL (signal 9)"),
|
||||||
|
"resuming a killed session is allowed, but the caller has to be told: {after_kill}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn describe_signal_names_the_ones_that_end_a_subagent() {
|
||||||
|
assert_eq!(describe_signal(9), "SIGKILL (signal 9)");
|
||||||
|
assert_eq!(describe_signal(15), "SIGTERM (signal 15)");
|
||||||
|
assert_eq!(describe_signal(2), "SIGINT (signal 2)");
|
||||||
|
assert_eq!(describe_signal(7), "signal 7");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn build_config_sets_cwd_only_when_a_dir_is_given() {
|
fn build_config_sets_cwd_only_when_a_dir_is_given() {
|
||||||
let with = build_config("n", None, None, Some("/tmp/some-worktree"));
|
let with = build_config("n", None, None, Some("/tmp/some-worktree"));
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue