perf(acp): parallelize extension loading in ACP server#8098
Conversation
The ACP server loaded extensions sequentially in create_agent_for_session and add_mcp_extensions, making total startup time the sum of all extension init times. The CLI session builder and session reload path already parallelize — this brings the ACP server in line. Parallelize both paths using futures::future::join_all: - create_agent_for_session: load all platform extensions concurrently via ExtensionManager::add_extension, keeping the developer extension sequential (it uses a special ACP-wrapped client) - add_mcp_extensions: split into sequential config conversion (fail-fast) and parallel loading via a new Agent::add_extensions_bulk() method that resolves working_dir and container once upfront to avoid lock contention, then persists extension state in a single transaction Benchmark with 9 real extensions: 31.5s sequential → 6.1s parallel (5.2x) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: ae775fe626
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Err(e) = self.persist_extension_state(session_id).await { | ||
| warn!("Failed to persist extension state after bulk load: {}", e); | ||
| } |
There was a problem hiding this comment.
add_extensions_bulk logs and ignores errors from persist_extension_state, so add_mcp_extensions can return success even when extension state was never saved. This matters when session storage is unavailable (e.g., I/O/permission issues): MCP servers appear added for the current process but are lost on session reload, creating inconsistent runtime vs persisted state. The previous agent.add_extension path surfaced these persistence failures to the caller, so this is a behavioral regression in ACP session setup.
Useful? React with 👍 / 👎.
…stating comments - Change add_extensions_bulk to return anyhow::Result so persistence failures propagate to callers instead of being silently logged - Update add_mcp_extensions to handle the new Result - Remove comments that restate what the code does Signed-off-by: Douwe Osinga <douwe@squareup.com>
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Looks good — the parallelization is a clean improvement that brings the ACP server in line with the CLI path.
I pushed a fixup commit that:
- Changes
add_extensions_bulkto returnanyhow::Resultso persistence failures propagate to callers (addresses the Codex P1 — previously these were silently logged) - Updates
add_mcp_extensionsto handle the newResult - Removes comments that restate what the code does
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce97ae6100
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let extension_futures = extensions | ||
| .into_iter() | ||
| .map(|ext| { | ||
| let ext_manager = Arc::clone(ext_manager); |
There was a problem hiding this comment.
Loading all configs via join_all here makes startup order-dependent when the same extension key appears more than once (e.g., a builtin also enabled in config). create_agent_for_session appends self.builtins to config-derived extensions, and ExtensionManager::add_extension does a pre-check and insert in separate phases, so duplicate keys can initialize concurrently and whichever future finishes last overwrites the other. The old sequential loop had deterministic behavior; this change introduces nondeterministic final extension config/tool state across runs.
Useful? React with 👍 / 👎.
2 participants
Summary
The ACP server loaded extensions sequentially in
create_agent_for_sessionandadd_mcp_extensions, making total startup time the sum of all extension init times.The CLI session builder (
builder.rs) and session reload path (load_extensions_from_session) already parallelize extension loading, this PR brings the ACP server in lineParallelize both paths using
futures::future::join_all:create_agent_for_session: Load all platform extensions concurrently viaExtensionManager::add_extension, keeping the developer extension sequential (it uses a special ACP-wrappedclient)
add_mcp_extensions: Split into sequential config conversion (fail-fast on bad config) and parallelloading via a new
Agent::add_extensions_bulk()method that resolvesworking_dirandcontaineronceupfront to avoid lock contention, then persists extension state in a single transaction
Testing
cargo check -p goose -p goose-acp— compiles cleanly with no warningsRelated Issues
N/A
Screenshots/Demos (for UX changes)
N/A