diff --git a/web/src/lib/attachments/viewFence.test.ts b/web/src/lib/attachments/viewFence.test.ts new file mode 100644 index 00000000..8da9b7f6 --- /dev/null +++ b/web/src/lib/attachments/viewFence.test.ts @@ -0,0 +1,226 @@ +import { describe, it, expect } from 'vitest'; +import { viewIdentity, createFence, createPaintFence } from './viewFence'; + +// The invariant these fences encode failed four review rounds running, always +// by comparing only PART of the identity. These tests pin the module directly: +// the identity rules, then each of the three fences, then the two collapses +// that a prior round proved are NOT safe. + +describe('viewIdentity', () => { + it('is the whole record, not any one part', () => { + let ws = 'ws-a'; + let item = 'item-1'; + const view = viewIdentity(() => ({ ws, item })); + + const token = view.capture(); + expect(token.changed()).toBe(false); + + // The item half. + item = 'item-2'; + expect(token.changed()).toBe(true); + item = 'item-1'; + expect(token.changed()).toBe(false); + + // The workspace half — the one that kept getting forgotten. + ws = 'ws-b'; + expect(token.changed()).toBe(true); + }); + + it('is not addressable while any part is missing', () => { + let item: string | null = null; + const view = viewIdentity(() => ({ ws: 'ws-a', item })); + + expect(view.key()).toBeNull(); + // A half-identified view must not pass a fence just because the half it + // does know still matches. + const blank = view.capture(); + expect(blank.changed()).toBe(true); + + item = ''; + expect(view.key()).toBeNull(); + + item = 'item-1'; + expect(view.key()).not.toBeNull(); + expect(view.capture().changed()).toBe(false); + }); + + it('never matches a null key', () => { + const view = viewIdentity(() => ({ ws: 'ws-a' })); + expect(view.matches(null)).toBe(false); + expect(view.matches(view.key())).toBe(true); + }); + + it('cannot confuse two different splits of the same characters', () => { + const a = viewIdentity(() => ({ ws: 'x', item: 'yz' })).key(); + const b = viewIdentity(() => ({ ws: 'xy', item: 'z' })).key(); + expect(a).not.toBe(b); + }); + + it('cannot be collided by parts that embed the serialiser\'s own syntax', () => { + // The specific way a delimiter join fails: a part containing the entry + // separator FOLLOWED BY the next part's name and the name/value + // separator re-creates the exact byte sequence the join itself emits, so + // two DIFFERENT identities serialise the same — and a stale fence passes, + // which is the failure this module exists to prevent (Codex round 1). + // Reproduced for the NUL/SOH join this module used to use, and for + // separators built from the characters JSON has to escape. + const shapes: [string, string][] = [ + [String.fromCharCode(0), String.fromCharCode(1)], + ['|', ':'], + ['"', ','], + ['\\', '"'], + [']', '['], + ]; + for (const [entrySep, pairSep] of shapes) { + const a = viewIdentity(() => ({ item: `x${entrySep}ws${pairSep}y`, ws: 'z' })).key(); + const b = viewIdentity(() => ({ item: 'x', ws: `y${entrySep}ws${pairSep}z` })).key(); + expect(a).not.toBeNull(); + expect(a).not.toBe(b); + } + + // A join that drops the part NAMES collides without any crafted run at + // all: `xy` + `z` and `x` + `yz` are the same list of values. + for (const sep of [',', '|', String.fromCharCode(0)]) { + expect(viewIdentity(() => ({ item: `x${sep}y`, ws: 'z' })).key()).not.toBe( + viewIdentity(() => ({ item: 'x', ws: `y${sep}z` })).key() + ); + } + }); + + it('does not depend on the order the parts are written in', () => { + const a = viewIdentity(() => ({ ws: 'w', item: 'i' })).key(); + const b = viewIdentity(() => ({ item: 'i', ws: 'w' })).key(); + expect(a).toBe(b); + }); + + it('snapshots the parts, so a request reads back what it was ISSUED for', () => { + let ws = 'ws-a'; + const view = viewIdentity(() => ({ ws })); + const token = view.capture(); + ws = 'ws-b'; + // This is what stops a continuation from calling the API — or keying a + // cache invalidation — with whichever workspace happens to be live. + expect(token.value.ws).toBe('ws-a'); + }); +}); + +describe('createFence (fences 1 and 2)', () => { + it('stales a token once the fence restarts — same identity', () => { + const view = viewIdentity(() => ({ ws: 'ws-a', item: 'item-1' })); + const fence = createFence(view); + + const first = fence.begin(); + expect(first.stale()).toBe(false); + const second = fence.restart(); + // A→A: the identity matches, so only the generation can tell a + // superseded request from the current one. + expect(first.stale()).toBe(true); + expect(second.stale()).toBe(false); + }); + + it('stales a token once the identity moves — same generation', () => { + let ws = 'ws-a'; + const fence = createFence(viewIdentity(() => ({ ws }))); + + const token = fence.begin(); + expect(token.stale()).toBe(false); + ws = 'ws-b'; + expect(token.stale()).toBe(true); + }); + + it('stales across an A→B→A round trip, where the identity matches again', () => { + let ws = 'ws-a'; + const fence = createFence(viewIdentity(() => ({ ws }))); + + const first = fence.restart(); + ws = 'ws-b'; + fence.restart(); + ws = 'ws-a'; + const current = fence.restart(); + + // The identity is back to A, so the identity compare alone would let + // A's first response write over A's current one. + expect(first.stale()).toBe(true); + expect(current.stale()).toBe(false); + }); + + it('begin() does not supersede its siblings; restart() does', () => { + const fence = createFence(viewIdentity(() => ({ ws: 'ws-a' }))); + + // Two concurrent mutations must both still be allowed to reconcile. + const one = fence.begin(); + const two = fence.begin(); + expect(one.stale()).toBe(false); + expect(two.stale()).toBe(false); + + fence.invalidate(); + expect(one.stale()).toBe(true); + expect(two.stale()).toBe(true); + }); + + it('keeps the request and view fences independent', () => { + // The whole reason there are two: a Retry restarts the REQUEST fence + // (superseding its own in-flight listing) without touching the VIEW + // fence, so a delete of a row still on screen keeps reconciling. + const view = viewIdentity(() => ({ ws: 'ws-a', item: 'item-1' })); + const requests = createFence(view); + const views = createFence(view); + + const del = views.begin(); + requests.restart(); // the Retry + expect(del.stale()).toBe(false); + + views.invalidate(); // a real switch, or unmount + expect(del.stale()).toBe(true); + }); +}); + +describe('createPaintFence (fence 3)', () => { + it('answers about the DOM, lagging the live props on purpose', () => { + let ws = 'ws-a'; + const view = viewIdentity(() => ({ ws })); + const paint = createPaintFence(view); + + expect(paint.isCurrent()).toBe(false); // nothing painted yet + paint.record(view.capture()); + expect(paint.isCurrent()).toBe(true); + + // Props update synchronously; the effect that repaints flushes later. + // In that window the controls on screen still belong to ws-a, and a + // click on one must be refused. + ws = 'ws-b'; + expect(paint.isCurrent()).toBe(false); + expect(paint.painted()?.value.ws).toBe('ws-a'); + + paint.record(view.capture()); + expect(paint.isCurrent()).toBe(true); + }); + + it('claims nothing for an un-addressable view', () => { + let item: string | null = null; + const view = viewIdentity(() => ({ ws: 'ws-a', item })); + const paint = createPaintFence(view); + + paint.record(view.capture()); + expect(paint.painted()).toBeNull(); + expect(paint.isCurrent()).toBe(false); + + // And a run that bails on a missing id stops claiming the previous view. + item = 'item-1'; + paint.record(view.capture()); + expect(paint.isCurrent()).toBe(true); + item = null; + paint.record(view.capture()); + expect(paint.painted()).toBeNull(); + }); + + it('is not a generation fence: repainting the same view stays current', () => { + const view = viewIdentity(() => ({ ws: 'ws-a' })); + const paint = createPaintFence(view); + const first = view.capture(); + paint.record(first); + paint.record(view.capture()); + expect(paint.isCurrent()).toBe(true); + expect(first.changed()).toBe(false); + }); +}); diff --git a/web/src/lib/attachments/viewFence.ts b/web/src/lib/attachments/viewFence.ts new file mode 100644 index 00000000..00298ba3 --- /dev/null +++ b/web/src/lib/attachments/viewFence.ts @@ -0,0 +1,207 @@ +/** + * The view-identity fence — ONE implementation of the invariant that a + * continuation resuming after an `await` may only write state belonging to the + * view the user is still looking at. + * + * WHY THIS EXISTS. Attachment surfaces (the item attachment strip, the Storage + * tab) stay MOUNTED across the navigations that change what they show: the + * strip lives outside ItemDetail's `{#key itemSlug}` block, and + * `/{user}/{ws}/settings` is one SvelteKit route, so a workspace switch changes + * `wsSlug` under a mounted tab. Every fetch, mutation and timer therefore has to + * say which view it belongs to. Written out by hand at each call site, that + * invariant failed the same way four review rounds running — always by + * comparing only PART of the identity, almost always by forgetting the + * workspace half. This module makes the identity a single value that is read in + * exactly one place per component and carried on a token, so no call site can + * assemble a partial one. + * + * THREE FENCES, THREE QUESTIONS. They are deliberately NOT collapsible into one + * — a prior review round established that merging any two loses either A→B + * suppression or same-item-Retry reconciliation: + * + * 1. REQUEST fence (`createFence`, restarted per request) — "is this RESPONSE + * still the current one?" A Retry supersedes its own predecessor here. + * 2. VIEW fence (`createFence`, invalidated only when the view really + * changes) — "may this async CONTINUATION still reconcile local state?" A + * Retry is the same view reloading and must NOT invalidate it, or an + * in-flight delete of a row still on screen stops rolling back. + * 3. PAINT-TIME fence (`createPaintFence`) — "does the CONTROL the user + * clicked belong to what is on screen?" Props update synchronously and + * effects flush later, so in between the DOM still shows the previous + * view. This one has to be at ENTRY: the other two run after an await, and + * no fence can unsend a request. + * + * Both (1) and (2) compare a captured GENERATION as well as the identity, + * because a counter alone misses A→B→A (the identity matches again) and an + * identity alone misses A→A (a superseded request for the same view). + */ + +/** + * The parts that name a view. A record rather than a tuple so the captured + * snapshot can be read back by name — `token.value.ws` is the workspace the + * request was ISSUED for, which is what callers should pass to the API instead + * of re-reading the live prop. + */ +export type IdentityParts = Record; + +/** + * Serialise the parts into a comparable key, or null when the view is not + * addressable yet. + * + * A MISSING PART MAKES THE WHOLE KEY NULL. That is the load-bearing rule: a + * view named by (workspace, item) is not identifiable while either half is + * unknown, and a null key never matches anything — so a half-identified view + * can't accidentally pass a fence. + */ +function serialize(parts: IdentityParts): string | null { + // Sorted so the key does not depend on property declaration order. + const names = Object.keys(parts).sort(); + if (names.length === 0) return null; + const pairs: [string, string][] = []; + for (const name of names) { + const value = parts[name]; + if (value === null || value === undefined || value === '') return null; + pairs.push([name, value]); + } + // JSON rather than a delimiter join: a hand-rolled separator has to be a + // character the parts cannot contain, and "identity parts are always slugs + // and uuids" is an assumption this module has no way to enforce. JSON quotes + // and escapes both halves, so no two distinct part sets can serialise the + // same — a collision here would silently let a stale fence pass, which is + // the exact failure this module exists to prevent (Codex round 1). + return JSON.stringify(pairs); +} + +/** A captured identity: what the view was named when this token was taken. */ +export interface IdentityToken { + /** Snapshot of the parts, for reading back the workspace/item that was captured. */ + readonly value: T; + /** The comparable key, or null when the view wasn't addressable. */ + readonly key: string | null; + /** + * True when the live view no longer matches this capture — including when + * the capture itself was never addressable. + */ + changed(): boolean; +} + +export interface ViewIdentity { + /** The live key, or null when a part is missing. */ + key(): string | null; + /** True when `key` names the live view. A null key never matches. */ + matches(key: string | null): boolean; + /** Take a token for the live view. */ + capture(): IdentityToken; +} + +/** + * Declare what names a view. `read` is called on every capture/compare and must + * read the LIVE reactive values: + * + * const view = viewIdentity(() => ({ ws: wsSlug, item: itemId })); + * + * This is the ONE place a component states its identity; every fence below is + * built from it, so there is no second call site that could state a shorter one. + */ +export function viewIdentity(read: () => T): ViewIdentity { + const identity: ViewIdentity = { + key: () => serialize(read()), + matches: (key) => key !== null && key === serialize(read()), + capture: () => { + const value = { ...read() } as T; + const key = serialize(value); + return { + value, + key, + changed: () => key === null || !identity.matches(key), + }; + }, + }; + return identity; +} + +/** An identity token that also carries a generation. */ +export interface FenceToken extends IdentityToken { + /** + * True when this token may no longer write: the fence was invalidated (or + * restarted) since it was taken, or the view moved on. + */ + stale(): boolean; +} + +export interface Fence { + /** + * Take a token WITHOUT superseding outstanding ones. For the view fence, and + * for anything that must coexist with its siblings — two concurrent deletes + * must not invalidate each other. + */ + begin(): FenceToken; + /** + * Supersede every outstanding token, then take a new one. For a request + * fence: issuing request N+1 means request N's response may no longer write. + */ + restart(): FenceToken; + /** Supersede every outstanding token without taking a new one. */ + invalidate(): void; +} + +/** + * A generation + identity fence. Two of these per surface: one restarted per + * request (fence 1) and one invalidated only on a real view change (fence 2). + */ +export function createFence(identity: ViewIdentity): Fence { + let generation = 0; + + function take(): FenceToken { + const captured = identity.capture(); + const gen = generation; + return { + value: captured.value, + key: captured.key, + changed: captured.changed, + stale: () => gen !== generation || captured.changed(), + }; + } + + return { + begin: take, + restart: () => { + generation++; + return take(); + }, + invalidate: () => { + generation++; + }, + }; +} + +export interface PaintFence { + /** + * Claim what is now on screen. Recording a token whose key is null (an + * un-addressable view) correctly claims nothing. + */ + record(token: IdentityToken | null): void; + /** The identity the last paint claimed, for reading the parts back. */ + painted(): IdentityToken | null; + /** True when what is painted is what the live props name. */ + isCurrent(): boolean; +} + +/** + * Fence 3. Deliberately LAGS the live props by the prop-update → effect-flush + * window: that lag is the only thing that can answer "was this control rendered + * for the view that is on screen NOW?", which the live props cannot — they + * already read the NEW view while the OLD controls are still mounted. + */ +export function createPaintFence( + identity: ViewIdentity +): PaintFence { + let painted: IdentityToken | null = null; + return { + record: (token) => { + painted = token && token.key !== null ? token : null; + }, + painted: () => painted, + isCurrent: () => painted !== null && identity.matches(painted.key), + }; +} diff --git a/web/src/lib/components/items/ItemAttachmentStrip.svelte b/web/src/lib/components/items/ItemAttachmentStrip.svelte index f9f9b736..c89d52a4 100644 --- a/web/src/lib/components/items/ItemAttachmentStrip.svelte +++ b/web/src/lib/components/items/ItemAttachmentStrip.svelte @@ -19,31 +19,22 @@ * block, so this component PERSISTS across an A→B item switch, per the * no-{#key} bug class from PLAN-2105 / TASK-2112. * - * THE FENCE MODEL. Identity is the PAIR (workspace, item) everywhere — see - * `viewKey`. There are exactly three fences, because there are exactly - * three distinct questions, each asked at a different moment: + * THE FENCE MODEL lives in `$lib/attachments/viewFence`, which owns the + * whole invariant — read its header for why there are exactly three fences + * and why they cannot be collapsed. This component supplies the identity + * once (`view`, the PAIR (workspace, item)) and builds all three from it: * - * 1. `switchedAway(gen, item, ws)` — "is this RESPONSE still current?" - * Guards every await-then-write inside the load effect, against the - * REQUEST generation. A Retry supersedes its own predecessor here. - * 2. `viewChanged(gen, item, ws)` — "may this async CONTINUATION still - * reconcile local state?" Guards mutation continuations, against the - * VIEW generation, which a Retry deliberately does not bump: a delete - * of a row still on screen must roll back and toast even if the user - * hit Retry while it was in flight. - * 3. PAINT-TIME identity — "does the CONTROL the user clicked belong to - * what is on screen?" Props update synchronously and effects flush - * later, so in between, the DOM still shows the previous view. Every - * entry point reached from a rendered control validates against an - * identity RECORDED WHEN THE EFFECT PAINTED (`paintedView`), never - * against the live props. Both control entry points use it — - * `handleDelete` for a tile, `retryLoad` for the error row. This fence - * has to be at ENTRY: the other two run after an await, and no fence - * can unsend a request. - * - * (1) and (2) compare a captured generation AND the pair, because a - * generation counter alone misses A→B→A and the pair alone misses A→B→A. - * (3) is a pure pair compare: it asks about the DOM, not about ordering. + * 1. `loadFence` — "is this RESPONSE still current?" Restarted per + * request; guards every await-then-write inside the load effect. + * 2. `viewFence` — "may this async CONTINUATION still reconcile local + * state?" Invalidated only when the view really changes, so a Retry + * (the same view reloading) leaves it alone: a delete of a row still + * on screen must roll back and toast even if the user hit Retry while + * it was in flight. + * 3. `paint` — "does the CONTROL the user clicked belong to what is on + * screen?" Both control entry points fence on it — `handleDelete` for + * a tile, `retryLoad` for the error row — at ENTRY, because the other + * two run after an await and no fence can unsend a request. */ import { onDestroy, untrack } from 'svelte'; import { api, PadApiError } from '$lib/api/client'; @@ -59,6 +50,7 @@ registerAttachmentDeletionListener, registerAttachmentUploadListener, } from '$lib/attachments/events'; + import { viewIdentity, createFence, createPaintFence } from '$lib/attachments/viewFence'; interface Props { wsSlug: string; @@ -175,43 +167,36 @@ */ let retryRequestedFor: string | null = null; /** - * View identity key. `itemId` alone isn't it: `wsSlug` is reactive and the - * strip stays mounted across a workspace change, so keying on the item - * would classify that as a same-view Retry — leaving the previous - * workspace's rows painted and its in-flight mutations still reconciling - * (Codex fresh-angle round 2). + * The ONE statement of what names this view. `itemId` alone isn't it: + * `wsSlug` is reactive and the strip stays mounted across a workspace + * change, so keying on the item would classify that as a same-view Retry — + * leaving the previous workspace's rows painted and its in-flight mutations + * still reconciling (Codex fresh-angle round 2). Declared once here so no + * individual fence call site can restate a shorter identity; every fence + * below derives from it, and every captured workspace is read back off a + * token rather than off the live prop. */ - function viewKey(ws: string, id: string): string { - return `${ws}\u0000${id}`; - } - + const view = viewIdentity(() => ({ ws: wsSlug, item: itemId })); + /** Fence 1 — request generation. Restarted on every (re)run of the load effect. */ + const loadFence = createFence(view); /** - * VIEW the currently painted tiles belong to — written by the load effect, - * so it lags the live props by exactly the prop-update → effect-flush - * window. That lag is the point: it is the only thing that can answer "was - * this control rendered for the view that is on screen NOW?", which the - * live props cannot (they already read the NEW view while the OLD tiles are - * still mounted). Entry fence for `handleDelete` — the `viewChanged` check - * in its catch runs after the await and cannot unsend a DELETE aimed at the - * wrong item or workspace (final review round 2). Non-reactive on purpose: - * read at click time, it must not re-trigger the render it describes. + * Fence 2 — view generation. Invalidated only when the view actually + * changes: a different (item, workspace), or unmount. A Retry is the SAME + * item reloading, so it leaves this alone: a delete of a row still on screen + * must reconcile (roll back, toast) even if the user hit Retry while it was + * in flight. Fencing mutations on the REQUEST generation instead made a + * Retry masquerade as an item switch and swallowed the failure. */ - let paintedView: string | null = null; + const viewFence = createFence(view); + /** + * Fence 3 — the view the currently painted tiles belong to. Recorded by the + * load effect, so it lags the live props by exactly the prop-update → + * effect-flush window; that lag is what makes it the only thing that can + * answer "was this control rendered for the view that is on screen NOW?" + */ + const paint = createPaintFence(view); + - // Two generations, because "which request is this?" and "which item is on - // screen?" are different questions and a Retry answers them differently - // (final-review P2). - // - // REQUEST generation — bumped on every (re)run of the fetch effect, - // including a Retry, so a superseded list() response can never write. - let loadGeneration = 0; - // VIEW generation — bumped only when the view actually changes: a different - // (item, workspace), or unmount. A Retry is the SAME item reloading, so it - // leaves this alone. Mutations fence on this: a delete of a row still on - // screen must reconcile (roll back, toast, broadcast) even if the user hit - // Retry while it was in flight. Fencing them on the request generation made - // a Retry masquerade as an item switch and swallowed the failure. - let viewGeneration = 0; // Ids confirmed deleted while this item's list was loading. A deletion // broadcast only filters the CURRENT array, so a list() response that was @@ -240,18 +225,19 @@ let pendingUploads: StripAttachment[] = []; $effect(() => { - const reqItemId = itemId; - const reqWsSlug = wsSlug; + // Restarting the request fence supersedes any response still in flight + // AND captures the identity this run belongs to. Reading the identity + // here is what makes (workspace, item) the effect's dependencies. + const req = loadFence.restart(); + const { ws: reqWsSlug, item: reqItemId } = req.value; // Read (and discard) so Retry re-runs this effect. The value never // matters — only the dependency does. void retryNonce; - const gen = ++loadGeneration; // Claim the retry marker whether or not this run goes on to fetch, so a // stale claim can't leak into a later, unrelated load. Read BEFORE the // reset below, which branches on it. - const isRetry = - retryRequestedFor !== null && retryRequestedFor === viewKey(reqWsSlug, reqItemId ?? ''); + const isRetry = retryRequestedFor !== null && retryRequestedFor === req.key; retryRequestedFor = null; // Clear synchronously on switch. Without this, A's tiles stay painted @@ -267,12 +253,13 @@ // Whatever this run paints — rows, error row, or nothing — belongs to // this (workspace, item). Recorded unconditionally: a Retry repaints // the SAME view, so the value is unchanged, and a run that bails - // below on a missing id must still stop claiming the previous view. - paintedView = reqItemId && reqWsSlug ? viewKey(reqWsSlug, reqItemId) : null; + // below on a missing id must still stop claiming the previous view + // (an un-addressable token records nothing). + paint.record(req); if (!isRetry) { // The view itself changed (new item / workspace), so in-flight // mutations captured against the old one must stop reconciling. - viewGeneration++; + viewFence.invalidate(); attachments = []; expanded = false; lightbox = null; @@ -290,7 +277,7 @@ // deferred write in this component. let loadingTimer: ReturnType | null = setTimeout(() => { loadingTimer = null; - if (switchedAway(gen, reqItemId, reqWsSlug)) return; + if (req.stale()) return; showLoading = true; }, LOADING_DELAY_MS); function stopLoadingMarker() { @@ -301,18 +288,56 @@ } void (async () => { + // Ids the pending buffer ALREADY held when this request went out. A + // response can be authoritative about exactly these — the row existed + // before the GET, so its absence is proof the row is gone (deleted by + // another tab or another user, which the in-process event bus cannot + // see). Merging them back in resurrected such rows, and because + // nothing ever consumed the buffer it did so on every subsequent + // load, forever (final review round 4). + // + // Entries announced AFTER this point are the buffer's actual purpose + // and are never touched: the GET may predate them, so their absence + // from the response says nothing. + const pendingAtRequest = new Set(pendingUploads.map((a) => a.id)); try { const res = await api.attachments.list(reqWsSlug, { item_id: reqItemId, limit: MAX_FETCH, }); - if (switchedAway(gen, reqItemId, reqWsSlug)) return; + if (req.stale()) return; const raw = res.attachments ?? []; const rows = raw.filter((a) => !deletedIds.has(a.id)).map(toStripAttachment); const seen = new Set(rows.map((a) => a.id)); + // ...but only a COMPLETE page is authoritative about absence. The + // request is bounded at MAX_FETCH, so a truncated page may simply + // not reach a row that is still perfectly alive — and retiring a + // live upload would delete a good tile permanently, which is a + // worse failure than the resurrection this is fixing (Codex + // round 2). `total` is the server's own statement of how many + // live rows there are and `offset` is always 0 here, so a page + // that holds all of them IS complete even when it is exactly + // MAX_FETCH long (Codex round 3). Only when the response omits + // `total` does the page length have to stand in for it. + const pageIsComplete = + typeof res.total === 'number' ? res.total <= raw.length : raw.length < MAX_FETCH; + const covered = pageIsComplete ? pendingAtRequest : new Set(); const missed = pendingUploads.filter( - (a) => !seen.has(a.id) && !deletedIds.has(a.id) + (a) => !seen.has(a.id) && !deletedIds.has(a.id) && !covered.has(a.id) ); + // Consume what this response covered. The exclusion above already + // makes every LATER load ignore these ids (a later request's own + // snapshot would contain them too), so this is the retention half + // of the fix rather than a second correctness gate: without it the + // buffer only ever grows, holding rows that nothing will ever read + // again. Deliberately not separately observable — a mutation that + // deletes this line survives the suite, and that is expected. + // + // Only on SUCCESS, and only for what the page COVERED: a failure + // is not authoritative, the catch arm below still needs the buffer + // to repaint the rows a failed listing would otherwise hide, and a + // truncated page proves nothing about what it didn't reach. + pendingUploads = pendingUploads.filter((a) => !covered.has(a.id)); // Cap the MERGE, not just the response: `missed` rides on top of // up to MAX_FETCH server rows (DR-11). const merged = [...missed, ...rows]; @@ -364,7 +389,7 @@ // roleLevel("guest") is below viewer, so they 403. That gap is // pre-existing (inline images are already broken for them) and // is tracked as BUG-2386, not absorbed here — PLAN-2382 DR-4b. - if (switchedAway(gen, reqItemId, reqWsSlug)) return; + if (req.stale()) return; // Keep anything uploaded while this request was in flight: the // upload SUCCEEDED, so dropping it would hide a row the editor // and server both have, until a remount (Codex review round 2). @@ -372,7 +397,7 @@ loadFailed = true; } finally { stopLoadingMarker(); - if (!switchedAway(gen, reqItemId, reqWsSlug)) showLoading = false; + if (!req.stale()) showLoading = false; } })(); @@ -387,7 +412,7 @@ // view generation here would put the Retry bug straight back. Unmount // is handled by onDestroy below, which runs only on destroy. return () => { - loadGeneration++; + loadFence.invalidate(); stopLoadingMarker(); }; }); @@ -395,7 +420,7 @@ // Destroy invalidates the VIEW too — an in-flight delete that resolves // after the component is gone must not toast or write. onDestroy(() => { - viewGeneration++; + viewFence.invalidate(); }); /** @@ -410,7 +435,6 @@ * uploaded while the request was in flight. */ function retryLoad() { - if (!itemId || !wsSlug) return; // The same ENTRY fence the delete control uses (fence 3): the painted // error must belong to the view that is on screen NOW. If the parent has // already swapped `itemId` or `wsSlug` and this effect hasn't flushed @@ -418,13 +442,19 @@ // its way, and honouring the retry would preserve the previous view's // rows across the switch (Codex round 11). // - // `loadFailed` is only ever set by a response that passed `switchedAway` - // under the run that also wrote `paintedView`, so the two together say - // exactly what a separate `loadFailedFor` field used to: WHICH view's - // failure is painted. One paint-time identity, not two kept in step. - if (!loadFailed || paintedView !== viewKey(wsSlug, itemId)) return; - for (const a of attachments) invalidateAttachmentMetadata(wsSlug, a.id); - retryRequestedFor = viewKey(wsSlug, itemId); + // `loadFailed` is only ever set by a response that passed the request + // fence under the run that also recorded the paint, so the two together + // say exactly what a separate `loadFailedFor` field used to: WHICH + // view's failure is painted. One paint-time identity, not two kept in + // step. + if (!loadFailed || !paint.isCurrent()) return; + const painted = paint.painted(); + if (!painted) return; + // Workspace read back off the PAINTED identity, not the live prop: + // `isCurrent()` has just established they agree, and taking it from the + // token is what stops the two from ever drifting apart again. + for (const a of attachments) invalidateAttachmentMetadata(painted.value.ws, a.id); + retryRequestedFor = painted.key; retryNonce++; } @@ -504,38 +534,6 @@ }); }); - // Mirrors ItemDetail's `switchedAway`: the generation catches a newer load, - // the identity compare closes the A→B→A gap where generations could - // otherwise line up. - // - // Identity is the (workspace, item) PAIR, exactly as `viewChanged` and - // `viewKey` treat it. Requests already capture the workspace they were - // issued against; checking only the item let a same-item workspace switch - // pass this fence, so a superseded response, failure or loading timer could - // write into the new view during the prop-update → effect-flush window - // (final review round 2). - // - // Deliberately untested, unlike the other two fences: a workspace change is - // a dependency of the load effect, and Svelte's scheduler always flushes - // that re-run — which clears every one of these fields — before a pending - // response continuation gets to write. So the omission has no observable - // symptom to pin a test to. It is fixed because "the next effect run happens - // to overwrite it" is a scheduling accident, not a fence, and because - // identity being the (workspace, item) pair should not have exceptions. - function switchedAway(gen: number, reqItemId: string, reqWsSlug: string): boolean { - return gen !== loadGeneration || itemId !== reqItemId || wsSlug !== reqWsSlug; - } - - /** - * The mutation fence. Same shape as `switchedAway`, but against the VIEW - * generation — a Retry re-runs the load without changing what is on screen, - * so an in-flight delete for this item must still reconcile (final-review - * P2). The id compare closes the A→B→A gap the counter alone can miss. - */ - function viewChanged(gen: number, reqItemId: string, reqWsSlug: string): boolean { - return gen !== viewGeneration || itemId !== reqItemId || wsSlug !== reqWsSlug; - } - let visible = $derived(expanded ? attachments : attachments.slice(0, COLLAPSED_TILES)); // Overflow is derived from the FETCHED ROWS, never the response's `total` // (DR-9) — otherwise an item with >50 attachments advertises a count that @@ -640,32 +638,30 @@ async function handleDelete(att: StripAttachment) { if (!canDelete) return; - const reqItemId = itemId; - const reqWsSlug = wsSlug; - if (!reqItemId || !reqWsSlug) return; // ENTRY fence (fence 3 — see the header). The clicked tile was painted - // for `paintedView`; the live props may already name a different view, - // because they update synchronously and the load effect that repaints - // these tiles flushes later. In that window a click on a stale tile - // would send a DELETE while the user is looking at another item or - // workspace — and the `viewChanged` check in the catch below runs after - // the request, so it can suppress the rollback but cannot unsend the - // request (final review round 2). The only fix is to refuse here, - // before the confirm and before the call. - if (paintedView !== viewKey(reqWsSlug, reqItemId)) return; + // for `paint`'s identity; the live props may already name a different + // view, because they update synchronously and the load effect that + // repaints these tiles flushes later. In that window a click on a stale + // tile would send a DELETE while the user is looking at another item or + // workspace — and the view-fence check in the catch below runs after the + // request, so it can suppress the rollback but cannot unsend the request + // (final review round 2). The only fix is to refuse here, before the + // confirm and before the call. + if (!paint.isCurrent()) return; if (typeof window !== 'undefined' && !window.confirm(confirmMessage(att))) return; // window.confirm blocks the thread, so nothing can have moved between // the fence above and here. - // Capture identity BEFORE the await: a switch mid-delete must not roll - // the tile back into a DIFFERENT item's strip, and must not toast over - // it. The DELETE itself still lands — it targets an id, not a view. - // `wsSlug` is captured for the same reason: it is reactive, and the - // broadcast + metadata-cache key must name the workspace the DELETE - // actually targeted, not whichever one is current when it resolves - // (Codex round 6). - const gen = viewGeneration; + // Capture identity BEFORE the await (fence 2): a switch mid-delete must + // not roll the tile back into a DIFFERENT item's strip, and must not + // toast over it. The DELETE itself still lands — it targets an id, not a + // view. The workspace comes off the token for the same reason: it is + // reactive, and the request + broadcast + metadata-cache key must name + // the workspace the DELETE actually targeted, not whichever one is + // current when it resolves (Codex round 6). + const req = viewFence.begin(); + const reqWsSlug = req.value.ws; const index = attachments.findIndex((a) => a.id === att.id); // Optimistic removal. @@ -703,7 +699,7 @@ return; } - if (viewChanged(gen, reqItemId, reqWsSlug)) return; + if (req.stale()) return; // Someone else announced this deletion while our own call was in // flight — the row is gone regardless of why ours failed, so don't diff --git a/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts b/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts index 299a64c3..8cd0b5ff 100644 --- a/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts +++ b/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts @@ -1264,6 +1264,115 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n.includes('old1.png'))).toBe(true); }); + it('lets a later load retire a pending upload deleted outside this tab', async () => { + // `pendingUploads` was retained forever and no successful response ever + // consumed it, so it re-merged onto EVERY later load. The event bus is + // process-local, so a deletion from another tab or another user, followed + // by a load that legitimately returns no row, resurrected the tile — and + // kept resurrecting it (final review round 4). + // + // The rule: a response is authoritative about the entries the buffer + // already held when that request went OUT. Entries announced while it was + // in flight are the buffer's actual purpose and stay. + listMock.mockRejectedValueOnce(new Error('offline')); + mountStrip('item-a'); + await settle(); + expect(errorRow()).not.toBeNull(); + + // Uploaded during the outage: buffered, and rightly shown. + broadcastUpload('item-a', uploaded('elsewhere-deleted')); + flushSync(); + expect(tiles()).toHaveLength(1); + + // Retry. The GET goes out AFTER the upload, so the server's answer is + // authoritative — and it says the row is gone, because another tab + // deleted it. + listMock.mockResolvedValueOnce(response([att({ id: 'still-here' })])); + target.querySelector('.att-retry')?.click(); + flushSync(); + await settle(); + + const names = tiles().map((el) => el.getAttribute('aria-label') ?? ''); + expect(names.some((n) => n.includes('elsewhere-deleted'))).toBe(false); + expect(names.some((n) => n.includes('still-here.png'))).toBe(true); + }); + + it('does not retire a pending upload on a TRUNCATED page', async () => { + // The request is bounded at 50 rows, so a FULL page is not proof of + // absence — a still-live row can simply be past it. Retiring it there + // would delete a good tile permanently, which is worse than the + // resurrection the retirement rule exists to fix (Codex round 2). + listMock.mockRejectedValueOnce(new Error('offline')); + mountStrip('item-a'); + await settle(); + + broadcastUpload('item-a', uploaded('past-the-bound')); + flushSync(); + + // The retry's page comes back FULL, and the server says there are more. + const full = Array.from({ length: 50 }, (_, i) => att({ id: `s${i}` })); + listMock.mockResolvedValueOnce({ attachments: full, total: 60, limit: 50, offset: 0 }); + target.querySelector('.att-retry')?.click(); + flushSync(); + await settle(); + + target.querySelector('.att-more-expand')?.click(); + flushSync(); + const names = tiles().map((el) => el.getAttribute('aria-label') ?? ''); + expect(names.some((n) => n.includes('past-the-bound.png'))).toBe(true); + }); + + it('treats a page that holds every live row as complete, even at the bound', async () => { + // `total` is the server's own count and `offset` is always 0 here, so a + // page of exactly MAX_FETCH rows with `total: 50` HAS reached everything. + // Reading "full page" as "truncated" would leave an externally deleted + // upload buffered — resurrectable indefinitely (Codex round 3). + listMock.mockRejectedValueOnce(new Error('offline')); + mountStrip('item-a'); + await settle(); + + broadcastUpload('item-a', uploaded('elsewhere-deleted')); + flushSync(); + + const full = Array.from({ length: 50 }, (_, i) => att({ id: `s${i}` })); + listMock.mockResolvedValueOnce({ attachments: full, total: 50, limit: 50, offset: 0 }); + target.querySelector('.att-retry')?.click(); + flushSync(); + await settle(); + + target.querySelector('.att-more-expand')?.click(); + flushSync(); + const names = tiles().map((el) => el.getAttribute('aria-label') ?? ''); + expect(names.some((n) => n.includes('elsewhere-deleted'))).toBe(false); + expect(names).toHaveLength(50); + }); + + it('still keeps an upload announced while THIS request was in flight', async () => { + // The other side of the same rule, and the reason the buffer exists at + // all: this GET was issued BEFORE the upload, so its silence about the + // row proves nothing and the tile must survive the response. Pins the + // consumption above to entries the request could actually have covered. + listMock.mockRejectedValueOnce(new Error('offline')); + mountStrip('item-a'); + await settle(); + + const pending = deferred(); + listMock.mockReturnValueOnce(pending.promise); + target.querySelector('.att-retry')?.click(); + flushSync(); + + // Announced AFTER the retry's request went out. + broadcastUpload('item-a', uploaded('mid-flight')); + flushSync(); + + pending.resolve(response([att({ id: 'server-row' })])); + await settle(); + + const names = tiles().map((el) => el.getAttribute('aria-label') ?? ''); + expect(names.some((n) => n.includes('mid-flight.png'))).toBe(true); + expect(names.some((n) => n.includes('server-row.png'))).toBe(true); + }); + it('does not double-count an upload the refetch also returns', async () => { const pending = deferred(); listMock.mockReturnValue(pending.promise); diff --git a/web/src/lib/components/settings/StorageTab.svelte b/web/src/lib/components/settings/StorageTab.svelte index 741d8cbe..dbc3835e 100644 --- a/web/src/lib/components/settings/StorageTab.svelte +++ b/web/src/lib/components/settings/StorageTab.svelte @@ -1,5 +1,5 @@