Repository navigation
fix(http): retry when a response body drops mid-stream - #14253
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to Response-body failures can now be retried, while the inspected JSON and version-list paths retain their existing behavior. No material issue remains before merge. Pre-merge checks |
|
5c6e084 to
6805afe
Compare
Co-authored-by: Binlogo <binboy@live.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6805afe to
c13e392
Compare
) <!-- entire-trail-link-start --> https://entire.io/gh/jdx/mise/trails/637 <!-- entire-trail-link-end --> `test_text_request_allow_html_returns_the_page` (added in #14253) times out on every run, which fails `unit-macos` on main (run 38058688044) and reproduces locally on Linux. Its second half checks that an HTML page is still rejected without `allow_html()`. For an `http://` URL, `TextRequest::send` retries as `https://` first. That retry sends a TLS hello to the plain-HTTP canned server, which drains until `\r\n\r\n` and so blocks until the 2s client timeout. nextest's CI ceiling is 1s, so both attempts are killed. The test now uses a small inline listener for that half: it answers the first connection with the HTML and drops the second, so the https retry fails immediately. The test goes from timing out to 0.03s, and all 34 `http::tests::retry` tests pass. No production code changes. *AI-assisted — Tool: Claude Code; model: anthropic/claude-sonnet-5-5; version: 2.1.293.* 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Test-only change in retry.rs; no runtime HTTP behavior is modified. > > **Overview** > Fixes a flaky/timeout failure in **`test_text_request_allow_html_returns_the_page`** on the assertion that HTML is rejected without **`allow_html()`**. > > The second request still uses an **`http://`** URL (so the client’s **https** retry runs), but the test no longer uses **`spawn_canned_server`** with two canned responses. It binds a one-off **`TcpListener`** that serves HTML on the first connection and leaves the second connection hanging without blocking until the client timeout—matching how a TLS hello on plain HTTP used to stall the canned server under nextest’s short CI deadline. > > Comments in the test document that behavior. **No production HTTP code changes.** > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a59cf87. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated coverage for HTTP-to-HTTPS retry behavior when an HTML response is followed by a dropped connection. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
<!-- entire-trail-link-start --> https://entire.io/gh/jdx/mise/trails/628 <!-- entire-trail-link-end --> ### 🚀 Features - **(runtime-symlinks)** add a global link to the version global config selects by @jdx in [#14271](#14271) ### 🐛 Bug Fixes - **(history)** skip rewriting unchanged checkpoint metadata on index rebuild by @jdx in [#14249](#14249) - **(http)** retry when a response body drops mid-stream by @JamBalaya56562 in [#14253](#14253) - **(lock)** decode percent-encoded release tags when checking a locked URL by @jdx in [#14252](#14252) - **(schema)** accept outputs = { auto = false } on tasks by @JamBalaya56562 in [#14265](#14265) ### 📚 Documentation - **(dotfiles)** add a recommended setup and a readable modes-and-keys table by @jdx in [#14267](#14267) ### 🧪 Testing - **(http)** stop the allow_html test waiting out the https retry by @jdx in [#14285](#14285) ### 📦 Registry - test ansible, azure and dvc against python 3.14 by @jdx in [#14268](#14268) ## 📦 Aqua Registry Updates ### New Packages (2) - [`runxhq/runx`](https://github.com/runxhq/runx) - [`takumin/passt-bin`](https://github.com/takumin/passt-bin)
A request whose response body dropped partway through failed on the first try, even with
http_retriesset. mise already counts a broken body read as transient, and #14049 extended that to an HTTP/2 stream reset during the read. But the retry loop ended once the response headers arrived, and callers read the body afterwards, outside it:So a version list or release page cut off mid-transfer failed with
error decoding response body, with no retry.The retry loop now takes a reader and runs it inside each attempt. These callers read the whole body inside the retry:
json/json_headers/json_headers_with_headersget_textand everything built on its request, including the cached variantsget_bytesversion_regex. It used to fetch the response and read it outside the loop, so it bypassedget_text's HTML check. It now goes throughget_textwith a newallow_html(), which keeps the HTML pages that regex parsing expects.Callers that need the response itself, such as HEAD, streamed downloads and error-body inspection, pass the response through unchanged, so their behavior is the same.
get_htmlis unchanged too: it checksContent-Typebefore reading, and reading inside the retry would download bodies it currently discards.This takes over #12974 by @Binlogo, which pr-closer closed after 7 days of failing checks. The HTTP client has since moved to
crates/mise-util/src/http.rs, so this ports the change and does not replay it. They are credited as co-author. The original description gives background: a 27 MB GitHub releases page inmise-versionsthat drops mid-stream. As it says, retrying alone will not always rescue a response that large.Review threads on #12974:
version_regexpath was still unretried. Fixed, as described above.json_headersresolves auth headers before checking offline mode. Not changed here. That order predates this PR:get_asyncresolved the headers beforeget_async_with_headerschecked offline mode. It is a separate fix.Validation
crates/mise-util/src/http/tests/retry.rs:get_text,jsonandget_byteseach get a response cut off before itsContent-Lengthon the first attempt and a full one on the second. Each returns the second body after two connections.allow_html()returns an HTML page, which is still rejected without it.cargo test -p mise-util --lib -- http::tests::retrypasses all 33 tests with this change, the four new ones among them.main's code, fail witherror decoding response body, the symptom this fixes.AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5-5; version: unavailable.
🤖 Generated with Claude Code
Summary by CodeRabbit