From fd032a16c58eeb5a30867eeb4dc240a24b1f4f6e Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 29 Sep 2026 12:23:07 +0200 Subject: [PATCH] fix: settle fork polls by the fork's existence, adopt in-flight creations Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/fork_background.rs | 7 ++ .../windmill-api-workspaces/src/workspaces.rs | 2 +- cli/src/commands/workspace/fork.ts | 50 ++++++++------ frontend/src/lib/utils/forkCreation.test.ts | 66 +++++++++++++++++++ frontend/src/lib/utils/forkCreation.ts | 39 +++++++---- 5 files changed, 131 insertions(+), 33 deletions(-) create mode 100644 frontend/src/lib/utils/forkCreation.test.ts diff --git a/backend/windmill-api-integration-tests/tests/fork_background.rs b/backend/windmill-api-integration-tests/tests/fork_background.rs index ae8a73bfde..5a62cca68c 100644 --- a/backend/windmill-api-integration-tests/tests/fork_background.rs +++ b/backend/windmill-api-integration-tests/tests/fork_background.rs @@ -45,6 +45,13 @@ async fn test_fork_created_in_background(db: Pool) -> anyhow::Result<( assert_eq!(resp.status(), 200, "{}", resp.text().await?); let status = wait_for_fork(&client, &base_url, "wm-fork-bg-ok").await; assert_eq!(status, json!({ "status": "completed" })); + // A member who neither started the fork nor administers the workspace cannot read its outcome. + let resp = client + .get(format!("{base_url}/fork_creation_status/wm-fork-bg-ok")) + .header("Authorization", "Bearer SECRET_TOKEN_2") + .send() + .await?; + assert_eq!(resp.status(), 404); let exists: bool = sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM workspace WHERE id = 'wm-fork-bg-ok')") .fetch_one(&db) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index b0dc1534ba..70b7293dcf 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -9050,7 +9050,7 @@ async fn create_workspace_fork( let response_fork_id = fork_id.clone(); sqlx::query( "DELETE FROM workspace_fork_creation - WHERE finished_at < now() - interval '7 days'", + WHERE COALESCE(finished_at, heartbeat_at) < now() - interval '7 days'", ) .execute(&db) .await?; diff --git a/cli/src/commands/workspace/fork.ts b/cli/src/commands/workspace/fork.ts index 1a6486ad5d..b9db16aa19 100644 --- a/cli/src/commands/workspace/fork.ts +++ b/cli/src/commands/workspace/fork.ts @@ -23,14 +23,17 @@ import { } from "../../core/conf.ts"; /** - * Wait for a fork requested with `background: true`. A server without background forks ignores - * the flag and has already created the fork when the request returns; it has no status route. + * Wait for a fork requested with `background: true`. A poll that fails says nothing about the + * fork: it can be lost to a proxy, or reach a server without background forks (an older version, + * possibly one replica during a rolling deploy), which ignored the flag and created the fork + * before answering. */ async function waitForForkCreation(parentWorkspace: string, forkId: string) { let failedPolls = 0; while (true) { - await new Promise((resolve) => setTimeout(resolve, 1500)); - let result: Awaited>; + let result: + | Awaited> + | undefined; try { result = await wmill.getForkCreationStatus({ workspace: parentWorkspace, @@ -38,15 +41,17 @@ async function waitForForkCreation(parentWorkspace: string, forkId: string) { }); failedPolls = 0; } catch (e) { - if ((e as { status?: number }).status === 404) return; - // A poll lost to a proxy says nothing about the fork, which is still being created. - if (++failedPolls >= 5) throw e; - continue; + const exists = await wmill + .existsWorkspace({ requestBody: { id: forkId } }) + .catch(() => false); + if (exists) return; + if (++failedPolls >= 10) throw e; } - if (result.status === "completed") return; - if (result.status === "failed") { + if (result?.status === "completed") return; + if (result?.status === "failed") { throw new Error(result.error ?? "Unknown error"); } + await new Promise((resolve) => setTimeout(resolve, 1500)); } } @@ -404,16 +409,21 @@ async function createWorkspaceFork( // --- Create the fork workspace --- try { - await wmill.createWorkspaceFork({ - workspace: workspace.workspaceId, - background: true, - requestBody: { - id: trueWorkspaceId, - name: opts.createWorkspaceName ?? workspaceName ?? trueWorkspaceId, - color: forkColor, - forked_datatables: forkedDatatables, - }, - }); + await wmill + .createWorkspaceFork({ + workspace: workspace.workspaceId, + background: true, + requestBody: { + id: trueWorkspaceId, + name: opts.createWorkspaceName ?? workspaceName ?? trueWorkspaceId, + color: forkColor, + forked_datatables: forkedDatatables, + }, + }) + .catch((e) => { + // Started by an earlier run: wait for that creation instead. + if (!String((e as { body?: unknown })?.body ?? "").includes("is already being created")) throw e; + }); await waitForForkCreation(workspace.workspaceId, trueWorkspaceId); log.info(colors.green(`✅ Created forked workspace ${trueWorkspaceId}`)); diff --git a/frontend/src/lib/utils/forkCreation.test.ts b/frontend/src/lib/utils/forkCreation.test.ts new file mode 100644 index 0000000000..a953b37600 --- /dev/null +++ b/frontend/src/lib/utils/forkCreation.test.ts @@ -0,0 +1,66 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('$lib/gen', () => ({ + WorkspaceService: { + createWorkspaceFork: vi.fn(), + getForkCreationStatus: vi.fn(), + existsWorkspace: vi.fn() + } +})) + +import { WorkspaceService } from '$lib/gen' +import { createWorkspaceForkAndWait } from './forkCreation' + +const svc = vi.mocked(WorkspaceService) +const fork = { id: 'wm-fork-x', name: 'x' } + +async function run() { + const p = createWorkspaceForkAndWait('ws', fork) + const settled = p.then( + () => 'completed', + (e) => `failed: ${e.message}` + ) + await vi.runAllTimersAsync() + return settled +} + +describe('createWorkspaceForkAndWait', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.resetAllMocks() + svc.createWorkspaceFork.mockResolvedValue('started') + svc.existsWorkspace.mockResolvedValue(false) + }) + afterEach(() => vi.useRealTimers()) + + it('waits while the fork is running and reports how it ended', async () => { + svc.getForkCreationStatus + .mockResolvedValueOnce({ status: 'running' }) + .mockResolvedValueOnce({ status: 'failed', error: 'boom' }) + expect(await run()).toBe('failed: boom') + expect(svc.getForkCreationStatus).toHaveBeenCalledTimes(2) + }) + + it('takes a failed poll for success once the fork exists', async () => { + // A server without background forks created it synchronously and has no status route. + svc.getForkCreationStatus.mockRejectedValue( + Object.assign(new Error('Not Found'), { status: 404 }) + ) + svc.existsWorkspace.mockResolvedValue(true) + expect(await run()).toBe('completed') + }) + + it('gives up after repeated failed polls for a fork that never appears', async () => { + svc.getForkCreationStatus.mockRejectedValue(new Error('upstream request timeout')) + expect(await run()).toBe('failed: upstream request timeout') + expect(svc.getForkCreationStatus).toHaveBeenCalledTimes(10) + }) + + it('waits for a creation an earlier request already started', async () => { + svc.createWorkspaceFork.mockRejectedValue({ + body: "Bad request: workspace 'wm-fork-x' is already being created" + }) + svc.getForkCreationStatus.mockResolvedValue({ status: 'completed' }) + expect(await run()).toBe('completed') + }) +}) diff --git a/frontend/src/lib/utils/forkCreation.ts b/frontend/src/lib/utils/forkCreation.ts index bc072afdb9..d5b5e6e5b5 100644 --- a/frontend/src/lib/utils/forkCreation.ts +++ b/frontend/src/lib/utils/forkCreation.ts @@ -1,7 +1,7 @@ import { WorkspaceService, type CreateWorkspaceFork } from '$lib/gen' const POLL_INTERVAL_MS = 1500 -const MAX_FAILED_POLLS = 5 +const MAX_FAILED_POLLS = 10 /** * Create a fork and resolve once it exists, rejecting with the reason it could not be created. @@ -13,16 +13,22 @@ export async function createWorkspaceForkAndWait( parentWorkspace: string, fork: CreateWorkspaceFork ): Promise { - await WorkspaceService.createWorkspaceFork({ - workspace: parentWorkspace, - background: true, - requestBody: fork - }) - // A poll lost to the same proxy says nothing about the fork, which is still being created. + try { + await WorkspaceService.createWorkspaceFork({ + workspace: parentWorkspace, + background: true, + requestBody: fork + }) + } catch (e) { + // A retry of a request whose response was lost: wait for the creation it started. + if (!String(e?.body ?? '').includes('is already being created')) throw e + } + // A poll that fails says nothing about the fork: it can be lost to the same proxy, or reach a + // server without background forks (one of an older version during a rolling deploy), which + // ignored the flag and created the fork before answering. let failedPolls = 0 while (true) { - await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)) - let result: Awaited> + let result: Awaited> | undefined try { result = await WorkspaceService.getForkCreationStatus({ workspace: parentWorkspace, @@ -30,10 +36,19 @@ export async function createWorkspaceForkAndWait( }) failedPolls = 0 } catch (e) { + if (await forkExists(fork.id)) return if (++failedPolls >= MAX_FAILED_POLLS) throw e - continue } - if (result.status === 'completed') return - if (result.status === 'failed') throw new Error(result.error ?? 'Unknown error') + if (result?.status === 'completed') return + if (result?.status === 'failed') throw new Error(result.error ?? 'Unknown error') + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)) + } +} + +async function forkExists(id: string): Promise { + try { + return await WorkspaceService.existsWorkspace({ requestBody: { id } }) + } catch { + return false } }