From 8bb2b0d777cc53903944753dfa41ca236add7c91 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 4 Sep 2026 11:28:44 +0200 Subject: [PATCH] fix(datatables): one lock, taken first, instead of an order per handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every path that touches a data table's permissions or a principal its roles can name takes the workspace settings row before it locks anything else, and a transaction spanning workspaces takes them in `workspace_id` order. One lock acquired first cannot deadlock, so the rule is stated once on the lock rather than as an order each handler has to reproduce — the rename can write its thirty tables in whatever order reads best, and the statements moved around to fake an order move back. What waits for the config to commit is now decided from the plan and checked per role: a request that disables permissions still lists every role, so the set built from it claimed them all and no drop waited at all. A role the committed config no longer names is dropped whatever else changed meanwhile, where before any unrelated edit — a tenant, the default role — made the whole cleanup stand down and left the login behind with nothing that would ever remove it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S5arH3G2Sa1Qqm32veJQ1n --- backend/ee-repo-ref.txt | 2 +- backend/windmill-api-groups/src/folders.rs | 20 +-- backend/windmill-api-users/src/users.rs | 49 +++--- .../src/datatable_permissions.rs | 141 ++++++++++-------- backend/windmill-api/src/users.rs | 82 +++++----- backend/windmill-common/src/workspaces.rs | 24 +-- 6 files changed, 169 insertions(+), 149 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 862676f204..e6df7f7752 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -3fe285af8293aafd4d4ba31002bfa84a30544586 +4c6cbb234019db102c44a91ada36c4a197721aa4 diff --git a/backend/windmill-api-groups/src/folders.rs b/backend/windmill-api-groups/src/folders.rs index 434335e034..4bfb6d10f4 100644 --- a/backend/windmill-api-groups/src/folders.rs +++ b/backend/windmill-api-groups/src/folders.rs @@ -800,6 +800,16 @@ async fn delete_folder( not_found_if_none(get_folderopt(&mut tx, &w_id, &name).await?, "Folder", &name)?; + // A data table role names its tenants by principal, so the name is free + // after this — and recreating a folder with it would inherit every role the + // old one could run as. + windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( + &w_id, + &format!("f/{name}"), + &mut tx, + ) + .await?; + let del = sqlx::query_scalar!( "DELETE FROM folder WHERE name = $1 AND workspace_id = $2 RETURNING 1", name, @@ -816,16 +826,6 @@ async fn delete_folder( ))); } - // A data table role names its tenants by principal, so the name is free - // after this — and recreating a folder with it would inherit every role the - // old one could run as. - windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( - &w_id, - &format!("f/{name}"), - &mut tx, - ) - .await?; - audit_log( &mut *tx, &authed, diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 55aeb56219..1b82897622 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -1656,18 +1656,10 @@ async fn delete_user( forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; - sqlx::query!("DELETE FROM token WHERE email = $1", &email_to_delete) - .execute(&mut *tx) - .await?; - sqlx::query!("DELETE FROM password WHERE email = $1", &email_to_delete) - .execute(&mut *tx) - .await?; - windmill_common::user_drafts::delete_drafts_of_email(&mut *tx, &email_to_delete).await?; - - // Read before deleting, so each membership's tenant can go first. A username - // is scoped to one workspace, and so are the tenants naming it. - // Ordered, so two deletions that share workspaces lock their settings rows - // in the same sequence rather than head-on. + // A username is scoped to one workspace, and so are the tenants naming it, so + // the memberships are read before anything is deleted. Ordered, and before + // every other write of this transaction — see + // `lock_workspace_settings_unchecked`. let memberships = sqlx::query!( "SELECT workspace_id, username FROM usr WHERE email = $1 ORDER BY workspace_id", &email_to_delete @@ -1676,7 +1668,7 @@ async fn delete_user( .await?; for row in &memberships { - // The username is free in that workspace once the row below is gone, so + // The username is free in that workspace once the rows below are gone, so // a role still naming it would hand itself to whoever takes it next. windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( &row.workspace_id, @@ -1686,6 +1678,14 @@ async fn delete_user( .await?; } + sqlx::query!("DELETE FROM token WHERE email = $1", &email_to_delete) + .execute(&mut *tx) + .await?; + sqlx::query!("DELETE FROM password WHERE email = $1", &email_to_delete) + .execute(&mut *tx) + .await?; + windmill_common::user_drafts::delete_drafts_of_email(&mut *tx, &email_to_delete).await?; + sqlx::query!("DELETE FROM usr WHERE email = $1", &email_to_delete) .execute(&mut *tx) .await?; @@ -2416,6 +2416,18 @@ pub async fn delete_workspace_user_internal( tx: &mut Transaction<'_, Postgres>, authed: Option<&ApiAuthed>, // None for system operations ) -> Result<()> { + // ---- Clean up data table role tenants ---- + + // The username is free once this user's rows are gone, so a tenant left + // behind would hand every role it names to whoever is invited into it next. + // First in the transaction — see `lock_workspace_settings_unchecked`. + windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( + w_id, + &format!("u/{username_to_delete}"), + tx, + ) + .await?; + // ---- Clean up extra_perms referencing this user ---- let extra_perms_tables = [ "script", @@ -2458,17 +2470,6 @@ pub async fn delete_workspace_user_internal( username_to_delete, w_id ).execute(&mut **tx).await?; - // ---- Clean up data table role tenants ---- - - // The username is free once the row below is gone, so a tenant left behind - // would hand every role it names to whoever is invited into it next. - windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( - w_id, - &format!("u/{username_to_delete}"), - tx, - ) - .await?; - // ---- Delete personal data ---- sqlx::query!( "DELETE FROM draft WHERE path LIKE ('u/' || $1 || '/%') AND workspace_id = $2", diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 2932fc52ca..aa54913777 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -116,13 +116,14 @@ pub struct DatatablePermissionsPreview { pub(crate) struct PlannedStatement { pub(crate) sql: String, pub(crate) display: String, - /// Whether this one waits for the config it belongs to to commit. + /// The Postgres role this statement is part of destroying, if any. /// /// Creating a role before the config names it leaves at worst a role the /// next save adopts, so those run inside the request. Dropping one is not - /// reversible — `DROP OWNED` discards every grant it accumulated — so a save - /// that fails after the drop would have destroyed what it then rolls back. - pub(crate) deferred: bool, + /// reversible — `DROP OWNED` discards every grant it accumulated — so these + /// wait for the config that stops naming the role to commit, and are then + /// checked against it one role at a time. + pub(crate) drops_role: Option, } impl PlannedStatement { @@ -132,7 +133,7 @@ impl PlannedStatement { allow(dead_code) )] pub(crate) fn plain(sql: String) -> Self { - Self { display: sql.clone(), sql, deferred: false } + Self { display: sql.clone(), sql, drops_role: None } } } @@ -516,25 +517,54 @@ pub(crate) async fn plan_drop_of_deleted_datatable( } } -/// Run statements the config had to commit before, against the data table's -/// database, once it has. +/// The Postgres logins the workspace's config currently names. +fn pg_rolenames_in_use(settings: Option<&serde_json::Value>) -> HashSet { + settings + .and_then(|s| s.get("datatables")) + .and_then(|d| d.as_object()) + .map(|datatables| { + datatables + .values() + .filter_map(|dt| dt.pointer("/permissions/roles")?.as_object()) + .flat_map(|roles| roles.values()) + .filter_map(|role| role.get("pg_rolename")?.as_str()) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() +} + +/// Destroy the roles a committed config stopped naming. /// -/// `still_wanted` is asked under the settings row, and that row is held through -/// the statements — which is the lock every save of a data table in this -/// workspace takes first, so nothing can adopt or depend on a role between the -/// question and the answer. Returns whether they ran. -async fn run_after_commit( +/// Asked under the settings row, one role at a time: a role the config names +/// again — a data table recreated under the same name, or a save that put the +/// role back — is left alone, and one it does not name is dropped whatever else +/// has changed in the meantime. Nobody else can be planning against these +/// between the question and the answer, since that row is what every save takes +/// first. +/// +/// Best-effort: a role outliving its config is recoverable, dropping one a live +/// data table depends on is not. +async fn drop_roles_the_config_no_longer_names( db: &DB, w_id: &str, client: &mut tokio_postgres::Client, statements: &[&PlannedStatement], - still_wanted: impl FnOnce(Option<&serde_json::Value>) -> bool, -) -> Result { +) -> Result<()> { let mut tx = db.begin().await?; let settings = windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, w_id).await?; - let wanted = still_wanted(settings.as_ref()); - if wanted { + let in_use = pg_rolenames_in_use(settings.as_ref()); + let to_run: Vec<&PlannedStatement> = statements + .iter() + .filter(|s| { + s.drops_role + .as_ref() + .is_none_or(|role| !in_use.contains(role)) + }) + .copied() + .collect(); + if !to_run.is_empty() { // 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 @@ -548,28 +578,14 @@ async fn run_after_commit( pg_error_message(&e) )) })?; - run_statements(client, statements).await?; + run_statements(client, &to_run).await?; } tx.commit().await?; - Ok(wanted) + Ok(()) } -fn datatable_is_configured(settings: Option<&serde_json::Value>, datatable_name: &str) -> bool { - settings - .and_then(|s| s.get("datatables")) - .and_then(|d| d.get(datatable_name)) - .is_some_and(|v| !v.is_null()) -} - -/// Run a planned drop, unless the data table came back. -/// -/// A role name is generated from `(workspace, data table, role)`, so a data -/// table recreated under the same name generates the same names — and its save -/// adopts the very roles this plan plans to drop, since they are still granted -/// to the same admin login. -/// -/// Best-effort: roles outliving their data table are recoverable, dropping the -/// ones a live data table depends on is not. +/// Drop the roles of a data table that was deleted, once the config saying so +/// has committed. pub(crate) async fn run_planned_drop( db: &DB, w_id: &str, @@ -577,19 +593,11 @@ pub(crate) async fn run_planned_drop( (mut client, plan): PlannedRoleDrop, ) { let statements: Vec<&PlannedStatement> = plan.statements.iter().collect(); - match run_after_commit(db, w_id, &mut client, &statements, |settings| { - !datatable_is_configured(settings, datatable_name) - }) - .await + if let Err(e) = drop_roles_the_config_no_longer_names(db, w_id, &mut client, &statements).await { - Ok(false) => tracing::info!( - "Data table {datatable_name} in {w_id} was recreated before its old Postgres roles \ - were dropped; leaving them to the data table that now holds them" - ), - Ok(true) => {} - Err(e) => tracing::error!( + tracing::error!( "Could not drop the Postgres roles of deleted data table {datatable_name} in {w_id}: {e:#}" - ), + ); } } @@ -753,9 +761,21 @@ async fn set_datatable_permissions( // failure after this point leaves roles the config does not know about, which // the next plan adopts (it reads `pg_roles`), whereas the reverse order would // leave the config naming roles that were never created. Dropping one has no - // such way back, so those wait below. - let (deferred, immediate): (Vec<&PlannedStatement>, Vec<&PlannedStatement>) = - plan.statements.iter().partition(|s| s.deferred); + // such way back, so those wait below — except where this save gives the freed + // name to another role, which only works in one order. + let keeps_the_name = |statement: &PlannedStatement| { + statement.drops_role.as_ref().is_some_and(|dropped| { + plan.permissions + .roles + .values() + .filter_map(|r| r.pg_rolename.as_ref()) + .any(|kept| kept == dropped) + }) + }; + let (deferred, immediate): (Vec<&PlannedStatement>, Vec<&PlannedStatement>) = plan + .statements + .iter() + .partition(|s| s.drops_role.is_some() && !keeps_the_name(s)); run_statements(&mut client, &immediate).await?; let permissions = serde_json::to_value(&plan.permissions) @@ -799,23 +819,16 @@ async fn set_datatable_permissions( tx.commit().await?; - // What the config no longer names, now that it says so. Skipped when another - // save has landed since: its own plan was built against `pg_roles` as they - // are, so these roles are its to keep or drop. + // What the config no longer names, now that it says so. if !deferred.is_empty() { - let ours = permissions.clone(); - run_after_commit(&db, &w_id, &mut client, &deferred, |settings| { - settings - .and_then(|s| s.pointer(&format!("/datatables/{datatable_name}/permissions"))) - .is_some_and(|current| *current == ours) - }) - .await - .map_err(|e| { - Error::ExecutionErr(format!( - "Permissions of data table {datatable_name} were saved, but removing the roles \ - they no longer name failed: {e}. Save them again to retry." - )) - })?; + drop_roles_the_config_no_longer_names(&db, &w_id, &mut client, &deferred) + .await + .map_err(|e| { + Error::ExecutionErr(format!( + "Permissions of data table {datatable_name} were saved, but removing the roles \ + they no longer name failed: {e}. Save them again to retry." + )) + })?; } Ok(format!( diff --git a/backend/windmill-api/src/users.rs b/backend/windmill-api/src/users.rs index 7e78219461..aeb1423ce5 100644 --- a/backend/windmill-api/src/users.rs +++ b/backend/windmill-api/src/users.rs @@ -265,6 +265,47 @@ async fn update_username_in_workpsace<'c>( new_username: &str, w_id: &str, ) -> error::Result<()> { + // Before anything else in the transaction — see + // `lock_workspace_settings_unchecked`. `rename_user` walks memberships in + // `workspace_id` order, so a rename spanning workspaces takes their rows in + // that order too. + let datatable_settings = + windmill_common::workspaces::lock_workspace_settings_unchecked(tx, w_id).await?; + + // ---- instance and workspace users ---- + + // Scoped to this workspace, like everything else here: `usr` and + // `usr_to_group` rows of another workspace answer to that workspace's own + // settings row. + sqlx::query!( + "UPDATE usr SET username = $1 WHERE email = $2 AND workspace_id = $3", + new_username, + email, + w_id + ) + .execute(&mut **tx) + .await?; + + sqlx::query!( + "UPDATE usr_to_group SET usr = $1 WHERE usr = $2 AND workspace_id = $3", + new_username, + old_username, + w_id + ) + .execute(&mut **tx) + .await?; + + // ---- group_ ---- + + sqlx::query!( + "UPDATE group_ SET extra_perms = extra_perms - ('u/' || $2) || jsonb_build_object(('u/' || $1), extra_perms->('u/' || $2)) WHERE extra_perms ? ('u/' || $2) AND workspace_id = $3", + new_username, + old_username, + w_id + ) + .execute(&mut **tx) + .await?; + // ---- v2_job ---- sqlx::query!( r#"UPDATE v2_job SET runnable_path = REGEXP_REPLACE(runnable_path,'u/' || $2 || '/(.*)','u/' || $1 || '/\1') WHERE runnable_path LIKE ('u/' || $2 || '/%') AND workspace_id = $3"#, @@ -848,13 +889,6 @@ async fn update_username_in_workpsace<'c>( // executor compares it against the caller's name. Left behind, the rename // takes the role away from the user it followed and hands it to whoever // takes the old name next. - let datatable_settings = sqlx::query_scalar!( - "SELECT datatable FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", - w_id - ) - .fetch_optional(&mut **tx) - .await? - .flatten(); if let Some(mut settings) = datatable_settings { let mut renamed = windmill_common::workspaces::rename_datatable_tenant( &mut settings, @@ -880,40 +914,6 @@ async fn update_username_in_workpsace<'c>( } } - // ---- group_ and workspace users ---- - - // Last, after the settings row above, and scoped to this workspace like - // everything else here: every path that frees or renames a principal takes - // `workspace_settings` before `group_`, `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 group_ SET extra_perms = extra_perms - ('u/' || $2) || jsonb_build_object(('u/' || $1), extra_perms->('u/' || $2)) WHERE extra_perms ? ('u/' || $2) AND workspace_id = $3", - new_username, - old_username, - w_id - ) - .execute(&mut **tx) - .await?; - - sqlx::query!( - "UPDATE usr SET username = $1 WHERE email = $2 AND workspace_id = $3", - new_username, - email, - w_id - ) - .execute(&mut **tx) - .await?; - - sqlx::query!( - "UPDATE usr_to_group SET usr = $1 WHERE usr = $2 AND workspace_id = $3", - new_username, - old_username, - w_id - ) - .execute(&mut **tx) - .await?; - Ok(()) } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 58453b05d2..fdce49840a 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1152,11 +1152,20 @@ pub fn remove_datatable_tenant(datatable: &mut serde_json::Value, tenant: &str) /// Take the workspace's settings row, and read the data table config under it, /// for the length of the caller's transaction. /// -/// Everything that reads that config, decides something from it and writes it -/// back holds this first: a role save, an ACL change, the settings form, and the -/// principal cleanups below. Without it each of them can persist a block it -/// computed before another one committed — a save that planned with `g/devs` -/// puts the tenant back after the group's deletion took it away. +/// This row is the one lock over everything a data table's permissions depend +/// on: the config itself, and the users, groups and folders its roles name as +/// tenants. Every path that touches either — a role save, an ACL change, the +/// settings form, a rename, a deletion — takes it, which is what stops one of +/// them persisting a block it computed before another committed: a save that +/// planned with `g/devs` would otherwise put the tenant back after the group's +/// deletion took it away. +/// +/// **Take it before the transaction locks anything else.** One lock, always +/// acquired first, cannot deadlock; a caller that writes `usr` or `group_` and +/// then reaches for this one holds two in an order some other path holds the +/// other way round. A transaction spanning workspaces takes them in +/// `workspace_id` order, for the same reason. That is the whole ordering rule: +/// what a handler writes after taking it, and in what order, does not matter. /// /// Authorization: performs none, for any workspace it is handed. What it returns /// is the config as stored, generated role passwords included, so callers MUST @@ -1181,10 +1190,7 @@ pub async fn lock_workspace_settings_unchecked( /// /// The tenant and the principal have to go in the same transaction: between the /// two the name is free while a role still names it, and taking it is enough to -/// inherit the role. Call this *before* deleting the principal's own row: this -/// takes the workspace settings row, so a caller that deleted first would hold -/// two rows in the opposite order to every other caller, and two of them at once -/// would deadlock. +/// inherit the role. /// /// Authorization: performs none. Callers MUST have already authorized the /// removal of the principal itself — the rules differ per caller (a workspace