From 00435553277107a1ddad44066d4616739aed008f Mon Sep 17 00:00:00 2001 From: Darren Cheng Date: Tue, 4 Aug 2026 14:27:59 -0700 Subject: [PATCH 1/2] Bound the live terminal emulator's scrollback to fix input lag on long sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit x/vt's Scrollback.Push evicts the oldest line via slices.Delete(s.lines, 0, 1) once at capacity β€” an O(cap) shift of the whole backing slice, paid on every line that scrolls off the top of the screen. The live agent-view emulator never overrode x/vt's 10K-line default, so any long-running, high-output session pays that shift on every subsequent scrolled line, synchronously on the tview main goroutine inside SafeEmuWrite β€” the same goroutine that processes keyboard input. Confirmed against a real 18+ hour, 2MB+ log task. Cap the live emulator's scrollback 10x smaller (1K lines), mirroring the existing pattern for the replay emulator's cap. Deep scroll-back-in-history already goes through the separate 50K-line replay emulator built for scroll mode, so this has no user-visible effect on how far back a user can scroll. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/tui/terminal/terminalpane.go | 24 ++++++++++++++++++-- internal/tui/terminal/terminalpane_test.go | 26 ++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/internal/tui/terminal/terminalpane.go b/internal/tui/terminal/terminalpane.go index 4398df15..916e414f 100644 --- a/internal/tui/terminal/terminalpane.go +++ b/internal/tui/terminal/terminalpane.go @@ -28,10 +28,29 @@ import ( // replayScrollbackSize is the scrollback buffer for replay emulators used // during scrollback browsing. 50K lines (~2500 screens at 20 rows) allows -// deep scrolling in long sessions without constant rebuilds. The live emulator -// uses x/vt's default (10K lines) since only the current viewport matters. +// deep scrolling in long sessions without constant rebuilds. const replayScrollbackSize = 50_000 +// liveScrollbackSize bounds the LIVE emulator's own scrollback buffer. x/vt's +// Scrollback.Push evicts the oldest line via slices.Delete(s.lines, 0, 1) once +// at capacity β€” an O(cap) shift of the whole backing slice, paid on every +// single line that scrolls off the top of the screen. x/vt's own default +// (10K lines) was never overridden here, so any live session whose output has +// scrolled past 10K total lines pays that shift on every subsequent scrolled +// line, on the tview main goroutine inside SafeEmuWrite β€” the same goroutine +// that processes keyboard input. A single large output burst that scrolls +// hundreds of lines in one feed does hundreds of these shifts back-to-back, +// which is exactly the input-lag symptom reported for a long-running, +// high-output solo task (noticing-lag-inputting-text). +// +// The live emulator doesn't need deep scrollback to begin with: real +// scroll-back-in-history already goes through the separate replayEmu above +// (50K lines, built fresh for scroll mode), and paintEmu's anchor-locked +// steady state reads almost entirely from the main screen buffer, not +// scrollback. Capping this 10x smaller cuts the per-push shift cost +// proportionally with no loss of user-visible history. +const liveScrollbackSize = 1_000 + // NewDrainedEmulator creates an x/vt SafeEmulator with a goroutine that drains // the response pipe. x/vt uses io.Pipe() internally β€” when the emulator // processes terminal query sequences (DA1, DA2, DSR, etc.), it writes responses @@ -1952,6 +1971,7 @@ func (tp *TerminalPane) newTrackedEmulator(cols, rows int) *xvt.SafeEmulator { func (tp *TerminalPane) newTrackedEmulatorWithCallback(cols, rows int, onCursorVisible func(bool)) *xvt.SafeEmulator { emu := NewDrainedEmulator(cols, rows) + emu.Emulator.SetScrollbackSize(liveScrollbackSize) if onCursorVisible != nil { emu.Emulator.SetCallbacks(xvt.Callbacks{ CursorVisibility: onCursorVisible, diff --git a/internal/tui/terminal/terminalpane_test.go b/internal/tui/terminal/terminalpane_test.go index 51a60338..e266eda7 100644 --- a/internal/tui/terminal/terminalpane_test.go +++ b/internal/tui/terminal/terminalpane_test.go @@ -1611,6 +1611,32 @@ func TestTerminalPane_ReplayEmulatorHasLargeScrollback(t *testing.T) { } } +// TestTerminalPane_LiveEmulatorHasBoundedScrollback is the regression guard +// for the O(n)-eviction input-lag fix (noticing-lag-inputting-text): the LIVE +// emulator must cap its scrollback well below x/vt's 10K default, since +// Scrollback.Push evicts via an O(cap) slice shift on every scrolled line +// once at capacity β€” a smaller cap bounds that per-push cost. If +// SetScrollbackSize(liveScrollbackSize) were removed from +// newTrackedEmulatorWithCallback, this test fails because the live emulator +// would silently fall back to x/vt's 10K default. +func TestTerminalPane_LiveEmulatorHasBoundedScrollback(t *testing.T) { + if testing.Short() { + t.Skip("feeds 2K lines to emulator") + } + tp := NewTerminalPane() + emu := tp.newTrackedEmulator(80, 24) + + // Feed well past liveScrollbackSize (1K) to prove the cap actually holds. + for i := 0; i < 2_000; i++ { + emu.Write([]byte("line of content for scrollback testing\n")) //nolint:errcheck + } + + sbLen := emu.ScrollbackLen() + if sbLen > liveScrollbackSize { + t.Errorf("live emulator scrollback=%d, want <=%d (SetScrollbackSize regression?)", sbLen, liveScrollbackSize) + } +} + func TestTerminalPane_ScrollUpWhileAlreadyScrolled(t *testing.T) { // Scrolling further up while already scrolled should NOT invalidate // the replay emu β€” it's still current for the scrolled region. From db6b9d5b5ea2f4c7893cdfc2198740ab3f673f82 Mon Sep 17 00:00:00 2001 From: Darren Cheng Date: Tue, 4 Aug 2026 15:24:51 -0700 Subject: [PATCH 2/2] Address review: document scrollback-cap fix as gotcha, ground cap in measured data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two warnings from /review on the live-scrollback-cap fix (#923): - Missing a gotchas/pty-terminal.md entry per this repo's own documentation rule for non-obvious invariants/fixes. Added, including the residual-cost caveat (this mitigates rather than eliminates the O(n) eviction) and the known resize-taller backfill trade-off the reviewer identified. - The 1,000-line cap wasn't grounded in measured data the way the sibling replayScrollbackSize constant is. Benchmarked a synthetic 500-line burst feed at the old 10K cap vs the new 1K cap (~7.9ms vs ~5.0ms on an Apple M5 Max) and folded the real numbers into the code comment. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- context/knowledge/gotchas/pty-terminal.md | 1 + context/knowledge/index.md | 2 +- internal/tui/terminal/terminalpane.go | 9 ++++++++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/context/knowledge/gotchas/pty-terminal.md b/context/knowledge/gotchas/pty-terminal.md index 7377c70c..27c59094 100644 --- a/context/knowledge/gotchas/pty-terminal.md +++ b/context/knowledge/gotchas/pty-terminal.md @@ -71,4 +71,5 @@ - **`tp.emu` rebuild must invalidate `paintCacheValid` before falling through.** Dimension change in `renderLive` creates a fresh emulator but the paint cache from the prior frame may still be flagged valid. The replay path (when no new bytes arrive but viewport matches) would serve stale cells for one frame before the next paintEmu refreshes. Set `paintCacheValid = false` inline at the rebuild branch in addition to the existing scroll/reset hooks. - **OSC sequences MUST be stripped before feeding the emulator (`oscfilter.go`), because x/ansi treats a 0x9C byte inside an OSC string as a C1 String Terminator even when it's a UTF-8 continuation byte.** `charmbracelet/x/ansi` `parser_decode.go` (StringState, `case ST:`) ends the OSC at the first 0x9C. Many glyphs encode 0x9C β€” e.g. Claude's spinner title `ESC]2;✳ …BEL` where ✳ = `E2 9C B3`. The parser truncates the title at the `9C` and renders the tail (" …") as printable ground text. An in-place `AskUserQuestion` repaint then overwrites most of the leak, but Claude positions each option label with `ESC[6G` (cursor to column 6), skipping column 5 β€” so exactly one leaked character survives in that gap, appearing between `1.` and the option label (the "stray character in AskUserQuestion"). tmux/xterm never reproduce it: in UTF-8 mode they don't treat a continuation byte as ST. The fix is the `oscFilter` streaming state machine that drops `ESC] … (BEL | ESC\)` entirely β€” it deliberately does **not** treat 0x9C as a terminator (that's the bug), and is safe because the pane never displays window titles and OSC-8 hyperlink text is ground text that survives. The persistent `tp.oscStrip` filter must be `reset()` whenever `tp.emu` is recreated (so it re-syncs to the clean escape boundary of the re-fed history) and carries state across incremental feeds (an OSC can split across two deltas). Upgrading x/ansi does not help β€” v0.11.7 has the identical code. The end-to-end regression `TestOSCFilter_FixesEmulatorLeak` skips (rather than fails) if a future x/vt fixes the leak, flagging that the workaround may be removable. - **A full-screen agent (alternate screen) gets the mouse WHEEL forwarded to it, NOT the pane's own scrollback (BUG-026).** Agents like Claude Code / Codex run as full-screen TUIs (`ESC[?1049h` + cursor-home `ESC[H` in-place redraws) that push ~zero lines into terminal scrollback, so wheel-up on the pane's own scrollback was a silent no-op (confirmed by feeding real session logs through the replay emulator: a coordinator that emitted scrolling output β†’ sbLenβ‰ˆ2791, scrollable; a worker that redrew in place β†’ sbLen=0, `maxScroll`=0). `TerminalPane.MouseHandler` now calls `agentOwnsWheel()` (live session AND `tp.emu.IsAltScreen()`) and `forwardWheel()`s the event as an SGR frame (`ESC[<64|65;Cx;Cy M`, button 64=up/65=down, 1-based inner-rect coords clamped like the plugin forwarder #681) to the agent via `sess.WriteInput`, so the agent scrolls its OWN view β€” mirroring how a real terminal hands the wheel to the foreground app. A non-alt-screen agent or a finished/replay session keeps the pane's terminal scrollback (its on-disk log replay is the only way to scroll its history). This is NOT a routing/click-to-focus bug β€” the wheel always reached the pane (scroll-DOWN visibly redrew); there was simply no scrollback to reveal. +- **The LIVE emulator's scrollback must be capped well below x/vt's 10K-line default β€” an unbounded/high cap causes real input lag on long, high-output sessions, not just memory growth.** The vendored `x/vt` library's `Scrollback.Push` evicts the oldest line via `slices.Delete(s.lines, 0, 1)` once at capacity β€” an O(cap) shift of the entire backing slice, paid on every line that scrolls off the top of the screen (`Screen.DeleteLine` β†’ `scrollback.PushN` β†’ `Push`). `newTrackedEmulatorWithCallback` never overrode x/vt's 10K-line default, so any live session whose output scrolled past 10K total lines paid that shift on every subsequent scrolled line, synchronously inside `SafeEmuWrite` on the tview main goroutine β€” the SAME goroutine that processes keyboard input. A single output burst that scrolls hundreds of lines in one feed does hundreds of these shifts back-to-back before control returns to the event loop, reproducing as continuous input lag for as long as the task keeps producing output (confirmed against a real 18+ hour, 2MB+ dogfood session log). Fix: `SetScrollbackSize(liveScrollbackSize)` (1K, 10x smaller) right after `NewDrainedEmulator` in `newTrackedEmulatorWithCallback`, mirroring the existing `replayScrollbackSize` (50K) override on `newDrainedReplayEmulator`. This is a mitigation, not a fix of the underlying O(n) eviction β€” a large enough burst still pays a proportionally larger (if smaller-constant) shift; measured on a synthetic 500-line burst, the 1K cap costs ~5.0ms vs ~7.9ms at the old 10K default (Apple M5 Max) β€” real output (with ANSI parsing overhead) or a slower machine costs more in absolute terms. The real fix would be an O(1) ring/deque eviction in `Scrollback` itself (a vendored dependency, not patched here). **Known trade-off:** in the (normally negligible) case where `paintEmu`'s viewport is TALLER than the current main-screen content β€” e.g. right after a resize-taller on a long session, before the screen has refilled β€” rendering backfills extra rows from the live emulator's OWN scrollback (not the separate replay emulator), so a long session can now show at most 1K lines of backfilled history there instead of up to 10K. Deep scroll-back-in-history via explicit scroll mode is unaffected β€” it already goes through the separate `replayEmu` (`newTrackedReplayEmulatorWithCallback`, still 50K lines). Regression test: `TestTerminalPane_LiveEmulatorHasBoundedScrollback`. - **The KEYBOARD scroll path and scroll-mode ENTRY are ALSO suppressed for alt-screen panes, not just the mouse wheel (BUG-031).** BUG-026 only guarded `MouseHandler`; the keyboard path (`Shift+↑`/`Shift+PgUp` in the main agent view, `PgUp` in a Hera pane) and `ScrollUp`/`AccelScrollUp` stayed unguarded, so they still entered argus's `[SCROLL]` mode and `asyncReplayRebuild` replayed the alt-screen log (cursor-home/erase/CUP in-place frames) through a fresh emulator AS LINEAR SCROLLBACK β€” stacked frames read as interleaved/columnar garbage. Fix: `TerminalPane.InAltScreen()` (`tp.emu != nil && tp.emu.IsAltScreen()`, the single alt-screen signal `agentOwnsWheel` now also reuses) makes `ScrollUp`/`AccelScrollUp` no-op, and the keyboard callers suppress + show a transient status-bar affordance ("Fullscreen agent β€” scroll within the agent") via `App.statusbar.SetInfo` / the Hera page's `OnInfo` callback. Do NOT try to reconstruct linear scrollback for alt-screen β€” alt-screen apps overwrite in place, so there is no meaningful history; suppress, don't rebuild. The guard does NOT latch: it keys off live `IsAltScreen()`, so `ESC[?1049l` (agent quit/exit) restores normal scrollback. `ScrollDown`/`AccelScrollDown` are intentionally NOT guarded β€” they only reduce `scrollOffset` toward the live tail, the recovery path if a pane ever holds a stale offset when the agent enters alt-screen. diff --git a/context/knowledge/index.md b/context/knowledge/index.md index b468e0f7..254bdef7 100644 --- a/context/knowledge/index.md +++ b/context/knowledge/index.md @@ -5,7 +5,7 @@ Non-obvious invariants and gotchas, split by topic. Read the relevant file when | File | Topic | Bullets | | --- | --- | --- | | [gotchas/daemon-rpc.md](gotchas/daemon-rpc.md) | Daemon lifecycle, RPC timeouts, reconciliation races, session resume, Claude /clear recapture, binary staleness (SHA-256 content hash, not mtime), self-update, launchd auto-start + PATH, stream Since offset, paste-boundary flush, *.test fork-bomb backstop, singleton flock, PR poller (eligibility, terminal-state skip, batched per-repo graphql w/ alias-safe ids + chunked keep-stale), evidence-based completion (ExitInfo.CleanExit predicate; reconcileβ†’InReview never Complete), hera worker finish policy (BUG-050 RollHeraWorkerToReview), startup hera-binding reconciliation, session-supervisor P1–P4 (dark PTY-owner, daemon-as-client behind cfg.Supervisor.Enabled, re-attach on bounce, default ON + in-process rollback, #707 cache-vs-EOF relay race), callWithTimeout nil-rpc guard, TUI supervisor restart, go-install skew (doctor restart-vs-path-divergence, supervisor-checked-on-auto-start, ProtocolVersion 2β†’3 old-supervisor-unknown, double-confirm supervisor restart), revive-restores-in_progress (BUG-B ReviveHeraWorkerToInProgress, inverse of RollHeraWorkerToReview), host-suspend watchdog (ARGUS_HOST_SUSPENDED advisory note β€” wall-clock gap>3m between 30s ticks, unconditional not Hera-gated, sibling of sendBounceSignals, one-shot no-dedup baseline-before-loop, monotonic-strip required, advisory-only no state mutation), Claude Code's own background-session supervisor (orphaned-worker root cause: single-PID SIGTERM can never reach a session Claude Code itself detached to its per-user supervisor; `claude agents`/`claude stop` detection+fix SHIPPED via internal/claudeagents + Runner.Stop fire-and-forget reap, not a signal-scoping bug), doctor Stop-hook registration check (detect-missing-coord-hook: REGISTERED/NOT REGISTERED/UNKNOWN, advisory-only, never gates the binary-coherence exit code), resume-time session-ID recapture (agent.RefreshResumeSessionID mirrors the exit hook because hera workers idle/StreamLost never reach captureSessionIDPostExit; Claude-only; wired at reattachSupervised orphans + TUI startSession + REST resume/restart; idempotent, never blanks/fabricates), doctor diligence-profile-library check (add-doctor-profile-check: FOUND/NONE FOUND/UNKNOWN, library-existence-only not per-project binding, missing-dir vs unreadable-dir tri-state, advisory-only), hera_revive coordinator PULL-revive (add-hera-revive: third ReviveHeraWorkerToInProgress caller, shared internal/hera.ReviveRole gate, deliberately not unified with the TUI's Enter-key revive), KNOWN UNFIXED daemon-bounce SessionStatus false-negative (Daemon.SessionStatus transiently reports a supervisor-still-alive session as dead right after a daemon restart, firing RollHeraWorkerToReview on a task that never died β€” found + documented by narrow-needs-input-sustained-active, not fixed there) | 110 | -| [gotchas/pty-terminal.md](gotchas/pty-terminal.md) | PTY sizing, x/vt emulator, ring buffer, replay cache, paint cache, lazyScreen, test concurrency, ESC-boundary alignment, live rebuild from log tail, monotonic firstByteOffset, scrollOffset clamp, waitLoop close-after-drain order, rerender gates (unchanged-cols, cache invalidation, blocked-on-prompt), OSC 0x9C-in-UTF8 strip filter, persistent preview emulator (PreviewVT reuse-via-RIS), plugin terminalpane cursor sync, alt-screen keyboard-scroll + scroll-mode-entry suppression (BUG-031, not just the wheel), scroll-past-window lazy extend (BUG-E), scroll replay authored-width emulate-clip (live-scroll corruption), dimension-change resize-in-place instead of lossy 8MB-tail rebuild (BUG-068, overlapping/garbled live-view corruption), ring-wrap exact-offset log catch-up instead of lossy 8MB-tail rebuild (BUG-073, BUG-068's ring-wrap sibling β€” reached by backgrounding a busy agent's pane, not resize), live incremental-feed atomic (raw,total) snapshot instead of two separate racy calls (BUG-075, TOCTOU race distinct from BUG-068/073/074 β€” reachable on an actively-streamed pane with no bind/resize at all, causes a duplicated recent phrase + a couple of dropped characters), log/ring merge no-splice-across-unrecoverable-gap (BUG-076, readLiveRebuildHistory understates emuFedTotal to logSize instead of gluing non-contiguous log+ring bytes together when the on-disk log lags the ring by more than 256KB under a heavy output burst β€” distinct root cause from BUG-075 sharing the same no-bind-event signature) | 66 | +| [gotchas/pty-terminal.md](gotchas/pty-terminal.md) | PTY sizing, x/vt emulator, ring buffer, replay cache, paint cache, lazyScreen, test concurrency, ESC-boundary alignment, live rebuild from log tail, monotonic firstByteOffset, scrollOffset clamp, waitLoop close-after-drain order, rerender gates (unchanged-cols, cache invalidation, blocked-on-prompt), OSC 0x9C-in-UTF8 strip filter, persistent preview emulator (PreviewVT reuse-via-RIS), plugin terminalpane cursor sync, alt-screen keyboard-scroll + scroll-mode-entry suppression (BUG-031, not just the wheel), scroll-past-window lazy extend (BUG-E), scroll replay authored-width emulate-clip (live-scroll corruption), dimension-change resize-in-place instead of lossy 8MB-tail rebuild (BUG-068, overlapping/garbled live-view corruption), ring-wrap exact-offset log catch-up instead of lossy 8MB-tail rebuild (BUG-073, BUG-068's ring-wrap sibling β€” reached by backgrounding a busy agent's pane, not resize), live incremental-feed atomic (raw,total) snapshot instead of two separate racy calls (BUG-075, TOCTOU race distinct from BUG-068/073/074 β€” reachable on an actively-streamed pane with no bind/resize at all, causes a duplicated recent phrase + a couple of dropped characters), log/ring merge no-splice-across-unrecoverable-gap (BUG-076, readLiveRebuildHistory understates emuFedTotal to logSize instead of gluing non-contiguous log+ring bytes together when the on-disk log lags the ring by more than 256KB under a heavy output burst β€” distinct root cause from BUG-075 sharing the same no-bind-event signature), live emulator scrollback cap 10x below x/vt's 10K default (mitigates, not eliminates, O(n) slices.Delete eviction cost causing input lag on long/high-output sessions β€” mirrors the existing replayScrollbackSize override pattern; known trade-off on resize-taller backfill, deep scroll-back-in-history unaffected via the separate replay emulator) | 67 | | [gotchas/ui-threading.md](gotchas/ui-threading.md) | tview thread safety, tick-goroutine rules, lazyScreen fill invariant, paste/input batching, tmux UX-tearing post-mortem (no Sync; 3 legit repair callsites), OnBranchChange log-only contract, EventFocus drift recovery, stderr/stdout-after-Init fd 2 guards, status-bar notice auto-expire (15s TTL, lazy revert via 1s tick, no Sync/timer), SetScreen swallows tcell Init() errors (no-ctty nil-tty EnableMouse panic, probeTerminal preflight guard), probeTerminal false-positive-on-every-real-terminal regression (tcell devTty.Close() nil-`f` β†’ os.ErrInvalid, fix discards Close() err + probeTerminalDev pty-slave test seam) | 29 | | [gotchas/ci-gates.md](gotchas/ci-gates.md) | `make pre-pr` per-gate failure recipes (fmt-check, test-cover-gate floor, lint-pr new-from-rev, vuln stdlib continue-on-error, macOS PTY-exhaustion flake in internal/agent under full-suite -race, same flake class also hits internal/tui/terminal + internal/tui) | 6 | | [gotchas/sandbox.md](gotchas/sandbox.md) | macOS sandbox-exec SBPL profiles, symlink resolution, allowed paths, Chrome support dir for Playwright, AppleEvent allowlist, Messages.app legacy alias, picker modal, TCC re-prompt fix (stable-signed local binary via workflow-neutral `make install-signed`; first-sign Keychain "Always Allow") | 26 | diff --git a/internal/tui/terminal/terminalpane.go b/internal/tui/terminal/terminalpane.go index 916e414f..083608af 100644 --- a/internal/tui/terminal/terminalpane.go +++ b/internal/tui/terminal/terminalpane.go @@ -48,7 +48,14 @@ const replayScrollbackSize = 50_000 // (50K lines, built fresh for scroll mode), and paintEmu's anchor-locked // steady state reads almost entirely from the main screen buffer, not // scrollback. Capping this 10x smaller cuts the per-push shift cost -// proportionally with no loss of user-visible history. +// proportionally with no loss of user-visible history for the common case β€” +// though it's a mitigation, not a fix of the underlying O(n) eviction: +// measured on a 500-line synthetic burst feed (Apple M5 Max), the 1K cap +// costs ~5.0ms vs ~7.9ms at the old 10K default β€” real output (with ANSI +// escape parsing overhead) or a slower machine will cost more in absolute +// terms, and an even larger burst still pays a proportionally larger shift. +// See gotchas/pty-terminal.md for the resize-taller edge case this trades +// off, and the residual-cost caveat. const liveScrollbackSize = 1_000 // NewDrainedEmulator creates an x/vt SafeEmulator with a goroutine that drains