From c91afd67aae8e8b49a46ccbc917bc246d44c0c63 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Wed, 7 Oct 2026 07:35:05 -0700 Subject: [PATCH] fix: guard session moves against concurrent turns and unsaved edits Co-Authored-By: Claude Opus 5.5 (1M context) --- backend/windmill-common/src/user_drafts.rs | 4 +- .../copilot/chat/AIChatManager.svelte.ts | 15 +++++- .../sessions/SessionMoveModal.svelte | 18 +++++-- .../components/sessions/SessionWrapper.svelte | 14 +++++- .../sessions/WorkspaceFamilyPicker.svelte | 10 +++- .../components/sessions/moveSessionToFork.ts | 48 ++++++++++++------- 6 files changed, 81 insertions(+), 28 deletions(-) diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index eb836058e7..cb17fb9257 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -645,9 +645,9 @@ pub async fn delete_own_draft_for_path( Ok(()) } -/// Copy drafts from `source` into its fork `target`, returning the `(path, typ)` of each row +/// Copy drafts from `source` into its fork `target`, returning the `(kind, path)` of each row /// copied. With `items = None` this is the fork-time clone: every row of `email` plus the legacy -/// NULL-email row. With `items`, only `email`'s own rows at those `(path, kind)` pairs. +/// NULL-email row. With `items`, only `email`'s own rows at those `(kind, path)` pairs. /// /// A plain INSERT: the caller guarantees the target holds none of the copied rows (an empty /// fork, or a conflict check under the same transaction), else `draft_pkey_with_user` aborts it. diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 9e2379fe33..dcb5879169 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -1305,8 +1305,21 @@ export class AIChatManager implements ChatViewHost { }, 5_000) } + #autoResumeHolds = 0 + /** Keep background-job completions from starting a turn until the returned release runs. */ + holdAutoResume(): () => void { + this.#autoResumeHolds++ + let released = false + return () => { + if (released) return + released = true + this.#autoResumeHolds-- + void this.#maybeAutoResumeFromJobs() + } + } + async #maybeAutoResumeFromJobs() { - if (this.#autoResuming) return + if (this.#autoResuming || this.#autoResumeHolds > 0) return // Global/sessions chat only (the only mode with a jobs tray + preamble). if (this.mode !== AIMode.GLOBAL) return // Mid-turn: the notes will ride that turn's preamble, so don't start another. diff --git a/frontend/src/lib/components/sessions/SessionMoveModal.svelte b/frontend/src/lib/components/sessions/SessionMoveModal.svelte index dbbd31ef13..785cafb9dc 100644 --- a/frontend/src/lib/components/sessions/SessionMoveModal.svelte +++ b/frontend/src/lib/components/sessions/SessionMoveModal.svelte @@ -9,6 +9,7 @@ open, parentName, targetName, + newFork = false, items, conflicts = [], error, @@ -20,6 +21,8 @@ open: boolean parentName: string targetName: string + // A new fork is cloned with every draft of the user, not only this session's. + newFork?: boolean items: Item[] conflicts?: DraftItemRef[] error?: string @@ -45,8 +48,13 @@ >

