Skip to content

Isolate startup probe parser and drain responses#869

Open
tyx3211 wants to merge 2 commits into
microsoft:mainfrom
tyx3211:codex/isolated-terminal-probe-parser
Open

Isolate startup probe parser and drain responses#869
tyx3211 wants to merge 2 commits into
microsoft:mainfrom
tyx3211:codex/isolated-terminal-probe-parser

Conversation

@tyx3211
Copy link
Copy Markdown

@tyx3211 tyx3211 commented Jun 4, 2026

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 local vt::Parser, drains follow-up probe responses for a short quiet window after DA, and run() 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::Parser instance:

  1. setup_terminal() read OSC color replies, CPR, and DA through the main parser.
  2. It treated a DA response whose CSI final byte is c as the end marker for startup responses.
  3. In the captured failure, another OSC color response began after DA and the read ended with a partial OSC/ST sequence.
  4. The main loop then reused that parser while it was still in OscEsc/Osc.
  5. On Unix, the synthetic resize (ESC[8;rows;cols t) was parsed as OSC payload instead of Input::Resize.
  6. Tui stayed at Size { width: 0, height: 0 }, and the first render panicked in chunks_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 between ESC and \. The fix therefore treats DA as the start of a bounded wind-down phase rather than proof that startup probing is complete.

Changes

  • Move creation of the main-loop vt::Parser and input::Parser until after setup_terminal().
  • Change setup_terminal() so it creates and owns its own probe vt::Parser.
  • Add a bounded probe drain after DA: finish immediately when the useful probe results are complete and the parser is back at ground, finish after a short quiet window when DA has arrived and the parser is back at ground, or stop at a hard deadline.
  • Expose vt::Parser::is_ground() so startup probing can avoid discarding a probe parser while it is still inside an unfinished VT sequence.
  • Count unique color response indices before applying the probed palette.
  • Clear the probe OSC buffer after every complete OSC response, including unrecognized or malformed responses, so a stale partial response cannot pollute later color replies.
  • Add parser behavior tests documenting that a partial OSC parser state does not emit the synthetic resize, while a fresh parser parses the same bytes as Input::Resize.
  • Add probe-state tests for the fast path, quiet-window path, pending-sequence wait, unique color counting, and OSC buffer cleanup after an invalid complete response.

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.

state = collecting
hard_deadline = now + hard_timeout

while true:
    # These checks observe the parser state after the previous read chunk was
    # fully parsed. A transient ground state inside a chunk is not a handoff point.
    if probe_parser.is_ground() and useful_probe_results_are_complete:
        state = finished
        break

    if probe_parser.is_ground() and DA_seen and now >= quiet_deadline:
        state = finished
        break

    if now >= hard_deadline:
        state = finished
        break

    read one stdin chunk using a timeout derived from:
        - quiet_deadline, when the probe parser is at ground
        - hard_deadline, when the probe parser is inside an unfinished VT sequence

    parse the entire chunk through the probe parser
    update collected probe results from complete OSC / CPR / DA tokens

    if DA was just seen:
        state = draining
        quiet_deadline = now + quiet_timeout

    if DA_seen and probe/control activity was observed:
        quiet_deadline = now + quiet_timeout

    if DA_seen and the chunk left the probe parser non-ground:
        keep reading through the same probe parser until it reaches ground,
        or until hard_deadline forces startup to continue

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 through vt::Parser::parse("") so parser states that intentionally complete on timeout, such as a lone ESC, can settle before setup_terminal() decides whether the parser is back at ground.

Validation

cargo fmt --check

Passed with exit code 0. Stable rustfmt printed warnings for nightly-only rustfmt options in the repository config.

cargo test -p edit

Passed:

  • library tests: 46 passed, 1 ignored
  • bin tests: 6 passed
  • doctests: 2 passed
CARGO_PROFILE_RELEASE_STRIP=false CARGO_PROFILE_RELEASE_DEBUG=full cargo build --release -p edit

Passed.

Diagnostic context

The issue was reproduced over Windows Terminal + PowerShell + OpenSSH SSH into Linux on both v2.0.0 and current main (737450cef9ee40030221d1ddc4e91a5e67173306). A local Codex-authored log-only diagnostic patch on current main captured startup response fragments where a DA response (CSI ? 61;... c) was followed by another OSC color response ending at a partial ESC. 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. The main release-with-symbols core showed:

  • tui.size == Size { width: 0, height: 0 }
  • framebuffer buffers were also 0x0
  • the shared vt_parser was still in State::Osc
  • the last parsed CSI was the DA response

The stripped v2.0.0 release binary manifested the same panic as SIGILL at ud2; release-with-symbols builds (CARGO_PROFILE_RELEASE_STRIP=false CARGO_PROFILE_RELEASE_DEBUG=full cargo build --release -p edit) showed the panic path through Framebuffer::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 current main payload. The same lifecycle isolation applies to both the reproducing v2.0.0 code path and current main.

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 as 0c0c/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 t sequence so the input parser can produce Input::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.

@tyx3211
Copy link
Copy Markdown
Author

tyx3211 commented Jun 4, 2026

@microsoft-github-policy-service agree

@tyx3211 tyx3211 force-pushed the codex/isolated-terminal-probe-parser branch from 74fc8b4 to c2b9ec1 Compare June 4, 2026 07:38
@tyx3211 tyx3211 changed the title Isolate terminal startup probe parser from main input parser Isolate terminal startup probing with bounded response drain Jun 4, 2026
@tyx3211 tyx3211 changed the title Isolate terminal startup probing with bounded response drain Isolate startup probe parser and drain responses Jun 4, 2026
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.

Startup can panic when terminal probe parser state leaks into main input parsing

1 participant