fix: Don't error if invalid value supplied for max retries

Currently, if an invalid value is supplied for the maximum retry count, the CLI will panic, if a command which reads this value is called.

Since we can always fall back to a reasonable default, we should probably warn instead of errroring at all when an invalid value is supplied.

This change is done in preparation for #2209, since that change will add retries to all endpoints, increasing the chance of a user encountering a panic if this behavior is left unfixed.

Author: szokeasaurusrex

src/api/errors/api_error.rs

@@ -44,8 +44,6 @@ pub(in crate::api) enum ApiErrorKind {
         "DSN missing. Please set the `SENTRY_DSN` environment variable to your project's DSN."
     )]
     DsnMissing,
-    #[error("Error preparing request")]
-    ErrorPreparingRequest,
 }
 
 impl fmt::Display for ApiError {

src/api/mod.rs

@@ -396,7 +396,7 @@ impl Api {
             .request(Method::Post, url, None)?
             .with_form_data(form)?
             .with_retry(
-                self.config.get_max_retry_count().unwrap(),
+                self.config.get_max_retry_count(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -968,7 +968,7 @@ impl<'a> AuthenticatedApi<'a> {
         self.request(Method::Post, &url)?
             .with_json_body(request)?
             .with_retry(
-                self.api.config.get_max_retry_count().unwrap(),
+                self.api.config.get_max_retry_count(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -1001,7 +1001,7 @@ impl<'a> AuthenticatedApi<'a> {
                 dist: None,
             })?
             .with_retry(
-                self.api.config.get_max_retry_count().unwrap(),
+                self.api.config.get_max_retry_count(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -1032,7 +1032,7 @@ impl<'a> AuthenticatedApi<'a> {
                 dist,
             })?
             .with_retry(
-                self.api.config.get_max_retry_count().unwrap(),
+                self.api.config.get_max_retry_count(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -1408,12 +1408,7 @@ impl RegionSpecificApi<'_> {
         self.request(Method::Post, &path)?
             .with_form_data(form)?
             .with_retry(
-                self.api.api.config.get_max_retry_count().map_err(|e| {
-                    ApiError::with_source(
-                        ApiErrorKind::ErrorPreparingRequest,
-                        e.context("Could not parse retry count"),
-                    )
-                })?,
+                self.api.api.config.get_max_retry_count(),
                 &[http::HTTP_STATUS_507_INSUFFICIENT_STORAGE],
             )
             .progress_bar_mode(ProgressBarMode::Request)
@@ -1472,7 +1467,7 @@ impl RegionSpecificApi<'_> {
             .request(Method::Post, &path)?
             .with_form_data(form)?
             .with_retry(
-                self.api.api.config.get_max_retry_count().unwrap(),
+                self.api.api.config.get_max_retry_count(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,

src/config.rs

@@ -1,8 +1,10 @@
 //! This module implements config access.
 use std::env;
+use std::env::VarError;
 use std::fs;
 use std::fs::OpenOptions;
 use std::io;
+use std::num::ParseIntError;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
@@ -26,6 +28,9 @@ use crate::utils::http::is_absolute_url;
 #[cfg(target_os = "macos")]
 use crate::utils::xcode;
 
+const MAX_RETRIES_ENV_VAR: &str = "SENTRY_HTTP_MAX_RETRIES";
+const MAX_RETRIES_INI_KEY: &str = "max_retries";
+
 /// Represents the auth information
 #[derive(Debug, Clone)]
 pub enum Auth {
@@ -462,14 +467,28 @@ impl Config {
             .unwrap_or(DEFAULT_MAX_DIF_ITEM_SIZE)
     }
 
-    pub fn get_max_retry_count(&self) -> Result<u32> {
-        if env::var_os("SENTRY_HTTP_MAX_RETRIES").is_some() {
-            Ok(env::var("SENTRY_HTTP_MAX_RETRIES")?.parse()?)
-        } else if let Some(val) = self.ini.get_from(Some("http"), "max_retries") {
-            Ok(val.parse()?)
-        } else {
-            Ok(DEFAULT_RETRIES)
-        }
+    /// Returns the configured maximum number of retries for failed HTTP requests.
+    pub fn get_max_retry_count(&self) -> u32 {
+        match max_retries_from_env() {
+            Ok(Some(val)) => return val,
+            Ok(None) => (),
+            Err(e) => {
+                warn!(
+                    "Ignoring invalid {MAX_RETRIES_ENV_VAR} environment variable: {}",
+                    e
+                );
+            }
+        };
+
+        match max_retries_from_ini(&self.ini) {
+            Ok(Some(val)) => return val,
+            Ok(None) => (),
+            Err(e) => {
+                warn!("Ignoring invalid {MAX_RETRIES_INI_KEY} ini key: {}", e);
+            }
+        };
+
+        DEFAULT_RETRIES
     }
 
     /// Return the DSN
@@ -520,6 +539,24 @@ impl Config {
     }
 }
 
+/// Computes the maximum number of retries from the `SENTRY_HTTP_MAX_RETRIES` environment variable.
+/// Returns `Ok(None)` if the environment variable is not set, other errors are returned as is.
+fn max_retries_from_env() -> Result<Option<u32>> {
+    match env::var(MAX_RETRIES_ENV_VAR) {
+        Ok(val) => Ok(Some(val.parse()?)),
+        Err(VarError::NotPresent) => Ok(None),
+        Err(e) => Err(e.into()),
+    }
+}
+
+/// Computes the maximum number of retries from the `max_retries` ini key.
+/// Returns `Ok(None)` if the key is not set, other errors are returned as is.
+fn max_retries_from_ini(ini: &Ini) -> Result<Option<u32>, ParseIntError> {
+    ini.get_from(Some("http"), MAX_RETRIES_INI_KEY)
+        .map(|val| val.parse())
+        .transpose()
+}
+
 fn warn_about_conflicting_urls(token_url: &str, manually_configured_url: Option<&str>) {
     if let Some(manually_configured_url) = manually_configured_url {
         if manually_configured_url != token_url {