Uh oh!
There was an error while loading. Please reload this page.
fix(runtime): evict oldest queued PTY data instead of pausing the source - #1873
Merged
Conversation
PtyScreenCollector paused the node-pty read stream when the parse queue exceeded its high-water mark. If the child exited while the stream was still paused, node-pty destroyed the socket after 200ms and the unread tail (including the final frame) was lost before 'exit' fired, so the final snapshot could be empty (CI: shell-run-manager 'drains the final frame after parser backpressure'). Backpressure now evicts the oldest queued data from a newest-first bounded queue, the source is never paused, and the final frame always reaches the terminal. Eviction marks the output truncated, matching scrollback eviction semantics.
Queue eviction removes bytes from the middle of the input stream. The dropped chunk may have carried the terminator of an escape sequence the xterm parser is still waiting on (an unterminated OSC/CSI/DCS), so the retained suffix would be interpreted against stale parser state and could be swallowed entirely — the newest frame again missing from the final snapshot. Reset the terminal through RIS (ESC c) before the next retained write: its full reset returns the parser to ground state and empties the buffer, and the old screen is discarded with it since it was built from incomplete input. historyTruncated stays set (eviction itself is truncation). Regression test drives an unterminated OSC into the terminal, evicts the chunk carrying its BEL terminator, and asserts the newest frame survives.
Eviction removed an entry from the pending array and decremented the byte counter, but the parse closure already chained on the sequence promise still referenced the entry — and through it the full payload string — until the chain reached it. With the source never paused, a slow parse write stalls the chain while fresh chunks keep arriving, so real memory could grow far beyond the queue budget even though the counter stayed bounded (synchronous flood of 100 x 2 MiB chunks reached ~209 MiB heap vs. a 1 MiB budget). The closure now reads entry.data and eviction clears it, releasing the payload immediately; only the small entry object remains until the chain advances.
Astro-Han
marked this pull request as ready for review
August 2, 2026 04:51
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the intermittent CI failure in
shell-run-manager.test.js"drains the final frame after parser backpressure and drops an evicted wrapped prefix" (AssertionError: /FINAL-DRAIN/with empty output). Thetestjob failed onmaintwice today (runs 30730791871, 30729759095); both failures were this test.Root cause:
PtyScreenCollectorpaused the node-pty read stream when the parse queue exceeded its high-water mark (1 MiB). If the child process exited while the stream was still paused, node-pty destroyed the socket after 200 ms (DESTROY_SOCKET_TIMEOUT_MS) and the unread tail of the output — including the final frame — was lost before the exit event fired. The final snapshot then saw only the partially-parsed output stream, whose rows are all soft-wrapped continuations;sanitizeBufferevicts the entire wrapped prefix, producing empty output withtruncated: true. Status stayedcompletedbecause the lost bytes were never delivered, which is why only the text assertion failed.Fix: backpressure now evicts the oldest queued data from a newest-first bounded queue instead of pausing the source. The source is never paused, so a fast-exiting child can never strand unread bytes behind node-pty's socket-destroy fence, and the final frame always reaches the terminal. This also matches the terminal's own newest-first semantics: scrollback eviction, wrapped-prefix eviction, and queue eviction are the same idea. Eviction marks the output
truncated.Design reasoning (first principles): pausing couples flow control (bounded memory) with delivery (complete data), and pause semantics conflict with process exit (exit means no more data will come, but a paused stream cannot deliver the final bytes within node-pty's destroy fence). Decoupling them — never pause the source, bound the parse queue, evict oldest — removes the race entirely.
Verification
PtyScreenCollector"keeps the newest frame and evicts the oldest queued data when the parse queue exceeds its budget": red on the old code (empty output), green on the new.node --test "dist/**/*.test.js"inpackages/runtime: 2693 tests, 0 fail.npm run lint,npm run format:check,npm run typecheck: clean.Note: the CI failure could not be reproduced locally on macOS (timing differences), so the red-to-green signal for the integration test comes from CI; the new collector unit test locks the eviction semantics locally.