fix(sync): conflict-check all entities of multi-entity ops (#8334) (#8377)

* fix(sync): conflict-check all entities of multi-entity ops #8334

Multi-entity ops (deleteTasks, moveToArchive, __updateMultipleTaskSimple,
round-time-spent, batch board/issue-provider actions) carry entityIds[], but
the server operations table only persisted the scalar entity_id (= entityIds[0]).
Once such an op was stored, only its first entity took part in future conflict
lookups, so a later stale write to a non-first entity found no prior writer and
was wrongly accepted instead of rejected as CONFLICT_SUPERSEDED/CONCURRENT.

- Add an entity_ids text[] column (populated for multi-entity ops only via
  getStoredEntityIds; single-entity ops store [] and use the scalar) + a GIN
  index. Migration is metadata-only with no backfill: pre-migration rows fall
  back to entity_id, so the fix is forward-only (entities 2..n of already-stored
  ops were never persisted and stay unrecoverable).
- detectConflictForEntity now runs two ordered LIMIT-1 lookups (scalar btree +
  entity_ids GIN) and takes the higher server_seq, preserving the fast ordered
  hot path instead of an OR's BitmapOr+sort.
- detectConflictForEntities / prefetchLatestEntityOpsForBatch match an entity as
  the scalar entity_id OR a member of entity_ids (unnest CASE + && / = ANY).
- Harden validateOp to bound entityIds (length + per-element), mirroring entityId.

Raw SQL validated against Postgres (PGlite) incl. GIN usage and two-query
correctness; single path + validation + migrations covered by unit tests. The
full conflict-detection.spec needs a generated Prisma client (CI), and the
hot-path round-trip tradeoff should be confirmed with a real-PG EXPLAIN.

* fix(sync): store entity_ids when a batch op dedups off the scalar #8334

Multi-review follow-up. getStoredEntityIds gated on `length > 1`, so a batch op
whose entityIds dedup to a single value that differs from entityId (the server
does not enforce entity_id === entityIds[0]) stored []  and that entity became
invisible to conflict lookups — reintroducing #8334 for it. Gate on "is the set
exactly [entity_id]?" instead, and cover it with unit tests.

Also: correct the array-branch comment/doc — a GIN(entity_ids) lookup has no
server_seq so it match-all-then-sorts (cheap only because multi-entity ops are
rare), it is not an ordered walk; drop a stale "OR filter" test docstring; add a
counter-note on getConflictEntityIds vs getStoredEntityIds to prevent swapping.

* refactor(sync): single OR lookup for entity conflict detection #8334

Third multi-review follow-up. Revert detectConflictForEntity from the two ordered
findFirst lookups back to a single Prisma OR [{entityId}, {entityIds:{has}}]. The
two-query split optimized the OR's BitmapOr+sort, but that is bounded by op-log
pruning (sub-ms in practice) while the split added a guaranteed extra round-trip on
the common single-entity path (doubled by the FIX-1.5 re-check) — a net-negative for
the median. The OR is simpler, fully typed/testable, and likely faster in aggregate;
a code comment documents the split as the escalation if a real-PG EXPLAIN ever shows
the OR is a problem.

Review polish (no behaviour change): correct the array-branch/getStoredEntityIds
comments (a GIN @> is match-all-then-sort, not an ordered walk; multi-entity-only
storage's win is GIN size + keeping single-entity inserts off the GIN, not sort
depth); note the batch unnest paths as the first EXPLAIN candidates under load; keep
the two batch queries' CASE/prefilter SQL inline (a shared fragment would shift the
positional params the conflict-detection.spec mock relies on) with a keep-in-sync
note; replace a non-ASCII <= in a client-facing validation error string.

* test(sync): update db mocks for OR + prefetch entity_ids lookups #8334

Running the full super-sync-server suite (with a generated Prisma client) surfaced
two specs whose inline db mocks hadn't tracked the new conflict-detection SQL:

- time-tracking-operations.spec: findFirst now models the single-entity lookup's
  OR: [{entityId}, {entityIds:{has}}] shape (it previously only matched the scalar
  entityId, so a concurrent single-entity update was wrongly "accepted").
- sync.service.spec: the prefetch $queryRaw mock parsed userId as the last param and
  flattened all params into the touched pairs; the #8334 prefilter adds idArray
  params after userId, so it now finds userId by type and reads the touched pairs
  from the VALUES join fragment only.

Production code unchanged; these are test-mock fidelity fixes. Full suite: 800 passing.
This commit is contained in:
Johannes Millan 2026-06-13 15:40:10 +02:00 committed by GitHub
parent 7445f1cdcd
commit d30fd5f434
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 511 additions and 47 deletions

View file

@ -227,6 +227,22 @@ An import is an explicit user action to restore **all clients** to a specific st
In `detectConflict()`, operations with `opType` of `SYNC_IMPORT`, `BACKUP_IMPORT`, or `REPAIR` return `{ hasConflict: false }` immediately. These operations replace entire state and don't operate on individual entities.
### Multi-Entity Ops and Server-Side Conflict Detection (issue #8334)
An op may carry `entityIds: string[]` (batch actions: `deleteTasks`, `moveToArchive`, `__updateMultipleTaskSimple`, round-time-spent, task-repeat-cfg/board/issue-provider batches). `detectConflict()` checks the **incoming** op against **all** of its `entityIds`. The op must also be **looked up** by all of its entities once stored — otherwise a later stale write to a non-first entity would find no prior writer and be wrongly accepted as non-conflicting.
To make that symmetric, the `operations` row stores:
- `entity_id` — the client-supplied scalar. For batch ops the client sets it to `entityIds[0]` (`operation-log.effects.ts`), but the **server does not enforce** `entity_id === entityIds[0]`. It is the lookup key for single-entity ops and the first entity of multi-entity ops, and is also used by duplicate detection.
- `entity_ids` — the entity set for **multi-entity ops only** (a `text[]` column; populated via `getStoredEntityIds(op)`, which returns `[]` for single-entity ops). Keeping single-entity rows out of this column keeps the `GIN(entity_ids)` index small and the array-branch lookup cheap.
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.
- `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.
### The `SyncImportFilterService` Algorithm
Implemented in `src/app/op-log/sync/sync-import-filter.service.ts`: