fix(terminal): keep the output a pane capture could not contain - #436
Conversation
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>
fb1b526 to
c9515b1
Compare
|
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 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. On the Mark it ready and it goes into the queue. |
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
batchTerminalWritequeues live events while_isLoadingBufferis true, and_finishBufferLoaddiscards that queue unless the caller passesflushQueued. The reasoning in its doc comment is sound for one case only: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-paneran. Everything printed afterwards is queued and then dropped, and nothing schedules a re-fetch to recover it:_onSessionNeedsRefreshis 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=1returns 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=returnsbyteHistory + 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.
sourcereadsmux-visibleormux-full-historyfor a capture, andhistoryfor 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_finishBufferLoadtakes asincecutoff. A capture load passes the response's arrival time. The pre-capture events then stay dropped and only the tail replays. Withoutsince, a flush would duplicate everything the capture already holds.src/web/public/terminal-ui.js—chunkedTerminalWritetakes 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 inselectSessionnever ran on the path that matters.src/web/public/terminal-ui.js—_beginBufferLoadkeeps the queue when one load re-enters it.selectSessionopens the load before its fetch andchunkedTerminalWriteopens 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_bufferLoadFinishOptshelper decides the policy from a response'ssource, and all four paths that fetch a terminal buffer and write it go through it:selectSession,_onSessionNeedsRefresh,_onSessionClearTerminaland_maybeRefetchFullHistory. They previously differed, and onlyselectSessiongot the fix._onSessionNeedsRefreshis 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 ofselectSessionruns 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 readsw.data.lengthto match the queue's new entry shape.selectSessiondeliberately stays on discard. It runs before the fetch, so its queue holds only events the capture that follows already contains.sincecutoff and the re-entry rule intest/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-syntaxnpm run check:lockfilenpm test, green at 7014 tests across 369 filesThe baseline on
masteris 7009 tests, so that is the added tests and no regressions.test/capture-load-window.browser.test.tsdrives the real client in chromium. It injects one live event from inside the response's ownjson()call, which is the one place guaranteed to land after the headers and before the chunked write. Run before and after the change:masterThe 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=1path it is conservative: events between the capture and the response are in no payload and still go missing, which is the server's ownpreparephase plus one network leg.Closing that last gap means estimating the capture instant from the
Server-Timingheader the route already emits, andselectSessionalready 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
_finishBufferLoadcall inselectSession, and its unit tests passed. The browser test above is what showed it never ran, becausechunkedTerminalWritehad already ended the load. The two extra changes interminal-ui.jscame 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_modulesneedsnode scripts/prepare-test-vendor.mjsfirst. Without the vendored xterm bundles the page never definesTerminal, and the test fails on its wait forapp.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
selectSessionbut not the same lines, and merge cleanly in either order.