mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-12 16:02:56 +00:00
fix: never merge a write onto the value resolution happened to read
Codex blocked the round on the other half of the previous fix. Reading the cell first covers an editor that stays open, but the user can also edit, save, and close the editor while a tool waits — `write_flow` waits on the provider catalog. Closing releases the cell, so the read fell back to the value captured at dispatch, which is now older than the row the editor just saved: the write reverted the saved summary and settings and dropped a staged rename, and it did not even conflict, because the save had advanced the syncer's timestamp. `readGlobalDraftValue` no longer consults that value at all — the cell if an editor holds one, else the backend. Both of its callers merge onto what it returns, so neither could ever use a value that predates the wait, and the `fresh` flag that let the deploy opt out of it goes with it: there is nothing left to opt out of. Only the storage key is reused across the wait now, never the contents. The cost is one request per write, which is what the captured value was saving. Two P1s came out of that saving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
4eb0979961
commit
62720b0912
@@ -3089,6 +3089,30 @@ describe('global AI tools', () => {
|
||||
expect(value?.summary).toBe('edited in the editor')
|
||||
})
|
||||
|
||||
// The editor can also close during that window, which releases the cell and leaves its
|
||||
// save in the backend row — newer than anything resolution saw.
|
||||
it('prefers the backend draft over the resolved value once the cell is released', async () => {
|
||||
const path = 'u/admin/released_cell'
|
||||
seedBackendDraft(
|
||||
'script',
|
||||
path,
|
||||
{ path, summary: 'saved on close', content: 'new', language: 'bun', kind: 'script' },
|
||||
{ workspace: WORKSPACE }
|
||||
)
|
||||
|
||||
// No cell: the editor that held one has closed.
|
||||
const stale = { path, summary: 'as dispatched', content: 'old', language: 'bun' }
|
||||
const value = await readGlobalDraftValue<any>(WORKSPACE, 'script', {
|
||||
path,
|
||||
storagePath: path,
|
||||
value: stale,
|
||||
fetched: true
|
||||
})
|
||||
|
||||
expect(value?.content).toBe('new')
|
||||
expect(value?.summary).toBe('saved on close')
|
||||
})
|
||||
|
||||
// A rename typed in the editor is only in the cell; the backend row still carries the old
|
||||
// name. Matching the row anyway would let the old name keep selecting the draft.
|
||||
it('does not resolve a renamed draft by the name its backend row still carries', async () => {
|
||||
|
||||
@@ -8538,13 +8538,9 @@ async function deployDraft(
|
||||
// The shared deployer deploys at the path inside the draft and doesn't report it
|
||||
// back, so read it here — post-flush, since a rename may have been parked. This is
|
||||
// where a draft-only or renamed item lands, and what the callbacks below must name.
|
||||
// Read after the flush above, which may have persisted a rename parked in the editor.
|
||||
deployedPath =
|
||||
chosenDraftName(
|
||||
type,
|
||||
// `fresh`: the flush above may have persisted a rename parked in the editor,
|
||||
// which the value read while resolving predates.
|
||||
await readGlobalDraftValue(workspace, type, target, { fresh: true })
|
||||
) ?? storagePath
|
||||
chosenDraftName(type, await readGlobalDraftValue(workspace, type, target)) ?? storagePath
|
||||
const result = await deployDraftToWorkspace(type, storagePath, workspace, {
|
||||
draftOnly,
|
||||
deploymentMessage
|
||||
|
||||
@@ -558,24 +558,25 @@ async function fetchBackendDraftValue(
|
||||
return resp.value ?? undefined
|
||||
}
|
||||
|
||||
/** Draft VALUE at a resolved target, in order: the cell if one is open (the user's freshest
|
||||
* in-tab edits), else the value resolving the target already read, else the current user's
|
||||
* backend draft. `fresh` skips the resolved value — for a caller that has since changed the
|
||||
* draft it is reading. */
|
||||
/** Draft VALUE at a resolved target: the cell if an editor holds one (the user's freshest
|
||||
* in-tab edits), else the current user's backend draft.
|
||||
*
|
||||
* Deliberately not the value resolving the target already read, though that would save a
|
||||
* request. Both callers merge onto what this returns, and a tool awaits other work before its
|
||||
* write — the provider catalog, a bundle. In that window the user can edit and then close the
|
||||
* editor, which releases the cell and leaves the backend row newer than anything resolution
|
||||
* saw; merging onto the older value silently reverts their save. Only the storage key is
|
||||
* reused across that wait, never the contents. */
|
||||
export async function readGlobalDraftValue<V>(
|
||||
workspace: string,
|
||||
type: WorkspaceItemType,
|
||||
target: ResolvedDraftTarget,
|
||||
opts: { triggerKind?: TriggerKind; fresh?: boolean } = {}
|
||||
opts: { triggerKind?: TriggerKind } = {}
|
||||
): Promise<V | undefined> {
|
||||
const itemKind = itemKindFor(type, opts.triggerKind)
|
||||
if (!itemKind) return undefined
|
||||
// Ahead of the resolved value, which was read when the call was dispatched: a tool awaits
|
||||
// other work before its write (the provider catalog, a bundle), and an edit made in the
|
||||
// editor during that window would be overwritten by the older snapshot.
|
||||
const cell = UserDraft.get<V>(itemKind, target.storagePath, { workspace })
|
||||
if (cell !== undefined) return cell
|
||||
if (target.fetched && !opts.fresh) return target.value as V | undefined
|
||||
return (await fetchBackendDraftValue(workspace, itemKind, target.storagePath)) as V | undefined
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user