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 09d3f13b4..375ec1c8a 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -1821,12 +1821,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, @@ -1834,30 +1837,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 @@ -1946,6 +1942,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. @@ -4885,3 +4937,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 25b92ab64..735e9dc02 100755 --- a/crates/engine/src/create_table.rs +++ b/crates/engine/src/create_table.rs @@ -219,6 +219,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 c54a45f1b..fab225bd3 100755 --- a/crates/engine/src/update_item.rs +++ b/crates/engine/src/update_item.rs @@ -256,11 +256,12 @@ pub(crate) async fn handle_update_item_with( 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 @@ -277,6 +278,7 @@ pub(crate) async fn handle_update_item_with( &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 10aca1d2b..f4a7146a9 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -2262,6 +2262,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 @@ -2328,13 +2376,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) @@ -2343,15 +2422,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 @@ -2380,14 +2464,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); @@ -2399,7 +2490,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 @@ -2415,6 +2508,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)); @@ -2435,6 +2533,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 @@ -2470,9 +2571,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, @@ -2808,10 +2969,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, @@ -2819,9 +2988,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, @@ -3878,11 +4045,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 480847fbf..f86f4a53a 100644 --- a/crates/storage-postgres/src/data/transactions.rs +++ b/crates/storage-postgres/src/data/transactions.rs @@ -13,15 +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, StreamCapture, TransactGetOp, TransactWriteOp}; +use extenddb_storage::{ + IdempotencyKey, StreamCapture, 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, stream_record_shard_in_tx, upsert_item_in_tx, - write_stream_record_to_shard_in_tx, + insert_item_if_absent_in_tx, record_cancelled_token, stream_record_shard_in_tx, + upsert_item_in_tx, write_stream_record_to_shard_in_tx, }; use crate::PostgresEngine; use crate::pg_util::is_conflict_abort; @@ -128,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()]; @@ -152,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( @@ -211,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. @@ -223,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), @@ -245,14 +274,26 @@ impl PostgresEngine { if transact_op_stream(op).is_none() { continue; } - let shard = stream_record_shard_in_tx( + let shard = match stream_record_shard_in_tx( &mut tx, transact_op_key_info(op), old_item.as_ref(), new_item.as_ref(), ) .await - .map_err(cancel_all)?; + { + Ok(shard) => shard, + Err(e) => { + return Err(after_token_error( + &self.data_pool, + Some(tx), + idempotency, + e, + ops.len(), + ) + .await); + } + }; if let Some(shard) = shard { records.push((shard, i)); } @@ -261,8 +302,8 @@ impl PostgresEngine { for (shard, i) in &records { let op = &ops[*i]; let (old_item, new_item) = &op_items[*i]; - if let Some(capture) = transact_op_stream(op) { - write_stream_record_to_shard_in_tx( + if let Some(capture) = transact_op_stream(op) + && let Err(e) = write_stream_record_to_shard_in_tx( &mut tx, transact_op_key_info(op), capture, @@ -271,7 +312,15 @@ impl PostgresEngine { shard, ) .await - .map_err(cancel_all)?; + { + return Err(after_token_error( + &self.data_pool, + Some(tx), + idempotency, + e, + ops.len(), + ) + .await); } } @@ -282,13 +331,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); } } @@ -309,7 +370,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, @@ -318,14 +379,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, @@ -336,15 +409,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, }; @@ -353,7 +448,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(); @@ -391,6 +496,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(); @@ -477,71 +621,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), @@ -582,7 +661,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)?; @@ -727,8 +811,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)?; @@ -764,18 +849,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 0ed177767..d554c8921 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}; @@ -477,21 +477,84 @@ pub(super) async fn write_stream_record_to_shard_in_tx( /// /// The token is scoped to `account_id`: the same token value from different /// accounts is stored and matched independently. Returns `Ok(())` for new -/// tokens (inserted), `Err(IdempotentReplay)` for matching replays, -/// `Err(IdempotentMismatch)` for fingerprint conflicts. +/// tokens (inserted, or a stored row taken for this run), +/// `Err(IdempotentReplay)` for matching replays, `Err(IdempotentMismatch)` +/// for fingerprint conflicts. +/// +/// Two sends of one request with one token can reach this check at the same +/// time (#851). The second send's insert waits on the first send's token row +/// and, once the first send commits, locks the committed row. The select of +/// the same statement reads with the snapshot taken when the statement +/// started, before that commit, so it returns no row, or the `cancelled:` row +/// that the committed row replaced. The locked row is neither: the insert's +/// `WHERE` is false on it, so it is less than 10 minutes old and is not this +/// request's cancelled row. In those two cases the statement runs once more. +/// At READ COMMITTED the second statement takes a new snapshot, which sees +/// the locked row. This transaction holds the lock until it ends, so the row +/// cannot change between the two statements and the second one does not wait +/// on it. A committed row of this request then answers as a replay, the +/// answer the first send got. +/// +/// The second statement reads the locked row when the transaction runs at +/// READ COMMITTED and `NOW()` is the same in both statements. +/// `transact_write_items_impl` opens the transaction with `begin()`, which +/// takes the default isolation level of the role or the database, so the +/// check also tests the second result. If it still returns no row, or this +/// request's `cancelled:` row, the check answers `Err(TransactionInProgress)` +/// and does not run the request. pub(super) async fn check_idempotency_token_in_tx( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, account_id: &str, token: &str, fingerprint: &str, ) -> Result<(), StorageError> { - let row: Option<(String, bool)> = sqlx::query_as( + let mut row = run_token_statement(tx, account_id, token, fingerprint).await?; + if stale_read(row.as_ref(), fingerprint) { + row = run_token_statement(tx, account_id, token, fingerprint).await?; + if stale_read(row.as_ref(), fingerprint) { + return Err(StorageError::TransactionInProgress); + } + } + + match row { + Some((_, true)) | None => Ok(()), + Some((stored, false)) => match StoredToken::of(&stored, fingerprint) { + StoredToken::Replay => Err(StorageError::IdempotentReplay), + StoredToken::Rerun | StoredToken::Mismatch => Err(StorageError::IdempotentMismatch), + }, + } +} + +/// A result of [`run_token_statement`] that a snapshot from before another +/// send's commit gives: no row, or this request's `cancelled:` row that the +/// statement did not take. +fn stale_read(row: Option<&(String, bool)>, fingerprint: &str) -> bool { + match row { + None => true, + Some((stored, false)) => StoredToken::of(stored, fingerprint) == StoredToken::Rerun, + Some((_, true)) => false, + } +} + +/// One statement of [`check_idempotency_token_in_tx`]: insert the token row, +/// or take a stored row that is older than 10 minutes or that this request +/// left cancelled (see `StoredToken::of`), and return it with `true`. +/// Otherwise return the stored row with `false`, read with the statement's +/// snapshot, or no row when that snapshot holds none less than 10 minutes old. +async fn run_token_statement( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + account_id: &str, + token: &str, + fingerprint: &str, +) -> Result, StorageError> { + sqlx::query_as( r"WITH ins AS ( 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 = $4 RETURNING fingerprint, TRUE AS inserted ) SELECT fingerprint, inserted FROM ins @@ -507,13 +570,36 @@ 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)?; + .map_err(db_error) +} - match row { - Some((_, true)) | None => Ok(()), - Some((stored, false)) if stored == fingerprint => Err(StorageError::IdempotentReplay), - Some((_, false)) => 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 7637c6251..974268f67 100644 --- a/crates/storage-postgres/tests/twi_conflict.rs +++ b/crates/storage-postgres/tests/twi_conflict.rs @@ -18,10 +18,10 @@ //! server whose role may create and drop databases. Without it every test here //! reports a skip and passes, the same convention the wire suites use. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::time::Duration; -use extenddb_core::expression::{self, Expr, ExpressionMaps}; +use extenddb_core::expression::{self, Expr, ExpressionMaps, UpdateAction}; use extenddb_core::types::{ AttributeDefinition, AttributeValue, BillingMode, CreateTableInput, GsiInput, Item, KeySchemaElement, KeyType, LsiInput, Projection, ProjectionType, @@ -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"; @@ -487,6 +578,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"); @@ -519,6 +674,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 { @@ -896,3 +1077,555 @@ 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; +} + +/// Rounds of the four tests below. Most rounds send the second request while +/// the first is still running. +const RACE_ROUNDS: usize = 20; + +/// The update `SET n = n + :one` and its maps. +fn increment() -> (Vec, ExpressionMaps) { + let tokens = expression::tokenize("SET n = n + :one").expect("tokenize"); + let actions = expression::parse_update(&tokens).expect("parse"); + let maps = ExpressionMaps::new( + HashMap::new(), + HashMap::from([("one".to_owned(), AttributeValue::N("1".to_owned()))]), + ); + (actions, maps) +} + +/// Seed the item `cnt` with `n` = 0. +async fn seed_counter(s: &Scratch, key_info: &TableKeyInfo) { + let cnt = BTreeMap::from([ + ("pk".to_owned(), AttributeValue::S("cnt".to_owned())), + ("n".to_owned(), AttributeValue::N("0".to_owned())), + ]); + s.engine + .put_item(key_info, cnt, false, None, &ExpressionMaps::default(), None) + .await + .expect("seed cnt"); +} + +/// The stored `n` of `cnt`. +async fn counter(db: &PgPool, table: &str) -> i64 { + let n: String = sqlx::query_scalar(&format!( + "SELECT item_data->'n'->>'N' FROM {table} WHERE pk = 'cnt'" + )) + .fetch_one(db) + .await + .expect("read n"); + n.parse().expect("n is an integer") +} + +/// Send `ops` twice at the same moment, once with each of `keys`, released by +/// a barrier, and return both answers. Bounded, so a send that never finishes +/// fails the test instead of hanging it. +async fn send_twice_at_once( + engine: &PostgresEngine, + ops: &[TransactWriteOp<'_>], + keys: [IdempotencyKey<'_>; 2], +) -> [Result<(), StorageError>; 2] { + let barrier = &tokio::sync::Barrier::new(2); + let send = move |key| async move { + barrier.wait().await; + engine.transact_write_items(ops, Some(key)).await + }; + let (first, second) = tokio::time::timeout(Duration::from_secs(30), async { + tokio::join!(send(keys[0]), send(keys[1])) + }) + .await + .expect("both sends finished"); + [first, second] +} + +/// One send applied the request and the other answered the replay or +/// TransactionInProgress. +fn applied_by_one(answers: &[Result<(), StorageError>; 2]) -> bool { + let applied = answers.iter().filter(|a| a.is_ok()).count(); + let waited = answers + .iter() + .filter(|a| { + matches!( + a, + Err(StorageError::IdempotentReplay | StorageError::TransactionInProgress) + ) + }) + .count(); + applied == 1 && waited == 1 +} + +#[tokio::test] +async fn two_sends_of_one_request_with_one_token_apply_it_once() { + // Measured on the service (2026-11-05): two sends of one request with one + // fresh token at the same moment applied it once in all 20 rounds. One + // send answered OK and the other TransactionInProgressException, or both + // answered OK. Here the second send's token insert waits on the first + // send's token row, and the second send answers the replay once the first + // commits (#851). + let test = "two_sends_of_one_request_with_one_token_apply_it_once"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let plain = ExpressionMaps::default(); + let key_info = seeded_table(&s, &plain).await; + seed_counter(&s, &key_info).await; + let table = data_table(&s.db).await; + let (actions, maps) = increment(); + let cnt = key("cnt"); + let ops = [TransactWriteOp::Update { + key_info: &key_info, + key: &cnt, + actions: &actions, + condition: None, + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + + let mut failed = Vec::new(); + for round in 0..RACE_ROUNDS { + let token = format!("twi-race-{round}"); + let key = IdempotencyKey { + account_id: ACCOUNT, + token: &token, + fingerprint: "fp-increment", + }; + let before = counter(&s.db, &table).await; + let answers = send_twice_at_once(&s.engine, &ops, [key; 2]).await; + let grew = counter(&s.db, &table).await - before; + if grew != 1 || !applied_by_one(&answers) { + failed.push(format!( + "round {round}: n grew by {grew}, answers {answers:?}" + )); + } + } + assert!( + failed.is_empty(), + "{} of {RACE_ROUNDS} rounds:\n{}", + failed.len(), + failed.join("\n") + ); + + s.cleanup().await; +} + +#[tokio::test] +async fn two_sends_that_run_a_cancelled_request_again_apply_it_once() { + // A request cancelled after the request-level key checks keeps its token, + // and the same request sent again with it runs again (measured + // 2026-11-05). Here the second run commits, and it is sent twice at the + // same moment. The second send waits on the first send's token row and + // answers the replay once the first commits. It does not answer the + // mismatch, which is the answer for a different request. + let test = "two_sends_that_run_a_cancelled_request_again_apply_it_once"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let plain = ExpressionMaps::default(); + let key_info = seeded_table(&s, &plain).await; + seed_counter(&s, &key_info).await; + let table = data_table(&s.db).await; + let (actions, maps) = increment(); + let tokens = expression::tokenize("attribute_exists(go)").expect("tokenize"); + let gate = expression::parse_condition(&tokens).expect("parse"); + let cnt = key("cnt"); + let ops = [TransactWriteOp::Update { + key_info: &key_info, + key: &cnt, + actions: &actions, + condition: Some(&gate), + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + let set_go = |sql: String| { + let db = s.db.clone(); + async move { + sqlx::query(&sql) + .execute(&db) + .await + .expect("change go on cnt"); + } + }; + + let mut failed = Vec::new(); + for round in 0..RACE_ROUNDS { + let token = format!("twi-rerun-race-{round}"); + let key = IdempotencyKey { + account_id: ACCOUNT, + token: &token, + fingerprint: "fp-gated-increment", + }; + // Without `go` the condition fails, and the token is kept cancelled. + set_go(format!( + "UPDATE {table} SET item_data = item_data - 'go' WHERE pk = 'cnt'" + )) + .await; + let first = tokio::time::timeout( + Duration::from_secs(30), + s.engine.transact_write_items(&ops, Some(key)), + ) + .await + .expect("the first send finished"); + assert!( + matches!(&first, Err(StorageError::TransactionCanceled(r)) + if r.len() == 1 && r[0].code == "ConditionalCheckFailed"), + "round {round}: {first:?}" + ); + // With `go` the same request commits when it runs again. + set_go(format!( + "UPDATE {table} SET item_data = item_data || \ + jsonb_build_object('go', jsonb_build_object('S', 'y')) WHERE pk = 'cnt'" + )) + .await; + let before = counter(&s.db, &table).await; + let answers = send_twice_at_once(&s.engine, &ops, [key; 2]).await; + let grew = counter(&s.db, &table).await - before; + if grew != 1 || !applied_by_one(&answers) { + failed.push(format!( + "round {round}: n grew by {grew}, answers {answers:?}" + )); + } + } + assert!( + failed.is_empty(), + "{} of {RACE_ROUNDS} rounds:\n{}", + failed.len(), + failed.join("\n") + ); + + s.cleanup().await; +} + +#[tokio::test] +async fn two_requests_with_one_token_at_once_apply_only_one() { + // Two different requests with one fresh token at the same moment. They + // send the same update with two fingerprints, so a second apply shows in + // `n`. Here the second send's token insert waits on the first send's + // token row, and its select reads with a snapshot from before the first + // send's commit, which holds no row. The statement runs again and reads + // the first request's row, so the second send answers the mismatch and + // applies nothing. + let test = "two_requests_with_one_token_at_once_apply_only_one"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let plain = ExpressionMaps::default(); + let key_info = seeded_table(&s, &plain).await; + seed_counter(&s, &key_info).await; + let table = data_table(&s.db).await; + let (actions, maps) = increment(); + let cnt = key("cnt"); + let ops = [TransactWriteOp::Update { + key_info: &key_info, + key: &cnt, + actions: &actions, + condition: None, + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + + let mut failed = Vec::new(); + for round in 0..RACE_ROUNDS { + let token = format!("twi-two-requests-race-{round}"); + let keys = ["fp-a", "fp-b"].map(|fingerprint| IdempotencyKey { + account_id: ACCOUNT, + token: &token, + fingerprint, + }); + let before = counter(&s.db, &table).await; + let answers = send_twice_at_once(&s.engine, &ops, keys).await; + let grew = counter(&s.db, &table).await - before; + let applied = answers.iter().filter(|a| a.is_ok()).count(); + let refused = answers + .iter() + .filter(|a| { + matches!( + a, + Err(StorageError::IdempotentMismatch | StorageError::TransactionInProgress) + ) + }) + .count(); + if grew != 1 || applied != 1 || refused != 1 { + failed.push(format!( + "round {round}: n grew by {grew}, answers {answers:?}" + )); + } + } + assert!( + failed.is_empty(), + "{} of {RACE_ROUNDS} rounds:\n{}", + failed.len(), + failed.join("\n") + ); + + s.cleanup().await; +} + +#[tokio::test] +async fn two_sends_that_take_an_old_token_row_apply_it_once() { + // The next request with a token whose row is more than 10 minutes old + // takes the row. Here another request stored the token 11 minutes ago, + // and one request is then sent twice at the same moment. The second + // send's token insert waits on the row the first send takes, and its + // select reads with a snapshot from before the first send's commit, + // which holds the old row. The 10-minute filter drops that row. The + // statement runs again and reads the first send's row, so the second send + // answers the replay. + let test = "two_sends_that_take_an_old_token_row_apply_it_once"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let plain = ExpressionMaps::default(); + let key_info = seeded_table(&s, &plain).await; + seed_counter(&s, &key_info).await; + let table = data_table(&s.db).await; + let (actions, maps) = increment(); + let cnt = key("cnt"); + let ops = [TransactWriteOp::Update { + key_info: &key_info, + key: &cnt, + actions: &actions, + condition: None, + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }]; + + let mut failed = Vec::new(); + for round in 0..RACE_ROUNDS { + let token = format!("twi-takeover-race-{round}"); + let key = IdempotencyKey { + account_id: ACCOUNT, + token: &token, + fingerprint: "fp-increment", + }; + sqlx::query( + "INSERT INTO idempotency_tokens (account_id, token, fingerprint, created_at) \ + VALUES ($1, $2, 'fp-other', NOW() - INTERVAL '11 minutes')", + ) + .bind(ACCOUNT) + .bind(&token) + .execute(&s.db) + .await + .expect("store the other request's token row"); + let before = counter(&s.db, &table).await; + let answers = send_twice_at_once(&s.engine, &ops, [key; 2]).await; + let grew = counter(&s.db, &table).await - before; + if grew != 1 || !applied_by_one(&answers) { + failed.push(format!( + "round {round}: n grew by {grew}, answers {answers:?}" + )); + } + } + assert!( + failed.is_empty(), + "{} of {RACE_ROUNDS} rounds:\n{}", + failed.len(), + failed.join("\n") + ); + + 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 0af8b654e..d969e4032 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, @@ -284,7 +284,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, @@ -304,11 +306,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()); @@ -328,6 +330,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; @@ -386,6 +420,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 848a25f00..ba0e80553 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 255c2a049..17ec7b568 100755 --- a/crates/storage/src/lib.rs +++ b/crates/storage/src/lib.rs @@ -49,7 +49,11 @@ pub use backup_definition::{ pub use restore_engine::{BulkLoadFile, Finished, RestoreEngine}; 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 b97fde9cc..6767cf7f3 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -99,7 +99,7 @@ from a request sent to that service or backend. | Multi-part base table keys | Not supported | Preview extension (opt-in via the `limits.allow_multipart_table_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). 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) diff --git a/tests/test_transact_write_conflict.py b/tests/test_transact_write_conflict.py index af8ca7f8c..6f5a1ac19 100644 --- a/tests/test_transact_write_conflict.py +++ b/tests/test_transact_write_conflict.py @@ -17,6 +17,11 @@ reason positions on Amazon DynamoDB. The mapping of a database-detected deadlock to TransactionConflict is pinned by the storage tests in crates/storage-postgres/tests/twi_conflict.rs. + +Two sends of one request with one ClientRequestToken at the same moment apply +the request once (measured on Amazon DynamoDB, us-east-1, 2026-11-05, 20 +rounds, retries off). In 18 rounds one send answered OK and the other +TransactionInProgressException; in 2 rounds both answered OK. """ from __future__ import annotations @@ -50,6 +55,10 @@ @pytest.fixture(scope="module") def raw_client(endpoint_url): """A client that never retries, so every 5xx and every cancellation is seen.""" + return _no_retry_client(endpoint_url) + + +def _no_retry_client(endpoint_url): kwargs: dict = { "service_name": "dynamodb", "region_name": os.environ.get("AWS_DEFAULT_REGION", "us-east-1"), @@ -217,3 +226,55 @@ def private() -> str: assert committed + len(failures) == attempts assert attempts == WORKERS * TXNS_PER_WORKER, f"time budget cut the run at {attempts}" assert committed <= _counter(dynamodb_client, table, hot) <= committed + n_5xx + + +TOKEN_ROUNDS = 10 +IN_PROGRESS = "TransactionInProgressException" + + +def test_two_sends_of_one_request_with_one_token_apply_it_once( + dynamodb_client, endpoint_url, table +): + """Two clients send one request with one fresh token at the same moment. + + In every round n grows by 1, and each send answers OK or + TransactionInProgressException, as on Amazon DynamoDB (module docstring). + The clients never retry, so a TransactionInProgressException is seen + instead of retried. + """ + pk = f"cnt-{uuid.uuid4().hex[:8]}" + dynamodb_client.put_item(TableName=table, Item={"pk": {"S": pk}, "n": {"N": "0"}}) + clients = [_no_retry_client(endpoint_url) for _ in range(2)] + request = [ + { + "Update": { + "TableName": table, + "Key": {"pk": {"S": pk}}, + "UpdateExpression": "SET n = n + :one", + "ExpressionAttributeValues": {":one": {"N": "1"}}, + } + } + ] + + def send(client, token: str, start: threading.Barrier) -> str: + start.wait() + try: + client.transact_write_items(TransactItems=request, ClientRequestToken=token) + return "OK" + except ClientError as e: + return e.response["Error"]["Code"] + + rounds = [] + with ThreadPoolExecutor(max_workers=2) as pool: + for _ in range(TOKEN_ROUNDS): + token = uuid.uuid4().hex + before = _counter(dynamodb_client, table, pk) + start = threading.Barrier(2, timeout=30) + futures = [pool.submit(send, c, token, start) for c in clients] + answers = [f.result(timeout=60) for f in futures] + rounds.append((answers, _counter(dynamodb_client, table, pk) - before)) + + wrong = [ + r for r in rounds if r[1] != 1 or any(a not in ("OK", IN_PROGRESS) for a in r[0]) + ] + assert not wrong, f"{len(wrong)} of {TOKEN_ROUNDS} rounds: {rounds}"