Repository navigation
Keep a command's output head and tail, drop its middle at pipe speed - #88
Conversation
|
Fork CI: indent-com#42 (yas-run Actions are stuck org-wide). |
![ultimator-agent[bot]](https://avatars.githubusercontent.com/in/5124335?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultimator's review of e51e829
Adds SPAWN_KEEP_OUTPUT. For each output stream the server keeps a head that ends on a UTF-8 character boundary and a ring of the last bytes. It drops and counts the middle as fast as the pipe gives it, and flushes the kept tails before an exit that outlives the streams. The EXIT event reports what was dropped, and the client reads it through Command::keep_output and Process::elided. The cutting logic holds up: I re-ran output_keep's tests, plus 20,000 extra random cases with drop_rest part-way through a stream (and part-way through a tail) and 300 cases with reads up to 20 KB. All counts matched from_utf8_lossy. The flush and abort ordering in the server's cleanup paths looks consistent. I found one gap in the client API and one misplaced attribute.
2 issues: 1 bug, 1 nit
No issue is open since the review of f59495d.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
Review of yas-run#88: Output had no place for what KEEP_OUTPUT dropped, and output() takes the process, so a kept command's head and tail came joined with nothing to say a middle was missing. Output.elided now carries it (the test's helper is output_limited again). And the new mod output_keep line took mod process's #[cfg(any(unix, windows))].
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of f59495d
f59495d adds elided to Output, filled in by output_limited after the wait, with the field docs updated. The test now collects kept output through output_limited. It also puts mod process back under #[cfg(any(unix, windows))]. process.rs is the only place in the workspace that builds Output as a struct literal, so the new field breaks nothing. Both open issues are fixed and I found nothing new.
No issues found · 2 resolved
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of fabc391
fabc391 switches the UTF-16 upper bound in OutputElision::decode to code_points.saturating_mul(2). Before, a server that sent over 2^63 code points overflowed the multiplication: debug builds panicked and release builds wrapped to a wrong result. The new test covers the exact 40-byte encoding, every truncated prefix, one input breaking each rule, and the two largest inputs, and each case behaves as it should under the new check. There was nothing left open and I found nothing new.
No issues found.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
Review of yas-run#88: Output had no place for what KEEP_OUTPUT dropped, and output() takes the process, so a kept command's head and tail came joined with nothing to say a middle was missing. Output.elided now carries it (the test's helper is output_limited again). And the new mod output_keep line took mod process's #[cfg(any(unix, windows))].
fabc391 to
763661d
Compare
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of 763661d
Rebased onto c812b11, now that #83 and #85 have landed; the PR's code is unchanged. git range-diff shows each of the six remaining commits as an identical patch of the one it replaces (the three REPORT_EXIT/tag-19 commits, plus e51e829, f59495d and fabc391). The tree difference from fabc391 to 763661d is exactly what main gained besides the stack, #89 and #90: the old stack tip (1afbacf) and c812b11 differ in the same 19 files, by the same stat. At the new head, cargo run -p xtask -- protocol --check reports the schema and generated artifacts current. Nothing new, and nothing is open.
No issues found.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
SPAWN_KEEP_OUTPUT (32), with REPORT_EXIT and SPAWN extension tag 4 [head_bytes, tail_bytes] (the tail at most 1 MiB), sends only the head and the tail of each output stream. What comes between is dropped as the server reads it, never held for the client's credit, so a command writing far more than its client keeps runs at the speed of its pipe instead of one window per round trip. The cuts fall between characters as a WHATWG UTF-8 decoder with replacement reads the whole stream (a JavaScript TextDecoder, Rust's from_utf8_lossy), so decoding the head and the tail gives exactly the characters they have within it. The EXIT event says what was dropped of each stream (extension tags 1 and 2, OutputElision: offset, bytes, lines, code points, UTF-16 units), so a client can say how much it did not get in the units it counts. The output reader waits for the owner only while the head goes out, keeps the last tail_bytes in a ring, and sends them at the stream's end, or before the exit is reported when the stream outlives it (a residue past its grace, TERMINATE, a lost owner, a forced cleanup): flush_kept. yas-client: Command::keep_output(head, tail), set where the server offers it; Process::elided(stderr) once the exit came.
Review of yas-run#88: Output had no place for what KEEP_OUTPUT dropped, and output() takes the process, so a kept command's head and tail came joined with nothing to say a middle was missing. Output.elided now carries it (the test's helper is output_limited again). And the new mod output_keep line took mod process's #[cfg(any(unix, windows))].
protocol-fuzz found OutputElision::decode multiplying a decoded code point count by two, which panics past u64::MAX / 2. Saturate instead: a count that wide bounds nothing it could hold. Test the five counts' rules, prefixes, and the widest values.
763661d to
3c38bb3
Compare
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of 3c38bb3
Rebased onto 889fa47 now that #86 has landed, and the code is the same as the round-4 head. The tree of 3c38bb3 is byte-for-byte the tree of 763661d (bfc88ed). #86's squash has the same tree as the old stack tip (446b0b0). git range-diff shows each of the three remaining commits as an identical patch of the one it replaces. Nothing new, and nothing is open.
No issues found.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
1 participant
Stacked on #86 (on #85, on #83): merge those first. This PR's own changes are the last two commits: e51e829, and the review fixes in f59495d.
A command that writes far more than its client keeps (Ultimator's bash keeps 150,000 UTF-16 units from each end) still has every byte sent today, one window per round trip. At 100 ms RTT, 20 MB takes 3.6 s with 1 MiB windows (88 s with the 24 KiB windows before #87). Everything between the first and last 150 KB is then thrown away on the client. With
KEEP_OUTPUT, the server drops the middle as it reads it and says exactly what it dropped.Protocol
SPAWN_KEEP_OUTPUT(32), opt-in. It needsREPORT_EXITand SPAWN extension tag 4,[head_bytes: u64, tail_bytes: u64]. The tail is at mostMAX_KEEP_OUTPUT_TAIL_BYTES, 1 MiB. Servers offer the flag in tag 19, which Report a spawned process's exit unasked: bash in one round trip #86 made a flag set, so older clients just don't see it.Cuts between characters. The cuts fall where a WHATWG UTF-8 decoder with replacement (a JS
TextDecoder, Rust'sfrom_utf8_lossy) reading the whole stream is between characters:head_bytes. It ends between characters, or where the next byte can't continue the character it is in;tail_bytes, starts between characters, and once anything was dropped never starts with a continuation byte.Decoding the head and the tail, apart or one after the other, gives exactly the characters they have within the whole stream.
The Transfer carries the head, then the tail, with contiguous offsets.
The
EXITevent says what was dropped, in extension tag 1 (stdout) and tag 2 (stderr), present only when something was. Each is anOutputElision:offset(the head's length), then the droppedbytes,lines,code_pointsandutf16_units, counted as that decoder reads them within the whole stream. A client can then say what it didn't get, in its own units.When the tail goes out. Normally at the stream's end. If the stream outlives the exit (a residue past its grace, TERMINATE, a lost owner, a forced cleanup), the tail goes out before the exit is reported, and whatever is written after that goes to nobody.
Server (
output_keep.rs,process.rs)KeptOutputhandles a stream's cuts and counts:tail_bytes;flush_keptasks the readers for their tails and waits for them asdrain_paceddoes. It runs beforeabandon_residue, before the forced cleanup aborts the pipes, in TERMINATE, and on owner loss. A stream stopped before its tail goes out counts what it kept as dropped (drop_rest).yas-client
Command::keep_output(head, tail)sets the flag and extension only where the server offersKEEP_OUTPUTandREPORT_EXIT; elsewhere all the output comes.Process::elided(stderr)returns the elision once the exit arrives, andOutput.elidedcarries both streams' elisions (review, f59495d).Tests
output_keepunit tests (6):from_utf8_lossy/encode_utf16of the dropped part;drop_rest.kept_output_is_its_head_and_tail_and_the_exit_counts_the_rest:yeson stdout plus 1 MiB on stderr: the exact head and tail bytes come, with exact counts on both streams;an_output_elision_is_its_five_counts_and_they_must_agree(fabc391): the encoding, every prefix rejected, each broken count rule rejected, and the widest counts accepted. protocol-fuzz had foundOutputElision::decodecomputing2 * code_pointsin plain u64 arithmetic, which overflows. The check now saturates.Checks
Run locally at e51e829 (then at f59495d: client_host 38/38 with
--include-ignored, clippy--workspace --all-targets, yas-client lib 33):cargo xtask protocol --checkpasses;--workspace --all-targetsand yas-server--no-default-featuresgive no warnings;--include-ignoredagainst Pace process output by its owner; drop a lagging watcher alone #85's server;Fork CI results will be posted here.