From d491693a8ad65ff2804da3ef6d9c7bffa3dcd2b4 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 12:41:05 +0200 Subject: [PATCH] fix: let an unasked read move the deployed side under a conflict without answering it Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/lib/itemStore.svelte.ts | 19 +++++---- frontend/src/lib/itemStore.test.ts | 45 ++++++++++++++++++++ frontend/src/lib/userDraftDbSyncer.svelte.ts | 14 +++--- 3 files changed, 64 insertions(+), 14 deletions(-) diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index 5bc9cb1c10..11f37305ab 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -353,7 +353,7 @@ class Entry { /** `editedBefore`: the edit count as of when this read was decided on, for a caller that did * something async first. */ - load(adapter: ItemAdapter, asOf?: AsOf): Promise { + load(adapter: ItemAdapter, asOf?: AsOf, unasked = false): Promise { this.loads++ return this.run(async (at) => { const key = this.key @@ -371,9 +371,14 @@ class Entry { } finally { this.loads-- } + // A conflicted key holds a value that is on screen only: the server refused it and + // 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. + 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 = this.edits !== at.edits + const keepValue = standOff || this.edits !== at.edits this.meta = res.meta this.error = undefined if (isTemporaryPath(key.path)) { @@ -385,7 +390,9 @@ class Entry { } this.serverDeployed = snapshot(res.deployed) this.origin = res.deployed !== undefined ? 'deployed' : 'draft' - this.adoptRow(key, res.draft, res.draftSavedAt, at.rows) + // The deployed side above always advances: the item did change, and a discard after + // the user keeps theirs has to land on what the server holds now. + if (!standOff) this.adoptRow(key, res.draft, res.draftSavedAt, at.rows) this.loaded = true if (!keepValue) { this.pristine = this.settles && this.origin === 'deployed' && res.draft === undefined @@ -401,15 +408,11 @@ class Entry { * so they come back over what was written; one typed since the read was asked for stays. */ async reread(): Promise { if (!this.loaded || this.retired || !this.adapter) return { ok: true } - // This re-read only holds because what is on screen comes back from its own row. A - // conflicted key has no row of ours on the server and no way to put one there, so reading - // would replace the value with the other writer's and call the conflict resolved. - if (this.ports.conflicted(this.key)) return { ok: true } // Taken before the row goes, not after it lands: an edit typed while that write is in // flight is in neither it nor the read that follows, and is newer than both. const asOf = this.asOf() await this.ports.flush(this.key) - return this.load(this.adapter, asOf) + return this.load(this.adapter, asOf, true) } /** An outside write (the AI chat, another editor): a real divergence, never settling. */ diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 1fb3444dd9..14abd4dc17 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -28,6 +28,8 @@ function fakeRows() { const conflicts = new Set() /** Paths whose next write the server fails. */ const failing = new Set() + /** Paths the server rejects when their queued row is actually sent. */ + const conflictOnFlush = new Set() const failures = new Map() const hints = new Map() /** Paths whose parked payload was dropped. */ @@ -55,6 +57,11 @@ function fakeRows() { const going = queued.has(key.path) ? { value: queued.get(key.path) } : undefined queued.delete(key.path) if (holdFlush) await holdFlush + if (going && conflictOnFlush.has(key.path)) { + conflictOnFlush.delete(key.path) + conflicts.add(key.path) + return + } if (going) sent.set(key.path, going.value) }, overwrite: async (key, value) => { @@ -80,6 +87,7 @@ function fakeRows() { port, writes, conflicts, + conflictOnFlush, failing, hints, dropped, @@ -1143,6 +1151,43 @@ describe('item store: conflicts', () => { // conflict resolved. It is the user's to resolve, so it stays. expect(open.value?.description).toBe('mine') expect(open.status).toBe('conflicted') + // The deployed side still learns the write: keeping mine and then discarding has to land + // on what the server holds now, not on the config it held before the panel wrote. + expect(open.deployed).toEqual({ ...b, args: { a: 2 } }) + expect(await open.discard()).toEqual({ removed: false }) + expect(open.value).toEqual({ ...b, args: { a: 2 } }) + }) + + it('keeps a local edit the flush before a re-read found a conflict for', async () => { + const rows = fakeRows() + const store = createItemStore(rows.port) + const b = { ...deployedRes, path: 'u/me/b' } + let deployed = b + const theirs = { ...b, description: 'the other tab' } + const { handle: open } = store.acquire( + { workspace: 'w', kind: 'resource', path: 'u/me/b' }, + { workspace: 'w', path: 'u/me/b' }, + adapter(async () => ({ deployed, draft: theirs })) + ) + await settle() + open.value = { ...b, description: 'mine' } + // The row is queued, not yet refused: the conflict only surfaces when the flush sends it, + // which is the flush the re-read does before reading. + rows.conflictOnFlush.add('u/me/b') + + const temporary = newItemPath() + const { handle: panel } = store.acquire( + { workspace: 'w', kind: 'resource', path: temporary }, + { workspace: 'w', path: temporary, template: b, standsFor: 'u/me/b' }, + adapter({}, async (ctx) => void (deployed = { ...ctx.value })) + ) + await settle() + panel.value = { ...b, args: { a: 2 } } + expect(await panel.save()).toMatchObject({ ok: true, path: 'u/me/b' }) + + expect(open.status).toBe('conflicted') + expect(open.value?.description).toBe('mine') + expect(open.deployed).toEqual({ ...b, args: { a: 2 } }) }) it('clears the conflict on a reload the user typed during', async () => { diff --git a/frontend/src/lib/userDraftDbSyncer.svelte.ts b/frontend/src/lib/userDraftDbSyncer.svelte.ts index 09f3642335..2306b4b336 100644 --- a/frontend/src/lib/userDraftDbSyncer.svelte.ts +++ b/frontend/src/lib/userDraftDbSyncer.svelte.ts @@ -532,17 +532,19 @@ export const UserDraftDbSyncer = { } }, - /** - * Seed the per-tab `last_sync` after an editor reads a draft from the - * server. Pass the response's `draft_saved_at` so the next save sends a - * matching `last_sync`; pass `undefined` when no draft existed (next - * save omits `last_sync`, the backend's first-push branch). - */ /** Rows handed over for `query` so far, for a reader to hand back to `recordRemoteSync`. */ sendsSoFar(query: UserDraftLastSyncQuery): number { return sends.get(draftKey(query.workspace, query.itemKind, query.path)) ?? 0 }, + /** + * Seed the per-tab `last_sync` after an editor reads a draft from the + * server. Pass the response's `draft_saved_at` so the next save sends a + * matching `last_sync`; pass `undefined` when no draft existed (next + * save omits `last_sync`, the backend's first-push branch). Pass the + * `sendsSoFar` taken before the read as `since`: a row handed over after + * that puts the syncer ahead of this response, which is then ignored. + */ recordRemoteSync( query: UserDraftLastSyncQuery, draftSavedAt: string | undefined,