fix(frontend): treat a displaced draft save as superseded, not failed (#10094)

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<void>
 }
 
 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<string>()
+	// Live chain promise per key, backing `settled`.
+	const chains = new Map<string, Promise<void>>()
 
 	async function chain(key: string, first: PendingTask): Promise<void> {
 		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<void>((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<void> {
+		return chains.get(key) ?? Promise.resolve()
+	}
+
+	return { submit, submitAndWait, cancel, isRunning, settled }
 }

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()

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<UserDraftDbSyncerSaveOpts, 'force'>): Promise<void> {
 		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

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<T = void>() {
+	let resolve!: (v: T) => void
+	const promise = new Promise<T>((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])
+	})
+})