perf(agent): prewarm session integrations before first turn#2454
Conversation
Add a task-local depth guard around run_subagent so runtime delegation cannot recurse beyond the documented harness limit. Co-authored-by: Cursor <cursoragent@cursor.com>
Prevents usize wrap-around that could bypass MAX_SPAWN_DEPTH guard in release builds. Addresses CodeRabbit review on PR tinyhumansai#2234.
The new with_spawn_depth task-local scope adds a second stacked task_local wrapper around run_typed_mode. Under cargo-llvm-cov instrumentation, the combined future state machine overflows tokio's default 2 MiB per-thread test stack in turn_dispatches_spawn_subagent_through_full_path. Box::pin the inner future so the large state machine lives on the heap.
|
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 (10)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds a Tokio task-local spawn-depth budget (MAX_SPAWN_DEPTH = 3) and enforcement that returns SubagentRunError::SpawnDepthExceeded when exceeded; threads a best-effort Composio integration prewarm through AgentBuilder, adds session flags/config to ensure integrations are initialized once per session, and updates related types and tests. ChangesSpawn-depth limiting and integration prewarming
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/agent/harness/session/builder.rs (1)
1305-1315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack only actually inserted synthesized tool names.
synthesized_tool_namesis currently derived before collision filtering. If a synthesized name collides with an existing direct tool, it won’t be inserted now, but it will still be marked synthesized and can be removed on the next refresh.Suggested fix
- let synthesized_tool_names: std::collections::HashSet<String> = delegation_tools - .iter() - .map(|t| t.name().to_string()) - .collect(); let existing_names: std::collections::HashSet<String> = tools.iter().map(|t| t.name().to_string()).collect(); - tools.extend( - delegation_tools - .into_iter() - .filter(|t| !existing_names.contains(t.name())), - ); + let filtered_delegation_tools: Vec<Box<dyn Tool>> = delegation_tools + .into_iter() + .filter(|t| !existing_names.contains(t.name())) + .collect(); + let synthesized_tool_names: std::collections::HashSet<String> = filtered_delegation_tools + .iter() + .map(|t| t.name().to_string()) + .collect(); + tools.extend(filtered_delegation_tools);🤖 Prompt for 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. In `@src/openhuman/agent/harness/session/builder.rs` around lines 1305 - 1315, synthesized_tool_names is computed from delegation_tools before filtering collisions, so names that were not actually inserted (because they collided with existing tools) are still marked synthesized; update the logic to compute synthesized_tool_names only from the delegation_tools that are actually inserted into tools — for example, apply the same filter used in tools.extend (the closure that checks !existing_names.contains(t.name())) and collect the names from that filtered set (or collect inserted tools into a temporary vec and derive synthesized_tool_names from it) so synthesized_tool_names reflects only truly added tools.
🤖 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/openhuman/agent/harness/session/builder.rs`:
- Around line 1523-1524: The first issue is a use-after-move on
prewarmed_integrations: check prewarmed_integrations.is_some() first (assign
connected_integrations_initialized = prewarmed_integrations.is_some()), then
assign agent.connected_integrations = prewarmed_integrations.unwrap_or_default()
(or clone prior to unwrap) so you don't read after move; update references to
prewarmed_integrations, connected_integrations, and
connected_integrations_initialized accordingly. The second issue is that
self.synthesized_tool_names is populated from delegation_tools before removing
name collisions, causing refresh_delegation_tools to drop direct tools; fix by
first filtering delegation_tools to remove any whose names collide with existing
direct tools, extend tools with that filtered list, and then set
self.synthesized_tool_names to the names of the actually inserted delegation
tools (use the filtered collection) so refresh_delegation_tools uses the correct
drop mask (affecting synthesized_tool_names, delegation_tools, tools, and
refresh_delegation_tools).
---
Outside diff comments:
In `@src/openhuman/agent/harness/session/builder.rs`:
- Around line 1305-1315: synthesized_tool_names is computed from
delegation_tools before filtering collisions, so names that were not actually
inserted (because they collided with existing tools) are still marked
synthesized; update the logic to compute synthesized_tool_names only from the
delegation_tools that are actually inserted into tools — for example, apply the
same filter used in tools.extend (the closure that checks
!existing_names.contains(t.name())) and collect the names from that filtered set
(or collect inserted tools into a temporary vec and derive
synthesized_tool_names from it) so synthesized_tool_names reflects only truly
added tools.
🪄 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: dedc5c1f-db40-4392-b7d7-c1d181373745
📒 Files selected for processing (12)
gitbooks/developing/architecture/agent-harness.mdsrc/openhuman/agent/harness/harness_gap_tests.rssrc/openhuman/agent/harness/mod.rssrc/openhuman/agent/harness/session/builder.rssrc/openhuman/agent/harness/session/runtime.rssrc/openhuman/agent/harness/session/tests.rssrc/openhuman/agent/harness/session/turn.rssrc/openhuman/agent/harness/session/types.rssrc/openhuman/agent/harness/spawn_depth_context.rssrc/openhuman/agent/harness/subagent_runner/ops.rssrc/openhuman/agent/harness/subagent_runner/ops_tests.rssrc/openhuman/agent/harness/subagent_runner/types.rs
graycyrus
left a comment
There was a problem hiding this comment.
LGTM — clean, well-structured performance optimisation.
Walkthrough: this PR prewarms session connected_integrations from the shared Composio cache during from_config_* agent construction, synthesises delegation tools against the prewarmed snapshot, and carries the runtime Config on the session agent to avoid Config::load_or_init() on the hot path. Cold-cache sessions preserve fallback behaviour.
| Area | Files | Key changes |
|---|---|---|
| Rust core / session | builder.rs |
Prewarm integrations from cache, build delegation tools against cached slice, track synthesized_tool_names post-collision-filter |
| Rust core / session | turn.rs |
Gate turn-1 fetch behind connected_integrations_initialized, use stored config for mid-session cache probes |
| Rust core / session | types.rs |
New fields: connected_integrations_initialized, integration_runtime_config |
| Rust core / session | runtime.rs |
set_connected_integrations now sets initialized flag + updates hash |
| Tests | tests.rs |
Regression test for initialized/hash bookkeeping |
CodeRabbit findings (already addressed): use-after-move on prewarmed_integrations ✅, synthesized tool mask alignment ✅ — both confirmed fixed in current diff.
Additional observations:
- Fallback paths are well-preserved: cold-cache → empty slice → turn-1 fetch → delegation rebuild. No correctness regression.
config.clone()for the snapshot is a one-time construction cost, acceptable for the hot-path savings.- Good test coverage for the bookkeeping; the prewarm path itself is integration-level (requires warm cache) which is reasonable.
No new findings beyond what CodeRabbit already covered. Moving to approval queue.
|
nice work @srikaanthh, prewarming the session integrations so turn-1 skips the fetch and delegation rebuild is a really clean win 🔥 always love seeing perf improvements that cut latency on the hot path like this. glad to have it merged, keep it coming! |
Summary
connected_integrationsfrom the shared Composio cache duringfrom_config_*agent constructiondelegate_<toolkit>surfaceConfigsnapshot on the session agent so mid-session integration-cache probes stop reloading config on the hot pathProblem
Agent::turn()before the first provider call.Configinside the hot path, and the session builder always synthesized delegation tools against an empty integration set, guaranteeing a repair pass on turn 1.Solution
composio::cached_active_integrations(config)during session construction to prewarmconnected_integrationswhen the shared cache is already warm.&[], then persist the synthesized-tool name set onto the builtAgent.connected_integrations_initialized; turn 1 now only fetches integrations and refreshes delegation tools when the builder could not prewarm the cache.Configsnapshot on the session agent so mid-session cache reads and fallback integration fetches do not callConfig::load_or_init()on the hot path.Arcreconciliation failures so correctness stays unchanged when prewarming is unavailable.Submission Checklist
pnpmunavailable on PATH, focused Rust tests blocked by missingcmake).N/A: behaviour-only change## Relateddocs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
Config::load_or_init()on subsequent cache probes.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpmon PATH, so this command could not be run here.pnpmon PATH, so this command could not be run here.whisper-rs-sysrequirescmake, which is not installed in this environment.cargo fmt --manifest-path Cargo.tomlpassed;git diff --check origin/main...HEADclean.cargo check --manifest-path app/src-tauri/Cargo.tomlattempt was also blocked because the vendoredtauri-cefdependency tree is missing in this environment.Validation Blocked
command:GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml set_connected_integrations_marks_session_initialized_and_updates_hash -- --nocaptureandGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml turn_without_tools_returns_text -- --nocaptureerror:whisper-rs-sysbuild script failed becausecmakeis not installed in the local environmentimpact:focused Rust tests did not complete locally; correctness is based on source review plus the added regression coverageBehavior Changes
Parity Contract
Arcreconciliation fallback, mid-session cache-driven refresh, and config-load fallback behavior remain intactDuplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Tests
Documentation