mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-25 16:02:11 +00:00
fix: page the presence probe, and keep a failed run retryable
probeImportedPaths called each list endpoint once. They paginate at 30 rows by default, so it answered correctly for a small project and silently under-reported a large one — every item past the first page went back through a create call that rejects an existing path. It now pages at 100 until a short page, with a 100-page stop so an endpoint that never returns one cannot loop. And a run that finished with failures offered only Finish. `done` is what the step reads as terminal, not `error`, so a failed migration left no way to run the SQL again. Retry now sits beside Finish whenever the run reports an error — beside rather than instead, so a migration that fails every time cannot trap the user short of step 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xg8vXUuHCH3aRkf91sfxjx
This commit is contained in:
@@ -414,6 +414,22 @@
|
||||
{/if}
|
||||
|
||||
{#if execution?.done}
|
||||
<!-- A finished run that reports failures is still finished — what landed is
|
||||
real — but it must stay actionable: without this the only way out of a
|
||||
failed migration or a failed item is to leave, and nothing downstream
|
||||
can run the SQL. Offered beside Finish rather than instead of it, so a
|
||||
migration that fails every time cannot trap the user short of step 4. -->
|
||||
{#if execution.error}
|
||||
<Button
|
||||
variant="subtle"
|
||||
unifiedSize="sm"
|
||||
disabled={execution.running}
|
||||
startIcon={{ icon: execution.running ? Loader2 : Download }}
|
||||
onClick={start}
|
||||
>
|
||||
Retry
|
||||
</Button>
|
||||
{/if}
|
||||
<!-- Disabled while the page is still deciding whether a setup step follows:
|
||||
finishing in that window leaves for the workspace and skips a step that
|
||||
the answer, a moment later, says was needed. -->
|
||||
|
||||
@@ -68,6 +68,14 @@ export async function probeWorkspace(
|
||||
}
|
||||
}
|
||||
|
||||
const PROBE_PAGE_SIZE = 100
|
||||
/**
|
||||
* A stop, not a limit on what may be imported: 100 pages is 10,000 items in one folder, far
|
||||
* past any project, and a paginating endpoint that never returns a short page would otherwise
|
||||
* loop forever. Hitting it under-reports, which only ever means "still to do".
|
||||
*/
|
||||
const MAX_PROBE_PAGES = 100
|
||||
|
||||
/**
|
||||
* Which of the items the import would write are already there, as `presenceKey` keys.
|
||||
*
|
||||
@@ -87,16 +95,34 @@ export async function probeImportedPaths(
|
||||
): Promise<Set<string>> {
|
||||
const pathStart = `f/${folder}/`
|
||||
const found = new Set<string>()
|
||||
const collect = (kind: ImportedKind) => (rows: unknown) => {
|
||||
for (const r of (rows as { path?: string }[] | undefined) ?? []) {
|
||||
if (r.path) found.add(presenceKey(kind, r.path))
|
||||
/**
|
||||
* Every page, not the first one. These endpoints paginate and default to 30 rows, so a
|
||||
* single call answers for a small project and quietly under-reports a large one — leaving
|
||||
* everything past the first page to be created again, and rejected as already existing.
|
||||
*/
|
||||
const collectAll = async (
|
||||
kind: ImportedKind,
|
||||
list: (page: number) => Promise<unknown>
|
||||
): Promise<void> => {
|
||||
for (let page = 1; page <= MAX_PROBE_PAGES; page++) {
|
||||
const rows = ((await list(page)) as { path?: string }[] | undefined) ?? []
|
||||
for (const r of rows) if (r.path) found.add(presenceKey(kind, r.path))
|
||||
if (rows.length < PROBE_PAGE_SIZE) return
|
||||
}
|
||||
}
|
||||
const calls: Promise<unknown>[] = [
|
||||
ScriptService.listScripts({ workspace, pathStart }).then(collect('script')),
|
||||
FlowService.listFlows({ workspace, pathStart }).then(collect('flow')),
|
||||
AppService.listApps({ workspace, pathStart }).then(collect('app')),
|
||||
ResourceService.listResource({ workspace, pathStart }).then(collect('resource'))
|
||||
collectAll('script', (page) =>
|
||||
ScriptService.listScripts({ workspace, pathStart, page, perPage: PROBE_PAGE_SIZE })
|
||||
),
|
||||
collectAll('flow', (page) =>
|
||||
FlowService.listFlows({ workspace, pathStart, page, perPage: PROBE_PAGE_SIZE })
|
||||
),
|
||||
collectAll('app', (page) =>
|
||||
AppService.listApps({ workspace, pathStart, page, perPage: PROBE_PAGE_SIZE })
|
||||
),
|
||||
collectAll('resource', (page) =>
|
||||
ResourceService.listResource({ workspace, pathStart, page, perPage: PROBE_PAGE_SIZE })
|
||||
)
|
||||
]
|
||||
if (opts?.triggers) {
|
||||
// `failedKinds` is deliberately ignored: a kind that could not be listed leaves its
|
||||
|
||||
@@ -0,0 +1,53 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
/**
|
||||
* The list endpoints paginate. A probe that reads one page answers correctly for a small
|
||||
* project and under-reports a large one, which puts every item past the first page back
|
||||
* through a create call that rejects it.
|
||||
*/
|
||||
|
||||
const calls = vi.hoisted(() => ({ script: [] as { page?: number; perPage?: number }[] }))
|
||||
|
||||
/** 150 scripts in the folder — more than one page at any page size the probe might pick. */
|
||||
const ALL = Array.from({ length: 150 }, (_, i) => ({ path: `f/calendly/s${i}` }))
|
||||
|
||||
vi.mock('$lib/gen', () => ({
|
||||
ScriptService: {
|
||||
listScripts: vi.fn(async (args: any) => {
|
||||
calls.script.push({ page: args.page, perPage: args.perPage })
|
||||
const per = args.perPage ?? 30
|
||||
return ALL.slice((args.page - 1) * per, args.page * per)
|
||||
})
|
||||
},
|
||||
FlowService: { listFlows: vi.fn(async () => []) },
|
||||
AppService: { listApps: vi.fn(async () => []) },
|
||||
ResourceService: { listResource: vi.fn(async () => []) },
|
||||
WorkspaceService: { listWorkspaces: vi.fn(async () => []), getDatatableFullSchema: vi.fn() }
|
||||
}))
|
||||
vi.mock('$lib/components/triggers/workspaceTriggersList', () => ({
|
||||
listAllWorkspaceTriggers: vi.fn(async () => ({ triggers: [], failedKinds: [] }))
|
||||
}))
|
||||
|
||||
import { probeImportedPaths } from './probe'
|
||||
import { presenceKey } from '$lib/components/workspaceSettings/projectInstall'
|
||||
|
||||
describe('probeImportedPaths paging', () => {
|
||||
beforeEach(() => {
|
||||
calls.script = []
|
||||
})
|
||||
|
||||
it('reads every page, not just the first', async () => {
|
||||
const found = await probeImportedPaths('w', 'calendly')
|
||||
expect(found.size).toBe(150)
|
||||
expect(found.has(presenceKey('script', 'f/calendly/s0'))).toBe(true)
|
||||
// The one that a single-page probe misses, and would then try to create again.
|
||||
expect(found.has(presenceKey('script', 'f/calendly/s149'))).toBe(true)
|
||||
})
|
||||
|
||||
it('stops on the first short page rather than asking forever', async () => {
|
||||
await probeImportedPaths('w', 'calendly')
|
||||
const pages = calls.script.map((c) => c.page)
|
||||
expect(pages).toEqual([1, 2])
|
||||
expect(new Set(calls.script.map((c) => c.perPage))).toEqual(new Set([100]))
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user