From 4b7389a101521ab33f610d59314b58ab1f5037da Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:41:45 +0000 Subject: [PATCH 01/14] fix: answer requests during a table's DELETING window as the service does A second DeleteTable and an UpdateTable during a table's DELETING window answer 400 ResourceInUseException with "Attempt to change a resource which is still in use: Table is being deleted: " on all four backends. PutItem, GetItem, Query and Scan are served during the window on PostgreSQL and SQLite and answer ResourceNotFoundException once the table is gone; a write made in the window goes away with the table. Two concurrent DeleteTable calls on one ACTIVE table get one description and one ResourceInUseException on PostgreSQL, SQLite and MongoDB. All measured against the service on 2026-10-24 and 2026-10-25. DeleteTable on PostgreSQL and SQLite looked the name up with a status filter that excluded DELETING rows and answered ResourceNotFoundException. On MongoDB and Cassandra it had no status filter and ran the delete again. UpdateTable on every backend answered "Table is not in ACTIVE state", the sentence for the other non-ACTIVE statuses. The data plane on PostgreSQL and SQLite answered ResourceNotFoundException for the whole window, because the key-info read that authorization and the handlers share mapped a DELETING row to not found. On SQLite the DeleteTable status lookup ran outside the write transaction and on MongoDB the DELETING mark was unconditioned, so two concurrent calls both proceeded. New StorageError::TableBeingDeleted, worded once in the engine's message table and mapped to ResourceInUseException by the DeleteTable, UpdateTable and TTL error mappers. The key-info read on PostgreSQL and SQLite reads a DELETING row as found; a statement or transaction that reaches a data table the control plane has dropped is answered as not found instead of as an internal error. SQLite's DeleteTable looks the row up inside its write transaction and fences the DELETING mark on the table id; MongoDB's mark is an update_one conditioned on the table id and status, and its final delete_one is keyed on both. On PostgreSQL and SQLite the DELETING row exists only on the asynchronous DeleteTable path (control_plane_delay_seconds of 1 or more; the default is 0.25), so on a default deployment the new answers are reachable after #504, which marks the row on both paths. On Cassandra DeleteTable never marks the row, so the check is reachable only for an abandoned restore target, and the data plane there is unchanged. MongoDB drops the collections inside the DeleteTable call a few milliseconds after the mark, so its data plane keeps answering not found during that window. The differences document carries the per-backend shape. Tests: engine unit tests for the three mappers; storage-level tests on each backend for both control-plane requests and for a CREATING table keeping its answer; on PostgreSQL and SQLite, for the four data-plane requests during the window, for a put, a get and the two transaction calls after the data table is dropped under a DELETING row, and for ten rounds of two concurrent DeleteTable calls (also on MongoDB); a dual-target wire test for the PostgreSQL case that holds the data table locked from a second connection, with the data-plane rows, which skips on a server whose delay is under 1 s until #504 lands. The PostgreSQL storage-level CI step runs the new binary. The tests #395 added for that call now expect the service's refusal (12 of 12 runs on 2026-10-30). Closes #505 --- .github/workflows/integration.yml | 11 +- crates/core/src/error/messages.rs | 10 + crates/engine/src/create_table.rs | 3 + crates/engine/src/delete_table.rs | 25 + crates/engine/src/ttl.rs | 28 + crates/engine/src/update_table.rs | 30 + crates/server/src/key_info_cache.rs | 9 +- crates/storage-cassandra/src/delete_table.rs | 14 +- crates/storage-cassandra/src/update_table.rs | 5 + .../tests/direct/deleting_window.rs | 145 +++++ .../tests/direct_integration.rs | 2 + .../src/deleting_window_tests.rs | 296 +++++++++ crates/storage-mongodb/src/table_engine.rs | 58 +- crates/storage-postgres/src/data/ddl.rs | 25 +- .../storage-postgres/src/data/delete_item.rs | 32 +- crates/storage-postgres/src/data/put_item.rs | 32 +- crates/storage-postgres/src/data/query.rs | 12 +- .../storage-postgres/src/data/tx_helpers.rs | 2 +- .../storage-postgres/src/data/update_item.rs | 18 +- crates/storage-postgres/src/delete_table.rs | 28 +- crates/storage-postgres/src/update_table.rs | 5 + .../storage-postgres/tests/deleting_window.rs | 598 ++++++++++++++++++ .../storage-postgres/tests/table_snapshot.rs | 52 +- crates/storage-sqlite/src/data/data_engine.rs | 61 +- crates/storage-sqlite/src/data/ddl.rs | 25 +- crates/storage-sqlite/src/delete_table.rs | 179 +++++- .../src/deleting_window_data_tests.rs | 308 +++++++++ crates/storage-sqlite/src/lib.rs | 2 + crates/storage-sqlite/src/update_table.rs | 5 + crates/storage/src/error.rs | 6 + docs/differences-from-dynamodb.md | 1 + docs/troubleshooting.md | 2 +- .../test_deleting_window_control_plane.py | 189 ++++++ tests/test_table_operations.py | 31 +- 34 files changed, 2044 insertions(+), 205 deletions(-) create mode 100644 crates/storage-cassandra/tests/direct/deleting_window.rs create mode 100644 crates/storage-mongodb/src/deleting_window_tests.rs create mode 100644 crates/storage-postgres/tests/deleting_window.rs create mode 100644 crates/storage-sqlite/src/deleting_window_data_tests.rs create mode 100644 tests/python/test_deleting_window_control_plane.py diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index da6c251d3..28b7b5978 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -300,10 +300,11 @@ 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, 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. + # 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. - 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 @@ -315,7 +316,7 @@ jobs: run: >- cargo test --release -p extenddb-storage-postgres --test vector_control_plane --test put_create_race --test twi_conflict - --test stream_shard_retention --test stream_shard_ownership --test stream_records_page_order --test backup_restore + --test stream_shard_retention --test stream_shard_ownership --test stream_records_page_order --test backup_restore --test deleting_window --test gsi_online_build --test gsi_stale_writer --test table_snapshot # The daemonized server logs to syslog; dump it so server-side failures 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 e9948c111..1f39983d6 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/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 b1a5b426e..479bb86c6 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..4b289870d --- /dev/null +++ b/crates/storage-cassandra/tests/direct/deleting_window.rs @@ -0,0 +1,145 @@ +// 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. + +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"; + +async fn set_status(engine: &CassandraEngine, account_id: &str, status: &str) { + let query = format!( + "UPDATE {}.tables SET table_status = ? 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"); +} + +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") + ); +} + +/// 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:?}" + ); +} + +/// 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:?}" + ); +} diff --git a/crates/storage-cassandra/tests/direct_integration.rs b/crates/storage-cassandra/tests/direct_integration.rs index 7c12889a0..f4ad73ef4 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/deleting_window_tests.rs b/crates/storage-mongodb/src/deleting_window_tests.rs new file mode 100644 index 000000000..c8aeb44a2 --- /dev/null +++ b/crates/storage-mongodb/src/deleting_window_tests.rs @@ -0,0 +1,296 @@ +// 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`. 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. + +use std::sync::Arc; + +use extenddb_core::types::{ + AttributeDefinition, BillingMode, CreateTableInput, DeleteTableInput, KeySchemaElement, + KeyType, ScalarAttributeType, UpdateTableInput, +}; +use extenddb_storage::TableEngine; +use extenddb_storage::error::StorageError; +use mongodb::bson::{Document, doc}; + +use crate::MongoEngine; + +const ACCOUNT: &str = "123456789012"; +const TABLE: &str = "t_deleting_window"; + +fn uri() -> String { + std::env::var("EXTENDDB_TEST_MONGODB_URI").expect( + "set EXTENDDB_TEST_MONGODB_URI to a MongoDB replica set, e.g. \ + mongodb://127.0.0.1:27105/?directConnection=true", + ) +} + +struct Fixture { + engine: MongoEngine, +} + +impl Fixture { + async fn new() -> Self { + 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"); + 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; + } + + 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"); + } + + 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 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] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn a_second_delete_table_during_the_window_is_table_being_deleted() { + let f = Fixture::new().await; + f.open_window().await; + + let err = f + .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!(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] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn update_table_during_the_window_is_table_being_deleted() { + let f = Fixture::new().await; + 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] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn update_table_on_a_creating_table_keeps_table_not_active() { + let f = Fixture::new().await; + 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; +} + +/// 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)] +#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] +async fn two_concurrent_delete_tables_have_one_winner() { + let f = Fixture::new().await; + 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; +} diff --git a/crates/storage-mongodb/src/table_engine.rs b/crates/storage-mongodb/src/table_engine.rs index 77706c3c7..016f7db60 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,15 +600,32 @@ 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. 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! { + "_id": { "account_id": account_id, "table_name": &input.table_name }, + "table_id": &desc.table_id, + "table_status": { "$ne": "DELETING" }, + }, doc! { "$set": { "table_status": "DELETING" } }, ) .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())); + } // Drop the data collection let coll_name = data_collection_name(&desc.table_id); @@ -633,11 +658,14 @@ impl MongoEngine { // 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 + // Delete the table metadata: only the document this call marked, so a + // table created into the same name meanwhile is left alone. tables_coll - .delete_one( - doc! { "_id": { "account_id": account_id, "table_name": &input.table_name } }, - ) + .delete_one(doc! { + "_id": { "account_id": account_id, "table_name": &input.table_name }, + "table_id": &desc.table_id, + "table_status": "DELETING", + }) .await .map_err(|e| StorageError::Internal(e.to_string()))?; @@ -1132,9 +1160,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 +1175,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 +1891,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-postgres/src/data/ddl.rs b/crates/storage-postgres/src/data/ddl.rs index 16f1a3172..cea5df447 100755 --- a/crates/storage-postgres/src/data/ddl.rs +++ b/crates/storage-postgres/src/data/ddl.rs @@ -387,11 +387,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())), @@ -542,11 +548,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 6a84dc1b7..5e20ce3a1 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, @@ -203,10 +205,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()) } @@ -343,10 +342,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 6468f159b..60c09220e 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 80ab9a515..8d1541b81 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) } }; @@ -233,7 +229,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); @@ -260,7 +256,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 } @@ -333,9 +329,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 f03d2c7f0..311393fb0 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..a53c43865 --- /dev/null +++ b/crates/storage-postgres/tests/deleting_window.rs @@ -0,0 +1,598 @@ +// 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("UPDATE settings SET value = '0' WHERE key = 'index_propagation_delay_ms'") + .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, + } +} + +async fn set_delay(catalog: &PgPool, seconds: &str) { + sqlx::query("UPDATE settings SET value = $1 WHERE key = 'control_plane_delay_seconds'") + .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, +/// re-reads the row after the commit, finds it excluded by the status filter, +/// and answers TableBeingDeleted from the unfiltered lookup. 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; +} diff --git a/crates/storage-postgres/tests/table_snapshot.rs b/crates/storage-postgres/tests/table_snapshot.rs index adfad63b2..907041e73 100644 --- a/crates/storage-postgres/tests/table_snapshot.rs +++ b/crates/storage-postgres/tests/table_snapshot.rs @@ -1146,8 +1146,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`. @@ -1180,7 +1181,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) => { @@ -1188,17 +1189,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 { @@ -2978,10 +2981,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; @@ -3003,25 +3006,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, @@ -3031,7 +3019,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..151587896 100644 --- a/crates/storage-sqlite/src/data/data_engine.rs +++ b/crates/storage-sqlite/src/data/data_engine.rs @@ -16,6 +16,49 @@ 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. A data-plane statement +/// also names the shared `stream_shards`, `stream_records`, `gsi_pending` and +/// `vector_index_holds` tables, and an index table a concurrent GSI delete has +/// just dropped; a missing one of those is reported the same way, so an +/// unapplied migration shows as `ResourceNotFoundException` on the wire and as +/// the named table in the server log. +fn dropped_table_as_not_found(key_info: &TableKeyInfo, e: StorageError) -> StorageError { + if super::index::is_no_such_table(&e) { + StorageError::TableNotFound(key_info.table_name.clone()) + } else { + e + } +} + +/// 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 +90,7 @@ impl DataEngine for SqliteEngine { stream.as_ref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -57,7 +101,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 +132,7 @@ impl DataEngine for SqliteEngine { stream.as_ref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -116,6 +165,7 @@ impl DataEngine for SqliteEngine { stream.as_ref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -145,6 +195,7 @@ impl DataEngine for SqliteEngine { index_name.as_deref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -170,6 +221,7 @@ impl DataEngine for SqliteEngine { index_name.as_deref(), ) .await + .map_err(|e| dropped_table_as_not_found(&key_info, e)) }) } @@ -209,7 +261,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 +283,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 +293,7 @@ impl DataEngine for SqliteEngine { }), ) .await + .map_err(|e| transaction_dropped_table_as_not_found(first, 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 476177d6c..0086d0a89 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,27 +140,28 @@ 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?; - sqlx::query( + // Fenced on the table id read above, so the mark can only land on + // the table this call described, never on a table created into the + // same name afterwards. + let marked = 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()))?; + if marked.rows_affected() == 0 { + return Err(StorageError::TableNotFound(input.table_name.clone())); + } tx.commit() .await .map_err(|e| StorageError::Internal(e.to_string()))?; @@ -209,3 +205,118 @@ 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::schema::apply(&engine.pool).await.expect("schema"); + sqlx::query("UPDATE settings SET value = '0' WHERE key = 'control_plane_delay_seconds'") + .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..afb850a68 --- /dev/null +++ b/crates/storage-sqlite/src/deleting_window_data_tests.rs @@ -0,0 +1,308 @@ +// 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") +} + +async fn seed(engine: &SqliteEngine) { + crate::schema::apply(&engine.pool).await.expect("schema"); + sqlx::query("UPDATE settings SET value = '0' WHERE key = 'control_plane_delay_seconds'") + .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"); +} + +/// 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() { + let target = std::env::var_os("CARGO_TARGET_DIR") + .map(std::path::PathBuf::from) + .unwrap_or_else(|| { + std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../target") + }); + 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}"); + sqlx::query("UPDATE settings SET value = '0' WHERE key = 'control_plane_delay_seconds'") + .execute(&engine.pool) + .await + .expect("delay 0 so the create completes inline"); + engine + .create_table_impl(ACCOUNT, create_input(&table), false) + .await + .expect("create table"); + sqlx::query("UPDATE settings SET value = '600' WHERE key = 'control_plane_delay_seconds'") + .execute(&engine.pool) + .await + .expect("delay 600 so the delete marks the row"); + + 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 7e8085ff9..29dd5e2ea 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 8adff6173..d3663565d 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 8fe3ac287..443d54168 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/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 80c1f671b..3cb166080 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 11a35c1b5..cd786d60c 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/python/test_deleting_window_control_plane.py b/tests/python/test_deleting_window_control_plane.py new file mode 100644 index 000000000..b6837856f --- /dev/null +++ b/tests/python/test_deleting_window_control_plane.py @@ -0,0 +1,189 @@ +# 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``), and a server whose ``DeleteTable`` marks the row +``DELETING`` before the data drop. Before #504 that is the asynchronous path +only, taken when ``control_plane_delay_seconds`` is 1 or more; on the +immediate path the catalog row is removed first and the drop waits on the held +lock inside the request, so the tests read the server's delay from the +``settings`` table and skip when it is under 1. #504 marks the row on both +paths and removes that condition. 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" + else: + delay = _catalog_setting("control_plane_delay_seconds") + try: + delay_secs = float(delay) if delay is not None else 0.25 + except ValueError: + delay_secs = 0.25 + if delay_secs < 1.0: + reason = ( + f"control_plane_delay_seconds is {delay}: DeleteTable removes the catalog row " + "before the drop, so a held lock blocks the request rather than holding the row " + "DELETING (#504 removes this condition)" + ) + if reason is not None: + dynamodb_client.delete_table(TableName=name) + pytest.skip(reason) + + +def _open_window(dynamodb_client, is_extenddb: bool, name: str) -> _HeldLock | None: + """Create the table and, on ExtendDB, hold the lock that keeps it DELETING.""" + dynamodb_client.create_table( + TableName=name, + AttributeDefinitions=[{"AttributeName": "pk", "AttributeType": "S"}], + KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], + BillingMode="PAY_PER_REQUEST", + ) + 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) + + +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" 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, ( From 5b4c9ff49f4fac8e1e0a0a1aace2027f38447dbc Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:41:45 +0000 Subject: [PATCH 02/14] test(cassandra): keep the DELETING-window rows out of the control-plane sweep The three deleting_window tests set a fresh table's catalog row to DELETING to produce the window. A fresh table carries its activation deadline in status_transition_at, and process_control_plane_transitions removes any DELETING row whose deadline has passed: it clears the table's TTL entries in the account keyspace, drops the indexes and the data table, and deletes the row. The tests left their rows in place, so the next test in the binary to run the sweep after their account keyspaces were dropped failed on the missing keyspace. On CI that was index::test_ttl_on_a_table_with_an_async_gsi_is_refused_as_a_validation_error in the cassandra-storage job. The sequence reproduces locally with --test-threads=1 on deleting_window, groups, then that test. set_status now clears status_transition_at together with the status, the column activation clears, so the sweep's deadline filter excludes the row for the rest of the test. Each test deletes its row before it returns. The same sequence passes with the change, the full direct_integration binary passes in parallel (152 of 152), and the catalog holds no DeletingWindowTable row afterwards. --- .../tests/direct/deleting_window.rs | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/crates/storage-cassandra/tests/direct/deleting_window.rs b/crates/storage-cassandra/tests/direct/deleting_window.rs index 4b289870d..cc43f7da4 100644 --- a/crates/storage-cassandra/tests/direct/deleting_window.rs +++ b/crates/storage-cassandra/tests/direct/deleting_window.rs @@ -11,6 +11,17 @@ //! 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; @@ -21,9 +32,12 @@ 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 = ? WHERE account_id = ? AND table_name = ? IF EXISTS", + "UPDATE {}.tables SET table_status = ?, status_transition_at = null \ + WHERE account_id = ? AND table_name = ? IF EXISTS", engine.catalog_keyspace() ); engine @@ -33,6 +47,20 @@ async fn set_status(engine: &CassandraEngine, account_id: &str, status: &str) { .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!( @@ -103,6 +131,7 @@ async fn a_second_delete_table_during_the_window_is_table_being_deleted() { 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 @@ -122,6 +151,7 @@ async fn update_table_during_the_window_is_table_being_deleted() { 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 @@ -142,4 +172,5 @@ async fn update_table_on_a_creating_table_keeps_table_not_active() { matches!(&err, StorageError::TableNotActive(name) if name == TABLE), "expected TableNotActive({TABLE:?}), got {err:?}" ); + remove_row(&engine, &account.account_id).await; } From e5839127f83f1aad7ac9413ec4a735a8b88cd6f7 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:22:15 +0000 Subject: [PATCH 03/14] fix(mongodb): finish a DeleteTable that stops after its DELETING mark A second DeleteTable during the DELETING window is refused, as the service refuses it, so on MongoDB nothing re-ran the steps of a delete that stopped between the mark and the removal of the table document. A client read timeout dropped the request future and the steps with it, and a crash or a failed drop did the same. The document then stayed DELETING and CreateTable of the name answered "Table already exists". The steps after the mark now run in a task of their own that the request awaits. Dropping the request detaches the task instead of cancelling it, so a delete stalled behind a transaction finishes once the transaction ends. The mark also writes a status_transition_at 60 s ahead, and the control-plane worker finishes every DELETING document whose time has passed, or that has none (a document an earlier build left). The worker claims a document by moving its time forward, conditioned on the value it read, so one process runs the steps at a time and a step that keeps failing is retried once a minute. The steps are idempotent: drop the data and index collections, remove the tags and the stream state, then delete only this table id's DELETING document. The DELETING window tests no longer carry #[ignore]: they skip without EXTENDDB_TEST_MONGODB_URI unless EXTENDDB_TEST_MONGODB_REQUIRED is set, and the MongoDB workflow runs them against its replica set. --- .github/workflows/integration-mongodb.yml | 8 + .../src/deleting_window_tests.rs | 320 ++++++++++++++++-- crates/storage-mongodb/src/table_engine.rs | 77 +++-- crates/storage-mongodb/src/ttl_worker.rs | 6 +- crates/storage-mongodb/src/worker_store.rs | 123 ++++++- docs/design/13-storage-mongodb.md | 10 +- 6 files changed, 487 insertions(+), 57 deletions(-) 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/crates/storage-mongodb/src/deleting_window_tests.rs b/crates/storage-mongodb/src/deleting_window_tests.rs index c8aeb44a2..b2994a509 100644 --- a/crates/storage-mongodb/src/deleting_window_tests.rs +++ b/crates/storage-mongodb/src/deleting_window_tests.rs @@ -4,7 +4,9 @@ //! 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`. Each test uses its own +//! `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 @@ -12,28 +14,40 @@ //! 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. +//! 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, UpdateTableInput, + KeyType, ScalarAttributeType, TableDescription, Tag, UpdateTableInput, }; -use extenddb_storage::TableEngine; use extenddb_storage::error::StorageError; -use mongodb::bson::{Document, doc}; +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"; -fn uri() -> String { - std::env::var("EXTENDDB_TEST_MONGODB_URI").expect( - "set EXTENDDB_TEST_MONGODB_URI to a MongoDB replica set, e.g. \ - mongodb://127.0.0.1:27105/?directConnection=true", - ) +/// 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 { @@ -41,9 +55,17 @@ struct Fixture { } impl Fixture { - async fn new() -> Self { + /// `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 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(), @@ -63,7 +85,7 @@ impl Fixture { .insert_one(doc! { "_id": "control_plane_delay_seconds", "value": "0" }) .await .expect("settings"); - Self { engine } + Some(Self { engine }) } /// Create `TABLE` and set its document to `DELETING`: the window. @@ -90,6 +112,113 @@ impl Fixture { 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 @@ -102,6 +231,20 @@ impl Fixture { .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 @@ -118,6 +261,19 @@ impl Fixture { } } +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(), @@ -137,19 +293,15 @@ fn update_input() -> UpdateTableInput { /// 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] -#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] async fn a_second_delete_table_during_the_window_is_table_being_deleted() { - let f = Fixture::new().await; + let Some(f) = Fixture::new().await else { + return; + }; f.open_window().await; let err = f .engine - .delete_table( - ACCOUNT, - DeleteTableInput { - table_name: TABLE.to_owned(), - }, - ) + .delete_table(ACCOUNT, delete_input()) .await .expect_err("DeleteTable on a DELETING table"); assert!( @@ -164,9 +316,10 @@ async fn a_second_delete_table_during_the_window_is_table_being_deleted() { /// 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] -#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] async fn update_table_during_the_window_is_table_being_deleted() { - let f = Fixture::new().await; + let Some(f) = Fixture::new().await else { + return; + }; f.open_window().await; let err = f @@ -198,9 +351,10 @@ async fn update_table_during_the_window_is_table_being_deleted() { /// A CREATING table keeps the existing answer: the new sentence is for /// DELETING only. #[tokio::test] -#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] async fn update_table_on_a_creating_table_keeps_table_not_active() { - let f = Fixture::new().await; + let Some(f) = Fixture::new().await else { + return; + }; f.open_window().await; f.set_status("CREATING").await; @@ -217,15 +371,127 @@ async fn update_table_on_a_creating_table_keeps_table_not_active() { 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; + } +} + +/// 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)] -#[ignore = "needs a MongoDB replica set in EXTENDDB_TEST_MONGODB_URI"] async fn two_concurrent_delete_tables_have_one_winner() { - let f = Fixture::new().await; + let Some(f) = Fixture::new().await else { + return; + }; let engine = Arc::new(f.engine.clone()); for round in 0..10 { diff --git a/crates/storage-mongodb/src/table_engine.rs b/crates/storage-mongodb/src/table_engine.rs index 016f7db60..a3302ea8a 100644 --- a/crates/storage-mongodb/src/table_engine.rs +++ b/crates/storage-mongodb/src/table_engine.rs @@ -604,7 +604,9 @@ impl MongoEngine { // 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. + // 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"); let marked = tables_coll .update_one( @@ -613,7 +615,10 @@ impl MongoEngine { "table_id": &desc.table_id, "table_status": { "$ne": "DELETING" }, }, - doc! { "$set": { "table_status": "DELETING" } }, + doc! { "$set": { + "table_status": "DELETING", + "status_transition_at": crate::worker_store::delete_finish_due(), + } }, ) .await .map_err(|e| StorageError::Internal(e.to_string()))?; @@ -627,9 +632,47 @@ impl MongoEngine { 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 tags and the stream + /// state, then the document. `DeleteTable` runs them in the request, and + /// the control plane runs them for a document a `DeleteTable` left + /// `DELETING` (`worker_store.rs`). Each step is idempotent, so running + /// them again after a failure, or from both at once, removes what is left + /// and changes nothing else. + 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 @@ -638,41 +681,37 @@ 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?; + self.drop_index_collections_for_table(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 }) + .delete_many(doc! { "resource_arn": table_arn }) .await .map_err(|e| StorageError::Internal(e.to_string()))?; - 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?; + self.cleanup_stream_state_for_table(table_id).await?; - // Delete the table metadata: only the document this call marked, so a - // table created into the same name meanwhile is left alone. - tables_coll + // Delete the table metadata: only the DELETING document of this table + // id, so a table created into the same name meanwhile is left alone. + self.catalog_db + .collection::("tables") .delete_one(doc! { - "_id": { "account_id": account_id, "table_name": &input.table_name }, - "table_id": &desc.table_id, + "_id": { "account_id": account_id, "table_name": table_name }, + "table_id": table_id, "table_status": "DELETING", }) .await .map_err(|e| StorageError::Internal(e.to_string()))?; - - Ok(TableDescription { - table_status: TableStatus::Deleting, - ..desc - }) + Ok(()) } pub(crate) async fn describe_table_impl( 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..c43c61cca 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,83 @@ 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 processes sharing + /// the catalog one runs the steps at a time, and a step that keeps + /// failing is retried 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 +209,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/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 From 1de1816a5efab50b9f4ef3c7fc407009fff88703 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:14 +0000 Subject: [PATCH 04/14] test(postgres): describe the concurrent DeleteTable test as the code runs The second DeleteTable reads the row again after the first commits, finds it DELETING and answers TableBeingDeleted. Since the rebase on #395 there is no status filter on that select and no second lookup. --- crates/storage-postgres/tests/deleting_window.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/storage-postgres/tests/deleting_window.rs b/crates/storage-postgres/tests/deleting_window.rs index f81e9ee38..68a7d44fb 100644 --- a/crates/storage-postgres/tests/deleting_window.rs +++ b/crates/storage-postgres/tests/deleting_window.rs @@ -541,9 +541,9 @@ async fn a_request_after_the_data_drop_is_table_not_found() { /// 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, -/// re-reads the row after the commit, finds it excluded by the status filter, -/// and answers TableBeingDeleted from the unfiltered lookup. Ten rounds, a -/// fresh table each; one row per table, DELETING, and its data table present. +/// 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() { From 040be0de61d83ad2be0e08f7411af77ea03839ce Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:14 +0000 Subject: [PATCH 05/14] fix(postgres): keep an abandoned restore target not found while it is removed The restore sweep claims an abandoned restore target by marking its row DELETING, then drops its data tables. Once the data plane read a DELETING row as found, a partly copied target was served for as long as the drop waited. The claim now sets the row's transition time to the epoch, a time no DeleteTable mark carries: DeleteTable schedules the removal at or after the time it runs. The key-info and index lookups read a DELETING row with that transition time as CREATING, so the target stays ResourceNotFoundException, as a table the service is still restoring is. The control plane still finds the row due, so a drop that fails is finished later as before. --- crates/storage-postgres/src/backup_engine.rs | 8 ++- crates/storage-postgres/src/data/ddl.rs | 24 ++++--- .../storage-postgres/tests/deleting_window.rs | 63 +++++++++++++++++++ 3 files changed, 87 insertions(+), 8 deletions(-) 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 f0ca377ce..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) @@ -535,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) diff --git a/crates/storage-postgres/tests/deleting_window.rs b/crates/storage-postgres/tests/deleting_window.rs index 68a7d44fb..d7aa3a2c4 100644 --- a/crates/storage-postgres/tests/deleting_window.rs +++ b/crates/storage-postgres/tests/deleting_window.rs @@ -604,3 +604,66 @@ async fn two_concurrent_delete_tables_have_one_winner() { 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; +} From acb03bfd1c3f7fa3aad6d28e7c5830a2ff9652d9 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:14 +0000 Subject: [PATCH 06/14] fix(sqlite): answer only a missing _ddb_ table as a table that is gone The funnel that answers a statement reaching a dropped data table as TableNotFound matched any "no such table". With stream_records missing on a stream-enabled ACTIVE table, PutItem answered 400 ResourceNotFoundException with no log line, where main answered 500 and logged the missing table at ERROR. The funnel now matches only this backend's data and index tables, all named _ddb_, as PostgreSQL's data_table_missing matches its own. A missing shared table stays an internal error. --- crates/storage-sqlite/src/data/data_engine.rs | 69 +++++++++++++++++-- 1 file changed, 62 insertions(+), 7 deletions(-) diff --git a/crates/storage-sqlite/src/data/data_engine.rs b/crates/storage-sqlite/src/data/data_engine.rs index 151587896..725abb351 100644 --- a/crates/storage-sqlite/src/data/data_engine.rs +++ b/crates/storage-sqlite/src/data/data_engine.rs @@ -22,20 +22,29 @@ use crate::store::SqliteEngine; /// `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. A data-plane statement -/// also names the shared `stream_shards`, `stream_records`, `gsi_pending` and -/// `vector_index_holds` tables, and an index table a concurrent GSI delete has -/// just dropped; a missing one of those is reported the same way, so an -/// unapplied migration shows as `ResourceNotFoundException` on the wire and as -/// the named table in the server log. +/// `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 super::index::is_no_such_table(&e) { + 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 @@ -476,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:?}" + ); + } + } +} From 9f87c9613f05eb984c85285b0fbc723a3ca3e586 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:14 +0000 Subject: [PATCH 07/14] refactor(sqlite): drop the unreachable no-row arm after the DELETING mark DeleteTable's status select and the DELETING mark run in one BEGIN IMMEDIATE transaction, so the mark always updates the row the select read, and the rows_affected() == 0 arm after it could not be reached. --- crates/storage-sqlite/src/delete_table.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/storage-sqlite/src/delete_table.rs b/crates/storage-sqlite/src/delete_table.rs index a5d0f7781..b587747a2 100644 --- a/crates/storage-sqlite/src/delete_table.rs +++ b/crates/storage-sqlite/src/delete_table.rs @@ -145,10 +145,10 @@ impl SqliteEngine { // 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?; - // Fenced on the table id read above, so the mark can only land on - // the table this call described, never on a table created into the - // same name afterwards. - let marked = sqlx::query( + // 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 = ? AND table_id = ?", ) @@ -159,9 +159,6 @@ impl SqliteEngine { .execute(&mut *tx) .await .map_err(|e| StorageError::Internal(e.to_string()))?; - if marked.rows_affected() == 0 { - return Err(StorageError::TableNotFound(input.table_name.clone())); - } tx.commit() .await .map_err(|e| StorageError::Internal(e.to_string()))?; From 8156c69d9ab90036834e1a773cdaf062a0e35228 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:15 +0000 Subject: [PATCH 08/14] test(sqlite): find the cargo target directory from the test binary's path The concurrent DeleteTable test read CARGO_TARGET_DIR and fell back to /../../target, so a relative CARGO_TARGET_DIR or a --target-dir put its database file in the wrong place or failed the existence check. It now takes the target directory from the test binary's own path, as the file engine in snapshot_engine does. --- .../src/deleting_window_data_tests.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/storage-sqlite/src/deleting_window_data_tests.rs b/crates/storage-sqlite/src/deleting_window_data_tests.rs index a47e4ac96..09ab59531 100644 --- a/crates/storage-sqlite/src/deleting_window_data_tests.rs +++ b/crates/storage-sqlite/src/deleting_window_data_tests.rs @@ -238,11 +238,15 @@ async fn a_request_after_the_data_drop_is_table_not_found() { /// here to finish the deletion). #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn two_concurrent_delete_tables_have_one_winner() { - let target = std::env::var_os("CARGO_TARGET_DIR") - .map(std::path::PathBuf::from) - .unwrap_or_else(|| { - std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../target") - }); + // 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( From 657e1c0fe447f617d86bc17f12584a87040a6f43 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:15 +0000 Subject: [PATCH 09/14] test: run the DELETING window wire tests at any control-plane delay 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 held lock keeps the row DELETING at the CI job's 0.05 s as well. The tests no longer skip below 1 s. --- .../test_deleting_window_control_plane.py | 23 ++++--------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/tests/python/test_deleting_window_control_plane.py b/tests/python/test_deleting_window_control_plane.py index b6837856f..a9135bc71 100644 --- a/tests/python/test_deleting_window_control_plane.py +++ b/tests/python/test_deleting_window_control_plane.py @@ -17,13 +17,10 @@ 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``), and a server whose ``DeleteTable`` marks the row -``DELETING`` before the data drop. Before #504 that is the asynchronous path -only, taken when ``control_plane_delay_seconds`` is 1 or more; on the -immediate path the catalog row is removed first and the drop waits on the held -lock inside the request, so the tests read the server's delay from the -``settings`` table and skip when it is under 1. #504 marks the row on both -paths and removes that condition. Without the connection string, or on a +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. @@ -79,18 +76,6 @@ def _skip_unless_window_can_be_held(dynamodb_client, name: str) -> None: 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" - else: - delay = _catalog_setting("control_plane_delay_seconds") - try: - delay_secs = float(delay) if delay is not None else 0.25 - except ValueError: - delay_secs = 0.25 - if delay_secs < 1.0: - reason = ( - f"control_plane_delay_seconds is {delay}: DeleteTable removes the catalog row " - "before the drop, so a held lock blocks the request rather than holding the row " - "DELETING (#504 removes this condition)" - ) if reason is not None: dynamodb_client.delete_table(TableName=name) pytest.skip(reason) From 3a0ee7d0f421a485ee756b510d0f3898ab0fd295 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:15 +0000 Subject: [PATCH 10/14] test: wait for a table being deleted at fixture teardown create_and_cleanup_table treated every ResourceInUseException from its DeleteTable as an UPDATING table and waited for ACTIVE. A test that had deleted its table itself got "Table is being deleted", then ResourceNotFoundException from DescribeTable inside wait_for_active, so test_delete_existing_table errored at teardown whenever the table was still DELETING. On that sentence the fixture now waits for the table to go. --- tests/conftest.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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) From f2a777a46174bfbbc09685090a9b2700456e43f0 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:15 +0000 Subject: [PATCH 11/14] fix(engine): answer an index read on a table that is gone as not found On PostgreSQL and SQLite a data-plane read during the DELETING window stores the table's key info in the key-info cache, and nothing drops the entry when the control plane removes the table. A base-table read then reaches the dropped data table and answers ResourceNotFoundException. A Query or Scan with IndexName looked the index up by the cached table id instead, found no index rows, and answered "The table does not have the specified index" until the entry expired, about 30 s later. When the index name does not resolve, Query and Scan now ask the catalog for the table again, uncached. A table it no longer has is answered ResourceNotFoundException and its cache entry is dropped. A table it still has keeps the ValidationException. The extra read runs only for an index name that does not resolve. --- crates/engine/src/index_helpers.rs | 32 ++++++++++++ crates/engine/src/query.rs | 4 +- crates/engine/src/scan.rs | 5 +- .../test_deleting_window_control_plane.py | 51 +++++++++++++++++-- 4 files changed, 86 insertions(+), 6 deletions(-) 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/tests/python/test_deleting_window_control_plane.py b/tests/python/test_deleting_window_control_plane.py index a9135bc71..6dbb2b27b 100644 --- a/tests/python/test_deleting_window_control_plane.py +++ b/tests/python/test_deleting_window_control_plane.py @@ -81,13 +81,28 @@ def _skip_unless_window_can_be_held(dynamodb_client, name: str) -> None: pytest.skip(reason) -def _open_window(dynamodb_client, is_extenddb: bool, name: str) -> _HeldLock | None: - """Create the table and, on ExtendDB, hold the lock that keeps it DELETING.""" +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=[{"AttributeName": "pk", "AttributeType": "S"}], + AttributeDefinitions=attributes, KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], BillingMode="PAY_PER_REQUEST", + **extra, ) wait_for_active(dynamodb_client, name) if not is_extenddb: @@ -172,3 +187,33 @@ def test_data_plane_is_served_until_the_table_is_gone(self, dynamodb_client, is_ 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" From 97b4befe9ec07982ee33923f80f7f5780f32ad32 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:41:16 +0000 Subject: [PATCH 12/14] fix(engine): refuse TagResource and UntagResource while a table is DELETING Amazon DynamoDB answers TagResource and UntagResource on a DELETING table with ResourceInUseException "Attempt to change a resource which is still in use: Table is being deleted: ", and serves ListTagsOfResource (measured 2026-11-03). Before this branch ExtendDB answered the first two ResourceNotFoundException. Once the data plane read a DELETING table as found, PostgreSQL and SQLite answered them 200, since the tag path checks existence through the same key-info read. The tag handlers now read the table's status with describe_table first and refuse both calls on a DELETING table with that sentence. ListTagsOfResource keeps the existence check alone. --- crates/engine/src/tagging.rs | 58 +++++++++++++++++-- .../test_deleting_window_control_plane.py | 21 +++++++ 2 files changed, 75 insertions(+), 4 deletions(-) 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/tests/python/test_deleting_window_control_plane.py b/tests/python/test_deleting_window_control_plane.py index 6dbb2b27b..83001ce8d 100644 --- a/tests/python/test_deleting_window_control_plane.py +++ b/tests/python/test_deleting_window_control_plane.py @@ -149,6 +149,27 @@ def test_update_table_is_being_deleted(self, dynamodb_client, is_extenddb): 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 is served (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.delete_table(TableName=name) + with pytest.raises(ClientError) as exc: + dynamodb_client.tag_resource(ResourceArn=arn, Tags=[{"Key": "k", "Value": "v"}]) + _assert_in_use(exc.value, name) + with pytest.raises(ClientError) as exc: + dynamodb_client.untag_resource(ResourceArn=arn, TagKeys=["k"]) + _assert_in_use(exc.value, name) + assert dynamodb_client.list_tags_of_resource(ResourceArn=arn)["Tags"] == [] + 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; From 34a9c9a4b1de07e6c836c6b2dfcd821fc08a25a9 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:02:27 +0000 Subject: [PATCH 13/14] fix(mongodb): remove a deleted table's tags only with its document Two runs of one delete's steps can overlap: the DeleteTable request's own task, still waiting on a drop when the mark's due time passes, and the control plane's pass, which finishes a document still DELETING 60 s after its mark. The tag step deleted by resource_arn, and a table created into the same name has the same ARN. A run that reached the tag step after the other run had removed the document and the name had been created again deleted the new table's tags. The other steps are keyed by the table id and leave the new table alone. The last step is now one transaction that deletes this table id's DELETING document and, only when that removed it, the tags of its ARN. As in the old order, no tag outlives the document: both go in one commit. A write conflict on the transaction means another run is claiming or removing the document, so the run answers Ok(()) and leaves the document to that run, or to the control plane if it stays DELETING. Any other error is internal, as before. The doc comments of finish_table_delete and finish_due_deletes, and the message of e5839127f, said the claim lets one process run the steps at a time and that a second run changes nothing else. The comments now say that passes claim a document one at a time, that a DeleteTable whose own task is still running runs the same steps alongside the pass, and that the tags are the one step keyed by the ARN. a_late_second_run_of_a_delete_leaves_a_new_table_of_the_name_whole runs the steps of a removed table again after the name is created again and checks the new table's document, collections, index document, tag and stream shards. a_run_that_meets_another_removing_the_document_leaves_it_to_that_run holds another run's transaction open with the document and the tag deleted, and checks that the run answers Ok(()) and that nothing is left once the other run commits. --- crates/storage-mongodb/src/data_engine.rs | 2 +- .../src/deleting_window_tests.rs | 96 +++++++++++++++++++ crates/storage-mongodb/src/table_engine.rs | 81 ++++++++++++---- crates/storage-mongodb/src/worker_store.rs | 13 +-- 4 files changed, 164 insertions(+), 28 deletions(-) diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index ab297a544..ee689b1d1 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -3565,7 +3565,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 index b2994a509..0d2940cc3 100644 --- a/crates/storage-mongodb/src/deleting_window_tests.rs +++ b/crates/storage-mongodb/src/deleting_window_tests.rs @@ -460,6 +460,50 @@ async fn the_control_plane_finishes_a_delete_left_deleting() { } } +/// 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] @@ -560,3 +604,55 @@ async fn two_concurrent_delete_tables_have_one_winner() { 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 a3302ea8a..86851382e 100644 --- a/crates/storage-mongodb/src/table_engine.rs +++ b/crates/storage-mongodb/src/table_engine.rs @@ -657,12 +657,17 @@ impl MongoEngine { } /// The steps of a `DeleteTable` after its document is marked `DELETING`: - /// drop the data and index collections, remove the tags and the stream - /// state, then the document. `DeleteTable` runs them in the request, and - /// the control plane runs them for a document a `DeleteTable` left - /// `DELETING` (`worker_store.rs`). Each step is idempotent, so running - /// them again after a failure, or from both at once, removes what is left - /// and changes nothing else. + /// 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, @@ -683,15 +688,6 @@ impl MongoEngine { // `_ddb_` collections. self.drop_index_collections_for_table(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": table_arn }) - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; - self.gsi_cache_invalidate(table_id); // Delete stream_shards, stream_records, and their sequence counters @@ -700,18 +696,61 @@ impl MongoEngine { // table's stream history. RFC-0003 §8.2 (table-name reuse). self.cleanup_stream_state_for_table(table_id).await?; - // Delete the table metadata: only the DELETING document of this table - // id, so a table created into the same name meanwhile is left alone. - self.catalog_db + // 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 + { + Ok(()) => Ok(()), + Err(e) if crate::data_engine::is_transient_write_conflict(&e) => Ok(()), + Err(e) => Err(StorageError::Internal(e.to_string())), + } + } + + /// 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", }) - .await - .map_err(|e| StorageError::Internal(e.to_string()))?; - Ok(()) + .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( diff --git a/crates/storage-mongodb/src/worker_store.rs b/crates/storage-mongodb/src/worker_store.rs index c43c61cca..b5d54bb17 100644 --- a/crates/storage-mongodb/src/worker_store.rs +++ b/crates/storage-mongodb/src/worker_store.rs @@ -83,12 +83,13 @@ impl MongoEngine { /// ([`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 processes sharing - /// the catalog one runs the steps at a time, and a step that keeps - /// failing is retried 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. + /// 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 From 4e1ebe00ac4f4cd76c976b3ea2336371fad32953 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:07:25 +0000 Subject: [PATCH 14/14] test: check that ListTagsOfResource lists a DELETING table's tag test_tag_changes_are_being_deleted_and_tags_are_listed checked that ListTagsOfResource answered an empty list for a table it never tagged, which also passes when the window loses the table's tags. The test now tags the table with k1/v1 before DeleteTable, asks UntagResource to remove k1 during the window, and checks that ListTagsOfResource still lists k1/v1, as the service did on 2026-11-03. PostgreSQL and SQLite keep the tag rows until the control plane removes the table. --- tests/python/test_deleting_window_control_plane.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/python/test_deleting_window_control_plane.py b/tests/python/test_deleting_window_control_plane.py index 83001ce8d..f9566b9f0 100644 --- a/tests/python/test_deleting_window_control_plane.py +++ b/tests/python/test_deleting_window_control_plane.py @@ -151,19 +151,22 @@ def test_update_table_is_being_deleted(self, dynamodb_client, is_extenddb): 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 is served (measured 2026-11-03).""" + 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": "k", "Value": "v"}]) + 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=["k"]) + dynamodb_client.untag_resource(ResourceArn=arn, TagKeys=["k1"]) _assert_in_use(exc.value, name) - assert dynamodb_client.list_tags_of_resource(ResourceArn=arn)["Tags"] == [] + listed = dynamodb_client.list_tags_of_resource(ResourceArn=arn)["Tags"] + assert listed == [{"Key": "k1", "Value": "v1"}] finally: if lock is not None: lock.release()