hive-runtime: respawn the ACP agent when its provider key changed after a failed turn
The provider key was resolved once, at spawn. A provider rejecting it shows only as an empty end_turn or an idle timeout, neither of which drops the agent, so a key rotated in the store never reached a running opencode until the process died for another reason. After a turn fails with EmptyEndTurn or IdleTimeout, the runtime now resolves the key once more. When that yields a key that differs from the one the agent was spawned with, the agent is dropped (SIGINT to its process group, as every other respawn) and the next turn spawns it with the new key and reloads the recorded session. No key read (inherited env value, no store, a failed or timed-out read, nothing stored) keeps the running agent. The failed turn's error is returned unchanged. Part of #5017 (slice A).
This commit is contained in:
parent
16e7a41bbd
commit
fa56931a99
1 changed files with 157 additions and 2 deletions
|
|
@ -22,6 +22,7 @@ use tokio::sync::futures::Notified;
|
|||
use tokio::sync::{Mutex, Notify};
|
||||
use tokio::time::Instant;
|
||||
|
||||
use self::provider_key::KeyVar;
|
||||
use self::rpc::{Connection, Incoming};
|
||||
use self::stream::StreamMapper;
|
||||
use crate::spec::AcpCommand;
|
||||
|
|
@ -356,11 +357,14 @@ pub struct AcpRuntime<P: CompactionPolicy> {
|
|||
compact_idle: Duration,
|
||||
choices: Choices,
|
||||
spend: Spend,
|
||||
resolve_key: provider_key::Resolve,
|
||||
}
|
||||
|
||||
/// The running agent process and the session loaded into it.
|
||||
struct Live {
|
||||
conn: Connection,
|
||||
/// The provider key added to the agent's environment at spawn.
|
||||
key: Option<KeyVar>,
|
||||
load_session: bool,
|
||||
/// The recorded session, or a new one whose first prompt is unanswered.
|
||||
loaded: Option<String>,
|
||||
|
|
@ -407,6 +411,7 @@ impl<P: CompactionPolicy> AcpRuntime<P> {
|
|||
compact_idle: COMPACT_IDLE,
|
||||
choices: Choices::default(),
|
||||
spend: Spend::default(),
|
||||
resolve_key: provider_key::resolver(),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -422,7 +427,7 @@ impl<P: CompactionPolicy> AcpRuntime<P> {
|
|||
let (_, servers) = mcp_servers(config)?;
|
||||
let cwd = session_cwd(config);
|
||||
let key = match &self.command.api_key_env {
|
||||
Some(name) => provider_key::resolve(name).await,
|
||||
Some(name) => (self.resolve_key)(name.clone()).await,
|
||||
None => None,
|
||||
};
|
||||
let conn = Connection::spawn(
|
||||
|
|
@ -453,6 +458,7 @@ impl<P: CompactionPolicy> AcpRuntime<P> {
|
|||
self.choices.reset();
|
||||
Ok(Live {
|
||||
conn,
|
||||
key,
|
||||
load_session: caps["loadSession"] == Value::Bool(true),
|
||||
loaded: None,
|
||||
model: None,
|
||||
|
|
@ -687,9 +693,34 @@ impl<P: CompactionPolicy> AcpRuntime<P> {
|
|||
{
|
||||
*guard = None;
|
||||
}
|
||||
if let Err(Error::Acp(AcpError::EmptyEndTurn | AcpError::IdleTimeout { .. })) = &result
|
||||
&& let Some(live) = guard.as_ref()
|
||||
&& self.key_changed(live).await
|
||||
{
|
||||
tracing::warn!(
|
||||
"the ACP provider key in the store differs from the one the agent was spawned \
|
||||
with; respawning the agent"
|
||||
);
|
||||
*guard = None;
|
||||
}
|
||||
result
|
||||
}
|
||||
|
||||
/// Whether the store holds a provider key other than the one `live` was
|
||||
/// spawned with. No key read is no change, so a store that cannot be read
|
||||
/// never takes away the key the agent has.
|
||||
async fn key_changed(&self, live: &Live) -> bool {
|
||||
let Some(name) = &self.command.api_key_env else {
|
||||
return false;
|
||||
};
|
||||
let Some(key) = (self.resolve_key)(name.clone()).await else {
|
||||
return false;
|
||||
};
|
||||
live.key
|
||||
.as_ref()
|
||||
.is_none_or(|spawned| spawned.value != key.value)
|
||||
}
|
||||
|
||||
/// Compact the recorded session, if there is one. `checkpoint` runs the
|
||||
/// policy's checkpoint turn before an advertised `compact` command too;
|
||||
/// without one it always runs, since archiving keeps nothing of the
|
||||
|
|
@ -1024,11 +1055,14 @@ mod tests {
|
|||
use std::collections::BTreeMap;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
||||
use std::time::Duration;
|
||||
|
||||
use hive_claude::{Config, NoopSink, PercentPolicy, Sink};
|
||||
|
||||
use super::{AcpError, AcpRuntime, Choice, ReportedCost, SessionChoices, Spend, TurnKind};
|
||||
use super::{
|
||||
AcpError, AcpRuntime, Choice, KeyVar, ReportedCost, SessionChoices, Spend, TurnKind,
|
||||
};
|
||||
use crate::{AcpCommand, Error, Runtime, TurnStop};
|
||||
|
||||
/// An ACP agent in plain `sh`. It answers `initialize`, numbers its
|
||||
|
|
@ -1064,6 +1098,10 @@ mod tests {
|
|||
const AGENT: &str = r#"
|
||||
n=0 prompt= model=m/think effort=low
|
||||
printf 'start\n' >> "$1.methods"
|
||||
# With TEST_PROVIDER_KEY set: its value at each start, in <log>.keys.
|
||||
if [ -n "$TEST_PROVIDER_KEY" ]; then
|
||||
printf '%s\n' "$TEST_PROVIDER_KEY" >> "$1.keys"
|
||||
fi
|
||||
# With TRAP_INT set: SIGINT creates <log>.int and exits 0.
|
||||
if [ -n "$TRAP_INT" ]; then
|
||||
trap ': > "$1.int"; exit 0' INT
|
||||
|
|
@ -1386,6 +1424,102 @@ done
|
|||
);
|
||||
}
|
||||
|
||||
/// A runtime whose agent gets its provider key in `TEST_PROVIDER_KEY`,
|
||||
/// resolved to `answers` in turn, the last one for every read after it.
|
||||
/// Returns how many reads were made.
|
||||
fn keyed(
|
||||
dir: &Path,
|
||||
mode: &str,
|
||||
answers: &'static [Option<&'static str>],
|
||||
) -> (AcpRuntime<PercentPolicy>, Arc<AtomicUsize>) {
|
||||
let mut runtime = runtime(dir, mode);
|
||||
runtime.command.api_key_env = Some("TEST_PROVIDER_KEY".to_owned());
|
||||
let reads = Arc::new(AtomicUsize::new(0));
|
||||
let counted = reads.clone();
|
||||
runtime.resolve_key = Arc::new(move |name| {
|
||||
let read = counted.fetch_add(1, Ordering::SeqCst);
|
||||
let answer = answers[read.min(answers.len() - 1)];
|
||||
Box::pin(async move {
|
||||
answer.map(|value| KeyVar {
|
||||
name,
|
||||
value: value.to_owned(),
|
||||
})
|
||||
})
|
||||
});
|
||||
(runtime, reads)
|
||||
}
|
||||
|
||||
/// The provider key each agent start was given.
|
||||
fn keys(dir: &Path) -> Vec<String> {
|
||||
std::fs::read_to_string(dir.join("prompts.keys"))
|
||||
.unwrap()
|
||||
.lines()
|
||||
.map(str::to_owned)
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn a_changed_key_after_an_empty_turn_respawns_the_agent_with_it() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let (runtime, reads) = keyed(dir.path(), "blank", &[Some("old"), Some("new")]);
|
||||
let config = config(dir.path());
|
||||
|
||||
let empty = runtime.run(&config, "one", &NoopSink).await;
|
||||
assert!(
|
||||
matches!(empty, Err(Error::Acp(AcpError::EmptyEndTurn))),
|
||||
"{empty:?}"
|
||||
);
|
||||
// Read at spawn and once after the failed turn.
|
||||
assert_eq!(reads.load(Ordering::SeqCst), 2);
|
||||
|
||||
runtime.run(&config, "two", &NoopSink).await.unwrap();
|
||||
assert_eq!(keys(dir.path()), ["old", "new"]);
|
||||
// The respawned agent carries on with the same session.
|
||||
assert_eq!(count(dir.path(), "session/new"), 1);
|
||||
assert_eq!(count(dir.path(), "session/load"), 1);
|
||||
assert_eq!(recorded(dir.path()), "s1");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn a_changed_key_after_an_idle_timeout_respawns_the_agent_with_it() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let (runtime, _) = keyed(dir.path(), "silent", &[Some("old"), Some("new")]);
|
||||
let config = idle_config(dir.path(), 300);
|
||||
|
||||
let stalled = runtime.run(&config, "one", &NoopSink).await;
|
||||
assert!(
|
||||
matches!(stalled, Err(Error::Acp(AcpError::IdleTimeout { .. }))),
|
||||
"{stalled:?}"
|
||||
);
|
||||
runtime.run(&config, "two", &NoopSink).await.unwrap();
|
||||
assert_eq!(keys(dir.path()), ["old", "new"]);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn an_unchanged_key_after_an_empty_turn_keeps_the_agent() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let (runtime, reads) = keyed(dir.path(), "blank", &[Some("same")]);
|
||||
let config = config(dir.path());
|
||||
|
||||
assert!(runtime.run(&config, "one", &NoopSink).await.is_err());
|
||||
runtime.run(&config, "two", &NoopSink).await.unwrap();
|
||||
assert_eq!(count(dir.path(), "start"), 1);
|
||||
assert_eq!(keys(dir.path()), ["same"]);
|
||||
assert_eq!(reads.load(Ordering::SeqCst), 2);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn no_key_read_after_an_empty_turn_keeps_the_agent_and_its_key() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let (runtime, _) = keyed(dir.path(), "blank", &[Some("old"), None]);
|
||||
let config = config(dir.path());
|
||||
|
||||
assert!(runtime.run(&config, "one", &NoopSink).await.is_err());
|
||||
runtime.run(&config, "two", &NoopSink).await.unwrap();
|
||||
assert_eq!(count(dir.path(), "start"), 1);
|
||||
assert_eq!(keys(dir.path()), ["old"]);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn an_end_turn_with_output_but_no_usage_succeeds() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
|
|
|
|||
Loading…
Reference in a new issue