fix(datatables): serialize deletion with role creation, periodic expiry sweep, generation-scoped teardown

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013fBWmrMs2DbpU1iYdCvGX6
This commit is contained in:
Diego Imbert
2026-07-26 11:26:30 +02:00
parent 85651e7fa3
commit 63ecb89528
16 changed files with 337 additions and 85 deletions
@@ -0,0 +1,15 @@
{
"db_name": "PostgreSQL",
"query": "DELETE FROM job_resolution WHERE workspace_id = $1 AND job_id = ANY($2)",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Text",
"UuidArray"
]
},
"nullable": []
},
"hash": "07bcd445061f34a5d370398ee56d98e0e8f42e9e4fd70f00ee5f120fc3b04ed9"
}
@@ -0,0 +1,12 @@
{
"db_name": "PostgreSQL",
"query": "DELETE FROM job_resolution jr WHERE NOT EXISTS (SELECT 1 FROM v2_job WHERE id = jr.job_id)",
"describe": {
"columns": [],
"parameters": {
"Left": []
},
"nullable": []
},
"hash": "0dafd0882f28604872a9187a5a9b3e707644fa3b9833328e75cf599df11e22b2"
}
@@ -0,0 +1,28 @@
{
"db_name": "PostgreSQL",
"query": "INSERT INTO job_resolution (job_id, workspace_id, resolved_by, note, automatic)\n SELECT c.id, c.workspace_id, $4, COALESCE($5, $7), false\n FROM v2_job_completed c\n JOIN v2_job j ON j.id = c.id\n WHERE c.id = ANY($1)\n AND c.workspace_id = $2\n AND ($3::TEXT[] IS NULL OR j.tag = ANY($3))\n AND c.status = 'failure'\n -- Resolution is a top-level triage state: a step resolved on its own\n -- would render orange inside a flow whose status is still red.\n AND j.flow_step_id IS NULL\n -- A supersession claim has to be proven, not trusted: a later success of the\n -- same identified runnable, itself visible to the caller. An unproven claim\n -- resolves nothing, so the caller learns it was rejected instead of having\n -- the fiction recorded as provenance.\n AND ($6::UUID IS NULL OR EXISTS (\n SELECT 1 FROM v2_job_completed sc\n JOIN v2_job sj ON sj.id = sc.id\n WHERE sc.id = $6\n AND sc.workspace_id = $2\n -- Tag scope is a read restriction enforced outside RLS, so it has\n -- to bind the evidence as well: otherwise the result reveals\n -- whether an out-of-scope run succeeded.\n AND ($3::TEXT[] IS NULL OR sj.tag = ANY($3))\n AND sc.status = 'success'\n AND sc.completed_at >= c.completed_at\n AND (j.runnable_id IS NOT NULL OR j.runnable_path IS NOT NULL)\n AND sj.runnable_id IS NOT DISTINCT FROM j.runnable_id\n AND sj.runnable_path IS NOT DISTINCT FROM j.runnable_path\n ))\n ON CONFLICT (job_id) DO UPDATE SET\n resolved_at = now(),\n -- Both COALESCEd: `resolution_attribution` returns NULLs outside EE and once the\n -- licence lapses, and bulk selections routinely include already-resolved rows,\n -- so overwriting would erase metadata recorded while it was valid. Clear either\n -- by unresolving first.\n resolved_by = COALESCE($4, job_resolution.resolved_by),\n -- A person's explanation replaces what was there; machine provenance only fills\n -- a blank, so re-running an already-explained failure never erases their words.\n note = COALESCE($5, job_resolution.note, $7),\n -- A human taking over an automatic resolution makes it no longer automatic.\n automatic = false\n RETURNING job_id",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "job_id",
"type_info": "Uuid"
}
],
"parameters": {
"Left": [
"UuidArray",
"Text",
"TextArray",
"Varchar",
"Text",
"Uuid",
"Text"
]
},
"nullable": [
false
]
},
"hash": "13d8e65ffd3ef6bcfb963525fc3ec0859463b1c153950f894a830a4b5ec33fcc"
}
@@ -1,6 +1,6 @@
{
"db_name": "PostgreSQL",
"query": "SELECT role_name, datatable FROM datatable_ephemeral_role\n WHERE workspace_id = $1 AND ($2::text IS NULL OR datatable = $2)",
"query": "SELECT role_name, datatable FROM datatable_ephemeral_role WHERE workspace_id = $1",
"describe": {
"columns": [
{
@@ -16,7 +16,6 @@
],
"parameters": {
"Left": [
"Text",
"Text"
]
},
@@ -25,5 +24,5 @@
false
]
},
"hash": "e5c2eb0b1303e0310d1002562fabcdb68b75a2a86141a6b2911f4c4625f2d075"
"hash": "3258241cf28c17d9b5dcf06f6a3e4b182ae4393eb8fb22d8521de538704d63fd"
}
@@ -0,0 +1,24 @@
{
"db_name": "PostgreSQL",
"query": "DELETE FROM job_resolution r\n USING v2_job_completed c\n JOIN v2_job j ON j.id = c.id\n WHERE r.job_id = c.id\n AND c.id = ANY($1)\n AND c.workspace_id = $2\n AND ($3::TEXT[] IS NULL OR j.tag = ANY($3))\n RETURNING r.job_id",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "job_id",
"type_info": "Uuid"
}
],
"parameters": {
"Left": [
"UuidArray",
"Text",
"TextArray"
]
},
"nullable": [
false
]
},
"hash": "41468bb803b63c190728938616a46409057a16a2723916d973eac5c70c2dc2c7"
}
@@ -0,0 +1,14 @@
{
"db_name": "PostgreSQL",
"query": "DELETE FROM job_resolution WHERE job_id = ANY($1)",
"describe": {
"columns": [],
"parameters": {
"Left": [
"UuidArray"
]
},
"nullable": []
},
"hash": "58bef783a5d85eb114293bd0e43b01013e27017765b3fdade4322ebdf1fe1408"
}
@@ -0,0 +1,28 @@
{
"db_name": "PostgreSQL",
"query": "SELECT owner_creds, perms_hash FROM datatable_ephemeral_role WHERE role_name = $1",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "owner_creds",
"type_info": "Text"
},
{
"ordinal": 1,
"name": "perms_hash",
"type_info": "Text"
}
],
"parameters": {
"Left": [
"Text"
]
},
"nullable": [
true,
false
]
},
"hash": "9b7255c12350fa9e493d6dd0d85c3fe5c6884cb18252390ebcfcd5fcad1c345f"
}
@@ -0,0 +1,35 @@
{
"db_name": "PostgreSQL",
"query": "SELECT role_name, datatable, perms_hash FROM datatable_ephemeral_role\n WHERE workspace_id = $1 AND ($2::text IS NULL OR datatable = $2)",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "role_name",
"type_info": "Text"
},
{
"ordinal": 1,
"name": "datatable",
"type_info": "Varchar"
},
{
"ordinal": 2,
"name": "perms_hash",
"type_info": "Text"
}
],
"parameters": {
"Left": [
"Text",
"Text"
]
},
"nullable": [
false,
false,
false
]
},
"hash": "a99066262acf851bd6f5f05c75d6fd43ebe1e723660cb6c894b1c914842a8f8e"
}
@@ -0,0 +1,14 @@
{
"db_name": "PostgreSQL",
"query": "WITH ids AS (SELECT id FROM v2_job WHERE workspace_id = $1),\n _de AS (DELETE FROM dispatch_event WHERE workspace_id = $1),\n _jr AS (DELETE FROM job_resolution WHERE workspace_id = $1),\n _fc AS (DELETE FROM flow_conversation_message WHERE job_id IN (SELECT id FROM ids))\n DELETE FROM zombie_job_counter WHERE job_id IN (SELECT id FROM ids)",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Text"
]
},
"nullable": []
},
"hash": "c65ce3580a6648a7168c0003d9ec4465bc94bf7845f1d38f6d0fd20f1154ff9e"
}
@@ -1,22 +0,0 @@
{
"db_name": "PostgreSQL",
"query": "SELECT owner_creds FROM datatable_ephemeral_role WHERE role_name = $1",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "owner_creds",
"type_info": "Text"
}
],
"parameters": {
"Left": [
"Text"
]
},
"nullable": [
true
]
},
"hash": "ddb7d633931e71c1620a10711593e323c6dd4eae99d42e098cb0d0f8ea8305fa"
}
@@ -0,0 +1,16 @@
{
"db_name": "PostgreSQL",
"query": "INSERT INTO job_resolution (job_id, workspace_id, resolved_by, note, automatic)\n SELECT c.id, c.workspace_id, NULL, NULL, true\n FROM v2_job_completed c\n JOIN v2_job j ON j.id = c.id\n WHERE c.status = 'failure'\n AND c.workspace_id = $2\n AND j.flow_step_id IS NULL\n AND j.runnable_id IS NOT DISTINCT FROM $3\n AND EXISTS (\n SELECT 1 FROM v2_job_completed sc\n JOIN v2_job sj ON sj.id = sc.id\n JOIN native_retry_attempt nra ON nra.job_id = sc.id\n WHERE sc.status = 'success'\n AND sc.workspace_id = $2\n AND sj.parent_job = $1\n AND sj.runnable_id IS NOT DISTINCT FROM $3\n )\n AND (\n (j.parent_job = $1\n AND EXISTS (SELECT 1 FROM native_retry_attempt WHERE job_id = c.id))\n OR (c.id = $1 AND j.parent_job IS NULL\n AND NOT EXISTS (SELECT 1 FROM native_retry_attempt WHERE job_id = c.id))\n )\n ON CONFLICT (job_id) DO NOTHING",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Uuid",
"Text",
"Int8"
]
},
"nullable": []
},
"hash": "e34cb7cd6ff9d80e77a51ae47c3cfb9b0772d5d3d239273fc5cea322cae4e258"
}
+6
View File
@@ -1259,6 +1259,12 @@ async fn report_token_expiration(db: &DB, token: &TokenRow, expired: bool) {
}
pub async fn delete_expired_items(db: &DB) -> () {
// Expired data table ephemeral roles must be revoked even when no new role
// creation happens to trigger the opportunistic sweep — a revoked caller
// could otherwise keep reconnecting with a self-set password until some
// unrelated access finally sweeps.
windmill_common::datatable_permissions::cleanup_expired_datatable_roles(db, 20).await;
let expired_tokens_r = sqlx::query_as!(
TokenRow,
"DELETE FROM token WHERE expiration <= now()
@@ -12,7 +12,7 @@ use windmill_audit::audit_oss::audit_log;
use windmill_audit::ActionKind;
use windmill_common::datatable_permissions::{
compute_effective_grants, datatable_license_valid, datatable_permissions_enabled,
drop_datatable_ephemeral_roles_best_effort, validate_grant_identifier,
snapshot_datatable_roles, teardown_snapshot_roles_best_effort, validate_grant_identifier,
PERMISSIONED_AS_FOLDER_PREFIX,
};
use windmill_common::error::{Error, JsonResult, Result};
@@ -252,6 +252,10 @@ async fn set_datatable_permissions(
),
)
.await?;
// Captured before the commit so the post-commit teardown only revokes the
// pre-edit role generation — a role concurrently recreated from the new
// grants must keep working.
let pre_edit_roles = snapshot_datatable_roles(&db, &w_id, Some(&datatable_name)).await?;
let perms_json =
serde_json::to_value(&perms).map_err(|e| Error::internal_err(e.to_string()))?;
sqlx::query!(
@@ -266,10 +270,9 @@ async fn set_datatable_permissions(
.await?;
tx.commit().await?;
// Existing ephemeral roles were built from the previous grants; drop them
// so nothing outlives the edit (new accesses would recreate them via the
// perms-hash check anyway, and disabled data tables stop using them).
drop_datatable_ephemeral_roles_best_effort(&db, &w_id, Some(&datatable_name)).await;
// The pre-edit roles were built from the previous grants; revoke them so
// nothing outlives the edit (active holders get NOLOGIN'd, the rest drop).
teardown_snapshot_roles_best_effort(&db, &w_id, pre_edit_roles).await;
Ok(format!(
"Edited permissions of data table {datatable_name} in workspace {w_id}"
@@ -3205,27 +3205,36 @@ async fn edit_datatable_config(
)
.await?;
// Tear down ephemeral roles of deleted/renamed/re-pointed data tables
// while the old config (needed to reach their previous database) is still
// readable outside this transaction — after the switch the cleanup sweep
// would look for the roles on the wrong cluster. Renames and database
// changes get fresh roles on next access.
// Capture the roles of deleted/renamed/re-pointed data tables before the
// commit: the post-commit teardown revokes exactly this pre-edit
// generation (their rows carry the recorded previous target), while any
// role recreated from the new config keeps working.
let mut pre_edit_roles = vec![];
for name in new_config
.deleted_datatables
.iter()
.chain(new_config.renames.iter().map(|r| &r.from))
.chain(database_changed_names.iter())
{
windmill_common::datatable_permissions::drop_datatable_ephemeral_roles_best_effort(
&db,
&w_id,
Some(name),
)
.await;
pre_edit_roles.extend(
windmill_common::datatable_permissions::snapshot_datatable_roles(
&db,
&w_id,
Some(name),
)
.await?,
);
}
tx.commit().await?;
windmill_common::datatable_permissions::teardown_snapshot_roles_best_effort(
&db,
&w_id,
pre_edit_roles,
)
.await;
Ok(format!("Edit datatable config for workspace {}", &w_id))
}
@@ -928,7 +928,13 @@ pub(crate) async fn delete_workspace(
// rows and the workspace key (which encrypts their recorded targets) still
// exist — both cascade with the workspace row, after which an unrevoked
// role could never be reached again. Failure (e.g. an unreachable external
// cluster) aborts the deletion; retry once the database is reachable.
// cluster) aborts the deletion; retry once the database is reachable. The
// workspace-scoped lock (held on this transaction until commit) keeps a
// concurrent resolution from creating a fresh role after the teardown scan.
sqlx::query(windmill_common::datatable_permissions::WORKSPACE_ROLES_LOCK)
.bind(&w_id)
.execute(&mut *tx)
.await?;
windmill_common::datatable_permissions::teardown_datatable_roles_strict(&db, &w_id)
.await
.map_err(|e| {
@@ -27,6 +27,12 @@ use crate::{
/// to touch a role that does not carry it.
pub const DATATABLE_EPHEMERAL_ROLE_PREFIX: &str = "wm_dt_";
/// Transaction-scoped advisory lock serializing a workspace's ephemeral role
/// creation with its deletion-time strict teardown. Bind the workspace id.
/// Lock order where both are taken: workspace lock first, then per-role lock.
pub const WORKSPACE_ROLES_LOCK: &str =
"SELECT pg_advisory_xact_lock(hashtextextended('wm_dt_ws:' || $1::text, 0))";
pub const PERMISSIONED_AS_FOLDER_PREFIX: &str = "f/";
const EPHEMERAL_ROLE_CONNECTION_LIMIT: u32 = 25;
@@ -586,7 +592,7 @@ async fn drop_or_disable_on_target(
.map_err(|e| pg_err("checking active sessions", e))?
.get(0);
if active > 0 {
disable_role_login(&client, role).await;
disable_role_login(&client, role).await?;
return Ok(DropOutcome::SkippedActive);
}
if role_exists(&client, role).await? {
@@ -652,8 +658,16 @@ async fn ensure_ephemeral_role(
// Slow path: (re)create the role under a per-role advisory lock on the
// main DB (advisory locks are per-database — never take them on the
// target cluster).
// target cluster). The workspace-scoped lock (taken first — same order as
// workspace deletion, which holds it from strict teardown through its
// commit) keeps a creation from racing workspace deletion: a role created
// after deletion's teardown scan would be orphaned forever once the
// bookkeeping rows and workspace key cascade away.
let mut tx = db.begin().await?;
sqlx::query(WORKSPACE_ROLES_LOCK)
.bind(w_id)
.execute(&mut *tx)
.await?;
sqlx::query("SELECT pg_advisory_xact_lock(hashtextextended('wm_dt_role:' || $1, 0))")
.bind(&role)
.execute(&mut *tx)
@@ -910,47 +924,81 @@ async fn cleanup_one_expired_role(db: &DB, role: &str, w_id: &str, datatable: &s
Ok(())
}
/// Best-effort teardown of every ephemeral role of a data table (or a whole
/// workspace when `datatable` is `None`), used on data table deletion,
/// database re-points and permission edits. Roles that cannot be dropped keep
/// their bookkeeping row (with the recorded target), so the expiry sweep
/// retries them — failure here loses nothing.
///
/// Authorization: mutates cluster roles; call only from admin-gated flows.
pub async fn drop_datatable_ephemeral_roles_best_effort(
/// One bookkeeping row captured before a permissions/config edit commits.
/// The post-commit teardown only revokes roles still carrying the captured
/// hash: a role concurrently recreated from the NEW config has a different
/// hash and must keep working.
pub struct RoleSnapshot {
role_name: String,
datatable: String,
perms_hash: String,
}
/// Capture the current roles of a data table (or whole workspace) with their
/// perms hashes. Take this BEFORE committing the edit that invalidates them
/// (ideally under the same serialization lock), then pass it to
/// [`teardown_snapshot_roles_best_effort`] after the commit.
pub async fn snapshot_datatable_roles(
db: &DB,
w_id: &str,
datatable: Option<&str>,
) {
if let Err(e) = teardown_datatable_roles(db, w_id, datatable).await {
tracing::warn!("tearing down datatable ephemeral roles: {e:#}");
}
}
/// Strict variant for workspace deletion: the bookkeeping rows (and the
/// workspace key their targets are encrypted with) are about to cascade away,
/// so every role must be revoked NOW — dropped, or at least stripped of LOGIN
/// when sessions are still active. Any failure (e.g. an unreachable external
/// cluster) must abort the deletion; proceeding would orphan a live LOGIN
/// role with no remaining way to ever revoke it.
///
/// Authorization: mutates cluster roles; call only from admin-gated flows.
pub async fn teardown_datatable_roles_strict(db: &DB, w_id: &str) -> Result<()> {
teardown_datatable_roles(db, w_id, None).await
}
async fn teardown_datatable_roles(db: &DB, w_id: &str, datatable: Option<&str>) -> Result<()> {
let rows = sqlx::query!(
"SELECT role_name, datatable FROM datatable_ephemeral_role
) -> Result<Vec<RoleSnapshot>> {
Ok(sqlx::query!(
"SELECT role_name, datatable, perms_hash FROM datatable_ephemeral_role
WHERE workspace_id = $1 AND ($2::text IS NULL OR datatable = $2)",
w_id,
datatable
)
.fetch_all(db)
.await?
.into_iter()
.map(|r| RoleSnapshot {
role_name: r.role_name,
datatable: r.datatable,
perms_hash: r.perms_hash,
})
.collect())
}
/// Best-effort teardown of a pre-edit role generation, used after permission
/// edits, data table deletion/renames and database re-points commit. Roles
/// that cannot be dropped keep their bookkeeping row (with the recorded
/// target), so the expiry sweep retries them — failure here loses nothing.
///
/// Authorization: mutates cluster roles; call only from admin-gated flows.
pub async fn teardown_snapshot_roles_best_effort(db: &DB, w_id: &str, snapshot: Vec<RoleSnapshot>) {
for s in snapshot {
if let Err(e) =
teardown_role(db, w_id, &s.datatable, &s.role_name, Some(&s.perms_hash)).await
{
tracing::warn!(
"tearing down datatable ephemeral role {}: {e:#}",
s.role_name
);
}
}
}
/// Strict teardown of every role of a workspace, for workspace deletion: the
/// bookkeeping rows (and the workspace key their targets are encrypted with)
/// are about to cascade away, so every role must be revoked NOW — dropped, or
/// at least stripped of LOGIN when sessions are still active. Any failure
/// (e.g. an unreachable external cluster) must abort the deletion; proceeding
/// would orphan a live LOGIN role with no remaining way to ever revoke it.
/// Callers must hold [`WORKSPACE_ROLES_LOCK`] until the deletion commits so
/// no new role is created after this scan.
///
/// Authorization: mutates cluster roles; call only from admin-gated flows.
pub async fn teardown_datatable_roles_strict(db: &DB, w_id: &str) -> Result<()> {
let rows = sqlx::query!(
"SELECT role_name, datatable FROM datatable_ephemeral_role WHERE workspace_id = $1",
w_id
)
.fetch_all(db)
.await?;
let mut failures: Vec<String> = vec![];
for row in rows {
if let Err(e) = teardown_role(db, w_id, &row.datatable, &row.role_name).await {
if let Err(e) = teardown_role(db, w_id, &row.datatable, &row.role_name, None).await {
failures.push(format!("{} ({}): {e:#}", row.role_name, row.datatable));
}
}
@@ -970,19 +1018,22 @@ async fn teardown_datatable_roles(db: &DB, w_id: &str, datatable: Option<&str>)
/// revocation. Only CREATEROLE can restore LOGIN, so this closes the
/// reconnect vector while letting in-flight queries finish; the kept
/// bookkeeping row makes the expiry sweep finish the drop later.
async fn disable_role_login(client: &tokio_postgres::Client, role: &str) {
/// Failure is propagated: callers (notably strict workspace-deletion
/// teardown) must not treat an active role as revoked when it still holds
/// LOGIN.
async fn disable_role_login(client: &tokio_postgres::Client, role: &str) -> Result<()> {
if !role.starts_with(DATATABLE_EPHEMERAL_ROLE_PREFIX) {
return;
return Err(Error::internal_err(format!(
"refusing to alter role '{role}' without the reserved prefix"
)));
}
if let Err(e) = client
client
.batch_execute(&format!(
"ALTER ROLE {} NOLOGIN CONNECTION LIMIT 0 PASSWORD NULL",
quote_ident(role)
))
.await
{
tracing::warn!("disabling login of ephemeral role {role}: {e:#}");
}
.map_err(|e| pg_err(&format!("disabling login of ephemeral role {role}"), e))
}
/// Where a role must be revoked: the stored owner target from its bookkeeping
@@ -1023,7 +1074,13 @@ async fn resolve_role_target(
}
}
async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Result<()> {
async fn teardown_role(
db: &DB,
w_id: &str,
datatable: &str,
role: &str,
only_if_hash: Option<&str>,
) -> Result<()> {
// Same per-role lock as role creation and the expiry sweep: without it,
// teardown could observe a stale role while a resolver is recreating it
// and drop the fresh role right before its bookkeeping row lands, leaving
@@ -1033,13 +1090,21 @@ async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Resu
.bind(role)
.execute(&mut *tx)
.await?;
let owner_creds = sqlx::query_scalar!(
"SELECT owner_creds FROM datatable_ephemeral_role WHERE role_name = $1",
let row = sqlx::query!(
"SELECT owner_creds, perms_hash FROM datatable_ephemeral_role WHERE role_name = $1",
role
)
.fetch_optional(&mut *tx)
.await?
.flatten();
.await?;
if let (Some(expected), Some(row)) = (only_if_hash, row.as_ref()) {
// The role was recreated from the post-edit config while we were
// getting here — it is current, not the generation this teardown
// targets. Leave it alone.
if row.perms_hash != expected {
return Ok(());
}
}
let owner_creds = row.and_then(|r| r.owner_creds);
if let Some((owner, is_instance)) = resolve_role_target(db, w_id, datatable, owner_creds).await
{
if matches!(