-
Notifications
You must be signed in to change notification settings - Fork 257
fix(db): reclaim collections that sync before anything subscribes #1744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When AGENTS.md reference: AGENTS.md:L371-L379 Useful? React with 👍 / 👎. |
||
| } catch (error) { | ||
| this.lifecycle.setStatus(`error`) | ||
| throw error | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<Person>({ | ||
| id, | ||
| getKey: (p) => p.id, | ||
| initialData: [ | ||
| { id: `1`, name: `Alice`, active: true }, | ||
| { id: `2`, name: `Bob`, active: false }, | ||
| ], | ||
| }), | ||
| ) | ||
|
|
||
| const makeOrphan = (source: ReturnType<typeof makeSource>, 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<typeof makeSource>, | ||
| person: Person, | ||
| ) => { | ||
| source.utils.begin() | ||
| source.utils.write({ type: `insert`, value: person }) | ||
| source.utils.commit() | ||
| } | ||
|
Comment on lines
+23
to
+57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add precise return types to the new test helpers. The new helper declarations omit return-type annotations. Add collection return types and annotate promise and side-effect helpers explicitly.
As per coding guidelines, “Always provide the most precise return type annotation; avoid 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| 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<number> = [] | ||
| 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<typeof makeSource>, 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) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not describe the 50 ms floor as an absolute guarantee.
A render can commit after the 50 ms timer fires. Describe the delay as a grace window for pending subscriptions.
Suggested wording
📝 Committable suggestion
🤖 Prompt for AI Agents