From 18888e7a2c23f0e53f4faa84f83977a9343cf698 Mon Sep 17 00:00:00 2001 From: Chris Guidry Date: Fri, 31 Jul 2026 11:23:51 -0400 Subject: [PATCH] Bracket render passes in a synchronized terminal update A script(1) recording showed the play loop issuing up to five unsynchronized write bursts at the same instant, a scroll plus several repaints, with no CSI ?2026 synchronized-update guard anywhere. The terminal is free to paint a half-finished frame, which reads as jank. Added a SyncGuard trait (begin/end) that brackets one render pass so the terminal paints it atomically; a terminal without CSI ?2026 support ignores the codes. screen::play brackets drain-then-draw in begin/end, and submit brackets its own player-line insert separately, since that paints during key handling, after the pass's own end. The guard closes before every key poll, since holding it open there would freeze the display between frames. terminal.rs implements the guard over stdout with crossterm's BeginSynchronizedUpdate/ EndSynchronizedUpdate. Adding the guard parameter pushed screen::play past clippy's too-many-arguments limit, so its three loose channel parameters (inputs, turns, cancel) were consolidated into the existing Worker struct that already bundles them, rather than suppressing the lint. Tests record begin/end/poll onto a timeline shared between the guard fake and the keys fake, so a check can confirm every pass is bracketed, pairs balance, updates never nest, and no poll ever lands while an update is open. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CUXjWo1zGFhdJUig1hcQGf --- src/play/mod.rs | 1 + src/play/screen.rs | 53 ++++++++++---- src/play/screen_sync_tests.rs | 133 ++++++++++++++++++++++++++++++++++ src/play/screen_tests.rs | 36 +++++++-- src/play/sync.rs | 29 ++++++++ src/play/terminal.rs | 23 +++++- 6 files changed, 251 insertions(+), 24 deletions(-) create mode 100644 src/play/screen_sync_tests.rs create mode 100644 src/play/sync.rs diff --git a/src/play/mod.rs b/src/play/mod.rs index fe03c15..fb00116 100644 --- a/src/play/mod.rs +++ b/src/play/mod.rs @@ -17,6 +17,7 @@ mod editor; mod history; pub mod keys; pub mod screen; +pub mod sync; #[cfg(not(coverage))] mod terminal; mod thinking; diff --git a/src/play/screen.rs b/src/play/screen.rs index f698c88..b97e70d 100644 --- a/src/play/screen.rs +++ b/src/play/screen.rs @@ -1,7 +1,7 @@ //! The render loop: the live region where a reply streams, the prompt the //! player types on, and the transcript above them both. -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::Ordering; use std::sync::mpsc::{Receiver, Sender, TryRecvError}; use ratatui::Frame; @@ -14,8 +14,9 @@ use ratatui::text::Text; use super::editor::Editor; use super::history::History; use super::keys::{Key, Keys}; +use super::sync::SyncGuard; use super::thinking; -use super::worker::TurnEvent; +use super::worker::{TurnEvent, Worker}; use super::wrap::{wrap, wrap_spans}; /// The rows the inline viewport takes at the bottom of the terminal. @@ -76,19 +77,25 @@ struct Screen { /// Runs the game until the player quits. /// -/// Reads keys from `keys`, sends submitted lines to `inputs`, and shows -/// what comes back on `turns`. Finished lines go above the viewport, into -/// the terminal's own scrollback, so they survive the game. `history` holds -/// the prompts the player can recall with Up and Down. Escape while a turn -/// is running sets `cancel`, which the worker checks to stop the turn. -pub fn play( +/// Reads keys from `keys`, sends submitted lines to `worker.inputs`, and +/// shows what comes back on `worker.events`. Finished lines go above the +/// viewport, into the terminal's own scrollback, so they survive the game. +/// `history` holds the prompts the player can recall with Up and Down. +/// Escape while a turn is running sets `worker.cancel`, which the worker +/// checks to stop the turn. +/// +/// `guard` brackets every render pass in a synchronized update, so the +/// terminal paints each pass in one go instead of painting whatever has +/// landed on the wire so far. The opening banner paints alone, before the +/// loop's first pass and nothing else, so there is nothing for it to race +/// and no update to bracket it with. +pub fn play( terminal: &mut Terminal, keys: &mut K, - inputs: &Sender, - turns: &Receiver, - cancel: &AtomicBool, + worker: &Worker, banner: &str, history: &mut History, + guard: &mut G, ) -> Result<(), B::Error> { let mut screen = Screen { input: Editor::default(), @@ -101,8 +108,13 @@ pub fn play( insert(terminal, &mut screen, banner, aside())?; loop { screen.tick = screen.tick.wrapping_add(1); - drain(&mut screen, terminal, turns)?; + guard.begin(); + // An error here returns with the guard still open. That is fine: + // the caller exits the program on this path, and a terminal that + // never sees a matching `end` recovers on its own safety timeout. + drain(&mut screen, terminal, &worker.events)?; terminal.draw(|frame| render(&screen, frame))?; + guard.end(); match keys.next_key() { None => {} Some(Key::Quit) => return Ok(()), @@ -130,8 +142,8 @@ pub fn play( let entry = history.down(); recall(&mut screen, entry); } - Some(Key::Enter) => submit(&mut screen, terminal, inputs, history)?, - Some(Key::Cancel) if screen.busy => cancel.store(true, Ordering::Relaxed), + Some(Key::Enter) => submit(&mut screen, terminal, &worker.inputs, history, guard)?, + Some(Key::Cancel) if screen.busy => worker.cancel.store(true, Ordering::Relaxed), Some(Key::Cancel) => {} } } @@ -152,18 +164,25 @@ fn recall(screen: &mut Screen, entry: Option) { /// the prompt until the turn ends. A send that fails means the worker is /// gone, which leaves nothing to do but keep drawing until the player /// quits. -fn submit( +/// +/// This insert happens while a key is handled, after the loop pass's own +/// `end`, so it brackets itself in its own synchronized update rather than +/// riding along with the next pass. +fn submit( screen: &mut Screen, terminal: &mut Terminal, inputs: &Sender, history: &mut History, + guard: &mut G, ) -> Result<(), B::Error> { if screen.busy || screen.input.text().trim().is_empty() { return Ok(()); } let input = screen.input.take(); let line = format!("{PLAYER_MARKER}{input}"); + guard.begin(); insert(terminal, screen, &line, player())?; + guard.end(); screen.busy = true; let _ = inputs.send(input.clone()); history.record(&input); @@ -367,3 +386,7 @@ mod tests; #[cfg(test)] #[path = "screen_turn_tests.rs"] mod turn_tests; + +#[cfg(test)] +#[path = "screen_sync_tests.rs"] +mod sync_tests; diff --git a/src/play/screen_sync_tests.rs b/src/play/screen_sync_tests.rs new file mode 100644 index 0000000..9a1b113 --- /dev/null +++ b/src/play/screen_sync_tests.rs @@ -0,0 +1,133 @@ +//! Tests that the play loop paints atomically: every render pass and the +//! submit insert bracket themselves in a synchronized update, the pairs +//! balance, updates never nest, and no update stays open across a key +//! poll. Shares `screen_tests.rs`'s harness rather than keeping its own +//! copy. + +use std::cell::RefCell; +use std::rc::Rc; + +use ratatui::text::Text; + +use super::tests::{Step, done, play_script, press, typing}; +use super::*; + +/// One step in the timeline a `RecordingGuard` and the harness's `Script` +/// keys fake share: a synchronized update opening or closing, or a key +/// poll landing. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Recorded { + Begin, + End, + Poll, +} + +/// A `SyncGuard` that appends every `begin` and `end` call to a timeline +/// shared with the harness's `Script` keys fake, so a test can see +/// whether a poll ever landed while a pass was still open. +#[derive(Clone, Default)] +pub(super) struct RecordingGuard { + pub(super) timeline: Rc>>, +} + +impl SyncGuard for RecordingGuard { + fn begin(&mut self) { + self.timeline.borrow_mut().push(Recorded::Begin); + } + + fn end(&mut self) { + self.timeline.borrow_mut().push(Recorded::End); + } +} + +/// Whether `timeline` never lets a poll land while a pass is open, and +/// every `Begin` is matched by exactly one `End` before the next `Begin`. +pub(super) fn brackets_hold(timeline: &[Recorded]) -> bool { + let closed = timeline + .iter() + .try_fold(false, |open, event| match (open, event) { + (false, Recorded::Begin) => Some(true), + (true, Recorded::End) => Some(false), + (false, Recorded::Poll) => Some(false), + _ => None, + }); + closed == Some(false) +} + +#[test] +fn an_idle_pass_begins_and_ends_before_polling() { + let played = play_script(vec![]); + + assert_eq!( + played.guard_timeline(), + vec![Recorded::Begin, Recorded::End, Recorded::Poll] + ); +} + +#[test] +fn submitting_brackets_its_own_insert_apart_from_the_pass_that_read_enter() { + let mut steps = typing("hi"); + steps.push(press(Key::Enter)); + + let played = play_script(steps); + let timeline = played.guard_timeline(); + + // Three passes read a key (h, i, Enter), and the submit that Enter + // triggers brackets its own insert, for five pairs in all. + let begins = timeline + .iter() + .filter(|event| **event == Recorded::Begin) + .count(); + let ends = timeline + .iter() + .filter(|event| **event == Recorded::End) + .count(); + assert_eq!(begins, 5); + assert_eq!(ends, 5); +} + +#[test] +fn a_turn_with_a_tool_call_and_a_cancel_keeps_every_pass_bracketed() { + let mut steps = typing("hi"); + steps.push(press(Key::Enter)); + steps.push(Step::Turn(TurnEvent::Tool(Text::raw("a tool line")))); + steps.push(done()); + steps.push(press(Key::Cancel)); + + let played = play_script(steps); + + assert!(brackets_hold(&played.guard_timeline())); +} + +#[test] +fn brackets_hold_accepts_a_balanced_timeline() { + let timeline = vec![Recorded::Begin, Recorded::End, Recorded::Poll]; + + assert!(brackets_hold(&timeline)); +} + +#[test] +fn brackets_hold_rejects_a_nested_begin() { + let timeline = vec![ + Recorded::Begin, + Recorded::Begin, + Recorded::End, + Recorded::End, + ]; + + assert!(!brackets_hold(&timeline)); +} + +#[test] +fn brackets_hold_rejects_a_poll_while_open() { + let timeline = vec![Recorded::Begin, Recorded::Poll, Recorded::End]; + + assert!(!brackets_hold(&timeline)); +} + +#[test] +fn brackets_hold_rejects_an_unmatched_begin() { + let timeline = vec![Recorded::Begin]; + + assert!(!brackets_hold(&timeline)); +} diff --git a/src/play/screen_tests.rs b/src/play/screen_tests.rs index 058c06b..d54c6c6 100644 --- a/src/play/screen_tests.rs +++ b/src/play/screen_tests.rs @@ -5,7 +5,9 @@ //! this file's harness rather than keeping its own copy, so `play`'s //! test-only type parameter is monomorphized once, not twice. +use std::cell::RefCell; use std::collections::VecDeque; +use std::rc::Rc; use std::sync::Arc; use std::sync::atomic::AtomicBool; use std::sync::mpsc::{self, Receiver, Sender, TryRecvError}; @@ -15,6 +17,7 @@ use ratatui::buffer::Buffer; use ratatui::layout::Position; use ratatui::{Terminal, TerminalOptions, Viewport}; +use super::sync_tests::{Recorded, RecordingGuard}; use super::*; /// One thing that happens while the loop runs: the player presses a key, @@ -40,13 +43,19 @@ pub(super) fn typing(text: &str) -> Vec { /// sending half of that channel, the same as a worker thread that panics /// mid-turn, and likewise reports no key. Quitting at the end of the /// script keeps every test bounded. +/// +/// `timeline` is the same timeline the play's `RecordingGuard` writes to. +/// Recording a poll onto it here lets a test see whether one ever landed +/// while a synchronized update was still open. struct Script { steps: VecDeque, turns: Option>, + timeline: Rc>>, } impl Keys for Script { fn next_key(&mut self) -> Option { + self.timeline.borrow_mut().push(Recorded::Poll); match self.steps.pop_front() { Some(Step::Press(key)) => Some(key), Some(Step::Turn(event)) => { @@ -67,8 +76,10 @@ pub(super) struct Played { terminal: Terminal, inputs: Receiver, pub(super) cancel: Arc, - /// Held open so an unsubmitted input reads as empty, not disconnected. - _sender: Sender, + /// Holds the worker stub's channels open, so an unsubmitted input on + /// `inputs` reads as empty, not disconnected. + _worker: Worker, + guard_timeline: Rc>>, } impl Played { @@ -91,6 +102,12 @@ impl Played { pub(super) fn submitted(&self) -> String { self.inputs.try_recv().unwrap() } + + /// The guard's begin/end calls, merged with every key poll, in the + /// order they happened. + pub(super) fn guard_timeline(&self) -> Vec { + self.guard_timeline.borrow().clone() + } } /// Joins every row of `buffer`, trailing spaces trimmed, one row per line. @@ -120,21 +137,27 @@ pub(super) fn play_script(steps: Vec) -> Played { .unwrap(); let (input_sender, inputs) = mpsc::channel(); let (turn_sender, turns) = mpsc::channel(); + let mut guard = RecordingGuard::default(); let mut keys = Script { steps: steps.into(), turns: Some(turn_sender), + timeline: Rc::clone(&guard.timeline), }; let mut history = History::in_memory(); let cancel = Arc::new(AtomicBool::new(false)); + let worker = Worker { + inputs: input_sender, + events: turns, + cancel: Arc::clone(&cancel), + }; play( &mut terminal, &mut keys, - &input_sender, - &turns, - &cancel, + &worker, "a banner", &mut history, + &mut guard, ) .unwrap(); @@ -142,7 +165,8 @@ pub(super) fn play_script(steps: Vec) -> Played { terminal, inputs, cancel, - _sender: input_sender, + _worker: worker, + guard_timeline: guard.timeline, } } diff --git a/src/play/sync.rs b/src/play/sync.rs new file mode 100644 index 0000000..5c314e5 --- /dev/null +++ b/src/play/sync.rs @@ -0,0 +1,29 @@ +//! The guard around one atomic terminal repaint. + +/// Brackets one render pass so the terminal paints it as a single update +/// instead of painting whatever has been written to it so far. +/// +/// `begin` starts a synchronized update, `CSI ?2026h`. From that point the +/// terminal holds the last frame it painted and queues every write that +/// follows without showing any of them. `end` sends the matching +/// `CSI ?2026l`, and the terminal paints everything queued since `begin` +/// in one pass. A terminal that does not support the sequence ignores +/// both codes and paints as it always did, so the guard is safe to use +/// unconditionally. +/// +/// The guard must close before the play loop polls for a key. A poll can +/// wait up to the loop's poll interval, and an open guard freezes the +/// display for as long as it stays open: leaving it open across the poll +/// would freeze the terminal between frames, not just during a repaint. +/// Terminals also carry their own safety timeout that ends an update on +/// its own a few seconds after `begin` with no matching `end`, but that +/// timeout exists to recover from a crash. It is not a substitute for +/// closing the guard on every pass. +pub trait SyncGuard { + /// Starts a synchronized update. + fn begin(&mut self); + + /// Ends a synchronized update, painting everything written since the + /// matching `begin`. + fn end(&mut self); +} diff --git a/src/play/terminal.rs b/src/play/terminal.rs index 58d3cb9..c2a6c9e 100644 --- a/src/play/terminal.rs +++ b/src/play/terminal.rs @@ -12,6 +12,7 @@ use std::time::Duration; use crossterm::cursor::MoveTo; use crossterm::event; use crossterm::execute; +use crossterm::terminal::{BeginSynchronizedUpdate, EndSynchronizedUpdate}; use ratatui::layout::Rect; use ratatui::{TerminalOptions, Viewport}; @@ -21,6 +22,7 @@ use crate::dm::Dm; use super::history::History; use super::keys::{self, Key, Keys}; use super::screen::{self, VIEWPORT_HEIGHT}; +use super::sync::SyncGuard; use super::worker::Worker; /// How long to wait for a key before looking at the worker again. @@ -52,11 +54,10 @@ pub fn run(overrides: &Overrides) -> Result<(), String> { let played = screen::play( &mut terminal, &mut CrosstermKeys, - &worker.inputs, - &worker.events, - &worker.cancel, + &worker, &banner, &mut history, + &mut CrosstermSyncGuard, ); let viewport = terminal.get_frame().area(); ratatui::restore(); @@ -98,3 +99,19 @@ impl Keys for CrosstermKeys { } } } + +/// The play loop's synchronized-update guard, backed by stdout. +struct CrosstermSyncGuard; + +impl SyncGuard for CrosstermSyncGuard { + /// A failed write here means the terminal is already in trouble, and + /// the next draw will fail too and end the game; there is nothing + /// useful to do with the error before then. + fn begin(&mut self) { + let _ = execute!(std::io::stdout(), BeginSynchronizedUpdate); + } + + fn end(&mut self) { + let _ = execute!(std::io::stdout(), EndSynchronizedUpdate); + } +} -- 2.51.2