mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
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
This commit is contained in:
xarmian
@@ -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<string[]> {
|
||||
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<void>((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/]);
|
||||
});
|
||||
});
|
||||
@@ -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<string, TagSaver>();
|
||||
// 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<Item | null> {
|
||||
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);
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
]
|
||||
}
|
||||
]
|
||||
|
||||
@@ -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": [
|
||||
|
||||
@@ -117,10 +117,17 @@ const ASYNC_FUNCTIONS: Record<string, Row> = {
|
||||
},
|
||||
},
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user