From 2f6cd41642d9c2680f17e5c1adf22ad8e1b0288a Mon Sep 17 00:00:00 2001
From: Alex Ostrovski <slowli@users.noreply.github.com>
Date: Mon, 17 Jun 2024 10:52:11 +0300
Subject: [PATCH] fix(object-store): Consider more GCS errors transient (#2246)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## What ❔

Considers `reqwest::Error`s with "request" kind transient for the object
store logic.

## Why ❔

Similar to some other kinds of `reqwest::Error`s, not all "request"
errors are truly transient, but considering them as such is a safer
option.

## Checklist

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Code has been formatted via `zk fmt` and `zk lint`.
- [x] Spellcheck has been run via `zk spellcheck`.
---
 core/lib/object_store/src/gcs.rs | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/core/lib/object_store/src/gcs.rs b/core/lib/object_store/src/gcs.rs
index 6960bd51f2fb..2d4fae77ab80 100644
--- a/core/lib/object_store/src/gcs.rs
+++ b/core/lib/object_store/src/gcs.rs
@@ -87,7 +87,7 @@ impl From<AuthError> for ObjectStoreError {
     fn from(err: AuthError) -> Self {
         let is_transient = matches!(
             &err,
-            AuthError::HttpError(err) if err.is_timeout() || err.is_connect() || has_transient_io_source(err) || upstream_unavailable(err)
+            AuthError::HttpError(err) if is_transient_http_error(err)
         );
         Self::Initialization {
             source: err.into(),
@@ -96,6 +96,17 @@ impl From<AuthError> for ObjectStoreError {
     }
 }
 
+fn is_transient_http_error(err: &reqwest::Error) -> bool {
+    err.is_timeout()
+        || err.is_connect()
+        // Not all request errors are logically transient, but a significant part of them are (e.g.,
+        // `hyper` protocol-level errors), and it's safer to consider an error transient.
+        || err.is_request()
+        || has_transient_io_source(err)
+        || err.status() == Some(StatusCode::BAD_GATEWAY)
+        || err.status() == Some(StatusCode::SERVICE_UNAVAILABLE)
+}
+
 fn has_transient_io_source(mut err: &(dyn StdError + 'static)) -> bool {
     loop {
         if err.is::<io::Error>() {
@@ -111,11 +122,6 @@ fn has_transient_io_source(mut err: &(dyn StdError + 'static)) -> bool {
     }
 }
 
-fn upstream_unavailable(err: &reqwest::Error) -> bool {
-    err.status() == Some(StatusCode::BAD_GATEWAY)
-        || err.status() == Some(StatusCode::SERVICE_UNAVAILABLE)
-}
-
 impl From<HttpError> for ObjectStoreError {
     fn from(err: HttpError) -> Self {
         let is_not_found = match &err {
@@ -131,7 +137,7 @@ impl From<HttpError> for ObjectStoreError {
         } else {
             let is_transient = matches!(
                 &err,
-                HttpError::HttpClient(err) if err.is_timeout() || err.is_connect() || has_transient_io_source(err) || upstream_unavailable(err)
+                HttpError::HttpClient(err) if is_transient_http_error(err)
             );
             ObjectStoreError::Other {
                 is_transient,