Skip to content

fix(terminal): keep the output a pane capture could not contain - #436

Open
irisitymichaelgrundberg wants to merge 1 commit into
Ark0N:masterfrom
irisitymichaelgrundberg:fix/replay-output-that-arrived-after-the-capture
Open

irisitymichaelgrundberg wants to merge 1 commit into
Ark0N:masterfrom
irisitymichaelgrundberg:fix/replay-output-that-arrived-after-the-capture

Conversation

@irisitymichaelgrundberg

@irisitymichaelgrundberg irisitymichaelgrundberg commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Live terminal events are queued while a buffer load runs, and the load discards that queue when it ends. That is right for the server's accumulated byte history and wrong for a tmux pane capture, which is current only up to capture time. Output emitted during the rest of the load was dropped with nothing to recover it.

The problem

batchTerminalWrite queues live events while _isLoadingBuffer is true, and _finishBufferLoad discards that queue unless the caller passes flushQueued. The reasoning in its doc comment is sound for one case only:

For an established session the loaded buffer from the API is the source of truth up to the response timestamp; SSE events queued during the fetch+write overlap already appear in that buffer.

That holds for the byte history, which the route keeps appending to right up to the moment it serializes the response. A pane capture is a photograph, current only as of the instant capture-pane ran. Everything printed afterwards is queued and then dropped, and nothing schedules a re-fetch to recover it: _onSessionNeedsRefresh is wired only to the 128KB overflow path. The CLI's next partial redraw then lands on a frame the terminal never received.

How much is lost depends on which capture the route served, and the two differ:

  • ?full=1 returns the capture alone, with no byte history in front of it. Nothing in the payload covers the gap, so the loss runs from the capture to the end of the chunked write.
  • ?tail= returns byteHistory + clear + capture, and the route reads that history after the capture. The history therefore covers up to the response, and the loss runs from the response to the end of the chunked write.

The chunked write dominates either way. It spreads across rAF frames after a fetch that measured 25–90ms against live sessions here, and the load has already sat through a 400ms redraw settle whenever the dimensions changed.

The response already distinguishes the sources. source reads mux-visible or mux-full-history for a capture, and history for the byte stream, so the client has what it needs to tell them apart.

Changes

  • src/web/public/terminal-ui.js — queue entries carry their arrival time, and _finishBufferLoad takes a since cutoff. A capture load passes the response's arrival time. The pre-capture events then stay dropped and only the tail replays. Without since, a flush would duplicate everything the capture already holds.
  • src/web/public/terminal-ui.jschunkedTerminalWrite takes the finish options and applies them at its own three finish sites. It is what ends the load for every non-empty buffer, so a policy set only in selectSession never ran on the path that matters.
  • src/web/public/terminal-ui.js_beginBufferLoad keeps the queue when one load re-enters it. selectSession opens the load before its fetch and chunkedTerminalWrite opens it again under the same owner, and the reset on that second call discarded the entire fetch window before anything could replay it. A genuinely new load still starts empty.
  • src/web/public/app.js — one _bufferLoadFinishOpts helper decides the policy from a response's source, and all four paths that fetch a terminal buffer and write it go through it: selectSession, _onSessionNeedsRefresh, _onSessionClearTerminal and _maybeRefetchFullHistory. They previously differed, and only selectSession got the fix. _onSessionNeedsRefresh is the one that stings — it exists to restore output the client already dropped once under backpressure, and it was dropping more output while performing that recovery. The call that remains at the end of selectSession runs only when the write was skipped, which is where COD-144 lives: a new session's first prompt predates the response, so an empty paint replays its queue whole rather than from the header timestamp. The queued-byte total reads w.data.length to match the queue's new entry shape.
  • The cache-hit write inside selectSession deliberately stays on discard. It runs before the fetch, so its queue holds only events the capture that follows already contains.
  • Tests: the since cutoff and the re-entry rule in test/terminal-buffer-flush.test.ts, plus a new browser suite.

Verification

Every CI gate passes locally:

  • npm run typecheck, npm run lint, npm run format:check, npm run check:frontend-syntax
  • npm run check:lockfile
  • npm test, green at 7014 tests across 369 files

The baseline on master is 7009 tests, so that is the added tests and no regressions.

test/capture-load-window.browser.test.ts drives the real client in chromium. It injects one live event from inside the response's own json() call, which is the one place guaranteed to land after the headers and before the chunked write. Run before and after the change:

buffer from a pane capture buffer from the byte history
on master fails: the event is dropped passes: the event is dropped
with this change passes: the event reaches the terminal passes: the event is dropped

The second column matters as much as the first. The byte history already contains everything up to the response, so replaying the queue on top of it would duplicate output, most visibly Ink's cursor-up redraws. That discard had to survive.

What this does not cover

The cutoff is headersReceivedAt, because that is the only clock the client holds. It is exactly right for the ?tail= path, where the byte history covers everything up to the response. On the ?full=1 path it is conservative: events between the capture and the response are in no payload and still go missing, which is the server's own prepare phase plus one network leg.

Closing that last gap means estimating the capture instant from the Server-Timing header the route already emits, and selectSession already reads that header. I did not do it. Halving an unmeasured round trip to date-stamp a frame felt like more guesswork than the remaining milliseconds justify, and a cutoff that guesses too early duplicates output instead of dropping it. Happy to add it if you would rather have the whole window.

