From 955aadbf1094c0fa645bc0ea23cc8433bb638a00 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 00:08:15 +0200 Subject: [PATCH] fix(datatables): only drop external databases Windmill marked, and check use under the lock Co-Authored-By: Claude Opus 5 (1M context) --- backend/windmill-api-settings/src/lib.rs | 16 ++---- .../windmill-api-workspaces/src/workspaces.rs | 5 ++ .../src/workspaces_extra.rs | 2 +- .../src/external_instance_pg.rs | 53 +++++++++++++++++-- .../src/external_instance_pg_oss.rs | 1 + 5 files changed, 59 insertions(+), 18 deletions(-) diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 0f2514632b..fc627f115f 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -1886,18 +1886,10 @@ async fn drop_external_instance_pg_database( ) -> JsonResult<()> { require_super_admin(&db, &authed).await?; // A data table naming a dropped database fails on every job, far from the drop that caused it. - if let Some(workspaces) = - windmill_common::external_instance_pg::external_instance_database_usages(&db) - .await? - .remove(dbname.trim()) - { - return Err(error::Error::BadRequest(format!( - "Database '{dbname}' is still used by data tables in {}", - workspaces.into_iter().collect::>().join(", ") - ))); - } - windmill_common::external_instance_pg::drop_external_instance_database_unchecked(&db, &dbname) - .await?; + windmill_common::external_instance_pg::drop_external_instance_database_unchecked( + &db, &dbname, true, + ) + .await?; windmill_audit::audit_oss::audit_log( &db, &authed, diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index d40239fe52..0908343fff 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -4037,6 +4037,11 @@ async fn edit_datatable_config( } if database.resource_type == DataTableCatalogResourceType::ExternalInstance { windmill_common::external_instance_pg::ensure_external_instance_available()?; + windmill_common::external_instance_pg::ensure_external_instance_database_registered( + &mut tx, + &database.resource_path, + ) + .await?; } if !is_superadmin { return Err(Error::BadRequest( diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index d0cb42e92a..975c178982 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -1422,7 +1422,7 @@ pub async fn drop_forked_datatable_databases( == windmill_common::workspaces::DataTableCatalogResourceType::ExternalInstance { windmill_common::external_instance_pg::drop_external_instance_database_unchecked( - &db, db_to_drop, + &db, db_to_drop, false, ) .await } else { diff --git a/backend/windmill-common/src/external_instance_pg.rs b/backend/windmill-common/src/external_instance_pg.rs index c0ad333191..89856608a8 100644 --- a/backend/windmill-common/src/external_instance_pg.rs +++ b/backend/windmill-common/src/external_instance_pg.rs @@ -130,8 +130,8 @@ pub async fn external_instance_databases(db: &DB) -> Result( + db: impl sqlx::PgExecutor<'c>, ) -> Result>> { let rows = sqlx::query_as::<_, (String, String)>( "SELECT ws.workspace_id, entry->'database'->>'resource_path' @@ -211,12 +211,55 @@ pub async fn create_external_instance_database_unchecked( .await } -/// Drop `dbname` from the external cluster. Only ever a database Windmill registered creating. +/// Drop `dbname` from the external cluster: only a database Windmill registered creating, and still +/// carries the mark it set there. With `refuse_if_used`, refuse while a data table names it. /// /// Authorization: checks nothing. Callers MUST be superadmin, or be deleting the fork that owns /// this `wm_fork_` database. -pub async fn drop_external_instance_database_unchecked(db: &DB, dbname: &str) -> Result<()> { - crate::external_instance_pg_oss::drop_external_instance_database_unchecked(db, dbname).await +pub async fn drop_external_instance_database_unchecked( + db: &DB, + dbname: &str, + refuse_if_used: bool, +) -> Result<()> { + crate::external_instance_pg_oss::drop_external_instance_database_unchecked( + db, + dbname, + refuse_if_used, + ) + .await +} + +/// Serializes everything that changes which databases exist on the external cluster, or which data +/// tables name them: setup, creates, drops, and data table saves. Held until `tx` ends. +pub async fn lock_external_instance_pg_state( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, +) -> Result<()> { + sqlx::query("SELECT pg_advisory_xact_lock(hashtext($1))") + .bind(EXTERNAL_INSTANCE_PG_STATE_SETTING) + .execute(&mut **tx) + .await?; + Ok(()) +} + +/// Refuse a data table naming `dbname` unless Windmill created it on the external cluster. Takes +/// the lock drops take, so none can remove the database before `tx`, which saves the data table, +/// commits. +pub async fn ensure_external_instance_database_registered( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + dbname: &str, +) -> Result<()> { + lock_external_instance_pg_state(tx).await?; + if read_external_instance_pg_state(&mut **tx) + .await? + .databases + .contains_key(dbname) + { + return Ok(()); + } + Err(Error::BadRequest(format!( + "Windmill did not create a database named '{dbname}' on the external instance cluster. \ + Create it from the instance settings first." + ))) } /// Check a write to [`EXTERNAL_INSTANCE_PG_SETTING`] before it happens: `None`, null or an empty diff --git a/backend/windmill-common/src/external_instance_pg_oss.rs b/backend/windmill-common/src/external_instance_pg_oss.rs index 104aece751..7f6168878e 100644 --- a/backend/windmill-common/src/external_instance_pg_oss.rs +++ b/backend/windmill-common/src/external_instance_pg_oss.rs @@ -73,6 +73,7 @@ mod ce { pub(crate) async fn drop_external_instance_database_unchecked( _db: &DB, _dbname: &str, + _refuse_if_used: bool, ) -> Result<()> { Err(unavailable()) }