mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-22 00:03:35 +00:00
fix: relock scripts and flow steps whose code changed on deploy (#11794)
* fix: relock scripts deployed from a draft that carries the deployed lock Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * chore: reword fetchDeployed lock comment Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix: drop the lock of flow steps whose code the chat changed Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix: drop stale flow step locks at deploy, from drafts and the flow editor Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix: keep locks of copied flows and never block a deploy on the lock check Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix: keep the lock of a renamed or duplicated flow step with unchanged code Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
f9513e82ee
commit
1f1df1254e
@@ -49,6 +49,7 @@
|
||||
import WorkspaceScriptSettingsDrawer from './flows/content/WorkspaceScriptSettingsDrawer.svelte'
|
||||
import FlowEditorDrawer from './flows/content/FlowEditorDrawer.svelte'
|
||||
import { dfs as dfsApply } from './flows/dfs'
|
||||
import { dropStaleStepLocks } from './flows/staleStepLocks'
|
||||
import {
|
||||
claimLinkedToolsFetch,
|
||||
invalidateLinkedToolsFetches,
|
||||
@@ -627,6 +628,16 @@
|
||||
}
|
||||
}
|
||||
|
||||
// The editor clears a step's lock when its code is edited, but a draft written elsewhere
|
||||
// can hold a step whose code changed and whose lock did not. A failed lookup skips the
|
||||
// check rather than blocking the deploy.
|
||||
const liveFlowValue = isNewFlow
|
||||
? undefined
|
||||
: await FlowService.getFlowByPath({ workspace: opWorkspace!, path: initialPath })
|
||||
.then((f) => f.value)
|
||||
.catch(() => undefined)
|
||||
dropStaleStepLocks(flow.value, liveFlowValue)
|
||||
|
||||
if (isNewFlow) {
|
||||
await FlowService.createFlow({
|
||||
workspace: opWorkspace!,
|
||||
|
||||
@@ -4773,6 +4773,39 @@ describe('global AI tools', () => {
|
||||
).resolves.toBe(code)
|
||||
})
|
||||
|
||||
// The worker reuses a non-empty step lock, so an edited step keeping its lock deploys
|
||||
// against the old dependencies.
|
||||
it('set_flow_module_code drops the lock of the edited step only', async () => {
|
||||
const step = (id: string) => ({
|
||||
id,
|
||||
value: {
|
||||
type: 'rawscript',
|
||||
language: 'python3',
|
||||
content: 'import requests',
|
||||
lock: 'requests==2.0',
|
||||
input_transforms: {}
|
||||
}
|
||||
})
|
||||
seedBackendDraft('flow', 'f/flows/locked', {
|
||||
path: 'f/flows/locked',
|
||||
summary: 'f',
|
||||
description: '',
|
||||
value: { modules: [step('edited'), step('untouched')] },
|
||||
schema: {}
|
||||
})
|
||||
|
||||
await callGlobalTool('set_flow_module_code', {
|
||||
path: 'f/flows/locked',
|
||||
module_id: 'edited',
|
||||
code: 'import yaml'
|
||||
})
|
||||
|
||||
const [edited, untouched] = getBackendDraft('flow', 'f/flows/locked').value.modules
|
||||
expect(edited.value.content).toBe('import yaml')
|
||||
expect(edited.value).not.toHaveProperty('lock')
|
||||
expect(untouched.value.lock).toBe('requests==2.0')
|
||||
})
|
||||
|
||||
it('warns about every empty rawscript body (top-level, nested, preprocessor, failure) and skips populated ones', async () => {
|
||||
const result = JSON.parse(
|
||||
await callGlobalTool('write_flow', {
|
||||
|
||||
@@ -52,6 +52,7 @@ import { withAgentDrafts } from '$lib/components/flows/linkedAgentDrafts'
|
||||
import { resolveLinkedAgentTools } from '$lib/components/flows/flowState'
|
||||
import { enabledToolNames, type AgentTool } from '$lib/components/flows/agentToolUtils'
|
||||
import { evalValue } from '$lib/components/flows/utils.svelte'
|
||||
import { dropStaleStepLocks } from '$lib/components/flows/staleStepLocks'
|
||||
import { quickjsReady } from '$lib/utils/quickjsEval.svelte'
|
||||
import { updateRawAppPolicy } from '$lib/components/raw_apps/rawAppPolicy'
|
||||
import {
|
||||
@@ -5804,7 +5805,12 @@ type ScriptDraftArgs = {
|
||||
const SCRIPT_SPEC: WriteSpec<NewScript, ScriptDraftArgs> = {
|
||||
probe: (workspace, path) => ScriptService.existsScriptByPath({ workspace, path }),
|
||||
fetchDeployed: async (workspace, path) => {
|
||||
const existing = await ScriptService.getScriptByPath({ workspace, path })
|
||||
// The deployed lock describes the deployed content, not the draft's; the deploy recomputes it.
|
||||
const {
|
||||
lock: _lock,
|
||||
lock_error_logs: _lockErrorLogs,
|
||||
...existing
|
||||
} = await ScriptService.getScriptByPath({ workspace, path })
|
||||
return { ...(existing as unknown as NewScript), parent_hash: existing.hash }
|
||||
},
|
||||
buildDraft: async (base, args, path) => {
|
||||
@@ -5903,6 +5909,8 @@ const FLOW_SPEC: WriteSpec<Flow, FlowDraftArgs> = {
|
||||
}
|
||||
}
|
||||
}
|
||||
// Also checked at deploy; dropped here too because a test run of the draft reads the locks.
|
||||
dropStaleStepLocks(value, base?.value)
|
||||
return base
|
||||
? {
|
||||
...structuredClone(base),
|
||||
|
||||
@@ -0,0 +1,25 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import { dropStaleStepLocks } from './staleStepLocks'
|
||||
|
||||
const step = (id: string, content: string, lock?: string) =>
|
||||
({ id, value: { type: 'rawscript', language: 'python3', content, lock } }) as any
|
||||
|
||||
describe('dropStaleStepLocks', () => {
|
||||
// A rename or duplicate gives the step a new id; its code and lock still match.
|
||||
it('keeps the lock of a renamed or duplicated step whose code is unchanged', () => {
|
||||
const base = { modules: [step('a', 'import requests', 'requests==2.0')] }
|
||||
const value = {
|
||||
modules: [
|
||||
step('a', 'import requests', 'requests==2.0'),
|
||||
step('a_copy', 'import requests', 'requests==2.0'),
|
||||
step('a_edited_copy', 'import yaml', 'requests==2.0')
|
||||
]
|
||||
}
|
||||
dropStaleStepLocks(value as any, base as any)
|
||||
expect(value.modules.map((m) => m.value.lock)).toEqual([
|
||||
'requests==2.0',
|
||||
'requests==2.0',
|
||||
undefined
|
||||
])
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,33 @@
|
||||
import type { FlowModule, FlowValue } from '$lib/gen'
|
||||
import { forEachFlowModule } from './dfs'
|
||||
|
||||
/**
|
||||
* Drops, in place, the lock of every inline step whose code differs from the same step in
|
||||
* `base`. The worker reuses any non-empty step lock, so a step whose code changed but kept its
|
||||
* lock runs against the old dependencies. Unchanged steps keep theirs, so a deploy does not
|
||||
* re-resolve (and possibly upgrade) what nobody touched. A step id `base` lacks (a renamed or
|
||||
* duplicated step) keeps its lock only when some `base` step has the same code. Without a base
|
||||
* nothing is dropped: a copied or imported flow carries locks that match its code.
|
||||
*/
|
||||
export function dropStaleStepLocks(value: FlowValue, base: FlowValue | undefined): void {
|
||||
if (!base) return
|
||||
const allModules = (v: FlowValue) =>
|
||||
[...(v.modules ?? []), v.preprocessor_module, v.failure_module].filter(
|
||||
(m): m is FlowModule => m != null
|
||||
)
|
||||
const contentOf = (m: FlowModule) => (m.value as { content?: unknown }).content
|
||||
const baseContentById = new Map<string, unknown>()
|
||||
const baseContents = new Set<unknown>()
|
||||
forEachFlowModule(allModules(base), (m) => {
|
||||
baseContentById.set(m.id, contentOf(m))
|
||||
baseContents.add(contentOf(m))
|
||||
})
|
||||
forEachFlowModule(allModules(value), (m) => {
|
||||
const step = m.value as { content?: unknown; lock?: unknown }
|
||||
if (step.lock == undefined) return
|
||||
const unchanged = baseContentById.has(m.id)
|
||||
? baseContentById.get(m.id) === step.content
|
||||
: baseContents.has(step.content)
|
||||
if (!unchanged) delete step.lock
|
||||
})
|
||||
}
|
||||
@@ -124,6 +124,64 @@ describe('deployDraft preserves on_behalf_of', () => {
|
||||
})
|
||||
})
|
||||
|
||||
// The backend skips the dependency job when `lock` is set, so a draft that inherited the
|
||||
// deployed lock would deploy new content against the old lockfile.
|
||||
describe('deployDraft: script lock', () => {
|
||||
beforeEach(() => vi.clearAllMocks())
|
||||
|
||||
it('never sends the draft lock, so the backend relocks', async () => {
|
||||
vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({
|
||||
hash: 'v1',
|
||||
draft: {
|
||||
path: 'f/admin/fetch',
|
||||
content: 'import requests',
|
||||
lock: 'certifi==2024.1.1',
|
||||
lock_error_logs: 'old error'
|
||||
}
|
||||
} as any)
|
||||
|
||||
await deployDraft('script', 'f/admin/fetch', 'ws')
|
||||
const body = vi.mocked(ScriptService.createScript).mock.calls[0][0].requestBody
|
||||
expect(body).not.toHaveProperty('lock')
|
||||
expect(body).not.toHaveProperty('lock_error_logs')
|
||||
})
|
||||
|
||||
it('flow: drops the lock of a step whose code differs from the deployed one only', async () => {
|
||||
const step = (id: string, content: string) => ({
|
||||
id,
|
||||
value: { type: 'rawscript', language: 'python3', content, lock: 'requests==2.0' }
|
||||
})
|
||||
vi.mocked(FlowService.getFlowByPath).mockResolvedValueOnce({
|
||||
value: { modules: [step('edited', 'import requests'), step('kept', 'import requests')] },
|
||||
draft: {
|
||||
path: 'f/admin/flow',
|
||||
value: { modules: [step('edited', 'import yaml'), step('kept', 'import requests')] }
|
||||
}
|
||||
} as any)
|
||||
|
||||
await deployDraft('flow', 'f/admin/flow', 'ws')
|
||||
const [edited, kept] = vi.mocked(FlowService.updateFlow).mock.calls[0][0].requestBody.value
|
||||
.modules as any[]
|
||||
expect(edited.value).not.toHaveProperty('lock')
|
||||
expect(kept.value.lock).toBe('requests==2.0')
|
||||
})
|
||||
|
||||
// A never-deployed flow is often a copy or an import, whose locks match its code.
|
||||
it('flow: keeps every lock when nothing is deployed to compare against', async () => {
|
||||
const step = { id: 'copied', value: { type: 'rawscript', content: 'x', lock: 'requests==2.0' } }
|
||||
vi.mocked(FlowService.getFlowByPath).mockResolvedValueOnce({
|
||||
no_deployed: true,
|
||||
value: { modules: [step] },
|
||||
draft: { path: 'f/admin/copy', value: { modules: [step] } }
|
||||
} as any)
|
||||
|
||||
await deployDraft('flow', 'f/admin/copy', 'ws', { draftOnly: true })
|
||||
const [copied] = vi.mocked(FlowService.createFlow).mock.calls[0][0].requestBody.value
|
||||
.modules as any[]
|
||||
expect(copied.value.lock).toBe('requests==2.0')
|
||||
})
|
||||
})
|
||||
|
||||
// The resource branch reads the item again when the deploy lands, and falls back to the deployed
|
||||
// row when the draft has gone. That row keeps its value under `value` and carries no `args` at
|
||||
// all, so reading it as a draft (`value: d.args ?? {}`) would replace a live resource with `{}`.
|
||||
|
||||
@@ -46,6 +46,7 @@ import { setLocalDraftHint } from '$lib/localDraftHints.svelte'
|
||||
import { userStore } from '$lib/stores'
|
||||
import { deployTriggers, type Trigger } from '$lib/components/triggers/utils'
|
||||
import { saveScheduleFromCfg } from '$lib/components/flows/scheduleUtils'
|
||||
import { dropStaleStepLocks } from '$lib/components/flows/staleStepLocks'
|
||||
import { saveHttpRouteFromCfg } from '$lib/components/triggers/http/utils'
|
||||
import { saveWebsocketTriggerFromCfg } from '$lib/components/triggers/websocket/utils'
|
||||
import { savePostgresTriggerFromCfg } from '$lib/components/triggers/postgres/utils'
|
||||
@@ -469,7 +470,15 @@ export async function deployDraft(
|
||||
const r = (await ScriptService.getScriptByPath({ workspace, path, getDraft: true })) as any
|
||||
const d = r.draft ?? r
|
||||
// Drop editor-only / server-managed keys; deploy as a real (non-draft) version.
|
||||
const { draft_triggers: draftTriggers, draft_only: _o, ...rest } = d
|
||||
// A draft seeded from the deployed script carries its `lock`, and the backend skips
|
||||
// the dependency job whenever `lock` is set, so the new content would keep the old lock.
|
||||
const {
|
||||
draft_triggers: draftTriggers,
|
||||
draft_only: _o,
|
||||
lock: _lock,
|
||||
lock_error_logs: _lockErrorLogs,
|
||||
...rest
|
||||
} = d
|
||||
const scriptPath = d.path ?? path
|
||||
// Deploy at the draft's path so a rename in the draft is honored (same as
|
||||
// the editor: createScript at the new path with parent_hash links lineage).
|
||||
@@ -490,6 +499,9 @@ export async function deployDraft(
|
||||
} else if (kind === 'flow') {
|
||||
const r = (await FlowService.getFlowByPath({ workspace, path, getDraft: true })) as any
|
||||
const d = r.draft ?? r
|
||||
const value = d.value ? structuredClone(d.value) : d.value
|
||||
// With nothing deployed the server fills `r.value` from the draft itself.
|
||||
if (value) dropStaleStepLocks(value, draftOnly || r.no_deployed ? undefined : r.value)
|
||||
const requestBody = {
|
||||
// Deploy at the draft's intended path: flow/app/raw-app drafts keep the
|
||||
// user-typed path in `draft_path` (a never-deployed item is parked at a
|
||||
@@ -498,7 +510,7 @@ export async function deployDraft(
|
||||
path: d.draft_path ?? d.path ?? path,
|
||||
summary: d.summary ?? '',
|
||||
description: d.description ?? '',
|
||||
value: d.value,
|
||||
value,
|
||||
schema: d.schema,
|
||||
tag: d.tag,
|
||||
dedicated_worker: d.dedicated_worker,
|
||||
|
||||
Reference in New Issue
Block a user