Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
szokeasaurusrex merged 1 commit into from
May 27, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/api/errors/api_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
17 changes: 6 additions & 11 deletions src/api/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
53 changes: 45 additions & 8 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Copy link

Copilot AI May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider updating the documentation for get_max_retry_count to clarify that it logs a warning and falls back to a default value when parsing errors occur.

Copilot uses AI. Check for mistakes.
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
Expand Down Expand Up @@ -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 {
Expand Down