diff --git a/src/core/jsonrpc.rs b/src/core/jsonrpc.rs index 2b87bdcb2..4b4244eab 100644 --- a/src/core/jsonrpc.rs +++ b/src/core/jsonrpc.rs @@ -1882,6 +1882,31 @@ fn register_domain_subscribers( .insert(group) } + // Seed the live tool-execution timeout from the persisted `[agent]` config + // so a user-configured value (Settings → Agent OS access → Action timeout) + // is in effect from the first tool call. `OPENHUMAN_TOOL_TIMEOUT_SECS`, when + // set, still overrides this inside `set_tool_timeout_secs`. This lives on + // the always-on boot path (not `channels::runtime::startup::start_channels`, + // which is skipped for channel-less / web-chat-only cores) so the timeout + // seeds for every install regardless of DomainSet — it is DomainSet- + // independent process-global state, not a gated subscriber (#5027). + // + // Deliberately OUTSIDE the `INFRA: Once` below: `bootstrap_core_runtime` + // re-runs on an in-process core restart (`CoreProcessHandle::restart` → + // `ensure_running`, and `reset_local_data`/Clear-Local-Data) with a freshly + // reloaded `Config`, but `INFRA` is already consumed — so a seed gated by it + // would leave the process-global timeout pinned to the previous config's + // value until Settings is re-saved. `set_tool_timeout_secs` is idempotent + // (a plain atomic store honouring the env override), so re-seeding on every + // call is correct and cheap and re-applies the current config each boot. + let effective_timeout = + crate::openhuman::tool_timeout::set_tool_timeout_secs(config.agent.agent_timeout_secs); + log::debug!( + "[tool_timeout] seeded tool-execution timeout from config: configured={}s effective={}s", + config.agent.agent_timeout_secs, + effective_timeout + ); + // Ungated core/platform infra — health, scheduler-gate, TokenJuice // content-router, session-token seeding, the SessionExpired handler, and // service restart/shutdown. These are DomainSet-independent, so they run diff --git a/src/core/jsonrpc_tests.rs b/src/core/jsonrpc_tests.rs index 8b189229a..50198a032 100644 --- a/src/core/jsonrpc_tests.rs +++ b/src/core/jsonrpc_tests.rs @@ -78,6 +78,57 @@ fn domain_subscriber_plan_harness_gates_by_owning_group() { assert!(!plan.mcp, "harness must skip mcp_registry bus init"); } +/// #5027 — the tool-execution timeout must be seeded on the always-on core boot +/// path (`register_domain_subscribers`), NOT inside +/// `channels::runtime::startup::start_channels`, which is skipped for +/// channel-less / web-chat-only cores (and when `OPENHUMAN_DISABLE_CHANNEL_LISTENERS` +/// is set). A minimal `DomainSet::none()` must still seed, because the seed is +/// DomainSet-independent. +/// +/// The seed sits just *before* the ungated `INFRA: Once` block, so it re-runs on +/// every `register_domain_subscribers` call (each `bootstrap_core_runtime`), +/// re-applying the freshly reloaded config on an in-process restart — a seed gated +/// by the process-global `Once` would only fire on the first boot. `TEST_ENV_LOCK` +/// (via `EnvVarGuard`) serializes with `OPENHUMAN_TOOL_TIMEOUT_SECS` cleared so the +/// operator env override cannot mask the config-derived value. Runs under a tokio +/// runtime like the real boot paths — the INFRA block calls `subscribe_global`, +/// which `tokio::spawn`s when the global bus is already initialized by another test +/// in the binary. +#[tokio::test] +async fn tool_timeout_seeds_on_channelless_core_boot() { + // Clear the operator override behind a panic-safe RAII guard: if any assertion + // below panics, `Drop` still restores the previous value, so sibling tests that + // share `TEST_ENV_LOCK` never inherit the cleared var. + let _env = EnvVarGuard::remove_many(vec!["OPENHUMAN_TOOL_TIMEOUT_SECS"]); + + // Distinctive, in-range (1..=3600) value so the assertion can only pass on a + // real seed, never on the default. Channel-less: `channels_config` stays empty, + // which is exactly the config for which `start_channels` is skipped. + let mut config = crate::openhuman::config::Config::default(); + config.agent.agent_timeout_secs = 1234; + assert!( + config.channels_config.active_channel.is_none(), + "test premise: channel-less config, so start_channels would be skipped" + ); + + let tmp = tempfile::tempdir().expect("tempdir"); + // Minimal DomainSet — INFRA (and thus the timeout seed) is DomainSet-independent, + // so even `none()` must seed. `embedded_core = true` skips the standalone + // process-exit shutdown subscriber. + super::register_domain_subscribers( + tmp.path().to_path_buf(), + config, + true, + crate::core::runtime::DomainSet::none(), + ); + + assert_eq!( + crate::openhuman::tool_timeout::tool_execution_timeout_secs(), + 1234, + "channel-less core boot must seed the tool-execution timeout from [agent].agent_timeout_secs" + ); +} + struct EnvVarGuard { old_values: Vec<(&'static str, Option)>, _lock: MutexGuard<'static, ()>, @@ -99,6 +150,25 @@ impl EnvVarGuard { _lock: lock, } } + + /// Remove the named vars (capturing their prior values) for the guard's + /// lifetime, restoring each on `Drop`. Mirrors [`set_many`] for tests that + /// need an env var *absent* rather than set to a fixed value. + fn remove_many(keys: Vec<&'static str>) -> Self { + let lock = crate::openhuman::config::TEST_ENV_LOCK + .lock() + .expect("test env lock poisoned"); + let mut old_values = Vec::with_capacity(keys.len()); + for key in keys { + let old = std::env::var_os(key); + std::env::remove_var(key); + old_values.push((key, old)); + } + Self { + old_values, + _lock: lock, + } + } } impl Drop for EnvVarGuard { diff --git a/src/openhuman/channels/runtime/startup.rs b/src/openhuman/channels/runtime/startup.rs index 8cd8ecfbf..7e98177ab 100644 --- a/src/openhuman/channels/runtime/startup.rs +++ b/src/openhuman/channels/runtime/startup.rs @@ -156,7 +156,6 @@ pub async fn start_channels(mut config: Config) -> Result<()> { let bus = event_bus::init_global(DEFAULT_CAPACITY); let _tracing_handle = bus.subscribe(Arc::new(TracingSubscriber)); crate::openhuman::health::bus::register_health_subscriber(); - crate::openhuman::skills::bus::register_workflow_cleanup_subscriber(); crate::openhuman::memory_conversations::register_conversation_persistence_subscriber( config.workspace_dir.clone(), ); @@ -276,17 +275,12 @@ pub async fn start_channels(mut config: Config) -> Result<()> { config.workspace_dir.clone(), config.action_dir.clone(), ); - // Seed the live tool-execution timeout from the persisted `[agent]` config so - // a user-configured value (Settings → Agent OS access → Action timeout) is in - // effect from the first tool call. `OPENHUMAN_TOOL_TIMEOUT_SECS`, when set, - // still overrides this inside `set_tool_timeout_secs`. - let effective_timeout = - crate::openhuman::tool_timeout::set_tool_timeout_secs(config.agent.agent_timeout_secs); - tracing::debug!( - configured = config.agent.agent_timeout_secs, - effective = effective_timeout, - "[startup] seeded tool-execution timeout from config" - ); + // NOTE: the live tool-execution timeout seed is done in + // `core::jsonrpc::register_domain_subscribers` (unconditional core boot), NOT + // here — `start_channels` is skipped when no channel is configured or + // `OPENHUMAN_DISABLE_CHANNEL_LISTENERS` is set, which would otherwise leave + // channel-less / web-chat-only cores running the default timeout instead of the + // user-configured `[agent].agent_timeout_secs` (#5027). // Phase 1 of #1401: audit logger is wired with defaults so emission paths // are exercised at runtime. A follow-up promotes `SecurityConfig` (and // therefore the `audit` knob) onto the runtime `Config` schema so users diff --git a/src/openhuman/skills/bus.rs b/src/openhuman/skills/bus.rs index 7ea4f84f0..73d551eb8 100644 --- a/src/openhuman/skills/bus.rs +++ b/src/openhuman/skills/bus.rs @@ -255,10 +255,6 @@ pub fn ensure_triggered_workflow_subscriber(workspace: &std::path::Path) { }); } -/// Legacy no-op retained while call-sites migrate to -/// [`register_triggered_workflow_subscriber`]. Safe to call multiple times. -pub fn register_workflow_cleanup_subscriber() {} - #[cfg(test)] mod tests { use super::*; @@ -446,10 +442,4 @@ mod tests { }; assert!(idx.matching_workflows(&event).is_empty()); } - - #[test] - fn register_skill_cleanup_subscriber_is_a_safe_noop() { - register_workflow_cleanup_subscriber(); - register_workflow_cleanup_subscriber(); - } } diff --git a/src/openhuman/skills/stub.rs b/src/openhuman/skills/stub.rs index a25092319..c09d9b9bc 100644 --- a/src/openhuman/skills/stub.rs +++ b/src/openhuman/skills/stub.rs @@ -82,7 +82,7 @@ pub mod registry { } // --------------------------------------------------------------------------- -// bus::{ensure_triggered_workflow_subscriber, register_workflow_cleanup_subscriber} +// bus::ensure_triggered_workflow_subscriber // --------------------------------------------------------------------------- pub mod bus { @@ -90,9 +90,6 @@ pub mod bus { pub fn ensure_triggered_workflow_subscriber(_workspace: &std::path::Path) { log::debug!("[skills-stub] ensure_triggered_workflow_subscriber skipped (skills disabled)"); } - - /// No-op: no skill run directories exist to clean up. - pub fn register_workflow_cleanup_subscriber() {} } // NOTE: no `tools` module here. The `pub use skills::tools::*` glob in diff --git a/src/openhuman/tool_timeout/README.md b/src/openhuman/tool_timeout/README.md index 3f3e0f091..a6a343942 100644 --- a/src/openhuman/tool_timeout/README.md +++ b/src/openhuman/tool_timeout/README.md @@ -7,7 +7,7 @@ Process-wide wall-clock timeout policy for tool execution (the node/tool runtime Highest precedence first: 1. `OPENHUMAN_TOOL_TIMEOUT_SECS` environment variable — operator override. When set to a valid value (`1..=3600`) it always wins; config pushes are ignored while it is present. -2. The persisted config value (`[agent].agent_timeout_secs`), pushed in via `set_tool_timeout_secs` at startup and on every `config.update_agent_settings` RPC. +2. The persisted config value (`[agent].agent_timeout_secs`), pushed in via `set_tool_timeout_secs` at startup (from `core::jsonrpc::register_domain_subscribers`, the always-on core boot path) and on every `config.update_agent_settings` RPC. 3. The built-in `DEFAULT_TIMEOUT_SECS` (`120`) default. ## Responsibilities @@ -54,7 +54,7 @@ The global timeout governs **non-scripting** tools only — a hung network/MCP c - `src/openhuman/tools/impl/system/{shell,node_exec,npm_exec}.rs` — scripting tools: unbounded by default, explicit `timeout_secs` via `explicit_call_timeout_*`. - `src/openhuman/agent/tools/delegate.rs` — bounds the delegated provider chat call with `tool_execution_timeout_secs`. - `src/openhuman/config/ops.rs` — `apply_agent_settings` calls `set_tool_timeout_secs` after persisting; `get_agent_settings` reports `effective_timeout_secs` / `env_override`. -- `src/openhuman/channels/runtime/startup.rs` — seeds the runtime value from config at core boot. +- `src/core/jsonrpc.rs` — `register_domain_subscribers` seeds the runtime value from config on the always-on core boot path (its ungated `INFRA: Once` block), so channel-less / web-chat-only cores get the configured timeout too (#5027). - `src/openhuman/agent/harness/harness_gap_tests.rs` — pins `parse_tool_timeout_secs` default/boundary behaviour. ## Notes / gotchas