From 8d9c81a4daaf6e61077a2d552d5e8f08d7897290 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 1 Aug 2026 16:05:43 +0200 Subject: [PATCH] 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) * fix: clear dbt retry state after the script delete, not before Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- ...6249fa16fd913dc1a2c7babc329cc2d636342.json | 15 ------ ...e7a5c2a3b75051618fc3c9635b4cd8d48e25.json} | 4 +- ...89ccc84720581d7e3661c5eb2719c8ccff08a.json | 15 ------ backend/windmill-api-scripts/src/scripts.rs | 30 ++++++----- backend/windmill-common/src/dbt_manifest.rs | 54 ++++--------------- .../tests/dbt_graph_storage.rs | 40 ++++++++++---- docs/dbt-runtime.md | 9 ++++ 7 files changed, 68 insertions(+), 99 deletions(-) delete mode 100644 backend/.sqlx/query-11d979216951a85835c15e02c756249fa16fd913dc1a2c7babc329cc2d636342.json rename backend/.sqlx/{query-71462a33d126a7b70c5ef570c63502cc7e848b82cc8a3e6032d8cd030ee8a1db.json => query-3ec280ad74cf63dc3cebb312c620e7a5c2a3b75051618fc3c9635b4cd8d48e25.json} (52%) delete mode 100644 backend/.sqlx/query-a2e7c8c0d1cc256e59976845c5189ccc84720581d7e3661c5eb2719c8ccff08a.json diff --git a/backend/.sqlx/query-11d979216951a85835c15e02c756249fa16fd913dc1a2c7babc329cc2d636342.json b/backend/.sqlx/query-11d979216951a85835c15e02c756249fa16fd913dc1a2c7babc329cc2d636342.json deleted file mode 100644 index 4c3c53120c..0000000000 --- a/backend/.sqlx/query-11d979216951a85835c15e02c756249fa16fd913dc1a2c7babc329cc2d636342.json +++ /dev/null @@ -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" -} diff --git a/backend/.sqlx/query-71462a33d126a7b70c5ef570c63502cc7e848b82cc8a3e6032d8cd030ee8a1db.json b/backend/.sqlx/query-3ec280ad74cf63dc3cebb312c620e7a5c2a3b75051618fc3c9635b4cd8d48e25.json similarity index 52% rename from backend/.sqlx/query-71462a33d126a7b70c5ef570c63502cc7e848b82cc8a3e6032d8cd030ee8a1db.json rename to backend/.sqlx/query-3ec280ad74cf63dc3cebb312c620e7a5c2a3b75051618fc3c9635b4cd8d48e25.json index 7b1509ce02..7cbc8115c9 100644 --- a/backend/.sqlx/query-71462a33d126a7b70c5ef570c63502cc7e848b82cc8a3e6032d8cd030ee8a1db.json +++ b/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" } diff --git a/backend/.sqlx/query-a2e7c8c0d1cc256e59976845c5189ccc84720581d7e3661c5eb2719c8ccff08a.json b/backend/.sqlx/query-a2e7c8c0d1cc256e59976845c5189ccc84720581d7e3661c5eb2719c8ccff08a.json deleted file mode 100644 index 679e03a3c7..0000000000 --- a/backend/.sqlx/query-a2e7c8c0d1cc256e59976845c5189ccc84720581d7e3661c5eb2719c8ccff08a.json +++ /dev/null @@ -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" -} diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 2d4f1c72b8..78180b8283 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/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(); diff --git a/backend/windmill-common/src/dbt_manifest.rs b/backend/windmill-common/src/dbt_manifest.rs index 7dfc796cfb..a294c9d8d9 100644 --- a/backend/windmill-common/src/dbt_manifest.rs +++ b/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 diff --git a/backend/windmill-common/tests/dbt_graph_storage.rs b/backend/windmill-common/tests/dbt_graph_storage.rs index 73a3320456..0ca33eea11 100644 --- a/backend/windmill-common/tests/dbt_graph_storage.rs +++ b/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) { 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) { +async fn deleting_the_script_cascades_to_every_sidecar(db: Pool) { 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 diff --git a/docs/dbt-runtime.md b/docs/dbt-runtime.md index ed389acd71..c408062d7c 100644 --- a/docs/dbt-runtime.md +++ b/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