-
Notifications
You must be signed in to change notification settings - Fork 13
Open
Labels
bugSomething isn't workingSomething isn't working
Description
Detail Bug Report
Summary
- Context:
RequestConfigprovides methods to convert request configuration into AWS SDK timeout and retry configs, which are used when overriding S3 client configuration for individual requests. - Bug: The
retry_config()method always creates a newRetryConfig::standard()even when no retry-related fields are set, inadvertently resetting any custom retry configuration from environment variables or AWS config files. - Actual vs. expected: When a user specifies only timeout overrides via the C0-Config header, the retry config is reset to standard defaults (3 max attempts) instead of preserving the S3 client's existing retry configuration.
- Impact: Users who configure retry behavior via
AWS_MAX_ATTEMPTSenvironment variable or AWS config file will have their settings silently overridden when using C0-Config header to adjust only timeout values.
Code with bug
pub fn retry_config(&self) -> aws_sdk_s3::config::retry::RetryConfig {
let mut config = aws_sdk_s3::config::retry::RetryConfig::standard(); // <-- BUG 🔴 Always creates new config
if let Some(max_attempts) = self.max_attempts {
config = config.with_max_attempts(max_attempts);
}
if let Some(initial_backoff) = self.initial_backoff {
config = config.with_initial_backoff(initial_backoff);
}
if let Some(max_backoff) = self.max_backoff {
config = config.with_max_backoff(max_backoff);
}
config
}Usage in src/object_store/downloader.rs:
request
.customize()
.config_override(
self.s3
.config()
.to_builder()
.timeout_config(req_config.timeout_config())
.retry_config(req_config.retry_config()), // <-- BUG 🔴 Always replaces retry config
)
.send()
.awaitFailing test
#[tokio::test]
async fn test_partial_timeout_config_overwrites_retry_config() {
let config = aws_sdk_s3::Config::builder()
.behavior_version(aws_config::BehaviorVersion::latest())
.credentials_provider(aws_sdk_s3::config::Credentials::new(
"test", "test", None, None, "test",
))
.region(aws_sdk_s3::config::Region::new("us-east-1"))
.retry_config(
aws_sdk_s3::config::retry::RetryConfig::standard()
.with_max_attempts(10)
)
.build();
let client = aws_sdk_s3::Client::from_conf(config);
let original_max = client.config().retry_config()
.map(|rc| rc.max_attempts())
.expect("should have retry config");
assert_eq!(original_max, 10);
let req_config = RequestConfig {
connect_timeout: Some(Duration::from_secs(5)),
read_timeout: None,
operation_timeout: None,
operation_attempt_timeout: None,
max_attempts: None,
initial_backoff: None,
max_backoff: None,
};
let new_config = client
.config()
.to_builder()
.timeout_config(req_config.timeout_config())
.retry_config(req_config.retry_config())
.build();
let new_client = aws_sdk_s3::Client::from_conf(new_config);
let new_max = new_client.config().retry_config()
.map(|rc| rc.max_attempts())
.expect("should have retry config");
assert_eq!(new_max, 10,
"BUG: max_attempts changed from 10 to {} when only timeout was configured",
new_max);
}Output:
assertion `left == right` failed: BUG: max_attempts changed from 10 to 3 when only timeout was configured
left: 3
right: 10
Recommended fix
Conditionally apply overrides only when corresponding fields are set.
In src/object_store/downloader.rs:
let mut builder = self.s3.config().to_builder();
if req_config.has_timeout_fields() {
builder = builder.timeout_config(req_config.timeout_config());
}
if req_config.has_retry_fields() {
builder = builder.retry_config(req_config.retry_config());
}
.config_override(builder)Add helpers to RequestConfig:
pub fn has_timeout_fields(&self) -> bool {
self.connect_timeout.is_some()
|| self.read_timeout.is_some()
|| self.operation_timeout.is_some()
|| self.operation_attempt_timeout.is_some()
}
pub fn has_retry_fields(&self) -> bool {
self.max_attempts.is_some()
|| self.initial_backoff.is_some()
|| self.max_backoff.is_some()
}Metadata
Metadata
Assignees
Labels
bugSomething isn't workingSomething isn't working