Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 18 additions & 16 deletions src/app_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}

Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
}
}
}
Expand Down Expand Up @@ -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))
}
Expand Down
68 changes: 35 additions & 33 deletions src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
}

Expand All @@ -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,
Expand Down Expand Up @@ -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))
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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() {
Expand Down
49 changes: 49 additions & 0 deletions src/app_state_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
9 changes: 6 additions & 3 deletions src/drain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 } => {
Expand Down
28 changes: 13 additions & 15 deletions src/input/mouse_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

Expand Down Expand Up @@ -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(
Expand All @@ -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())
}
Expand Down Expand Up @@ -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
Expand Down
20 changes: 12 additions & 8 deletions src/input_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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}"),
}
}
}
Expand All @@ -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);
}
Expand Down
10 changes: 6 additions & 4 deletions src/pane_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
}
Expand Down
Loading