From d7b177fa5630578115c9bd1220499cf110b8a490 Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Thu, 30 Jul 2026 11:49:25 -0700 Subject: [PATCH] Fix TUI agent-pane live-stream TOCTOU race (BUG-075) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit renderLive's incremental-feed path read totalWritten via a separate, earlier sess.TotalWritten() call, then later read raw via a separate sess.RecentOutput() call. readLoop is a live, independent goroutine, so for an actively streaming session bytes can land in the gap between the two calls, making raw longer than the earlier totalWritten accounts for. Slicing raw[len(raw)-newBytes:] against that stale newBytes then feeds the wrong suffix: it silently skips the true next bytes (dropped characters) and instead feeds bytes from further ahead, understating emuFedTotal β€” the next frame then re-feeds that same already-fed tail (a duplicated recent phrase). This is distinct from BUG-068/BUG-073/BUG-074, all of which fire only around a bind/resize/rebuild event β€” this race is reachable on a pane that's actively being watched with no such event involved at all, which made it easy to mistake for those bind-time reconstruction defects. agent.Session.RecentOutputTailWithTotal's own doc comment already named this exact hazard (for the /output HTTP endpoint's cursor); renderLive's live-feed path just hadn't been updated to use it. Fix: fetch (raw, totalWritten) together via a single RecentOutputTailWithTotal call and recompute newBytes from that same snapshot, skipped only when emuMissing (that branch is unconditionally a full replay regardless of raw/newBytes, and calls readLiveRebuildHistory, which does its own independent ring read). πŸ€– 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 | 29 +++++- internal/tui/terminal/terminalpane_test.go | 106 ++++++++++++++++++++- 4 files changed, 131 insertions(+), 7 deletions(-) diff --git a/context/knowledge/gotchas/pty-terminal.md b/context/knowledge/gotchas/pty-terminal.md index 160b6a1a..d6948aac 100644 --- a/context/knowledge/gotchas/pty-terminal.md +++ b/context/knowledge/gotchas/pty-terminal.md @@ -29,6 +29,7 @@ - **Never resize the agent's real PTY from the preview path.** A Resize RPC per cursor move would SIGWINCH every live agent the user scrolls past, triggering full repaints. Width mismatch is resolved emulation-side only (emulate wide, clip to pane). - **New emulators must default `cursorVisible` to `false`.** Agents send `\e[?25l` early, but after ring buffer wrap or emu rebuild, that sequence is lost. Defaulting to `true` causes a phantom cursor at bottom-left. Also, `lastContentRow` must not extend to cursor position when cursor is hidden. - **`renderLive` must skip `RecentOutput()` when `newBytes == 0`.** The 256KB ring buffer copy on every draw causes typing lag β€” keystroke redraws trigger `Draw()` before PTY echo arrives, so copying is wasted. `emuFedTotal` must only advance when bytes are actually fed to the emulator; advancing it on an empty `raw` silently skips those bytes permanently. +- **The live incremental-feed path must read `raw` and its paired `total` from ONE atomic call (`RecentOutputTailWithTotal`), never `TotalWritten()` and `RecentOutput()` as two separate calls (BUG-075).** `readLoop` is a live, independent goroutine β€” for an actively streaming session (no bind, no resize, nothing but ordinary agent output arriving), bytes can land in the gap between two unsynchronized calls, making `raw` (sampled later) longer than the earlier `totalWritten` accounts for. Slicing `raw[len(raw)-newBytes:]` against that stale `newBytes` then feeds the WRONG suffix: it silently skips the true next bytes (visible as a couple of dropped characters, e.g. "Independent" β†’ "Indepe dent") and instead feeds bytes from further ahead, recording `emuFedTotal` short of what was actually fed β€” the very next frame then re-feeds that same already-fed tail (visible as a short recent phrase duplicating). Both artifacts land together, on a pane that's actively being watched with no bind/resize/rebuild involved at all, which made this easy to mistake for the bind-time reconstruction defects (BUG-068/BUG-073/BUG-074) β€” it is a distinct, plain TOCTOU race in the steady-state live-feed path, not a reconstruction-from-history problem. `agent.Session.RecentOutputTailWithTotal`'s own doc comment already named this exact hazard (for the `/output` HTTP endpoint's cursor) β€” `renderLive`'s live-feed path just hadn't been updated to use it. Fix: fetch `(raw, totalWritten)` together via `sess.RecentOutputTailWithTotal(256*1024)` and recompute `newBytes` from that SAME call, but only when `!emuMissing` β€” the `emuMissing` (fresh-attach) branch is unconditionally `fullReplay` regardless of `raw`/`newBytes` (short-circuited by the `||`) and calls `readLiveRebuildHistory`, which does its own independent ring read, so fetching here too would just be a wasted extra 256KB copy. Regression test: `TestRenderLive_LiveFeedUsesAtomicSnapshotNotStaleTotal` (a `raceAdapter` mock whose `TotalWritten()` always reports an earlier snapshot than `RecentOutputTailWithTotal()`, simulating readLoop advancing in between β€” confirmed to fail on the pre-fix code with exactly the predicted "HELLOLD" garble before being fixed). - **`paintEmu` must cache cells for replay on idle redraws.** tview's `screen.Clear()` defeats tcell's dirty tracking (fills all cells with spaces β†’ every `SetContent` marks cells dirty β†’ full terminal I/O). On no-change redraws, replay cached `[]cachedCell` directly β€” skips 10K+ mutex ops, allocations, and style conversions per frame. Invalidate cache on scroll, reset, or session change. - **`startAgentRedrawLoop` must skip `QueueUpdateDraw` when idle.** Keystroke and resize events trigger their own tview redraws; the 200ms loop only needs to fire when new PTY output arrives (`TotalWritten` changed). - **`spinnerLoop` must only fire redraws for active (non-idle) running tasks.** Idle tasks show a static moon icon, not the spinner. Firing `QueueUpdateDraw` at 100ms when all tasks are idle causes unnecessary full-screen repaints that interfere with tmux hyperlink hover and waste CPU. diff --git a/context/knowledge/index.md b/context/knowledge/index.md index 7ebe13cd..456c63cc 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) | 108 | -| [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) | 64 | +| [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) | 65 | | [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) | 5 | | [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 287948db..94a941ff 100644 --- a/internal/tui/terminal/terminalpane.go +++ b/internal/tui/terminal/terminalpane.go @@ -1629,8 +1629,33 @@ func (tp *TerminalPane) renderLive(screen tcell.Screen, x, y, w, h int, ptyCols, if newBytes > 0 || emuMissing { var raw []byte - if sess != nil { - raw = sess.RecentOutput() + if sess != nil && !emuMissing { + // Atomic (raw, total) snapshot β€” NOT the totalWritten sampled + // above paired with a separate RecentOutput() call. readLoop is + // a live, independent goroutine: for an actively streaming + // session, bytes can arrive in the gap between two + // unsynchronized calls, making raw LONGER than the earlier + // totalWritten accounts for. Slicing raw[len(raw)-newBytes:] + // against that stale newBytes then feeds the WRONG suffix β€” it + // silently skips the true next bytes (dropped characters) and + // instead feeds bytes from further ahead, recording emuFedTotal + // short of what was actually fed; the next frame re-feeds that + // same already-fed tail (a duplicated recent phrase). Reachable + // on an actively-worked pane with no bind/resize/rebuild + // involved at all β€” the resulting garble sits alongside, and is + // easy to mistake for, the bind-time reconstruction defects + // (BUG-068/BUG-073/BUG-074) since it looks similar, but this is + // a plain TOCTOU race in the steady-state live-feed path. Re- + // deriving totalWritten from this SAME atomic read (discarding + // the racy one above) keeps raw and newBytes always consistent. + // + // Skipped when emuMissing: fullReplay is unconditionally true + // in that case (short-circuited below), and the fullReplay + // branch calls readLiveRebuildHistory, which does its own + // independent ring read β€” fetching raw/totalWritten here too + // would just be a wasted extra 256KB copy. + raw, totalWritten = sess.RecentOutputTailWithTotal(256 * 1024) + newBytes = totalWritten - tp.emuFedTotal } // "Full replay" is required when the emulator was just created // (no prior state to preserve) OR the ring wrapped past our last diff --git a/internal/tui/terminal/terminalpane_test.go b/internal/tui/terminal/terminalpane_test.go index 610f1de6..b43d0c03 100644 --- a/internal/tui/terminal/terminalpane_test.go +++ b/internal/tui/terminal/terminalpane_test.go @@ -1057,15 +1057,21 @@ func TestTerminalPane_ResetVTClearsReplayCache(t *testing.T) { } } -// countingAdapter wraps mockAdapter and counts RecentOutput calls. +// countingAdapter wraps mockAdapter and counts the expensive ring-buffer +// copy calls renderLive's live-feed path makes. It instruments +// RecentOutputTailWithTotal β€” the atomic (raw, total) snapshot renderLive +// uses β€” not RecentOutput, which that path no longer calls (see the +// live-streaming-race fix: reading totalWritten and raw via two separate, +// unsynchronized calls let readLoop advance the ring in between for an +// actively streaming session, misaligning the incremental-feed slice). type countingAdapter struct { mockAdapter recentOutputCalls int } -func (c *countingAdapter) RecentOutput() []byte { +func (c *countingAdapter) RecentOutputTailWithTotal(n int) ([]byte, uint64) { c.recentOutputCalls++ - return c.mockAdapter.RecentOutput() + return c.mockAdapter.RecentOutputTailWithTotal(n) } func TestTerminalPane_RenderLiveSkipsCopyWhenIdle(t *testing.T) { @@ -1087,7 +1093,7 @@ func TestTerminalPane_RenderLiveSkipsCopyWhenIdle(t *testing.T) { firstEmu := tp.emu - // Second render with same TotalWritten β€” should NOT call RecentOutput. + // Second render with same TotalWritten β€” should NOT call RecentOutputTailWithTotal. tp.renderLive(screen, 0, 0, 40, 10, 40, 10) testutil.Equal(t, sess.recentOutputCalls, 1) // still 1 if tp.emu != firstEmu { @@ -3136,6 +3142,98 @@ func TestRenderLive_EmuRebuildInvalidatesPaintCache(t *testing.T) { } } +// raceAdapter simulates the live-streaming TOCTOU race renderLive must not +// be vulnerable to: TotalWritten() always reports an EARLIER snapshot +// (staleTotal), while RecentOutput()/RecentOutputTailWithTotal() always +// report a LATER one (freshOutput/freshTotal) β€” exactly what a real Session +// looks like when readLoop advances the ring between two separately-called, +// unsynchronized accessors. A correct renderLive must derive newBytes from +// the SAME atomic call that produced raw (RecentOutputTailWithTotal), never +// from a totalWritten sampled by a separate, earlier TotalWritten() call. +type raceAdapter struct { + alive bool + staleTotal uint64 + freshOutput []byte + freshTotal uint64 +} + +func (r *raceAdapter) WriteInput(p []byte) (int, error) { return len(p), nil } +func (r *raceAdapter) Resize(rows, cols uint16) error { return nil } +func (r *raceAdapter) RecentOutput() []byte { return r.freshOutput } +func (r *raceAdapter) RecentOutputTail(n int) []byte { + if n >= len(r.freshOutput) { + return r.freshOutput + } + return r.freshOutput[len(r.freshOutput)-n:] +} +func (r *raceAdapter) RecentOutputTailWithTotal(n int) ([]byte, uint64) { + return r.RecentOutputTail(n), r.freshTotal +} +func (r *raceAdapter) TotalWritten() uint64 { return r.staleTotal } +func (r *raceAdapter) Alive() bool { return r.alive } +func (r *raceAdapter) PTYSize() (int, int) { return 80, 24 } + +// TestRenderLive_LiveFeedUsesAtomicSnapshotNotStaleTotal is a regression test +// for a live-streaming race distinct from BUG-068/BUG-073/BUG-074 (all of +// which fire only around a bind/resize/rebuild event): renderLive samples +// totalWritten via a separate, EARLIER TotalWritten() call, then later reads +// raw via RecentOutput() β€” readLoop is a live, independent goroutine, so for +// an actively streaming session (no pane switch, no resize, nothing but +// ordinary output arriving) bytes can land in the gap between those two +// calls, making raw longer than the earlier totalWritten accounts for. +// Slicing raw[len(raw)-newBytes:] against that stale newBytes feeds the +// WRONG suffix β€” it silently skips the true next bytes (dropped characters) +// and instead feeds bytes from further ahead, understating emuFedTotal; the +// very next frame then re-feeds that same already-fed tail (a duplicated +// recent phrase) β€” together producing exactly the "duplicated recent text + +// a couple of dropped characters, on an actively-worked pane, no bind +// involved" signature. Fix: re-derive totalWritten from the SAME atomic +// RecentOutputTailWithTotal call that produces raw. +func TestRenderLive_LiveFeedUsesAtomicSnapshotNotStaleTotal(t *testing.T) { + tp := NewTerminalPane() + sess := &raceAdapter{alive: true, staleTotal: 5, freshOutput: []byte("HELLO"), freshTotal: 5} + tp.SetSession(sess) + + screen := tcell.NewSimulationScreen("UTF-8") + if err := screen.Init(); err != nil { + t.Fatalf("screen.Init: %v", err) + } + defer screen.Fini() + screen.SetSize(80, 24) + tp.renderLive(screen, 0, 0, 80, 10, 80, 10) + + // Simulate the race: by the time TotalWritten() was sampled, only "WO" + // (2 of the next 5 bytes) had landed β€” but by the time RecentOutput() + // (or, post-fix, the atomic RecentOutputTailWithTotal) is actually + // called, all 5 new bytes ("WORLD") are already in the ring. + sess.staleTotal = 7 // 5 + 2: what a separately-called TotalWritten() would have seen + sess.freshOutput = []byte("HELLOWORLD") + sess.freshTotal = 10 // the true total by the time output is read + + screen2 := tcell.NewSimulationScreen("UTF-8") + if err := screen2.Init(); err != nil { + t.Fatalf("screen2.Init: %v", err) + } + defer screen2.Fini() + screen2.SetSize(80, 24) + tp.renderLive(screen2, 0, 0, 80, 10, 80, 10) + + if tp.emuFedTotal != 10 { + t.Errorf("emuFedTotal = %d, want 10 (fully caught up to the TRUE total, not the stale TotalWritten() sample)", tp.emuFedTotal) + } + if s, _, _ := screen2.Get(0, 0); s != "H" { + t.Fatalf("row corrupted at (0,0): got %q, want \"H\" (start of HELLOWORLD)", s) + } + got := "" + for col := 0; col < len("HELLOWORLD"); col++ { + s, _, _ := screen2.Get(col, 0) + got += s + } + if got != "HELLOWORLD" { + t.Errorf("row 0 = %q, want \"HELLOWORLD\" (no dropped chars, no duplicated tail from the race)", got) + } +} + // TestTerminalPane_ResizePreservesEmulatorState is a regression test for a // bug where a PTY dimension change (a real terminal resize, the Ctrl+Z // fullscreen toggle, a split/zoom layout change β€” anything that alters the