From eb21e4bd256bf04a2dc86e52c647833da9b95fe4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 10 Dec 2024 07:33:08 -0500 Subject: [PATCH] Retry on tar extraction errors (#9753) ## Summary So the error here is: ```rust ExtractError("cpython-3.11.11%2B20241206-aarch64-apple-darwin-install_only_stripped.tar.gz", Io(Custom { kind: UnexpectedEof, error: TarError { desc: "failed to unpack `/Users/crmarsh/.local/share/uv/python/.cache/.tmpkqFzqE/python/lib/libpython3.11.dylib`", io: Custom { kind: UnexpectedEof, error: TarError { desc: "failed to unpack `python/lib/libpython3.11.dylib` into `/Users/crmarsh/.local/share/uv/python/.cache/.tmpkqFzqE/python/lib/libpython3.11.dylib`", io: Custom { kind: UnexpectedEof, error: "unexpected end of file" } } } } })) ``` This isn't a Reqwest error, so we miss it in `is_extended_transient_error`. We could add `TarError` or `ExtractError` here, but... should we? This PR just extends it to any error that has an IO source. I don't see much of a downside. Closes https://github.com/astral-sh/uv/issues/9747. ## Test Plan First, ran: `uv run ./scripts/create-python-mirror.py --name cpython --arch aarch64 --os darwin`. Then, dropped this into `./scripts/mirror/server.py`: ```python import os import random from http.server import SimpleHTTPRequestHandler, HTTPServer class GlitchyStaticServer(SimpleHTTPRequestHandler): def do_GET(self): """Handle GET request.""" file_path = self.translate_path(self.path) if not os.path.exists(file_path): self.send_error(404, "File not found") return try: with open(file_path, 'rb') as f: file_content = f.read() # Introduce an "unexpected end of file" glitch randomly if random.random() < 0.75: # 75% chance of glitch glitch_point = random.randint(1, len(file_content) - 1) file_content = file_content[:glitch_point] self.send_response(200) self.send_header("Content-type", self.guess_type(file_path)) self.send_header("Content-Length", len(file_content)) self.end_headers() self.wfile.write(file_content) except Exception as e: self.send_error(500, f"Internal Server Error: {e}") def run(server_class=HTTPServer, handler_class=GlitchyStaticServer, port=8080): """Run the server.""" server_address = ('', port) httpd = server_class(server_address, handler_class) print(f"Serving on port {port} with glitchy behavior") httpd.serve_forever() if __name__ == "__main__": run() ``` Then ran `python server.py` from that directory. From there, ran `UV_PYTHON_INSTALL_MIRROR="http://localhost:8080" cargo run python install 3.11 --reinstall --verbose` to reliably test retries. --- crates/uv-client/src/base_client.rs | 53 +++++------------------------ 1 file changed, 9 insertions(+), 44 deletions(-) diff --git a/crates/uv-client/src/base_client.rs b/crates/uv-client/src/base_client.rs index d9acc8964..bf26034e9 100644 --- a/crates/uv-client/src/base_client.rs +++ b/crates/uv-client/src/base_client.rs @@ -25,7 +25,7 @@ use uv_warnings::warn_user_once; use crate::linehaul::LineHaul; use crate::middleware::OfflineMiddleware; use crate::tls::read_identity; -use crate::{Connectivity, WrappedReqwestError}; +use crate::Connectivity; pub const DEFAULT_RETRIES: u32 = 3; @@ -458,51 +458,16 @@ impl RetryableStrategy for UvRetryableStrategy { pub fn is_extended_transient_error(err: &dyn Error) -> bool { trace!("Attempting to retry error: {err:?}"); - if let Some(err) = find_source::(&err) { - // First, look for `WrappedReqwestError`, which wraps `reqwest::Error` but doesn't always - // include it in the source. - if let Some(io) = find_source::(&err) { - if io.kind() == std::io::ErrorKind::ConnectionReset - || io.kind() == std::io::ErrorKind::UnexpectedEof - { - trace!( - "Retrying error: `ConnectionReset` or `UnexpectedEof` (`WrappedReqwestError`)" - ); - return true; - } - trace!("Cannot retry error: not one of `ConnectionReset` or `UnexpectedEof` (`WrappedReqwestError`)"); - } else { - trace!("Cannot retry error: not an IO error (`WrappedReqwestError`)"); - } - } else if let Some(err) = find_source::(&err) { - // Next, look for `reqwest_middleware::Error`, which wraps `reqwest::Error`, but also - // includes errors from the middleware stack. - if let Some(io) = find_source::(&err) { - if io.kind() == std::io::ErrorKind::ConnectionReset - || io.kind() == std::io::ErrorKind::UnexpectedEof - { - trace!("Retrying error: `ConnectionReset` or `UnexpectedEof` (`reqwest_middleware::Error`)"); - return true; - } - trace!("Cannot retry error: not one of `ConnectionReset` or `UnexpectedEof` (`reqwest_middleware::Error`)"); - } else { - trace!("Cannot retry error: not an IO error (`reqwest_middleware::Error`)"); - } - } else if let Some(err) = find_source::(&err) { - // Finally, look for `reqwest::Error`, which is the most common error type. - if let Some(io) = find_source::(&err) { - if io.kind() == std::io::ErrorKind::ConnectionReset - || io.kind() == std::io::ErrorKind::UnexpectedEof - { - trace!("Retrying error: `ConnectionReset` or `UnexpectedEof` (`reqwest::Error`)"); - return true; - } - trace!("Cannot retry error: not one of `ConnectionReset` or `UnexpectedEof` (`reqwest::Error`)"); - } else { - trace!("Cannot retry error: not an IO error (`reqwest::Error`)"); + if let Some(io) = find_source::(&err) { + if io.kind() == std::io::ErrorKind::ConnectionReset + || io.kind() == std::io::ErrorKind::UnexpectedEof + { + trace!("Retrying error: `ConnectionReset` or `UnexpectedEof`"); + return true; } + trace!("Cannot retry error: not one of `ConnectionReset` or `UnexpectedEof`"); } else { - trace!("Cannot retry error: not a reqwest error"); + trace!("Cannot retry error: not an IO error"); } false