From 1c94f7a06673a05d856f7bea69f40f326d65f908 Mon Sep 17 00:00:00 2001 From: Henry Guo Date: Mon, 11 May 2026 10:56:08 +0800 Subject: [PATCH] fix(sftp): classify backend errors by type (#2909) --- crates/protocols/src/common/dummy_storage.rs | 24 ++- crates/protocols/src/sftp/driver.rs | 8 +- crates/protocols/src/sftp/errors.rs | 157 +++++++++++++------ crates/protocols/src/sftp/write.rs | 6 +- 4 files changed, 129 insertions(+), 66 deletions(-) diff --git a/crates/protocols/src/common/dummy_storage.rs b/crates/protocols/src/common/dummy_storage.rs index 98e0fb4c8..4917021a1 100644 --- a/crates/protocols/src/common/dummy_storage.rs +++ b/crates/protocols/src/common/dummy_storage.rs @@ -44,23 +44,21 @@ use std::sync::{Arc, Mutex}; use thiserror::Error; use tokio::sync::Notify; -/// Error type returned by DummyBackend. Display strings include substrings -/// the driver's error-mapping helpers match against, so a queued NoSuchKey -/// error is reported as a not-found status at the protocol layer and an -/// AccessDenied error is reported as a permission-denied status. +/// Error type returned by DummyBackend. Variants model the backend error +/// categories that SFTP maps onto wire status codes. #[derive(Debug, Error)] pub enum DummyError { - /// Display includes the NoSuchKey substring. S3-style error mappers - /// map this to not-found. #[error("NoSuchKey: {0}")] NoSuchKey(String), - /// Display includes the NoSuchBucket substring. S3-style error mappers - /// map this to not-found. #[error("NoSuchBucket: {0}")] NoSuchBucket(String), - /// Free-form error string pre-seeded by a test. Must contain one of the - /// S3 error-code substrings if the test wants a specific status code - /// from the driver's error-mapping helper. + #[error("AccessDenied: {0}")] + AccessDenied(String), + #[error("NoSuchUpload: {0}")] + NoSuchUpload(String), + /// Free-form backend failure pre-seeded by a test. SFTP status-code + /// classification ignores this text; use a typed variant above when a test + /// needs a specific wire status. #[error("{0}")] Injected(String), /// Default response when the per-method queue is empty and the method @@ -295,9 +293,7 @@ impl DummyBackend { self.inner.lock().expect("lock").upload_part.push_back(Ok(out)); } - /// Queue an upload_part error. The error string flows through the - /// driver's error-mapping helper, so Injected("AccessDenied") produces - /// a permission-denied status at the driver boundary. + /// Queue an upload_part error. pub fn queue_upload_part_err(&self, err: DummyError) { self.inner.lock().expect("lock").upload_part.push_back(Err(err)); } diff --git a/crates/protocols/src/sftp/driver.rs b/crates/protocols/src/sftp/driver.rs index 4dbcca743..90acff2e8 100644 --- a/crates/protocols/src/sftp/driver.rs +++ b/crates/protocols/src/sftp/driver.rs @@ -25,8 +25,7 @@ use super::attrs; use super::constants::limits::S3_COPY_OBJECT_MAX_SIZE; -use super::constants::s3_error_codes; -use super::errors::{SftpError, auth_err, auth_err_unreachable, ok_status, s3_error_to_sftp}; +use super::errors::{SftpError, auth_err, auth_err_unreachable, is_no_such_upload_error, ok_status, s3_error_to_sftp}; use super::lifecycle::SessionDiag; use super::paths::{parse_s3_path, sanitise_control_bytes}; use super::state::{HandleState, WritePhase}; @@ -227,7 +226,7 @@ impl SftpDriver { pub(super) async fn run_backend(&self, op: &'static str, fut: F) -> Result where F: std::future::Future>, - E: std::fmt::Display, + E: std::fmt::Display + 'static, { match tokio::time::timeout(std::time::Duration::from_secs(self.backend_op_timeout_secs), fut).await { Ok(Ok(v)) => Ok(v), @@ -1076,8 +1075,7 @@ impl Drop for SftpDriver { // successful CompleteMultipartUpload, returning // NoSuchUpload. Log at debug to keep error-level // logs reserved for genuine abort failures. - let msg = e.to_string(); - if msg.contains(s3_error_codes::NO_SUCH_UPLOAD) { + if is_no_such_upload_error(&e) { tracing::debug!( bucket = %bucket, key = %key, diff --git a/crates/protocols/src/sftp/errors.rs b/crates/protocols/src/sftp/errors.rs index d53562886..35f46c85f 100644 --- a/crates/protocols/src/sftp/errors.rs +++ b/crates/protocols/src/sftp/errors.rs @@ -18,7 +18,8 @@ use super::constants::{http_error_codes, s3_error_codes}; use russh_sftp::protocol::{Status, StatusCode}; -use std::fmt::Display; +use s3s::{S3Error, S3ErrorCode}; +use std::{any::Any, fmt::Display}; /// Error type for SFTP operations. Converts to StatusCode for the wire. #[derive(Debug)] @@ -36,26 +37,72 @@ impl SftpError { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum BackendErrorKind { + NotFound, + PermissionDenied, + NoSuchUpload, + Other, +} + +fn classify_s3_code(code: &S3ErrorCode) -> BackendErrorKind { + match code { + S3ErrorCode::NoSuchKey | S3ErrorCode::NoSuchBucket => BackendErrorKind::NotFound, + S3ErrorCode::AccessDenied => BackendErrorKind::PermissionDenied, + S3ErrorCode::NoSuchUpload => BackendErrorKind::NoSuchUpload, + _ => match code.as_str() { + s3_error_codes::NO_SUCH_KEY + | s3_error_codes::NO_SUCH_BUCKET + | s3_error_codes::NOT_FOUND + | http_error_codes::NOT_FOUND => BackendErrorKind::NotFound, + s3_error_codes::ACCESS_DENIED | s3_error_codes::FORBIDDEN | http_error_codes::FORBIDDEN => { + BackendErrorKind::PermissionDenied + } + s3_error_codes::NO_SUCH_UPLOAD => BackendErrorKind::NoSuchUpload, + _ => BackendErrorKind::Other, + }, + } +} + +#[cfg(test)] +fn classify_dummy_error(err: &crate::common::dummy_storage::DummyError) -> BackendErrorKind { + match err { + crate::common::dummy_storage::DummyError::NoSuchKey(_) | crate::common::dummy_storage::DummyError::NoSuchBucket(_) => { + BackendErrorKind::NotFound + } + crate::common::dummy_storage::DummyError::AccessDenied(_) => BackendErrorKind::PermissionDenied, + crate::common::dummy_storage::DummyError::NoSuchUpload(_) => BackendErrorKind::NoSuchUpload, + crate::common::dummy_storage::DummyError::Injected(_) | crate::common::dummy_storage::DummyError::Unconfigured(_) => { + BackendErrorKind::Other + } + } +} + +fn classify_backend_error(err: &E) -> BackendErrorKind { + let any = err as &dyn Any; + if let Some(err) = any.downcast_ref::() { + return classify_s3_code(err.code()); + } + + #[cfg(test)] + if let Some(err) = any.downcast_ref::() { + return classify_dummy_error(err); + } + + BackendErrorKind::Other +} + /// Map an S3 backend error into an SFTP status code and log the underlying /// detail server-side. The wire response only carries the status code. The -/// full error is written to the server log for operator diagnosis. Error -/// strings that mention the common "not found" or "access denied" patterns -/// are mapped to the matching SFTP status. Everything else is Failure. -pub(super) fn s3_error_to_sftp(op: &str, err: E) -> SftpError { +/// full error is written to the server log for operator diagnosis. Typed +/// backend errors are mapped to the matching SFTP status. Everything else is +/// Failure. +pub(super) fn s3_error_to_sftp(op: &str, err: E) -> SftpError { let msg = err.to_string(); - let code = if msg.contains(s3_error_codes::NO_SUCH_KEY) - || msg.contains(s3_error_codes::NO_SUCH_BUCKET) - || msg.contains(s3_error_codes::NOT_FOUND) - || msg.contains(http_error_codes::NOT_FOUND) - { - StatusCode::NoSuchFile - } else if msg.contains(s3_error_codes::ACCESS_DENIED) - || msg.contains(s3_error_codes::FORBIDDEN) - || msg.contains(http_error_codes::FORBIDDEN) - { - StatusCode::PermissionDenied - } else { - StatusCode::Failure + let code = match classify_backend_error(&err) { + BackendErrorKind::NotFound => StatusCode::NoSuchFile, + BackendErrorKind::PermissionDenied => StatusCode::PermissionDenied, + BackendErrorKind::NoSuchUpload | BackendErrorKind::Other => StatusCode::Failure, }; tracing::warn!(op = %op, err = %msg, "SFTP backend error"); SftpError::code(code) @@ -94,20 +141,23 @@ pub(super) fn ok_status(id: u32) -> Status { } } -/// Classify an S3 backend error string as the not-found category that +/// Classify a backend error as the not-found category that /// distinguishes the EXCLUDE create accept path (object does not exist) -/// from a backend failure that needs propagating. Mirrors the prefix set +/// from a backend failure that needs propagating. Mirrors the typed set /// recognised by s3_error_to_sftp. -pub(super) fn is_not_found_error(err: &E) -> bool { - let msg = err.to_string(); - msg.contains(s3_error_codes::NO_SUCH_KEY) - || msg.contains(s3_error_codes::NO_SUCH_BUCKET) - || msg.contains(s3_error_codes::NOT_FOUND) - || msg.contains(http_error_codes::NOT_FOUND) +pub(super) fn is_not_found_error(err: &E) -> bool { + classify_backend_error(err) == BackendErrorKind::NotFound +} + +/// Returns true when AbortMultipartUpload reports an already-missing upload. +pub(super) fn is_no_such_upload_error(err: &E) -> bool { + classify_backend_error(err) == BackendErrorKind::NoSuchUpload } #[cfg(test)] mod tests { + use crate::common::dummy_storage::DummyError; + use super::*; #[test] @@ -120,23 +170,30 @@ mod tests { } #[test] - fn is_not_found_recognises_standard_error_patterns() { - struct E(&'static str); - impl std::fmt::Display for E { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(self.0) - } - } - assert!(is_not_found_error(&E("S3Error: NoSuchKey"))); - assert!(is_not_found_error(&E("backend returned NoSuchBucket"))); - assert!(is_not_found_error(&E("NotFound (404)"))); - assert!(is_not_found_error(&E("response status 404"))); - assert!(!is_not_found_error(&E("AccessDenied"))); - assert!(!is_not_found_error(&E("generic backend failure"))); + fn is_not_found_uses_s3_error_code_not_message_text() { + let err = S3Error::with_message(S3ErrorCode::NoSuchKey, "object missing"); + assert!(is_not_found_error(&err)); + + let err = S3Error::with_message(S3ErrorCode::NoSuchBucket, "bucket missing"); + assert!(is_not_found_error(&err)); + + let err = S3Error::with_message(S3ErrorCode::AccessDenied, "not found text in deny message"); + assert!(!is_not_found_error(&err)); } #[test] - fn s3_error_to_sftp_maps_access_denied_to_permission_denied() { + fn s3_error_to_sftp_uses_s3_error_code_not_message_text() { + let check = |code, msg| -> StatusCode { StatusCode::from(s3_error_to_sftp("test", S3Error::with_message(code, msg))) }; + assert!(matches!(check(S3ErrorCode::AccessDenied, "policy denied"), StatusCode::PermissionDenied)); + assert!(matches!(check(S3ErrorCode::NoSuchKey, "object missing"), StatusCode::NoSuchFile)); + assert!(matches!( + check(S3ErrorCode::InternalError, "AccessDenied appears only in message"), + StatusCode::Failure + )); + } + + #[test] + fn unknown_errors_do_not_classify_by_display_substrings() { struct E(&'static str); impl std::fmt::Display for E { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -144,10 +201,22 @@ mod tests { } } let check = |msg: &'static str| -> StatusCode { StatusCode::from(s3_error_to_sftp("test", E(msg))) }; - assert!(matches!(check("AccessDenied"), StatusCode::PermissionDenied)); - assert!(matches!(check("Forbidden"), StatusCode::PermissionDenied)); - assert!(matches!(check("403"), StatusCode::PermissionDenied)); - assert!(matches!(check("NoSuchKey"), StatusCode::NoSuchFile)); + assert!(matches!(check("AccessDenied"), StatusCode::Failure)); + assert!(matches!(check("Forbidden"), StatusCode::Failure)); + assert!(matches!(check("403"), StatusCode::Failure)); + assert!(matches!(check("NoSuchKey"), StatusCode::Failure)); assert!(matches!(check("something unexpected"), StatusCode::Failure)); } + + #[test] + fn no_such_upload_uses_s3_error_code() { + let err = S3Error::with_message(S3ErrorCode::NoSuchUpload, "upload is already gone"); + assert!(is_no_such_upload_error(&err)); + + let err = S3Error::with_message(S3ErrorCode::InternalError, "NoSuchUpload appears only in message"); + assert!(!is_no_such_upload_error(&err)); + + let err = DummyError::NoSuchUpload("upload is already gone".to_string()); + assert!(is_no_such_upload_error(&err)); + } } diff --git a/crates/protocols/src/sftp/write.rs b/crates/protocols/src/sftp/write.rs index aaeb6e28c..134269409 100644 --- a/crates/protocols/src/sftp/write.rs +++ b/crates/protocols/src/sftp/write.rs @@ -2049,7 +2049,7 @@ mod tests { #[tokio::test(flavor = "current_thread")] async fn run_backend_with_err_passes_backend_error_through_unchanged() { let backend = Arc::new(DummyBackend::new()); - backend.queue_head_object_err(DummyError::Injected("AccessDenied: pinned".to_string())); + backend.queue_head_object_err(DummyError::AccessDenied("pinned".to_string())); let driver = build_driver(backend, TEST_PART_SIZE); let result = driver @@ -2057,7 +2057,7 @@ mod tests { .await; match result { - Ok(Err(e)) => assert!(format!("{e}").contains("AccessDenied")), + Ok(Err(e)) => assert!(matches!(e, DummyError::AccessDenied(_))), other => panic!("expected backend Err passed through; got {other:?}"), } } @@ -2120,7 +2120,7 @@ mod tests { #[tokio::test(flavor = "current_thread")] async fn commit_write_does_not_retry_on_access_denied() { let backend = Arc::new(DummyBackend::new()); - backend.queue_put_object_err(DummyError::Injected("AccessDenied: policy".into())); + backend.queue_put_object_err(DummyError::AccessDenied("policy".into())); // A second response so a retry attempt would surface a // wrong-status assertion failure rather than the // configured-miss default. If the loop wrongly retries, the