fix(core): seed tool-execution timeout on the always-on boot path (#5027)#5078
Conversation
…mansai#5027) Move the [agent].agent_timeout_secs seed into the ungated INFRA: Once block of register_domain_subscribers, so channel-less / web-chat-only cores (where start_channels is skipped) still honour the configured tool-execution timeout from the first tool call. The seed is DomainSet-independent, so even a minimal DomainSet::none() boot applies it. Add a channel-less regression test.
…umansai#5027) The seed now lives on the always-on core boot path in core::jsonrpc::register_domain_subscribers (previous commit). Leave a tinyhumansai#5003-style NOTE pointing at the new home and explaining why start_channels is the wrong place (skipped for channel-less cores).
…-op (tinyhumansai#5027) The legacy no-op has been superseded by ensure_triggered_workflow_subscriber (already registered on the always-on path in core::jsonrpc). Delete its call in start_channels, the empty fn + safe-noop test in skills::bus, and the stub in skills::stub. Sweep the tool_timeout README to name the seed's new home (core::jsonrpc::register_domain_subscribers).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCore boot now seeds the global tool-execution timeout from persisted configuration, including channel-less cores. Channel startup no longer performs that seed or registers the legacy workflow-cleanup subscriber, whose APIs and no-op test were removed. ChangesCore boot initialization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11394080f0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let effective_timeout = | ||
| crate::openhuman::tool_timeout::set_tool_timeout_secs(config.agent.agent_timeout_secs); |
There was a problem hiding this comment.
When the desktop core is bounced in the same Tauri process (checked CoreProcessHandle::restart and reset_local_data, both stop the task and then call ensure_running), this INFRA Once is already consumed, so the restarted core never applies the newly loaded [agent].agent_timeout_secs. For example, after Clear Local Data deletes the old config and restarts into defaults, the process-global tool timeout can remain at the previous user's value until Settings is opened/saved; set_tool_timeout_secs is idempotent, so it should run on each bootstrap rather than only inside this Once.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — valid. `bootstrap_core_runtime` does re-run on an in-process restart (`CoreProcessHandle::restart` → `ensure_running`, and `reset_local_data`) with a freshly reloaded `Config`, but the `INFRA: Once` is already consumed, so the seed never re-fired. Moved `set_tool_timeout_secs` out of the `Once` in `e61aaed8c` — it now re-seeds on every `register_domain_subscribers` call (idempotent atomic store), so a restart re-applies the current config.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/jsonrpc_tests.rs`:
- Around line 98-103: Update the test setup around TEST_ENV_LOCK and
OPENHUMAN_TOOL_TIMEOUT_SECS to use an RAII or equivalent deferred-cleanup guard
that captures the original value, removes it for the test, and restores or
removes it on scope exit. Ensure cleanup runs even if setup, boot registration,
or assertions panic, including the corresponding restoration logic near the
additional referenced section.
- Around line 81-95: Update the test setup around the
OPENHUMAN_TOOL_TIMEOUT_SECS environment variable to use the existing RAII
environment guard pattern instead of manual cleanup. Ensure the guard captures
and restores the previous value even when the assertion panics, while preserving
the TEST_ENV_LOCK serialization and config-derived timeout behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79e97ce5-1a14-4115-8fd2-7e2dd78ccf30
📒 Files selected for processing (6)
src/core/jsonrpc.rssrc/core/jsonrpc_tests.rssrc/openhuman/channels/runtime/startup.rssrc/openhuman/skills/bus.rssrc/openhuman/skills/stub.rssrc/openhuman/tool_timeout/README.md
💤 Files with no reviewable changes (1)
- src/openhuman/skills/bus.rs
…mansai#5027) Move the tool-execution-timeout seed out of the process-global INFRA:Once block in register_domain_subscribers. 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 the Once is already consumed, so the previous config's timeout stayed pinned until Settings was re-saved. set_tool_timeout_secs is idempotent (atomic store honouring the env override), so re-seeding on every call re-applies the current config each boot at no cost.
…humansai#5027) Replace the channelless-boot seed test's manual TEST_ENV_LOCK acquisition + end-of-test env restore with an EnvVarGuard (new remove_many) so a panicking assertion still restores OPENHUMAN_TOOL_TIMEOUT_SECS on unwind — a shared-lock sibling can no longer inherit the cleared var. Refresh the doc comment to match the seed now living just before INFRA:Once (re-runs every boot), not inside it.
|
Addressed both reviews (pushed `e61aaed8c` + `849c4767b`):
Gates: `cargo check`/`fmt --check`/`clippy -D warnings` clean, gates-off `--no-default-features --features tokenjuice-treesitter` clean, `core::jsonrpc` + `openhuman::tool_timeout` tests green (`tool_timeout_seeds_on_channelless_core_boot` passes). @coderabbitai review |
|
✅ Action performedReview finished.
|
2 participants
Summary
[agent].agent_timeout_secstakes effect on every boot — not only when a chat channel is connected.register_workflow_cleanup_subscriber) whose real successor is already registered on the always-on path.Problem
start_channelsis skipped entirely when no chat integration is configured (spawn_channels_serviceearly-returns on!has_listening_integrations(), and also underOPENHUMAN_DISABLE_CHANNEL_LISTENERS=1). Two things were still wired inside it:set_tool_timeout_secs. On channel-less installs the persisted[agent].agent_timeout_secswas silently ignored and tools ran on the built-in default until a Settings save happened to re-seed it.register_workflow_cleanup_subscriber— a documented legacy no-op (pub fn …() {}); its successorensure_triggered_workflow_subscriberis already registered on the always-on boot path. Moving a no-op would only mislead readers.Same coupling class as #5003 (learning subscribers), which this file already carries a NOTE about.
Solution
INFRA: Onceblock ofregister_domain_subscribers, right aftertokenjuice::install_from_config. That block is the file's home for config-derived process-global boot seeds, isDomainSet-independent (so the timeout seeds even underharness()/none()), runs exactly once withconfigin scope, and runs earlier thanstart_channelsdid — so the configured timeout is live strictly sooner.start_channels, leaving a fix(learning): email-signature subscriber + rebuild trigger silently depend on channels being configured #5003-style NOTE pointing at the new home and why.pub fn, its doc, its_is_a_safe_nooptest, and the matching stub fn (keepingensure_triggered_workflow_subscriber). Post-changegit grep register_workflow_cleanup_subscriberis empty.tool_timeout/README.mdnow namescore::jsonrpc::register_domain_subscribersas the seed site.Submission Checklist
tool_timeout_seeds_on_channelless_core_boot— channel-lessConfig(agent_timeout_secs = 1234), minimalDomainSet::none(), asserts the effective timeout is seeded (the failure the fix targets: without the move, a channel-less boot leaves the default). ENV_LOCK-serialized,OPENHUMAN_TOOL_TIMEOUT_SECScleared.## Related.Closes #5027in## Related.Impact
Once; the settings-update RPC path still re-seeds on change. Verified:cargo check(default + gates-off),fmt/clippy -D warningsclean,tool_timeout14 passed,core::jsonrpc94 passed (incl. new test),skills::bus16 passed.Related
startup.rs/skillsvs feat(core): channels compile-time feature gate (#4801) — completes epic #4795 #5029's channels cfg); not stacked. Landing this first makes feat(core): channels compile-time feature gate (#4801) — completes epic #4795 #5029's channels-off build correct for free (the seed lives in always-compiled code rather than being compiled out).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/5027-tool-timeout-seed-decouple11394080fValidation Run
pnpm typecheck/format:check— no TypeScript or frontend files changedcargo fmt --checkclean;cargo check(default) clean;cargo clippy -- -D warningscleanGGML_NATIVE=OFF cargo check --no-default-features --features tokenjuice-treesittercargo test --lib openhuman::tool_timeout(14) +core::jsonrpc(94, incl. new) +skills::bus(16)Behavior Changes
[agent].agent_timeout_secsis honoured from the first tool call.Parity Contract
register_workflow_cleanup_subscriberwas a no-op; its successorensure_triggered_workflow_subscriberremains registered on the always-on path.Duplicate / Superseded PR Handling
Summary by CodeRabbit