-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Still don't see retry on request or body error when fetching metadata #9246
Comments
Hm maybe the |
Huh? |
Like we're relying on uv/crates/uv-client/src/base_client.rs Line 474 in 821f3de
uv/crates/uv-client/src/base_client.rs Line 491 in 821f3de
and we cast the error at uv/crates/uv-client/src/registry_client.rs Line 390 in 821f3de
uv/crates/uv-client/src/error.rs Lines 183 to 184 in 3eda248
but... we actually implement uv/crates/uv-client/src/error.rs Lines 332 to 343 in 3eda248
|
Oh that wasn't directed at you -- sorry. I'm more just confused as to how this could still be happening. |
Figured I'd share my train of thought anyway. Also very surprised. |
I only bring the hard/annoying bugs 😉 |
Time to add logs, I guess. #9248 |
I don't know how to reproduce it, but you could be right about the source thing. Given: impl std::error::Error for WrappedReqwestError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
if self.is_likely_offline() {
match &self.0 {
reqwest_middleware::Error::Middleware(err) => Some(err.as_ref()),
reqwest_middleware::Error::Reqwest(err) => Some(err),
}
} else {
self.0.source()
}
}
} I think if we hit the |
Actually, in both cases, aren't we skipping |
Oh true! |
I'd like to try and reproduce somehow. |
I guess I'd use a patched |
Ok believe I've reproduced by patching |
## Summary It turns out that `WrappedReqwestError` skips the `reqwest::Error` itself in order to hack the display. This PR adds it to the list of variants we check when retrying transient errors. Closes #9246. ## Test Plan Patched `reqwest` locally to return an error in `bytes()`. Verified that it was _not_ caught prior to this PR, but was caught afterwards.
I just tried 0.5.4 and my network didn't trigger the usual error but I did get a slightly different error which I wasn't sure if worth raising a new issue (it didn't block resolution / install):
The And I guess |
Yeah if it fails I think we just download-then-unzip. |
To round this out, I see it working in the trace logs!
And then the next request presumably worked because I don't see an error. |
uv 0.5.3, inside a docker build script installed via
COPY --from=ghcr.io/astral-sh/uv:0.5.3 /uv /uvx /bin/
.Follow up to #8144
uv trace logs enabled by
ENV RUST_LOG=uv=trace
I appreciate the network environment is bad, but the file does download using other popular tools which include retry by default (pip, curl, etc.), and I don't see any retry information in the logs.
The text was updated successfully, but these errors were encountered: