fix(session): answer terminal status queries from the embedded emulator - #114
Merged
Conversation
An application inside the PTY that asks the terminal a question got no answer: vt100 implements no query at all, so `ESC [ 6 n` (cursor position report) landed in `unhandled_csi` and died there. The asker then blocked until its own timeout — atuin's history search fails outright on the first Up arrow with "The cursor position could not be read within a normal duration", which is exactly what issue #113 reports, and works over plain `ssh` because the real terminal replies. Collect the answers in the parser's callbacks and write them back into the PTY from `Session::drain`, so every session answers whether or not its tab is the visible one — unlike a clipboard write there is nothing for the frame to decide. Answered: DSR 6 (from our own grid, which mirrors the remote screen), DSR 5, and DA1, the last of which also ends the two-second stall every crossterm TUI took at startup probing for the kitty keyboard protocol. Private queries we don't actually speak stay unanswered, since silence is what tells the caller they are unsupported. The end-to-end test drives the whole loop through a real PTY: the child asks, reads six bytes back, and prints what it got. It fails without the write. Closes #113 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial review of the previous commit turned up two real defects in it. The cursor position report could name a column the terminal does not have. vt100 parks the cursor one past the right margin once a character lands in the last column, resolving the pending wrap only when the next one arrives (`col_inc` is unclamped, vt100-0.16.2 grid.rs), so `cursor_position()` returns `cols` on a `cols`-wide grid and we reported `cols + 1`. That is exactly the state a prompt filling the line leaves behind — the moment the #113 reporter presses Up — and whoever measures the room left from it underflows. Clamp to the screen size, and say out loud that origin mode stays wrong because vt100 handles DECOM itself and exposes no accessor. The answers also went out unthrottled through a blocking `write_all` on the PTY master, from the single-threaded frame loop. The remote picks how often it asks; one that asks in a loop and never reads its own stdin makes `ssh` stop reading ours, and ~32 KiB of replies then park every tab, input and rendering included — reachable in under a second at the old rate. A token bucket over reply bytes puts the sustained rate below what a person typing already writes into the same PTY, and an over-budget batch is dropped whole: half an escape sequence in the remote's input is worse than an unanswered query. Also swap the reply after the auto-typed secret. Both write to the same PTY, and a reply queued ahead of the password would be read as part of it by whatever asks for a line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`dd bs=1 count=6` was right only because the cursor happened to sit at the home position. A prelude that prints anything first makes the answer longer, and the fixed count then truncates it and leaves the tail in the input queue — the child reads a wrong position instead of the test failing. Read with `-icanon min 0 time 5` and a generous count instead: whatever arrived within half a second, no guess about its length. That makes the right margin testable end to end, which is where the review found the real bug: 80 characters on an 80-column grid leave vt100's cursor in pending wrap at column 80, and the child must be told `1;80R`, not the `1;81R` it got before the clamp. Also assert that discarding a background tab's clipboard write leaves its query answers alone. Both come out of the same `PtyCallbacks`, and gating answers on visibility the way the clipboard is gated would quietly bring #113 back for every tab the user is not looking at. The queue-cap assertion checked an arithmetic accident (1024 divides by the 4-byte DSR 5 answer); assert the bound it actually means. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured, because the token bucket in 2a34f69 turned out to only postpone the freeze rather than remove it. Same hostile shape both times — a child in raw mode emitting `ESC [ 5 n` in a loop and never reading its own stdin, which is what a remote in a query loop does to the local `ssh` that feeds us: before drain() stopped returning at t≈76-80s and never came back (4096 burst + 256 B/s reaches the master's ~20 KiB limit at t≈64s, so the arithmetic matches the observation) after 9401 drains over 200s, worst single drain 98ms, ran to completion A control with the identical loop emitting plain text instead of queries completed in both builds, so the reply write was the only difference. `write_all` on the PTY master blocks once the child stops reading, and `Session::drain` runs for every session on the single frame loop — so that block parked every tab, input and rendering included, with no timeout and no recovery. The master fd is `dup`ed for the reader, so O_NONBLOCK is not available per-fd; the write moves to its own thread behind a bounded queue instead. One thread keeps keystrokes, the auto-typed secret, pastes and query answers in order, and a queue that fills means the far end is gone, which is when dropping is the honest outcome. The thread is not joined on drop: waiting for a blocked write would just move the freeze into session teardown. The token bucket stays, with its claim corrected. It no longer guards against the freeze — it keeps a query flood from filling the shared write queue and dropping the user's keystrokes instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What
SSHub emulates the terminal for its embedded session with the
vt100crate, which implements no terminal query sequences at all. An application inside the PTY that asks the terminal something got no answer and blocked until its own timeout:ESC [ 6 n(DSR, cursor position report) landed inCallbacks::unhandled_csi, whose default impl drops it. That is issue #113 —atuin's history search dies on the first Up arrow withError: The cursor position could not be read within a normal duration(crossterm gives the query 2 s,crossterm/src/cursor/sys/unix.rs), and works over plainsshbecause the real terminal replies.The parser callbacks now collect answers and
Session::drainwrites them back into the PTY — for every session, visible or not: unlike a clipboard write there is nothing for the frame to decide, an application waiting on a reply blocks whether or not its tab is on screen.Answered:
CSI 6 n— DSR, cursor positionCSI <row>;<col> Rfrom our own grid, 1-basedCSI 5 n— DSR, device statusCSI 0 n5nto be sure something replies (e.g.ratatui-image's picker) otherwise hang for their whole timeoutCSI c/CSI 0 c— DA1CSI ? 1 ; 2 csupports_keyboard_enhancementwritesESC[?u ESC[cand waits for eitherDeliberately not answered: private-prefix queries we don't actually speak —
CSI ? u(kitty keyboard protocol),CSI > c(DA2),CSI ? 6 n(DECXCPR). Silence is what tells a caller they are unsupported, and it is what makes crossterm's detection come out right once DA1 arrives. Nothing dangerous became answerable: vt100 0.16.2 has no DCS callback at all (DECRQSS, XTGETTCAP unanswerable), ENQ lands in the default no-opunhandled_control, and OSC colour queries inunhandled_osc, which we don't implement. The OSC 52 read refusal is untouched. No answer contains CR or LF.Two hardening details, both from the adversarial review of the first commit (second commit):
The report is clamped to the screen size. vt100 parks the cursor one column past the right margin once a character lands in the last column, resolving the pending wrap only when the next one arrives (
col_incis unclamped) — socursor_position()returnscolson acols-wide grid and the report namedcols + 1. That is exactly the state a prompt filling the line leaves behind, i.e. the moment the reporter presses Up, and code measuring the room left from it underflows. Origin mode stays wrong and says so in a comment: vt100 handles DECOM itself and exposes no accessor.PTY writes moved off the frame loop.
write_allon the master blocks once the child stops reading, andSession::drainruns for every session on the single frame loop — so a remote in a query loop that never reads its own stdin parked every tab, input and rendering included, with no timeout and no recovery. Measured with a child in raw mode emittingESC [ 5 nin a loop, against a control running the identical loop with plain text:drain()The control completed in both builds, so the reply write was the only difference. The master fd is
duped for the reader, soO_NONBLOCKis not available per-fd; the write goes to its own thread behind a bounded queue instead. One thread keeps keystrokes, the auto-typed secret, pastes and query answers in order, and a queue that fills means nothing is reaching the remote anyway. The thread is deliberately not joined on drop — waiting for a blocked write would move the freeze into session teardown.The answers are rate-limited (token bucket over reply bytes, burst 4 KiB, 256 B/s sustained). This does not guard the freeze above — measured, it only moved the wedge from under a second to ~80 s, which is what sent the write to its own thread. What it does is keep a query flood from filling the shared write queue and dropping the user's keystrokes instead. An over-budget batch is dropped whole, because half an escape sequence in the remote's input is worse than an unanswered query. Also the reply now goes out after the auto-typed secret — both write to the same PTY, and a reply queued ahead of the password would be read as part of it.
How it was tested
ESC[6n, reads six bytes back and prints what it got (the_pty_child_receives_a_cursor_position_report). Verified it fails without the write —child never got its cursor position back, tail: "\n".cursor_position().Session::drainagainst a hostile child (table above); the probe is not committed — the per-frame behaviour it measures needs minutes, which does not belong in the suite.just test— 1180 + 84 + 79 + 1 passing, 0 failed.cargo fmt --checkandcargo clippy --all-targetsclean.Closes #113
Written by Claude Opus 5 (Claude Code) on behalf of the maintainer.