From fdf0f62011bb1fbf87ef5b7a97b87d83c52b1061 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Mon, 20 Jul 2026 15:34:58 +0200 Subject: [PATCH] docs(supersync): stop the vector-clock doc recommending the outage query vector-clocks.md still described detectConflictForEntity as the combined OR + orderBy filter and vouched for it - 'a BitmapOr + sort bounded by the entity's stored version depth (op-log pruning keeps that small)'. That is the exact false assumption behind the 2026-07-20 outage, in the authoritative doc. Worse, the escalation it recommended (two ordered LIMIT 1 lookups) is itself a measured-catastrophic shape: the array side still cannot order on GIN. Also fix a test that could not fail. 'does not alias a POST-split misc write onto tasks' probed 'tasks-v2-only', but the legacy-misc branch is entered only for entityId === 'tasks', so it never reached the gate it claimed to guard - breaking the gate to 'lte' left all 915 tests green. It now probes 'tasks' under its own tenant, and is verified red under that mutation. Correct three overclaims in the comments: production does get custom plans for the first ~5 executions before settling generic; a partial expression GIN can skip the single-entity majority; fastupdate=off stops new pending entries but does not flush existing ones, and 140 blocks is not a ceiling. --- docs/sync-and-op-log/vector-clocks.md | 25 ++++++++++++++++- ...conflict-entity-lookup-plan.pglite.spec.ts | 27 ++++++++++++++----- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/docs/sync-and-op-log/vector-clocks.md b/docs/sync-and-op-log/vector-clocks.md index fcef56bdda..ef6761edca 100644 --- a/docs/sync-and-op-log/vector-clocks.md +++ b/docs/sync-and-op-log/vector-clocks.md @@ -237,7 +237,30 @@ To make that symmetric, the `operations` row stores: The lookups in `conflict.ts` match a requested entity as the scalar `entity_id` **or** a member of `entity_ids`: -- `detectConflictForEntity` (single) — Prisma `where: { OR: [{ entityId }, { entityIds: { has: entityId } }] }`, ordered by `server_seq`. The `OR` spans the `entity_id` btree and the `entity_ids` GIN, so the planner uses a `BitmapOr` + sort bounded by the entity's stored version depth (op-log pruning keeps that small). If a real-Postgres `EXPLAIN` on a deep-history entity ever shows this hot path is a problem, the escalation is two ordered `LIMIT 1` lookups (scalar btree + `entity_ids` GIN) taking the higher `server_seq` — the array side stays small because the column is multi-entity-only. +- `detectConflictForEntity` (single) — **two separately-indexed lookups, never one combined filter.** A scalar `findFirst` on `{ userId, entityType, entityId }` ordered by `server_seq` (served end to end by the `(user_id, entity_type, entity_id, server_seq)` btree), plus a raw-SQL `MATERIALIZED` CTE taking `MAX(server_seq)` over `entity_ids @> ARRAY[id]`, with the winning row then fetched by the `(user_id, server_seq)` unique key. + + > ⚠️ **Do not "simplify" this back into one query.** It used to be + > `where: { OR: [{ entityId }, { entityIds: { has: entityId } }] }` + `orderBy: { serverSeq: 'desc' }`, + > and on 2026-07-20 that caused a total sync outage — 47 stuck backends, longest 75 minutes, + > 61/66 connections consumed. The `OR` spans two different indexes and GIN cannot supply + > `server_seq` ordering, so the planner abandons **both** index paths and walks the user's + > history. Nothing bounds that walk when the entity has no matching rows — i.e. the + > first-ever op for a new task, the most common upload there is. Op-log pruning does **not** + > bound it; that assumption is what this paragraph used to assert, and it was wrong. + > + > The obvious escalations are broken too. Two ordered `LIMIT 1` lookups still leave the + > array side unable to order on GIN. Measured under generic planning on a 40k-row seed, the + > outage query, the naive array-only `LIMIT 1`, the flat `MAX`, Prisma's `aggregate({ _max })` + > and the CTE with `MATERIALIZED` dropped **all** read the user's whole entity-type slice — + > 816 blocks and 2500 rows discarded, against 143 blocks and 0 discarded for the shipped form. + > + > Measure any change here with `SET plan_cache_mode = force_generic_plan`. Prisma sends + > parameterized prepared statements, so Postgres settles on a **generic** plan after ~5 + > executions, and `EXPLAIN` with literal constants builds a custom plan that makes every one + > of those broken shapes look perfect. See + > `packages/super-sync-server/tests/conflict-entity-lookup-plan.pglite.spec.ts` and the note + > at `detectConflictForEntity` in `packages/super-sync-server/src/sync/conflict.ts`. + - `detectConflictForEntities` / `prefetchLatestEntityOpsForBatch` (batch) — raw SQL unnesting `CASE WHEN cardinality(entity_ids) > 0 THEN entity_ids ELSE ARRAY[entity_id] END`, with a `entity_ids && ... OR entity_id = ANY(...)` prefilter so the `GIN(entity_ids)` index (migration `20260613000001`) and the existing `entity_id` btree stay usable. **Forward-only by design:** rows written before migration `20260613000000` have an empty `entity_ids` array and fall back to the scalar `entity_id` (= first entity) in the `CASE` expression / scalar branch above — there is no `UPDATE` backfill. Entities 2..n of already-stored multi-entity ops were never persisted and are unrecoverable, so they remain invisible to conflict detection until that entity gets a fresh write. This residual is bounded: client-side LWW is unaffected (the client persists the full op and `VectorClockService.getEntityFrontier()` fans each op out to **every** entity), and the server only builds an authoritative snapshot from non-encrypted ops (`replayOpsToState()` throws on encrypted ops), so the pre-fix gap could only surface a stale value to a fresh client on non-encrypted self-hosted servers. diff --git a/packages/super-sync-server/tests/conflict-entity-lookup-plan.pglite.spec.ts b/packages/super-sync-server/tests/conflict-entity-lookup-plan.pglite.spec.ts index 5b05a73008..85ce5d8535 100644 --- a/packages/super-sync-server/tests/conflict-entity-lookup-plan.pglite.spec.ts +++ b/packages/super-sync-server/tests/conflict-entity-lookup-plan.pglite.spec.ts @@ -46,11 +46,14 @@ import type { Operation } from '../src/sync/sync.types'; * inserts land in the GIN pending list (fastupdate defaults to on) which `@>` must * scan linearly: same data, same query, 140 blocks and a 14x larger index (1120 kB * vs 72 kB). VACUUM flushes the pending list and restores 2 blocks, so the real cost - * OSCILLATES between the two across the autovacuum cycle, bounded above by - * gin_pending_list_limit rather than by anything in this seed. This suite therefore - * seeds in production order and never vacuums: it measures the WORST realistic - * steady state, which is what a regression budget should sit above. Setting - * fastupdate=off removes the oscillation and pins the array branch at 2 blocks. + * OSCILLATES across the autovacuum cycle. This suite therefore seeds in production + * order and never vacuums, measuring the dirty end of that cycle rather than the + * freshly-vacuumed end — but 140 is NOT a ceiling: the pending list is bounded by + * gin_pending_list_limit (4MB default), not by anything in this seed, so a + * production write burst can be worse. The budget below is calibrated to catch the + * mis-plan, NOT to certify a maximum. Setting fastupdate=off stops new entries + * queueing — it does not flush what is already pending, which still needs a VACUUM — + * and so removes the oscillation going forward. * * REMAINING FIDELITY LIMIT: PGlite is not the production cluster — different major * version, and it reports every block as a cache hit, so these counts cannot model @@ -488,6 +491,8 @@ describe('detectConflictForEntity behaviour is unchanged by the query split (PGl const TIME_DELTA_ACTION = '[TimeTracking] Sync time spent'; const OTHER_USER_ID = 7; + /** Own tenant, so the legacy-misc row seeded for USER_ID cannot mask the gate. */ + const POST_SPLIT_USER_ID = 8; const seed = async (op: { id: string; @@ -778,7 +783,16 @@ describe('detectConflictForEntity behaviour is unchanged by the query split (PGl it('does not alias a POST-split misc write onto tasks', async () => { // The alias is gated on the fixed v1→v2 split boundary; a v2+ misc write is // disjoint from tasks and must not fabricate a conflict. + // + // MUST probe exactly 'tasks'. detectConflictForEntity enters the legacy-misc branch + // only for entityId === 'tasks', so probing any other id (this once read + // 'tasks-v2-only') never reaches the gate and passes no matter what the gate does — + // verified: breaking it to `lte` left all 915 tests green. + // + // Runs as its OWN user because the preceding test leaves a schema_version 1 misc row + // for USER_ID in the shared table, which would legitimately alias and mask this. await seed({ + userId: POST_SPLIT_USER_ID, id: 'op-modern-misc', serverSeq: 11, clientId: 'other', @@ -789,7 +803,8 @@ describe('detectConflictForEntity behaviour is unchanged by the query split (PGl }); expect( - (await detect('tasks-v2-only', { entityType: 'GLOBAL_CONFIG' })).hasConflict, + (await detect('tasks', { entityType: 'GLOBAL_CONFIG' }, POST_SPLIT_USER_ID)) + .hasConflict, ).toBe(false); }); });