From b3011b97c341ed210f6600c47deba759fca45b4c Mon Sep 17 00:00:00 2001 From: openfang Date: Wed, 29 Jul 2026 05:48:33 +0000 Subject: [PATCH 1/2] fix(kernel): stop declaring agents dead while they wait on the model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An autonomous config could make the liveness bound TIGHTER than the default, and did so for every agent using the default 30s heartbeat interval: `30 * UNRESPONSIVE_MULTIPLIER` is 60s, against a default of 180s and a turn timeout of 600s. Observed on a collector agent waiting on a rate-limited reasoning model, flapping on an exact 60-second cycle: 05:37:48 Unresponsive Running agent marked as Crashed inactive_secs=89 05:38:48 Agent recovered successfully 05:39:48 Unresponsive Running agent marked as Crashed inactive_secs=89 05:40:48 Agent recovered successfully 05:41:48 Unresponsive Running agent marked as Crashed inactive_secs=77 Each "recovery" restarts the agent loop and discards the in-flight turn, so a sweep slower than 60s could never finish — it was killed and restarted indefinitely. Waiting on the model is not being hung. Two changes: * The per-agent value now only ever RAISES the bound — `max()` against the default — so a short heartbeat interval cannot shorten the death threshold. The interval describes how often an agent checks in, not how long a turn may legitimately take. * The default moves 180 -> 600 to match DEFAULT_AGENT_TURN_TIMEOUT_SECS. An agent still inside its turn budget is by definition not dead, and the turn timeout remains the mechanism that ends an over-running turn; this value only decides when to call an agent dead. The old 180s already conceded that "browser tasks and complex LLM calls can take 1-3 minutes" — it was simply short of what a full turn is allowed. test_active_agent_detected_unresponsive derived its timings from the old 180s bound, so it now derives them from the constant instead and keeps testing "past the timeout" if the default moves again. Co-Authored-By: Claude Opus 5 --- crates/openfang-kernel/src/heartbeat.rs | 83 ++++++++++++++++++++----- 1 file changed, 67 insertions(+), 16 deletions(-) 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] From ff5319a434cc3194d73c327e798d710d1dd561ef Mon Sep 17 00:00:00 2001 From: openfang Date: Wed, 29 Jul 2026 18:49:54 +0000 Subject: [PATCH 2/2] fix(hands): keep a named hand instance named across a restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `persist_state` never wrote `instance_name`, `load_state` never returned it, and the restore path passed `None`: match self.activate_hand(&hand_id, config, None) { So every named instance came back as the unnamed one. The name decides the agent id — `hand_instance_` versus the legacy id derived from `hand_id` — and the agent name, and therefore the workspace directory. A collector activated as "underground" returned after a deploy as "collector-hand" with a different agent id, in a different workspace, leaving its context.md and accumulated output orphaned. The original agent survived as a stray with no hand registration, so the box ended up running two collectors against one knowledge graph. It also defeated the cron reassignment immediately below it: jobs were migrated onto the wrong agent, so a tuned schedule (cadence, timeout) silently reverted to the hand's defaults on every restart. Persist the name, return it, and restore under it. `instance_name` is read with `.get()` so state written before this field existed still loads, as unnamed — matching today's behaviour for those entries. Two tests: a named instance survives persist+reload, and legacy state without the field loads as unnamed. Co-Authored-By: Claude Opus 5 --- crates/openfang-hands/src/registry.rs | 82 +++++++++++++++++++++++++-- crates/openfang-kernel/src/kernel.rs | 9 ++- 2 files changed, 84 insertions(+), 7 deletions(-) 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/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