fix(datatables): write the external cluster setting under the lifecycle lock, and check fork targets are registered

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/ee-repo-ref.txt

@@ -1 +1 @@
-54ba0fd48049008c6fb7de8e454401a42802d939
+f920b69b68d0ce2e907774c75573ffd4d9f916ca

backend/windmill-api-settings/src/lib.rs

@@ -893,6 +893,14 @@ pub async fn set_global_setting_internal(
         )));
     }
 
+    if key == EXTERNAL_INSTANCE_PG_SETTING {
+        return windmill_common::external_instance_pg::write_external_instance_pg_setting(
+            db,
+            Some(&value),
+        )
+        .await;
+    }
+
     run_setting_pre_write_hook(db, &key, &value).await?;
 
     match value {
@@ -954,13 +962,6 @@ async fn run_setting_pre_write_hook(
     value: &serde_json::Value,
 ) -> error::Result<()> {
     match key {
-        EXTERNAL_INSTANCE_PG_SETTING => {
-            windmill_common::external_instance_pg::check_external_instance_pg_write(
-                db,
-                Some(value),
-            )
-            .await?;
-        }
         // The instance AI config is written as an untyped blob through this generic
         // endpoint, so it never passes the typed check the workspace handler applies.
         // Rates that reach a cost total unbounded would make it negative or infinite.
@@ -1281,7 +1282,7 @@ async fn set_instance_config(
     let desired_map = desired.global_settings.to_settings_map();
     if !desired_map.is_empty() {
         let current_map = current.global_settings.to_settings_map();
-        let settings_diff =
+        let mut settings_diff =
             instance_config::diff_global_settings(&current_map, &desired_map, ApplyMode::Merge);
         let ai_config_changed = settings_diff
             .upserts
@@ -1310,16 +1311,15 @@ async fn set_instance_config(
         }
 
         for (key, value) in &settings_diff.upserts {
-            run_setting_pre_write_hook(&db, key, value).await?;
-        }
-        if settings_diff
-            .deletes
-            .iter()
-            .any(|k| k == EXTERNAL_INSTANCE_PG_SETTING)
-        {
-            windmill_common::external_instance_pg::check_external_instance_pg_write(&db, None)
-                .await?;
+            if key != EXTERNAL_INSTANCE_PG_SETTING {
+                run_setting_pre_write_hook(&db, key, value).await?;
+            }
         }
+        windmill_common::external_instance_pg::write_external_instance_pg_from_diff(
+            &db,
+            &mut settings_diff,
+        )
+        .await?;
 
         instance_config::apply_settings_diff(&db, &settings_diff)
             .await

backend/windmill-api-workspaces/src/workspaces.rs

@@ -8328,6 +8328,13 @@ async fn apply_forked_datatable(
         })?,
     };
 
+    if database.resource_type == DataTableCatalogResourceType::ExternalInstance {
+        windmill_common::external_instance_pg::ensure_external_instance_database_registered(
+            tx,
+            &fdt.new_dbname,
+        )
+        .await?;
+    }
     if database.resource_type.is_windmill_managed() {
         // The whole `database` object, not just its `resource_path`: a pointer entry has none to
         // patch. `reference` goes with it — exactly one of the two may be set. The copy was created

backend/windmill-common/src/external_instance_pg.rs

@@ -265,23 +265,73 @@ pub async fn ensure_external_instance_database_registered(
     )))
 }
 
-/// Check a write to [`EXTERNAL_INSTANCE_PG_SETTING`] before it happens: `None`, null or an empty
-/// string unsets it. Every writer of global settings calls this, the per-key and bulk endpoints
-/// as well as the declarative sync.
-pub async fn check_external_instance_pg_write(
+/// Write [`EXTERNAL_INSTANCE_PG_SETTING`]: `None`, null or an empty string unsets it. Every writer
+/// of global settings goes through this for that key — the per-key and bulk endpoints as well as
+/// the declarative sync — instead of writing the row itself.
+///
+/// The checks and the write share one transaction holding [`lock_external_instance_pg_state`]. A
+/// check taken outside it could pass while a database create still reads the old cluster, which
+/// would then register a database there after the setting names another one.
+///
+/// Authorization: checks nothing. Callers MUST be superadmin.
+pub async fn write_external_instance_pg_setting(
     db: &DB,
     value: Option<&serde_json::Value>,
 ) -> Result<()> {
+    let value = match value {
+        None | Some(serde_json::Value::Null) => None,
+        Some(serde_json::Value::String(s)) if s.trim().is_empty() => None,
+        Some(value) => Some(value),
+    };
+    let mut tx = db.begin().await?;
+    lock_external_instance_pg_state(&mut tx).await?;
     match value {
-        None | Some(serde_json::Value::Null) => ensure_external_instance_pg_removable(db).await,
-        Some(serde_json::Value::String(s)) if s.trim().is_empty() => {
-            ensure_external_instance_pg_removable(db).await
+        None => {
+            ensure_external_instance_pg_removable(db).await?;
+            sqlx::query("DELETE FROM global_settings WHERE name = $1")
+                .bind(EXTERNAL_INSTANCE_PG_SETTING)
+                .execute(&mut *tx)
+                .await?;
         }
         Some(value) => {
             crate::external_instance_pg_oss::validate_external_instance_pg_setting(value)?;
-            ensure_external_instance_pg_not_repointed(db, value).await
+            ensure_external_instance_pg_not_repointed(db, value).await?;
+            sqlx::query(
+                "INSERT INTO global_settings (name, value) VALUES ($1, $2)
+                 ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value, updated_at = now()",
+            )
+            .bind(EXTERNAL_INSTANCE_PG_SETTING)
+            .bind(value)
+            .execute(&mut *tx)
+            .await?;
         }
     }
+    tx.commit().await?;
+    tracing::info!(
+        "{} global setting {EXTERNAL_INSTANCE_PG_SETTING}",
+        if value.is_some() { "Set" } else { "Unset" }
+    );
+    Ok(())
+}
+
+/// [`write_external_instance_pg_setting`] for a settings diff: writes the key if the diff touches
+/// it, and takes it out of the diff so the generic apply does not write it again.
+pub async fn write_external_instance_pg_from_diff(
+    db: &DB,
+    diff: &mut crate::instance_config::SettingsDiff,
+) -> Result<()> {
+    if let Some(value) = diff.upserts.remove(EXTERNAL_INSTANCE_PG_SETTING) {
+        write_external_instance_pg_setting(db, Some(&value)).await?;
+    }
+    if let Some(i) = diff
+        .deletes
+        .iter()
+        .position(|k| k == EXTERNAL_INSTANCE_PG_SETTING)
+    {
+        diff.deletes.remove(i);
+        write_external_instance_pg_setting(db, None).await?;
+    }
+    Ok(())
 }
 
 /// Refuse pointing the setting at another host or port while databases live on the current one.

backend/windmill-common/src/instance_config.rs

@@ -1395,14 +1395,8 @@ pub async fn sync_global_settings_declarative(
     crate::global_settings::parse_allowed_origins_setting(desired.get(origins_key))
         .map_err(|e| anyhow::anyhow!("{origins_key}: {e}"))?;
 
-    let diff = diff_global_settings(current, desired, ApplyMode::Replace);
-    let external_pg_key = crate::global_settings::EXTERNAL_INSTANCE_PG_SETTING;
-    if diff.deletes.iter().any(|k| k == external_pg_key) {
-        crate::external_instance_pg::check_external_instance_pg_write(db, None).await?;
-    }
-    if let Some(value) = diff.upserts.get(external_pg_key) {
-        crate::external_instance_pg::check_external_instance_pg_write(db, Some(value)).await?;
-    }
+    let mut diff = diff_global_settings(current, desired, ApplyMode::Replace);
+    crate::external_instance_pg::write_external_instance_pg_from_diff(db, &mut diff).await?;
     apply_settings_diff(db, &diff).await?;
 
     Ok(())