From d4f3d22b602451fbf4300f57a46552e23a757788 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 20 Oct 2026 07:43:57 +0000 Subject: [PATCH 1/2] test(e2e): stop quit_and_wait deadlocking on a slow quit prompt quit_and_wait resends the quit key every 500 ms and answers the quit-confirm prompt with "y" once it sees it. When the dashboard takes between one and two resend intervals to act on a key, every decision is made with one earlier "q" still in flight. That "q" flips the prompt after the screen was read, so the chosen key lands on the other state and the loop repeats "q, q, y" until the 30 s deadline. On a loaded mock lane this timed out dash-26 with the prompt open on screen. Double the wait after each attempt instead. Once it outlasts the TUI's latency nothing is in flight when the screen is read, so the key sent matches the state it lands on. The first resend still happens after 500 ms, so a quit key swallowed by a transient state is retried as quickly as before. The loop moves into the e2e-cucumber library, next to send_until, so a unit test can drive it against a simulated dashboard with a fixed key latency: it fails with the old fixed cadence and passes with this one. Signed-off-by: Roman Inflianskas --- tests/e2e-cucumber/src/lib.rs | 1 + tests/e2e-cucumber/src/quit_and_wait.rs | 238 +++++++++++++++++++++ tests/e2e-cucumber/tests/e2e/tui_driver.rs | 84 ++++---- 3 files changed, 285 insertions(+), 38 deletions(-) create mode 100644 tests/e2e-cucumber/src/quit_and_wait.rs diff --git a/tests/e2e-cucumber/src/lib.rs b/tests/e2e-cucumber/src/lib.rs index 499e9a072..0d1dc1ba8 100644 --- a/tests/e2e-cucumber/src/lib.rs +++ b/tests/e2e-cucumber/src/lib.rs @@ -15,6 +15,7 @@ pub mod model_id; pub mod monotonic_clock; pub mod paced_download; pub mod panic_capture; +pub mod quit_and_wait; pub mod reader_failure; pub mod send_until; pub mod serve_log; diff --git a/tests/e2e-cucumber/src/quit_and_wait.rs b/tests/e2e-cucumber/src/quit_and_wait.rs new file mode 100644 index 000000000..ea99348ab --- /dev/null +++ b/tests/e2e-cucumber/src/quit_and_wait.rs @@ -0,0 +1,238 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Retry loop for quitting a TUI that may open a quit-confirm prompt first. + +use std::future::Future; +use std::pin::Pin; +use std::time::{Duration, Instant}; + +use crate::send_until::RetryTiming; + +/// Key that approves the dashboard's quit-confirm prompt. +pub const QUIT_CONFIRM_KEY: &str = "y"; + +/// What one bounded wait for the child to exit observed. +#[derive(Debug, PartialEq, Eq)] +pub enum ExitPoll { + /// The child exited, or the session hit a terminal error; the loop stops + /// and returns this result. + Finished(Result<(), String>), + /// The child is still running. + Running, +} + +/// Borrowing future returned by the exit wait used by [`quit_and_wait`]. +pub type ExitFuture<'a> = Pin + Send + 'a>>; + +/// Why [`quit_and_wait`] gave up. +#[derive(Debug, PartialEq, Eq)] +pub enum QuitError { + /// The child exited unsuccessfully, or the session failed. + Failed(String), + /// The child was still running at the deadline. + TimedOut, +} + +/// Send `gesture` until the child exits, answering the quit-confirm prompt +/// with [`QUIT_CONFIRM_KEY`] whenever `confirming` reports it on screen. +/// +/// The gesture is resent because a bare send can be consumed by a transient +/// state and never reach the quit handler. Resending is not idempotent once +/// the prompt can open, though: the prompt reads the gesture as Cancel. If the +/// TUI takes longer than the resend interval to act on a key, every decision is +/// made with an earlier `gesture` still in flight, which flips the prompt after +/// the screen was read — at a fixed cadence that can repeat until the deadline. +/// So the wait doubles after each attempt: once it outlasts the TUI's latency, +/// nothing is in flight when the screen is read, and the key sent matches it. +pub async fn quit_and_wait( + state: &mut S, + gesture: &str, + timing: RetryTiming, + mut confirming: Confirming, + mut send: SendInput, + mut wait: Wait, +) -> Result<(), QuitError> +where + S: Send, + Confirming: FnMut(&S) -> bool + Send, + SendInput: FnMut(&mut S, &str) -> Result<(), String> + Send, + Wait: for<'a> FnMut(&'a mut S, Duration) -> ExitFuture<'a> + Send, +{ + let RetryTiming { + timeout, + resend_interval, + } = timing; + let deadline = Instant::now() + timeout; + let mut interval = resend_interval; + loop { + let key = if confirming(state) { + QUIT_CONFIRM_KEY + } else { + gesture + }; + send(state, key).map_err(QuitError::Failed)?; + let remaining = deadline.saturating_duration_since(Instant::now()); + match wait(state, interval.min(remaining)).await { + ExitPoll::Finished(result) => return result.map_err(QuitError::Failed), + ExitPoll::Running => {} + } + if Instant::now() >= deadline { + return Err(QuitError::TimedOut); + } + interval = interval.saturating_mul(2); + } +} + +#[cfg(test)] +mod tests { + use super::{ExitPoll, QUIT_CONFIRM_KEY, QuitError, quit_and_wait}; + use crate::send_until::RetryTiming; + use std::time::{Duration, Instant}; + + /// A dashboard whose prompt and exit only reflect a key `latency` after it + /// was sent, as on a loaded host: `q` toggles the prompt (it opens it, and + /// the open prompt reads `q` as Cancel), `y` on the open prompt quits, and + /// `y` with no prompt is ignored. + struct SlowDashboard { + latency: Duration, + sent: Vec<(Instant, String)>, + } + + impl SlowDashboard { + fn new(latency: Duration) -> Self { + Self { + latency, + sent: Vec::new(), + } + } + + /// `(prompt open, exited)` after every key whose latency has elapsed. + fn observed(&self) -> (bool, bool) { + let now = Instant::now(); + let mut prompt = false; + for (at, key) in &self.sent { + if *at + self.latency > now { + break; + } + match key.as_str() { + "q" => prompt = !prompt, + QUIT_CONFIRM_KEY if prompt => return (true, true), + _ => {} + } + } + (prompt, false) + } + } + + async fn quit(dashboard: &mut SlowDashboard, timing: RetryTiming) -> Result<(), QuitError> { + quit_and_wait( + dashboard, + "q", + timing, + |d| d.observed().0, + |d, key| { + d.sent.push((Instant::now(), key.to_string())); + Ok(()) + }, + |d, budget| { + Box::pin(async move { + let deadline = Instant::now() + budget; + loop { + if d.observed().1 { + return ExitPoll::Finished(Ok(())); + } + if Instant::now() >= deadline { + return ExitPoll::Running; + } + tokio::time::sleep(Duration::from_millis(2)).await; + } + }) + }, + ) + .await + } + + #[tokio::test(flavor = "current_thread")] + async fn a_prompt_that_renders_within_the_resend_interval_is_confirmed() { + let mut dashboard = SlowDashboard::new(Duration::from_millis(10)); + let timing = RetryTiming { + timeout: Duration::from_secs(3), + resend_interval: Duration::from_millis(50), + }; + + quit(&mut dashboard, timing) + .await + .expect("a responsive prompt must be confirmed"); + + let keys: Vec<&str> = dashboard.sent.iter().map(|(_, k)| k.as_str()).collect(); + assert_eq!(keys, ["q", QUIT_CONFIRM_KEY]); + } + + #[tokio::test(flavor = "current_thread")] + async fn a_prompt_slower_than_the_resend_interval_is_still_confirmed() { + // Each key takes between one and two resend intervals to show, so a + // fixed cadence always has exactly one `q` in flight when it decides: + // that `q` flips the prompt after the decision, the decided key lands on + // the other state, and the two stay out of phase until the deadline. + let mut dashboard = SlowDashboard::new(Duration::from_millis(80)); + let timing = RetryTiming { + timeout: Duration::from_secs(3), + resend_interval: Duration::from_millis(50), + }; + + quit(&mut dashboard, timing).await.unwrap_or_else(|e| { + let keys: Vec<&str> = dashboard.sent.iter().map(|(_, k)| k.as_str()).collect(); + panic!("slow prompt must still be confirmed, got {e:?} after {keys:?}") + }); + } + + #[tokio::test(flavor = "current_thread")] + async fn an_unsuccessful_exit_is_returned_without_resending() { + let mut sends = 0; + let error = quit_and_wait( + &mut sends, + "q", + RetryTiming { + timeout: Duration::from_secs(1), + resend_interval: Duration::from_millis(10), + }, + |_| false, + |sends, _| { + *sends += 1; + Ok(()) + }, + |_, _| Box::pin(async { ExitPoll::Finished(Err("exit code 1".to_string())) }), + ) + .await + .expect_err("an unsuccessful exit must fail"); + + assert_eq!(error, QuitError::Failed("exit code 1".to_string())); + assert_eq!(sends, 1); + } + + #[tokio::test(flavor = "current_thread")] + async fn a_child_that_never_exits_times_out() { + let error = quit_and_wait( + &mut (), + "q", + RetryTiming { + timeout: Duration::from_millis(60), + resend_interval: Duration::from_millis(10), + }, + |()| false, + |(), _| Ok(()), + |(), budget| { + Box::pin(async move { + tokio::time::sleep(budget).await; + ExitPoll::Running + }) + }, + ) + .await + .expect_err("a child that never exits must time out"); + + assert_eq!(error, QuitError::TimedOut); + } +} diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 092b54456..678f3d5c0 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -32,6 +32,7 @@ use portable_pty::{Child, CommandBuilder, MasterPty, PtySize, native_pty_system} use e2e_cucumber::paced_download::is_intermediate_download_progress_frame; use e2e_cucumber::panic_capture::panic_message; +use e2e_cucumber::quit_and_wait::{ExitPoll, QuitError, quit_and_wait as retry_quit_and_wait}; use e2e_cucumber::reader_failure::{ReaderFailure, ReaderFailureObservation}; use e2e_cucumber::send_until::{RetryTiming, TerminalState, send_until as retry_send_until}; @@ -697,13 +698,14 @@ impl TuiSession { /// the app has finished acting on whatever came just before it (e.g. right /// after an unsynchronized `send` like the Escape key), it can be consumed /// by that transient state and never reach the quit handler — the process - /// then never exits and this hangs until `timeout`. Re-sending on a short - /// cadence closes that gap the same way [`send_until`] does for screen - /// markers: if the first attempt landed (the common case), the process - /// has already exited by the first check and the loop below never reaches - /// a second send. What's resent each pass isn't always the same literal - /// `gesture` either — see the quit-confirm handling below, which switches - /// to `"y"` once the prompt is on screen. + /// then never exits and this hangs until `timeout`. Re-sending closes that + /// gap the same way [`send_until`] does for screen markers: if the first + /// attempt landed (the common case), the process has already exited by the + /// first check and the loop never reaches a second send. What's resent + /// each pass isn't always the same literal `gesture` either — see the + /// quit-confirm handling below, which switches to `"y"` once the prompt is + /// on screen — and the wait between sends doubles each pass, for the + /// reason [`e2e_cucumber::quit_and_wait::quit_and_wait`] gives. pub async fn quit_and_wait(&mut self, timeout: Duration) -> Result<(), String> { // Several fixtures leave a model serving (`simulated`/demo sessions // never do — `AppState::has_live_instance` always returns `false` @@ -719,38 +721,44 @@ impl TuiSession { // for `/quit\r` — type "uit" into chat as a message once the leading // `q` cancels the prompt. Either way it never exits. let gesture = if self.is_chat { "/quit\r" } else { "q" }; - let deadline = Instant::now() + timeout; - loop { - let confirming = self.screen_snapshot().0.contains(QUIT_CONFIRM_MARKER); - self.send(if confirming { "y" } else { gesture })?; - let remaining = deadline.saturating_duration_since(Instant::now()); - let attempt = KEY_RESEND_INTERVAL.min(remaining); - match self.wait_for_exit_code_within(attempt).await { - Some(code) => { - return if code == 0 { - Ok(()) - } else { - Err(format!( - "TUI exited unsuccessfully (code {code}).\n{}", - self.framed_screen() - )) - }; - } - None => { - if let Some(panic_message) = self.take_reader_panic() { - return Err(format!( - "pty reader thread panicked while waiting to quit: {panic_message}\n{}", - self.framed_screen() - )); - } - } - } - if Instant::now() >= deadline { - return Err(format!( - "timed out after {timeout:?} waiting for the TUI to exit after repeating {gesture:?}.\n{}", + let result = retry_quit_and_wait( + self, + gesture, + RetryTiming { + timeout, + resend_interval: KEY_RESEND_INTERVAL, + }, + |session| session.screen_snapshot().0.contains(QUIT_CONFIRM_MARKER), + Self::send, + |session, attempt| Box::pin(session.exit_poll_within(attempt)), + ) + .await; + match result { + Ok(()) => Ok(()), + Err(QuitError::Failed(error)) => Err(error), + Err(QuitError::TimedOut) => Err(format!( + "timed out after {timeout:?} waiting for the TUI to exit after repeating {gesture:?}.\n{}", + self.framed_screen() + )), + } + } + + /// One bounded wait of [`quit_and_wait`](Self::quit_and_wait): whether the + /// child exited within `budget` (and how), or the reader thread failed. + async fn exit_poll_within(&mut self, budget: Duration) -> ExitPoll { + match self.wait_for_exit_code_within(budget).await { + Some(0) => ExitPoll::Finished(Ok(())), + Some(code) => ExitPoll::Finished(Err(format!( + "TUI exited unsuccessfully (code {code}).\n{}", + self.framed_screen() + ))), + None => match self.take_reader_panic() { + Some(panic_message) => ExitPoll::Finished(Err(format!( + "pty reader thread panicked while waiting to quit: {panic_message}\n{}", self.framed_screen() - )); - } + ))), + None => ExitPoll::Running, + }, } } From 4f872dd4bb346dabdf3baba0822fb27de15a7594 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 20 Oct 2026 08:18:36 +0000 Subject: [PATCH 2/2] test(e2e): cover quit_and_wait's failed send and deadline clamp Both branches were untested: dropping the error from a failed send, or the clamp that cuts the last wait to the deadline, left every test green. The clamp matters more now the wait doubles, since an unclamped final wait can run as long as the whole step timeout past it. Signed-off-by: Roman Inflianskas --- tests/e2e-cucumber/src/quit_and_wait.rs | 60 +++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/e2e-cucumber/src/quit_and_wait.rs b/tests/e2e-cucumber/src/quit_and_wait.rs index ea99348ab..f8c34a7de 100644 --- a/tests/e2e-cucumber/src/quit_and_wait.rs +++ b/tests/e2e-cucumber/src/quit_and_wait.rs @@ -212,6 +212,66 @@ mod tests { assert_eq!(sends, 1); } + #[tokio::test(flavor = "current_thread")] + async fn a_failed_send_is_returned_without_waiting() { + let mut waits = 0; + let error = quit_and_wait( + &mut waits, + "q", + RetryTiming { + timeout: Duration::from_secs(1), + resend_interval: Duration::from_millis(10), + }, + |_| false, + |_, _| Err("pty closed".to_string()), + |waits, _| { + *waits += 1; + Box::pin(async { ExitPoll::Running }) + }, + ) + .await + .expect_err("a failed send must fail"); + + assert_eq!(error, QuitError::Failed("pty closed".to_string())); + assert_eq!( + waits, 0, + "nothing was sent, so there is nothing to wait for" + ); + } + + #[tokio::test(flavor = "current_thread")] + async fn a_wait_longer_than_the_time_left_is_cut_to_the_deadline() { + // Without the clamp a wait runs its whole interval past the deadline, + // and with doubling that can be as long as the timeout itself. + let timeout = Duration::from_millis(100); + let started = Instant::now(); + let error = quit_and_wait( + &mut (), + "q", + RetryTiming { + timeout, + resend_interval: Duration::from_secs(5), + }, + |()| false, + |(), _| Ok(()), + |(), budget| { + Box::pin(async move { + tokio::time::sleep(budget).await; + ExitPoll::Running + }) + }, + ) + .await + .expect_err("a child that never exits must time out"); + + assert_eq!(error, QuitError::TimedOut); + assert!( + started.elapsed() < Duration::from_secs(2), + "the wait must stop at the {timeout:?} deadline, took {:?}", + started.elapsed() + ); + } + #[tokio::test(flavor = "current_thread")] async fn a_child_that_never_exits_times_out() { let error = quit_and_wait(