Skip to content

SF-3893 Prevent stale offline snapshots from corrupting live query results #4039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Nateowami wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

SF-3893 Prevent stale offline snapshots from corrupting live query results #4039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions doc/offline-query-membership.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
# Offline query membership and stale snapshot reconciliation

This document explains a subtle class of bugs in the frontend realtime layer (`xforge-common`),
and how `RealtimeQuery`/`RealtimeDoc` guard against it. It exists because the complexity is easy
to lose in scattered comments; see SF-3893 for the bug that motivated it.

## Background: three copies of every doc

For each realtime doc the client can hold up to three copies:

1. **Server** (ShareDB/Mongo) — authoritative.
2. **In-memory doc** (`RealtimeDoc`/ShareDB client doc) — kept up to date by ops _only while the
client is subscribed to the doc, or the doc is in the results of a subscribed query_.
3. **Offline store** (IndexedDB) — written by `RealtimeDoc.updateOfflineData()`, used to render
before the server responds and to work offline.

A subscribed `RealtimeQuery` gets its membership (which docs are in the results) from two
sources:

- **Remote**: the ShareDB query subscription pushes diffs when the server's result set changes.
- **Local**: `RealtimeQuery.localQuery()` re-runs the query's Mongo-style filter against the
offline store (via mingo). This happens on initial subscribe (offline-first render) and — via
`RealtimeService.onLocalDocUpdate()` — on **every local submit to any doc in the collection**,
so that local edits are reflected in query results immediately and while offline.

## The problem: snapshots nobody will ever correct

