mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 08:02:47 +00:00
fix: only call a read stale when no newer value came with the row
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
47afd85bcc
commit
f753d993ff
@@ -2490,6 +2490,39 @@ describe('global AI tools', () => {
|
||||
}
|
||||
})
|
||||
|
||||
// A deploy carries the draft as it stood when it started. An editor changed since has a
|
||||
// divergence from that which is still undeployed, so saying "Draft removed" sends the model
|
||||
// and the user away from an edit that still needs deploying.
|
||||
it('reports a draft the deploy deliberately left in place', async () => {
|
||||
const path = 'u/admin/kept_res'
|
||||
seedBackendDraft('resource', path, { path, value: { a: 2 } })
|
||||
registerLiveItemBridge({
|
||||
seed: () => false,
|
||||
// The editor kept a row: it diverged from what was deployed while the deploy ran.
|
||||
read: () => ({ value: { path, value: { a: 3 } } }),
|
||||
refresh: () => Promise.resolve('done' as const),
|
||||
itemDeleted: () => Promise.resolve(),
|
||||
discard: () => Promise.resolve('done' as const),
|
||||
list: () => []
|
||||
})
|
||||
try {
|
||||
const cleanup = await deleteGlobalDraft(WORKSPACE, 'resource', path, undefined, {
|
||||
preserveLiveDraft: true,
|
||||
deployed: true
|
||||
})
|
||||
expect(cleanup).toEqual({ removed: false })
|
||||
} finally {
|
||||
registerLiveItemBridge({
|
||||
seed: () => false,
|
||||
read: () => undefined,
|
||||
refresh: () => undefined,
|
||||
itemDeleted: () => Promise.resolve(),
|
||||
discard: () => undefined,
|
||||
list: () => []
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
// A failed server delete must surface (throw), not silently report removed —
|
||||
// the same guard the write path got, applied to the delete path.
|
||||
it('deleteGlobalDraft throws when the server delete fails', async () => {
|
||||
|
||||
@@ -7994,7 +7994,7 @@ async function deployDraft(
|
||||
// fork comparisons before the fallible draft cleanup below.
|
||||
invalidateWorkspaceComparison(workspace)
|
||||
|
||||
await deleteGlobalDraft(workspace, type, path, triggerKind, {
|
||||
const cleanup = await deleteGlobalDraft(workspace, type, path, triggerKind, {
|
||||
preserveLiveDraft: true,
|
||||
deployed: true
|
||||
})
|
||||
@@ -8030,9 +8030,11 @@ async function deployDraft(
|
||||
return JSON.stringify(
|
||||
{
|
||||
success: true,
|
||||
message: `Deployed draft ${type} "${path}" to the workspace. Draft removed.${
|
||||
deployNote ? ` ${deployNote}` : ''
|
||||
}`,
|
||||
message: `Deployed draft ${type} "${path}" to the workspace. ${
|
||||
cleanup.removed
|
||||
? 'Draft removed.'
|
||||
: 'Its editor was changed while the deploy ran, so a draft of those later changes remains and is not deployed.'
|
||||
}${deployNote ? ` ${deployNote}` : ''}`,
|
||||
type,
|
||||
path,
|
||||
triggerKind
|
||||
|
||||
@@ -589,15 +589,17 @@ type DeleteGlobalDraftOptions = {
|
||||
itemDeleted?: boolean
|
||||
}
|
||||
|
||||
/** `removed` is false when a draft is deliberately left in place: an open editor diverged from
|
||||
* what was deployed while the deploy was running, and that divergence is still to be deployed. */
|
||||
export async function deleteGlobalDraft(
|
||||
workspace: string,
|
||||
type: WorkspaceItemType,
|
||||
path: string,
|
||||
triggerKind?: TriggerKind,
|
||||
options: DeleteGlobalDraftOptions = {}
|
||||
): Promise<void> {
|
||||
): Promise<{ removed: boolean }> {
|
||||
const itemKind = itemKindFor(type, triggerKind)
|
||||
if (!itemKind) return
|
||||
if (!itemKind) return { removed: false }
|
||||
const storagePath = resolveDraftStoragePath(workspace, itemKind, path)
|
||||
if (options.itemDeleted) {
|
||||
// Nothing to discard against: the item is gone. The editor is told that, and the row is
|
||||
@@ -612,7 +614,7 @@ export async function deleteGlobalDraft(
|
||||
immediate: true
|
||||
})
|
||||
await assertDraftCleanupLanded(workspace, itemKind, path, storagePath)
|
||||
return
|
||||
return { removed: true }
|
||||
}
|
||||
if (options.deployed) {
|
||||
// Re-reading the open editor is the whole cleanup, and discarding on top of it is wrong:
|
||||
@@ -625,7 +627,9 @@ export async function deleteGlobalDraft(
|
||||
// settled — with no draft, or with the newer edit.
|
||||
await UserDraftDbSyncer.flush({ workspace, itemKind, path: storagePath })
|
||||
await assertDraftCleanupLanded(workspace, itemKind, path, storagePath)
|
||||
return
|
||||
// The editor keeps a row when it diverged from what was deployed while the deploy was
|
||||
// running. Saying it was removed would send the caller away from an undeployed edit.
|
||||
return { removed: !UserDraft.has(itemKind, storagePath, { workspace }) }
|
||||
}
|
||||
if (refreshed === 'failed') {
|
||||
// It has the item and a baseline from before the deploy. Discarding its row would
|
||||
@@ -664,6 +668,7 @@ export async function deleteGlobalDraft(
|
||||
})
|
||||
}
|
||||
await assertDraftCleanupLanded(workspace, itemKind, path, storagePath)
|
||||
return { removed: true }
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -414,16 +414,16 @@ class Entry<V> {
|
||||
// nothing can send it, so a read nobody asked for must not put the server's value in
|
||||
// its place, nor answer the conflict on the user's behalf. Checked here rather than
|
||||
// before the read, because the flush that precedes one is itself a way to find out.
|
||||
// A row was handed over while this read was out, so its response predates it. The value
|
||||
// is still shown, for want of a better one, but writing from it would post it back
|
||||
// over that row on the baseline the row advanced: the newer write lost to an older
|
||||
// read. Nothing goes out until a reload settles which is which.
|
||||
this.stale = this.ports.rowMark(key) !== rowsAtRead
|
||||
const standOff = unasked && this.ports.conflicted(key)
|
||||
// Whatever landed since the read was asked for — an external write, an edit — is
|
||||
// newer than what it read, so the read only moves the deployed side under it.
|
||||
const keepValue =
|
||||
standOff || this.edits !== at.edits || this.externals !== at.externals
|
||||
// A row landed while the read was out and no newer value came with it, so it was
|
||||
// written by someone this entry cannot see. Its response predates that row, and
|
||||
// writing from it would post it back over it on the baseline that row advanced — so
|
||||
// nothing goes out, and nothing is saved or discarded, until a reload.
|
||||
this.stale = this.ports.rowMark(key) !== rowsAtRead && !keepValue
|
||||
this.meta = res.meta
|
||||
this.error = undefined
|
||||
if (isTemporaryPath(key.path)) {
|
||||
|
||||
@@ -713,6 +713,8 @@ describe('item store: origins', () => {
|
||||
// The chat writes after asking for the discard, so its value is the newer of the two.
|
||||
const fromChat = { ...deployedRes, description: 'from the chat' }
|
||||
item.applyExternal(fromChat)
|
||||
// `persistGlobalDraft` hands the value here and writes its own row through the syncer.
|
||||
rows.handExternally('u/me/r')
|
||||
read.resolve({ deployed: deployedRes })
|
||||
|
||||
expect(await discarding).toEqual({ removed: false })
|
||||
@@ -843,6 +845,34 @@ describe('item store: origins', () => {
|
||||
expect(item.value?.description).toBe('typed during the deploy')
|
||||
})
|
||||
|
||||
it('still autosaves after the chat wrote a row during its first read', async () => {
|
||||
const rows = fakeRows()
|
||||
const read = deferred<ItemLoad<Res>>()
|
||||
const store = createItemStore(rows.port)
|
||||
const key: ItemKey = { workspace: 'w', kind: 'resource', path: 'u/me/r' }
|
||||
const { handle: item } = store.acquire(
|
||||
key,
|
||||
{ workspace: 'w', path: 'u/me/r' },
|
||||
adapter(() => read.promise)
|
||||
)
|
||||
await settle()
|
||||
// The chat writes while the GET is out. Its row is this entry's own doing, and its value
|
||||
// is right here — so the read is behind the server, but not in a way that loses anything.
|
||||
const fromChat = { ...deployedRes, description: 'from the chat' }
|
||||
item.applyExternal(fromChat)
|
||||
// `persistGlobalDraft` hands the value here and writes its own row through the syncer.
|
||||
rows.handExternally('u/me/r')
|
||||
read.resolve({ deployed: deployedRes })
|
||||
await settle()
|
||||
|
||||
expect(item.value).toEqual(fromChat)
|
||||
// The editor opened on the chat's draft; typing from here has to keep reaching the row.
|
||||
expect(item.canSave).toBe(true)
|
||||
const typed = { ...deployedRes, description: 'typed after opening' }
|
||||
item.value = typed
|
||||
expect(rows.writes.at(-1)).toEqual({ path: 'u/me/r', value: typed })
|
||||
})
|
||||
|
||||
it('does not post a read back over a row another editor landed during it', async () => {
|
||||
const rows = fakeRows()
|
||||
const read = deferred<ItemLoad<Res>>()
|
||||
|
||||
Reference in New Issue
Block a user