From 17df7545a34ac533eedf5b628f49f2f1ad60e44e Mon Sep 17 00:00:00 2001 From: jif Date: Tue, 4 Aug 2026 13:24:05 +0000 Subject: [PATCH] Handle late MCP startup results after lag timeout (#36895) ## Why MCP startup status delivery can lag. Treating a server that is still starting or has not reported yet as cancelled produces a misleading interruption warning and can hide its eventual terminal result. ## What changed - Only report an interrupted startup when a server explicitly enters the cancelled state. - Allow a complete set of terminal updates received after the lag timeout to be promoted and reported, including failures and cancellations. ## Testing Added TUI tests covering unresolved servers at the lag timeout and late failure and cancellation updates. GitOrigin-RevId: 555f3d13ac87bda6e9b0bf72335a1c04851ac25a --- codex-rs/tui/src/chatwidget/mcp_startup.rs | 7 +- .../tui/src/chatwidget/tests/mcp_startup.rs | 89 ++++++++++++++++++- 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/chatwidget/mcp_startup.rs b/codex-rs/tui/src/chatwidget/mcp_startup.rs index 170faa992d..48f001099f 100644 --- a/codex-rs/tui/src/chatwidget/mcp_startup.rs +++ b/codex-rs/tui/src/chatwidget/mcp_startup.rs @@ -236,11 +236,9 @@ impl ChatWidget { for name in server_names { match current.get(&name) { - Some(McpStartupStatus::Ready) => {} Some(McpStartupStatus::Failed { .. }) => failed.push(name), - Some(McpStartupStatus::Cancelled | McpStartupStatus::Starting) | None => { - cancelled.push(name); - } + Some(McpStartupStatus::Cancelled) => cancelled.push(name), + Some(McpStartupStatus::Ready | McpStartupStatus::Starting) | None => {} } } @@ -249,6 +247,7 @@ impl ChatWidget { 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 { diff --git a/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs b/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs index 081f65e4cb..5614bade00 100644 --- a/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs +++ b/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs @@ -488,7 +488,11 @@ async fn mcp_startup_complete_preserves_review_status() { async fn app_server_mcp_startup_lag_settles_startup_and_ignores_late_updates() { 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(["alpha".to_string(), "beta".to_string()]); + chat.set_mcp_startup_expected_servers([ + "alpha".to_string(), + "beta".to_string(), + "gamma".to_string(), + ]); notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Starting); notify_mcp_status_error( @@ -507,9 +511,9 @@ async fn app_server_mcp_startup_lag_settles_startup_and_ignores_late_updates() { .iter() .map(|lines| lines_to_single_string(lines)) .collect::(); - assert!(summary_text.contains("MCP startup interrupted")); - assert!(summary_text.contains("beta")); - assert!(summary_text.contains("MCP startup incomplete (failed: alpha)")); + insta::assert_snapshot!(summary_text, @r" +⚠ MCP startup incomplete (failed: alpha) +"); assert!(!chat.bottom_pane.is_task_running()); notify_mcp_status(&mut chat, "beta", McpServerStartupState::Starting); @@ -523,6 +527,83 @@ async fn app_server_mcp_startup_lag_settles_startup_and_ignores_late_updates() { assert!(!chat.bottom_pane.is_task_running()); } +#[tokio::test] +async fn app_server_mcp_startup_lag_reports_only_explicit_cancellations() { + 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([ + "alpha".to_string(), + "beta".to_string(), + "gamma".to_string(), + ]); + + notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Starting); + notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Cancelled); + notify_mcp_status(&mut chat, "beta", McpServerStartupState::Starting); + + let _ = drain_insert_history(&mut rx); + assert!(chat.bottom_pane.is_task_running()); + + chat.finish_mcp_startup_after_lag(); + + let summary_text = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_eq!( + summary_text, + "⚠ MCP startup interrupted. The following servers were not initialized: alpha\n" + ); + assert!(!chat.bottom_pane.is_task_running()); +} + +#[tokio::test] +async fn app_server_mcp_startup_reports_failure_after_lag() { + 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(["alpha".to_string()]); + notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Starting); + + chat.finish_mcp_startup_after_lag(); + assert!(drain_insert_history(&mut rx).is_empty()); + + notify_mcp_status_error( + &mut chat, + "alpha", + "MCP client for `alpha` failed to start: handshake failed", + ); + + let warning_text = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert!(warning_text.contains("MCP client for `alpha` failed to start: handshake failed")); + assert!(warning_text.contains("MCP startup incomplete (failed: alpha)")); + assert!(!warning_text.contains("MCP startup interrupted")); +} + +#[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; + chat.show_welcome_banner = false; + chat.set_mcp_startup_expected_servers(["alpha".to_string()]); + notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Starting); + + chat.finish_mcp_startup_after_lag(); + assert!(drain_insert_history(&mut rx).is_empty()); + + notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Cancelled); + + let warning_text = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_eq!( + warning_text, + "⚠ MCP startup interrupted. The following servers were not initialized: alpha\n" + ); +} + #[tokio::test] async fn app_server_mcp_startup_after_lag_can_settle_without_starting_updates() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;