diff --git a/.github/workflows/integration-mongodb.yml b/.github/workflows/integration-mongodb.yml index 3bf940c58..4dcc9c9d6 100644 --- a/.github/workflows/integration-mongodb.yml +++ b/.github/workflows/integration-mongodb.yml @@ -104,6 +104,14 @@ jobs: EXTENDDB_TEST_MONGODB_STANDALONE_URI: mongodb://127.0.0.1:27019/?directConnection=true EXTENDDB_TEST_MONGODB_REQUIRED: "1" run: cargo test -p extenddb-storage-mongodb snapshot_engine + - name: Run DELETING window tests + # What DeleteTable and UpdateTable answer while a table is DELETING, + # and that a delete which stopped after its mark is finished. Each + # test works in databases of its own on the replica set above. + env: + EXTENDDB_TEST_MONGODB_URI: mongodb://127.0.0.1:27018/?directConnection=true + EXTENDDB_TEST_MONGODB_REQUIRED: "1" + run: cargo test -p extenddb-storage-mongodb --lib deleting_window_tests - name: Stop MongoDB containers if: always() run: docker rm -f extenddb-runtests-mongo-27018 extenddb-snapshot-standalone-27019 diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 7fd08685e..a31d34261 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -407,14 +407,15 @@ jobs: # storage layer directly against this job's PostgreSQL. The PutItem create # race tests hold a create open in an outside transaction, the # write-transaction conflict tests force a deadlock, the stream clock test - # holds a row lock open while a write waits on it, and the backup restore - # cases need catalog rows the current binary would not write; all need - # direct access. The online GSI build tests reorder queue rows by hand and - # pause writers with a test hook. They build their own throwaway databases; - # the connection string is the server, not a database. `--tests` runs - # every target of the crate, so a new test file needs no edit here, and - # EXTENDDB_TEST_PG_REQUIRED turns a test's skip for a missing connection - # string into a failure. + # holds a row lock open while a write waits on it, the backup restore + # cases need catalog rows the current binary would not write, and the + # DELETING window tests hold a table DELETING with no poller to finish it; + # all need direct access. The online GSI build tests reorder queue rows by + # hand and pause writers with a test hook. They build their own throwaway + # databases; the connection string is the server, not a database. `--tests` + # runs every target of the crate, so a new test file needs no edit here, + # and EXTENDDB_TEST_PG_REQUIRED turns a test's skip for a missing + # connection string into a failure. - name: Run PostgreSQL storage-level tests # table_snapshot: the SnapshotEngine consistency check; it needs the # shared page-size hook, compiled only into the crate's own test diff --git a/crates/core/src/error/messages.rs b/crates/core/src/error/messages.rs index 18d3e521d..e5aedc057 100755 --- a/crates/core/src/error/messages.rs +++ b/crates/core/src/error/messages.rs @@ -40,6 +40,10 @@ pub enum ErrorMessageKey { TableNotFound, TableAlreadyExists, TableInUse, + /// A control-plane change refused because the table is `DELETING`. + /// Measured on the service 2026-10-24 for a second `DeleteTable` and for + /// `UpdateTable` during the window. + TableBeingDeleted, MissingKeySchema, KeySchemaTooMany, KeySchemaFirstNotHash, @@ -82,6 +86,12 @@ pub fn error_message(key: ErrorMessageKey, args: &[&str]) -> String { ErrorMessageKey::TableInUse => { format!("Table is being created, updated, or deleted: {}", args[0]) } + ErrorMessageKey::TableBeingDeleted => { + format!( + "Attempt to change a resource which is still in use: Table is being deleted: {}", + args[0] + ) + } ErrorMessageKey::MissingKeySchema => { "1 validation error detected: Value null at 'keySchema' failed to satisfy constraint: \ Member must not be null" diff --git a/crates/engine/src/create_table.rs b/crates/engine/src/create_table.rs index 7b827cd3a..9a15dad75 100755 --- a/crates/engine/src/create_table.rs +++ b/crates/engine/src/create_table.rs @@ -132,6 +132,9 @@ pub fn storage_err_to_dynamo(e: extenddb_storage::error::StorageError) -> Dynamo ErrorMessageKey::TableInUse, &[&name], )), + StorageError::TableBeingDeleted(name) => DynamoDbError::ResourceInUseException( + error_message(ErrorMessageKey::TableBeingDeleted, &[&name]), + ), // Only RestoreTableFromBackup raises this; its own mapping makes it // TableInUseException. Elsewhere (a plain CreateTable into a name // that is a CREATING restore target) the service's text is the one diff --git a/crates/engine/src/delete_table.rs b/crates/engine/src/delete_table.rs index 4c48e4239..022c38793 100755 --- a/crates/engine/src/delete_table.rs +++ b/crates/engine/src/delete_table.rs @@ -45,3 +45,28 @@ pub async fn handle_delete_table( }; serialize_output(&output) } + +#[cfg(test)] +mod tests { + use super::*; + use extenddb_storage::error::StorageError; + + /// A second DeleteTable during the DELETING window: the measured sentence + /// (2026-10-24), byte for byte. The other non-ACTIVE statuses keep theirs. + #[test] + fn a_deleting_table_has_its_own_sentence() { + match storage_err_to_dynamo(StorageError::TableBeingDeleted("t".to_owned())) { + DynamoDbError::ResourceInUseException(msg) => assert_eq!( + msg, + "Attempt to change a resource which is still in use: Table is being deleted: t" + ), + other => panic!("expected ResourceInUseException, got {other:?}"), + } + match storage_err_to_dynamo(StorageError::TableNotActive("t".to_owned())) { + DynamoDbError::ResourceInUseException(msg) => { + assert_eq!(msg, "Table is being created, updated, or deleted: t"); + } + other => panic!("expected ResourceInUseException, got {other:?}"), + } + } +} diff --git a/crates/engine/src/index_helpers.rs b/crates/engine/src/index_helpers.rs index e5f78eaf5..fd4c70147 100755 --- a/crates/engine/src/index_helpers.rs +++ b/crates/engine/src/index_helpers.rs @@ -127,6 +127,38 @@ pub fn classify_vector_index_read( } } +/// The answer for a GSI or LSI name that `index_info_by_table_id` reported +/// missing (`err`) on the table `key_info` describes. +/// +/// The key info can come from the key-info cache, and the cache keeps an entry +/// read while the table was `DELETING` after the control plane removes the +/// table, since that removal does not reach the cache. The index rows go with +/// the table, so a cached entry would turn a read on a table that is gone into +/// `The table does not have the specified index` until the entry expires. The +/// catalog is therefore asked for the table again. A table it no longer has is +/// answered as a read on a missing table (`ResourceNotFoundException`) and its +/// cache entry is dropped; a table it still has keeps `err`. The extra read +/// runs only for an index name that does not resolve. +pub async fn unresolved_index_error( + ctx: &OperationContext, + key_info: &TableKeyInfo, + err: extenddb_storage::error::StorageError, +) -> DynamoDbError { + match ctx + .storage + .table_key_info(&ctx.account_id, &key_info.table_name) + .await + { + Err(gone @ extenddb_storage::error::StorageError::TableNotFound(_)) => { + ctx.auth_cache + .invalidate_table_key_info(&ctx.account_id, &key_info.table_name) + .await; + crate::create_table::storage_err_to_dynamo(gone) + } + _ => crate::create_table::storage_err_to_dynamo(err), + } +} + /// Resolve an index name that `index_info_by_table_id` reported missing /// against the table's vector index metadata. /// diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index b6d133e00..79f501d02 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -22,7 +22,7 @@ use crate::create_table::storage_err_to_dynamo; use crate::expression_helpers::{build_expression_maps, parse_optional_filter}; use crate::index_helpers::{ VectorIndexReadRefusal, classify_unresolved_index_read, combined_lek_key_schema, - validate_query_select_index_support, + unresolved_index_error, validate_query_select_index_support, }; use crate::legacy_filter::{desugar_filter, desugar_key_conditions}; use crate::read_helpers::{PostRead, read_page}; @@ -101,7 +101,7 @@ pub async fn handle_query( Err(err @ StorageError::IndexNotFound(_)) => { match classify_unresolved_index_read(ctx, &key_info, idx_name).await? { VectorIndexReadRefusal::NotFound => { - return Err(storage_err_to_dynamo(err)); + return Err(unresolved_index_error(ctx, &key_info, err).await); } // Query refuses a backfilling vector index with the same // message as an active one (measured 2026-08-20; Scan diff --git a/crates/engine/src/scan.rs b/crates/engine/src/scan.rs index 6a6e523d5..f853ceb4b 100755 --- a/crates/engine/src/scan.rs +++ b/crates/engine/src/scan.rs @@ -21,6 +21,7 @@ use crate::create_table::storage_err_to_dynamo; use crate::expression_helpers::{build_expression_maps, parse_optional_filter}; use crate::index_helpers::{ VectorIndexReadRefusal, classify_unresolved_index_read, combined_lek_key_schema, + unresolved_index_error, }; use crate::legacy_filter::desugar_filter; use crate::read_helpers::{PostRead, read_page}; @@ -259,7 +260,9 @@ pub async fn handle_scan( Err(err @ StorageError::IndexNotFound(_)) => { return Err( match classify_unresolved_index_read(ctx, &key_info, idx_name).await? { - VectorIndexReadRefusal::NotFound => storage_err_to_dynamo(err), + VectorIndexReadRefusal::NotFound => { + unresolved_index_error(ctx, &key_info, err).await + } VectorIndexReadRefusal::Backfilling => { DynamoDbError::ValidationException(format!( "{VECTOR_INDEX_BACKFILLING_SCAN_PREFIX}{idx_name}" diff --git a/crates/engine/src/tagging.rs b/crates/engine/src/tagging.rs index 432d75dd4..c94b8b716 100755 --- a/crates/engine/src/tagging.rs +++ b/crates/engine/src/tagging.rs @@ -46,6 +46,48 @@ fn extract_table_name_from_arn(arn: &str) -> Option<&str> { Some(table_name.split('/').next().unwrap_or(table_name)) } +/// What [`validate_resource_arn`] answers for a table that is `DELETING`. +#[derive(Clone, Copy, PartialEq, Eq)] +enum WhileDeleting { + /// `ListTagsOfResource`: the table is listed until it is gone. + Serve, + /// `TagResource` and `UntagResource`: refused as a table still in use. + Refuse, +} + +/// Refuse a tag change on a table that is being deleted. The service answers +/// `TagResource` and `UntagResource` on a `DELETING` table with +/// `ResourceInUseException` and the sentence `DeleteTable` and `UpdateTable` +/// give in that window, and serves `ListTagsOfResource` (measured +/// 2026-11-03). The key info carries no status, and the data plane reads a +/// `DELETING` table as found on PostgreSQL and SQLite, so the status is read +/// with `describe_table`. Any other answer from it is left to the existence +/// check that follows. +async fn refuse_while_deleting( + table_name: &str, + ctx: &OperationContext, +) -> Result<(), DynamoDbError> { + if let Ok(desc) = ctx + .storage + .describe_table( + &ctx.account_id, + extenddb_core::types::DescribeTableInput { + table_name: table_name.to_owned(), + }, + ) + .await + && desc.table_status == extenddb_core::types::TableStatus::Deleting + { + return Err(DynamoDbError::ResourceInUseException( + extenddb_core::error::error_message( + extenddb_core::error::ErrorMessageKey::TableBeingDeleted, + &[table_name], + ), + )); + } + Ok(()) +} + /// Validate that the ARN refers to an existing table owned by the caller. /// /// The error classes and messages below match `DynamoDB`, verified against the @@ -61,13 +103,18 @@ fn extract_table_name_from_arn(arn: &str) -> Option<&str> { /// | Account differs from the caller's | `AccessDeniedException` | /// | Table ARN with a different partition, service, or region | `ValidationException` (invalid `TableArn`) | /// | Table does not exist | `ResourceNotFoundException` | +/// | Table is `DELETING`, for `TagResource` and `UntagResource` (`while_deleting` is `Refuse`) | `ResourceInUseException` (`Table is being deleted`) | /// /// Account is checked before the partition, service, and region, matching the /// service: a cross-account ARN is denied without revealing whether its other /// fields are valid. For a table ARN, a no-permissions principal is denied by /// authorization before this runs; the partition, service, region, and /// existence errors therefore surface only for an authorized principal. -async fn validate_resource_arn(arn: &str, ctx: &OperationContext) -> Result<(), DynamoDbError> { +async fn validate_resource_arn( + arn: &str, + ctx: &OperationContext, + while_deleting: WhileDeleting, +) -> Result<(), DynamoDbError> { if !arn.starts_with("arn:") { return Err(DynamoDbError::ValidationException(format!( "One or more parameter values were invalid: ARNs must start with 'arn:': {arn}" @@ -140,6 +187,9 @@ async fn validate_resource_arn(arn: &str, ctx: &OperationContext) -> Result<(), )) })?; + if while_deleting == WhileDeleting::Refuse { + refuse_while_deleting(table_name, ctx).await?; + } // Verify the table exists via table_key_info (lightweight check). ctx.storage .table_key_info(&ctx.account_id, table_name) @@ -175,7 +225,7 @@ pub async fn handle_tag_resource( )); } - validate_resource_arn(&input.resource_arn, ctx).await?; + validate_resource_arn(&input.resource_arn, ctx, WhileDeleting::Refuse).await?; ctx.storage .tag_resource(&input.resource_arn, &input.tags) @@ -212,7 +262,7 @@ pub async fn handle_untag_resource( )); } - validate_resource_arn(&input.resource_arn, ctx).await?; + validate_resource_arn(&input.resource_arn, ctx, WhileDeleting::Refuse).await?; ctx.storage .untag_resource(&input.resource_arn, &input.tag_keys) @@ -247,7 +297,7 @@ pub async fn handle_list_tags_of_resource( )); } - validate_resource_arn(&input.resource_arn, ctx).await?; + validate_resource_arn(&input.resource_arn, ctx, WhileDeleting::Serve).await?; let tags = ctx .storage diff --git a/crates/engine/src/ttl.rs b/crates/engine/src/ttl.rs index fac027d4b..ed1e509bc 100755 --- a/crates/engine/src/ttl.rs +++ b/crates/engine/src/ttl.rs @@ -226,6 +226,15 @@ fn storage_to_dynamo(e: StorageError) -> DynamoDbError { StorageError::TableNotActive(name) => { DynamoDbError::ResourceInUseException(format!("Table {name} is not in ACTIVE state")) } + // No TTL path raises this today (the TTL metadata paths answer + // TableNotActive for every non-ACTIVE status), but the variant exists + // and a backend that raises it here must not become a 500. + StorageError::TableBeingDeleted(name) => { + DynamoDbError::ResourceInUseException(extenddb_core::error::error_message( + extenddb_core::error::ErrorMessageKey::TableBeingDeleted, + &[&name], + )) + } other => { tracing::error!(internal_error = %other, "storage internal error"); DynamoDbError::InternalServerError("Internal server error".to_owned()) @@ -237,6 +246,25 @@ fn storage_to_dynamo(e: StorageError) -> DynamoDbError { mod tests { use super::*; + /// The TTL mapper renders a DELETING table with the service's sentence and + /// keeps the other non-ACTIVE statuses on theirs. + #[test] + fn a_deleting_table_has_its_own_sentence() { + match storage_to_dynamo(StorageError::TableBeingDeleted("t".to_owned())) { + DynamoDbError::ResourceInUseException(msg) => assert_eq!( + msg, + "Attempt to change a resource which is still in use: Table is being deleted: t" + ), + other => panic!("expected ResourceInUseException, got {other:?}"), + } + match storage_to_dynamo(StorageError::TableNotActive("t".to_owned())) { + DynamoDbError::ResourceInUseException(msg) => { + assert_eq!(msg, "Table t is not in ACTIVE state"); + } + other => panic!("expected ResourceInUseException, got {other:?}"), + } + } + #[test] fn valid_ttl_attribute_names() { assert!(validate_ttl_attribute_name("ttl").is_ok()); diff --git a/crates/engine/src/update_table.rs b/crates/engine/src/update_table.rs index 6db763f82..6cba5c610 100755 --- a/crates/engine/src/update_table.rs +++ b/crates/engine/src/update_table.rs @@ -321,6 +321,15 @@ fn update_table_err_to_dynamo( StorageError::TableNotActive(name) => { DynamoDbError::ResourceInUseException(format!("Table {name} is not in ACTIVE state")) } + // The service words a DELETING table differently from the other + // non-ACTIVE statuses (measured 2026-10-24); the sentence lives with + // the engine's other message templates. + StorageError::TableBeingDeleted(name) => { + DynamoDbError::ResourceInUseException(extenddb_core::error::error_message( + extenddb_core::error::ErrorMessageKey::TableBeingDeleted, + &[&name], + )) + } StorageError::IndexAlreadyExists(name) => DynamoDbError::ValidationException(format!( "One or more parameter values were invalid: Index already exists: {name}" )), @@ -500,4 +509,25 @@ mod tests { update_table_err_to_dynamo(StorageError::Internal("disk on fire".to_owned()), "t"); assert!(matches!(err, DynamoDbError::InternalServerError(_))); } + + /// UpdateTable on a DELETING table: the measured sentence (2026-10-24), + /// byte for byte, and the other non-ACTIVE statuses keep theirs. + #[test] + fn a_deleting_table_has_its_own_sentence() { + let err = update_table_err_to_dynamo(StorageError::TableBeingDeleted("t".to_owned()), "t"); + match err { + DynamoDbError::ResourceInUseException(msg) => assert_eq!( + msg, + "Attempt to change a resource which is still in use: Table is being deleted: t" + ), + other => panic!("expected ResourceInUseException, got {other:?}"), + } + let err = update_table_err_to_dynamo(StorageError::TableNotActive("t".to_owned()), "t"); + match err { + DynamoDbError::ResourceInUseException(msg) => { + assert_eq!(msg, "Table t is not in ACTIVE state"); + } + other => panic!("expected ResourceInUseException, got {other:?}"), + } + } } diff --git a/crates/server/src/key_info_cache.rs b/crates/server/src/key_info_cache.rs index ba527f659..ddd42040a 100644 --- a/crates/server/src/key_info_cache.rs +++ b/crates/server/src/key_info_cache.rs @@ -160,10 +160,11 @@ impl CachedTableKeyInfoStore { /// cached and is what the caller sees. /// /// Every backend's `table_key_info` answers `TableNotFound` both for a name -/// the catalog does not have and for one it has in a state other than -/// `ACTIVE` (a `CreateTable` or `RestoreTableFromBackup` target that is still -/// `CREATING`; the data plane reports both as `ResourceNotFoundException`, as -/// the service does). Caching the second as a negative entry made reads on a +/// the catalog does not have and for one it has as `CREATING` (a `CreateTable` +/// or `RestoreTableFromBackup` target; the data plane reports both as +/// `ResourceNotFoundException`, as the service does). PostgreSQL and SQLite +/// answer the key info for a `DELETING` row, since the service serves the data +/// plane until the table is gone. Caching the second as a negative entry made reads on a /// restored table fail for up to `negative_ttl` after `DescribeTable` reported /// it `ACTIVE`: a probe during the copy stored the entry, and nothing /// invalidated it at the transition (the service's first diff --git a/crates/storage-cassandra/src/delete_table.rs b/crates/storage-cassandra/src/delete_table.rs index 4a62507fc..eb89bb0b7 100644 --- a/crates/storage-cassandra/src/delete_table.rs +++ b/crates/storage-cassandra/src/delete_table.rs @@ -79,6 +79,17 @@ impl CassandraEngine { match caller { DeleteTableCaller::Client => { + // A DELETING row belongs to the deletion already under way + // (the abandoned-restore sweep sets it on this backend). The + // service refuses a second DeleteTable in that window as a + // table still in use (measured 2026-10-24); checked before + // deletion protection, as the SQL backends order it. + let status: String = + crate::cassandra_util::get_column(table_row, "table_status", "delete_table")?; + if status == "DELETING" { + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); + } + // Check deletion protection let deletion_protection: bool = table_row .get_r_by_name("deletion_protection_enabled") @@ -94,12 +105,11 @@ impl CassandraEngine { // A restore target is owned by its copy until the copy // activates it (or the sweep removes it). Deleting it mid-copy // would race the copy's writes into dropped tables. - let status: Option = table_row.get_by_name("table_status").ok().flatten(); let restoring_from: Option = table_row .get_by_name("restore_source_backup_arn") .ok() .flatten(); - if status.as_deref() == Some("CREATING") && restoring_from.is_some() { + if status == "CREATING" && restoring_from.is_some() { return Err(table_being_restored(&input.table_name)); } } diff --git a/crates/storage-cassandra/src/update_table.rs b/crates/storage-cassandra/src/update_table.rs index 4a20b8618..334f34eb0 100644 --- a/crates/storage-cassandra/src/update_table.rs +++ b/crates/storage-cassandra/src/update_table.rs @@ -41,6 +41,11 @@ impl CassandraEngine { let status: String = crate::cassandra_util::get_column(&row, "table_status", "update_table")?; + // A DELETING table has its own sentence on the service (measured + // 2026-10-24); every other non-ACTIVE status keeps the existing one. + if status == "DELETING" { + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); + } if status != "ACTIVE" { return Err(StorageError::TableNotActive(input.table_name.clone())); } diff --git a/crates/storage-cassandra/tests/direct/deleting_window.rs b/crates/storage-cassandra/tests/direct/deleting_window.rs new file mode 100644 index 000000000..cc43f7da4 --- /dev/null +++ b/crates/storage-cassandra/tests/direct/deleting_window.rs @@ -0,0 +1,176 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Direct tests for what the control plane answers while a table is +//! `DELETING`. +//! +//! Amazon DynamoDB answers a second `DeleteTable` and an `UpdateTable` in that +//! window with `ResourceInUseException` and the sentence `Attempt to change a +//! resource which is still in use: Table is being deleted: ` (measured +//! 2026-10-24). On this backend `DeleteTable` drops the data and the catalog +//! row in one call and never sets `DELETING` itself; the abandoned-restore +//! sweep does. The window is produced here by setting the row's status +//! directly. +//! +//! The catalog keyspace is shared by every test in this binary, and the +//! control-plane sweep (`process_control_plane_transitions`) removes any +//! `DELETING` row whose `status_transition_at` has passed, then clears the +//! table's TTL entries in the account keyspace. A fresh table carries its +//! activation deadline in that column, so a row left as `DELETING` with it +//! would be swept by whichever test runs the sweep next; after this test's +//! account keyspace is dropped, that sweep fails on the missing keyspace and +//! takes an unrelated test down with it. `set_status` therefore clears the +//! column, as activation does, which keeps the row out of the sweep, and each +//! test removes its row before it returns. + +use extenddb_core::types::{DeleteTableInput, UpdateTableInput}; +use extenddb_storage::TableEngine; +use extenddb_storage::error::StorageError; +use extenddb_storage_cassandra::CassandraEngine; + +use crate::helpers::{TestAccount, TestTable, setup_engine, test_config}; + +const TABLE: &str = "DeletingWindowTable"; + +/// Sets the row's status and clears its transition deadline, so the +/// control-plane sweep leaves the row alone for the rest of the test. +async fn set_status(engine: &CassandraEngine, account_id: &str, status: &str) { + let query = format!( + "UPDATE {}.tables SET table_status = ?, status_transition_at = null \ + WHERE account_id = ? AND table_name = ? IF EXISTS", + engine.catalog_keyspace() + ); + engine + .session() + .query_with_values(&query, cdrs_tokio::query_values!(status, account_id, TABLE)) + .await + .expect("set the table status"); +} + +/// Removes the catalog row the test set to a non-ACTIVE status; the account +/// keyspace goes with the `TestAccount`. +async fn remove_row(engine: &CassandraEngine, account_id: &str) { + let query = format!( + "DELETE FROM {}.tables WHERE account_id = ? AND table_name = ? IF EXISTS", + engine.catalog_keyspace() + ); + engine + .session() + .query_with_values(&query, cdrs_tokio::query_values!(account_id, TABLE)) + .await + .expect("remove the table row"); +} + +async fn status_of(engine: &CassandraEngine, account_id: &str) -> Option { + use cdrs_tokio::types::IntoRustByName; + let query = format!( + "SELECT table_status FROM {}.tables WHERE account_id = ? AND table_name = ?", + engine.catalog_keyspace() + ); + let rows = engine + .session() + .query_with_values(&query, cdrs_tokio::query_values!(account_id, TABLE)) + .await + .expect("read the row") + .response_body() + .expect("response body") + .into_rows() + .unwrap_or_default(); + rows.first() + .map(|row| row.get_r_by_name("table_status").expect("table_status")) +} + +fn update_input() -> UpdateTableInput { + UpdateTableInput { + table_name: TABLE.to_owned(), + billing_mode: None, + provisioned_throughput: None, + deletion_protection_enabled: Some(true), + global_secondary_index_updates: None, + attribute_definitions: None, + stream_specification: None, + table_class: None, + on_demand_throughput: None, + table_throughput_mode: None, + vector_index_updates: None, + } +} + +/// An engine, an account, an ACTIVE `TABLE`, and its row set to `DELETING`. +async fn open_window() -> (CassandraEngine, TestAccount, TestTable) { + let engine = setup_engine().await; + let account = TestAccount::new(&engine, &test_config().keyspace_prefix).await; + let table = TestTable::with_account(&engine, &account.account_id, TABLE, false).await; + set_status(&engine, &account.account_id, "DELETING").await; + (engine, account, table) +} + +/// A second DeleteTable during the window reports the table as being deleted, +/// not as absent, and does not run the drop: the row stays. +#[tokio::test] +async fn a_second_delete_table_during_the_window_is_table_being_deleted() { + if crate::helpers::skip_without_cassandra() { + return; + } + let (engine, account, _table) = open_window().await; + + let err = engine + .delete_table( + &account.account_id, + DeleteTableInput { + table_name: TABLE.to_owned(), + }, + ) + .await + .expect_err("DeleteTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == TABLE), + "expected TableBeingDeleted({TABLE:?}), got {err:?}" + ); + assert_eq!( + status_of(&engine, &account.account_id).await.as_deref(), + Some("DELETING") + ); + remove_row(&engine, &account.account_id).await; +} + +/// UpdateTable during the window reports the table as being deleted, not with +/// the sentence for the other non-ACTIVE statuses. +#[tokio::test] +async fn update_table_during_the_window_is_table_being_deleted() { + if crate::helpers::skip_without_cassandra() { + return; + } + let (engine, account, _table) = open_window().await; + + let err = engine + .update_table(&account.account_id, update_input()) + .await + .expect_err("UpdateTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == TABLE), + "expected TableBeingDeleted({TABLE:?}), got {err:?}" + ); + remove_row(&engine, &account.account_id).await; +} + +/// A CREATING table keeps the existing answer: the new sentence is for +/// DELETING only. +#[tokio::test] +async fn update_table_on_a_creating_table_keeps_table_not_active() { + if crate::helpers::skip_without_cassandra() { + return; + } + let (engine, account, _table) = open_window().await; + set_status(&engine, &account.account_id, "CREATING").await; + + let err = engine + .update_table(&account.account_id, update_input()) + .await + .expect_err("UpdateTable on a CREATING table"); + assert!( + matches!(&err, StorageError::TableNotActive(name) if name == TABLE), + "expected TableNotActive({TABLE:?}), got {err:?}" + ); + remove_row(&engine, &account.account_id).await; +} diff --git a/crates/storage-cassandra/tests/direct_integration.rs b/crates/storage-cassandra/tests/direct_integration.rs index 1df0244eb..8bdfcc65f 100644 --- a/crates/storage-cassandra/tests/direct_integration.rs +++ b/crates/storage-cassandra/tests/direct_integration.rs @@ -29,6 +29,8 @@ mod cassandra_engine; mod concurrent_account_creation; #[path = "direct/delete_item.rs"] mod delete_item; +#[path = "direct/deleting_window.rs"] +mod deleting_window; #[path = "direct/groups.rs"] mod groups; #[path = "direct/index.rs"] diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index 201f48333..1734a040b 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -3606,7 +3606,7 @@ impl From for TxErr { /// UnknownTransactionCommitResult label, under which the commit may /// already have applied. Commit errors are classified by /// `classify_commit_error` instead. -fn is_transient_write_conflict(e: &mongodb::error::Error) -> bool { +pub(crate) fn is_transient_write_conflict(e: &mongodb::error::Error) -> bool { if e.contains_label(mongodb::error::TRANSIENT_TRANSACTION_ERROR) { return true; } diff --git a/crates/storage-mongodb/src/deleting_window_tests.rs b/crates/storage-mongodb/src/deleting_window_tests.rs new file mode 100644 index 000000000..0d2940cc3 --- /dev/null +++ b/crates/storage-mongodb/src/deleting_window_tests.rs @@ -0,0 +1,658 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Storage-level tests for what the control plane answers while a table is +//! `DELETING`, against a live MongoDB replica set from +//! `EXTENDDB_TEST_MONGODB_URI`, for example +//! `mongodb://127.0.0.1:27105/?directConnection=true`. Without the variable +//! every test here reports a skip and passes, unless +//! `EXTENDDB_TEST_MONGODB_REQUIRED` is set, as in CI. Each test uses its own +//! catalog and data databases and drops them when it passes. +//! +//! Amazon DynamoDB answers a second `DeleteTable` and an `UpdateTable` in that +//! window with `ResourceInUseException` and the sentence `Attempt to change a +//! resource which is still in use: Table is being deleted: ` (measured +//! 2026-10-24). On this backend `DeleteTable` marks the document `DELETING` +//! and drops the collections in the same call, so the window is produced by +//! setting the document's status directly. Once the service accepts a +//! `DeleteTable` the table is removed, so a delete that stops after the mark +//! is finished by the call's own task or by the control plane, never left +//! `DELETING`. + +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use extenddb_core::types::{ + AttributeDefinition, BillingMode, CreateTableInput, DeleteTableInput, KeySchemaElement, + KeyType, ScalarAttributeType, TableDescription, Tag, UpdateTableInput, +}; +use extenddb_storage::error::StorageError; +use extenddb_storage::{MetadataEngine, TableEngine, WorkerStore}; +use mongodb::bson::{Bson, Document, doc}; + +use crate::MongoEngine; + +const ACCOUNT: &str = "123456789012"; +const TABLE: &str = "t_deleting_window"; + +/// The replica set URI, or `None` to skip. With +/// `EXTENDDB_TEST_MONGODB_REQUIRED` set a missing URI is a failure, so CI +/// cannot pass by skipping. +fn uri() -> Option { + let uri = std::env::var("EXTENDDB_TEST_MONGODB_URI") + .ok() + .filter(|u| !u.trim().is_empty()); + assert!( + uri.is_some() || std::env::var_os("EXTENDDB_TEST_MONGODB_REQUIRED").is_none(), + "EXTENDDB_TEST_MONGODB_REQUIRED is set but EXTENDDB_TEST_MONGODB_URI is not; the \ + MongoDB DELETING window tests must run, not skip" + ); + uri +} + +struct Fixture { + engine: MongoEngine, +} + +impl Fixture { + /// `None`, after printing why, when no replica set is named. + async fn new() -> Option { + let Some(uri) = uri() else { + eprintln!( + "SKIP: EXTENDDB_TEST_MONGODB_URI is not set, so there is no MongoDB replica set \ + to test against." + ); + return None; + }; + let tag = uuid::Uuid::new_v4().simple().to_string(); + let client = mongodb::Client::with_uri_str(uri).await.expect("client"); + let slots = crate::backup_engine::restore_server_slot_count(10); + let engine = MongoEngine { + client: client.clone(), + catalog_db: client.database(&format!("t{}_cat", &tag[..12])), + data_db: client.database(&format!("t{}_data", &tag[..12])), + region: "us-east-1".to_owned(), + gsi_cache: Arc::default(), + tx_read_concern: mongodb::options::ReadConcern::snapshot(), + backup_liveness: crate::backup_engine::BackupLiveness::default(), + restore_server_slot_count: slots, + restore_server_slots: Arc::new(tokio::sync::Semaphore::new(slots)), + restore_account_slots: crate::backup_engine::RestoreAccountSlots::default(), + }; + engine + .catalog_db + .collection::("settings") + .insert_one(doc! { "_id": "control_plane_delay_seconds", "value": "0" }) + .await + .expect("settings"); + Some(Self { engine }) + } + + /// Create `TABLE` and set its document to `DELETING`: the window. + async fn open_window(&self) { + self.engine + .create_table( + ACCOUNT, + CreateTableInput { + table_name: TABLE.to_owned(), + key_schema: vec![KeySchemaElement { + attribute_name: "pk".to_owned(), + key_type: KeyType::Hash, + }], + attribute_definitions: vec![AttributeDefinition { + attribute_name: "pk".to_owned(), + attribute_type: ScalarAttributeType::S, + }], + billing_mode: Some(BillingMode::PayPerRequest), + ..Default::default() + }, + ) + .await + .expect("create the table"); + self.set_status("DELETING").await; + } + + /// Create `TABLE` with a GSI and a stream, as the table that stalled a + /// delete was, and a tag on it. Returns the description and the GSI's + /// index ids. + async fn create_full_table(&self) -> (TableDescription, Vec) { + let input: CreateTableInput = serde_json::from_value(serde_json::json!({ + "TableName": TABLE, + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "AttributeDefinitions": [ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "gk", "AttributeType": "S"} + ], + "GlobalSecondaryIndexes": [{ + "IndexName": "gsi1", + "KeySchema": [{"AttributeName": "gk", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"} + }], + "StreamSpecification": {"StreamEnabled": true, "StreamViewType": "NEW_AND_OLD_IMAGES"}, + "BillingMode": "PAY_PER_REQUEST" + })) + .expect("create input"); + let desc = self + .engine + .create_table(ACCOUNT, input) + .await + .expect("create the table"); + self.engine + .tag_resource( + &desc.table_arn, + &[Tag { + key: "k".to_owned(), + value: "v".to_owned(), + }], + ) + .await + .expect("tag the table"); + let index_ids = self + .engine + .catalog_db + .collection::("indexes") + .distinct("index_id", doc! { "_id.table_id": &desc.table_id }) + .await + .expect("read the index ids") + .into_iter() + .filter_map(|id| id.as_str().map(str::to_owned)) + .collect::>(); + assert_eq!(index_ids.len(), 1, "one GSI"); + assert!( + self.leftovers(&desc, &index_ids).await.len() == 6, + "precondition: the document, both collections, the index document, the tag and \ + the stream shards exist" + ); + (desc, index_ids) + } + + /// What of the table `desc` describes is still in the databases: its + /// document, its data and index collections, its index documents, its + /// tags and its stream shards. + async fn leftovers(&self, desc: &TableDescription, index_ids: &[String]) -> Vec { + let mut left = Vec::new(); + if self.status().await.is_some() { + left.push("the table document".to_owned()); + } + let collections = self + .engine + .data_db + .list_collection_names() + .await + .expect("list the data collections"); + for id in std::iter::once(&desc.table_id).chain(index_ids) { + let name = crate::data::data_collection_name(id); + if collections.contains(&name) { + left.push(name); + } + } + let counts = [ + ( + "index documents", + &self.engine.catalog_db, + "indexes", + doc! { "_id.table_id": &desc.table_id }, + ), + ( + "tags", + &self.engine.catalog_db, + "tags", + doc! { "resource_arn": &desc.table_arn }, + ), + ( + "stream shards", + &self.engine.data_db, + "stream_shards", + doc! { "table_id": &desc.table_id }, + ), + ]; + for (what, db, coll, filter) in counts { + let n = db + .collection::(coll) + .count_documents(filter) + .await + .expect("count documents"); + if n > 0 { + left.push(format!("{n} {what}")); + } + } + left + } + + async fn set_status(&self, status: &str) { + self.engine + .catalog_db + .collection::("tables") + .update_one( + doc! { "_id": { "account_id": ACCOUNT, "table_name": TABLE } }, + doc! { "$set": { "table_status": status } }, + ) + .await + .expect("set the table status"); + } + + /// Mark the document `DELETING` with `due` as its transition time, as a + /// `DeleteTable` that stopped after its mark leaves it. + async fn leave_deleting(&self, due: Bson) { + self.engine + .catalog_db + .collection::("tables") + .update_one( + doc! { "_id": { "account_id": ACCOUNT, "table_name": TABLE } }, + doc! { "$set": { "table_status": "DELETING", "status_transition_at": due } }, + ) + .await + .expect("mark the table DELETING"); + } + + async fn status(&self) -> Option { + self.engine + .catalog_db + .collection::("tables") + .find_one(doc! { "_id": { "account_id": ACCOUNT, "table_name": TABLE } }) + .await + .expect("read the table document") + .map(|d| d.get_str("table_status").expect("table_status").to_owned()) + } + + async fn finish(self) { + self.engine.catalog_db.drop().await.expect("drop catalog"); + self.engine.data_db.drop().await.expect("drop data"); + } +} + +fn delete_input() -> DeleteTableInput { + DeleteTableInput { + table_name: TABLE.to_owned(), + } +} + +/// A `status_transition_at` `seconds` from now (negative for the past). +fn transition_in(seconds: i64) -> Bson { + Bson::DateTime(mongodb::bson::DateTime::from_millis( + mongodb::bson::DateTime::now().timestamp_millis() + seconds * 1000, + )) +} + +fn update_input() -> UpdateTableInput { + UpdateTableInput { + table_name: TABLE.to_owned(), + billing_mode: None, + provisioned_throughput: None, + deletion_protection_enabled: Some(true), + global_secondary_index_updates: None, + attribute_definitions: None, + stream_specification: None, + table_class: None, + on_demand_throughput: None, + table_throughput_mode: None, + vector_index_updates: None, + } +} + +/// A second DeleteTable during the window reports the table as being deleted, +/// not as absent, and does not run the drop again: the document stays. +#[tokio::test] +async fn a_second_delete_table_during_the_window_is_table_being_deleted() { + let Some(f) = Fixture::new().await else { + return; + }; + f.open_window().await; + + let err = f + .engine + .delete_table(ACCOUNT, delete_input()) + .await + .expect_err("DeleteTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == TABLE), + "expected TableBeingDeleted({TABLE:?}), got {err:?}" + ); + assert_eq!(f.status().await.as_deref(), Some("DELETING")); + + f.finish().await; +} + +/// UpdateTable during the window reports the table as being deleted, not with +/// the sentence for the other non-ACTIVE statuses, and changes nothing. +#[tokio::test] +async fn update_table_during_the_window_is_table_being_deleted() { + let Some(f) = Fixture::new().await else { + return; + }; + f.open_window().await; + + let err = f + .engine + .update_table(ACCOUNT, update_input()) + .await + .expect_err("UpdateTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == TABLE), + "expected TableBeingDeleted({TABLE:?}), got {err:?}" + ); + let doc = f + .engine + .catalog_db + .collection::("tables") + .find_one(doc! { "_id": { "account_id": ACCOUNT, "table_name": TABLE } }) + .await + .expect("read the table document") + .expect("the document is still there"); + assert_ne!( + doc.get_bool("deletion_protection_enabled").ok(), + Some(true), + "a refused UpdateTable must not have written" + ); + + f.finish().await; +} + +/// A CREATING table keeps the existing answer: the new sentence is for +/// DELETING only. +#[tokio::test] +async fn update_table_on_a_creating_table_keeps_table_not_active() { + let Some(f) = Fixture::new().await else { + return; + }; + f.open_window().await; + f.set_status("CREATING").await; + + let err = f + .engine + .update_table(ACCOUNT, update_input()) + .await + .expect_err("UpdateTable on a CREATING table"); + assert!( + matches!(&err, StorageError::TableNotActive(name) if name == TABLE), + "expected TableNotActive({TABLE:?}), got {err:?}" + ); + + f.finish().await; +} + +/// A DeleteTable whose caller gives up while a drop waits (a client read +/// timeout) still finishes the delete once the drop can run, with no further +/// call: the steps after the mark run in a task the request only awaits. A +/// transaction holds the data collection, so the drop waits for it, and the +/// call is dropped after 2 s, as an HTTP client's timeout drops the request. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_delete_table_whose_caller_gives_up_still_finishes() { + let Some(f) = Fixture::new().await else { + return; + }; + let (desc, index_ids) = f.create_full_table().await; + + let mut session = f.engine.client.start_session().await.expect("session"); + session + .start_transaction() + .await + .expect("start a transaction"); + f.engine + .data_db + .collection::(&crate::data::data_collection_name(&desc.table_id)) + .insert_one(doc! { "_id": "held", "pk": "held" }) + .session(&mut session) + .await + .expect("write in the transaction"); + + let gave_up = tokio::time::timeout( + Duration::from_secs(2), + f.engine.delete_table(ACCOUNT, delete_input()), + ) + .await; + assert!( + gave_up.is_err(), + "the drop waits for the transaction, so the call is still running at 2 s: {gave_up:?}" + ); + assert_eq!(f.status().await.as_deref(), Some("DELETING")); + + session + .abort_transaction() + .await + .expect("end the transaction"); + let deadline = Instant::now() + Duration::from_secs(30); + loop { + let left = f.leftovers(&desc, &index_ids).await; + if left.is_empty() { + break; + } + assert!( + Instant::now() < deadline, + "30 s after the transaction ended, with no further call, these remain: {left:?}" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } + + f.finish().await; +} + +/// The control plane finishes a delete whose DeleteTable stopped after the +/// mark (a step that failed, a process that died): the document, the +/// collections, the index documents, the tags and the stream shards all go, +/// and the name can be created again. A transition time in the past is one a +/// stopped DeleteTable leaves once `DELETE_FINISH_AFTER` has passed; none at +/// all is what a build before this one left. +#[tokio::test] +async fn the_control_plane_finishes_a_delete_left_deleting() { + for due in [transition_in(-1), Bson::Null] { + let Some(f) = Fixture::new().await else { + return; + }; + let (desc, index_ids) = f.create_full_table().await; + f.leave_deleting(due.clone()).await; + + let transitions = f + .engine + .process_control_plane_transitions() + .await + .expect("control plane pass"); + assert_eq!( + transitions, + [(TABLE.to_owned(), "DELETING → deleted")], + "due {due:?}" + ); + let left = f.leftovers(&desc, &index_ids).await; + assert!(left.is_empty(), "due {due:?}: these remain: {left:?}"); + f.create_full_table().await; + + f.finish().await; + } +} + +/// Two runs of one delete can overlap (the request's task and a control-plane +/// pass), so one can reach its last steps after the other has removed the +/// document and the name has been created again. That late run leaves the new +/// table whole, its tag included: tags are keyed by the ARN, which the new +/// table shares. +#[tokio::test] +async fn a_late_second_run_of_a_delete_leaves_a_new_table_of_the_name_whole() { + let Some(f) = Fixture::new().await else { + return; + }; + let (old, old_index_ids) = f.create_full_table().await; + f.leave_deleting(transition_in(60)).await; + + f.engine + .finish_table_delete(ACCOUNT, TABLE, &old.table_id, &old.table_arn) + .await + .expect("the first run"); + let left = f.leftovers(&old, &old_index_ids).await; + assert!( + left.is_empty(), + "after the first run these remain: {left:?}" + ); + + let (new, new_index_ids) = f.create_full_table().await; + assert_eq!( + new.table_arn, old.table_arn, + "precondition: the ARN is shared" + ); + f.engine + .finish_table_delete(ACCOUNT, TABLE, &old.table_id, &old.table_arn) + .await + .expect("the late second run"); + let left = f.leftovers(&new, &new_index_ids).await; + assert_eq!( + left.len(), + 6, + "the new table's document, collections, index document, tag and stream shards \ + stay: {left:?}" + ); + assert_eq!(f.status().await.as_deref(), Some("ACTIVE")); + + f.finish().await; +} + +/// A delete whose transition time is still ahead belongs to a DeleteTable +/// that may still be running, and the control plane leaves it alone. +#[tokio::test] +async fn the_control_plane_leaves_a_delete_that_is_not_due() { + let Some(f) = Fixture::new().await else { + return; + }; + let (desc, index_ids) = f.create_full_table().await; + f.leave_deleting(transition_in(60)).await; + + let transitions = f + .engine + .process_control_plane_transitions() + .await + .expect("control plane pass"); + assert!(transitions.is_empty(), "{transitions:?}"); + assert_eq!(f.status().await.as_deref(), Some("DELETING")); + assert_eq!(f.leftovers(&desc, &index_ids).await.len(), 6); + + f.finish().await; +} + +/// Two concurrent DeleteTable calls on one ACTIVE table: the conditioned mark +/// matches for one of them, the other answers TableBeingDeleted (the service's +/// answer for the second of two concurrent calls, measured 2026-10-25). Ten +/// rounds, a fresh table each; afterwards no catalog document and no data +/// collection for the table remain. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn two_concurrent_delete_tables_have_one_winner() { + let Some(f) = Fixture::new().await else { + return; + }; + let engine = Arc::new(f.engine.clone()); + + for round in 0..10 { + let table = format!("t_pair_{round}"); + let desc = engine + .create_table( + ACCOUNT, + CreateTableInput { + table_name: table.clone(), + key_schema: vec![KeySchemaElement { + attribute_name: "pk".to_owned(), + key_type: KeyType::Hash, + }], + attribute_definitions: vec![AttributeDefinition { + attribute_name: "pk".to_owned(), + attribute_type: ScalarAttributeType::S, + }], + billing_mode: Some(BillingMode::PayPerRequest), + ..Default::default() + }, + ) + .await + .expect("create the table"); + + let tasks: Vec<_> = (0..2) + .map(|_| { + let engine = Arc::clone(&engine); + let table = table.clone(); + tokio::spawn(async move { + engine + .delete_table(ACCOUNT, DeleteTableInput { table_name: table }) + .await + }) + }) + .collect(); + let mut ok = 0; + let mut being_deleted = 0; + for task in tasks { + match task.await.expect("task") { + Ok(_) => ok += 1, + Err(StorageError::TableBeingDeleted(name)) if name == table => being_deleted += 1, + Err(other) => panic!("round {round}: unexpected {other:?}"), + } + } + assert_eq!((ok, being_deleted), (1, 1), "round {round}"); + + let doc = engine + .catalog_db + .collection::("tables") + .find_one(doc! { "_id": { "account_id": ACCOUNT, "table_name": &table } }) + .await + .expect("read the table document"); + assert!(doc.is_none(), "round {round}: the catalog document is gone"); + let collections = engine + .data_db + .list_collection_names() + .await + .expect("list collections"); + let data_coll = crate::data::data_collection_name(&desc.table_id); + assert!( + !collections.contains(&data_coll), + "round {round}: the data collection is gone" + ); + } + + drop(engine); + f.finish().await; +} + +/// A run whose last step meets another run removing the document (a write +/// conflict on the transaction) answers `Ok`, not an internal error, and +/// leaves the document to that run. The other run is held inside its +/// transaction, with the document and the tag deleted and not yet committed. +#[tokio::test] +async fn a_run_that_meets_another_removing_the_document_leaves_it_to_that_run() { + let Some(f) = Fixture::new().await else { + return; + }; + let (desc, index_ids) = f.create_full_table().await; + f.leave_deleting(transition_in(60)).await; + + let mut other = f.engine.client.start_session().await.expect("session"); + other + .start_transaction() + .await + .expect("start a transaction"); + f.engine + .catalog_db + .collection::("tables") + .delete_one(doc! { + "_id": { "account_id": ACCOUNT, "table_name": TABLE }, + "table_id": &desc.table_id, + "table_status": "DELETING", + }) + .session(&mut other) + .await + .expect("the other run deletes the document"); + f.engine + .catalog_db + .collection::("tags") + .delete_many(doc! { "resource_arn": &desc.table_arn }) + .session(&mut other) + .await + .expect("the other run deletes the tag"); + + f.engine + .finish_table_delete(ACCOUNT, TABLE, &desc.table_id, &desc.table_arn) + .await + .expect("a write conflict leaves the document to the other run"); + assert_eq!(f.status().await.as_deref(), Some("DELETING")); + + other + .commit_transaction() + .await + .expect("the other run commits"); + let left = f.leftovers(&desc, &index_ids).await; + assert!(left.is_empty(), "these remain: {left:?}"); + + f.finish().await; +} diff --git a/crates/storage-mongodb/src/table_engine.rs b/crates/storage-mongodb/src/table_engine.rs index 77706c3c7..86851382e 100644 --- a/crates/storage-mongodb/src/table_engine.rs +++ b/crates/storage-mongodb/src/table_engine.rs @@ -576,6 +576,14 @@ impl MongoEngine { .describe_table_impl(account_id, &input.table_name) .await?; + // The document stays, marked DELETING, while the collections drop + // below. A second DeleteTable in that window is refused the way the + // service refuses it (measured 2026-10-24), before the deletion + // protection check, as the SQL backends order it. + if desc.table_status == TableStatus::Deleting { + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); + } + // Check deletion protection if desc.deletion_protection_enabled { return Err(StorageError::DeletionProtected(input.table_name.clone())); @@ -592,19 +600,84 @@ impl MongoEngine { ))); } - // Mark as DELETING + // Mark as DELETING, conditioned on the document still being the one + // described above and not yet DELETING. Two concurrent DeleteTable + // calls both read the ACTIVE document; only one update matches. The + // table id in the filter keeps a stalled call from marking a table + // created into the same name after this one was gone. The mark also + // sets the time from which the control plane finishes the delete if + // this call stops before it does (`worker_store.rs`). let tables_coll = self.catalog_db.collection::("tables"); - tables_coll + let marked = tables_coll .update_one( - doc! { "_id": { "account_id": account_id, "table_name": &input.table_name } }, - doc! { "$set": { "table_status": "DELETING" } }, + doc! { + "_id": { "account_id": account_id, "table_name": &input.table_name }, + "table_id": &desc.table_id, + "table_status": { "$ne": "DELETING" }, + }, + doc! { "$set": { + "table_status": "DELETING", + "status_transition_at": crate::worker_store::delete_finish_due(), + } }, ) .await .map_err(|e| StorageError::Internal(e.to_string()))?; + if marked.matched_count == 0 { + // The document this call read is now DELETING or already removed, + // and only a concurrent DeleteTable does either: this call lost the + // race. The service answers the second of two concurrent calls on + // an ACTIVE table with the DELETING sentence (measured 2026-10-25, + // five rounds of five), whether or not the first has finished by + // the time the second looks again. + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); + } + + // The rest of the delete runs in a task of its own, which the request + // awaits. A client that gives up on the call (a read timeout, a closed + // connection) drops the request future, and dropping it detaches the + // task rather than cancelling it, so a delete stalled behind a lock + // still finishes once the lock is released. A step that fails, or a + // process that dies, leaves the document DELETING for the control + // plane to finish. + let engine = self.clone(); + let (account, name) = (account_id.to_owned(), input.table_name.clone()); + let (table_id, table_arn) = (desc.table_id.clone(), desc.table_arn.clone()); + tokio::spawn(async move { + engine + .finish_table_delete(&account, &name, &table_id, &table_arn) + .await + }) + .await + .map_err(|e| StorageError::Internal(e.to_string()))??; + + Ok(TableDescription { + table_status: TableStatus::Deleting, + ..desc + }) + } + /// The steps of a `DeleteTable` after its document is marked `DELETING`: + /// drop the data and index collections, remove the stream state, then + /// remove the document and the tags together. `DeleteTable` runs them in + /// the request, and the control plane runs them for a document a + /// `DeleteTable` left `DELETING` (`worker_store.rs`). Two runs of one + /// delete can overlap: the request's task, still waiting on a drop when + /// the mark's due time passes, and a control-plane pass. Each step is + /// idempotent and keyed by the table id, except the tags, which are keyed + /// by the ARN and go only with this table id's `DELETING` document. A + /// second run, alongside the first or after a table was created into the + /// same name, removes what is left of this table and nothing of the new + /// one. + pub(crate) async fn finish_table_delete( + &self, + account_id: &str, + table_name: &str, + table_id: &str, + table_arn: &str, + ) -> Result<(), StorageError> { // Drop the data collection - let coll_name = data_collection_name(&desc.table_id); - self.drop_collection_if_exists(&coll_name).await?; + self.drop_collection_if_exists(&data_collection_name(table_id)) + .await?; // The table is already DELETING, so data-plane readers are rejected; // unlike individual index deletion, physical cleanup can safely occur @@ -613,38 +686,71 @@ impl MongoEngine { // metadata. The index documents are stored separately from the table // document, so dropping the base collection alone does not remove // `_ddb_` collections. - self.drop_index_collections_for_table(&desc.table_id) - .await?; - - // Tags are keyed by the table ARN rather than table_id. Remove them - // before deleting the table metadata so a table recreated with the - // same name cannot inherit the previous table's tags. - self.catalog_db - .collection::("tags") - .delete_many(doc! { "resource_arn": &desc.table_arn }) - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + self.drop_index_collections_for_table(table_id).await?; - self.gsi_cache_invalidate(&desc.table_id); + self.gsi_cache_invalidate(table_id); // Delete stream_shards, stream_records, and their sequence counters // for this table. Prevents a table recreated with the same name // (which will get a fresh table_id) from inheriting the deleted // table's stream history. RFC-0003 §8.2 (table-name reuse). - self.cleanup_stream_state_for_table(&desc.table_id).await?; - - // Delete the table metadata - tables_coll - .delete_one( - doc! { "_id": { "account_id": account_id, "table_name": &input.table_name } }, - ) + self.cleanup_stream_state_for_table(table_id).await?; + + // Remove the document and the tags in one transaction, so no tag + // outlives the document, and remove the tags only when this call + // removed this table id's DELETING document: tags are keyed by the + // ARN, which a table created into the same name shares. A write + // conflict aborts the transaction with nothing removed. The writer it + // met is another run of these steps, claiming or removing the + // document, so the document is left to that run, or to the control + // plane if it stays DELETING. + match self + .remove_deleting_document(account_id, table_name, table_id, table_arn) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + { + Ok(()) => Ok(()), + Err(e) if crate::data_engine::is_transient_write_conflict(&e) => Ok(()), + Err(e) => Err(StorageError::Internal(e.to_string())), + } + } - Ok(TableDescription { - table_status: TableStatus::Deleting, - ..desc - }) + /// The last step of [`Self::finish_table_delete`]: in one transaction, + /// delete this table id's `DELETING` document and, only when that + /// removed it, the tags of `table_arn`. + async fn remove_deleting_document( + &self, + account_id: &str, + table_name: &str, + table_id: &str, + table_arn: &str, + ) -> mongodb::error::Result<()> { + let mut session = self.client.start_session().await?; + session + .start_transaction() + .with_options( + mongodb::options::TransactionOptions::builder() + .write_concern(crate::majority_write_concern(self.client.write_concern())) + .build(), + ) + .await?; + let removed = self + .catalog_db + .collection::("tables") + .delete_one(doc! { + "_id": { "account_id": account_id, "table_name": table_name }, + "table_id": table_id, + "table_status": "DELETING", + }) + .session(&mut session) + .await?; + if removed.deleted_count == 1 { + self.catalog_db + .collection::("tags") + .delete_many(doc! { "resource_arn": table_arn }) + .session(&mut session) + .await?; + } + session.commit_transaction().await } pub(crate) async fn describe_table_impl( @@ -1132,9 +1238,10 @@ impl MongoEngine { } /// `TableNotFound` when no catalog document matches `filter`, - /// `TableNotActive` when one does but its `table_status` is not + /// `TableBeingDeleted` when one does and its `table_status` is + /// `DELETING`, `TableNotActive` when it is any other status but /// `ACTIVE`. UpdateTable's guard; also used after a conditional write - /// matched nothing to report which of the two happened. + /// matched nothing to report which of the three happened. async fn require_active_table( &self, tables_coll: &mongodb::Collection, @@ -1146,10 +1253,11 @@ impl MongoEngine { .await .map_err(|e| StorageError::Internal(e.to_string()))? .ok_or_else(|| StorageError::TableNotFound(table_name.to_owned()))?; - if table_doc.get_str("table_status").unwrap_or("ACTIVE") != "ACTIVE" { - return Err(StorageError::TableNotActive(table_name.to_owned())); + match table_doc.get_str("table_status").unwrap_or("ACTIVE") { + "ACTIVE" => Ok(()), + "DELETING" => Err(StorageError::TableBeingDeleted(table_name.to_owned())), + _ => Err(StorageError::TableNotActive(table_name.to_owned())), } - Ok(()) } pub(crate) async fn table_key_info_impl( @@ -1861,6 +1969,10 @@ fn active_table_filter(filter: &Document) -> Document { active } +#[cfg(test)] +#[path = "deleting_window_tests.rs"] +mod deleting_window_tests; + #[cfg(test)] mod tests { use super::active_table_filter; diff --git a/crates/storage-mongodb/src/ttl_worker.rs b/crates/storage-mongodb/src/ttl_worker.rs index 9d309f575..d30bdcee7 100644 --- a/crates/storage-mongodb/src/ttl_worker.rs +++ b/crates/storage-mongodb/src/ttl_worker.rs @@ -563,9 +563,9 @@ fn build_ttl_condition( } /// Background poller that flips tables out of the transient `CREATING` state -/// once their scheduled `status_transition_at` has passed. See -/// [`crate::worker_store`] for how rows enter `CREATING`. Returns once -/// `shutdown` fires. +/// once their scheduled `status_transition_at` has passed, and finishes the +/// deletes a `DeleteTable` left `DELETING`. See [`crate::worker_store`] for how +/// rows enter either state. Returns once `shutdown` fires. pub(crate) async fn control_plane_worker(storage: Arc, shutdown: CancellationToken) { while sleep_or_shutdown(&shutdown, CONTROL_PLANE_POLL_INTERVAL).await { match WorkerStore::process_control_plane_transitions(&*storage).await { diff --git a/crates/storage-mongodb/src/worker_store.rs b/crates/storage-mongodb/src/worker_store.rs index 4b51bf6bf..b5d54bb17 100644 --- a/crates/storage-mongodb/src/worker_store.rs +++ b/crates/storage-mongodb/src/worker_store.rs @@ -12,16 +12,22 @@ //! When `control_plane_delay_seconds` is 0 the create/restore paths write //! `ACTIVE` directly and this worker has nothing to do. //! -//! `DeleteTable` remains inline (the catalog row and collections are removed in -//! the request handler), so there is no `DELETING` transient state to reconcile -//! here. GSI create is handled separately by -//! [`ttl_worker::gsi_backfill_worker`] on the `indexes` catalog collection. +//! `DeleteTable` runs inline: it marks the document `DELETING`, then drops the +//! collections and removes the document in the request. A `DeleteTable` that +//! stops in between (a step that fails, a process that dies) leaves the +//! document `DELETING`, and a second `DeleteTable` is refused as the service +//! refuses it, so this worker finishes such a delete once +//! [`DELETE_FINISH_AFTER`] has passed since the mark. GSI create is handled +//! separately by [`ttl_worker::gsi_backfill_worker`] on the `indexes` catalog +//! collection. //! //! [`ttl_worker::gsi_backfill_worker`]: crate::ttl_worker::gsi_backfill_worker +use std::time::Duration; + use futures::TryStreamExt; use futures::future::BoxFuture; -use mongodb::bson::{Document, doc}; +use mongodb::bson::{Bson, Document, doc}; use extenddb_storage::WorkerStore; use extenddb_storage::error::StorageError; @@ -32,6 +38,31 @@ use crate::MongoEngine; /// unparseable: the workers and the console's default read this one value. pub(crate) const DEFAULT_CONTROL_PLANE_DELAY_SECONDS: f64 = 0.25; +/// How long after a `DeleteTable` marks a document `DELETING` the control plane +/// finishes the delete itself, and how long it waits before it tries again +/// after a pass that could not finish one. +/// +/// `DeleteTable` finishes the delete in the request, in milliseconds when +/// nothing holds the collections. The longest wait among its steps is a +/// collection drop behind a client transaction that holds the collection, +/// and MongoDB aborts a transaction once it is older than +/// `transactionLifetimeLimitSeconds`, 60 seconds by default. A document still +/// `DELETING` this long after its mark belongs to a `DeleteTable` that +/// stopped. A `DeleteTable` still running then runs the same idempotent steps +/// as the control plane ([`MongoEngine::finish_table_delete`]). +pub(crate) const DELETE_FINISH_AFTER: Duration = Duration::from_secs(60); + +/// The `status_transition_at` a `DELETING` mark made now carries: the time +/// from which the control plane finishes the delete. +pub(crate) fn delete_finish_due() -> mongodb::bson::DateTime { + let after = i64::try_from(DELETE_FINISH_AFTER.as_millis()).unwrap_or(i64::MAX); + mongodb::bson::DateTime::from_millis( + mongodb::bson::DateTime::now() + .timestamp_millis() + .saturating_add(after), + ) +} + impl MongoEngine { /// Read `control_plane_delay_seconds` from the settings collection, /// falling back to the default. A value <= 0 means "no CREATING window" @@ -47,6 +78,84 @@ impl MongoEngine { .filter(|v| *v >= 0.0) .unwrap_or(DEFAULT_CONTROL_PLANE_DELAY_SECONDS) } + + /// Finish every delete whose `DELETING` document is due + /// ([`DELETE_FINISH_AFTER`] after its mark, or at once for a document + /// with no transition time, which a build before this one wrote). Each + /// document is first claimed by moving its transition time forward, + /// conditioned on the value read, so that of several control-plane + /// passes, in one process or several, one claims a document at a time; a + /// `DeleteTable` whose own task is still running then runs the same steps + /// alongside it. The claim also makes a step that keeps failing retry + /// once per [`DELETE_FINISH_AFTER`] rather than at every poll. A failed + /// step is logged and leaves the document `DELETING`; it does not stop + /// the pass. Returns the names of the tables removed. + async fn finish_due_deletes(&self) -> Result, StorageError> { + let tables_coll = self.catalog_db.collection::("tables"); + let due: Vec = tables_coll + .find(doc! { + "table_status": "DELETING", + "$or": [ + { "status_transition_at": { "$lte": mongodb::bson::DateTime::now() } }, + { "status_transition_at": Bson::Null }, + ], + }) + .await + .map_err(|e| StorageError::Internal(e.to_string()))? + .try_collect() + .await + .map_err(|e| StorageError::Internal(e.to_string()))?; + + let mut removed = Vec::new(); + for table_doc in due { + let (Ok(id), Ok(table_id), Ok(table_arn)) = ( + table_doc.get_document("_id"), + table_doc.get_str("table_id"), + table_doc.get_str("table_arn"), + ) else { + tracing::warn!("a DELETING table document lacks _id, table_id or table_arn"); + continue; + }; + let (Ok(account_id), Ok(table_name)) = + (id.get_str("account_id"), id.get_str("table_name")) + else { + tracing::warn!("a DELETING table document's _id lacks account_id or table_name"); + continue; + }; + let read_due = match table_doc.get("status_transition_at") { + Some(Bson::DateTime(at)) => Bson::DateTime(*at), + _ => Bson::Null, + }; + let claimed = tables_coll + .update_one( + doc! { + "_id": id.clone(), + "table_id": table_id, + "table_status": "DELETING", + "status_transition_at": read_due, + }, + doc! { "$set": { "status_transition_at": delete_finish_due() } }, + ) + .await + .map_err(|e| StorageError::Internal(e.to_string()))?; + if claimed.matched_count == 0 { + // Another pass claimed it, or a DeleteTable finished it. + continue; + } + match self + .finish_table_delete(account_id, table_name, table_id, table_arn) + .await + { + Ok(()) => removed.push(table_name.to_owned()), + Err(e) => tracing::warn!( + "table '{table_name}' ({table_id}) stays DELETING; the control plane \ + tries again in {} s: {e}", + DELETE_FINISH_AFTER.as_secs() + ), + } + } + Ok(removed) + } } impl WorkerStore for MongoEngine { @@ -101,6 +210,11 @@ impl WorkerStore for MongoEngine { transitions.push((table_name, "CREATING → active")); } + // DELETING → removed: deletes a DeleteTable left unfinished. + for name in self.finish_due_deletes().await? { + transitions.push((name, "DELETING → deleted")); + } + Ok(transitions) }) } diff --git a/crates/storage-postgres/src/backup_engine.rs b/crates/storage-postgres/src/backup_engine.rs index 6d93da862..8c0bbb38a 100755 --- a/crates/storage-postgres/src/backup_engine.rs +++ b/crates/storage-postgres/src/backup_engine.rs @@ -698,10 +698,16 @@ impl PostgresEngine { /// exactly one wins. The claim also schedules the row for the ordinary /// DELETING removal, so if a drop below fails, the row stays DELETING with /// its index rows and the control plane finishes the job later. + /// + /// The claim's transition time is the epoch, a time no `DeleteTable` mark + /// carries, since `DeleteTable` schedules the removal at or after the time + /// it runs. The data plane serves a table `DeleteTable` marked until it is + /// gone, and reads a row with this transition time as the restore target it + /// was, which it never served (`DATA_PLANE_STATUS` in `data/ddl.rs`). async fn abort_restore(&self, table_id: &str) -> Result { let mut tx = self.pool.begin().await.map_err(db_err)?; let claimed = sqlx::query( - "UPDATE tables SET table_status = 'DELETING', status_transition_at = NOW() \ + "UPDATE tables SET table_status = 'DELETING', status_transition_at = 'epoch' \ WHERE table_id = $1 AND table_status = 'CREATING' AND status_transition_at IS NULL", ) .bind(table_id) diff --git a/crates/storage-postgres/src/data/ddl.rs b/crates/storage-postgres/src/data/ddl.rs index 789447fd4..cd25b0758 100755 --- a/crates/storage-postgres/src/data/ddl.rs +++ b/crates/storage-postgres/src/data/ddl.rs @@ -29,6 +29,15 @@ use super::{all_sort_key_info, data_table_name, index_table_name, vector_table_n /// on these columns. Their results are still ordered correctly, because the /// predicates carry the collation explicitly; only the index use differs. const TEXT_KEY: &str = "TEXT COLLATE \"C\""; + +/// The status the data plane classifies a `tables` row by. The restore sweep +/// claims an abandoned restore target by marking it `DELETING` with the epoch +/// as its transition time (`abort_restore`), a time no `DeleteTable` mark +/// carries. That row is read as the `CREATING` restore target it was, which +/// the data plane answers as not found, as the service answers a table it is +/// still restoring; a row `DeleteTable` marked is served until it is gone. +const DATA_PLANE_STATUS: &str = "CASE WHEN table_status = 'DELETING' \ + AND status_transition_at = 'epoch' THEN 'CREATING' ELSE table_status END"; use crate::PostgresEngine; /// Row shape returned by the table-info query: (`key_schema`, `attr_defs`, status, `table_id`, `stream_spec`). @@ -371,12 +380,12 @@ impl PostgresEngine { account_id: &str, table_name: &str, ) -> Result { - let row: Option = sqlx::query_as( - "SELECT key_schema, attribute_definitions, table_status, table_id, \ + let row: Option = sqlx::query_as(&format!( + "SELECT key_schema, attribute_definitions, {DATA_PLANE_STATUS}, table_id, \ stream_specification \ FROM tables \ - WHERE account_id = $1 AND table_name = $2", - ) + WHERE account_id = $1 AND table_name = $2" + )) .bind(account_id) .bind(table_name) .fetch_optional(&self.pool) @@ -387,11 +396,17 @@ impl PostgresEngine { row.ok_or_else(|| StorageError::TableNotFound(table_name.to_owned()))?; match status.as_str() { - "ACTIVE" => {} - // A table that is still being created, or is being deleted, is not - // usable for data-plane operations; DynamoDB reports it as not found - // (not as in-use). UPDATING keeps the not-active classification. - "CREATING" | "DELETING" => { + // DynamoDB serves reads and writes on a DELETING table until the + // table is gone (measured 2026-10-25); a write made in that window + // goes away with the table. The data tables stay until the control + // plane drops them, so the data plane reads a DELETING row as found; + // a statement that reaches a dropped data table is answered as not + // found by the `DataEngine` implementation. + "ACTIVE" | "DELETING" => {} + // A table that is still being created is not usable for data-plane + // operations; DynamoDB reports it as not found (not as in-use). + // UPDATING keeps the not-active classification. + "CREATING" => { return Err(StorageError::TableNotFound(table_name.to_owned())); } _ => return Err(StorageError::TableNotActive(table_name.to_owned())), @@ -529,9 +544,10 @@ impl PostgresEngine { index_name: &str, ) -> Result { // First get the table_id and verify the table is ACTIVE - let row: Option<(String, String)> = sqlx::query_as( - "SELECT table_id, table_status FROM tables WHERE account_id = $1 AND table_name = $2", - ) + let row: Option<(String, String)> = sqlx::query_as(&format!( + "SELECT table_id, {DATA_PLANE_STATUS} FROM tables \ + WHERE account_id = $1 AND table_name = $2" + )) .bind(account_id) .bind(table_name) .fetch_optional(&self.pool) @@ -542,11 +558,10 @@ impl PostgresEngine { row.ok_or_else(|| StorageError::TableNotFound(table_name.to_owned()))?; match status.as_str() { - "ACTIVE" => {} - // A table that is still being created, or is being deleted, is not - // usable for data-plane operations; DynamoDB reports it as not found - // (not as in-use). UPDATING keeps the not-active classification. - "CREATING" | "DELETING" => { + // Same classification as `fetch_table_key_info`: a DELETING table + // is served until it is gone, a CREATING one is not found. + "ACTIVE" | "DELETING" => {} + "CREATING" => { return Err(StorageError::TableNotFound(table_name.to_owned())); } _ => return Err(StorageError::TableNotActive(table_name.to_owned())), diff --git a/crates/storage-postgres/src/data/delete_item.rs b/crates/storage-postgres/src/data/delete_item.rs index 9a6dfda93..163cc3dae 100755 --- a/crates/storage-postgres/src/data/delete_item.rs +++ b/crates/storage-postgres/src/data/delete_item.rs @@ -10,7 +10,7 @@ use extenddb_storage::error::StorageError; use extenddb_storage::util::{SortKeyValue, parse_sk, pk_to_text, sk_column, sk_info}; use super::gsi_build::fix_up_stale_write; -use super::index::{enqueue_async_indexes, fetch_write_path_indexes, sync_indexes}; +use super::index::{db_error, enqueue_async_indexes, fetch_write_path_indexes, sync_indexes}; use super::query::check_condition; use super::tx_helpers::write_stream_record_in_tx; use super::{data_table_name, json_to_item}; @@ -91,11 +91,7 @@ impl PostgresEngine { ); let delete_sql = format!("DELETE FROM {ddb_table} WHERE pk = $1 AND {sk_col} = $2"); - let mut tx = self - .data_pool - .begin() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + let mut tx = self.data_pool.begin().await.map_err(db_error)?; let old: Option<(serde_json::Value,)> = bind_sk_fetch_optional!(&select_sql, pk_text.as_ref(), &sk, &mut *tx)?; @@ -147,7 +143,7 @@ impl PostgresEngine { .await } } - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; // Sync GSI/LSI update within transaction (D-4). let old_item_for_idx = if indexes.is_empty() { @@ -220,9 +216,7 @@ impl PostgresEngine { async_enqueued += fix_up_stale_write(&mut tx, &self.pool, stamp, key_info, &indexes, key).await?; - tx.commit() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + tx.commit().await.map_err(db_error)?; if async_enqueued > 0 && let Some(ref q) = self.gsi_queue @@ -261,7 +255,7 @@ impl PostgresEngine { .await } } - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; self.fix_up_autocommit(stamp, key_info, key).await; Ok(None) } @@ -272,17 +266,13 @@ impl PostgresEngine { format!("SELECT item_data FROM {ddb_table} WHERE pk = $1 FOR UPDATE"); let delete_sql = format!("DELETE FROM {ddb_table} WHERE pk = $1"); - let mut tx = self - .data_pool - .begin() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + let mut tx = self.data_pool.begin().await.map_err(db_error)?; let old: Option<(serde_json::Value,)> = sqlx::query_as(&select_sql) .bind(pk_text.as_ref()) .fetch_optional(&mut *tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; if let Some((ref old_json,)) = old { let old_item: Item = json_to_item(old_json.clone())?; @@ -309,7 +299,7 @@ impl PostgresEngine { .bind(pk_text.as_ref()) .execute(&mut *tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; // Sync GSI/LSI update within transaction (D-4). let old_item_for_idx = if indexes.is_empty() { @@ -382,9 +372,7 @@ impl PostgresEngine { async_enqueued += fix_up_stale_write(&mut tx, &self.pool, stamp, key_info, &indexes, key).await?; - tx.commit() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + tx.commit().await.map_err(db_error)?; if async_enqueued > 0 && let Some(ref q) = self.gsi_queue @@ -404,7 +392,7 @@ impl PostgresEngine { .bind(pk_text.as_ref()) .execute(&self.data_pool) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; self.fix_up_autocommit(stamp, key_info, key).await; Ok(None) } diff --git a/crates/storage-postgres/src/data/put_item.rs b/crates/storage-postgres/src/data/put_item.rs index 2fbc3102b..34fdf3010 100755 --- a/crates/storage-postgres/src/data/put_item.rs +++ b/crates/storage-postgres/src/data/put_item.rs @@ -10,7 +10,7 @@ use extenddb_storage::error::StorageError; use extenddb_storage::util::{composite_pk_to_text, parse_sk, pk_to_text, sk_column, sk_info}; use super::gsi_build::fix_up_stale_write; -use super::index::{enqueue_async_indexes, fetch_write_path_indexes, sync_indexes}; +use super::index::{db_error, enqueue_async_indexes, fetch_write_path_indexes, sync_indexes}; use super::query::check_condition; use super::tx_helpers::write_stream_record_in_tx; use super::{data_table_name, json_to_item}; @@ -111,11 +111,7 @@ impl PostgresEngine { "SELECT item_data FROM {ddb_table} WHERE pk = $1 AND {sk_col} = $2 FOR UPDATE" ); - let mut tx = self - .data_pool - .begin() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + let mut tx = self.data_pool.begin().await.map_err(db_error)?; let mut old: Option<(serde_json::Value,)> = bind_sk_fetch_optional!(&select_sql, pk_text.as_str(), &sk, &mut *tx)?; @@ -243,9 +239,7 @@ impl PostgresEngine { fix_up_stale_write(&mut tx, &self.pool, stamp, key_info, &indexes, &item) .await?; - tx.commit() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + tx.commit().await.map_err(db_error)?; if async_enqueued > 0 && let Some(ref q) = self.gsi_queue @@ -280,18 +274,14 @@ impl PostgresEngine { let select_sql = format!("SELECT item_data FROM {ddb_table} WHERE pk = $1 FOR UPDATE"); - let mut tx = self - .data_pool - .begin() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + let mut tx = self.data_pool.begin().await.map_err(db_error)?; let fetch_old = async |tx: &mut sqlx::PgConnection| { sqlx::query_as::<_, (serde_json::Value,)>(&select_sql) .bind(pk_text.as_str()) .fetch_optional(tx) .await - .map_err(|e| StorageError::Internal(e.to_string())) + .map_err(db_error) }; let mut old: Option<(serde_json::Value,)> = fetch_old(&mut tx).await?; @@ -314,7 +304,7 @@ impl PostgresEngine { .bind(&item_json) .execute(&mut *tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; break; } let empty = std::collections::BTreeMap::new(); @@ -334,7 +324,7 @@ impl PostgresEngine { .bind(&item_json) .execute(&mut *tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; if result.rows_affected() == 1 { break; } @@ -421,9 +411,7 @@ impl PostgresEngine { fix_up_stale_write(&mut tx, &self.pool, stamp, key_info, &indexes, &item) .await?; - tx.commit() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + tx.commit().await.map_err(db_error)?; if async_enqueued > 0 && let Some(ref q) = self.gsi_queue @@ -447,7 +435,7 @@ impl PostgresEngine { .bind(&item_json) .execute(&self.data_pool) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; self.fix_up_autocommit(stamp, key_info, &item).await; Ok(None) } @@ -486,7 +474,7 @@ impl PostgresEngine { .bind(pk_text.as_ref()) .fetch_optional(&self.data_pool) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; row.map(|(v,)| v) }; diff --git a/crates/storage-postgres/src/data/query.rs b/crates/storage-postgres/src/data/query.rs index a57b69bec..cdc50e24f 100755 --- a/crates/storage-postgres/src/data/query.rs +++ b/crates/storage-postgres/src/data/query.rs @@ -14,6 +14,8 @@ use extenddb_storage::error::StorageError; use extenddb_storage::util::SortKeyValue; use extenddb_storage::util::{parse_sk, pk_to_text, sk_info}; +use super::index::db_error; + /// Extra pagination bind values for index queries. /// /// Built in `query_scan.rs` in the same branch that generates the SQL placeholders, @@ -207,10 +209,7 @@ pub(crate) async fn execute_query_sql( } } - let rows: Vec<(serde_json::Value,)> = query - .fetch_all(pool) - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + let rows: Vec<(serde_json::Value,)> = query.fetch_all(pool).await.map_err(db_error)?; Ok(rows.into_iter().map(|(v,)| v).collect()) } @@ -347,10 +346,7 @@ pub(crate) async fn execute_scan_sql( } } - let rows: Vec<(serde_json::Value,)> = query - .fetch_all(pool) - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + let rows: Vec<(serde_json::Value,)> = query.fetch_all(pool).await.map_err(db_error)?; Ok(rows.into_iter().map(|(v,)| v).collect()) } diff --git a/crates/storage-postgres/src/data/tx_helpers.rs b/crates/storage-postgres/src/data/tx_helpers.rs index d17d0c437..535a963f5 100755 --- a/crates/storage-postgres/src/data/tx_helpers.rs +++ b/crates/storage-postgres/src/data/tx_helpers.rs @@ -48,7 +48,7 @@ pub(super) async fn fetch_item_in_tx( .bind(pk_text.as_ref()) .fetch_optional(&mut **tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; row.map(|(v,)| v) }; diff --git a/crates/storage-postgres/src/data/update_item.rs b/crates/storage-postgres/src/data/update_item.rs index 759990b40..7d80c81ae 100755 --- a/crates/storage-postgres/src/data/update_item.rs +++ b/crates/storage-postgres/src/data/update_item.rs @@ -11,7 +11,7 @@ use extenddb_storage::error::StorageError; use extenddb_storage::util::{parse_sk, pk_to_text, sk_column, sk_info}; use super::gsi_build::fix_up_stale_write; -use super::index::{enqueue_async_indexes, fetch_write_path_indexes, sync_indexes}; +use super::index::{db_error, enqueue_async_indexes, fetch_write_path_indexes, sync_indexes}; use super::query::check_condition; use super::tx_helpers::write_stream_record_in_tx; use super::{data_table_name, json_to_item}; @@ -40,11 +40,7 @@ impl PostgresEngine { let pk_text = pk_to_text(pk_value)?; // UpdateItem always needs a transaction (read-modify-write) - let mut tx = self - .data_pool - .begin() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + let mut tx = self.data_pool.begin().await.map_err(db_error)?; // Both index families in one catalog visit (D-4: sync + async split for the // secondary indexes). @@ -133,7 +129,7 @@ impl PostgresEngine { .bind(pk_text.as_ref()) .fetch_optional(&mut *tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; row.map(|(v,)| v) } }; @@ -241,7 +237,7 @@ impl PostgresEngine { .bind(&item_json) .execute(&mut *tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + .map_err(db_error)?; } } break (old_item, new_item, item, pre_mutation_item); @@ -268,7 +264,7 @@ impl PostgresEngine { .bind(&item_json) .execute(&mut *tx) .await - .map_err(|e| StorageError::Internal(e.to_string()))? + .map_err(db_error)? .rows_affected() == 1 } @@ -341,9 +337,7 @@ impl PostgresEngine { async_enqueued += fix_up_stale_write(&mut tx, &self.pool, stamp, key_info, &indexes, key).await?; - tx.commit() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; + tx.commit().await.map_err(db_error)?; if async_enqueued > 0 && let Some(ref q) = self.gsi_queue diff --git a/crates/storage-postgres/src/delete_table.rs b/crates/storage-postgres/src/delete_table.rs index 81b75856b..adbf26e7c 100755 --- a/crates/storage-postgres/src/delete_table.rs +++ b/crates/storage-postgres/src/delete_table.rs @@ -39,8 +39,8 @@ impl PostgresEngine { .map_err(|e| StorageError::Internal(e.to_string()))?; // Lock and fetch the row atomically with SELECT ... FOR UPDATE. A - // DELETING row is fetched too: DeleteTable of a table that is already - // being deleted returns its description, as the service does. + // DELETING row is fetched too, so that a DeleteTable of a table already + // being deleted can be answered the way the service answers it. let row: Option = sqlx::query_as( r"SELECT table_name, key_schema, attribute_definitions, billing_mode, provisioned_throughput, stream_specification, table_status, @@ -59,6 +59,16 @@ impl PostgresEngine { .map_err(|e| StorageError::Internal(e.to_string()))?; let row = row.ok_or_else(|| StorageError::TableNotFound(input.table_name.clone()))?; + if row.table_status == "DELETING" { + // The service answers a DeleteTable of a table that is already + // being deleted as a table still in use (measured 2026-10-25 and + // 2026-10-30), not with the table's description. The rollback + // releases the row lock. + tx.rollback() + .await + .map_err(|e| StorageError::Internal(e.to_string()))?; + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); + } // Fetch indexes for the response description. let index_rows: Vec = sqlx::query_as( @@ -71,20 +81,6 @@ impl PostgresEngine { .await .map_err(|e| StorageError::Internal(e.to_string()))?; - if row.table_status == "DELETING" { - // Already being removed (a deferred drop the control plane - // retries, or a delay of a second or more): nothing to change. - // The rollback releases the row lock. - tx.rollback() - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; - let desc = self.build_table_description_from_row(account_id, row, index_rows)?; - return Ok(TableDescription { - table_status: TableStatus::Deleting, - ..desc - }); - } - // REQ: DeletionProtectionEnabled check — real DynamoDB returns ValidationException if row.deletion_protection_enabled { return Err(StorageError::DeletionProtected(row.table_arn.clone())); diff --git a/crates/storage-postgres/src/update_table.rs b/crates/storage-postgres/src/update_table.rs index dec99a93b..814e736ff 100755 --- a/crates/storage-postgres/src/update_table.rs +++ b/crates/storage-postgres/src/update_table.rs @@ -131,6 +131,11 @@ impl PostgresEngine { let (status, table_id, ks_json, ad_json, stored_billing_mode) = row.ok_or_else(|| StorageError::TableNotFound(input.table_name.clone()))?; + // A DELETING table has its own sentence on the service (measured + // 2026-10-24); every other non-ACTIVE status keeps the existing one. + if status == "DELETING" { + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); + } if status != "ACTIVE" { return Err(StorageError::TableNotActive(input.table_name.clone())); } diff --git a/crates/storage-postgres/tests/deleting_window.rs b/crates/storage-postgres/tests/deleting_window.rs new file mode 100644 index 000000000..d7aa3a2c4 --- /dev/null +++ b/crates/storage-postgres/tests/deleting_window.rs @@ -0,0 +1,669 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Storage-level tests for what the control plane answers while a table is +//! `DELETING`. +//! +//! Amazon DynamoDB answers a second `DeleteTable` and an `UpdateTable` in that +//! window with `ResourceInUseException` and the sentence `Attempt to change a +//! resource which is still in use: Table is being deleted: ` (measured +//! 2026-10-24). The window is produced here by `delete_table` itself: with a +//! control-plane delay of one second or more the row is marked `DELETING` with +//! a scheduled transition, and these tests run no poller to finish it. +//! +//! Each test builds its own throwaway database, applies the shipped migrations +//! to it, and drops it when it passes. A failing test leaves its database behind +//! on purpose, named `eddb_delw_*`, so the state that failed can be inspected. +//! +//! Requires `EXTENDDB_TEST_PG_CONNECTION_STRING`, a base URL with no database +//! component (for example `postgresql://postgres@127.0.0.1:5432`), pointing at a +//! 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. +//! +//! The data-plane tests below pin the other half of the window: DynamoDB serves +//! `PutItem`, `GetItem`, `Query` and `Scan` on a `DELETING` table until the +//! table is gone, then answers `ResourceNotFoundException` (measured +//! 2026-10-25). Two `DeleteTable` calls on one `ACTIVE` table get one `200` and +//! one `ResourceInUseException` with the `Table is being deleted` sentence. + +use std::collections::{BTreeMap, HashMap}; +use std::sync::Arc; + +use extenddb_core::expression::{Expr, ExpressionMaps, KeyCondition, PathElement}; +use extenddb_core::types::{ + AttributeDefinition, AttributeValue, BillingMode, CreateTableInput, DeleteTableInput, Item, + KeySchemaElement, KeyType, ReturnValuesOnConditionCheckFailure, ScalarAttributeType, + TableKeyInfo, TableStatus, UpdateTableInput, +}; +use extenddb_storage::error::StorageError; +use extenddb_storage::{DataEngine, TableEngine, TransactGetOp, TransactWriteOp}; +use extenddb_storage_postgres::{PostgresConfig, PostgresEngine}; +use sqlx::PgPool; +use sqlx::postgres::PgPoolOptions; + +const ACCOUNT: &str = "123456789012"; +const REGION: &str = "us-east-1"; +const TABLE: &str = "t_deleting_window"; + +struct Scratch { + engine: PostgresEngine, + catalog: PgPool, + admin: PgPool, + db_name: String, +} + +impl Scratch { + /// Drop the scratch database. Called only on the success path, so a failure + /// leaves the database for inspection. + async fn cleanup(self) { + let Scratch { + engine, + catalog, + admin, + db_name, + } = self; + drop(engine); + catalog.close().await; + sqlx::query(&format!( + "DROP DATABASE IF EXISTS \"{db_name}\" WITH (FORCE)" + )) + .execute(&admin) + .await + .expect("drop the scratch database"); + admin.close().await; + } +} + +fn base_conn() -> Option { + let conn = std::env::var("EXTENDDB_TEST_PG_CONNECTION_STRING").ok()?; + (!conn.trim().is_empty()).then(|| conn.trim_end_matches('/').to_owned()) +} + +/// Every `.sql` file under `migrations/` and `data_migrations/`, each +/// directory in filename order. +fn shipped_migrations() -> Vec { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + let mut out = Vec::new(); + for dir in ["migrations", "data_migrations"] { + let mut files: Vec<_> = std::fs::read_dir(root.join(dir)) + .expect("read the migrations directory") + .map(|e| e.expect("directory entry").path()) + .filter(|p| p.extension().is_some_and(|e| e == "sql")) + .collect(); + files.sort(); + for f in files { + out.push(std::fs::read_to_string(&f).expect("read a migration file")); + } + } + out +} + +fn skip(test: &str) { + eprintln!( + "SKIP {test}: EXTENDDB_TEST_PG_CONNECTION_STRING is not set, so there is no PostgreSQL \ + to build a scratch catalog in." + ); +} + +/// A scratch database with the shipped schema, a zero control-plane delay so +/// `create_table` completes inline, and an engine on it. +async fn scratch() -> Scratch { + let base = base_conn().expect("caller checks base_conn() first"); + let db_name = format!("eddb_delw_{}", uuid::Uuid::new_v4().simple())[..24].to_owned(); + + let admin = PgPoolOptions::new() + .max_connections(1) + .connect(&format!("{base}/postgres")) + .await + .expect("connect to the postgres maintenance database"); + sqlx::query(&format!("CREATE DATABASE \"{db_name}\"")) + .execute(&admin) + .await + .expect("create the scratch database"); + + let url = format!("{base}/{db_name}"); + let catalog = PgPoolOptions::new() + .max_connections(2) + .connect(&url) + .await + .expect("connect to the scratch database"); + for sql in shipped_migrations() { + sqlx::raw_sql(&sql) + .execute(&catalog) + .await + .expect("apply a shipped migration"); + } + set_delay(&catalog, "0").await; + sqlx::query( + "INSERT INTO settings (key, value) VALUES ('index_propagation_delay_ms', '0') \ + ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value", + ) + .execute(&catalog) + .await + .expect("pin the propagation delay to zero"); + sqlx::query("INSERT INTO accounts (account_id, account_name) VALUES ($1, $2)") + .bind(ACCOUNT) + .bind(format!("acct-{db_name}")) + .execute(&catalog) + .await + .expect("seed the account row"); + + let engine = PostgresEngine::new( + &PostgresConfig { + connection_string: url, + pool_size: 10, + max_item_size_bytes: 400_000, + }, + REGION, + ) + .await + .expect("open a PostgresEngine on the scratch database"); + + Scratch { + engine, + catalog, + admin, + db_name, + } +} + +/// Write the control-plane delay. The row is written, not only updated: +/// migration 1 seeds no setting. +async fn set_delay(catalog: &PgPool, seconds: &str) { + sqlx::query( + "INSERT INTO settings (key, value) VALUES ('control_plane_delay_seconds', $1) \ + ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value", + ) + .bind(seconds) + .execute(catalog) + .await + .expect("set the control-plane delay"); +} + +async fn status_of(catalog: &PgPool, table: &str) -> Option { + sqlx::query_scalar("SELECT table_status FROM tables WHERE account_id = $1 AND table_name = $2") + .bind(ACCOUNT) + .bind(table) + .fetch_optional(catalog) + .await + .expect("read the table status") +} + +fn update_input(table: &str) -> UpdateTableInput { + UpdateTableInput { + table_name: table.to_owned(), + billing_mode: None, + provisioned_throughput: None, + deletion_protection_enabled: Some(true), + global_secondary_index_updates: None, + attribute_definitions: None, + stream_specification: None, + table_class: None, + on_demand_throughput: None, + table_throughput_mode: None, + vector_index_updates: None, + } +} + +/// Create `table` with a single string hash key, completing inline (delay 0). +async fn create_table_named(s: &Scratch, table: &str) { + s.engine + .create_table( + ACCOUNT, + CreateTableInput { + table_name: table.to_owned(), + key_schema: vec![KeySchemaElement { + attribute_name: "pk".to_owned(), + key_type: KeyType::Hash, + }], + attribute_definitions: vec![AttributeDefinition { + attribute_name: "pk".to_owned(), + attribute_type: ScalarAttributeType::S, + }], + billing_mode: Some(BillingMode::PayPerRequest), + ..Default::default() + }, + ) + .await + .expect("create the table"); + assert_eq!( + status_of(&s.catalog, table).await.as_deref(), + Some("ACTIVE") + ); +} + +/// Delete `TABLE` with a delay long enough that the row stays `DELETING` for +/// the rest of the test: the window held open. +async fn start_deleting(s: &Scratch) { + set_delay(&s.catalog, "600").await; + let desc = s + .engine + .delete_table( + ACCOUNT, + DeleteTableInput { + table_name: TABLE.to_owned(), + }, + ) + .await + .expect("the first DeleteTable"); + assert_eq!(desc.table_status, TableStatus::Deleting); + assert_eq!( + status_of(&s.catalog, TABLE).await.as_deref(), + Some("DELETING"), + "the row must be DELETING for the rest of the test" + ); +} + +/// Create `TABLE`, then delete it with the window held open. +async fn open_window(s: &Scratch) { + create_table_named(s, TABLE).await; + start_deleting(s).await; +} + +fn item(pk: &str) -> Item { + BTreeMap::from([("pk".to_owned(), AttributeValue::S(pk.to_owned()))]) +} + +fn query_pk(pk: &str) -> (KeyCondition, ExpressionMaps) { + let condition = KeyCondition { + pk_path: vec![PathElement::Attribute("pk".to_owned())], + pk_value: Expr::Placeholder(":pk".to_owned()), + extra_pk_conditions: Vec::new(), + sk_condition: None, + extra_sk_conditions: Vec::new(), + }; + let maps = ExpressionMaps::new( + HashMap::new(), + HashMap::from([(":pk".to_owned(), AttributeValue::S(pk.to_owned()))]), + ); + (condition, maps) +} + +/// Create `TABLE`, write the item `pre`, and open the window. Returns the key +/// info read while the row was still ACTIVE, the snapshot a cache would hold. +async fn open_window_with_item(s: &Scratch) -> TableKeyInfo { + create_table_named(s, TABLE).await; + let key_info = s + .engine + .table_key_info(ACCOUNT, TABLE) + .await + .expect("key info of the ACTIVE table"); + s.engine + .put_item( + &key_info, + item("pre"), + false, + None, + &ExpressionMaps::default(), + None, + ) + .await + .expect("seed the pre-existing item"); + start_deleting(s).await; + key_info +} + +/// A second DeleteTable during the window reports the table as being deleted, +/// not as absent, and leaves the row in place for the deletion under way. +#[tokio::test] +async fn a_second_delete_table_during_the_window_is_table_being_deleted() { + if base_conn().is_none() { + return skip("a_second_delete_table_during_the_window_is_table_being_deleted"); + } + let s = scratch().await; + open_window(&s).await; + + let err = s + .engine + .delete_table( + ACCOUNT, + DeleteTableInput { + table_name: TABLE.to_owned(), + }, + ) + .await + .expect_err("DeleteTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == TABLE), + "expected TableBeingDeleted({TABLE:?}), got {err:?}" + ); + assert_eq!( + status_of(&s.catalog, TABLE).await.as_deref(), + Some("DELETING") + ); + + s.cleanup().await; +} + +/// UpdateTable during the window reports the table as being deleted, not with +/// the sentence for the other non-ACTIVE statuses, and changes nothing. +#[tokio::test] +async fn update_table_during_the_window_is_table_being_deleted() { + if base_conn().is_none() { + return skip("update_table_during_the_window_is_table_being_deleted"); + } + let s = scratch().await; + open_window(&s).await; + + let err = s + .engine + .update_table(ACCOUNT, update_input(TABLE)) + .await + .expect_err("UpdateTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == TABLE), + "expected TableBeingDeleted({TABLE:?}), got {err:?}" + ); + let protected: bool = sqlx::query_scalar( + "SELECT deletion_protection_enabled FROM tables WHERE account_id = $1 AND table_name = $2", + ) + .bind(ACCOUNT) + .bind(TABLE) + .fetch_one(&s.catalog) + .await + .expect("read the flag"); + assert!(!protected, "a refused UpdateTable must not have written"); + + s.cleanup().await; +} + +/// A CREATING table keeps the existing answer: the new sentence is for +/// DELETING only. +#[tokio::test] +async fn update_table_on_a_creating_table_keeps_table_not_active() { + if base_conn().is_none() { + return skip("update_table_on_a_creating_table_keeps_table_not_active"); + } + let s = scratch().await; + open_window(&s).await; + sqlx::query( + "UPDATE tables SET table_status = 'CREATING' WHERE account_id = $1 AND table_name = $2", + ) + .bind(ACCOUNT) + .bind(TABLE) + .execute(&s.catalog) + .await + .expect("mark CREATING"); + + let err = s + .engine + .update_table(ACCOUNT, update_input(TABLE)) + .await + .expect_err("UpdateTable on a CREATING table"); + assert!( + matches!(&err, StorageError::TableNotActive(name) if name == TABLE), + "expected TableNotActive({TABLE:?}), got {err:?}" + ); + + s.cleanup().await; +} + +/// During the window the key-info read finds the table, and PutItem, GetItem +/// (the pre-existing item and the one just written), Query and Scan are +/// served, as the service serves them. +#[tokio::test] +async fn data_plane_is_served_during_the_window() { + if base_conn().is_none() { + return skip("data_plane_is_served_during_the_window"); + } + let s = scratch().await; + open_window_with_item(&s).await; + let key_info = s + .engine + .table_key_info(ACCOUNT, TABLE) + .await + .expect("a DELETING table is found by the data plane"); + + s.engine + .put_item( + &key_info, + item("w1"), + false, + None, + &ExpressionMaps::default(), + None, + ) + .await + .expect("PutItem during the window"); + let got = s + .engine + .get_item(&key_info, &item("pre")) + .await + .expect("GetItem of the pre-existing item"); + assert_eq!(got, Some(item("pre"))); + let got = s + .engine + .get_item(&key_info, &item("w1")) + .await + .expect("GetItem of the window write"); + assert_eq!(got, Some(item("w1"))); + let (condition, maps) = query_pk("pre"); + let (rows, _) = s + .engine + .query(&key_info, &condition, &maps, true, None, None, None) + .await + .expect("Query during the window"); + assert_eq!(rows, vec![item("pre")]); + let (rows, _) = s + .engine + .scan(&key_info, Some(1), None, None, None, None) + .await + .expect("Scan during the window"); + assert_eq!(rows.len(), 1); + assert_eq!( + status_of(&s.catalog, TABLE).await.as_deref(), + Some("DELETING") + ); + + s.cleanup().await; +} + +/// Once the data table is dropped while the row still reads DELETING, a +/// request that reaches storage, here with the key info read before the +/// delete (the snapshot a cache would hold), answers the table as not found +/// rather than as an internal error. +#[tokio::test] +async fn a_request_after_the_data_drop_is_table_not_found() { + if base_conn().is_none() { + return skip("a_request_after_the_data_drop_is_table_not_found"); + } + let s = scratch().await; + let key_info = open_window_with_item(&s).await; + sqlx::query(&format!("DROP TABLE \"_ddb_{}\"", key_info.table_id)) + .execute(&s.catalog) + .await + .expect("drop the data table under the DELETING row"); + + let err = s + .engine + .put_item( + &key_info, + item("w1"), + false, + None, + &ExpressionMaps::default(), + None, + ) + .await + .expect_err("PutItem after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == TABLE), + "expected TableNotFound({TABLE:?}), got {err:?}" + ); + let err = s + .engine + .get_item(&key_info, &item("pre")) + .await + .expect_err("GetItem after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == TABLE), + "expected TableNotFound({TABLE:?}), got {err:?}" + ); + // The transaction paths run outside the single-request statements, so + // they are funnelled at their own entry points. + let pre = item("pre"); + let err = s + .engine + .transact_get_items(&[TransactGetOp { + key_info: &key_info, + key: &pre, + }]) + .await + .expect_err("TransactGetItems after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == TABLE), + "expected TableNotFound({TABLE:?}), got {err:?}" + ); + let maps = ExpressionMaps::default(); + let w2 = item("w2"); + let err = s + .engine + .transact_write_items( + &[TransactWriteOp::Put { + key_info: &key_info, + item: &w2, + condition: None, + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }], + None, + ) + .await + .expect_err("TransactWriteItems after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == TABLE), + "expected TableNotFound({TABLE:?}), got {err:?}" + ); + + s.cleanup().await; +} + +/// Two concurrent DeleteTable calls on one ACTIVE table: the first locks the +/// row `FOR UPDATE` and marks it DELETING; the second, blocked on that lock, +/// reads the row again after the commit, finds it DELETING, and answers +/// TableBeingDeleted. Ten rounds, a fresh table each; one row per table, +/// DELETING, and its data table present. +#[tokio::test] +async fn two_concurrent_delete_tables_have_one_winner() { + if base_conn().is_none() { + return skip("two_concurrent_delete_tables_have_one_winner"); + } + let s = scratch().await; + let engine = Arc::new(s.engine.clone()); + + for round in 0..10 { + let table = format!("t_pair_{round}"); + set_delay(&s.catalog, "0").await; + create_table_named(&s, &table).await; + set_delay(&s.catalog, "600").await; + + let tasks: Vec<_> = (0..2) + .map(|_| { + let engine = Arc::clone(&engine); + let table = table.clone(); + tokio::spawn(async move { + engine + .delete_table(ACCOUNT, DeleteTableInput { table_name: table }) + .await + }) + }) + .collect(); + let mut ok = 0; + let mut being_deleted = 0; + for task in tasks { + match task.await.expect("task") { + Ok(_) => ok += 1, + Err(StorageError::TableBeingDeleted(name)) if name == table => being_deleted += 1, + Err(other) => panic!("round {round}: unexpected {other:?}"), + } + } + assert_eq!((ok, being_deleted), (1, 1), "round {round}"); + + let rows: Vec<(String, String)> = sqlx::query_as( + "SELECT table_status, table_id FROM tables WHERE account_id = $1 AND table_name = $2", + ) + .bind(ACCOUNT) + .bind(&table) + .fetch_all(&s.catalog) + .await + .expect("rows"); + assert_eq!(rows.len(), 1, "round {round}: one catalog row"); + assert_eq!(rows[0].0, "DELETING", "round {round}"); + let present: Option = sqlx::query_scalar("SELECT to_regclass($1)::text") + .bind(format!("\"_ddb_{}\"", rows[0].1)) + .fetch_one(&s.catalog) + .await + .expect("to_regclass"); + assert!( + present.is_some(), + "round {round}: the data table is still there" + ); + } + + drop(engine); + s.cleanup().await; +} + +/// The restore sweep removes an abandoned restore target by marking its row +/// DELETING and then dropping its data table. The target was never served, +/// and the service answers a table it is still restoring as not found, so the +/// data plane keeps answering it that way while the drop waits, where a table +/// DeleteTable marked is served. A transaction holding ACCESS SHARE on the data +/// table keeps the drop waiting. +#[tokio::test] +async fn an_abandoned_restore_target_being_removed_is_not_found() { + if base_conn().is_none() { + return skip("an_abandoned_restore_target_being_removed_is_not_found"); + } + let s = scratch().await; + create_table_named(&s, TABLE).await; + // A restore target: CREATING with no scheduled transition, older than the + // sweep's grace period, and no process holding its restore lock. + let table_id: String = sqlx::query_scalar( + "UPDATE tables SET table_status = 'CREATING', status_transition_at = NULL, \ + creation_date_time = NOW() - INTERVAL '1 day' \ + WHERE account_id = $1 AND table_name = $2 RETURNING table_id", + ) + .bind(ACCOUNT) + .bind(TABLE) + .fetch_one(&s.catalog) + .await + .expect("make the table a restore target"); + + let mut holder = s.catalog.begin().await.expect("the holder's transaction"); + sqlx::query(&format!( + "LOCK TABLE \"_ddb_{table_id}\" IN ACCESS SHARE MODE" + )) + .execute(&mut *holder) + .await + .expect("hold the data table"); + + let sweep = s.engine.process_control_plane_transitions(); + let check = async { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + while status_of(&s.catalog, TABLE).await.as_deref() != Some("DELETING") { + assert!( + std::time::Instant::now() < deadline, + "the sweep did not claim the restore target within 30 s" + ); + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + let key_info = s.engine.table_key_info(ACCOUNT, TABLE).await; + holder.rollback().await.expect("release the data table"); + key_info + }; + let (swept, key_info) = tokio::join!(sweep, check); + swept.expect("the control-plane pass"); + assert!( + matches!(&key_info, Err(StorageError::TableNotFound(name)) if name == TABLE), + "expected TableNotFound({TABLE:?}) while the restore target is removed, got {key_info:?}" + ); + assert_eq!( + status_of(&s.catalog, TABLE).await, + None, + "the sweep removed the row" + ); + + s.cleanup().await; +} diff --git a/crates/storage-postgres/tests/table_snapshot.rs b/crates/storage-postgres/tests/table_snapshot.rs index 7a985c88d..5781f7418 100644 --- a/crates/storage-postgres/tests/table_snapshot.rs +++ b/crates/storage-postgres/tests/table_snapshot.rs @@ -1151,8 +1151,9 @@ on_both_layouts!(concurrent_delete_tables_do_not_exhaust_the_pool); /// deletes wait on the row the open holds `FOR SHARE` with catalog /// connections while the open needs only a data connection, and the open /// then streams every item; the first delete finds the table in use and -/// leaves it `DELETING`, and the other eight find it `DELETING` and return -/// its description. On the shared layout the open holds no row lock and, at +/// leaves it `DELETING`, and the other eight find it `DELETING` and are +/// refused as `TableBeingDeleted`, as the service refuses a second +/// `DeleteTable`. On the shared layout the open holds no row lock and, at /// this pause, no lock on the data table yet, so the first delete drops the /// table at once, the other eight find it gone, and the open reports /// `TableNotFound`. @@ -1185,7 +1186,7 @@ async fn an_open_held_with_deletes_queued_completes(shared: bool) { .await .expect("the open and the deletes finish well inside the 30 s acquire timeout"); eprintln!("open and nine deletes took {:?}", started.elapsed()); - let mut returned = 0; + let (mut returned, mut being_deleted) = (0, 0); for result in deletes { match result { Ok(d) => { @@ -1193,17 +1194,19 @@ async fn an_open_held_with_deletes_queued_completes(shared: bool) { returned += 1; } Err(extenddb_storage::error::StorageError::TableNotFound(_)) => {} + Err(extenddb_storage::error::StorageError::TableBeingDeleted(_)) => being_deleted += 1, Err(e) => panic!("a queued DeleteTable failed: {e}"), } } - let (want, status) = if shared { - (1, None) + let (want, want_being_deleted, status) = if shared { + (1, 0, None) } else { - (9, Some("DELETING")) + (1, 8, Some("DELETING")) }; assert_eq!( - returned, want, - "DeleteTables that returned the table's description" + (returned, being_deleted), + (want, want_being_deleted), + "DeleteTables that returned the table's description, and that were refused as being deleted" ); assert_eq!(catalog_status(&s, "queued").await.as_deref(), status); if shared { @@ -2983,10 +2986,10 @@ async fn create_table_is_not_held_up_by_deferred_drops(shared: bool) { on_both_layouts!(create_table_is_not_held_up_by_deferred_drops); /// `DeleteTable` of a table that is already `DELETING` (a retried or -/// duplicated request while a reader holds it) returns the table's -/// description with `TableStatus: DELETING`, as the service does, and -/// changes nothing: the row and its scheduled removal stay as they were. -async fn delete_table_of_a_deleting_table_returns_its_description(shared: bool) { +/// duplicated request while a reader holds it) is refused as a table still +/// in use, as the service refuses it (measured 2026-10-25 and 2026-10-30), +/// and changes nothing: the row and its scheduled removal stay as they were. +async fn delete_table_of_a_deleting_table_is_refused_as_being_deleted(shared: bool) { let s = scratch_with(shared).await; create_active(&s, snapshot_conformance::table_input("twice")).await; let relations = base_and_gsi_relations(&s, "twice").await; @@ -3008,25 +3011,10 @@ async fn delete_table_of_a_deleting_table_returns_its_description(shared: bool) let before = scheduled().await; assert!(before.starts_with("DELETING "), "precondition: {before}"); - let second = delete_within_5s(&s, "twice") - .await - .expect("DeleteTable of a DELETING table returns its description"); - assert_eq!( - second.table_status, - extenddb_core::types::TableStatus::Deleting - ); - assert_eq!(second.table_arn, first.table_arn); - assert_eq!(second.table_id, first.table_id); - assert_eq!(second.key_schema, first.key_schema); - assert_eq!( - second - .global_secondary_indexes - .iter() - .flatten() - .map(|g| g.index_name.clone()) - .collect::>(), - ["gi"], - "the description lists the indexes" + let second = delete_within_5s(&s, "twice").await; + assert!( + matches!(&second, Err(extenddb_storage::error::StorageError::TableBeingDeleted(name)) if name == "twice"), + "DeleteTable of a DELETING table: {second:?}" ); assert_eq!( scheduled().await, @@ -3036,7 +3024,7 @@ async fn delete_table_of_a_deleting_table_returns_its_description(shared: bool) release_relation(holder).await; pump_until_removed(&s, "twice", &relations).await; } -on_both_layouts!(delete_table_of_a_deleting_table_returns_its_description); +on_both_layouts!(delete_table_of_a_deleting_table_is_refused_as_being_deleted); /// A drop deferred by a reader is due at once, whatever /// `control_plane_delay_seconds` is (below 1, so `DeleteTable` tried to drop diff --git a/crates/storage-sqlite/src/data/data_engine.rs b/crates/storage-sqlite/src/data/data_engine.rs index 964ea5f96..725abb351 100644 --- a/crates/storage-sqlite/src/data/data_engine.rs +++ b/crates/storage-sqlite/src/data/data_engine.rs @@ -16,6 +16,58 @@ use futures::future::BoxFuture; use crate::store::SqliteEngine; +/// Answer a statement that found the table's data table gone as a missing table. +/// +/// The key info was read while the catalog row was `ACTIVE` or `DELETING`, and +/// `DeleteTable` dropped the data table before the statement ran. SQLite +/// reports the dropped table as `no such table`. DynamoDB answers a request on +/// a table that is gone with `ResourceNotFoundException`, so the error is +/// `TableNotFound` here rather than an internal error. Only this backend's data +/// and index tables, all named `_ddb_`, are matched, as PostgreSQL's +/// `data_table_missing` matches its own. A data-plane statement also names the +/// shared `stream_shards`, `stream_records`, `gsi_pending` and +/// `vector_index_holds` tables; one of those missing is a catalog that lacks a +/// migration, not a table that is gone, so it stays an internal error and is +/// logged with its name. +fn dropped_table_as_not_found(key_info: &TableKeyInfo, e: StorageError) -> StorageError { + if data_table_missing(&e) { + StorageError::TableNotFound(key_info.table_name.clone()) + } else { + e + } +} + +/// Whether an error is SQLite's `no such table` for a `_ddb_` data or +/// index table. SQLite names the table as the statement wrote it, with or +/// without the `main.` schema. +fn data_table_missing(e: &StorageError) -> bool { + matches!(e, StorageError::Internal(msg) + if msg.contains("no such table: _ddb_") || msg.contains("no such table: main._ddb_")) +} + +/// The transaction form of [`dropped_table_as_not_found`]: a transaction's +/// statements name every table in it, so the first operation's table stands +/// for the request. DynamoDB answers a transaction on a table that is gone +/// with `ResourceNotFoundException` without naming the table. +fn transaction_dropped_table_as_not_found( + first: Option<&TableKeyInfo>, + e: StorageError, +) -> StorageError { + match first { + Some(key_info) => dropped_table_as_not_found(key_info, e), + None => e, + } +} + +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, + } +} + impl DataEngine for SqliteEngine { /// Declares vector support by handing over the implementation. `Some(self)` /// only compiles because `SqliteEngine` implements `VectorSearchEngine`, so @@ -47,6 +99,7 @@ impl DataEngine for SqliteEngine { stream.as_ref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -57,7 +110,11 @@ impl DataEngine for SqliteEngine { ) -> BoxFuture<'_, Result, StorageError>> { let key_info = key_info.clone(); let key = key.clone(); - Box::pin(async move { self.get_item_impl(&key_info, &key).await }) + Box::pin(async move { + self.get_item_impl(&key_info, &key) + .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) + }) } fn delete_item( @@ -84,6 +141,7 @@ impl DataEngine for SqliteEngine { stream.as_ref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -116,6 +174,7 @@ impl DataEngine for SqliteEngine { stream.as_ref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -145,6 +204,7 @@ impl DataEngine for SqliteEngine { index_name.as_deref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -170,6 +230,7 @@ impl DataEngine for SqliteEngine { index_name.as_deref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -209,7 +270,10 @@ impl DataEngine for SqliteEngine { .iter() .map(|(key_info, key)| TransactGetOp { key_info, key }) .collect(); - self.transact_get_items_impl(&borrowed).await + let first = owned.first().map(|(key_info, _)| key_info); + self.transact_get_items_impl(&borrowed) + .await + .map_err(|e| transaction_dropped_table_as_not_found(first, e)) }) } @@ -228,6 +292,7 @@ impl DataEngine for SqliteEngine { }); Box::pin(async move { let borrowed: Vec = owned.iter().map(OwnedWriteOp::as_op).collect(); + let first = borrowed.first().map(transact_write_op_key_info); self.transact_write_items_impl( &borrowed, idempotency.as_ref().map(|(a, t, f)| IdempotencyKey { @@ -237,6 +302,7 @@ impl DataEngine for SqliteEngine { }), ) .await + .map_err(|e| transaction_dropped_table_as_not_found(first, e)) }) } @@ -419,3 +485,49 @@ impl OwnedWriteOp { } } } + +#[cfg(test)] +mod dropped_table_tests { + use super::*; + + fn key_info() -> TableKeyInfo { + TableKeyInfo { + table_name: "t".to_owned(), + ..TableKeyInfo::default() + } + } + + /// The table's own data or index table gone: the table is gone. + #[test] + fn a_missing_ddb_table_is_a_missing_table() { + for msg in [ + "error returned from database: (code: 1) no such table: _ddb_0b0e", + "error returned from database: (code: 1) no such table: main._ddb_0b0e", + ] { + let e = dropped_table_as_not_found(&key_info(), StorageError::Internal(msg.to_owned())); + assert!( + matches!(e, StorageError::TableNotFound(ref n) if n == "t"), + "{msg}: {e:?}" + ); + } + } + + /// A shared table missing is a catalog without a migration, not a table + /// that is gone: it stays an internal error with the table's name. + #[test] + fn a_missing_shared_table_stays_internal() { + for table in [ + "stream_shards", + "stream_records", + "gsi_pending", + "vector_index_holds", + ] { + let msg = format!("error returned from database: (code: 1) no such table: {table}"); + let e = dropped_table_as_not_found(&key_info(), StorageError::Internal(msg.clone())); + assert!( + matches!(e, StorageError::Internal(ref m) if m == &msg), + "{table}: {e:?}" + ); + } + } +} diff --git a/crates/storage-sqlite/src/data/ddl.rs b/crates/storage-sqlite/src/data/ddl.rs index 739e1310e..bb859ae11 100644 --- a/crates/storage-sqlite/src/data/ddl.rs +++ b/crates/storage-sqlite/src/data/ddl.rs @@ -358,11 +358,17 @@ impl SqliteEngine { row.ok_or_else(|| StorageError::TableNotFound(table_name.to_owned()))?; match status.as_str() { - "ACTIVE" => {} - // A table still being created, or being deleted, is not usable for - // data-plane operations; DynamoDB reports it as not found (not - // in-use). UPDATING keeps the not-active classification. - "CREATING" | "DELETING" => { + // DynamoDB serves reads and writes on a DELETING table until the + // table is gone (measured 2026-10-25); a write made in that window + // goes away with the table. The data tables stay until the control + // plane drops them, so the data plane reads a DELETING row as found; + // a statement that reaches a dropped data table is answered as not + // found by the `DataEngine` implementation. + "ACTIVE" | "DELETING" => {} + // A table still being created is not usable for data-plane + // operations; DynamoDB reports it as not found (not in-use). + // UPDATING keeps the not-active classification. + "CREATING" => { return Err(StorageError::TableNotFound(table_name.to_owned())); } _ => return Err(StorageError::TableNotActive(table_name.to_owned())), @@ -505,11 +511,10 @@ impl SqliteEngine { let (table_id, status) = row.ok_or_else(|| StorageError::TableNotFound(table_name.to_owned()))?; match status.as_str() { - "ACTIVE" => {} - // A table still being created, or being deleted, is not usable for - // data-plane operations; DynamoDB reports it as not found (not - // in-use). UPDATING keeps the not-active classification. - "CREATING" | "DELETING" => { + // Same classification as `fetch_table_key_info`: a DELETING table + // is served until it is gone, a CREATING one is not found. + "ACTIVE" | "DELETING" => {} + "CREATING" => { return Err(StorageError::TableNotFound(table_name.to_owned())); } _ => return Err(StorageError::TableNotActive(table_name.to_owned())), diff --git a/crates/storage-sqlite/src/delete_table.rs b/crates/storage-sqlite/src/delete_table.rs index 272695c89..b587747a2 100644 --- a/crates/storage-sqlite/src/delete_table.rs +++ b/crates/storage-sqlite/src/delete_table.rs @@ -22,34 +22,35 @@ impl SqliteEngine { ) -> Result { Self::validate_account_id(account_id)?; - // A DELETING row is fetched too: DeleteTable of a table that is - // already being deleted returns its description, as the service does. + // The status lookup runs inside the write transaction, so two + // DeleteTable calls on one table are serialized: the second reads the + // row the first committed (DELETING, or gone) rather than the ACTIVE + // row both would otherwise read before either wrote. A DELETING row is + // fetched too, so that a DeleteTable of a table already being deleted + // can be answered the way the service answers it. + let _writer = self.write_lock.lock().await; + let mut tx = self + .pool + .begin_with("BEGIN IMMEDIATE") + .await + .map_err(|e| StorageError::Internal(e.to_string()))?; + let row: Option = sqlx::query_as(&format!( "SELECT {TABLE_COLUMNS} FROM tables WHERE account_id = ? AND table_name = ? \ AND table_status IN ('ACTIVE', 'CREATING', 'DELETING')" )) .bind(account_id) .bind(&input.table_name) - .fetch_optional(&self.pool) + .fetch_optional(&mut *tx) .await .map_err(|e| StorageError::Internal(e.to_string()))?; let row = row.ok_or_else(|| StorageError::TableNotFound(input.table_name.clone()))?; if row.table_status == "DELETING" { - // Already scheduled for removal by the control-plane worker: - // nothing to change. - let index_rows: Vec = sqlx::query_as(&format!( - "SELECT {INDEX_COLUMNS} FROM indexes WHERE table_id = ?" - )) - .bind(&row.table_id) - .fetch_all(&self.pool) - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; - let desc = self.build_table_description_from_row(account_id, row, index_rows)?; - return Ok(TableDescription { - table_status: TableStatus::Deleting, - ..desc - }); + // The service answers a DeleteTable of a table that is already + // being deleted as a table still in use (measured 2026-10-25 and + // 2026-10-30), not with the table's description. + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); } if row.deletion_protection_enabled { return Err(StorageError::DeletionProtected(row.table_arn.clone())); @@ -66,7 +67,7 @@ impl SqliteEngine { AND table_status = 'CREATING' AND status_transition_at IS NULL)", ) .bind(&row.table_id) - .fetch_one(&self.pool) + .fetch_one(&mut *tx) .await .map_err(|e| StorageError::Internal(e.to_string()))?; if restoring { @@ -82,14 +83,14 @@ impl SqliteEngine { "SELECT {INDEX_COLUMNS} FROM indexes WHERE table_id = ?" )) .bind(&row.table_id) - .fetch_all(&self.pool) + .fetch_all(&mut *tx) .await .map_err(|e| StorageError::Internal(e.to_string()))?; let delay_secs: f64 = sqlx::query_scalar::<_, String>( "SELECT value FROM settings WHERE key = 'control_plane_delay_seconds'", ) - .fetch_optional(&self.pool) + .fetch_optional(&mut *tx) .await .map_err(|e| StorageError::Internal(e.to_string()))? .and_then(|v| v.parse::().ok()) @@ -100,12 +101,6 @@ impl SqliteEngine { let table_arn = row.table_arn.clone(); if delay_secs < 1.0 { - let _writer = self.write_lock.lock().await; - let mut tx = self - .pool - .begin_with("BEGIN IMMEDIATE") - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; // A vector index with `backfilling` set is an UpdateTable build in // flight (the with-table path never sets the column), and the // service refuses to delete the table underneath one; the refusal @@ -145,24 +140,22 @@ impl SqliteEngine { let transition = format_timestamp( time::OffsetDateTime::now_utc() + time::Duration::seconds_f64(delay_secs), ); - let _writer = self.write_lock.lock().await; - let mut tx = self - .pool - .begin_with("BEGIN IMMEDIATE") - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; // Same in-flight-build refusal as the immediate branch, and inside // the transaction that flips the table to DELETING for the same // reason: without it, a build committing between an earlier check // and this flip would have its table deleted underneath it. Self::refuse_if_index_build_in_flight(&mut tx, &row.table_id).await?; + // The select above ran in this `BEGIN IMMEDIATE` transaction, so + // no other writer has changed the row since: the mark updates the + // row this call read, which the table id names. sqlx::query( "UPDATE tables SET table_status = 'DELETING', status_transition_at = ? \ - WHERE account_id = ? AND table_name = ?", + WHERE account_id = ? AND table_name = ? AND table_id = ?", ) .bind(&transition) .bind(account_id) .bind(&input.table_name) + .bind(&table_id) .execute(&mut *tx) .await .map_err(|e| StorageError::Internal(e.to_string()))?; @@ -209,3 +202,121 @@ impl SqliteEngine { Ok(()) } } + +#[cfg(test)] +mod deleting_window_tests { + use crate::SqliteEngine; + use extenddb_core::types::{DeleteTableInput, UpdateTableInput}; + use extenddb_storage::TableEngine; + use extenddb_storage::error::StorageError; + use serde_json::json; + + const ACCOUNT: &str = "000000000000"; + + /// An engine with one ACTIVE table `t`, whose catalog row is then set to + /// DELETING directly: the window a slow data drop would hold open. + async fn engine_with_deleting_table() -> SqliteEngine { + 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 INTO settings (key, value) VALUES ('control_plane_delay_seconds', '0') \ + ON CONFLICT(key) DO UPDATE SET value = excluded.value", + ) + .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", + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "AttributeDefinitions": [{"AttributeName": "pk", "AttributeType": "S"}], + "BillingMode": "PAY_PER_REQUEST" + })) + .expect("input"); + engine + .create_table_impl(ACCOUNT, input, false) + .await + .expect("create table"); + sqlx::query("UPDATE tables SET table_status = 'DELETING' WHERE table_name = 't'") + .execute(&engine.pool) + .await + .expect("mark DELETING"); + engine + } + + /// A second DeleteTable during the window reports the table as being + /// deleted, not as absent, and leaves the row in place. + #[tokio::test] + async fn a_second_delete_table_during_the_window_is_table_being_deleted() { + let engine = engine_with_deleting_table().await; + let err = engine + .delete_table( + ACCOUNT, + DeleteTableInput { + table_name: "t".to_owned(), + }, + ) + .await + .expect_err("DeleteTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == "t"), + "expected TableBeingDeleted(\"t\"), got {err:?}" + ); + let status: String = + sqlx::query_scalar("SELECT table_status FROM tables WHERE table_name = 't'") + .fetch_one(&engine.pool) + .await + .expect("row still present"); + assert_eq!(status, "DELETING"); + } + + /// UpdateTable during the window reports the table as being deleted, + /// not with the sentence for the other non-ACTIVE statuses. + #[tokio::test] + async fn update_table_during_the_window_is_table_being_deleted() { + let engine = engine_with_deleting_table().await; + let update: UpdateTableInput = serde_json::from_value(json!({ + "TableName": "t", + "DeletionProtectionEnabled": true + })) + .expect("update"); + let err = engine + .update_table(ACCOUNT, update) + .await + .expect_err("UpdateTable on a DELETING table"); + assert!( + matches!(&err, StorageError::TableBeingDeleted(name) if name == "t"), + "expected TableBeingDeleted(\"t\"), got {err:?}" + ); + } + + /// A CREATING table keeps the existing answer: the new sentence is for + /// DELETING only. + #[tokio::test] + async fn update_table_on_a_creating_table_keeps_table_not_active() { + let engine = engine_with_deleting_table().await; + sqlx::query("UPDATE tables SET table_status = 'CREATING' WHERE table_name = 't'") + .execute(&engine.pool) + .await + .expect("mark CREATING"); + let update: UpdateTableInput = serde_json::from_value(json!({ + "TableName": "t", + "DeletionProtectionEnabled": true + })) + .expect("update"); + let err = engine + .update_table(ACCOUNT, update) + .await + .expect_err("UpdateTable on a CREATING table"); + assert!( + matches!(&err, StorageError::TableNotActive(name) if name == "t"), + "expected TableNotActive(\"t\"), got {err:?}" + ); + } +} diff --git a/crates/storage-sqlite/src/deleting_window_data_tests.rs b/crates/storage-sqlite/src/deleting_window_data_tests.rs new file mode 100644 index 000000000..09ab59531 --- /dev/null +++ b/crates/storage-sqlite/src/deleting_window_data_tests.rs @@ -0,0 +1,318 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Storage-level tests for the data plane while a table is `DELETING`, and +//! for two `DeleteTable` calls racing on one table. +//! +//! Amazon DynamoDB serves `PutItem`, `GetItem`, `Query` and `Scan` on a +//! `DELETING` table until the table is gone, and answers them +//! `ResourceNotFoundException` once it is (measured 2026-10-25). Two +//! `DeleteTable` calls on one `ACTIVE` table get one `200` and one +//! `ResourceInUseException` with the `Table is being deleted` sentence. +//! +//! The window is produced by setting the catalog row to `DELETING` directly, +//! as the control-plane tests in `delete_table.rs` do; the pair test lets +//! `delete_table` itself mark the row, with a delay long enough that no worker +//! finishes the deletion during the test. + +use std::collections::{BTreeMap, HashMap}; +use std::sync::Arc; + +use extenddb_core::expression::{Expr, ExpressionMaps, KeyCondition, PathElement}; +use extenddb_core::types::{ + AttributeValue, DeleteTableInput, Item, ReturnValuesOnConditionCheckFailure, TableKeyInfo, +}; +use extenddb_storage::error::StorageError; +use extenddb_storage::{DataEngine, TableEngine, TransactGetOp, TransactWriteOp}; +use serde_json::json; + +use crate::SqliteEngine; + +const ACCOUNT: &str = "000000000000"; + +fn item(pk: &str) -> Item { + BTreeMap::from([("pk".to_owned(), AttributeValue::S(pk.to_owned()))]) +} + +fn create_input(table: &str) -> extenddb_core::types::CreateTableInput { + serde_json::from_value(json!({ + "TableName": table, + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "AttributeDefinitions": [{"AttributeName": "pk", "AttributeType": "S"}], + "BillingMode": "PAY_PER_REQUEST" + })) + .expect("input") +} + +/// Write the control-plane delay. The row is written, not only updated: the +/// migrations seed no setting. +async fn set_delay(engine: &SqliteEngine, seconds: &str) { + sqlx::query( + "INSERT INTO settings (key, value) VALUES ('control_plane_delay_seconds', ?) \ + ON CONFLICT(key) DO UPDATE SET value = excluded.value", + ) + .bind(seconds) + .execute(&engine.pool) + .await + .expect("set the control-plane delay"); +} + +async fn seed(engine: &SqliteEngine) { + crate::migrations::apply_for_tests(&engine.pool).await; + set_delay(engine, "0").await; + sqlx::query("INSERT INTO accounts (account_id, account_name) VALUES (?, 'default')") + .bind(ACCOUNT) + .execute(&engine.pool) + .await + .expect("account"); +} + +/// An in-memory engine with one table `t` holding the item `pre`, whose +/// catalog row is then set to `DELETING`: the window a slow data drop holds +/// open. Returns the engine and the key info read while the row was ACTIVE. +async fn engine_with_deleting_table() -> (SqliteEngine, TableKeyInfo) { + let engine = SqliteEngine::new(":memory:", 1, "us-east-1", 409_600) + .await + .expect("engine"); + seed(&engine).await; + engine + .create_table_impl(ACCOUNT, create_input("t"), false) + .await + .expect("create table"); + let key_info = engine.table_key_info(ACCOUNT, "t").await.expect("key info"); + engine + .put_item( + &key_info, + item("pre"), + false, + None, + &ExpressionMaps::default(), + None, + ) + .await + .expect("seed the pre-existing item"); + sqlx::query("UPDATE tables SET table_status = 'DELETING' WHERE table_name = 't'") + .execute(&engine.pool) + .await + .expect("mark DELETING"); + (engine, key_info) +} + +fn query_pk(pk: &str) -> (KeyCondition, ExpressionMaps) { + let condition = KeyCondition { + pk_path: vec![PathElement::Attribute("pk".to_owned())], + pk_value: Expr::Placeholder(":pk".to_owned()), + extra_pk_conditions: Vec::new(), + sk_condition: None, + extra_sk_conditions: Vec::new(), + }; + let maps = ExpressionMaps::new( + HashMap::new(), + HashMap::from([(":pk".to_owned(), AttributeValue::S(pk.to_owned()))]), + ); + (condition, maps) +} + +/// During the window the key-info read finds the table, and PutItem, GetItem +/// (the pre-existing item and the one just written), Query and Scan are +/// served, as the service serves them. +#[tokio::test] +async fn data_plane_is_served_during_the_window() { + let (engine, _) = engine_with_deleting_table().await; + let key_info = engine + .table_key_info(ACCOUNT, "t") + .await + .expect("a DELETING table is found by the data plane"); + + engine + .put_item( + &key_info, + item("w1"), + false, + None, + &ExpressionMaps::default(), + None, + ) + .await + .expect("PutItem during the window"); + let got = engine + .get_item(&key_info, &item("pre")) + .await + .expect("GetItem of the pre-existing item"); + assert_eq!(got, Some(item("pre"))); + let got = engine + .get_item(&key_info, &item("w1")) + .await + .expect("GetItem of the window write"); + assert_eq!(got, Some(item("w1"))); + let (condition, maps) = query_pk("pre"); + let (rows, _) = engine + .query(&key_info, &condition, &maps, true, None, None, None) + .await + .expect("Query during the window"); + assert_eq!(rows, vec![item("pre")]); + let (rows, _) = engine + .scan(&key_info, Some(1), None, None, None, None) + .await + .expect("Scan during the window"); + assert_eq!(rows.len(), 1); +} + +/// Once the data table is dropped while the row still reads DELETING, a +/// request that reaches storage answers the table as not found, not as an +/// internal error. +#[tokio::test] +async fn a_request_after_the_data_drop_is_table_not_found() { + let (engine, key_info) = engine_with_deleting_table().await; + sqlx::query(&format!( + "DROP TABLE {}", + crate::data::data_table_name(&key_info.table_id) + )) + .execute(&engine.pool) + .await + .expect("drop the data table under the DELETING row"); + + let err = engine + .put_item( + &key_info, + item("w1"), + false, + None, + &ExpressionMaps::default(), + None, + ) + .await + .expect_err("PutItem after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == "t"), + "expected TableNotFound(\"t\"), got {err:?}" + ); + let err = engine + .get_item(&key_info, &item("pre")) + .await + .expect_err("GetItem after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == "t"), + "expected TableNotFound(\"t\"), got {err:?}" + ); + // The transaction paths run outside the single-request statements, so + // they are funnelled at their own entry points. + let pre = item("pre"); + let err = engine + .transact_get_items(&[TransactGetOp { + key_info: &key_info, + key: &pre, + }]) + .await + .expect_err("TransactGetItems after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == "t"), + "expected TableNotFound(\"t\"), got {err:?}" + ); + let maps = ExpressionMaps::default(); + let w2 = item("w2"); + let err = engine + .transact_write_items( + &[TransactWriteOp::Put { + key_info: &key_info, + item: &w2, + condition: None, + maps: &maps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure::None, + stream: None, + }], + None, + ) + .await + .expect_err("TransactWriteItems after the data drop"); + assert!( + matches!(&err, StorageError::TableNotFound(name) if name == "t"), + "expected TableNotFound(\"t\"), got {err:?}" + ); +} + +/// Two concurrent DeleteTable calls on one ACTIVE table: one marks the row +/// DELETING and answers the description, the other reads the marked row and +/// answers TableBeingDeleted. Ten rounds, a fresh table each; the row is left +/// DELETING exactly once and its data table is still present (no worker runs +/// here to finish the deletion). +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn two_concurrent_delete_tables_have_one_winner() { + // The cargo target directory, found from this test binary's own path + // (`//deps/`) as `snapshot_engine`'s file engine + // finds it, so a relative `CARGO_TARGET_DIR` or a `--target-dir` is + // honoured too. + let exe = std::env::current_exe().expect("the test binary's path"); + let target = exe + .ancestors() + .nth(3) + .unwrap_or_else(|| panic!("{} is not //deps/", exe.display())); + assert!(target.is_dir(), "cargo target directory must exist"); + let path = target.join(format!("deleting-pair-{}.sqlite", uuid::Uuid::new_v4())); + let engine = Arc::new( + SqliteEngine::new(path.to_str().expect("UTF-8 path"), 4, "us-east-1", 409_600) + .await + .expect("file engine"), + ); + seed(&engine).await; + + for round in 0..10 { + let table = format!("t_pair_{round}"); + // Delay 0 so the create completes inline, then 600 so the delete + // marks the row. + set_delay(&engine, "0").await; + engine + .create_table_impl(ACCOUNT, create_input(&table), false) + .await + .expect("create table"); + set_delay(&engine, "600").await; + + let tasks: Vec<_> = (0..2) + .map(|_| { + let engine = Arc::clone(&engine); + let table = table.clone(); + tokio::spawn(async move { + engine + .delete_table(ACCOUNT, DeleteTableInput { table_name: table }) + .await + }) + }) + .collect(); + let mut ok = 0; + let mut being_deleted = 0; + for task in tasks { + match task.await.expect("task") { + Ok(_) => ok += 1, + Err(StorageError::TableBeingDeleted(name)) if name == table => being_deleted += 1, + Err(other) => panic!("round {round}: unexpected {other:?}"), + } + } + assert_eq!((ok, being_deleted), (1, 1), "round {round}"); + + let rows: Vec<(String, String)> = + sqlx::query_as("SELECT table_status, table_id FROM tables WHERE table_name = ?") + .bind(&table) + .fetch_all(&engine.pool) + .await + .expect("rows"); + assert_eq!(rows.len(), 1, "round {round}: one catalog row"); + assert_eq!(rows[0].0, "DELETING", "round {round}"); + let data_table = format!("_ddb_{}", rows[0].1); + let present: Option = + sqlx::query_scalar("SELECT name FROM sqlite_master WHERE type = 'table' AND name = ?") + .bind(&data_table) + .fetch_optional(&engine.pool) + .await + .expect("sqlite_master"); + assert_eq!( + present.as_deref(), + Some(data_table.as_str()), + "round {round}" + ); + } + + engine.pool.close().await; + for suffix in ["", "-wal", "-shm"] { + let _ = std::fs::remove_file(format!("{}{suffix}", path.display())); + } +} diff --git a/crates/storage-sqlite/src/lib.rs b/crates/storage-sqlite/src/lib.rs index 567343611..8398f7cac 100644 --- a/crates/storage-sqlite/src/lib.rs +++ b/crates/storage-sqlite/src/lib.rs @@ -30,6 +30,8 @@ mod create_table; mod credential_store; mod data; mod delete_table; +#[cfg(test)] +mod deleting_window_data_tests; mod hooks; mod management_store; mod metadata; diff --git a/crates/storage-sqlite/src/update_table.rs b/crates/storage-sqlite/src/update_table.rs index d1e0e5769..1cc63a8f7 100644 --- a/crates/storage-sqlite/src/update_table.rs +++ b/crates/storage-sqlite/src/update_table.rs @@ -136,6 +136,11 @@ impl SqliteEngine { // disagree. A NULL stored value means PROVISIONED. let (status, table_id, ks_json, ad_json, stored_billing_mode) = row.ok_or_else(|| StorageError::TableNotFound(input.table_name.clone()))?; + // A DELETING table has its own sentence on the service (measured + // 2026-10-24); every other non-ACTIVE status keeps the existing one. + if status == "DELETING" { + return Err(StorageError::TableBeingDeleted(input.table_name.clone())); + } if status != "ACTIVE" { return Err(StorageError::TableNotActive(input.table_name.clone())); } diff --git a/crates/storage/src/error.rs b/crates/storage/src/error.rs index c936675a5..5565181fd 100755 --- a/crates/storage/src/error.rs +++ b/crates/storage/src/error.rs @@ -10,6 +10,12 @@ pub enum StorageError { TableAlreadyExists(String), #[error("Table is not in ACTIVE state: {0}")] TableNotActive(String), + /// A control-plane change (a second `DeleteTable`, an `UpdateTable`) + /// refused because the table is `DELETING`. Distinct from + /// [`Self::TableNotActive`] because the service words this status + /// differently from the others; the engine owns the sentence. + #[error("Table is being deleted: {0}")] + TableBeingDeleted(String), /// A table name taken by a `CREATING` restore target. Raised by /// `RestoreTableFromBackup` into that name (the engine answers /// `TableInUseException`), distinct from [`Self::TableAlreadyExists`]. diff --git a/docs/design/13-storage-mongodb.md b/docs/design/13-storage-mongodb.md index e04adbd40..2ac7b0952 100644 --- a/docs/design/13-storage-mongodb.md +++ b/docs/design/13-storage-mongodb.md @@ -1036,8 +1036,11 @@ The backend implements every trait in `extenddb-storage`: from `CREATING` to `ACTIVE` once their `status_transition_at` passes. `create_table_impl` (and restore) write `CREATING` with a scheduled transition when `control_plane_delay_seconds` > 0, or - `ACTIVE` directly when it is 0. `delete_table_impl` remains inline - (no `DELETING` transient state). + `ACTIVE` directly when it is 0. `delete_table_impl` marks the + document `DELETING` and finishes the delete in the request, in a task + of its own that a client's timeout does not cancel; the same pass + finishes a delete whose document is still `DELETING` 60 s after the + mark (a step that failed, a process that died). - `ManagementStore`, `AdminStore`, `SettingsStore`, `MetricsStore`, `RateLimitStore` — the catalog trait surface. - `AuthorizationStore` — user/group/role/permissions-boundary/session @@ -1053,7 +1056,8 @@ The backend implements every trait in `extenddb-storage`: - `gsi_backfill_worker` — drain `indexes` rows in `CREATING` state, scan the base collection with a persistent cursor, flip to ACTIVE. - `control_plane_worker` — flip `tables` rows from `CREATING` to - `ACTIVE` once their scheduled `status_transition_at` passes. + `ACTIVE` once their scheduled `status_transition_at` passes, and + finish the deletes left `DELETING` once theirs passes. ## 13. Testing Strategy diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 95d235cff..e936a2737 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -48,6 +48,7 @@ adaptation when switching between ExtendDB and the real service. |------|----------|------| | Table creation delay | Returns `CREATING` immediately; transitions to `ACTIVE` typically within seconds. Same behavior for on-demand and provisioned | Configurable via `control_plane_delay_seconds` runtime setting (default: 5s) | | DeletionProtectionEnabled | Enforced | Enforced (accepted and stored, DeleteTable rejects when enabled) | +| Requests during a table's `DELETING` window | A second `DeleteTable` and an `UpdateTable` answer `ResourceInUseException` `Attempt to change a resource which is still in use: Table is being deleted: `; `PutItem`, `GetItem`, `Query` and `Scan` are served until the table is gone, and a write made in the window goes away with the table (measured 2026-10-25) | The same on PostgreSQL and SQLite, where `DeleteTable` marks the row `DELETING` on the asynchronous path (`control_plane_delay_seconds` of 1 or more) and the data tables stay until the control plane drops them; a request that reaches a data table after the drop answers `ResourceNotFoundException`. On MongoDB the collections are dropped inside the `DeleteTable` call a few milliseconds after the document is marked, so the data plane reads a `DELETING` document as not found (`ResourceNotFoundException`); serving a write there would recreate the collection after its drop. On Cassandra `DeleteTable` is synchronous and never marks the row `DELETING`; the only `DELETING` row on that backend is an abandoned restore target under the restore sweep, whose data is partial, and the data plane reads it as not found | ## Time to Live (TTL) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index f22a2c4d1..78179fb3a 100755 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -357,7 +357,7 @@ If the health check fails, start extenddb. If it succeeds, check your `--endpoin **Cause:** The background transition poller processes status changes when notified by CreateTable/DeleteTable, or on a 60-second defensive sweep. If the server was stopped while a table was in a transitional state, the transition completes on the next server startup. -A second cause, on PostgreSQL only, is a session holding any lock on one of the table's relations (the base data table, a GSI or LSI data table, or a vector-index data table): a reader (a table snapshot, an in-catalog backup, a Scan or Query, `pg_dump`, any client transaction that read the table), an open write transaction, an index build, or `VACUUM`. DeleteTable then returns `DELETING` instead of dropping the tables in the request: at once for a reader, and after about a second for any other holder, which is how long its drops wait before they give up. The poller then checks the table for holders about once a second without waiting on them, and drops it at the first check that finds none, so the table disappears within about a second of the last holder's release however many tables are waiting this way, also when the server was restarted in between. A table snapshot or an in-catalog backup that ends also wakes the poller, which then checks at once. Until then a CreateTable of the same name is refused with `ResourceInUseException`, and a DeleteTable of the table returns its description with `TableStatus: DELETING`. A table that stays `DELETING` for longer has a long-running holder. To find it, list the table's relations on the catalog database: +A second cause, on PostgreSQL only, is a session holding any lock on one of the table's relations (the base data table, a GSI or LSI data table, or a vector-index data table): a reader (a table snapshot, an in-catalog backup, a Scan or Query, `pg_dump`, any client transaction that read the table), an open write transaction, an index build, or `VACUUM`. DeleteTable then returns `DELETING` instead of dropping the tables in the request: at once for a reader, and after about a second for any other holder, which is how long its drops wait before they give up. The poller then checks the table for holders about once a second without waiting on them, and drops it at the first check that finds none, so the table disappears within about a second of the last holder's release however many tables are waiting this way, also when the server was restarted in between. A table snapshot or an in-catalog backup that ends also wakes the poller, which then checks at once. Until then a CreateTable of the same name is refused with `ResourceInUseException`, and a DeleteTable of the table is refused with `ResourceInUseException` (`Table is being deleted: `). A table that stays `DELETING` for longer has a long-running holder. To find it, list the table's relations on the catalog database: ```sql SELECT '_ddb_' || t.table_id FROM tables t diff --git a/tests/conftest.py b/tests/conftest.py index 971547dc8..92c20d547 100755 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -177,7 +177,13 @@ def _create(table_name: str | None = None, **kwargs) -> dict: dynamodb_client.delete_table(TableName=name) except dynamodb_client.exceptions.ResourceNotFoundException: continue - except dynamodb_client.exceptions.ResourceInUseException: + except dynamodb_client.exceptions.ResourceInUseException as exc: + # The test deleted the table and the delete is still running: + # DeleteTable of a DELETING table answers "Table is being deleted". + # Wait for it to go. + if "Table is being deleted" in exc.response["Error"]["Message"]: + wait_for_deleted(dynamodb_client, name) + continue # Table may be UPDATING (e.g., billing mode switch). Wait for # ACTIVE then retry the delete. wait_for_active(dynamodb_client, name) diff --git a/tests/python/test_deleting_window_control_plane.py b/tests/python/test_deleting_window_control_plane.py new file mode 100644 index 000000000..f9566b9f0 --- /dev/null +++ b/tests/python/test_deleting_window_control_plane.py @@ -0,0 +1,243 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Requests during a table's DELETING window. + +Amazon DynamoDB keeps a table describable as ``TableStatus DELETING`` for a +few seconds after ``DeleteTable`` returns. In that window a second +``DeleteTable`` and an ``UpdateTable`` answer ``ResourceInUseException`` with +``Attempt to change a resource which is still in use: Table is being deleted: +`` (measured 2026-10-24), and ``PutItem``, ``GetItem``, ``Query`` and +``Scan`` are served; once the table is gone, ``GetItem`` of an item written in +the window is ``ResourceNotFoundException`` (measured 2026-10-25). + +Against the service these tests run on its natural window. Against ExtendDB on +PostgreSQL the window is held open the way a slow reader would hold it: an +outside transaction keeps ``ACCESS SHARE`` on the table's data table, so the +drop waits and the catalog row stays ``DELETING``. That needs +``EXTENDDB_TEST_PG_ADMIN_CONNECTION_STRING`` (a base URL with no database +component) and the catalog database name (``EXTENDDB_TEST_PG_CATALOG_DB``, +default ``extenddb_catalog``). A ``DeleteTable`` that finds another session +holding the data table marks the row ``DELETING`` and leaves the drop to the +control plane, whatever ``control_plane_delay_seconds`` is, so the row stays +``DELETING`` until the lock is released. Without the connection string, or on a +backend other than PostgreSQL (``EXTENDDB_TEST_BACKEND``, exported by +``devtools/run-tests --backend``), these tests skip on ExtendDB; the +storage-level tests in each backend crate carry the rows there. +""" + +from __future__ import annotations + +import os + +import pytest +from botocore.exceptions import ClientError + +from helpers import unique_name, wait_for_active, wait_for_deleted + +PG_ADMIN = os.environ.get("EXTENDDB_TEST_PG_ADMIN_CONNECTION_STRING", "").rstrip("/") +CATALOG_DB = os.environ.get("EXTENDDB_TEST_PG_CATALOG_DB", "extenddb_catalog") +BACKEND = os.environ.get("EXTENDDB_TEST_BACKEND", "").strip() + + +def _being_deleted(name: str) -> str: + return f"Attempt to change a resource which is still in use: Table is being deleted: {name}" + + +def _catalog_setting(key: str) -> str | None: + import psycopg2 + + with psycopg2.connect(f"{PG_ADMIN}/{CATALOG_DB}") as catalog, catalog.cursor() as cur: + cur.execute("SELECT value FROM settings WHERE key = %s", (key,)) + row = cur.fetchone() + return row[0] if row else None + + +class _HeldLock: + """``ACCESS SHARE`` on ``_ddb_``, held until ``release``.""" + + def __init__(self, table_id: str): + import psycopg2 + + data_db = _catalog_setting("data_database_name") or CATALOG_DB + self.conn = psycopg2.connect(f"{PG_ADMIN}/{data_db}") + cur = self.conn.cursor() + cur.execute(f'LOCK TABLE "_ddb_{table_id}" IN ACCESS SHARE MODE') + + def release(self) -> None: + self.conn.rollback() + self.conn.close() + + +def _skip_unless_window_can_be_held(dynamodb_client, name: str) -> None: + """Skip, after removing the table, when this deployment cannot hold the window.""" + reason = None + if BACKEND and BACKEND != "postgres": + reason = f"the window is held through PostgreSQL only (EXTENDDB_TEST_BACKEND={BACKEND})" + elif not PG_ADMIN: + reason = "EXTENDDB_TEST_PG_ADMIN_CONNECTION_STRING is not set; cannot hold the table DELETING" + if reason is not None: + dynamodb_client.delete_table(TableName=name) + pytest.skip(reason) + + +def _open_window( + dynamodb_client, is_extenddb: bool, name: str, with_index: bool = False +) -> _HeldLock | None: + """Create the table (with a GSI ``gsi1`` on ``gk`` when ``with_index``) and, on + ExtendDB, hold the lock that keeps it DELETING.""" + attributes = [{"AttributeName": "pk", "AttributeType": "S"}] + extra = {} + if with_index: + attributes.append({"AttributeName": "gk", "AttributeType": "S"}) + extra["GlobalSecondaryIndexes"] = [ + { + "IndexName": "gsi1", + "KeySchema": [{"AttributeName": "gk", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + } + ] + dynamodb_client.create_table( + TableName=name, + AttributeDefinitions=attributes, + KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], + BillingMode="PAY_PER_REQUEST", + **extra, + ) + wait_for_active(dynamodb_client, name) + if not is_extenddb: + return None + _skip_unless_window_can_be_held(dynamodb_client, name) + table_id = dynamodb_client.describe_table(TableName=name)["Table"]["TableId"] + return _HeldLock(table_id) + + +def _assert_in_use(exc: ClientError, name: str) -> None: + err = exc.response["Error"] + assert err["Code"] == "ResourceInUseException" + assert err["Message"] == _being_deleted(name) + + +class TestDeletingWindowControlPlane: + def test_second_delete_table_is_being_deleted(self, dynamodb_client, is_extenddb): + """A second DeleteTable while the first is still running is ResourceInUseException.""" + name = unique_name("del-window") + lock = _open_window(dynamodb_client, is_extenddb, name) + try: + resp = dynamodb_client.delete_table(TableName=name) + assert resp["TableDescription"]["TableStatus"] == "DELETING" + with pytest.raises(ClientError) as exc: + dynamodb_client.delete_table(TableName=name) + _assert_in_use(exc.value, name) + finally: + if lock is not None: + lock.release() + wait_for_deleted(dynamodb_client, name, timeout=120.0) + + def test_update_table_is_being_deleted(self, dynamodb_client, is_extenddb): + """UpdateTable while the table is DELETING is ResourceInUseException with the DELETING sentence.""" + name = unique_name("del-window") + lock = _open_window(dynamodb_client, is_extenddb, name) + try: + dynamodb_client.delete_table(TableName=name) + with pytest.raises(ClientError) as exc: + dynamodb_client.update_table(TableName=name, DeletionProtectionEnabled=True) + _assert_in_use(exc.value, name) + finally: + if lock is not None: + lock.release() + wait_for_deleted(dynamodb_client, name, timeout=120.0) + + + def test_tag_changes_are_being_deleted_and_tags_are_listed(self, dynamodb_client, is_extenddb): + """TagResource and UntagResource while the table is DELETING are ResourceInUseException + with the DELETING sentence, and ListTagsOfResource lists the tag the table had before + DeleteTable (measured 2026-11-03).""" + name = unique_name("del-window") + lock = _open_window(dynamodb_client, is_extenddb, name) + try: + arn = dynamodb_client.describe_table(TableName=name)["Table"]["TableArn"] + dynamodb_client.tag_resource(ResourceArn=arn, Tags=[{"Key": "k1", "Value": "v1"}]) + dynamodb_client.delete_table(TableName=name) + with pytest.raises(ClientError) as exc: + dynamodb_client.tag_resource(ResourceArn=arn, Tags=[{"Key": "k2", "Value": "v2"}]) + _assert_in_use(exc.value, name) + with pytest.raises(ClientError) as exc: + dynamodb_client.untag_resource(ResourceArn=arn, TagKeys=["k1"]) + _assert_in_use(exc.value, name) + listed = dynamodb_client.list_tags_of_resource(ResourceArn=arn)["Tags"] + assert listed == [{"Key": "k1", "Value": "v1"}] + finally: + if lock is not None: + lock.release() + wait_for_deleted(dynamodb_client, name, timeout=120.0) + + +class TestDeletingWindowDataPlane: + def test_data_plane_is_served_until_the_table_is_gone(self, dynamodb_client, is_extenddb): + """PutItem, GetItem, Query and Scan are served while the table is DELETING; + GetItem of the item written in the window is ResourceNotFoundException once + the table is gone.""" + name = unique_name("del-window") + lock = _open_window(dynamodb_client, is_extenddb, name) + try: + dynamodb_client.put_item(TableName=name, Item={"pk": {"S": "pre"}}) + resp = dynamodb_client.delete_table(TableName=name) + assert resp["TableDescription"]["TableStatus"] == "DELETING" + + dynamodb_client.put_item(TableName=name, Item={"pk": {"S": "w1"}}) + got = dynamodb_client.get_item( + TableName=name, Key={"pk": {"S": "pre"}}, ConsistentRead=True + ) + assert got["Item"] == {"pk": {"S": "pre"}} + got = dynamodb_client.get_item( + TableName=name, Key={"pk": {"S": "w1"}}, ConsistentRead=True + ) + assert got["Item"] == {"pk": {"S": "w1"}} + queried = dynamodb_client.query( + TableName=name, + KeyConditionExpression="pk = :pk", + ExpressionAttributeValues={":pk": {"S": "pre"}}, + ConsistentRead=True, + ) + assert queried["Items"] == [{"pk": {"S": "pre"}}] + scanned = dynamodb_client.scan(TableName=name, Limit=1, ConsistentRead=True) + assert len(scanned["Items"]) == 1 + finally: + if lock is not None: + lock.release() + wait_for_deleted(dynamodb_client, name, timeout=120.0) + + with pytest.raises(ClientError) as exc: + dynamodb_client.get_item(TableName=name, Key={"pk": {"S": "w1"}}, ConsistentRead=True) + assert exc.value.response["Error"]["Code"] == "ResourceNotFoundException" + + def test_index_reads_after_the_table_is_gone_are_not_found(self, dynamodb_client, is_extenddb): + """Query and Scan on an index of a table that is gone are ResourceNotFoundException, + as GetItem is, after a GetItem during the window: on ExtendDB that read stores the + DELETING table's key info in the key-info cache, and the entry outlives the table.""" + name = unique_name("del-window") + lock = _open_window(dynamodb_client, is_extenddb, name, with_index=True) + try: + resp = dynamodb_client.delete_table(TableName=name) + assert resp["TableDescription"]["TableStatus"] == "DELETING" + got = dynamodb_client.get_item( + TableName=name, Key={"pk": {"S": "absent"}}, ConsistentRead=True + ) + assert "Item" not in got + finally: + if lock is not None: + lock.release() + wait_for_deleted(dynamodb_client, name, timeout=120.0) + + with pytest.raises(ClientError) as exc: + dynamodb_client.query( + TableName=name, + IndexName="gsi1", + KeyConditionExpression="gk = :gk", + ExpressionAttributeValues={":gk": {"S": "x"}}, + ) + assert exc.value.response["Error"]["Code"] == "ResourceNotFoundException" + with pytest.raises(ClientError) as exc: + dynamodb_client.scan(TableName=name, IndexName="gsi1") + assert exc.value.response["Error"]["Code"] == "ResourceNotFoundException" diff --git a/tests/test_table_operations.py b/tests/test_table_operations.py index 568702c1b..ff18739b4 100755 --- a/tests/test_table_operations.py +++ b/tests/test_table_operations.py @@ -177,12 +177,13 @@ def test_delete_protected_table(self, create_and_cleanup_table, dynamodb_client, DeletionProtectionEnabled=False, ) - def test_delete_of_a_deleting_table_returns_its_description( + def test_delete_of_a_deleting_table_is_refused_as_being_deleted( self, create_and_cleanup_table, dynamodb_client, unique_table_name ): - """DeleteTable of a table that is already DELETING returns the table's - description with TableStatus DELETING and no error, as the service does, - and leaves the table as it was. + """DeleteTable of a table that is already DELETING is refused with + ResourceInUseException, as the service refuses it (measured 2026-10-25 + and 2026-10-30, empty tables and tables of 300 items, 0 to 3 s into + the window), and leaves the table as it was. On ExtendDB the DELETING state is written into the catalog directly: on PostgreSQL a reader or writer holding the table leaves it there, and on @@ -192,10 +193,17 @@ def test_delete_of_a_deleting_table_returns_its_description( DELETING, so there the test does not apply. """ create_and_cleanup_table(unique_table_name) + expected = ( + "Attempt to change a resource which is still in use: " + f"Table is being deleted: {unique_table_name}" + ) if _is_real_dynamodb(): dynamodb_client.delete_table(TableName=unique_table_name) - second = dynamodb_client.delete_table(TableName=unique_table_name) - assert second["TableDescription"]["TableStatus"] == "DELETING" + with pytest.raises(ClientError) as exc_info: + dynamodb_client.delete_table(TableName=unique_table_name) + err = exc_info.value.response["Error"] + assert err["Code"] == "ResourceInUseException" + assert err["Message"] == expected return backend = configured_backend() or ( "postgres" if config_value("storage.postgres", "connection_string") else None @@ -207,12 +215,11 @@ def test_delete_of_a_deleting_table_returns_its_description( _set_status(catalog, backend, unique_table_name, "DELETING") try: before = _status_row(catalog, backend, unique_table_name) - second = dynamodb_client.delete_table(TableName=unique_table_name) - desc = second["TableDescription"] - assert desc["TableStatus"] == "DELETING" - assert desc["TableName"] == unique_table_name - assert desc["TableArn"].endswith(f"table/{unique_table_name}") - assert desc["KeySchema"] == [{"AttributeName": "pk", "KeyType": "HASH"}] + with pytest.raises(ClientError) as exc_info: + dynamodb_client.delete_table(TableName=unique_table_name) + err = exc_info.value.response["Error"] + assert err["Code"] == "ResourceInUseException" + assert err["Message"] == expected described = dynamodb_client.describe_table(TableName=unique_table_name) assert described["Table"]["TableStatus"] == "DELETING" assert _status_row(catalog, backend, unique_table_name) == before, (