fix(shell): O(1) job-panel refresh — drop the full-buffer clone under the mutex
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<Mutex<Vec<u8>>>, cursor: &mut usize) -> (Vec<u8>, 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<Mutex<Vec<u8>>>, 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 {
|
||||
|
||||
Reference in New Issue
Block a user