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.
This commit is contained in:
@@ -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<String> {
|
||||
/// 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<Sender<(PathBuf, String)>> = 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::<Vec<_>>()
|
||||
.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));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user