diff --git a/.changeset/clean-query-ownership.md b/.changeset/clean-query-ownership.md new file mode 100644 index 000000000..b86854f27 --- /dev/null +++ b/.changeset/clean-query-ownership.md @@ -0,0 +1,5 @@ +--- +'@tanstack/query-db-collection': patch +--- + +Clean up empty query ownership state while preserving authoritative empty results and retained-row lifecycle behavior. diff --git a/packages/query-db-collection/src/query.ts b/packages/query-db-collection/src/query.ts index 7b4578d9e..8ef7a7ea1 100644 --- a/packages/query-db-collection/src/query.ts +++ b/packages/query-db-collection/src/query.ts @@ -737,7 +737,8 @@ export function queryCollectionOptions( // hashedQueryKey → queryKey const hashToQueryKey = new Map() - // queryKey → Set + // queryKey → Set. Entry presence means ownership is resolved; + // an empty set represents a resolved query that currently owns no rows. const queryToRows = new Map>() // RowKey → Set @@ -775,6 +776,11 @@ export function queryCollectionOptions( } const addRowOwners = (rowKey: string | number, owners: Set) => { + if (owners.size === 0) { + rowToQueries.delete(rowKey) + return + } + rowToQueries.set(rowKey, new Set(owners)) owners.forEach((owner) => { const ownedRows = queryToRows.get(owner) || new Set() @@ -784,16 +790,16 @@ export function queryCollectionOptions( } const removeRowOwner = (rowKey: string | number, hashedQueryKey: string) => { - const owners = rowToQueries.get(rowKey) || new Set() - owners.delete(hashedQueryKey) - rowToQueries.set(rowKey, owners) + const owners = rowToQueries.get(rowKey) + owners?.delete(hashedQueryKey) + if (!owners?.size) { + rowToQueries.delete(rowKey) + } - const ownedRows = - queryToRows.get(hashedQueryKey) || new Set() - ownedRows.delete(rowKey) - queryToRows.set(hashedQueryKey, ownedRows) + const ownedRows = queryToRows.get(hashedQueryKey) + ownedRows?.delete(rowKey) - return owners.size === 0 + return !owners?.size } const removeQueryOwnership = (hashedQueryKey: string) => { @@ -1075,6 +1081,7 @@ export function queryCollectionOptions( `${QUERY_COLLECTION_GC_PREFIX}${hashedQueryKey}`, ) commit() + queryToRows.delete(hashedQueryKey) } const schedulePersistedRetentionExpiry = ( @@ -1386,6 +1393,13 @@ export function queryCollectionOptions( const previouslyOwnedRows = shouldUsePersistedBaseline ? new Set(persistedBaseline.keys()) : getHydratedOwnedRowsForQueryBaseline(hashedQueryKey) + // From this point onward the result, including an empty result, is the + // authoritative ownership baseline until this query is cleaned up. + queryToRows.set( + hashedQueryKey, + queryToRows.get(hashedQueryKey) ?? new Set(), + ) + const newItemsMap = new Map() newItemsArray.forEach((item) => { const key = getKey(item) @@ -2014,6 +2028,15 @@ export function queryCollectionOptions( } } + if (typeof process !== `undefined` && process.env.NODE_ENV === `test`) { + Object.defineProperty(enhancedInternalSync, `__getOwnershipMapsForTests`, { + value: () => ({ + rowToQueries, + queryToRows, + }), + }) + } + // Create write utils using the manual-sync module const writeUtils = createWriteUtils( () => writeContext, diff --git a/packages/query-db-collection/tests/query.test.ts b/packages/query-db-collection/tests/query.test.ts index c711d661f..3005db911 100644 --- a/packages/query-db-collection/tests/query.test.ts +++ b/packages/query-db-collection/tests/query.test.ts @@ -47,6 +47,28 @@ interface CategorisedItem { const getKey = (item: TestItem) => item.id +type OwnershipMaps = { + rowToQueries: Map> + queryToRows: Map> +} + +function inspectOwnershipMaps(options: { + sync: { sync: unknown } +}): OwnershipMaps { + const sync = options.sync.sync as { + __getOwnershipMapsForTests?: () => OwnershipMaps + } + const maps = sync.__getOwnershipMapsForTests?.() + if (!maps) { + throw new Error(`Ownership-map test inspection is unavailable`) + } + return maps +} + +function expectNoEmptyRowOwnershipSets(maps: OwnershipMaps): void { + maps.rowToQueries.forEach((owners) => expect(owners.size).toBeGreaterThan(0)) +} + // Helper to advance timers and allow microtasks to flush const flushPromises = () => new Promise((resolve) => setTimeout(resolve, 0)) @@ -5074,6 +5096,193 @@ describe(`QueryCollection`, () => { }) }) + describe(`ownership lifecycle characterization`, () => { + it(`removes only rows whose final subset owner is unloaded`, async () => { + const queryFn = vi + .fn() + .mockResolvedValueOnce([ + { id: `1`, name: `First only` }, + { id: `2`, name: `Shared` }, + ]) + .mockResolvedValueOnce([ + { id: `2`, name: `Shared` }, + { id: `3`, name: `Second only` }, + ]) + const options = queryCollectionOptions({ + id: `ownership-overlapping-subsets-test`, + queryClient, + queryKey: [`ownership-overlapping-subsets-test`], + queryFn, + getKey, + syncMode: `on-demand`, + }) + const ownershipMaps = inspectOwnershipMaps(options) + const collection = createCollection(options) + const firstSubset = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .where(({ item }) => inArray(item.id, [`1`, `2`])), + }) + const secondSubset = createLiveQueryCollection({ + query: (q) => + q + .from({ item: collection }) + .where(({ item }) => inArray(item.id, [`2`, `3`])), + }) + + await firstSubset.preload() + await secondSubset.preload() + expect(collection.size).toBe(3) + + await firstSubset.cleanup() + await vi.waitFor(() => { + expect(collection.has(`1`)).toBe(false) + expect(collection.has(`2`)).toBe(true) + expect(collection.has(`3`)).toBe(true) + }) + expectNoEmptyRowOwnershipSets(ownershipMaps) + + await secondSubset.cleanup() + await vi.waitFor(() => { + expect(collection.size).toBe(0) + }) + expectNoEmptyRowOwnershipSets(ownershipMaps) + expect(ownershipMaps.rowToQueries.size).toBe(0) + expect(ownershipMaps.queryToRows.size).toBe(0) + }) + + it(`expires the Query cache entry after unload without restoring deleted rows`, async () => { + const expiringQueryClient = new QueryClient({ + defaultOptions: { + queries: { gcTime: 100, retry: false, staleTime: Infinity }, + }, + }) + const queryKey = [`ownership-query-cache-expiry-test`] + const collection = createCollection( + queryCollectionOptions({ + id: `ownership-query-cache-expiry-test`, + queryClient: expiringQueryClient, + queryKey, + queryFn: async () => [{ id: `1`, name: `Only row` }], + getKey, + syncMode: `on-demand`, + }), + ) + const liveQuery = createLiveQueryCollection({ + query: (q) => q.from({ item: collection }), + }) + + await liveQuery.preload() + expect(collection.has(`1`)).toBe(true) + + vi.useFakeTimers() + try { + await liveQuery.cleanup() + expect(collection.size).toBe(0) + expect( + expiringQueryClient.getQueryCache().find({ queryKey }), + ).toBeDefined() + + await vi.advanceTimersByTimeAsync(101) + + expect( + expiringQueryClient.getQueryCache().find({ queryKey }), + ).toBeUndefined() + expect(collection.size).toBe(0) + } finally { + vi.useRealTimers() + expiringQueryClient.clear() + } + }) + + it(`hydrates a retained row and deletes it when its query revalidates empty`, async () => { + const queryKey = [`ownership-retained-hydration-test`] + const queryHash = hashKey(queryKey) + const retainedRow: CategorisedItem = { + id: `1`, + name: `Retained row`, + category: `A`, + } + let releaseRevalidation!: () => void + const revalidationReleased = new Promise((resolve) => { + releaseRevalidation = resolve + }) + const queryFn = vi.fn(async () => { + await revalidationReleased + return [] + }) + const baseOptions = queryCollectionOptions({ + id: `ownership-retained-hydration-test`, + queryClient, + queryKey, + queryFn, + getKey: (item) => item.id, + startSync: true, + }) + const originalSync = baseOptions.sync + const ownershipMaps = inspectOwnershipMaps(baseOptions) + const metadataHarness = createInMemorySyncMetadataApi< + string | number, + CategorisedItem + >({ + persistedRows: new Map([[retainedRow.id, retainedRow]]), + rowMetadata: new Map([ + [ + retainedRow.id, + { + queryCollection: { owners: { [queryHash]: true } }, + }, + ], + ]), + collectionMetadata: new Map([ + [ + `queryCollection:gc:${queryHash}`, + { queryHash, mode: `until-revalidated` }, + ], + ]), + }) + const collection = createCollection({ + ...baseOptions, + sync: { + sync: (params: Parameters[0]) => { + params.begin({ immediate: true }) + params.write({ type: `insert`, value: retainedRow }) + params.commit() + return originalSync.sync({ + ...params, + metadata: metadataHarness.api, + }) + }, + }, + }) + + expect(collection.has(retainedRow.id)).toBe(true) + expect( + metadataHarness.collectionMetadata.has( + `queryCollection:gc:${queryHash}`, + ), + ).toBe(true) + + releaseRevalidation() + await vi.waitFor(() => { + expect(queryFn).toHaveBeenCalledTimes(1) + expect(collection.has(retainedRow.id)).toBe(false) + }) + expect(metadataHarness.rowMetadata.get(retainedRow.id)).toBeUndefined() + expectNoEmptyRowOwnershipSets(ownershipMaps) + expect(ownershipMaps.rowToQueries.size).toBe(0) + expect(ownershipMaps.queryToRows.get(queryHash)).toEqual(new Set()) + expect( + metadataHarness.collectionMetadata.has( + `queryCollection:gc:${queryHash}`, + ), + ).toBe(false) + + await collection.cleanup() + }) + }) + it(`should handle duplicate subset loads correctly (refcount bug)`, async () => { // This test catches Bug 1: missing refcount increment when reusing existing observer // When two subscriptions load the same subset, unloading one should NOT destroy @@ -5671,6 +5880,7 @@ describe(`QueryCollection`, () => { const baseOptions = queryCollectionOptions(config) const originalSync = baseOptions.sync + const ownershipMaps = inspectOwnershipMaps(baseOptions) const metadataHarness = createInMemorySyncMetadataApi< string | number, CategorisedItem @@ -5703,6 +5913,13 @@ describe(`QueryCollection`, () => { await liveQuery.cleanup() + expect(ownershipMaps.queryToRows.has(retainedQueryHash)).toBe(true) + expect(ownershipMaps.queryToRows.get(retainedQueryHash)).toEqual( + new Set([`1`]), + ) + expect(ownershipMaps.rowToQueries.get(`1`)).toEqual( + new Set([retainedQueryHash]), + ) expect( metadataHarness.collectionMetadata.get( `queryCollection:gc:${retainedQueryHash}`, @@ -5722,6 +5939,8 @@ describe(`QueryCollection`, () => { ), ).toBeUndefined() expect(collection.has(`1`)).toBe(false) + expect(ownershipMaps.queryToRows.has(retainedQueryHash)).toBe(false) + expect(ownershipMaps.rowToQueries.has(`1`)).toBe(false) } finally { vi.useRealTimers() }