order script hard-delete and dbt graph publication locks consistently (#10446)

* fix: order script hard-delete and dbt graph publication locks consistently

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

* fix: clear dbt retry state after the script delete, not before

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

---------

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

backend/.sqlx/query-11d979216951a85835c15e02c756249fa16fd913dc1a2c7babc329cc2d636342.json

@@ -1,15 +0,0 @@
-{
-  "db_name": "PostgreSQL",
-  "query": "DELETE FROM dbt_graph_snapshot WHERE workspace_id = $1 AND script_path = $2",
-  "describe": {
-    "columns": [],
-    "parameters": {
-      "Left": [
-        "Text",
-        "Text"
-      ]
-    },
-    "nullable": []
-  },
-  "hash": "11d979216951a85835c15e02c756249fa16fd913dc1a2c7babc329cc2d636342"
-}

backend/.sqlx/query-3ec280ad74cf63dc3cebb312c620e7a5c2a3b75051618fc3c9635b4cd8d48e25.json

@@ -1,6 +1,6 @@
 {
   "db_name": "PostgreSQL",
-  "query": "DELETE FROM dbt_node WHERE workspace_id = $1 AND script_path = $2",
+  "query": "DELETE FROM script WHERE workspace_id = $1 AND path = $2",
   "describe": {
     "columns": [],
     "parameters": {
@@ -11,5 +11,5 @@
     },
     "nullable": []
   },
-  "hash": "71462a33d126a7b70c5ef570c63502cc7e848b82cc8a3e6032d8cd030ee8a1db"
+  "hash": "3ec280ad74cf63dc3cebb312c620e7a5c2a3b75051618fc3c9635b4cd8d48e25"
 }

backend/.sqlx/query-a2e7c8c0d1cc256e59976845c5189ccc84720581d7e3661c5eb2719c8ccff08a.json

@@ -1,15 +0,0 @@
-{
-  "db_name": "PostgreSQL",
-  "query": "DELETE FROM dbt_edge WHERE workspace_id = $1 AND script_path = $2",
-  "describe": {
-    "columns": [],
-    "parameters": {
-      "Left": [
-        "Text",
-        "Text"
-      ]
-    },
-    "nullable": []
-  },
-  "hash": "a2e7c8c0d1cc256e59976845c5189ccc84720581d7e3661c5eb2719c8ccff08a"
-}

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

@@ -3447,9 +3447,13 @@ async fn delete_script_by_hash(
 
     check_scopes(&authed, || format!("scripts:write:{}", &script.path))?;
 
-    clear_static_asset_usage_by_script_hash(&mut *tx, &w_id, hash).await?;
+    // Graph before assets, the order both publishers take them in — the reverse
+    // deadlocks against a job that clears its own graph and then rewrites the
+    // path's `asset` rows. A clear is needed at all because this route only
+    // soft-deletes the `script` row, so nothing cascades.
     windmill_common::dbt_manifest::clear_dbt_manifest_version(&mut tx, &w_id, &script.path, hash.0)
         .await?;
+    clear_static_asset_usage_by_script_hash(&mut *tx, &w_id, hash).await?;
     windmill_common::dbt_manifest::clear_dbt_run_state_if_path_retired(
         &mut tx,
         &w_id,
@@ -3540,12 +3544,6 @@ async fn delete_script_by_path(
     .fetch_all(&mut *tx)
     .await?;
 
-    // Before the DELETE: both are keyed on the path with no script foreign key,
-    // so anything left behind here would attach itself to whatever is created
-    // at this path next.
-    windmill_common::dbt_manifest::clear_dbt_manifest(&mut tx, &w_id, path).await?;
-    windmill_common::dbt_manifest::clear_dbt_run_state(&mut tx, &w_id, path).await?;
-
     let script = sqlx::query_scalar!(
         "DELETE FROM script WHERE path = $1 AND workspace_id = $2 RETURNING path",
         path,
@@ -3555,6 +3553,12 @@ async fn delete_script_by_path(
     .await
     .map_err(|e| Error::internal_err(format!("deleting script by path {w_id}: {e:#}")))?;
 
+    // After the DELETE, never before: every dbt writer locks the `script` row
+    // first, so taking a sidecar ahead of it deadlocks one of the pair. The graph
+    // needs no clear at all, cascading off `script`; the retry state does, being
+    // keyed by path alone and so inherited by whatever is created here next.
+    windmill_common::dbt_manifest::clear_dbt_run_state(&mut tx, &w_id, path).await?;
+
     if !trash_scripts.is_empty() {
         let mut trash_data = serde_json::json!({"scripts": trash_scripts});
         if !trash_drafts.is_empty() {
@@ -3709,12 +3713,6 @@ async fn delete_scripts_bulk(
         }
     }
 
-    // Same reason as the single-path delete: neither has a foreign key.
-    for p in &request.paths {
-        windmill_common::dbt_manifest::clear_dbt_manifest(&mut tx, &w_id, p).await?;
-        windmill_common::dbt_manifest::clear_dbt_run_state(&mut tx, &w_id, p).await?;
-    }
-
     let mut deleted_paths = sqlx::query_scalar!(
         "DELETE FROM script WHERE workspace_id = $1 AND path = ANY($2) RETURNING path",
         w_id,
@@ -3724,6 +3722,12 @@ async fn delete_scripts_bulk(
     .await
     .map_err(|e| Error::internal_err(format!("deleting scripts in bulk {w_id}: {e:#}")))?;
 
+    // Same reason as the single-path delete, over every requested path rather
+    // than the deleted ones: a path that had no script left can still hold state.
+    for p in &request.paths {
+        windmill_common::dbt_manifest::clear_dbt_run_state(&mut tx, &w_id, p).await?;
+    }
+
     // remove duplicates from deleted_paths
     deleted_paths.sort();
     deleted_paths.dedup();

backend/windmill-common/src/dbt_manifest.rs

@@ -31,7 +31,7 @@
 //! # Mutator contract
 //!
 //! Every `pub` mutator in this module — the manifest ones
-//! (`replace_dbt_manifest`, `clear_dbt_manifest`, `clear_dbt_manifest_version`),
+//! (`replace_dbt_manifest`, `clear_dbt_manifest_version`),
 //! the snapshot sweep, and the retry-state ones (`move_dbt_run_state`,
 //! `clear_dbt_run_state`, `clear_dbt_run_state_if_path_retired`) — takes the
 //! workspace and the script to act on as plain arguments and enforces nothing:
@@ -906,10 +906,16 @@ const NODE_INSERT_CHUNK: usize = 2000;
 /// Six columns, so the same ceiling allows far more.
 const EDGE_INSERT_CHUNK: usize = 8000;
 
-/// Clear one VERSION's graph, the unit the archive and delete routes act on:
-/// both target a single `hash`, and the other versions of the path stay live
-/// and keep needing their own models, SQL and lineage. The path-wide sibling
-/// below is for the case where a path stops being a dbt script at all.
+/// Clear one VERSION's graph: the delete-by-hash route, which only soft-deletes
+/// its `script` row and so fires no cascade, and the ingest that finds no
+/// warehouse identity left to key assets on. Both target a single `hash`, and the
+/// other versions of the path stay live and keep needing their own models, SQL
+/// and lineage.
+///
+/// There is no path-wide sibling. The routes that remove the `script` rows
+/// outright let the tables' `ON DELETE CASCADE` take the graph, which is also
+/// what keeps them from locking it ahead of the script row and deadlocking with
+/// a concurrent publication.
 ///
 /// See the mutator contract above: this authorizes nothing.
 pub async fn clear_dbt_manifest_version(
@@ -950,44 +956,6 @@ pub async fn clear_dbt_manifest_version(
     Ok(())
 }
 
-/// Drop every version of one path's graph, for the routes that retire the whole
-/// path — archive-by-path, delete-by-path and the bulk delete.
-///
-/// NOT for a rename, nor for a path whose newest version stops being dbt:
-/// every graph query joins on `(path, hash)` through a `language = 'dbt'` CTE,
-/// so an old version's rows cannot attach to whatever lives at that path next,
-/// and its own finished runs still render from them.
-///
-/// See the mutator contract above: this authorizes nothing.
-pub async fn clear_dbt_manifest(
-    tx: &mut Transaction<'_, Postgres>,
-    workspace_id: &str,
-    script_path: &str,
-) -> Result<()> {
-    sqlx::query!(
-        "DELETE FROM dbt_node WHERE workspace_id = $1 AND script_path = $2",
-        workspace_id,
-        script_path
-    )
-    .execute(&mut **tx)
-    .await?;
-    sqlx::query!(
-        "DELETE FROM dbt_edge WHERE workspace_id = $1 AND script_path = $2",
-        workspace_id,
-        script_path
-    )
-    .execute(&mut **tx)
-    .await?;
-    sqlx::query!(
-        "DELETE FROM dbt_graph_snapshot WHERE workspace_id = $1 AND script_path = $2",
-        workspace_id,
-        script_path
-    )
-    .execute(&mut **tx)
-    .await?;
-    Ok(())
-}
-
 /// Move a dbt script's saved retry state to its new path.
 ///
 /// Keyed by path like the sidecar, but unlike the sidecar it is not

backend/windmill-common/tests/dbt_graph_storage.rs

@@ -7,8 +7,8 @@
 
 use sqlx::{Pool, Postgres};
 use windmill_common::dbt_manifest::{
-    clear_dbt_manifest, clear_dbt_manifest_version, prune_dbt_run_graphs, replace_dbt_manifest,
-    IngestedManifest, IngestedNode, DEPLOYED_GRAPH, DEPLOYED_GRAPH_VERSIONS_KEPT,
+    clear_dbt_manifest_version, prune_dbt_run_graphs, replace_dbt_manifest, IngestedManifest,
+    IngestedNode, DEPLOYED_GRAPH, DEPLOYED_GRAPH_VERSIONS_KEPT,
 };
 
 const WS: &str = "test-workspace";
@@ -234,26 +234,44 @@ async fn clearing_one_version_leaves_the_others(db: Pool<Postgres>) {
     assert_eq!(edges_for(&db, 2).await, 1, "the other version keeps its own");
 }
 
-/// The path-wide clear is for the routes that retire the whole path, and it has
-/// to take the markers too — a marker standing for rows that are gone is read
-/// as a snapshot, and its digest still answers the suppression check.
+/// The routes that hard-delete a path clear no graph rows: they delete the
+/// `script` rows and let `ON DELETE CASCADE` take the sidecars, since taking
+/// those first would lock them ahead of the script row and deadlock a concurrent
+/// publication. So the cascade has to reach every sidecar, snapshots and markers
+/// included.
 #[sqlx::test(migrations = "../migrations", fixtures("base"))]
-async fn clearing_the_path_takes_markers_with_it(db: Pool<Postgres>) {
+async fn deleting_the_script_cascades_to_every_sidecar(db: Pool<Postgres>) {
     deploy_script(&db, 1).await;
     deploy_script(&db, 2).await;
+    let job = uuid::Uuid::from_u128(7);
     let mut tx = db.begin().await.unwrap();
-    replace_dbt_manifest(&mut tx, WS, PATH, 1, None, &manifest(&["a"]), "root")
+    replace_dbt_manifest(&mut tx, WS, PATH, 1, None, &manifest(&["a", "b"]), "root")
         .await
         .unwrap();
-    replace_dbt_manifest(&mut tx, WS, PATH, 2, None, &manifest(&["b"]), "root")
+    replace_dbt_manifest(&mut tx, WS, PATH, 1, Some(job), &manifest(&["a"]), "root")
+        .await
+        .unwrap();
+    replace_dbt_manifest(&mut tx, WS, PATH, 2, None, &manifest(&["b", "c"]), "root")
         .await
         .unwrap();
-    clear_dbt_manifest(&mut tx, WS, PATH).await.unwrap();
     tx.commit().await.unwrap();
+    assert_eq!(markers_for_path(&db).await, 3, "two versions and one run");
+
+    sqlx::query!(
+        "DELETE FROM script WHERE workspace_id = $1 AND path = $2",
+        WS,
+        PATH
+    )
+    .execute(&db)
+    .await
+    .unwrap();
 
-    assert_eq!(markers(&db, 1).await, 0);
-    assert_eq!(markers(&db, 2).await, 0);
     assert_eq!(nodes_for(&db, 1, DEPLOYED_GRAPH).await, 0);
+    assert_eq!(nodes_for(&db, 1, job).await, 0, "a run's snapshot goes too");
+    assert_eq!(nodes_for(&db, 2, DEPLOYED_GRAPH).await, 0);
+    assert_eq!(edges_for(&db, 1).await, 0);
+    assert_eq!(edges_for(&db, 2).await, 0);
+    assert_eq!(markers_for_path(&db).await, 0);
 }
 
 /// The sweep ages out run snapshots and never a version's own graph, which is

docs/dbt-runtime.md

@@ -494,6 +494,15 @@ rows carry a composite foreign key to `script (workspace_id, hash)` with
 `ON DELETE CASCADE`, so a version's graph dies with the version and nothing has
 to sweep it.
 
+The routes that hard-delete a script rely on exactly that and clear no graph
+rows of their own. Clearing them first would lock the sidecars ahead of the
+`script` rows, the reverse of the order a publication takes — `script` row
+`FOR UPDATE`, then the sidecars — and Postgres would abort one of the two for
+deadlock. The consequence is that every sidecar has to be reachable by cascade:
+`dbt_node`, `dbt_edge` and `dbt_graph_snapshot` are, while `dbt_run_state` (keyed
+by path, no script key) is cleared explicitly and `dbt_run_progress` (keyed by
+job, no key to either) is reclaimed only by its age sweep.
+
 Two consequences worth knowing:
 
 * **Concurrent deploys no longer race for the graph.** Two versions write