diff --git a/web/src/lib/stores/localIndex.svelte.ts b/web/src/lib/stores/localIndex.svelte.ts index 36ca5cdfb..e0c975479 100644 --- a/web/src/lib/stores/localIndex.svelte.ts +++ b/web/src/lib/stores/localIndex.svelte.ts @@ -1220,12 +1220,31 @@ export const localIndex = { for (const row of resp.items) { mergeRow(state, row); } - if (cursorAsNum(resp.cursor) > cursorAsNum(state.cursor)) { - state.cursor = resp.cursor; - } + // THE PIN (IDEA-2924). A snapshot whose cursor is BEHIND the + // cache's is a stale response that landed late — a delta was + // consumed while this request was in flight. Keeping the + // higher cursor is what made that reinstatement PERMANENT: + // nothing would ever replay the range the snapshot did not + // see, so a row the delta evicted stayed evicted only as long + // as a per-id record remembered it (TASK-2920's + // `movedOutFloor`). + // + // Pinning to the snapshot's cursor is the discipline + // `resyncProjectionScope` has always had, and TASK-2906 + // measured its safety: a RAM regression is safe because the + // merge keeps the newer row per id and a replayed range is + // idempotent. Only the DURABLE cursor may not regress, and + // nothing here writes one — see the persist below. + // Unconditional, which is "pin or advance" written once: below + // is the pin, above is the advance, equal is a no-op. The + // previous `only if higher` was the advance alone. + const behind = cursorAsNum(resp.cursor) < cursorAsNum(state.cursor); + state.cursor = resp.cursor; state.bootstrapState = 'ready'; - // Cold path is a full snapshot — nothing pending. - state.pendingResync = false; + // A cold snapshot that was NOT overtaken is complete, so + // nothing is pending. One that was overtaken owes a replay of + // the range it did not see, and says so. + state.pendingResync = behind; // Server rows carry the live collection slug — drop any // rename recorded pre-snapshot rather than re-applying // it over fresher server truth (BUG-2601). @@ -1262,6 +1281,44 @@ export const localIndex = { ).catch( () => undefined, ); + // THE REPLAY OWNER (IDEA-2924), and it has to be here because + // MEASUREMENT SAYS NOBODY ELSE IS: the collection route calls + // `deltaSync` right after `bootstrap` in its own effect, but + // `ItemDetail` — the other `bootstrap` caller — calls it with + // NOTHING following, so on the item route a pinned cursor + // would sit un-replayed until an unrelated `sync_required` or + // item event happened along, which in a quiet workspace is + // never. + // + // Through the SAME loop both other doors use, not a second + // one: `reconcileWorkspace` is the function TASK-2921 made + // singular, called here with this bootstrap's own generation + // check. It drains from the cursor just pinned, so the range + // the snapshot did not see is re-delivered and every eviction + // in it is re-applied. + // + // Only when the snapshot was overtaken. A cold boot that won + // its race is complete and has nothing to drain. + if (behind) { + // NON-FATAL, and the mutation run is how I noticed it + // mattered. The snapshot is good and already installed; + // failing the whole bootstrap because a FOLLOW-UP drain + // blipped would throw away usable data and flip the UI to + // 'error' over a network hiccup — which the warm branch + // has always refused to do for the same reason. The ask + // stays set, so the next `bootstrap()` resumes. + // + // An auth error is the exception and is rethrown: a 401 / + // 403 means the rows are not ours to display, and + // `reconcileWorkspace`'s caller must still purge. Swallow + // that and the cache would go on serving revoked rows. + try { + await reconcileWorkspace(ws, state, isStale); + } catch (err) { + if (isAuthError(err)) throw err; + } + if (isStale()) return; + } } } catch (err) { // Same order, same reason as the reconcile catch above (codex diff --git a/web/src/lib/stores/localIndexInverseColdCheck.svelte.test.ts b/web/src/lib/stores/localIndexInverseColdCheck.svelte.test.ts index ce24692c0..bc4270b68 100644 --- a/web/src/lib/stores/localIndexInverseColdCheck.svelte.test.ts +++ b/web/src/lib/stores/localIndexInverseColdCheck.svelte.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; -import { api } from '$lib/api/client'; +import { api, PadApiError } from '$lib/api/client'; import type { ItemIndexRow } from '$lib/types'; import { localIndex } from './localIndex.svelte'; import { localSearch } from './localSearch.svelte'; @@ -66,6 +66,16 @@ describe('cold bootstrap vs. an eviction that raced the snapshot', () => { it('does not reinstate a row whose moved_out was applied while /items-index was in flight', async () => { const gate = deferred>>(); vi.spyOn(api.items, 'listIndex').mockReturnValueOnce(gate.promise); + // The pin (IDEA-2924) makes an overtaken cold boot REPLAY from the + // snapshot's cursor, so this door is now reached where it never used to + // be. The server re-delivers the eviction, because `ListMovedOutSince` is + // a `seq > since` query and the pinned cursor is below it. + vi.spyOn(api.items, 'changes').mockResolvedValue({ + changes: [{ id: 'secret', seq: 100, moved_out: true }], + cursor: '100', + includes_unparented_metadata: false, + access_epoch: 'epoch-1', + }); // Cold boot begins: empty IDB, so this takes the /items-index branch. const boot = localIndex.bootstrap(ws, { userId: null }); @@ -95,9 +105,22 @@ describe('cold bootstrap vs. an eviction that raced the snapshot', () => { expect(canSee('secret', 'revoked')).toBe(false); }); - it('leaves no replay that could heal it: the cursor stays past the eviction and nothing is pending', async () => { + it('pins the cursor to the snapshot and replays from it, rather than skipping the gap', async () => { + // THIS TEST REPLACES ONE THAT ASSERTED THE OPPOSITE. Under TASK-2920 the + // cold branch kept the HIGHER cursor and set `pendingResync = false`, and + // a test here pinned that — deliberately, because it was what made the + // reinstatement permanent rather than cosmetic. IDEA-2924 removes the + // permanence, so the old assertions are now assertions about a defect + // that is gone; they are replaced rather than deleted, and this comment + // is the record of why a green test was changed. const gate = deferred>>(); vi.spyOn(api.items, 'listIndex').mockReturnValueOnce(gate.promise); + const changes = vi.spyOn(api.items, 'changes').mockResolvedValue({ + changes: [], + cursor: '95', + includes_unparented_metadata: false, + access_epoch: 'epoch-1', + }); const boot = localIndex.bootstrap(ws, { userId: null }); await settle(); @@ -111,12 +134,91 @@ describe('cold bootstrap vs. an eviction that raced the snapshot', () => { }); await boot; - // This is the half that makes the first assertion's failure PERMANENT - // rather than transient, and it is asserted separately so a fix that - // merely re-fires a sync is not mistaken for a fix that holds the - // property. A later `/items-changes?since=100` cannot re-deliver a - // change at seq 100. - expect(localIndex.cursorFor(ws)).toBe('100'); + // THE PIN, observable: the replay was issued FROM the snapshot's cursor, + // not from the higher one the delta had left behind. Asserting the + // request's `since` rather than the final cursor, because the loop + // advances the cursor again immediately and the end state cannot tell + // the two builds apart. + expect(changes).toHaveBeenCalled(); + expect(changes.mock.calls[0][1]).toBe('95'); + }); + + it('keeps the ask set when the replay fails, and does NOT fail the boot over it', async () => { + // Two properties in one leg because they are one decision. A mutation run + // found `pendingResync = behind` undetectable — the replay clears the ask + // on success, so the only moment it is observable is when the replay does + // NOT succeed. Writing that test then showed the replay's failure was + // taking the whole bootstrap down with it, discarding a snapshot that was + // already installed and usable. + const gate = deferred>>(); + vi.spyOn(api.items, 'listIndex').mockReturnValueOnce(gate.promise); + vi.spyOn(api.items, 'changes').mockRejectedValue(new Error('network')); + + const boot = localIndex.bootstrap(ws, { userId: null }); + await settle(); + localIndex.applyDelta(ws, [{ id: 'secret', seq: 100, moved_out: true }], '100', false); + gate.resolve({ + items: [row('keeper', 1, 'kept'), row('secret', 40, 'revoked')], + total: 2, + cursor: '95', + includes_unparented_metadata: false, + access_epoch: 'epoch-1', + }); + + // The boot RESOLVES — the snapshot is good. + await expect(boot).resolves.toBeUndefined(); + expect(localIndex.bootstrapStateFor(ws)).toBe('ready'); + expect(canSee('keeper', 'kept')).toBe(true); + // ...and the ask is still outstanding, so a later bootstrap resumes. + expect(localIndex.pendingResyncFor(ws)).toBe(true); + }); + + it('does NOT swallow a 403 from the replay — the purge still happens', async () => { + // The auth carve-out in the replay's catch. A mutant that swallowed + // everything survived the suite: a revocation surfacing on the REPLAY + // rather than on the snapshot would have left the cache serving rows the + // caller can no longer see, which is the whole thing TASK-1360 exists to + // stop. Non-fatal for a network blip is right; non-fatal for a 403 is the + // bug wearing the same clothes. + const gate = deferred>>(); + vi.spyOn(api.items, 'listIndex').mockReturnValueOnce(gate.promise); + vi.spyOn(api.items, 'changes').mockImplementation(async () => { + // What api.request() does, in order: the global handler resets first. + localIndex.reset(ws); + throw new PadApiError({ code: 'forbidden', message: 'forbidden' }); + }); + + const boot = localIndex.bootstrap(ws, { userId: null }); + await settle(); + localIndex.applyDelta(ws, [{ id: 'secret', seq: 100, moved_out: true }], '100', false); + gate.resolve({ + items: [row('keeper', 1, 'kept'), row('secret', 40, 'revoked')], + total: 2, + cursor: '95', + includes_unparented_metadata: false, + access_epoch: 'epoch-1', + }); + + await expect(boot).rejects.toThrow(); + expect(localIndex.accessRevokedFor(ws)).toBe(true); + }); + + it('makes NO replay when the cold boot was not overtaken', async () => { + // The control leg. Without it the assertions above would also pass on a + // build that replayed unconditionally, which would be a different and + // worse change — an extra round-trip on every cold boot. + vi.spyOn(api.items, 'listIndex').mockResolvedValueOnce({ + items: [row('keeper', 1, 'kept')], + total: 1, + cursor: '95', + includes_unparented_metadata: false, + access_epoch: 'epoch-1', + }); + const changes = vi.spyOn(api.items, 'changes'); + + await localIndex.bootstrap(ws, { userId: null }); + + expect(changes).not.toHaveBeenCalled(); expect(localIndex.pendingResyncFor(ws)).toBe(false); }); });