fix(minimald): bound diag stream on reader idleness, not total transfer - #936
Closed
norrietaylor wants to merge 1 commit into
Closed
fix(minimald): bound diag stream on reader idleness, not total transfer#936norrietaylor wants to merge 1 commit into
norrietaylor wants to merge 1 commit into
Conversation
The diag bundle pump wrapped `tokio::io::copy` in a single 300s `STREAM_TIMEOUT`, capping the whole transfer. But `build_bundle` runs up to fifteen 30s-deadlined collectors back to back, so on a wedged host — the very host a diagnostic bundle is wanted from — a bundle the client is actively reading can outlast the fixed total and be cut off mid-stream, delivering a truncated archive. The expiry error also blamed the client unconditionally, even when a slow server-side collection caused the stall. Replace the whole-transfer bound with `pump_until_idle`, a manual copy loop whose deadline resets on every chunk that moves and fires only when a single read-or-write step makes no progress for `STREAM_TIMEOUT`. A slow-but- progressing transfer now runs as long as bytes keep flowing; a genuine stall (a client that stops reading, or a wedged builder) is still bounded. Reword the expiry error to name the stall without asserting which side caused it. Closes #915 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Member
Author
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
1 participant
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
Replace the diag bundle pump's whole-transfer
STREAM_TIMEOUTwith anidleness bound.
serve_stream_diag_bundlepreviously wrappedtokio::io::copyin a single 300stokio::time::timeout, so the timeoutcapped the entire transfer rather than reader inactivity.
build_bundleruns up to fifteen collectors back to back, each deadlinedindividually at
COLLECTOR_TIMEOUT(30s) — a worst case of 450s. On awedged host (the exact host a diagnostic bundle is wanted from), a bundle
the client is actively reading could outlast the fixed 300s total and be
cut off mid-stream, delivering a truncated archive. The expiry error also
blamed the client (
"client stopped reading the bundle …") regardless ofwhether the stall was server-side collection.
How
Add
pump_until_idle, a manual read/write copy loop whose deadline resetson every chunk that moves and fires only when a single read-or-write step
makes no progress for
STREAM_TIMEOUT. A slow-but-progressing transfer nowruns as long as bytes keep flowing; a genuine stall — a client that stops
reading, or a build that emits nothing at all for the window — is still
bounded. The drop-order and cancellation invariants around
rx/writerthat the original relied on are preserved. Reword the expiry error to name
the stall without asserting which side caused it.
Progress is measured on the compressed bytes reaching the channel; the doc
comments state this honestly rather than claiming total decoupling from the
collection budget.
Tests
Two unit tests cover the new semantics: a slow-but-progressing transfer
whose total duration exceeds the idle bound completes without a timeout,
and a stalled reader trips a
TimedOuterror whose message asserts noblame.
cargo test -p minimaldpasses (20 diag tests);cargo fmt --checkand
cargo clippy -p minimaldare clean for the touched file.Closes #915
Note
Bound
minimalddiagnostic stream timeout on reader idleness rather than total transfer durationtokio::time::timeoutwrappingtokio::io::copyinstream_diag_bundlewith a newpump_until_idleutility that resets the deadline on each chunk of progress.STREAM_TIMEOUT(300s) of inactivity on either side.ErrorKind::TimedOut.Macroscope summarized 7c6f752.