Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/core/jsonrpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
70 changes: 70 additions & 0 deletions src/core/jsonrpc_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, ()>,
Expand All @@ -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 {
Expand Down
18 changes: 6 additions & 12 deletions src/openhuman/channels/runtime/startup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
);
Expand Down Expand Up @@ -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
Expand Down
10 changes: 0 additions & 10 deletions src/openhuman/skills/bus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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();
}
}
5 changes: 1 addition & 4 deletions src/openhuman/skills/stub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,14 @@ pub mod registry {
}

// ---------------------------------------------------------------------------
// bus::{ensure_triggered_workflow_subscriber, register_workflow_cleanup_subscriber}
// bus::ensure_triggered_workflow_subscriber
// ---------------------------------------------------------------------------

pub mod bus {
/// No-op: there are no triggered skills to subscribe to the event bus for.
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
Expand Down
4 changes: 2 additions & 2 deletions src/openhuman/tool_timeout/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading