From 83d44be0f30f7b2cffb6bfdef8041b4bd8bc75c2 Mon Sep 17 00:00:00 2001 From: Eliah Rusin Date: Sat, 26 Sep 2026 06:59:09 +0300 Subject: [PATCH] volume: detect S3 not-found by typed SDK errors, not the "service error" string (#11444) * volume: detect S3 not-found by typed SDK errors, not the "service error" string remote_storage/s3.rs decided ObjectNotFound by matching the Display output of an aws_sdk_s3 SdkError against "NoSuchKey" / "404" / "NotFound". In the locked SDK (aws-smithy-runtime-api 1.11.6, src/client/result.rs:487-497) that Display is a fixed string per variant, "service error" for every S3 error, so ObjectNotFound was unreachable: every missing remote object surfaced as Other("s3 get object: service error") with the real cause discarded. Go (weed/remote_storage/s3/s3_storage_client.go) uses typed checks: HEAD (373-374): awserr.RequestFailure with StatusCode() == 404; GET (436-437): awserr.Error with Code() == s3.ErrCodeNoSuchKey. read_file now matches SdkError::ServiceError whose GetObjectError is_no_such_key(); a bare 404 on GET stays a generic error, as in Go. stat_file matches HeadObjectError::is_not_found() or a raw HTTP 404 status, Go's actual condition. Non-service errors fall through to Other unchanged. Every SdkError message in s3.rs and s3_tier.rs is formatted with DisplayErrorContext so the S3 error code and message survive instead of "service error". Six network-free unit tests drive the client through a canned HttpClient (404 NoSuchKey, bare 404 on GET and HEAD, 404 with a foreign body on HEAD, 403 AccessDenied on GET and HEAD). They need aws-smithy-runtime-api as a dev-dependency; it is already in the lock at a single version, so no new crates. Co-Authored-By: Claude Fable 5.1 * volume: HEAD not-found is the raw 404 status alone, as in Go Review follow-up. The HEAD arm also accepted the SDK's NotFound error code on any status, so a 400 carrying NotFound became a missing object. Go's stat looks only at RequestFailure.StatusCode() == 404 (weed/remote_storage/s3/s3_storage_client.go:373); do the same. The raw status still covers the body-less 404 the SDK turns into NotFound and a 404 whose body names a foreign code. Regression test for the non-404 NotFound body, which failed against the previous arm. Co-Authored-By: Claude Fable 5.1 * volume: trim comments on the typed S3 not-found checks Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: franchb Co-authored-by: Claude Fable 5.1 Co-authored-by: Chris Lu Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- seaweed-volume/Cargo.lock | 1 + seaweed-volume/Cargo.toml | 3 + seaweed-volume/src/remote_storage/s3.rs | 240 +++++++++++++++++-- seaweed-volume/src/remote_storage/s3_tier.rs | 67 +++++- 4 files changed, 281 insertions(+), 30 deletions(-) diff --git a/seaweed-volume/Cargo.lock b/seaweed-volume/Cargo.lock index 7c459a2db..8e6393efc 100644 --- a/seaweed-volume/Cargo.lock +++ b/seaweed-volume/Cargo.lock @@ -4555,6 +4555,7 @@ dependencies = [ "aws-config", "aws-credential-types", "aws-sdk-s3", + "aws-smithy-runtime-api", "aws-types", "axum", "base64", diff --git a/seaweed-volume/Cargo.toml b/seaweed-volume/Cargo.toml index e47de98ee..1e4a58f2a 100644 --- a/seaweed-volume/Cargo.toml +++ b/seaweed-volume/Cargo.toml @@ -157,6 +157,9 @@ windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem"] } [dev-dependencies] tempfile = "3" +# Already a transitive dependency of aws-sdk-s3 at a single locked version; +# needed directly only for the canned HttpClient in remote_storage::s3 tests. +aws-smithy-runtime-api = "1" [build-dependencies] tonic-prost-build = "0.14" diff --git a/seaweed-volume/src/remote_storage/s3.rs b/seaweed-volume/src/remote_storage/s3.rs index 8b86e64e4..03731c03f 100644 --- a/seaweed-volume/src/remote_storage/s3.rs +++ b/seaweed-volume/src/remote_storage/s3.rs @@ -4,6 +4,7 @@ use aws_sdk_s3::Client; use aws_sdk_s3::config::{BehaviorVersion, Credentials, Region}; +use aws_sdk_s3::error::{DisplayErrorContext, SdkError}; use aws_sdk_s3::primitives::ByteStream; use super::{RemoteEntry, RemoteStorageClient, RemoteStorageError}; @@ -25,6 +26,23 @@ impl S3RemoteStorageClient { endpoint: &str, force_path_style: bool, ) -> Self { + let client = Client::from_conf( + Self::config_builder(access_key, secret_key, region, endpoint, force_path_style) + .build(), + ); + + S3RemoteStorageClient { client, conf } + } + + /// Build the SDK config for the given credentials and endpoint. Split out so + /// tests can attach a canned HTTP client before building the [`Client`]. + fn config_builder( + access_key: &str, + secret_key: &str, + region: &str, + endpoint: &str, + force_path_style: bool, + ) -> aws_sdk_s3::config::Builder { let region = if region.is_empty() { "us-east-1" } else { @@ -49,9 +67,7 @@ impl S3RemoteStorageClient { s3_config = s3_config.endpoint_url(endpoint); } - let client = Client::from_conf(s3_config.build()); - - S3RemoteStorageClient { client, conf } + s3_config } } @@ -75,13 +91,14 @@ impl RemoteStorageClient for S3RemoteStorageClient { req = req.range(format!("bytes={}-", offset)); } - let resp = req.send().await.map_err(|e| { - let msg = format!("{}", e); - if msg.contains("NoSuchKey") || msg.contains("404") { + let resp = req.send().await.map_err(|e| match e { + // Go compares `aerr.Code()` to NoSuchKey on GET + // (s3_storage_client.go:436): a bare 404 maps to "NotFound" + // and stays a generic error, as it does here. + SdkError::ServiceError(ref se) if se.err().is_no_such_key() => { RemoteStorageError::ObjectNotFound(format!("{}/{}", loc.bucket, key)) - } else { - RemoteStorageError::Other(format!("s3 get object: {}", e)) } + e => RemoteStorageError::Other(format!("s3 get object: {}", DisplayErrorContext(&e))), })?; let data = resp @@ -108,7 +125,9 @@ impl RemoteStorageClient for S3RemoteStorageClient { .body(ByteStream::from(data.to_vec())) .send() .await - .map_err(|e| RemoteStorageError::Other(format!("s3 put object: {}", e)))?; + .map_err(|e| { + RemoteStorageError::Other(format!("s3 put object: {}", DisplayErrorContext(&e))) + })?; Ok(RemoteEntry { size: data.len() as i64, @@ -134,13 +153,18 @@ impl RemoteStorageClient for S3RemoteStorageClient { .key(key) .send() .await - .map_err(|e| { - let msg = format!("{}", e); - if msg.contains("404") || msg.contains("NotFound") { + .map_err(|e| match e { + // Go checks only the raw HTTP status on HEAD + // (s3_storage_client.go:373): a HEAD response carries no + // error body, so a 404 is not-found whatever code the SDK + // assigns, and a non-404 is not. + SdkError::ServiceError(ref se) if se.raw().status().as_u16() == 404 => { RemoteStorageError::ObjectNotFound(format!("{}/{}", loc.bucket, key)) - } else { - RemoteStorageError::Other(format!("s3 head object: {}", e)) } + e => RemoteStorageError::Other(format!( + "s3 head object: {}", + DisplayErrorContext(&e) + )), })?; Ok(RemoteEntry { @@ -160,18 +184,17 @@ impl RemoteStorageClient for S3RemoteStorageClient { .key(key) .send() .await - .map_err(|e| RemoteStorageError::Other(format!("s3 delete object: {}", e)))?; + .map_err(|e| { + RemoteStorageError::Other(format!("s3 delete object: {}", DisplayErrorContext(&e))) + })?; Ok(()) } async fn list_buckets(&self) -> Result, RemoteStorageError> { - let resp = self - .client - .list_buckets() - .send() - .await - .map_err(|e| RemoteStorageError::Other(format!("s3 list buckets: {}", e)))?; + let resp = self.client.list_buckets().send().await.map_err(|e| { + RemoteStorageError::Other(format!("s3 list buckets: {}", DisplayErrorContext(&e))) + })?; Ok(resp .buckets() @@ -184,3 +207,178 @@ impl RemoteStorageClient for S3RemoteStorageClient { &self.conf } } + +#[cfg(test)] +mod tests { + use super::*; + use aws_sdk_s3::config::http::{HttpRequest, HttpResponse}; + use aws_sdk_s3::config::retry::RetryConfig; + use aws_sdk_s3::config::{HttpClient, RuntimeComponents}; + use aws_sdk_s3::primitives::SdkBody; + use aws_smithy_runtime_api::client::http::{ + HttpConnector, HttpConnectorFuture, HttpConnectorSettings, SharedHttpConnector, + }; + use aws_smithy_runtime_api::http::StatusCode; + + /// An SDK HTTP client that answers every request with one canned response, + /// so the error-mapping paths can be exercised without a network or a + /// running S3 server. + #[derive(Debug, Clone)] + struct CannedResponse { + status: u16, + body: &'static str, + } + + impl HttpConnector for CannedResponse { + fn call(&self, _request: HttpRequest) -> HttpConnectorFuture { + let status = StatusCode::try_from(self.status).expect("valid HTTP status"); + HttpConnectorFuture::ready(Ok(HttpResponse::new(status, SdkBody::from(self.body)))) + } + } + + impl HttpClient for CannedResponse { + fn http_connector( + &self, + _settings: &HttpConnectorSettings, + _components: &RuntimeComponents, + ) -> SharedHttpConnector { + SharedHttpConnector::new(self.clone()) + } + } + + fn client_with(status: u16, body: &'static str) -> S3RemoteStorageClient { + let config = S3RemoteStorageClient::config_builder( + "AKIATEST", + "secret", + "us-east-1", + "http://127.0.0.1:1", + true, + ) + .http_client(CannedResponse { status, body }) + .retry_config(RetryConfig::disabled()) + .build(); + S3RemoteStorageClient { + client: Client::from_conf(config), + conf: RemoteConf::default(), + } + } + + fn location() -> RemoteStorageLocation { + RemoteStorageLocation { + name: "remote".to_string(), + bucket: "bucket".to_string(), + path: "/dir/missing".to_string(), + ..Default::default() + } + } + + const NO_SUCH_KEY: &str = r#" +NoSuchKeyThe specified key does not exist.dir/missing"#; + + const NOT_FOUND_BODY: &str = r#" +NotFoundNot Found"#; + + const ACCESS_DENIED: &str = r#" +AccessDeniedAccess Denied"#; + + #[tokio::test] + async fn get_no_such_key_is_object_not_found() { + let err = client_with(404, NO_SUCH_KEY) + .read_file(&location(), 0, 0) + .await + .unwrap_err(); + assert!( + matches!(&err, RemoteStorageError::ObjectNotFound(path) if path == "bucket/dir/missing"), + "expected ObjectNotFound, got {err:?}" + ); + } + + #[tokio::test] + async fn get_bare_404_is_not_object_not_found() { + // Go compares codes, not statuses, on GET: a body-less 404 stays generic. + let err = client_with(404, "") + .read_file(&location(), 0, 0) + .await + .unwrap_err(); + assert!( + matches!(err, RemoteStorageError::Other(_)), + "expected Other, got {err:?}" + ); + } + + #[tokio::test] + async fn head_404_is_object_not_found() { + let err = client_with(404, "") + .stat_file(&location()) + .await + .unwrap_err(); + assert!( + matches!(&err, RemoteStorageError::ObjectNotFound(path) if path == "bucket/dir/missing"), + "expected ObjectNotFound, got {err:?}" + ); + } + + #[tokio::test] + async fn head_404_with_foreign_error_body_is_object_not_found() { + // The raw status check makes a 404 not-found whatever body it carries. + let err = client_with(404, NO_SUCH_KEY) + .stat_file(&location()) + .await + .unwrap_err(); + assert!( + matches!(err, RemoteStorageError::ObjectNotFound(_)), + "expected ObjectNotFound, got {err:?}" + ); + } + + #[tokio::test] + async fn head_not_found_code_on_a_non_404_status_is_not_object_not_found() { + // A NotFound body on a non-404 status stays an error, as in Go. + let err = client_with(400, NOT_FOUND_BODY) + .stat_file(&location()) + .await + .unwrap_err(); + assert!( + matches!(&err, RemoteStorageError::Other(msg) if msg.contains("NotFound")), + "expected Other naming the code, got {err:?}" + ); + } + + #[tokio::test] + async fn get_access_denied_keeps_service_error_code() { + let err = client_with(403, ACCESS_DENIED) + .read_file(&location(), 0, 0) + .await + .unwrap_err(); + let msg = err.to_string(); + assert!( + matches!(err, RemoteStorageError::Other(_)), + "expected Other, got {err:?}" + ); + assert!( + msg.contains("AccessDenied"), + "message should carry the S3 error code, got: {msg}" + ); + assert!( + !msg.ends_with("service error"), + "message should not be the bare SdkError Display, got: {msg}" + ); + } + + #[tokio::test] + async fn head_access_denied_keeps_service_error_code() { + let err = client_with(403, ACCESS_DENIED) + .stat_file(&location()) + .await + .unwrap_err(); + let msg = err.to_string(); + assert!( + matches!(err, RemoteStorageError::Other(_)), + "expected Other, got {err:?}" + ); + assert!( + msg.contains("AccessDenied"), + "message should carry the S3 error code, got: {msg}" + ); + } +} diff --git a/seaweed-volume/src/remote_storage/s3_tier.rs b/seaweed-volume/src/remote_storage/s3_tier.rs index 570a093d5..3d6390356 100644 --- a/seaweed-volume/src/remote_storage/s3_tier.rs +++ b/seaweed-volume/src/remote_storage/s3_tier.rs @@ -9,6 +9,7 @@ use std::sync::{Arc, OnceLock, RwLock}; use aws_sdk_s3::Client; use aws_sdk_s3::config::{BehaviorVersion, Credentials, Region}; +use aws_sdk_s3::error::DisplayErrorContext; use aws_sdk_s3::types::{CompletedMultipartUpload, CompletedPart}; use tokio::io::{AsyncReadExt, AsyncSeekExt, AsyncWriteExt}; use tokio::sync::Semaphore; @@ -119,7 +120,12 @@ impl S3TierBackend { ) .send() .await - .map_err(|e| format!("failed to create multipart upload: {}", e))?; + .map_err(|e| { + format!( + "failed to create multipart upload: {}", + DisplayErrorContext(&e) + ) + })?; let upload_id = create_resp .upload_id() @@ -183,7 +189,12 @@ impl S3TierBackend { .send() .await .map_err(|e| { - format!("failed to upload part {} at offset {}: {}", pn, off, e) + format!( + "failed to upload part {} at offset {}: {}", + pn, + off, + DisplayErrorContext(&e) + ) })?; let e_tag = upload_part_resp.e_tag().unwrap_or_default().to_string(); @@ -236,7 +247,12 @@ impl S3TierBackend { .multipart_upload(completed_upload) .send() .await - .map_err(|e| format!("failed to complete multipart upload: {}", e))?; + .map_err(|e| { + format!( + "failed to complete multipart upload: {}", + DisplayErrorContext(&e) + ) + })?; Ok::<(), String>(()) } @@ -293,7 +309,7 @@ impl S3TierBackend { .key(key) .send() .await - .map_err(|e| format!("failed to head object {}: {}", key, e))?; + .map_err(|e| format!("failed to head object {}: {}", key, DisplayErrorContext(&e)))?; let file_size = head_resp.content_length().unwrap_or(0) as u64; @@ -356,7 +372,14 @@ impl S3TierBackend { .range(&range) .send() .await - .map_err(|e| format!("failed to get object {} range {}: {}", key, range, e))?; + .map_err(|e| { + format!( + "failed to get object {} range {}: {}", + key, + range, + DisplayErrorContext(&e) + ) + })?; let body = get_resp .body @@ -431,7 +454,14 @@ impl S3TierBackend { .range(&range) .send() .await - .map_err(|e| format!("failed to get object {} range {}: {}", key, range, e))?; + .map_err(|e| { + format!( + "failed to get object {} range {}: {}", + key, + range, + DisplayErrorContext(&e) + ) + })?; let body = resp .body @@ -449,7 +479,13 @@ impl S3TierBackend { .key(key) .send() .await - .map_err(|e| format!("failed to delete object {}: {}", key, e))?; + .map_err(|e| { + format!( + "failed to delete object {}: {}", + key, + DisplayErrorContext(&e) + ) + })?; Ok(()) } @@ -464,7 +500,13 @@ impl S3TierBackend { .key(&key) .send() .await - .map_err(|e| format!("failed to delete object {}: {}", key, e))?; + .map_err(|e| { + format!( + "failed to delete object {}: {}", + key, + DisplayErrorContext(&e) + ) + })?; Ok(()) }) } @@ -488,7 +530,14 @@ impl S3TierBackend { .range(&range) .send() .await - .map_err(|e| format!("failed to get object {} range {}: {}", key, range, e))?; + .map_err(|e| { + format!( + "failed to get object {} range {}: {}", + key, + range, + DisplayErrorContext(&e) + ) + })?; let body = resp .body