diff --git a/Cargo.lock b/Cargo.lock index 19bb60c5..f5652c62 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1343,6 +1343,7 @@ dependencies = [ "sha2 0.10.9", "tokio", "tracing", + "tracing-subscriber", "uuid", ] diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 3634c915..313a0d03 100755 --- a/crates/engine/Cargo.toml +++ b/crates/engine/Cargo.toml @@ -21,3 +21,6 @@ hmac = "0.12" sha2 = "0.10" hex = "0.4" tokio = { workspace = true } + +[dev-dependencies] +tracing-subscriber = { workspace = true } diff --git a/crates/engine/src/backup.rs b/crates/engine/src/backup.rs index 67114a01..2670643a 100755 --- a/crates/engine/src/backup.rs +++ b/crates/engine/src/backup.rs @@ -1250,7 +1250,7 @@ pub(crate) async fn handle_restore_table_to_point_in_time( } /// Convert storage errors to `DynamoDB` errors. -fn storage_err_to_dynamo(e: StorageError) -> DynamoDbError { +pub(crate) fn storage_err_to_dynamo(e: StorageError) -> DynamoDbError { match e { StorageError::TableNotFound(msg) => DynamoDbError::ResourceNotFoundException(msg), StorageError::TableAlreadyExists(msg) @@ -1278,6 +1278,9 @@ fn storage_err_to_dynamo(e: StorageError) -> DynamoDbError { StorageError::Unsupported(msg) => DynamoDbError::ValidationException(msg), StorageError::LimitExceeded(msg) => DynamoDbError::LimitExceededException(msg), StorageError::BackupInUse(msg) => DynamoDbError::BackupInUseException(msg), + // A backend-classified client error passes through unchanged, before + // the catch-all that reports a storage fault. + StorageError::Service(e) => e, other => { tracing::error!(internal_error = %other, "backup storage error"); DynamoDbError::InternalServerError("Internal server error".to_owned()) @@ -1287,4 +1290,6 @@ fn storage_err_to_dynamo(e: StorageError) -> DynamoDbError { mod overrides; #[cfg(test)] +mod service_error_tests; +#[cfg(test)] mod tests; diff --git a/crates/engine/src/backup/service_error_tests.rs b/crates/engine/src/backup/service_error_tests.rs new file mode 100644 index 00000000..db01c826 --- /dev/null +++ b/crates/engine/src/backup/service_error_tests.rs @@ -0,0 +1,93 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 +//! A backend-classified client error reaches the client unchanged through the +//! backup mapping, keeping its variant and message. Control: a raw `Internal` +//! message is still redacted behind the fixed string. + +use extenddb_core::error::DynamoDbError; +use extenddb_storage::error::StorageError; + +use super::{ + create_backup_storage_err_to_dynamo, restore_storage_err_to_dynamo, storage_err_to_dynamo, +}; + +const INTERNAL_MSG: &str = + "The server encountered an internal error trying to fulfill the request."; +const UNAVAILABLE_MSG: &str = "The database is temporarily unavailable, retry shortly."; +const CAPACITY_MSG: &str = + "The level of configured provisioned throughput for the table was exceeded."; + +#[test] +fn service_internal_server_error_passes_through() { + let err = storage_err_to_dynamo(StorageError::Service(DynamoDbError::InternalServerError( + INTERNAL_MSG.to_owned(), + ))); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, INTERNAL_MSG), + other => panic!("expected InternalServerError, got {other:?}"), + } +} + +#[test] +fn service_service_unavailable_passes_through() { + let err = storage_err_to_dynamo(StorageError::Service(DynamoDbError::ServiceUnavailable( + UNAVAILABLE_MSG.to_owned(), + ))); + match err { + DynamoDbError::ServiceUnavailable(msg) => assert_eq!(msg, UNAVAILABLE_MSG), + other => panic!("expected ServiceUnavailable, got {other:?}"), + } +} + +#[test] +fn service_capacity_error_passes_through() { + let err = storage_err_to_dynamo(StorageError::Service( + DynamoDbError::ProvisionedThroughputExceededException(CAPACITY_MSG.to_owned()), + )); + match err { + DynamoDbError::ProvisionedThroughputExceededException(msg) => { + assert_eq!(msg, CAPACITY_MSG); + } + other => panic!("expected ProvisionedThroughputExceededException, got {other:?}"), + } +} + +// The restore mapping delegates everything it does not name, so a Service +// error passes through it as well. +#[test] +fn service_error_passes_through_the_restore_mapping() { + let err = restore_storage_err_to_dynamo(StorageError::Service( + DynamoDbError::BackupNotFoundException("Backup not found: arn".to_owned()), + )); + match err { + DynamoDbError::BackupNotFoundException(msg) => assert_eq!(msg, "Backup not found: arn"), + other => panic!("expected BackupNotFoundException, got {other:?}"), + } +} + +// The create-backup mapping names two table-state variants and delegates the +// rest, so a Service error passes through it too. +#[test] +fn service_error_passes_through_the_create_backup_mapping() { + let err = create_backup_storage_err_to_dynamo( + StorageError::Service(DynamoDbError::LimitExceededException( + "Too many backups in progress".to_owned(), + )), + "t", + ); + match err { + DynamoDbError::LimitExceededException(msg) => { + assert_eq!(msg, "Too many backups in progress"); + } + other => panic!("expected LimitExceededException, got {other:?}"), + } +} + +#[test] +fn internal_driver_text_is_still_redacted() { + let err = storage_err_to_dynamo(StorageError::Internal("driver text".to_owned())); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "Internal server error"), + other => panic!("expected InternalServerError, got {other:?}"), + } +} diff --git a/crates/engine/src/create_table.rs b/crates/engine/src/create_table.rs index 424e27b5..34c56a0b 100755 --- a/crates/engine/src/create_table.rs +++ b/crates/engine/src/create_table.rs @@ -220,6 +220,17 @@ pub fn storage_err_to_dynamo(e: extenddb_storage::error::StorageError) -> Dynamo tracing::error!(internal_error = %msg, "storage internal error"); DynamoDbError::InternalServerError("Internal server error".to_owned()) } + // The backend already classified this failure into the service error + // vocabulary and chose the client-facing message. Pass it through + // with its variant and message unchanged; it is not a storage fault, + // so it is not logged as one. The one rule the engine keeps for itself + // is the item on a conditional check error: this generic path discards + // it, as it does for its own ConditionFailed, and + // `storage_err_to_dynamo_with_ccf` returns it when the caller asked. + StorageError::Service(DynamoDbError::ConditionalCheckFailedException(msg, _)) => { + DynamoDbError::ConditionalCheckFailedException(msg, None) + } + StorageError::Service(e) => e, } } @@ -244,6 +255,212 @@ pub(crate) fn storage_err_to_dynamo_with_ccf( return_item, ) } + // The engine applies the ReturnValuesOnConditionCheckFailure rule to a + // backend-classified conditional check error as it does to its own: the + // message is the backend's, and the item crosses only when the caller + // asked for ALL_OLD, which only this mapper can see. + StorageError::Service(DynamoDbError::ConditionalCheckFailedException(msg, item)) => { + let return_item = if ccf == ReturnValuesOnConditionCheckFailure::AllOld { + item + } else { + None + }; + DynamoDbError::ConditionalCheckFailedException(msg, return_item) + } other => storage_err_to_dynamo(other), } } + +#[cfg(test)] +mod storage_error_mapping_tests { + use super::{storage_err_to_dynamo, storage_err_to_dynamo_with_ccf}; + use extenddb_core::error::DynamoDbError; + use extenddb_core::types::ReturnValuesOnConditionCheckFailure; + use extenddb_storage::error::StorageError; + + const INTERNAL_MSG: &str = + "The server encountered an internal error trying to fulfill the request."; + const UNAVAILABLE_MSG: &str = "The database is temporarily unavailable, retry shortly."; + const CAPACITY_MSG: &str = + "The level of configured provisioned throughput for the table was exceeded."; + + // storage_err_to_dynamo: a backend-classified error reaches the client + // unchanged, keeping both its variant and its message. + + #[test] + fn service_internal_server_error_passes_through() { + let err = storage_err_to_dynamo(StorageError::Service(DynamoDbError::InternalServerError( + INTERNAL_MSG.to_owned(), + ))); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, INTERNAL_MSG), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + + #[test] + fn service_service_unavailable_passes_through() { + let err = storage_err_to_dynamo(StorageError::Service(DynamoDbError::ServiceUnavailable( + UNAVAILABLE_MSG.to_owned(), + ))); + match err { + DynamoDbError::ServiceUnavailable(msg) => assert_eq!(msg, UNAVAILABLE_MSG), + other => panic!("expected ServiceUnavailable, got {other:?}"), + } + } + + // Any service error class passes through, not only the two above. + #[test] + fn service_capacity_error_passes_through() { + let err = storage_err_to_dynamo(StorageError::Service( + DynamoDbError::ProvisionedThroughputExceededException(CAPACITY_MSG.to_owned()), + )); + match err { + DynamoDbError::ProvisionedThroughputExceededException(msg) => { + assert_eq!(msg, CAPACITY_MSG); + } + other => panic!("expected ProvisionedThroughputExceededException, got {other:?}"), + } + } + + // Control: a raw driver message is still redacted behind the fixed string. + #[test] + fn internal_driver_text_is_still_redacted() { + let err = storage_err_to_dynamo(StorageError::Internal("driver text".to_owned())); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "Internal server error"), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + + // storage_err_to_dynamo_with_ccf delegates everything but ConditionFailed, + // so the same four cases hold through it. + + #[test] + fn service_internal_server_error_passes_through_with_ccf() { + let err = storage_err_to_dynamo_with_ccf( + StorageError::Service(DynamoDbError::InternalServerError(INTERNAL_MSG.to_owned())), + ReturnValuesOnConditionCheckFailure::None, + ); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, INTERNAL_MSG), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + + #[test] + fn service_service_unavailable_passes_through_with_ccf() { + let err = storage_err_to_dynamo_with_ccf( + StorageError::Service(DynamoDbError::ServiceUnavailable( + UNAVAILABLE_MSG.to_owned(), + )), + ReturnValuesOnConditionCheckFailure::AllOld, + ); + match err { + DynamoDbError::ServiceUnavailable(msg) => assert_eq!(msg, UNAVAILABLE_MSG), + other => panic!("expected ServiceUnavailable, got {other:?}"), + } + } + + #[test] + fn service_capacity_error_passes_through_with_ccf() { + let err = storage_err_to_dynamo_with_ccf( + StorageError::Service(DynamoDbError::ProvisionedThroughputExceededException( + CAPACITY_MSG.to_owned(), + )), + ReturnValuesOnConditionCheckFailure::None, + ); + match err { + DynamoDbError::ProvisionedThroughputExceededException(msg) => { + assert_eq!(msg, CAPACITY_MSG); + } + other => panic!("expected ProvisionedThroughputExceededException, got {other:?}"), + } + } + + #[test] + fn internal_driver_text_is_still_redacted_with_ccf() { + let err = storage_err_to_dynamo_with_ccf( + StorageError::Internal("driver text".to_owned()), + ReturnValuesOnConditionCheckFailure::None, + ); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "Internal server error"), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + + // A Service error carrying ConditionalCheckFailedException keeps the + // backend's message, and its item follows the caller's + // ReturnValuesOnConditionCheckFailure like the engine's own ConditionFailed. + fn backend_item() -> extenddb_core::types::Item { + let mut item = extenddb_core::types::Item::new(); + item.insert( + "pk".to_owned(), + extenddb_core::types::AttributeValue::S("secret".to_owned()), + ); + item + } + + // The generic mapper has no ReturnValuesOnConditionCheckFailure to read, so + // it discards the item from a backend's conditional check error the way it + // does from the engine's own, and keeps the message. + #[test] + fn service_condition_failed_drops_the_backend_item_on_the_generic_path() { + let err = storage_err_to_dynamo(StorageError::Service( + DynamoDbError::ConditionalCheckFailedException( + "The conditional request failed".to_owned(), + Some(backend_item()), + ), + )); + match err { + DynamoDbError::ConditionalCheckFailedException(msg, item) => { + assert_eq!(msg, "The conditional request failed"); + assert!( + item.is_none(), + "item reached the client on the generic path: {item:?}" + ); + } + other => panic!("expected ConditionalCheckFailedException, got {other:?}"), + } + } + + #[test] + fn service_condition_failed_drops_the_backend_item_with_ccf_none() { + let err = storage_err_to_dynamo_with_ccf( + StorageError::Service(DynamoDbError::ConditionalCheckFailedException( + "The conditional request failed".to_owned(), + Some(backend_item()), + )), + ReturnValuesOnConditionCheckFailure::None, + ); + match err { + DynamoDbError::ConditionalCheckFailedException(msg, item) => { + assert_eq!(msg, "The conditional request failed"); + assert!( + item.is_none(), + "item reached the client with ccf None: {item:?}" + ); + } + other => panic!("expected ConditionalCheckFailedException, got {other:?}"), + } + } + + #[test] + fn service_condition_failed_keeps_the_backend_item_with_ccf_all_old() { + let err = storage_err_to_dynamo_with_ccf( + StorageError::Service(DynamoDbError::ConditionalCheckFailedException( + "The conditional request failed".to_owned(), + Some(backend_item()), + )), + ReturnValuesOnConditionCheckFailure::AllOld, + ); + match err { + DynamoDbError::ConditionalCheckFailedException(msg, item) => { + assert_eq!(msg, "The conditional request failed"); + assert_eq!(item, Some(backend_item())); + } + other => panic!("expected ConditionalCheckFailedException, got {other:?}"), + } + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 74083fbf..8550b0d1 100755 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -31,6 +31,8 @@ mod read_helpers; mod request_limits; mod scan; mod search_vectors; +#[cfg(test)] +mod service_error_log_tests; pub mod stream_capture; mod streams; mod tagging; @@ -146,7 +148,12 @@ pub(crate) fn serialize_output( /// /// Use this for storage calls that don't go through `storage_err_to_dynamo` /// (e.g., tagging, metadata operations that return `StorageError` directly). +/// A [`StorageError::Service`] error is the backend's own classification of +/// the failure and reaches the client unchanged, without the fault log line. pub(crate) fn sanitize_storage_error(e: extenddb_storage::error::StorageError) -> DynamoDbError { + if let extenddb_storage::error::StorageError::Service(e) = e { + return e; + } tracing::error!(internal_error = %e, "storage internal error"); DynamoDbError::InternalServerError("Internal server error".to_owned()) } @@ -527,3 +534,40 @@ pub async fn dispatch( _ => Err(DynamoDbError::UnknownOperationException(String::new())), } } + +#[cfg(test)] +mod sanitize_storage_error_tests { + use super::sanitize_storage_error; + use extenddb_core::error::DynamoDbError; + use extenddb_storage::error::StorageError; + + // The metadata and tagging paths map through this function; a + // backend-classified error keeps its variant and message there too. + #[test] + fn service_error_passes_through() { + let err = sanitize_storage_error(StorageError::Service(DynamoDbError::ServiceUnavailable( + "retry shortly".to_owned(), + ))); + match err { + DynamoDbError::ServiceUnavailable(msg) => assert_eq!(msg, "retry shortly"), + other => panic!("expected ServiceUnavailable, got {other:?}"), + } + } + + // Control: every other variant is still redacted behind the fixed string. + #[test] + fn other_variants_are_redacted() { + for e in [ + StorageError::Internal("driver text".to_owned()), + StorageError::Connection("host unreachable".to_owned()), + StorageError::Transient("pool timeout".to_owned()), + ] { + match sanitize_storage_error(e) { + DynamoDbError::InternalServerError(msg) => { + assert_eq!(msg, "Internal server error"); + } + other => panic!("expected InternalServerError, got {other:?}"), + } + } + } +} diff --git a/crates/engine/src/service_error_log_tests.rs b/crates/engine/src/service_error_log_tests.rs new file mode 100644 index 00000000..56b4936b --- /dev/null +++ b/crates/engine/src/service_error_log_tests.rs @@ -0,0 +1,102 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 +//! The second half of the `StorageError::Service` contract: a backend-classified +//! error is not logged as a storage fault, while an unclassified one still is. +//! One subscriber counts error-level events around each engine mapping site +//! that has its own catch-all. + +use std::sync::{Arc, Mutex}; + +use extenddb_core::error::DynamoDbError; +use extenddb_storage::error::StorageError; +use tracing::{Event, Level, Subscriber}; +use tracing_subscriber::layer::{Context, Layer, SubscriberExt}; + +/// Counts error-level events. +#[derive(Default)] +struct Errors(Arc>); + +impl Layer for Errors { + fn on_event(&self, event: &Event<'_>, _: Context<'_, S>) { + if *event.metadata().level() == Level::ERROR { + *self.0.lock().unwrap() += 1; + } + } +} + +/// Runs `f` under a fresh subscriber and returns how many error events it logged. +fn error_events_during(f: impl FnOnce()) -> usize { + let errors = Errors::default(); + let count = Arc::clone(&errors.0); + let subscriber = tracing_subscriber::registry().with(errors); + let _guard = tracing::subscriber::set_default(subscriber); + f(); + *count.lock().unwrap() +} + +fn service() -> StorageError { + StorageError::Service(DynamoDbError::ServiceUnavailable( + "retry shortly".to_owned(), + )) +} + +fn internal() -> StorageError { + StorageError::Internal("driver text".to_owned()) +} + +type Site = (&'static str, fn(StorageError) -> DynamoDbError); + +/// Every engine function that maps a `StorageError` and owns a catch-all arm. +fn sites() -> Vec { + vec![ + ( + "create_table::storage_err_to_dynamo", + crate::create_table::storage_err_to_dynamo, + ), + ( + "backup::storage_err_to_dynamo", + crate::backup::storage_err_to_dynamo, + ), + ("update_table::update_table_err_to_dynamo", |e| { + crate::update_table::update_table_err_to_dynamo(e, "t") + }), + ( + "streams::storage_to_dynamo", + crate::streams::storage_to_dynamo, + ), + ("ttl::storage_to_dynamo", crate::ttl::storage_to_dynamo), + ("sanitize_storage_error", crate::sanitize_storage_error), + ] +} + +#[test] +fn a_service_error_is_not_logged_as_a_storage_fault_at_any_site() { + for (name, map) in sites() { + let logged = error_events_during(|| { + let err = map(service()); + assert!( + matches!(err, DynamoDbError::ServiceUnavailable(_)), + "{name}: expected ServiceUnavailable, got {err:?}" + ); + }); + assert_eq!(logged, 0, "{name} logged a Service error as a fault"); + } +} + +// Control: the same sites still log an unclassified failure once. +#[test] +fn an_internal_error_is_logged_once_as_a_storage_fault_at_every_site() { + for (name, map) in sites() { + let logged = error_events_during(|| { + let err = map(internal()); + assert!( + matches!(err, DynamoDbError::InternalServerError(_)), + "{name}: expected InternalServerError, got {err:?}" + ); + }); + assert_eq!( + logged, 1, + "{name} logged {logged} fault events for an Internal error" + ); + } +} diff --git a/crates/engine/src/streams.rs b/crates/engine/src/streams.rs index 83b29725..527fac74 100755 --- a/crates/engine/src/streams.rs +++ b/crates/engine/src/streams.rs @@ -349,10 +349,14 @@ pub async fn handle_get_records( serialize_output(&output) } -fn storage_to_dynamo(e: StorageError) -> DynamoDbError { +pub(crate) fn storage_to_dynamo(e: StorageError) -> DynamoDbError { match e { StorageError::Validation(msg) => DynamoDbError::ValidationException(msg), StorageError::TableNotFound(name) => DynamoDbError::ResourceNotFoundException(name), + // The backend already classified this failure into the service error + // vocabulary and chose the client-facing message. Pass it through + // unchanged; it is not a storage fault, so it is not logged as one. + StorageError::Service(e) => e, other => { tracing::error!(internal_error = %other, "storage internal error"); DynamoDbError::InternalServerError("Internal server error".to_owned()) @@ -360,6 +364,60 @@ fn storage_to_dynamo(e: StorageError) -> DynamoDbError { } } +#[cfg(test)] +mod storage_error_mapping_tests { + use super::storage_to_dynamo; + use extenddb_core::error::DynamoDbError; + use extenddb_storage::error::StorageError; + + // A backend-classified error reaches the client unchanged through the + // streams mapper, keeping both its variant and its message. + + #[test] + fn service_internal_server_error_passes_through() { + let err = storage_to_dynamo(StorageError::Service(DynamoDbError::InternalServerError( + "backend internal".to_owned(), + ))); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "backend internal"), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + + #[test] + fn service_service_unavailable_passes_through() { + let err = storage_to_dynamo(StorageError::Service(DynamoDbError::ServiceUnavailable( + "retry shortly".to_owned(), + ))); + match err { + DynamoDbError::ServiceUnavailable(msg) => assert_eq!(msg, "retry shortly"), + other => panic!("expected ServiceUnavailable, got {other:?}"), + } + } + + #[test] + fn service_expired_iterator_passes_through() { + let err = storage_to_dynamo(StorageError::Service( + DynamoDbError::ExpiredIteratorException("Iterator expired.".to_owned()), + )); + match err { + DynamoDbError::ExpiredIteratorException(msg) => assert_eq!(msg, "Iterator expired."), + other => panic!("expected ExpiredIteratorException, got {other:?}"), + } + } + + // Control: an unclassified driver error is still redacted behind the fixed + // string, with the driver text absent from the client message. + #[test] + fn internal_driver_text_is_redacted() { + let err = storage_to_dynamo(StorageError::Internal("driver text".to_owned())); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "Internal server error"), + other => panic!("expected InternalServerError, got {other:?}"), + } + } +} + #[cfg(test)] mod tests { /// AT_SEQUENCE_NUMBER converts to AFTER by subtracting 1 and padding to diff --git a/crates/engine/src/ttl.rs b/crates/engine/src/ttl.rs index fac027d4..90257518 100755 --- a/crates/engine/src/ttl.rs +++ b/crates/engine/src/ttl.rs @@ -214,7 +214,7 @@ fn validate_ttl_attribute_name(name: &str) -> Result<(), DynamoDbError> { Ok(()) } -fn storage_to_dynamo(e: StorageError) -> DynamoDbError { +pub(crate) fn storage_to_dynamo(e: StorageError) -> DynamoDbError { match e { // A backend's refusal of the request (the Cassandra backend refuses // TTL on a table with an asynchronously propagated GSI): a client @@ -226,6 +226,10 @@ fn storage_to_dynamo(e: StorageError) -> DynamoDbError { StorageError::TableNotActive(name) => { DynamoDbError::ResourceInUseException(format!("Table {name} is not in ACTIVE state")) } + // The backend already classified this failure into the service error + // vocabulary and chose the client-facing message. Pass it through + // unchanged; it is not a storage fault, so it is not logged as one. + StorageError::Service(e) => e, other => { tracing::error!(internal_error = %other, "storage internal error"); DynamoDbError::InternalServerError("Internal server error".to_owned()) @@ -237,6 +241,53 @@ fn storage_to_dynamo(e: StorageError) -> DynamoDbError { mod tests { use super::*; + // A backend-classified error reaches the client unchanged through the TTL + // mapper, keeping both its variant and its message. + + #[test] + fn service_internal_server_error_passes_through() { + let err = storage_to_dynamo(StorageError::Service(DynamoDbError::InternalServerError( + "backend internal".to_owned(), + ))); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "backend internal"), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + + #[test] + fn service_service_unavailable_passes_through() { + let err = storage_to_dynamo(StorageError::Service(DynamoDbError::ServiceUnavailable( + "retry shortly".to_owned(), + ))); + match err { + DynamoDbError::ServiceUnavailable(msg) => assert_eq!(msg, "retry shortly"), + other => panic!("expected ServiceUnavailable, got {other:?}"), + } + } + + #[test] + fn service_limit_exceeded_passes_through() { + let err = storage_to_dynamo(StorageError::Service( + DynamoDbError::LimitExceededException("limit reached".to_owned()), + )); + match err { + DynamoDbError::LimitExceededException(msg) => assert_eq!(msg, "limit reached"), + other => panic!("expected LimitExceededException, got {other:?}"), + } + } + + // Control: an unclassified driver error is still redacted behind the fixed + // string, with the driver text absent from the client message. + #[test] + fn internal_driver_text_is_redacted() { + let err = storage_to_dynamo(StorageError::Internal("driver text".to_owned())); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "Internal server error"), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + #[test] fn valid_ttl_attribute_names() { assert!(validate_ttl_attribute_name("ttl").is_ok()); diff --git a/crates/engine/src/update_table.rs b/crates/engine/src/update_table.rs index 6db763f8..357f6c80 100755 --- a/crates/engine/src/update_table.rs +++ b/crates/engine/src/update_table.rs @@ -309,7 +309,7 @@ fn projected_after_gsi_updates( /// yet returns `Unsupported` or `IndexesInUse` from `update_table`, and the /// vector capability gate refuses vector requests before the backend is called /// at all, so a wire test cannot provoke either one today. -fn update_table_err_to_dynamo( +pub(crate) fn update_table_err_to_dynamo( e: extenddb_storage::error::StorageError, table_name: &str, ) -> DynamoDbError { @@ -339,6 +339,10 @@ fn update_table_err_to_dynamo( // request. The backend supplies the whole message because only it knows // the state. StorageError::IndexesInUse(msg) => DynamoDbError::ResourceInUseException(msg), + // The backend already classified this failure into the service error + // vocabulary and chose the client-facing message. Pass it through + // unchanged; it is not a storage fault, so it is not logged as one. + StorageError::Service(e) => e, other => { tracing::error!(internal_error = %other, "storage internal error"); DynamoDbError::InternalServerError("Internal server error".to_owned()) @@ -351,6 +355,50 @@ mod tests { use super::*; use extenddb_storage::error::StorageError; + // A backend-classified error reaches the client unchanged through the + // UpdateTable mapper, keeping both its variant and its message. + + #[test] + fn service_service_unavailable_passes_through() { + let err = update_table_err_to_dynamo( + StorageError::Service(DynamoDbError::ServiceUnavailable( + "retry shortly".to_owned(), + )), + "t", + ); + match err { + DynamoDbError::ServiceUnavailable(msg) => assert_eq!(msg, "retry shortly"), + other => panic!("expected ServiceUnavailable, got {other:?}"), + } + } + + #[test] + fn service_capacity_error_passes_through() { + let err = update_table_err_to_dynamo( + StorageError::Service(DynamoDbError::ProvisionedThroughputExceededException( + "capacity".to_owned(), + )), + "t", + ); + match err { + DynamoDbError::ProvisionedThroughputExceededException(msg) => { + assert_eq!(msg, "capacity"); + } + other => panic!("expected ProvisionedThroughputExceededException, got {other:?}"), + } + } + + // Control: an unclassified driver error is still redacted behind the fixed + // string, with the driver text absent from the client message. + #[test] + fn internal_driver_text_is_redacted() { + let err = update_table_err_to_dynamo(StorageError::Internal("driver text".to_owned()), "t"); + match err { + DynamoDbError::InternalServerError(msg) => assert_eq!(msg, "Internal server error"), + other => panic!("expected InternalServerError, got {other:?}"), + } + } + #[test] fn a_created_gsi_throughput_below_one_is_refused() { let input = |pt: Value| -> UpdateTableInput { diff --git a/crates/storage/src/error.rs b/crates/storage/src/error.rs index 34a93c34..65e36425 100755 --- a/crates/storage/src/error.rs +++ b/crates/storage/src/error.rs @@ -86,4 +86,24 @@ pub enum StorageError { Unsupported(String), #[error("Internal error: {0}")] Internal(String), + /// A client error the backend has already classified. The engine passes it + /// to the client with its variant and message unchanged and does not log it + /// as a storage fault. One exception: a `ConditionalCheckFailedException` + /// carries its item to the client only when the request set + /// `ReturnValuesOnConditionCheckFailure` to `ALL_OLD`, the rule the engine + /// applies to its own `ConditionFailed`. Use this + /// when the backend can map its own failure onto a specific service error + /// with its own message, for example a hosted database reporting a + /// documented unavailability or capacity condition. + /// + /// The message inside crosses the wire verbatim; the engine does not + /// inspect or redact it. It must be the text the service documents for that + /// error class, or text of the same shape, and must not contain driver + /// output, host names, connection strings or credentials. A failure the + /// backend cannot classify stays [`Self::Internal`], which the engine + /// redacts behind a fixed string. Distinct from [`Self::Transient`], which + /// the engine also redacts and which the queue workers read as the retry + /// signal; `Service` is for request paths only. + #[error(transparent)] + Service(extenddb_core::error::DynamoDbError), } diff --git a/docs/adr/0023-backend-classified-client-error.md b/docs/adr/0023-backend-classified-client-error.md new file mode 100644 index 00000000..6049626b --- /dev/null +++ b/docs/adr/0023-backend-classified-client-error.md @@ -0,0 +1,92 @@ +# ADR-0023: A backend-classified client error passes through the engine unchanged + +- Status: Accepted +- Date: 2026-10-24 +- Deciders: @LeeroyHannigan + +## Context + +Every storage trait method returns `Result`. The engine maps a +`StorageError` to the client-facing `DynamoDbError` at nine functions +(`crates/engine/src/create_table.rs`, `backup.rs`, `update_table.rs`, +`streams.rs`, `ttl.rs`, `lib.rs`), each of which names the variants it knows +and ends in a catch-all that logs the error at error level and returns +`InternalServerError("Internal server error")`. The catch-all is the redaction +rule the security model documents: driver text, SQL, paths and connection +details never reach a client (`docs/manuals/10-security-model.md`, Error +Message Security; RFC-0003 section 9.2). + +The rule has a cost for a backend that knows more than the engine does. A +hosted database reports documented conditions with their own class and text: +a capacity limit, a maintenance window, a regional unavailability. The engine +has no variant for "here is the client error, already classified", so such a +backend can only return `Internal` (redacted to a generic 500 and logged as a +fault) or `Transient` (also redacted, and read by the in-tree queue workers as +the retry signal). The client sees a message that says nothing about what to +do, and the operator's log counts a documented condition among storage faults. + +## Options Considered + +1. **A `Service(DynamoDbError)` variant passed through unchanged.** The backend + constructs the client error; every engine mapping site returns it without + inspecting, rewriting or logging it. The backend takes on the redaction + obligation for the message it supplies. +2. **A structured variant per condition** (`Throttled { kind, reasons }`, + `Unavailable { retry_after }`, and so on), with the engine choosing the + message. Keeps the message in the engine, at the cost of one variant and + one engine mapping per condition a backend may want to report, and a + message the backend cannot tailor. +3. **Leave the enum as it is.** A backend reports every failure as `Internal` + or `Transient`; the client sees the fixed 500 text. + +## Decision + +Add `StorageError::Service(DynamoDbError)`, marked `#[error(transparent)]`, +and make every engine function that maps a `StorageError` to a client error +return it unchanged and without the fault log line. The engine applies one +rule of its own to the payload: for `ConditionalCheckFailedException`, the +request's `ReturnValuesOnConditionCheckFailure` decides whether the item is +returned, as it does for the engine's `ConditionFailed`. + +## Rationale + +- The engine cannot know the message for a condition only the backend sees; + option 2 would re-derive each hosted database's documented text inside the + engine, one variant at a time, and lag the databases. +- The status code and the body shape still come from the `DynamoDbError` + variant through the one serializer, so a backend cannot produce a status or + a shape outside the service's catalog through this variant. +- The redaction rule for `Internal` and every other variant is unchanged, and + a test at each site keeps it so. The one new path is named in the security + model, the backend author manual, RFC-0003 section 9.2 and the variant's + doc comment, with the obligation stated the same way in each: the message + crosses the wire verbatim and must be service-shaped text with no driver + output, host names, connection strings or credentials. +- A subscriber-based test counts error-level log events at every mapping + site: none for a `Service` error, one for an `Internal` error, so the + "not logged as a fault" half of the contract cannot regress silently. + +## Consequences + +- A backend can report a documented hosted-database condition with the + service's error class and its own message, and the operator's fault log no + longer counts it. +- Reviewers of a backend crate read the text it puts inside `Service` as + client-visible wording, under the same rule as the engine's own messages. +- `StorageError` gains a variant; RFC-0002 now states that variants may be + added in a minor release, so a backend crate matching on it exhaustively + carries a catch-all arm. The engine's own data path match stays exhaustive + so the compiler reports a mapping site that misses a new variant. +- No in-tree backend returns `Service` yet. The first one to do so adds the + condition it reports and the measured service text to its fidelity notes. + +--- + +## License + +Copyright 2026 ExtendDB contributors. Licensed under the Apache License, Version 2.0. +See [LICENSE](../../LICENSE) for the full text. + +This software is provided "as is" without warranty of any kind. ExtendDB is not +affiliated with, endorsed by, or sponsored by Amazon Web Services. "DynamoDB" is +a trademark of Amazon.com, Inc. diff --git a/docs/adr/README.md b/docs/adr/README.md index 2deec125..69f3ea0b 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -48,4 +48,5 @@ decision, write a new ADR. | [0019](0019-cassandra-logical-backup-restore.md) | Cassandra: logical backup and restore | Accepted | | [0020](0020-cassandra-occ-update-item.md) | Cassandra: optimistic concurrency control for UpdateItem and PutItem | Accepted | | [0021](0021-cassandra-lwt-delete-atomicity-gap.md) | Cassandra: LWT delete atomicity gap and required worker/saga pattern | Accepted | -| [0022](0022-trust-policy-deny-over-allow.md) | Role trust-policy evaluation follows IAM Deny-over-Allow order | Accepted | \ No newline at end of file +| [0022](0022-trust-policy-deny-over-allow.md) | Role trust-policy evaluation follows IAM Deny-over-Allow order | Accepted | +| [0023](0023-backend-classified-client-error.md) | A backend-classified client error passes through the engine unchanged | Accepted | diff --git a/docs/design/04-component-storage.md b/docs/design/04-component-storage.md index d3591a5b..a15034a7 100755 --- a/docs/design/04-component-storage.md +++ b/docs/design/04-component-storage.md @@ -334,8 +334,21 @@ Storage backends write stream records atomically with data writes when `stream` - `IdempotentMismatch` — idempotency token exists with different operations - `IndexNotFound` — secondary index does not exist - `Internal(String)` — backend-specific error - -The `engine` crate maps `StorageError` to wire protocol error responses. +- `Service(DynamoDbError)`: a client error the backend has already classified, carrying its own message + +The `engine` crate maps `StorageError` to wire protocol error responses. An +`Internal` error is logged at error level and reaches the client as the fixed +`InternalServerError` text, so backend driver messages never cross the wire. +A `Service` error is the exception: the backend has mapped its own failure onto +a service error class (a hosted database reporting a documented unavailability +or capacity condition, for example), so every engine mapping site returns it +to the client with its variant and message unchanged and does not log it as a +storage fault (a conditional check error carries its item only when the +request asked for `ALL_OLD`, as with the engine's own). The backend is +responsible for the message it puts inside: it crosses the wire verbatim, so it +must be service-shaped text with no driver output, host names, connection +strings or credentials, and a failure the backend cannot classify stays +`Internal`. ## 5. PostgreSQL Backend Implementation diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index bb7a1c75..3c7561af 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -222,6 +222,7 @@ Import/export file paths are validated: - DynamoDB-fidelity error messages reproduce real DynamoDB exactly (no additional information) - Internal errors (database connection failures, SQL errors, I/O errors) are logged server-side but return generic messages to clients - Stack traces, file paths, and SQL text are never exposed to clients +- One path carries backend-chosen text to a client: a backend may return `StorageError::Service(DynamoDbError)` when it has classified its own failure into a service error class, and the engine returns that error with its message unchanged. The message inside is client-facing text the backend author owns; it must not contain driver output, SQL text, paths or connection details, and it is reviewed as client-visible wording under the same rule as the engine's own messages ## Operational Security diff --git a/docs/manuals/12-extending-extenddb-storage.md b/docs/manuals/12-extending-extenddb-storage.md index 75f701d5..7e0eb5ac 100755 --- a/docs/manuals/12-extending-extenddb-storage.md +++ b/docs/manuals/12-extending-extenddb-storage.md @@ -98,6 +98,7 @@ Key design decisions: - **Items** are `BTreeMap`. A new backend must handle the full `AttributeValue` type (S, N, B, SS, NS, BS, L, M, BOOL, NULL). - **Query** must support forward/reverse sort order, exclusive start key pagination, and routing to secondary index storage. - **Parallel scan** uses `segment` and `total_segments` to partition the keyspace. +- **Errors**: return the named `StorageError` variant for every condition the engine maps (`TableNotFound`, `ConditionFailed`, `TransactionCanceled`, `Validation`, `Unsupported`, `Transient` and the rest); the engine chooses the client-facing message and redacts anything it does not recognise behind a fixed `InternalServerError`. When the backend can classify its own failure into a service error class with a message of its own (a hosted database reporting a documented unavailability or capacity condition), return `StorageError::Service(DynamoDbError)`: the engine passes it to the client with its variant and message unchanged and does not log it as a storage fault (a conditional check error carries its item only when the request asked for `ALL_OLD`, as with the engine's own). That message crosses the wire verbatim, so it must be service-shaped text with no driver output, host names, connection strings or credentials. A failure the backend cannot classify stays `Internal`. ### MetadataEngine diff --git a/docs/rfcs/0002-backend-plugin-policy.md b/docs/rfcs/0002-backend-plugin-policy.md index 7d67abdb..40901214 100644 --- a/docs/rfcs/0002-backend-plugin-policy.md +++ b/docs/rfcs/0002-backend-plugin-policy.md @@ -255,7 +255,7 @@ cargo build --no-default-features --bin extenddb ### Release model -ExtendDB will move to a release model that includes releases through Crates.io. All crates follow semantic versioning. Breaking changes to `extenddb-storage` trigger coordinated releases. Binaries will also be available as GitHub releases. The project will also prioritize releasing images through Docker Hub. +ExtendDB will move to a release model that includes releases through Crates.io. All crates follow semantic versioning. Breaking changes to `extenddb-storage` trigger coordinated releases. Variants may be added to `StorageError` in a minor release; a backend crate that matches on it exhaustively must carry a catch-all arm. Binaries will also be available as GitHub releases. The project will also prioritize releasing images through Docker Hub. ### Third-party backends outside ExtendDB organization diff --git a/docs/rfcs/0003-backend-acceptance-criteria.md b/docs/rfcs/0003-backend-acceptance-criteria.md index 2c919950..f5dc37e9 100644 --- a/docs/rfcs/0003-backend-acceptance-criteria.md +++ b/docs/rfcs/0003-backend-acceptance-criteria.md @@ -125,7 +125,7 @@ If implementing pushdown, numeric comparisons must be numeric (not string-lexico **9.1. No silent degradation.** If an operation cannot be performed correctly, the backend must return an error, never silently drop side-effects (stream records, index updates), return partial results without a `LastEvaluatedKey`, or succeed while leaving inconsistent state. -**9.2. DynamoDB error codes.** Client-visible errors must use DynamoDB-compatible error codes and HTTP status codes. Internal storage errors (connection failures, type mismatches) must map to appropriate DynamoDB errors, typically `InternalServerError` (500) with retry semantics, or `ValidationException` (400) for input problems. Backend-specific error codes must never leak to clients. +**9.2. DynamoDB error codes.** Client-visible errors must use DynamoDB-compatible error codes and HTTP status codes. Internal storage errors (connection failures, type mismatches) must map to appropriate DynamoDB errors, typically `InternalServerError` (500) with retry semantics, or `ValidationException` (400) for input problems. Backend-specific error codes must never leak to clients. A backend that returns `StorageError::Service` is responsible for the message it carries: the engine passes that error to the client with its message unchanged and does not redact it, so the message must be service-shaped text with no driver output, host names, connection strings or credentials. ### 10. Operational Correctness