From 7b16bb6a224f745f531b72f6b28c4adbd6dfa51f Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 14:48:42 +0700 Subject: [PATCH 1/7] fix(linux-ssh): create the app runtime levels a host install lacks --- linux/ARCHITECTURE.md | 2 +- linux/Cargo.lock | 1 + .../crates/session/tests/session_postgres.rs | 4 +- linux/crates/ssh/Cargo.toml | 2 + linux/crates/ssh/src/runtime.rs | 95 +++++++++++++++++++ linux/crates/ssh/tests/openssh_session.rs | 7 +- .../crates/storage/tests/runtime_fallback.rs | 25 +++++ 7 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 linux/crates/storage/tests/runtime_fallback.rs diff --git a/linux/ARCHITECTURE.md b/linux/ARCHITECTURE.md index b3c518a3be..f2f4724214 100644 --- a/linux/ARCHITECTURE.md +++ b/linux/ARCHITECTURE.md @@ -202,7 +202,7 @@ The cancel token is checked between stages, and a stage that finishes after a ca PostgreSQL, SQL Server and ClickHouse open every stream through `open()`. sqlx dials its own sockets, so the MySQL driver passes it the route instead: host and port, or the forward's socket path. Nothing relays bytes between the two. The socket options above apply only to streams `open()` makes, so a direct MySQL connection has no keepalive and finds a dead peer through the probe and its deadlines. -SSH runs the system OpenSSH client ([ADR 0013](docs/decisions/0013-openssh-client-transport.md)): one ControlMaster (`ssh -M -N -T`) per connection, with the user's `~/.ssh/config` applied, and one `ssh -O forward` Unix socket per server it reaches. The sockets live under `$XDG_RUNTIME_DIR/app//ssh/` in directories only the user can open, with mode 0600. Every prompt (host key, password, passphrase, keyboard-interactive) reaches the app through the `tablepro-askpass` helper, and the 20 s handshake timeout pauses while a prompt is open. Under Flatpak, `ssh` comes from the GNOME runtime, and a `ProxyCommand` or `Match exec` that runs a host program, the host's `/etc/ssh` and FIDO keys kept in files do not work. +SSH runs the system OpenSSH client ([ADR 0013](docs/decisions/0013-openssh-client-transport.md)): one ControlMaster (`ssh -M -N -T`) per connection, with the user's `~/.ssh/config` applied, and one `ssh -O forward` Unix socket per server it reaches. The sockets live under `$XDG_RUNTIME_DIR/app//ssh/` in directories only the user can open, with mode 0600. Only Flatpak creates `app/`, so a host install makes the missing levels with mode 0700, each opened without following a symlink and checked to be the user's; with `XDG_RUNTIME_DIR` unset, GLib puts the same layout under the cache directory. Every prompt (host key, password, passphrase, keyboard-interactive) reaches the app through the `tablepro-askpass` helper, and the 20 s handshake timeout pauses while a prompt is open. Under Flatpak, `ssh` comes from the GNOME runtime, and a `ProxyCommand` or `Match exec` that runs a host program, the host's `/etc/ssh` and FIDO keys kept in files do not work. ### Trust diff --git a/linux/Cargo.lock b/linux/Cargo.lock index 5a1af6ffb2..46d73fb052 100644 --- a/linux/Cargo.lock +++ b/linux/Cargo.lock @@ -4610,6 +4610,7 @@ version = "0.0.0" dependencies = [ "async-trait", "futures", + "rustix", "secrecy", "tablepro-core", "tablepro-test-support", diff --git a/linux/crates/session/tests/session_postgres.rs b/linux/crates/session/tests/session_postgres.rs index a794a2f7b8..a12b64996b 100644 --- a/linux/crates/session/tests/session_postgres.rs +++ b/linux/crates/session/tests/session_postgres.rs @@ -93,8 +93,8 @@ fn isolated_ssh() -> IsolatedSsh { 0o755, ) .expect("the ssh wrapper"); - let runtime = dir.path().join("runtime"); - std::fs::create_dir(&runtime).expect("the runtime directory"); + // `app/` is left for the runtime to create, as on a host install. + let runtime = dir.path().join("runtime").join("app").join("app.tablepro.TablePro"); let services = SshServices { runtime: Arc::new(SshRuntimeCell::new(runtime)), ssh_program: wrapper, diff --git a/linux/crates/ssh/Cargo.toml b/linux/crates/ssh/Cargo.toml index 5f3cdeda7a..e7491d1ac2 100644 --- a/linux/crates/ssh/Cargo.toml +++ b/linux/crates/ssh/Cargo.toml @@ -17,6 +17,8 @@ path = "src/bin/tablepro-askpass.rs" tablepro-core = { path = "../core" } async-trait.workspace = true futures.workspace = true +# mkdirat and openat with O_NOFOLLOW, and the effective uid, none of which std has. +rustix = { version = "1.1", default-features = false, features = ["std", "fs", "process"] } secrecy.workspace = true thiserror.workspace = true tokio = { workspace = true, features = ["io-util", "process", "net", "time", "sync", "fs"] } diff --git a/linux/crates/ssh/src/runtime.rs b/linux/crates/ssh/src/runtime.rs index ecacc293b8..88f90936dd 100644 --- a/linux/crates/ssh/src/runtime.rs +++ b/linux/crates/ssh/src/runtime.rs @@ -4,6 +4,8 @@ use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; use std::sync::Arc; +use rustix::fs::{CWD, Mode, OFlags, fstat, mkdirat, openat}; +use rustix::io::Errno; use tablepro_core::SshFailure; use uuid::Uuid; @@ -17,6 +19,7 @@ pub struct SshRuntime { impl SshRuntime { pub fn acquire(runtime_dir: &Path) -> Result, SshFailure> { + create_missing_levels(runtime_dir)?; let root = runtime_dir.join("ssh"); let root_metadata = private_dir(&root)?; let (instance_id, instance_dir) = create_unique_dir(&root, 8)?; @@ -65,6 +68,67 @@ impl SshRuntime { } } +/// Only Flatpak creates `$XDG_RUNTIME_DIR/app/`, and with the +/// variable unset GLib names a directory under the cache instead, so a +/// host install makes the missing levels itself. Each one is made and +/// then opened through its parent's descriptor without following a +/// symlink, and must belong to this user, so a link or a directory +/// planted in the path cannot move the control sockets. +fn create_missing_levels(runtime_dir: &Path) -> Result<(), SshFailure> { + let missing: Vec<&Path> = runtime_dir + .ancestors() + .filter(|dir| !dir.as_os_str().is_empty()) + .take_while(|dir| is_missing(dir)) + .collect(); + let Some(top) = missing.last() else { + return Ok(()); + }; + let base = top + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + let mut parent = openat( + CWD, + base, + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::CLOEXEC, + Mode::empty(), + ) + .map_err(|errno| unsafe_dir(base, &errno.into()))?; + let owner = rustix::process::geteuid().as_raw(); + for dir in missing.into_iter().rev() { + let Some(name) = dir.file_name() else { + return Err(SshFailure::RuntimeDirUnsafe { + path: dir.to_owned(), + detail: "the path does not end in a directory name".to_owned(), + }); + }; + match mkdirat(&parent, name, Mode::RWXU) { + Ok(()) | Err(Errno::EXIST) => {} + Err(errno) => return Err(unsafe_dir(dir, &errno.into())), + } + let child = openat( + &parent, + name, + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC, + Mode::empty(), + ) + .map_err(|errno| unsafe_dir(dir, &errno.into()))?; + let stat = fstat(&child).map_err(|errno| unsafe_dir(dir, &errno.into()))?; + if stat.st_uid != owner { + return Err(SshFailure::RuntimeDirUnsafe { + path: dir.to_owned(), + detail: "the directory belongs to another user".to_owned(), + }); + } + parent = child; + } + Ok(()) +} + +fn is_missing(path: &Path) -> bool { + matches!(std::fs::symlink_metadata(path), Err(error) if error.kind() == io::ErrorKind::NotFound) +} + fn private_dir(path: &Path) -> Result { match DirBuilder::new().mode(0o700).create(path) { Ok(()) => {} @@ -150,6 +214,37 @@ mod tests { assert_ne!(first.instance_dir(), second.instance_dir()); } + #[test] + fn acquire_creates_the_app_levels_a_host_install_lacks() { + let temp = tempfile::tempdir().unwrap(); + let xdg_runtime = temp.path().join("runtime"); + std::fs::create_dir(&xdg_runtime).unwrap(); + std::fs::set_permissions(&xdg_runtime, std::fs::Permissions::from_mode(0o700)).unwrap(); + let runtime_dir = xdg_runtime.join("app").join("app.tablepro.TablePro"); + + let runtime = SshRuntime::acquire(&runtime_dir).unwrap(); + + assert_eq!(runtime.root(), runtime_dir.join("ssh")); + assert_eq!(mode(&xdg_runtime.join("app")), 0o700); + assert_eq!(mode(&runtime_dir), 0o700); + assert!(runtime.create_master_dir().is_ok()); + } + + #[test] + fn acquire_creates_every_level_of_the_cache_fallback() { + // With XDG_RUNTIME_DIR unset GLib answers the cache directory, + // which a new account may not have yet either. + let home = tempfile::tempdir().unwrap(); + let runtime_dir = home.path().join(".cache").join("app").join("app.tablepro.TablePro"); + + let runtime = SshRuntime::acquire(&runtime_dir).unwrap(); + + for level in [home.path().join(".cache"), home.path().join(".cache/app"), runtime_dir] { + assert_eq!(mode(&level), 0o700, "{}", level.display()); + } + assert_eq!(mode(runtime.root()), 0o700); + } + #[test] fn group_writable_root_is_unsafe() { let temp = tempfile::tempdir().unwrap(); diff --git a/linux/crates/ssh/tests/openssh_session.rs b/linux/crates/ssh/tests/openssh_session.rs index 7eb888cdd7..5a648010c3 100644 --- a/linux/crates/ssh/tests/openssh_session.rs +++ b/linux/crates/ssh/tests/openssh_session.rs @@ -65,8 +65,11 @@ async fn start() -> TestResult { 0o755, )?; - let runtime_dir = temp.path().join("runtime"); - std::fs::create_dir(&runtime_dir)?; + // The layout a .deb or .rpm install gets: `XDG_RUNTIME_DIR` exists, + // the `app/` levels under it do not. + let xdg_runtime = temp.path().join("runtime"); + std::fs::create_dir(&xdg_runtime)?; + let runtime_dir = xdg_runtime.join("app").join("app.tablepro.TablePro"); let services = SshServices { runtime: Arc::new(SshRuntimeCell::new(runtime_dir)), ssh_program: wrapper, diff --git a/linux/crates/storage/tests/runtime_fallback.rs b/linux/crates/storage/tests/runtime_fallback.rs new file mode 100644 index 0000000000..f215e2ad83 --- /dev/null +++ b/linux/crates/storage/tests/runtime_fallback.rs @@ -0,0 +1,25 @@ +//! With `XDG_RUNTIME_DIR` unset, as under some display managers and in a +//! plain `ssh -X` login, GLib answers the cache directory, so the SSH +//! control sockets go under `$XDG_CACHE_HOME/app/`, which nothing +//! else creates. +//! +//! GLib reads the variables once per process, which is why this is a test +//! binary of its own with one test, and why they are set before GLib is +//! asked anything. + +use tablepro_storage::StoragePaths; + +#[test] +fn an_unset_runtime_dir_falls_back_to_the_cache_dir() { + let cache = tempfile::tempdir().expect("a directory"); + // SAFETY: the harness runs this binary's one test alone, and nothing + // in it reads the environment yet. + unsafe { + std::env::remove_var("XDG_RUNTIME_DIR"); + std::env::set_var("XDG_CACHE_HOME", cache.path()); + } + + let paths = StoragePaths::from_glib("tablepro", "app.tablepro.TablePro"); + + assert_eq!(paths.runtime, cache.path().join("app").join("app.tablepro.TablePro")); +} From 841be2267cc230cca638d7d3a784ea27bbd63a78 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 15:27:06 +0700 Subject: [PATCH 2/7] fix(linux-editor): find batch starts in one pass over the script --- .../crates/app/src/ui/editor/script_runner.rs | 28 ++++++-- .../src/sql_syntax/script/text_offsets.rs | 67 ++++++++++++++++++- 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/linux/crates/app/src/ui/editor/script_runner.rs b/linux/crates/app/src/ui/editor/script_runner.rs index dc486e4e02..06d6d1beaf 100644 --- a/linux/crates/app/src/ui/editor/script_runner.rs +++ b/linux/crates/app/src/ui/editor/script_runner.rs @@ -4,9 +4,8 @@ use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; use std::time::{Duration, Instant}; use tablepro_core::sql_syntax::SqlGrammar; -use tablepro_core::sql_syntax::script::{ - BatchErrorPolicy, LexicalSettings, ScriptDiagnostic, ScriptPlan, text_offsets, -}; +use tablepro_core::sql_syntax::script::text_offsets::CharCounter; +use tablepro_core::sql_syntax::script::{BatchErrorPolicy, LexicalSettings, ScriptDiagnostic, ScriptPlan}; use tablepro_core::{ EditorSession, EditorSessionState, ResultEvent, ResultSink, RowLimit, RunOptions, SinkFlow, UserSql, }; @@ -50,6 +49,7 @@ pub fn plan_batches( base_line: usize, ) -> (Vec, BatchErrorPolicy, Vec) { let plan = ScriptPlan::build(&scoped.text, grammar, LexicalSettings::default_for(grammar)); + let mut chars = CharCounter::new(&scoped.text); let batches = plan .batches() .iter() @@ -58,7 +58,7 @@ pub fn plan_batches( (!text.is_empty()).then(|| RunBatch { text: text.to_owned(), repeat: batch.repeat.get(), - error_base: scoped.base_char + text_offsets::byte_to_char(&scoped.text, batch.range.start), + error_base: scoped.base_char + chars.char_at(batch.range.start), }) }) .collect(); @@ -457,6 +457,26 @@ mod tests { assert_eq!(batches[1].error_base, 50 + 14); } + #[test] + fn a_long_script_finds_every_start_in_one_pass() { + // Counting each start from the top of the text is about 10^11 + // steps here, which no run finishes; one pass is about 10^6. + const STATEMENTS: usize = 100_000; + let statement = "SELECT 'é';\n"; + let scoped = ScopedText { + text: statement.repeat(STATEMENTS), + base_char: 7, + }; + + let (batches, _, _) = plan_batches(&scoped, SqlGrammar::PostgreSql, 0); + + assert_eq!(batches.len(), STATEMENTS); + let width = statement.chars().count(); + for (index, batch) in batches.iter().enumerate() { + assert_eq!(batch.error_base, 7 + index * width, "batch {index}"); + } + } + #[test] fn a_timed_out_run_is_a_stop() { assert!(!continues(Verdict::Stopped, BatchErrorPolicy::ContinueNextBatch)); diff --git a/linux/crates/core/src/sql_syntax/script/text_offsets.rs b/linux/crates/core/src/sql_syntax/script/text_offsets.rs index acd4991528..21fab65215 100644 --- a/linux/crates/core/src/sql_syntax/script/text_offsets.rs +++ b/linux/crates/core/src/sql_syntax/script/text_offsets.rs @@ -4,14 +4,58 @@ pub fn char_to_byte(text: &str, char_offset: usize) -> usize { .map_or(text.len(), |(byte, _)| byte) } +/// How many chars start before `byte`. pub fn byte_to_char(text: &str, byte: usize) -> usize { - text.char_indices().take_while(|(index, _)| *index < byte).count() + CharCounter::new(text).char_at(byte) +} + +/// `byte_to_char` for offsets that only grow, such as a script's batch +/// starts. Each call counts only the text since the previous one, so a +/// whole script is one pass rather than one pass per batch. +#[derive(Debug)] +pub struct CharCounter<'a> { + text: &'a str, + byte: usize, + chars: usize, +} + +impl<'a> CharCounter<'a> { + pub fn new(text: &'a str) -> Self { + Self { + text, + byte: 0, + chars: 0, + } + } + + /// An offset behind the previous one counts from the start again. + pub fn char_at(&mut self, byte: usize) -> usize { + let byte = byte.min(self.text.len()); + if byte < self.byte { + self.byte = 0; + self.chars = 0; + } + self.chars += self.text.as_bytes().get(self.byte..byte).map_or(0, char_starts); + self.byte = byte; + self.chars + } +} + +/// Every byte that is not a UTF-8 continuation byte starts a char. +fn char_starts(bytes: &[u8]) -> usize { + bytes.iter().filter(|byte| **byte & 0xC0 != 0x80).count() } #[cfg(test)] mod tests { + use proptest::prelude::*; + use super::*; + fn counted_from_the_start(text: &str, byte: usize) -> usize { + text.char_indices().take_while(|(index, _)| *index < byte).count() + } + #[test] fn converts_ascii_offsets_one_to_one() { assert_eq!(char_to_byte("select", 3), 3); @@ -23,4 +67,25 @@ mod tests { assert_eq!(char_to_byte("ab", 10), 2); assert_eq!(byte_to_char("ab", 10), 2); } + + #[test] + fn an_offset_inside_a_char_counts_that_char() { + // 'é' is bytes 1 and 2. + assert_eq!(byte_to_char("aéb", 2), counted_from_the_start("aéb", 2)); + assert_eq!(byte_to_char("aéb", 2), 2); + } + + proptest! { + #[test] + fn the_counter_agrees_with_counting_from_the_start( + text in "[ -~\n\té😀]{0,80}", + mut offsets in proptest::collection::vec(0usize..400, 0..12), + ) { + offsets.sort_unstable(); + let mut counter = CharCounter::new(&text); + for offset in offsets.iter().copied().chain(offsets.iter().rev().copied()) { + prop_assert_eq!(counter.char_at(offset), counted_from_the_start(&text, offset)); + } + } + } } From ad978eefd6aeb19ea82362709ec6944d92ac00cb Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 15:27:07 +0700 Subject: [PATCH 3/7] fix(linux-grid): cut every long value in Row as JSON, refuse huge edits --- linux/crates/app/src/ui/grid/cell_edit.rs | 45 +++++- .../app/src/ui/grid/cell_editor_kind.rs | 5 + .../crates/app/src/ui/grid/row_json_dialog.rs | 134 +++++++++++++++--- 3 files changed, 161 insertions(+), 23 deletions(-) diff --git a/linux/crates/app/src/ui/grid/cell_edit.rs b/linux/crates/app/src/ui/grid/cell_edit.rs index d2d8e5a706..6fc3b0ddeb 100644 --- a/linux/crates/app/src/ui/grid/cell_edit.rs +++ b/linux/crates/app/src/ui/grid/cell_edit.rs @@ -3,7 +3,7 @@ use gtk4::prelude::*; use tablepro_core::edit::{CellInput, EditParseError, EditText, edit_text}; use super::calendar_editor::CalendarEditor; -use super::cell_editor_kind::CellEditorKind; +use super::cell_editor_kind::{CellEditorKind, EDIT_MAX_BYTES}; use super::edit_commit::empty_placeholder; use super::inline_editor::{InlineEditor, InlineOptions}; use super::multiline_editor::MultilineEditor; @@ -44,6 +44,12 @@ pub(crate) fn begin_edit(cell: &GridCell) { } return; } + EditText::Editable(editable) if editable.len() > EDIT_MAX_BYTES => { + if let Some(sink) = cell.sink() { + sink(GridMsg::ShowToast(too_large_to_edit())); + } + return; + } EditText::Editable(editable) => editable.clone(), EditText::ReadOnly(reason) => { if let Some(sink) = cell.sink() { @@ -81,6 +87,11 @@ pub(crate) fn begin_edit(cell: &GridCell) { } } +fn too_large_to_edit() -> String { + let size = gtk::glib::format_size(EDIT_MAX_BYTES as u64); + crate::i18n::gettext_f("Values over {size} can't be edited here", &[("size", size.as_str())]) +} + pub(super) fn install_edit_triggers(cell: &GridCell) { // Editing takes a double-click, since a single click in a grid // selects the row. Capture phase, because the column view's row @@ -104,3 +115,35 @@ pub(super) fn install_edit_triggers(cell: &GridCell) { }); cell.add_controller(gesture); } + +#[cfg(test)] +mod tests { + use tablepro_core::Value; + + use super::*; + use crate::ui::grid::test_grid::{Grid, named}; + + #[gtk4::test] + fn a_value_too_large_to_lay_out_does_not_open_and_says_why() { + let large = "x".repeat(EDIT_MAX_BYTES + 1); + let grid = Grid::open( + vec![named("id", "integer", true), named("body", "text", false)], + vec![vec![Value::Int(1), Value::Text(large)]], + ); + let cell = grid.cell_of(1, 1); + + begin_edit(&cell); + + assert!(!cell.is_editing(), "an editor opened on the whole value"); + let toasts: Vec = grid + .take_messages() + .into_iter() + .filter_map(|message| match message { + GridMsg::ShowToast(text) => Some(text), + _ => None, + }) + .collect(); + // GLib puts a no-break space between the number and the unit. + assert_eq!(toasts, ["Values over 1.0\u{a0}MB can't be edited here"]); + } +} diff --git a/linux/crates/app/src/ui/grid/cell_editor_kind.rs b/linux/crates/app/src/ui/grid/cell_editor_kind.rs index ff88848b00..4a64716827 100644 --- a/linux/crates/app/src/ui/grid/cell_editor_kind.rs +++ b/linux/crates/app/src/ui/grid/cell_editor_kind.rs @@ -10,6 +10,11 @@ use super::CellLock; /// one-line entry lays all of it out at once and shows almost none. pub const INLINE_EDIT_MAX_BYTES: usize = 4096; +/// Past this many bytes a value does not open for editing at all: the +/// multi-line editor lays out every line of it on the GTK thread, and a +/// value of megabytes stalls the window. +pub const EDIT_MAX_BYTES: usize = 1_000_000; + /// The editor a cell opens with. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CellEditorKind { diff --git a/linux/crates/app/src/ui/grid/row_json_dialog.rs b/linux/crates/app/src/ui/grid/row_json_dialog.rs index 901f27bece..ca85a19a49 100644 --- a/linux/crates/app/src/ui/grid/row_json_dialog.rs +++ b/linux/crates/app/src/ui/grid/row_json_dialog.rs @@ -16,6 +16,10 @@ use crate::ui::format::{format_thousands, plural_count}; /// Longer text is cut in the dialog: the view lays out every character /// it holds, and a value of megabytes would stall it. const SHOWN_CHARS: usize = 10_000; +/// Binary shows as hex, two characters a byte. +const SHOWN_BYTES: usize = SHOWN_CHARS / 2; +/// Each item of an array is a line of its own. +const SHOWN_ITEMS: usize = 1_000; /// One row to show as JSON, under the grid's visible columns. #[derive(Debug)] @@ -35,19 +39,7 @@ fn render(request: RowJsonRequest) -> RowJson { let values = request.row.to_values(); let full = tablepro_core::export::row_to_json(&request.columns, &values); let mut shortened = false; - let shown_values: Vec = values - .into_iter() - .map(|value| match value { - Value::Text(text) => match shorten(&text) { - Some(cut) => { - shortened = true; - Value::Text(cut) - } - None => Value::Text(text), - }, - other => other, - }) - .collect(); + let shown_values: Vec = values.into_iter().map(|value| shown(value, &mut shortened)).collect(); let shown = match shortened { true => tablepro_core::export::row_to_json(&request.columns, &shown_values), false => full.clone(), @@ -55,23 +47,71 @@ fn render(request: RowJsonRequest) -> RowJson { RowJson { full, shown, shortened } } +/// `value` as the dialog shows it, with anything long cut short and +/// saying how much is left out. A document cut short is no longer JSON, +/// so it shows as text. +fn shown(value: Value, shortened: &mut bool) -> Value { + let cut = match &value { + Value::Text(text) => shorten(text), + Value::Json(json) => shorten(json.as_str()), + Value::Other(other) => shorten(&other.text), + Value::Bits(bits) => shorten(&bits.to_string()), + Value::Decimal(decimal) => shorten(&decimal.to_string()), + Value::Bytes(bytes) => shorten_bytes(bytes), + _ => None, + }; + if let Some(cut) = cut { + *shortened = true; + return Value::Text(cut); + } + let Value::Array(items) = value else { + return value; + }; + let more = items.len().saturating_sub(SHOWN_ITEMS) as u64; + let mut kept: Vec = items + .into_iter() + .take(SHOWN_ITEMS) + .map(|item| shown(item, shortened)) + .collect(); + if more > 0 { + *shortened = true; + kept.push(Value::Text(crate::i18n::ngettext_f( + "… ({count} more item)", + "… ({count} more items)", + plural_count(more), + &[("count", &format_thousands(more))], + ))); + } + Value::Array(kept) +} + /// `text` cut at a character boundary after `SHOWN_CHARS` characters, /// saying how much is left out, or `None` when it fits. fn shorten(text: &str) -> Option { let (end, _) = text.char_indices().nth(SHOWN_CHARS)?; - let more = (text.len() - end) as u64; + Some(format!("{}{}", &text[..end], more_bytes((text.len() - end) as u64))) +} + +/// The hex the grid writes for binary, cut after `SHOWN_BYTES` bytes. +fn shorten_bytes(bytes: &[u8]) -> Option { + let more = bytes.len().checked_sub(SHOWN_BYTES).filter(|more| *more > 0)?; + let head = bytes.get(..SHOWN_BYTES)?; Some(format!( - "{}{}", - &text[..end], - crate::i18n::ngettext_f( - "… ({count} more byte)", - "… ({count} more bytes)", - plural_count(more), - &[("count", &format_thousands(more))], - ) + "0x{}{}", + tablepro_core::hex::encode_lower(head), + more_bytes(more as u64) )) } +fn more_bytes(more: u64) -> String { + crate::i18n::ngettext_f( + "… ({count} more byte)", + "… ({count} more bytes)", + plural_count(more), + &[("count", &format_thousands(more))], + ) +} + /// Render the row off the GTK thread, then show it over `parent`. pub(crate) async fn present_row_json( parent: gtk::Widget, @@ -231,6 +271,56 @@ mod tests { runtime.shutdown(); } + #[test] + fn every_kind_of_long_value_is_cut_for_display_and_kept_for_copy() { + const LONG: usize = 50_000; + let column = |name: &str, kind: &str| ResultColumn::new(name, ColumnType::untyped_text(kind)); + let document = format!("[{}]", vec!["1"; LONG].join(",")); + let shape = "x".repeat(LONG); + let request = RowJsonRequest { + columns: vec![ + column("doc", "jsonb"), + column("blob", "bytea"), + column("shape", "geometry"), + column("tags", "text[]"), + ] + .into(), + row: RowSnapshot::new( + vec![ + Value::Json(tablepro_core::value::JsonText::parse(document.clone()).expect("JSON")), + Value::Bytes(vec![0xab; LONG]), + Value::Other(Box::new(tablepro_core::value::OtherValue { + type_name: "geometry".to_owned(), + text: shape.clone(), + })), + Value::Array(vec![Value::Text("t".to_owned()); LONG]), + ] + .into(), + Vec::new(), + ), + }; + + let rendered = render(request); + + assert!(rendered.shortened); + let longest = rendered.shown.lines().map(str::len).max().unwrap_or_default(); + assert!(longest < SHOWN_CHARS + 100, "a line of {longest} bytes is laid out"); + assert!(rendered.shown.len() < 100_000, "{} bytes shown", rendered.shown.len()); + assert!( + rendered.shown.contains("… (45,000 more bytes)"), + "the binary does not say what is left" + ); + assert!( + rendered.shown.contains("… (49,000 more items)"), + "the array does not say what is left" + ); + let full: serde_json::Value = serde_json::from_str(&rendered.full).expect("the copy is JSON"); + assert_eq!(full["doc"].as_array().map(Vec::len), Some(LONG)); + assert_eq!(full["blob"].as_str().map(str::len), Some(2 + 2 * LONG)); + assert_eq!(full["shape"], serde_json::json!(shape)); + assert_eq!(full["tags"].as_array().map(Vec::len), Some(LONG)); + } + #[gtk4::test] fn a_closed_dialog_lets_go_of_the_row() { adw::init().expect("libadwaita initialises under the test backend"); From 20b4088686e7465a5b6e52807cbc62167d90e967 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 15:38:55 +0700 Subject: [PATCH 4/7] feat(linux): save a crash report and announce it at the next launch --- linux/crates/app/Cargo.toml | 7 + .../app/src/application/crash_notice.rs | 62 ++++++ linux/crates/app/src/application/imp.rs | 3 + linux/crates/app/src/application/mod.rs | 2 + linux/crates/app/src/crash_report.rs | 186 ++++++++++++++++++ linux/crates/app/src/lib.rs | 5 +- .../app/src/ui/session_window/crash_notice.rs | 159 +++++++++++++++ linux/crates/app/src/ui/session_window/imp.rs | 3 + linux/crates/app/src/ui/session_window/mod.rs | 2 + .../crates/app/src/ui/session_window/tests.rs | 8 +- linux/crates/app/tests/crash_report.rs | 144 ++++++++++++++ linux/crates/storage/src/fs.rs | 37 +++- linux/crates/storage/src/paths.rs | 4 + linux/docs/error-handling.md | 12 +- linux/docs/storage.md | 3 + linux/po/POTFILES.in | 3 + 16 files changed, 636 insertions(+), 4 deletions(-) create mode 100644 linux/crates/app/src/application/crash_notice.rs create mode 100644 linux/crates/app/src/crash_report.rs create mode 100644 linux/crates/app/src/ui/session_window/crash_notice.rs create mode 100644 linux/crates/app/tests/crash_report.rs diff --git a/linux/crates/app/Cargo.toml b/linux/crates/app/Cargo.toml index 7f67634c2d..01fe197258 100644 --- a/linux/crates/app/Cargo.toml +++ b/linux/crates/app/Cargo.toml @@ -28,6 +28,13 @@ name = "logging" path = "tests/logging.rs" harness = false +# A panic hook is process-wide and the crash ends the process, so each +# case runs in a child of the binary. +[[test]] +name = "crash_report" +path = "tests/crash_report.rs" +harness = false + # These start the application, which GTK allows only on the process's # main thread, so they run without the test harness. [[test]] diff --git a/linux/crates/app/src/application/crash_notice.rs b/linux/crates/app/src/application/crash_notice.rs new file mode 100644 index 0000000000..29ff4b724c --- /dev/null +++ b/linux/crates/app/src/application/crash_notice.rs @@ -0,0 +1,62 @@ +use std::path::PathBuf; + +use gtk4::glib; +use gtk4::prelude::*; +use gtk4::subclass::prelude::*; + +use super::TableProApplication; +use crate::ui::SessionWindow; + +impl TableProApplication { + /// Looks off the GTK thread for a report the last run left when it + /// crashed, and has a window say so. + pub(super) fn look_for_crash_report(&self) { + let Some(services) = self.services() else { + return; + }; + let dir = services.storage.paths().crashes_dir(); + let looking = services + .tasks + .spawn_blocking_task(move || crate::crash_report::take_unannounced(&dir)); + let app = self.downgrade(); + glib::spawn_future_local(async move { + let report = match looking.await { + Ok(Ok(Some(report))) => report, + Ok(Ok(None)) => return, + Ok(Err(error)) => { + tracing::warn!(%error, "could not look for a crash report"); + return; + } + Err(failure) => { + tracing::warn!(%failure, "the crash report lookup failed"); + return; + } + }; + if let Some(app) = app.upgrade() { + app.announce_crash(report); + } + }); + } + + /// The window in front says it, or the first one to open when the + /// windows of a restored session are still on their way. + fn announce_crash(&self, report: PathBuf) { + let window = self.active_window().and_downcast::().or_else(|| { + self.windows() + .into_iter() + .find_map(|window| window.downcast::().ok()) + }); + match window { + Some(window) => window.announce_crash(report), + None => { + self.imp().crash_report.replace(Some(report)); + } + } + } + + pub(super) fn hand_over_crash_report(&self, window: &SessionWindow) { + if let Some(report) = self.imp().crash_report.take() { + window.announce_crash(report); + } + } +} diff --git a/linux/crates/app/src/application/imp.rs b/linux/crates/app/src/application/imp.rs index 03efb25f1b..bdcb53208b 100644 --- a/linux/crates/app/src/application/imp.rs +++ b/linux/crates/app/src/application/imp.rs @@ -33,6 +33,8 @@ pub struct TableProApplication { /// Files handed over while the record is read, opened after the /// windows it restores. pub(super) pending_files: RefCell>, + /// A crash report found before any window opened. + pub(super) crash_report: RefCell>, } #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] @@ -82,6 +84,7 @@ impl ApplicationImpl for TableProApplication { install_application_accelerators(&*app); if let Some(services) = self.services.get() { services.start(); + app.look_for_crash_report(); self.session_signals .replace(Some(SessionSignals::start(&services.sessions))); } diff --git a/linux/crates/app/src/application/mod.rs b/linux/crates/app/src/application/mod.rs index 79cd75c8d5..70e9f21ddc 100644 --- a/linux/crates/app/src/application/mod.rs +++ b/linux/crates/app/src/application/mod.rs @@ -8,6 +8,7 @@ mod accelerators; mod app_actions; mod app_services; mod app_window_host; +mod crash_notice; mod debug_info; mod imp; mod legal_section; @@ -101,6 +102,7 @@ impl TableProApplication { let host: Rc = Rc::new(AppWindowHost::new(self)); self.imp().windows.insert(window.id(), window.clone()); window.start(SessionWindowInit { services, host, launch }); + self.hand_over_crash_report(&window); window.present(); Some(window.upcast()) } diff --git a/linux/crates/app/src/crash_report.rs b/linux/crates/app/src/crash_report.rs new file mode 100644 index 0000000000..733a930480 --- /dev/null +++ b/linux/crates/app/src/crash_report.rs @@ -0,0 +1,186 @@ +//! What a crash leaves behind for the user to send, and how the next +//! launch finds it. + +use std::backtrace::Backtrace; +use std::panic::PanicHookInfo; +use std::path::{Path, PathBuf}; +use std::sync::OnceLock; + +use tablepro_storage::StorageError; + +use crate::config; + +/// GTK calls the app back through C, which a panic cannot unwind, so a +/// panic on the GTK thread ends the process. A panic on a runtime thread +/// is caught there and shown as a failed task, so only these are +/// crashes. +const GTK_THREAD: &str = "main"; + +const REPORT_PREFIX: &str = "crash-"; +const REPORT_SUFFIX: &str = ".txt"; +/// Holds the name of the newest report the user was told about. +const ANNOUNCED: &str = "announced"; +/// A launch that crashes every time leaves one report each, so the +/// oldest go once there are more than this. +const KEPT_REPORTS: usize = 20; + +/// Keep the default hook, which writes the panic to stderr and so to +/// the journal, and also write it with a backtrace to a file of its own +/// in `dir`. A GTK callback that panics panics a second time when it +/// cannot unwind, and that goes in the same file. +pub fn install(dir: PathBuf) { + let report = OnceLock::new(); + let default_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + default_hook(info); + if std::thread::current().name() != Some(GTK_THREAD) { + return; + } + let path = report.get_or_init(|| dir.join(report_name(chrono::Utc::now(), std::process::id()))); + let text = report_text(info, &Backtrace::force_capture(), chrono::Utc::now()); + if let Err(error) = tablepro_storage::fs::append_private_blocking(path, text.as_bytes()) { + tracing::error!(%error, "could not save the crash report"); + } + })); +} + +/// Names sort by time, so the newest report is the last name. +fn report_name(now: chrono::DateTime, pid: u32) -> String { + format!("{REPORT_PREFIX}{}-{pid}{REPORT_SUFFIX}", now.format("%Y%m%dT%H%M%SZ")) +} + +fn report_text(info: &PanicHookInfo<'_>, backtrace: &Backtrace, now: chrono::DateTime) -> String { + let message = info.payload_as_str().unwrap_or("(no message)"); + let location = info + .location() + .map_or_else(|| "an unknown place".to_owned(), ToString::to_string); + let build = if cfg!(debug_assertions) { "debug" } else { "release" }; + format!( + "TablePro {version}\n\ + Application ID: {app_id}\n\ + Profile: {profile}\n\ + Build: {build}\n\ + Time: {time}\n\ + \n\ + Panicked at {location}:\n\ + {message}\n\ + \n\ + Backtrace:\n\ + {backtrace}\n\n", + version = config::VERSION, + app_id = config::APP_ID, + profile = config::PROFILE, + time = now.to_rfc3339_opts(chrono::SecondsFormat::Secs, true), + ) +} + +/// The newest report the user has not been told about, marked as told. +/// Runs at startup, before this run could have written one, and drops +/// the oldest reports past `KEPT_REPORTS`. +pub fn take_unannounced(dir: &Path) -> Result, StorageError> { + let mut reports = match std::fs::read_dir(dir) { + Ok(entries) => entries + .filter_map(Result::ok) + .filter_map(|entry| entry.file_name().into_string().ok()) + .filter(|name| name.starts_with(REPORT_PREFIX) && name.ends_with(REPORT_SUFFIX)) + .collect::>(), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(source) => { + return Err(StorageError::Io { + path: dir.to_owned(), + source, + }); + } + }; + reports.sort_unstable(); + let surplus = reports.len().saturating_sub(KEPT_REPORTS); + for name in reports.drain(..surplus) { + if let Err(error) = std::fs::remove_file(dir.join(&name)) { + tracing::warn!(%error, "could not remove an old crash report"); + } + } + let Some(newest) = reports.pop() else { + return Ok(None); + }; + let marker = dir.join(ANNOUNCED); + let announced = std::fs::read_to_string(&marker).unwrap_or_default(); + if announced.trim() >= newest.as_str() { + return Ok(None); + } + tablepro_storage::fs::write_private_blocking(&marker, newest.as_bytes())?; + Ok(Some(dir.join(newest))) +} + +#[cfg(test)] +mod tests { + use chrono::TimeZone; + + use super::*; + + fn report(dir: &Path, name: &str) { + tablepro_storage::fs::append_private_blocking(&dir.join(name), b"a report").expect("a report"); + } + + #[test] + fn report_names_sort_by_time() { + let earlier = chrono::Utc + .with_ymd_and_hms(2026, 9, 30, 23, 59, 59) + .single() + .expect("a time"); + let later = chrono::Utc + .with_ymd_and_hms(2026, 10, 1, 0, 0, 0) + .single() + .expect("a time"); + + assert_eq!(report_name(later, 42), "crash-20261001T000000Z-42.txt"); + assert!(report_name(earlier, 99_999) < report_name(later, 1)); + } + + #[test] + fn the_newest_report_is_announced_once() { + let dir = tempfile::tempdir().expect("a directory"); + report(dir.path(), "crash-20261001T000000Z-1.txt"); + report(dir.path(), "crash-20261002T000000Z-2.txt"); + + let first = take_unannounced(dir.path()).expect("a listing"); + + assert_eq!(first, Some(dir.path().join("crash-20261002T000000Z-2.txt"))); + assert_eq!(take_unannounced(dir.path()).expect("a listing"), None); + + report(dir.path(), "crash-20261003T000000Z-3.txt"); + assert_eq!( + take_unannounced(dir.path()).expect("a listing"), + Some(dir.path().join("crash-20261003T000000Z-3.txt")) + ); + } + + #[test] + fn no_directory_is_no_report() { + let dir = tempfile::tempdir().expect("a directory"); + + assert_eq!(take_unannounced(&dir.path().join("crashes")).expect("a listing"), None); + } + + #[test] + fn only_the_newest_reports_are_kept() { + let dir = tempfile::tempdir().expect("a directory"); + for day in 1..=KEPT_REPORTS + 3 { + report(dir.path(), &format!("crash-202610{day:02}T000000Z-1.txt")); + } + + take_unannounced(dir.path()).expect("a listing"); + + let left = std::fs::read_dir(dir.path()) + .expect("a listing") + .filter_map(Result::ok) + .filter(|entry| entry.file_name().to_string_lossy().starts_with(REPORT_PREFIX)) + .count(); + assert_eq!(left, KEPT_REPORTS); + assert!(!dir.path().join("crash-20261001T000000Z-1.txt").exists()); + assert!( + dir.path() + .join(format!("crash-202610{:02}T000000Z-1.txt", KEPT_REPORTS + 3)) + .exists() + ); + } +} diff --git a/linux/crates/app/src/lib.rs b/linux/crates/app/src/lib.rs index d229488b68..bd13811331 100644 --- a/linux/crates/app/src/lib.rs +++ b/linux/crates/app/src/lib.rs @@ -8,6 +8,7 @@ use tablepro_storage::{AppSettings, StoragePaths}; pub mod application; pub mod config; +pub mod crash_report; pub mod i18n; pub mod logging; mod persistence; @@ -45,10 +46,12 @@ pub fn run() -> Result { tracing::warn!(%error, "translations unavailable; falling back to the source strings"); } + let paths = StoragePaths::from_glib(config::storage_dir_name(), config::APP_ID); + crash_report::install(paths.crashes_dir()); + let runtime = runtime::AppRuntime::build().map_err(StartupError::Runtime)?; gio::resources_register_include!("tablepro.gresource").map_err(StartupError::Resources)?; let settings = Rc::new(AppSettings::open(config::APP_ID)?); - let paths = StoragePaths::from_glib(config::storage_dir_name(), config::APP_ID); let drivers = build_registry(); tracing::info!(drivers = drivers.len(), "starting tablepro"); diff --git a/linux/crates/app/src/ui/session_window/crash_notice.rs b/linux/crates/app/src/ui/session_window/crash_notice.rs new file mode 100644 index 0000000000..b35ec65431 --- /dev/null +++ b/linux/crates/app/src/ui/session_window/crash_notice.rs @@ -0,0 +1,159 @@ +use std::path::{Path, PathBuf}; + +use gtk4::prelude::*; +use gtk4::subclass::prelude::*; +use gtk4::{gio, glib}; +use libadwaita as adw; + +use super::SessionWindow; +use crate::i18n::gettext; +use crate::ui::file_reveal::reveal_failure_toast; + +/// The report the last run left when it crashed, said until the user or +/// the toast's timeout dismisses it. +pub(crate) struct CrashNotice { + report: PathBuf, + shown: Option<(adw::Toast, glib::SignalHandlerId)>, +} + +impl SessionWindow { + pub(crate) fn announce_crash(&self, report: PathBuf) { + self.withdraw_crash_toast(); + self.imp() + .crash_notice + .replace(Some(CrashNotice { report, shown: None })); + self.show_crash_notice(); + } + + /// A toast lives in the overlay of the page it was added to, so a + /// restored session, which goes from the picker through connecting + /// to its workspace, would take an unread one away. Each page shown + /// gets it again until it is dismissed. + pub(super) fn show_crash_notice(&self) { + self.withdraw_crash_toast(); + let Some(report) = self + .imp() + .crash_notice + .borrow() + .as_ref() + .map(|notice| notice.report.clone()) + else { + return; + }; + let toast = adw::Toast::builder() + .title(gettext("TablePro quit unexpectedly last time")) + .button_label(gettext("Show Report")) + .use_markup(false) + .build(); + let window = self.downgrade(); + toast.connect_button_clicked(move |_| { + if let Some(window) = window.upgrade() { + window.reveal_crash_report(&report); + } + }); + let window = self.downgrade(); + let dismissed = toast.connect_dismissed(move |_| { + if let Some(window) = window.upgrade() { + window.imp().crash_notice.take(); + } + }); + if let Some(notice) = self.imp().crash_notice.borrow_mut().as_mut() { + notice.shown = Some((toast.clone(), dismissed)); + } + self.add_toast(toast); + } + + /// Takes the toast off the page it is on without counting that as + /// the user dismissing it. An overlay that goes away dismisses its + /// toasts, so this runs before the old page can. + fn withdraw_crash_toast(&self) { + let shown = self + .imp() + .crash_notice + .borrow_mut() + .as_mut() + .and_then(|notice| notice.shown.take()); + if let Some((toast, dismissed)) = shown { + toast.disconnect(dismissed); + toast.dismiss(); + } + } + + fn reveal_crash_report(&self, report: &Path) { + let Some(state) = self.imp().state.get() else { + return; + }; + let revealing = state + .services + .file_revealer + .reveal(gio::File::for_path(report), Some(self.clone().upcast())); + let window = self.downgrade(); + glib::spawn_future_local(async move { + let revealed = revealing.await; + if let (Some(toast), Some(window)) = (reveal_failure_toast(&revealed), window.upgrade()) { + window.add_toast(toast); + } + }); + } +} + +#[cfg(test)] +mod tests { + use std::rc::Rc; + + use super::super::SessionWindowInput; + use super::super::tests::{Script, TestWindow, WAIT}; + use super::*; + use crate::test_support::{descendants, drain_main_context, wait_until}; + use crate::ui::file_reveal::RecordingRevealer; + + const TITLE: &str = "TablePro quit unexpectedly last time"; + + fn said(window: &SessionWindow) -> usize { + window + .imp() + .toasts + .borrow() + .iter() + .filter(|toast| toast.title().as_deref() == Some(TITLE)) + .count() + } + + fn show_report(window: &SessionWindow) -> Option { + descendants(window) + .into_iter() + .filter_map(|widget| widget.downcast::().ok()) + .find(|button| button.label().as_deref() == Some("Show Report") && button.is_mapped()) + } + + #[gtk4::test] + fn a_crash_report_is_said_on_each_page_until_the_user_opens_it() { + let revealer = Rc::new(RecordingRevealer::succeeding()); + let test = TestWindow::with_script( + "CrashNotice", + Script { + revealer: Some(revealer.clone()), + ..Script::default() + }, + ); + let report = test.dir().join("crash-20261017T000000Z-1.txt"); + + test.window.announce_crash(report.clone()); + assert_eq!(said(&test.window), 1); + + test.window + .send(SessionWindowInput::OpenSaved(Box::new(test.saved.clone()))); + test.wait_for_page("workspace"); + assert!(said(&test.window) > 1, "the toast stayed on the picker"); + wait_until(WAIT, || show_report(&test.window).is_some()).expect("the toast on the workspace"); + + show_report(&test.window).expect("the button").emit_clicked(); + drain_main_context(); + + assert_eq!(revealer.revealed(), [report]); + assert!(test.window.imp().crash_notice.borrow().is_none()); + let before = said(&test.window); + test.window.show_crash_notice(); + assert_eq!(said(&test.window), before, "a dismissed notice came back"); + } +} diff --git a/linux/crates/app/src/ui/session_window/imp.rs b/linux/crates/app/src/ui/session_window/imp.rs index 32cb4e34b2..391937f241 100644 --- a/linux/crates/app/src/ui/session_window/imp.rs +++ b/linux/crates/app/src/ui/session_window/imp.rs @@ -8,6 +8,7 @@ use libadwaita as adw; use libadwaita::subclass::prelude::*; use tablepro_session::{ConnectAttempts, SessionKey}; +use super::crash_notice::CrashNotice; use super::locate_file::LocateChooser; use super::picker_page::PickerPage; use super::session_page::SessionPage; @@ -56,6 +57,8 @@ pub struct SessionWindow { /// looked for. pub(super) dialog_opening: Cell, pub(super) shut_down: Cell, + /// The crash the last run ended in, until its toast is dismissed. + pub(super) crash_notice: RefCell>, /// Every toast shown, which the overlay does not list. #[cfg(test)] pub(super) toasts: RefCell>, diff --git a/linux/crates/app/src/ui/session_window/mod.rs b/linux/crates/app/src/ui/session_window/mod.rs index 30ae2db2c2..12f9f04570 100644 --- a/linux/crates/app/src/ui/session_window/mod.rs +++ b/linux/crates/app/src/ui/session_window/mod.rs @@ -9,6 +9,7 @@ mod connect_failure_view; mod connect_flow; mod connecting_page; mod connections; +mod crash_notice; mod imp; mod locate_file; mod notification_request; @@ -446,6 +447,7 @@ impl SessionWindow { } let left = imp.page.replace(page); imp.page_toasts.replace(toasts); + self.show_crash_notice(); // A Cancel or Retry left as the default would take Enter on a // page that no longer shows it. self.set_default_widget(None::<>k::Widget>); diff --git a/linux/crates/app/src/ui/session_window/tests.rs b/linux/crates/app/src/ui/session_window/tests.rs index 01fe6c315f..7aa6f62721 100644 --- a/linux/crates/app/src/ui/session_window/tests.rs +++ b/linux/crates/app/src/ui/session_window/tests.rs @@ -53,6 +53,8 @@ pub(crate) struct Script { pub unreadable_list: bool, /// The size the window opens at, as the settings remember it. pub window_size: Option<(i32, i32)>, + /// Stands in for the file manager. + pub revealer: Option>, } /// A TablePro over fakes with one window open on the picker and `saved` @@ -97,6 +99,7 @@ impl TestWindow { tunnel, unreadable_list, window_size, + revealer, } = script; if let Some(size) = window_size { remember_window_size(&settings, size); @@ -133,7 +136,10 @@ impl TestWindow { storage, tasks: runtime.tasks(), }; - let services = scripted_services(parts, hold.clone(), credentials, ssh); + let mut services = scripted_services(parts, hold.clone(), credentials, ssh); + if let Some(revealer) = revealer { + services.file_revealer = revealer; + } let (app, window) = open_picker_window(name, services); let services = app.services().expect("the application's services"); let test = Self { diff --git a/linux/crates/app/tests/crash_report.rs b/linux/crates/app/tests/crash_report.rs new file mode 100644 index 0000000000..f2731e54a0 --- /dev/null +++ b/linux/crates/app/tests/crash_report.rs @@ -0,0 +1,144 @@ +//! A panic in a GTK callback cannot unwind through C, so it ends the +//! process. The hook has to leave a report behind that the next launch +//! finds, and the default output on stderr has to stay. +//! +//! A panic hook is process-wide and the crash ends the process, so each +//! case runs in a child of its own. + +#![expect( + clippy::print_stderr, + reason = "a harness-free test binary reports its own failures on stderr" +)] + +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::{Command, ExitCode, Output, Stdio}; + +const CHILD_DIR: &str = "TABLEPRO_CRASH_REPORT_DIR"; +const CHILD_OFF_THREAD: &str = "TABLEPRO_CRASH_REPORT_OFF_THREAD"; +const PROBE: &str = "the crash report probe"; + +fn main() -> ExitCode { + match std::env::var_os(CHILD_DIR) { + Some(dir) => child(PathBuf::from(dir)), + None => parent(), + } +} + +#[expect(clippy::panic, reason = "the panic is what the case is about")] +fn child(dir: PathBuf) -> ExitCode { + tablepro_app::crash_report::install(dir); + if std::env::var_os(CHILD_OFF_THREAD).is_some() { + // A runtime thread's panic is caught where it happens and ends + // a task, not the app. + let worker = std::thread::Builder::new() + .name("tokio-runtime-worker".to_owned()) + .spawn(|| panic!("{PROBE}")); + if let Ok(worker) = worker { + let _ = worker.join(); + } + return ExitCode::SUCCESS; + } + glib::idle_add_local_once(|| panic!("{PROBE}")); + glib::MainLoop::new(None, false).run(); + ExitCode::SUCCESS +} + +fn parent() -> ExitCode { + let Ok(root) = tempfile::tempdir() else { + eprintln!("no temporary directory"); + return ExitCode::FAILURE; + }; + let crashes = root.path().join("crashes"); + match crash_leaves_one_private_report(root.path(), &crashes) { + Ok(()) => ExitCode::SUCCESS, + Err(message) => { + eprintln!("{message}"); + ExitCode::FAILURE + } + } +} + +fn crash_leaves_one_private_report(root: &Path, crashes: &Path) -> Result<(), String> { + let caught = run_child(root, crashes, true)?; + if !caught.status.success() || crashes.exists() { + return Err(format!( + "a panic off the GTK thread was taken for a crash ({}):\n{}", + caught.status, + String::from_utf8_lossy(&caught.stderr) + )); + } + + let crashed = run_child(root, crashes, false)?; + let stderr = String::from_utf8_lossy(&crashed.stderr); + if crashed.status.success() { + return Err(format!("a panic in a GTK callback did not end the process:\n{stderr}")); + } + if !stderr.contains(PROBE) { + return Err(format!( + "the default hook no longer writes the panic to stderr:\n{stderr}" + )); + } + + let reports = reports_in(crashes)?; + let [report] = reports.as_slice() else { + return Err(format!("expected one report, found {reports:?}")); + }; + let mode = |path: &Path| std::fs::metadata(path).map(|metadata| metadata.permissions().mode() & 0o777); + if mode(crashes).ok() != Some(0o700) || mode(report).ok() != Some(0o600) { + return Err(format!( + "the report is not private: directory {:?}, file {:?}", + mode(crashes).ok(), + mode(report).ok() + )); + } + let text = std::fs::read_to_string(report).map_err(|error| format!("could not read the report: {error}"))?; + for needle in [ + format!("TablePro {}", tablepro_app::config::VERSION), + format!("Profile: {}", tablepro_app::config::PROFILE), + PROBE.to_owned(), + "Backtrace:\n".to_owned(), + ] { + if !text.contains(&needle) { + return Err(format!("the report is missing {needle:?}:\n{text}")); + } + } + + let announced = tablepro_app::crash_report::take_unannounced(crashes).map_err(|error| error.to_string())?; + if announced.as_ref() != Some(report) { + return Err(format!("the next launch found {announced:?}, not {}", report.display())); + } + let again = tablepro_app::crash_report::take_unannounced(crashes).map_err(|error| error.to_string())?; + if again.is_some() { + return Err("the same crash was announced twice".to_owned()); + } + Ok(()) +} + +fn run_child(root: &Path, crashes: &Path, off_thread: bool) -> Result { + let mut command = Command::new(std::env::args_os().next().unwrap_or_default()); + command + .env(CHILD_DIR, crashes) + .env_remove("RUST_BACKTRACE") + // An abort can leave a core file in the working directory. + .current_dir(root) + .stdin(Stdio::null()); + if off_thread { + command.env(CHILD_OFF_THREAD, "1"); + } + command + .output() + .map_err(|error| format!("could not run the child: {error}")) +} + +fn reports_in(dir: &Path) -> Result, String> { + let entries = std::fs::read_dir(dir).map_err(|error| format!("no crash directory: {error}"))?; + Ok(entries + .filter_map(Result::ok) + .map(|entry| entry.path()) + .filter(|path| { + path.file_name() + .is_some_and(|name| name.to_string_lossy().starts_with("crash-")) + }) + .collect()) +} diff --git a/linux/crates/storage/src/fs.rs b/linux/crates/storage/src/fs.rs index 869cfde71f..b9edaa5cc4 100644 --- a/linux/crates/storage/src/fs.rs +++ b/linux/crates/storage/src/fs.rs @@ -1,5 +1,5 @@ use std::fs::{DirBuilder, OpenOptions, Permissions}; -use std::io::ErrorKind; +use std::io::{ErrorKind, Write}; use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt, PermissionsExt}; use std::path::Path; @@ -89,6 +89,28 @@ pub fn create_private_file_if_missing(path: &Path) -> Result<(), StorageError> { } } +/// Add bytes to the end of a file, creating it 0600 under a 0700 +/// directory when it is absent. Unlike `write_private_blocking` it never +/// replaces what is there, so a second write in the same moment adds to +/// the first. +pub fn append_private_blocking(path: &Path, bytes: &[u8]) -> Result<(), StorageError> { + if let Some(parent) = path.parent() { + ensure_private_dir(parent)?; + } + let io_error = |source| StorageError::Io { + path: path.to_owned(), + source, + }; + let mut file = OpenOptions::new() + .append(true) + .create(true) + .mode(PRIVATE_FILE) + .open(path) + .map_err(io_error)?; + file.write_all(bytes).map_err(io_error)?; + file.sync_all().map_err(io_error) +} + /// Rename a file out of the way, returning the name it now has. /// /// The caller shows that name, so the user can find the file again. The @@ -353,6 +375,19 @@ mod tests { ); } + #[test] + fn append_private_creates_0600_under_0700_and_adds_to_the_end() { + let root = tempfile::tempdir().expect("tempdir"); + let file = root.path().join("crashes").join("crash.txt"); + + append_private_blocking(&file, b"first\n").expect("append"); + append_private_blocking(&file, b"second\n").expect("append"); + + assert_eq!(mode_of(&file), 0o600); + assert_eq!(mode_of(file.parent().expect("parent")), 0o700); + assert_eq!(std::fs::read(&file).expect("read"), b"first\nsecond\n"); + } + #[test] fn create_private_file_if_missing_keeps_existing_contents() { let root = tempfile::tempdir().expect("tempdir"); diff --git a/linux/crates/storage/src/paths.rs b/linux/crates/storage/src/paths.rs index 09ab19737f..31b11ec7c0 100644 --- a/linux/crates/storage/src/paths.rs +++ b/linux/crates/storage/src/paths.rs @@ -61,6 +61,10 @@ impl StoragePaths { pub fn drafts_dir(&self) -> PathBuf { self.data.join("drafts") } + + pub fn crashes_dir(&self) -> PathBuf { + self.state.join("crashes") + } } #[cfg(test)] diff --git a/linux/docs/error-handling.md b/linux/docs/error-handling.md index 7514ffb740..d7534eaf13 100644 --- a/linux/docs/error-handling.md +++ b/linux/docs/error-handling.md @@ -128,7 +128,7 @@ The buttons are `TlsRemedy` in `crates/app/src/ui/error_text/tls_remedy.rs`: the ## Logging -Logging goes through `tracing`. `crates/app/src/logging.rs` installs a `tracing_subscriber` fmt layer that writes to **stderr**, which the GNOME session journals for both Flatpak and system installs. Panics land in the same stream, so a panic and the lines around it stay in order. There is no journald writer and no panic hook. +Logging goes through `tracing`. `crates/app/src/logging.rs` installs a `tracing_subscriber` fmt layer that writes to **stderr**, which the GNOME session journals for both Flatpak and system installs. Panics land in the same stream, so a panic and the lines around it stay in order. There is no journald writer. - The filter comes from `RUST_LOG`. Without it, a development build logs at `debug` and an installed one at `info`. A `RUST_LOG` that does not parse falls back to that default and logs a warning saying so. - `TABLEPRO_LOG_FORMAT=json` writes one JSON object per line; `text`, the default, writes plain lines. @@ -145,6 +145,16 @@ Never log a password, a secret, a key or a bound value. sqlx's own statement log The libraries under the drivers log what a database was sent or answered: tokio-postgres the text of every statement at `debug`, through `log`, which `tracing-subscriber` forwards; sqlx every statement, and a slow one as a warning; tiberius rows at `trace` and server messages as errors; the clickhouse crate exceptions that quote the query; deadpool the errors of the connections it recycles; sqlparser, also through `log`, the text of every statement, filter and default it parses, and the literals in it, at `debug`. So the subscriber drops every event whose target starts with `tokio_postgres`, `deadpool`, `tiberius`, `sqlx`, `clickhouse` or `sqlparser`, at every level. That filter sits beside the `RUST_LOG` one, so no directive turns them back on, however specific. A driver logs what it needs from their errors itself. A new driver's client library, or any other library that logs SQL text, joins `SILENCED_LIBRARIES` in `logging.rs`. +## Crashes + +A panic on the GTK thread ends the process: GTK calls the app back through C, which a panic cannot unwind, so it aborts. A panic on a runtime thread is caught there (`session/src/runtime/catch_panic.rs`, and tokio's own `JoinError`) and ends one task, which the UI shows as a failure. + +- `crates/app/src/crash_report.rs` installs a panic hook in `run()`, right after logging. It runs the default hook first, so stderr and the journal keep the usual line. +- For a panic on the GTK thread it then appends a report to `StoragePaths::crashes_dir()` (`$XDG_STATE_HOME//crashes`): the version, application ID, profile, debug or release build, the time, the message and place, and a backtrace captured whatever `RUST_BACKTRACE` says. The file is 0600 in a 0700 directory, written through `tablepro_storage::fs::append_private_blocking`. One file per process, named `crash--.txt`; the second panic Rust raises when the first cannot unwind goes in the same file. +- At the next launch the primary instance looks for the newest report it has not announced, off the GTK thread, and records its name in `crashes/announced`. The first window shows a toast, "TablePro quit unexpectedly last time", whose Show Report button opens the folder with the report selected. The toast moves with the window from page to page until it is dismissed or times out. Only the 20 newest reports are kept. +- Nothing leaves the machine: the user decides whether to attach the report to an issue. +- Still open: pending grid and Structure edits live only in memory and are lost with the process. Editor text and the tab list are saved 500 ms after a change. + ## `unwrap`, `expect` and `panic!` The workspace lints deny `unwrap_used`, `expect_used`, `panic`, `todo` and `unimplemented`. `clippy.toml` allows `unwrap`, `expect` and `panic!` in tests; `todo!` and `unimplemented!` are denied there too. Return a real error instead. The one accepted use outside tests is in a `const`, where the compiler proves the value, as `RowLimit::EDITOR_DEFAULT` does. diff --git a/linux/docs/storage.md b/linux/docs/storage.md index 1ac3863914..62605d1b73 100644 --- a/linux/docs/storage.md +++ b/linux/docs/storage.md @@ -116,6 +116,7 @@ nothing reaches outside it. | `history_database()` | `$XDG_STATE_HOME//history.db` | | `workspaces_dir()` | `$XDG_STATE_HOME//workspaces`: one file per session, and `open-sessions.json` | | `drafts_dir()` | `$XDG_DATA_HOME//drafts` | +| `crashes_dir()` | `$XDG_STATE_HOME//crashes`: crash reports ([error-handling.md](error-handling.md#crashes)) | `` is `tablepro` for an installed build and `tablepro-devel` for a development one, so `cargo run` never writes over an installed build's @@ -134,6 +135,8 @@ Everything private goes through `tablepro_storage::fs`: a mix, and the new one survives a power cut. It tightens an existing file to 0600 first, because that call keeps an existing file's mode and only applies the mode argument when it creates the file. +- `append_private_blocking` appends to a file it creates 0600 when absent, + for the crash report, which a second panic adds to. - `create_private_file_if_missing` pre-creates an empty 0600 file. SQLite creates its database, WAL and SHM with the umask mode, so pre-creating the database is what keeps all three off world-readable. diff --git a/linux/po/POTFILES.in b/linux/po/POTFILES.in index 66ad367755..1316fc369b 100644 --- a/linux/po/POTFILES.in +++ b/linux/po/POTFILES.in @@ -5,6 +5,7 @@ crates/app/src/application/accelerators.rs crates/app/src/application/app_actions.rs crates/app/src/application/app_services.rs crates/app/src/application/app_window_host.rs +crates/app/src/application/crash_notice.rs crates/app/src/application/debug_info.rs crates/app/src/application/imp.rs crates/app/src/application/legal_section.rs @@ -23,6 +24,7 @@ crates/app/src/application/window_host.rs crates/app/src/application/window_launch.rs crates/app/src/application/window_registry.rs crates/app/src/config.rs +crates/app/src/crash_report.rs crates/app/src/i18n.rs crates/app/src/lib.rs crates/app/src/logging.rs @@ -357,6 +359,7 @@ crates/app/src/ui/session_window/connect_failure_view.rs crates/app/src/ui/session_window/connect_flow.rs crates/app/src/ui/session_window/connecting_page.rs crates/app/src/ui/session_window/connections.rs +crates/app/src/ui/session_window/crash_notice.rs crates/app/src/ui/session_window/imp.rs crates/app/src/ui/session_window/locate_file.rs crates/app/src/ui/session_window/mod.rs From 24cd6429762064e19010e346e0d22ccb225b34d1 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 15:47:59 +0700 Subject: [PATCH 5/7] fix(linux-editor): keep at most 100,000 rows across one run's results --- linux/crates/app/src/ui/editor/mod.rs | 3 +- linux/crates/app/src/ui/editor/run_log.rs | 96 ++++++++++++++--- .../crates/app/src/ui/editor/script_runner.rs | 102 +++++++++++++++++- .../app/src/ui/result_pane/result_summary.rs | 41 ++++++- linux/docs/state-management.md | 4 +- 5 files changed, 225 insertions(+), 21 deletions(-) diff --git a/linux/crates/app/src/ui/editor/mod.rs b/linux/crates/app/src/ui/editor/mod.rs index f427c7721a..4eca35f97c 100644 --- a/linux/crates/app/src/ui/editor/mod.rs +++ b/linux/crates/app/src/ui/editor/mod.rs @@ -46,7 +46,7 @@ use self::imp::RunningScript; use self::result_widgets::PageActions; use self::run_log::{EntryKind, RunEvent, RunLog}; use self::run_scope::ScopedText; -use self::script_runner::{ScriptRun, plan_batches, run_in_tab}; +use self::script_runner::{RUN_ROW_LIMIT, ScriptRun, plan_batches, run_in_tab}; use super::clipboard::{CopyRequest, InClauseRows}; use super::export::{ExportOffer, ExportOrigin, ExportQuery, ExportSource}; use super::grid::{GridMsg, GridSink, present_row_json}; @@ -383,6 +383,7 @@ impl SqlEditor { policy, statement_timeout: deps.settings.query_timeout(), row_limit: RowLimit::EDITOR_DEFAULT, + run_row_limit: RUN_ROW_LIMIT, }; let metadata = session.metadata().clone(); // Read again at the end from the run's own session, never the diff --git a/linux/crates/app/src/ui/editor/run_log.rs b/linux/crates/app/src/ui/editor/run_log.rs index 861aeeb35d..1cceb46635 100644 --- a/linux/crates/app/src/ui/editor/run_log.rs +++ b/linux/crates/app/src/ui/editor/run_log.rs @@ -94,6 +94,25 @@ pub enum RunEvent { Finished, } +/// The rows a batch may still keep under its run's limit. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct RowRoom { + /// The run's limit, which a result it cuts short names. + pub run_limit: u64, + pub left: u64, + /// The statement was sent with `left` as its row limit, below its + /// own, so a result the driver cut short was cut by the run's limit. + pub limits_statement: bool, +} + +impl RowRoom { + pub const UNLIMITED: Self = Self { + run_limit: u64::MAX, + left: u64::MAX, + limits_statement: false, + }; +} + /// Turns the events of one run into results list lines, on the runtime, /// so only finished lines cross to the GTK thread. #[derive(Debug)] @@ -103,6 +122,7 @@ pub struct BatchCollector { entries: Vec, /// Where each result set of this run sits in `entries`. sets: Vec, + room: RowRoom, } impl BatchCollector { @@ -111,9 +131,15 @@ impl BatchCollector { statement, entries: Vec::new(), sets: Vec::new(), + room: RowRoom::UNLIMITED, } } + /// Rows past `room` are dropped, and the result says why. + pub fn within(self, room: RowRoom) -> Self { + Self { room, ..self } + } + pub fn record(&mut self, event: ResultEvent) { match event { ResultEvent::ResultSetStarted { columns } => { @@ -128,36 +154,46 @@ impl BatchCollector { result.columns = columns; } } - ResultEvent::Rows(rows) => match self.last_set() { - Some(result) => { - result.rows.extend(rows.into_iter().map(Into::into)); + ResultEvent::Rows(rows) => { + if self.sets.is_empty() { + // Rows with no columns would draw a grid that cannot + // say what any cell is. + tracing::warn!(rows = rows.len(), "rows arrived before their result set"); + return; + } + let kept = usize::try_from(self.room.left).unwrap_or(usize::MAX).min(rows.len()); + self.room.left -= kept as u64; + let cut_by = (kept < rows.len()).then_some(self.room.run_limit); + if let Some(result) = self.last_set() { + result.rows.extend(rows.into_iter().take(kept).map(Into::into)); result.summary.retained = result.rows.len() as u64; + result.summary.run_limit = result.summary.run_limit.or(cut_by); } - // Rows with no columns would draw a grid that cannot say - // what any cell is. - None => tracing::warn!(rows = rows.len(), "rows arrived before their result set"), - }, + } // Until the set ends and says how far the read went, all // that is known is that more rows were left behind. - ResultEvent::RowLimitReached { retained } => { + ResultEvent::RowLimitReached { .. } => { + let cut_by = self.room.limits_statement.then_some(self.room.run_limit); if let Some(result) = self.last_set() { result.summary = ResultSummary { - retained, + retained: result.rows.len() as u64, total_rows: None, truncation: Truncation::ServerStopped, + run_limit: result.summary.run_limit.or(cut_by), }; } } ResultEvent::ResultSetFinished { - retained, - total_rows, - truncation, + total_rows, truncation, .. } => { + let cut_by = (self.room.limits_statement && truncation == Truncation::ReadPastLimit) + .then_some(self.room.run_limit); if let Some(result) = self.last_set() { result.summary = ResultSummary { - retained, + retained: result.rows.len() as u64, total_rows, truncation, + run_limit: result.summary.run_limit.or(cut_by), }; } } @@ -514,6 +550,7 @@ mod tests { retained: 2, total_rows: Some(5), truncation: Truncation::ServerStopped, + run_limit: None, } ); assert_eq!(result.statement.as_str(), "SELECT a FROM t"); @@ -521,6 +558,37 @@ mod tests { assert!(matches!(entries[2], EntryKind::Command { rows_affected: Some(4) })); } + #[test] + fn a_set_the_driver_cut_at_the_runs_room_names_the_run_limit() { + let room = RowRoom { + run_limit: 100, + left: 1, + limits_statement: true, + }; + let mut collector = BatchCollector::new(UserSql::new("SELECT a FROM t")).within(room); + for event in [ + ResultEvent::ResultSetStarted { + columns: columns(&["a"]), + }, + ResultEvent::Rows(vec![vec![Value::Int(1)]]), + ResultEvent::RowLimitReached { retained: 1 }, + ResultEvent::ResultSetFinished { + retained: 1, + total_rows: None, + truncation: Truncation::ReadPastLimit, + }, + ] { + collector.record(event); + } + let (entries, _, _) = collector.finish(summary(RunEnd::Completed), EditorSessionState::Ready, 0); + + let EntryKind::ResultSet(result) = &entries[0] else { + panic!("expected rows: {entries:?}"); + }; + assert_eq!(result.row_count(), 1); + assert_eq!(result.summary.run_limit, Some(100)); + } + #[test] fn a_set_cut_by_the_limit_and_then_stopped_stays_truncated() { let (entries, _) = collect( @@ -552,11 +620,13 @@ mod tests { retained: 100, total_rows: None, truncation: Truncation::ServerStopped, + run_limit: None, }; let counted = ResultSummary { retained: 100, total_rows: Some(250), truncation: Truncation::ReadPastLimit, + run_limit: None, }; let history = |entries: Vec| { let mut log = RunLog::new(vec![UserSql::new("SELECT a FROM t")], Vec::new()); diff --git a/linux/crates/app/src/ui/editor/script_runner.rs b/linux/crates/app/src/ui/editor/script_runner.rs index 06d6d1beaf..4a57b1b79b 100644 --- a/linux/crates/app/src/ui/editor/script_runner.rs +++ b/linux/crates/app/src/ui/editor/script_runner.rs @@ -14,7 +14,7 @@ use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; use super::editor_session_slot::{AcquireError, EditorSessionSlot, close_quietly}; -use super::run_log::{BatchCollector, BatchReport, EntryKind, RunEvent, Verdict, continues}; +use super::run_log::{BatchCollector, BatchReport, EntryKind, RowRoom, RunEvent, Verdict, continues}; use super::run_scope::ScopedText; /// One unit the server is sent: a statement on most engines, a whole @@ -28,6 +28,11 @@ pub struct RunBatch { pub error_base: usize, } +/// Rows one run keeps across all its results. `row_limit` bounds each +/// result, and a script of many SELECTs would otherwise hold all of them +/// until the next run. +pub const RUN_ROW_LIMIT: u64 = 100_000; + /// What one press of Run sends, and how. #[derive(Debug, Clone)] pub struct ScriptRun { @@ -35,6 +40,21 @@ pub struct ScriptRun { pub policy: BatchErrorPolicy, pub statement_timeout: Option, pub row_limit: RowLimit, + pub run_row_limit: u64, +} + +/// What a statement may keep once the run has kept `kept` rows, and the +/// row limit it goes out with. With no room left it still runs, for +/// what it changes, and reads one row, which is dropped. +fn room_after(run: &ScriptRun, kept: u64) -> (RowRoom, RowLimit) { + let left = run.run_row_limit.saturating_sub(kept); + let own = run.row_limit.get(); + let room = RowRoom { + run_limit: run.run_row_limit, + left, + limits_statement: left < own, + }; + (room, RowLimit::new(left.clamp(1, own)).unwrap_or(run.row_limit)) } /// The batches of the scoped text and what the plan warns about. @@ -94,16 +114,18 @@ pub async fn run_batches( cancel: &CancellationToken, events: &mpsc::Sender, ) { + let mut kept: u64 = 0; 'batches: for (index, batch) in run.batches.iter().enumerate() { for _ in 0..batch.repeat.max(1) { if cancel.is_cancelled() { break 'batches; } let sql = UserSql::new(batch.text.as_str()); - let collector = Arc::new(Mutex::new(BatchCollector::new(sql.clone()))); + let (room, row_limit) = room_after(run, kept); + let collector = Arc::new(Mutex::new(BatchCollector::new(sql.clone()).within(room))); let options = RunOptions::new(cancel.child_token()) .with_deadline(run.statement_timeout) - .with_row_limit(run.row_limit); + .with_row_limit(row_limit); let sink = Box::new(CollectingSink { collector: collector.clone(), }); @@ -113,6 +135,13 @@ pub async fn run_batches( let collected = std::mem::replace(&mut *lock(&collector), BatchCollector::new(sql)); let state = editor.state(); let (entries, verdict, transaction) = collected.finish(outcome, state, batch.error_base); + kept += entries + .iter() + .map(|entry| match entry { + EntryKind::ResultSet(result) => result.row_count() as u64, + _ => 0, + }) + .sum::(); let report = BatchReport { batch: index, entries, @@ -226,6 +255,7 @@ mod tests { policy, statement_timeout: None, row_limit: RowLimit::EDITOR_DEFAULT, + run_row_limit: RUN_ROW_LIMIT, }, statements, warnings, @@ -250,6 +280,16 @@ mod tests { cancel: &CancellationToken, ) -> RunLog { let (run, statements, warnings) = script(text, grammar); + play(editor, &run, statements, warnings, cancel).await + } + + async fn play( + editor: &Arc, + run: &ScriptRun, + statements: Vec, + warnings: Vec, + cancel: &CancellationToken, + ) -> RunLog { let (events, mut received) = mpsc::channel(8); let session: Arc = editor.clone(); let collect = async move { @@ -263,7 +303,7 @@ mod tests { } log }; - let (_, log) = tokio::join!(run_batches(&session, &run, cancel, &events), collect); + let (_, log) = tokio::join!(run_batches(&session, run, cancel, &events), collect); log } @@ -444,6 +484,60 @@ mod tests { assert!(log.lost()); } + fn sixty_rows() -> FakeRun { + let columns: Arc<[tablepro_core::column::ResultColumn]> = vec![tablepro_core::column::ResultColumn::new( + "n", + tablepro_core::column::ColumnType::untyped_text("int4"), + )] + .into(); + FakeRun::completed(vec![ + ResultEvent::ResultSetStarted { columns }, + ResultEvent::Rows((0..60).map(|n| vec![tablepro_core::Value::Int(n)]).collect()), + ResultEvent::ResultSetFinished { + retained: 60, + total_rows: None, + truncation: tablepro_core::Truncation::Complete, + }, + ]) + } + + #[tokio::test] + async fn a_run_keeps_rows_up_to_its_limit_across_all_results() { + let mut runs = Replies::default(); + runs.always(sixty_rows()); + let editor = Arc::new(FakeEditorSession::new(runs)); + let (mut run, statements, warnings) = script("SELECT 1; SELECT 2; SELECT 3", SqlGrammar::PostgreSql); + run.run_row_limit = 100; + + let log = play(&editor, &run, statements, warnings, &CancellationToken::new()).await; + + assert_eq!(texts(&editor).len(), 3, "a statement past the limit was not sent"); + let kept: Vec<(usize, Option)> = log + .entries() + .iter() + .filter_map(|entry| match &entry.kind { + EntryKind::ResultSet(result) => Some((result.row_count(), result.summary.run_limit)), + _ => None, + }) + .collect(); + assert_eq!(kept, [(60, None), (40, Some(100)), (0, Some(100))]); + } + + #[test] + fn a_statement_reads_only_the_rows_its_run_has_room_for() { + let (mut run, _, _) = script("SELECT 1", SqlGrammar::PostgreSql); + run.run_row_limit = 100_000; + let limit = |kept| { + let (room, limit) = room_after(&run, kept); + (room.left, room.limits_statement, limit.get()) + }; + + assert_eq!(limit(0), (100_000, false, 10_000)); + assert_eq!(limit(95_000), (5_000, true, 5_000)); + assert_eq!(limit(100_000), (0, true, 1)); + assert_eq!(limit(250_000), (0, true, 1)); + } + #[test] fn batches_know_where_they_start_in_the_buffer() { let scoped = ScopedText { diff --git a/linux/crates/app/src/ui/result_pane/result_summary.rs b/linux/crates/app/src/ui/result_pane/result_summary.rs index baa8bc8474..af01ec7ab8 100644 --- a/linux/crates/app/src/ui/result_pane/result_summary.rs +++ b/linux/crates/app/src/ui/result_pane/result_summary.rs @@ -12,6 +12,9 @@ pub struct ResultSummary { /// What the server says the whole set held, when it says. pub total_rows: Option, pub truncation: Truncation, + /// The run's limit on the rows of all its results, when that limit + /// cut this one short. + pub run_limit: Option, } impl ResultSummary { @@ -21,11 +24,12 @@ impl ResultSummary { retained, total_rows: None, truncation: Truncation::Complete, + run_limit: None, } } pub(crate) fn is_truncated(&self) -> bool { - self.truncation != Truncation::Complete + self.truncation != Truncation::Complete || self.run_limit.is_some() } /// What the banner over a cut-short result says. `None` for a @@ -36,6 +40,21 @@ impl ResultSummary { fn limit_title_with(&self, format: impl Fn(u64) -> String) -> Option { let shown = format(self.retained); + if let Some(limit) = self.run_limit { + let limit = format(limit); + return Some(match self.retained { + 0 => gettext_f( + "No rows kept; a run keeps at most {limit} rows across its results", + &[("limit", &limit)], + ), + retained => ngettext_f( + "Showing the first {shown} row; a run keeps at most {limit} rows across its results", + "Showing the first {shown} rows; a run keeps at most {limit} rows across its results", + plural_count(retained), + &[("shown", &shown), ("limit", &limit)], + ), + }); + } match (self.truncation, self.total_rows) { (Truncation::Complete, _) => None, (Truncation::ReadPastLimit, Some(total)) => Some(ngettext_f( @@ -97,6 +116,7 @@ mod tests { retained, total_rows, truncation, + run_limit: None, } } @@ -125,6 +145,25 @@ mod tests { ); } + #[test] + fn limit_title_names_the_run_limit_when_it_cut_the_result() { + let cut = |retained| ResultSummary { + run_limit: Some(100_000), + ..summary(retained, None, Truncation::Complete) + }; + + assert_eq!( + cut(2_500).limit_title_with(english).as_deref(), + Some("Showing the first 2,500 rows; a run keeps at most 100,000 rows across its results") + ); + assert_eq!( + cut(0).limit_title_with(english).as_deref(), + Some("No rows kept; a run keeps at most 100,000 rows across its results") + ); + assert_eq!(cut(0).tab_label_with(4, english), "Result 5 (0+)"); + assert_eq!(cut(0).history_row_count(), None); + } + #[test] fn limit_title_none_when_complete() { assert_eq!( diff --git a/linux/docs/state-management.md b/linux/docs/state-management.md index cfda3a4037..267bb8f086 100644 --- a/linux/docs/state-management.md +++ b/linux/docs/state-management.md @@ -98,7 +98,7 @@ After a run that left its session `Lost`, the runner lets it go and asks the win 1. **Scope.** Run sends the selection when there is one and the whole buffer otherwise (`run_scope::for_run`). Run at Cursor selects the statement under the cursor and sends that. `ScopedText` keeps where the text starts in the buffer, in characters, so a server's error position becomes a buffer position. 2. **Plan.** `plan_batches` reads the text with `ScriptPlan` and the grammar of the window session's dialect, or PostgreSQL's when there is none. Each `RunBatch` is one statement on PostgreSQL, MySQL, SQLite and ClickHouse, and one `GO` batch on SQL Server, which `GO ` repeats. psql, mysql and sqlite3 send a script the same way: no multi-statement text reaches the server, so an implicit transaction never rolls back statements that already reported success. On SQL Server the unit is the `GO` batch, as in sqlcmd: a batch reaches the server whole, whatever `;` it holds. The plan is built again at every run, never kept. 3. **Warnings.** A string, quoted name, comment or dollar quote that never closes adds a warning line naming its line. The text is still sent, so the server has the last word. -4. **Runs.** `run_batches` sends each batch as one `EditorSession::run`, with the statement timeout from Preferences, the editor's row limit and a child of the run's cancel token. A run that gets `EditorSessionBusy`, because the last one is still settling, is shown as failed and not sent again. +4. **Runs.** `run_batches` sends each batch as one `EditorSession::run`, with the statement timeout from Preferences, the editor's row limit and a child of the run's cancel token. A run keeps at most `RUN_ROW_LIMIT` (100,000) rows across all its results: once it has, a statement still runs but reads one row, which is dropped, and its result says the run's limit cut it. A run that gets `EditorSessionBusy`, because the last one is still settling, is shown as failed and not sent again. 5. **Stopping.** A failed statement stops the script, since the ones after it were written assuming it worked. SQL Server batches run on their own, as in sqlcmd, so a failed `GO` batch goes on to the next (`BatchErrorPolicy::ContinueNextBatch`). A cancel, a timeout or a session error stops the script on every engine. Each batch never sent gets a Not Run line. The editor plans with the grammar's defaults (`LexicalSettings::default_for`): backslash escapes on for MySQL and ClickHouse, off for the rest. It does not read the session's own settings. The PostgreSQL driver plans what each run sends again with the session's `standard_conforming_strings`, which it reads from the server's parameter status, and plans the rest of that text again after a statement that changes it. @@ -112,7 +112,7 @@ Events cross to the GTK thread through a channel. `BatchCollector` turns one bat - One line needs no list. The list and its toggle appear with the second line, and the list opens by itself while the pane is wide. Below 560sp it folds over the result, and opening it is left to the toggle (`sidebar_policy`). - A page is built when its line is first shown, and the 8 shown last keep theirs, so a script of thousands of statements builds a handful. - When the run ends, the first failure or timeout is selected, or else the first line. A failure's page offers Go to Error when the server named a position. -- A result set's page is a read-only grid of the rows the run kept, under a `ResultLimitBanner` when the row limit cut it short. +- A result set's page is a read-only grid of the rows the run kept, under a `ResultLimitBanner` when the row limit, or the run's, cut it short. A finished run goes to `HistoryService::record` with the text that was run, `DELIMITER` and `GO` lines included, judged by the dialect of the session that ran it, not the window's current one ([storage.md](storage.md#what-goes-into-the-history)). From 87f3fd1ea64d188b3bfd6a581dab1e15dcd43551 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 15:48:00 +0700 Subject: [PATCH 6/7] fix(linux): name the Enter Password action only with its target --- .../ui/session_window/connect_failed_page.rs | 34 +++++++++++++++++++ .../data/resources/ui/connect-failed-page.ui | 5 +-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/linux/crates/app/src/ui/session_window/connect_failed_page.rs b/linux/crates/app/src/ui/session_window/connect_failed_page.rs index a3653a9d66..58fd91e327 100644 --- a/linux/crates/app/src/ui/session_window/connect_failed_page.rs +++ b/linux/crates/app/src/ui/session_window/connect_failed_page.rs @@ -83,8 +83,11 @@ impl ConnectFailedPage { imp.edit_button.set_visible(view.can_edit); if let Some(kind) = reenter { imp.reenter_button.set_label(&super::secret_retry::reenter_label(kind)); + // The target goes first: an action that takes one, named on a + // button without it, warns that it cannot be activated. imp.reenter_button .set_action_target_value(Some(&kind.attribute().to_variant())); + imp.reenter_button.set_action_name(Some("win.retry-with-new-secret")); imp.reenter_button.set_visible(true); imp.retry_button.remove_css_class("suggested-action"); } @@ -159,3 +162,34 @@ impl ConnectFailedPage { self.imp().announced.borrow().clone() } } + +#[cfg(test)] +mod tests { + use super::*; + + fn page(reenter: Option) -> ConnectFailedPage { + adw::init().expect("libadwaita initialises under the test backend"); + crate::register_test_resources(); + let view = ConnectFailureView { + message: "refused".to_owned(), + can_edit: true, + }; + ConnectFailedPage::new(&view, "Edit Connection…", reenter, None) + } + + #[gtk4::test] + fn the_reenter_action_is_named_only_with_its_target() { + let hidden = page(None).imp().reenter_button.get(); + assert!(!hidden.is_visible()); + assert_eq!( + hidden.action_name(), + None, + "an action that takes a target was named on a button without one" + ); + + let kind = SecretKind::DatabasePassword; + let shown = page(Some(kind)).imp().reenter_button.get(); + assert_eq!(shown.action_name().as_deref(), Some("win.retry-with-new-secret")); + assert_eq!(shown.action_target_value(), Some(kind.attribute().to_variant())); + } +} diff --git a/linux/data/resources/ui/connect-failed-page.ui b/linux/data/resources/ui/connect-failed-page.ui index 1dded9256b..289ab02b16 100644 --- a/linux/data/resources/ui/connect-failed-page.ui +++ b/linux/data/resources/ui/connect-failed-page.ui @@ -22,11 +22,12 @@ + would only send the refused secret again. Its + action takes the secret's kind, so the code + names it once it has set that target. --> false Enter Password… - win.retry-with-new-secret