starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-08 08:04:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: send the sweep's delete straight to the API, not through the syncer

Browse Source 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f
This commit is contained in:
Diego Imbert
2026-09-05 20:25:31 +02:00
co-authored by Claude Opus 5
parent a0b411eb98
commit 2343c16f50
2 changed files with 65 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/lib/userDraftPrune.test.ts
+37 -70
View File
@@ -5,48 +5,34 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'
// per-kind diff — and assert on what it discards.
const listDrafts = vi.fn()
const getDraftDiffValues = vi.fn()
const discardDraft = vi.fn(async () => ({ success: true }))
const updateDraft = vi.fn(async () => ({ status: 'saved', current_timestamp: 'x' }))
vi.mock('./gen', () => ({
DraftService: { listDrafts: (...a: unknown[]) => listDrafts(...(a as [])) }
DraftService: {
listDrafts: (...a: unknown[]) => listDrafts(...(a as [])),
updateDraft: (...a: unknown[]) => updateDraft(...(a as []))
}
}))
// Only the two network-touching functions are stubbed; `canDiffDraftKind` is
// the real one, so the kind filter is pinned against the actual overlay table.
// Only `getDraftDiffValues` is stubbed; `canDiffDraftKind` is the real one, so
// the kind filter is pinned against the actual overlay table.
vi.mock('./utils_draft_deploy', async (orig) => ({
...(await orig<Record<string, unknown>>()),
getDraftDiffValues: (...a: unknown[]) => getDraftDiffValues(...(a as [])),
discardDraft: (...a: unknown[]) => discardDraft(...(a as []))
getDraftDiffValues: (...a: unknown[]) => getDraftDiffValues(...(a as []))
}))
vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() }))
vi.mock('./workspaceDrafts.svelte', () => ({ invalidateWorkspaceDrafts: vi.fn() }))
const sendUserToast = vi.fn()
vi.mock('./toast', () => ({ sendUserToast: (...a: unknown[]) => sendUserToast(...(a as [])) }))
// The sweep reads exactly one thing from the syncer — whether this tab is
// mid-write on the key — and writes nothing back to it.
let syncState = 'none'
let conflict: unknown = undefined
const recordRemoteSync = vi.fn()
const dropPending = vi.fn()
/** Ordered log of the two calls whose ORDER is the guard being pinned. */
let syncerCalls: string[] = []
vi.mock('./userDraftDbSyncer.svelte', () => ({
UserDraftDbSyncer: {
save: vi.fn(),
dropPending: (...a: unknown[]) => {
syncerCalls.push('dropPending')
return dropPending(...(a as []))
},
recordRemoteSync: (...a: unknown[]) => {
syncerCalls.push('recordRemoteSync')
return recordRemoteSync(...(a as []))
},
getState: () => ({
get state() {
return syncState
}
}),
getConflict: () => ({
get conflict() {
return conflict
}
})
}
}))
@@ -76,16 +62,14 @@ const diff = (over: Record<string, unknown> = {}) => ({
noDeployed: false,
...over
})
const discardedPaths = () => discardDraft.mock.calls.map((c: any[]) => c[1])
const discardedPaths = () => updateDraft.mock.calls.map((c: any[]) => c[0].path as string)
beforeEach(() => {
localStorage.clear()
vi.clearAllMocks()
discardDraft.mockResolvedValue({ success: true })
updateDraft.mockResolvedValue({ status: 'saved', current_timestamp: 'x' })
syncState = 'none'
conflict = undefined
liveDraft = false
syncerCalls = []
})
describe('pruneMeaninglessDrafts', () => {
@@ -100,28 +84,28 @@ describe('pruneMeaninglessDrafts', () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockResolvedValue(diff({ draft: { value: { host: 'other' } } }))
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('never touches a draft-only item — the draft is the whole item', async () => {
listDrafts.mockResolvedValue([row({ draft_only: true })])
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('never touches another user’s row, or one it cannot write', async () => {
listDrafts.mockResolvedValue([row({ mine: false }), row({ path: 'u/me/b', can_write: false })])
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('leaves a draft alone when its diff cannot be fetched, and retries it later', async () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockRejectedValue(new Error('boom'))
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
// A row that could not be judged is not a row that carries changes, so
// the pass must stay open rather than strand it.
getDraftDiffValues.mockResolvedValue(diff())
@@ -132,36 +116,29 @@ describe('pruneMeaninglessDrafts', () => {
it('conditions the delete on the timestamp it judged, so a row that moved is spared', async () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockResolvedValue(diff())
conflict = { serverTimestamp: '2026-01-02T00:00:00Z', localLastSync: null }
updateDraft.mockResolvedValue({ status: 'conflict', current_timestamp: 'newer' })
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(recordRemoteSync).toHaveBeenCalledWith(
{ workspace: 'main', itemKind: 'resource', path: 'u/me/r' },
'2026-01-01T00:00:00Z'
)
// The discard was attempted but refused, so nothing is reported as cleared.
expect(updateDraft).toHaveBeenCalledWith({
workspace: 'main',
kind: 'resource',
path: 'u/me/r',
requestBody: { value: null, last_sync: '2026-01-01T00:00:00Z', force: false }
})
// Refused, so nothing is reported as cleared — and the sweep leaves no
// state behind for the editor's own autosave to trip over.
expect(sendUserToast).not.toHaveBeenCalled()
// …and the refused baseline is handed back to the server's, so the next
// autosave for this key is not refused too.
expect(recordRemoteSync).toHaveBeenLastCalledWith(
{ workspace: 'main', itemKind: 'resource', path: 'u/me/r' },
'2026-01-02T00:00:00Z'
)
// Order matters: the refused delete stays parked for the pagehide flush,
// which would re-send it with whatever baseline is current. Adopting the
// server's timestamp first would be handing it the one value that works.
expect(syncerCalls.slice(-2)).toEqual(['dropPending', 'recordRemoteSync'])
})
it('does not keep retrying a row the server will never judge', async () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockRejectedValue({ status: 404 })
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
// Sealed: a 4xx is final, unlike the transient case above.
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('gives up after a bounded number of unresolved passes', async () => {
@@ -183,7 +160,7 @@ describe('pruneMeaninglessDrafts', () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('leaves alone a draft this tab is editing', async () => {
@@ -191,7 +168,7 @@ describe('pruneMeaninglessDrafts', () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('leaves alone a draft with a write queued or in flight', async () => {
@@ -199,26 +176,16 @@ describe('pruneMeaninglessDrafts', () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('does not count, or seal the pass on, a delete the syncer failed to send', async () => {
it('does not count, or seal the pass on, a delete that failed to send', async () => {
listDrafts.mockResolvedValue([row()])
getDraftDiffValues.mockResolvedValue(diff())
// The discard's own POST is what fails, so the state flips during it.
discardDraft.mockImplementation(async () => {
syncState = 'failed'
return { success: true }
})
updateDraft.mockRejectedValueOnce(new Error('network'))
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(sendUserToast).not.toHaveBeenCalled()
discardDraft.mockResolvedValue({ success: true })
// A same-session retry sees the key as busy (a failed save leaves the
// payload parked), attempts nothing — and must still not seal the pass.
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardedPaths()).toEqual(['u/me/r'])
// Once the failure clears, the draft left behind is finally retried.
syncState = 'none'
// The pass stayed open, so the draft left behind is retried.
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardedPaths()).toEqual(['u/me/r', 'u/me/r'])
})
@@ -227,7 +194,7 @@ describe('pruneMeaninglessDrafts', () => {
listDrafts.mockResolvedValue([row({ legacy_draft: true })])
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).not.toHaveBeenCalled()
expect(updateDraft).not.toHaveBeenCalled()
})
it('only sweeps the kinds whose editors are gated', async () => {
@@ -260,9 +227,9 @@ describe('pruneMeaninglessDrafts', () => {
getDraftDiffValues.mockResolvedValue(diff())
await pruneMeaninglessDrafts('main', 'me@x.dev')
await pruneMeaninglessDrafts('main', 'me@x.dev')
expect(discardDraft).toHaveBeenCalledTimes(1)
expect(updateDraft).toHaveBeenCalledTimes(1)
await pruneMeaninglessDrafts('other', 'me@x.dev')
expect(discardDraft).toHaveBeenCalledTimes(2)
expect(updateDraft).toHaveBeenCalledTimes(2)
})
it('retries next mount when the listing failed', async () => {
frontend/src/lib/userDraftPrune.ts
+28 -30
View File
@@ -21,21 +21,25 @@
*
* Deleting is the dangerous half, and the equality behind it is always stale:
* it was read one round trip ago, and every candidate is read before any is
* deleted. Two guards keep that from eating work. Anything this tab is
* currently writing is skipped outright — a discard POSTs immediately, which
* cancels the autosave the user's keystrokes have queued. And the delete
* itself is a compare-and-delete: the listing's timestamp is seeded as the
* `last_sync` baseline, so the backend refuses it if the row moved since,
* whoever moved it. Without the seed a freshly loaded tab has no baseline and
* the delete is unconditional.
* deleted. So the delete is a compare-and-delete — `last_sync` is the
* timestamp the row was judged on, and the backend drops it only if nothing has
* written it since, whoever wrote it.
*
* It is sent straight to `DraftService`, NOT through `UserDraftDbSyncer`. That
* syncer exists to autosave an editor's own live value, and everything it does
* for that — parking the payload for the `pagehide` flush, debouncing, holding
* a per-tab `last_sync` baseline and conflict state — is a way for a one-shot
* delete to reach back into whatever the user is doing in the same tab. This
* sweep wants exactly one conditional request and no state afterwards.
*/
import { DraftService } from './gen'
import type { UserDraftItemKind } from './gen'
import { sendUserToast } from './toast'
import { setLocalDraftHint } from './localDraftHints.svelte'
import { UserDraft, draftValuesEqual } from './userDraft.svelte'
import { UserDraftDbSyncer } from './userDraftDbSyncer.svelte'
import { canDiffDraftKind, discardDraft, getDraftDiffValues } from './utils_draft_deploy'
import { canDiffDraftKind, getDraftDiffValues } from './utils_draft_deploy'
import { invalidateWorkspaceDrafts } from './workspaceDrafts.svelte'
const SENTINEL_PREFIX = 'userdraft/pruned/v1/'
@@ -183,28 +187,22 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string)
unresolved++
continue
}
const q = { workspace, itemKind: c.kind, path: c.path }
UserDraftDbSyncer.recordRemoteSync(q, c.createdAt)
const res = await discardDraft(c.kind, c.path, workspace, false, false, false)
// Neither outcome throws: the syncer swallows an HTTP failure into its
// per-key state, and a delete refused for a moved row comes back as a
// conflict. So `success` alone says nothing about whether the row went.
const conflict = UserDraftDbSyncer.getConflict(q).conflict
if (!res.success || UserDraftDbSyncer.getState(q).state === 'failed') unresolved++
else if (conflict) {
// The row moved past the baseline we seeded. Drop our refused delete
// BEFORE adopting the server's timestamp: a refused save stays parked
// for the `pagehide` keepalive flush, which re-sends it with whatever
// baseline is current by then — so adopting first would hand it the
// one timestamp that makes the delete succeed, against a row that now
// holds someone's newer draft.
UserDraftDbSyncer.dropPending(q)
// Then re-baseline. The syncer keeps a refused baseline until
// something resolves it, and nothing here would: these drawer editors
// never re-seed on load and mount no conflict modal, so every later
// autosave for this key would be refused and the user's edit lost.
UserDraftDbSyncer.recordRemoteSync(q, conflict.serverTimestamp)
} else discarded++
try {
const resp = await DraftService.updateDraft({
workspace,
kind: c.kind,
path: c.path,
requestBody: { value: null, last_sync: c.createdAt, force: false }
})
// `conflict` means the row moved past the timestamp we judged it on,
// so it is no longer the empty draft we decided to drop.
if (resp.status === 'saved') {
setLocalDraftHint(workspace, c.kind, c.path, false)
discarded++
}
} catch {
unresolved++
}
}
if (discarded > 0) {
invalidateWorkspaceDrafts(workspace)