- The session will act on {targetName}. These - drafts are copied there: + The session will act on {targetName}. + {#if newFork} + The new fork starts with a copy of all your drafts in {parentName}, including these from + this session: + {:else} + These drafts are copied there: + {/if}

    {#each items as item (`${item.kind}:${item.path}`)} @@ -61,13 +69,13 @@ {/each}
- -
+ +
+
{#if conflicts.length > 0}

diff --git a/frontend/src/lib/components/sessions/SessionWrapper.svelte b/frontend/src/lib/components/sessions/SessionWrapper.svelte index 236a227811..005ddbb708 100644 --- a/frontend/src/lib/components/sessions/SessionWrapper.svelte +++ b/frontend/src/lib/components/sessions/SessionWrapper.svelte @@ -12,7 +12,7 @@ import { DraftService, WorkspaceService, type DraftItemRef } from '$lib/gen' import WorkspaceFamilyPicker from './WorkspaceFamilyPicker.svelte' import SessionMoveModal from './SessionMoveModal.svelte' - import { moveSessionToFork, type MoveTarget } from './moveSessionToFork' + import { flushPendingSaves, moveSessionToFork, type MoveTarget } from './moveSessionToFork' import { maskHasDraftRow } from './modifiedItemsMask' import { sendUserToast } from '$lib/toast' import Toggle from '$lib/components/Toggle.svelte' @@ -375,6 +375,12 @@ const mask = sessionMask() let items: MoveRequest['items'] let conflicts: DraftItemRef[] = [] + // The list comes from the server, so an edit still debounced would be left out of it. + const unsaved = await flushPendingSaves(parent) + if (unsaved) { + sendUserToast(unsaved, true) + return + } try { // Only the user's own rows move; a legacy workspace-level draft is nobody's. items = (await DraftService.listDrafts({ workspace: parent })) @@ -414,10 +420,12 @@ const req = moveRequest if (!session || !req || moving) return moving = true + const releaseAutoResume = runtime?.manager.holdAutoResume() try { const res = await moveSessionToFork(session.id, req.parent, req.target, { items: req.items.map(({ kind, path }) => ({ kind, path })), - removeFromParent: moveRemoveFromParent + removeFromParent: moveRemoveFromParent, + isBusy: () => !canMoveToFork }) if (!res.ok) { if (!req.open) { @@ -443,6 +451,7 @@ sendUserToast(`Session moved to ${req.targetName}`) } finally { moving = false + releaseAutoResume?.() } } @@ -709,6 +718,7 @@ open={moveRequest.open} parentName={moveRequest.parentName} targetName={moveRequest.targetName} + newFork={moveRequest.target.kind === 'new'} items={moveRequest.items} conflicts={moveRequest.conflicts} error={moveRequest.error} diff --git a/frontend/src/lib/components/sessions/WorkspaceFamilyPicker.svelte b/frontend/src/lib/components/sessions/WorkspaceFamilyPicker.svelte index 599a01c314..48284b6de2 100644 --- a/frontend/src/lib/components/sessions/WorkspaceFamilyPicker.svelte +++ b/frontend/src/lib/components/sessions/WorkspaceFamilyPicker.svelte @@ -192,10 +192,16 @@ // is premium-only (backend caps it per paid seat). DisableWorkspaceForking on the active workspace // (a locked prod) doesn't apply when there's a dev to fork from instead — the dev isn't locked, and // devOfRoot only resolves when the user is a member of it. + // In `forksOf` mode the fork is always based on that workspace, so only its own rule counts. const forksGateOpen = $derived( (!isCloudHosted() || $maybePremium) && - $workspaceStore !== 'admins' && - (canCreateFork($userStore) || !!devOfRoot) + (forksOf + ? forksOf !== 'admins' && + !rootRulesetsResource.loading && + !rootUserInfoResource.loading && + (!isRuleActiveInRulesets(rootRulesets, 'DisableWorkspaceForking') || + canUserBypassRuleKindInRulesets(rootRulesets, 'DisableWorkspaceForking', rootUserInfo)) + : $workspaceStore !== 'admins' && (canCreateFork($userStore) || !!devOfRoot)) ) // A fork is a new workspace, so it's subject to the community-edition cap on // the number of non-'admins' workspaces (backend _check_nb_of_workspaces, diff --git a/frontend/src/lib/components/sessions/moveSessionToFork.ts b/frontend/src/lib/components/sessions/moveSessionToFork.ts index ad25b38166..03b6f2458c 100644 --- a/frontend/src/lib/components/sessions/moveSessionToFork.ts +++ b/frontend/src/lib/components/sessions/moveSessionToFork.ts @@ -15,6 +15,26 @@ export type MoveSessionResult = | { ok: true; forkId: string; copied: number; removed: number } | { ok: false; error: string; conflicts?: DraftItemRef[] } +/** + * Send every debounced draft save for `workspace` to the server. Returns an error message + * when one of them failed or conflicted. + * + * A save still debounced in an open editor would otherwise land in the parent after the + * move: re-creating a draft just removed there, or missing from the copy. Three debounces + * stand between a keystroke and the server, and each hands on to the next through an + * effect, hence the ticks. + */ +export async function flushPendingSaves(workspace: string): Promise { + flushPendingKeystrokes() + await tick() + flushEditorSaves(workspace) + await tick() + const unsaved = await UserDraftDbSyncer.flushWorkspace(workspace) + if (unsaved.length > 0) { + return `Could not save ${unsaved.map((q) => q.path).join(', ')} before moving. Resolve it and retry.` + } +} + /** * Move a session into a fork of its workspace, bringing its drafts along. The session's * workspace changes only once its drafts are where they must be, so a failure at any step @@ -28,23 +48,18 @@ export async function moveSessionToFork( sessionId: string, parent: string, target: MoveTarget, - opts: { items: DraftItemRef[]; removeFromParent: boolean } -): Promise { - // A save still debounced in an open editor would otherwise land in the parent after the - // move: re-creating a draft just removed there, or missing from the copy. Three debounces - // stand between a keystroke and the server, and each hands on to the next through an - // effect, hence the ticks. - flushPendingKeystrokes() - await tick() - flushEditorSaves(parent) - await tick() - const unsaved = await UserDraftDbSyncer.flushWorkspace(parent) - if (unsaved.length > 0) { - return { - ok: false, - error: `Could not save ${unsaved.map((q) => q.path).join(', ')} before moving. Resolve it and retry.` - } + opts: { + items: DraftItemRef[] + removeFromParent: boolean + // A turn running in the parent keeps writing there while its drafts move away. + isBusy?: () => boolean } +): Promise { + const busy = { ok: false as const, error: 'A chat turn is running. Retry once it ends.' } + const unsaved = await flushPendingSaves(parent) + if (unsaved) return { ok: false, error: unsaved } + + if (opts.isBusy?.()) return busy let forkId: string let copied = 0 @@ -53,6 +68,7 @@ export async function moveSessionToFork( if (target.kind === 'new') { const created = await materializeFork(target.fork) if (!created) return { ok: false, error: 'Could not create the fork' } + if (opts.isBusy?.()) return busy forkId = created copied = opts.items.length if (opts.removeFromParent && opts.items.length > 0) {