fix: retry S3 multipart parts on transient transport errors - #81
Merged
Conversation
S3-compatible services (especially Cloudflare R2) occasionally reset connections during large multipart uploads. Without retry, a single transient failure on any part aborted the entire upload, wasting all successfully uploaded parts. This caused ~1 GB broker backup uploads to fail intermittently on Railway (part ~127 of 128). Changes: - Add send_with_retry() helper with exponential backoff for transient transport errors (timeout, connection reset, body decode errors) - Apply retry to each multipart part upload, initiate, and complete - Add 300s overall request timeout to S3 HTTP client - Add max_retries and retry_backoff_ms to S3Config, configurable via ONEIO_S3_MAX_RETRIES (default 3) and ONEIO_S3_RETRY_BACKOFF_MS (default 1000ms) - HTTP status errors (4xx/5xx) are not retried (handled by caller)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness of OneIO’s S3 multipart upload path by retrying transient transport failures (e.g., timeouts / connection resets) so that a single failed part doesn’t abort large uploads, and adds a longer overall request timeout to reduce mid-upload failures on slow networks.
Changes:
- Add
send_with_retry()for transient reqwest transport errors with exponential backoff, and apply it to multipart initiate/part upload/complete. - Extend the S3 HTTP client with a 300s overall request timeout (in addition to the existing connect timeout).
- Add retry configuration via
S3Configfields and environment variables, plus a new ignored integration test for large multipart uploads.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/s3/mod.rs |
Adds retry helper and uses it during multipart upload; sets S3 client request timeout. |
src/s3/config.rs |
Introduces retry-related config fields and env var parsing. |
tests/s3_large_upload.rs |
Adds ignored integration test reproducer for large multipart uploads and a small baseline test. |
CHANGELOG.md |
Documents the retry behavior and new environment variables under Unreleased. |
Comment on lines
+435
to
+441
| let part_data = std::mem::replace(&mut chunk, Vec::with_capacity(chunk_size as usize)); | ||
| let response = ensure_s3_success(send_with_retry( | ||
| || { | ||
| get_s3_client() | ||
| .put(url.clone()) | ||
| .body(part_data.clone()) | ||
| .send() |
- Use saturating_mul for backoff to prevent overflow with large env values - Restructure part upload to avoid cloning the full chunk on every attempt: first attempt moves the body, retries re-read from file at the recorded offset. No extra allocation on the happy path. - Extract upload_part_with_retry() for the part-specific retry logic
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
tests/s3_large_upload.rs:23
- The default test file path is hard-coded to
/tmp/..., which is not portable (e.g. Windows) and diverges from the existing S3 integration tests that usestd::env::temp_dir()for temporary paths.
let test_file =
env::var("ONEIO_S3_TEST_FILE").unwrap_or_else(|_| "/tmp/test_upload_large.bin".to_string());
tests/s3_large_upload.rs:53
- This test currently passes even if
s3_statsfails after a successful upload, which means it can silently skip the intended size verification.
// Verify: download stats
match oneio::s3_stats(&bucket, &key) {
Ok(stats) => {
println!("Remote object: {} bytes", stats.content_length);
assert_eq!(stats.content_length, size, "Remote size mismatch");
tests/s3_large_upload.rs:90
- The small upload baseline test uses a fixed
/tmp/test_upload_small.binpath. This is non-portable and can collide if tests are run concurrently; prefer a unique path underenv::temp_dir()(similar totests/s3_integration.rs).
// Create a small 1MB file (under 5MB multipart threshold)
let test_file = "/tmp/test_upload_small.bin";
let size = 1024 * 1024; // 1MB
let data = vec![0x42u8; size];
fs::write(test_file, &data).expect("failed to write test file");
Comment on lines
+422
to
+425
| let mut buf = Vec::with_capacity(part_len as usize); | ||
| file.seek(SeekFrom::Start(offset))?; | ||
| file.by_ref().take(part_len).read_to_end(&mut buf)?; | ||
| buf |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/s3/mod.rs:445
max_retries + 1can overflow (e.g., ifONEIO_S3_MAX_RETRIESis set tou32::MAX), which can panic in debug builds and produce incorrect totals in logs. Use saturating/checked arithmetic when computing the displayed total attempts.
eprintln!(
"oneio: S3 part upload failed (attempt {}/{}), retrying in {}ms: {e}",
attempt + 1,
max_retries + 1,
backoff_ms
tests/s3_large_upload.rs:79
- The codebase’s other S3 integration tests use
#[ignore = "…"]to document why they’re ignored. Using bare#[ignore]makescargo testoutput less informative when a developer tries to run ignored tests.
#[ignore]
Comment on lines
+366
to
+371
| eprintln!( | ||
| "oneio: S3 request failed (attempt {}/{}), retrying in {}ms: {e}", | ||
| attempt + 1, | ||
| max_retries + 1, | ||
| backoff_ms | ||
| ); |
Comment on lines
+78
to
+81
| /// Maximum retry attempts for transient S3 transport errors (default: 3). | ||
| pub max_retries: u32, | ||
| /// Initial backoff in milliseconds for S3 retry (default: 1000ms, doubles each attempt). | ||
| pub retry_backoff_ms: u64, |
| use std::time::Instant; | ||
|
|
||
| #[test] | ||
| #[ignore] |
Comment on lines
+86
to
+107
| let test_file = "/tmp/test_upload_small.bin"; | ||
| let size = 1024 * 1024; // 1MB | ||
| let data = vec![0x42u8; size]; | ||
| fs::write(test_file, &data).expect("failed to write test file"); | ||
|
|
||
| let key = format!("test-small-upload/{}.bin", std::process::id()); | ||
| println!("Uploading 1MB file to s3://{bucket}/{key}"); | ||
|
|
||
| let start = Instant::now(); | ||
| let result = oneio::s3_upload(&bucket, &key, test_file); | ||
| let elapsed = start.elapsed(); | ||
|
|
||
| match &result { | ||
| Ok(()) => println!("✅ Small upload succeeded in {:.1}s", elapsed.as_secs_f64()), | ||
| Err(e) => { | ||
| println!("❌ Small upload FAILED: {e}"); | ||
| panic!("Small upload failed: {e}"); | ||
| } | ||
| } | ||
|
|
||
| let _ = oneio::s3_delete(&bucket, &key); | ||
| let _ = fs::remove_file(test_file); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/s3/mod.rs:445
- These retry paths print to stderr on every transient failure. Since this is a library, that can be noisy/unexpected for downstream apps; consider compiling these logs only when the
clifeature is enabled (or otherwise making them opt-in).
eprintln!(
"oneio: S3 part upload failed (attempt {}/{}), retrying in {}ms: {e}",
attempt + 1,
max_retries + 1,
backoff_ms
Comment on lines
+48
to
+62
| // Verify: download stats | ||
| match oneio::s3_stats(&bucket, &key) { | ||
| Ok(stats) => { | ||
| println!("Remote object: {} bytes", stats.content_length); | ||
| assert_eq!(stats.content_length, size, "Remote size mismatch"); | ||
| println!("✅ Size verified: {} bytes", size); | ||
| } | ||
| Err(e) => { | ||
| println!("⚠️ Upload succeeded but s3_stats failed: {e}"); | ||
| } | ||
| } | ||
|
|
||
| // Cleanup | ||
| let _ = oneio::s3_delete(&bucket, &key); | ||
| println!("Cleaned up s3://{bucket}/{key}"); |
Comment on lines
+15
to
+16
| //! - `ONEIO_S3_MAX_RETRIES` - Max retry attempts for transient transport errors (default: 3) | ||
| //! - `ONEIO_S3_RETRY_BACKOFF_MS` - Initial retry backoff in ms, doubles each attempt (default: 1000) |
Comment on lines
+366
to
+372
| eprintln!( | ||
| "oneio: S3 request failed (attempt {}/{}), retrying in {}ms: {e}", | ||
| attempt + 1, | ||
| max_retries + 1, | ||
| backoff_ms | ||
| ); | ||
| std::thread::sleep(Duration::from_millis(backoff_ms)); |
Comment on lines
+21
to
+22
| let test_file = | ||
| env::var("ONEIO_S3_TEST_FILE").unwrap_or_else(|_| "/tmp/test_upload_large.bin".to_string()); |
Comment on lines
+78
to
+81
| /// Number of retry attempts after the initial S3 request (default: 3). | ||
| pub max_retries: u32, | ||
| /// Initial retry backoff in milliseconds (default: 1000ms; doubles after each retry). | ||
| pub retry_backoff_ms: u64, |
Comment on lines
+346
to
+356
| fn s3_retry_config() -> (u32, u64) { | ||
| let max_retries = std::env::var("ONEIO_S3_MAX_RETRIES") | ||
| .ok() | ||
| .and_then(|value| value.parse().ok()) | ||
| .unwrap_or(3); | ||
| let retry_backoff_ms = std::env::var("ONEIO_S3_RETRY_BACKOFF_MS") | ||
| .ok() | ||
| .and_then(|value| value.parse().ok()) | ||
| .unwrap_or(1000); | ||
| (max_retries, retry_backoff_ms) | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/s3_large_upload.rs:97
- This key uses only the process id, so the large and small upload tests (or multiple invocations of the ignored tests) can write/delete the same remote object concurrently. Use the already-generated
unique_idas part of the key to avoid collisions.
let key = format!("test-small-upload/{}.bin", std::process::id());
Comment on lines
+198
to
+200
| let mut builder = reqwest::blocking::Client::builder() | ||
| .connect_timeout(Duration::from_secs(30)) | ||
| .timeout(Duration::from_secs(300)); |
| (size + 8 * 1024 * 1024 - 1) / (8 * 1024 * 1024) | ||
| ); | ||
|
|
||
| let key = format!("test-large-upload/{}.bin", std::process::id()); |
| )?; | ||
| let part_data = std::mem::replace(&mut chunk, Vec::with_capacity(chunk_size as usize)); | ||
| let part_len = part_data.len() as u64; | ||
| let part_offset = file.stream_position()? - part_len; |
Comment on lines
+482
to
+486
| let response = ensure_s3_success(send_with_retry(|| { | ||
| get_s3_client() | ||
| .post(url.clone()) | ||
| .timeout(S3_UPLOAD_REQUEST_TIMEOUT) | ||
| .send() |
Comment on lines
+553
to
+557
| let response = match send_with_retry(|| { | ||
| get_s3_client() | ||
| .post(url.clone()) | ||
| .timeout(S3_UPLOAD_REQUEST_TIMEOUT) | ||
| .header("content-type", "application/xml") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
S3 multipart uploads fail intermittently on large files (~1 GB) when a transient transport error (connection reset, timeout) hits any part. Without retry, a single failure on part N aborts the entire upload, wasting all previously uploaded parts. This caused bgpkit-broker backup uploads to fail consistently on Railway (part ~127 of 128, after ~105s).
Root Cause
The production bgpkit-broker on Railway uploads a ~1 GB SQLite backup to Cloudflare R2 (128 parts × 8 MB). Each part called
.send()?with zero retry — one dropped connection wasted 126 successful parts. The Railway→R2 network path has lower throughput (~10 MB/s vs ~18 MB/s from other hosts) and occasional connection resets.Reproduced: the same 1 GB upload succeeds 5/5 times from scouty (higher-bandwidth host), confirming the code is functionally correct but fragile against transient transport failures.
Changes
send_with_retry()helper: retries transient transport errors (timeout, connection reset, body decode) with exponential backoff. HTTP status errors (4xx/5xx) are not retried — they arrive asResponse, notError.ONEIO_S3_MAX_RETRIES(default 3),ONEIO_S3_RETRY_BACKOFF_MS(default 1000ms, doubles each attempt).S3Configremains source-compatible for downstream struct literals.Testing
tests/s3_large_upload.rs): 1 GB file, 128 parts, upload + size verification. Passed with retry code active (67.6s).cargo fmt --check,cargo clippy --all-features -- -D warnings,cargo clippy --no-default-features,cargo build --all-features,cargo build --no-default-featuresall clean.