Don't fail read_line when the cursor-position query times out at prompt init - #1202
Conversation
…pt init `initialize_prompt_position` asks the terminal where the cursor is before painting a prompt. crossterm waits a fixed 2s for the reply and then fails; a terminal busy repainting, a multiplexer briefly holding the reply, or a slow remote link is enough to trip it. That is a transient condition, not a broken terminal -- but this was the only query site that still propagated the error, so it aborted the whole `read_line`, and a host application typically treats that as fatal (reubeno/brush#1329: one late reply exits the interactive shell). Every other `cursor_position()` call in the painter already degrades to its last-known row. Do the same here: print a newline so the prompt at least starts at column 0 on a row of its own, keep the row as `Stale` so the next paint's drift check asks again (and itself tolerates no answer), and carry on. With no row known at all, assume the bottom of the screen: the drift check only re-anchors when the cursor turns out to be *above* the cached row, so a guess that errs high self-heals on the next answered query, whereas row 0 would never be corrected. The query is split out of `initialize_prompt_position` into `anchor_prompt` so it can be unit-tested without a tty; the test writers already return an error from `cursor_position()`, which is exactly the case being handled. Verified against brush (nushell/reedline 0.49.0 + this change, host with no retry of its own): a pty harness that withholds the reply to one query no longer ends the session.
| let row = match self.prompt_start_row { | ||
| PromptStartRow::Verified(row) | PromptStartRow::Stale(row) => row, | ||
| PromptStartRow::Unverified => self.screen_height().saturating_sub(1), | ||
| }; |
There was a problem hiding this comment.
| let row = match self.prompt_start_row { | |
| PromptStartRow::Verified(row) | PromptStartRow::Stale(row) => row, | |
| PromptStartRow::Unverified => self.screen_height().saturating_sub(1), | |
| }; | |
| self.prompt_start_row = | |
| PromptStartRow::Stale(self.screen_height().saturating_sub(1)); | |
| return Ok(()); |
There was a problem hiding this comment.
I think reusing the old row is wrong here.
We are essentially substituting the row measurement, so what matters is which side of the
truth the substitute can land on.
The old row can land on either side, and in the common REPL case it lands below the true
cursor, which the drift check cannot repair since it only re-anchors upward.
The anchor is what clear_from_anchor erases down from, so that paint wipes the output
sitting above the prompt.
The bottom of the screen can only land on the repairable side, since no row is greater.
Your own reasoning in the comment above covers this, it just wants to apply to this branch
too: self.screen_height().saturating_sub(1) unconditionally.
There was a problem hiding this comment.
You're right, and the mismatch is that I only applied the "guess must be able to self-heal" invariant to the Unverified arm. Reusing the last-known row breaks it in the other two: anchor_prompt has already printed \r\n by the time the fallback runs, so the true cursor is below the cached row in the common REPL case, and that undershoot is the one direction the drift check can't fix. Bottom of the screen dominates in all three arms, not just Unverified.
Applied as 56cb1c2: unconditional Stale(self.screen_height().saturating_sub(1)), comment rewritten around that argument, and the test that asserted the old row was reused now asserts Stale(9) (renamed to ..._assumes_bottom_over_last_known_row). cargo test --lib still 1662 pass with only the pre-existing palette failure; clippy clean.
There was a problem hiding this comment.
Confirmed empirically against a real host. Backported both variants onto 0.49.0 under brush main (which has its own retry now, so the harness withholds every query) and put a VT100 emulator in the loop so the drift check gets the true cursor row rather than a canned 1;1R:
echo BEFORE_42, withhold the pre-prompt query, answer the drift check truthfully, next prompt. Last-known row (as first submitted):BEFORE_42and its command line are erased from screen and scrollback —clear_from_anchorran from the stale row. Bottom row (56cb1c2): both preserved (scrolled, not wiped).- With no query ever answered,
read_linekeeps returning and the shell stays interactive in both variants; 2s per unanswered query as expected.
…unanswered Reusing the last-known row was wrong: `anchor_prompt` has already printed a newline by then, so in the common REPL case the true cursor sits below the cached row. The drift check only re-anchors upward, so an undershooting guess is never repaired, and `clear_from_anchor` then wipes the output above the prompt. The bottom of the screen is the only guess that always lands on the repairable side. Review feedback on nushell#1202.
Summary
initialize_prompt_positionasks the terminal for the cursor position before painting a prompt. crossterm waits a fixed 2s for the reply and then fails; a terminal busy repainting, a multiplexer briefly holding the reply, or a slow remote link is enough to trip it. That's a transient condition, not a broken terminal — but this was the onlycursor_position()call site in the painter that still propagated the error, so it aborted the wholeread_line(), and a host typically treats that as fatal (reubeno/brush#1329: one late reply exits the interactive shell).No public API change; observable behavior changes only in the failure case described.
Before
read_line()returnsErr("The cursor position could not be read within a normal duration")if the terminal doesn't answer the pre-prompt DSR query within 2s. Nothing was consumed from the user's input; the host just loses the editor.After
On no answer: print
\r\nso the prompt at least starts at column 0 on a row of its own, set the row to the bottom of the screen asStaleso the next paint's drift check asks again (and itself tolerates no answer), and carry on.The substitute is always the bottom row, never the last-known one. The drift check only re-anchors when the cursor turns out to be above the cached row, so a guess is only repairable if it errs high. The newline just printed has already moved the cursor past the last-known row in the common REPL case, and
clear_from_anchorerases down from the anchor, so reusing it would wipe the output above the prompt with no way to recover. No row is greater than the bottom, so it is the only guess that always lands on the repairable side.The query is split out into
anchor_promptso it can be unit-tested without a tty; the test writers already return an error fromcursor_position(), which is exactly the case handled. Two tests added.Additional notes
Verified against a real host: brush at
main(which has no retry of its own) built against 0.49.0 + this change — a pty harness that withholds the reply to exactly one query no longer ends the session, where stock 0.49.0 does. Ported cleanly tomain; the only difference isself.stdout.cursor_position()vscursor::position().cargo test --libonmain: 1662 pass, 1 pre-existing failure unrelated to this change (crossterm_defaults_were_the_bright_palette_entries, fails identically on cleanmainwith rustc 1.98).Complementary, not a substitute, for a host-side retry — reubeno/brush#1331 adds one there.