From d67a02e78551c3a5dbf06ebcf5fe2e32a72b291f Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 22 Sep 2026 12:19:23 -0400 Subject: [PATCH] fix(web): the tag saver no longer overwrites a concurrent tag change (BUG-3143) (#1432) The item pane's tag saver PATCHed the whole tag set with no version token. Another writer's tag change disappeared, with no conflict anywhere to retry on. Every tag batch now: - carries the version token of the row whose tags it last confirmed (seq via occTokenFor, as updateField sends), so a write that landed since is refused with a 409 instead of overwritten; - sends the burst's GESTURE applied to that row, rederiveListWrite(base, desired, confirmed), the same helper BUG-3038 added. It does this on EVERY batch, not only after a 409. The saver coalesces edits into absolute sets taken from a display that can lack the other writer's tag, and a batch that follows a 409 goes out with a current token, so a replay there would erase the tag with no conflict. The conflict path is a new top-level sendTagBatch: identityHeld before the refetch, the refetch, identityHeld again, advance, re-send. flushTagSaver keeps its original single-await shape and its BUG-3084 check. When a batch drains with nothing queued, the saver is rebased onto the returned row. The tag overlay would otherwise keep hiding the other writer's tag after the burst. Playwright: real server conflicts, one batch and two. Removing the token, re-deriving only on a 409, or dropping the rebase each turns a leg red. Claude-Session: https://claude.ai/code/session_01X4uYKs5ZPF28eCWfm8VPvE --- web/e2e/bug-3143-tag-saver-occ.spec.ts | 172 ++++++++++++++++++ .../lib/components/items/ItemDetail.svelte | 73 +++++++- .../itemDetailIdentityFence.round10.json | 6 +- .../items/itemDetailIdentityFence.round6.json | 4 +- .../items/itemDetailIdentityFenceAst.test.ts | 13 +- 5 files changed, 256 insertions(+), 12 deletions(-) create mode 100644 web/e2e/bug-3143-tag-saver-occ.spec.ts diff --git a/web/e2e/bug-3143-tag-saver-occ.spec.ts b/web/e2e/bug-3143-tag-saver-occ.spec.ts new file mode 100644 index 000000000..63dfde4a9 --- /dev/null +++ b/web/e2e/bug-3143-tag-saver-occ.spec.ts @@ -0,0 +1,172 @@ +import { test, expect } from './fixtures'; +import { browserLogin } from './lib/collab-helpers'; +import type { APIRequestContext, APIResponse, Page, Route } from '@playwright/test'; +import type { SuiteFixture } from './fixtures'; + +/** + * BUG-3143 — the item pane's tag saver must not overwrite a concurrent tag + * change. + * + * It used to PATCH the whole tag set with no version token, so another writer's + * tag disappeared with no conflict anywhere. It now sends the row's token (so a + * write that landed since is refused, 409) and sends the burst's GESTURE applied + * to the row that token names, on every batch — not only after a conflict, since + * a batch after a 409 goes out with a current token and would otherwise replay + * a display that never showed the other writer's tag. + * + * Every conflict here is REAL: the other writer's PATCH goes to the server from + * inside our request's route handler, before our request is let through. + */ + +const DESKTOP = { width: 1200, height: 900 }; + +function authHeaders(fixture: SuiteFixture) { + return { Authorization: `Bearer ${fixture.apiToken}`, 'Content-Type': 'application/json' }; +} + +async function seed(fixture: SuiteFixture, request: APIRequestContext, tag: string) { + const ws = `/api/v1/workspaces/${fixture.workspaceSlug}`; + const coll = await request.post(`${ws}/collections`, { + headers: authHeaders(fixture), + data: { name: `B3143 ${tag} ${Date.now()}`, prefix: 'BTG', schema: JSON.stringify({ fields: [] }) }, + }); + expect(coll.ok(), await coll.text()).toBeTruthy(); + const { slug: collSlug } = await coll.json(); + const created = await request.post(`${ws}/collections/${collSlug}/items`, { + headers: authHeaders(fixture), + data: { title: `B3143 ${tag} ${Date.now()}`, fields: JSON.stringify({}), content: '' }, + }); + expect(created.ok(), await created.text()).toBeTruthy(); + const item = (await created.json()) as { id: string; slug: string }; + const tagged = await request.patch(`${ws}/items/${item.id}`, { + headers: authHeaders(fixture), + data: { tags: JSON.stringify(['ta', 'tb']) }, + }); + expect(tagged.ok(), await tagged.text()).toBeTruthy(); + return { collSlug, item }; +} + +async function serverTags(fixture: SuiteFixture, request: APIRequestContext, id: string): Promise { + const row = await ( + await request.get(`/api/v1/workspaces/${fixture.workspaceSlug}/items/${id}`, { headers: authHeaders(fixture) }) + ).json(); + return JSON.parse(row.tags || '[]'); +} + +function tagChips(page: Page) { + return page.locator('.item-page-host > .item-page').locator('.tag-input .tag-chip'); +} + +function isTagPatch(route: Route, itemId: string) { + return ( + route.request().method() === 'PATCH' && + new URL(route.request().url()).pathname.endsWith(`/items/${itemId}`) && + 'tags' in JSON.parse(route.request().postData() ?? '{}') + ); +} + +test.describe('BUG-3143: the tag saver does not overwrite a concurrent tag change', () => { + test.beforeEach(async ({ page }, testInfo) => { + test.skip(testInfo.project.name !== 'desktop-chromium', 'one browser is enough for a write body'); + await page.setViewportSize(DESKTOP); + await browserLogin(page); + // No SSE: its re-read would refresh the pane before our edit and there + // would be no conflict. The conflict is the premise. + await page.route('**/api/v1/events*', (route) => route.abort()); + }); + + test('(a) removing a tag keeps a tag another writer added', async ({ page, fixture, request }) => { + const { collSlug, item } = await seed(fixture, request, 'a'); + const bodies: string[][] = []; + let otherWriterRan = false; + await page.route(`**/api/v1/workspaces/*/items/${item.id}`, async (route) => { + if (!isTagPatch(route, item.id)) return route.fallback(); + bodies.push(JSON.parse(JSON.parse(route.request().postData()!).tags)); + if (!otherWriterRan) { + otherWriterRan = true; + const other = await request.patch(`/api/v1/workspaces/${fixture.workspaceSlug}/items/${item.id}`, { + headers: authHeaders(fixture), + data: { tags: JSON.stringify(['ta', 'tb', 'tc']) }, + }); + expect(other.ok(), await other.text()).toBeTruthy(); + } + return route.fallback(); + }); + + await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}/${collSlug}/${item.slug}`); + await expect(tagChips(page)).toHaveCount(2); + const settled = page.waitForResponse( + (r) => r.request().method() === 'PATCH' && r.url().includes(`/items/${item.id}`) && r.ok(), + ); + await page.getByRole('button', { name: 'Remove tb' }).click(); + await settled; + + expect(otherWriterRan, 'PREMISE: the concurrent add ran').toBe(true); + expect(bodies[0], 'PREMISE: the first attempt was the gesture against the old set').toEqual(['ta']); + expect(await serverTags(fixture, request, item.id), 'BUG-3143: the tag save erased a concurrent tag').toEqual([ + 'ta', + 'tc', + ]); + // A chip's text carries its remove glyph, so match the tag at its start. + await expect(tagChips(page)).toHaveText([/^\s*ta\b/, /^\s*tc\b/]); + }); + + test('(b) a later batch of the same burst keeps it too', async ({ page, fixture, request }) => { + const { collSlug, item } = await seed(fixture, request, 'b'); + const bodies: string[][] = []; + let otherWriterRan = false; + let heldRetry: { route: Route; response: APIResponse } | null = null; + let retryHeld!: () => void; + const retryIsHeld = new Promise((r) => (retryHeld = r)); + await page.route(`**/api/v1/workspaces/*/items/${item.id}`, async (route) => { + if (!isTagPatch(route, item.id)) return route.fallback(); + bodies.push(JSON.parse(JSON.parse(route.request().postData()!).tags)); + if (!otherWriterRan) { + otherWriterRan = true; + const other = await request.patch(`/api/v1/workspaces/${fixture.workspaceSlug}/items/${item.id}`, { + headers: authHeaders(fixture), + data: { tags: JSON.stringify(['ta', 'tb', 'tc']) }, + }); + expect(other.ok(), await other.text()).toBeTruthy(); + return route.fallback(); + } + if (!heldRetry) { + // Batch 1's retry: it commits now, and its answer is held so the + // user's next edit coalesces into a SECOND batch of this burst. + heldRetry = { route, response: await route.fetch() }; + retryHeld(); + return; + } + return route.fallback(); + }); + + await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}/${collSlug}/${item.slug}`); + await expect(tagChips(page)).toHaveCount(2); + await page.getByRole('button', { name: 'Remove tb' }).click(); + await retryIsHeld; + expect(await serverTags(fixture, request, item.id), 'PREMISE: batch 1 re-derived and kept tc').toEqual([ + 'ta', + 'tc', + ]); + + // The display still shows the burst's own set, which never had tc. The + // user removes ta from it: the next batch's absolute set is []. + const batch2 = page.waitForResponse( + (r) => + r.request().method() === 'PATCH' && + r.url().includes(`/items/${item.id}`) && + r.ok() && + bodies.length >= 3, + ); + await page.getByRole('button', { name: 'Remove ta' }).click(); + const held = heldRetry as { route: Route; response: APIResponse } | null; + await held!.route.fulfill({ response: held!.response }); + await batch2; + + expect(bodies.length, 'PREMISE: the second edit went out as a second batch').toBeGreaterThanOrEqual(3); + expect(await serverTags(fixture, request, item.id), 'BUG-3143: a later batch erased a concurrent tag').toEqual([ + 'tc', + ]); + await expect(tagChips(page)).toHaveText([/^\s*tc\b/]); + }); +}); diff --git a/web/src/lib/components/items/ItemDetail.svelte b/web/src/lib/components/items/ItemDetail.svelte index ef9218f86..018040ffe 100644 --- a/web/src/lib/components/items/ItemDetail.svelte +++ b/web/src/lib/components/items/ItemDetail.svelte @@ -3385,6 +3385,15 @@ desired: string[]; // latest desired set (in-flight OR pending) for reload reapply running: boolean; confirmed: string; // last server-acknowledged tags JSON (revert target) + // BUG-3143: the set every send of this burst re-derives from. The row's + // tags when the burst started — the list the user's first gesture was made + // against — and never advanced: each `desired` is the display after ALL of + // the burst's gestures, so (base, desired) is the burst's whole delta. + base: string[]; + // The version token of the row whose tags are `confirmed`. Sent on every + // PATCH, so a write that landed since is REFUSED (409) rather than + // overwritten; before this the tag PATCH carried no token at all. + token: OCCToken; // The identity this burst was typed under (BUG-3084). A same-item reload // after an identity change passes every item-id check below, so the // drain would otherwise send the previous user's queued tags on the new @@ -3396,6 +3405,9 @@ // and coalesce into it, not spawn a second concurrent A saver. Per Codex // PR #659 round 6. const tagSavers = new Map(); + // Refetch-and-retry cycles per tag batch on a 409 — the same bound + // updateField uses for a field write. + const MAX_TAG_OCC_RETRIES = 2; function updateTags(newTags: string[]) { if (!item || !canEdit) return; @@ -3423,12 +3435,55 @@ desired: newTags, running: false, confirmed: targetItem.tags, // confirmed baseline captured at burst start + base: parseTags(targetItem), + token: occTokenFor(targetItem), epoch: captureIdentity() }; tagSavers.set(targetItem.id, saver); void flushTagSaver(saver); } + /** + * Send one batch of a tag burst, re-deriving it and retrying on a conflict + * (BUG-3143). Answers the row the write produced, or null when the identity + * changed inside the conflict path — the caller's own check then stops the + * drain. + * + * Every send carries the version token of the row whose tags are + * `confirmed`, so a write that landed since is REFUSED instead of + * overwritten; before this the tag PATCH carried no token. And every send is + * the burst's GESTURE applied to that row — rederiveListWrite(base, desired, + * confirmed) — not the absolute set the display produced: the display can + * lack a tag another writer added, and a batch after a 409 goes out with a + * CURRENT token, so replaying the display's set there would erase that tag + * with no conflict at all. With no other writer, confirmed == base on the + * first send and this is exactly `desired`. + * + * BUG-3084's invariant holds here as in the drain: the first PATCH is issued + * synchronously after the drain's own check; the conflict refetch is a + * request issued after an await, so it is checked BEFORE; and the re-send + * follows a check with no await between. + */ + async function sendTagBatch(saver: TagSaver): Promise { + for (let attempt = 0; ; attempt++) { + const toSave = rederiveListWrite(saver.base, saver.desired, parseTags({ tags: saver.confirmed })); + const token = saver.token; + try { + return await api.items.update(saver.ws, saver.itemId, { + tags: JSON.stringify(toSave), + ...(token.kind === 'seq' ? { expected_seq: token.value } : { expected_updated_at: token.value }) + }); + } catch (e) { + if (!isUpdateConflictError(e) || attempt >= MAX_TAG_OCC_RETRIES) throw e; + if (!identityHeld(saver.epoch)) return null; + const latest = await api.items.get(saver.ws, saver.itemId); + if (!identityHeld(saver.epoch)) return null; + saver.confirmed = latest.tags; + saver.token = occTokenFor(latest); + } + } + } + async function flushTagSaver(saver: TagSaver) { saver.running = true; saveStatus = 'saving'; @@ -3438,11 +3493,8 @@ // to the freeze (TagInput stays interactive via `readonly={!canEdit}`) // and a tag write is a side-independent single-item REST PATCH, so it // runs from either side; `updateTags` gates on canEdit (permission). - const toSave = saver.pending; saver.pending = null; - const fresh = await api.items.update(saver.ws, saver.itemId, { - tags: JSON.stringify(toSave) - }); + const fresh = await sendTagBatch(saver); // After the await and before BOTH the commit and the next send: a // second batch is only ever sent from here, with no await between // this check and it, so one check covers every send after the first @@ -3453,13 +3505,26 @@ saver.pending = null; return; } + // sendTagBatch answers null only when the identity was lost inside + // its conflict path, which the check above has already caught. + if (fresh === null) return; saver.confirmed = fresh.tags; + saver.token = occTokenFor(fresh); // Reconcile the UI to server truth only when nothing newer is // queued (avoids flicker) and we're still on this item. Route // through adoptServerItem so the tag PATCH echo can't clobber // unsaved editor content — its response carries the server's // `content`, and non-collab editors mirror item.content. Per // Codex PR #659 round 11. + if (saver.pending === null) { + // Nothing queued, so the burst's gesture is fully applied and + // the returned row IS its result — including any tag another + // writer added, which `desired` never had. Rebase the burst + // onto it: withInflightTags overlays `desired` on every snapshot + // while this saver runs, and the adopt below would otherwise + // hide that tag until the next refresh (BUG-3143). + saver.base = saver.desired = parseTags(fresh); + } if (saver.pending === null && item && item.id === saver.itemId) { item = adoptServerItem(fresh); } diff --git a/web/src/lib/components/items/itemDetailIdentityFence.round10.json b/web/src/lib/components/items/itemDetailIdentityFence.round10.json index fcd942ffc..854d2c871 100644 --- a/web/src/lib/components/items/itemDetailIdentityFence.round10.json +++ b/web/src/lib/components/items/itemDetailIdentityFence.round10.json @@ -74,7 +74,7 @@ ] }, { - "id": "R10-E a component-level class declaring a name a unit references (JSON, which flushTagSaver calls)", + "id": "R10-E a component-level class declaring a name a unit references (JSON, which sendTagBatch calls; flushTagSaver did before BUG-3143 moved the PATCH)", "subs": [ [ "\tlet loadGeneration = 0;\n", @@ -82,7 +82,7 @@ ] ], "refuses": [ - "flushTagSaver() has changed since its row was reviewed" + "sendTagBatch() has changed since its row was reviewed" ] }, { @@ -106,7 +106,7 @@ ] ], "refuses": [ - "flushTagSaver() has changed since its row was reviewed" + "sendTagBatch() has changed since its row was reviewed" ] } ] diff --git a/web/src/lib/components/items/itemDetailIdentityFence.round6.json b/web/src/lib/components/items/itemDetailIdentityFence.round6.json index 33ee61bc9..007ddaa7d 100644 --- a/web/src/lib/components/items/itemDetailIdentityFence.round6.json +++ b/web/src/lib/components/items/itemDetailIdentityFence.round6.json @@ -491,8 +491,8 @@ "class": 8, "subs": [ [ - "\t\t\t\t\ttags: JSON.stringify(toSave)\n\t\t\t\t});\n", - "\t\t\t\t\ttags: JSON.stringify(toSave)\n\t\t\t\t});\n\t\t\t\tsaver.epoch = captureIdentity();\n" + "\t\t\t\tconst fresh = await sendTagBatch(saver);\n", + "\t\t\t\tconst fresh = await sendTagBatch(saver);\n\t\t\t\tsaver.epoch = captureIdentity();\n" ] ], "refuses": [ diff --git a/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts b/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts index 090cf80e9..fcffa6dc5 100644 --- a/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts +++ b/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts @@ -117,10 +117,17 @@ const ASYNC_FUNCTIONS: Record = { }, }, flushTagSaver: { - reviewed: '04c1d764e777', - why: 'identityHeld(saver.epoch) before every commit and send; the unfenced writes are to this burst\'s own identity-stamped record, and the finally deletes that record only if the registry still holds it (the get)', + reviewed: 'c2b3dd381073', + why: 'identityHeld(saver.epoch) before every commit and send (each batch goes through sendTagBatch, which fences its own conflict path); the unfenced writes are to this burst\'s own identity-stamped record, and the finally deletes that record only if the registry still holds it (the get)', may: ['saver', 'tagSavers.get', 'tagSavers.delete'], }, + // BUG-3143: one tag batch, re-derived and retried on a conflict. Writes + // only this burst's own record (confirmed, token), like flushTagSaver. + sendTagBatch: { + reviewed: 'e0db17122ab9', + why: 'identityHeld(saver.epoch) before the conflict refetch and after it, so the re-send follows a check with no await between; answers null on a lost identity and the drain\'s own check stops', + may: ['saver'], + }, refreshCollectionIfMoved: { reviewed: '9d45f94352dd', why: 'gen against loadGeneration after the fetch' }, loadTagSuggestions: { reviewed: 'b03a62bf7294', why: 'identityHeld after the fetch; the identity listener re-runs it' }, stampSourceUrl: { reviewed: 'b9d3f9d400ae', why: 'switchedAway on both arms' }, @@ -1530,7 +1537,7 @@ describe('ItemDetail identity gate: a fenced unit cannot change without a re-rea // not. const anchor = '\tfunction identityHeld(captured: number): boolean {\n'; expect(SOURCE.split(anchor).length - 1).toBe(1); - const call = '\t\t\t\tconst fresh = await api.items.update(saver.ws, saver.itemId, {\n'; + const call = '\t\t\t\tconst fresh = await sendTagBatch(saver);\n'; expect(SOURCE.split(call).length - 1).toBe(1); const base = SOURCE.replace( anchor,