fix: address audit round 3 findings (P1, B1, P2, R1) (#26)

* fix: address audit round 3 findings P1, B1, P2, R1

P1 (HIGH): the PTY reader cloned the whole parser grid into latest_grid on
every output burst. Apply only the changed rows onto a persistent snapshot
instead (Grid::apply_dirty_row), syncing cursor and hyperlinks since GridDiff
carries neither. A criterion bench (nexterm-vt/benches/grid_snapshot) shows a
single-row update drop from ~19.1us to ~0.53us (~36x) for the common partial
-update case.

B1 (MEDIUM): std::sync::Mutex sites used .expect("...poisoned"), so one panic
while holding a lock cascaded into a panic on every later access. Add
lock_recover() which recovers the inner value and logs, and route the plugin
manager, web session/OAuth/TOTP-setup locks through it.

P2 (MEDIUM): Screen::take_dirty_rows now sizes the result Vec once from the
dirty-row count instead of reallocating as rows are pushed.

R1 (MEDIUM): glyph atlas LRU capacity math used unchecked u32 multiplication;
a large user-configured gpu.atlas_size (>= 65536) could overflow. Use
saturating u64 math with a non-zero fallback.

Tests: Grid::apply_dirty_row parity/bounds tests, lock_recover poison-recovery
test, glyph atlas overflow/zero-dimension tests. cargo test / clippy -D
warnings / fmt --check all green. The attach regression test
(make_full_refresh_reflects_pty_output_emitted_before_attach) is
cfg(not(windows)) and runs in Linux/macOS CI.

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>

* style: fix import ordering in grid_snapshot bench

The editor hook's rustfmt reordered the criterion import differently from the
workspace `cargo fmt --all`, which the CI `fmt --check` leg enforces. Normalize
with the workspace formatter.

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Happy <yesreply@happy.engineering>

Author: 3 people

nexterm-client-gpu/src/glyph_atlas.rs

@@ -157,8 +157,11 @@ impl GlyphAtlas {
         });
 
         // LRU capacity upper bound: size*size divided by the smallest glyph area (8×8).
-        let lru_cap = NonZeroUsize::new(((size * size) / 64).max(256) as usize)
-            .expect("lru capacity is non-zero");
+        // Audit round 3 (R1): use u64 math so a large configured atlas size cannot
+        // overflow the u32 multiplication (debug panic / release wrap).
+        let atlas_sq = (size as u64).saturating_mul(size as u64);
+        let lru_cap = NonZeroUsize::new((atlas_sq / 64).max(256).min(usize::MAX as u64) as usize)
+            .unwrap_or(NonZeroUsize::MIN);
 
         Self {
             texture,
@@ -203,9 +206,12 @@ impl GlyphAtlas {
     /// This is a pure function that can be called without a GPU device, making
     /// it straightforward to unit-test independently.
     fn lru_cap_from_cell(atlas_size: u32, cell_w: u32, cell_h: u32) -> NonZeroUsize {
-        let cell_area = (cell_w as u64 * cell_h as u64).max(1) as u32;
-        NonZeroUsize::new(((atlas_size * atlas_size) / cell_area).max(256) as usize)
-            .expect("lru capacity is non-zero")
+        // Audit round 3 (R1): guard against u32 overflow in atlas_size * atlas_size
+        // and cell_w * cell_h (atlas_size is user-configurable via gpu.atlas_size).
+        let atlas_sq = (atlas_size as u64).saturating_mul(atlas_size as u64);
+        let cell_area = (cell_w as u64).saturating_mul(cell_h as u64).max(1);
+        let cap = (atlas_sq / cell_area).max(256).min(usize::MAX as u64) as usize;
+        NonZeroUsize::new(cap).unwrap_or(NonZeroUsize::MIN)
     }
 
     /// Update LRU capacity based on the actual font cell dimensions.
@@ -417,4 +423,23 @@ mod tests {
         let cap2 = GlyphAtlas::lru_cap_from_cell(2048, 16, 32).get();
         assert_eq!(cap2, cap1 * 4);
     }
+
+    #[test]
+    fn lru_cap_survives_overflowing_atlas_size() {
+        // Audit round 3 (R1): atlas_size is user-configurable; a value whose
+        // square overflows u32 (>= 65536) must not panic or wrap. It should
+        // simply yield a large, valid capacity via saturating u64 math.
+        let cap = GlyphAtlas::lru_cap_from_cell(100_000, 8, 8);
+        assert!(cap.get() >= 256);
+        // 100_000^2 / 64 must be computed in u64, not a wrapped u32.
+        let expected = ((100_000u64 * 100_000) / 64) as usize;
+        assert_eq!(cap.get(), expected);
+    }
+
+    #[test]
+    fn lru_cap_handles_zero_cell_dimensions() {
+        // Degenerate cell dimensions must not divide by zero.
+        let cap = GlyphAtlas::lru_cap_from_cell(1024, 0, 0);
+        assert!(cap.get() >= 256);
+    }
 }

nexterm-proto/src/grid.rs

@@ -82,6 +82,24 @@ impl Grid {
         }
     }
 
