diff --git a/backend/.sqlx/query-3e3651d23d759a4cb3480afb5d12a05791832d91ca27ab2be3370ad19e46fc1a.json b/backend/.sqlx/query-07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8.json similarity index 53% rename from backend/.sqlx/query-3e3651d23d759a4cb3480afb5d12a05791832d91ca27ab2be3370ad19e46fc1a.json rename to backend/.sqlx/query-07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8.json index 3e69d0950d..a8b1eaa95c 100644 --- a/backend/.sqlx/query-3e3651d23d759a4cb3480afb5d12a05791832d91ca27ab2be3370ad19e46fc1a.json +++ b/backend/.sqlx/query-07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT\n (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND (email = $4 OR email IS NULL)\n LIMIT 1) as \"at_target\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email IS NULL) as \"legacy!\" ", + "query": "SELECT\n -- Own row first: with both an own and a legacy row at the destination,\n -- a bare LIMIT 1 would name an arbitrary one and the two need different\n -- remedies (discard your own vs. ask an admin).\n (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND (email = $4 OR email IS NULL)\n ORDER BY email NULLS LAST LIMIT 1) as \"at_target\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email = $4) as \"at_target_own!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email IS NULL) as \"legacy!\" ", "describe": { "columns": [ { @@ -10,11 +10,16 @@ }, { "ordinal": 1, - "name": "poisoned!", + "name": "at_target_own!", "type_info": "Bool" }, { "ordinal": 2, + "name": "poisoned!", + "type_info": "Bool" + }, + { + "ordinal": 3, "name": "legacy!", "type_info": "Bool" } @@ -64,10 +69,11 @@ ] }, "nullable": [ + null, null, null, null ] }, - "hash": "3e3651d23d759a4cb3480afb5d12a05791832d91ca27ab2be3370ad19e46fc1a" + "hash": "07247926f7acb6578cd0edd021e56b0d0cadbc506823f6f2051f8764bf727fb8" } diff --git a/backend/tests/drafts_move_taken.rs b/backend/tests/drafts_move_taken.rs index eee01c79dd..fc2790e3dc 100644 --- a/backend/tests/drafts_move_taken.rs +++ b/backend/tests/drafts_move_taken.rs @@ -74,6 +74,59 @@ async fn test_rename_onto_a_draft_is_refused(db: Pool) -> anyhow::Resu Ok(()) } +/// A legacy (ownerless) draft occupies its path too: a deploy there deletes it together +/// with the caller's own row, so a move that parks a second draft beside it would discard +/// edits the caller never saw. Only an admin can clear it, so the refusal says so. +#[sqlx::test(fixtures("base", "drafts_move_taken"))] +async fn test_draft_move_refuses_a_legacy_destination(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let resp = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/move/script/u/test-user/mvtaken_b" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ "new_path": "u/test-user/mvtaken_legacy" })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 400, "move onto a legacy draft was allowed: {body}"); + assert!( + body.contains("legacy workspace draft") && body.contains("workspace admin"), + "the refusal did not point at the one remedy: {body}" + ); + + // Both rows stayed where they were: the caller's own, and the legacy one the list + // synthesizes under the caller's name. + let list: Vec = reqwest::Client::new() + .get(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/list" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .send() + .await? + .json() + .await?; + let mut at = list + .iter() + .filter_map(|d| Some((d["kind"].as_str()?, d["path"].as_str()?))) + .filter(|(_, p)| p.starts_with("u/test-user/mvtaken_b") || p.ends_with("mvtaken_legacy")) + .collect::>(); + at.sort(); + assert_eq!( + at, + vec![ + ("script", "u/test-user/mvtaken_b"), + ("script", "u/test-user/mvtaken_legacy") + ], + "{list:?}" + ); + Ok(()) +} + /// A classic app and a raw app deploy into the same table, so a draft-only move onto /// the other kind's draft must be refused: deploying either path afterwards deletes /// the caller's drafts of both kinds, taking the loser's item with it. @@ -93,7 +146,10 @@ async fn test_draft_move_refuses_the_other_app_kind(db: Pool) -> anyho .await?; let status = resp.status(); let body = resp.text().await?; - assert_eq!(status, 400, "move onto a classic app draft was allowed: {body}"); + assert_eq!( + status, 400, + "move onto a classic app draft was allowed: {body}" + ); assert!( body.contains("already have a draft at 'u/test-user/mvtaken_app' (app)"), "the refusal did not name the occupying kind: {body}" diff --git a/backend/tests/fixtures/drafts_move_taken.sql b/backend/tests/fixtures/drafts_move_taken.sql index debb27c734..85b2543b26 100644 --- a/backend/tests/fixtures/drafts_move_taken.sql +++ b/backend/tests/fixtures/drafts_move_taken.sql @@ -16,6 +16,13 @@ VALUES ('test-workspace', 'u/test-user/mvtaken_b', 'script', '{"path": "u/test-user/mvtaken_b", "summary": "B", "content": ""}', 'test@windmill.dev'); +-- An ownerless (pre per-user drafts) row at a third path: a deploy there deletes it +-- together with the caller's, so it occupies the path just as an own draft does. +INSERT INTO draft (workspace_id, path, typ, value, email) +VALUES ('test-workspace', 'u/test-user/mvtaken_legacy', 'script', + '{"path": "u/test-user/mvtaken_legacy", "summary": "legacy", "content": ""}', + NULL); + -- A draft-only classic app and a draft-only raw app of the same owner. They share -- the `app` table, so one occupies the other's path. INSERT INTO draft (workspace_id, path, typ, value, email) VALUES diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index abd0d80205..948358e217 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -773,9 +773,14 @@ async fn move_draft( if moved.is_none() { let row = sqlx::query!( r#"SELECT + -- Own row first: with both an own and a legacy row at the destination, + -- a bare LIMIT 1 would name an arbitrary one and the two need different + -- remedies (discard your own vs. ask an admin). (SELECT typ::text FROM draft WHERE workspace_id = $1 AND path = $3 AND typ::text = ANY($6::text[]) AND (email = $4 OR email IS NULL) - LIMIT 1) as "at_target", + ORDER BY email NULLS LAST LIMIT 1) as "at_target", + EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3 + AND typ::text = ANY($6::text[]) AND email = $4) as "at_target_own!", EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5 AND typ = $2 AND email = $4 AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0 @@ -811,7 +816,17 @@ async fn move_draft( // Naming the kind matters for the app pair: a classic-app draft refusing a // raw-app move is invisible in the raw-app list the caller is looking at. let occupant = occupant.replace('_', " "); - format!("You already have a draft at '{new_path}' ({occupant})") + if row.at_target_own { + format!("You already have a draft at '{new_path}' ({occupant})") + } else { + // An ownerless row the caller cannot clear themselves, so send them to + // the one place it can be resolved rather than to "discard your draft". + format!( + "A legacy workspace draft with no owner is already at '{new_path}' \ + ({occupant}). A workspace admin can claim or discard it on the Review & \ + deploy drafts page." + ) + } } else { format!("You have no draft at '{path}'") })); diff --git a/frontend/src/lib/components/DiffDrawer.svelte b/frontend/src/lib/components/DiffDrawer.svelte index bda1d82a89..963edba36f 100644 --- a/frontend/src/lib/components/DiffDrawer.svelte +++ b/frontend/src/lib/components/DiffDrawer.svelte @@ -78,6 +78,26 @@ diffViewer?.closeDrawer() } + /** 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 — 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. */ + let openingToken = 0 + + export function beginOpening(): number { + return ++openingToken + } + + export function ownsOpening(token: number): boolean { + return token === openingToken + } + + /** Drop the opening in flight: the editor that started it is going away. */ + export function abandonOpening() { + openingToken++ + } + function prepareDiff(data: Value) { const metadata = structuredClone(cleanValueProperties(replaceFalseWithUndefined(data))) const content = metadata['content'] diff --git a/frontend/src/lib/components/FlowBuilder.svelte b/frontend/src/lib/components/FlowBuilder.svelte index fdef732939..4ddfc53847 100644 --- a/frontend/src/lib/components/FlowBuilder.svelte +++ b/frontend/src/lib/components/FlowBuilder.svelte @@ -41,7 +41,7 @@ import DeployOverrideConfirmationModal from '$lib/components/common/confirmationModal/DeployOverrideConfirmationModal.svelte' import AIChangesWarningModal from '$lib/components/copilot/chat/flow/AIChangesWarningModal.svelte' - import { getContext, setContext, untrack } from 'svelte' + import { getContext, onDestroy, setContext, untrack } from 'svelte' import { writable } from 'svelte/store' import CenteredPage from './CenteredPage.svelte' import { Button } from './common' @@ -484,7 +484,7 @@ }) // A superseded opening must not write these: the current one would then render // and offer Take latest against the older head. - if (opening != null && opening !== diffOpening) return + if (opening != null && !diffDrawer?.ownsOpening(opening)) return deployedValue = replaceFalseWithUndefined({ ...flow, edited_at: undefined, @@ -1166,22 +1166,26 @@ } } - /** Bumped per drawer opening: the fetches below are awaited, so a reopen (or a - * path change) while they run must not have the older one land last. */ - let diffOpening = 0 + // An opening outlives this editor when a path change remounts it mid-fetch; without + // this it would still open the drawer on the item the user left. + onDestroy(() => diffDrawer?.abandonOpening()) export async function openDiffDrawer() { - const opening = ++diffOpening if (!savedFlow) return + // The fetches below are awaited, so a reopen (or a path change, which remounts + // this editor but not the drawer) while they run must not have the older one + // land last. The drawer counts the openings for that reason. + const opening = diffDrawer?.beginOpening() + if (opening == null) return await syncWithDeployed(opening) const currentDraftTriggers = structuredClone(triggersState.getDraftTriggersSnapshot()) // Blanking the drawer belongs to the opening that will fill it. - if (opening !== diffOpening) return - diffDrawer?.openDrawer() + if (!diffDrawer?.ownsOpening(opening)) return + diffDrawer.openDrawer() const currentFlow = flowStore.val const versions = await deployedVersionOptions() - if (opening !== diffOpening) return - diffDrawer?.setDiff({ + if (!diffDrawer?.ownsOpening(opening)) return + diffDrawer.setDiff({ mode: 'normal', deployed: deployedValue ?? savedFlow, deployedLabel, diff --git a/frontend/src/lib/components/ScriptBuilder.svelte b/frontend/src/lib/components/ScriptBuilder.svelte index 89e5079a3e..a915727282 100644 --- a/frontend/src/lib/components/ScriptBuilder.svelte +++ b/frontend/src/lib/components/ScriptBuilder.svelte @@ -87,7 +87,7 @@ import { writable } from 'svelte/store' import { defaultScriptLanguages, processLangs } from '$lib/scripts' import DefaultScripts from './DefaultScripts.svelte' - import { getContext, onMount, setContext, tick, untrack } from 'svelte' + import { getContext, onDestroy, onMount, setContext, tick, untrack } from 'svelte' import EditorHeader from './EditorHeader.svelte' import ScriptSettingsBadges from './ScriptSettingsBadges.svelte' import Badge from './common/badge/Badge.svelte' @@ -629,7 +629,7 @@ // A superseded opening must not write these: the current one would then render // and offer Take latest against the older head. - if (opening != null && opening !== diffOpening) return + if (opening != null && !diffDrawer?.ownsOpening(opening)) return deployedValue = replaceFalseWithUndefined({ ...latestScript, workspace_id: undefined, @@ -873,15 +873,19 @@ } } - /** Bumped per drawer opening: the fetches below are awaited, so a reopen (or a - * path change) while they run must not have the older one land last. */ - let diffOpening = 0 + // An opening outlives this editor when a path change remounts it mid-fetch; without + // this it would still open the drawer on the item the user left. + onDestroy(() => diffDrawer?.abandonOpening()) export async function openDiffDrawer() { - const opening = ++diffOpening if (!savedScript) { return } + // The fetches below are awaited, so a reopen (or a path change, which remounts + // this editor but not the drawer) while they run must not have the older one + // land last. The drawer counts the openings for that reason. + const opening = diffDrawer?.beginOpening() + if (opening == null) return await syncWithDeployed(opening) const currentDraftTriggers = structuredClone(triggersState.getDraftTriggersSnapshot()) @@ -900,12 +904,12 @@ if (current.assets && !current.assets.length) delete current.assets // Blanking the drawer belongs to the opening that will fill it. - if (opening !== diffOpening) return - diffDrawer?.openDrawer() + if (!diffDrawer?.ownsOpening(opening)) return + diffDrawer.openDrawer() const headHash = (deployed as { hash?: string } | undefined)?.hash const versions = await deployedVersionOptions(headHash) - if (opening !== diffOpening) return - diffDrawer?.setDiff({ + if (!diffDrawer?.ownsOpening(opening)) return + diffDrawer.setDiff({ mode: 'normal', deployed, deployedLabel: deployedVersionLabel(deployed), diff --git a/frontend/src/lib/components/diff_drawer.ts b/frontend/src/lib/components/diff_drawer.ts index 511728f508..c7af1d4930 100644 --- a/frontend/src/lib/components/diff_drawer.ts +++ b/frontend/src/lib/components/diff_drawer.ts @@ -56,4 +56,11 @@ export interface DiffDrawerI { openDrawer: () => void closeDrawer: () => void setDiff: (diff: DiffDrawerDiff) => void + /** Claim the drawer for one opening. Filling it takes awaited fetches, and a path + * change remounts the editor while this drawer stays, so the token lives here: an + * editor checks `ownsOpening` before every write and drops the opening it started + * when it goes away. */ + beginOpening: () => number + ownsOpening: (token: number) => boolean + abandonOpening: () => void } diff --git a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte index 49d475da9d..43fa1abfe0 100644 --- a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte @@ -28,7 +28,7 @@ Undo, WandSparkles } from 'lucide-svelte' - import { untrack } from 'svelte' + import { onDestroy, untrack } from 'svelte' import { orderedJsonStringify, type Value, replaceFalseWithUndefined } from '../../utils' import { random_adj } from '$lib/components/random_positive_adjetive' @@ -406,7 +406,7 @@ // A superseded opening must not write these: the current one would then render // and offer Take latest against the older head. - if (opening != null && opening !== diffOpening) return + if (opening != null && !diffDrawer?.ownsOpening(opening)) return deployedBy = deployedApp.created_by const shownVersions = (deployedApp as { versions?: number[] }).versions deployedVersionShown = Array.isArray(shownVersions) @@ -453,25 +453,29 @@ } } - /** Bumped per drawer opening: the fetches below are awaited, so a reopen (or a - * path change) while they run must not have the older one land last. */ - let diffOpening = 0 + // An opening outlives this editor when a path change remounts it mid-fetch; without + // this it would still open the drawer on the item the user left. + onDestroy(() => diffDrawer?.abandonOpening()) export async function openDiffDrawer() { - const opening = ++diffOpening if (!savedApp) { return } + // The fetches below are awaited, so a reopen (or a path change, which remounts + // this editor but not the drawer) while they run must not have the older one + // land last. The drawer counts the openings for that reason. + const opening = diffDrawer?.beginOpening() + if (opening == null) return // deployedValue should be syncronized when we open Diff await syncWithDeployed(opening) // Blanking the drawer belongs to the opening that will fill it. - if (opening !== diffOpening) return - diffDrawer?.openDrawer() + if (!diffDrawer?.ownsOpening(opening)) return + diffDrawer.openDrawer() const versions = await deployedVersionOptions() - if (opening !== diffOpening) return - diffDrawer?.setDiff({ + if (!diffDrawer?.ownsOpening(opening)) return + diffDrawer.setDiff({ mode: 'normal', deployed: deployedValue ?? stripRawAppDiffNoise(savedApp), versions, @@ -692,12 +696,16 @@ if (!savedApp || newApp) { return } + // The other entry point into the same drawer, so it takes an opening too. + const opening = diffDrawer?.beginOpening() + if (opening == null) return // deployedValue should be syncronized when we open Diff - await syncWithDeployed() + await syncWithDeployed(opening) + if (!diffDrawer?.ownsOpening(opening)) return saveDrawerOpen = false - diffDrawer?.openDrawer() - diffDrawer?.setDiff({ + diffDrawer.openDrawer() + diffDrawer.setDiff({ mode: 'normal', deployed: deployedValue ?? stripRawAppDiffNoise(savedApp), current: currentDiffValue, diff --git a/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte index 31f82c0d48..98e55cfd10 100644 --- a/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte @@ -384,15 +384,19 @@ OtherUserDraftLoad.clear($workspaceStore!, 'script', draftPath) } if (pendingLoad) { + const theirs = (pendingLoad.value as { parent_hash?: string })?.parent_hash const loadedValue = { ...deployedScript, ...(pendingLoad.value as object), - parent_hash: (pendingLoad.value as { parent_hash?: string })?.parent_hash ?? parentHash + parent_hash: theirs } as EditableScript // Their draft's base, not ours: the prompt and the deploy guard read these, // and deploying their content on our base would claim a version it never - // forked from. - draftBaseHash = loadedValue.parent_hash + // forked from. A draft that has none keeps none — seeding `parentHash` + // (the head, when we have no draft here) would mark their older content as + // forked from the current deploy and silence the stale prompt. + if (theirs == null) delete (loadedValue as { parent_hash?: string }).parent_hash + draftBaseHash = theirs if (hasOwnDraft) { OtherUserDraftLoad.beginOverlay({ workspace: $workspaceStore!,