fix(datatables): keep the fork schema baseline, and bounce streams on every removal

Three fixes from review.

`edit_datatable_config` took `forked_from` wholesale from the stored entry, so
the fork schema diff's save of an advanced baseline was silently discarded and
an applied change was offered again. Whether an entry carries a clone stamp is
still carried from the store, since that is what marks its database droppable,
but the baseline inside it is now taken from the request.

The stranded-pointer warning and the stream bounce ran over the optional
`deleted_datatables` hint, which the settings-sync CLI never sends, so removing
a governing data table through `wmill` bounced nothing. Removals are now derived
from the stored configuration against the saved one.

`delete_workspace` read the pointers to bounce before its transaction, so a fork
committing a pointer during the deletion was missed. The read now happens inside
the transaction, after the workspace row is deleted: a fork's insert key-share
locks that row through its parent foreign key, so it is either seen or fails on
the missing parent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Diego Imbert
2026-09-16 15:14:29 +02:00
co-authored by Claude Opus 5
parent 92c723f735
commit b68b97f5ea
3 changed files with 115 additions and 26 deletions
@@ -3803,15 +3803,18 @@ async fn edit_datatable_config(
Some(true)
}
};
// Three fields this form does not own, carried across from the stored entry rather than
// taken from the request. `permissions` is an access decision, edited through its own
// endpoint; `reference` is what makes a fork answer to the workspace that governs its data
// table, and letting a save clear it would hand the fork the database outright; and
// `forked_from` is the clone stamp the fork flow writes. Only fork creation writes any of
// them, so a settings save can neither widen nor lose them.
// Carried across from the stored entry rather than taken from the request. `permissions`
// is an access decision, edited through its own endpoint; `reference` is what makes a fork
// answer to the workspace that governs its data table, and letting a save clear it would
// hand the fork the database outright. `forked_from` is the clone stamp the fork flow
// writes: whether an entry has one is carried the same way, since it is what marks the
// database droppable, but the schema baseline inside it is the diff view's to advance.
dt.permissions = old.and_then(|old| old.permissions.clone());
dt.reference = old.and_then(|old| old.reference.clone());
dt.forked_from = old.and_then(|old| old.forked_from.clone());
dt.forked_from = match old.and_then(|old| old.forked_from.as_ref()) {
Some(stored) => Some(dt.forked_from.take().unwrap_or_else(|| stored.clone())),
None => None,
};
// Carrying the block onto a resource-backed entry would produce a data table the chokepoint
// refuses on every job — a save that succeeds and breaks everything afterwards. Refuse it
// instead: turning roles off first is one step, and it keeps discarding an access decision
@@ -3872,6 +3875,18 @@ async fn edit_datatable_config(
}
}
// Every entry this save removes, derived rather than taken from `deleted_datatables`: that list
// is a hint the settings-sync CLI never sends, and the stranded-pointer warning and the stream
// bounce below must run for a removal whether or not the caller named it.
let removed: Vec<String> = old_datatables
.keys()
.filter(|name| {
!new_config.settings.datatables.contains_key(*name)
&& !new_config.renames.iter().any(|r| &r.from == *name)
})
.cloned()
.collect();
let config: serde_json::Value = serde_json::to_value(new_config.settings)
.map_err(|err| Error::internal_err(err.to_string()))?;
@@ -3911,7 +3926,7 @@ async fn edit_datatable_config(
// A deletion cannot be followed the same way — there is nothing to point at any more. Read who
// is left stranded so the caller is told, the way deleting a workspace does.
let mut stranded: Vec<StrandedReference> = Vec::new();
for name in &new_config.deleted_datatables {
for name in &removed {
let rows = sqlx::query!(
r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!"
FROM workspace_settings ws
@@ -3936,8 +3951,7 @@ async fn edit_datatable_config(
// a listener that reconnects finds the entry gone instead of streaming on.
crate::datatable_permissions::restart_streams_named(
&mut *tx,
new_config
.deleted_datatables
removed
.iter()
.map(|name| (w_id.clone(), name.clone()))
.chain(
@@ -995,21 +995,6 @@ pub(crate) async fn delete_workspace(
// but the destructive cleanup itself runs only after the commit below: a delete that
// fails mid-way must never leave a live workspace with its fork data destroyed and no
// registry row to retry from. Read-only: nothing is dropped here.
// Read before the delete: another workspace's data table entry can point at one of this
// workspace's, and deleting the workspace it names leaves that pointer resolving to nothing.
// Nothing sweeps them — turning them back into copies would hand each fork the database
// outright — so the deleter is told which data tables they just stranded.
let stranded_pointers = sqlx::query!(
r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!"
FROM workspace_settings ws
CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt
WHERE dt.value->'reference'->>'workspace_id' = $1
ORDER BY ws.workspace_id, dt.key"#,
&w_id,
)
.fetch_all(&db)
.await
.unwrap_or_default();
let fork_ducklake_cleanups = prepare_fork_ducklake_cleanups(&db, &w_id, None)
.await
@@ -1265,6 +1250,22 @@ pub(crate) async fn delete_workspace(
None,
)
.await?;
// Who points here, read after the delete above and inside this transaction. A fork writes its
// pointer in the transaction that inserts it, and that insert key-share locks this row through
// the parent foreign key: so the fork either committed before the delete and is seen here, or
// waits on it and then fails on the missing parent. No pointer can escape this list. Nothing
// sweeps them afterwards — turning them back into copies would hand each fork the database
// outright — so the deleter is told which data tables they stranded.
let stranded_pointers = sqlx::query!(
r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!"
FROM workspace_settings ws
CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt
WHERE dt.value->'reference'->>'workspace_id' = $1
ORDER BY ws.workspace_id, dt.key"#,
&w_id,
)
.fetch_all(&mut *tx)
.await?;
// Each fork pointing here keeps the replication stream it opened while this workspace
// governed it. Bounced in this transaction, so a listener that reconnects finds its pointer
// dangling instead of streaming on.