+    /// Applies a differential [`DirtyRow`] onto this grid in place.
+    ///
+    /// This is the incremental counterpart to cloning the whole grid: the PTY
+    /// reader keeps a `latest_grid` snapshot fresh for late-attaching clients by
+    /// applying only the rows that changed, instead of copying every cell on
+    /// each output burst (audit round 3, finding P1). Out-of-bounds rows are
+    /// ignored; if the row widths differ, only the overlapping prefix is copied.
+    pub fn apply_dirty_row(&mut self, dirty: &DirtyRow) {
+        if let Some(row) = self.rows.get_mut(dirty.row as usize) {
+            if row.len() == dirty.cells.len() {
+                row.clone_from(&dirty.cells);
+            } else {
+                let n = row.len().min(dirty.cells.len());
+                row[..n].clone_from_slice(&dirty.cells[..n]);
+            }
+        }
+    }
+
     /// Copies the contents of row `src` into row `dst` (out-of-bounds indices are
     /// ignored instead of panicking).
     pub fn copy_row(&mut self, dst: u16, src: u16) {
@@ -187,6 +205,115 @@ mod tests {
         assert_eq!(row, decoded);
     }
 
+    #[test]
+    fn apply_dirty_row_updates_only_target_row() {
+        let mut grid = Grid::new(4, 3);
+        let cells = vec![
+            Cell {
+                ch: 'w',
+                ..Cell::default()
+            },
+            Cell {
+                ch: 'x',
+                ..Cell::default()
+            },
+            Cell {
+                ch: 'y',
+                ..Cell::default()
+            },
+            Cell {
+                ch: 'z',
+                ..Cell::default()
+            },
+        ];
+        grid.apply_dirty_row(&DirtyRow { row: 1, cells });
+        // Row 1 reflects the new content.
+        assert_eq!(grid.get(0, 1).unwrap().ch, 'w');
+        assert_eq!(grid.get(3, 1).unwrap().ch, 'z');
+        // Other rows stay blank.
+        assert_eq!(grid.get(0, 0).unwrap().ch, ' ');
+        assert_eq!(grid.get(0, 2).unwrap().ch, ' ');
+    }
+
+    #[test]
+    fn apply_dirty_row_matches_incremental_and_full_snapshot() {
+        // Applying every dirty row of a screen incrementally must yield the same
+        // grid content as a one-shot full copy would (audit round 3, P1 parity).
+        let mut incremental = Grid::new(3, 2);
+        let mut full = Grid::new(3, 2);
+        let rows = [
+            DirtyRow {
+                row: 0,
+                cells: vec![
+                    Cell {
+                        ch: 'a',
+                        ..Cell::default()
+                    };
+                    3
+                ],
+            },
+            DirtyRow {
+                row: 1,
+                cells: vec![
+                    Cell {
+                        ch: 'b',
+                        ..Cell::default()
+                    };
+                    3
+                ],
+            },
+        ];
+        for d in &rows {
+            incremental.apply_dirty_row(d);
+        }
+        for d in &rows {
+            full.rows[d.row as usize] = d.cells.clone();
+        }
+        assert_eq!(incremental.rows, full.rows);
+    }
+
+    #[test]
+    fn apply_dirty_row_ignores_out_of_bounds_row() {
+        let mut grid = Grid::new(2, 2);
+        // Row index beyond height must be a no-op, not a panic.
+        grid.apply_dirty_row(&DirtyRow {
+            row: 99,
+            cells: vec![
+                Cell {
+                    ch: 'X',
+                    ..Cell::default()
+                };
+                2
+            ],
+        });
+        assert_eq!(grid.get(0, 0).unwrap().ch, ' ');
+    }
+
+    #[test]
+    fn apply_dirty_row_copies_overlapping_prefix_on_width_mismatch() {
+        let mut grid = Grid::new(2, 1);
+        // A wider dirty row: only the first 2 cells fit; no panic.
+        grid.apply_dirty_row(&DirtyRow {
+            row: 0,
+            cells: vec![
+                Cell {
+                    ch: 'p',
+                    ..Cell::default()
+                },
+                Cell {
+                    ch: 'q',
+                    ..Cell::default()
+                },
+                Cell {
+                    ch: 'r',
+                    ..Cell::default()
+                },
+            ],
+        });
+        assert_eq!(grid.get(0, 0).unwrap().ch, 'p');
+        assert_eq!(grid.get(1, 0).unwrap().ch, 'q');
+    }
+
     #[test]
     fn hyperlink_span_field_check() {
         let span = HyperlinkSpan {

nexterm-server/src/ipc/plugin_dispatch.rs

@@ -14,10 +14,7 @@ pub(super) async fn handle_list_plugins(
 ) {
     // Drop the lock before any await (MutexGuard is not Send).
     let paths = {
-        let lock = manager
-            .plugin_manager
-            .lock()
-            .expect("plugin_manager poisoned");
+        let lock = crate::lock_recover(&manager.plugin_manager, "plugin_manager");
         lock.as_ref()
             .map(|m| {
                 m.plugin_paths()
@@ -37,10 +34,7 @@ pub(super) async fn handle_load_plugin(
     path: &str,
 ) {
     let result = {
-        let mut lock = manager
-            .plugin_manager
-            .lock()
-            .expect("plugin_manager poisoned");
+        let mut lock = crate::lock_recover(&manager.plugin_manager, "plugin_manager");
         match lock.as_mut() {
             Some(m) => m.load(std::path::Path::new(path)),
             None => Err(anyhow::anyhow!("plugin manager is not initialized")),
@@ -72,10 +66,7 @@ pub(super) async fn handle_unload_plugin(
     path: &str,
 ) {
     let result = {
-        let mut lock = manager
-            .plugin_manager
-            .lock()
-            .expect("plugin_manager poisoned");
+        let mut lock = crate::lock_recover(&manager.plugin_manager, "plugin_manager");
         match lock.as_mut() {
             Some(m) => m.unload(std::path::Path::new(path)),
             None => Err(anyhow::anyhow!("plugin manager is not initialized")),
@@ -114,10 +105,7 @@ pub(super) async fn handle_reload_plugin(
     path: &str,
 ) {
     let result = {
-        let mut lock = manager
-            .plugin_manager
-            .lock()
-            .expect("plugin_manager poisoned");
+        let mut lock = crate::lock_recover(&manager.plugin_manager, "plugin_manager");
         match lock.as_mut() {
             Some(m) => m.reload(std::path::Path::new(path)),
             None => Err(anyhow::anyhow!("plugin manager is not initialized")),

nexterm-server/src/lib.rs

@@ -29,6 +29,23 @@ pub use runtime_config::{
 use session::SessionManager;
 use snapshot::ServerSnapshot;
 
+/// Lock a [`std::sync::Mutex`], recovering the inner value instead of panicking
+/// when the lock was poisoned by a panic in another holder.
+///
+/// A poisoned lock means a previous holder panicked mid-update, so the guarded
+/// data may be inconsistent. For a long-running terminal server the pragmatic
+/// choice is to log and keep serving rather than let one panic cascade into a
+/// process-wide crash on every subsequent lock (audit round 3, finding B1).
+pub(crate) fn lock_recover<'a, T>(
+    mutex: &'a std::sync::Mutex<T>,
+    context: &str,
+) -> std::sync::MutexGuard<'a, T> {
+    mutex.lock().unwrap_or_else(|poisoned| {
+        warn!("{context}: mutex poisoned; recovering inner state");
+        poisoned.into_inner()
+    })
+}
+
 /// Run the main logic of `nexterm-server`, loading the config file from disk.
 ///
 /// Use this entry point when running as a standalone `nexterm-server` binary.
@@ -370,3 +387,28 @@ async fn shutdown_signal() {
         .expect("failed to install Ctrl+C handler");
     info!("received Ctrl+C");
 }
+
+#[cfg(test)]
+mod lock_recover_tests {
+    use super::lock_recover;
+    use std::sync::{Arc, Mutex};
+
+    #[test]
+    fn recovers_a_poisoned_mutex_without_panicking() {
+        let mutex = Arc::new(Mutex::new(41));
+        // Poison the mutex: panic while holding the guard on another thread.
+        let poison = Arc::clone(&mutex);
+        let _ = std::thread::spawn(move || {
+            let _guard = poison.lock().expect("first lock cannot be poisoned yet");
+            panic!("intentional panic to poison the mutex");
+        })
+        .join();
+
+        assert!(mutex.lock().is_err(), "mutex should now be poisoned");
+
+        // The recovering helper still yields a usable guard.
+        let mut guard = lock_recover(&mutex, "test store");
+        *guard += 1;
+        assert_eq!(*guard, 42);
+    }
+}

nexterm-server/src/pane.rs

@@ -866,15 +866,32 @@ impl Pane {
                                 );
                                 logged_first_diff = true;
                             }
-                            // v1.9.3 fix: refresh the full-grid snapshot so a
+                            let (cursor_col, cursor_row) = parser.screen().cursor();
+
+                            // v1.9.3 fix: keep the full-grid snapshot fresh so a
                             // client attaching after this burst still sees the
                             // current screen via `make_full_refresh`. Without
                             // this the parser state is trapped in the reader
                             // thread and late-attachers get an empty grid.
+                            //
+                            // Audit round 3 (P1): apply only the changed rows
+                            // instead of cloning the entire parser grid on every
+                            // burst. Under heavy output (`yes`, `cat largefile`)
+                            // the full clone copied every cell each time; here we
+                            // touch only the dirty rows we already computed. The
+                            // reader's parser is never resized, so `latest_grid`
+                            // and the dirty rows always share dimensions. Cursor
+                            // and hyperlinks are synced too because `GridDiff`
+                            // carries neither, so a late attach relies on this
+                            // snapshot for both.
                             if let Ok(mut g) = latest_grid_clone.lock() {
-                                *g = parser.screen().full_refresh_grid();
+                                for d in &dirty {
+                                    g.apply_dirty_row(d);
+                                }
+                                g.cursor_col = cursor_col;
+                                g.cursor_row = cursor_row;
+                                g.hyperlinks.clone_from(&parser.screen().grid().hyperlinks);
                             }
-                            let (cursor_col, cursor_row) = parser.screen().cursor();
 
                             // F3 / ADR-0008: move lines that scrolled off during
                             // this burst into the pane-side scrollback mirror,

nexterm-server/src/session.rs

@@ -623,7 +623,7 @@ impl SessionManager {
 
     /// Set the plugin manager (called at server startup).
     pub fn set_plugin_manager(&self, mgr: nexterm_plugin::PluginManager) {
-        let mut lock = self.plugin_manager.lock().expect("plugin_manager poisoned");
+        let mut lock = crate::lock_recover(&self.plugin_manager, "plugin_manager");
         *lock = Some(mgr);
     }
 

nexterm-server/src/web/auth.rs

@@ -54,7 +54,7 @@ impl AuthManager {
             .collect();
 
         let expiry = Instant::now() + self.ttl;
-        let mut sessions = self.sessions.lock().expect("session store mutex poisoned");
+        let mut sessions = crate::lock_recover(&self.sessions, "session store");
 
         // Purge expired sessions up front.
         sessions.retain(|_, v| Instant::now() < v.expiry);
@@ -84,7 +84,7 @@ impl AuthManager {
 
     /// Check whether a session token is still valid.
     pub fn is_valid(&self, token: &str) -> bool {
-        let sessions = self.sessions.lock().expect("session store mutex poisoned");
+        let sessions = crate::lock_recover(&self.sessions, "session store");
         sessions
             .get(token)
             .map(|entry| Instant::now() < entry.expiry)
@@ -93,7 +93,7 @@ impl AuthManager {
 
     /// Return session metadata (for the access log).
     pub fn session_info(&self, token: &str) -> Option<(String, String)> {
-        let sessions = self.sessions.lock().expect("session store mutex poisoned");
+        let sessions = crate::lock_recover(&self.sessions, "session store");
         sessions.get(token).and_then(|entry| {
             if Instant::now() < entry.expiry {
                 Some((entry.auth_method.clone(), entry.user_id.clone()))
@@ -105,16 +105,13 @@ impl AuthManager {
 
     /// Explicitly remove a session (used for logout).
     pub fn revoke_session(&self, token: &str) {
-        self.sessions
-            .lock()
-            .expect("session store mutex poisoned")
-            .remove(token);
+        crate::lock_recover(&self.sessions, "session store").remove(token);
     }
 
     /// Return the number of active sessions (excluding expired ones).
     #[allow(dead_code)]
     pub fn active_count(&self) -> usize {
-        let sessions = self.sessions.lock().expect("session store mutex poisoned");
+        let sessions = crate::lock_recover(&self.sessions, "session store");
         sessions
             .values()
             .filter(|v| Instant::now() < v.expiry)