From 2fe999f66cd15acd81850f970ada31e9892abff2 Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Wed, 15 Jul 2026 17:55:01 +0200 Subject: [PATCH] fix(frontend): treat a displaced draft save as superseded, not failed (#10094) --- frontend/src/lib/coalescingRunner.svelte.ts | 22 ++- frontend/src/lib/coalescingRunner.test.ts | 81 +++++++++++ frontend/src/lib/userDraftDbSyncer.svelte.ts | 41 ++++-- .../src/lib/userDraftDisplacedSave.test.ts | 133 ++++++++++++++++++ 4 files changed, 264 insertions(+), 13 deletions(-) create mode 100644 frontend/src/lib/userDraftDisplacedSave.test.ts diff --git a/frontend/src/lib/coalescingRunner.svelte.ts b/frontend/src/lib/coalescingRunner.svelte.ts index 0f687d4829..61849c3e36 100644 --- a/frontend/src/lib/coalescingRunner.svelte.ts +++ b/frontend/src/lib/coalescingRunner.svelte.ts @@ -30,6 +30,9 @@ export type CoalescingKeyedRunner = { cancel(key: string): boolean /** Reactively whether `key`'s chain is running (SvelteSet-backed). */ isRunning(key: string): boolean + /** Resolves once `key`'s chain has drained (nothing running, nothing + * pending), immediately if it's idle. Never rejects. */ + settled(key: string): Promise } type PendingTask = { @@ -51,6 +54,8 @@ export function createCoalescingKeyedRunner(): CoalescingKeyedRunner { // Reactive mirror of keys with a running chain, kept in lock-step with // `state` (SvelteSet for per-key `isRunning` subscriptions). const runningKeys = new SvelteSet() + // Live chain promise per key, backing `settled`. + const chains = new Map>() async function chain(key: string, first: PendingTask): Promise { let current: PendingTask | undefined = first @@ -71,6 +76,7 @@ export function createCoalescingKeyedRunner(): CoalescingKeyedRunner { } state.delete(key) runningKeys.delete(key) + chains.delete(key) } /** Set `task` pending for `key`, displacing (and rejecting) any prior @@ -84,7 +90,15 @@ export function createCoalescingKeyedRunner(): CoalescingKeyedRunner { } state.set(key, { pending: undefined }) runningKeys.add(key) - void chain(key, task) + // Register the chain promise BEFORE the first task runs. `chain` invokes + // the task synchronously, so a task that calls `settled(key)` (or that + // throws synchronously, running cleanup) would otherwise race ahead of a + // `chains.set(key, chain(...))` and leave the map wrong. A separate + // deferred sidesteps that: it's live before the task starts and resolves + // when the chain drains. + let done!: () => void + chains.set(key, new Promise((resolve) => (done = resolve))) + void chain(key, task).finally(done) } function submit(key: string, fn: CoalescingTask): void { @@ -114,5 +128,9 @@ export function createCoalescingKeyedRunner(): CoalescingKeyedRunner { return runningKeys.has(key) } - return { submit, submitAndWait, cancel, isRunning } + function settled(key: string): Promise { + return chains.get(key) ?? Promise.resolve() + } + + return { submit, submitAndWait, cancel, isRunning, settled } } diff --git a/frontend/src/lib/coalescingRunner.test.ts b/frontend/src/lib/coalescingRunner.test.ts index 49cca4688e..1ae97e7db1 100644 --- a/frontend/src/lib/coalescingRunner.test.ts +++ b/frontend/src/lib/coalescingRunner.test.ts @@ -115,6 +115,87 @@ describe('createCoalescingKeyedRunner', () => { expect(runner.cancel('k')).toBe(false) }) + it('settled resolves immediately for an idle key', async () => { + const runner = createCoalescingKeyedRunner() + await expect(runner.settled('k')).resolves.toBeUndefined() + }) + + it('settled resolves once the chain drains, including the displacing task', async () => { + const runner = createCoalescingKeyedRunner() + const d = deferred() + const last = deferred() + const h = vi.fn(() => last.promise) + + runner.submit('k', () => d.promise) // in flight + void runner.submitAndWait('k', () => Promise.resolve()).catch(() => {}) // displaced below + runner.submit('k', h) + + let drained = false + void runner.settled('k').then(() => (drained = true)) + + d.resolve() + await d.promise + await Promise.resolve() + await Promise.resolve() + expect(h).toHaveBeenCalledTimes(1) + expect(drained).toBe(false) // h still running + + last.resolve() + await runner.settled('k') + expect(drained).toBe(true) + expect(runner.isRunning('k')).toBe(false) + }) + + it('settled called synchronously from within the first task does not resolve early', async () => { + const runner = createCoalescingKeyedRunner() + const d = deferred() + let settledEarly = false + let settledResolved = false + runner.submit('k', () => { + // Re-entrant: the task is invoked synchronously as the chain starts. + const p = runner.settled('k') + void p.then(() => (settledResolved = true)) + // Give the microtask a tick to (wrongly) resolve if the entry is missing. + void Promise.resolve().then(() => { + if (settledResolved) settledEarly = true + }) + return d.promise + }) + await Promise.resolve() + await Promise.resolve() + expect(settledEarly).toBe(false) + expect(settledResolved).toBe(false) // still running + + d.resolve() + await runner.settled('k') + expect(settledResolved).toBe(true) + }) + + it('a synchronously-throwing first task leaves no stale chain entry', async () => { + const runner = createCoalescingKeyedRunner() + const err = vi.spyOn(console, 'error').mockImplementation(() => {}) + runner.submit('k', () => { + throw new Error('sync boom') + }) + // Chain drained synchronously; the key must be idle and settled a no-op. + expect(runner.isRunning('k')).toBe(false) + await expect(runner.settled('k')).resolves.toBeUndefined() + // A fresh submit still starts a new chain (map wasn't left stale). + const ran = vi.fn(() => Promise.resolve()) + runner.submit('k', ran) + expect(ran).toHaveBeenCalledTimes(1) + err.mockRestore() + }) + + it('settled ignores a task failure (the chain survives it)', async () => { + const runner = createCoalescingKeyedRunner() + const err = vi.spyOn(console, 'error').mockImplementation(() => {}) + runner.submit('k', () => Promise.reject(new Error('boom'))) + await expect(runner.settled('k')).resolves.toBeUndefined() + expect(runner.isRunning('k')).toBe(false) + err.mockRestore() + }) + it('does not abort the in-flight task on cancel', async () => { const runner = createCoalescingKeyedRunner() const d = deferred() diff --git a/frontend/src/lib/userDraftDbSyncer.svelte.ts b/frontend/src/lib/userDraftDbSyncer.svelte.ts index a75f6b6837..77e2eecc88 100644 --- a/frontend/src/lib/userDraftDbSyncer.svelte.ts +++ b/frontend/src/lib/userDraftDbSyncer.svelte.ts @@ -1,7 +1,7 @@ import { SvelteMap } from 'svelte/reactivity' import { DraftService, type UserDraftItemKind } from './gen' import { OpenAPI } from './gen/core/OpenAPI' -import { createCoalescingKeyedRunner } from './coalescingRunner.svelte' +import { createCoalescingKeyedRunner, CoalescingDisplacedError } from './coalescingRunner.svelte' import { createDebouncerByKey } from './debouncerByKey.svelte' import { setLocalDraftHint } from './localDraftHints.svelte' @@ -92,10 +92,16 @@ export type UserDraftDbSyncerSaveOpts = { value: unknown | null /** Bypass the debouncer: cancel any pending autosave for this key (it * would otherwise overwrite what we send), route through the coalescing - * runner to preserve ordering against an in-flight POST, and resolve - * the returned promise only once the POST lands. Use for + * runner to preserve ordering against an in-flight POST, and resolve only + * once the key's save chain has drained. Use for * `await save(...); read-the-server` flows where a fire-and-forget save - * would race the next read. */ + * would race the next read. + * + * Resolving means "the key is settled", NOT "your payload won": a newer + * save can displace this one (it then carries the later state), and — as + * with every other `save` — `postSave` routes a rejected or failed POST to + * `conflicts` / `failures` rather than throwing. Read those to know what + * actually landed. */ immediate?: boolean /** Skip the optimistic-concurrency check and overwrite the server row. * Used by the conflict-resolution UI ("Overwrite the remote"). Default @@ -441,10 +447,19 @@ export const UserDraftDbSyncer = { pendingSaveOpts.set(key, opts) if (opts.immediate) { // Drop the queued autosave — firing it after our POST would - // re-save the pre-delete value. + // re-save the pre-delete value. `submitAndWait` displaces the + // runner's own pending task, so no `runner.cancel` needed. debouncer.cancel(key) - runner.cancel(key) - await runner.submitAndWait(key, () => postSave(opts)) + try { + await runner.submitAndWait(key, () => postSave(opts)) + } catch (e) { + // Displacement is not a failure: a newer save took our slot, so + // re-POSTing ours would undo it. Wait for the chain instead — + // callers await this to know the key is settled, not to know + // their own payload won. + if (!(e instanceof CoalescingDisplacedError)) throw e + await runner.settled(key) + } return } // Auto-save off: opts stay parked (above) for an explicit flush but @@ -563,8 +578,10 @@ export const UserDraftDbSyncer = { /** * Force-save: bypass the `last_sync` check and overwrite the server row - * (conflict modal's "Overwrite the remote"). Resolves only after the - * POST lands so the caller can `await` before navigating / refetching. + * (conflict modal's "Overwrite the remote"). Resolves once the key's save + * chain drains — see `immediate`; resolution means the chain settled, not + * that this force payload won (a later save can displace it). Callers + * `await` before navigating / refetching. */ async overwrite(opts: Omit): Promise { await this.save({ ...opts, immediate: true, force: true }) @@ -572,8 +589,10 @@ export const UserDraftDbSyncer = { /** * Flush the draft's queued autosave NOW (explicit Ctrl/Cmd+S). Re-submits - * the parked opts with `immediate: true` and resolves only after the POST - * lands, so callers can `await flush(...); show "Saved"`. + * the parked opts with `immediate: true` and resolves once the key's save + * chain drains (see `immediate` — the parked payload may be displaced by a + * later save carrying newer state), so callers can `await flush(...); show + * "Saved"`. * * No-op when nothing is pending. "No pending" does NOT mean "nothing to * save" — Monaco may hold unmaterialized text; flush the editor diff --git a/frontend/src/lib/userDraftDisplacedSave.test.ts b/frontend/src/lib/userDraftDisplacedSave.test.ts new file mode 100644 index 0000000000..50e24ba5c6 --- /dev/null +++ b/frontend/src/lib/userDraftDisplacedSave.test.ts @@ -0,0 +1,133 @@ +import { describe, it, expect, afterEach, vi } from 'vitest' + +// Mocked so a test can hold a POST in flight — that window is what makes a +// queued save displaceable. +const updateDraft = vi.fn(async (..._args: any[]) => ({ + status: 'saved' as const, + current_timestamp: '2020-01-01T00:00:00Z' +})) + +vi.mock('./gen', () => ({ + DraftService: { updateDraft: (...a: unknown[]) => updateDraft(...(a as [])) } +})) +vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } })) +vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) + +import { UserDraftDbSyncer } from './userDraftDbSyncer.svelte' + +function deferred() { + let resolve!: (v: T) => void + const promise = new Promise((res) => (resolve = res)) + return { promise, resolve } +} + +afterEach(() => { + vi.clearAllMocks() + updateDraft.mockResolvedValue({ status: 'saved', current_timestamp: '2020-01-01T00:00:00Z' }) +}) + +/** + * Deploying queues several saves for one draft key back-to-back (mirror write, + * post-deploy delete, unmount flush), so the runner displaces one of them. A + * displaced save must read as "superseded", never as a failure. + */ +describe('UserDraftDbSyncer immediate save displacement', () => { + it('resolves a displaced immediate save once the superseding save lands', async () => { + const q = { workspace: 'w', itemKind: 'script' as const, path: 'u/me/displaced_a' } + const inFlight = deferred() + updateDraft.mockImplementationOnce(async () => { + await inFlight.promise + return { status: 'saved', current_timestamp: '2020-01-01T00:00:00Z' } + }) + + const first = UserDraftDbSyncer.save({ ...q, value: { content: '1' }, immediate: true }) + // Queues behind `first`, then gets displaced by the delete below. + const displaced = UserDraftDbSyncer.save({ ...q, value: { content: '2' }, immediate: true }) + const deleting = UserDraftDbSyncer.save({ ...q, value: null, immediate: true }) + + inFlight.resolve() + await expect(displaced).resolves.toBeUndefined() + await Promise.all([first, deleting]) + + // The displaced task never POSTed — the delete carries the later state. + expect(updateDraft).toHaveBeenCalledTimes(2) + expect(updateDraft.mock.calls.map((c: any[]) => c[0].requestBody.value)).toEqual([ + { content: '1' }, + null + ]) + }) + + it('does not resolve a displaced save before the superseding POST lands', async () => { + const q = { workspace: 'w', itemKind: 'script' as const, path: 'u/me/displaced_b' } + const inFlight = deferred() + const deletePost = deferred() + updateDraft + .mockImplementationOnce(async () => { + await inFlight.promise + return { status: 'saved', current_timestamp: '2020-01-01T00:00:00Z' } + }) + .mockImplementationOnce(async () => { + await deletePost.promise + return { status: 'saved', current_timestamp: '2020-01-01T00:00:01Z' } + }) + + const first = UserDraftDbSyncer.save({ ...q, value: { content: '1' }, immediate: true }) + const displaced = UserDraftDbSyncer.save({ ...q, value: { content: '2' }, immediate: true }) + const deleting = UserDraftDbSyncer.save({ ...q, value: null, immediate: true }) + + let displacedSettled = false + void displaced.then(() => (displacedSettled = true)) + + inFlight.resolve() + await vi.waitFor(() => expect(updateDraft).toHaveBeenCalledTimes(2)) + // Delete still in flight: callers that `await save()` before invalidating + // must not read the server yet. + expect(displacedSettled).toBe(false) + + deletePost.resolve() + await Promise.all([first, displaced, deleting]) + expect(displacedSettled).toBe(true) + }) + + it('resolves a pending save dropped by lockSync without POSTing it', async () => { + const q = { workspace: 'w', itemKind: 'script' as const, path: 'u/me/displaced_lock' } + const inFlight = deferred() + updateDraft.mockImplementationOnce(async () => { + await inFlight.promise + return { status: 'saved', current_timestamp: '2020-01-01T00:00:00Z' } + }) + + const first = UserDraftDbSyncer.save({ ...q, value: { content: '1' }, immediate: true }) + const dropped = UserDraftDbSyncer.save({ ...q, value: { content: '2' }, immediate: true }) + // Another user's draft was loaded: this value must never reach the server. + UserDraftDbSyncer.lockSync(q) + + inFlight.resolve() + // Resolves like every other save on a locked key — the lock's whole point + // is that the write is dropped, so the caller has nothing to wait for. + await expect(dropped).resolves.toBeUndefined() + await first + expect(updateDraft).toHaveBeenCalledTimes(1) + expect(updateDraft.mock.calls[0][0].requestBody.value).toEqual({ content: '1' }) + UserDraftDbSyncer.unlockSync(q) + }) + + it('resolves a flush displaced by a later immediate save', async () => { + const q = { workspace: 'w', itemKind: 'script' as const, path: 'u/me/displaced_c' } + const inFlight = deferred() + updateDraft.mockImplementationOnce(async () => { + await inFlight.promise + return { status: 'saved', current_timestamp: '2020-01-01T00:00:00Z' } + }) + + // Park opts (the reactive mirror's autosave) so `flush` has something to send. + void UserDraftDbSyncer.save({ ...q, value: { content: 'typed' }, auto: true }) + const first = UserDraftDbSyncer.save({ ...q, value: { content: 'x' }, immediate: true }) + const flushed = UserDraftDbSyncer.flush(q) // pending behind `first` + const deleting = UserDraftDbSyncer.save({ ...q, value: null, immediate: true }) // displaces it + + inFlight.resolve() + await expect(flushed).resolves.toBeUndefined() + await Promise.all([first, deleting]) + }) +})