fix(sftp): classify backend errors by type (#2909)

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
+/// Error type returned by DummyBackend. Variants model the backend error
-/// the driver's error-mapping helpers match against, so a queued NoSuchKey
+/// categories that SFTP maps onto wire status codes.
-/// error is reported as a not-found status at the protocol layer and an
-/// AccessDenied error is reported as a permission-denied status.
 #[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
+    #[error("AccessDenied: {0}")]
-    /// S3 error-code substrings if the test wants a specific status code
+    AccessDenied(String),
-    /// from the driver's error-mapping helper.
+    #[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
+    /// Queue an upload_part error.
-    /// driver's error-mapping helper, so Injected("AccessDenied") produces
-    /// a permission-denied status at the driver boundary.
     pub fn queue_upload_part_err(&self, err: DummyError) {
         self.inner.lock().expect("lock").upload_part.push_back(Err(err));
     }

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, is_no_such_upload_error, ok_status, s3_error_to_sftp};
-use super::errors::{SftpError, auth_err, auth_err_unreachable, 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<S: StorageBackend + Send + Sync + 'static> SftpDriver<S> {
     pub(super) async fn run_backend<F, T, E>(&self, op: &'static str, fut: F) -> Result<T, SftpError>
     where
         F: std::future::Future<Output = Result<T, E>>,
-        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<S: StorageBackend + Send + Sync + 'static> Drop for SftpDriver<S> {
                         // successful CompleteMultipartUpload, returning
                         // NoSuchUpload. Log at debug to keep error-level
                         // logs reserved for genuine abort failures.
-                        let msg = e.to_string();
+                        if is_no_such_upload_error(&e) {
-                        if msg.contains(s3_error_codes::NO_SUCH_UPLOAD) {
                             tracing::debug!(
                                 bucket = %bucket,
                                 key = %key,

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<E: Display + 'static>(err: &E) -> BackendErrorKind {
+    let any = err as &dyn Any;
+    if let Some(err) = any.downcast_ref::<S3Error>() {
+        return classify_s3_code(err.code());
+    }
+
+    #[cfg(test)]
+    if let Some(err) = any.downcast_ref::<crate::common::dummy_storage::DummyError>() {
+        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
+/// full error is written to the server log for operator diagnosis. Typed
-/// strings that mention the common "not found" or "access denied" patterns
+/// backend errors are mapped to the matching SFTP status. Everything else is
-/// are mapped to the matching SFTP status. Everything else is Failure.
+/// Failure.
-pub(super) fn s3_error_to_sftp<E: Display>(op: &str, err: E) -> SftpError {
+pub(super) fn s3_error_to_sftp<E: Display + 'static>(op: &str, err: E) -> SftpError {
     let msg = err.to_string();
-    let code = if msg.contains(s3_error_codes::NO_SUCH_KEY)
+    let code = match classify_backend_error(&err) {
-        || msg.contains(s3_error_codes::NO_SUCH_BUCKET)
+        BackendErrorKind::NotFound => StatusCode::NoSuchFile,
-        || msg.contains(s3_error_codes::NOT_FOUND)
+        BackendErrorKind::PermissionDenied => StatusCode::PermissionDenied,
-        || msg.contains(http_error_codes::NOT_FOUND)
+        BackendErrorKind::NoSuchUpload | BackendErrorKind::Other => StatusCode::Failure,
-    {
-        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
     };
     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<E: Display>(err: &E) -> bool {
+pub(super) fn is_not_found_error<E: Display + 'static>(err: &E) -> bool {
-    let msg = err.to_string();
+    classify_backend_error(err) == BackendErrorKind::NotFound
-    msg.contains(s3_error_codes::NO_SUCH_KEY)
+}
-        || msg.contains(s3_error_codes::NO_SUCH_BUCKET)
+
-        || msg.contains(s3_error_codes::NOT_FOUND)
+/// Returns true when AbortMultipartUpload reports an already-missing upload.
-        || msg.contains(http_error_codes::NOT_FOUND)
+pub(super) fn is_no_such_upload_error<E: Display + 'static>(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() {
+    fn is_not_found_uses_s3_error_code_not_message_text() {
-        struct E(&'static str);
+        let err = S3Error::with_message(S3ErrorCode::NoSuchKey, "object missing");
-        impl std::fmt::Display for E {
+        assert!(is_not_found_error(&err));
-            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+
-                f.write_str(self.0)
+        let err = S3Error::with_message(S3ErrorCode::NoSuchBucket, "bucket missing");
-            }
+        assert!(is_not_found_error(&err));
-        }
+
-        assert!(is_not_found_error(&E("S3Error: NoSuchKey")));
+        let err = S3Error::with_message(S3ErrorCode::AccessDenied, "not found text in deny message");
-        assert!(is_not_found_error(&E("backend returned NoSuchBucket")));
+        assert!(!is_not_found_error(&err));
-        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")));
     }
 
     #[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("AccessDenied"), StatusCode::Failure));
-        assert!(matches!(check("Forbidden"), StatusCode::PermissionDenied));
+        assert!(matches!(check("Forbidden"), StatusCode::Failure));
-        assert!(matches!(check("403"), StatusCode::PermissionDenied));
+        assert!(matches!(check("403"), StatusCode::Failure));
-        assert!(matches!(check("NoSuchKey"), StatusCode::NoSuchFile));
+        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));
+    }
 }

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