From 7a87aebec7cf3af05a579b785bc5142ecea38db6 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sat, 23 May 2026 13:05:59 -0500 Subject: [PATCH] fix(tui): move composer history persistence off UI thread Move composer history persistence off the Enter hot path so submitting messages does not block the TUI event loop. --- crates/tui/src/composer_history.rs | 116 ++++++++++++++++++++++++++++- 1 file changed, 113 insertions(+), 3 deletions(-) diff --git a/crates/tui/src/composer_history.rs b/crates/tui/src/composer_history.rs index df12ea46..0f972cfd 100644 --- a/crates/tui/src/composer_history.rs +++ b/crates/tui/src/composer_history.rs @@ -9,10 +9,22 @@ //! Entries that begin with `/` (slash commands) are NOT stored — they //! pollute the recall stream and the fuzzy slash-menu already covers //! them. Empty / whitespace-only inputs are also skipped. +//! +//! ## Off-thread writes (#1927) +//! +//! [`append_history`] used to block the caller for a read-then-atomic- +//! rewrite of the full file. That ran on the UI thread inside +//! `submit_input`, contributing a perceptible stall after Enter. The +//! public entry point now hands work to a dedicated writer thread via +//! [`writer_sender`] and returns immediately. Submissions stay serialised +//! in arrival order, so the on-disk file keeps its "oldest first" +//! invariant. use std::fs; use std::io::{BufRead, BufReader}; use std::path::{Path, PathBuf}; +use std::sync::OnceLock; +use std::sync::mpsc::{Sender, channel}; /// Hard cap on persisted history. Keeps the file small (typical entries /// are < 200 chars, so 1000 entries ≈ 200 KB) and bounds startup load @@ -50,13 +62,52 @@ fn load_history_from(path: &Path) -> Vec { /// stay within [`MAX_HISTORY_ENTRIES`]. Slash-commands and empty input /// are skipped — those don't help recall. /// -/// Best-effort — failures are logged via `tracing` but not propagated -/// because composer history is a UX nicety, not a correctness concern. +/// Best-effort and non-blocking — work is forwarded to a dedicated writer +/// thread so the caller (typically the UI submit handler) returns +/// immediately. See module docs for the rationale (#1927). Failures on +/// the writer thread are logged via `tracing` but not propagated. pub fn append_history(entry: &str) { let Some(path) = default_history_path() else { return; }; - append_history_to(&path, entry); + append_history_dispatched(&path, entry); +} + +/// Path-injectable variant of [`append_history`] used by tests. Forwards +/// the work to the dedicated writer thread (or falls back to a synchronous +/// write if the channel send fails) so callers never block on disk I/O. +fn append_history_dispatched(path: &Path, entry: &str) { + let entry = entry.to_string(); + if writer_sender() + .send((path.to_path_buf(), entry.clone())) + .is_err() + { + append_history_to(path, &entry); + } +} + +/// Lazy singleton sender for the dedicated composer-history writer +/// thread. Initialised on first use; the thread runs for the lifetime +/// of the process and drains queued writes in arrival order. +fn writer_sender() -> &'static Sender<(PathBuf, String)> { + static SENDER: OnceLock> = OnceLock::new(); + SENDER.get_or_init(|| { + let (tx, rx) = channel::<(PathBuf, String)>(); + let spawn_result = std::thread::Builder::new() + .name("composer-history-writer".to_string()) + .spawn(move || { + // recv() returns Err when all senders have dropped, which + // only happens at process shutdown because the singleton + // sender lives in a static for the lifetime of the process. + while let Ok((path, entry)) = rx.recv() { + append_history_to(&path, &entry); + } + }); + if let Err(err) = spawn_result { + tracing::warn!("Failed to spawn composer-history-writer: {err}"); + } + tx + }) } fn append_history_to(path: &Path, entry: &str) { @@ -172,4 +223,63 @@ mod tests { let (_tmp, path) = temp_history_path(); assert!(load_history_from(&path).is_empty()); } + + /// Regression for #1927 — the dispatched append path must return + /// promptly even when a synchronous write of the seeded file would + /// be slow. We pre-populate the file with ~1000 entries (the cap) + /// so a sync read-modify-write would take real disk time on any + /// platform, then call `append_history_dispatched` many times and + /// assert that the cumulative wall-clock cost stays well below the + /// stall the user reports. + #[test] + fn append_history_dispatched_does_not_block_the_caller() { + use std::time::{Duration, Instant}; + + let (_tmp, path) = temp_history_path(); + // Seed close to the cap so a synchronous rewrite is non-trivial. + let seed = (0..(MAX_HISTORY_ENTRIES - 50)) + .map(|i| format!("seed entry {i}")) + .collect::>() + .join("\n") + + "\n"; + std::fs::write(&path, seed).expect("seed history"); + + let start = Instant::now(); + for i in 0..50 { + append_history_dispatched(&path, &format!("new entry {i}")); + } + let dispatch_elapsed = start.elapsed(); + + // 50 sync read-modify-write cycles on a ~200KB file would be + // measurable (tens of ms even on a fast SSD). The dispatch path + // hands work to the writer thread and returns; the whole loop + // should finish in single-digit ms. Pick a generous CI-safe + // bound that still catches a regression to the old sync path. + assert!( + dispatch_elapsed < Duration::from_millis(150), + "append_history dispatch was too slow: {dispatch_elapsed:?} \ + (likely re-introduced #1927: caller blocked on disk write)" + ); + + // Give the writer thread time to drain the queue, then verify the + // new entries landed. + let deadline = Instant::now() + Duration::from_secs(5); + loop { + let loaded = load_history_from(&path); + if loaded.iter().any(|line| line == "new entry 49") { + // Last dispatched entry observed; queue is drained. + assert!(loaded.iter().any(|line| line == "new entry 0")); + break; + } + if Instant::now() >= deadline { + panic!( + "writer thread did not persist the dispatched entries; \ + loaded {} entries, last = {:?}", + loaded.len(), + loaded.last() + ); + } + std::thread::sleep(Duration::from_millis(25)); + } + } }