gsadmin/pad
1
0
Fork 0
mirror of https://github.com/PerpetualSoftware/pad.git synced 2026-10-26 14:30:13 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(web): the cold path pins its cursor to the snapshot and owns the replay (IDEA-2924) (#1280)

Browse Source 
A cold /items-index snapshot whose cursor is BEHIND the cache's is a stale
response that landed late — a delta was consumed while it was in flight. Keeping
the higher cursor is what made TASK-2920's reinstatement PERMANENT: nothing
would ever replay the range the snapshot did not see.

The cold branch now pins to the snapshot's cursor, the discipline
resyncProjectionScope has always had and whose safety TASK-2906 measured (a RAM
regression is safe; the merge keeps the newer row per id and a replayed range is
idempotent). Nothing changes in IDB — the durable gate already refuses a behind
cursor, whole, and the recon on the idea's trail establishes that both reachable
states make that refusal correct.

THE REPLAY OWNER IS MEASURED, NOT ASSUMED. The collection route calls deltaSync
right after bootstrap in its own effect; ItemDetail — the only other bootstrap
caller — calls it with NOTHING following. So on the item route a pinned cursor
would sit un-replayed until an unrelated event happened along, which in a quiet
workspace is never. The cold branch therefore drains it itself, through the SAME
reconcileWorkspace both other doors use rather than a second loop, and only when
the snapshot was overtaken.

Non-fatal, except for auth. The snapshot is already installed and usable, so a
network blip on a FOLLOW-UP drain must not flip the UI to 'error' — the warm
branch has always refused that. A 401/403 is rethrown so the purge still
happens.

movedOutFloor STAYS. It keeps the property true while the pin is unproven, and
removing a guard in the same change that replaces it is how a gap ships green;
its removal is its own later unit.

Two tests replaced rather than deleted: they asserted the cold reinstatement was
PERMANENT, which was true and deliberate under TASK-2920 and is what this
removes. The comment in the file records why a green test was changed.

Mutation: 7 mutants, 7 killed. Two of them found defects rather than weak tests
— the ask was undetectable until a failing-replay test existed, and writing that
test showed the replay's failure was taking the whole bootstrap down with it;
and swallowing a 403 on the replay path survived until its own leg was added.

Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
This commit is contained in:
xarmian
2026-09-07 13:12:44 -04:00
committed by GitHub
parent 83f00a2c66
commit d387d540cd
2 changed files with 172 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/lib/stores/localIndex.svelte.ts
+62 -5
View File
@@ -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
web/src/lib/stores/localIndexInverseColdCheck.svelte.test.ts
+110 -8
View File
@@ -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<Awaited<ReturnType<typeof api.items.listIndex>>>();
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<Awaited<ReturnType<typeof api.items.listIndex>>>();
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<Awaited<ReturnType<typeof api.items.listIndex>>>();
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<Awaited<ReturnType<typeof api.items.listIndex>>>();
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);
});
});