test(e2e): harden PTY session cleanup

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>

Author: rominf

tests/e2e-cucumber/tests/e2e.rs

@@ -192,29 +192,15 @@ impl Default for E2eWorld {
 
 impl E2eWorld {
     /// The isolation/behaviour environment every spawned `rocm` gets, as owned
-    /// `(key, value)` pairs. One source of truth for both the piped
-    /// `std::process::Command` path (`isolate_cmd`) and the pseudo-terminal path
-    /// (`tui_driver`, whose `portable_pty::CommandBuilder` has its own env API and
-    /// can't take a `std::process::Command`) — so the interactive and
-    /// non-interactive spawns can never drift.
+    /// `(key, value)` pairs. Shared by the piped `std::process::Command` path and
+    /// the pseudo-terminal path; PTY-only isolation is added by [`pty_env`].
     pub fn isolate_env(&self) -> Vec<(&'static str, std::ffi::OsString)> {
         let mut env = Vec::new();
         if let Some(root) = &self.isolated_root {
             let root = root.path();
             env.push(("ROCM_CLI_CONFIG_DIR", root.join("config").into_os_string()));
             env.push(("ROCM_CLI_DATA_DIR", root.join("data").into_os_string()));
             env.push(("ROCM_CLI_CACHE_DIR", root.join("cache").into_os_string()));
-            // The dashboard socket default is derived from HOME/XDG rather than
-            // AppPaths, so isolate it too; otherwise a host daemon can answer
-            // the scenario and hide the planted service registry. Create both
-            // directories because some PTY implementations use HOME as the
-            // child's working directory.
-            let home = root.join("home");
-            let runtime = root.join("runtime");
-            std::fs::create_dir_all(&home).expect("failed to create isolated HOME");
-            std::fs::create_dir_all(&runtime).expect("failed to create isolated runtime dir");
-            env.push(("HOME", home.into_os_string()));
-            env.push(("XDG_RUNTIME_DIR", runtime.into_os_string()));
         }
         // Share only STATE-FREE, content-addressed caches across scenarios when
         // CI provides a persistent shared dir (see shared_cache_dir): HF model
@@ -253,6 +239,24 @@ impl E2eWorld {
         env
     }
 
+    /// Additional isolation for interactive sessions. The dashboard socket
+    /// default is derived from HOME/XDG rather than the CLI AppPaths, so a PTY
+    /// must not inherit the host daemon's socket location. Piped scenarios keep
+    /// their historical HOME/XDG environment, including GPU/runtime defaults.
+    pub fn pty_env(&self) -> Vec<(&'static str, std::ffi::OsString)> {
+        let Some(root) = &self.isolated_root else {
+            return Vec::new();
+        };
+        let home = root.path().join("home");
+        let runtime = root.path().join("runtime");
+        std::fs::create_dir_all(&home).expect("failed to create isolated HOME");
+        std::fs::create_dir_all(&runtime).expect("failed to create isolated runtime dir");
+        vec![
+            ("HOME", home.into_os_string()),
+            ("XDG_RUNTIME_DIR", runtime.into_os_string()),
+        ]
+    }
+
     pub fn isolate_cmd(&self, cmd: &mut std::process::Command) {
         for (key, value) in self.isolate_env() {
             cmd.env(key, value);

tests/e2e-cucumber/tests/e2e/dash_steps.rs

@@ -103,6 +103,8 @@ async fn open_dashboard(world: &mut E2eWorld) {
 
 #[when("the user opens the ROCm view")]
 async fn open_rocm_view(world: &mut E2eWorld) {
+    // Dashboard tabs are currently ordered Home, ROCm, Serving, Observe; these
+    // numeric shortcuts intentionally exercise that user-visible ordering.
     session(world)
         .send("2")
         .unwrap_or_else(|e| panic!("failed to switch to the ROCm tab: {e}"));
@@ -141,6 +143,8 @@ async fn open_command_palette(world: &mut E2eWorld) {
 #[when("the user chooses Serving")]
 async fn choose_serving(world: &mut E2eWorld) {
     let tui = session(world);
+    // The palette initially selects Home; Serving is the third destination, so
+    // two downward moves intentionally assert the current destination ordering.
     tui.send("jj")
         .unwrap_or_else(|e| panic!("failed to select Serving: {e}"));
     tui.send("\r")

tests/e2e-cucumber/tests/e2e/tui_driver.rs

@@ -45,6 +45,9 @@ const DETAIL_COLS: u16 = 120;
 /// poll cadence, not a fixed readiness sleep: every wait has a deadline and
 /// returns the instant its condition holds.
 const POLL_INTERVAL: Duration = Duration::from_millis(20);
+/// Maximum time to let the PTY reader consume the child's final frame after the
+/// process exits. This is bounded so a misbehaving PTY cannot stall a scenario.
+const DRAIN_TIMEOUT: Duration = Duration::from_millis(250);
 
 /// Default wall-clock budget for a single wait. Generous enough for a cold dash
 /// start plus the embedded-daemon connect, while still turning a genuine hang
@@ -108,7 +111,7 @@ impl TuiSession {
         for (key, value) in std::env::vars_os() {
             cmd.env(key, value);
         }
-        for (key, value) in world.isolate_env() {
+        for (key, value) in world.isolate_env().into_iter().chain(world.pty_env()) {
             cmd.env(key, value);
         }
         // Deterministic terminal type; the PTY ioctl size above is authoritative
@@ -157,12 +160,21 @@ impl TuiSession {
     /// already resolved escape sequences and styling into cells, so this is
     /// exactly what a user sees — color/attribute independent.
     pub fn screen_text(&self) -> String {
+        self.screen_snapshot().0
+    }
+
+    fn screen_snapshot(&self) -> (String, (u16, u16)) {
         self.parser
             .lock()
-            .map(|p| p.screen().contents())
+            .map(|p| (p.screen().contents(), p.screen().size()))
             .unwrap_or_default()
     }
 
+    fn framed_screen(&self) -> String {
+        let (screen, (rows, cols)) = self.screen_snapshot();
+        format!("--- last screen ({cols}x{rows}) ---\n{screen}\n--- end screen ---")
+    }
+
     /// Resize both the real PTY and the emulated screen. The application receives
     /// the normal terminal resize event; assertions continue to inspect exactly
     /// what a user would see at the new geometry.
@@ -201,24 +213,41 @@ impl TuiSession {
             if self.screen_text().contains(marker) {
                 return Ok(());
             }
-            // If the process is gone, give the reader a beat to drain any final
-            // bytes, then check once more before declaring failure.
+            // If the process is gone, let the reader drain the final frame for a
+            // short bounded window. A single poll is not enough when a large frame
+            // is still buffered behind the process exit notification.
             if let Ok(Some(status)) = self.child.try_wait() {
                 self.finished = true;
-                tokio::time::sleep(POLL_INTERVAL).await;
-                let screen = self.screen_text();
-                if screen.contains(marker) {
+                self.record_once(i32::try_from(status.exit_code()).unwrap_or(-1));
+                let drain_deadline = Instant::now() + DRAIN_TIMEOUT;
+                while Instant::now() < drain_deadline {
+                    if self.screen_text().contains(marker) {
+                        return Ok(());
+                    }
+                    if self
+                        .reader
+                        .as_ref()
+                        .is_some_and(std::thread::JoinHandle::is_finished)
+                    {
+                        break;
+                    }
+                    tokio::time::sleep(POLL_INTERVAL).await;
+                }
+                // Final check after the drain window closes: the reader may have
+                // committed the last frame between the loop's screen check and the
+                // `is_finished`/deadline exit, so re-read before declaring failure.
+                if self.screen_text().contains(marker) {
                     return Ok(());
                 }
                 return Err(format!(
                     "process exited ({status:?}) before {marker:?} appeared.\n{}",
-                    framed_screen(&screen)
+                    self.framed_screen()
                 ));
             }
             if Instant::now() >= deadline {
                 return Err(format!(
                     "timed out after {timeout:?} waiting for {marker:?}.\n{}",
-                    framed_screen(&self.screen_text())
+                    self.framed_screen()
                 ));
             }
             tokio::time::sleep(POLL_INTERVAL).await;
@@ -250,7 +279,7 @@ impl TuiSession {
                     } else {
                         Err(format!(
                             "TUI exited unsuccessfully ({status:?}).\n{}",
-                            framed_screen(&self.screen_text())
+                            self.framed_screen()
                         ))
                     };
                 }
@@ -260,7 +289,7 @@ impl TuiSession {
             if Instant::now() >= deadline {
                 return Err(format!(
                     "timed out after {timeout:?} waiting for the TUI to exit.\n{}",
-                    framed_screen(&self.screen_text())
+                    self.framed_screen()
                 ));
             }
             tokio::time::sleep(POLL_INTERVAL).await;
@@ -288,8 +317,14 @@ impl Drop for TuiSession {
         // without an explicit quit (e.g. the consent-gate scenarios).
         if !self.finished {
             let _ = self.child.kill();
-            let _ = self.child.wait();
+            let rc = self
+                .child
+                .wait()
+                .ok()
+                .and_then(|status| i32::try_from(status.exit_code()).ok())
+                .unwrap_or(-1);
             self.finished = true;
+            self.record_once(rc);
         }
         self.reader_stop.store(true, Ordering::Relaxed);
         if let Some(handle) = self.reader.take() {
@@ -327,8 +362,3 @@ fn spawn_reader(
         }
     })
 }
-
-/// Wrap a screen dump in delimiters so failure messages are easy to read.
-fn framed_screen(screen: &str) -> String {
-    format!("--- last screen ({COLS}x{ROWS}) ---\n{screen}\n--- end screen ---")
-}