Repository navigation
Watch the output during peek --wait, not only the screen (#206) - #208
Merged
Merged
Conversation
peek --wait read the plain visible screen every 200 ms, as the Node tool does. A marker printed and scrolled away between two reads was never seen. The wait then ran on to its timeout, or, with no timeout, until the session exited and lastLines happened to include the marker. The wait now holds one read-only peek connection for its whole length. The daemon sends that connection the screen and then every byte of output after it. The wait matches the screen, and the visible text of the output as it arrives: - Escape sequences are dropped, also when a chunk boundary splits one. - Only a pattern's length of text is kept across chunks. The 200 ms screen read stays, so text drawn by cursor movement is still found. The watch's own screen is the first look, so a pattern already on screen costs no more connections than before. On a timeout the wait reads the full scrollback once. A pattern found there was printed before the wait began, and a second line says so. The first line and the exit code are Node's. Decision 0019 records the difference.
…s styles Two review findings on the output watch: - wait_until applied its deadline only to poll. Under output that never pauses, poll reports ready every time, so the read went on past the deadline. The wait then reached neither its screen read nor its timeout. It now returns once the deadline has passed, after at most one more chunk. - The scanner dropped every CSI and joined the text either side, so "END \e[2;1H004" matched "END 004" although the terminal never showed it. Text now joins only across SGR, a charset designation or an escape string. Every other CSI and the single-byte escapes reset the carried text, after checking it for a match. Text drawn that way is left to the screen read. Tests: - A fake daemon that floods the watch: a 1 s wait times out in about 1 s. - A unit test: past its deadline, the watch reads one chunk of a full socket and returns. - Negative scanner cases for cursor moves.
Contributor
Author
|
Independent read GO at exact head Read:
Risks (low):
|
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.
Fixes #206.
pty peek --waitmissed a marker that was printed and scrolled off the screen between two of its 200 ms screen reads. A finite wait then ended only at its bound, and a wait with no bound ended only when the session exited.Cause.
peek_wait(crates/pty-client/src/peek.rs), like Node'scmdPeekWait, only ever looked at snapshots of the visible screen. Output that came and went between two snapshots was never seen.Fix.
The output stream. The wait holds one read-only peek connection for its whole length. The daemon sends it the screen and then every byte of output after it. The wait matches that screen, and the visible text of each output chunk as it arrives.
ESC 8can move the cursor, so they reset the carried text (after checking it for a match).END \e[2;1H004doesn't matchEND 004. Text drawn that way is left to the screen read.The screen reads stay. The 200 ms screen read still runs, so text drawn by cursor movement is still found on screen. If the watch connection ends, the wait carries on with screen reads alone.
Connections. The watch's own screen is the first look, so a pattern already on screen opens no more connections than before.
cli_peek_color_cleanup's fake daemon counts them and still passes.Missed is told apart from never printed. On a timeout the wait reads the full scrollback once. If the pattern is there, it was printed before the wait began, and a second line says so. The first line and exit code 1 are unchanged.
A new
PeekWaitError::Missedvariant carries this. Nothing outside pty matches on the enum, and fractal doesn't use it.Decision 0019 records the difference from Node, and docs/parity.md lists it beside decision 0006.
Measured. The repro is on Linux at main, using the installed
0.13.0-rust+79d526d, whose peek code is identical to main's. The session waits 1 s, printsEND 004, and then prints 200 lines. The command ispty peek --plain --wait 'END 004' -t 6:Tests.
crates/pty/tests/cli_peek_wait.rs.a_marker_scrolled_away_while_waiting_satisfies_the_wait: one write prints the marker and 200 lines, so no screen read can see it. The test also checks that the marker really is off screen and in the scrollback.a_marker_from_before_the_wait_is_reported_as_missed: this case also checks that a pattern that never appeared keeps Node's single line.crates/pty-client/tests/client_peek.rs.peek_wait_sees_a_marker_that_only_the_output_carried: the marker is split across chunks and styles.peek_wait_tells_a_missed_pattern_from_one_that_never_appeared.peek::visible_text_tests. It covers split escapes, an OSC title and a DCS that must not match, and the tail bound.peek_wait_keeps_its_bound_under_output_that_never_pauses(fake daemon flooding the watch): a 1 s wait times out in 1.01 to 1.02 s (3 runs). At 55d0f09 it overran in 2 of 3 runs, one past 10 s and one at 3.9 s; one finished at 1.46 s.peek::output_watch_tests::past_its_deadline_the_watch_reads_one_chunk_and_returnsis the deterministic control. Past the deadline, the watch reads one chunk of a socket holding 93 KB and returns. At 55d0f09 it read all 93,465 bytes.peek::visible_text_tests::text_either_side_of_a_cursor_move_is_not_joinedmatchedEND \e[2;1H004at 55d0f09.peek.rsand only the new behavior tests applied, all four fail:TimedOutafter 5 s;peek --waittests pass unchanged. These are the fake-daemonclient_peektests,cli_peek_color_cleanup, and the conformancepeek_wait.rsport.cargo test --workspace --no-fail-fast, Linux, sandboxed root, at 55d0f09): 2155 passed, 1 failed, 9 ignored. The failure isinteractive::typing_filters_the_list_and_escape_clears_it, which also fails on main in my checkouts: the checkout path fuzzy-matches the filter text. It passes in CI. At 7ee880e I ran pty-client (137 passed),cli_peek_waitandcli_peek_color_cleanup(3 passed), and the conformancepeek_waitandscrollback_fidelityports (17 and 8 passed).Limits.
peek -f.--waithelp line is Node's fixture text ("Block until appears on screen") and is unchanged.