fix: keep refused discards, one writer per key, toggles past saves

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViTUkt4czrvZdxdWwxqRAQ
2026-09-21 00:02:30 +00:00 · 2026-09-11 18:29:19 +02:00
parent 29d1ab3850
commit 8b06fee6f3
2 changed files with 141 additions and 11 deletions
@@ -172,7 +172,13 @@ class Entry<V> {
 	absorbs: ((next: V, deployed: V) => boolean) | undefined
 	refs = 0
 	disposed = false
+	/** Replaced by an entry that moved onto its key: it no longer owns the row there. */
+	retired = false
+	handles = new Set<Handle<V>>()
 	stopWatch: (() => void) | undefined
+	/** Fields a `patch` has put on the deployed side ahead of the server; a save landing
+	 *  meanwhile keeps them rather than rolling the baseline back past them. */
+	private patched: Record<string, unknown> = {}
 	/** The value as last observed, serialized: `touch` acts only on a real change. */
 	private seen: string | undefined
 	/** The row last handed to the syncer (`null`: none, `undefined`: not known — the next
@@ -217,7 +223,7 @@ class Entry<V> {
 	/** Mirror `dirty ? value : null` to the draft row. The only place a row is written. */
 	reconcile(): void {
 		const key = this.key
-		if (!this.loaded || this.origin === 'new' || isTemporaryPath(key.path)) return
+		if (this.retired || !this.loaded || this.origin === 'new' || isTemporaryPath(key.path)) return
 		const desired = this.dirty ? (serialize(this.value) ?? null) : null
 		this.ports.hint(key, desired !== null)
 		if (desired === this.row) return
@@ -358,7 +364,7 @@ class Entry<V> {
 				return { ok: false, error: this.error }
 			}
 			this.error = undefined
-			this.deployed = sent
+			this.deployed = { ...sent, ...this.patched } as V
 			this.origin = 'deployed'
 			this.template = undefined
 			const moved = to !== from.path
@@ -377,9 +383,17 @@ class Entry<V> {
 		return this.run(async () => {
 			if (!this.loaded) return { removed: false }
 			if (this.origin === 'draft') {
+				const kept = snapshot(this.value)
+				const keptRow = this.row
 				this.replaceValue(undefined)
 				this.reconcile()
 				await this.settleRows([this.key])
+				// The row is the item: while its delete has not landed, the item is still there.
+				if (this.rowRefused(this.key)) {
+					this.row = keptRow
+					this.replaceValue(kept)
+					return { removed: false }
+				}
 				this.removed = true
 				return { removed: true }
 			}
@@ -400,6 +414,11 @@ class Entry<V> {
 		await Promise.all(keys.filter((k) => !isTemporaryPath(k.path)).map((k) => this.ports.flush(k)))
 	}

+	/** The syncer settles a rejected or failed write without throwing; this is how to tell. */
+	private rowRefused(key: ItemKey): boolean {
+		return this.ports.conflicted(key) || this.ports.failure(key) !== undefined
+	}
+
 	/**
 	 * Re-key to `path`. The row at the path left behind follows from nothing living there. The
 	 * one at `path` was never read: whatever it holds predates the write that just landed there,
@@ -427,12 +446,19 @@ class Entry<V> {
 		const before = snapshot(this[baseKey]) as Record<string, unknown> | undefined
 		const sent = snapshot(fields) as Record<string, unknown>
 		if (before !== undefined) this[baseKey] = { ...before, ...sent } as V
+		if (baseKey === 'deployed') Object.assign(this.patched, sent)
 		if (this.value !== undefined) Object.assign(this.value as object, fields)
 		this.touch()
+		const settled = () => {
+			for (const [k, v] of Object.entries(sent)) {
+				if (deepEqual(this.patched[k], v)) delete this.patched[k]
+			}
+		}
 		return this.run(async () => {
 			try {
 				await write(this.key)
 			} catch (e) {
+				settled()
 				const giveBack = (side: V | undefined): V | undefined => {
 					if (side === undefined || before === undefined) return undefined
 					const out = snapshot(side) as Record<string, unknown>
@@ -453,8 +479,8 @@ class Entry<V> {
 				this.reconcile()
 				return { ok: false, error: this.error }
 			}
+			settled()
 			this.error = undefined
-			// A save that landed while this waited set the baseline to what it sent.
 			if (this[baseKey] !== undefined) this[baseKey] = { ...this[baseKey], ...sent } as V
 			this.touch()
 			this.reconcile()
@@ -550,7 +576,8 @@ export type ItemSpec<V> = {
 /** A class, not a literal: Svelte deep-proxies plain objects put in `$state`, and a proxied
 *  handle would no longer be the one `saveEach` recognizes. */
 class Handle<V> implements ItemHandle<V> {
-	readonly entry: Entry<V>
+	/** Reactive: a handle follows its item onto the entry that took over its key. */
+	entry: Entry<V> = $state.raw()!
 	readonly spec: ItemSpec<V>
 	readonly adapter: ItemAdapter<V>

@@ -558,6 +585,7 @@ class Handle<V> implements ItemHandle<V> {
 		this.entry = entry
 		this.spec = spec
 		this.adapter = adapter
+		entry.handles.add(this)
 	}

 	get key() {
@@ -654,10 +682,30 @@ export function createItemStore(ports: ItemRowPort) {
 		rekey(entry, from) {
 			const k = keyString(from)
 			if (entries.get(k) === entry) entries.delete(k)
-			entries.set(keyString(entry.key), entry)
+			const to = keyString(entry.key)
+			const displaced = entries.get(to)
+			if (displaced && displaced !== entry) retire(displaced, entry)
+			entries.set(to, entry)
 		}
 	}

+	/**
+	 * An entry moved onto a key another live entry holds. One key has one row writer, so the
+	 * old one stops writing and every handle on it moves to the entry that now is the item —
+	 * the same outcome as when nobody had it open: what was there is superseded by the write.
+	 */
+	function retire(old: Entry<any>, into: Entry<any>): void {
+		old.retired = true
+		old.stopWatch?.()
+		for (const handle of old.handles) {
+			handle.entry = into
+			into.handles.add(handle)
+			into.refs++
+			old.refs--
+		}
+		old.handles.clear()
+	}
+
 	function watch(entry: Entry<any>): void {
 		// Detached from whichever component acquired it first: the entry outlives that component
 		// while another holder, or a command in flight, still has it.
@@ -691,14 +739,15 @@ export function createItemStore(ports: ItemRowPort) {
 			watch(entry)
 		}
 		entry.refs++
-		const held = entry
+		const handle = new Handle(entry, spec, adapter)
 		let released = false
 		return {
-			handle: new Handle(held, spec, adapter),
+			handle,
 			release() {
 				if (released) return
 				released = true
-				internals.release(held)
+				handle.entry.handles.delete(handle)
+				internals.release(handle.entry)
 			}
 		}
 	}
@@ -26,9 +26,15 @@ function deferred<T = void>() {
 function fakeRows() {
 	const writes: { path: string; value: unknown }[] = []
 	const conflicts = new Set<string>()
+	/** Paths whose next write the server fails. */
+	const failing = new Set<string>()
+	const failures = new Map<string, string>()
 	const hints = new Map<string, boolean>()
 	const port: ItemRowPort = {
-		write: (key, value) => void writes.push({ path: key.path, value }),
+		write: (key, value) => {
+			writes.push({ path: key.path, value })
+			if (failing.has(key.path)) failures.set(key.path, 'unreachable')
+		},
 		flush: async () => {},
 		overwrite: async (key, value) => {
 			conflicts.delete(key.path)
@@ -36,11 +42,11 @@ function fakeRows() {
 		},
 		seedSync: () => {},
 		conflicted: (key) => conflicts.has(key.path),
-		failure: () => undefined,
+		failure: (key) => failures.get(key.path),
 		dropPending: () => {},
 		hint: (key, on) => void hints.set(key.path, on)
 	}
-	return { port, writes, conflicts, hints }
+	return { port, writes, conflicts, failing, hints }
 }

 const deployedRes: Res = { path: 'u/me/r', description: 'deployed', args: { a: 1 } }
@@ -193,6 +199,34 @@ describe('item store: commands', () => {
 		expect([first.dirty, second.dirty, second.busy]).toEqual([true, true, false])
 	})

+	it('keeps a toggle made during a save out of the draft row', async () => {
+		type Sched = { path: string; enabled: boolean; summary: string }
+		const rows = fakeRows()
+		const saveGate = deferred()
+		const store = createItemStore(rows.port)
+		const { handle: item } = store.acquire(
+			{ workspace: 'w', kind: 'trigger_schedule', path: 's' },
+			{ workspace: 'w', path: 's' },
+			{
+				load: async () => ({ deployed: { path: 's', enabled: true, summary: 'a' } }),
+				write: () => saveGate.promise
+			} as ItemAdapter<Sched>
+		)
+		await settle()
+		item.value = { path: 's', enabled: true, summary: 'b' }
+		const saving = item.save()
+		const toggling = item.patch({ enabled: false }, async () => {})
+		saveGate.resolve()
+		await saving
+		// The save's baseline predates the toggle; the toggle's field survives it, so nothing
+		// is left unsaved and the row goes.
+		expect(item.dirty).toBe(false)
+		expect(rows.writes.at(-1)).toEqual({ path: 's', value: null })
+		await toggling
+		expect(item.deployed).toEqual({ path: 's', enabled: false, summary: 'b' })
+		expect(rows.writes.at(-1)).toEqual({ path: 's', value: null })
+	})
+
 	it('holds a toggle behind a save, and keeps an unrelated edit through the toggle', async () => {
 		type Sched = { path: string; enabled: boolean; summary: string }
 		const rows = fakeRows()
@@ -348,6 +382,18 @@ describe('item store: origins', () => {
 		expect(rows.writes).toEqual([{ path: 'u/me/r', value: null }])
 	})

+	it('keeps a draft-only item whose delete did not land', async () => {
+		const rows = fakeRows()
+		const draft = { ...deployedRes, description: 'only a draft' }
+		const { item } = await open(rows, adapter({ draft }))
+		rows.failing.add('u/me/r')
+
+		expect(await item.discard()).toEqual({ removed: false })
+		expect(item.removed).toBe(false)
+		expect(item.value).toEqual(draft)
+		expect(item.status).toBe('failed')
+	})
+
 	it('creates under a temporary path, then moves to the real one and clears any row there', async () => {
 		const rows = fakeRows()
 		const temp = newItemPath()
@@ -387,6 +433,41 @@ describe('item store: origins', () => {
 	})
 })

+describe('item store: one entry per key', () => {
+	it('moves onto a key another editor holds, which then follows the item there', async () => {
+		const rows = fakeRows()
+		const store = createItemStore(rows.port)
+		const at = (path: string, load: ItemLoad<Res>) =>
+			store.acquire(
+				{ workspace: 'w', kind: 'resource', path },
+				{ workspace: 'w', path },
+				adapter(load)
+			).handle
+		const other = at('u/me/b', {
+			draft: { ...deployedRes, path: 'u/me/b', description: 'draft b' }
+		})
+		const moving = at('u/me/r', { deployed: deployedRes })
+		await settle()
+
+		moving.value = { ...deployedRes, path: 'u/me/b' }
+		expect(await moving.save()).toMatchObject({ ok: true, moved: true })
+
+		expect(other.key.path).toBe('u/me/b')
+		expect(other.value).toEqual({ ...deployedRes, path: 'u/me/b' })
+		expect(other.origin).toBe('deployed')
+		// The displaced entry writes nothing more: `u/me/b` has one writer.
+		const before = rows.writes.length
+		other.value = { ...deployedRes, path: 'u/me/b', description: 'typed in the other editor' }
+		expect(moving.value?.description).toBe('typed in the other editor')
+		expect(rows.writes.slice(before)).toEqual([
+			{
+				path: 'u/me/b',
+				value: { ...deployedRes, path: 'u/me/b', description: 'typed in the other editor' }
+			}
+		])
+	})
+})
+
 describe('item store: conflicts', () => {
 	it('stops writing a key the server rejected, until the conflict is resolved', async () => {
 		const rows = fakeRows()