fix(db): a key in BasicIndex/BTreeIndex lives in at most one bucket#1517
Open
kevin-dp wants to merge 3 commits into
Open
fix(db): a key in BasicIndex/BTreeIndex lives in at most one bucket#1517kevin-dp wants to merge 3 commits into
kevin-dp wants to merge 3 commits into
Conversation
Pin down the invariant that calling update or add on an index never leaves the same key in two buckets at once, even when the caller- supplied oldItem disagrees with what the index recorded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: +192 B (+0.17%) Total Size: 114 kB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 4.24 kB ℹ️ View Unchanged
|
Both BasicIndex and BTreeIndex now keep a key->indexedValue map and use it on remove/update instead of evaluating the caller-supplied oldItem. add() also drops the key from its current bucket first so a missed remove (or a second add) can't leave the key in two buckets at once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kevin-dp
changed the title
Use describe.each to run each test against both index implementations instead of duplicating the BasicIndex and BTreeIndex variants by hand. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
Collaborator
|
I reviewed this alongside #1691 and the stacked follow-up in K-Mistele#1. This PR has the right core invariant, but it can be made both faster and more thoroughly tested once that stack lands. Suggested implementation after #1691Treat the reverse
This fits #1691's direct bucket helpers and avoids the old public Evaluate The cost remains one extra map entry per indexed key, per index. That memory trade-off buys an authoritative one-key/one-bucket invariant and avoids scanning buckets. Property-test expansionThe stacked follow-up adds model-based property tests for both
For this PR, extend the action generator with:
After every action, also assert directly that each modeled key occurs in exactly one bucket and that its bucket agrees with the reference model. Keep the optimistic-to-synced collection regression in this PR as the end-to-end proof. Interaction with the stacked follow-upThe follow-up currently has tests that pin the legacy evaluator-failure fallback and a narrow LandingMy plan is to pick this up after #1691 and K-Mistele#1 land, then adapt this branch with normal follow-up commits. That keeps the performance work and the one-key/one-bucket correctness change independently reviewable while letting this PR use the new property-test and update infrastructure. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Both
BasicIndexandBTreeIndexcould leave a key in two buckets at once wheneverupdate/removewas called with anoldItemthat evaluated to a value other than the one the index actually had recorded for that key —removewould silently no-op on the bucket map while the subsequentaddplaced the key in a new bucket. Callingaddtwice for the same key (e.g. when an upstream "update" change message arrives without apreviousValue, so the collection treats it as an insert) had the same effect.This is the underlying cause of the user-reported "stale auto-index after optimistic→synced update on the indexed column" bug, where a kanban card snaps back to its old column because the auto-index still has the row in the old stage's bucket.
The fix: each index now tracks every key's currently-indexed value internally and uses that on
remove/updateinstead of trusting the caller-suppliedoldItem.addalso drops the key from its current bucket first if it's already indexed.The first commit adds a regression test that demonstrates the invariant violation (and fails on
main); the second commit lands the fix.Test plan
tests/index-key-bucket-tracking.test.tsfails onmainand passes after the fix@tanstack/dbindex, auto-index, subscribe-changes tests still pass@tanstack/db+@tanstack/offline-transactions+@tanstack/electric-db-collectionsuites still pass locally🤖 Generated with Claude Code