fix(core): seed tool-execution timeout on the always-on boot path (#5027) (#5078)

This commit is contained in:
oxoxDev
2026-07-21 20:10:08 +03:00
committed by GitHub
parent 86e1a4c524
commit e2f9014575
6 changed files with 104 additions and 28 deletions
+25
View File
@@ -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
+70
View File
@@ -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<OsString>)>,
_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 {
+6 -12
View File
@@ -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
-10
View File
@@ -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();
}
}
+1 -4
View File
@@ -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
+2 -2
View File
@@ -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