Isolate startup probe parser and drain responses#869
Open
tyx3211 wants to merge 2 commits into
Open
Conversation
Author
|
@microsoft-github-policy-service agree |
74fc8b4 to
c2b9ec1
Compare
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 free
to 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
This isolates the VT parser used for terminal startup probing from the VT parser used by the main input loop, and adds a bounded post-DA drain for startup probe responses.
setup_terminal()now owns a localvt::Parser, drains follow-up probe responses for a short quiet window after DA, andrun()creates the application input parser only after startup probing has completed. This prevents parser state left by startup probing from consuming subsequent application input, including the Unix synthetic resize sequence, while also reducing the chance that a split OSC color response tail is parsed as editor text.Fixes #868.
Reported and reproduced by @tyx3211. Analysis/write-up assisted by Codex.
Root cause
Before this change, startup probing and application input shared the same
vt::Parserinstance:setup_terminal()read OSC color replies, CPR, and DA through the main parser.cas the end marker for startup responses.OscEsc/Osc.ESC[8;rows;cols t) was parsed as OSC payload instead ofInput::Resize.Tuistayed atSize { width: 0, height: 0 }, and the first render panicked inchunks_exact(0).This does not require the terminal to emit an invalid VT sequence. DA is a response token, not a transaction boundary for all earlier startup queries. A legal byte stream may contain DA followed by a later ST-terminated OSC response, and the OS/PTY
read()boundary may split that OSC terminator betweenESCand\. The fix therefore treats DA as the start of a bounded wind-down phase rather than proof that startup probing is complete.Changes
vt::Parserandinput::Parseruntil aftersetup_terminal().setup_terminal()so it creates and owns its own probevt::Parser.vt::Parser::is_ground()so startup probing can avoid discarding a probe parser while it is still inside an unfinished VT sequence.Input::Resize.Probe drain state machine
The startup probe now progresses through a bounded collect/drain flow. The important boundary is the read-chunk boundary:
setup_terminal()does not stop in the middle of parsing a chunk just because DA was seen, or because the parser temporarily returned to ground. Exit checks are made only after the previous stdin chunk has been fully parsed by the probe parser.This means the normal handoff to the main loop requires both a probe completion condition and a clean parser boundary. If all useful probe results have already been collected but the probe parser is still inside
OSC,OscEsc,CSI, or another unfinished VT sequence, the fast path is not taken. Startup continues reading with the probe parser until the sequence closes and the parser returns to ground, or until the hard deadline is reached.Finishing startup probing means applying whatever complete probe results were collected, discarding the probe parser and any unfinished probe state, and only then creating the main-loop
vt::Parser/input::Parser. Timeout reads are still passed throughvt::Parser::parse("")so parser states that intentionally complete on timeout, such as a loneESC, can settle beforesetup_terminal()decides whether the parser is back at ground.Validation
Passed with exit code 0. Stable rustfmt printed warnings for nightly-only rustfmt options in the repository config.
Passed:
Passed.
Diagnostic context
The issue was reproduced over Windows Terminal + PowerShell + OpenSSH SSH into Linux on both
v2.0.0and currentmain(737450cef9ee40030221d1ddc4e91a5e67173306). A local Codex-authored log-only diagnostic patch on currentmaincaptured startup response fragments where a DA response (CSI ? 61;... c) was followed by another OSC color response ending at a partialESC. That temporary instrumentation logged the read stage, byte length, and an escaped representation of each stdin chunk before passing the same bytes to the existing parser; it did not reset parser state, reorder bytes, or include this PR's parser-lifecycle fix. Themainrelease-with-symbols core showed:tui.size == Size { width: 0, height: 0 }0x0vt_parserwas still inState::OscThe stripped
v2.0.0release binary manifested the same panic as SIGILL atud2; release-with-symbols builds (CARGO_PROFILE_RELEASE_STRIP=false CARGO_PROFILE_RELEASE_DEBUG=full cargo build --release -p edit) showed the panic path throughFramebuffer::render()/Tui::render().Separate log-only builds traced
setup_terminal()input without adding this parser-lifecycle fix, preserving the original crash behavior. The linked issue includes the currentmainpayload. The same lifecycle isolation applies to both the reproducingv2.0.0code path and currentmain.Design note
This PR keeps the scope targeted: it fixes the startup parser lifecycle leak and adds bounded probe draining, without redesigning all of terminal setup.
The repair is intentionally not terminal-specific compatibility code. It addresses an edit-side state-machine assumption: startup probing cannot rely on DA ordering to imply that all OSC replies have already arrived. The test payload uses ordinary ST-terminated OSC replies and ordered byte delivery; the hazardous part is handing a non-ground probe parser to the main input loop.
Parser isolation alone prevents the startup probe parser from consuming the first synthetic resize in the main loop, which fixes the panic. However, if setup stopped immediately after DA while the probe parser had already consumed the start of a later OSC reply, the remaining reply tail could be read by the fresh main parser without its
ESC ]context and appear as editor text such as0c0c/0c0c/0c0c. The bounded drain keeps those follow-up bytes in the probe parser for the common delayed-response case.It still treats DA (
CSI ... c) as the signal that startup probing can begin winding down, not as proof that every earlier terminal query has completed. After DA, the probe parser keeps reading for a short quiet window, but only exits that path when the probe parser has returned to ground. That quiet window reads from the same stdin stream as real input, so extremely early typeahead can still be consumed during startup probing. I think that is an acceptable tradeoff for this startup phase: the main input parser has not been created yet, the main editor loop has not rendered the editable UI yet, and ignoring input before the editor is visibly editable matches the user-visible startup boundary. The window is intentionally short, and only probe/control activity can extend it. If the terminal leaves a control sequence unfinished, probing stops at a hard deadline rather than blocking startup indefinitely. In that extreme case a very late response tail may still reach the main loop, but the hard deadline is intentional: startup should not wait forever for a terminal response that may never complete.A more complete terminal setup redesign would be a larger follow-up. It may be worth separating window-size initialization from terminal byte parsing more strongly: the current Unix path gets the real window size from the system, then encodes it as a synthetic
CSI 8 ; rows ; cols tsequence so the input parser can produceInput::Resize. Replacing that with a typed resize event or direct TUI initialization would touch the sys/input/run-loop boundary, so I kept it out of this crash fix.