fix: retry truncated response bodies - #326
Conversation
✅ Deploy Preview for yarn-v6 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
a894dda to
fc5f64f
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Body retries ignore response status
- Body consumption failures are now only retried for success, server error, and 408/413/429 responses, and send_bytes returns the response with a partial body on non-retryable client errors so npm callers can inspect status.
Or push these changes by commenting:
@cursor push 60fe0d1435
You can send follow-ups to the cloud agent here.
fc5f64f to
4e1534f
Compare
0b815c3 to
05154eb
Compare
ruimartin
left a comment
ruimartin
left a comment
There was a problem hiding this comment.
Implementation notes for the non-obvious retry and response-preservation choices:
⏱️ Benchmark Resultsgatsby install-full-cold
📊 Raw benchmark data (gatsby install-full-cold)Base times: 3.916s, 3.893s, 3.898s, 3.858s, 3.879s, 3.865s, 3.855s, 3.870s, 3.868s, 3.872s, 3.891s, 3.860s, 3.911s, 3.896s, 3.820s, 3.942s, 3.884s, 3.844s, 3.840s, 3.908s, 3.844s, 3.853s, 3.872s, 3.879s, 3.862s, 3.892s, 3.918s, 3.947s, 3.910s, 3.860s Head times: 3.907s, 3.825s, 3.860s, 3.871s, 3.933s, 3.935s, 3.853s, 3.914s, 3.908s, 3.896s, 3.856s, 3.891s, 3.922s, 3.847s, 3.884s, 3.826s, 3.834s, 3.902s, 3.869s, 3.864s, 3.846s, 3.854s, 3.865s, 3.801s, 3.871s, 3.823s, 3.900s, 3.896s, 3.874s, 4.012s gatsby install-cache-only
📊 Raw benchmark data (gatsby install-cache-only)Base times: 1.282s, 1.281s, 1.321s, 1.309s, 1.282s, 1.322s, 1.296s, 1.288s, 1.293s, 1.381s, 1.309s, 1.309s, 1.297s, 1.313s, 1.310s, 1.317s, 1.284s, 1.290s, 1.654s, 1.374s, 1.315s, 1.319s, 1.329s, 1.311s, 1.316s, 1.318s, 1.320s, 1.297s, 1.313s, 1.311s Head times: 1.313s, 1.308s, 1.313s, 1.325s, 1.361s, 1.320s, 1.293s, 1.309s, 1.290s, 1.306s, 1.303s, 1.309s, 1.303s, 1.293s, 1.293s, 1.378s, 1.307s, 1.295s, 1.304s, 1.488s, 1.302s, 1.291s, 1.314s, 1.311s, 1.320s, 1.325s, 1.316s, 1.325s, 1.289s, 1.318s gatsby install-cache-and-lock (warm, with lockfile)
📊 Raw benchmark data (gatsby install-cache-and-lock (warm, with lockfile))Base times: 0.365s, 0.367s, 0.366s, 0.374s, 0.368s, 0.377s, 0.370s, 0.366s, 0.367s, 0.367s, 0.368s, 0.370s, 0.371s, 0.373s, 0.372s, 0.372s, 0.372s, 0.368s, 0.368s, 0.368s, 0.370s, 0.406s, 0.387s, 0.378s, 0.377s, 0.373s, 0.375s, 0.374s, 0.404s, 0.375s Head times: 0.369s, 0.373s, 0.369s, 0.388s, 0.375s, 0.400s, 0.363s, 0.363s, 0.362s, 0.361s, 0.361s, 0.361s, 0.367s, 0.372s, 0.369s, 0.366s, 0.368s, 0.366s, 0.372s, 0.402s, 0.369s, 0.366s, 0.369s, 0.364s, 0.373s, 0.378s, 0.372s, 0.376s, 0.369s, 0.369s |
05154eb to
bcbdb5b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Empty body for allowed 3xx
- Changed send_bytes to skip body reads only for 4xx/5xx (matching error_for_status) instead of all non-2xx statuses, so 3xx responses retain their payload on unchecked caller paths.
Or push these changes by commenting:
@cursor push 0bec40bf07
Preview (0bec40bf07)
diff --git a/packages/zpm/src/http.rs b/packages/zpm/src/http.rs
--- a/packages/zpm/src/http.rs
+++ b/packages/zpm/src/http.rs
@@ -274,7 +274,9 @@
= self.enable_status_check;
self.send_with(move |mut response| async move {
- if !enable_status_check && !response.status().is_success() {
+ if !enable_status_check
+ && (response.status().is_client_error() || response.status().is_server_error())
+ {
return Ok((response, Bytes::new()));
}You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit bcbdb5b. Configure here.
bcbdb5b to
4251e57
Compare
4251e57 to
948d7af
Compare
948d7af to
ab22943
Compare
ab22943 to
50af162
Compare
50af162 to
3aeb95d
Compare
ruimartin
left a comment
ruimartin
left a comment
There was a problem hiding this comment.
Implementation notes for the retry and response-preservation choices:
| response | ||
| }; | ||
|
|
||
| let result |
There was a problem hiding this comment.
The consumer runs inside the existing request loop so header and body failures share the same retry budget and backoff.
| = self.enable_status_check; | ||
|
|
||
| self.send_with(move |mut response| async move { | ||
| if !enable_status_check |
There was a problem hiding this comment.
Only unchecked 4xx/5xx and 304 responses bypass body reads. This keeps auth and cache statuses inspectable while preserving bodies for allowed 3xx responses.
| let mut body | ||
| = BytesMut::with_capacity(capacity); | ||
|
|
||
| while let Some(chunk) = response.chunk().await? { |
There was a problem hiding this comment.
Reading chunks retains the drained response for callers that inspect status or headers; Response::bytes() would consume it.
2 participants
Motivation
httpRetrycurrently covers sending a request, but not reading its response body. Once the headers arrive, a truncated metadata or archive download fails with a body decoding error instead of retrying.This is more likely to affect large installs because thousands of downloads amplify otherwise rare network faults.
Changes
Read full-body GET responses inside the existing HTTP retry loop. Body-read failures now use the same retry budget and backoff as connection failures, without adding separate retry policies to each fetcher or retrying the whole install.
The npm paths that inspect the response first keep their existing status handling. Truncated 401/OTP and 304 responses remain inspectable, while allowed 3xx responses retain their bodies.
QA Instructions
I ran the new HTTP retry test file against
origin/mainwith only the test file applied:UnexpectedEofwithout retrying.Blast Radius
Limited to GET callers that consume the full response body. Successful downloads are unchanged. Transient body failures use the existing retry budget, and status-first npm responses keep their previous authentication and cache handling.