Fix MCP startup tracking when attaching to shared app servers (#60280)

## Why

Clients attaching to a shared app server can miss startup notifications and have a different local MCP configuration. Waiting for locally configured servers can leave the TUI stuck in a busy state.

## What changed

- Track only reported MCP servers for shared app servers and subagents, settling startup once observed starts complete.
- Handle standalone terminal updates without reopening startup, while reporting failures and cancellations even when their starts were missed.
- Suppress duplicate diagnostics until an observed retry or notification lag, and include all received outcomes in each active round's summary.

## Testing

Add regression coverage for attachment before, during, and after startup; idle keyboard behavior after startup settles; terminal-only failures and cancellations; and repeated failures after retries or notification lag.

GitOrigin-RevId: e19f23c212f2f3cee1adeb5c3b94a508aa599242
This commit is contained in:
Charlie Marsh
2026-11-08 19:04:10 +00:00
committed by copyberry
parent 9ba7a3df52
commit ee51bb132e
6 changed files with 333 additions and 114 deletions
+5 -5
View File
@@ -73,11 +73,11 @@ impl App {
|| (thread_id != primary_thread_id
&& !self.side_threads.contains_key(&thread_id))
});
if is_subagent {
// Deferred servers may start in a later round on their own. Track the
// servers that actually report startup in each round.
self.chat_widget
.set_mcp_startup_expected_servers(std::iter::empty());
if is_subagent || !matches!(self.app_server_target, crate::AppServerTarget::Embedded) {
// Shared app servers only send startup updates after subscription and may
// have different configured servers. Subagents can start deferred servers
// individually. In both cases, wait only for the startups we observe.
self.chat_widget.track_reported_mcp_startup_servers();
return;
}
+201
View File
@@ -16,6 +16,207 @@ fn configure_mcp_servers(app: &mut App) {
.expect("test MCP servers should accept any configuration");
}
#[tokio::test]
async fn remote_mcp_startup_settles_when_attaching_mid_round() -> Result<()> {
use crate::terminal_program_status::ProgramStatus;
use McpServerStartupState::Ready;
use McpServerStartupState::Starting;
use codex_app_server_client::AppServerEvent;
let mut tui = crate::tui::test_support::make_test_tui()?;
// Subscribe before startup, during startup, or after every server is ready.
for missed_updates in [0, 2, 5, 6] {
let (mut app, mut events, _ops) = make_test_app_with_channels().await;
let mut app_server = crate::start_embedded_app_server_for_picker(&app.config).await?;
app.app_server_target = AppServerTarget::Remote {
endpoint: crate::resolve_remote_addr("ws://127.0.0.1:4500")?,
};
configure_mcp_servers(&mut app);
let id = ThreadId::new();
app.enqueue_primary_thread_session(
test_thread_session(id, app.config.cwd.to_path_buf()),
Vec::new(),
)
.await?;
let update = |name: &str, status| {
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
thread_id: Some(id.to_string()),
name: name.to_string(),
status,
error: None,
failure_reason: None,
})
};
// Only the app server has `late` in its configuration.
let startup = [
(update("eager", Starting), true),
(update("deferred", Starting), true),
(update("late", Starting), true),
(update("eager", Ready), true),
(update("deferred", Ready), true),
(update("late", Ready), false),
];
for (notification, running) in startup.into_iter().skip(missed_updates).chain([
(turn_started_notification(id, "turn"), true),
(
turn_completed_notification(id, "turn", TurnStatus::Completed),
false,
),
// A later reconnect can report Ready without a corresponding Starting.
(update("late", Ready), false),
]) {
app.handle_app_server_event(
&app_server,
AppServerEvent::ServerNotification(Box::new(notification)),
)
.await;
app.drain_active_thread_events(&mut tui).await?;
assert_eq!(
app.chat_widget.is_task_running_for_test(),
running,
"missed {missed_updates} startup updates"
);
}
assert_eq!(
app.chat_widget.desired_program_status(),
ProgramStatus::Done
);
app.transcript_cells
.push(Arc::new(history_cell::new_user_prompt(
"Previous prompt".into(),
Vec::new(),
Vec::new(),
Vec::new(),
)));
app.handle_key_event(&mut tui, &mut app_server, KeyCode::Esc.into())
.await;
let composer = render_bottom_popup(&app.chat_widget, /*width*/ 80);
let hint = composer
.lines()
.find(|line| line.contains("edit previous message"))
.unwrap_or_default()
.trim();
insta::allow_duplicates! {
insta::assert_snapshot!(hint, @"esc again to edit previous message");
}
app.handle_key_event(
&mut tui,
&mut app_server,
KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL),
)
.await;
assert!(
std::iter::from_fn(|| events.try_recv().ok())
.any(|event| matches!(event, AppEvent::Exit(ExitMode::ShutdownFirst)))
);
app_server.shutdown().await?;
}
Ok(())
}
#[tokio::test]
async fn remote_mcp_startup_reports_terminal_only_failures_and_cancellations() -> Result<()> {
use McpServerStartupState::Cancelled;
use McpServerStartupState::Failed;
use McpServerStartupState::Ready;
use McpServerStartupState::Starting;
use codex_app_server_client::AppServerEvent;
use codex_app_server_protocol::McpServerStartupFailureReason::ReauthenticationRequired;
let mut tui = crate::tui::test_support::make_test_tui()?;
let (mut app, mut events, _ops) = make_test_app_with_channels().await;
let app_server = crate::start_embedded_app_server_for_picker(&app.config).await?;
app.app_server_target = AppServerTarget::Remote {
endpoint: crate::resolve_remote_addr("ws://127.0.0.1:4500")?,
};
configure_mcp_servers(&mut app);
let id = ThreadId::new();
app.enqueue_primary_thread_session(
test_thread_session(id, app.config.cwd.to_path_buf()),
Vec::new(),
)
.await?;
let error = "Session expired";
let failed = "MCP startup incomplete (failed: deferred)";
let both_failed = "MCP startup incomplete (failed: deferred, eager)";
let cancelled = "MCP startup interrupted. The following servers were not initialized: other";
for (name, status, running, expected) in [
("eager", Ready, false, &[][..]),
("deferred", Failed, false, &[error, failed]),
("deferred", Failed, false, &[]),
("other", Cancelled, false, &[cancelled]),
// An observed retry emits a fresh diagnostic even if the error is unchanged.
("deferred", Starting, true, &[]),
("deferred", Failed, false, &[error, failed]),
// Only eager's Starting was delivered for the next reload. Both failures count.
("eager", Starting, true, &[]),
("deferred", Failed, true, &[]),
("other", Cancelled, true, &[]),
("eager", Failed, false, &[error, cancelled, both_failed]),
] {
let notification =
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
thread_id: Some(id.to_string()),
name: name.to_string(),
error: (status == Failed).then(|| error.to_string()),
failure_reason: (status == Failed).then_some(ReauthenticationRequired),
status,
});
app.handle_app_server_event(
&app_server,
AppServerEvent::ServerNotification(Box::new(notification)),
)
.await;
app.drain_active_thread_events(&mut tui).await?;
let mut emitted = Vec::new();
while let Ok(event) = events.try_recv() {
if let AppEvent::InsertHistoryCell(cell) = event {
if let Some(warning) = cell
.as_any()
.downcast_ref::<history_cell::StartupWarningsCell>()
{
emitted.push(warning.messages.join("\n"));
}
app.insert_history_cell(&mut tui, cell);
}
}
assert_eq!(emitted, expected, "warning events for {name}");
assert_eq!(
app.chat_widget.is_task_running_for_test(),
running,
"progress for {name}"
);
}
let warnings = history_cell::warning_entries(&app.transcript_cells)
.into_iter()
.map(|entry| format!("{}\n{}", entry.source, entry.details))
.collect::<Vec<_>>();
insta::assert_snapshot!(warnings.join("\n\n"), @r"
MCP · deferred
Session expired
MCP startup incomplete (failed: deferred)
MCP startup incomplete (failed: deferred, eager)
Sign-in required.
MCP · eager
Session expired
MCP startup incomplete (failed: deferred, eager)
Sign-in required.
MCP · other
MCP startup interrupted. The following servers were not initialized: other
");
app_server.shutdown().await?;
Ok(())
}
#[tokio::test]
async fn subagent_mcp_startup_settles_while_cached_servers_remain_deferred() {
let (mut app, _app_event_rx, _op_rx) = make_test_app_with_channels().await;
+3 -7
View File
@@ -635,14 +635,10 @@ pub(crate) struct ChatWidget {
unified_exec_processes: Vec<UnifiedExecProcessSummary>,
/// Exits observed during the pending list request take precedence over its response.
pending_terminal_restore: Option<(uuid::Uuid, HashSet<String>)>,
/// Tracks per-server MCP startup state while startup is in progress.
///
/// The map is `Some(_)` from the first startup status update until the
/// app-server-backed startup round settles, and the bottom pane is treated
/// as "running" while this is populated, even if no agent turn is currently
/// executing.
/// Latest status per server in the active startup round. Its presence keeps
/// the composer busy, and its terminal outcomes determine the round's summary.
mcp_startup_status: Option<HashMap<String, McpStartupStatus>>,
/// Expected MCP servers for the current startup round, seeded from enabled local config.
/// Whether round membership comes from configuration or the notifications we receive.
mcp_startup_expected_servers: Option<mcp_startup::ExpectedMcpServers>,
/// After startup settles, ignore stale updates until enough notifications confirm a new round.
mcp_startup_ignore_updates_until_next_start: bool,
+104 -96
View File
@@ -3,10 +3,12 @@
//! The app server reports MCP server startup as per-server status updates. This
//! module keeps the TUI's buffered startup round state coherent and translates
//! those updates into status headers, warnings, and queued-input release points.
//! Each active round includes every outcome it receives, even if the client has
//! already shown the same diagnostic in an earlier round.
//! Initial and runtime diagnostics remain in retained history and the warnings viewer;
//! compact history hides their full text behind the persistent warning notice.
use std::collections::BTreeSet;
use std::collections::HashMap;
use std::collections::HashSet;
use codex_app_server_protocol::McpServerStartupFailureReason;
@@ -29,17 +31,26 @@ pub(crate) enum McpStartupStatus {
Cancelled,
}
impl McpStartupStatus {
fn is_diagnostic(&self) -> bool {
matches!(self, Self::Failed { .. } | Self::Cancelled)
}
}
pub(super) enum ExpectedMcpServers {
EveryRound(HashSet<String>),
InitialRound(HashSet<String>),
ReportedOnly,
/// A subscriber can miss the starts. A repeated diagnostic can still belong to a new round.
ReportedOnly {
last_diagnostics: HashMap<String, McpStartupStatus>,
},
}
impl ExpectedMcpServers {
fn iter(&self) -> impl Iterator<Item = &String> {
match self {
Self::EveryRound(names) | Self::InitialRound(names) => Some(names),
Self::ReportedOnly => None,
Self::ReportedOnly { .. } => None,
}
.into_iter()
.flatten()
@@ -47,21 +58,32 @@ impl ExpectedMcpServers {
}
impl ChatWidget {
/// Record one MCP startup update, promoting it into either the active startup
/// round or a buffered "next" round.
///
/// This path has to deal with lossy app-server delivery. After
/// `finish_mcp_startup()` or `finish_mcp_startup_after_lag()`, we briefly
/// ignore incoming updates so stale events from the just-finished round do not
/// reopen startup. While that guard is active we buffer updates for a possible
/// next round, and only reactivate once the buffered set is coherent enough to
/// treat as a fresh startup round.
fn update_mcp_startup_status(
&mut self,
server: String,
status: McpStartupStatus,
complete_when_settled: bool,
) {
/// Record a startup update for the displayed thread. A configured round buffers
/// possible starts after it settles until there is enough evidence of a new round.
/// A subscriber that tracks reported servers can also receive a standalone outcome.
fn update_mcp_startup_status(&mut self, server: String, status: McpStartupStatus) {
let mut report_diagnostic = true;
if let Some(ExpectedMcpServers::ReportedOnly { last_diagnostics }) =
&mut self.mcp_startup_expected_servers
{
report_diagnostic = if status.is_diagnostic() {
last_diagnostics
.insert(server.clone(), status.clone())
.as_ref()
!= Some(&status)
} else {
last_diagnostics.remove(&server);
false
};
// An idle client can receive a terminal update without ever seeing its start.
if self.mcp_startup_status.is_none() && !matches!(status, McpStartupStatus::Starting) {
if report_diagnostic {
self.report_mcp_startup_failure(&server, &status);
self.report_mcp_startup_summary([(&server, &status)]);
}
return;
}
}
let mut activated_pending_round = false;
let startup_status = if self.mcp_startup_ignore_updates_until_next_start {
// Ignore-mode buffers the next plausible round so stale post-finish
@@ -105,17 +127,8 @@ impl ChatWidget {
// Normal path: fold the update into the active round and surface
// per-server failures immediately.
let mut startup_status = self.mcp_startup_status.take().unwrap_or_default();
if let McpStartupStatus::Failed {
error,
failure_reason,
} = &status
&& startup_status.get(&server) != Some(&status)
{
self.add_mcp_startup_warning(
vec![error.clone()],
[server.clone()],
*failure_reason,
);
if report_diagnostic && startup_status.get(&server) != Some(&status) {
self.report_mcp_startup_failure(&server, &status);
}
startup_status.insert(server, status);
startup_status
@@ -123,17 +136,7 @@ impl ChatWidget {
if activated_pending_round {
// A promoted buffered round may already contain terminal failures.
for (server, state) in &startup_status {
if let McpStartupStatus::Failed {
error,
failure_reason,
} = state
{
self.add_mcp_startup_warning(
vec![error.clone()],
[server.clone()],
*failure_reason,
);
}
self.report_mcp_startup_failure(server, state);
}
}
self.mcp_startup_status = Some(startup_status);
@@ -142,8 +145,7 @@ impl ChatWidget {
// App-server-backed startup completes when every expected server has
// reported a non-Starting status. Lag handling can force an earlier
// settle via `finish_mcp_startup_after_lag()`.
if complete_when_settled
&& let Some(current) = &self.mcp_startup_status
if let Some(current) = &self.mcp_startup_status
&& let Some(expected_servers) = &self.mcp_startup_expected_servers
&& !current.is_empty()
&& expected_servers
@@ -153,19 +155,7 @@ impl ChatWidget {
.values()
.all(|state| !matches!(state, McpStartupStatus::Starting))
{
let mut failed = Vec::new();
let mut cancelled = Vec::new();
for (name, state) in current {
match state {
McpStartupStatus::Ready => {}
McpStartupStatus::Failed { .. } => failed.push(name.clone()),
McpStartupStatus::Cancelled => cancelled.push(name.clone()),
McpStartupStatus::Starting => {}
}
}
failed.sort();
cancelled.sort();
self.finish_mcp_startup(failed, cancelled);
self.finish_mcp_startup();
return;
}
if let Some(header) = self.mcp_startup_status_header() {
@@ -218,6 +208,17 @@ impl ChatWidget {
));
}
pub(crate) fn track_reported_mcp_startup_servers(&mut self) {
if !matches!(
self.mcp_startup_expected_servers,
Some(ExpectedMcpServers::ReportedOnly { .. })
) {
self.mcp_startup_expected_servers = Some(ExpectedMcpServers::ReportedOnly {
last_diagnostics: HashMap::new(),
});
}
}
/// After the initial batch a dormant helper can start a round on its own.
pub(crate) fn set_initial_mcp_startup_expected_servers(
&mut self,
@@ -225,15 +226,38 @@ impl ChatWidget {
) {
if !matches!(
self.mcp_startup_expected_servers,
Some(ExpectedMcpServers::ReportedOnly)
Some(ExpectedMcpServers::ReportedOnly { .. })
) {
self.mcp_startup_expected_servers =
Some(ExpectedMcpServers::InitialRound(names.collect()));
}
}
pub(super) fn finish_mcp_startup(&mut self, failed: Vec<String>, cancelled: Vec<String>) {
fn report_mcp_startup_failure(&mut self, server: &str, status: &McpStartupStatus) {
if let McpStartupStatus::Failed {
error,
failure_reason,
} = status
{
self.add_mcp_startup_warning(vec![error.clone()], [server.to_owned()], *failure_reason);
}
}
fn report_mcp_startup_summary<'a>(
&mut self,
outcomes: impl IntoIterator<Item = (&'a String, &'a McpStartupStatus)>,
) {
let mut failed = Vec::new();
let mut cancelled = Vec::new();
for (server, status) in outcomes {
match status {
McpStartupStatus::Failed { .. } => failed.push(server.clone()),
McpStartupStatus::Cancelled => cancelled.push(server.clone()),
McpStartupStatus::Starting | McpStartupStatus::Ready => {}
}
}
if !cancelled.is_empty() {
cancelled.sort();
self.add_mcp_startup_warning(
vec![format!(
"MCP startup interrupted. The following servers were not initialized: {}",
@@ -243,25 +267,31 @@ impl ChatWidget {
/*failure_reason*/ None,
);
}
let mut parts = Vec::new();
if !failed.is_empty() {
parts.push(format!("failed: {}", failed.join(", ")));
}
if !parts.is_empty() {
failed.sort();
self.add_mcp_startup_warning(
vec![format!("MCP startup incomplete ({})", parts.join("; "))],
vec![format!(
"MCP startup incomplete (failed: {})",
failed.join(", ")
)],
failed,
/*failure_reason*/ None,
);
}
}
pub(super) fn finish_mcp_startup(&mut self) {
let mcp_startup_owned_status = self.status_header_is_mcp_startup_owned();
self.mcp_startup_status = None;
let mut settled = self.mcp_startup_status.take().unwrap_or_default();
self.report_mcp_startup_summary(&settled);
if matches!(
self.mcp_startup_expected_servers,
Some(ExpectedMcpServers::InitialRound(_))
) {
self.mcp_startup_expected_servers = Some(ExpectedMcpServers::ReportedOnly);
settled.retain(|_, status| status.is_diagnostic());
self.mcp_startup_expected_servers = Some(ExpectedMcpServers::ReportedOnly {
last_diagnostics: settled,
});
}
self.mcp_startup_ignore_updates_until_next_start = true;
self.mcp_startup_allow_terminal_only_next_round = false;
@@ -279,39 +309,21 @@ impl ChatWidget {
}
pub(crate) fn finish_mcp_startup_after_lag(&mut self) {
if self.mcp_startup_status.is_some() {
self.finish_mcp_startup();
}
if let Some(ExpectedMcpServers::ReportedOnly { last_diagnostics }) =
&mut self.mcp_startup_expected_servers
{
// A missed Starting may have begun a new attempt with the same error.
last_diagnostics.clear();
}
if self.mcp_startup_ignore_updates_until_next_start {
if self.mcp_startup_pending_next_round.is_empty() {
self.mcp_startup_pending_next_round_saw_starting = false;
}
self.mcp_startup_allow_terminal_only_next_round = true;
}
let Some(current) = &self.mcp_startup_status else {
return;
};
let mut failed = Vec::new();
let mut cancelled = Vec::new();
let mut server_names: BTreeSet<String> = current.keys().cloned().collect();
if let Some(expected_servers) = &self.mcp_startup_expected_servers {
server_names.extend(expected_servers.iter().cloned());
}
for name in server_names {
match current.get(&name) {
Some(McpStartupStatus::Failed { .. }) => failed.push(name),
Some(McpStartupStatus::Cancelled) => cancelled.push(name),
Some(McpStartupStatus::Ready | McpStartupStatus::Starting) | None => {}
}
}
failed.sort();
failed.dedup();
cancelled.sort();
cancelled.dedup();
self.finish_mcp_startup(failed, cancelled);
self.mcp_startup_allow_terminal_only_next_round = true;
}
pub(super) fn status_header_is_mcp_startup_owned(&self) -> bool {
@@ -357,11 +369,7 @@ impl ChatWidget {
},
McpServerStartupState::Cancelled => McpStartupStatus::Cancelled,
};
self.update_mcp_startup_status(
notification.name,
status,
/*complete_when_settled*/ true,
);
self.update_mcp_startup_status(notification.name, status);
if refresh_connector_mentions {
self.refresh_connector_mentions(/*force_refresh*/ false);
}
@@ -346,7 +346,7 @@ async fn pending_mcp_startup_does_not_block_queued_follow_up() {
}
]);
chat.finish_mcp_startup(Vec::new(), Vec::new());
chat.finish_mcp_startup();
assert!(chat.input_queue.user_turn_pending_start);
}
@@ -462,7 +462,7 @@ async fn pending_mcp_startup_does_not_unblock_external_review() {
@"■ '/fork' is disabled while a task is in progress."
);
chat.finish_mcp_startup(Vec::new(), Vec::new());
chat.finish_mcp_startup();
assert!(chat.bottom_pane.is_task_running());
assert!(chat.bottom_pane.status_indicator_visible());
assert_eq!(chat.input_queue.queued_user_messages.len(), 1);
@@ -489,7 +489,7 @@ async fn pending_mcp_startup_does_not_unblock_foreground_shell() {
assert_eq!(chat.input_queue.queued_user_messages.len(), 1);
assert_no_submit_op(&mut op_rx);
chat.finish_mcp_startup(Vec::new(), Vec::new());
chat.finish_mcp_startup();
assert!(chat.bottom_pane.is_task_running());
assert_eq!(chat.input_queue.queued_user_messages.len(), 1);
assert_no_submit_op(&mut op_rx);
@@ -808,6 +808,20 @@ async fn app_server_mcp_startup_reports_failure_after_lag() {
assert!(!warning_text.contains("MCP startup interrupted"));
}
#[tokio::test]
async fn reported_mcp_startup_rechecks_an_identical_failure_after_lag() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.track_reported_mcp_startup_servers();
notify_mcp_status_error(&mut chat, "alpha", "handshake failed");
let first = drain_insert_history(&mut rx);
assert!(!first.is_empty());
chat.finish_mcp_startup_after_lag();
notify_mcp_status_error(&mut chat, "alpha", "handshake failed");
assert_eq!(drain_insert_history(&mut rx), first);
assert!(!chat.bottom_pane.is_task_running());
}
#[tokio::test]
async fn app_server_mcp_startup_reports_cancellation_after_lag() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
@@ -988,7 +1002,7 @@ async fn app_server_mcp_startup_next_round_with_empty_expected_servers_reactivat
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.show_welcome_banner = false;
chat.set_mcp_startup_expected_servers(std::iter::empty::<String>());
chat.finish_mcp_startup(Vec::new(), Vec::new());
chat.finish_mcp_startup();
notify_mcp_status(&mut chat, "runtime", McpServerStartupState::Starting);
assert!(drain_insert_history(&mut rx).is_empty());
@@ -1521,7 +1521,7 @@ async fn plan_slash_command_with_hidden_shell_paste_queued_before_session_submit
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
chat.queue_user_message(UserMessage::from("queued follow-up"));
chat.finish_mcp_startup(Vec::new(), Vec::new());
chat.finish_mcp_startup();
assert_eq!(
chat.queued_user_message_texts(),
vec![format!("/plan {payload}"), "queued follow-up".to_string()]
@@ -1547,7 +1547,7 @@ async fn rejected_initial_image_does_not_submit_later_queued_prompt() {
initial_message.remote_image_urls = vec!["https://example.com/image.png".to_string()];
chat.initial_user_message = Some(initial_message);
chat.queue_user_message(UserMessage::from("queued follow-up"));
chat.finish_mcp_startup(Vec::new(), Vec::new());
chat.finish_mcp_startup();
let mut session = plan_test_session(ThreadId::new());
session.model = current_model;