diff --git a/crates/maestro-daemon/src/attach.rs b/crates/maestro-daemon/src/attach.rs index c53fb89..eff4e71 100644 --- a/crates/maestro-daemon/src/attach.rs +++ b/crates/maestro-daemon/src/attach.rs @@ -26,11 +26,17 @@ pub(crate) async fn serve( ) -> Result<(), axum::Error> { let (mut sink, mut stream) = socket.split(); + // This client's claim on the shared window size. Registered up front and + // released on every exit below, so a socket that dies mid-session stops + // constraining the size for the clients still attached. + let viewport = manager.attach_viewport(); + // NAP-229: send buffered scrollback up-front so the client sees the last // screenful immediately instead of a blank terminal. The replay lock the // bridge held during snapshot+subscribe guarantees no byte appears both // here and on the live receiver, and none is dropped at the seam. if !replay.is_empty() && sink.send(Message::Binary(replay)).await.is_err() { + manager.release_viewport(id, viewport).await; return Ok(()); } @@ -70,7 +76,7 @@ pub(crate) async fn serve( if let Ok(ctrl) = serde_json::from_str::(&text) { match ctrl { ClientControl::Resize { cols, rows } => { - let _ = manager.resize(id, cols, rows).await; + let _ = manager.set_viewport(id, viewport, cols, rows).await; } ClientControl::Ping => { let pong = serde_json::to_string(&ClientControl::Pong) @@ -88,6 +94,8 @@ pub(crate) async fn serve( } } + manager.release_viewport(id, viewport).await; + if session_ended { let _ = sink.send(Message::Close(None)).await; } diff --git a/crates/maestro-daemon/src/lib.rs b/crates/maestro-daemon/src/lib.rs index b67b707..6d67cea 100644 --- a/crates/maestro-daemon/src/lib.rs +++ b/crates/maestro-daemon/src/lib.rs @@ -30,6 +30,7 @@ pub mod spawn_history; #[cfg(feature = "web")] pub mod static_bundle; pub mod tmux; +pub mod viewport; pub mod worktree; pub mod worktree_probe; diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index ecfea91..fbecd5d 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -7,6 +7,7 @@ use std::collections::{HashMap, HashSet}; use std::path::PathBuf; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; use std::time::Duration; @@ -23,6 +24,7 @@ use crate::pty::{pipe_pane_command, PtyBridge}; use crate::sidecar::{SidecarError, SidecarStore}; use crate::spawn_history::{RecentDir, SpawnHistory, SpawnHistoryRecord}; use crate::tmux::TmuxError; +use crate::viewport::{ViewportId, Viewports}; use crate::worktree::{WorktreeError, WorktreeManager}; /// Capacity of the session-events broadcast channel. A subscriber that falls @@ -166,6 +168,14 @@ struct Inner { report_ttl: Duration, sessions: Mutex>, events: broadcast::Sender, + /// Sizes reported by the clients attached to each session, and what tmux + /// was last told. See [`crate::viewport`] for why the window is not simply + /// set to whatever arrived most recently. + viewports: Mutex, + /// Hands out [`ViewportId`]s. Monotonic for the daemon's lifetime, so a + /// client that reconnects never collides with the registration its + /// previous socket has not finished releasing. + next_viewport: AtomicU64, } impl SessionManager { @@ -206,6 +216,8 @@ impl SessionManager { report_ttl: crate::activity::REPORT_TTL, sessions: Mutex::new(HashMap::new()), events, + viewports: Mutex::new(Viewports::new()), + next_viewport: AtomicU64::new(0), }), } } @@ -751,6 +763,52 @@ impl SessionManager { .map_err(SessionError::Tmux) } + /// Register a client that is about to start reporting its display size, + /// and hand back the handle it reports under. + /// + /// Cheap and infallible: the session it will report against is not + /// consulted until the first [`Self::set_viewport`]. + pub fn attach_viewport(&self) -> ViewportId { + self.inner.next_viewport.fetch_add(1, Ordering::Relaxed) + } + + /// A client reported the size it can display. + /// + /// The shared tmux window is resized only when the size arbitrated across + /// every attached client actually changes — see [`crate::viewport`]. Two + /// clients of different sizes used to resize the window past each other on + /// every report, which leaves a full-screen agent repainting into rows it + /// never clears and looking, to the operator, like a pane that has stopped + /// accepting input. + pub async fn set_viewport( + &self, + id: Uuid, + viewport: ViewportId, + cols: u16, + rows: u16, + ) -> Result<(), SessionError> { + let target = self + .inner + .viewports + .lock() + .await + .set(id, viewport, cols, rows); + match target { + Some((cols, rows)) => self.resize(id, cols, rows).await, + None => Ok(()), + } + } + + /// Drop a detached client's claim on the window size, resizing to what the + /// clients still attached can display. Errors are the caller's to ignore: + /// a socket closing because its session died has nothing left to resize. + pub async fn release_viewport(&self, id: Uuid, viewport: ViewportId) { + let target = self.inner.viewports.lock().await.release(id, viewport); + if let Some((cols, rows)) = target { + let _ = self.resize(id, cols, rows).await; + } + } + /// Rename a session (#125): set `metadata.name`, or clear it when the /// requested name is blank so the session falls back to its command. /// diff --git a/crates/maestro-daemon/src/viewport.rs b/crates/maestro-daemon/src/viewport.rs new file mode 100644 index 0000000..704b3fb --- /dev/null +++ b/crates/maestro-daemon/src/viewport.rs @@ -0,0 +1,186 @@ +//! What size the shared tmux window should be, given every client watching it. +//! +//! A tmux window has one size, but a session can have several clients attached +//! at once — two browser panes, a browser and the TUI. Each reports the size +//! *it* can display, and applying whichever arrived last makes the window +//! flip-flop. Every flip forces a full-screen application (Claude Code, vim) to +//! repaint at a new height, and the rows it drew at the old height are never +//! cleared: the operator ends up looking at a stale input box, types into a +//! box that is not where the screen says it is, and concludes the pane is +//! stuck. +//! +//! So the window is sized to the smallest attached client on each axis — the +//! same rule tmux uses for its own multiple clients, and the only size every +//! client can render without truncation. The size last handed to tmux is +//! remembered, so a report that changes nothing costs no `tmux` fork at all. + +use std::collections::HashMap; + +use uuid::Uuid; + +/// One attached client within a session. Allocated by +/// [`crate::session::SessionManager`] and opaque to everything else. +pub type ViewportId = u64; + +/// Per-session client sizes, and what tmux was last told. +#[derive(Debug, Default)] +pub struct Viewports { + clients: HashMap>, + applied: HashMap, +} + +impl Viewports { + pub fn new() -> Self { + Self::default() + } + + /// Record what `viewport` can display. Returns the size tmux must be set + /// to, or `None` when the window already has it. + pub fn set( + &mut self, + session: Uuid, + viewport: ViewportId, + cols: u16, + rows: u16, + ) -> Option<(u16, u16)> { + self.clients + .entry(session) + .or_default() + .insert(viewport, (cols, rows)); + self.settle(session) + } + + /// Drop a client that has detached. Returns the size tmux must be set to + /// now that it no longer constrains the window. + /// + /// The last client leaving settles nothing: the window keeps the size it + /// has rather than snapping to some remembered default no one is watching. + pub fn release(&mut self, session: Uuid, viewport: ViewportId) -> Option<(u16, u16)> { + let clients = self.clients.get_mut(&session)?; + clients.remove(&viewport); + if clients.is_empty() { + self.clients.remove(&session); + self.applied.remove(&session); + return None; + } + self.settle(session) + } + + /// The smallest size every attached client can render, but only when it + /// differs from what tmux was last told. + /// + /// A zero on either axis is a client that cannot display anything yet — a + /// pane still laying out, a hidden tab — and must not drag the shared + /// window down with it, so it is held but left out of the arbitration. + fn settle(&mut self, session: Uuid) -> Option<(u16, u16)> { + let clients = self.clients.get(&session)?; + let mut cols = u16::MAX; + let mut rows = u16::MAX; + for &(c, r) in clients.values() { + if c == 0 || r == 0 { + continue; + } + cols = cols.min(c); + rows = rows.min(r); + } + if cols == u16::MAX || rows == u16::MAX { + return None; + } + let target = (cols, rows); + if self.applied.get(&session) == Some(&target) { + return None; + } + self.applied.insert(session, target); + Some(target) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_first_report_settles_the_window() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + assert_eq!(v.set(s, 1, 80, 24), Some((80, 24))); + } + + #[test] + fn a_report_that_changes_nothing_costs_no_resize() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + v.set(s, 1, 80, 24); + assert_eq!(v.set(s, 1, 80, 24), None); + assert_eq!(v.set(s, 1, 80, 24), None); + } + + /// The bug this module exists for: two clients of different sizes used to + /// resize the window past each other on every report. + #[test] + fn two_clients_settle_on_the_smaller_instead_of_fighting() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + assert_eq!(v.set(s, 1, 53, 46), Some((53, 46))); + assert_eq!(v.set(s, 2, 53, 20), Some((53, 20))); + // Whatever order they re-report in, the answer stays the smaller one. + assert_eq!(v.set(s, 1, 53, 46), None); + assert_eq!(v.set(s, 2, 53, 20), None); + assert_eq!(v.set(s, 1, 53, 46), None); + } + + #[test] + fn each_axis_is_arbitrated_on_its_own() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + v.set(s, 1, 200, 20); + assert_eq!(v.set(s, 2, 53, 46), Some((53, 20))); + } + + #[test] + fn a_zero_sized_client_does_not_shrink_the_window() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + assert_eq!(v.set(s, 1, 80, 24), Some((80, 24))); + assert_eq!(v.set(s, 2, 0, 0), None); + assert_eq!(v.set(s, 2, 0, 30), None); + } + + #[test] + fn releasing_the_small_client_gives_the_window_back() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + v.set(s, 1, 80, 46); + v.set(s, 2, 80, 20); + assert_eq!(v.release(s, 2), Some((80, 46))); + } + + #[test] + fn releasing_the_last_client_settles_nothing() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + v.set(s, 1, 80, 24); + assert_eq!(v.release(s, 1), None); + // ...and the session is forgotten, so a fresh attach settles again. + assert_eq!(v.set(s, 2, 80, 24), Some((80, 24))); + } + + #[test] + fn releasing_an_unknown_viewport_is_harmless() { + let mut v = Viewports::new(); + let s = Uuid::new_v4(); + assert_eq!(v.release(s, 7), None); + v.set(s, 1, 80, 24); + assert_eq!(v.release(s, 7), None); + } + + #[test] + fn sessions_do_not_arbitrate_against_each_other() { + let mut v = Viewports::new(); + let a = Uuid::new_v4(); + let b = Uuid::new_v4(); + assert_eq!(v.set(a, 1, 80, 24), Some((80, 24))); + assert_eq!(v.set(b, 2, 53, 20), Some((53, 20))); + assert_eq!(v.set(a, 1, 80, 24), None); + } +} diff --git a/crates/maestro-daemon/tests/session_decisions.rs b/crates/maestro-daemon/tests/session_decisions.rs index dae0158..04f4516 100644 --- a/crates/maestro-daemon/tests/session_decisions.rs +++ b/crates/maestro-daemon/tests/session_decisions.rs @@ -555,3 +555,63 @@ async fn spawn_overrides_client_supplied_maestro_variables() { Some(&"http://127.0.0.1:7070".to_string()) ); } + +/// Two clients attached to one session used to resize the shared window past +/// each other on every report they made. Every flip repaints a full-screen +/// agent at a new height and orphans the rows it drew at the old one, which +/// reads to the operator as a pane that has stopped taking input. +#[tokio::test] +async fn clients_of_different_sizes_do_not_resize_the_window_past_each_other() { + let fx = Fx::new(); + let s = fx.manager.create(fx.request("bash")).await.expect("create"); + let web = fx.manager.attach_viewport(); + let tui = fx.manager.attach_viewport(); + + fx.manager.set_viewport(s.id, web, 53, 46).await.unwrap(); + fx.manager.set_viewport(s.id, tui, 53, 20).await.unwrap(); + // Both keep reporting, as a browser's ResizeObserver and the TUI's own + // redraw loop do. Neither report changes the arbitrated size. + for _ in 0..5 { + fx.manager.set_viewport(s.id, web, 53, 46).await.unwrap(); + fx.manager.set_viewport(s.id, tui, 53, 20).await.unwrap(); + } + + let resizes: Vec<_> = fx + .fake + .calls() + .into_iter() + .filter_map(|c| match c { + Call::ResizeWindow { cols, rows, .. } => Some((cols, rows)), + _ => None, + }) + .collect(); + assert_eq!( + resizes, + vec![(53, 46), (53, 20)], + "the window was resized more than the arbitrated size actually changed" + ); +} + +/// The smaller client leaving hands the window back to the one still attached. +#[tokio::test] +async fn releasing_a_client_resizes_to_what_is_left() { + let fx = Fx::new(); + let s = fx.manager.create(fx.request("bash")).await.expect("create"); + let web = fx.manager.attach_viewport(); + let tui = fx.manager.attach_viewport(); + fx.manager.set_viewport(s.id, web, 80, 46).await.unwrap(); + fx.manager.set_viewport(s.id, tui, 80, 20).await.unwrap(); + + fx.manager.release_viewport(s.id, tui).await; + + let resizes: Vec<_> = fx + .fake + .calls() + .into_iter() + .filter_map(|c| match c { + Call::ResizeWindow { cols, rows, .. } => Some((cols, rows)), + _ => None, + }) + .collect(); + assert_eq!(resizes, vec![(80, 46), (80, 20), (80, 46)]); +}