From 43d2c299884851213fb0f73f22b72d42ae9899a2 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 4 Sep 2026 10:09:48 +0200 Subject: [PATCH] fix(datatables): a rename reaches only the workspace it is renaming in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update_username_in_workpsace` locks one workspace's settings row but wrote `usr` and `usr_to_group` across every workspace holding that name, so it waited on principals another workspace's settings row guards — the deadlock the last commit closed, one workspace over. Both writes are scoped now, and the rename walks memberships in workspace order. The deferred role drop holds the settings row while it runs, so its session carries a statement timeout: the statements run on a database the workspace does not control, and a lock held there would stall every save behind it. `PgRoleInventory` is read by the enterprise planner alone, which a community build compiles without. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S5arH3G2Sa1Qqm32veJQ1n --- ...73a916b00fd73600943b98f69639fac2071be.json | 16 ++++++++++++++ ...d708f200a52aaf55c7cf40f7b210f97688c15.json | 16 ++++++++++++++ .../src/datatable_permissions.rs | 18 +++++++++++++++ backend/windmill-api/src/users.rs | 22 ++++++++++++------- 4 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 backend/.sqlx/query-58002bfd04efea3d5f955d0bc5073a916b00fd73600943b98f69639fac2071be.json create mode 100644 backend/.sqlx/query-af0bcc6aaf455f75cdf44a503eed708f200a52aaf55c7cf40f7b210f97688c15.json diff --git a/backend/.sqlx/query-58002bfd04efea3d5f955d0bc5073a916b00fd73600943b98f69639fac2071be.json b/backend/.sqlx/query-58002bfd04efea3d5f955d0bc5073a916b00fd73600943b98f69639fac2071be.json new file mode 100644 index 0000000000..48ec1178d6 --- /dev/null +++ b/backend/.sqlx/query-58002bfd04efea3d5f955d0bc5073a916b00fd73600943b98f69639fac2071be.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE usr SET username = $1 WHERE email = $2 AND workspace_id = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "58002bfd04efea3d5f955d0bc5073a916b00fd73600943b98f69639fac2071be" +} diff --git a/backend/.sqlx/query-af0bcc6aaf455f75cdf44a503eed708f200a52aaf55c7cf40f7b210f97688c15.json b/backend/.sqlx/query-af0bcc6aaf455f75cdf44a503eed708f200a52aaf55c7cf40f7b210f97688c15.json new file mode 100644 index 0000000000..4c04e5eb0e --- /dev/null +++ b/backend/.sqlx/query-af0bcc6aaf455f75cdf44a503eed708f200a52aaf55c7cf40f7b210f97688c15.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE usr_to_group SET usr = $1 WHERE usr = $2 AND workspace_id = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "af0bcc6aaf455f75cdf44a503eed708f200a52aaf55c7cf40f7b210f97688c15" +} diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 2c5ccb81b0..1986afc13f 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -227,6 +227,11 @@ pub(crate) struct AdminConnection { /// The `wm_` logins the cluster holds, split by whether this data table may take /// one over. #[derive(Default)] +/// Only the planner reads these, and that is the enterprise module. +#[cfg_attr( + not(all(feature = "private", feature = "enterprise")), + allow(dead_code) +)] pub(crate) struct PgRoleInventory { /// Every one of them. `pg_roles` is a cluster catalog, so this spans every /// database and every workspace on the instance — a name in here cannot be @@ -530,6 +535,19 @@ pub(crate) async fn run_planned_drop( .and_then(|d| d.get(datatable_name)) .is_some_and(|v| !v.is_null()); if !recreated { + // The settings row is held for as long as these run, and they run on + // a database this workspace does not control — a lock held there, or + // a role with a great deal to reassign, would otherwise stall every + // save of every data table in the workspace behind it. + client + .batch_execute("SET statement_timeout = '60s'") + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to bound the role drop: {}", + pg_error_message(&e) + )) + })?; run_statements(&mut client, &plan).await?; } tx.commit().await?; diff --git a/backend/windmill-api/src/users.rs b/backend/windmill-api/src/users.rs index 4b935cbce0..ef257146b1 100644 --- a/backend/windmill-api/src/users.rs +++ b/backend/windmill-api/src/users.rs @@ -216,8 +216,10 @@ async fn rename_user( .execute(&mut *tx) .await?; + // Ordered, so a rename and a deletion that touch the same workspaces take + // their settings rows in the same sequence rather than head-on. let workspace_usernames = sqlx::query!( - "SELECT workspace_id, username FROM usr WHERE email = $1", + "SELECT workspace_id, username FROM usr WHERE email = $1 ORDER BY workspace_id", &user_email ) .fetch_all(&mut *tx) @@ -891,21 +893,25 @@ async fn update_username_in_workpsace<'c>( // ---- instance and workspace users ---- - // Last, after the settings row above: every path that frees or renames a - // username takes `workspace_settings` before `usr` and `usr_to_group`, and - // one that took them the other way round would deadlock against it. + // Last, after the settings row above, and scoped to this workspace like + // everything else here: every path that frees or renames a username takes + // `workspace_settings` before `usr` and `usr_to_group`, and reaching into + // another workspace's rows would hold this one's settings row while waiting + // on a principal that workspace's own settings row guards. sqlx::query!( - "UPDATE usr SET username = $1 WHERE email = $2", + "UPDATE usr SET username = $1 WHERE email = $2 AND workspace_id = $3", new_username, - email + email, + w_id ) .execute(&mut **tx) .await?; sqlx::query!( - "UPDATE usr_to_group SET usr = $1 WHERE usr = $2", + "UPDATE usr_to_group SET usr = $1 WHERE usr = $2 AND workspace_id = $3", new_username, - old_username + old_username, + w_id ) .execute(&mut **tx) .await?;