The server only pushes ops for docs the client is subscribed to (directly, or via the doc being
in a subscribed query's current results). Consequently, **a doc that changes on the server while
this client is not listening ends up with a stale offline snapshot, and no invalidation for it
will ever arrive.** Typical causes: the page was closed or reloaded between the change and the
next look, or a websocket reconnect window.

Nothing in the doc lifecycle repairs this on its own:

- `updateOfflineData()` early-returns when the adapter's version equals the version already
stored offline — which is exactly the case when the doc was loaded _from_ the stale snapshot.
- `checkExists()` only answers "was it deleted?", which handles server-side deletions (the
offline entry is purged) but not modifications.
- Docs are never disposed during normal navigation, so the stale entry survives indefinitely.

The failure mode (SF-3893): a checker's client had an archived question cached offline with
`isArchived: false`. The live query correctly showed the server's results — until the checker
answered a _different_ question. That local submit triggered `localQuery()`, the stale snapshot
matched the `isArchived: false` filter, and the archived question was spliced back into the live
results. The server never corrects this, because _its_ result set did not change. The same
mechanism works in the opposite direction (a doc whose stale snapshot wrongly fails the filter
vanishes from results on any unrelated local write).

Note that per-consumer defensive filtering (e.g. `.filter(q => !q.data.isArchived)` in a
component) does **not** fix this: the resurrected doc's in-memory data comes from the same stale
snapshot, so the filter passes it.

## The solution

Two cooperating mechanisms. The principle: **while the remote query subscription is live, the
server's membership is authoritative; local results may only diverge from it for docs with
pending (unacknowledged) local ops. Any other disagreement proves an offline snapshot is stale
and triggers its repair.**

### 1. Membership gate — `RealtimeQuery.reconcileWithRemote()`

When `localQuery()` runs while the remote query is live (`adapter.ready && adapter.subscribed`),
its results are merged with the server's current results. Per doc:

| server includes | offline matches filter | pending local ops | in results? | notes |
| --------------- | ---------------------- | ----------------- | ----------- | --------------------------------------------------- |
| yes | yes | — | yes | agreement |
| no | no | — | no | agreement |
| no | yes | yes | yes | optimistic add: client just created/changed it |
| no | yes | no | no | stale snapshot → reconcile (SF-3893) |
| yes | no | yes | no | optimistic removal: client just archived/changed it |
| yes | no | no | yes | stale snapshot (inverse direction) → reconcile |

- "Pending local ops" (`RealtimeDoc.hasPendingOps`) includes the in-flight op, so it is `true`
at the moment `submit()` triggers the local re-query, before the server acknowledges.
- Paged queries (`$skip`/`$limit`): two differently-paged result sets cannot be meaningfully
merged, so the server's page is used as-is while live.
- Row 6 appends docs out of sort order; the order self-corrects once reconciliation refreshes
the offline snapshot.
- Before the remote query is ready (initial load, fully offline), local results are used
unchanged — offline-first behavior is unaffected, and all offline mutations carry pending ops,
so they survive the gate after reconnecting.

### 1b. Serialized change application — `RealtimeQuery.onChange()`

Implementing the gate surfaced a latent race: `onChange()` diffs the new result ids against the
current results and applies the diff with index-based splices, but it is async (inserting docs
awaits their offline data loading). Two overlapping invocations — e.g. a server-driven change
interleaving with a local re-query at an await point — each capture a `before` snapshot and can
splice against state the other has already changed, duplicating or misplacing docs. Changes are
now applied strictly one at a time via an internal lock. When no change is in flight, a change
still starts synchronously, preserving the previous timing in the common case.

(Implementation note: the lock is deliberately written with async/await only. ts-mockito
discovers mockable method names by scanning the class _source text_, so a call to a promise's
"then" method anywhere in the `RealtimeQuery` source would make every mocked `RealtimeQuery`
instance a thenable that never settles when awaited or passed to `Promise.resolve()` in tests.)

### 2. Snapshot repair — `RealtimeDoc.reconcileOfflineData()`

Fired for the two "stale snapshot" rows above, and also when the server removes a doc from a
subscribed query's results while the doc's local data still matches the query filter (which
proves staleness without waiting for the next local write — this repairs SF-3893-style staleness
at load time, before the user does anything). One server fetch resolves all cases:

- doc still exists → rewrite the offline snapshot (forced, since the doc may no longer be in any
subscribed query);
- doc deleted → purge the offline entry and emit `delete$` (a generalization of `checkExists()`);
- fetch fails (offline, or the user may no longer read the doc) → leave the offline copy; the
membership gate already excludes the doc, so nothing incorrect is shown.

Repair is self-limiting (once the snapshot agrees with the server the trigger disappears) and
concurrent triggers share one round trip.

## Known limitations

- **Initial flash**: with a stale offline store, a since-archived doc can appear briefly on load
until the remote query becomes ready. Fixing this would mean not rendering offline results —
a product tradeoff. Repair at least limits it to one stale session per doc.
- **Ack window**: after an op is acknowledged but before the server's query diff arrives, an
unrelated local write can briefly drop a just-added doc; the diff restores it moments later.
- **Permission revocation**: `sharedb-access` rejects whole read requests with a 403 when any
snapshot is unreadable, so a no-longer-readable doc looks like an error, not like "gone". Its
offline data is _not_ purged (only logout's `deleteDB()` clears it), and project-level
removal is handled at the application layer (components navigate away). A deliberate purge of
unreadable docs' offline data is possible future work.
- **No offline sweep**: docs that never pass through a subscribed query again (e.g. a whole
project the user lost access to) keep their offline entries until logout.

## Tests

- `xforge-common/models/realtime-query.spec.ts` — the decision table, including the SF-3893
regression (resurrection on unrelated local write) and the proactive repair on remote removal.
- `xforge-common/models/realtime-doc.spec.ts` — `reconcileOfflineData()` semantics
(update/purge/leave-on-error/deduplication).
- The memory test doubles (`memory-realtime-remote-store.ts`) model an _instantly consistent_
server: local submits are written back to the remote store and query adapters re-query it on
access. Without this, tests would exercise a state (acknowledged op, stale server result set)
that the real ShareDB adapters only pass through transiently.
86 changes: 63 additions & 23 deletions src/SIL.XForge.Scripture/ClientApp/src/xforge-common/memory-realtime-remote-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ export class MemoryRealtimeRemoteStore extends RealtimeRemoteStore {
return collectionSnapshots.values();
}

removeSnapshot(collection: string, id: string): void {
this.snapshots.get(collection)?.delete(id);
}

clear(): void {
this.snapshots.clear();
}
Expand All @@ -45,9 +49,9 @@ export class MemoryRealtimeRemoteStore extends RealtimeRemoteStore {
snapshot = collectionSnapshots.get(id);
}
if (snapshot == null) {
return new MemoryRealtimeDocAdapter(collection, id);
return new MemoryRealtimeDocAdapter(collection, id, undefined, undefined, undefined, this);
}
return new MemoryRealtimeDocAdapter(collection, id, snapshot.data, types.map[snapshot.type], snapshot.v);
return new MemoryRealtimeDocAdapter(collection, id, snapshot.data, types.map[snapshot.type], snapshot.v, this);
}

createQueryAdapter(collection: string, parameters: QueryParameters): RealtimeQueryAdapter {
Expand Down Expand Up @@ -101,7 +105,8 @@ export class MemoryRealtimeDocAdapter implements RealtimeDocAdapter {
public readonly id: string,
public data?: any,
public type: OTType | undefined = OTJson0.type,
version?: number
version?: number,
private readonly remoteStore?: MemoryRealtimeRemoteStore
) {
if (version != null) {
this.version = version;
Expand All @@ -115,6 +120,7 @@ export class MemoryRealtimeDocAdapter implements RealtimeDocAdapter {
this.data = data;
this.type = types.map[type];
this.version = 0;
this.syncSnapshotToStore();
this.emitCreate();
return Promise.resolve();
}
Expand Down Expand Up @@ -143,6 +149,7 @@ export class MemoryRealtimeDocAdapter implements RealtimeDocAdapter {
}
this.data = this.type.apply(this.data, op);
this.version++;
this.syncSnapshotToStore();
this.emitChange(op);
if (!source) {
this.emitRemoteChange(op);
Expand All @@ -162,6 +169,7 @@ export class MemoryRealtimeDocAdapter implements RealtimeDocAdapter {
this.data = undefined;
this.version = -1;
this.type = undefined;
this.remoteStore?.removeSnapshot(this.collection, this.id);
this.emitDelete();
return Promise.resolve();
}
Expand All @@ -174,6 +182,23 @@ export class MemoryRealtimeDocAdapter implements RealtimeDocAdapter {
return Promise.resolve();
}

/**
* Keeps the remote store's snapshot in sync with this adapter, so that the memory
* implementation behaves like an instantly-consistent server: local submits are immediately
* reflected in query results (MemoryRealtimeQueryAdapter re-queries the store on access).
*/
private syncSnapshotToStore(): void {
if (this.remoteStore == null || this.type == null) {
return;
}
this.remoteStore.addSnapshot(this.collection, {
id: this.id,
data: this.data,
v: this.version,
type: this.type.name
});
}

emitChange(op?: any): void {
this.changes$.next(op);
}
Expand All @@ -194,62 +219,77 @@ export class MemoryRealtimeDocAdapter implements RealtimeDocAdapter {
export class MemoryRealtimeQueryAdapter implements RealtimeQueryAdapter {
subscribed: boolean = false;
ready: boolean = true;
unpagedCount: number = 0;
docIds: string[] = [];
count: number = 0;

readonly ready$ = new Subject<void>();
readonly remoteChanges$ = new Subject<void>();

private lastDocIds: string[] = [];
private lastCount: number = 0;

constructor(
private readonly remoteStore: MemoryRealtimeRemoteStore,
public readonly collection: string,
public readonly parameters: QueryParameters
) {}

// The results are re-queried from the remote store on every access, so that the memory
// implementation behaves like an instantly-consistent server (there is no notion of an
// in-flight op or a not-yet-polled query subscription, as there is with the ShareDB adapters).
get docIds(): string[] {
return this.performQuery().docIds;
}

get count(): number {
return this.performQuery().count;
}

get unpagedCount(): number {
return this.performQuery().unpagedCount;
}

fetch(): Promise<void> {
this.performQuery();
this.rememberResults();
this.ready = true;
this.ready$.next();
return Promise.resolve();
}

subscribe(_initialDocIds?: string[]): void {
this.performQuery();
this.rememberResults();
this.subscribed = true;
this.ready = true;
this.ready$.next();
}

updateResults(): void {
if (this.performQuery()) {
if (this.rememberResults()) {
this.remoteChanges$.next();
}
}

destroy(): void {}

private performQuery(): boolean {
let changed = false;
/** Re-queries the results and reports whether they changed since the last remembered results. */
private rememberResults(): boolean {
const { docIds, count } = this.performQuery();
const changed: boolean = !isEqual(this.lastDocIds, docIds) || this.lastCount !== count;
this.lastDocIds = docIds;
this.lastCount = count;
return changed;
}

private performQuery(): { docIds: string[]; count: number; unpagedCount: number } {
const snapshots = Array.from(this.remoteStore.getSnapshots(this.collection));
const { results, unpagedCount } = performQuery(this.parameters, snapshots);
let docIds: string[];
let count: number;
if (results instanceof Array) {
const before = this.docIds;
const after = results.map(s => s.id);
this.docIds = after;
if (!isEqual(before, after)) {
changed = true;
}
docIds = results.map(s => s.id);
count = results.length;
} else {
docIds = [];
count = results;
}
if (this.count !== count) {
this.count = count;
changed = true;
}
this.unpagedCount = unpagedCount;
return changed;
return { docIds: docIds, count: count, unpagedCount: unpagedCount };
}
}
Loading
Loading