From 7e03fd15fb874d2b8ea5e3b66976cd1b43287bbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Ram=C3=ADrez=20Norambuena?= Date: Mon, 29 Jun 2026 18:16:19 -0400 Subject: [PATCH] docs(drain): document the per-pane lock hierarchy and no-nesting invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser thread and the main thread share three per-pane locks (grid, log_file, pending_resize) with no written acquisition rule, so a future change could introduce inconsistent ordering and risk deadlock. Document the primary invariant (never hold more than one at a time; every site acquires, works, and releases before the next) and the canonical order for any future nesting (log_file > grid > pending_resize), and add a cross-reference at the pane_ops resize site. Audit of drain.rs, render_ops.rs, and pane_ops.rs found no violations — no code reorder needed. --- CHANGELOG.md | 3 +++ src/drain.rs | 34 ++++++++++++++++++++++++++++++++++ src/pane_ops.rs | 3 +++ 3 files changed, 40 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4abdeff..4e1bc35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added - activity indicator: tabs with output while not focused now show the activity dot in the tab bar; the marker clears as soon as the tab is focused +### Documentation +- document the per-pane lock hierarchy (`log_file` > `grid` > `pending_resize`) and the never-hold-two-at-once invariant in `drain.rs` + ### Fixed - 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 diff --git a/src/drain.rs b/src/drain.rs index b52ec8f..9408d23 100644 --- a/src/drain.rs +++ b/src/drain.rs @@ -13,6 +13,40 @@ use crate::terminal::grid::Grid; use super::App; +// ── Lock hierarchy (per pane) ───────────────────────────────────────────────── +// +// Each `PaneEntry` carries three locks shared between the parser thread (this +// module) and the main thread (`renderer/render_ops.rs`, `pane_ops.rs`): +// +// * `grid` — `Arc>` (cell grid + scrollback) +// * `log_file` — `Arc>>` (raw-byte session log) +// * `pending_resize` — `Arc>>` (main → parser signal) +// +// PRIMARY INVARIANT — never hold more than one of these at a time. +// Every site acquires one lock, does the minimum work, and releases it before +// acquiring the next. This is the rule that actually prevents deadlock and it +// holds everywhere today: +// - parser thread (below): `log_file` is taken and dropped (~:130), then +// `pending_resize.take()` is taken and dropped (~:137), and only then is the +// single `grid.write()` held (~:143). The resize is applied *inside* that +// grid write lock so the main thread never needs `grid.write()` — it only +// sets `pending_resize` and returns. +// - `render_ops::redraw` reads `grid` in a scoped block, drops it, then checks +// `log_file` separately. +// - `pane_ops::sync_pane_sizes_tab` reads `grid` in a scoped block, drops it, +// then sets `pending_resize`. +// +// SECONDARY RULE — should future code ever be forced to hold two at once, +// acquire them outermost-to-innermost in this order, and never acquire an outer +// lock while already holding an inner one: +// +// log_file > grid > pending_resize +// +// The `log_file > grid` half is load-bearing: the parser reaches `log_file` +// before `grid`, so the main thread must never hold `grid` while waiting on +// `log_file` (it would stall both threads — see the note in +// `renderer/render_ops.rs::redraw`). + #[cfg(test)] #[path = "drain_test.rs"] mod tests; diff --git a/src/pane_ops.rs b/src/pane_ops.rs index 1de705b..da7db4e 100644 --- a/src/pane_ops.rs +++ b/src/pane_ops.rs @@ -261,6 +261,9 @@ impl App { let (cols, rows) = entry .metrics .grid_size_for(w.saturating_sub(pad2), h.saturating_sub(pad2)); + // 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)