diff --git a/crates/openfang-hands/src/registry.rs b/crates/openfang-hands/src/registry.rs index f2e73e66dd..b3ceb12f88 100644 --- a/crates/openfang-hands/src/registry.rs +++ b/crates/openfang-hands/src/registry.rs @@ -110,6 +110,12 @@ impl HandRegistry { .map(|e| { serde_json::json!({ "hand_id": e.hand_id, + // Without the name, a named instance is restored as an + // unnamed one: it takes the legacy agent id derived from + // hand_id, is renamed to the hand's default, and lands in a + // different workspace — orphaning the context.md and files + // the original instance had accumulated. + "instance_name": e.instance_name, "config": e.config, "agent_id": e.agent_id, }) @@ -123,12 +129,24 @@ impl HandRegistry { } /// Load persisted hand state and re-activate hands. - /// Returns list of (hand_id, config, old_agent_id) that should be activated. - /// The `old_agent_id` is the agent UUID from before the restart, used to + /// + /// Returns `(hand_id, instance_name, config, old_agent_id)` for each hand + /// that should be activated. `instance_name` must be carried through: + /// activating without it derives a different agent id and a different agent + /// name, so a named instance would not survive a restart. It is optional in + /// the file so state written before this field existed still loads. + /// + /// `old_agent_id` is the agent UUID from before the restart, used to /// reassign cron jobs to the newly spawned agent (issue #402). + #[allow(clippy::type_complexity)] pub fn load_state( path: &std::path::Path, - ) -> Vec<(String, HashMap, Option)> { + ) -> Vec<( + String, + Option, + HashMap, + Option, + )> { let data = match std::fs::read_to_string(path) { Ok(d) => d, Err(_) => return Vec::new(), @@ -144,12 +162,16 @@ impl HandRegistry { .into_iter() .filter_map(|e| { let hand_id = e["hand_id"].as_str()?.to_string(); + let instance_name = e + .get("instance_name") + .and_then(|v| v.as_str()) + .map(str::to_string); let config: HashMap = serde_json::from_value(e["config"].clone()).unwrap_or_default(); let old_agent_id: Option = e .get("agent_id") .and_then(|v| serde_json::from_value(v.clone()).ok()); - Some((hand_id, config, old_agent_id)) + Some((hand_id, instance_name, config, old_agent_id)) }) .collect() } @@ -1355,11 +1377,61 @@ system_prompt = "v2 — schedule changed." reg.persist_state(&state_file).unwrap(); let reloaded = HandRegistry::load_state(&state_file); assert_eq!(reloaded.len(), 1); - let (hand_id, config, _agent_id) = &reloaded[0]; + let (hand_id, _instance_name, config, _agent_id) = &reloaded[0]; assert_eq!(hand_id, "browser"); assert_eq!( config.get("headless"), Some(&serde_json::Value::String("true".into())) ); } + + /// A named instance must come back named. + /// + /// The name decides the agent id (`hand_instance_` vs the legacy + /// id derived from `hand_id`), the agent name, and therefore the workspace + /// directory. Dropping it on restore silently re-homed the instance and + /// orphaned everything it had accumulated there. + #[test] + fn test_instance_name_survives_persist_reload() { + let reg = test_registry_with_dummy_hand("collector"); + let inst = reg + .activate("collector", HashMap::new(), Some("underground".into())) + .unwrap(); + assert_eq!(inst.instance_name.as_deref(), Some("underground")); + + let tmp = tempfile::tempdir().unwrap(); + let state_file = tmp.path().join("hands.json"); + reg.persist_state(&state_file).unwrap(); + + let reloaded = HandRegistry::load_state(&state_file); + assert_eq!(reloaded.len(), 1); + let (hand_id, instance_name, _config, _agent_id) = &reloaded[0]; + assert_eq!(hand_id, "collector"); + assert_eq!( + instance_name.as_deref(), + Some("underground"), + "named instance must be restored under the same name" + ); + } + + /// State written before `instance_name` existed must still load. + #[test] + fn test_load_state_without_instance_name_is_unnamed() { + let tmp = tempfile::tempdir().unwrap(); + let state_file = tmp.path().join("hands.json"); + std::fs::write( + &state_file, + r#"[{"hand_id":"collector","config":{},"agent_id":null}]"#, + ) + .unwrap(); + + let reloaded = HandRegistry::load_state(&state_file); + assert_eq!(reloaded.len(), 1); + let (hand_id, instance_name, _config, _agent_id) = &reloaded[0]; + assert_eq!(hand_id, "collector"); + assert!( + instance_name.is_none(), + "legacy state has no name and must load as unnamed" + ); + } } diff --git a/crates/openfang-kernel/src/heartbeat.rs b/crates/openfang-kernel/src/heartbeat.rs index ad76319376..baf4ddaa24 100644 --- a/crates/openfang-kernel/src/heartbeat.rs +++ b/crates/openfang-kernel/src/heartbeat.rs @@ -22,6 +22,17 @@ const DEFAULT_CHECK_INTERVAL_SECS: u64 = 30; /// multiples of its heartbeat interval. const UNRESPONSIVE_MULTIPLIER: u64 = 2; +/// Seconds of inactivity before a Running agent is presumed unresponsive. +/// +/// Must not be shorter than the longest a legitimate turn may take, or live +/// work gets killed and restarted: an agent still inside its turn budget is +/// waiting, not hung. `kernel::DEFAULT_AGENT_TURN_TIMEOUT_SECS` is 600, and +/// per-job `timeout_secs` is bounded by `scheduler::MAX_TIMEOUT_SECS` (3600), +/// so 600 matches the default turn and the turn timeout remains the mechanism +/// that ends an over-running turn. This value only decides when to call an +/// agent *dead*. +const DEFAULT_UNRESPONSIVE_TIMEOUT_SECS: u64 = 600; + /// Default maximum recovery attempts before giving up. const DEFAULT_MAX_RECOVERY_ATTEMPTS: u32 = 3; @@ -61,8 +72,7 @@ impl Default for HeartbeatConfig { fn default() -> Self { Self { check_interval_secs: DEFAULT_CHECK_INTERVAL_SECS, - // 180s default: browser tasks and complex LLM calls can take 1-3 minutes - default_timeout_secs: 180, + default_timeout_secs: DEFAULT_UNRESPONSIVE_TIMEOUT_SECS, max_recovery_attempts: DEFAULT_MAX_RECOVERY_ATTEMPTS, recovery_cooldown_secs: DEFAULT_RECOVERY_COOLDOWN_SECS, } @@ -157,12 +167,28 @@ pub fn check_agents(registry: &AgentRegistry, config: &HeartbeatConfig) -> Vec> created_at) but has gone // silent for longer than the timeout — should be flagged unresponsive. let registry = crate::registry::AgentRegistry::new(); - let ten_min_ago = Utc::now() - Duration::seconds(600); - let five_min_ago = Utc::now() - Duration::seconds(300); - let active_agent = make_entry( - "active-agent", - AgentState::Running, - ten_min_ago, - five_min_ago, - ); + // Derive the timings from the threshold rather than hardcoding, so + // this keeps testing "past the timeout" if the default ever moves. + let timeout = DEFAULT_UNRESPONSIVE_TIMEOUT_SECS as i64; + let created = Utc::now() - Duration::seconds(timeout * 3); + let last_active = Utc::now() - Duration::seconds(timeout + 60); + let active_agent = make_entry("active-agent", AgentState::Running, created, last_active); registry.register(active_agent).unwrap(); - let config = HeartbeatConfig::default(); // timeout = 180s, inactive = ~300s + let config = HeartbeatConfig::default(); let statuses = check_agents(®istry, &config); assert_eq!(statuses.len(), 1); @@ -489,7 +513,7 @@ mod tests { let healthy_agent = make_entry("healthy-agent", AgentState::Running, ten_min_ago, just_now); registry.register(healthy_agent).unwrap(); - let config = HeartbeatConfig::default(); // timeout = 180s + let config = HeartbeatConfig::default(); let statuses = check_agents(®istry, &config); assert_eq!(statuses.len(), 1); @@ -544,7 +568,34 @@ mod tests { fn test_heartbeat_config_default() { let config = HeartbeatConfig::default(); assert_eq!(config.check_interval_secs, 30); - assert_eq!(config.default_timeout_secs, 180); + assert_eq!( + config.default_timeout_secs, + DEFAULT_UNRESPONSIVE_TIMEOUT_SECS + ); + } + + /// An autonomous config may only grant an agent MORE time before it is + /// presumed dead, never less. + /// + /// Read literally, `heartbeat_interval_secs * UNRESPONSIVE_MULTIPLIER` is + /// 60s for the default 30s interval — tighter than the default timeout, so + /// an agent waiting on a slow model was declared crashed and had its + /// in-flight turn restarted every 60 seconds. + #[test] + fn test_autonomous_config_never_shortens_timeout() { + let config = HeartbeatConfig::default(); + let effective = + |interval: u64| (interval * UNRESPONSIVE_MULTIPLIER).max(config.default_timeout_secs); + + // The default 30s interval must not drag the bound below the default. + assert_eq!(effective(30), config.default_timeout_secs); + assert!( + effective(30) >= config.default_timeout_secs, + "a short heartbeat interval must not shorten the death threshold" + ); + + // A generous interval still raises it. + assert_eq!(effective(600), 1200); } #[test] diff --git a/crates/openfang-kernel/src/kernel.rs b/crates/openfang-kernel/src/kernel.rs index f4025b416a..084c584d7d 100644 --- a/crates/openfang-kernel/src/kernel.rs +++ b/crates/openfang-kernel/src/kernel.rs @@ -4414,8 +4414,13 @@ impl OpenFangKernel { let saved_hands = openfang_hands::registry::HandRegistry::load_state(&state_path); if !saved_hands.is_empty() { info!("Restoring {} persisted hand(s)", saved_hands.len()); - for (hand_id, config, old_agent_id) in saved_hands { - match self.activate_hand(&hand_id, config, None) { + for (hand_id, instance_name, config, old_agent_id) in saved_hands { + // Restore under the SAME instance name. Passing None here made + // every named instance come back as the unnamed one: a + // different agent id, the hand's default agent name, and a + // different workspace — so its context.md and accumulated files + // were orphaned on every restart. + match self.activate_hand(&hand_id, config, instance_name) { Ok(inst) => { info!(hand = %hand_id, instance = %inst.instance_id, "Hand restored"); // Reassign cron jobs and triggers from the pre-restart