Worth saying plainly: the first version of this change put the policy only at the _finishBufferLoad call in selectSession, and its unit tests passed. The browser test above is what showed it never ran, because chunkedTerminalWrite had already ended the load. The two extra changes in terminal-ui.js came out of that. A later review pass found the same fix still covered only one of the four load paths, which is where the shared helper came from.

Running the browser suite in a tree that borrows another checkout's node_modules needs node scripts/prepare-test-vendor.mjs first. Without the vendored xterm bundles the page never defines Terminal, and the test fails on its wait for app.terminal.

Related

Follows #395, #396 and #397, which fixed the ways the replayed frame itself could disagree with the terminal. This one is about output that no frame contained.

Independent of #435, which reports the height a capture was taken at. The two touch selectSession but not the same lines, and merge cleanly in either order.

Live terminal events are queued while a buffer load runs, and the load discards
that queue when it ends. That is right when the loaded buffer is the server's
accumulated byte history. The route appends to that history right up to the
moment it serializes the response, so a queued event already appears in it and
replaying it would duplicate output, most visibly Ink's cursor-up redraws.

A tmux pane capture is a photograph, current only as of the instant
`capture-pane` ran. Output printed afterwards was queued and then dropped, and
nothing scheduled a re-fetch to recover it: `_onSessionNeedsRefresh` is wired
only to the 128KB overflow path. The CLI's next partial redraw then landed on a
frame the terminal never received.

How much went missing depended on which capture the route served. A `?full=1`
load returns the capture alone, with no history in front of it, so it lost
everything from the capture to the end of the chunked write. A `?tail=` load
returns history, a clear, and then the capture, and the route reads that history
after the capture, so it lost everything from the response to the end of that
write. The chunked write dominates either way. An agent CLI hides the loss on
its next full redraw; a shell session does not, because its output is linear and
nothing repaints it.

Queue entries now carry their arrival time, and `_finishBufferLoad` takes a
`since` cutoff, so a capture load replays exactly the tail that arrived after
the response headers. The earlier events stay dropped, because a payload that
carries history does hold those.

All four paths that fetch a terminal buffer and write it now decide this the
same way, through one `_bufferLoadFinishOpts` helper, so they cannot drift
apart: `selectSession`, `_onSessionNeedsRefresh`, `_onSessionClearTerminal` and
`_maybeRefetchFullHistory`. The second of those is the one that stings. It
exists to restore output the client already dropped once under backpressure, and
it was dropping more output while performing that recovery. The cache-hit write
inside `selectSession` stays on discard deliberately: it runs before the fetch,
so its queue holds only events the capture that follows already contains.

Two further things had to change for that tail to still exist when the load
ends, and a browser test is what found both. `chunkedTerminalWrite` is what ends
the load for every non-empty buffer, so the flush policy travels to its own
finish calls; the call in `selectSession` runs only when the write was skipped.
`_beginBufferLoad` no longer empties the queue when one load re-enters it, which
it does on every write, because that reset discarded the whole fetch window
before anything could replay it.

The response already distinguishes the sources. `source` reads `mux-visible` or
`mux-full-history` for a capture and `history` for the byte stream.

Follows Ark0N#395, Ark0N#396 and Ark0N#397, which fixed the ways the replayed frame itself
could disagree with the terminal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@irisitymichaelgrundberg
irisitymichaelgrundberg force-pushed the fix/replay-output-that-arrived-after-the-capture branch from fb1b526 to c9515b1 Compare September 15, 2026 16:15
@irisitymichaelgrundberg
irisitymichaelgrundberg marked this pull request as ready for review September 15, 2026 16:23
@Ark0N

Ark0N commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Same as on #435: both jobs are green on this head, and the reviewer here skips drafts, so this is currently outside the review queue rather than at the back of it. If the draft flag marks something unfinished, tell me what and I will wait. Otherwise please mark it ready for review.

I am putting 1.29.2 together and this is the kind of change I would want in it, but not unreviewed.

Worth knowing: master moved an hour ago (1.29.1, carrying #376's auto-naming, which touches app.js). This still merges clean over it and clean alongside the other open PRs, in any order, so no rebase needed. I verified your "independent of #435, merge cleanly in either order" note directly and it holds.

Two remarks ahead of the actual review, neither of them blocking:

The paragraph about your first version passing its unit tests while never running is the most useful thing in this description, and I would rather you kept writing them than trimmed them out. chunkedTerminalWrite ending the load before the policy in selectSession could apply is exactly the failure mode that makes a green suite worthless, and "a later review pass found the same fix still covered only one of the four load paths" is why the shared helper is the right shape rather than a fourth copy. That is the invariant worth pinning, and it is close to something CLAUDE.md already says about single resolvers for call sites that must not disagree.

On the ?full=1 window you deliberately left open: I agree with the call. A cutoff that guesses too early duplicates output rather than dropping it, and duplicated Ink redraws are more visible and more confusing than a few missing milliseconds. Leave it. If it ever turns out to matter, the Server-Timing route is there and the PR description already explains it better than a comment would.

Mark it ready and it goes into the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants