Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
61 changes: 43 additions & 18 deletions crates/goose-acp/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,21 +479,32 @@ impl GooseAcpAgent {
let skip_developer = acp_developer.is_some();
let sid_str = session_id.map(|s| s.0.to_string());

for ext in extensions {
if skip_developer && ext.name() == "developer" {
continue;
}
let name = ext.name().to_string();
match agent
.extension_manager
.add_extension(ext, None, None, sid_str.as_deref())
.await
{
Ok(_) => info!(extension = %name, "extension loaded"),
Err(e) => warn!(extension = %name, error = %e, "extension load failed"),
}
// Filter out the developer extension (handled separately via ACP client)
if skip_developer {
extensions.retain(|ext| ext.name() != "developer");
}

// Load all extensions in parallel
let ext_manager = &agent.extension_manager;
let extension_futures = extensions
.into_iter()
.map(|ext| {
let ext_manager = Arc::clone(ext_manager);
Comment on lines +487 to +490

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Deduplicate extension keys before parallel startup

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 👍 / 👎.

let sid = sid_str.clone();
async move {
let name = ext.name().to_string();
match ext_manager
.add_extension(ext, None, None, sid.as_deref())
.await
{
Ok(_) => info!(extension = %name, "extension loaded"),
Err(e) => warn!(extension = %name, error = %e, "extension load failed"),
}
}
})
.collect::<Vec<_>>();
futures::future::join_all(extension_futures).await;

if let Some((client, config)) = acp_developer {
let info = client.get_info().cloned();
agent
Expand Down Expand Up @@ -932,21 +943,35 @@ impl GooseAcpAgent {
}

async fn add_mcp_extensions(
agent: &Agent,
agent: &Arc<Agent>,
mcp_servers: Vec<McpServer>,
session_id: &str,
) -> Result<(), sacp::Error> {
// Phase 1: Convert all configs sequentially (fast, pure computation, fail-fast)
let mut configs = Vec::with_capacity(mcp_servers.len());
for mcp_server in mcp_servers {
let config = match mcp_server_to_extension_config(mcp_server) {
Ok(c) => c,
Err(msg) => {
return Err(sacp::Error::invalid_params().data(msg));
}
};
let name = config.name().to_string();
if let Err(e) = agent.add_extension(config, session_id).await {
return Err(sacp::Error::internal_error()
.data(format!("Failed to add MCP server '{}': {}", name, e)));
configs.push(config);
}

if configs.is_empty() {
return Ok(());
}

// Phase 2: Load all extensions in parallel, persist state once
let results = agent.add_extensions_bulk(configs, session_id).await;
for result in &results {
if !result.success {
let error_msg = result.error.as_deref().unwrap_or("unknown error");
return Err(sacp::Error::internal_error().data(format!(
"Failed to add MCP server '{}': {}",
result.name, error_msg
)));
}
}
Ok(())
Expand Down
70 changes: 70 additions & 0 deletions crates/goose/src/agents/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,76 @@ impl Agent {
Ok(())
}

/// Load multiple extensions in parallel, persisting state once at the end.
///
/// Unlike `add_extension`, this avoids per-extension persistence and acquires
/// the container lock once upfront to prevent serialisation of the parallel futures.
pub async fn add_extensions_bulk(
self: &Arc<Self>,
extensions: Vec<ExtensionConfig>,
session_id: &str,
) -> Vec<ExtensionLoadResult> {
// Resolve session working_dir and container once, before spawning futures,
// so each future doesn't re-acquire the container lock.
let working_dir = match self
.config
.session_manager
.get_session(session_id, false)
.await
{
Ok(session) => Some(session.working_dir),
Err(e) => {
warn!("Failed to get session for bulk load: {}", e);
None
}
};
let container = self.container.lock().await.clone();

let extension_futures = extensions
.into_iter()
.map(|config| {
let ext_manager = Arc::clone(&self.extension_manager);
let working_dir = working_dir.clone();
let container = container.clone();
let sid = session_id.to_string();

async move {
let name = config.name().to_string();
match ext_manager
.add_extension(config, working_dir, container.as_ref(), Some(&sid))
.await
{
Ok(_) => ExtensionLoadResult {
name,
success: true,
error: None,
},
Err(e) => {
let error_msg = e.to_string();
warn!("Failed to load extension {}: {}", name, error_msg);
ExtensionLoadResult {
name,
success: false,
error: Some(error_msg),
}
}
}
}
})
.collect::<Vec<_>>();

let results = futures::future::join_all(extension_futures).await;

// Persist once after all extensions are loaded
if results.iter().any(|r| r.success) {
if let Err(e) = self.persist_extension_state(session_id).await {
warn!("Failed to persist extension state after bulk load: {}", e);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Propagate extension state persistence failures

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 👍 / 👎.

}

results
}

async fn add_extension_inner(
&self,
extension: ExtensionConfig,
Expand Down
Loading