Repository navigation
Conversation
KuSh
force-pushed
the
fix/4120-followup-recovery-policy
branch
from
September 20, 2026 18:47
73adf82 to
b76747b
Compare
📊 Automated PR Analysis
SummaryRefines the stdout_only() stderr cap introduced in #4120: the recovery store is now only written to when the capped prefix hides at least MIN_HIDDEN_STDERR_BYTES, preventing clean-but-chatty runs from evicting stored failure logs for a negligible byte savings. It also fixes the rationale in the doc comment for last_lines_offset (CRLF preservation was never the real reason) and adds tests pinning both behaviors. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
KuSh
force-pushed
the
fix/4120-followup-recovery-policy
branch
5 times, most recently
from
September 20, 2026 19:44
a191acd to
99fd60b
Compare
Merged
…e worth recalling The stderr cap on a stdout-only filter's success path wrote a recovery entry for any stderr over the store's floor, however little the cap removed. A twelve-line `go: downloading …` stderr was stored to save nine bytes, and every stored entry pushes an older one out of a bounded store: ten such runs in legacy tee mode with `tee_on_success` filled the twenty-file ring and evicted the failure logs someone had kept. The stored entry holds what recall adds to the screen, which is the hidden prefix and not the whole stderr, so the prefix is what has to clear the store's floor. Below it the run forwards stderr whole and writes nothing, which also removes the lone case where the cap asked the store for a hint and then declined to use it. The chatty runs the cap exists for are unaffected. Also correct the doc comment on `last_lines_offset`: `stream::read_lines_lossy` strips the `\r` of a CRLF ending before the runner sees stderr, so the CRLF the comment claimed the byte offset preserves cannot reach it. The byte offset stays for the reasons that do hold — the forwarded bytes are the tool's own, and the skipped lines are never read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KuSh
force-pushed
the
fix/4120-followup-recovery-policy
branch
from
October 7, 2026 00:02
99fd60b to
0adc59f
Compare
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
Two follow-ups on the stderr cap that
stdout_only()filters got in #4120.A recovery entry now has to be worth the slot it takes. On a clean run the cap asked the
recovery store for a hint whenever stderr cleared the store's floor, however little the cap
actually removed. A twelve-line
go: downloading …stderr was stored to save nine bytes. Thestore is bounded —
max_entries(200) in sqlite mode,tee_max_files(20) in legacy teemode — so entries compete for slots, and once it is full clean runs evict failure logs.
The entry holds the whole stderr, but all recall adds to what is already on screen is the
prefix the cap removed, so that prefix is what has to be worth a slot.
MIN_HIDDEN_STDERR_BYTESis the threshold:tee::MIN_TEE_SIZE, the byte floor the failurepath already applies before storing. Under it the run forwards stderr whole and writes
nothing, and the check sits before the
recovery_hintcall because asking is what writes.The whole-stderr floor check it replaces is subsumed — the hidden prefix is always shorter
than the stderr it came from — so the new rule is a strict superset of the old one and can
only ever decline to cap.
The cap still asks for a hint before it can tell whether the capped form is smaller — the
hint's length is only knowable by asking — so a write it then discards is still possible in
principle. The floor puts it out of reach at any realistic hint: losing now needs a note and
hint together longer than the 500-byte prefix, i.e. a log path of some 460 characters.
The
… (+N earlier stderr lines not shown)note is still only ever emitted with a hint besideit, so no count is shown without a recovery path.
Corrected the rationale on
last_lines_offset. Its doc comment justified the byte-offsetslice with CRLF preservation.
stream::read_lines_lossypops the\rof a CRLF ending beforethe runner ever sees stderr — verified end to end: a child writing 60 CRLF lines to stderr
produces 0 CR bytes in what the runner forwards. Since
raw_stderris itself assembled byre-joining those lines with
\n, byte fidelity is not what the offset buys either. The offsetstays for the one reason that does hold: a re-join would read and allocate every line it is
about to throw away. The test that went with the old comment is gone — its only discriminating
input was a CRLF ending, which cannot arrive.
Measured
Stub
goon PATH:go: downloading …lines on stderr, oneokline on stdout, exit 0.Recovery store, tee directory and history DB redirected to a throwaway sandbox.
Chatty run (150 lines), sqlite mode — unchanged, which is the point:
Barely-chatty run (12 lines), sqlite mode:
Legacy tee mode with
tee_on_success = true, a failure log seeded in the ring, then ten clean12-line runs:
What it costs
Between the line cap and the floor, a clean run now forwards its stderr whole instead of
capping it. Sweeping stderr line count in sqlite mode, total bytes rtk emits against what the
command emits:
Those ratios are stderr's alone: the stub's stdout barely compresses (28 B to 24 B), so it
contributes nothing either way. On a run like that — one whose stdout the filter has little to
take off — the 13 to 21 line band goes from 9–42% down to ~0% and stops clearing the 20% floor
CONTRIBUTING.mdsets. On a realgh,glab,prettierorruffinvocation the filteredstdout normally dominates the ratio, so a stderr in that band does not by itself put the
command under the floor; what it gives up is the stderr share of the saving. Stderr of that
size is ordinary for those tools, so the share is not negligible.
It is the right trade because of what the old saving was bought with. Capping there removed at
most ~400 B from one run's stderr, and paid for it with one of a bounded number of recovery
slots — a slot the next failure needs, and that a run of clean chatty commands was emptying
before anyone got to use it. Recall is the only way back to output rtk has thrown away, so
spending its capacity on a run that saves a few hundred bytes is the wrong way round. Above
the floor, where the cap earns 45–92%, nothing changes; and the stdout compression these
filters exist for is untouched at every size.
Not changed, and why
Under
tee_on_successa clean chatty run still writes two files: the combined<epoch>_go_test.logfrom the stdout tee and<epoch>_go_test-stderr.logfrom the cap. At thedefault
tee_max_file_sizethe second really is a subset of the first — measured, 40 chatterlines: 1818 B and 1790 B, nothing truncated — so it costs a second of twenty slots for bytes
the ring already holds.
It is left alone for two reasons. It cannot happen in the default recovery mode: sqlite stores
nothing for a successful run, so the cap's entry is the only one, and the duplicate needs both
mode = "tee"andtee_on_success, neither of which is the default. And removing it meanspointing the cap's
+Nnote at the combined log, which is only sound while that log iscomplete.
write_tee_filekeeps the head and stderr is appended after stdout, so pasttee_max_file_sizethe combined log holds none of the lines the note counts — measured at thedefault 1 MiB with a 1.2 MiB run:
go_test.logcontained 0 of the 60downloadinglines,go_test-stderr.logcontained all 60. Getting that condition wrong emits a count with norecovery path, the one thing this code exists to prevent, and getting it right means threading
the combined log's identity and completeness out of
tee_and_hintand throughprint_with_hintinto a runner shared by 32 call sites — teaching it about tee file caps andrecovery modes to save a slot under a configuration that is off by default.
Known interaction
On a
never_worsefallback,emit_guarded'sprintln!adds a newline to araw_stdoutthatalready ends in one, so rtk emits one byte more than the command. This change does not cause
that and does not fix it — it is being fixed separately — but it widens the window where it is
reachable: in tee mode the totals go from 1 byte over native at 11–12 stderr lines to 1 byte
over at 11–21, because those runs now forward stderr whole instead of capping.
Shared-runner impact
forwarded_stderrhas one caller,run_captured_filter, reached by the 32RunOptions::stdout_only()call sites in 18 modules (container,psql,gh,glab,go,golangci,prettier,phpstan,phpunit,pint,pytest,ruff,sqlfluff,rspec,rubocop,ls,tree,wc). The new rule only ever makes the success path decline to cap,so for every one of them the effect is strictly more stderr forwarded verbatim and strictly
fewer store writes — no tool can lose a diagnostic it used to get. The failure and empty-stdout
paths, where stderr is the report, are untouched:
golangci-lint runwith a bad config stillforwards its
level=error msg="can't load config: …"whole and exits 3.Each branch is pinned by a test that fails when it is removed: the floor by
a_cap_hiding_less_than_the_store_floor_writes_nothing, its boundary bythe_floor_is_inclusive(<widened to<=fails it), where capping starts bya_cap_that_would_not_shrink_the_output_is_not_applied, and thecapped.len() >= stderr.len()guard by the long-hint case in that same test.
cargo fmt --all,cargo clippy --all-targetsandcargo test --allare clean.🤖 Generated with Claude Code