From 34961fc191c79cecf3f4312880511f477df35fab Mon Sep 17 00:00:00 2001 From: Isaac Date: Thu, 1 Oct 2026 20:33:16 -0600 Subject: [PATCH 1/2] fix(db): reject in-place sync row changes without previousValue in development Core keeps the object a sync source writes as the stored row. A source that changed that object in place and wrote it again had already overwritten the previous value, so live queries saw an update whose old and new values matched and kept a row in a filter it left. In development, the write now throws SyncRowReusedWithoutPreviousValueError unless it names previousValue. The check compares a shallow snapshot of each written object, so rewriting an unchanged object, as the live-query Collection does, stays valid. Production builds skip it. Co-authored-by: Isaac --- .changeset/reject-reused-sync-rows.md | 5 + docs/contributing/oracle-coverage.md | 1 + docs/guides/collection-options-creator.md | 15 +++ packages/db/mangle-cache.json | 5 +- packages/db/src/collection/sync.ts | 60 ++++++++++++ packages/db/src/errors.ts | 10 ++ packages/db/tests/sync-reused-row.test.ts | 110 ++++++++++++++++++++++ 7 files changed, 205 insertions(+), 1 deletion(-) create mode 100644 .changeset/reject-reused-sync-rows.md create mode 100644 packages/db/tests/sync-reused-row.test.ts diff --git a/.changeset/reject-reused-sync-rows.md b/.changeset/reject-reused-sync-rows.md new file mode 100644 index 0000000000..117dbf44f4 --- /dev/null +++ b/.changeset/reject-reused-sync-rows.md @@ -0,0 +1,5 @@ +--- +'@tanstack/db': patch +--- + +Throw `SyncRowReusedWithoutPreviousValueError` in development when a sync source changes a row object it already wrote and writes it again without `previousValue`. The collection keeps the written object as the stored row, so the in-place change overwrote the previous value, and live queries could keep the row in a filter it left. Production builds skip the check. diff --git a/docs/contributing/oracle-coverage.md b/docs/contributing/oracle-coverage.md index f7a05e5d54..e27b177dd0 100644 --- a/docs/contributing/oracle-coverage.md +++ b/docs/contributing/oracle-coverage.md @@ -265,6 +265,7 @@ comment and the current API/architecture contract before extending its model. | WHERE predicate publication | [WHERE predicate publication oracle](https://github.com/TanStack/db/blob/main/packages/db/tests/query/where-predicate-publication-oracle.property.test.ts) | An independent Kleene evaluator judges `eq`/`not`/`and`/`or` predicates over strings, a normalization-prefixed string, booleans, numbers, `NaN`, a valid Date, `null`, a missing field, and virtual fields. Snapshot histories with pending optimistic inserts compare a live query, direct subscribers with and without initial state, and `currentStateAsChanges`; change histories apply multi-key sync transactions, including reinsertion of a key deleted earlier, and compare every consumer's key set after each commit, on scan and `BasicIndex` paths. Peer subscribers share one field with different literals, plus one on a virtual field, so every published change must reach exactly the subscribers whose predicate it can satisfy; routing mutants that ignored the previous value, ignored the field, delivered a change twice, or routed while stale published rows awaited reconciliation fail here. Fixed witnesses cover a layout-only publication reaching a filtered subscriber as one empty batch, a filtered subscriber's empty Collection-readiness batch, retraction of a vanished row after eager cleanup and restart, and the new row plus exact subscriber update payload when a same-key row stays TRUE. A stale-live-value mutant survives the generated membership checks but fails the payload witness. A focused [property-visibility test](https://github.com/TanStack/db/blob/main/packages/db/tests/query/where-prefilter-property-visibility.test.ts) compares the public unindexed snapshot with the enriched-row contract for inherited, non-enumerable, enumerable-own, and nested getter paths; the pre-fix stored-row shortcut failed three of its four controls. Other property-descriptor and stateful-getter histories remain outside those fixed controls. Hostile mutants for FALSE-for-UNKNOWN `eq`, `or` or number-literal subscription prefilters, a prefilter that ignores the previous value, a skipped readiness batch, a skip while stale published rows await reconciliation, and an unindexed snapshot scan that reads only synced rows while an optimistic insert or delete changes visibility passed the prior `@tanstack/db` suite and fail here. Change routing withholds a dropped row from an `eq` subscription before its sent key could be recorded, so a mutant that records dropped rows as sent is equivalent for routed predicates in this oracle. A focused `collection-subscription.test.ts` witness checks that a dropped insert or update, alone or beside a matching change, cannot advance a limited subscription's page offset under a routed `eq` and an unrouted `or` predicate; the unrouted cases beside a matching change kill that mutant, and the unrouted `alone` case does not. A loose-equality prefilter is an equivalent mutant: it only skips less. Skipping during truncate replay also survives; the replay's baseline diff re-derives the same retraction, so the guard keeps the prior dataflow without its own witness. Generated cleanup and restart histories for filtered subscribers remain open for the lifecycle publication owner. No-op updates, other comparison operators, Temporal and binary operands, joins, ordering, optimistic updates, and truncate are outside this owner. It runs in `@tanstack/db`'s `test:oracles` campaign. The [2026-09-30 review](https://github.com/TanStack/db/blob/main/docs/contributing/oracle-reviews/2026-09-30-where-predicate-and-join-keys.md) records each ORC outcome. | | Joined result keys | [Joined result key oracle](https://github.com/TanStack/db/blob/main/packages/db/tests/query/join-result-key-oracle.property.test.ts) | A nested-loop model recomputes the pair set of an inner, left, or full join over source keys drawn from plain, comma-bearing, bracket-bearing, and quoted strings, numbers beside the strings that print the same, both infinities, and `NaN`. After preload and each synced group change, the published rows must equal the model's pairs and the result key count must equal the row count. The comma-joined key encoding fails its two pinned histories and both campaigns; plain `JSON.stringify`, which prints infinities as the missing-side `null`, fails the pinned infinity history and both campaigns. A pinned history in which a `NaN`-keyed pair leaves and re-forms fails when the join index compares source-key prefixes with `===`. Joins over subqueries, more than two sources, custom `getKey`, and optimistic mutations are outside this owner. It runs in `@tanstack/db`'s `test:oracles` campaign. The [2026-09-30 review](https://github.com/TanStack/db/blob/main/docs/contributing/oracle-reviews/2026-09-30-where-predicate-and-join-keys.md) records each ORC outcome. | | D2 Index storage | [Index refinement oracle](https://github.com/TanStack/db/blob/main/packages/db-ivm/tests/index-refinement-oracle.property.test.ts) | A plain-`Map` model sums multiplicities under a type-and-value identity in which `NaN` equals `NaN` and `-0` equals `0`. Histories of up to twelve additions to two keys mix prefixed arrays with `NaN`, `0`, `-0`, `1`, `'1'`, `1n`, and `'a'` prefixes and unprefixed values, with cancellation and reappearance. After each addition, `get` and `has` must equal the model. Comparing prefixes with `===`, which disagrees with the `Map` that holds them, fails three pinned histories and both campaigns; the `-0` control passes under both. Compaction, presence tracking, and structural payloads are outside this owner; the join operator tests and incrementalization law own operator behavior. | +| Reused sync row objects | [reused row witness](https://github.com/TanStack/db/blob/main/packages/db/tests/sync-reused-row.test.ts) | Focused witnesses for a sync source that changes a stored row object in place: in development, writing it again as an update without `previousValue` throws, while the same write with `previousValue`, or with a new object, moves the row out of an `eq` live query. Production skips the check. The check compares a shallow snapshot taken when the object was written, so it misses changes to nested fields; a write that names `previousValue` and a row whose fields throw on read are not checked. | | Index suggestions | [collection-size suggestion oracle](https://github.com/TanStack/db/blob/main/packages/db/tests/index-suggestion-oracle.test.ts) | A public filtered live-query Collection checks manual/default, eager, threshold, matching-index, and unrelated-index cases against the documented collection-size suggestion policy. The original manual-silence/eager-noise implementation failed two expected observations. The owner does not judge repeated-query warning volume, slow-query timing, query work, production-build suppression, or every query shape. It runs in `@tanstack/db`'s `test:oracles` campaign. | | Minified DB public API | [consumer bundle check](https://github.com/TanStack/db/blob/main/scripts/test-minified-db.mjs) | CI builds `@tanstack/db`, then bundles its built `dist` entry with db-ivm inlined through esbuild `minify: true`. The build renames TypeScript-private members from `packages/db/mangle-cache.json`, so this lane runs against the renamed output that consumers install. It rejects a `dist` that is older than `src`, the cache, or the package's build configuration. `pnpm --filter @tanstack/db test:dist` also runs five db test files against the built `dist`, chosen for coverage of the modules with the most renamed members. `pnpm check:mangle` fails when a cached name is used other than as a private member in db src, is read by another package, appears as a string, or has a short name that collides with a source identifier ([private-member mangling review](oracle-reviews/code-weight-private-member-mangling.md)). It checks every exported error class's current public `name`, the built-in `BasicIndex` resolver name in index metadata and its event, complete query rows (after removing the four documented virtual fields) against an independent array filter/sort/projection, and one live update. The `new.target.name`, public-member mangle, and extra-output-field calibration modes fail at the intended observations. This fixed slice does not replace the unminified generated oracles, exercise framework adapters, or establish stable names for custom index resolvers. | | Boundary refinements | [cleanup/restart](https://github.com/TanStack/db/blob/main/packages/db/tests/collection-cleanup-restart-oracle.test.ts), [issue #1891 async-cleanup review](oracle-reviews/issue-1891-async-cleanup.md), [issue #1891 cleanup-start follow-up](oracle-reviews/issue-1891-cleanup-start-followup.md), [metadata publication](https://github.com/TanStack/db/blob/main/packages/db/tests/collection-metadata-publication-oracle.property.test.ts), [state retention](https://github.com/TanStack/db/blob/main/packages/db/tests/collection-state-retention-oracle.property.test.ts), [acquisition cells](https://github.com/TanStack/db/blob/main/packages/db/tests/collection-subscription-lifecycle-oracle.test.ts), [D2 source reconciliation](https://github.com/TanStack/db/blob/main/packages/db/tests/d2-source-reconciliation-oracle.property.test.ts), [top-K support windows](https://github.com/TanStack/db/blob/main/packages/db-ivm/tests/operators/topk-support-window-oracle.test.ts), [nested Query work](https://github.com/TanStack/db/blob/main/packages/query-db-collection/tests/includes-work-counter-oracle.test.ts) | Explicit lifecycle products, independent source maps and weighted relations, exact publication cuts, support/multiplicity, and value-plus-work observations. These refine the larger subsystem models; they do not replace them. | diff --git a/docs/guides/collection-options-creator.md b/docs/guides/collection-options-creator.md index 0c63762faa..771f22ccb0 100644 --- a/docs/guides/collection-options-creator.md +++ b/docs/guides/collection-options-creator.md @@ -180,6 +180,21 @@ applying its rows. A successful `loadSubset` must await or return every commit receipt that establishes its result. Do not use `begin({ immediate: true })` to bypass that ordering just to settle a load. +The collection keeps the object you pass to `write()` as the row's stored +value. Pass a new object for each update. If your source changes a row object +in place and writes it again, pass the row's previous value as +`previousValue`. Without it, the change already overwrote the value the +collection needs to publish the update, and live queries can keep the row in +a result it left. In development, the collection throws +`SyncRowReusedWithoutPreviousValueError` for that write. + +```ts +// Changes a stored row in place, so it must name the previous value +const previousValue = { ...row } +row.status = `done` +write({ type: `update`, value: row, previousValue }) +``` + For request-scoped writes, pass the request's abort signal to `commit(signal)`. Cancellation before application rejects the receipt with `AbortError`; aborting after application does not undo published rows. Do not attach one request's diff --git a/packages/db/mangle-cache.json b/packages/db/mangle-cache.json index c5df07ef3d..5e8c5034ed 100644 --- a/packages/db/mangle-cache.json +++ b/packages/db/mangle-cache.json @@ -366,5 +366,8 @@ "canRetryRepair": "f0", "restore": "f1", "attach": "f2", - "compilations": "f3" + "compilations": "f3", + "writtenRows": "f4", + "equalityRoute": "f5", + "checkReusedRow": "f6" } diff --git a/packages/db/src/collection/sync.ts b/packages/db/src/collection/sync.ts index d220b4294b..d30a7d130b 100644 --- a/packages/db/src/collection/sync.ts +++ b/packages/db/src/collection/sync.ts @@ -7,6 +7,7 @@ import { NoPendingSyncTransactionCommitError, NoPendingSyncTransactionWriteError, SyncCleanupError, + SyncRowReusedWithoutPreviousValueError, SyncTransactionAlreadyCommittedError, SyncTransactionAlreadyCommittedWriteError, } from '../errors' @@ -46,6 +47,27 @@ type LoadSubsetOperation = { deferred?: Deferred } +function shallowEqual( + left: Record, + right: Record, +): boolean { + const keys = Object.keys(left) + return ( + keys.length === Object.keys(right).length && + keys.every((key) => Object.is(left[key], right[key])) + ) +} + +// Bundlers inline `process.env.NODE_ENV`; without a bundler or `process`, +// the development checks stay off. +function isDevelopment(): boolean { + try { + return process.env.NODE_ENV !== `production` + } catch { + return false + } +} + export class CollectionSyncManager< TOutput extends object = Record, TKey extends string | number = string | number, @@ -59,6 +81,8 @@ export class CollectionSyncManager< private config!: CollectionConfig private id: string private syncMode: `eager` | `on-demand` + // Development only: each written row object's fields when it was written. + private writtenRows: WeakMap> | undefined public preloadPromise: Promise | null = null private rejectPreload?: (error: unknown) => void @@ -111,6 +135,38 @@ export class CollectionSyncManager< this._events = deps.events } + /** + * Core keeps the object a source writes as the stored row. A source that + * changes that object in place and writes it again has already + * overwritten the previous value core would publish, unless it passes + * `previousValue`. Rewriting an unchanged object stays valid. + */ + private checkReusedRow( + key: TKey, + type: string, + message: { value: TOutput; previousValue?: TOutput }, + ): void { + const value = message.value as Record + const writtenRows = (this.writtenRows ??= new WeakMap()) + try { + const written = writtenRows.get(value) + if ( + written && + type === `update` && + // A write that names its previous value declares the reuse. + !(`previousValue` in message) && + !shallowEqual(written, value) + ) { + throw new SyncRowReusedWithoutPreviousValueError(key) + } + writtenRows.set(value, { ...value }) + } catch (error) { + if (error instanceof SyncRowReusedWithoutPreviousValueError) throw error + // A row that cannot be read reports its failure where it is applied. + writtenRows.delete(value) + } + } + private createDuplicateKeyError(key: TKey): DuplicateKeySyncError { const utils = this.config.utils as | Partial @@ -219,6 +275,10 @@ export class CollectionSyncManager< messageType = disposition } + if (`value` in messageWithOptionalKey && isDevelopment()) { + this.checkReusedRow(key, messageType, messageWithOptionalKey) + } + const message = { ...messageWithOptionalKey, type: messageType, diff --git a/packages/db/src/errors.ts b/packages/db/src/errors.ts index 3e3f4dd745..8c20f16881 100644 --- a/packages/db/src/errors.ts +++ b/packages/db/src/errors.ts @@ -393,6 +393,16 @@ export class SyncTransactionAlreadyCommittedWriteError extends TransactionError } } +export class SyncRowReusedWithoutPreviousValueError extends TransactionError { + constructor(key: string | number) { + super( + `A sync update for key "${key}" wrote a row object that changed in place since it was last written. ` + + `The change overwrote the row's previous value. ` + + `Write a new object, or pass the row's previous value as \`previousValue\`.`, + ) + } +} + export class NoPendingSyncTransactionCommitError extends TransactionError { constructor() { super(`No pending sync transaction to commit`) diff --git a/packages/db/tests/sync-reused-row.test.ts b/packages/db/tests/sync-reused-row.test.ts new file mode 100644 index 0000000000..63747e2b1b --- /dev/null +++ b/packages/db/tests/sync-reused-row.test.ts @@ -0,0 +1,110 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { createCollection } from '../src/collection/index.js' +import { SyncRowReusedWithoutPreviousValueError } from '../src/errors.js' +import { createLiveQueryCollection, eq } from '../src/query/index.js' +import type { SyncConfig } from '../src/types.js' + +/** + * A sync update tells the Collection a row's new value. Core keeps the + * object a source writes as the row's stored value, so a source that + * changes that object in place and writes it again has already overwritten + * the previous value core would publish. Live queries then see an update + * whose old and new values are the same object, and a row that left a + * filter stays in it. A source that reuses its row object must say what the + * row was through `previousValue`. In development, core rejects the write + * that omits it; production keeps the cheaper unchecked path. + */ +type Row = { id: string; group: string } + +function setup() { + let sync!: Parameters[`sync`]>[0] + const row: Row = { id: `r`, group: `a` } + const collection = createCollection({ + id: `sync-reused-row`, + getKey: (item) => item.id, + startSync: true, + sync: { + rowUpdateMode: `full`, + sync: (actions) => { + sync = actions + actions.begin() + actions.write({ type: `insert`, value: row }) + actions.commit() + actions.markReady() + }, + }, + }) + const groupA = createLiveQueryCollection({ + query: (q) => q.from({ r: collection }).where(({ r }) => eq(r.group, `a`)), + startSync: true, + }) + return { sync: () => sync, row, collection, groupA } +} + +describe(`sync writes of a reused row object`, () => { + afterEach(() => vi.unstubAllEnvs()) + + it(`rejects an in-place update without previousValue in development`, async () => { + vi.stubEnv(`NODE_ENV`, `development`) + const { sync, row, collection, groupA } = setup() + await groupA.preload() + row.group = `b` + sync().begin() + expect(() => sync().write({ type: `update`, value: row })).toThrow( + SyncRowReusedWithoutPreviousValueError, + ) + await collection.cleanup() + }) + + it(`accepts an in-place update that names its previous value`, async () => { + vi.stubEnv(`NODE_ENV`, `development`) + const { sync, row, collection, groupA } = setup() + await groupA.preload() + expect([...groupA.keys()]).toEqual([`r`]) + const previousValue = { ...row } + row.group = `b` + sync().begin() + sync().write({ type: `update`, value: row, previousValue }) + sync().commit() + expect([...groupA.keys()]).toEqual([]) + await collection.cleanup() + }) + + it(`accepts an unchanged rewrite after a declared in-place update`, async () => { + vi.stubEnv(`NODE_ENV`, `development`) + const { sync, row, collection, groupA } = setup() + await groupA.preload() + const previousValue = { ...row } + row.group = `b` + sync().begin() + sync().write({ type: `update`, value: row, previousValue }) + sync().commit() + // The live-query Collection rewrites an unchanged row the same way. + sync().begin() + expect(() => sync().write({ type: `update`, value: row })).not.toThrow() + sync().commit() + await collection.cleanup() + }) + + it(`accepts an update with a new object`, async () => { + vi.stubEnv(`NODE_ENV`, `development`) + const { sync, collection, groupA } = setup() + await groupA.preload() + sync().begin() + sync().write({ type: `update`, value: { id: `r`, group: `b` } }) + sync().commit() + expect([...groupA.keys()]).toEqual([]) + await collection.cleanup() + }) + + it(`does not check in production`, async () => { + vi.stubEnv(`NODE_ENV`, `production`) + const { sync, row, collection, groupA } = setup() + await groupA.preload() + row.group = `b` + sync().begin() + expect(() => sync().write({ type: `update`, value: row })).not.toThrow() + sync().commit() + await collection.cleanup() + }) +}) From 5a7a8104dc345e4e1c8bde4996f6a0eb8a23ed56 Mon Sep 17 00:00:00 2001 From: Isaac Date: Fri, 2 Oct 2026 09:20:59 -0600 Subject: [PATCH 2/2] fix: list the reused-row error and state the check's shallow scope The minified public API check lists every exported error, so it needs SyncRowReusedWithoutPreviousValueError. The guide and changeset now say the development check compares shallow copies and misses nested changes. Co-authored-by: Isaac --- .changeset/reject-reused-sync-rows.md | 2 +- docs/guides/collection-options-creator.md | 4 +++- scripts/test-minified-db.mjs | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.changeset/reject-reused-sync-rows.md b/.changeset/reject-reused-sync-rows.md index 117dbf44f4..2ad5397fdb 100644 --- a/.changeset/reject-reused-sync-rows.md +++ b/.changeset/reject-reused-sync-rows.md @@ -2,4 +2,4 @@ '@tanstack/db': patch --- -Throw `SyncRowReusedWithoutPreviousValueError` in development when a sync source changes a row object it already wrote and writes it again without `previousValue`. The collection keeps the written object as the stored row, so the in-place change overwrote the previous value, and live queries could keep the row in a filter it left. Production builds skip the check. +Throw `SyncRowReusedWithoutPreviousValueError` in development when a sync source changes a top-level field of a row object it already wrote and writes it again without `previousValue`. The collection keeps the written object as the stored row, so the in-place change overwrote the previous value, and live queries could keep the row in a filter it left. The check compares shallow copies, so it does not detect a change inside a nested object. Production builds skip the check. diff --git a/docs/guides/collection-options-creator.md b/docs/guides/collection-options-creator.md index 771f22ccb0..e0453eea4c 100644 --- a/docs/guides/collection-options-creator.md +++ b/docs/guides/collection-options-creator.md @@ -186,7 +186,9 @@ in place and writes it again, pass the row's previous value as `previousValue`. Without it, the change already overwrote the value the collection needs to publish the update, and live queries can keep the row in a result it left. In development, the collection throws -`SyncRowReusedWithoutPreviousValueError` for that write. +`SyncRowReusedWithoutPreviousValueError` for that write when a top-level field +changed. The check compares shallow copies, so it does not detect a change +inside a nested object. Pass `previousValue` for those writes too. ```ts // Changes a stored row in place, so it must name the previous value diff --git a/scripts/test-minified-db.mjs b/scripts/test-minified-db.mjs index 3245ba762f..a1f7e88d7d 100644 --- a/scripts/test-minified-db.mjs +++ b/scripts/test-minified-db.mjs @@ -84,7 +84,7 @@ const errorGroups = { CollectionStateError: `CollectionStateError CollectionInErrorStateError InvalidCollectionStatusTransitionError CollectionIsInErrorStateError NegativeActiveSubscribersError LiveQueryObserverDisposedError LiveQueryWindowControllerDisposedError`, CollectionOperationError: `CollectionOperationError UndefinedKeyError InvalidKeyError DuplicateKeyError DuplicateKeySyncError MissingUpdateArgumentError NoKeysPassedToUpdateError UpdateKeyNotFoundError KeyUpdateNotAllowedError NoKeysPassedToDeleteError DeleteKeyNotFoundError`, MissingHandlerError: `MissingHandlerError MissingInsertHandlerError MissingUpdateHandlerError MissingDeleteHandlerError`, - TransactionError: `TransactionError MissingMutationFunctionError TransactionNotPendingMutateError TransactionAlreadyCompletedRollbackError TransactionNotPendingCommitError NoPendingSyncTransactionWriteError SyncTransactionAlreadyCommittedWriteError NoPendingSyncTransactionCommitError SyncTransactionAlreadyCommittedError`, + TransactionError: `TransactionError MissingMutationFunctionError TransactionNotPendingMutateError TransactionAlreadyCompletedRollbackError TransactionNotPendingCommitError NoPendingSyncTransactionWriteError SyncTransactionAlreadyCommittedWriteError SyncRowReusedWithoutPreviousValueError NoPendingSyncTransactionCommitError SyncTransactionAlreadyCommittedError`, QueueCapacityExceededError: `QueueCapacityExceededError`, QueueDisposedError: `QueueDisposedError`, ThrottleCallDroppedError: `ThrottleCallDroppedError`,