From 04bae85e939259c9096bcb4baffa1c954c251dab Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 14:46:53 +0200 Subject: [PATCH] refactor: pin the version this deploy wrote even when one landed on top, and tighten three comment blocks Co-Authored-By: Claude Opus 5 (1M context) --- .../windmill-api-workspaces/src/workspaces.rs | 21 ++++------ backend/windmill-common/src/user_drafts.rs | 39 +++++++------------ frontend/src/lib/components/DiffDrawer.svelte | 16 ++++---- .../apps/editor/AppEditorHeader.svelte | 2 +- .../apps/editor/appDeploy.svelte.ts | 19 +++------ .../components/apps/editor/appDeploy.test.ts | 18 +++++++-- .../raw_apps/RawAppEditorHeader.svelte | 3 +- 7 files changed, 54 insertions(+), 64 deletions(-) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index a899189b95..23b6ce615f 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -7316,20 +7316,15 @@ async fn clone_drafts( // clone's own principal at deploy time, which is the more accurate answer of the two. sqlx::query!( // A script hash is content-addressed and copied as-is, so a script draft's base - // still names a version the clone has. `clone_flows` / `clone_apps` mint new - // `flow_version` / `app_version` ids, so those drafts arrive with no base (their - // staleness falls back to the timestamps) rather than one naming a version of the - // source workspace, and the lineage field goes with it so the next autosave cannot - // re-derive the source id. + // still names a version the clone has. `clone_flows` / `clone_apps` mint new ids, + // so those drafts arrive with no base (staleness falls back to the timestamps), + // lineage field included, or the next autosave would re-derive the source id. // - // `clean` is `strip_json_nul`'s parity rule in SQL: a draft written before that - // sanitizer can carry a U+0000 escape, which `to_jsonb` rejects. Sanitizing on the - // way in keeps such a row from either aborting the clone or arriving with its - // principal unstripped. Escaped backslashes are parked on chr(1) first, so only an - // odd-parity backslash-u0000 (a real NUL) is removed; chr(1) is lossless as a - // placeholder because a `json` value's text cannot hold a raw control byte (JSON - // escapes them), so nothing in the payload can collide with it. chr(92) spells the - // backslash so no escape sequence reaches this source file. + // `clean` is `strip_json_nul`'s parity rule in SQL, so a pre-sanitizer U+0000 + // escape cannot abort the clone on `to_jsonb` or arrive with its principal + // unstripped: escaped backslashes park on chr(1) (lossless, a `json` value's text + // cannot hold a raw control byte) so only a real NUL is removed, and chr(92) + // spells the backslash so no escape sequence reaches this source file. r#"INSERT INTO draft (workspace_id, path, typ, value, created_at, email, base) SELECT $2, path, typ, to_json( diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index 563cd83cf5..cf9fcee3fd 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -529,7 +529,6 @@ pub async fn delete_drafts_of_email(conn: &mut sqlx::PgConnection, email: &str) Ok(()) } - /// Move the drafts an address owns onto its new address, for the same reason /// [`delete_drafts_of_email`] exists: no foreign key follows the rename, so drafts left behind are /// stranded on an address that no longer authenticates. Same authorization contract, for a rename. @@ -651,26 +650,19 @@ pub async fn delete_own_draft_for_path( /// string, so without this a move detaches every draft on the item. No owner /// filter: teammates' rows and the legacy NULL-email row follow too. /// -/// **The caller must have authorized the underlying item move first.** This -/// rewrites every owner's row at `old_path`, deliberately including rows the -/// caller has no permission on, and enforces nothing itself — it takes the paths -/// on trust. It is safe only because it runs inside a deploy that has already -/// cleared both paths for the caller; reached any other way it is a cross-user -/// write with no gate. +/// **The caller must have authorized the underlying item move first.** This rewrites +/// rows the caller has no permission on and enforces nothing itself, taking both paths +/// on trust: it is safe only inside a deploy that has already cleared them, and is a +/// cross-user write with no gate anywhere else. /// -/// The value keeps its base version: a move is a deploy like any other, so every -/// draft on the item is now behind the head, which the editor reports through -/// the ordinary stale-draft prompt. Of its two path keys (`path`, `draft_path`), -/// one still naming `old_path` is not a rename the user staged — the editors write -/// the item's own path there on every save — so it follows the row, or deploying -/// the draft would send the item back where it came from. Any other value is a -/// staged rename, and is kept. +/// The value keeps its base version, so every carried draft reads as behind the head the +/// move minted. Of its two path keys, one still naming `old_path` follows the row (the +/// editors write the item's own path there on every save, so it is the item's path and +/// not a staged rename); any other value is a rename the user staged, and is kept. /// -/// A draft already at `new_path` (a never-deployed item, or one left on an -/// archived script there) occupies that path the way a deployed item does, and -/// the move is refused with `BadRequest` — inside the deploy's transaction, so -/// the rename itself is what gets refused. Moving onto it would either merge two -/// items or strand the row that lost the collision. +/// A draft already at `new_path` occupies it the way a deployed item does, so the move is +/// refused with `BadRequest` inside the deploy's transaction, refusing the rename itself: +/// moving onto it would merge two items or strand the row that lost. pub async fn move_drafts_for_path( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, w_id: &str, @@ -760,11 +752,10 @@ pub async fn move_drafts_for_path( /// draft write at `old_path` (any owner's, for an item move), and enforces nothing /// itself. /// -/// A record outlives the editors that need it: it ends when a later move touches -/// either path or an item is deployed at `old_path`, so a save that means to start -/// a NEW draft at a vacated path would be routed instead. Nothing does that today — -/// every surface parks a new item at a minted `u//draft_` key -/// (`mintDraftPath.ts`) and carries the user-typed name inside the value. +/// A record outlives the editors that need it: it ends when a later move touches either +/// path or an item is deployed at `old_path`, so a save meaning to start a NEW draft at a +/// vacated path would be routed instead. Nothing does that today: every surface parks a +/// new item at a minted `u//draft_` key (`mintDraftPath.ts`). pub async fn record_draft_move( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, w_id: &str, diff --git a/frontend/src/lib/components/DiffDrawer.svelte b/frontend/src/lib/components/DiffDrawer.svelte index 2250acda5d..120d939f31 100644 --- a/frontend/src/lib/components/DiffDrawer.svelte +++ b/frontend/src/lib/components/DiffDrawer.svelte @@ -85,10 +85,9 @@ } /** Counted per opening, and counted here rather than in the editor that opens one: a - * path change remounts the editor while this drawer stays mounted, so a counter local - * to the editor is one an outlived request still matches, and it would open and fill - * the drawer with the item the user just left. Every write an opening makes (the - * blanking `openDrawer` included) checks `ownsOpening` first. */ + * path change remounts the editor while this drawer stays, so a counter local to it is + * one an outlived request still matches, and that request would fill the drawer with + * the item the user just left. Every write an opening makes checks this first. */ let openingToken = 0 export function beginOpening(): number { @@ -99,11 +98,10 @@ return token === openingToken } - /** Drop an opening and everything it put on screen: the editor that started it is going - * away (a path change remounts it), so a diff it already filled describes an item this - * drawer can no longer act on — Take latest would move the base of whatever loaded in - * its place — and one still in flight would leave the spinner behind. A no-op once - * another opening owns the drawer: what it shows is then someone else's. */ + /** Drop an opening and everything it put on screen: its editor is going away, so a diff + * it filled acts on an item that is gone (Take latest would move the base of whatever + * loaded in its place) and one still in flight would leave the spinner behind. A no-op + * once another opening owns the drawer: what it shows is then someone else's. */ export function abandonOpening(token: number) { if (token !== openingToken) return openingToken++ diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte index 497df93496..f4cc212870 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte @@ -393,7 +393,7 @@ path: npath }) version = versionThisDeployWrote(appHistory, $userStore?.username) - // Re-pin the fork base to the just-deployed head: the editor stays open, so a + // Re-pin the fork base to the version just written: the editor stays open, so a // follow-up deploy (or a new edit) would otherwise compare against the now- // superseded base and falsely warn. parent_version is in // DRAFT_COMPARE_IGNORED_FIELDS, so this write can't spawn a spurious draft. diff --git a/frontend/src/lib/components/apps/editor/appDeploy.svelte.ts b/frontend/src/lib/components/apps/editor/appDeploy.svelte.ts index b071166dee..810b3b53ea 100644 --- a/frontend/src/lib/components/apps/editor/appDeploy.svelte.ts +++ b/frontend/src/lib/components/apps/editor/appDeploy.svelte.ts @@ -7,22 +7,15 @@ export function computeSecretUrl(secretUrl: string) { } /** - * The version a just-finished deploy wrote, read back from the history it lands in. - * The deploy and this read are two requests, so someone else's deploy in between is the - * newest entry too; taking it would make their version the fork base of content it never - * contained, and the next deploy would find base === head and overwrite them with no - * warning. A head this caller did not write therefore yields `undefined`: an unknown - * base, which the timestamps still cover, rather than a wrong one. - * - * Author is all the history can be matched on (`deployment_msg` is written later, by the - * dependency job), so a second deploy by the same user inside that window still reads as - * this one's. + * The version a just-finished deploy wrote: this caller's newest history entry, which the + * awaited deploy guarantees is the one it wrote. Taking the head instead would pin a + * deploy that landed in between as the base of content it never contained, and the next + * deploy would then find base === head and overwrite it with no warning. */ export function versionThisDeployWrote( history: { version: number; created_by?: string }[] | undefined, deployedBy: string | undefined ): number | undefined { - const head = history?.[0] - if (!head || !deployedBy || head.created_by !== deployedBy) return undefined - return head.version + if (!deployedBy) return undefined + return history?.find((h) => h.created_by === deployedBy)?.version } diff --git a/frontend/src/lib/components/apps/editor/appDeploy.test.ts b/frontend/src/lib/components/apps/editor/appDeploy.test.ts index 184df32a96..e29b08e36c 100644 --- a/frontend/src/lib/components/apps/editor/appDeploy.test.ts +++ b/frontend/src/lib/components/apps/editor/appDeploy.test.ts @@ -6,9 +6,21 @@ describe('versionThisDeployWrote', () => { expect(versionThisDeployWrote([{ version: 7, created_by: 'alice' }], 'alice')).toBe(7) }) - it('claims nothing when another deploy landed on top', () => { - // The base would otherwise be a version this content never contained, and the - // next deploy would find base === head and overwrite it unwarned. + it('finds its own version under a deploy that landed on top', () => { + // Pinning the head would make Bob's version the base of content it never + // contained, and the next deploy would find base === head and overwrite it. + expect( + versionThisDeployWrote( + [ + { version: 8, created_by: 'bob' }, + { version: 7, created_by: 'alice' } + ], + 'alice' + ) + ).toBe(7) + }) + + it('claims nothing it cannot attribute', () => { expect(versionThisDeployWrote([{ version: 8, created_by: 'bob' }], 'alice')).toBe(undefined) expect(versionThisDeployWrote([], 'alice')).toBe(undefined) expect(versionThisDeployWrote([{ version: 7, created_by: 'alice' }], undefined)).toBe(undefined) diff --git a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte index bd5923116b..b8e4ce8686 100644 --- a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte @@ -545,7 +545,8 @@ workspace: opWorkspace!, path: npath }) - // Only a version this deploy can claim becomes the draft's base below. + // The version this deploy wrote, not the head: they differ when someone else's + // deploy landed in between, and this becomes the next draft's base below. version = versionThisDeployWrote(appHistory, $userStore?.username) closeSaveDrawer()