diff --git a/src/pane.rs b/src/pane.rs index 345a7513..43f71e39 100644 --- a/src/pane.rs +++ b/src/pane.rs @@ -52,6 +52,16 @@ pub use self::{ terminal::{ScrollMetrics, TerminalCursorState}, }; +pub(crate) struct TerminalDirtyPatchSnapshot { + pub patch: TerminalDirtyPatchOutcome, + pub content_revision: u64, + pub scroll_metrics: Option, + pub mouse_reporting: bool, + pub sgr_pixel_mouse: bool, + pub alternate_screen_active: bool, + pub graphics_may_have_placements: bool, +} + const RELEASE_REACQUIRE_SUPPRESSION: std::time::Duration = std::time::Duration::from_secs(1); const TERMINAL_COMPRESSION_IDLE: std::time::Duration = std::time::Duration::from_millis(250); const TERMINAL_COMPRESSION_STEP: std::time::Duration = std::time::Duration::from_millis(1); @@ -3087,12 +3097,36 @@ impl PaneRuntime { self.terminal.render(frame, area, show_cursor); } - pub(crate) fn collect_dirty_patch( + pub(crate) fn collect_dirty_patch_snapshot( &self, area_width: u16, area_height: u16, - ) -> TerminalDirtyPatchOutcome { - self.terminal.collect_dirty_patch(area_width, area_height) + ) -> Option { + // PTY/resize writers announce changes before locking the terminal core. + // Exclude them until rows and metadata have been paired with their revision. + let _content_guard = self + .content_write_lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let revision = self.content_seq(); + if !revision.is_multiple_of(2) { + return None; + } + let patch = self.terminal.collect_dirty_patch(area_width, area_height); + if matches!(patch, TerminalDirtyPatchOutcome::Fallback) { + return None; + } + let snapshot = TerminalDirtyPatchSnapshot { + patch, + content_revision: revision, + scroll_metrics: self.scroll_metrics(), + mouse_reporting: self.mouse_reporting_enabled(), + sgr_pixel_mouse: self.sgr_pixel_mouse_enabled(), + alternate_screen_active: self.alternate_screen_active(), + graphics_may_have_placements: crate::kitty_graphics::is_enabled() + && self.kitty_graphics_may_have_placements(), + }; + (self.content_seq() == revision).then_some(snapshot) } pub fn visible_hyperlinks(&self, area: Rect) -> Vec<((u16, u16), String, String)> { @@ -3329,6 +3363,60 @@ impl PaneRuntime { Self::test_with_scrollback_bytes(cols, rows, 0, bytes) } + pub(crate) fn test_contend_during_dirty_collection( + &self, + bytes: Vec, + ) -> (std::sync::mpsc::Sender<()>, std::thread::JoinHandle) { + let terminal = self.terminal.clone(); + let sequence = self.content_seq.clone(); + let write_lock = self.content_write_lock.clone(); + let pane_id = self.pane_id; + let (start_tx, start_rx) = std::sync::mpsc::channel(); + let (ready_tx, ready_rx) = std::sync::mpsc::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + self.terminal + .ghostty + .core + .lock() + .unwrap() + .dirty_collection_hook = Some(Box::new(move || { + start_tx.send(()).unwrap(); + ready_rx + .recv_timeout(std::time::Duration::from_secs(5)) + .unwrap(); + })); + let writer = std::thread::spawn(move || { + start_rx + .recv_timeout(std::time::Duration::from_secs(5)) + .unwrap(); + let guard = match write_lock.try_lock() { + Ok(guard) => Some(guard), + Err(std::sync::TryLockError::WouldBlock) => None, + Err(error) => panic!("poisoned content lock: {error}"), + }; + let announced = guard.is_some(); + if announced { + sequence.fetch_add(1, Ordering::AcqRel); + assert!(matches!( + terminal.ghostty.core.try_lock(), + Err(std::sync::TryLockError::WouldBlock) + )); + } + ready_tx.send(()).unwrap(); + let _ = release_rx.recv(); + let _guard = guard.unwrap_or_else(|| { + let guard = write_lock.lock().unwrap(); + sequence.fetch_add(1, Ordering::AcqRel); + guard + }); + let (tx, _rx) = mpsc::channel(1); + let _ = terminal.process_pty_bytes(pane_id, 0, &bytes, &tx); + sequence.fetch_add(1, Ordering::Release); + announced + }); + (release_tx, writer) + } + pub(crate) fn test_process_pty_bytes(&self, bytes: &[u8]) { let _content_write_guard = match self.content_write_lock.lock() { Ok(guard) => guard, @@ -3400,6 +3488,63 @@ impl PaneRuntime { mod tests { use super::*; + #[tokio::test] + async fn dirty_patch_snapshot_keeps_clean_metadata_and_terminal_fallback() { + let (runtime, _rx) = PaneRuntime::test_with_channel(20, 4); + runtime + .collect_dirty_patch_snapshot(20, 4) + .expect("initial snapshot"); + runtime.test_process_pty_bytes(b"\x1b[?1003h\x1b[?1016h"); + let snapshot = runtime + .collect_dirty_patch_snapshot(20, 4) + .expect("mode snapshot"); + assert!(matches!(snapshot.patch, TerminalDirtyPatchOutcome::Clean)); + assert_eq!(snapshot.content_revision, runtime.content_seq()); + assert!(snapshot.content_revision.is_multiple_of(2)); + assert!(snapshot.mouse_reporting); + assert!(snapshot.sgr_pixel_mouse); + assert!(!snapshot.alternate_screen_active); + + runtime.test_process_pty_bytes(b"\x1b]8;;https://example.com\x1b\\link\x1b]8;;\x1b\\"); + assert!(runtime.collect_dirty_patch_snapshot(20, 4).is_none()); + assert!(runtime.content_write_lock.try_lock().is_ok()); + } + + #[tokio::test] + async fn dirty_patch_snapshot_tracks_serialized_scroll_and_resize() { + let runtime = PaneRuntime::test_with_scrollback_bytes( + 20, + 4, + 100_000, + b"one\r\ntwo\r\nthree\r\nfour\r\nfive\r\nsix", + ); + runtime + .collect_dirty_patch_snapshot(20, 4) + .expect("live snapshot"); + runtime.scroll_up(1); + let scrolled = runtime + .collect_dirty_patch_snapshot(20, 4) + .expect("scrolled snapshot"); + assert_eq!( + scrolled.scroll_metrics.expect("metrics").offset_from_bottom, + 1 + ); + runtime.scroll_reset(); + runtime.resize(5, 24, 0, 0); + let resized = runtime + .collect_dirty_patch_snapshot(24, 5) + .expect("resized snapshot"); + let metrics = resized.scroll_metrics.expect("resized metrics"); + assert_eq!(metrics.offset_from_bottom, 0); + assert_eq!(metrics.viewport_rows, 5); + assert!(resized.content_revision.is_multiple_of(2)); + let TerminalDirtyPatchOutcome::Patch(patch) = resized.patch else { + panic!("resize must dirty the viewport"); + }; + assert_eq!(patch.rows.len(), 5); + assert!(patch.rows.iter().all(|(_, cells)| cells.len() == 24)); + } + #[test] fn pane_launch_env_removes_outer_codex_thread_id() { let mut cmd = CommandBuilder::new("shell"); diff --git a/src/pane/terminal.rs b/src/pane/terminal.rs index 071bb20a..b201702b 100644 --- a/src/pane/terminal.rs +++ b/src/pane/terminal.rs @@ -192,6 +192,8 @@ pub(crate) struct GhosttyPaneTerminal { } pub(crate) struct GhosttyPaneCore { + #[cfg(test)] + pub dirty_collection_hook: Option>, pub terminal: crate::ghostty::Terminal, #[cfg(windows)] recent_fallback: windows_recent_fallback::Cache, @@ -1145,6 +1147,8 @@ impl GhosttyPaneTerminal { key_encoder.set_from_terminal(&terminal); Ok(Self { core: Mutex::new(GhosttyPaneCore { + #[cfg(test)] + dirty_collection_hook: None, terminal, #[cfg(windows)] recent_fallback: windows_recent_fallback::Cache::default(), @@ -2363,7 +2367,13 @@ impl GhosttyPaneTerminal { self.core .lock() .ok() - .map(|mut core| ghostty_collect_dirty_patch(&mut core, area_width, area_height)) + .map(|mut core| { + #[cfg(test)] + if let Some(hook) = core.dirty_collection_hook.take() { + hook(); + } + ghostty_collect_dirty_patch(&mut core, area_width, area_height) + }) .unwrap_or(TerminalDirtyPatchOutcome::Fallback) } } diff --git a/src/server/headless/retained_surface.rs b/src/server/headless/retained_surface.rs index 259714a3..d0fb9bba 100644 --- a/src/server/headless/retained_surface.rs +++ b/src/server/headless/retained_surface.rs @@ -317,11 +317,10 @@ impl HeadlessServer { ) else { fallback!("runtime_missing"); }; - let revision_before = runtime.content_seq(); - if !revision_before.is_multiple_of(2) { - fallback!("unstable_content"); - } - let patch = match runtime.collect_dirty_patch(width, height) { + let Some(snapshot) = runtime.collect_dirty_patch_snapshot(width, height) else { + fallback!("terminal_snapshot"); + }; + let patch = match snapshot.patch { crate::pane::TerminalDirtyPatchOutcome::Clean => { crate::render_prof::event("retained_surface.pane_clean"); crate::pane::TerminalDirtyPatch { rows: Vec::new() } @@ -331,21 +330,15 @@ impl HeadlessServer { fallback!("terminal_patch"); } }; - let graphics_may_have_placements = - crate::kitty_graphics::is_enabled() && runtime.kitty_graphics_may_have_placements(); - let revision = runtime.content_seq(); - if revision != revision_before || !revision.is_multiple_of(2) { - fallback!("content_changed"); - } collected.push(CollectedPanePatch { pane_id: public_pane_id, patch, - content_revision: revision, - scroll_metrics: runtime.scroll_metrics(), - mouse_reporting: runtime.mouse_reporting_enabled(), - sgr_pixel_mouse: runtime.sgr_pixel_mouse_enabled(), - alternate_screen_active: runtime.alternate_screen_active(), - graphics_may_have_placements, + content_revision: snapshot.content_revision, + scroll_metrics: snapshot.scroll_metrics, + mouse_reporting: snapshot.mouse_reporting, + sgr_pixel_mouse: snapshot.sgr_pixel_mouse, + alternate_screen_active: snapshot.alternate_screen_active, + graphics_may_have_placements: snapshot.graphics_may_have_placements, }); } diff --git a/src/server/headless/tests/mod.rs b/src/server/headless/tests/mod.rs index f124e52e..d666ae22 100644 --- a/src/server/headless/tests/mod.rs +++ b/src/server/headless/tests/mod.rs @@ -1000,6 +1000,62 @@ fn recv_pane_surface_patch( } } +#[tokio::test] +async fn retained_snapshot_survives_a_writer_waiting_for_the_terminal_core() { + let mut server = test_headless_server(); + let pane_id = install_shared_view_test_runtime(&mut server); + let (control, render) = connect_matching_test_shell(&mut server, 7); + let _ = control.recv().expect("snapshot"); + server.render_and_stream(); + let _ = recv_pane_surface(&render, "initial surface"); + + let (release, writer, revision) = { + let runtime = server + .app + .state + .runtime_for_pane_in_workspace(&server.app.terminal_runtimes, 0, pane_id) + .expect("runtime"); + runtime.test_process_pty_bytes(b"\rAAAA\x1b[?1003h"); + let revision = runtime.content_seq(); + let (release, writer) = + runtime.test_contend_during_dirty_collection(b"\rBBBB\x1b[?1003l".to_vec()); + (release, writer, revision) + }; + let retained = server.render_retained_pane_surface_and_stream(&HashSet::from([pane_id])); + release.send(()).expect("release waiting writer"); + let announced = writer.join().expect("writer completed"); + + assert!( + retained, + "a waiting writer must not invalidate the collected snapshot" + ); + assert!( + !announced, + "writer must wait before announcing a new revision" + ); + let patch = recv_pane_surface_patch(&render, "snapshot before waiting write"); + assert_eq!(patch.panes[0].content_revision, revision); + assert!(revision.is_multiple_of(2)); + assert!(patch.panes[0].mouse_reporting); + let surface = server.clients[&7] + .render_state + .last_pane_surface() + .expect("surface"); + assert!(frame_text(&surface.frame).contains("AAAA")); + assert!(!frame_text(&surface.frame).contains("BBBB")); + + assert!(server.render_retained_pane_surface_and_stream(&HashSet::from([pane_id]))); + let next = recv_pane_surface_patch(&render, "waiting write remains dirty"); + assert_eq!(next.panes[0].content_revision, revision + 2); + assert!(!next.panes[0].mouse_reporting); + let surface = server.clients[&7] + .render_state + .last_pane_surface() + .expect("next surface"); + assert!(frame_text(&surface.frame).contains("BBBB")); + shutdown_test_runtimes(&mut server); +} + #[tokio::test] async fn different_size_shells_receive_geometry_specific_patches_from_one_dirty_collection() { let mut server = test_headless_server(); diff --git a/src/terminal/runtime.rs b/src/terminal/runtime.rs index 57d45b92..a409545b 100644 --- a/src/terminal/runtime.rs +++ b/src/terminal/runtime.rs @@ -411,22 +411,18 @@ impl TerminalRuntime { self.0.render(frame, area, show_cursor); } - pub(crate) fn collect_dirty_patch( + pub(crate) fn collect_dirty_patch_snapshot( &self, area_width: u16, area_height: u16, - ) -> crate::pane::TerminalDirtyPatchOutcome { - self.0.collect_dirty_patch(area_width, area_height) + ) -> Option { + self.0.collect_dirty_patch_snapshot(area_width, area_height) } pub fn visible_hyperlinks(&self, area: Rect) -> Vec<((u16, u16), String, String)> { self.0.visible_hyperlinks(area) } - pub(crate) fn kitty_graphics_may_have_placements(&self) -> bool { - self.0.kitty_graphics_may_have_placements() - } - pub fn kitty_image_placements_with_data_filter( &self, needs_data: F, @@ -572,6 +568,13 @@ impl TerminalRuntime { #[cfg(test)] impl TerminalRuntime { + pub(crate) fn test_contend_during_dirty_collection( + &self, + bytes: Vec, + ) -> (std::sync::mpsc::Sender<()>, std::thread::JoinHandle) { + self.0.test_contend_during_dirty_collection(bytes) + } + pub(crate) fn test_with_channel(cols: u16, rows: u16) -> (Self, mpsc::Receiver) { let (runtime, rx) = crate::pane::PaneRuntime::test_with_channel(cols, rows); (Self(runtime), rx)