From 2950cadc36ae57eceebcf9c7f66800fe58ec5314 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Tue, 12 May 2026 01:28:21 -0500 Subject: [PATCH] =?UTF-8?q?fix(shell):=20O(1)=20job-panel=20refresh=20?= =?UTF-8?q?=E2=80=94=20drop=20the=20full-buffer=20clone=20under=20the=20mu?= =?UTF-8?q?tex?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1299. The TUI's job panel refreshes every 2.5 seconds by calling `job_snapshot()`, which previously called `full_output()` to clone the entire accumulated stdout/stderr buffer under the `ShellManager` mutex. For long-running jobs that flood stdout (browser automation drivers, large `cargo build` runs, anything streaming progress to a pipe) the buffer grew unboundedly; cloning held the mutex for O(total_bytes) time, starving the `crossterm::event::poll` loop and producing the input freeze users reported as "TUI locks up after ~30 seconds of output." The fix: 1. `job_snapshot()` no longer calls `full_output()`. A new `tail_from_buffer()` reads only the last `max_tail_chars * 4` bytes under the lock and decodes them. Lock hold time is now O(1) regardless of total output volume. The job-panel display only needs the tail anyway — never the whole stream. 2. `take_delta_from_buffer()` reduces its clone footprint: the old code did `buffer.lock().map(|d| d.clone())` — eagerly cloning the full buffer before slicing the unread delta out. New code slices `[cursor..total]` inside the lock guard so only the unread bytes are allocated. 3. `tail_start` can land mid-codepoint after the buffer wraps. Before slicing, the code now skips any UTF-8 continuation bytes (`& 0xC0 == 0x80`) so `from_utf8_lossy` never sees an invalid leading byte and never emits a leading U+FFFD in the job-panel tail. `stdout_len` / `stderr_len` still report the true total byte counts so no caller invariant changes. `job_detail()` (the user-triggered detail view) still calls `full_output()` intentionally — detail views are rare and not on the hot refresh path. The orphan-grandchild `collect_output()` path is already handled on Unix via `kill_child_process_group`; the equivalent Windows fix is filed as a separate concern (see PR body). Harvested from PR #1494 by @CrepuscularIRIS Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 18 +++++++++++++ crates/tui/src/tools/shell.rs | 51 +++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63d19920..bf4373be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,24 @@ real world uses." ### Fixed +- **TUI input no longer freezes while long-running shell jobs + flood stdout** (#1299, harvested from PR #1494 by + **@CrepuscularIRIS / autoghclaw**). The job-panel refresh path + was calling `full_output()` from inside the `ShellManager` + mutex, which cloned the entire accumulated stdout/stderr buffer + every 2.5 seconds. For browser-automation or large-build jobs + the buffer grew unboundedly; cloning held the mutex for + O(total_bytes) time, starving the `crossterm::event::poll` loop + and dropping keystrokes. The refresh now reads only the last + `max_tail_chars * 4` bytes under the lock (lock hold time is + O(1) regardless of total output volume) and decodes those into + a tail string for display. `stdout_len` / `stderr_len` still + report the true total byte counts so no caller invariant + breaks. Also tightens `take_delta_from_buffer` to slice + `[cursor..total]` inside the lock guard instead of cloning the + whole buffer first, and skips UTF-8 continuation bytes at + `tail_start` so `from_utf8_lossy` never emits a leading U+FFFD + in the job panel. - **`@`-mention truncation no longer splits multi-byte UTF-8 sequences** (#1441, harvested from PR #1495 by **@CrepuscularIRIS / autoghclaw**). When `@`-mentioning a file diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index d567ed03..99fa2bcd 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -469,7 +469,17 @@ impl BackgroundShell { } fn job_snapshot(&self) -> ShellJobSnapshot { - let (stdout_full, stderr_full, stdout_len, stderr_len) = self.full_output(); + // Use tail_from_buffer instead of full_output so we never clone the + // entire accumulated stdout/stderr for display purposes. full_output + // is O(total_bytes_written), which caused the ShellManager mutex to be + // held for an arbitrarily long time during list_jobs() calls from the + // TUI event loop — freezing input handling on long automation runs. + let (stdout_len, stdout_tail) = tail_from_buffer(&self.stdout_buffer, 1200); + let (stderr_len, stderr_tail) = self + .stderr_buffer + .as_ref() + .map(|buf| tail_from_buffer(buf, 1200)) + .unwrap_or((0, String::new())); ShellJobSnapshot { id: self.id.clone(), job_id: self.id.clone(), @@ -478,8 +488,8 @@ impl BackgroundShell { status: self.status.clone(), exit_code: self.exit_code, elapsed_ms: u64::try_from(self.started_at.elapsed().as_millis()).unwrap_or(u64::MAX), - stdout_tail: tail_text(&stdout_full, 1200), - stderr_tail: tail_text(&stderr_full, 1200), + stdout_tail, + stderr_tail, stdout_len, stderr_len, stdin_available: self.stdin.is_some() && self.status == ShellStatus::Running, @@ -1370,11 +1380,36 @@ impl ShellManager { } fn take_delta_from_buffer(buffer: &Arc>>, cursor: &mut usize) -> (Vec, usize) { - let data = buffer.lock().map(|d| d.clone()).unwrap_or_default(); - let start = (*cursor).min(data.len()); - let delta = data[start..].to_vec(); - *cursor = data.len(); - (delta, data.len()) + let guard = buffer.lock().unwrap_or_else(|e| e.into_inner()); + let total = guard.len(); + let start = (*cursor).min(total); + // Clone only the unread portion (the delta), not the entire accumulated buffer. + // Long-running processes can produce megabytes of output; cloning the full + // buffer on every poll held the ShellManager mutex for O(total_bytes) time. + let delta = guard[start..].to_vec(); + *cursor = total; + (delta, total) +} + +/// Read only the tail of a byte buffer and return (total_len, tail_string). +/// +/// Avoids cloning the full buffer when only a trailing excerpt is needed +/// (e.g. for the job-panel display). `max_tail_chars` is in Unicode scalar +/// values; we read at most `max_tail_chars * 4` bytes from the end to account +/// for multi-byte UTF-8 sequences. +fn tail_from_buffer(buffer: &Arc>>, max_tail_chars: usize) -> (usize, String) { + let guard = buffer.lock().unwrap_or_else(|e| e.into_inner()); + let total = guard.len(); + // Over-estimate byte count (4 bytes per char worst case for UTF-8). + let mut tail_start = total.saturating_sub(max_tail_chars.saturating_mul(4)); + // Snap forward to the next valid UTF-8 codepoint boundary so we don't + // pass a slice beginning with continuation bytes (0x80–0xBF) to + // from_utf8_lossy, which would emit a leading U+FFFD replacement char. + while tail_start < total && (guard[tail_start] & 0xC0) == 0x80 { + tail_start += 1; + } + let tail_str = String::from_utf8_lossy(&guard[tail_start..]).into_owned(); + (total, tail_text(&tail_str, max_tail_chars)) } fn tail_text(text: &str, max_chars: usize) -> String {