diff --git a/crates/core/src/types/transaction.rs b/crates/core/src/types/transaction.rs index 2f311abfe..d89053093 100755 --- a/crates/core/src/types/transaction.rs +++ b/crates/core/src/types/transaction.rs @@ -316,11 +316,21 @@ impl CancellationReason { } } - /// Internal reason for a wrong-typed key found while preparing a + /// Internal reason for a wrong-typed key supplied in the request of a /// transaction member: a table key element that does not match the - /// schema, or a secondary-index key (on a written item or as a literal - /// operand) that does not match its attribute definition. Never reaches a - /// client: [`Self::resolve_key_faults`] rewrites it to `ValidationError`. + /// schema, or a secondary-index key (on a Put item or as an Update's + /// literal operand) that does not match its attribute definition. Never + /// reaches a client: [`Self::resolve_key_faults`] rewrites it to + /// `ValidationError`. + /// + /// A wrong type an Update writes from `if_not_exists` or from an + /// attribute reference is not this reason. With the path present the + /// value comes from the stored item; with the path absent the fallback + /// is a literal from the request, and the service ranks it with the + /// values taken from the item. The service reports both as a plain + /// positional `ValidationError` and leaves the other members' outcomes + /// in place, so the storage layers carry them as + /// [`Self::validation_error`]. #[must_use] pub fn key_type_mismatch(msg: impl Into) -> Self { Self { @@ -342,47 +352,47 @@ impl CancellationReason { } } - /// Whether this reason is one of the internal key faults that - /// [`Self::resolve_key_faults`] gives precedence to. A backend that runs - /// members out of request order can stop once the earliest such member - /// in request order is known: a wrong-typed key there outranks every later - /// member, and an empty index key there outranks every later member that - /// is not itself a wrong-typed key. - #[must_use] - pub fn is_key_fault(&self) -> bool { - self.code == KEY_TYPE_MISMATCH_CODE || self.code == INDEX_KEY_EMPTY_ABORT_CODE - } - /// Apply the precedence `DynamoDB` gives key faults across the members of - /// a `TransactWriteItems` request, measured on the service. A wrong-typed - /// key (table or secondary index) on any member cancels the transaction - /// with the first such member's `ValidationError` in position and `None` - /// in every other position, whatever those members' own faults were. - /// Otherwise an empty secondary-index key on any member aborts the whole - /// request with a top-level `ValidationException`, outranking failed - /// conditions and unapplicable statements on the other members. Otherwise - /// the reasons are returned as they are. + /// a `TransactWriteItems` request, measured on the service: the first + /// internal key fault in request order decides. A wrong-typed key there + /// (see [`Self::key_type_mismatch`]) cancels the transaction with that + /// member's `ValidationError` and `None` in every other position, + /// whatever those members' own faults were. An empty secondary-index key + /// there (see [`Self::index_key_empty_abort`]) answers the whole request + /// with a top-level `ValidationException` carrying its message. With no + /// internal key fault the reasons are returned as they are: a wrong type + /// an Update writes from the stored item arrives as a plain + /// `ValidationError` and the other members keep their own outcomes beside + /// it (measured: `[ValidationError, ConditionalCheckFailed]`). + /// + /// The storage layers resolve the faults supplied in the request (table + /// keys, a Put item's index keys, an Update's literal types) over every + /// member before any member is applied. They then run the members before + /// the first Update member with an empty index-key literal and put that + /// literal's reason at its position, so the reasons carry only empty + /// values, the first of which decides, beside plain reasons: an empty + /// value an earlier member wrote from the item answers ahead of the + /// literal (measured 2026-11-01). /// /// # Errors /// /// Returns `Err(message)` when the request must be answered with a /// top-level `ValidationException` carrying `message`. - pub fn resolve_key_faults(reasons: Vec) -> Result, String> { - let first_type_mismatch = reasons + pub fn resolve_key_faults(mut reasons: Vec) -> Result, String> { + let Some(first) = reasons .iter() - .position(|r| r.code == KEY_TYPE_MISMATCH_CODE); - let mut reasons = reasons; + .position(|r| r.code == KEY_TYPE_MISMATCH_CODE || r.code == INDEX_KEY_EMPTY_ABORT_CODE) + else { + return Ok(reasons); + }; + if reasons[first].code == INDEX_KEY_EMPTY_ABORT_CODE { + return Err(reasons.swap_remove(first).message.unwrap_or_default()); + } for (i, reason) in reasons.iter_mut().enumerate() { - match first_type_mismatch { - Some(first) if i == first => reason.code = "ValidationError".to_owned(), - Some(_) => *reason = Self::none(), - None if reason.code == INDEX_KEY_EMPTY_ABORT_CODE => { - if let Some(message) = reason.message.take() { - return Err(message); - } - *reason = Self::none(); - } - None => {} + if i == first { + reason.code = "ValidationError".to_owned(); + } else { + *reason = Self::none(); } } Ok(reasons) @@ -436,41 +446,165 @@ mod tests { ); } + // The kinds of fault a transaction member can carry into the resolver: + // two internal key faults (a wrong type supplied in the request, an empty + // index key) and the plain reasons beside them (a wrong type an Update + // writes from the stored item, a failed condition). Each test below puts + // one pair in both member orders; the expected answers are the measured + // ones (2026-10-30 rows A2, B1, C1, D1, E1, F1, F2 and their reversals; + // 2026-10-31 section 1, a Put item's empty key against a wrong type in + // the request, which the first in request order decides). + const COMPUTED: &str = + "The update expression attempted to update the secondary index key to unsupported type"; + + fn type_fault() -> CancellationReason { + CancellationReason::key_type_mismatch("type") + } + + fn empty_fault() -> CancellationReason { + CancellationReason::index_key_empty_abort("empty key") + } + + fn computed_fault() -> CancellationReason { + CancellationReason::validation_error(COMPUTED) + } + + fn failed_condition() -> CancellationReason { + CancellationReason::condition_check_failed_with_item(None) + } + + /// `[first, second]` and `[second, first]`, resolved. + fn both_orders( + first: fn() -> CancellationReason, + second: fn() -> CancellationReason, + ) -> [Result, String>; 2] { + [ + CancellationReason::resolve_key_faults(vec![first(), second()]), + CancellationReason::resolve_key_faults(vec![second(), first()]), + ] + } + + /// Assert a cancellation with `ValidationError` carrying the type fault's + /// message at `at` and `None`, with no message, everywhere else. + fn assert_type_fault_at(out: &Result, String>, at: usize) { + let out = out.as_ref().expect("a cancellation"); + for (i, reason) in out.iter().enumerate() { + if i == at { + assert_eq!(reason.code, "ValidationError"); + assert_eq!(reason.message.as_deref(), Some("type")); + } else { + assert_eq!(reason.code, "None", "position {i}"); + assert_eq!(reason.message, None, "position {i}"); + } + } + } + #[test] - fn an_empty_index_key_becomes_a_top_level_message_over_other_member_faults() { - let reasons = vec![ - CancellationReason::condition_check_failed_with_item(None), - CancellationReason::index_key_empty_abort("empty key"), - CancellationReason::validation_error("cannot apply"), - ]; + fn a_wrong_type_and_an_empty_key_answer_whichever_comes_first() { + let [type_first, empty_first] = both_orders(type_fault, empty_fault); + assert_type_fault_at(&type_first, 0); + assert_eq!(empty_first.unwrap_err(), "empty key"); + } + + #[test] + fn a_wrong_type_in_the_request_masks_a_computed_wrong_type_in_either_order() { + let [type_first, computed_first] = both_orders(type_fault, computed_fault); + assert_type_fault_at(&type_first, 0); + assert_type_fault_at(&computed_first, 1); + } + + #[test] + fn a_wrong_type_in_the_request_masks_a_failed_condition_in_either_order() { + let [type_first, condition_first] = both_orders(type_fault, failed_condition); + assert_type_fault_at(&type_first, 0); + assert_type_fault_at(&condition_first, 1); + } + + #[test] + fn an_empty_key_outranks_a_computed_wrong_type_in_either_order() { + for out in both_orders(empty_fault, computed_fault) { + assert_eq!(out.unwrap_err(), "empty key"); + } + } + + #[test] + fn an_empty_key_outranks_a_failed_condition_in_either_order() { + for out in both_orders(empty_fault, failed_condition) { + assert_eq!(out.unwrap_err(), "empty key"); + } + } + + #[test] + fn a_computed_wrong_type_and_a_failed_condition_keep_their_own_outcomes() { + let [computed_first, condition_first] = both_orders(computed_fault, failed_condition); + let computed_first = computed_first.unwrap(); assert_eq!( - CancellationReason::resolve_key_faults(reasons).unwrap_err(), - "empty key" + codes(&computed_first), + ["ValidationError", "ConditionalCheckFailed"] + ); + assert_eq!(computed_first[0].message.as_deref(), Some(COMPUTED)); + let condition_first = condition_first.unwrap(); + assert_eq!( + codes(&condition_first), + ["ConditionalCheckFailed", "ValidationError"] ); + assert_eq!(condition_first[1].message.as_deref(), Some(COMPUTED)); } #[test] - fn a_type_mismatch_masks_every_other_member_including_an_empty_key() { - let reasons = vec![ - CancellationReason::condition_check_failed_with_item(None), - CancellationReason::index_key_empty_abort("empty key"), - CancellationReason::key_type_mismatch("type"), - CancellationReason::validation_error("cannot apply"), - ]; - let out = CancellationReason::resolve_key_faults(reasons).unwrap(); - assert_eq!(codes(&out), ["None", "None", "ValidationError", "None"]); - assert_eq!(out[2].message.as_deref(), Some("type")); + fn only_the_first_of_two_wrong_types_is_reported() { + for (first, second) in [("second", "third"), ("third", "second")] { + let reasons = vec![ + CancellationReason::none(), + CancellationReason::key_type_mismatch(first), + CancellationReason::key_type_mismatch(second), + ]; + let out = CancellationReason::resolve_key_faults(reasons).unwrap(); + assert_eq!(codes(&out), ["None", "ValidationError", "None"]); + assert_eq!(out[1].message.as_deref(), Some(first)); + assert_eq!(out[2].message, None); + } } #[test] - fn only_the_first_type_mismatch_by_position_is_reported() { - let reasons = vec![ - CancellationReason::none(), - CancellationReason::key_type_mismatch("second"), - CancellationReason::key_type_mismatch("third"), + fn the_first_of_two_empty_keys_answers() { + for (first, second) in [ + ("put wording", "update wording"), + ("update wording", "put wording"), + ] { + let reasons = vec![ + failed_condition(), + CancellationReason::index_key_empty_abort(first), + CancellationReason::index_key_empty_abort(second), + ]; + assert_eq!( + CancellationReason::resolve_key_faults(reasons).unwrap_err(), + first + ); + } + } + + #[test] + fn the_first_internal_fault_decides_among_plain_reasons() { + let empty_before_type = vec![ + failed_condition(), + empty_fault(), + type_fault(), + CancellationReason::validation_error("cannot apply"), ]; - let out = CancellationReason::resolve_key_faults(reasons).unwrap(); - assert_eq!(codes(&out), ["None", "ValidationError", "None"]); - assert_eq!(out[1].message.as_deref(), Some("second")); + assert_eq!( + CancellationReason::resolve_key_faults(empty_before_type).unwrap_err(), + "empty key" + ); + let type_before_empty = vec![ + failed_condition(), + type_fault(), + empty_fault(), + CancellationReason::validation_error("cannot apply"), + ]; + assert_type_fault_at( + &CancellationReason::resolve_key_faults(type_before_empty), + 1, + ); } } diff --git a/crates/core/src/validation/mod.rs b/crates/core/src/validation/mod.rs index a849f2c95..1202f3d59 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -1799,12 +1799,15 @@ fn ordered_indexes<'b, 'a>(indexes: &'b [IndexKeyRef<'a>]) -> Vec<&'b IndexKeyRe ordered } -/// The type an update operand produces when it is built from literals alone, -/// `None` when any part of it reads the item (an attribute path, or a -/// function whose result depends on one such as `if_not_exists`). +/// The type an update operand produces that is known from the statement +/// alone, `None` when only the item can tell (an attribute path, or +/// `if_not_exists`, whose result is the stored value when the path exists). /// -/// `list_append` of two literals is a list; arithmetic on two literals is a -/// number. Anything else that is literal-only is a placeholder. +/// A placeholder has its value's type. `list_append` always yields a list +/// and arithmetic always a number, whatever their operands read, and the +/// service checks both on the statement like a placeholder (measured +/// 2026-10-31: `SET sidx = list_append(l, :l)` and `SET sidx = n + :one` on a +/// string key are type faults ahead of the condition). fn literal_operand_type( expr: &crate::expression::Expr, maps: &crate::expression::ExpressionMaps, @@ -1812,30 +1815,23 @@ fn literal_operand_type( use crate::expression::Expr; match expr { Expr::Placeholder(name) => maps.values.get(name).map(attribute_value_type_tag), - Expr::Function { name, args } if name.eq_ignore_ascii_case("list_append") => { - let all_literal = args.iter().all(|a| literal_operand_type(a, maps).is_some()); - all_literal.then_some("L") - } - Expr::Arithmetic { left, right, .. } => { - let both = literal_operand_type(left, maps).is_some() - && literal_operand_type(right, maps).is_some(); - both.then_some("N") - } + Expr::Function { name, .. } if name.eq_ignore_ascii_case("list_append") => Some("L"), + Expr::Arithmetic { .. } => Some("N"), _ => None, } } /// Validate, before a condition is evaluated or a statement applied, that every /// `SET`, `ADD` or `DELETE` action whose target is a secondary-index key -/// attribute and whose operand is built from literals alone carries a value of -/// the key's declared type. +/// attribute and whose operand's type the statement fixes (a placeholder, +/// `list_append`, arithmetic) carries a value of the key's declared type. /// /// `DynamoDB` runs this check on the statement text ahead of the condition and /// ahead of applying the statement, so it is reported even when the condition /// would fail or another action in the statement could not be applied. A value -/// that depends on the existing item (an attribute reference, the fallback of -/// `if_not_exists`) is not checked here; the storage layer checks the written -/// item afterwards and reports it with the computed-value wording. +/// written from an attribute reference or from `if_not_exists` is not checked +/// here; the storage layer checks the written item afterwards and reports it +/// with the computed-value wording. /// /// `ADD` of a number onto a number key keeps the key's type and passes. `ADD` /// or `DELETE` of a set onto any index key, and `ADD` of a number onto a @@ -1924,6 +1920,62 @@ pub fn validate_index_key_literal_operands( Ok(()) } +/// Validate, before a condition is evaluated or a statement applied, that no +/// `SET` action assigns an empty string or empty binary placeholder to a +/// secondary-index key attribute. +/// +/// `DynamoDB` checks an empty literal on the statement, after the literal type +/// check of [`validate_index_key_literal_operands`] and ahead of the +/// condition, of an action that cannot apply and of a wrong type computed +/// from the item (measured 2026-10-31, `UpdateItem` and `TransactWriteItems`). +/// An empty value written from the item or from the fallback of +/// `if_not_exists` is not checked here; it is found on the written item after +/// the condition. Actions are checked in statement order. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the update-expression +/// empty-key wording, naming the empty value's kind (string or binary). +pub fn validate_index_key_empty_literals( + actions: &[crate::expression::UpdateAction], + indexes: &[IndexKeyRef<'_>], + maps: &crate::expression::ExpressionMaps, +) -> Result<(), DynamoDbError> { + use crate::expression::{Expr, PathElement, UpdateAction}; + for action in actions { + let UpdateAction::Set { path, value } = action else { + continue; + }; + // Only a top-level attribute can be an index key. + let [PathElement::Attribute(raw)] = path.as_slice() else { + continue; + }; + let Expr::Placeholder(name) = value else { + continue; + }; + let attr: &str = match raw.strip_prefix('#') { + Some(alias) => maps.resolve_name(alias)?, + None => raw.as_str(), + }; + let keys_an_index = indexes + .iter() + .any(|idx| idx.key_schema.iter().any(|ks| ks.attribute_name == attr)); + if !keys_an_index { + continue; + } + let kind = match maps.values.get(name) { + Some(AttributeValue::S(s)) if s.is_empty() => "string", + Some(AttributeValue::B(b)) if b.is_empty() => "binary", + _ => continue, + }; + return Err(DynamoDbError::ValidationException(format!( + "One or more parameter values are not valid. The update expression attempted to update a secondary index key to a value that is not supported. \ + The AttributeValue for a key attribute cannot contain an empty {kind} value." + ))); + } + Ok(()) +} + /// Readable flags for the `is_query` parameter of [`validate_select_projection`]. /// Real DynamoDB prepends `1 validation error detected: ` to some rejections for /// Query but not for Scan, so callers pass one of these instead of a bare bool. @@ -4815,3 +4867,175 @@ mod update_size_tests { ); } } + +#[cfg(test)] +mod index_key_literal_tests { + use std::collections::HashMap; + + use super::{ + IndexKeyRef, validate_index_key_empty_literals, validate_index_key_literal_operands, + }; + use crate::expression::{ExpressionMaps, UpdateAction, parse_update, tokenize}; + use crate::types::{ + AttributeDefinition, AttributeValue, KeySchemaElement, KeyType, ScalarAttributeType, + }; + + /// The measured table: GSI `gsi_s` on `sidx` (S), GSI `gsi_b` on `bidx` (B). + fn table() -> ( + Vec, + Vec, + Vec, + ) { + let hash = |a: &str| KeySchemaElement { + attribute_name: a.to_owned(), + key_type: KeyType::Hash, + }; + let def = |a: &str, t| AttributeDefinition { + attribute_name: a.to_owned(), + attribute_type: t, + }; + ( + vec![hash("sidx")], + vec![hash("bidx")], + vec![ + def("pk", ScalarAttributeType::S), + def("sidx", ScalarAttributeType::S), + def("bidx", ScalarAttributeType::B), + ], + ) + } + + fn build(expr: &str, values: &[(&str, AttributeValue)]) -> (Vec, ExpressionMaps) { + let actions = parse_update(&tokenize(expr).unwrap()).unwrap(); + let map: HashMap = values + .iter() + .map(|(k, v)| ((*k).to_owned(), v.clone())) + .collect(); + (actions, ExpressionMaps::new(HashMap::new(), map)) + } + + /// The literal type check on `expr`: `Ok(())` or the error message. + fn type_check(expr: &str, values: &[(&str, AttributeValue)]) -> Result<(), String> { + let (gsi_s, gsi_b, defs) = table(); + let refs = [ + IndexKeyRef { + index_name: "gsi_s", + key_schema: &gsi_s, + }, + IndexKeyRef { + index_name: "gsi_b", + key_schema: &gsi_b, + }, + ]; + let (actions, maps) = build(expr, values); + validate_index_key_literal_operands(&actions, &refs, &defs, &maps) + .map_err(|e| e.to_string()) + } + + /// The empty literal check on `expr`: `Ok(())` or the error message. + fn empty_check(expr: &str, values: &[(&str, AttributeValue)]) -> Result<(), String> { + let (gsi_s, gsi_b, _) = table(); + let refs = [ + IndexKeyRef { + index_name: "gsi_s", + key_schema: &gsi_s, + }, + IndexKeyRef { + index_name: "gsi_b", + key_schema: &gsi_b, + }, + ]; + let (actions, maps) = build(expr, values); + validate_index_key_empty_literals(&actions, &refs, &maps).map_err(|e| e.to_string()) + } + + fn list() -> AttributeValue { + AttributeValue::L(vec![AttributeValue::S("y".to_owned())]) + } + + fn one() -> AttributeValue { + AttributeValue::N("1".to_owned()) + } + + const TYPE_L: &str = "One or more parameter values were invalid: Type mismatch for Index Key sidx Expected: S Actual: L IndexName: gsi_s"; + const TYPE_N: &str = "One or more parameter values were invalid: Type mismatch for Index Key sidx Expected: S Actual: N IndexName: gsi_s"; + + #[test] + fn list_append_is_a_list_whatever_its_operands_read() { + for expr in [ + "SET sidx = list_append(:l, :l)", + "SET sidx = list_append(l, :l)", + ] { + let err = type_check(expr, &[("l", list())]).unwrap_err(); + assert!(err.ends_with(TYPE_L), "{expr}: {err}"); + } + } + + #[test] + fn arithmetic_is_a_number_whatever_its_operands_read() { + for expr in [ + "SET sidx = :one + :one", + "SET sidx = n + :one", + "SET sidx = n - m", + ] { + let err = type_check(expr, &[("one", one())]).unwrap_err(); + assert!(err.ends_with(TYPE_N), "{expr}: {err}"); + } + } + + #[test] + fn a_value_read_from_the_item_is_left_to_the_written_item() { + for expr in [ + "SET sidx = n", + "SET sidx = if_not_exists(n, :s)", + "SET sidx = if_not_exists(nope, :one)", + ] { + let values = [("one", one()), ("s", AttributeValue::S("x".to_owned()))]; + assert_eq!(type_check(expr, &values), Ok(()), "{expr}"); + } + } + + #[test] + fn an_empty_literal_on_an_index_key_is_refused_with_its_kind() { + let empty_s = ("e", AttributeValue::S(String::new())); + let empty_b = ("eb", AttributeValue::B(Vec::new())); + let string = empty_check("SET sidx = :e", std::slice::from_ref(&empty_s)).unwrap_err(); + assert!( + string.ends_with("cannot contain an empty string value."), + "{string}" + ); + let binary = + empty_check("SET sidx = n, bidx = :eb", std::slice::from_ref(&empty_b)).unwrap_err(); + assert!( + binary.ends_with("cannot contain an empty binary value."), + "{binary}" + ); + // Two empty literals of different kinds in one statement: the service + // answered the binary one in one run and the string one in another + // (measured 2026-11-01), so either answer passes. + let either = empty_check("SET bidx = :eb, sidx = :e", &[empty_s, empty_b]).unwrap_err(); + assert!( + either.ends_with("cannot contain an empty binary value.") + || either.ends_with("cannot contain an empty string value."), + "{either}" + ); + } + + #[test] + fn only_a_literal_on_an_index_key_is_an_empty_literal() { + let empty_s = ("e", AttributeValue::S(String::new())); + for expr in [ + "SET v = :e", + "SET sidx = e", + "SET sidx = if_not_exists(nope, :e)", + "SET m.sidx = :e", + "REMOVE sidx", + ] { + assert_eq!( + empty_check(expr, std::slice::from_ref(&empty_s)), + Ok(()), + "{expr}" + ); + } + } +} diff --git a/crates/engine/src/create_table.rs b/crates/engine/src/create_table.rs index 7b827cd3a..cdb777e5b 100755 --- a/crates/engine/src/create_table.rs +++ b/crates/engine/src/create_table.rs @@ -212,6 +212,9 @@ pub fn storage_err_to_dynamo(e: extenddb_storage::error::StorageError) -> Dynamo // TransactionConflictException here — never InternalServerError. DynamoDbError::TransactionConflictException(msg) } + StorageError::TransactionInProgress => DynamoDbError::TransactionInProgressException( + "Transaction from previous request is still in progress".to_owned(), + ), StorageError::Internal(msg) => { // Log the raw message for debugging but do not expose storage // backend details (e.g. PostgreSQL error text) to the client. diff --git a/crates/engine/src/update_item.rs b/crates/engine/src/update_item.rs index ee9658117..392c1ecdf 100755 --- a/crates/engine/src/update_item.rs +++ b/crates/engine/src/update_item.rs @@ -244,11 +244,12 @@ pub async fn handle_update_item( validate_no_key_updates(&actions, &key_info, &maps)?; // A literal operand assigned to a secondary-index key must carry the key's - // declared type. DynamoDB checks this on the statement itself, ahead of - // the condition and ahead of applying any action, so it is reported even - // when the condition would fail or another action could not be applied. - // A value computed from the existing item is checked by the storage layer - // on the written item instead. + // declared type, and then may not be empty. DynamoDB checks both on the + // statement itself, in that order, ahead of the condition and ahead of + // applying any action, so they are reported even when the condition would + // fail or another action could not be applied. A value written from an + // attribute reference or from if_not_exists is checked by the storage + // layer on the written item instead. { let idx_refs: Vec> = key_info .global_secondary_indexes @@ -265,6 +266,7 @@ pub async fn handle_update_item( &key_info.attribute_definitions, &maps, )?; + extenddb_core::validation::validate_index_key_empty_literals(&actions, &idx_refs, &maps)?; } let return_old = matches!( diff --git a/crates/storage-cassandra/src/data/transactions.rs b/crates/storage-cassandra/src/data/transactions.rs index b6a64f5cf..a68adf040 100644 --- a/crates/storage-cassandra/src/data/transactions.rs +++ b/crates/storage-cassandra/src/data/transactions.rs @@ -30,6 +30,14 @@ use crate::data::{ query_with_pk_sk_item_txnid_ts, query_with_pk_sk_txnid, query_with_txnid_ts_pk_sk, select_by_pk, }; +/// How long a `ClientRequestToken` row lives after its last write: the +/// service keeps a token used 9 minutes after its request, committed or +/// cancelled, and treats it as new 11 minutes after (measured 2026-11-05), +/// as the 10 minutes the other backends read through `created_at`. A send +/// that fails after the reservation or the take leaves its running row for +/// as long. +const IDEMPOTENCY_TOKEN_TTL_SECONDS: u32 = 600; + /// What the transaction paths read from one base row. #[derive(Debug, Default)] struct TransactionRow { @@ -177,7 +185,8 @@ impl CassandraEngine { /// reason) ahead of the member's condition, then the condition against the /// pre-update image, then the statement itself, then on the written item a /// computed index-key value of the wrong type (a cancellation reason) and - /// an index key left empty (a top-level abort). + /// an index key such a value leaves empty (a top-level abort). An empty + /// literal never reaches this: the request-level checks find it first. /// /// Used on the first PREPARE pass and again on every create-race retry, so /// the two paths cannot drift apart. @@ -204,9 +213,12 @@ impl CassandraEngine { .map_err(|e| CancellationReason::validation_error(e.to_string()))?; let idx_refs = Self::transact_index_key_refs(key_info); if !idx_refs.is_empty() { - // A value computed from the existing item (an attribute reference, - // the fallback of if_not_exists) is only known now, and the service - // reports it with its own wording as a cancellation reason. + // A value written from an attribute reference or from + // if_not_exists is only known now, and the service reports a wrong + // type with its own wording as a cancellation reason. With the + // path absent the if_not_exists fallback is a literal from the + // request, which the service ranks with the values taken from the + // item. validation::validate_index_key_types_after_update( &item, &idx_refs, @@ -421,6 +433,45 @@ impl CassandraEngine { let account_keyspace = self.account_keyspace(first_account); + // Key checks first, for every member in request order, before the + // token is checked and before the ledger entry and PREPARE. They need + // only the request and the table metadata, and a fault among them + // decides the request under the service's precedence (the first such + // fault in request order) and leaves the token unused, before any + // item is read or any marker is written, as on the other backends. + extenddb_storage::transact_key_check_pass(ops.iter().map(|op| { + let key_info = transact_write_op_key_info(op); + extenddb_storage::transact_op_key_checks( + op, + &Self::transact_index_key_refs(key_info), + &key_info.attribute_definitions, + ) + }))?; + // The first Update member with an empty index-key literal. Only the + // members before it are prepared: an empty value one of them writes + // from the item answers ahead of the literal, and no member after it + // is read or marked. + let empty_literal = extenddb_storage::transact_first_empty_literal(ops.iter().map(|op| { + extenddb_storage::transact_op_empty_literal_checks( + op, + &Self::transact_index_key_refs(transact_write_op_key_info(op)), + ) + })); + let run_ops = &ops[..empty_literal.as_ref().map_or(ops.len(), |(p, _)| *p)]; + // The reasons of the members that ran, the empty literal at its + // position, `None` after it, resolved as the service ranks them. + let decide = |mut reasons: Vec| { + if let Some((_, reason)) = &empty_literal { + reasons.truncate(run_ops.len()); + reasons.push(reason.clone()); + reasons.resize(ops.len(), CancellationReason::none()); + } + match CancellationReason::resolve_key_faults(reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + } + }; + // Check the account-scoped idempotency token before creating the ledger. if let Some((idempotency_account, token, fingerprint)) = idempotency { if idempotency_account != first_account { @@ -437,12 +488,18 @@ impl CassandraEngine { .await?; } + if run_ops.is_empty() { + // The first member's empty literal decides: nothing to prepare. + self.record_cancelled_token(idempotency).await; + return Err(decide(Vec::new())); + } + // Generate unique transaction ID let txn_id = Uuid::new_v4(); let started_at = crate::cassandra_util::now_millis(); // Build initial ledger ops (pk/sk known; item_data for UPDATE filled in after PREPARE) - let mut ledger_ops = Self::initial_ledger_ops(ops)?; + let mut ledger_ops = Self::initial_ledger_ops(run_ops)?; let initial_blob = serde_json::to_string(&ledger_ops) .map_err(|e| StorageError::Internal(format!("serialize ledger ops: {e}")))?; @@ -459,9 +516,23 @@ impl CassandraEngine { .await?; // T4.2: PREPARE phase - returns computed item_data for UPDATE ops - let prepare_result = self.execute_prepare_phase(ops, txn_id, started_at).await; + let prepare_result = self + .execute_prepare_phase(run_ops, txn_id, started_at) + .await; match prepare_result { + Ok(_) if empty_literal.is_some() => { + // The members before the empty literal prepared cleanly, so + // the literal decides: roll them back. + if let Err(e) = self + .execute_rollback_phase(&account_keyspace, run_ops, txn_id) + .await + { + tracing::error!("transact_write: rollback failed for txn {txn_id}: {e}"); + } + self.record_cancelled_token(idempotency).await; + Err(decide(vec![CancellationReason::none(); run_ops.len()])) + } Ok(computed_items) => { // Fill in the UPDATE post-images and every op's pre-commit // image now that PREPARE has read them. @@ -486,7 +557,9 @@ impl CassandraEngine { txn_id, started_at, ) - .await + .await?; + self.mark_token_committed(idempotency).await; + Ok(()) } Err(reasons) => { // T4.4: ROLLBACK phase @@ -495,30 +568,21 @@ impl CassandraEngine { reasons.len() ); if let Err(e) = self - .execute_rollback_phase(&account_keyspace, ops, txn_id) + .execute_rollback_phase(&account_keyspace, run_ops, txn_id) .await { // Rollback failures are recoverable by the background worker. // Do not replace the original cancellation error. tracing::error!("transact_write: rollback failed for txn {txn_id}: {e}"); } - // B5: release the token so a retry with the same token can proceed. - if let Some((idempotency_account, token, _)) = idempotency { - self.delete_idempotency_token( - &self.catalog_keyspace(), - idempotency_account, - token, - ) - .await; - } + // The request passed the key checks, so its token stays used, + // as the service keeps it; the same request runs again (B5). + self.record_cancelled_token(idempotency).await; // Key faults leave PREPARE under internal reason codes; the // shared resolver applies the service's precedence across // members and decides between a cancellation and a top-level // ValidationException. - match CancellationReason::resolve_key_faults(reasons) { - Ok(reasons) => Err(StorageError::TransactionCanceled(reasons)), - Err(message) => Err(StorageError::Validation(message)), - } + Err(decide(reasons)) } } } @@ -1146,11 +1210,16 @@ fn eval_condition( /// Extract `account_id` from a `TransactWriteOp`. fn transact_write_op_account_id<'a>(op: &'a TransactWriteOp<'_>) -> &'a str { + &transact_write_op_key_info(op).account_id +} + +/// The table metadata of a `TransactWriteOp`. +fn transact_write_op_key_info<'a>(op: &'a TransactWriteOp<'_>) -> &'a TableKeyInfo { match op { TransactWriteOp::Put { key_info, .. } | TransactWriteOp::Delete { key_info, .. } | TransactWriteOp::Update { key_info, .. } - | TransactWriteOp::ConditionCheck { key_info, .. } => &key_info.account_id, + | TransactWriteOp::ConditionCheck { key_info, .. } => key_info, } } @@ -1228,8 +1297,15 @@ impl CassandraEngine { /// Check and reserve an account-scoped idempotency token. /// - /// Returns `Ok(())` when the token is freshly reserved. + /// Returns `Ok(())` when the token is freshly reserved, or taken back from + /// a cancellation of the same request. The row then holds the request's + /// fingerprint after [`RUNNING_TOKEN_PREFIX`] until the request commits + /// ([`Self::mark_token_committed`]) or is cancelled + /// ([`Self::record_cancelled_token`]). /// Returns `Err(IdempotentReplay)` / `Err(IdempotentMismatch)` when already present. + /// Returns `Err(TransactionInProgress)` when another send of the same + /// request holds the token and still runs, or won the reservation or the + /// take. async fn check_idempotency_token( &self, keyspace: &str, @@ -1252,29 +1328,40 @@ impl CassandraEngine { .await? { let stored_fp: String = get_column(&row, "fingerprint", "check_idempotency_token")?; - return if stored_fp == fingerprint { - Err(StorageError::IdempotentReplay) - } else { - Err(StorageError::IdempotentMismatch) - }; + return self + .answer_token_row(keyspace, account_id, token, fingerprint, &stored_fp) + .await; } // Reserve with LWT to handle concurrent requests racing on the same token. + // The row expires IDEMPOTENCY_TOKEN_TTL_SECONDS after this write, so + // a send that fails before it commits or is cancelled leaves its + // running row for 10 minutes (#837). let insert_query = format!( "INSERT INTO {keyspace}.idempotency_tokens_by_account \ - (account_id, \"token\", fingerprint, created_at) VALUES (?, ?, ?, ?) IF NOT EXISTS" + (account_id, \"token\", fingerprint, created_at) VALUES (?, ?, ?, ?) \ + IF NOT EXISTS USING TTL {IDEMPOTENCY_TOKEN_TTL_SECONDS}" ); let now = crate::cassandra_util::now_millis(); let result = crate::cassandra_util::apply_lwt( &self.session, &insert_query, - cdrs_tokio::query_values!(account_id, token, fingerprint, now), + cdrs_tokio::query_values!( + account_id, + token, + running_token_fingerprint(fingerprint), + now + ), "check_idempotency_token insert", ) .await?; if !result { - // Lost the LWT race — read back to return the right error. + // Lost the LWT race: another send reserved the token between the + // read above and the insert. Read it back to answer as the + // service does (measured 2026-11-05): the same request still + // running answers TransactionInProgressException, the same + // request cancelled runs again, another request is the mismatch. let row = query_optional::( &self.session, &select_query, @@ -1282,41 +1369,160 @@ impl CassandraEngine { "check_idempotency_token recheck", ) .await?; - if let Some(row) = row { - let stored_fp: String = - get_column(&row, "fingerprint", "check_idempotency_token recheck")?; - return if stored_fp == fingerprint { - Err(StorageError::IdempotentReplay) - } else { - Err(StorageError::IdempotentMismatch) - }; - } + let Some(row) = row else { + // The winner's row is gone already: let the client send again. + return Err(StorageError::TransactionInProgress); + }; + let stored_fp: String = + get_column(&row, "fingerprint", "check_idempotency_token recheck")?; + return self + .answer_token_row(keyspace, account_id, token, fingerprint, &stored_fp) + .await; } Ok(()) } - /// Delete a previously reserved idempotency token. - /// - /// Called on every cancellation path to prevent token poisoning (B5): a - /// token reserved before the ledger is committed must be released if the - /// transaction is cancelled, so a retry with the same token can proceed. - async fn delete_idempotency_token(&self, keyspace: &str, account_id: &str, token: &str) { - let delete_query = format!( - "DELETE FROM {keyspace}.idempotency_tokens_by_account \ - WHERE account_id = ? AND \"token\" = ?" + /// Answer a request with `fingerprint` from the token row that holds + /// `stored_fp` (see [`token_row`]): a running send of the same request + /// answers `TransactionInProgress`, a committed one a replay, a + /// cancelled one is taken back to run again, and another request is the + /// mismatch. + async fn answer_token_row( + &self, + keyspace: &str, + account_id: &str, + token: &str, + fingerprint: &str, + stored_fp: &str, + ) -> Result<(), StorageError> { + match token_row(stored_fp, fingerprint) { + TokenRow::Replay => Err(StorageError::IdempotentReplay), + TokenRow::Mismatch => Err(StorageError::IdempotentMismatch), + TokenRow::InProgress => Err(StorageError::TransactionInProgress), + TokenRow::Rerun => { + self.take_cancelled_token(keyspace, account_id, token, fingerprint, stored_fp) + .await + } + } + } + + /// Take back a token that a cancellation of the same request left + /// (`stored_fp` is its `cancelled:` fingerprint) for this run. The take + /// restarts the token's window of [`IDEMPOTENCY_TOKEN_TTL_SECONDS`]. Of two + /// sends of the request at once one takes it, and the other answers + /// `TransactionInProgress`, as the service does. + async fn take_cancelled_token( + &self, + keyspace: &str, + account_id: &str, + token: &str, + fingerprint: &str, + stored_fp: &str, + ) -> Result<(), StorageError> { + let take_query = format!( + "UPDATE {keyspace}.idempotency_tokens_by_account \ + USING TTL {IDEMPOTENCY_TOKEN_TTL_SECONDS} \ + SET fingerprint = ?, created_at = ? \ + WHERE account_id = ? AND \"token\" = ? IF fingerprint = ?" + ); + let now = crate::cassandra_util::now_millis(); + let taken = crate::cassandra_util::apply_lwt( + &self.session, + &take_query, + cdrs_tokio::query_values!( + running_token_fingerprint(fingerprint), + now, + account_id, + token, + stored_fp + ), + "check_idempotency_token rerun", + ) + .await?; + if taken { + Ok(()) + } else { + // Another send of the same request took it and is running. + Err(StorageError::TransactionInProgress) + } + } + + /// Replace the running fingerprint of the request's token row with the + /// plain fingerprint once the request has committed, so a send of the + /// same request then answers as a replay. A failed write is logged: the + /// row keeps the running fingerprint, so the same request answers + /// `TransactionInProgress` until the row expires, 10 minutes after the + /// reservation or the take. The mark writes the row with + /// [`IDEMPOTENCY_TOKEN_TTL_SECONDS`], so the committed token stays used + /// for 10 minutes from the commit. + async fn mark_token_committed(&self, idempotency: Option<(&str, &str, &str)>) { + let Some((account_id, token, fingerprint)) = idempotency else { + return; + }; + let keyspace = self.catalog_keyspace(); + let mark_query = format!( + "UPDATE {keyspace}.idempotency_tokens_by_account \ + USING TTL {IDEMPOTENCY_TOKEN_TTL_SECONDS} \ + SET fingerprint = ? WHERE account_id = ? AND \"token\" = ? IF fingerprint = ?" ); + let marked: Result = crate::cassandra_util::apply_lwt( + &self.session, + &mark_query, + cdrs_tokio::query_values!( + fingerprint, + account_id, + token, + running_token_fingerprint(fingerprint) + ), + "mark_token_committed", + ) + .await; + match marked { + Ok(true) => {} + Ok(false) => tracing::warn!( + "mark_token_committed: the token row no longer holds the running fingerprint" + ), + Err(e) => tracing::warn!("mark_token_committed failed (the token stays running): {e}"), + } + } + + /// Keep a reserved token used after its request was cancelled past the + /// request-level key checks, as the service keeps it: store it with + /// [`extenddb_storage::cancelled_token_fingerprint`], so a different + /// request with the token answers `IdempotentParameterMismatchException` + /// and the same request runs again (B5: a retry is not answered as a + /// replay of a write that never happened). The row expires + /// [`IDEMPOTENCY_TOKEN_TTL_SECONDS`] after this write, so the token stays + /// used for 10 minutes from the last cancellation (measured 2026-11-05). + async fn record_cancelled_token(&self, idempotency: Option<(&str, &str, &str)>) { + let Some((account_id, token, fingerprint)) = idempotency else { + return; + }; + let keyspace = self.catalog_keyspace(); + let upsert_query = format!( + "INSERT INTO {keyspace}.idempotency_tokens_by_account \ + (account_id, \"token\", fingerprint, created_at) VALUES (?, ?, ?, ?) \ + USING TTL {IDEMPOTENCY_TOKEN_TTL_SECONDS}" + ); + let cancelled = extenddb_storage::cancelled_token_fingerprint(fingerprint); + let now = crate::cassandra_util::now_millis(); if let Err(e) = crate::cassandra_util::execute::( &self.session, - &delete_query, - cdrs_tokio::query_values!(account_id, token), - "delete_idempotency_token", + &upsert_query, + cdrs_tokio::query_values!(account_id, token, cancelled, now), + "record_cancelled_token", ) .await { - // Non-fatal: the token will eventually expire via Cassandra TTL. - // Log so it is visible in ops, but do not replace the original error. - tracing::warn!("delete_idempotency_token failed (token will expire): {e}"); + // Non-fatal: the reserved row keeps the request's running + // fingerprint, so a different request still answers the mismatch + // and the same request answers TransactionInProgress (#837), + // until the row expires 10 minutes after the reservation or the + // take. + // Log so it is visible in ops, but do not replace the original + // error. + tracing::warn!("record_cancelled_token failed (the reserved token stays): {e}"); } } @@ -2919,3 +3125,41 @@ fn ledger_sk_to_sort_key(sk_col: &str, sk_val: &str) -> Result String { + format!("{RUNNING_TOKEN_PREFIX}{fingerprint}") +} + +/// What a token row answers a request with `fingerprint`. +enum TokenRow { + /// The request committed: answer as a replay. + Replay, + /// Another request holds the token: answer the mismatch. + Mismatch, + /// The request was cancelled: take the token back and run again. + Rerun, + /// Another send of the request holds the token and still runs: answer + /// `TransactionInProgress`, as the service does (measured 2026-11-05). + InProgress, +} + +fn token_row(stored_fp: &str, fingerprint: &str) -> TokenRow { + if let Some(running) = stored_fp.strip_prefix(RUNNING_TOKEN_PREFIX) { + return if running == fingerprint { + TokenRow::InProgress + } else { + TokenRow::Mismatch + }; + } + match extenddb_storage::StoredToken::of(stored_fp, fingerprint) { + extenddb_storage::StoredToken::Replay => TokenRow::Replay, + extenddb_storage::StoredToken::Mismatch => TokenRow::Mismatch, + extenddb_storage::StoredToken::Rerun => TokenRow::Rerun, + } +} diff --git a/crates/storage-cassandra/tests/direct/transact_write_items.rs b/crates/storage-cassandra/tests/direct/transact_write_items.rs index 1e1a8b122..49cbb21c1 100644 --- a/crates/storage-cassandra/tests/direct/transact_write_items.rs +++ b/crates/storage-cassandra/tests/direct/transact_write_items.rs @@ -583,3 +583,541 @@ async fn test_idempotency_token_is_scoped_to_account() { .await .expect("the same token in another account must not replay"); } + +/// A Put of `{id}` with `token`, whose fingerprint differs from every other +/// request in `test_idempotency_token_after_a_key_fault`. +async fn put_with_token( + engine: &extenddb_storage_cassandra::CassandraEngine, + key_info: &extenddb_core::types::TableKeyInfo, + id: &str, + token: &str, +) -> Result<(), extenddb_storage::error::StorageError> { + let mut item = Item::new(); + item.insert("id".to_owned(), AttributeValue::S(id.to_owned())); + let maps = ExpressionMaps::default(); + let ops = [TransactWriteOp::Put { + key_info, + item: &item, + condition: None, + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let fingerprint = format!("fp-put-{id}"); + engine + .transact_write_items( + &ops, + Some(IdempotencyKey { + account_id: &key_info.account_id, + token, + fingerprint: &fingerprint, + }), + ) + .await +} + +/// A token beside a key fault, as the service keeps it (measured +/// 2026-11-01): a fault of the request-level checks (a wrong-typed table key) +/// leaves the token unused, so another request with it commits. A fault found +/// after them (an empty index-key literal, a failed condition) leaves it used: +/// the same request with it answers that fault again, and another request +/// with it answers the mismatch and writes nothing. +#[tokio::test] +async fn test_idempotency_token_after_a_key_fault() { + use extenddb_core::expression::{self, Expr, PathElement}; + use extenddb_storage::TableEngine; + use extenddb_storage::error::StorageError; + use std::collections::HashMap; + + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let table = TestTable::with_gsi(&engine, "TxnTokenKeyFault", "GkIndex", "gk").await; + // The index is in the key info only once the table is active. + tokio::time::sleep(std::time::Duration::from_millis(350)).await; + engine + .process_control_plane_transitions() + .await + .expect("process table transitions"); + let key_info = engine + .table_key_info(&table.key_info.account_id, "TxnTokenKeyFault") + .await + .expect("key info"); + assert_eq!(key_info.global_secondary_indexes.len(), 1, "{key_info:?}"); + let present = |id: &'static str| { + let engine = &engine; + let key_info = &key_info; + async move { + let key = Item::from([("id".to_owned(), AttributeValue::S(id.to_owned()))]); + engine + .get_item(key_info, &key) + .await + .expect("get_item") + .is_some() + } + }; + let token = |name: &str| format!("tok-{name}-{}", uuid::Uuid::new_v4().simple()); + let plain = ExpressionMaps::default(); + + // A wrong-typed table key: answered by the request-level checks, and the + // token stays unused. + let bad_key = Item::from([("id".to_owned(), AttributeValue::N("1".to_owned()))]); + let t1 = token("key"); + let ops = [TransactWriteOp::Put { + key_info: &key_info, + item: &bad_key, + condition: None, + maps: &plain, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let key_fault = IdempotencyKey { + account_id: &key_info.account_id, + token: &t1, + fingerprint: "fp-key", + }; + let result = engine.transact_write_items(&ops, Some(key_fault)).await; + assert!( + matches!(&result, Err(StorageError::TransactionCanceled(r)) if r[0].code == "ValidationError"), + "{result:?}" + ); + put_with_token(&engine, &key_info, "after-key", &t1) + .await + .expect("the token is unused after a request-level fault"); + assert!(present("after-key").await); + + // An empty index-key literal: found after the token is checked, so the + // token stays used. + let empty = ExpressionMaps::new( + HashMap::new(), + HashMap::from([("e".to_owned(), AttributeValue::S(String::new()))]), + ); + let tokens = expression::tokenize("SET gk = :e").expect("tokenize"); + let actions = expression::parse_update(&tokens).expect("parse"); + let key_u = Item::from([("id".to_owned(), AttributeValue::S("u".to_owned()))]); + let t2 = token("empty"); + let ops = [TransactWriteOp::Update { + key_info: &key_info, + key: &key_u, + actions: &actions, + condition: None, + maps: &empty, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let empty_fault = IdempotencyKey { + account_id: &key_info.account_id, + token: &t2, + fingerprint: "fp-empty", + }; + for send in ["first", "again"] { + let result = engine.transact_write_items(&ops, Some(empty_fault)).await; + assert!( + matches!(&result, Err(StorageError::Validation(m)) if m.ends_with("cannot contain an empty string value.")), + "{send}: {result:?}" + ); + } + let result = put_with_token(&engine, &key_info, "after-empty", &t2).await; + assert!( + matches!(result, Err(StorageError::IdempotentMismatch)), + "{result:?}" + ); + assert!(!present("after-empty").await); + + // A failed condition: the same. + engine + .put_item( + &key_info, + Item::from([("id".to_owned(), AttributeValue::S("c1".to_owned()))]), + false, + None, + &plain, + None, + ) + .await + .expect("seed c1"); + let c1 = Item::from([("id".to_owned(), AttributeValue::S("c1".to_owned()))]); + let not_exists = Expr::Function { + name: "attribute_not_exists".to_owned(), + args: vec![Expr::Path(vec![PathElement::Attribute("id".to_owned())])], + }; + let t3 = token("ccf"); + let ops = [TransactWriteOp::Put { + key_info: &key_info, + item: &c1, + condition: Some(¬_exists), + maps: &plain, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let ccf = IdempotencyKey { + account_id: &key_info.account_id, + token: &t3, + fingerprint: "fp-ccf", + }; + for send in ["first", "again"] { + let result = engine.transact_write_items(&ops, Some(ccf)).await; + assert!( + matches!(&result, Err(StorageError::TransactionCanceled(r)) if r[0].code == "ConditionalCheckFailed"), + "{send}: {result:?}" + ); + } + let result = put_with_token(&engine, &key_info, "after-ccf", &t3).await; + assert!( + matches!(result, Err(StorageError::IdempotentMismatch)), + "{result:?}" + ); + assert!(!present("after-ccf").await); +} + +/// The fingerprint of the token row of `token` and the seconds it has left +/// (`TTL(fingerprint)`, `None` when the row never expires). +async fn token_row( + engine: &extenddb_storage_cassandra::CassandraEngine, + account_id: &str, + token: &str, +) -> (String, Option) { + use cdrs_tokio::types::IntoRustByName; + let query = format!( + "SELECT fingerprint, TTL(fingerprint) AS ttl FROM {}.idempotency_tokens_by_account \ + WHERE account_id = ? AND \"token\" = ?", + engine.catalog_keyspace() + ); + let rows = extenddb_storage_cassandra::cassandra_util::query_rows::< + extenddb_storage::error::StorageError, + >( + &engine.session_arc(), + &query, + cdrs_tokio::query_values!(account_id, token), + "read the token row", + ) + .await + .expect("read the token row"); + let row = rows.first().expect("a token row"); + let fingerprint: Option = row.get_by_name("fingerprint").ok().flatten(); + let ttl: Option = row.get_by_name("ttl").ok().flatten(); + (fingerprint.expect("a fingerprint"), ttl) +} + +/// A table with the item `c1`, and a Put of `c1` with +/// `attribute_not_exists(id)`, which `c1` fails. +async fn table_with_a_failing_put( + engine: &extenddb_storage_cassandra::CassandraEngine, + name: &str, +) -> TestTable { + let table = TestTable::new(engine, name, false).await; + let seeded = Item::from([("id".to_owned(), AttributeValue::S("c1".to_owned()))]); + engine + .put_item( + &table.key_info, + seeded, + false, + None, + &ExpressionMaps::default(), + None, + ) + .await + .expect("seed c1"); + table +} + +fn condition_failed(result: &Result<(), extenddb_storage::error::StorageError>) -> bool { + matches!(result, Err(extenddb_storage::error::StorageError::TransactionCanceled(r)) + if r.len() == 1 && r[0].code == "ConditionalCheckFailed") +} + +/// The service keeps a cancelled token used for 10 minutes from its last +/// cancellation (measured 2026-11-05, Q1a to Q1c). Cassandra keeps that +/// window as the TTL of the token row: a cancellation writes it with 600 +/// seconds, and a cancellation of the same request sent again starts it again. +#[tokio::test] +async fn test_a_cancelled_token_row_expires_ten_minutes_after_its_last_cancellation() { + use extenddb_core::expression::{Expr, PathElement}; + + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let table = table_with_a_failing_put(&engine, "TxnTokenTtl").await; + let key_info = &table.key_info; + let not_exists = Expr::Function { + name: "attribute_not_exists".to_owned(), + args: vec![Expr::Path(vec![PathElement::Attribute("id".to_owned())])], + }; + let maps = ExpressionMaps::default(); + let ccf_item = Item::from([ + ("id".to_owned(), AttributeValue::S("c1".to_owned())), + ("v".to_owned(), AttributeValue::S("x".to_owned())), + ]); + let ops = [TransactWriteOp::Put { + key_info, + item: &ccf_item, + condition: Some(¬_exists), + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let token = format!("tok-ttl-{}", uuid::Uuid::new_v4().simple()); + let key = IdempotencyKey { + account_id: &key_info.account_id, + token: &token, + fingerprint: "fp-ttl", + }; + + let first = engine.transact_write_items(&ops, Some(key)).await; + assert!(condition_failed(&first), "{first:?}"); + let (fingerprint, ttl) = token_row(&engine, &key_info.account_id, &token).await; + assert_eq!(fingerprint, "cancelled:fp-ttl"); + let first_ttl = ttl.expect("the cancelled token row has a TTL"); + assert!((590..=600).contains(&first_ttl), "TTL {first_ttl}"); + + tokio::time::sleep(std::time::Duration::from_secs(3)).await; + let aged_ttl = token_row(&engine, &key_info.account_id, &token) + .await + .1 + .expect("a TTL"); + assert!( + aged_ttl < first_ttl, + "TTL {aged_ttl} after 3 s, {first_ttl} before" + ); + + let again = engine.transact_write_items(&ops, Some(key)).await; + assert!(condition_failed(&again), "{again:?}"); + let (fingerprint, ttl) = token_row(&engine, &key_info.account_id, &token).await; + assert_eq!(fingerprint, "cancelled:fp-ttl"); + let renewed_ttl = ttl.expect("a TTL"); + assert!( + renewed_ttl > aged_ttl, + "TTL {renewed_ttl} after the second cancellation, {aged_ttl} before it" + ); +} + +/// A send of a request whose token another send of the same request holds +/// while it runs answers TransactionInProgressException, as the service +/// does (measured 2026-11-05), and a different request with the token +/// answers the mismatch. The running send's row is written by hand, as the +/// reservation leaves it. +#[tokio::test] +async fn test_a_send_that_meets_a_running_send_of_its_request_answers_in_progress() { + use extenddb_storage::error::StorageError; + + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let table = TestTable::new(&engine, "TxnTokenRunning", false).await; + let key_info = &table.key_info; + let token = format!("tok-running-{}", uuid::Uuid::new_v4().simple()); + let reserve = format!( + "INSERT INTO {}.idempotency_tokens_by_account \ + (account_id, \"token\", fingerprint, created_at) VALUES (?, ?, ?, ?) USING TTL 600", + engine.catalog_keyspace() + ); + extenddb_storage_cassandra::cassandra_util::execute::( + &engine.session_arc(), + &reserve, + cdrs_tokio::query_values!( + key_info.account_id.as_str(), + token.as_str(), + "running:fp-put-run1", + extenddb_storage_cassandra::cassandra_util::now_millis() + ), + "reserve the token as a running send", + ) + .await + .expect("write the running row"); + + let same = put_with_token(&engine, key_info, "run1", &token).await; + assert!( + matches!(same, Err(StorageError::TransactionInProgress)), + "{same:?}" + ); + let other = put_with_token(&engine, key_info, "other", &token).await; + assert!( + matches!(other, Err(StorageError::IdempotentMismatch)), + "{other:?}" + ); + for id in ["run1", "other"] { + let key = Item::from([("id".to_owned(), AttributeValue::S(id.to_owned()))]); + assert!( + engine + .get_item(key_info, &key) + .await + .expect("get") + .is_none(), + "{id} was written" + ); + } + let (fingerprint, _) = token_row(&engine, &key_info.account_id, &token).await; + assert_eq!(fingerprint, "running:fp-put-run1"); +} + +/// Two sends of one request with one token at the same moment (measured +/// 2026-11-05, Q9a and Q9b): the service answers one with the request's own +/// answer and the other with TransactionInProgressException, or both with +/// the request's answer. Never a replay of a write that did not happen, and +/// never IdempotentParameterMismatchException for the request's own token. +/// Each send runs in a task of its own on a runtime with two worker threads, +/// and both start when a barrier opens. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_two_sends_of_one_request_answer_its_cancellation_or_in_progress() { + use extenddb_storage::error::StorageError; + use std::sync::Arc; + + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let table = table_with_a_failing_put(&engine, "TxnTokenRace").await; + let key_info = &table.key_info; + // One send of the Put of c1 that fails its condition, with `token`, from + // a task of its own that waits on `barrier` first. + let send = |token: &str, barrier: &Arc| { + let engine = engine.clone(); + let key_info = key_info.clone(); + let token = token.to_owned(); + let barrier = barrier.clone(); + tokio::spawn(async move { + use extenddb_core::expression::{Expr, PathElement}; + let not_exists = Expr::Function { + name: "attribute_not_exists".to_owned(), + args: vec![Expr::Path(vec![PathElement::Attribute("id".to_owned())])], + }; + let maps = ExpressionMaps::default(); + let ccf_item = Item::from([ + ("id".to_owned(), AttributeValue::S("c1".to_owned())), + ("v".to_owned(), AttributeValue::S("x".to_owned())), + ]); + let ops = [TransactWriteOp::Put { + key_info: &key_info, + item: &ccf_item, + condition: Some(¬_exists), + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let key = IdempotencyKey { + account_id: &key_info.account_id, + token: &token, + fingerprint: "fp-race", + }; + barrier.wait().await; + engine.transact_write_items(&ops, Some(key)).await + }) + }; + + let mut canceled = 0; + let mut in_progress = 0; + for round in 0..10 { + // First two first sends of a fresh token, then a cancelled request + // sent twice at once. + for cancelled_before in [false, true] { + let token = format!( + "tok-race-{round}-{cancelled_before}-{}", + uuid::Uuid::new_v4().simple() + ); + if cancelled_before { + let first = send(&token, &Arc::new(tokio::sync::Barrier::new(1))) + .await + .expect("the first send"); + assert!(condition_failed(&first), "{first:?}"); + } + let barrier = Arc::new(tokio::sync::Barrier::new(2)); + let (a, b) = (send(&token, &barrier), send(&token, &barrier)); + let (a, b) = (a.await.expect("send a"), b.await.expect("send b")); + for answer in [a, b] { + if condition_failed(&answer) { + canceled += 1; + } else if matches!(answer, Err(StorageError::TransactionInProgress)) { + in_progress += 1; + } else { + panic!("round {round} (cancelled before: {cancelled_before}): {answer:?}"); + } + } + } + } + eprintln!("40 answers: {canceled} cancelled, {in_progress} in progress"); + let c1 = Item::from([("id".to_owned(), AttributeValue::S("c1".to_owned()))]); + let stored = engine + .get_item(key_info, &c1) + .await + .expect("get c1") + .expect("c1"); + assert!(!stored.contains_key("v"), "c1 was written: {stored:?}"); +} + +/// The seconds the `created_at` cell of the token row of `token` has left. +async fn created_at_ttl( + engine: &extenddb_storage_cassandra::CassandraEngine, + account_id: &str, + token: &str, +) -> Option { + use cdrs_tokio::types::IntoRustByName; + let query = format!( + "SELECT TTL(created_at) AS ttl FROM {}.idempotency_tokens_by_account \ + WHERE account_id = ? AND \"token\" = ?", + engine.catalog_keyspace() + ); + let rows = extenddb_storage_cassandra::cassandra_util::query_rows::< + extenddb_storage::error::StorageError, + >( + &engine.session_arc(), + &query, + cdrs_tokio::query_values!(account_id, token), + "read the TTL of created_at", + ) + .await + .expect("read the TTL of created_at"); + rows.first() + .expect("a token row") + .get_by_name("ttl") + .ok() + .flatten() +} + +/// A send that fails after the reservation leaves the running row it +/// reserved, and the same request answers TransactionInProgressException +/// while the row lives. The reservation writes the row with a TTL of 600 +/// seconds, so the row expires 10 minutes after it. The test commits a Put +/// and reads the TTL of `created_at`, which on that path only the +/// reservation writes. +#[tokio::test] +async fn test_a_reserved_token_row_expires_ten_minutes_after_its_reservation() { + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let table = TestTable::new(&engine, "TxnTokenReservedTtl", false).await; + let key_info = &table.key_info; + let token = format!("tok-reserved-ttl-{}", uuid::Uuid::new_v4().simple()); + put_with_token(&engine, key_info, "reserved", &token) + .await + .expect("the Put commits"); + let ttl = created_at_ttl(&engine, &key_info.account_id, &token) + .await + .expect("the reservation has a TTL"); + assert!((590..=600).contains(&ttl), "TTL(created_at) {ttl}"); +} + +/// The service keeps the token of a committed request used 9 minutes after +/// the request and treats it as new 11 minutes after (measured 2026-11-05). +/// Cassandra keeps that window as the TTL of the commit mark, the write of +/// the plain fingerprint. +#[tokio::test] +async fn test_a_committed_token_row_expires_ten_minutes_after_its_commit() { + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let table = TestTable::new(&engine, "TxnTokenCommittedTtl", false).await; + let key_info = &table.key_info; + let token = format!("tok-committed-ttl-{}", uuid::Uuid::new_v4().simple()); + put_with_token(&engine, key_info, "committed", &token) + .await + .expect("the Put commits"); + let (fingerprint, ttl) = token_row(&engine, &key_info.account_id, &token).await; + assert_eq!(fingerprint, "fp-put-committed"); + let ttl = ttl.expect("the committed token row has a TTL"); + assert!((590..=600).contains(&ttl), "TTL {ttl}"); +} diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index 249e5e33e..28cdf42d9 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -2259,6 +2259,54 @@ impl MongoEngine { Retry, } + // Key checks first, for every member in request order, before the + // session transaction starts and before the token is checked. They + // need only the request and the table's indexes, and a fault among + // them decides the request under the service's precedence (the first + // such fault in request order) and leaves the token unused, as on the + // other backends. + let mut index_schemas: std::collections::HashMap< + &str, + Vec<(String, Vec)>, + > = std::collections::HashMap::new(); + for op in ops { + let table_id = op.table_id(); + if !index_schemas.contains_key(table_id) { + let schemas = self.fetch_index_key_schemas(table_id).await?; + index_schemas.insert(table_id, schemas); + } + } + extenddb_storage::transact_key_check_pass(ops.iter().map(|op| { + let idx_refs: Vec> = index_schemas + [op.table_id()] + .iter() + .map(|(n, ks)| extenddb_core::validation::IndexKeyRef { + index_name: n.as_str(), + key_schema: ks.as_slice(), + }) + .collect(); + extenddb_storage::transact_op_key_checks( + &op.as_op(), + &idx_refs, + &op.key_info().attribute_definitions, + ) + }))?; + // The first Update member with an empty index-key literal. Only the + // members before it run: an empty value one of them writes from the + // item answers ahead of the literal. + let empty_literal = extenddb_storage::transact_first_empty_literal(ops.iter().map(|op| { + let idx_refs: Vec> = index_schemas + [op.table_id()] + .iter() + .map(|(n, ks)| extenddb_core::validation::IndexKeyRef { + index_name: n.as_str(), + key_schema: ks.as_slice(), + }) + .collect(); + extenddb_storage::transact_op_empty_literal_checks(&op.as_op(), &idx_refs) + })); + let run = empty_literal.as_ref().map_or(ops.len(), |(p, _)| *p); + // Rehydrate the `IdempotencyKey` per attempt from owned strings. // The input struct holds `&str`s, so it cannot be moved across // loop iterations. This keeps the retry loop lifetime-clean @@ -2325,13 +2373,44 @@ impl MongoEngine { }) }); + let mut rerun = false; if let Some(existing_doc) = existing { let stored_fp = existing_doc.get_str("fingerprint").unwrap_or_default(); - return Err(if stored_fp == key.fingerprint { - StorageError::IdempotentReplay - } else { - StorageError::IdempotentMismatch - }); + match extenddb_storage::StoredToken::of(stored_fp, key.fingerprint) { + extenddb_storage::StoredToken::Replay => { + return Err(StorageError::IdempotentReplay); + } + extenddb_storage::StoredToken::Mismatch => { + return Err(StorageError::IdempotentMismatch); + } + extenddb_storage::StoredToken::Rerun => { + // The same request left the token cancelled: + // take it for this run inside the session. A + // concurrent rerun conflicts and retries. + let taken = idem_coll + .update_one( + doc! { + "account_id": key.account_id, + "token": key.token, + "fingerprint": stored_fp, + }, + doc! { "$set": { + "fingerprint": key.fingerprint, + "created_at": mongodb::bson::DateTime::now(), + } }, + ) + .session(&mut session) + .await; + match taken { + Ok(res) if res.modified_count == 1 => rerun = true, + Ok(_) => return Ok(AttemptOutcome::Retry), + Err(e) if is_transient_write_conflict(&e) => { + return Ok(AttemptOutcome::Retry); + } + Err(e) => return Err(StorageError::Internal(e.to_string())), + } + } + } } // Store the token. A unique index on (account_id, token) @@ -2340,15 +2419,20 @@ impl MongoEngine { // us to the insert. On E11000, resolve the winner by // re-reading outside the session — same replay/mismatch // logic as the pre-check path. - let insert_res = idem_coll - .insert_one(doc! { - "account_id": key.account_id, - "token": key.token, - "fingerprint": key.fingerprint, - "created_at": mongodb::bson::DateTime::now(), - }) - .session(&mut session) - .await; + let insert_res = if rerun { + Ok(()) + } else { + idem_coll + .insert_one(doc! { + "account_id": key.account_id, + "token": key.token, + "fingerprint": key.fingerprint, + "created_at": mongodb::bson::DateTime::now(), + }) + .session(&mut session) + .await + .map(|_| ()) + }; if let Err(e) = insert_res { if is_duplicate_key(&e) { let winner = idem_coll @@ -2377,14 +2461,21 @@ impl MongoEngine { if winner.is_none() { return Ok(AttemptOutcome::Retry); } - return Err( - match winner.as_ref().and_then(|d| d.get_str("fingerprint").ok()) { - Some(fp) if fp == key.fingerprint => { - StorageError::IdempotentReplay - } - _ => StorageError::IdempotentMismatch, - }, - ); + // A winner the same request left cancelled is + // taken on the next attempt. + return match winner + .as_ref() + .and_then(|d| d.get_str("fingerprint").ok()) + .map(|fp| extenddb_storage::StoredToken::of(fp, key.fingerprint)) + { + Some(extenddb_storage::StoredToken::Replay) => { + Err(StorageError::IdempotentReplay) + } + Some(extenddb_storage::StoredToken::Rerun) => { + Ok(AttemptOutcome::Retry) + } + _ => Err(StorageError::IdempotentMismatch), + }; } if is_transient_write_conflict(&e) { return Ok(AttemptOutcome::Retry); @@ -2396,7 +2487,9 @@ impl MongoEngine { let mut reasons: Vec = Vec::with_capacity(ops.len()); let mut any_failed = false; - for (op, cache_generation) in ops.iter().zip(cache_generations.iter().copied()) { + for (op, cache_generation) in + ops.iter().zip(cache_generations.iter().copied()).take(run) + { match self .execute_transact_write_op_in_session(op, cache_generation, &mut session) .await @@ -2412,6 +2505,11 @@ impl MongoEngine { Err(TransactOpError::Storage(e)) => return Err(e), } } + if let Some((_, reason)) = &empty_literal { + any_failed = true; + reasons.push(reason.clone()); + reasons.resize(ops.len(), CancellationReason::none()); + } if any_failed { return Ok(AttemptOutcome::CanceledReasons(reasons)); @@ -2432,6 +2530,9 @@ impl MongoEngine { } Ok(AttemptOutcome::CanceledReasons(reasons)) => { let _ = session.abort_transaction().await; + // The request passed the key checks, so its token stays + // used, as the service keeps it. + self.keep_cancelled_token(idem_owned.as_ref()).await; // Key faults left the members as internal reasons; the // shared resolver applies the service's cross-member // precedence and decides between a cancellation and a @@ -2467,9 +2568,69 @@ impl MongoEngine { item: None, }) .collect(); + // A cancellation after the key checks keeps the token used, also a + // `TransactionConflict` (measured 2026-11-05). Every attempt's session + // transaction was aborted, so no token document of this request is + // stored. + self.keep_cancelled_token(idem_owned.as_ref()).await; Err(StorageError::TransactionCanceled(reasons)) } + /// [`Self::record_cancelled_token`] for the request's owned token, if it + /// has one. A failed write is logged: nothing was applied, so the request + /// still answers its cancellation, and the token is left unused. + async fn keep_cancelled_token(&self, idempotency: Option<&(String, String, String)>) { + if let Some((account_id, token, fingerprint)) = idempotency + && let Err(e) = self + .record_cancelled_token(account_id, token, fingerprint) + .await + { + tracing::warn!("record_cancelled_token failed (the token stays unused): {e}"); + } + } + + /// Keep a token used after its request was cancelled past the + /// request-level key checks, once the session transaction has aborted: + /// store it with [`extenddb_storage::cancelled_token_fingerprint`] and the + /// time now, so the token stays used for 10 minutes from the last + /// cancellation (measured 2026-11-05). A document older than the + /// 10-minute read window is replaced, as the SQL backends replace an + /// expired row; a document another request or another send stored less + /// than 10 minutes ago is kept. + async fn record_cancelled_token( + &self, + account_id: &str, + token: &str, + fingerprint: &str, + ) -> Result<(), StorageError> { + let idem_coll = self.data_db.collection::("idempotency_tokens"); + let cancelled = extenddb_storage::cancelled_token_fingerprint(fingerprint); + let now = mongodb::bson::DateTime::now(); + let expired_before = + mongodb::bson::DateTime::from_millis(now.timestamp_millis().saturating_sub(600_000)); + let stored = idem_coll + .update_one( + doc! { + "account_id": account_id, + "token": token, + "$or": [ + { "created_at": { "$lte": expired_before } }, + { "fingerprint": cancelled.as_str() }, + ], + }, + doc! { "$set": { "fingerprint": cancelled.as_str(), "created_at": now } }, + ) + .upsert(true) + .await; + match stored { + // No document matched and the upsert met the unique index: a + // document younger than 10 minutes of another request or another + // send holds the token, and stays. + Err(e) if !is_duplicate_key(&e) => Err(StorageError::Internal(e.to_string())), + _ => Ok(()), + } + } + async fn execute_transact_write_op_in_session( &self, op: &OwnedTransactWriteOp, @@ -2805,10 +2966,18 @@ impl MongoEngine { TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) })?; - // Secondary-index keys on the written item: a value computed - // from the existing item (an attribute reference, the fallback - // of if_not_exists) of the wrong type, reported with the - // service's computed-value wording, and a key left empty. + // Secondary-index keys on the written item: a wrong-typed value + // the statement wrote from an attribute reference or from + // if_not_exists, reported with the service's computed-value + // wording, and a key such a value leaves empty (an empty + // literal was found by the second check of + // transact_write_items_impl). With the path absent the + // if_not_exists fallback is a literal from the request, which + // the service ranks with the values taken from the item. The + // type fault is a plain positional reason: the service leaves + // the other members' outcomes in place beside it, where a + // wrong type supplied in the request (the first check of + // transact_write_items_impl) masks them. if !idx_refs.is_empty() { extenddb_core::validation::validate_index_key_types_after_update( &item, @@ -2816,9 +2985,7 @@ impl MongoEngine { &key_info.attribute_definitions, ) .map_err(|e| { - TransactOpError::Cancel(CancellationReason::key_type_mismatch( - e.to_string(), - )) + TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) })?; extenddb_core::validation::validate_index_key_not_empty( &item, @@ -3875,11 +4042,81 @@ enum OwnedTransactWriteOp { impl OwnedTransactWriteOp { fn table_id(&self) -> &str { + &self.key_info().table_id + } + + fn key_info(&self) -> &TableKeyInfo { match self { Self::Put { key_info, .. } | Self::Delete { key_info, .. } | Self::Update { key_info, .. } - | Self::ConditionCheck { key_info, .. } => &key_info.table_id, + | Self::ConditionCheck { key_info, .. } => key_info, + } + } + + /// The borrowed form the shared transaction helpers take. + fn as_op(&self) -> TransactWriteOp<'_> { + match self { + Self::Put { + key_info, + item, + condition, + maps, + return_values_on_ccf, + stream, + } => TransactWriteOp::Put { + key_info, + item, + condition: condition.as_ref(), + maps, + return_values_on_ccf: *return_values_on_ccf, + stream: stream.clone(), + }, + Self::Delete { + key_info, + key, + condition, + maps, + return_values_on_ccf, + stream, + } => TransactWriteOp::Delete { + key_info, + key, + condition: condition.as_ref(), + maps, + return_values_on_ccf: *return_values_on_ccf, + stream: stream.clone(), + }, + Self::Update { + key_info, + key, + actions, + condition, + maps, + return_values_on_ccf, + stream, + } => TransactWriteOp::Update { + key_info, + key, + actions, + condition: condition.as_ref(), + maps, + return_values_on_ccf: *return_values_on_ccf, + stream: stream.clone(), + }, + Self::ConditionCheck { + key_info, + key, + condition, + maps, + return_values_on_ccf, + } => TransactWriteOp::ConditionCheck { + key_info, + key, + condition, + maps, + return_values_on_ccf: *return_values_on_ccf, + }, } } } diff --git a/crates/storage-mongodb/src/data_engine_live_tests.rs b/crates/storage-mongodb/src/data_engine_live_tests.rs index 4ea858a58..6db30d8c5 100644 --- a/crates/storage-mongodb/src/data_engine_live_tests.rs +++ b/crates/storage-mongodb/src/data_engine_live_tests.rs @@ -38,6 +38,9 @@ mod txn_create; #[path = "data_engine_live_tests_repair_pass.rs"] mod repair_pass; +#[path = "data_engine_live_tests_token.rs"] +mod token; + fn uri() -> String { std::env::var("EXTENDDB_TEST_MONGODB_URI").expect( "set EXTENDDB_TEST_MONGODB_URI to a MongoDB replica set, e.g. \ diff --git a/crates/storage-mongodb/src/data_engine_live_tests_token.rs b/crates/storage-mongodb/src/data_engine_live_tests_token.rs new file mode 100644 index 000000000..aed988c5d --- /dev/null +++ b/crates/storage-mongodb/src/data_engine_live_tests_token.rs @@ -0,0 +1,198 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! Live MongoDB tests of the `ClientRequestToken` that a cancelled +//! `TransactWriteItems` keeps. The service keeps a cancelled token used for 10 +//! minutes from its last cancellation (measured 2026-11-05): another request +//! with it answers `IdempotentParameterMismatchException`, and the same +//! request runs again. The tests set `created_at` back in the token +//! collection instead of waiting. +//! +//! The tests read the replica set URI from `EXTENDDB_TEST_MONGODB_URI` and are +//! `#[ignore]`d, like the tests in the parent module. + +use bson::{Document, doc}; +use extenddb_core::expression::{Expr, ExpressionMaps, PathElement}; +use extenddb_core::types::{ + AttributeValue, Item, ReturnValuesOnConditionCheckFailure, TableKeyInfo, +}; +use extenddb_storage::error::StorageError; +use extenddb_storage::{DataEngine, IdempotencyKey, TransactWriteOp}; +use mongodb::IndexModel; +use mongodb::options::IndexOptions; + +use super::{finish, scratch_engine, table}; +use crate::MongoEngine; + +const ACCOUNT: &str = "000000000000"; + +/// The token collection with the unique index of the baseline migration, so +/// an insert of a held token fails as it does on a migrated deployment. +async fn token_collection(engine: &MongoEngine) -> mongodb::Collection { + let coll = engine.data_db.collection::("idempotency_tokens"); + let model = IndexModel::builder() + .keys(doc! { "account_id": 1, "token": 1 }) + .options(IndexOptions::builder().unique(true).build()) + .build(); + coll.create_index(model).await.expect("unique token index"); + coll +} + +/// The fingerprint of `token` and the age of its document in milliseconds. +async fn token_document(coll: &mongodb::Collection, token: &str) -> (String, i64) { + let found = coll + .find_one(doc! { "account_id": ACCOUNT, "token": token }) + .await + .expect("read the token") + .expect("a token document"); + let created_at = found.get_datetime("created_at").expect("created_at"); + let age = bson::DateTime::now() + .timestamp_millis() + .saturating_sub(created_at.timestamp_millis()); + ( + found + .get_str("fingerprint") + .expect("fingerprint") + .to_owned(), + age, + ) +} + +/// Set the `created_at` of `token` `minutes` back. +async fn age_token(coll: &mongodb::Collection, token: &str, minutes: i64) { + let then = + bson::DateTime::from_millis(bson::DateTime::now().timestamp_millis() - minutes * 60_000); + let updated = coll + .update_one( + doc! { "account_id": ACCOUNT, "token": token }, + doc! { "$set": { "created_at": then } }, + ) + .await + .expect("age the token"); + assert_eq!(updated.matched_count, 1); +} + +fn item(id: &str) -> Item { + Item::from([("pk".to_owned(), AttributeValue::S(id.to_owned()))]) +} + +/// A cancelled request keeps its token used, and a cancellation of the same +/// request sent again starts the 10 minutes again (Q1c): the token document +/// reads `cancelled:` with the time of the last cancellation. +#[tokio::test] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn live_a_cancellation_keeps_the_token_used_from_its_last_cancellation() { + let engine = scratch_engine().await; + let coll = token_collection(&engine).await; + let key_info = TableKeyInfo { + account_id: ACCOUNT.to_owned(), + ..table(&engine, "token-window", false) + }; + let maps = ExpressionMaps::default(); + engine + .put_item(&key_info, item("c1"), false, None, &maps, None) + .await + .expect("seed c1"); + let not_exists = Expr::Function { + name: "attribute_not_exists".to_owned(), + args: vec![Expr::Path(vec![PathElement::Attribute("pk".to_owned())])], + }; + let c1 = item("c1"); + let failing = [TransactWriteOp::Put { + key_info: &key_info, + item: &c1, + condition: Some(¬_exists), + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let c2 = item("c2"); + let other = [TransactWriteOp::Put { + key_info: &key_info, + item: &c2, + condition: None, + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let token = format!("tok-window-{}", uuid::Uuid::new_v4().simple()); + let key = |fingerprint: &'static str| IdempotencyKey { + account_id: ACCOUNT, + token: &token, + fingerprint, + }; + let cancelled = |result: &Result<(), StorageError>| { + matches!(result, Err(StorageError::TransactionCanceled(r)) + if r.len() == 1 && r[0].code == "ConditionalCheckFailed") + }; + + let first = engine + .transact_write_items(&failing, Some(key("fp-a"))) + .await; + assert!(cancelled(&first), "{first:?}"); + let (fingerprint, age) = token_document(&coll, &token).await; + assert_eq!(fingerprint, "cancelled:fp-a"); + assert!(age < 60_000, "age {age} ms"); + + let mismatch = engine.transact_write_items(&other, Some(key("fp-b"))).await; + assert!( + matches!(mismatch, Err(StorageError::IdempotentMismatch)), + "{mismatch:?}" + ); + assert!( + engine + .get_item(&key_info, &c2) + .await + .expect("get c2") + .is_none(), + "c2 was written" + ); + + age_token(&coll, &token, 9).await; + let again = engine + .transact_write_items(&failing, Some(key("fp-a"))) + .await; + assert!(cancelled(&again), "{again:?}"); + let (fingerprint, age) = token_document(&coll, &token).await; + assert_eq!(fingerprint, "cancelled:fp-a"); + assert!(age < 60_000, "age {age} ms after the second cancellation"); + + finish(engine).await; +} + +/// A cancellation replaces a token document older than the 10-minute read +/// window, as the SQL backends replace an expired row, and keeps a document +/// of another request stored less than 10 minutes ago. +#[tokio::test] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn live_a_cancellation_replaces_only_a_token_document_older_than_ten_minutes() { + let engine = scratch_engine().await; + let coll = token_collection(&engine).await; + let token = format!("tok-replace-{}", uuid::Uuid::new_v4().simple()); + + engine + .record_cancelled_token(ACCOUNT, &token, "fp-a") + .await + .expect("record fp-a"); + engine + .record_cancelled_token(ACCOUNT, &token, "fp-b") + .await + .expect("record fp-b over a young document"); + assert_eq!(token_document(&coll, &token).await.0, "cancelled:fp-a"); + + age_token(&coll, &token, 11).await; + engine + .record_cancelled_token(ACCOUNT, &token, "fp-b") + .await + .expect("record fp-b over an old document"); + let (fingerprint, age) = token_document(&coll, &token).await; + assert_eq!(fingerprint, "cancelled:fp-b"); + assert!(age < 60_000, "age {age} ms"); + assert_eq!( + coll.count_documents(doc! { "token": &token }) + .await + .expect("count"), + 1 + ); + + finish(engine).await; +} diff --git a/crates/storage-postgres/src/data/transactions.rs b/crates/storage-postgres/src/data/transactions.rs index 6748ed00d..98d9fe2f9 100644 --- a/crates/storage-postgres/src/data/transactions.rs +++ b/crates/storage-postgres/src/data/transactions.rs @@ -13,14 +13,19 @@ use extenddb_core::types::{ use extenddb_core::validation; use extenddb_storage::error::StorageError; use extenddb_storage::util::pk_to_text; -use extenddb_storage::{IdempotencyKey, TransactGetOp, TransactWriteOp}; +use extenddb_storage::{ + IdempotencyKey, TransactGetOp, TransactWriteOp, transact_first_empty_literal, + transact_key_check_pass, transact_op_empty_literal_checks, transact_op_key_checks, +}; use super::index::{ - IndexMeta, db_error, enqueue_async_indexes, fetch_write_path_indexes, sync_indexes, + IndexMeta, db_error, enqueue_async_indexes, fetch_write_path_indexes, fresh_attr_defs, + sync_indexes, }; use super::tx_helpers::{ check_idempotency_token_in_tx, delete_item_in_tx, fetch_item_for_update, fetch_item_in_tx, - insert_item_if_absent_in_tx, upsert_item_in_tx, write_stream_record_in_tx, + insert_item_if_absent_in_tx, record_cancelled_token, upsert_item_in_tx, + write_stream_record_in_tx, }; use crate::PostgresEngine; use crate::pg_util::is_conflict_abort; @@ -127,23 +132,54 @@ impl PostgresEngine { // applies to this transaction rather than up to 30 s later. let sys_delay = self.index_propagation_delay().await; + // Key checks first, for every member, before the token is checked and + // before any row is locked. They need only the request and the table + // metadata, and a fault among them decides the request under the + // service's precedence (the first such fault in request order) and + // leaves the token unused, so a request that is already decided never + // waits on a row another transaction holds. + transact_key_check_pass(ops.iter().map(|op| { + let indexes = &table_indexes[transact_op_table_name(op)]; + transact_op_key_checks( + op, + &index_key_refs(indexes), + fresh_attr_defs(indexes, &transact_op_key_info(op).attribute_definitions), + ) + }))?; + // The first Update member with an empty index-key literal. Only the + // members before it run: an empty value one of them writes from the + // item answers ahead of the literal, and no member after it is locked. + // A member before it waits on a row another transaction holds as any + // member does. + let empty_literal = transact_first_empty_literal(ops.iter().map(|op| { + transact_op_empty_literal_checks( + op, + &index_key_refs(&table_indexes[transact_op_table_name(op)]), + ) + })); + let run = empty_literal.as_ref().map_or(ops.len(), |(p, _)| *p); + let mut tx = self .data_pool .begin() .await .map_err(|e| StorageError::Internal(e.to_string()))?; - // A conflict abort outside the per-op loop cannot be tied to one item. + // A conflict abort outside the per-op loop cannot be tied to one item: + // `after_token_error` cancels every member and keeps the token used. // Unreachable at READ COMMITTED, where the ops already hold every row // lock; a stricter operator isolation can raise 40001 at commit. - let cancel_all = |e: StorageError| conflict_cancels_all(e, ops.len()); // Check the idempotency token within the transaction so token storage // and data writes commit together. The token is scoped to its account. - if let Some(key) = idempotency { - check_idempotency_token_in_tx(&mut tx, key.account_id, key.token, key.fingerprint) - .await - .map_err(cancel_all)?; + if let Some(key) = idempotency + && let Err(e) = + check_idempotency_token_in_tx(&mut tx, key.account_id, key.token, key.fingerprint) + .await + { + return Err( + after_token_error(&self.data_pool, Some(tx), idempotency, e, ops.len()).await, + ); } let mut reasons: Vec = vec![CancellationReason::none(); ops.len()]; @@ -151,36 +187,15 @@ impl PostgresEngine { let mut op_items: Vec<(Option, Option)> = vec![(None, None); ops.len()]; let mut any_failed = false; let mut failed_op: Option<(usize, StorageError)> = None; - - // Key checks first, for every member, before any row is locked. They - // need only the request and the table metadata, and a fault among them - // decides the request under the service's precedence (a wrong-typed - // key on any member, else an empty index key on any member), so a - // request that is already decided never waits on a row another - // transaction holds. - { - let mut key_reasons: Vec = - vec![CancellationReason::none(); ops.len()]; - let mut any_key_fault = false; - for (i, op) in ops.iter().enumerate() { - let indexes = &table_indexes[transact_op_table_name(op)]; - if let Err(r) = transact_op_key_checks(op, indexes) { - any_key_fault = true; - key_reasons[i] = r; - } - } - if any_key_fault { - return Err(match CancellationReason::resolve_key_faults(key_reasons) { - Ok(reasons) => StorageError::TransactionCanceled(reasons), - Err(message) => StorageError::Validation(message), - }); - } + if let Some((p, reason)) = &empty_literal { + any_failed = true; + reasons[*p] = reason.clone(); } // Run the ops in key order, not request order, so every transaction // locks its items in one global order and two cannot deadlock on each // other. Results keep their request positions. - for i in execution_order(ops) { + for i in execution_order(ops).into_iter().filter(|&i| i < run) { let op = &ops[i]; let indexes = &table_indexes[transact_op_table_name(op)]; let reason = execute_transact_write_op( @@ -210,8 +225,20 @@ impl PostgresEngine { if is_conflict_abort(&e) { // PostgreSQL broke a lock conflict by aborting this transaction. // Cancel it with the contended item named, as the service does. + // A member that ran earlier in execution order may have left an + // internal key fault (an Update's empty index key); the shared + // resolver ranks it as it does without the conflict, so no + // internal code reaches the client. The request passed the + // key checks, so its token stays used. reasons[i] = CancellationReason::transaction_conflict(); - return Err(StorageError::TransactionCanceled(reasons)); + // End the aborted transaction first: its uncommitted token row + // would block the record below. + tx.rollback().await.map_err(db_error)?; + keep_cancelled_token(&self.data_pool, idempotency).await; + return Err(match CancellationReason::resolve_key_faults(reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + }); } // Infrastructure error: abort without leaking internal details // into cancellation reasons. @@ -222,8 +249,11 @@ impl PostgresEngine { // Key faults left the members as internal reasons; the shared // resolver applies the service's cross-member precedence and // decides between a cancellation and a top-level - // ValidationException. Every member has run, so the first - // wrong-typed key in request order is the one named. + // ValidationException. The request passed the key checks, so + // its token stays used, as the service keeps it: roll back the + // writes, then record the token as cancelled. + tx.rollback().await.map_err(db_error)?; + keep_cancelled_token(&self.data_pool, idempotency).await; return Err(match CancellationReason::resolve_key_faults(reasons) { Ok(reasons) => StorageError::TransactionCanceled(reasons), Err(message) => StorageError::Validation(message), @@ -238,8 +268,8 @@ impl PostgresEngine { | TransactWriteOp::Update { stream, .. } => stream.as_ref(), TransactWriteOp::ConditionCheck { .. } => None, }; - if let Some(capture) = capture { - write_stream_record_in_tx( + if let Some(capture) = capture + && let Err(e) = write_stream_record_in_tx( &mut tx, match op { TransactWriteOp::Put { key_info, .. } @@ -252,7 +282,15 @@ impl PostgresEngine { new_item.as_ref(), ) .await - .map_err(cancel_all)?; + { + return Err(after_token_error( + &self.data_pool, + Some(tx), + idempotency, + e, + ops.len(), + ) + .await); } } @@ -263,13 +301,25 @@ impl PostgresEngine { for op in ops { let name = transact_op_table_name(op); if !missed.contains_key(name) { - let found = super::gsi_build::missed_gsis( + let found = match super::gsi_build::missed_gsis( &self.pool, transact_op_table_id(op), &table_indexes[name], ) .await - .map_err(cancel_all)?; + { + Ok(found) => found, + Err(e) => { + return Err(after_token_error( + &self.data_pool, + Some(tx), + idempotency, + e, + ops.len(), + ) + .await); + } + }; missed.insert(name.to_owned(), found); } } @@ -290,7 +340,7 @@ impl PostgresEngine { | TransactWriteOp::ConditionCheck { key_info, .. } => key_info, }; // One row per async index, each honoring its own propagation delay. - let n = enqueue_async_indexes( + let n = match enqueue_async_indexes( &mut tx, key_info, indexes, @@ -299,14 +349,26 @@ impl PostgresEngine { sys_delay, ) .await - .map_err(cancel_all)?; + { + Ok(n) => n, + Err(e) => { + return Err(after_token_error( + &self.data_pool, + Some(tx), + idempotency, + e, + ops.len(), + ) + .await); + } + }; // Vector maintenance for all three write kinds in one place, rather // than in each branch above: this loop already visits exactly the ops // that changed an item, with both images in hand, and it runs inside // the same transaction. Three call sites would have been three chances // to diverge on which image is passed. - let vector_n = crate::data::vector_index::maintain_vector_indexes( + let vector_n = match crate::data::vector_index::maintain_vector_indexes( &mut tx, &table_vector_metas[transact_op_table_name(op)], &key_info.table_id, @@ -317,15 +379,37 @@ impl PostgresEngine { sys_delay, ) .await - .map_err(cancel_all)?; + { + Ok(n) => n, + Err(e) => { + return Err(after_token_error( + &self.data_pool, + Some(tx), + idempotency, + e, + ops.len(), + ) + .await); + } + }; let repair_n = match ( missed.get(transact_op_table_name(op)), new_item.as_ref().or(old_item.as_ref()), ) { (Some(m), Some(item)) if !m.is_empty() => { - super::gsi_build::queue_repairs(&mut tx, key_info, m, item) - .await - .map_err(cancel_all)? + match super::gsi_build::queue_repairs(&mut tx, key_info, m, item).await { + Ok(n) => n, + Err(e) => { + return Err(after_token_error( + &self.data_pool, + Some(tx), + idempotency, + e, + ops.len(), + ) + .await); + } + } } _ => 0, }; @@ -334,7 +418,17 @@ impl PostgresEngine { } } - tx.commit().await.map_err(db_error).map_err(cancel_all)?; + if let Err(e) = tx.commit().await { + // A failed commit has ended the transaction already. + return Err(after_token_error( + &self.data_pool, + None, + idempotency, + db_error(e), + ops.len(), + ) + .await); + } if needs_notify && let Some(ref q) = self.gsi_queue { q.notify_workers(); @@ -372,6 +466,45 @@ fn conflict_cancels_all(e: StorageError, n_ops: usize) -> StorageError { } } +/// Keep the token of a request cancelled after the request-level key checks +/// used, once its transaction has ended (`record_cancelled_token`). A failed +/// write is logged: nothing was applied, so the request still answers its +/// cancellation, and the token is left unused. +async fn keep_cancelled_token(pool: &sqlx::PgPool, idempotency: Option>) { + if let Some(key) = idempotency + && let Err(e) = + record_cancelled_token(pool, key.account_id, key.token, key.fingerprint).await + { + tracing::warn!("record_cancelled_token failed (the token stays unused): {e}"); + } +} + +/// An error after the token check, outside the member loop. A conflict abort +/// cancels the request with `TransactionConflict` on every member and keeps +/// its token used, as every cancellation after the key checks does (the +/// service keeps a token used after `TransactionConflict`, measured +/// 2026-11-05). `tx`, when the transaction has not ended yet, is rolled back +/// first: its uncommitted token row would make the record wait. Any other +/// error passes through, and `tx` rolls back when it is dropped. +async fn after_token_error( + pool: &sqlx::PgPool, + tx: Option>, + idempotency: Option>, + e: StorageError, + n_ops: usize, +) -> StorageError { + if !is_conflict_abort(&e) { + return e; + } + if let Some(tx) = tx + && let Err(rollback) = tx.rollback().await + { + tracing::warn!("rollback after a conflict abort failed: {rollback}"); + } + keep_cancelled_token(pool, idempotency).await; + conflict_cancels_all(e, n_ops) +} + /// Request positions of `ops`, sorted by table and primary key. fn execution_order(ops: &[TransactWriteOp<'_>]) -> Vec { let mut order: Vec = (0..ops.len()).collect(); @@ -399,6 +532,16 @@ fn lock_key<'a>(op: &'a TransactWriteOp<'_>) -> (&'a str, Vec>) { (&key_info.table_id, values) } +/// The table metadata of a transactional write operation. +fn transact_op_key_info<'a>(op: &'a TransactWriteOp<'_>) -> &'a TableKeyInfo { + match op { + TransactWriteOp::Put { key_info, .. } + | TransactWriteOp::Delete { key_info, .. } + | TransactWriteOp::Update { key_info, .. } + | TransactWriteOp::ConditionCheck { key_info, .. } => key_info, + } +} + /// Extract the table name from a transactional write operation. fn transact_op_table_name<'a>(op: &'a TransactWriteOp<'_>) -> &'a str { match op { @@ -437,71 +580,6 @@ pub(super) fn index_key_refs(indexes: &[IndexMeta]) -> Vec, - indexes: &[IndexMeta], -) -> Result<(), CancellationReason> { - match op { - TransactWriteOp::Put { key_info, item, .. } => { - validation::validate_item_keys( - item, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; - let idx_refs = index_key_refs(indexes); - validation::validate_index_key_types( - item, - &idx_refs, - super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; - validation::validate_index_key_not_empty( - item, - &idx_refs, - validation::SecondaryIndexEmptyContext::Item, - ) - .map_err(|e| CancellationReason::index_key_empty_abort(e.to_string())) - } - TransactWriteOp::Update { - key_info, - key, - actions, - maps, - .. - } => { - validation::validate_batch_key_only( - key, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; - validation::validate_index_key_literal_operands( - actions, - &index_key_refs(indexes), - super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), - maps, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) - } - TransactWriteOp::Delete { key_info, key, .. } - | TransactWriteOp::ConditionCheck { key_info, key, .. } => { - validation::validate_batch_key_only( - key, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) - } - } -} - enum TxnOpError { /// User-driven failure — becomes a per-item cancellation reason. Cancel(CancellationReason), @@ -542,7 +620,12 @@ async fn execute_transact_write_op( // reasons, matching real DynamoDB behavior. // Key checks ran in the pre-pass; repeated so the arm stays // correct if called on its own. - transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; + transact_op_key_checks( + op, + &index_key_refs(indexes), + fresh_attr_defs(indexes, &key_info.attribute_definitions), + ) + .map_err(TxnOpError::Cancel)?; let mut existing = fetch_item_for_update(tx, key_info, item) .await .map_err(TxnOpError::Storage)?; @@ -687,8 +770,9 @@ async fn execute_transact_write_op( return_values_on_ccf, .. } => { - transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; let idx_refs = index_key_refs(indexes); + let attr_defs = fresh_attr_defs(indexes, &key_info.attribute_definitions); + transact_op_key_checks(op, &idx_refs, attr_defs).map_err(TxnOpError::Cancel)?; let mut existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; @@ -724,18 +808,21 @@ async fn execute_transact_write_op( .map_err(|e| { TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) })?; - // Secondary-index keys on the written item: a value computed from - // the existing item (an attribute reference, the fallback of - // if_not_exists) of the wrong type, reported with the service's - // computed-value wording, and a key left empty. - validation::validate_index_key_types_after_update( - &item, - &idx_refs, - super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), - ) - .map_err(|e| { - TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) - })?; + // Secondary-index keys on the written item: a wrong-typed value + // the statement wrote from an attribute reference or from + // if_not_exists, reported with the service's computed-value + // wording, and a key such a value leaves empty (an empty + // literal was found before any member ran). With the path + // absent the if_not_exists fallback is a literal from the + // request, which the service ranks with the values taken from + // the item. The type fault is a plain positional reason: the + // service leaves the other members' outcomes in place beside + // it, where a wrong type supplied in the request (the + // pre-pass) masks them. + validation::validate_index_key_types_after_update(&item, &idx_refs, attr_defs) + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) + })?; validation::validate_index_key_not_empty( &item, &idx_refs, diff --git a/crates/storage-postgres/src/data/tx_helpers.rs b/crates/storage-postgres/src/data/tx_helpers.rs index d17d0c437..239108f62 100755 --- a/crates/storage-postgres/src/data/tx_helpers.rs +++ b/crates/storage-postgres/src/data/tx_helpers.rs @@ -10,9 +10,9 @@ use extenddb_core::types::{ AttributeValue, Item, StreamEventName, StreamRecord, StreamRecordData, StreamViewType, TableKeyInfo, item_size_bytes, }; -use extenddb_storage::StreamCapture; use extenddb_storage::error::StorageError; use extenddb_storage::util::{SortKeyValue, parse_sk, pk_to_text, sk_column, sk_info}; +use extenddb_storage::{StoredToken, StreamCapture, cancelled_token_fingerprint}; use super::index::db_error; use super::{data_table_name, json_to_item}; @@ -403,6 +403,8 @@ pub(super) async fn check_idempotency_token_in_tx( token: &str, fingerprint: &str, ) -> Result<(), StorageError> { + // A token older than 10 minutes, or one the same request left cancelled + // (see `StoredToken::of`), is taken for this run. let row: Option<(String, bool)> = sqlx::query_as( r"WITH ins AS ( INSERT INTO idempotency_tokens (account_id, token, fingerprint) @@ -410,6 +412,7 @@ pub(super) async fn check_idempotency_token_in_tx( ON CONFLICT (account_id, token) DO UPDATE SET fingerprint = $3, created_at = NOW() WHERE idempotency_tokens.created_at <= NOW() - INTERVAL '10 minutes' + OR idempotency_tokens.fingerprint = $4 RETURNING fingerprint, TRUE AS inserted ) SELECT fingerprint, inserted FROM ins @@ -425,13 +428,44 @@ pub(super) async fn check_idempotency_token_in_tx( .bind(account_id) .bind(token) .bind(fingerprint) + .bind(cancelled_token_fingerprint(fingerprint)) .fetch_optional(&mut **tx) .await .map_err(db_error)?; match row { Some((_, true)) | None => Ok(()), - Some((stored, false)) if stored == fingerprint => Err(StorageError::IdempotentReplay), - Some((_, false)) => Err(StorageError::IdempotentMismatch), + Some((stored, false)) => match StoredToken::of(&stored, fingerprint) { + StoredToken::Replay => Err(StorageError::IdempotentReplay), + StoredToken::Rerun | StoredToken::Mismatch => Err(StorageError::IdempotentMismatch), + }, } } + +/// Keep a token used after its request was cancelled past the request-level +/// key checks, once the request's transaction has rolled back: store it with +/// [`cancelled_token_fingerprint`] and the time now, so the token stays used +/// for 10 minutes from the last cancellation (measured 2026-11-05). A row +/// another request or another send stored less than 10 minutes ago is kept. +pub(super) async fn record_cancelled_token( + pool: &sqlx::PgPool, + account_id: &str, + token: &str, + fingerprint: &str, +) -> Result<(), StorageError> { + sqlx::query( + r"INSERT INTO idempotency_tokens (account_id, token, fingerprint) + VALUES ($1, $2, $3) + ON CONFLICT (account_id, token) DO UPDATE + SET fingerprint = $3, created_at = NOW() + WHERE idempotency_tokens.created_at <= NOW() - INTERVAL '10 minutes' + OR idempotency_tokens.fingerprint = $3", + ) + .bind(account_id) + .bind(token) + .bind(cancelled_token_fingerprint(fingerprint)) + .execute(pool) + .await + .map_err(db_error)?; + Ok(()) +} diff --git a/crates/storage-postgres/src/data/update_item.rs b/crates/storage-postgres/src/data/update_item.rs index a92ae9660..8bffe9940 100755 --- a/crates/storage-postgres/src/data/update_item.rs +++ b/crates/storage-postgres/src/data/update_item.rs @@ -67,19 +67,23 @@ impl PostgresEngine { let (indexes, vector_metas, stamp) = fetch_write_path_indexes(&key_info.table_id, &self.pool).await?; // Literal operands assigned to a secondary-index key, checked on the - // statement ahead of the condition as the service checks them. The - // engine ran this check against its cached key info, which does not list - // a GSI until the UpdateTable that adds it returns; this read lists it - // from the moment its CREATING row commits, so a literal of the wrong - // type for an index still being built gets the same per-index wording. + // statement ahead of the condition as the service checks them: the type + // first, then an empty value. The engine ran these checks against its + // cached key info, which does not list a GSI until the UpdateTable that + // adds it returns; this read lists it from the moment its CREATING row + // commits, so a literal for an index still being built gets the same + // answer. if !indexes.is_empty() { + let idx_refs = super::transactions::index_key_refs(&indexes); validation::validate_index_key_literal_operands( actions, - &super::transactions::index_key_refs(&indexes), + &idx_refs, super::index::fresh_attr_defs(&indexes, &key_info.attribute_definitions), maps, ) .map_err(|e| StorageError::Validation(e.to_string()))?; + validation::validate_index_key_empty_literals(actions, &idx_refs, maps) + .map_err(|e| StorageError::Validation(e.to_string()))?; } // Read whenever anything can propagate, secondary or vector. Gating this on // the secondary set alone made a vector-only table ignore the configured diff --git a/crates/storage-postgres/tests/twi_conflict.rs b/crates/storage-postgres/tests/twi_conflict.rs index 8d5205469..c63143272 100644 --- a/crates/storage-postgres/tests/twi_conflict.rs +++ b/crates/storage-postgres/tests/twi_conflict.rs @@ -29,7 +29,7 @@ use extenddb_core::types::{ StreamViewType, TableKeyInfo, }; use extenddb_storage::error::StorageError; -use extenddb_storage::{DataEngine, StreamCapture, TableEngine, TransactWriteOp}; +use extenddb_storage::{DataEngine, IdempotencyKey, StreamCapture, TableEngine, TransactWriteOp}; use extenddb_storage_postgres::{PostgresConfig, PostgresEngine}; use sqlx::postgres::PgPoolOptions; use sqlx::{PgPool, Postgres, Transaction}; @@ -249,13 +249,9 @@ async fn a_deadlock_cancels_with_transaction_conflict_on_the_waiting_item() { // An outside transaction holds `b`. The write transaction locks `a`, then // waits on `b`. The outside transaction then asks for `a`, which closes the - // cycle. The write transaction waited first, so its deadlock check runs - // first and PostgreSQL aborts it. - let mut holder = s.db.begin().await.expect("begin the outside transaction"); - let holder_pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()") - .fetch_one(&mut *holder) - .await - .expect("read the backend pid"); + // cycle. The write transaction's deadlock check runs first (see + // `outside_transaction`), so PostgreSQL aborts it. + let (mut holder, holder_pid) = outside_transaction(&s.db).await; sqlx::query(&format!("SELECT 1 FROM {table} WHERE pk = 'b' FOR UPDATE")) .execute(&mut *holder) .await @@ -308,6 +304,101 @@ async fn a_deadlock_cancels_with_transaction_conflict_on_the_waiting_item() { s.cleanup().await; } +#[tokio::test] +async fn a_deadlock_after_an_empty_index_key_answers_the_empty_key() { + // An Update member that writes an empty value to an index key from the + // stored item records that fault once its statement is applied (an empty + // literal would be found before any row is locked), and the member after + // it in execution order then deadlocks. The request answers the empty + // key's ValidationException, as it does without the conflict, and no + // internal cancellation code reaches the caller. + let test = "a_deadlock_after_an_empty_index_key_answers_the_empty_key"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let key_info = gsi_table(&s).await; + let plain = ExpressionMaps::default(); + let mut seed_a = item("a", "seed"); + seed_a.insert("e".to_owned(), AttributeValue::S(String::new())); + for seed in [seed_a, item("b", "seed")] { + s.engine + .put_item(&key_info, seed, false, None, &plain, None) + .await + .expect("seed an item"); + } + let table = data_table(&s.db).await; + + // An outside transaction holds `b`. The ops run in key order, so the + // Update of `a` runs first: it locks `a`, applies `SET a1 = e` and + // records the empty key. The Put of `b` then waits on `b`, and the + // outside transaction asks for `a`, which closes the cycle. The write + // transaction's deadlock check runs first (see `outside_transaction`), so + // PostgreSQL aborts it. + let (mut holder, holder_pid) = outside_transaction(&s.db).await; + sqlx::query(&format!("SELECT 1 FROM {table} WHERE pk = 'b' FOR UPDATE")) + .execute(&mut *holder) + .await + .expect("lock b"); + + let tokens = expression::tokenize("SET a1 = e").expect("tokenize"); + let actions = expression::parse_update(&tokens).expect("parse"); + let (key_a, new_b) = (key("a"), item("b", "twi")); + // Request order is b, a; the empty key decides wherever it is. + let ops = [ + put(&key_info, &new_b, &plain), + TransactWriteOp::Update { + key_info: &key_info, + key: &key_a, + actions: &actions, + condition: None, + maps: &plain, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }, + ]; + let twi = s.engine.transact_write_items(&ops, None); + let close_cycle = async { + wait_for_lock_waiters(&s.db, holder_pid, 1).await; + sqlx::query(&format!("SELECT 1 FROM {table} WHERE pk = 'a' FOR UPDATE")) + .execute(&mut *holder) + .await + .expect("the outside transaction gets a once the write transaction aborts"); + }; + // Bounded, so a transaction that never closes the cycle fails the test + // instead of hanging it. + let (result, ()) = tokio::time::timeout(Duration::from_secs(30), async { + tokio::join!(twi, close_cycle) + }) + .await + .expect("the write transaction finished"); + holder.rollback().await.expect("release the outside locks"); + + match result { + Err(StorageError::Validation(msg)) => assert_eq!( + msg, + "One or more parameter values are not valid. The update expression attempted to \ + update a secondary index key to a value that is not supported. The AttributeValue \ + for a key attribute cannot contain an empty string value." + ), + other => panic!("expected the empty key's ValidationException, got {other:?}"), + } + // The canceled transaction applied nothing. + for pk in ["a", "b"] { + let (v, a1): (String, Option) = sqlx::query_as(&format!( + "SELECT item_data->'v'->>'S', item_data->'a1'->>'S' FROM {table} WHERE pk = $1" + )) + .bind(pk) + .fetch_one(&s.db) + .await + .expect("read the item"); + assert_eq!(v, "seed", "item {pk}"); + assert_eq!(a1, None, "item {pk}"); + } + + s.cleanup().await; +} + #[tokio::test] async fn opposite_request_orders_do_not_deadlock() { let test = "opposite_request_orders_do_not_deadlock"; @@ -486,6 +577,70 @@ async fn an_invalid_first_op_answers_without_waiting_on_later_locks() { s.cleanup().await; } +#[tokio::test] +async fn an_empty_literal_first_answers_without_waiting_on_later_locks() { + // An Update member's empty index-key literal decides the request when no + // member before it writes an empty value from the item. With it first no + // member runs, so the request must not wait on the next op's row, held + // here by an outside transaction. + let test = "an_empty_literal_first_answers_without_waiting_on_later_locks"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let maps = ExpressionMaps::default(); + let key_info = gsi_table(&s).await; + let b = item("b", "seed"); + s.engine + .put_item(&key_info, b.clone(), false, None, &maps, None) + .await + .expect("seed b"); + let table = data_table(&s.db).await; + let mut holder = s.db.begin().await.expect("begin the outside transaction"); + sqlx::query(&format!("SELECT 1 FROM {table} WHERE pk = 'b' FOR UPDATE")) + .execute(&mut *holder) + .await + .expect("lock b"); + + let empty = ExpressionMaps::new( + std::collections::HashMap::new(), + std::collections::HashMap::from([("e".to_owned(), AttributeValue::S(String::new()))]), + ); + let tokens = expression::tokenize("SET a1 = :e").expect("tokenize"); + let actions = expression::parse_update(&tokens).expect("parse"); + let key_a = key("a"); + let ops = [ + TransactWriteOp::Update { + key_info: &key_info, + key: &key_a, + actions: &actions, + condition: None, + maps: &empty, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }, + put(&key_info, &b, &maps), + ]; + let result = tokio::time::timeout( + Duration::from_secs(5), + s.engine.transact_write_items(&ops, None), + ) + .await + .expect("the transaction answered without waiting on b"); + holder.rollback().await.expect("release b"); + match result { + Err(StorageError::Validation(msg)) => assert_eq!( + msg, + "One or more parameter values are not valid. The update expression attempted to \ + update a secondary index key to a value that is not supported. The AttributeValue \ + for a key attribute cannot contain an empty string value." + ), + other => panic!("expected the empty literal's ValidationException, got {other:?}"), + } + + s.cleanup().await; +} + /// The condition `attribute_not_exists(pk)`. fn not_exists() -> Expr { let tokens = expression::tokenize("attribute_not_exists(pk)").expect("tokenize"); @@ -518,6 +673,32 @@ async fn create_in_flight( (creator, pid) } +/// Begin the outside transaction of a deadlock test, with its deadlock check +/// later than the write transaction's, and return it with its backend pid. +/// +/// PostgreSQL aborts the transaction whose deadlock check finds the cycle, and +/// each waiter checks once, `deadlock_timeout` after it starts to wait. The +/// write transaction waits first, but with the default 1 s on both sides the +/// two checks are milliseconds apart, and under load the outside +/// transaction's can run first and abort it instead. With 5 s here, the write +/// transaction's check runs 1 s into its wait, with the cycle closed, and the +/// outside transaction gets its lock when the write transaction aborts. +/// `SET LOCAL` ends with the transaction, so the pooled connection keeps the +/// server default. The role must be allowed to set `deadlock_timeout` (a +/// superuser, as in CI). +async fn outside_transaction(db: &PgPool) -> (sqlx::Transaction<'static, sqlx::Postgres>, i32) { + let mut holder = db.begin().await.expect("begin the outside transaction"); + sqlx::query("SET LOCAL deadlock_timeout = '5s'") + .execute(&mut *holder) + .await + .expect("set the outside transaction's deadlock timeout"); + let pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()") + .fetch_one(&mut *holder) + .await + .expect("read the backend pid"); + (holder, pid) +} + /// Wait until `n` backends other than `own_pid` wait on a lock. async fn wait_for_lock_waiters(db: &PgPool, own_pid: i32, n: i64) { for _ in 0..500 { @@ -889,3 +1070,201 @@ async fn a_delete_that_loses_its_reservation_removes_the_winner_everywhere() { s.cleanup().await; } + +#[tokio::test] +async fn a_conflict_cancellation_keeps_the_token_used_and_the_same_request_runs_again() { + // Measured on the service (2026-11-05, Q4a and Q4b): a request canceled + // with TransactionConflict keeps its ClientRequestToken used. Another + // request with the token answers IdempotentParameterMismatchException and + // writes nothing, and the same request sent again with the token runs. + let test = "a_conflict_cancellation_keeps_the_token_used_and_the_same_request_runs_again"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let maps = ExpressionMaps::default(); + let key_info = seeded_table(&s, &maps).await; + let table = data_table(&s.db).await; + let key = IdempotencyKey { + account_id: ACCOUNT, + token: "twi-conflict-token", + fingerprint: "fp-conflict", + }; + + // The deadlock of a_deadlock_cancels_with_transaction_conflict_on_the_waiting_item, + // with a token. + let (mut holder, holder_pid) = outside_transaction(&s.db).await; + sqlx::query(&format!("SELECT 1 FROM {table} WHERE pk = 'b' FOR UPDATE")) + .execute(&mut *holder) + .await + .expect("lock b"); + let (new_a, new_b) = (item("a", "twi"), item("b", "twi")); + let ops = [put(&key_info, &new_b, &maps), put(&key_info, &new_a, &maps)]; + let twi = s.engine.transact_write_items(&ops, Some(key)); + let close_cycle = async { + wait_for_lock_waiters(&s.db, holder_pid, 1).await; + sqlx::query(&format!("SELECT 1 FROM {table} WHERE pk = 'a' FOR UPDATE")) + .execute(&mut *holder) + .await + .expect("the outside transaction gets a once the write transaction aborts"); + }; + // Bounded, so a transaction that never closes the cycle fails the test + // instead of hanging it. + let (result, ()) = tokio::time::timeout(Duration::from_secs(30), async { + tokio::join!(twi, close_cycle) + }) + .await + .expect("the write transaction finished"); + holder.rollback().await.expect("release the outside locks"); + match result { + Err(StorageError::TransactionCanceled(reasons)) => { + let codes: Vec<&str> = reasons.iter().map(|r| r.code.as_str()).collect(); + assert_eq!(codes, ["TransactionConflict", "None"], "{reasons:?}"); + } + other => panic!("expected a TransactionConflict cancellation, got {other:?}"), + } + + // Q4a: another request with the token. + let new_q = item("q", "other"); + let other = [put(&key_info, &new_q, &maps)]; + let other_key = IdempotencyKey { + fingerprint: "fp-other", + ..key + }; + let result = tokio::time::timeout( + Duration::from_secs(30), + s.engine.transact_write_items(&other, Some(other_key)), + ) + .await + .expect("the other request finished"); + assert!( + matches!(result, Err(StorageError::IdempotentMismatch)), + "{result:?}" + ); + let written: i64 = sqlx::query_scalar(&format!("SELECT count(*) FROM {table} WHERE pk = 'q'")) + .fetch_one(&s.db) + .await + .expect("count q"); + assert_eq!(written, 0, "the other request wrote q"); + + // Q4b: the same request again, with nothing held, runs and commits. + tokio::time::timeout( + Duration::from_secs(30), + s.engine.transact_write_items(&ops, Some(key)), + ) + .await + .expect("the same request finished") + .expect("the same request runs again and commits"); + for pk in ["a", "b"] { + let v: String = sqlx::query_scalar(&format!( + "SELECT item_data->'v'->>'S' FROM {table} WHERE pk = $1" + )) + .bind(pk) + .fetch_one(&s.db) + .await + .expect("read the item"); + assert_eq!(v, "twi", "item {pk}"); + } + + s.cleanup().await; +} + +#[tokio::test] +async fn a_cancelled_token_is_used_for_ten_minutes_from_its_last_cancellation() { + // Measured on the service (2026-11-05, Q1a to Q1c): a cancelled token is + // still used 9 minutes after its request and free 11 minutes after it, + // and a cancellation of the same request sent again starts the window + // again. The test moves the stored created_at back instead of waiting. + let test = "a_cancelled_token_is_used_for_ten_minutes_from_its_last_cancellation"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let maps = ExpressionMaps::default(); + let key_info = seeded_table(&s, &maps).await; + let table = data_table(&s.db).await; + let token = "twi-window-token"; + let key = IdempotencyKey { + account_id: ACCOUNT, + token, + fingerprint: "fp-ccf", + }; + let age = |minutes: i32| { + let db = s.db.clone(); + async move { + sqlx::query( + "UPDATE idempotency_tokens SET created_at = NOW() - make_interval(mins => $1) \ + WHERE token = $2", + ) + .bind(minutes) + .bind(token) + .execute(&db) + .await + .expect("move created_at back"); + } + }; + // A Put of `a` with attribute_not_exists(pk): `a` exists, so it fails. + let condition = not_exists(); + let ccf_item = item("a", "ccf"); + let ccf = [TransactWriteOp::Put { + key_info: &key_info, + item: &ccf_item, + condition: Some(&condition), + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let cancelled = |result: Result<(), StorageError>| { + matches!(&result, Err(StorageError::TransactionCanceled(r)) + if r.len() == 1 && r[0].code == "ConditionalCheckFailed") + }; + + assert!(cancelled( + s.engine.transact_write_items(&ccf, Some(key)).await + )); + // Q1c: 6 minutes later the same request is cancelled again, which starts + // the window again. + age(6).await; + assert!(cancelled( + s.engine.transact_write_items(&ccf, Some(key)).await + )); + let seconds: f64 = sqlx::query_scalar( + "SELECT EXTRACT(EPOCH FROM NOW() - created_at)::float8 FROM idempotency_tokens \ + WHERE token = $1", + ) + .bind(token) + .fetch_one(&s.db) + .await + .expect("read created_at"); + assert!( + seconds < 60.0, + "created_at is {seconds} s old after the second cancellation" + ); + + // Q1b: 9 minutes after the last cancellation the token is still used. + let new_q = item("q", "other"); + let other = [put(&key_info, &new_q, &maps)]; + let other_key = IdempotencyKey { + fingerprint: "fp-other", + ..key + }; + age(9).await; + let result = s.engine.transact_write_items(&other, Some(other_key)).await; + assert!( + matches!(result, Err(StorageError::IdempotentMismatch)), + "{result:?}" + ); + // Q1a: 11 minutes after it the token is free. + age(11).await; + s.engine + .transact_write_items(&other, Some(other_key)) + .await + .expect("a token 11 minutes past its cancellation is free"); + let written: i64 = sqlx::query_scalar(&format!("SELECT count(*) FROM {table} WHERE pk = 'q'")) + .fetch_one(&s.db) + .await + .expect("count q"); + assert_eq!(written, 1); + + s.cleanup().await; +} diff --git a/crates/storage-sqlite/src/data/transactions.rs b/crates/storage-sqlite/src/data/transactions.rs index f3192c7e5..d2e0b5a57 100644 --- a/crates/storage-sqlite/src/data/transactions.rs +++ b/crates/storage-sqlite/src/data/transactions.rs @@ -13,12 +13,17 @@ use extenddb_core::expression::{self, ExpressionMaps}; use extenddb_core::types::{CancellationReason, Item, ReturnValuesOnConditionCheckFailure}; use extenddb_core::validation; use extenddb_storage::error::StorageError; -use extenddb_storage::{IdempotencyKey, TransactGetOp, TransactWriteOp}; +use extenddb_storage::{ + IdempotencyKey, TransactGetOp, TransactWriteOp, transact_first_empty_literal, + transact_key_check_pass, transact_op_empty_literal_checks, transact_op_key_checks, +}; -use super::index::{IndexMeta, enqueue_async_indexes, fetch_indexes_for_table, sync_indexes}; +use super::index::{ + IndexMeta, enqueue_async_indexes, fetch_indexes_for_table, fresh_attr_defs, sync_indexes, +}; use super::tx_helpers::{ check_idempotency_token_in_tx, delete_item_in_tx, fetch_item_for_update, fetch_item_in_tx, - upsert_item_in_tx, write_stream_record_in_tx, + record_cancelled_token, upsert_item_in_tx, write_stream_record_in_tx, }; use crate::sqlite_util::format_timestamp; use crate::store::SqliteEngine; @@ -86,6 +91,27 @@ impl SqliteEngine { } } + // Key checks first, for every member, before the token is checked and + // before any row is read. They need only the request and the table + // metadata, and a fault among them decides the request under the + // service's precedence (the first such fault in request order) and + // leaves the token unused. + transact_key_check_pass(ops.iter().map(|op| { + let indexes = &table_indexes[op_table_name(op)]; + transact_op_key_checks( + op, + &index_key_refs(indexes), + fresh_attr_defs(indexes, &op_key_info(op).attribute_definitions), + ) + }))?; + // The first Update member with an empty index-key literal. Only the + // members before it run: an empty value one of them writes from the + // item answers ahead of the literal. + let empty_literal = transact_first_empty_literal(ops.iter().map(|op| { + transact_op_empty_literal_checks(op, &index_key_refs(&table_indexes[op_table_name(op)])) + })); + let run = empty_literal.as_ref().map_or(ops.len(), |(p, _)| *p); + let mut tx = self .pool .begin_with("BEGIN IMMEDIATE") @@ -100,35 +126,11 @@ impl SqliteEngine { .await?; } - // Key checks first, for every member, before any row is read or - // locked. They need only the request and the table metadata, and a - // fault among them decides the request under the service's precedence - // (a wrong-typed key on any member, else an empty index key on any - // member), so a decided request never waits on a later member's row. - { - let mut key_reasons: Vec = - vec![CancellationReason::none(); ops.len()]; - let mut any_key_fault = false; - for (i, op) in ops.iter().enumerate() { - let indexes = &table_indexes[op_table_name(op)]; - if let Err(r) = transact_op_key_checks(op, indexes) { - any_key_fault = true; - key_reasons[i] = r; - } - } - if any_key_fault { - return Err(match CancellationReason::resolve_key_faults(key_reasons) { - Ok(reasons) => StorageError::TransactionCanceled(reasons), - Err(message) => StorageError::Validation(message), - }); - } - } - let mut reasons: Vec = Vec::with_capacity(ops.len()); let mut op_items: Vec<(Option, Option)> = Vec::with_capacity(ops.len()); let mut any_failed = false; - for op in ops { + for op in &ops[..run] { let indexes = &table_indexes[op_table_name(op)]; match execute_transact_write_op( &mut tx, @@ -151,11 +153,29 @@ impl SqliteEngine { Err(TxnOpError::Storage(e)) => return Err(e), } } + if let Some((_, reason)) = empty_literal { + any_failed = true; + reasons.push(reason); + reasons.resize(ops.len(), CancellationReason::none()); + } if any_failed { - // Dropping `tx` without commit rolls back all writes. Key faults - // left the members as internal reasons; the shared resolver applies - // the service's cross-member precedence. + // Roll back every write. Key faults left the members as internal + // reasons; the shared resolver applies the service's cross-member + // precedence. The request passed the key checks, so its token + // stays used, as the service keeps it. + tx.rollback() + .await + .map_err(|e| StorageError::Internal(e.to_string()))?; + if let Some(key) = idempotency + && let Err(e) = + record_cancelled_token(&self.pool, key.account_id, key.token, key.fingerprint) + .await + { + // Nothing was applied, so the request still answers its + // cancellation; the token is left unused. + tracing::warn!("record_cancelled_token failed (the token stays unused): {e}"); + } return Err(match CancellationReason::resolve_key_faults(reasons) { Ok(reasons) => StorageError::TransactionCanceled(reasons), Err(message) => StorageError::Validation(message), @@ -290,71 +310,6 @@ pub(crate) fn index_key_refs(indexes: &[IndexMeta]) -> Vec, - indexes: &[IndexMeta], -) -> Result<(), CancellationReason> { - match op { - TransactWriteOp::Put { key_info, item, .. } => { - validation::validate_item_keys( - item, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; - let idx_refs = index_key_refs(indexes); - validation::validate_index_key_types( - item, - &idx_refs, - super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; - validation::validate_index_key_not_empty( - item, - &idx_refs, - validation::SecondaryIndexEmptyContext::Item, - ) - .map_err(|e| CancellationReason::index_key_empty_abort(e.to_string())) - } - TransactWriteOp::Update { - key_info, - key, - actions, - maps, - .. - } => { - validation::validate_batch_key_only( - key, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; - validation::validate_index_key_literal_operands( - actions, - &index_key_refs(indexes), - super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), - maps, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) - } - TransactWriteOp::Delete { key_info, key, .. } - | TransactWriteOp::ConditionCheck { key_info, key, .. } => { - validation::validate_batch_key_only( - key, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) - } - } -} - /// Execute one transact-write op, returning `(old, new)` images for stream /// capture, or a cancellation reason on a failed condition / validation. async fn execute_transact_write_op( @@ -375,7 +330,12 @@ async fn execute_transact_write_op( } => { // Key checks ran in the pre-pass; repeated so the arm stays // correct if called on its own. - transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; + transact_op_key_checks( + op, + &index_key_refs(indexes), + fresh_attr_defs(indexes, &key_info.attribute_definitions), + ) + .map_err(TxnOpError::Cancel)?; let existing = fetch_item_for_update(tx, key_info, item) .await .map_err(TxnOpError::Storage)?; @@ -459,8 +419,9 @@ async fn execute_transact_write_op( return_values_on_ccf, .. } => { - transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; let idx_refs = index_key_refs(indexes); + let attr_defs = fresh_attr_defs(indexes, &key_info.attribute_definitions); + transact_op_key_checks(op, &idx_refs, attr_defs).map_err(TxnOpError::Cancel)?; let existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; @@ -486,18 +447,20 @@ async fn execute_transact_write_op( validation::validate_transact_update_item_size(&item, max_item_size_bytes).map_err( |e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())), )?; - // Secondary-index keys on the written item: a value computed from - // the existing item (an attribute reference, the fallback of - // if_not_exists) of the wrong type, reported with the service's - // computed-value wording, and a key left empty. - validation::validate_index_key_types_after_update( - &item, - &idx_refs, - super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), - ) - .map_err(|e| { - TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) - })?; + // Secondary-index keys on the written item: a wrong-typed value + // the statement wrote from an attribute reference or from + // if_not_exists, reported with the service's computed-value + // wording, and a key such a value leaves empty (an empty literal + // was found before any member ran). With the path absent the + // if_not_exists fallback is a literal from the request, which the + // service ranks with the values taken from the item. The type + // fault is a plain positional reason: the service leaves the + // other members' outcomes in place beside it, where a wrong type + // supplied in the request (the pre-pass) masks them. + validation::validate_index_key_types_after_update(&item, &idx_refs, attr_defs) + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) + })?; validation::validate_index_key_not_empty( &item, &idx_refs, @@ -591,3 +554,94 @@ fn eval_condition( } Ok(()) } + +#[cfg(test)] +mod token_tests { + //! The `ClientRequestToken` of a request cancelled after the key checks. + + use std::collections::BTreeMap; + + use extenddb_core::expression::{self, ExpressionMaps}; + use extenddb_core::types::{AttributeValue, Item, ReturnValuesOnConditionCheckFailure}; + use extenddb_storage::error::StorageError; + use extenddb_storage::{DataEngine, IdempotencyKey, TableEngine, TransactWriteOp}; + use serde_json::json; + + use crate::SqliteEngine; + + const ACCOUNT: &str = "123456789012"; + + /// A failed write of the cancelled token leaves the answer as it is: + /// nothing was applied, so the request answers its cancellation, not an + /// internal error. + #[tokio::test] + async fn a_failed_token_write_keeps_the_cancellation() { + let engine = SqliteEngine::new(":memory:", 1, "us-east-1", 409_600) + .await + .expect("engine"); + crate::migrations::apply_for_tests(&engine.pool).await; + sqlx::query( + "INSERT OR REPLACE INTO settings (key, value) VALUES ('control_plane_delay_seconds', '0')", + ) + .execute(&engine.pool) + .await + .expect("delay"); + sqlx::query("INSERT INTO accounts (account_id, account_name) VALUES (?, 'default')") + .bind(ACCOUNT) + .execute(&engine.pool) + .await + .expect("account"); + let input: extenddb_core::types::CreateTableInput = serde_json::from_value(json!({ + "TableName": "t_token", + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "AttributeDefinitions": [{"AttributeName": "pk", "AttributeType": "S"}], + "BillingMode": "PAY_PER_REQUEST" + })) + .expect("input"); + engine.create_table(ACCOUNT, input).await.expect("create"); + let key_info = engine + .table_key_info(ACCOUNT, "t_token") + .await + .expect("key info"); + let maps = ExpressionMaps::default(); + let seeded: Item = BTreeMap::from([("pk".to_owned(), AttributeValue::S("bp".to_owned()))]); + engine + .put_item(&key_info, seeded.clone(), false, None, &maps, None) + .await + .expect("seed bp"); + // Every write of a cancelled token fails from here on. + sqlx::query( + "CREATE TRIGGER fail_cancelled_tokens BEFORE INSERT ON idempotency_tokens \ + WHEN NEW.fingerprint LIKE 'cancelled:%' \ + BEGIN SELECT RAISE(ABORT, 'injected'); END", + ) + .execute(&engine.pool) + .await + .expect("trigger"); + + let condition = expression::parse_condition( + &expression::tokenize("attribute_not_exists(pk)").expect("tokenize"), + ) + .expect("parse"); + let ops = [TransactWriteOp::Put { + key_info: &key_info, + item: &seeded, + condition: Some(&condition), + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let key = IdempotencyKey { + account_id: ACCOUNT, + token: "tok-failed-write", + fingerprint: "fp", + }; + match engine.transact_write_items(&ops, Some(key)).await { + Err(StorageError::TransactionCanceled(reasons)) => { + assert_eq!(reasons.len(), 1); + assert_eq!(reasons[0].code, "ConditionalCheckFailed", "{reasons:?}"); + } + other => panic!("expected the cancellation, got {other:?}"), + } + } +} diff --git a/crates/storage-sqlite/src/data/tx_helpers.rs b/crates/storage-sqlite/src/data/tx_helpers.rs index 0ee6ba208..06c640610 100644 --- a/crates/storage-sqlite/src/data/tx_helpers.rs +++ b/crates/storage-sqlite/src/data/tx_helpers.rs @@ -15,9 +15,9 @@ use extenddb_core::types::{ AttributeValue, Item, StreamEventName, StreamRecord, StreamRecordData, StreamViewType, TableKeyInfo, item_size_bytes, }; -use extenddb_storage::StreamCapture; use extenddb_storage::error::StorageError; use extenddb_storage::util::{pk_to_text, sk_column, sk_info}; +use extenddb_storage::{StoredToken, StreamCapture, cancelled_token_fingerprint}; use super::{ bind_sk_execute, bind_sk_fetch_optional, bind_sk_only_execute, data_table_name, json_to_item, @@ -279,7 +279,9 @@ pub(super) async fn write_stream_record_in_tx( /// Check (and record) an idempotency token within a transaction. Tokens are /// valid for 10 minutes; the cutoff is computed in Rust as RFC 3339 so it -/// compares correctly against the stored RFC 3339 `created_at`. +/// compares correctly against the stored RFC 3339 `created_at`. A token the +/// same request left cancelled is taken again for this run; see +/// [`StoredToken::of`]. pub(super) async fn check_idempotency_token_in_tx( tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, account_id: &str, @@ -299,11 +301,11 @@ pub(super) async fn check_idempotency_token_in_tx( .map_err(|e| StorageError::Internal(e.to_string()))?; if let Some((stored_fp,)) = existing { - return if stored_fp == fingerprint { - Err(StorageError::IdempotentReplay) - } else { - Err(StorageError::IdempotentMismatch) - }; + match StoredToken::of(&stored_fp, fingerprint) { + StoredToken::Replay => return Err(StorageError::IdempotentReplay), + StoredToken::Mismatch => return Err(StorageError::IdempotentMismatch), + StoredToken::Rerun => {} + } } let now = format_timestamp(time::OffsetDateTime::now_utc()); @@ -323,6 +325,38 @@ pub(super) async fn check_idempotency_token_in_tx( Ok(()) } +/// Keep a token used after its request was cancelled past the request-level +/// key checks, once the request's transaction has rolled back: store it with +/// [`cancelled_token_fingerprint`] and the time now, so the token stays used +/// for 10 minutes from the last cancellation (measured 2026-11-05). A row +/// another request stored less than 10 minutes ago is kept. Runs under the +/// engine write lock, after the rollback released the database. +pub(super) async fn record_cancelled_token( + pool: &sqlx::SqlitePool, + account_id: &str, + token: &str, + fingerprint: &str, +) -> Result<(), StorageError> { + let now = time::OffsetDateTime::now_utc(); + let cutoff = format_timestamp(now - time::Duration::minutes(10)); + sqlx::query( + "INSERT INTO idempotency_tokens (account_id, token, fingerprint, created_at) \ + VALUES (?, ?, ?, ?) \ + ON CONFLICT (account_id, token) DO UPDATE SET fingerprint = excluded.fingerprint, \ + created_at = excluded.created_at WHERE idempotency_tokens.created_at <= ? \ + OR idempotency_tokens.fingerprint = excluded.fingerprint", + ) + .bind(account_id) + .bind(token) + .bind(cancelled_token_fingerprint(fingerprint)) + .bind(format_timestamp(now)) + .bind(&cutoff) + .execute(pool) + .await + .map_err(|e| StorageError::Internal(e.to_string()))?; + Ok(()) +} + #[cfg(test)] mod idempotency_tests { use super::check_idempotency_token_in_tx; @@ -381,6 +415,94 @@ mod idempotency_tests { // independent — not a replay and not a mismatch. check(&pool, "acctB", "tok", "fpX").await.unwrap(); } + + /// Set the stored `created_at` of `(account, token)` to `minutes` ago. + async fn age(pool: &SqlitePool, account: &str, token: &str, minutes: i64) { + let then = crate::sqlite_util::format_timestamp( + time::OffsetDateTime::now_utc() - time::Duration::minutes(minutes), + ); + sqlx::query( + "UPDATE idempotency_tokens SET created_at = ? WHERE account_id = ? AND token = ?", + ) + .bind(then) + .bind(account) + .bind(token) + .execute(pool) + .await + .unwrap(); + } + + async fn stored(pool: &SqlitePool, account: &str, token: &str) -> (String, String) { + sqlx::query_as( + "SELECT fingerprint, created_at FROM idempotency_tokens \ + WHERE account_id = ? AND token = ?", + ) + .bind(account) + .bind(token) + .fetch_one(pool) + .await + .unwrap() + } + + /// The service keeps a cancelled token used 9 minutes after the + /// cancellation and treats it as new 11 minutes after it (measured + /// 2026-11-05, Q1b and Q1a). + #[tokio::test] + async fn a_cancelled_token_is_used_for_ten_minutes() { + let pool = pool_with_schema().await; + super::record_cancelled_token(&pool, "acct", "tok", "fpX") + .await + .unwrap(); + age(&pool, "acct", "tok", 9).await; + assert!(matches!( + check(&pool, "acct", "tok", "fpY").await, + Err(StorageError::IdempotentMismatch) + )); + age(&pool, "acct", "tok", 11).await; + check(&pool, "acct", "tok", "fpY").await.unwrap(); + assert_eq!(stored(&pool, "acct", "tok").await.0, "fpY"); + } + + /// A cancellation of the same request sent again starts the window again + /// (measured 2026-11-05, Q1c: used 5 minutes after the second + /// cancellation, 11 minutes after the first). + #[tokio::test] + async fn a_cancelled_rerun_starts_the_window_again() { + let pool = pool_with_schema().await; + super::record_cancelled_token(&pool, "acct", "tok", "fpX") + .await + .unwrap(); + age(&pool, "acct", "tok", 6).await; + let (_, aged) = stored(&pool, "acct", "tok").await; + super::record_cancelled_token(&pool, "acct", "tok", "fpX") + .await + .unwrap(); + let (fingerprint, renewed) = stored(&pool, "acct", "tok").await; + assert_eq!(fingerprint, "cancelled:fpX"); + assert!(renewed > aged, "{renewed} is not later than {aged}"); + age(&pool, "acct", "tok", 5).await; + assert!(matches!( + check(&pool, "acct", "tok", "fpY").await, + Err(StorageError::IdempotentMismatch) + )); + } + + /// A token another request stored less than 10 minutes ago stays; one + /// stored longer ago is replaced. + #[tokio::test] + async fn a_cancellation_keeps_another_requests_live_token() { + let pool = pool_with_schema().await; + check(&pool, "acct", "tok", "fpY").await.unwrap(); + super::record_cancelled_token(&pool, "acct", "tok", "fpX") + .await + .unwrap(); + assert_eq!(stored(&pool, "acct", "tok").await.0, "fpY"); + age(&pool, "acct", "tok", 11).await; + super::record_cancelled_token(&pool, "acct", "tok", "fpX") + .await + .unwrap(); + assert_eq!(stored(&pool, "acct", "tok").await.0, "cancelled:fpX"); + } } #[cfg(test)] diff --git a/crates/storage-sqlite/src/data/update_item.rs b/crates/storage-sqlite/src/data/update_item.rs index 0eabdde21..a17b13158 100644 --- a/crates/storage-sqlite/src/data/update_item.rs +++ b/crates/storage-sqlite/src/data/update_item.rs @@ -44,20 +44,23 @@ impl SqliteEngine { // added GSI (UpdateTable holds the same lock) is not missed. let indexes = fetch_indexes_for_table(&key_info.table_id, &self.pool).await?; // Literal operands assigned to a secondary-index key, checked on the - // statement ahead of the condition as the service checks them. The - // engine ran this check against its cached key info, which does not list - // a GSI until the UpdateTable that adds it returns; a write that read the - // cache before then and takes the write lock after the build sees the GSI - // only in this read, and a literal of the wrong type for it gets the same - // per-index wording. + // statement ahead of the condition as the service checks them: the type + // first, then an empty value. The engine ran these checks against its + // cached key info, which does not list a GSI until the UpdateTable that + // adds it returns; a write that read the cache before then and takes the + // write lock after the build sees the GSI only in this read, and a + // literal for it gets the same answer. if !indexes.is_empty() { + let idx_refs = index_key_refs(&indexes); validation::validate_index_key_literal_operands( actions, - &index_key_refs(&indexes), + &idx_refs, super::index::fresh_attr_defs(&indexes, &key_info.attribute_definitions), maps, ) .map_err(|e| StorageError::Validation(e.to_string()))?; + validation::validate_index_key_empty_literals(actions, &idx_refs, maps) + .map_err(|e| StorageError::Validation(e.to_string()))?; } let mut tx = self diff --git a/crates/storage/src/error.rs b/crates/storage/src/error.rs index c936675a5..9b991e481 100755 --- a/crates/storage/src/error.rs +++ b/crates/storage/src/error.rs @@ -41,6 +41,12 @@ pub enum StorageError { IdempotentReplay, #[error("Idempotent parameter mismatch")] IdempotentMismatch, + /// Another send of the same `TransactWriteItems` request holds the + /// `ClientRequestToken` and has not finished. Maps to + /// `TransactionInProgressException`, which the service answers in this + /// case (measured 2026-11-05: two sends of one request at once). + #[error("Transaction from previous request is still in progress")] + TransactionInProgress, /// A single-item write raced an in-flight `TransactWriteItems` on /// the same item, and the backend was unable to serialize the two. /// Maps to `DynamoDbError::TransactionConflictException` at the diff --git a/crates/storage/src/lib.rs b/crates/storage/src/lib.rs index f5e744799..92ba2f937 100755 --- a/crates/storage/src/lib.rs +++ b/crates/storage/src/lib.rs @@ -40,7 +40,11 @@ pub use backend::{Backend, BackendAlreadySet, backend_name, set_backend, try_bac pub use backup_definition::{BackupRestoreSource, RestoreRequest, RestoreTableOverrides}; pub use snapshot::{SnapshotEngine, SnapshotInfo, TableSnapshot}; -pub use transact::{IdempotencyKey, TransactGetOp, TransactWriteOp}; +pub use transact::{ + IdempotencyKey, StoredToken, TransactGetOp, TransactWriteOp, cancelled_token_fingerprint, + transact_first_empty_literal, transact_key_check_pass, transact_op_empty_literal_checks, + transact_op_key_checks, +}; pub use server_components::{ BackendError, ServerComponents, ServerComponentsFactory, create_server_components, diff --git a/crates/storage/src/transact.rs b/crates/storage/src/transact.rs index e3b326eac..d88becb54 100755 --- a/crates/storage/src/transact.rs +++ b/crates/storage/src/transact.rs @@ -5,9 +5,14 @@ //! `DataEngine::transact_write_items`. use extenddb_core::expression::{Expr, ExpressionMaps, UpdateAction}; -use extenddb_core::types::{Item, ReturnValuesOnConditionCheckFailure, TableKeyInfo}; +use extenddb_core::types::{ + AttributeDefinition, CancellationReason, Item, ReturnValuesOnConditionCheckFailure, + TableKeyInfo, +}; +use extenddb_core::validation::{self, IndexKeyRef, SecondaryIndexEmptyContext}; use crate::StreamCapture; +use crate::error::StorageError; /// Idempotency scope for a `TransactWriteItems` request. /// @@ -15,6 +20,7 @@ use crate::StreamCapture; /// globally, so the token is stored and looked up under `(account_id, token)`. /// `fingerprint` distinguishes a genuine replay from a reused token carrying a /// different request, within that same account scope. +#[derive(Clone, Copy)] pub struct IdempotencyKey<'a> { /// Account that owns the token. Scopes the dedup keyspace. pub account_id: &'a str, @@ -24,6 +30,56 @@ pub struct IdempotencyKey<'a> { pub fingerprint: &'a str, } +/// What a request does with a `ClientRequestToken` already stored with the +/// fingerprint `stored`, see [`StoredToken::of`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum StoredToken { + /// The same request committed with this token: answer it as a replay. + Replay, + /// The same request was cancelled with this token after the + /// request-level key checks: run it again. + Rerun, + /// A different request holds the token. + Mismatch, +} + +impl StoredToken { + /// Classify the fingerprint `stored` with a token against the + /// fingerprint of the request that carries the token now. + /// + /// The service keeps a token used when its request is cancelled after the + /// request-level key checks (an empty index-key literal, an empty or + /// wrong-typed value written from the item, a failed condition): a + /// different request with the token then answers + /// `IdempotentParameterMismatchException` and the same request answers + /// what it answered before (measured 2026-11-01). The backends store + /// such a token with [`cancelled_token_fingerprint`] and run the same + /// request again when it returns. + #[must_use] + pub fn of(stored: &str, fingerprint: &str) -> Self { + if stored == fingerprint { + Self::Replay + } else if stored.strip_prefix(CANCELLED_TOKEN_PREFIX) == Some(fingerprint) { + Self::Rerun + } else { + Self::Mismatch + } + } +} + +/// Prefix of the fingerprint stored for a token whose request was cancelled. +/// A request fingerprint is hex, so no request fingerprint carries it. +const CANCELLED_TOKEN_PREFIX: &str = "cancelled:"; + +/// The fingerprint a backend stores for a token whose request with +/// `fingerprint` was cancelled after the request-level key checks, so the +/// token stays used; see [`StoredToken::of`]. A request cancelled by those +/// checks leaves its token unused. +#[must_use] +pub fn cancelled_token_fingerprint(fingerprint: &str) -> String { + format!("{CANCELLED_TOKEN_PREFIX}{fingerprint}") +} + /// A single get operation within a transactional read. pub struct TransactGetOp<'a> { /// Table metadata. @@ -95,3 +151,199 @@ pub enum TransactWriteOp<'a> { return_values_on_ccf: ReturnValuesOnConditionCheckFailure, }, } + +/// The key checks a transaction member can fail from the request alone, +/// before any row is read or locked: the table key's shape and types, and the +/// secondary-index keys the member supplies itself (a Put item's attributes, +/// an Update statement's literal operands). Faults carry the internal reasons +/// that [`CancellationReason::resolve_key_faults`] ranks: a wrong type as +/// [`CancellationReason::key_type_mismatch`], a Put item's empty index key as +/// [`CancellationReason::index_key_empty_abort`] with the Put wording. +/// +/// The backends run this check over every member before the +/// `ClientRequestToken` is checked, as the service does (measured +/// 2026-11-01): a fault here is answered even when the token belongs to a +/// different request, and leaves the token unused. +/// +/// `index_keys` and `index_attr_defs` are the table's secondary indexes and +/// the attribute definitions the backend checks their keys against. An +/// Update's empty literal is [`transact_op_empty_literal_checks`]; the values +/// an Update writes from the stored item are checked by the member's arm once +/// the statement is applied. +/// +/// # Errors +/// +/// Returns the member's first fault as its internal cancellation reason. +pub fn transact_op_key_checks( + op: &TransactWriteOp<'_>, + index_keys: &[IndexKeyRef<'_>], + index_attr_defs: &[AttributeDefinition], +) -> Result<(), CancellationReason> { + let type_fault = |e: extenddb_core::error::DynamoDbError| { + CancellationReason::key_type_mismatch(e.to_string()) + }; + match op { + TransactWriteOp::Put { key_info, item, .. } => { + validation::validate_item_keys( + item, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(type_fault)?; + validation::validate_index_key_types(item, index_keys, index_attr_defs) + .map_err(type_fault)?; + validation::validate_index_key_not_empty( + item, + index_keys, + SecondaryIndexEmptyContext::Item, + ) + .map_err(|e| CancellationReason::index_key_empty_abort(e.to_string())) + } + TransactWriteOp::Update { + key_info, + key, + actions, + maps, + .. + } => { + validation::validate_batch_key_only( + key, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(type_fault)?; + validation::validate_index_key_literal_operands( + actions, + index_keys, + index_attr_defs, + maps, + ) + .map_err(type_fault) + } + TransactWriteOp::Delete { key_info, key, .. } + | TransactWriteOp::ConditionCheck { key_info, key, .. } => { + validation::validate_batch_key_only( + key, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(type_fault) + } + } +} + +/// An Update member's empty index-key literal (`SET k = :v`, `:v` an empty +/// string or binary), carried as [`CancellationReason::index_key_empty_abort`] +/// with the update wording. The service reports it on the member's +/// statement, below every fault of [`transact_op_key_checks`] and ahead of +/// the member's condition, of an action that cannot apply and of anything the +/// statement writes from the item. An empty value written from the item or +/// from the fallback of `if_not_exists` is not checked here; the member's arm +/// finds it on the written item. See [`transact_first_empty_literal`] for how +/// it ranks against the other members. +/// +/// # Errors +/// +/// Returns the member's empty literal as its internal cancellation reason. +pub fn transact_op_empty_literal_checks( + op: &TransactWriteOp<'_>, + index_keys: &[IndexKeyRef<'_>], +) -> Result<(), CancellationReason> { + match op { + TransactWriteOp::Update { actions, maps, .. } => { + validation::validate_index_key_empty_literals(actions, index_keys, maps) + .map_err(|e| CancellationReason::index_key_empty_abort(e.to_string())) + } + TransactWriteOp::Put { .. } + | TransactWriteOp::Delete { .. } + | TransactWriteOp::ConditionCheck { .. } => Ok(()), + } +} + +/// The first member, in request order, that fails +/// [`transact_op_empty_literal_checks`], with its position and internal +/// reason; `member_checks` yields each member's result in request order. +/// +/// Across members the service reports the first empty index-key value in +/// request order, whether a member supplies it as a literal or writes it from +/// the item (measured 2026-11-01). So with an empty literal at position `p` +/// the backends run only the members before `p`, as they run every member +/// otherwise, put this reason at `p` and resolve the reasons with +/// [`CancellationReason::resolve_key_faults`]: an empty value an earlier +/// member writes from the item answers ahead of the literal, and no member +/// after `p` is read or locked. A member whose own condition fails writes +/// nothing, so its empty `if_not_exists` fallback is never reported. +pub fn transact_first_empty_literal( + member_checks: impl IntoIterator>, +) -> Option<(usize, CancellationReason)> { + member_checks + .into_iter() + .enumerate() + .find_map(|(i, check)| check.err().map(|reason| (i, reason))) +} + +/// The request-level answer of [`transact_op_key_checks`] over every member: +/// `member_checks` yields each member's result in request order. When every +/// member passes the request goes on; otherwise +/// [`CancellationReason::resolve_key_faults`] decides it from those reasons +/// alone, before the token is checked and before any member is applied. +/// +/// # Errors +/// +/// Returns `StorageError::TransactionCanceled` or `StorageError::Validation` +/// as the resolver decides when any member failed. +pub fn transact_key_check_pass( + member_checks: impl IntoIterator>, +) -> Result<(), StorageError> { + let mut any_fault = false; + let reasons: Vec = member_checks + .into_iter() + .map(|check| match check { + Ok(()) => CancellationReason::none(), + Err(reason) => { + any_fault = true; + reason + } + }) + .collect(); + if !any_fault { + return Ok(()); + } + Err(match CancellationReason::resolve_key_faults(reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_stored_token_is_a_replay_a_rerun_or_a_mismatch() { + let fp = "0123456789abcdef"; + assert_eq!(StoredToken::of(fp, fp), StoredToken::Replay); + assert_eq!( + StoredToken::of(&cancelled_token_fingerprint(fp), fp), + StoredToken::Rerun + ); + assert_eq!( + StoredToken::of("fedcba9876543210", fp), + StoredToken::Mismatch + ); + assert_eq!( + StoredToken::of(&cancelled_token_fingerprint("fedcba9876543210"), fp), + StoredToken::Mismatch + ); + } + + #[test] + fn the_first_empty_literal_in_request_order_is_returned() { + let empty = |m: &str| Err(CancellationReason::index_key_empty_abort(m)); + assert!(transact_first_empty_literal([Ok(()), Ok(())]).is_none()); + let (p, reason) = + transact_first_empty_literal([Ok(()), empty("second"), empty("third")]).unwrap(); + assert_eq!(p, 1); + assert_eq!(reason.message.as_deref(), Some("second")); + } +} diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 0686d8445..e94bdd2f8 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -98,7 +98,7 @@ from a request sent to that service or backend. | Multi-part base table keys | Not supported | Preview extension (opt-in via `enable_multipart_keys` setting). Standard single/composite keys work identically. | | GSI throughput on a `PROVISIONED` table | Every GSI needs `ProvisionedThroughput` of at least 1 read and 1 write unit | A GSI `ProvisionedThroughput` below 1 (including 0/0) is `ValidationException` on `CreateTable` of a `PROVISIONED` table and on `UpdateTable`'s GSI `Create` ("ReadCapacityUnits and WriteCapacityUnits must both be greater than or equal to 1 for index: "; the service's text was not captured). A GSI with no `ProvisionedThroughput` is still accepted on a `PROVISIONED` table, and `UpdateTable` to `PROVISIONED` leaves an on-demand table's GSIs without one; see the `BillingModeOverride` row under Backup and Restore for how a backup of such a table restores | | Projected attributes across all indexes | At most 100 | Enforced on `CreateTable` (and so on `RestoreTableFromBackup`, which validates its target the same way) and on `UpdateTable`'s GSI `Create`: every `NonKeyAttributes` entry of every GSI and LSI is counted, so an attribute projected into two indexes counts twice. Over 100 is `ValidationException` ("Number of projected attributes in all indexes exceeds limit of 100, projected: "; the service's text was not captured) | -| Index key validation on `UpdateItem` and transaction writes | A secondary-index key attribute must carry its declared scalar type and may not be empty. On `UpdateItem` a literal operand of the wrong type (`SET k = :v`, `ADD`, `DELETE`, `list_append` of literals) is refused on the statement, ahead of the `ConditionExpression`, naming the attribute and the index; a value computed from the item (an attribute reference, the fallback of `if_not_exists`) is refused after the condition passes with the message "The update expression attempted to update the secondary index key to unsupported type"; a key left empty is refused after the condition passes with the update-expression wording. In `TransactWriteItems` a wrong-typed key (table or index) on any member cancels the transaction with the first such member's `ValidationError` in position and `None` elsewhere; otherwise an empty index key on any member is a top-level `ValidationException`; otherwise members report their own conditions. On a `Put` member both checks come ahead of the member's condition. (Measured on the service.) | Matches on every backend. Two known shared differences: an empty index key supplied through the legacy `AttributeUpdates` parameter is reported with the update-expression wording where the service uses the `PutItem` wording naming the index; and a request whose own value exceeds 400 KB is refused for size by the service before any index-key check, where ExtendDB reports the index-key fault first. | +| Index key validation on `UpdateItem` and transaction writes | A secondary-index key attribute must carry its declared scalar type and may not be empty. On `UpdateItem` a literal operand is checked on the statement, ahead of the `ConditionExpression` and of an action that cannot apply: a value of the wrong type (`SET k = :v`, `ADD`, `DELETE`, and `list_append` or arithmetic, which yield a list and a number whatever their operands read) is refused naming the attribute and the index, and then an empty string or binary `SET k = :v` is refused with the update-expression wording. A value written from an attribute reference or from `if_not_exists` is checked after the condition passes: a wrong type with "The update expression attempted to update the secondary index key to unsupported type", an empty value with the update-expression wording. In `TransactWriteItems` the keys supplied in the request are checked first, before the `ClientRequestToken` and before any member is applied: a wrong-typed table key, a `Put` item's wrong-typed or empty index key, and an `Update`'s wrong-typed literal operand. The first such fault in request order decides. A wrong type cancels the transaction with that member's `ValidationError` and `None` elsewhere, an empty key is a top-level `ValidationException`, and the token stays unused. Otherwise the first empty index-key value in request order is a top-level `ValidationException`, whether an `Update` supplies it as a literal or writes it from the item. Within one statement the literal comes first, and a member whose condition fails writes nothing. Otherwise members report their own outcomes, and a wrong type an `Update` writes from the item is a `ValidationError` at that member beside the others (`[ValidationError, ConditionalCheckFailed]`). Every cancellation after the first check keeps the token used for 10 minutes from the last cancellation: a failed condition on any member, an empty literal, an empty or wrong-typed value written from the item, an item past 400 KB, an action that cannot apply and `TransactionConflict`. Another request with that token answers `IdempotentParameterMismatchException` and is not applied, and the same request runs again. A send that meets another send of the same request still running answers `TransactionInProgressException`, or waits and answers as that send did. On a `Put` member both checks come ahead of the member's condition. (Measured on the service, 2026-10-30, 2026-10-31, 2026-11-01 and 2026-11-05.) | Matches on every backend, with these shared differences. Arithmetic on an index key is refused with `Actual: N` where the service lists a set of number types (#702). A wrong-typed table key on `GetItem`, `DeleteItem`, `UpdateItem` and `TransactGetItems` answers `Type mismatch for key expected: actual: ` where the service answers "The provided key element does not match the schema" (#701). An empty index key supplied through the legacy `AttributeUpdates` parameter is reported with the update-expression wording where the service uses the `PutItem` wording naming the index (#423). `IdempotentParameterMismatchException` carries the message "The request uses the same client token as a previous, but different, request." where the service answers "Specified idempotent token was used with different request parameters within the idempotency window" (#760). The token's fingerprint covers `TransactItems` only, so the same items sent again with another `ReturnConsumedCapacity` answer as a replay after a commit and run again after a cancellation, where the service answers the mismatch (#514). PostgreSQL differs in one more case. Two sends of one request can arrive with one token at the same moment. The second send then waits on the token row of the first. When the first send commits, the second reads the token store as it was before that commit. It finds no token and applies the request again, where the service applies it once (#851). On MongoDB the token can be free from 540 seconds after its last write, when the TTL monitor deletes its document. The service keeps it used at 570 seconds. | | `Query` on a GSI with several RANGE attributes (MongoDB and Cassandra backends) | Orders results over every RANGE attribute in key-schema order and serves a key condition on each of them (equality on every one before the last, any operator on the last) | SQLite and PostgreSQL match the service. The MongoDB and Cassandra backends filter the first RANGE attribute of such an index only: a key condition on a later RANGE attribute is refused with `ValidationException` naming the attribute and the backend. MongoDB orders results by the first RANGE attribute then the base table key. Cassandra orders a page over every RANGE attribute then the base table key, and refuses a `Query` that carries an `ExclusiveStartKey` on such an index with `ValidationException` (the cursor compares the first RANGE attribute only). Every refusal the service gives for the condition's shape (one condition per key, condition count, missing HASH attribute, RANGE gap, non-key attribute, value type) is the same on every backend. | ## Backup and Restore diff --git a/tests/test_index_key_validation_on_update.py b/tests/test_index_key_validation_on_update.py index 5271c290b..a565ab531 100644 --- a/tests/test_index_key_validation_on_update.py +++ b/tests/test_index_key_validation_on_update.py @@ -6,19 +6,29 @@ - An UpdateItem whose statement leaves a secondary-index key empty (empty string or empty binary) or wrong-typed is refused with ValidationException. The empty case carries the update-expression wording; the type case carries - the Type mismatch wording naming the index. The type case is reported ahead - of a ConditionExpression; the empty case after the condition passes. + the Type mismatch wording naming the index. A literal is checked on the + statement, the type first and then an empty value, ahead of a + ConditionExpression and of an action that cannot apply; list_append and + arithmetic count as literals of their result type. A value written from an + attribute reference or from if_not_exists is checked after the condition + passes, the empty case included. - In TransactWriteItems a wrong-typed index key on a Put item or on the post-update item of an Update cancels the transaction with a positional ValidationError reason. An index key left empty aborts the whole transaction with a top-level ValidationException and no CancellationReasons. On a Put member both are reported ahead of the member's condition; on an Update - member the type case is reported ahead of the condition and the empty case - after it. -- Across members: a wrong-typed index key on any member wins (the transaction - is cancelled, None in the other positions); otherwise an empty index key on - any member wins over failed conditions and unapplicable statements on the - others. + member a literal of either kind is reported ahead of the condition and a + value written from the item after it. +- Across members: a wrong-typed index key supplied in the request (a Put + item's attribute, an Update's literal operand) or a Put item's empty index + key decides, the first such fault in request order (a wrong type cancels the + transaction with None in the other positions); otherwise an Update's empty + literal on any member wins over failed conditions, unapplicable statements + and computed wrong types on the others; otherwise each member reports its + own outcome, an empty value an Update wrote from the item aborting the + request, and a wrong type an Update computes from the stored item is a + ValidationError at that member beside them. The member orders of each case + are in tests/test_transact_index_key_cross_member.py. - if_not_exists(k, :empty) is judged on what the statement writes: accepted when k is present, refused when k is absent. - Nothing is written in any refused case. @@ -334,9 +344,10 @@ def test_wrong_type_reason_is_positional(self, dynamodb_client, indexed_table): class TestOrderAgainstTheCondition: """Where the index-key checks sit relative to a ConditionExpression, as measured. - A wrong-typed index key is reported ahead of the condition. A failed - condition is reported ahead of an empty index key and ahead of a statement - that cannot be applied (arithmetic on a missing attribute). + A wrong-typed index-key literal is reported ahead of the condition, and so + is an empty index-key literal (measured 2026-10-31). A failed condition is + reported ahead of a statement that cannot be applied (arithmetic on a + missing attribute). """ def test_update_item_type_mismatch_beats_a_failing_condition(self, dynamodb_client, indexed_table): @@ -354,7 +365,7 @@ def test_update_item_type_mismatch_beats_a_failing_condition(self, dynamodb_clie attr="bidx", expected="B", actual="N", index="bidx-index" ) - def test_update_item_failing_condition_beats_an_empty_key(self, dynamodb_client, indexed_table): + def test_update_item_empty_literal_beats_a_failing_condition(self, dynamodb_client, indexed_table): key = _seed(dynamodb_client, indexed_table, "o-empty") with pytest.raises(ClientError) as exc_info: dynamodb_client.update_item( @@ -364,7 +375,9 @@ def test_update_item_failing_condition_beats_an_empty_key(self, dynamodb_client, ConditionExpression="attribute_not_exists(pk)", ExpressionAttributeValues={":v": {"S": ""}}, ) - assert _err(exc_info)["Code"] == "ConditionalCheckFailedException" + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="string") + assert "sidx" not in _item(dynamodb_client, indexed_table, key) def test_update_item_failing_condition_beats_a_statement_that_cannot_apply(self, dynamodb_client, indexed_table): key = _key("o-apply-missing") @@ -401,7 +414,7 @@ def test_transact_update_type_mismatch_beats_a_failing_condition(self, dynamodb_ attr="bidx", expected="B", actual="N", index="bidx-index" ) - def test_transact_update_failing_condition_beats_an_empty_key(self, dynamodb_client, indexed_table): + def test_transact_update_empty_literal_beats_its_failing_condition(self, dynamodb_client, indexed_table): key = _seed(dynamodb_client, indexed_table, "xo-empty") with pytest.raises(ClientError) as exc_info: dynamodb_client.transact_write_items( @@ -417,9 +430,10 @@ def test_transact_update_failing_condition_beats_an_empty_key(self, dynamodb_cli } ] ) - resp = exc_info.value.response - assert resp["Error"]["Code"] == "TransactionCanceledException" - assert [r["Code"] for r in resp["CancellationReasons"]] == ["ConditionalCheckFailed"] + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="string") + assert "CancellationReasons" not in exc_info.value.response + assert "sidx" not in _item(dynamodb_client, indexed_table, key) def test_transact_update_failing_condition_beats_a_statement_that_cannot_apply(self, dynamodb_client, indexed_table): with pytest.raises(ClientError) as exc_info: @@ -650,16 +664,19 @@ def test_two_wrong_types_report_only_the_first(self, dynamodb_client, indexed_ta attr="bidx", expected="B", actual="N", index="bidx-index" ) - def test_an_empty_key_whose_own_condition_fails_is_a_plain_cancellation(self, dynamodb_client, indexed_table): - _seed(dynamodb_client, indexed_table, "cc-a") + def test_an_empty_literal_beats_its_own_failing_condition_beside_a_plain_member( + self, dynamodb_client, indexed_table + ): + a = _seed(dynamodb_client, indexed_table, "cc-a") _seed(dynamodb_client, indexed_table, "cc-b") empty_failing = _upd(indexed_table, "cc-a", "SET sidx = :v", {":v": {"S": ""}}, "attribute_not_exists(pk)") plain = _upd(indexed_table, "cc-b", "SET v = :v", {":v": {"N": "2"}}) with pytest.raises(ClientError) as exc_info: dynamodb_client.transact_write_items(TransactItems=[empty_failing, plain]) - resp = exc_info.value.response - assert resp["Error"]["Code"] == "TransactionCanceledException" - assert [r["Code"] for r in resp["CancellationReasons"]] == ["ConditionalCheckFailed", "None"] + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="string") + assert "CancellationReasons" not in exc_info.value.response + assert "sidx" not in _item(dynamodb_client, indexed_table, a) assert _item(dynamodb_client, indexed_table, _key("cc-b"))["v"] == {"N": "1"} @@ -813,7 +830,7 @@ def test_a_literal_fault_beats_an_invalid_document_path(self, dynamodb_client, i assert _err(exc_info)["Code"] == "ValidationException" assert _err(exc_info)["Message"] == TYPE_WORDING.format(attr="bidx", expected="B", actual="N", index="bidx-index") - def test_an_empty_literal_does_not_beat_an_action_that_cannot_apply(self, dynamodb_client, indexed_table): + def test_an_empty_literal_beats_an_action_that_cannot_apply(self, dynamodb_client, indexed_table): with pytest.raises(ClientError) as exc_info: dynamodb_client.update_item( TableName=indexed_table, Key=_key("lc-empty-apply"), @@ -821,7 +838,8 @@ def test_an_empty_literal_does_not_beat_an_action_that_cannot_apply(self, dynamo ExpressionAttributeValues={":one": {"N": "1"}, ":e": {"S": ""}}, ) assert _err(exc_info)["Code"] == "ValidationException" - assert _err(exc_info)["Message"] == "The provided expression refers to an attribute that does not exist in the item" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="string") + assert _item(dynamodb_client, indexed_table, _key("lc-empty-apply")) is None def test_transacted_computed_value_is_a_cancellation_with_the_computed_wording(self, dynamodb_client, indexed_table): key = _key("lc-txn-ref") diff --git a/tests/test_transact_index_key_cross_member.py b/tests/test_transact_index_key_cross_member.py new file mode 100644 index 000000000..5c6f7b186 --- /dev/null +++ b/tests/test_transact_index_key_cross_member.py @@ -0,0 +1,476 @@ +"""TransactWriteItems with a bad secondary-index key on one member and another +fault on the other, in both member orders, as measured on DynamoDB (us-east-1, +2026-10-30, two tables, every request sent twice with identical answers). + +Table: pk S; GSI gsi_s on sidx S; GSI gsi_b on bidx B. Items: a = {pk: a, n: N 5}, +bp = {pk: bp}; b0 and c are absent. Three kinds of index-key fault appear: + +- a wrong type the Update computes from the stored item (SET sidx = n): a + ValidationError at that member with the computed-value wording, and every + other member keeps its own outcome (A2: [ValidationError, + ConditionalCheckFailed]); +- a wrong-typed value supplied in the request, in a Put item or as an Update's + literal operand: ValidationError at that member and None at every other + member, ahead of a failed condition (E1) and ahead of a computed wrong type + (F1, F2); +- an empty value supplied in the request, in a Put item or as a literal + operand: a top-level ValidationException with no CancellationReasons, in + either member order, ahead of a computed wrong type (B1, C1) and ahead of a + failed condition (D1). + +Nothing is written in any case. S1 to S5 are the single-request controls for +the same values. + +Rows G to J were measured on 2026-10-31 (us-east-1, one table of the same +shape, 337 requests, every request sent twice with identical answers), with +two more items: r = {pk: r, n: N 5, l: L [S x], e: S "", s: S x} and d0 +absent. They put the empty and wrong-typed values supplied in the request +against each other: + +- a Put item's empty index key against a wrong type supplied in the request + (a literal, list_append of literals, ADD, DELETE, arithmetic of literals, a + wrong-typed table key): the first in request order decides (G rows); +- a Put item's empty index key against an Update member's empty value, a + literal or one the statement writes from the stored item or from + if_not_exists: the Put item's message, in either order (H rows); +- an Update member's empty literal ahead of a wrong-typed literal on another + member: the wrong type (J1 to J3); an empty value the statement writes + from the stored item against a failed condition: the empty key (J4). + +The same measurement typed list_append by its result: SET sidx = +list_append(l, :l), which reads the item, is a wrong type supplied in the +request beside every partner (L rows) and ahead of the condition on +UpdateItem (la1, la2). A third measurement on 2026-10-31 (15 requests, each +sent twice) found an Update's empty index-key literal reported on the +statement, ahead of the member's own failing condition and of a wrong type +computed from the item in the same statement, on UpdateItem (s1 to s9) and +in TransactWriteItems (t1 to t6); between two such members the first in +request order decides (K1, K1r). The rows with the list_append member second +(L4r, L9r, L10r) answer the first member's fault. + +A fourth measurement on 2026-11-01 (each request sent twice) put an Update's +empty literal against an empty value another member writes from the item: +the first empty value in request order decides, literal or not (U3a, U3b, +U3c); within one statement the literal comes first (U3f); and a member whose +own condition fails writes nothing, so its empty if_not_exists fallback is +never reported (U1a to U1c). Two empty literals of different kinds in one +statement had no fixed order on the service, so no row here has that shape. + +Run with EXTENDDB_TEST_ENDPOINT unset to assert the same against the service. +""" + +from __future__ import annotations + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +COMPUTED_WORDING = "The update expression attempted to update the secondary index key to unsupported type" +TYPE_WORDING = ( + "One or more parameter values were invalid: Type mismatch for Index Key sidx " + "Expected: S Actual: N IndexName: gsi_s" +) +EMPTY_UPDATE_WORDING = ( + "One or more parameter values are not valid. The update expression attempted to " + "update a secondary index key to a value that is not supported. The AttributeValue " + "for a key attribute cannot contain an empty string value." +) +EMPTY_ITEM_WORDING = ( + "One or more parameter values are not valid. A value specified for a secondary index " + "key is not supported. The AttributeValue for a key attribute cannot contain an empty " + "string value. IndexName: gsi_s, IndexKey: sidx" +) +TYPE_WORDING_L = ( + "One or more parameter values were invalid: Type mismatch for Index Key sidx " + "Expected: S Actual: L IndexName: gsi_s" +) +TYPE_WORDING_SS = ( + "One or more parameter values were invalid: Type mismatch for Index Key sidx " + "Expected: S Actual: SS IndexName: gsi_s" +) +KEY_SCHEMA_WORDING = "The provided key element does not match the schema" +EMPTY_UPDATE_WORDING_B = ( + "One or more parameter values are not valid. The update expression attempted to " + "update a secondary index key to a value that is not supported. The AttributeValue " + "for a key attribute cannot contain an empty binary value." +) +CANCELLED = "Transaction cancelled, please refer cancellation reasons for specific reasons" +CCF_WORDING = "The conditional request failed" + + +@pytest.fixture(scope="module") +def table(dynamodb_client): + """The measured table: pk S, GSI gsi_s on sidx S, GSI gsi_b on bidx B.""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "sidx", "AttributeType": "S"}, + {"AttributeName": "bidx", "AttributeType": "B"}, + ], + key_schema=[{"AttributeName": "pk", "KeyType": "HASH"}], + GlobalSecondaryIndexes=[ + { + "IndexName": "gsi_s", + "KeySchema": [{"AttributeName": "sidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + }, + { + "IndexName": "gsi_b", + "KeySchema": [{"AttributeName": "bidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + }, + ], + ) as name: + yield name + + +ITEM_A = {"pk": {"S": "a"}, "n": {"N": "5"}} +ITEM_BP = {"pk": {"S": "bp"}} +ITEM_R = {"pk": {"S": "r"}, "n": {"N": "5"}, "l": {"L": [{"S": "x"}]}, "e": {"S": ""}, "s": {"S": "x"}} + + +@pytest.fixture(autouse=True) +def seeded(dynamodb_client, table): + """Items a, bp and r present, b0, c and d0 absent, before every case.""" + for item in (ITEM_A, ITEM_BP, ITEM_R): + dynamodb_client.put_item(TableName=table, Item=item) + for pk in ("b0", "c", "d0"): + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": pk}}) + + +def _update( + table: str, + key: str | dict, + expr: str, + names: dict, + values: dict | None = None, + cond: str | None = None, +) -> dict: + member = { + "TableName": table, + "Key": {"pk": key if isinstance(key, dict) else {"S": key}}, + "UpdateExpression": expr, + "ExpressionAttributeNames": names, + } + if values: + member["ExpressionAttributeValues"] = values + if cond: + member["ConditionExpression"] = cond + return {"Update": member} + + +def _put(table: str, item: dict) -> dict: + return {"Put": {"TableName": table, "Item": item}} + + +def members(table: str) -> dict: + """The member shapes of the measurement, by name.""" + return { + # Update a SET sidx = n: sidx (S) gets the stored N 5. + "set_sidx_n": _update(table, "a", "SET #s = #n", {"#s": "sidx", "#n": "n"}), + # Update a SET bidx = n: bidx (B) gets the stored N 5. + "set_bidx_n": _update(table, "a", "SET #b = #n", {"#b": "bidx", "#n": "n"}), + # Update b0 (absent) with attribute_not_exists(pk): the condition holds. + "cond_absent": _update( + table, "b0", "SET #v = :v", {"#v": "v", "#p": "pk"}, {":v": {"S": "x"}}, "attribute_not_exists(#p)" + ), + # Update bp (present) with attribute_not_exists(pk): the condition fails. + "cond_present": _update( + table, "bp", "SET #v = :v", {"#v": "v", "#p": "pk"}, {":v": {"S": "x"}}, "attribute_not_exists(#p)" + ), + # Update b0 SET sidx = :e, :e = S "": an empty literal on an index key. + "set_sidx_empty": _update(table, "b0", "SET #s = :e", {"#s": "sidx"}, {":e": {"S": ""}}), + # Put {pk: c, sidx: S ""}: an empty index key in the item. + "put_empty": _put(table, {"pk": {"S": "c"}, "sidx": {"S": ""}}), + # Update a SET sidx = :n, :n = N 1: a wrong-typed literal on an index key. + "set_sidx_lit_n": _update(table, "a", "SET #s = :n", {"#s": "sidx"}, {":n": {"N": "1"}}), + # Update b0 SET sidx = :n, :n = N 1: the same on an absent item. + "set_b0_lit_n": _update(table, "b0", "SET #s = :n", {"#s": "sidx"}, {":n": {"N": "1"}}), + # Put {pk: c, sidx: N 1}: a wrong-typed index key in the item. + "put_wrong": _put(table, {"pk": {"S": "c"}, "sidx": {"N": "1"}}), + # Update d0 SET sidx = :n, :n = N 1: a wrong-typed literal on an absent item. + "set_d0_lit_n": _update(table, "d0", "SET #s = :n", {"#s": "sidx"}, {":n": {"N": "1"}}), + # Update r SET sidx = list_append(:l, :l): a list built from literals. + "set_sidx_list_lit": _update( + table, "r", "SET #s = list_append(:l, :l)", {"#s": "sidx"}, {":l": {"L": [{"S": "y"}]}} + ), + # Update r ADD sidx :one: a number added onto a string key. + "add_sidx_one": _update(table, "r", "ADD #s :one", {"#s": "sidx"}, {":one": {"N": "1"}}), + # Update r DELETE sidx :ss: a set removed from a string key. + "delete_sidx_ss": _update(table, "r", "DELETE #s :ss", {"#s": "sidx"}, {":ss": {"SS": ["a"]}}), + # Update r SET sidx = :one + :one: arithmetic of literals. + "set_sidx_sum_lit": _update(table, "r", "SET #s = :one + :one", {"#s": "sidx"}, {":one": {"N": "1"}}), + # Update with Key {pk: N 1}: a wrong-typed table key. + "bad_table_key": _update(table, {"N": "1"}, "SET #v = :v", {"#v": "v"}, {":v": {"S": "x"}}), + # Update b0 SET bidx = :e, :e = B "": an empty binary literal on an index key. + "set_bidx_empty": _update(table, "b0", "SET #b = :e", {"#b": "bidx"}, {":e": {"B": b""}}), + # Update r SET sidx = e: the stored empty string written to an index key. + "set_sidx_stored_empty": _update(table, "r", "SET #s = #e", {"#s": "sidx", "#e": "e"}), + # Update r SET sidx = if_not_exists(nope, :e): the empty fallback written. + "set_sidx_fallback_empty": _update( + table, "r", "SET #s = if_not_exists(#x, :e)", {"#s": "sidx", "#x": "nope"}, {":e": {"S": ""}} + ), + # Update r SET sidx = list_append(l, :l): a list built from the stored l. + "set_sidx_list_ref": _update( + table, "r", "SET #s = list_append(#l, :l)", {"#s": "sidx", "#l": "l"}, {":l": {"L": [{"S": "y"}]}} + ), + # Update b0 SET v = v + :one: arithmetic on an attribute b0 lacks. + "cannot_apply": _update(table, "b0", "SET #v = #v + :one", {"#v": "v"}, {":one": {"N": "1"}}), + # ConditionCheck bp with attribute_not_exists(pk): the condition fails. + "check_present": { + "ConditionCheck": { + "TableName": table, + "Key": {"pk": {"S": "bp"}}, + "ConditionExpression": "attribute_not_exists(#p)", + "ExpressionAttributeNames": {"#p": "pk"}, + } + }, + # Delete bp with attribute_not_exists(pk): the condition fails. + "delete_present": { + "Delete": { + "TableName": table, + "Key": {"pk": {"S": "bp"}}, + "ConditionExpression": "attribute_not_exists(#p)", + "ExpressionAttributeNames": {"#p": "pk"}, + } + }, + # Update r SET sidx = n, bidx = :eb: a computed wrong type and an empty + # binary literal in one statement. + "set_sidx_n_bidx_empty": _update( + table, "r", "SET #s = #n, #b = :eb", {"#s": "sidx", "#n": "n", "#b": "bidx"}, {":eb": {"B": b""}} + ), + # The 2026-10-31 statement rows on r, each with the failing condition + # attribute_not_exists(pk) where named so, and a plain Update of a. + "r_sidx_empty_failing": _update( + table, "r", "SET #s = :e", {"#s": "sidx", "#p": "pk"}, {":e": {"S": ""}}, "attribute_not_exists(#p)" + ), + "r_bidx_empty_failing": _update( + table, "r", "SET #b = :eb", {"#b": "bidx", "#p": "pk"}, {":eb": {"B": b""}}, "attribute_not_exists(#p)" + ), + "r_sidx_n_bidx_empty_failing": _update( + table, + "r", + "SET #s = #n, #b = :eb", + {"#s": "sidx", "#n": "n", "#b": "bidx", "#p": "pk"}, + {":eb": {"B": b""}}, + "attribute_not_exists(#p)", + ), + "r_sidx_empty_bidx_pk": _update( + table, "r", "SET #s = :e, #b = #p", {"#s": "sidx", "#b": "bidx", "#p": "pk"}, {":e": {"S": ""}} + ), + "plain_a": _update(table, "a", "SET #v = :v", {"#v": "v"}, {":v": {"S": "x"}}), + # The 2026-11-01 rows: Update r SET sidx = e, bidx = :eb, the stored + # empty string and an empty binary literal in one statement; and the + # empty if_not_exists fallback with the failing condition. + "set_sidx_e_bidx_empty": _update( + table, "r", "SET #s = #e, #b = :eb", {"#s": "sidx", "#e": "e", "#b": "bidx"}, {":eb": {"B": b""}} + ), + "r_fallback_empty_failing": _update( + table, + "r", + "SET #s = if_not_exists(#x, :e)", + {"#s": "sidx", "#x": "nope", "#p": "pk"}, + {":e": {"S": ""}}, + "attribute_not_exists(#p)", + ), + } + + +# Cancellation reasons by position, as (Code, Message). +VE_COMPUTED = ("ValidationError", COMPUTED_WORDING) +VE_TYPE = ("ValidationError", TYPE_WORDING) +VE_TYPE_L = ("ValidationError", TYPE_WORDING_L) +CCF = ("ConditionalCheckFailed", CCF_WORDING) +NONE = ("None", None) + +# Each row: id, member names in request order, expected answer. A tuple of +# reasons is a TransactionCanceledException with those CancellationReasons; a +# string is a top-level ValidationException with that message. +ROWS = [ + ("A1", ["set_sidx_n", "cond_absent"], (VE_COMPUTED, NONE)), + ("A2", ["set_sidx_n", "cond_present"], (VE_COMPUTED, CCF)), + ("A2r", ["cond_present", "set_sidx_n"], (CCF, VE_COMPUTED)), + ("B1", ["set_sidx_n", "set_sidx_empty"], EMPTY_UPDATE_WORDING), + ("B1r", ["set_sidx_empty", "set_sidx_n"], EMPTY_UPDATE_WORDING), + ("C1", ["put_empty", "set_bidx_n"], EMPTY_ITEM_WORDING), + ("C1r", ["set_bidx_n", "put_empty"], EMPTY_ITEM_WORDING), + ("D1", ["cond_present", "set_sidx_empty"], EMPTY_UPDATE_WORDING), + ("E1", ["set_sidx_lit_n", "cond_present"], (VE_TYPE, NONE)), + ("E1r", ["cond_present", "set_sidx_lit_n"], (NONE, VE_TYPE)), + ("E2", ["put_wrong", "cond_present"], (VE_TYPE, NONE)), + ("E2r", ["cond_present", "put_wrong"], (NONE, VE_TYPE)), + ("F1", ["set_sidx_n", "put_wrong"], (NONE, VE_TYPE)), + ("F1r", ["put_wrong", "set_sidx_n"], (VE_TYPE, NONE)), + ("F2", ["set_sidx_n", "set_b0_lit_n"], (NONE, VE_TYPE)), + ("F2r", ["set_b0_lit_n", "set_sidx_n"], (VE_TYPE, NONE)), + # 2026-10-31. A Put item's empty index key against a wrong type supplied + # in the request: the first in request order decides. + ("G1", ["put_empty", "set_d0_lit_n"], EMPTY_ITEM_WORDING), + ("G1r", ["set_d0_lit_n", "put_empty"], (VE_TYPE, NONE)), + ("G2", ["put_empty", "set_sidx_list_lit"], EMPTY_ITEM_WORDING), + ("G2r", ["set_sidx_list_lit", "put_empty"], (("ValidationError", TYPE_WORDING_L), NONE)), + ("G3", ["put_empty", "add_sidx_one"], EMPTY_ITEM_WORDING), + ("G3r", ["add_sidx_one", "put_empty"], (VE_TYPE, NONE)), + ("G4", ["put_empty", "delete_sidx_ss"], EMPTY_ITEM_WORDING), + ("G5", ["put_empty", "set_sidx_sum_lit"], EMPTY_ITEM_WORDING), + ("G6", ["put_empty", "bad_table_key"], EMPTY_ITEM_WORDING), + ("G6r", ["bad_table_key", "put_empty"], (("ValidationError", KEY_SCHEMA_WORDING), NONE)), + # A Put item's empty index key against an Update member's empty value: + # the Put item's message in either order. + ("H1", ["set_sidx_empty", "put_empty"], EMPTY_ITEM_WORDING), + ("H1r", ["put_empty", "set_sidx_empty"], EMPTY_ITEM_WORDING), + ("H2", ["set_bidx_empty", "put_empty"], EMPTY_ITEM_WORDING), + ("H3", ["set_sidx_stored_empty", "put_empty"], EMPTY_ITEM_WORDING), + ("H4", ["set_sidx_fallback_empty", "put_empty"], EMPTY_ITEM_WORDING), + # An Update member's empty literal against a wrong-typed literal + # elsewhere: the wrong type. A written empty value against a failed + # condition: the empty key. + ("J1", ["set_sidx_empty", "set_sidx_list_lit"], (NONE, ("ValidationError", TYPE_WORDING_L))), + ("J2", ["set_sidx_empty", "add_sidx_one"], (NONE, VE_TYPE)), + ("J3", ["set_sidx_empty", "delete_sidx_ss"], (NONE, ("ValidationError", TYPE_WORDING_SS))), + ("J4", ["set_sidx_stored_empty", "cond_present"], EMPTY_UPDATE_WORDING), + # list_append that reads the item is typed by its result, a list, and + # ranks as a wrong type supplied in the request beside every partner. + ("L1", ["set_sidx_list_ref"], (VE_TYPE_L,)), + ("L2", ["set_sidx_list_ref", "cond_present"], (VE_TYPE_L, NONE)), + ("L2r", ["cond_present", "set_sidx_list_ref"], (NONE, VE_TYPE_L)), + ("L3", ["set_sidx_list_ref", "set_sidx_empty"], (VE_TYPE_L, NONE)), + ("L3r", ["set_sidx_empty", "set_sidx_list_ref"], (NONE, VE_TYPE_L)), + ("L4", ["set_sidx_list_ref", "set_d0_lit_n"], (VE_TYPE_L, NONE)), + ("L4r", ["set_d0_lit_n", "set_sidx_list_ref"], (VE_TYPE, NONE)), + ("L5", ["set_sidx_list_ref", "set_sidx_n"], (VE_TYPE_L, NONE)), + ("L5r", ["set_sidx_n", "set_sidx_list_ref"], (NONE, VE_TYPE_L)), + ("L6", ["set_sidx_list_ref", "cannot_apply"], (VE_TYPE_L, NONE)), + ("L6r", ["cannot_apply", "set_sidx_list_ref"], (NONE, VE_TYPE_L)), + ("L7", ["set_sidx_list_ref", "check_present"], (VE_TYPE_L, NONE)), + ("L7r", ["check_present", "set_sidx_list_ref"], (NONE, VE_TYPE_L)), + ("L8", ["set_sidx_list_ref", "delete_present"], (VE_TYPE_L, NONE)), + ("L8r", ["delete_present", "set_sidx_list_ref"], (NONE, VE_TYPE_L)), + ("L9", ["set_sidx_list_ref", "put_empty"], (VE_TYPE_L, NONE)), + ("L9r", ["put_empty", "set_sidx_list_ref"], EMPTY_ITEM_WORDING), + ("L10", ["set_sidx_list_ref", "put_wrong"], (VE_TYPE_L, NONE)), + ("L10r", ["put_wrong", "set_sidx_list_ref"], (VE_TYPE, NONE)), + # An Update member's empty literal is checked before any member runs, + # below the faults above: ahead of the member's own failing condition + # (t1, t2, t5, t6) and of a wrong type computed from the item in the same + # statement (t3, t4), and the first such member in request order decides + # (K1, K1r). + ("t1", ["r_sidx_empty_failing", "plain_a"], EMPTY_UPDATE_WORDING), + ("t2", ["r_bidx_empty_failing", "plain_a"], EMPTY_UPDATE_WORDING_B), + ("t3", ["r_sidx_n_bidx_empty_failing"], EMPTY_UPDATE_WORDING_B), + ("t4", ["r_sidx_empty_bidx_pk"], EMPTY_UPDATE_WORDING), + ("t5", ["r_sidx_empty_failing"], EMPTY_UPDATE_WORDING), + ("t6", ["r_bidx_empty_failing"], EMPTY_UPDATE_WORDING_B), + ("K1", ["set_sidx_n_bidx_empty", "set_sidx_empty"], EMPTY_UPDATE_WORDING_B), + ("K1r", ["set_sidx_empty", "set_sidx_n_bidx_empty"], EMPTY_UPDATE_WORDING), + # 2026-11-01. Across members the first empty value in request order + # decides, an empty literal or an empty value written from the item + # (U3a, U3b, U3c); within one statement the literal comes first (U3f). A + # member whose own condition fails writes nothing, so its empty + # if_not_exists fallback is never reported (U1a to U1c). + ("U3a", ["set_sidx_stored_empty", "set_bidx_empty"], EMPTY_UPDATE_WORDING), + ("U3b", ["set_sidx_fallback_empty", "set_bidx_empty"], EMPTY_UPDATE_WORDING), + ("U3c", ["set_bidx_empty", "set_sidx_stored_empty"], EMPTY_UPDATE_WORDING_B), + ("U3f", ["set_sidx_e_bidx_empty"], EMPTY_UPDATE_WORDING_B), + ("U1a", ["r_fallback_empty_failing"], (CCF,)), + ("U1b", ["r_fallback_empty_failing", "plain_a"], (CCF, NONE)), + ("U1c", ["plain_a", "r_fallback_empty_failing"], (NONE, CCF)), +] + + +def _items(client, table: str) -> dict: + return { + pk: client.get_item(TableName=table, Key={"pk": {"S": pk}}, ConsistentRead=True).get("Item") + for pk in ("a", "b0", "bp", "c", "d0", "r") + } + + +SEEDED = {"a": ITEM_A, "b0": None, "bp": ITEM_BP, "c": None, "d0": None, "r": ITEM_R} + + +@pytest.mark.parametrize("row_id, names, expected", ROWS, ids=[r[0] for r in ROWS]) +def test_transaction_answers_as_the_service_does(dynamodb_client, table, row_id, names, expected): + shapes = members(table) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=[shapes[n] for n in names]) + resp = exc_info.value.response + if isinstance(expected, str): + assert resp["Error"]["Code"] == "ValidationException", resp["Error"] + assert resp["Error"]["Message"] == expected + assert "CancellationReasons" not in resp + else: + assert resp["Error"]["Code"] == "TransactionCanceledException", resp["Error"] + codes = [code for code, _ in expected] + assert resp["Error"]["Message"] == f"{CANCELLED} [{', '.join(codes)}]" + got = [(r["Code"], r.get("Message")) for r in resp["CancellationReasons"]] + assert got == list(expected) + assert _items(dynamodb_client, table) == SEEDED + + +SINGLE = [ + ("S1", "update", "set_sidx_n", COMPUTED_WORDING), + ("S2", "update", "set_sidx_empty", EMPTY_UPDATE_WORDING), + ("S3", "put", "put_empty", EMPTY_ITEM_WORDING), + ("S4", "update", "set_sidx_lit_n", TYPE_WORDING), + ("S5", "put", "put_wrong", TYPE_WORDING), +] + + +@pytest.mark.parametrize("row_id, kind, name, message", SINGLE, ids=[r[0] for r in SINGLE]) +def test_single_request_controls(dynamodb_client, table, row_id, kind, name, message): + shape = members(table)[name] + with pytest.raises(ClientError) as exc_info: + if kind == "update": + dynamodb_client.update_item(**shape["Update"]) + else: + dynamodb_client.put_item(**shape["Put"]) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == message + assert _items(dynamodb_client, table) == SEEDED + + +# UpdateItem on r, measured on 2026-10-31: an empty index-key literal is +# reported on the statement, after the literal type check and ahead of the +# condition and of a wrong type computed from the item (s1 to s9), and +# list_append that reads the item is a wrong type supplied in the request, +# ahead of the condition (la1, la2). Each row: id, UpdateExpression, names, +# values, whether the failing condition attribute_not_exists(pk) is attached, +# and the ValidationException message. +STATEMENT_ROWS = [ + ("s1", "SET #s = :e", {"#s": "sidx"}, {":e": {"S": ""}}, True, EMPTY_UPDATE_WORDING), + ("s2", "SET #b = :eb", {"#b": "bidx"}, {":eb": {"B": b""}}, True, EMPTY_UPDATE_WORDING_B), + ("s3", "SET #s = #n, #b = :eb", {"#s": "sidx", "#n": "n", "#b": "bidx"}, {":eb": {"B": b""}}, True, EMPTY_UPDATE_WORDING_B), + ("s4", "SET #b = :eb, #s = #n", {"#s": "sidx", "#n": "n", "#b": "bidx"}, {":eb": {"B": b""}}, True, EMPTY_UPDATE_WORDING_B), + ("s5", "SET #s = :e, #b = #k", {"#s": "sidx", "#b": "bidx", "#k": "pk"}, {":e": {"S": ""}}, True, EMPTY_UPDATE_WORDING), + ("s6", "SET #s = #n, #b = :eb", {"#s": "sidx", "#n": "n", "#b": "bidx"}, {":eb": {"B": b""}}, False, EMPTY_UPDATE_WORDING_B), + ("s7", "SET #s = :e, #b = #k", {"#s": "sidx", "#b": "bidx", "#k": "pk"}, {":e": {"S": ""}}, False, EMPTY_UPDATE_WORDING), + ("s8", "SET #s = :e", {"#s": "sidx"}, {":e": {"S": ""}}, False, EMPTY_UPDATE_WORDING), + ("s9", "SET #b = :eb", {"#b": "bidx"}, {":eb": {"B": b""}}, False, EMPTY_UPDATE_WORDING_B), + ("la1", "SET #s = list_append(#l, :l)", {"#s": "sidx", "#l": "l"}, {":l": {"L": [{"S": "y"}]}}, False, TYPE_WORDING_L), + ("la2", "SET #s = list_append(#l, :l)", {"#s": "sidx", "#l": "l"}, {":l": {"L": [{"S": "y"}]}}, True, TYPE_WORDING_L), +] + + +@pytest.mark.parametrize( + "row_id, expr, names, values, failing, message", STATEMENT_ROWS, ids=[r[0] for r in STATEMENT_ROWS] +) +def test_update_item_answers_as_the_service_does(dynamodb_client, table, row_id, expr, names, values, failing, message): + request = { + "TableName": table, + "Key": {"pk": {"S": "r"}}, + "UpdateExpression": expr, + "ExpressionAttributeNames": dict(names), + "ExpressionAttributeValues": values, + } + if failing: + request["ExpressionAttributeNames"]["#p"] = "pk" + request["ConditionExpression"] = "attribute_not_exists(#p)" + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item(**request) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == message + assert _items(dynamodb_client, table) == SEEDED diff --git a/tests/test_transact_index_key_token.py b/tests/test_transact_index_key_token.py new file mode 100644 index 000000000..274dba62e --- /dev/null +++ b/tests/test_transact_index_key_token.py @@ -0,0 +1,346 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""A ClientRequestToken beside a key fault in TransactWriteItems, as measured on +DynamoDB (us-east-1, 2026-11-01, each sequence sent twice with a fresh token, +identical answers). + +Table: pk S; GSI gsi_s on sidx S; GSI gsi_b on bidx B. Items r = {pk: r, n: N 5, +e: S ""} and bp = {pk: bp}; b0 and c are absent. + +- The request-level checks come before the token check: a wrong-typed literal + on an index key, a wrong-typed table key on a Put or a Delete, and a Put + item's empty index key are answered even when the token belongs to another + request (O rows), and they leave the token unused: another request with the + token then succeeds (U rows). +- A fault found after them leaves the token used: an Update's empty literal, + an empty or wrong-typed value written from the item, and a failed + condition. Another request with the token then answers + IdempotentParameterMismatchException and is not applied (O and U rows), and + the same request with its token answers what it answered the first time (R + rows). + +A second measurement on 2026-11-05 (each sequence sent twice with a fresh +token and fresh keys, identical answers) found that every cancellation keeps +the token used against another request: an Update that grows the item past +400 KB (Q2a), an action that cannot apply (Q3a), and a failed condition on a +ConditionCheck, a Delete or a Put member (Q5a to Q5c). The same request sent +again runs again (Q3b). A request with the token of a cancelled request and +other ReturnConsumedCapacity answers the mismatch (Q7a). Items a = {pk: a} +and big = {pk: big, blob: S of 300 000 bytes} take part in these rows. + +The mismatch is compared by its error code, as +tests/test_idempotency_account_scope.py does. + +Run with EXTENDDB_TEST_ENDPOINT unset to assert the same against the service. +""" + +from __future__ import annotations + +import os +import uuid + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +TYPE_WORDING = ( + "One or more parameter values were invalid: Type mismatch for Index Key sidx " + "Expected: S Actual: N IndexName: gsi_s" +) +PUT_KEY_TYPE_WORDING = "One or more parameter values were invalid: Type mismatch for key pk expected: S actual: N" +KEY_SCHEMA_WORDING = "The provided key element does not match the schema" +COMPUTED_WORDING = "The update expression attempted to update the secondary index key to unsupported type" +EMPTY_ITEM_WORDING = ( + "One or more parameter values are not valid. A value specified for a secondary index key " + "is not supported. The AttributeValue for a key attribute cannot contain an empty string " + "value. IndexName: gsi_s, IndexKey: sidx" +) +EMPTY_UPDATE_WORDING = ( + "One or more parameter values are not valid. The update expression attempted to " + "update a secondary index key to a value that is not supported. The AttributeValue " + "for a key attribute cannot contain an empty string value." +) +CCF_WORDING = "The conditional request failed" +UPDATE_SIZE_WORDING = "Item size to update has exceeded the maximum allowed size" +ABSENT_OPERAND_WORDING = "The provided expression refers to an attribute that does not exist in the item" +CANCELLED = "Transaction cancelled, please refer cancellation reasons for specific reasons" +MISMATCH = "IdempotentParameterMismatchException" + +# UpdateItem and TransactWriteItems size an Update on the finished item on +# Cassandra and apply no item-size limit to updates on MongoDB (#818, #761), +# so the 400 KB row runs on the other backends and against the service only, +# as in tests/test_item_size_at_limit.py. +ON_DEFAULT_BACKENDS_ONLY = pytest.mark.skipif( + os.environ.get("EXTENDDB_TEST_BACKEND", "") in {"mongodb", "cassandra"}, + reason="an Update is sized on the finished item (Cassandra) or unbounded (MongoDB): #818, #761", +) +# The fingerprint covers TransactItems only, where the service's covers +# ReturnConsumedCapacity too (#514). +XFAIL_ON_EXTENDDB_514 = pytest.mark.xfail( + bool(os.environ.get("EXTENDDB_TEST_ENDPOINT", "").strip()), + reason="#514: the token fingerprint does not cover ReturnConsumedCapacity", + raises=AssertionError, + strict=False, +) + + +@pytest.fixture(scope="module") +def table(dynamodb_client): + """The measured table: pk S, GSI gsi_s on sidx S, GSI gsi_b on bidx B.""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "sidx", "AttributeType": "S"}, + {"AttributeName": "bidx", "AttributeType": "B"}, + ], + key_schema=[{"AttributeName": "pk", "KeyType": "HASH"}], + GlobalSecondaryIndexes=[ + { + "IndexName": "gsi_s", + "KeySchema": [{"AttributeName": "sidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + }, + { + "IndexName": "gsi_b", + "KeySchema": [{"AttributeName": "bidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + }, + ], + ) as name: + yield name + + +ITEM_R = {"pk": {"S": "r"}, "n": {"N": "5"}, "e": {"S": ""}} +ITEM_BP = {"pk": {"S": "bp"}} +ITEM_A = {"pk": {"S": "a"}} +ITEM_BIG = {"pk": {"S": "big"}, "blob": {"S": "x" * 300_000}} +SEEDED = {"a": ITEM_A, "b0": None, "bp": ITEM_BP, "c": None, "r": ITEM_R} + + +@pytest.fixture(autouse=True) +def seeded(dynamodb_client, table): + """Items r, bp and a present, b0 and c absent, before every case.""" + for item in (ITEM_R, ITEM_BP, ITEM_A): + dynamodb_client.put_item(TableName=table, Item=item) + for pk in ("b0", "c"): + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": pk}}) + + +def _update(table: str, pk: str, expr: str, names: dict, values: dict | None = None, cond: str | None = None) -> dict: + member = { + "TableName": table, + "Key": {"pk": {"S": pk}}, + "UpdateExpression": expr, + "ExpressionAttributeNames": names, + } + if values: + member["ExpressionAttributeValues"] = values + if cond: + member["ConditionExpression"] = cond + return {"Update": member} + + +def faults(table: str) -> dict: + """The one-member requests of the measurement, by fault.""" + return { + # Update b0 SET sidx = :n, :n = N 1: a wrong-typed literal on an index key. + "lit-type": _update(table, "b0", "SET #s = :n", {"#s": "sidx"}, {":n": {"N": "1"}}), + # Put {pk: N 1} and Delete {pk: N 1}: a wrong-typed table key. + "key-type-put": {"Put": {"TableName": table, "Item": {"pk": {"N": "1"}}}}, + "key-type-del": {"Delete": {"TableName": table, "Key": {"pk": {"N": "1"}}}}, + # Put {pk: c, sidx: S ""}: an empty index key in the item. + "put-empty": {"Put": {"TableName": table, "Item": {"pk": {"S": "c"}, "sidx": {"S": ""}}}}, + # Update b0 SET sidx = :e, :e = S "": an empty literal on an index key. + "lit-empty": _update(table, "b0", "SET #s = :e", {"#s": "sidx"}, {":e": {"S": ""}}), + # Update r SET sidx = n: the stored N 5 written to the string key. + "comp-type": _update(table, "r", "SET #s = #n", {"#s": "sidx", "#n": "n"}), + # Update r SET sidx = e: the stored empty string written to the key. + "comp-empty": _update(table, "r", "SET #s = #e", {"#s": "sidx", "#e": "e"}), + # Update bp with attribute_not_exists(pk): the condition fails. + "ccf": _update( + table, "bp", "SET #v = :v", {"#v": "v", "#p": "pk"}, {":v": {"S": "x"}}, "attribute_not_exists(#p)" + ), + # 2026-11-05. Update big SET blob2 = :b: 300 000 and 150 000 bytes, + # past 400 KB. + "size": _update(table, "big", "SET #b = :b", {"#b": "blob2"}, {":b": {"S": "y" * 150_000}}), + # Update a SET v = v + :one: a has no v, so the action cannot apply. + "cannot-apply": _update(table, "a", "SET #v = #v + :one", {"#v": "v"}, {":one": {"N": "1"}}), + # attribute_not_exists(pk) on bp, which exists, on a ConditionCheck, + # a Delete and a Put member. + "check-ccf": { + "ConditionCheck": { + "TableName": table, + "Key": {"pk": {"S": "bp"}}, + "ConditionExpression": "attribute_not_exists(#p)", + "ExpressionAttributeNames": {"#p": "pk"}, + } + }, + "delete-ccf": { + "Delete": { + "TableName": table, + "Key": {"pk": {"S": "bp"}}, + "ConditionExpression": "attribute_not_exists(#p)", + "ExpressionAttributeNames": {"#p": "pk"}, + } + }, + "put-ccf": { + "Put": { + "TableName": table, + "Item": {"pk": {"S": "bp"}, "v": {"S": "x"}}, + "ConditionExpression": "attribute_not_exists(#p)", + "ExpressionAttributeNames": {"#p": "pk"}, + } + }, + } + + +# The answer to each fault alone: a tuple of (Code, Message) cancellation +# reasons, or the message of a top-level ValidationException. +ANSWERS = { + "lit-type": (("ValidationError", TYPE_WORDING),), + "key-type-put": (("ValidationError", PUT_KEY_TYPE_WORDING),), + "key-type-del": (("ValidationError", KEY_SCHEMA_WORDING),), + "put-empty": EMPTY_ITEM_WORDING, + "lit-empty": EMPTY_UPDATE_WORDING, + "comp-type": (("ValidationError", COMPUTED_WORDING),), + "comp-empty": EMPTY_UPDATE_WORDING, + "ccf": (("ConditionalCheckFailed", CCF_WORDING),), + "size": (("ValidationError", UPDATE_SIZE_WORDING),), + "cannot-apply": (("ValidationError", ABSENT_OPERAND_WORDING),), + "check-ccf": (("ConditionalCheckFailed", CCF_WORDING),), + "delete-ccf": (("ConditionalCheckFailed", CCF_WORDING),), + "put-ccf": (("ConditionalCheckFailed", CCF_WORDING),), +} +# Answered before the token is checked; the token stays unused. +BEFORE_TOKEN = ["lit-type", "key-type-put", "key-type-del", "put-empty"] +# Found after the token is checked; the token stays used. +AFTER_TOKEN = ["lit-empty", "comp-type", "comp-empty", "ccf"] +# The cancellations of the 2026-11-05 measurement. +CANCELLATIONS = [ + pytest.param("size", id="Q2a", marks=ON_DEFAULT_BACKENDS_ONLY), + pytest.param("cannot-apply", id="Q3a"), + pytest.param("check-ccf", id="Q5a"), + pytest.param("delete-ccf", id="Q5b"), + pytest.param("put-ccf", id="Q5c"), +] + + +def _token() -> str: + return f"t416-{uuid.uuid4().hex[:30]}" + + +def _send(client, items: list, token: str, **params): + """Send TransactWriteItems with the token (and `params`) and return the + answer: "OK", a tuple of (Code, Message) reasons, a ValidationException + message, or the error code of anything else.""" + try: + client.transact_write_items(TransactItems=items, ClientRequestToken=token, **params) + except ClientError as exc: + resp = exc.response + code = resp["Error"]["Code"] + if code == "TransactionCanceledException": + reasons = tuple((r["Code"], r.get("Message")) for r in resp["CancellationReasons"]) + assert resp["Error"]["Message"] == f"{CANCELLED} [{', '.join(c for c, _ in reasons)}]" + return reasons + if code == "ValidationException": + assert "CancellationReasons" not in resp + return resp["Error"]["Message"] + return code + return "OK" + + +def _item(client, table: str, pk: str): + return client.get_item(TableName=table, Key={"pk": {"S": pk}}, ConsistentRead=True).get("Item") + + +def _seeded_unchanged(client, table: str) -> bool: + return {pk: _item(client, table, pk) for pk in SEEDED} == SEEDED + + +def _fresh_put(table: str) -> tuple[str, dict]: + pk = f"t416-{uuid.uuid4().hex[:12]}" + return pk, {"Put": {"TableName": table, "Item": {"pk": {"S": pk}}}} + + +@pytest.mark.parametrize("fault", BEFORE_TOKEN + AFTER_TOKEN, ids=[f"O-{f}" for f in BEFORE_TOKEN + AFTER_TOKEN]) +def test_a_token_a_committed_request_used(dynamodb_client, table, fault): + """A Put commits with the token, then a request with the fault and the same + token: the request-level faults answer themselves, the later ones the + mismatch.""" + token = _token() + pk, put = _fresh_put(table) + try: + assert _send(dynamodb_client, [put], token) == "OK" + expected = ANSWERS[fault] if fault in BEFORE_TOKEN else MISMATCH + assert _send(dynamodb_client, [faults(table)[fault]], token) == expected + assert _seeded_unchanged(dynamodb_client, table) + finally: + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": pk}}) + + +@pytest.mark.parametrize("fault", BEFORE_TOKEN + AFTER_TOKEN, ids=[f"U-{f}" for f in BEFORE_TOKEN + AFTER_TOKEN]) +def test_a_token_a_cancelled_request_used(dynamodb_client, table, fault): + """A request with the fault is cancelled with the token, then a Put with + the same token: it succeeds after a request-level fault and answers the + mismatch, writing nothing, after a later fault.""" + token = _token() + pk, put = _fresh_put(table) + try: + assert _send(dynamodb_client, [faults(table)[fault]], token) == ANSWERS[fault] + if fault in BEFORE_TOKEN: + assert _send(dynamodb_client, [put], token) == "OK" + assert _item(dynamodb_client, table, pk) is not None + else: + assert _send(dynamodb_client, [put], token) == MISMATCH + assert _item(dynamodb_client, table, pk) is None + assert _seeded_unchanged(dynamodb_client, table) + finally: + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": pk}}) + + +@pytest.mark.parametrize( + "fault", AFTER_TOKEN + ["cannot-apply"], ids=[f"R-{f}" for f in AFTER_TOKEN] + ["Q3b"] +) +def test_the_same_cancelled_request_with_its_token(dynamodb_client, table, fault): + """A request cancelled after the request-level checks, sent again with its + token, answers what it answered the first time.""" + token = _token() + request = [faults(table)[fault]] + assert _send(dynamodb_client, request, token) == ANSWERS[fault] + assert _send(dynamodb_client, request, token) == ANSWERS[fault] + assert _seeded_unchanged(dynamodb_client, table) + + +@pytest.mark.parametrize("fault", CANCELLATIONS) +def test_a_cancellation_keeps_the_token_used(dynamodb_client, table, fault): + """A request is cancelled with the token, then a Put with the same token + answers the mismatch and writes nothing, whatever cancelled the request.""" + token = _token() + pk, put = _fresh_put(table) + if fault == "size": + dynamodb_client.put_item(TableName=table, Item=ITEM_BIG) + try: + assert _send(dynamodb_client, [faults(table)[fault]], token) == ANSWERS[fault] + assert _send(dynamodb_client, [put], token) == MISMATCH + assert _item(dynamodb_client, table, pk) is None + assert _seeded_unchanged(dynamodb_client, table) + if fault == "size": + assert _item(dynamodb_client, table, "big") == ITEM_BIG + finally: + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": pk}}) + if fault == "size": + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": "big"}}) + + +@XFAIL_ON_EXTENDDB_514 +def test_a_cancelled_token_with_other_return_parameters(dynamodb_client, table): + """Q7a: the cancelled request sent again with ReturnConsumedCapacity is + another request for the token.""" + token = _token() + request = [faults(table)["ccf"]] + assert _send(dynamodb_client, request, token) == ANSWERS["ccf"] + assert _send(dynamodb_client, request, token, ReturnConsumedCapacity="TOTAL") == MISMATCH + assert _seeded_unchanged(dynamodb_client, table)