fix(db): publish one event per key for metadata-only sync commits - #1795
fix(db): publish one event per key for metadata-only sync commits#1795elylucas wants to merge 1 commit into
Conversation
A sync transaction that writes only row metadata, with no row operations, still retires a completed optimistic mutation and flips virtual props such as `$synced`. `commitPendingTransactions` accounted for that, but `capturePreSyncVisibleState` derived its key set from row operations alone, so the key never entered `recentlySyncedKeys`. `recomputeOptimisticState` and `commitPendingTransactions` then each emitted the same transition, and the published batch named one key twice. That is a malformed diff: the live query counts the old row at -2 and the new one at +2, so the next optimistic write on the key leaves two positive contributors and the keyed reduction throws "Query contributors with the same row key are not congruent". Extract the shared key derivation into `collectAffectedKeys` and use it in all three sites that ask which keys a sync commit touches, so they cannot drift apart again. Fixes TanStack#1767 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe collection state now includes metadata-only sync keys when tracking affected rows. Sync commit, cancellation, and pre-sync capture use the shared helper. New regression tests cover duplicate events and consecutive optimistic writebacks. ChangesMetadata-only sync event deduplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change prevents duplicate events for metadata-only sync commits and protects optimistic writebacks from corrupting keyed query state; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes address issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
🎯 Changes
Fixes #1767.
Two optimistic writes in a row on the same key blow up on the second one:
What's going on
When a collection publishes a batch of changes, that batch is a diff, so each key should appear in it at most once. Here it appeared twice, with both events saying the same thing.
The sequence, using a query collection as the example:
mutationFnacknowledges it withutils.writeUpsert.writeUpsertrefreshes the query cache, and applying that result records row ownership metadata. That metadata write goes into its own sync transaction — one with no row operations at all, just a metadata entry.$synced: false → true), and both emit an event for it. Both land in the same published batch.The guard meant to prevent that double-emit is
recentlySyncedKeys, seeded bycapturePreSyncVisibleState. But it derived its keys from a sync transaction's row operations only, whilecommitPendingTransactionsderives its keys from operations plus row metadata writes. A metadata-only transaction fell straight through the gap between those two sets.Downstream, the duplicated diff leaves the live query counting the old row at -2 and the new one at +2. The next optimistic write on that key then leaves two positive contributors with different values, which is exactly what the keyed reduction rejects.
That also explains the shape of the bug report: round one is what corrupts the bookkeeping, and round two is what notices.
The fix
Pull the "which keys does this sync commit touch" derivation into a single helper,
collectAffectedKeys, and use it in all three places that ask the question —commitPendingTransactions,capturePreSyncVisibleState, andcancelPendingSyncedTransaction. The bug was those key sets drifting apart, so this keeps them from drifting again rather than just patching the one that was behind.Behaviour change is limited to the two sites that were missing metadata keys.
commitPendingTransactionsalready had them.Tests
Two regression tests, both of which fail with the exact error before the fix:
packages/db/tests/collection-metadata-only-sync-commit.test.ts— core-level. Drives a metadata-only sync transaction directly through the sync API while an optimistic mutation is in flight, and asserts no published batch ever names one key twice.packages/query-db-collection/tests/optimistic-writeback.test.ts— the reported path, viautils.writeUpsert.Why the existing tests missed it
The suite covers sync transactions that carry row operations, and covers row metadata, but never a transaction that writes metadata with no row operations while an optimistic mutation is pending — the one shape where the two key sets disagree. The core test above closes that gap, and its "a batch never names a key twice" assertion is the more general form of the invariant, so it should catch other ways of producing a malformed batch too.
Verification
db3071 ·query-db-collection170 + 114 e2e ·electric242 ·react-db161 ·offline-transactions65 ·powersync92 · plus trailbase, rxdb, solid, svelte, vue and angular — all passing, typecheck and lint clean.One note:
collection-events.test.ts > should emit events with correct structuretimes out under full-suite load on my machine. It does the same on an unmodifiedmain(3 runs out of 3), so it looks pre-existing and unrelated to this change.✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests