From 7dae8c53d97e61cd774e4d6bcca5243c29ca615c Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Thu, 24 Sep 2026 02:00:24 +0000 Subject: [PATCH] Avoid recursive TUI event dispatch for model picker selections (#47717) ## Why Confirming an Astra model selection recursively polled the large event dispatcher on the TUI thread's stack. ## What changed Unwrap `AstraSelectedFromModelPicker` before dispatching its model update, then trigger the sparkle afterward when eligible. Keep the eligibility check and model update in the same event so queued backend updates cannot turn a no-op confirmation into a sparkle. ## Testing Add a regression test that confirms `gpt-6-astra` with `XHigh` reasoning on a thread with a 4 MiB stack and checks the resulting model and reasoning effort. GitOrigin-RevId: 89df8b417a9fd0107ebb918edeb2d240b53d42bd --- codex-rs/tui/src/app/event_dispatch.rs | 36 +++++----- .../tui/src/app/tests/fresh_sparkle_tests.rs | 72 +++++++++++++++++++ 2 files changed, 92 insertions(+), 16 deletions(-) diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 7fc9161a24..94d35eee71 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -102,6 +102,21 @@ impl App { return Ok(AppRunControl::Continue); } + // Keep the picker check and model update in one event without recursively polling this + // large dispatcher on the TUI thread's stack. + let (event, sparkle_model) = match event { + AppEvent::AstraSelectedFromModelPicker { + thread_id, + model, + action, + } => { + let should_offer = self.chat_widget.current_model() != model + && self.chat_widget.sparkle_thread_for_picker_action(&model) == Some(thread_id); + let next_event = action.into_app_event(model.clone()); + (next_event, should_offer.then_some(model)) + } + event => (event, None), + }; match event { AppEvent::OpenDaemonMenu => self.open_daemon_menu(), AppEvent::ConfirmDaemonUpdate(source) => self.confirm_daemon_update(source), @@ -1904,22 +1919,7 @@ impl App { .await; } } - AppEvent::AstraSelectedFromModelPicker { thread_id, model, action } => { - // Check and apply in the same event so a queued backend update cannot turn a - // no-op picker confirmation into a sparkle. - let should_offer = self.chat_widget.current_model() != model - && self.chat_widget.sparkle_thread_for_picker_action(&model) == Some(thread_id); - let control = Box::pin(self.handle_event( - tui, - app_server, - action.into_app_event(model.clone()), - )) - .await?; - if should_offer { - self.chat_widget.on_sparkle_model_selected_from_picker(&model); - } - return Ok(control); - } + AppEvent::AstraSelectedFromModelPicker { .. } => unreachable!("picker event unwrapped"), AppEvent::BackgroundVoiceError { thread_id, message } => { if self.chat_widget.thread_id() == Some(thread_id) { self.chat_widget.add_error_message(message); @@ -3172,6 +3172,10 @@ impl App { } } } + if let Some(model) = sparkle_model { + self.chat_widget + .on_sparkle_model_selected_from_picker(&model); + } Ok(AppRunControl::Continue) } diff --git a/codex-rs/tui/src/app/tests/fresh_sparkle_tests.rs b/codex-rs/tui/src/app/tests/fresh_sparkle_tests.rs index 9885fa5f8a..714e9bd22b 100644 --- a/codex-rs/tui/src/app/tests/fresh_sparkle_tests.rs +++ b/codex-rs/tui/src/app/tests/fresh_sparkle_tests.rs @@ -571,6 +571,78 @@ async fn astra_picker_confirms_the_model_at_application_after_an_automatic_updat Ok(()) } +#[test] +fn astra_picker_xhigh_applies_on_a_bounded_stack() -> Result<()> { + const STACK_SIZE: usize = 4 * 1024 * 1024; + const SERVER_STACK_SIZE: usize = 8 * 1024 * 1024; + const WORKER_THREADS: usize = 1; + + std::thread::Builder::new() + .name("astra-picker".into()) + .stack_size(STACK_SIZE) + .spawn(|| { + let runtime = tokio::runtime::Builder::new_multi_thread() + .worker_threads(WORKER_THREADS) + .thread_stack_size(SERVER_STACK_SIZE) + .enable_all() + .build()?; + runtime.block_on(Box::pin(async { + let (mut app, mut events, _ops) = Box::pin(make_test_app_with_channels()).await; + app.config.model = Some("gpt-5.5".into()); + let mut server = Box::pin(start_config_write_test_app_server(&app)).await?; + let mut tui = crate::tui::test_support::make_test_tui()?; + let thread = server.start_thread(&app.config).await?; + Box::pin(app.replace_chat_widget_with_app_server_thread( + &mut tui, + thread, + ThreadAttachPresentation::Fresh, + /*initial_user_message*/ None, + )) + .await?; + + let mut preset = crate::test_support::TEST_MODEL_PRESETS[0].clone(); + preset.model = "gpt-6-astra".into(); + preset.default_reasoning_effort = ReasoningEffortConfig::XHigh; + preset.supported_reasoning_efforts = vec![ + ReasoningEffortPreset { + effort: ReasoningEffortConfig::XHigh, + description: "Extra High".into(), + }, + ReasoningEffortPreset { + effort: ReasoningEffortConfig::Low, + description: "Low".into(), + }, + ]; + app.chat_widget.open_reasoning_popup(preset); + app.chat_widget.handle_key_event(KeyCode::Enter.into()); + let mut confirmed = false; + while let Ok(event) = events.try_recv() { + confirmed |= matches!(event, AppEvent::AstraSelectedFromModelPicker { .. }); + if matches!( + &event, + AppEvent::AstraSelectedFromModelPicker { .. } + | AppEvent::UpdateReasoningEffort(_) + | AppEvent::PersistModelSelection { .. } + ) { + Box::pin(app.handle_event(&mut tui, &mut server, event)).await?; + } + } + assert!(confirmed); + assert_eq!( + ( + app.chat_widget.current_model(), + app.chat_widget.current_reasoning_effort(), + ), + ("gpt-6-astra", Some(ReasoningEffortConfig::XHigh)), + ); + server.shutdown().await?; + Ok(()) + })) + })? + .join() + .expect("Astra picker thread panicked") +} + #[tokio::test] async fn session_only_astra_picker_shows_stars_only_on_an_untouched_task() -> Result<()> { let (mut app, mut events, _ops) = make_test_app_with_channels().await;