From bb9fb7c4a64a6a8488edeaed6a1fdc3d6bb03dc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Ram=C3=ADrez=20Norambuena?= Date: Mon, 29 Jun 2026 18:27:44 -0400 Subject: [PATCH] fix(locks): degrade instead of crashing on a poisoned grid lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A panic in a parser thread while it holds a pane's grid write lock poisons that RwLock; every main-thread `.read()/.write().unwrap()` then panicked in cascade, taking down the whole app. Add poison-tolerant `Pane::grid_read` and `Pane::grid_write` helpers that log a warning and return `None`, and migrate the ~30 main-thread grid/log_file/pending_resize acquisitions (render, input, search, scroll, resize, restore, paste, toggle-log) to skip the frame/action on poison rather than panic. The parser thread's own writes are left as-is — a thread cannot observe poison it caused itself. Add a regression test that poisons the active pane's grid and confirms update_search_matches / active_grid_rows / scroll_up / scroll_top return degraded values without panicking. --- CHANGELOG.md | 1 + src/app_event.rs | 34 ++++++++++--------- src/app_state.rs | 68 ++++++++++++++++++++------------------ src/app_state_test.rs | 49 +++++++++++++++++++++++++++ src/drain.rs | 9 +++-- src/input/mouse_ops.rs | 28 ++++++++-------- src/input_ops.rs | 20 ++++++----- src/pane_ops.rs | 10 +++--- src/renderer/render_ops.rs | 21 ++++++++---- src/renderer/views.rs | 7 ++-- src/restore.rs | 9 +++-- src/ui/pane.rs | 48 ++++++++++++++++++++++----- 12 files changed, 201 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8b9ec8..6ad5f33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - document the per-pane lock hierarchy (`log_file` > `grid` > `pending_resize`) and the never-hold-two-at-once invariant in `drain.rs` ### Fixed +- degrade instead of crashing when a pane's grid lock is poisoned by a panicked parser thread: main-thread reads (render, input, search, scroll, resize) now skip the frame/action and log a warning rather than panicking in cascade - scroll the tab bar to keep the active tab visible when many tabs are open; previously tabs overflowed the window width and the header rendered garbled - avoid a ghost pane and a crash when a PTY spawn fails: a failed split/tab/restore no longer wires the layout or focus to a non-existent pane, and closing a pane in that state no longer panics - exit cleanly with an error message when no shell can be spawned at startup (e.g. an invalid `shell`/`$SHELL`) instead of panicking on the first window event diff --git a/src/app_event.rs b/src/app_event.rs index dc2d6ad..868c8f8 100644 --- a/src/app_event.rs +++ b/src/app_event.rs @@ -201,7 +201,9 @@ impl App { let Some(entry) = self.state.tabs[tab_idx].panes.get(&active) else { return; }; - let grid = entry.pane.grid.read().unwrap(); + let Some(grid) = entry.pane.grid_read() else { + return; + }; let text = search::extract_match_text(&grid, abs_row, col, len); if !text.is_empty() { let cb = self @@ -227,8 +229,9 @@ impl App { let tab_idx = self.state.active_tab; let active = self.state.tabs[tab_idx].active; - if let Some(entry) = self.state.tabs[tab_idx].panes.get(&active) { - let grid = entry.pane.grid.read().unwrap(); + if let Some(entry) = self.state.tabs[tab_idx].panes.get(&active) + && let Some(grid) = entry.pane.grid_read() + { self.state.search_matches = search::compute_search_matches(&grid, &query); } @@ -250,10 +253,7 @@ impl App { let (sb_len, grid_rows) = self.state.tabs[tab_idx] .panes .get(&active) - .map(|e| { - let g = e.pane.grid.read().unwrap(); - (g.scrollback.len(), g.rows) - }) + .and_then(|e| e.pane.grid_read().map(|g| (g.scrollback.len(), g.rows))) .unwrap_or((0, 24)); let new_offset = search::compute_scroll_offset(abs_row, sb_len, grid_rows); @@ -296,12 +296,13 @@ impl App { let t = &self.state.theme; for tab in &mut self.state.tabs { for entry in tab.panes.values_mut() { - let mut g = entry.pane.grid.write().unwrap(); - g.palette = t.palette; - g.default_fg = t.foreground; - g.default_bg = t.background; - g.cursor_color = t.cursor; - g.selection_color = t.selection; + if let Some(mut g) = entry.pane.grid_write() { + g.palette = t.palette; + g.default_fg = t.foreground; + g.default_bg = t.background; + g.cursor_color = t.cursor; + g.selection_color = t.selection; + } } } } @@ -429,9 +430,10 @@ impl App { let tab = self.tab(); tab.panes .get(&tab.active) - .map(|e| { - let g = e.pane.grid.read().unwrap(); - (g.cols, g.rows, g.application_cursor_keys) + .and_then(|e| { + e.pane + .grid_read() + .map(|g| (g.cols, g.rows, g.application_cursor_keys)) }) .unwrap_or((80, 24, false)) } diff --git a/src/app_state.rs b/src/app_state.rs index cbd1d35..1e59810 100644 --- a/src/app_state.rs +++ b/src/app_state.rs @@ -205,8 +205,9 @@ impl AppState { } let tab_idx = self.active_tab; let active = self.tabs[tab_idx].active; - if let Some(entry) = self.tabs[tab_idx].panes.get(&active) { - let grid = entry.pane.grid.read().unwrap(); + if let Some(entry) = self.tabs[tab_idx].panes.get(&active) + && let Some(grid) = entry.pane.grid_read() + { self.search_matches = crate::search::compute_search_matches(&grid, &query); } if !self.search_matches.is_empty() { @@ -228,10 +229,7 @@ impl AppState { let (sb_len, grid_rows) = self.tabs[tab_idx] .panes .get(&active) - .map(|e| { - let g = e.pane.grid.read().unwrap(); - (g.scrollback.len(), g.rows) - }) + .and_then(|e| e.pane.grid_read().map(|g| (g.scrollback.len(), g.rows))) .unwrap_or((0, 24)); let new_offset = crate::search::compute_scroll_offset(abs_row, sb_len, grid_rows); if let Some(entry) = self.tabs[tab_idx].panes.get_mut(&active) { @@ -263,7 +261,7 @@ impl AppState { fn active_grid_rows(&self) -> usize { self.active_entry() - .map(|e| e.pane.grid.read().unwrap().rows) + .and_then(|e| e.pane.grid_read().map(|g| g.rows)) .unwrap_or(1) } @@ -286,12 +284,11 @@ impl AppState { let Some(entry) = self.active_entry() else { return; }; - let (nc, nr) = motion( - &entry.pane.grid.read().unwrap(), - entry.pane.scroll_offset, - cur_col, - cur_row, - ); + let Some(grid) = entry.pane.grid_read() else { + return; + }; + let (nc, nr) = motion(&grid, entry.pane.scroll_offset, cur_col, cur_row); + drop(grid); self.mode = InputMode::Visual { start_col, start_row, @@ -363,8 +360,10 @@ impl AppState { if e.pane.scroll_offset > 0 { (0, 0) } else { - let g = e.pane.grid.read().unwrap(); - (g.cursor_col, g.cursor_row) + e.pane + .grid_read() + .map(|g| (g.cursor_col, g.cursor_row)) + .unwrap_or((0, 0)) } }) .unwrap_or((0, 0)) @@ -381,14 +380,16 @@ impl AppState { anchored: true, } = self.mode.clone() { - let text = self.active_entry().map(|entry| { - entry.pane.grid.read().unwrap().selected_text( - start_col, - start_row, - cur_col, - cur_row, - entry.pane.scroll_offset, - ) + let text = self.active_entry().and_then(|entry| { + entry.pane.grid_read().map(|g| { + g.selected_text( + start_col, + start_row, + cur_col, + cur_row, + entry.pane.scroll_offset, + ) + }) }); if let Some(text) = text { self.copy_text_to_clipboard(text); @@ -399,10 +400,11 @@ impl AppState { fn do_visual_yank_line(&mut self) { if let InputMode::Visual { cur_row, .. } = self.mode.clone() { - let text = self.active_entry().map(|entry| { - let grid = entry.pane.grid.read().unwrap(); - let cols = grid.cols.saturating_sub(1); - grid.selected_text(0, cur_row, cols, cur_row, entry.pane.scroll_offset) + let text = self.active_entry().and_then(|entry| { + entry.pane.grid_read().map(|grid| { + let cols = grid.cols.saturating_sub(1); + grid.selected_text(0, cur_row, cols, cur_row, entry.pane.scroll_offset) + }) }); if let Some(text) = text { self.copy_text_to_clipboard(text); @@ -804,12 +806,12 @@ impl AppState { fn do_clear_scrollback(&mut self) { if let Some(e) = self.active_entry_mut() { - let mut g = e.pane.grid.write().unwrap(); - g.scrollback.clear(); - g.clear_screen(); - g.cursor_col = 0; - g.cursor_row = 0; - drop(g); + if let Some(mut g) = e.pane.grid_write() { + g.scrollback.clear(); + g.clear_screen(); + g.cursor_col = 0; + g.cursor_row = 0; + } e.pane.scroll_bottom(); } if !self.tabs.is_empty() { diff --git a/src/app_state_test.rs b/src/app_state_test.rs index 810dff5..873a257 100644 --- a/src/app_state_test.rs +++ b/src/app_state_test.rs @@ -567,6 +567,55 @@ fn update_search_matches_finds_content() { assert!(!s.search_matches.is_empty()); } +/// Poison the active pane's grid lock (simulating a parser thread that panicked +/// while holding the write lock) and confirm the main-thread read paths degrade +/// instead of panicking in cascade. Before the poison-tolerant `grid_read` +/// helper, `update_search_matches` / `active_grid_rows` called `.read().unwrap()` +/// and would panic here, crashing the whole app. +#[test] +fn poisoned_grid_does_not_crash_main_thread_reads() { + use std::sync::Arc; + + let mut s = make_state_with_pane(); + let active = s.tab().active; + + // Poison the grid RwLock: take the write lock in a thread and panic. + let grid = s + .tab() + .panes + .get(&active) + .map(|e| Arc::clone(&e.pane.grid)) + .expect("active pane exists"); + let handle = std::thread::spawn(move || { + let _guard = grid.write().unwrap(); + panic!("intentional panic to poison the grid lock"); + }); + assert!(handle.join().is_err(), "helper thread should have panicked"); + assert!( + s.tab().panes.get(&active).unwrap().pane.grid.is_poisoned(), + "grid lock must be poisoned for this test to be meaningful" + ); + + // None of these may panic; they must return degraded values. + s.mode = InputMode::Search { + query: "needle".to_string(), + history_pos: None, + }; + s.update_search_matches(); + assert!( + s.search_matches.is_empty(), + "no matches from a poisoned grid" + ); + + assert_eq!(s.active_grid_rows(), 1, "active_grid_rows falls back to 1"); + + // Scroll helpers on the pane must also degrade rather than panic. + if let Some(e) = s.tab_mut().panes.get_mut(&active) { + e.pane.scroll_up(3); + e.pane.scroll_top(); + } +} + #[test] fn dispatch_send_to_pty_scrolls_to_bottom() { let mut s = make_state_with_pane(); diff --git a/src/drain.rs b/src/drain.rs index 1056c15..b5c70d9 100644 --- a/src/drain.rs +++ b/src/drain.rs @@ -319,9 +319,12 @@ impl App { // drain could have shrunk the scrollback, making `new` // stale. Using `new` would allow scroll_offset > actual // scrollback, causing a viewport glitch. - let current_sb = e.pane.grid.read().unwrap().scrollback_len(); - e.pane.scroll_offset = - (e.pane.scroll_offset + added).min(current_sb); + if let Some(current_sb) = + e.pane.grid_read().map(|g| g.scrollback_len()) + { + e.pane.scroll_offset = + (e.pane.scroll_offset + added).min(current_sb); + } } } ParseEffect::Resized { delta, new_sb } => { diff --git a/src/input/mouse_ops.rs b/src/input/mouse_ops.rs index 8f3f0f3..b5f7a7a 100644 --- a/src/input/mouse_ops.rs +++ b/src/input/mouse_ops.rs @@ -29,10 +29,7 @@ impl App { self.tab() .panes .get(&active) - .map(|e| { - let g = e.pane.grid.read().unwrap(); - (g.mouse_mode, g.mouse_sgr) - }) + .and_then(|e| e.pane.grid_read().map(|g| (g.mouse_mode, g.mouse_sgr))) .unwrap_or((0, false)) } @@ -62,7 +59,7 @@ impl App { let entry = tab.panes.get(&pane_id)?; let m = &entry.metrics; let (grid_cols, grid_rows) = { - let g = entry.pane.grid.read().unwrap(); + let g = entry.pane.grid_read()?; (g.cols, g.rows) }; geometry::pixel_to_cell( @@ -81,7 +78,7 @@ impl App { let (col, row) = self.pixel_to_cell(pane_id, px, py)?; let tab = self.tab(); let entry = tab.panes.get(&pane_id)?; - let grid = entry.pane.grid.read().unwrap(); + let grid = entry.pane.grid_read()?; let url = geometry::cell_url_at_scroll(&grid, entry.pane.scroll_offset, col, row)?; Some(url.as_ref().clone()) } @@ -160,15 +157,16 @@ impl App { ) { let active = self.tab().active; // Extract text first so the immutable borrow on tab() is released. - let text = self.tab().panes.get(&active).map(|entry| { - let grid = entry.pane.grid.read().unwrap(); - grid.selected_text( - start_col, - start_row, - cur_col, - cur_row, - entry.pane.scroll_offset, - ) + let text = self.tab().panes.get(&active).and_then(|entry| { + entry.pane.grid_read().map(|grid| { + grid.selected_text( + start_col, + start_row, + cur_col, + cur_row, + entry.pane.scroll_offset, + ) + }) }); if let Some(text) = text.filter(|t| !t.is_empty()) { let cb = self diff --git a/src/input_ops.rs b/src/input_ops.rs index 6bd8f67..ea3902a 100644 --- a/src/input_ops.rs +++ b/src/input_ops.rs @@ -92,7 +92,7 @@ impl App { .tabs .get_mut(tab_idx) .and_then(|t| t.panes.get_mut(&pane_id)) - && entry.pane.grid.read().unwrap().focus_report + && entry.pane.grid_read().is_some_and(|g| g.focus_report) { let seq: &[u8] = if gained { b"\x1b[I" } else { b"\x1b[O" }; let _ = entry.pty.write_input(seq); @@ -148,12 +148,16 @@ impl App { let active = self.tab().active; let log_dir = self.state.config.logging.log_dir.clone(); if let Some(entry) = self.tab_mut().panes.get_mut(&active) { - let mut guard = entry.log_file.lock().unwrap(); - if guard.is_some() { - *guard = None; - log::info!("Logging stopped for pane {active}"); - } else { - *guard = pane_ops::open_log_file(active, &log_dir); + match entry.log_file.lock() { + Ok(mut guard) => { + if guard.is_some() { + *guard = None; + log::info!("Logging stopped for pane {active}"); + } else { + *guard = pane_ops::open_log_file(active, &log_dir); + } + } + Err(e) => log::warn!("log_file lock poisoned, toggle skipped: {e}"), } } } @@ -168,7 +172,7 @@ impl App { if let Some(text) = text { let active = self.tab().active; if let Some(entry) = self.tab_mut().panes.get_mut(&active) { - let bracketed = entry.pane.grid.read().unwrap().bracketed_paste; + let bracketed = entry.pane.grid_read().is_some_and(|g| g.bracketed_paste); let data = bracketed_paste_encode(&text, bracketed); let _ = entry.pty.write_input(&data); } diff --git a/src/pane_ops.rs b/src/pane_ops.rs index da7db4e..e9a450a 100644 --- a/src/pane_ops.rs +++ b/src/pane_ops.rs @@ -264,9 +264,9 @@ impl App { // Lock hierarchy (see drain.rs): read `grid` in this scoped block // and drop it before taking `pending_resize` below — the two are // never held at once. - let (grid_cols, grid_rows) = { - let g = entry.pane.grid.read().unwrap(); - (g.cols, g.rows) + let Some((grid_cols, grid_rows)) = entry.pane.grid_read().map(|g| (g.cols, g.rows)) + else { + continue; }; if grid_cols != cols || grid_rows != rows { // Update rect immediately so the renderer uses the new layout @@ -276,7 +276,9 @@ impl App { // existing write lock. This avoids blocking the event loop on // grid.write() while the parser holds it (up to ~36 ms), keeping // resize fluid even during heavy output. - *entry.pending_resize.lock().unwrap() = Some((cols, rows)); + if let Ok(mut pr) = entry.pending_resize.lock() { + *pr = Some((cols, rows)); + } let _ = entry.pty.resize(cols as u16, rows as u16); } } diff --git a/src/renderer/render_ops.rs b/src/renderer/render_ops.rs index 7bd110b..a96e705 100644 --- a/src/renderer/render_ops.rs +++ b/src/renderer/render_ops.rs @@ -156,11 +156,12 @@ impl App { [self.state.active_tab] .panes .get(&active_id) - .map(|e| { - let g = e.pane.grid.read().unwrap(); - let cwd = g.cwd.clone().map(|p| statusbar::shorten_home(&p, &home)); - let osc_title = g.osc_title.clone(); - (cwd, osc_title, g.shell_state, g.last_exit_code) + .and_then(|e| { + e.pane.grid_read().map(|g| { + let cwd = g.cwd.clone().map(|p| statusbar::shorten_home(&p, &home)); + let osc_title = g.osc_title.clone(); + (cwd, osc_title, g.shell_state, g.last_exit_code) + }) }) .unwrap_or((None, None, crate::terminal::grid::ShellState::Unknown, None)); // Shell integration UI is gated: when disabled, suppress the indicators. @@ -172,7 +173,7 @@ impl App { let is_logging = self.state.tabs[self.state.active_tab] .panes .get(&active_id) - .is_some_and(|e| e.log_file.lock().unwrap().is_some()); + .is_some_and(|e| e.log_file.lock().is_ok_and(|g| g.is_some())); // build_tab_titles acquires short-lived read-locks (osc_title) on pane grids. // It must run BEFORE the render guards block — holding read-locks via guards @@ -196,7 +197,13 @@ impl App { let screenshot_outcome = { let guards: Vec<(usize, std::sync::RwLockReadGuard)> = grid_arcs .iter() - .map(|(id, arc)| (*id, arc.read().unwrap())) + .filter_map(|(id, arc)| match arc.read() { + Ok(g) => Some((*id, g)), + Err(e) => { + log::warn!("grid read lock poisoned, skipping pane {id}: {e}"); + None + } + }) .collect(); let views = views::collect_pane_views(&self.state, &guards, w, h, tab_h, status_h); diff --git a/src/renderer/views.rs b/src/renderer/views.rs index 86abd1f..3f0d152 100644 --- a/src/renderer/views.rs +++ b/src/renderer/views.rs @@ -34,7 +34,7 @@ pub(crate) fn acquire_grid_guards(state: &AppState) -> Vec<(usize, RwLockReadGua let tab = &state.tabs[state.active_tab]; tab.panes .iter() - .map(|(id, e)| (*id, e.pane.grid.read().unwrap())) + .filter_map(|(id, e)| e.pane.grid_read().map(|g| (*id, g))) .collect() } @@ -131,10 +131,7 @@ pub fn build_tab_titles(state: &AppState) -> Vec<(String, bool, bool)> { let osc_title = tab .panes .get(&tab.active) - .and_then(|e| { - let g = e.pane.grid.read().unwrap(); - g.osc_title.clone() - }) + .and_then(|e| e.pane.grid_read().and_then(|g| g.osc_title.clone())) .filter(|t| !t.starts_with('/') && !t.starts_with('~')); let rename_buf = if is_active { if let InputMode::RenameTab { buf } = &state.mode { diff --git a/src/restore.rs b/src/restore.rs index acc25a0..2ec72d6 100644 --- a/src/restore.rs +++ b/src/restore.rs @@ -29,8 +29,10 @@ impl App { }) .collect(); for (slot, id) in id_order.iter().enumerate() { - if let Some(e) = tab.panes.get(id) { - let lines = e.pane.grid.read().unwrap().scrollback_as_text(); + if let Some(e) = tab.panes.get(id) + && let Some(g) = e.pane.grid_read() + { + let lines = g.scrollback_as_text(); let path = session::scrollback_path_for(self.scope.as_deref(), tab_i, slot); if let Err(err) = session::save_scrollback(&path, &lines) { log::warn!("failed to save scrollback tab={tab_i} slot={slot}: {err}"); @@ -104,11 +106,12 @@ impl App { let lines = session::load_scrollback(&path); if !lines.is_empty() && let Some(entry) = self.state.tabs[tab_idx].panes.get_mut(&pane_id) + && let Some(mut g) = entry.pane.grid_write() { // Seed the live screen with the saved tail so the new shell // prompt continues right where the session left off; the view // stays pinned to the bottom (scroll_offset 0). - entry.pane.grid.write().unwrap().restore_screen(&lines); + g.restore_screen(&lines); } } } diff --git a/src/ui/pane.rs b/src/ui/pane.rs index 4676fbe..75f97dd 100644 --- a/src/ui/pane.rs +++ b/src/ui/pane.rs @@ -1,4 +1,4 @@ -use std::sync::{Arc, RwLock}; +use std::sync::{Arc, RwLock, RwLockReadGuard, RwLockWriteGuard}; use crate::terminal::grid::Grid; @@ -17,24 +17,52 @@ impl Pane { } } + /// Acquire the grid read lock, degrading instead of panicking if it was + /// poisoned (a parser thread panicked while holding the write lock). Main- + /// thread callers use this to drop a frame / skip an action rather than + /// crash the whole app in cascade. Returns `None` on poison. + pub fn grid_read(&self) -> Option> { + match self.grid.read() { + Ok(g) => Some(g), + Err(e) => { + log::warn!("grid read lock poisoned, skipping: {e}"); + None + } + } + } + + /// Acquire the grid write lock, degrading instead of panicking on poison. + /// See [`Pane::grid_read`]. + pub fn grid_write(&self) -> Option> { + match self.grid.write() { + Ok(g) => Some(g), + Err(e) => { + log::warn!("grid write lock poisoned, skipping: {e}"); + None + } + } + } + pub fn resize(&mut self, cols: usize, rows: usize, rect: [u32; 4]) { // Compute delta and new_sb atomically within the write lock so the parser // thread cannot add scrollback between the two reads. - let (delta, new_sb) = { - let mut g = self.grid.write().unwrap(); + let delta_sb = self.grid_write().map(|mut g| { let delta = g.resize(cols, rows); - let new_sb = g.scrollback_len(); - (delta, new_sb) - }; + (delta, g.scrollback_len()) + }); self.rect = rect; - if self.scroll_offset > 0 { + if let Some((delta, new_sb)) = delta_sb + && self.scroll_offset > 0 + { self.scroll_offset = (self.scroll_offset as isize + delta).max(0) as usize; self.scroll_offset = self.scroll_offset.min(new_sb); } } pub fn scroll_up(&mut self, n: usize) { - let max = self.grid.read().unwrap().scrollback_len(); + let Some(max) = self.grid_read().map(|g| g.scrollback_len()) else { + return; + }; self.scroll_offset = (self.scroll_offset + n).min(max); } @@ -43,7 +71,9 @@ impl Pane { } pub fn scroll_top(&mut self) { - self.scroll_offset = self.grid.read().unwrap().scrollback_len(); + if let Some(len) = self.grid_read().map(|g| g.scrollback_len()) { + self.scroll_offset = len; + } } pub fn scroll_bottom(&mut self) {