From e0231e0c7d97be16e0806d9e544f4c2b016efc7f Mon Sep 17 00:00:00 2001 From: wan9chi Date: Mon, 12 Oct 2026 15:27:48 +0800 Subject: [PATCH] test: wait for each Windows milestone to be seen before the next ConPTY emits only the latest console title when it renders, so a milestone could be replaced by another process's milestone before then. On Windows, `mark_milestone` now holds a lock shared by the processes in the test terminal from setting its title until the harness, having read the title, signals an event named after the milestone's id. `TestTerminal::spawn` names the lock through `VT_PTY_TERMINAL_TEST_MILESTONE_LOCK`. This lets `pending_uploads` and `hide_pending_uploads` run on Windows. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + crates/vt_bin/src/vtt/stalled_remote_cache.rs | 4 - .../fixtures/remote_cache/snapshots.toml | 7 - crates/vt_pty_terminal_test/Cargo.toml | 3 + crates/vt_pty_terminal_test/README.md | 13 +- crates/vt_pty_terminal_test/src/lib.rs | 79 ++++++++-- .../vt_pty_terminal_test/tests/milestone.rs | 57 ++++++++ crates/vt_pty_terminal_test_client/Cargo.toml | 2 +- crates/vt_pty_terminal_test_client/src/lib.rs | 138 +++++++++++++++--- 9 files changed, 262 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9e3103998..ca755cc05 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5232,6 +5232,7 @@ dependencies = [ "vt_pty_terminal", "vt_pty_terminal_test_client", "vt_subprocess_test", + "winapi", ] [[package]] diff --git a/crates/vt_bin/src/vtt/stalled_remote_cache.rs b/crates/vt_bin/src/vtt/stalled_remote_cache.rs index 51376f6ed..e09929f46 100644 --- a/crates/vt_bin/src/vtt/stalled_remote_cache.rs +++ b/crates/vt_bin/src/vtt/stalled_remote_cache.rs @@ -13,10 +13,6 @@ use std::{ /// forwarded or answered: each emits a "stalled" milestone when it arrives, /// and its connection is held until the client closes it. Other requests are /// forwarded, each on its own connection. Ctrl-C is left to the command. -/// -/// On Windows a milestone is the console title, which `ConPTY` sends when it -/// next renders, so if the command sets one at about the same time, the -/// earlier title can be lost. pub fn run(args: &[String]) -> Result<(), Box> { let mut args = args; let mut stalled_routes = Vec::new(); diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml index 74f5130f2..36e8c5f5f 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml @@ -341,10 +341,6 @@ steps = [ [[e2e]] name = "pending_uploads" -# Skipped on Windows, where a milestone is the console title and ConPTY sends -# only the latest one when it next renders: the proxy's `stalled` milestone for -# check's upload can replace `uploads-pending`. -cfg = "not(windows)" ignore = true steps = [ { argv = [ @@ -615,9 +611,6 @@ steps = [ [[e2e]] name = "hide_pending_uploads" -# Skipped on Windows, where a milestone is the console title and ConPTY sends -# it only when it next renders: nothing is printed after `uploads-pending`. -cfg = "not(windows)" ignore = true steps = [ { argv = [ diff --git a/crates/vt_pty_terminal_test/Cargo.toml b/crates/vt_pty_terminal_test/Cargo.toml index 48fd4386d..50c94b872 100644 --- a/crates/vt_pty_terminal_test/Cargo.toml +++ b/crates/vt_pty_terminal_test/Cargo.toml @@ -13,6 +13,9 @@ portable-pty = { workspace = true } vt_pty_terminal = { workspace = true } vt_pty_terminal_test_client = { workspace = true } +[target.'cfg(windows)'.dependencies] +winapi = { workspace = true, features = ["handleapi", "synchapi", "winnt"] } + [dev-dependencies] crossterm = { workspace = true } ctor = { workspace = true } diff --git a/crates/vt_pty_terminal_test/README.md b/crates/vt_pty_terminal_test/README.md index d9b5f374f..e66c2af25 100644 --- a/crates/vt_pty_terminal_test/README.md +++ b/crates/vt_pty_terminal_test/README.md @@ -53,11 +53,13 @@ vt-pty-terminal-test:<32-hex-random-id>: `Reader::expect_milestone` works like this: -1. Drain title events captured by `PtyReader`. +1. Drain title events captured by `PtyReader`, acknowledging each milestone. 2. Ignore ordinary titles and completed non-target milestones. 3. If no match exists, continue reading from the PTY and repeat. 4. Return the current screen once the requested title is observed. +`Reader::wait_for_exit` also acknowledges the milestones it reads. + ## Cross-platform behavior On Windows the client calls `SetConsoleTitleW`; ConPTY emits the resulting title @@ -65,6 +67,15 @@ through its asynchronous renderer after preceding text and cursor state. On Unix the client emits an OSC 2 title update, which follows normal PTY byte ordering. The same token decoder and test API are used on every platform. +ConPTY emits only the latest title when it renders, so a title set by another +process before then would replace a milestone. `TestTerminal::spawn` sets +`VT_PTY_TERMINAL_TEST_MILESTONE_LOCK` for the child on Windows. A producer +that inherits it holds a named lock shared by the processes in the terminal +from setting its title until the reader acknowledges the milestone by +signaling a named event derived from the milestone's random id. A process that +doesn't inherit it, such as a task that `vp` runs with a filtered environment, +only sets the title. + ## Typical test pattern In the child process: diff --git a/crates/vt_pty_terminal_test/src/lib.rs b/crates/vt_pty_terminal_test/src/lib.rs index bf139c8f7..2c45d132c 100644 --- a/crates/vt_pty_terminal_test/src/lib.rs +++ b/crates/vt_pty_terminal_test/src/lib.rs @@ -1,4 +1,4 @@ -use std::io::{BufReader, Read}; +use std::io::{BufReader, Read as _}; pub use portable_pty::CommandBuilder; use vt_pty_terminal::terminal::{PtyReader, Terminal}; @@ -32,6 +32,21 @@ impl TestTerminal { /// /// Returns an error if the PTY cannot be opened or the command fails to spawn. pub fn spawn(size: ScreenSize, cmd: CommandBuilder) -> anyhow::Result { + // Producers on Windows wait for each milestone to be seen while + // holding a lock shared by the processes in this terminal. + #[cfg(windows)] + let cmd = { + use std::sync::atomic::{AtomicU64, Ordering}; + + static NEXT_TERMINAL: AtomicU64 = AtomicU64::new(0); + let mut cmd = cmd; + let terminal = NEXT_TERMINAL.fetch_add(1, Ordering::Relaxed); + cmd.env( + vt_pty_terminal_test_client::MILESTONE_LOCK_ENV, + format!("{}-{terminal}", std::process::id()), + ); + cmd + }; let Terminal { pty_reader, pty_writer, child_handle, .. } = Terminal::spawn(size, cmd)?; Ok(Self { writer: pty_writer, @@ -61,6 +76,8 @@ impl Reader { /// /// Milestones use a uniform title token across platforms. /// + /// Every milestone read before the named one is skipped. + /// /// # Panics /// /// Panics if the child process exits (EOF) before the named milestone is received, @@ -70,12 +87,8 @@ impl Reader { let mut buf = [0u8; 4096]; loop { - while let Some(title) = self.pty.get_ref().take_window_title() { - if vt_pty_terminal_test_client::decode_milestone_title(&title) - .is_some_and(|milestone| milestone == name) - { - return self.screen_contents(); - } + if self.take_milestones().any(|milestone| milestone == name) { + return self.screen_contents(); } let n = self.pty.read(&mut buf).expect("PTY read failed"); @@ -83,6 +96,21 @@ impl Reader { } } + /// Takes the names of the milestones read so far, letting each producer + /// continue once its milestone is taken. + fn take_milestones(&self) -> impl Iterator { + std::iter::from_fn(|| { + loop { + let title = self.pty.get_ref().take_window_title()?; + if let Some(milestone) = vt_pty_terminal_test_client::decode_milestone_title(&title) + { + acknowledge(milestone.id); + return Some(milestone.name); + } + } + }) + } + /// Reads all remaining PTY output until the child exits, then returns the exit status. /// /// # Errors @@ -93,8 +121,41 @@ impl Reader { /// /// Panics if reading from the PTY fails. pub fn wait_for_exit(&mut self) -> anyhow::Result { - let mut discard = Vec::new(); - self.pty.read_to_end(&mut discard).expect("PTY read_to_end failed"); + let mut buf = [0u8; 4096]; + loop { + self.take_milestones().for_each(drop); + if self.pty.read(&mut buf).expect("PTY read failed") == 0 { + break; + } + } self.child_handle.wait() } } + +/// Signals the producer of milestone `id` that the milestone was seen. +/// +/// See `vt_pty_terminal_test_client::mark_milestone` for why it waits. +#[cfg(windows)] +fn acknowledge(id: u128) { + use winapi::um::{handleapi, synchapi, winnt}; + + let name = vt_pty_terminal_test_client::milestone_ack_event_name(id) + .encode_utf16() + .chain([0]) + .collect::>(); + // SAFETY: `name` is a valid NUL-terminated UTF-16 name. + let event = unsafe { synchapi::OpenEventW(winnt::EVENT_MODIFY_STATE, 0, name.as_ptr()) }; + // The producer doesn't wait if it didn't inherit the lock environment + // variable, and closes the event once it is signaled. + if event.is_null() { + return; + } + // SAFETY: `event` is a valid event handle owned here. + unsafe { + synchapi::SetEvent(event); + handleapi::CloseHandle(event); + } +} + +#[cfg(not(windows))] +const fn acknowledge(_id: u128) {} diff --git a/crates/vt_pty_terminal_test/tests/milestone.rs b/crates/vt_pty_terminal_test/tests/milestone.rs index e64b97e28..18439ce4a 100644 --- a/crates/vt_pty_terminal_test/tests/milestone.rs +++ b/crates/vt_pty_terminal_test/tests/milestone.rs @@ -152,3 +152,60 @@ fn descendant_process_can_mark_milestone() { let _ = reader.expect_milestone("nested"); assert!(reader.wait_for_exit().unwrap().success()); } + +/// Two processes mark milestones at the same time. `ConPTY` emits only the +/// latest console title when it renders, so without the lock in +/// `mark_milestone` the nested process's titles could replace `target`. +#[test] +#[timeout(5000)] +#[expect(clippy::redundant_clone, reason = "command_for_fn evaluates its argument twice")] +fn concurrent_milestones_are_not_lost() { + let nested = command_for_fn!((), |(): ()| { + for _ in 0..20 { + vt_pty_terminal_test_client::mark_milestone("noise"); + } + }); + let nested_program = nested.program.to_string_lossy().into_owned(); + let nested_args = + nested.args.iter().map(|arg| arg.to_string_lossy().into_owned()).collect::>(); + + let cmd = CommandBuilder::from(command_for_fn!( + (nested_program.clone(), nested_args.clone()), + |(nested_program, nested_args): (String, Vec)| { + let mut nested = + std::process::Command::new(nested_program).args(nested_args).spawn().unwrap(); + vt_pty_terminal_test_client::mark_milestone("target"); + assert!(nested.wait().unwrap().success()); + } + )); + + let TestTerminal { writer: _, mut reader, child_handle: _ } = + TestTerminal::spawn(ScreenSize { rows: 80, cols: 80 }, cmd).unwrap(); + let _ = reader.expect_milestone("target"); + assert!(reader.wait_for_exit().unwrap().success()); +} + +/// The child prints nothing after the milestone until the harness responds. +#[test] +#[timeout(5000)] +fn milestone_without_later_output() { + let cmd = CommandBuilder::from(command_for_fn!((), |(): ()| { + use std::io::{Write, stdout}; + + let mut stdout = stdout(); + write!(stdout, "waiting").unwrap(); + stdout.flush().unwrap(); + vt_pty_terminal_test_client::mark_milestone("waiting"); + + let mut line = String::new(); + std::io::stdin().read_line(&mut line).unwrap(); + })); + + let TestTerminal { mut writer, mut reader, child_handle: _ } = + TestTerminal::spawn(ScreenSize { rows: 80, cols: 80 }, cmd).unwrap(); + let screen = reader.expect_milestone("waiting"); + assert_eq!(screen.trim(), "waiting"); + + writer.write_line(b"").unwrap(); + assert!(reader.wait_for_exit().unwrap().success()); +} diff --git a/crates/vt_pty_terminal_test_client/Cargo.toml b/crates/vt_pty_terminal_test_client/Cargo.toml index cf39f0983..89c1fc0a4 100644 --- a/crates/vt_pty_terminal_test_client/Cargo.toml +++ b/crates/vt_pty_terminal_test_client/Cargo.toml @@ -16,7 +16,7 @@ base64 = { workspace = true } getrandom = { workspace = true, optional = true } [target.'cfg(windows)'.dependencies] -winapi = { workspace = true, features = ["wincon"], optional = true } +winapi = { workspace = true, features = ["synchapi", "winbase", "wincon"], optional = true } [dev-dependencies] getrandom = { workspace = true } diff --git a/crates/vt_pty_terminal_test_client/src/lib.rs b/crates/vt_pty_terminal_test_client/src/lib.rs index 3b1aa31dc..245f4bb9f 100644 --- a/crates/vt_pty_terminal_test_client/src/lib.rs +++ b/crates/vt_pty_terminal_test_client/src/lib.rs @@ -2,18 +2,42 @@ use base64::{Engine as _, engine::general_purpose::URL_SAFE_NO_PAD}; const MILESTONE_TITLE_MARKER: &str = "vt-pty-terminal-test:"; +/// Environment variable through which the test harness asks milestone +/// producers on Windows to wait until it has seen each milestone. Its value +/// identifies the test terminal. +pub const MILESTONE_LOCK_ENV: &str = "VT_PTY_TERMINAL_TEST_MILESTONE_LOCK"; + +/// A milestone decoded from a window title. +#[derive(Debug, PartialEq, Eq)] +pub struct Milestone { + /// Random identity that distinguishes repeated milestones with the same name. + pub id: u128, + pub name: String, +} + +/// Name of the Windows event that the producer of milestone `id` waits on +/// until the test harness has seen its title. +#[must_use] +pub fn milestone_ack_event_name(id: u128) -> String { + format!("Local\\vt-pty-terminal-test-ack-{id:032x}") +} + #[cfg(any(feature = "testing", test))] -fn encode_milestone_title(name: &str) -> String { +fn new_milestone_id() -> u128 { let mut random = [0u8; 16]; getrandom::fill(&mut random).expect("failed to generate milestone identity"); - let id = u128::from_be_bytes(random); + u128::from_be_bytes(random) +} + +#[cfg(any(feature = "testing", test))] +fn encode_milestone_title(id: u128, name: &str) -> String { let encoded_name = URL_SAFE_NO_PAD.encode(name.as_bytes()); format!("{MILESTONE_TITLE_MARKER}{id:032x}:{encoded_name}") } /// Decodes a milestone title, ignoring ordinary application title updates. #[must_use] -pub fn decode_milestone_title(title: &[u8]) -> Option { +pub fn decode_milestone_title(title: &[u8]) -> Option { let encoded = title.strip_prefix(MILESTONE_TITLE_MARKER.as_bytes())?; let (encoded_id, encoded_name) = encoded.split_at_checked(32)?; let (&b':', encoded_name) = encoded_name.split_first()? else { @@ -23,11 +47,13 @@ pub fn decode_milestone_title(title: &[u8]) -> Option { return None; } + let id = u128::from_str_radix(std::str::from_utf8(encoded_id).ok()?, 16).ok()?; + let name_bytes = URL_SAFE_NO_PAD.decode(encoded_name).ok()?; if URL_SAFE_NO_PAD.encode(&name_bytes).as_bytes() != encoded_name { return None; } - String::from_utf8(name_bytes).ok() + Some(Milestone { id, name: String::from_utf8(name_bytes).ok()? }) } /// Emits a milestone marker as a unique window-title update. @@ -40,6 +66,12 @@ pub fn decode_milestone_title(title: &[u8]) -> Option { /// preceding text and cursor state. Other platforms emit the equivalent OSC 2 /// title update through the ordered PTY byte stream. /// +/// `ConPTY` emits only the latest title when it renders, so on Windows a title +/// set by another process before then would replace the milestone. When +/// [`MILESTONE_LOCK_ENV`] is set, this holds a lock named after it, shared by +/// the processes in the test terminal, until the harness signals the event +/// named by [`milestone_ack_event_name`] after it has seen the title. +/// /// When the `testing` feature is disabled, this is a no-op. /// /// # Panics @@ -47,26 +79,88 @@ pub fn decode_milestone_title(title: &[u8]) -> Option { /// Panics if secure randomness is unavailable or emitting the title fails. #[cfg(feature = "testing")] pub fn mark_milestone(name: &str) { - emit_title(&encode_milestone_title(name)).expect("failed to emit milestone title"); + let id = new_milestone_id(); + emit_title(id, &encode_milestone_title(id, name)).expect("failed to emit milestone title"); } #[cfg(all(feature = "testing", windows))] -fn emit_title(title: &str) -> std::io::Result<()> { - use std::io::Write as _; +fn emit_title(id: u128, title: &str) -> std::io::Result<()> { + use std::{ + io::Write as _, + os::windows::io::{FromRawHandle as _, OwnedHandle}, + ptr, + }; - std::io::stdout().flush()?; - let mut wide = title.encode_utf16().collect::>(); - wide.push(0); - // SAFETY: `wide` is a valid NUL-terminated UTF-16 title. - if unsafe { winapi::um::wincon::SetConsoleTitleW(wide.as_ptr()) } == 0 { - Err(std::io::Error::last_os_error()) - } else { - Ok(()) + use winapi::um::{synchapi, winbase, wincon}; + + fn wide(s: &str) -> Vec { + s.encode_utf16().chain([0]).collect() } + + fn check_handle(handle: winapi::um::winnt::HANDLE) -> std::io::Result { + if handle.is_null() { + return Err(std::io::Error::last_os_error()); + } + // SAFETY: `handle` is a valid handle that nothing else owns. + Ok(unsafe { OwnedHandle::from_raw_handle(handle.cast()) }) + } + + fn wait(handle: &OwnedHandle) -> std::io::Result<()> { + use std::os::windows::io::AsRawHandle as _; + + // SAFETY: `handle` is a valid synchronization object. + match unsafe { + synchapi::WaitForSingleObject(handle.as_raw_handle().cast(), winbase::INFINITE) + } { + // A lock abandoned by a process that exited while holding it is + // still acquired. + winbase::WAIT_OBJECT_0 | winbase::WAIT_ABANDONED => Ok(()), + _ => Err(std::io::Error::last_os_error()), + } + } + + struct Release<'a>(&'a OwnedHandle); + impl Drop for Release<'_> { + fn drop(&mut self) { + use std::os::windows::io::AsRawHandle as _; + + // SAFETY: the current thread owns the mutex. + unsafe { synchapi::ReleaseMutex(self.0.as_raw_handle().cast()) }; + } + } + + std::io::stdout().flush()?; + let title = wide(title); + let set_title = || { + // SAFETY: `title` is a valid NUL-terminated UTF-16 title. + if unsafe { wincon::SetConsoleTitleW(title.as_ptr()) } == 0 { + Err(std::io::Error::last_os_error()) + } else { + Ok(()) + } + }; + let Ok(terminal) = std::env::var(MILESTONE_LOCK_ENV) else { + return set_title(); + }; + + let lock_name = wide(&format!("Local\\vt-pty-terminal-test-lock-{terminal}")); + // SAFETY: `lock_name` is a valid NUL-terminated UTF-16 name. + let lock = + check_handle(unsafe { synchapi::CreateMutexW(ptr::null_mut(), 0, lock_name.as_ptr()) })?; + wait(&lock)?; + let _release = Release(&lock); + + let ack_name = wide(&milestone_ack_event_name(id)); + // SAFETY: `ack_name` is a valid NUL-terminated UTF-16 name. The event is + // manual-reset and starts unsignaled. + let ack = + check_handle(unsafe { synchapi::CreateEventW(ptr::null_mut(), 1, 0, ack_name.as_ptr()) })?; + set_title()?; + wait(&ack) } #[cfg(all(feature = "testing", not(windows)))] -fn emit_title(title: &str) -> std::io::Result<()> { +fn emit_title(_id: u128, title: &str) -> std::io::Result<()> { use std::io::Write as _; let mut stdout = std::io::stdout().lock(); @@ -87,13 +181,17 @@ mod tests { #[test] fn title_round_trip() { - let title = encode_milestone_title("task-select:lib#:0"); - assert_eq!(decode_milestone_title(title.as_bytes()).as_deref(), Some("task-select:lib#:0")); + let id = new_milestone_id(); + let title = encode_milestone_title(id, "task-select:lib#:0"); + assert_eq!( + decode_milestone_title(title.as_bytes()), + Some(Milestone { id, name: "task-select:lib#:0".to_owned() }) + ); } #[test] - fn repeated_names_get_unique_titles() { - assert_ne!(encode_milestone_title("ready"), encode_milestone_title("ready")); + fn repeated_names_get_unique_ids() { + assert_ne!(new_milestone_id(), new_milestone_id()); } #[test]