agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

[codex] Make MCP connection startup fallible (#27261)

Browse Source 
## Why

Required MCP server startup was enforced in `Session::new` after
`McpConnectionManager` had already created the clients. That split let
other manager construction paths bypass the same requirement and exposed
manager internals solely so the session could validate them. Keeping
required-server readiness in the constructor gives every caller one
consistent startup contract.

## What changed

- make `McpConnectionManager::new` return `anyhow::Result<Self>` and
fail when an enabled, required server cannot initialize
- pass the startup cancellation token into the constructor so
required-server waits remain cancellable
- propagate constructor failures through resource reads, connector
discovery, and MCP status collection
- preserve the active manager and cancellation token when a refreshed
replacement fails
- keep required-startup failure collection private and cover the
constructor error contract directly

## Validation

- updated the focused connection-manager test to assert the complete
required-server startup error
- local tests not run; relying on CI
This commit is contained in:
Ahmed Ibrahim
2026-06-10 00:17:58 -07:00
committed by GitHub
parent 00a25e1e0c
commit a7b6baecc6
8 changed files with 129 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/connectors.rs
+4 -1
View File
@@ -17,6 +17,7 @@ use codex_protocol::models::PermissionProfile;
use codex_tools::DiscoverableTool;
use rmcp::model::ToolAnnotations;
use serde::Deserialize;
use tokio_util::sync::CancellationToken;
use tracing::warn;
use crate::config::Config;
@@ -285,13 +286,15 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager(
let (tx_event, rx_event) = unbounded();
drop(rx_event);
let (mut mcp_connection_manager, cancel_token) = McpConnectionManager::new(
let cancel_token = CancellationToken::new();
let mut mcp_connection_manager = McpConnectionManager::new(
&mcp_servers,
config.mcp_oauth_credentials_store_mode,
auth_status_entries,
&config.permissions.approval_policy,
INITIAL_SUBMIT_ID.to_owned(),
tx_event,
cancel_token.clone(),
PermissionProfile::default(),
// Connector discovery is threadless. Use an actually configured env if
// one exists, but do not reintroduce the old hidden-local fallback.
codex-rs/core/src/mcp_tool_call_tests.rs
+3 -1
View File
@@ -46,6 +46,7 @@ use std::path::Path;
use std::path::PathBuf;
use std::sync::Arc;
use tempfile::tempdir;
use tokio_util::sync::CancellationToken;
use tracing::Instrument;
use tracing::Level;
use tracing_subscriber::fmt::format::FmtSpan;
@@ -1256,13 +1257,14 @@ fn codex_apps_auth_failure_metadata() -> McpToolApprovalMetadata {
async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: &TurnContext) {
let auth = session.services.auth_manager.auth().await;
let (manager, _cancel_token) = codex_mcp::McpConnectionManager::new(
let manager = codex_mcp::McpConnectionManager::new(
&HashMap::new(),
turn_context.config.mcp_oauth_credentials_store_mode,
HashMap::new(),
&turn_context.approval_policy,
turn_context.sub_id.clone(),
session.get_tx_event(),
CancellationToken::new(),
turn_context.permission_profile(),
codex_mcp::McpRuntimeContext::new(Arc::clone(&session.services.environment_manager), {
#[allow(deprecated)]
codex-rs/core/src/session/mcp.rs
+7 -12
View File
@@ -338,18 +338,21 @@ impl Session {
turn_context.cwd.to_path_buf(),
),
};
{
let mcp_startup_cancellation_token = {
let mut guard = self.services.mcp_startup_cancellation_token.lock().await;
guard.cancel();
*guard = CancellationToken::new();
}
let (refreshed_manager, cancel_token) = McpConnectionManager::new(
let cancellation_token = CancellationToken::new();
*guard = cancellation_token.clone();
cancellation_token
};
let refreshed_manager = McpConnectionManager::new(
&mcp_servers,
store_mode,
auth_statuses,
&turn_context.approval_policy,
turn_context.sub_id.clone(),
self.get_tx_event(),
mcp_startup_cancellation_token,
turn_context.permission_profile(),
mcp_runtime_context,
config.codex_home.to_path_buf(),
@@ -366,14 +369,6 @@ impl Session {
let current_manager = self.services.mcp_connection_manager.read().await;
refreshed_manager.set_elicitations_auto_deny(current_manager.elicitations_auto_deny());
}
{
let mut guard = self.services.mcp_startup_cancellation_token.lock().await;
if guard.is_cancelled() {
cancel_token.cancel();
}
*guard = cancel_token;
}
let mut old_manager = {
let mut manager = self.services.mcp_connection_manager.write().await;
std::mem::replace(&mut *manager, refreshed_manager)
codex-rs/core/src/session/session.rs
+11 -53
View File
@@ -469,10 +469,6 @@ impl Session {
#[instrument(name = "session_init", level = "info", skip_all)]
#[allow(clippy::too_many_arguments)]
#[expect(
clippy::await_holding_invalid_type,
reason = "session initialization must serialize access through session-owned manager guards"
)]
pub(crate) async fn new(
mut session_configuration: SessionConfiguration,
config: Arc<Config>,
@@ -1116,15 +1112,6 @@ impl Session {
sess.send_event_raw(event).await;
}
let mut required_mcp_servers: Vec<String> = mcp_servers
.iter()
.filter(|(_, server)| server.enabled() && server.required())
.map(|(name, _)| name.clone())
.collect();
required_mcp_servers.sort();
let enabled_mcp_server_count =
mcp_servers.values().filter(|server| server.enabled()).count();
let required_mcp_server_count = required_mcp_servers.len();
let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config.as_ref()).await;
let host_owned_codex_apps_enabled = config
.features
@@ -1137,11 +1124,13 @@ impl Session {
} else {
ElicitationCapability::default()
};
{
let mcp_startup_cancellation_token = {
let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await;
cancel_guard.cancel();
*cancel_guard = CancellationToken::new();
}
let cancel_token = CancellationToken::new();
*cancel_guard = cancel_token.clone();
cancel_token
};
let turn_environment = crate::environment_selection::resolve_environment_selections(
sess.services.environment_manager.as_ref(),
session_configuration.environment_selections(),
@@ -1164,13 +1153,14 @@ impl Session {
session_configuration.cwd().to_path_buf(),
),
};
let (mcp_connection_manager, cancel_token) = McpConnectionManager::new(
let mcp_connection_manager = McpConnectionManager::new(
&mcp_servers,
config.mcp_oauth_credentials_store_mode,
auth_statuses.clone(),
auth_statuses,
&session_configuration.approval_policy,
INITIAL_SUBMIT_ID.to_owned(),
tx_event.clone(),
mcp_startup_cancellation_token,
session_configuration.permission_profile(),
mcp_runtime_context,
config.codex_home.to_path_buf(),
@@ -1185,43 +1175,11 @@ impl Session {
.instrument(info_span!(
"session_init.mcp_manager_init",
otel.name = "session_init.mcp_manager_init",
session_init.enabled_mcp_server_count = enabled_mcp_server_count,
session_init.required_mcp_server_count = required_mcp_server_count,
))
.await;
{
let mut manager_guard = sess.services.mcp_connection_manager.write().await;
*manager_guard = mcp_connection_manager;
}
{
let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await;
if cancel_guard.is_cancelled() {
cancel_token.cancel();
}
*cancel_guard = cancel_token;
}
if !required_mcp_servers.is_empty() {
let failures = sess
.services
.mcp_connection_manager
.read()
.await
.required_startup_failures(&required_mcp_servers)
.instrument(info_span!(
"session_init.required_mcp_wait",
otel.name = "session_init.required_mcp_wait",
session_init.required_mcp_server_count = required_mcp_server_count,
))
.await;
if !failures.is_empty() {
let details = failures
.iter()
.map(|failure| format!("{}: {}", failure.server, failure.error))
.collect::<Vec<_>>()
.join("; ");
anyhow::bail!("required MCP servers failed to initialize: {details}");
}
}
sess.services
.install_mcp_connection_manager(mcp_connection_manager)
.await?;
sess.schedule_startup_prewarm(session_configuration.base_instructions.clone())
.await;
let session_start_source = match &initial_history {
codex-rs/core/src/state/service.rs
+21
View File
@@ -15,6 +15,7 @@ use crate::tools::code_mode::CodeModeService;
use crate::tools::network_approval::NetworkApprovalService;
use crate::tools::sandboxing::ApprovalStore;
use crate::unified_exec::UnifiedExecProcessManager;
use anyhow::Result;
use arc_swap::ArcSwap;
use arc_swap::ArcSwapOption;
use codex_analytics::AnalyticsEventsClient;
@@ -82,3 +83,23 @@ pub(crate) struct SessionServices {
/// the same manager through child-thread spawn paths without reconstructing it.
pub(crate) environment_manager: Arc<EnvironmentManager>,
}
impl SessionServices {
/// Installs the manager before validating required servers so startup-time elicitation can
/// resolve through the session's manager while validation waits.
#[expect(
clippy::await_holding_invalid_type,
reason = "required MCP validation keeps the installed manager reachable for startup-time elicitation"
)]
pub(crate) async fn install_mcp_connection_manager(
&self,
manager: McpConnectionManager,
) -> Result<()> {
*self.mcp_connection_manager.write().await = manager;
self.mcp_connection_manager
.read()
.await
.validate_required_servers()
.await
}
}