Retry streaming distribution and Python downloads if failing mid-stream

[konstin]

We have retry support if there is an IO or status code error in the initial request (#13897). If the initial response succeeded (usually with status code 200) and we convert the response to a streaming download, the streaming download-and-unpack is not retried, and we lose the reliable retry information.

Broken down into items:

• Show previous retries when a distribution download fails mid-streaming
• Perform retries when a distribution download fails mid-streaming
• Show previous retries when a Python download fails mid-streaming (Keep track of retries in ManagedPythonDownload::fetch_with_retry #14378)
• Perform retries when a Python download fails mid-streaming
  • (Jack: It's not clear that this is missing? The only cases I've seen fail to retry are synthetic.)

[oconnor663]

In playing with #14171, I'm noticing that we retry certain errors more than I think(?) we intended. For example if I have my local Python mirror force a ConnectionReset error during the "streaming phase" of a download, we retry 3 times (for a total of 4 attempts), but if I force the same error at the very start of the GET request, I see 16 attempts. My theory is that we have nested retrying going on, with the inner loop done (indirectly) by apply_middleware:

uv/crates/uv-client/src/base_client.rs

Lines 412 to 420 in 1c7c174

	// Avoid uncloneable errors with a streaming body during publish.
	if self.retries > 0 {
	// Initialize the retry strategy.
	let retry_strategy = RetryTransientMiddleware::new_with_policy_and_strategy(
	self.retry_policy(),
	UvRetryableStrategy,
	);
	client = client.with(retry_strategy);
	}

and the outer loop done (explicitly) by fetch_with_retry:

uv/crates/uv-python/src/downloads.rs

Lines 719 to 730 in 1c7c174

	if let reqwest_retry::RetryDecision::Retry { execute_after } = retry_decision {
	debug!(
	"Transient failure while handling response for {}; retrying...",
	self.key()
	);
	let duration = execute_after
	.duration_since(SystemTime::now())
	.unwrap_or_else(|_| Duration::default());
	tokio::time::sleep(duration).await;
	n_past_retries += 1;
	continue;
	}

Is that correct? Is it expected, or maybe a bug?

[zanieb]

Sounds plausible that is unintentional

[konstin]

There's definitely retry nesting, mainly due to io errors vs. status code errors, where some of the nesting behavior is unintentional.

[oconnor663]

@konstin do you think it's a good idea to "fix" some of these "excessive" retries as part of getting the logging correct, or maybe better to just get the logging correct but keep the retry behavior itself unchanged? (Like I can see an argument that if we want 3 retries, but you get 3 distinct errors with different causes, it's not actually a bad thing to keep retrying until you actually get the same error 3 times.)

[konstin]

Having a consistent number of retries is preferable, though excessive retries are not really a problem. I'm more worried that the logic is overly complex and thereby faulty through retrying on slightly different conditions in different layers.