mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
## Problem The introduction of `notify_sandbox_state_change()` in #7112 caused a regression where the blocking call in `Session::new()` waits for all MCP servers to fully initialize before returning. This prevents the TUI event loop from starting, resulting in `McpStartupUpdateEvent` messages being emitted but never consumed or displayed. As a result, the app appears to hang during startup, and users do not see the expected "Booting MCP server: {name}" status line. Issue: [#7827](https://github.com/openai/codex/issues/7827) ## Solution This change moves sandbox state notification into each MCP server's background initialization task. The notification is sent immediately after the server transitions to the Ready state. This approach: - Avoids blocking `Session::new()`, allowing the TUI event loop to start promptly. - Ensures each MCP server receives its sandbox state before handling any tool calls. - Restores the display of "Booting MCP server" status lines during startup. ## Key Changes - Added `ManagedClient::notify_sandbox_state()` method. - Passed sandbox_state to `McpConnectionManager::initialize()`. - Sends sandbox state notification in the background task after the server reaches Ready status. - Removed blocking notify_sandbox_state_change() methods. - Added a chatwidget snapshot test for the "Booting MCP server" status line. ## Regression Details Regression was bisected to #7112, which introduced the blocking behavior. --------- Co-authored-by: Michael Bolin <bolinfest@gmail.com> Co-authored-by: Michael Bolin <mbolin@openai.com>
This commit is contained in:
co-authored by
Michael Bolin
Michael Bolin
parent
54feceea46
commit
c978b6e222
+32
-19
@@ -700,6 +700,14 @@ impl Session {
|
||||
for event in events {
|
||||
sess.send_event_raw(event).await;
|
||||
}
|
||||
|
||||
// Construct sandbox_state before initialize() so it can be sent to each
|
||||
// MCP server immediately after it becomes ready (avoiding blocking).
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: session_configuration.sandbox_policy.clone(),
|
||||
codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: session_configuration.cwd.clone(),
|
||||
};
|
||||
sess.services
|
||||
.mcp_connection_manager
|
||||
.write()
|
||||
@@ -710,25 +718,10 @@ impl Session {
|
||||
auth_statuses.clone(),
|
||||
tx_event.clone(),
|
||||
sess.services.mcp_startup_cancellation_token.clone(),
|
||||
sandbox_state,
|
||||
)
|
||||
.await;
|
||||
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: session_configuration.sandbox_policy.clone(),
|
||||
codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: session_configuration.cwd.clone(),
|
||||
};
|
||||
if let Err(e) = sess
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.notify_sandbox_state_change(&sandbox_state)
|
||||
.await
|
||||
{
|
||||
tracing::error!("Failed to notify sandbox state change: {e}");
|
||||
}
|
||||
|
||||
// record_initial_history can emit events. We record only after the SessionConfiguredEvent is emitted.
|
||||
sess.record_initial_history(initial_history).await;
|
||||
|
||||
@@ -840,14 +833,34 @@ impl Session {
|
||||
sub_id: String,
|
||||
updates: SessionSettingsUpdate,
|
||||
) -> Arc<TurnContext> {
|
||||
let session_configuration = {
|
||||
let (session_configuration, sandbox_policy_changed) = {
|
||||
let mut state = self.state.lock().await;
|
||||
let session_configuration = state.session_configuration.clone().apply(&updates);
|
||||
let sandbox_policy_changed =
|
||||
state.session_configuration.sandbox_policy != session_configuration.sandbox_policy;
|
||||
state.session_configuration = session_configuration.clone();
|
||||
session_configuration
|
||||
(session_configuration, sandbox_policy_changed)
|
||||
};
|
||||
|
||||
let per_turn_config = Self::build_per_turn_config(&session_configuration);
|
||||
|
||||
if sandbox_policy_changed {
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: per_turn_config.sandbox_policy.clone(),
|
||||
codex_linux_sandbox_exe: per_turn_config.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: per_turn_config.cwd.clone(),
|
||||
};
|
||||
if let Err(e) = self
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.notify_sandbox_state_change(&sandbox_state)
|
||||
.await
|
||||
{
|
||||
warn!("Failed to notify sandbox state change to MCP servers: {e:#}");
|
||||
}
|
||||
}
|
||||
|
||||
let model_family = self
|
||||
.services
|
||||
.models_manager
|
||||
|
||||
@@ -1,14 +1,18 @@
|
||||
pub mod auth;
|
||||
use std::collections::HashMap;
|
||||
use std::env;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use async_channel::unbounded;
|
||||
use codex_protocol::protocol::McpListToolsResponseEvent;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use mcp_types::Tool as McpTool;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
|
||||
use crate::config::Config;
|
||||
use crate::mcp::auth::compute_auth_statuses;
|
||||
use crate::mcp_connection_manager::McpConnectionManager;
|
||||
use crate::mcp_connection_manager::SandboxState;
|
||||
|
||||
const MCP_TOOL_NAME_PREFIX: &str = "mcp";
|
||||
const MCP_TOOL_NAME_DELIMITER: &str = "__";
|
||||
@@ -34,6 +38,13 @@ pub async fn collect_mcp_snapshot(config: &Config) -> McpListToolsResponseEvent
|
||||
drop(rx_event);
|
||||
let cancel_token = CancellationToken::new();
|
||||
|
||||
// Use ReadOnly sandbox policy for MCP snapshot collection (safest default)
|
||||
let sandbox_state = SandboxState {
|
||||
sandbox_policy: SandboxPolicy::ReadOnly,
|
||||
codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(),
|
||||
sandbox_cwd: env::current_dir().unwrap_or_else(|_| PathBuf::from("/")),
|
||||
};
|
||||
|
||||
mcp_connection_manager
|
||||
.initialize(
|
||||
config.mcp_servers.clone(),
|
||||
@@ -41,6 +52,7 @@ pub async fn collect_mcp_snapshot(config: &Config) -> McpListToolsResponseEvent
|
||||
auth_status_entries.clone(),
|
||||
tx_event,
|
||||
cancel_token.clone(),
|
||||
sandbox_state,
|
||||
)
|
||||
.await;
|
||||
|
||||
|
||||
@@ -182,6 +182,21 @@ struct ManagedClient {
|
||||
server_supports_sandbox_state_capability: bool,
|
||||
}
|
||||
|
||||
impl ManagedClient {
|
||||
async fn notify_sandbox_state_change(&self, sandbox_state: &SandboxState) -> Result<()> {
|
||||
if !self.server_supports_sandbox_state_capability {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
self.client
|
||||
.send_custom_notification(
|
||||
MCP_SANDBOX_STATE_NOTIFICATION,
|
||||
Some(serde_json::to_value(sandbox_state)?),
|
||||
)
|
||||
.await
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
struct AsyncManagedClient {
|
||||
client: Shared<BoxFuture<'static, Result<ManagedClient, StartupOutcomeError>>>,
|
||||
@@ -231,17 +246,7 @@ impl AsyncManagedClient {
|
||||
|
||||
async fn notify_sandbox_state_change(&self, sandbox_state: &SandboxState) -> Result<()> {
|
||||
let managed = self.client().await?;
|
||||
if !managed.server_supports_sandbox_state_capability {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
managed
|
||||
.client
|
||||
.send_custom_notification(
|
||||
MCP_SANDBOX_STATE_NOTIFICATION,
|
||||
Some(serde_json::to_value(sandbox_state)?),
|
||||
)
|
||||
.await
|
||||
managed.notify_sandbox_state_change(sandbox_state).await
|
||||
}
|
||||
}
|
||||
|
||||
@@ -274,6 +279,7 @@ impl McpConnectionManager {
|
||||
auth_entries: HashMap<String, McpAuthStatusEntry>,
|
||||
tx_event: Sender<Event>,
|
||||
cancel_token: CancellationToken,
|
||||
initial_sandbox_state: SandboxState,
|
||||
) {
|
||||
if cancel_token.is_cancelled() {
|
||||
return;
|
||||
@@ -302,13 +308,25 @@ impl McpConnectionManager {
|
||||
clients.insert(server_name.clone(), async_managed_client.clone());
|
||||
let tx_event = tx_event.clone();
|
||||
let auth_entry = auth_entries.get(&server_name).cloned();
|
||||
let sandbox_state = initial_sandbox_state.clone();
|
||||
join_set.spawn(async move {
|
||||
let outcome = async_managed_client.client().await;
|
||||
if cancel_token.is_cancelled() {
|
||||
return (server_name, Err(StartupOutcomeError::Cancelled));
|
||||
}
|
||||
let status = match &outcome {
|
||||
Ok(_) => McpStartupStatus::Ready,
|
||||
Ok(_) => {
|
||||
// Send sandbox state notification immediately after Ready
|
||||
if let Err(e) = async_managed_client
|
||||
.notify_sandbox_state_change(&sandbox_state)
|
||||
.await
|
||||
{
|
||||
warn!(
|
||||
"Failed to notify sandbox state to MCP server {server_name}: {e:#}",
|
||||
);
|
||||
}
|
||||
McpStartupStatus::Ready
|
||||
}
|
||||
Err(error) => {
|
||||
let error_str = mcp_init_error_display(
|
||||
server_name.as_str(),
|
||||
|
||||
Reference in New Issue
Block a user