test(db): reproduce stale buffered sync when optimistic snapback races a persisting transaction#1561
Closed
kevin-dp wants to merge 1 commit into
Closed
Conversation
…s a persisting transaction Adds `tests/auto-index-snapback.test.ts` with the failing case marked `it.fails`. See PR description for the full bug write-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughA new Vitest test suite validates auto-index snapback behavior during optimistic updates with concurrent in-flight transactions. The suite includes a baseline scenario confirming correct state and live-query updates after optimistic+sync cycles, and a bug-reproduction scenario modeling overlapping transactions where one resolves before buffered sync delivery while another persists. ChangesAuto-index snapback validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/db/tests/auto-index-snapback.test.ts (7)
11-11: ⚡ Quick winUse descriptive parameter name for the resolve callback.
The parameter
ris not descriptive. Consider renaming it toresolvefor clarity. As per coding guidelines, prefer descriptive variable names that describe role or purpose.♻️ Proposed fix
-const flushTasks = () => new Promise((r) => setTimeout(r, 0)) +const flushTasks = () => new Promise((resolve) => setTimeout(resolve, 0))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/auto-index-snapback.test.ts` at line 11, Rename the anonymous resolve parameter in the flushTasks helper to a descriptive name: change the arrow function new Promise((r) => setTimeout(r, 0)) to use new Promise((resolve) => setTimeout(resolve, 0)) so the role of the parameter is clear; update any references inside the promise callback accordingly in the flushTasks definition.
117-124: ⚡ Quick winUse descriptive parameter name for the resolve callback.
The parameter
rin the Promise constructor is not descriptive. Consider renaming it toresolvefor clarity. As per coding guidelines, use descriptive variable names that describe role or purpose.♻️ Proposed fix
let resolveT1!: () => void const t1 = createTransaction<Row>({ autoCommit: false, mutationFn: async () => { - await new Promise<void>((r) => { - resolveT1 = r + await new Promise<void>((resolve) => { + resolveT1 = resolve }) }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/auto-index-snapback.test.ts` around lines 117 - 124, Rename the anonymous Promise constructor parameter `r` to a descriptive name (e.g., `resolve`) inside the mutationFn where `resolveT1` is assigned: locate the createTransaction<Row> block that sets `resolveT1` via `new Promise<void>((r) => { resolveT1 = r })` and change the parameter to `resolve` so it reads `new Promise<void>((resolve) => { resolveT1 = resolve })` to improve clarity.
37-44: ⚡ Quick winUse descriptive parameter name for the resolve callback.
The parameter
rin the Promise constructor is not descriptive. Consider renaming it toresolvefor clarity. As per coding guidelines, use descriptive variable names that describe role or purpose.♻️ Proposed fix
let resolveMutation!: () => void const tx = createTransaction<Row>({ autoCommit: false, mutationFn: async () => { - await new Promise<void>((r) => { - resolveMutation = r + await new Promise<void>((resolve) => { + resolveMutation = resolve }) }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/auto-index-snapback.test.ts` around lines 37 - 44, The Promise constructor inside the mutationFn uses a non-descriptive parameter name `r`; rename it to `resolve` (or another clear name) to improve clarity: update the Promise callback in the test where `resolveMutation` is assigned (inside createTransaction<Row> with autoCommit: false and mutationFn) to use `resolve` instead of `r`, and ensure the existing `resolveMutation = resolve` assignment and any references remain correct.
114-114: ⚡ Quick winAvoid
anytype in map callbacks.The map callback parameters are typed as
any. Consider providing proper type annotations. As per coding guidelines, avoid usinganytypes and provide proper type annotations.♻️ Proposed fix
-expect(liveA.toArray.map((r: any) => r.id).sort()).toEqual([`P`, `Q`]) +expect(liveA.toArray.map((r: Row) => r.id).sort()).toEqual([`P`, `Q`]) // ... later in the test ... -expect(liveA.toArray.map((r: any) => r.id)).toEqual([]) +expect(liveA.toArray.map((r: Row) => r.id)).toEqual([]) // ... later in the test ... -expect(liveA.toArray.map((r: any) => r.id).sort()).toEqual([`Q`]) +expect(liveA.toArray.map((r: Row) => r.id).sort()).toEqual([`Q`])Also applies to: 147-147, 177-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/auto-index-snapback.test.ts` at line 114, The test uses an `any` typed map callback in expressions like `liveA.toArray.map((r: any) => r.id)` (also at the other occurrences around the same test), so replace the `any` with a specific type for the record returned by `toArray` (for example the entity shape used in this suite, e.g. `{ id: string }` or the actual model/interface name you have for these rows) or import and use the appropriate test fixture type; update all three instances (the one using `liveA.toArray.map`, and the similar lines at the other two locations) so the callback signature is strongly typed (e.g. `(r: { id: string }) => r.id`) instead of `(r: any) => r.id`.
28-34: ⚡ Quick winAvoid
anytypes in query callbacks.The query parameter types are declared as
any. Consider providing proper type annotations or usingunknownwith type guards for type safety. As per coding guidelines, avoid usinganytypes and provide proper type annotations.♻️ Proposed fix
const liveA = createLiveQueryCollection({ - query: (q: any) => + query: (q) => q .from({ p: collection }) - .where(({ p }: any) => inArray(p.stage, [`A`])), + .where(({ p }) => inArray(p.stage, [`A`])), startSync: true, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/auto-index-snapback.test.ts` around lines 28 - 34, The query callbacks use unsafe any types (parameters q and the destructured ({ p }: any) in the createLiveQueryCollection call); replace those anys with concrete types or unknown plus type guards: give createLiveQueryCollection the proper generic/document interface (e.g., a Document type that includes stage), type the query parameter (q) with the library's Query/QueryBuilder generic, and change the where callback to accept a strongly typed parameter like ({ p }: { p: Document }) or validate/guard unknown before use so there are no any annotations in createLiveQueryCollection, query, or the where callback.
68-68: ⚡ Quick winAvoid
anytype in map callback.The map callback parameter is typed as
any. Consider providing a proper type annotation. As per coding guidelines, avoid usinganytypes and provide proper type annotations.♻️ Proposed fix
-expect(liveA.toArray.map((r: any) => r.id).sort()).toEqual([`Q`]) +expect(liveA.toArray.map((r: Row) => r.id).sort()).toEqual([`Q`])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/auto-index-snapback.test.ts` at line 68, The map callback uses the `any` type for `r`; replace it with a concrete type such as `{ id: string }` (or the actual element interface used by `liveA.toArray()`), e.g. change `(r: any) => r.id` to `(r: { id: string }) => r.id` (or use the appropriate model/interface) so the test `expect(liveA.toArray.map((r: any) => r.id).sort()).toEqual([`Q`])` avoids `any` while preserving the same behavior.
106-113: ⚡ Quick winAvoid
anytypes in query callbacks.The query parameter types are declared as
any. Consider providing proper type annotations or using type inference for type safety. As per coding guidelines, avoid usinganytypes and provide proper type annotations.♻️ Proposed fix
const liveA = createLiveQueryCollection({ - query: (q: any) => + query: (q) => q .from({ p: collection }) - .where(({ p }: any) => inArray(p.stage, [`A`])), + .where(({ p }) => inArray(p.stage, [`A`])), startSync: true, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/auto-index-snapback.test.ts` around lines 106 - 113, The query callback currently uses `any` for the query and predicate parameters (in createLiveQueryCollection's `query: (q: any) => ...` and `where(({ p }: any) => ...)`); remove the `any` and either let TypeScript infer types (e.g. `query: q => ...` and `where(({ p }) => ...)`) or annotate with the proper collection/query types used in your codebase (for example a `QueryBuilder<T>` or the concrete collection row type) so both the `q` parameter in `createLiveQueryCollection` and the `p` destructured predicate are typed instead of `any`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/tests/auto-index-snapback.test.ts`:
- Line 92: The test declaration for "snapback + buffered sync remains stale
while another transaction is persisting" is currently a regular it(...) and
should be marked as an expected failure; change the test invocation to
it.fails(...) (keeping the same test title string and async callback body) so
Vitest treats this as a known failing test and CI stays green; ensure you only
replace the `it(` call for that test with `it.fails(` and do not alter the test
implementation or assertions inside the async function.
---
Nitpick comments:
In `@packages/db/tests/auto-index-snapback.test.ts`:
- Line 11: Rename the anonymous resolve parameter in the flushTasks helper to a
descriptive name: change the arrow function new Promise((r) => setTimeout(r, 0))
to use new Promise((resolve) => setTimeout(resolve, 0)) so the role of the
parameter is clear; update any references inside the promise callback
accordingly in the flushTasks definition.
- Around line 117-124: Rename the anonymous Promise constructor parameter `r` to
a descriptive name (e.g., `resolve`) inside the mutationFn where `resolveT1` is
assigned: locate the createTransaction<Row> block that sets `resolveT1` via `new
Promise<void>((r) => { resolveT1 = r })` and change the parameter to `resolve`
so it reads `new Promise<void>((resolve) => { resolveT1 = resolve })` to improve
clarity.
- Around line 37-44: The Promise constructor inside the mutationFn uses a
non-descriptive parameter name `r`; rename it to `resolve` (or another clear
name) to improve clarity: update the Promise callback in the test where
`resolveMutation` is assigned (inside createTransaction<Row> with autoCommit:
false and mutationFn) to use `resolve` instead of `r`, and ensure the existing
`resolveMutation = resolve` assignment and any references remain correct.
- Line 114: The test uses an `any` typed map callback in expressions like
`liveA.toArray.map((r: any) => r.id)` (also at the other occurrences around the
same test), so replace the `any` with a specific type for the record returned by
`toArray` (for example the entity shape used in this suite, e.g. `{ id: string
}` or the actual model/interface name you have for these rows) or import and use
the appropriate test fixture type; update all three instances (the one using
`liveA.toArray.map`, and the similar lines at the other two locations) so the
callback signature is strongly typed (e.g. `(r: { id: string }) => r.id`)
instead of `(r: any) => r.id`.
- Around line 28-34: The query callbacks use unsafe any types (parameters q and
the destructured ({ p }: any) in the createLiveQueryCollection call); replace
those anys with concrete types or unknown plus type guards: give
createLiveQueryCollection the proper generic/document interface (e.g., a
Document type that includes stage), type the query parameter (q) with the
library's Query/QueryBuilder generic, and change the where callback to accept a
strongly typed parameter like ({ p }: { p: Document }) or validate/guard unknown
before use so there are no any annotations in createLiveQueryCollection, query,
or the where callback.
- Line 68: The map callback uses the `any` type for `r`; replace it with a
concrete type such as `{ id: string }` (or the actual element interface used by
`liveA.toArray()`), e.g. change `(r: any) => r.id` to `(r: { id: string }) =>
r.id` (or use the appropriate model/interface) so the test
`expect(liveA.toArray.map((r: any) => r.id).sort()).toEqual([`Q`])` avoids `any`
while preserving the same behavior.
- Around line 106-113: The query callback currently uses `any` for the query and
predicate parameters (in createLiveQueryCollection's `query: (q: any) => ...`
and `where(({ p }: any) => ...)`); remove the `any` and either let TypeScript
infer types (e.g. `query: q => ...` and `where(({ p }) => ...)`) or annotate
with the proper collection/query types used in your codebase (for example a
`QueryBuilder<T>` or the concrete collection row type) so both the `q` parameter
in `createLiveQueryCollection` and the `p` destructured predicate are typed
instead of `any`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16905e74-86cf-4598-ba4c-5ae6051c0c13
📒 Files selected for processing (1)
packages/db/tests/auto-index-snapback.test.ts
| // | ||
| // This test asserts the *correct* behaviour and is expected to FAIL until the bug is | ||
| // fixed; the failing CI is intentional. | ||
| it(`snapback + buffered sync remains stale while another transaction is persisting`, async () => { |
Contributor
There was a problem hiding this comment.
Add .fails modifier to mark this test as expected to fail.
The PR description states this test should be "marked it.fails", but the code uses a regular it() call. Vitest's .fails modifier is the standard pattern for bug reproduction tests because:
- It marks the test as expected to fail, keeping CI green
- When the bug is fixed and the test passes, Vitest will fail the test run, prompting you to remove the
.failsmarker - It clearly documents that this is a known failing case
Without .fails, CI will be red, which blocks other PRs and obscures real failures.
🐛 Proposed fix
- it(`snapback + buffered sync remains stale while another transaction is persisting`, async () => {
+ it.fails(`snapback + buffered sync remains stale while another transaction is persisting`, async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it(`snapback + buffered sync remains stale while another transaction is persisting`, async () => { | |
| it.fails(`snapback + buffered sync remains stale while another transaction is persisting`, async () => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/tests/auto-index-snapback.test.ts` at line 92, The test
declaration for "snapback + buffered sync remains stale while another
transaction is persisting" is currently a regular it(...) and should be marked
as an expected failure; change the test invocation to it.fails(...) (keeping the
same test title string and async callback body) so Vitest treats this as a known
failing test and CI stays green; ensure you only replace the `it(` call for that
test with `it.fails(` and do not alter the test implementation or assertions
inside the async function.
Contributor
Author
|
Closing this issue because the behaviour is expected as we don't provide transactional guarantees across shapes. The problem here comes from the fact that each column has its own shape. And moving an item between different columns is a transactions that span 2 shapes. This means that the tx ID will be observed on both shape streams and when the first one notices it it marks that tx ID as seen but actually we have only seen the changes of that transaction to one of the shapes and not yet for the second shape. So this can cause these kind of snapbacks as optimistic state is being dropped etc. |
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
1 participant
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
Adds a failing reproduction test for a stale-data bug originally reported as "auto-index returns the row in the wrong bucket after an optimistic→synced update", and confirms that the same symptom still reproduces against the fix in #1517.
The repro narrows the bug down to a different mechanism than the index-bucket lifecycle issue #1517 addressed: it lives in
commitPendingTransactions's gating onhasPersistingTransaction, not inBasicIndex/BTreeIndexor inisRedundantSync.CI is intentionally red: the failing test describes the correct behaviour, and stays failing until the bug is fixed.
What the test reproduces
Models the user's "fast successive drags" kanban scenario:
T1starts persisting.T2starts persisting.T1'smutationFnresolves (server-confirmed). In the user's app,awaitTxIdraces out via the snapshot path before the shape stream has delivered P's row change — so when the optimistic delta lifts, no buffered sync exists for P, andrecomputeOptimisticStatedrops P's optimistic upsert as "stale". Visible state for P snaps from B back to A.T2is still persisting, socommitPendingTransactionsis gated onhasPersistingTransactionand the sync message sits buffered inpendingSyncedTransactionsindefinitely.T2keeps persisting, anyone reading the live query sees P stuck in stage A — even though the server has confirmed it as stage B, and the sync message for that change is sitting inpendingSyncedTransactions.In the user's app, with continuous drag traffic the "another transaction is persisting" condition is essentially always true, so the buffered sync never flushes → "stays until reload".
The test also includes a baseline case (single drag, no concurrent traffic) that passes — demonstrating that the bug only surfaces when a concurrent persisting transaction is blocking sync application for unrelated keys.
Relationship to #1517
The user originally suspected
BasicIndexwas leaving keys in stale buckets; #1517 addressed that by tracking key→bucket and dropping prior buckets onadd. They retested with #1517 applied and the snapback still reproduced. I ran this repro on bothmainandkevin/stale-index-bucket-repro(the #1517 branch) — same failure mode either way. So this is a separate bug; the user'sawaitMatchworkaround sidesteps it by preventing the premature optimistic lift in step 3.This PR does not propose a fix. The probable direction is for
commitPendingTransactionsto be able to apply syncs for keys not touched by any currently-persisting transaction, instead of all-or-nothing gating onhasPersistingTransaction.Test plan
tests/auto-index-snapback.test.tslocally — baseline passes, bug-repro fails as expected.kevin/stale-index-bucket-repro(fix(db): a key in BasicIndex/BTreeIndex lives in at most one bucket #1517) branch.🤖 Generated with Claude Code
Summary by CodeRabbit