From 6285b81ffc3463bfc7607af49d76bab4fc48ec04 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:45:46 +0000 Subject: [PATCH 1/2] feat(storage): a StorageError::Service variant the engine passes through A backend that can classify its own failure into the service's error vocabulary (a hosted database reporting a documented unavailability or capacity condition) had no way to return that error with its own message: every unrecognised StorageError variant reaches the client as the fixed "Internal server error" string after an error-level log line. StorageError::Service(DynamoDbError) carries a fully formed client error. Nine engine functions map a StorageError to a client error. Six own a catch-all and gain a Service arm that returns the error with its variant and message unchanged and without the fault log line: the data path mapping in create_table.rs, the backup mapping in backup.rs, the UpdateTable mapping, the streams and TTL mappings, and sanitize_storage_error for the metadata and tagging paths. The other three (the condition-check variant of the data path mapping, the create-backup and restore mappings) delegate their catch-all to one of the six. The server's authorization read delegates to the data path mapping. The engine keeps one rule for itself. A ConditionalCheckFailedException carries its item to the client only when the request set ReturnValuesOnConditionCheckFailure to ALL_OLD, which the condition-check mapping applies to a backend's error as it does to the engine's own ConditionFailed; the generic data path mapping, which has no such value to read, discards the item as it does for ConditionFailed. The message is kept. Tests: a Service pass-through test at every one of the nine mappings; at each of the six with a catch-all, a control that Internal("driver text") still becomes the fixed string, and a subscriber-based test that counts error-level log events (none for a Service error, one for an Internal error). tracing-subscriber becomes a dev-dependency of the engine crate for that test. The condition-check item rule has a test per ReturnValuesOnConditionCheckFailure value and one for the generic path. The doc comment states the backend's obligation: the message crosses the wire verbatim, so it must be service-shaped text with no driver output, host names, connection strings or credentials; an unclassified failure stays Internal, and Transient remains the queue workers' retry signal. --- Cargo.lock | 1 + crates/engine/Cargo.toml | 3 + crates/engine/src/backup.rs | 7 +- .../engine/src/backup/service_error_tests.rs | 93 ++++++++ crates/engine/src/create_table.rs | 211 +++++++++++++++++- crates/engine/src/lib.rs | 44 ++++ crates/engine/src/service_error_log_tests.rs | 102 +++++++++ crates/engine/src/streams.rs | 60 ++++- crates/engine/src/ttl.rs | 53 ++++- crates/engine/src/update_table.rs | 50 ++++- crates/storage/src/error.rs | 20 ++ 11 files changed, 639 insertions(+), 5 deletions(-) create mode 100644 crates/engine/src/backup/service_error_tests.rs create mode 100644 crates/engine/src/service_error_log_tests.rs diff --git a/Cargo.lock b/Cargo.lock index c59cdea4e..b54f22d7b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1821,6 +1821,7 @@ dependencies = [ "sha2 0.10.9", "tokio", "tracing", + "tracing-subscriber", "uuid", ] diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 3634c9152..313a0d034 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 67114a013..2670643a3 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 000000000..db01c8263 --- /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 e9948c111..7638ba53b 100755 --- a/crates/engine/src/create_table.rs +++ b/crates/engine/src/create_table.rs @@ -227,6 +227,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, } } @@ -251,16 +262,34 @@ 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; - use super::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."; /// A snapshot the backend discarded is retryable, like a transient error. #[test] @@ -271,4 +300,184 @@ mod storage_error_mapping_tests { "{err:?}" ); } + + // 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 74083fbfa..8550b0d13 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 000000000..56b4936bd --- /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 c8ba419bc..de33cb9d7 100755 --- a/crates/engine/src/streams.rs +++ b/crates/engine/src/streams.rs @@ -406,10 +406,14 @@ fn get_records_limit(limit: Option) -> Result { } } -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()) @@ -417,6 +421,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 fac027d4b..902575181 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 6db763f82..357f6c80f 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 8fe3ac287..5a03a7d30 100755 --- a/crates/storage/src/error.rs +++ b/crates/storage/src/error.rs @@ -92,4 +92,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), } From d994093eab779ee73fd88c4b7ad9df392b9688f7 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:45:46 +0000 Subject: [PATCH 2/2] docs: describe StorageError::Service in the storage design, the manuals, the RFCs and an ADR The storage design lists the variant beside the Internal redaction rule and states the backend's obligation for the message. The security model's error message section names it as the one path that carries backend-chosen text to a client, under the same wording rule as the engine's own messages. The backend author manual's DataEngine section states when to return it and when a failure stays Internal. RFC-0003 section 9.2 adds the obligation to the error fidelity criteria. RFC-0002 states that variants may be added to StorageError in a minor release, so a backend crate matching on it exhaustively carries a catch-all arm. ADR-0023 records the decision, the options not taken and the consequences. --- .../0023-backend-classified-client-error.md | 92 +++++++++++++++++++ docs/adr/README.md | 3 +- docs/design/04-component-storage.md | 17 +++- docs/manuals/10-security-model.md | 1 + docs/manuals/12-extending-extenddb-storage.md | 1 + docs/rfcs/0002-backend-plugin-policy.md | 2 +- docs/rfcs/0003-backend-acceptance-criteria.md | 2 +- 7 files changed, 113 insertions(+), 5 deletions(-) create mode 100644 docs/adr/0023-backend-classified-client-error.md 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 000000000..6049626bd --- /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 2deec1259..69f3ea0b8 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 dcfda3a3d..73565e3e8 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 bb7a1c75d..3c7561af2 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 75f701d54..7e0eb5ac8 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 7d67abdb3..409012146 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 2c919950e..f5dc37e96 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