diff --git a/.changeset/lucky-donkeys-repeat.md b/.changeset/lucky-donkeys-repeat.md new file mode 100644 index 0000000000..5d39d35d8f --- /dev/null +++ b/.changeset/lucky-donkeys-repeat.md @@ -0,0 +1,11 @@ +--- +'@tanstack/db': patch +--- + +Garbage collect collections that start syncing before anything subscribes to them. + +`startGCTimer` only ran when the last subscriber left, so a collection whose subscriber count never rose above zero never armed it. Sync started from `startSync: true`, `preload()` or `startSyncImmediate()` therefore ran forever, and a live query started that way held a subscription on every collection it read from for the lifetime of the page — regardless of how short its `gcTime` was. + +Framework adapters build their live query collection while rendering and subscribe once that render commits, so every render React, Vue, Svelte or Solid discards before committing — a suspended subtree, a render that throws, a time-sliced render restarted by an interleaved update — stranded a fully compiled query graph rooted at a long-lived source collection. Under a route that repeatedly re-rendered without committing this exhausted the renderer's heap. + +Sync starting without a subscriber now arms the same GC timer, floored at 50ms so that a subscriber arriving with the commit is never beaten to it. Collections with `gcTime: 0` still opt out of GC entirely, and the timer armed when the last subscriber leaves still fires on `gcTime` exactly. diff --git a/packages/db/skills/db-core/live-queries/SKILL.md b/packages/db/skills/db-core/live-queries/SKILL.md index 00da8635ff..39c81ca7c9 100644 --- a/packages/db/skills/db-core/live-queries/SKILL.md +++ b/packages/db/skills/db-core/live-queries/SKILL.md @@ -203,7 +203,11 @@ const activeUserPosts = createLiveQueryCollection((q) => Create derived collections once at module scope and reuse them. Do not recreate on every render or navigation. Live query collections default to `gcTime: 5_000`. An explicit `gcTime: 0` is -preserved and disables garbage collection for that derived collection. +preserved and disables garbage collection for that derived collection -- +including the reclamation of a collection that started syncing and never gained +a subscriber. Note this is the opposite of `gcTime: 0` in TanStack Query, where +it collects as soon as the query goes inactive; use a small positive value if +you want prompt collection here. ## Virtual Properties diff --git a/packages/db/skills/meta-framework/SKILL.md b/packages/db/skills/meta-framework/SKILL.md index d8aef668f6..ff3d248c81 100644 --- a/packages/db/skills/meta-framework/SKILL.md +++ b/packages/db/skills/meta-framework/SKILL.md @@ -373,7 +373,7 @@ export const Route = createFileRoute('/todos')({ }) ``` -Without preloading, the collection starts syncing only when the component mounts, causing a loading flash. Preloading in the route loader starts sync during navigation, making data available immediately when the component renders. +Without preloading, the collection starts syncing when the component first renders, causing a loading flash. Preloading in the route loader starts sync during navigation, so the data is already there on that first render. ### MEDIUM Creating separate collection instances in one scope diff --git a/packages/db/src/collection/cleanup-queue.ts b/packages/db/src/collection/cleanup-queue.ts index 1acf7751ae..1f8468928d 100644 --- a/packages/db/src/collection/cleanup-queue.ts +++ b/packages/db/src/collection/cleanup-queue.ts @@ -42,6 +42,15 @@ export class CleanupQueue { public cancel(key: unknown): void { this.tasks.delete(key) + + // Retire the root timer with the last task. A non-empty queue keeps its + // timer even when the cancelled task was the earliest: it wakes early, + // finds nothing due and reschedules, which costs less than rescanning + // every task on each cancellation. + if (this.tasks.size === 0 && this.timeoutId !== null) { + clearTimeout(this.timeoutId) + this.timeoutId = null + } } /** diff --git a/packages/db/src/collection/lifecycle.ts b/packages/db/src/collection/lifecycle.ts index 8f8cced21f..e685d8e0d5 100644 --- a/packages/db/src/collection/lifecycle.ts +++ b/packages/db/src/collection/lifecycle.ts @@ -17,6 +17,16 @@ import type { CollectionChangesManager } from './changes' import type { CollectionSyncManager } from './sync' import type { CollectionStateManager } from './state' +/** + * Floor applied to the GC delay of a collection that started syncing before + * anything subscribed. Adapters build their live query while rendering and + * subscribe when that render commits, so the delay has to outlive that gap; + * `gcTime` cannot, because adapters pass a near-zero one to make teardown on + * unmount immediate. Does not apply to the timer armed when the last + * subscriber leaves, which still honours `gcTime` exactly. + */ +const UNSUBSCRIBED_GC_FLOOR_MS = 50 + export class CollectionLifecycleManager< TOutput extends object = Record, TKey extends string | number = string | number, @@ -158,11 +168,23 @@ export class CollectionLifecycleManager< } } + /** + * Start the garbage collection timer for a collection with no subscribers + * Called when sync starts outside a subscription + */ + public startGCTimerIfUnsubscribed(): void { + if (this.changes.activeSubscribersCount > 0) { + return + } + + this.startGCTimer(UNSUBSCRIBED_GC_FLOOR_MS) + } + /** * Start the garbage collection timer * Called when the collection becomes inactive (no subscribers) */ - public startGCTimer(): void { + public startGCTimer(minDelay = 0): void { const gcTime = this.config.gcTime ?? 300000 // 5 minutes default // If gcTime is 0, negative, or non-finite (Infinity, -Infinity, NaN), GC is disabled. @@ -172,12 +194,16 @@ export class CollectionLifecycleManager< return } - CleanupQueue.getInstance().schedule(this, gcTime, () => { - if (this.changes.activeSubscribersCount === 0) { - // Schedule cleanup during idle time to avoid blocking the UI thread - this.scheduleIdleCleanup() - } - }) + CleanupQueue.getInstance().schedule( + this, + Math.max(gcTime, minDelay), + () => { + if (this.changes.activeSubscribersCount === 0) { + // Schedule cleanup during idle time to avoid blocking the UI thread + this.scheduleIdleCleanup() + } + }, + ) } /** diff --git a/packages/db/src/collection/sync.ts b/packages/db/src/collection/sync.ts index d717106110..28c4ef2381 100644 --- a/packages/db/src/collection/sync.ts +++ b/packages/db/src/collection/sync.ts @@ -272,6 +272,12 @@ export class CollectionSyncManager< `Either provide a loadSubset handler or use syncMode "eager".`, ) } + + // Every route into sync passes through here, so it is the one place + // that sees sync start ahead of the subscriber that would justify it. + // `addSubscriber` counts itself in before calling us, so a subscription + // starting sync leaves the timer alone. + this.lifecycle.startGCTimerIfUnsubscribed() } catch (error) { this.lifecycle.setStatus(`error`) throw error diff --git a/packages/db/tests/orphaned-live-query-gc.test.ts b/packages/db/tests/orphaned-live-query-gc.test.ts new file mode 100644 index 0000000000..9e08823754 --- /dev/null +++ b/packages/db/tests/orphaned-live-query-gc.test.ts @@ -0,0 +1,184 @@ +import { describe, expect, it } from 'vitest' +import { createCollection } from '../src/collection/index.js' +import { createLiveQueryCollection } from '../src/query/live-query-collection.js' +import { mockSyncCollectionOptions } from './utils.js' + +/** + * A live query collection created with `startSync: true` subscribes to its + * source collections at construction time. `startGCTimer()` is only ever + * called from `removeSubscriber()` on the 1 -> 0 transition, so a collection + * that never gains a subscriber never arms its GC timer and is never cleaned + * up -- no matter how small `gcTime` is. + * + * This is exactly the state left behind by a React render attempt that is + * discarded before commit: `useLiveQuery` constructs the collection with + * `startSync: true` in the render body, and the only teardown path runs from + * the `useSyncExternalStore` subscribe cleanup, which never happens. + */ + +type Person = { id: string; name: string; active: boolean } + +const GC_TIME_MS = 1 + +const makeSource = (id: string) => + createCollection( + mockSyncCollectionOptions({ + id, + getKey: (p) => p.id, + initialData: [ + { id: `1`, name: `Alice`, active: true }, + { id: `2`, name: `Bob`, active: false }, + ], + }), + ) + +const makeOrphan = (source: ReturnType, id: string) => + createLiveQueryCollection({ + id, + // Same two options `useLiveQuery` passes from its render body. + startSync: true, + gcTime: GC_TIME_MS, + query: (q) => + q + .from({ p: source }) + .select(({ p }) => ({ id: p.id, name: p.name, active: p.active })), + }) + +// Well past gcTime, plus room for the CleanupQueue's batching microtask. +const waitPastGcTime = () => new Promise((r) => setTimeout(r, 100)) + +const writeToSource = ( + source: ReturnType, + person: Person, +) => { + source.utils.begin() + source.utils.write({ type: `insert`, value: person }) + source.utils.commit() +} + +describe(`live query collections that never gain a subscriber`, () => { + it(`is cleaned up after gcTime even though nothing ever subscribed`, async () => { + const source = makeSource(`gc-single-source`) + expect(source.subscriberCount).toBe(0) + + const orphan = makeOrphan(source, `gc-single-orphan`) + + // Construction started sync, which subscribed to the source. + expect(source.subscriberCount).toBe(1) + + await waitPastGcTime() + + // Nothing ever subscribed to `orphan`, so it is unreachable from user code + // the moment the caller drops its reference. After gcTime it should have + // released the source. + expect(orphan.status).toBe(`cleaned-up`) + expect(source.subscriberCount).toBe(0) + }) + + it(`releases the source when every orphan's gcTime has elapsed`, async () => { + const source = makeSource(`gc-many-source`) + const N = 200 + + const counts: Array = [] + for (let i = 0; i < N; i++) { + makeOrphan(source, `gc-many-orphan-${i}`) + counts.push(source.subscriberCount) + } + + // Strictly monotonic: every construction adds a subscription, none is + // ever removed. + expect(counts[0]).toBe(1) + expect(counts[N - 1]).toBe(N) + + await waitPastGcTime() + + console.log( + `[orphan-gc] after ${N} never-subscribed constructions + ${GC_TIME_MS}ms gcTime: source.subscriberCount =`, + source.subscriberCount, + ) + + expect(source.subscriberCount).toBe(0) + }) + + it(`stops reprocessing source changes once gcTime has elapsed`, async () => { + const N = 200 + + const bare = makeSource(`gc-cost-bare-source`) + const loaded = makeSource(`gc-cost-loaded-source`) + for (let i = 0; i < N; i++) { + makeOrphan(loaded, `gc-cost-orphan-${i}`) + } + + await waitPastGcTime() + + const time = (source: ReturnType, tag: string) => { + const start = performance.now() + for (let i = 0; i < 50; i++) { + writeToSource(source, { id: `w${i}`, name: `W${i}`, active: true }) + } + const ms = performance.now() - start + + console.log(`[orphan-gc] 50 source writes (${tag}): ${ms.toFixed(2)}ms`) + return ms + } + + const bareMs = time(bare, `no orphans`) + const loadedMs = time(loaded, `${N} orphans, all past gcTime`) + + console.log( + `[orphan-gc] write cost multiplier with ${N} orphans:`, + (loadedMs / Math.max(bareMs, 0.001)).toFixed(1) + `x`, + ) + + // Orphans that were never subscribed to should have been reclaimed, so a + // write should cost about the same either way. + expect(loadedMs).toBeLessThan(bareMs * 5 + 5) + }) + + it(`retains no heap once gcTime has elapsed`, async () => { + const source = makeSource(`gc-heap-source`) + const N = 1000 + + const forceGc = (globalThis as { gc?: () => void }).gc + forceGc?.() + const before = process.memoryUsage().heapUsed + + for (let i = 0; i < N; i++) { + makeOrphan(source, `gc-heap-orphan-${i}`) + } + + await waitPastGcTime() + forceGc?.() + const after = process.memoryUsage().heapUsed + + const retained = after - before + + console.log( + `[orphan-gc] ${N} never-subscribed live queries retained ${(retained / 1024 / 1024).toFixed(1)} MB` + + ` (${Math.round(retained / N / 1024)} KB each), forced GC: ${forceGc ? `yes` : `unavailable`},` + + ` source.subscriberCount = ${source.subscriberCount}`, + ) + + expect(source.subscriberCount).toBe(0) + // Whatever the exact per-graph size, nothing should survive. Only + // meaningful when the heap can actually be collected first, so run this + // file under `NODE_OPTIONS=--expose-gc` for the byte assertion. + if (forceGc) { + expect(retained).toBeLessThan(N * 1024) + } + }) + + it(`is cleaned up when a subscriber does attach and then detach (control)`, async () => { + const source = makeSource(`gc-control-source`) + const live = makeOrphan(source, `gc-control-live`) + + const subscription = live.subscribeChanges(() => {}) + expect(live.subscriberCount).toBe(1) + + subscription.unsubscribe() + await waitPastGcTime() + + expect(live.status).toBe(`cleaned-up`) + expect(source.subscriberCount).toBe(0) + }) +}) diff --git a/packages/db/tests/unsubscribed-sync-gc.test.ts b/packages/db/tests/unsubscribed-sync-gc.test.ts new file mode 100644 index 0000000000..38aac62633 --- /dev/null +++ b/packages/db/tests/unsubscribed-sync-gc.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from 'vitest' +import { createCollection } from '../src/collection/index.js' +import { createLiveQueryCollection } from '../src/query/live-query-collection.js' +import { mockSyncCollectionOptions } from './utils.js' + +/** + * Sync can start before any subscriber exists — `startSync: true`, `preload()` + * and `startSyncImmediate()` all do it. These cover the guarantees around + * reclaiming those collections, in particular the ones that keep a collection + * whose subscriber is still on its way from being torn down underneath it. + */ + +type Person = { id: string; name: string } + +const makeSource = (id: string) => + createCollection( + mockSyncCollectionOptions({ + id, + getKey: (p) => p.id, + initialData: [ + { id: `1`, name: `Alice` }, + { id: `2`, name: `Bob` }, + ], + }), + ) + +const makeLiveQuery = ( + source: ReturnType, + id: string, + gcTime = 1, +) => + createLiveQueryCollection({ + id, + startSync: true, + gcTime, + query: (q) => + q.from({ p: source }).select(({ p }) => ({ id: p.id, name: p.name })), + }) + +const wait = (ms: number) => new Promise((r) => setTimeout(r, ms)) + +describe(`collections that start syncing without a subscriber`, () => { + it(`survives longer than its gcTime, so a subscriber still on its way can attach`, async () => { + const source = makeSource(`grace-source`) + const live = makeLiveQuery(source, `grace-live`) + + // `gcTime` is 1ms. Frameworks build the collection while rendering and + // subscribe when that render commits, so reclaiming it on `gcTime` alone + // would race the commit. + await wait(15) + + expect(live.status).not.toBe(`cleaned-up`) + expect(source.subscriberCount).toBe(1) + }) + + it(`cancels the pending reclamation once a subscriber attaches`, async () => { + const source = makeSource(`cancel-source`) + const live = makeLiveQuery(source, `cancel-live`) + + const subscription = live.subscribeChanges(() => {}) + + await wait(100) + + expect(live.status).toBe(`ready`) + expect(live.size).toBe(2) + expect(source.subscriberCount).toBe(1) + + subscription.unsubscribe() + }) + + it(`still reclaims on gcTime when the last subscriber leaves`, async () => { + const source = makeSource(`unmount-source`) + const live = makeLiveQuery(source, `unmount-live`) + + const subscription = live.subscribeChanges(() => {}) + subscription.unsubscribe() + + // The grace period covers the gap before the first subscriber only. Once + // one has come and gone, teardown runs on `gcTime` — 1ms here, which is + // what adapters rely on to release a query as its component unmounts. + await wait(20) + + expect(live.status).toBe(`cleaned-up`) + expect(source.subscriberCount).toBe(0) + }) + + it(`restarts sync when a subscriber attaches after reclamation`, async () => { + const source = makeSource(`restart-source`) + const live = makeLiveQuery(source, `restart-live`) + + await wait(100) + expect(live.status).toBe(`cleaned-up`) + expect(source.subscriberCount).toBe(0) + + live.subscribeChanges(() => {}) + + expect(live.status).not.toBe(`cleaned-up`) + expect(live.size).toBe(2) + expect(source.subscriberCount).toBe(1) + }) + + it(`leaves collections alone when gcTime disables GC`, async () => { + const source = makeSource(`disabled-source`) + const live = makeLiveQuery(source, `disabled-live`, 0) + + await wait(100) + + expect(live.status).toBe(`ready`) + expect(source.subscriberCount).toBe(1) + }) + + it(`reclaims a collection warmed by preload that nothing goes on to use`, async () => { + const source = makeSource(`preload-source`) + const live = createLiveQueryCollection({ + id: `preload-live`, + gcTime: 1, + query: (q) => q.from({ p: source }).select(({ p }) => ({ id: p.id })), + }) + + await live.preload() + expect(source.subscriberCount).toBe(1) + + await wait(100) + + expect(live.status).toBe(`cleaned-up`) + expect(source.subscriberCount).toBe(0) + }) +}) diff --git a/packages/electric-db-collection/tests/electric.test.ts b/packages/electric-db-collection/tests/electric.test.ts index c48121a9c1..80c5b160c4 100644 --- a/packages/electric-db-collection/tests/electric.test.ts +++ b/packages/electric-db-collection/tests/electric.test.ts @@ -2605,6 +2605,11 @@ describe(`Electric Integration`, () => { }), ) + // Other collections in this file keep the shared GC timer armed, so + // assert this load leaves no timer of its own rather than none at all. + await Promise.resolve() // the GC queue picks its timer in a microtask + const ambientTimers = vi.getTimerCount() + let loadSettled = false const load = Promise.resolve( testCollection._sync.loadSubset({ limit: 10 }), @@ -2621,13 +2626,13 @@ describe(`Electric Integration`, () => { expect(mockRequestSnapshot).toHaveBeenCalledTimes(1) expect(loadSettled).toBe(true) await testCollection.cleanup() - expect(vi.getTimerCount()).toBe(0) + expect(vi.getTimerCount()).toBe(ambientTimers) resolveRefresh() await refresh await Promise.resolve() expect(mockRequestSnapshot).toHaveBeenCalledTimes(1) - expect(vi.getTimerCount()).toBe(0) + expect(vi.getTimerCount()).toBe(ambientTimers) } finally { vi.useRealTimers() } @@ -2656,6 +2661,11 @@ describe(`Electric Integration`, () => { }), ) + // Other collections in this file keep the shared GC timer armed, so + // assert this load leaves no timer of its own rather than none at all. + await Promise.resolve() // the GC queue picks its timer in a microtask + const ambientTimers = vi.getTimerCount() + const load = testCollection._sync.loadSubset({ limit: 10 }) await vi.advanceTimersByTimeAsync(250) await load @@ -2664,7 +2674,7 @@ describe(`Electric Integration`, () => { await expect(refresh).rejects.toThrow(`late refresh failure`) await Promise.resolve() expect(mockRequestSnapshot).toHaveBeenCalledTimes(1) - expect(vi.getTimerCount()).toBe(0) + expect(vi.getTimerCount()).toBe(ambientTimers) } finally { vi.useRealTimers() } @@ -2689,10 +2699,15 @@ describe(`Electric Integration`, () => { }), ) + // Other collections in this file keep the shared GC timer armed, so + // assert this load leaves no timer of its own rather than none at all. + await Promise.resolve() // the GC queue picks its timer in a microtask + const ambientTimers = vi.getTimerCount() + await testCollection._sync.loadSubset({ limit: 10 }) expect(mockRequestSnapshot).toHaveBeenCalledTimes(1) - expect(vi.getTimerCount()).toBe(0) + expect(vi.getTimerCount()).toBe(ambientTimers) } finally { vi.useRealTimers() } diff --git a/packages/react-db/tests/useLiveQuery.uncommitted-render.test.tsx b/packages/react-db/tests/useLiveQuery.uncommitted-render.test.tsx new file mode 100644 index 0000000000..8a3a68cbbd --- /dev/null +++ b/packages/react-db/tests/useLiveQuery.uncommitted-render.test.tsx @@ -0,0 +1,130 @@ +import { describe, expect, it } from 'vitest' +import { Component, Suspense } from 'react' +import { render, waitFor } from '@testing-library/react' +import { createCollection } from '@tanstack/db' +import { useLiveQuery } from '../src/useLiveQuery' +import { mockSyncCollectionOptions } from '../../db/tests/utils' +import type { ReactNode } from 'react' + +/** + * `useLiveQuery` constructs its live query collection with `startSync: true` + * from the render body, which subscribes to the source collections + * immediately. Teardown only runs from the `useSyncExternalStore` subscribe + * cleanup, which React runs only after a commit. + * + * React discards render attempts routinely -- a subtree that suspends, a + * throw, a time-sliced concurrent render restarted by an interleaved update. + * Every discarded attempt leaves a fully compiled, fully subscribed query + * graph attached to the (long-lived) source collection, and nothing ever + * reclaims it: `startGCTimer()` is only called on the 1 -> 0 subscriber + * transition, so a collection that never gained a subscriber never arms its + * timer. + */ + +type Person = { id: string; name: string } + +const makeSource = (id: string) => + createCollection( + mockSyncCollectionOptions({ + id, + getKey: (p) => p.id, + initialData: [{ id: `1`, name: `A` }], + }), + ) + +// `useLiveQuery` uses gcTime = 1ms, so this is far past due. +const waitPastGcTime = () => new Promise((r) => setTimeout(r, 250)) + +class Boundary extends Component<{ children: ReactNode }, { failed: boolean }> { + state = { failed: false } + static getDerivedStateFromError() { + return { failed: true } + } + render() { + return this.state.failed ? null : this.props.children + } +} + +describe(`useLiveQuery in renders that never commit`, () => { + it(`releases the source when the subtree suspends after the hook ran`, async () => { + const source = makeSource(`uncommitted-suspend`) + const neverResolves = new Promise(() => {}) + + const Suspender = () => { + throw neverResolves + } + + const Route = () => { + useLiveQuery((q) => + q.from({ p: source }).select(({ p }) => ({ id: p.id, name: p.name })), + ) + return + } + + const ATTEMPTS = 10 + for (let i = 0; i < ATTEMPTS; i++) { + render( + + + , + ) + } + + await waitPastGcTime() + + console.log( + `[uncommitted] ${ATTEMPTS} suspended render attempts -> source.subscriberCount =`, + source.subscriberCount, + ) + + expect(source.subscriberCount).toBe(0) + }) + + it(`releases the source when the render throws after the hook ran`, async () => { + const source = makeSource(`uncommitted-throw`) + + const Thrower = () => { + useLiveQuery((q) => + q.from({ p: source }).select(({ p }) => ({ id: p.id, name: p.name })), + ) + throw new Error(`render discarded`) + } + + const ATTEMPTS = 20 + for (let i = 0; i < ATTEMPTS; i++) { + render( + + + , + ) + } + + await waitPastGcTime() + + console.log( + `[uncommitted] ${ATTEMPTS} throwing render attempts -> source.subscriberCount =`, + source.subscriberCount, + ) + + expect(source.subscriberCount).toBe(0) + }) + + it(`releases the source after a normal mount and unmount (control)`, async () => { + const source = makeSource(`uncommitted-control`) + + const Ok = () => { + const { data } = useLiveQuery((q) => + q.from({ p: source }).select(({ p }) => ({ id: p.id, name: p.name })), + ) + return
{data.length}
+ } + + const { unmount } = render() + await waitFor(() => expect(source.subscriberCount).toBeGreaterThan(0)) + + unmount() + await waitPastGcTime() + + expect(source.subscriberCount).toBe(0) + }) +})