fixup! Ensure that bad HTTP status codes result in the producer retrying

Author: Qqwy

opsqueue/src/producer/client.rs

@@ -3,6 +3,7 @@ use std::time::Duration;
 use backon::BackoffBuilder;
 use backon::FibonacciBuilder;
 use backon::Retryable;
+use http::StatusCode;
 
 use crate::common::submission::{SubmissionId, SubmissionStatus};
 use crate::tracing::CarrierMap;
@@ -162,15 +163,18 @@ impl Client {
 
     /// Get the server's version from the `/version` endpoint.
     ///
-    /// The result will be the value of [`VERSION_CARGO_SEMVER`][crate::VERSION_CARGO_SEMVER]
+    /// A successful result will be the value of [`VERSION_CARGO_SEMVER`][crate::VERSION_CARGO_SEMVER]
     /// prefixed with a "v", for example `v0.30.5`.
+    ///
+    /// Upon connection failure, this will not attempt to do any retrying.
     pub async fn server_version(&self) -> Result<String, InternalProducerClientError> {
         let base_url = &self.base_url;
         let resp = self
             .http_client
             .get(format!("{base_url}/version"))
             .send()
-            .await?;
+            .await?
+            .error_for_status()?;
         let text = resp.text().await?;
         Ok(text)
     }
@@ -191,10 +195,24 @@ impl InternalProducerClientError {
             // So even cleaner would be a tiny retry loop for this special case.
             // However, we certainly **do not** want to wait multiple minutes before returning.
             Self::ResponseDecodingError(_) => false,
-            // Reqwest doesn't make this very easy as it has a single error typed used for _everything_
-            // Maybe a different HTTP client library is nicer in this regard?
+            // Reqwest doesn't make this very easy as it has a single error typed used for _everything_.
             Self::HTTPClientError(inner) => {
-                inner.is_status() || inner.is_connect() || inner.is_timeout() || inner.is_decode()
+                // Failures in which a connection could not be established are ephemeral,
+                // as these can be caused by network failures, so we want to retry:
+                if inner.is_connect() || inner.is_timeout() || inner.is_decode() {
+                    true
+                } else if inner.is_status() {
+                    // Failures where the server returns a 5xx status code might indicate the server is (temporarily!) unhealthy,
+                    // or in the process of a restart.
+                    // Any other status is considered a permanent failure however
+                    inner
+                        .status()
+                        .map(|status| status.is_server_error())
+                        .unwrap_or(false)
+                } else {
+                    // Anything else is a permanent failure.
+                    false
+                }
             }
         }
     }