mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-30 03:00:57 +00:00
77557077f8
* fix(supersync): split conflict entity lookup to avoid full-history scan
detectConflictForEntity ran one Prisma filter with
`OR: [{ entityId }, { entityIds: { has: entityId } }]` and
`orderBy: { serverSeq: 'desc' }`. The OR spans the
(user_id, entity_type, entity_id, server_seq) btree and the entity_ids
GIN index, and GIN cannot supply server_seq ordering — so with LIMIT 1
the planner abandoned both index paths and walked server_seq backwards
applying the OR as a filter, betting it would exit early. For an entity
with no matching rows (the first op for a new task, the most common
upload there is) that bet loses and it scans the user's whole history.
The function runs twice per operation on the default serial upload path,
so every upload paid it twice.
Production incident: 47 backends stuck on this query, longest 75 minutes,
wait_event=DataFileRead, 61 of 66 connections consumed. Prisma pool
exhaustion made every user's upload AND download fail with "Unable to
start a transaction in the given time". No index was missing or invalid.
Split into separately-indexed lookups:
- scalar branch: findFirst on {userId, entityType, entityId} — the btree
covers all equality columns plus the sort column
- array branch: aggregate({_max: {serverSeq}}) — Prisma's OFFSET 0
subquery is a planner fence that forbids the LIMIT-driven walk and
forces the GIN bitmap path
- winner fetch: findUnique on the (userId, serverSeq) unique key, only
when the array max beats the scalar
Measured on production Postgres (88k-op user, zero-match probe) after an
ANALYZE and a GIN pending-list flush: old shape 12 buffers, new shape 9
(scalar 4 + array 5). In that regime the two are comparable — the value
of this split is NOT throughput. It is that neither branch offers the
planner a LIMIT-driven early-exit bet, so it cannot degenerate into the
backward walk regardless of statistics. Even post-ANALYZE the planner
still estimates rows=96 for a zero-match `entity_ids @>` key, so the old
shape stays one autovacuum re-sample away from the incident plan.
Wall-clock is not quoted: on a live server the same old-shape query
measured 255ms and 915ms on consecutive runs while its buffer count
fell. Buffers are the stable metric here.
Semantics are unchanged: both forms return the highest-serverSeq row
across the union of the two predicates. Ties need no tie-break because
@@unique([userId, serverSeq]) makes an equal serverSeq the same row.
The legacy GLOBAL_CONFIG:misc alias path is untouched.
The batch paths (detectConflictForEntities, prefetchLatestEntityOpsForBatch)
are not exposed to this trap — no LIMIT, and DISTINCT ON forces full
evaluation — and are left unchanged.
* fix(supersync): force the GIN path with a MATERIALIZED CTE
The previous commit's array branch used `aggregate({_max: {serverSeq}})`,
relying on Prisma's `OFFSET 0` subquery as a planner fence. Production
EXPLAIN under `SET plan_cache_mode = force_generic_plan` shows that does
not work:
Aggregate
-> Index Scan using operations_user_id_entity_type_entity_id_server_seq_idx
Index Cond: ((user_id = $1) AND (entity_type = $2))
Filter: (entity_ids @> ARRAY[$3])
It scans the whole (user_id, entity_type) slice — ~88k rows for a
deep-history user. Dropping LIMIT removed the early-exit bet but did not
force the GIN path: the planner costs btree+filter at ~80 against ~875
for the GIN bitmap and takes the btree.
The root reason both earlier designs were misjudged: Prisma sends
parameterized prepared statements, so after ~5 executions Postgres builds
a GENERIC plan that cannot see parameter values. Every EXPLAIN run with
literal constants measures a custom plan production never receives. Two
designs looked correct that way and were catastrophic in production.
Array branch is now raw SQL:
WITH cand AS MATERIALIZED (
SELECT user_id, entity_type, server_seq FROM operations
WHERE entity_ids @> ARRAY[$1]::text[]
)
SELECT MAX(server_seq)::int AS "maxSeq" FROM cand
WHERE user_id = $2 AND entity_type = $3
Inside the CTE the only predicate is `entity_ids @>`, so the composite
btree has no usable leading column and GIN is the only option — the plan
is forced structurally rather than won on cost, which is what makes it
survive statistics drift. MATERIALIZED prevents the outer predicates
being pushed down, which would hand the btree back.
Adding `server_seq > <scalar>` to bound the read was evaluated and
rejected: under generic planning the value is invisible, so it lands as a
post-GIN filter and buys nothing, and placing it inside the CTE creates a
new cliff (no leading-column anchor, so cost scales with total table size
rather than the probing user's history).
Semantics unchanged: verified by differential testing old-vs-new across
20,000 randomized in-memory histories and 1,500 against real Postgres,
zero divergences, with 18 sabotage mutations confirming the harness can
fail.
Tests: mocks moved from `aggregate` to `$queryRaw` behind a shared
discriminator; all plan assertions now run under force_generic_plan; the
plan spec's block accounting no longer double-counts (Postgres buffer
counts are cumulative, so parents already include children).
* test(supersync): make the conflict-lookup plan spec able to fail
The plan spec could not catch the outage it was written for. Three
independent problems, each of which made a load-bearing property
invisible:
1. The spec seeded ONE user and ONE entity_type. In that shape the GIN
estimate (DEFAULT_CONTAIN_SEL * whole table) and the btree-slice
estimate (N / users*entity_types) are the same rows, so GIN always
wins on cost and no regression is detectable. Reseeded to 20k rows for
the probed user plus 20k across ~20k other users over 8 entity types,
which reproduces production's plan node-for-node. Seeds in ~490ms.
2. The measuring shim rebuilt the array-branch SQL from a CONSTANT, so
changing MAX(server_seq) to MIN in conflict.ts could never be
observed. It now reconstructs the SQL from the real tagged template,
putting the aggregate, the MATERIALIZED fence and the CTE shape
genuinely under test.
3. The shim ignored `select`, so dropping actionType from the array
branch was invisible — that field decides whether concurrent
time-tracking deltas merge or get rejected. It now honors `select` and
throws on an unmapped key.
Before this, all three of these mutations left the suite green:
MAX->MIN (silently accepts a write that should conflict, overwriting a
concurrent remote edit), dropping actionType (rejects a client's tracked
time), and hardcoding userId in the findUnique.
Measured after reseeding: shipped CTE 2 blocks / 0 rows filtered; the old
OR, the flat MAX, Prisma's aggregate form, and MATERIALIZED-dropped all
land at 806 blocks / 2500 filtered. One budget assertion (<100 blocks)
now catches every known route back to the incident, with a 403x margin.
All plan assertions run under force_generic_plan; plan-NAME assertions
were removed after measurement showed the old OR no longer plans as a
Backward walk at this seed.
Also corrects a false claim this change introduced. The comment justified
the cross-tenant CTE scan with "entity ids are client-generated nanoids,
so overlap is not expected". The client hard-codes globally shared ids
(TODAY, EM_URGENT, EM_IMPORTANT, KANBAN_IN_PROGRESS, INBOX_PROJECT,
EISENHOWER_MATRIX, KANBAN_DEFAULT, plus fixed GLOBAL_CONFIG keys) which
are byte-identical for every user, and updateBoard({id:'KANBAN_DEFAULT'})
routes here as a single-entity op. Measured: 3885 blocks to reach the 10
rows belonging to the probing user, versus 4 for a unique id. Correctness
is preserved by the outer WHERE user_id; cost is not bounded per-user.
The fix is a btree_gin composite index, which needs a real-Postgres
EXPLAIN (PGlite lacks the extension) and is filed separately.
Deletions: the entity-ids-conflict helper that hand-wrote SQL production
never sends and modelled the abandoned aggregate design (-71 lines), a
dead aggregate mock that would have MASKED a regression by returning an
undefined max, the server_seq-bound test that asserted a rejected design
still works (so it could never fail informatively), and four stale
comments describing a call that no longer exists.
sync-operations.spec.ts had a $queryRaw mock never wired to the array
branch; it fell through to a vestigial value that reads as null, so every
conflict assertion there ran with the array branch stubbed out. Wired up,
and all six raw-query sites now throw on an unrecognised query rather
than falling through.
* test(supersync): seed the conflict-lookup plan spec in production order
Building the GIN after the bulk load produced a pending-list-free index and
measured 2 blocks for the array branch - a state production only sees right
after a vacuum. With the index pre-existing and rows arriving one at a time,
as in production, the same query reads 140 blocks against a 14x larger index.
Seed in production order and never vacuum, so the spec measures the worst
realistic steady state, and re-derive the budget (100 -> 300) against it.
Also move the GIN-not-btree structural assertions onto the real tagged
template, and replace the five hardcoded regression shapes with a single
canary whose stated job is proving the seed still reproduces the mis-plan.
The five EXPLAINed strings that exist only in the test file and responded to
no source mutation; the budget and plan assertions on the real SQL do.
* docs(supersync): correct two false claims in the conflict-lookup comment
The cross-tenant scan vector was wrong. updateTag({id:'TODAY'}) and
updateBoard({id:'KANBAN_DEFAULT'}) are single-entity, so getStoredEntityIds
persists '{}' and they never enter the GIN under that id. The actual vector is
the bulk sortBoards action, which stores the shared board ids in entity_ids.
'Needs btree_gin, unverifiable in PGlite' was also wrong: a GIN index on
(ARRAY['u:'||user_id] || entity_ids) has a text[] operand served by the
built-in array_ops, builds in PGlite with no extension, and was measured
flat against a baseline that scales with tenant count. Recorded as a
tradeoff rather than a free win - it needs the predicate rewritten to match
and indexes every row rather than the multi-entity minority.
Drop the force_generic_plan methodology lecture duplicated from the spec
header, keeping a pointer.
Also make the #8334 mock honest: StoredRow now requires actionType and
findUnique honours select, so a dropped column changes what it returns
instead of silently yielding undefined.
* test(supersync): cover the scalar branch picking the newest op
Flipping detectConflictForEntity's scalar orderBy from 'desc' to 'asc' passed
the entire 914-test server suite. Every existing case gave an entity at most
one scalar row, so the ordering was unobservable.
The bug it hides is silent data loss: with the oldest row returned, conflict
detection compares the incoming clock against a stale one, so an op that is a
clean successor of the OLD state but CONCURRENT with the current one is
accepted and overwrites a remote edit.
Seed two scalar rows whose verdicts differ, and verify by mutation that the
test is red under 'asc' and green under 'desc'.
* 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(supersync): pin the two fail-open invariants in the conflict lookup
Both are unreachable today and both fail toward ACCEPTING a conflicting
write if a future edit breaks them, which is silent data loss rather than
an error. No runtime guards - they could never fire, and the sync layer does
not need more unreproducible hardening. Documented at the exact spot an
editor would stand instead.
- The aggregate fold is safe only because a bare aggregate returns exactly
one row; a GROUP BY (or a revert to Prisma aggregate()) makes zero rows
possible, which reads as 'no prior op'.
- 'arrayOp ?? scalarOp' conflates 'array branch lost' with 'array row
vanished'. The second needs RepeatableRead to stay unreachable.
* docs(supersync): stop overclaiming GIN is forced; fix the batch-lookup doc
Three misleading safety claims, all in text a future contributor would rely on:
- vector-clocks.md still documented the batch lookup as the mutually exclusive
'CASE WHEN cardinality(entity_ids) > 0 THEN entity_ids ELSE ARRAY[entity_id]'.
Production unions the two columns instead, and the exclusive form is the
#8334 bug: it drops a scalar that is not a member of its own entity_ids, so
a later concurrent write to that entity is wrongly accepted - silent data
loss. The section above it was rewritten last commit; this one was missed.
- 'The CTE wins STRUCTURALLY, not on cost' overstated. Removing the competing
btree is structural; GIN being chosen is not. A sequential scan is always
available and wins for an unselective id - reproducible on PG16, and exactly
what a globally shared entity id produces. Reframed as a measured outcome,
since a confident comment asserting what the planner will do is what
preceded the outage.
- The plan spec still said production receives 'a CUSTOM plan nobody
receives'. It receives custom plans for roughly the first 5 executions
before settling generic, and this file covers only the generic mode.
* docs(supersync): correct the plan-cache and aggregate claims; match Int in tests
Round-4 review. Four inaccuracies, all in text a contributor would rely on.
- plan_cache_mode=auto does NOT unconditionally switch to generic after 5
executions: it compares the generic cost against the average custom cost
and may keep using custom plans indefinitely. Stated as an observation for
THIS statement (custom_plans=5, generic_plans=15 on production) rather than
a universal rule, in conflict.ts, the plan spec and vector-clocks.md.
Note: commit
|
||
|---|---|---|
| .. | ||
| long-term-plans | ||
| plans | ||
| promotion | ||
| research | ||
| screens | ||
| sync-and-op-log | ||
| wiki | ||
| add-new-integration.md | ||
| android-edge-to-edge-keyboard.md | ||
| apple-release-automation.md | ||
| build-and-publish-notes.md | ||
| documentation-guide.md | ||
| ENV_SETUP.md | ||
| github-access-token-instructions.md | ||
| gitlab-access-token-instructions.md | ||
| handover.md | ||
| how-to-rate.md | ||
| howto-refresh-snap-credentials.md | ||
| i18n-script-usage.md | ||
| legacy-webview-analysis.md | ||
| mac-app-store-code-signing-guide.md | ||
| non-sync-code-simplification-audit.md | ||
| performance-project-tag-report.md | ||
| plainspace-api-extension-plan.md | ||
| plainspace-integration-plan.md | ||
| plugin-development.md | ||
| styling-guide.md | ||
| theming-contract.md | ||
| TRANSLATING.md | ||
| unused-translations-analysis.md | ||
| update-android-app.md | ||
| update-mac-certificates.md | ||