Repository navigation
Conversation
The Electric client registers a visibilitychange listener (AppState on React Native) when a ShapeStream is constructed and removes it only in unsubscribeAll(). Sync cleanup unsubscribed its own callback and aborted the stream, so the listener stayed registered and kept the dead stream reachable. Each cleanup and restart, and each managed cache rotation, retained one more stream. Cleanup now calls stream.unsubscribeAll(). The stream is private to the sync session, so this affects no other consumer. A generated lane in the installed-SDK delivery oracle checks that cleanup leaves no listener from a run's streams across eager, on-demand and progressive runs that stay open, give up, abort or restart. The managed cache rotation case checks the retired session.
🦋 Changeset detectedLatest commit: 3075725 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Release stream listeners during cleanup .changeset/electric-cleanup-stream-listeners.md, packages/electric-db-collection/src/electric.ts, packages/electric-db-collection/tests/* |
Session cleanup calls stream.unsubscribeAll() after removing the collection listener and before aborting the stream controller. Stream test doubles now provide unsubscribeAll. |
Verify and register listener release packages/electric-db-collection/tests/electric-sdk-delivery-oracle.property.test.ts, packages/db/tests/oracle-config.ts, packages/db/tests/oracle-replay-manifest.ts, docs/contributing/oracle-coverage.md |
The delivery oracle checks listener counts during cache rotation and across sync modes and run histories. The source-release property is registered in the static oracle configuration and replay manifest, and added to the ownership documentation. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related issues
- Enforce oracle-first work: oracle lint rules, CI checks, and agent feedback hooks #2234: This PR adds and registers an oracle property for Electric cleanup, but does not add the issue’s requested lint or CI checks.
Possibly related PRs
- TanStack/db#2237: Both PRs change replay ownership for
electric-sdk-delivery-oracle.property.test.ts; this PR adds the source-release property to that manifest. - TanStack/db#2236: Both PRs affect campaign selection and replay registration for the Electric delivery oracle; this PR adds listener-cleanup coverage.
- TanStack/db#2233: This PR imports
createOracleReachfrom the helper introduced by that PR for the new cleanup property.
Suggested reviewers: kyleamathews
Merge Risk: 
30757
-
Sync cleanup now releases the Electric stream's visibility listener, so retired streams are no longer retained after cleanup or restart. No concrete merge-blocking risk was found. The new test checks the browser listener in jsdom and does not cover React Native's
AppStatepath.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 12 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the fix and its scope: the Electric shape stream's visibility listener is released during cleanup. The changeset adds stream.unsubscribeAll() to sync cleanup, which matches this. The t… Description check The description covers the required template sections. Changes explains the cleanup call and its motivation. Oracle Coverage names the owner, states the law, histories, production path, observation po… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the fix and its scope: the Electric shape stream's visibility listener is released during cleanup. The changeset adds stream.unsubscribeAll() to sync cleanup, which matches this. The title is short and specific.
Full details: Description check
-
Explanation
The description covers the required template sections. Changes explains the cleanup call and its motivation. Oracle Coverage names the owner, states the law, histories, production path, observation points, RED/GREEN results, mutants, rotation witness, and limits. The checklist marks local testing, and Release Impact marks a changeset. The template's Oracle Coverage section asks for bug-fix evidence, and the author supplied it in detail.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 12 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Changes
Sync cleanup now calls
stream.unsubscribeAll(), so a cleaned-up Electric collection no longer leaves itsShapeStream's visibility listener registered ondocumentor on React Native'sAppState.We run TanStack DB with Electric in production, in web apps and in Expo apps on iOS and Android, and this line has been a local patch on our side since July. I'm sending it upstream so we can drop the patch.
The Electric client registers a
visibilitychangelistener in theShapeStreamconstructor, so the stream can pause while the page is hidden. From@electric-sql/client1.5.23, on React Native it registers anAppStatesubscription through its runtime visibility adapter instead. The client removes either one only inunsubscribeAll(). Aborting the stream's signal runs the client's teardown, which removes the wake-detection timer but not the visibility listener.The adapter's cleanup unsubscribed its own callback and aborted the stream, so the listener stayed registered, and its closure kept the whole dead stream reachable: fetch wrappers, parser, and sync state. Every cleanup followed by a restart left one more. So did every managed cache rotation on an on-demand collection with SQLite persistence, because the retired provider session goes through the same cleanup. We found it in one of our Expo apps, where a screen rebuilt its collection every minute and kept one more dead stream each time.
The stream is private to the sync session, so
unsubscribeAll()affects no other consumer. AfterunsubscribeStream()it only removes the visibility and wake listeners. It does not abort or pause anything, and the abort that follows still owns in-flight requests and pendingawaitTxIdandawaitMatchwaits.The root cause is arguably in the client: its documented way to stop a stream, aborting the signal, leaves the listener behind. That is a separate change for
@electric-sql/client. This one holds either way, because cleanup should release what the session created, and it works across the whole@electric-sql/clientrange this package accepts.Oracle Coverage
Coverage-map owner: Electric and TrailBase, installed-SDK delivery (
electric-sdk-delivery-oracle.property.test.ts). The receiving boundary is the installed Electric client, which is where the listener lives, so the law goes in the lane that drives the realShapeStreamover controlled HTTP rather than in the controlled-stream oracles.Law: cleanup ends a sync run and releases its adapter-owned resources (
CleanupFninpackages/db/src/types.ts; "Sync Implementation" in the collection options creator guide). After cleanup,documentholds novisibilitychangelistener from a stream that the run created, whatever state the source reached. While a run is active it holds at most its one stream's listener.Model: the number of active sync runs, 0 or 1. It reads no adapter or client state, and it does not predict when an active stream drops its listener, so a client that starts releasing it on abort still passes.
Histories: new property
electric.sdk-source-release. Eager, on-demand, or progressive mode; one to three sync runs, the later ones restarted throughpreload()after cleanup. In each run the provider answers up to three requests with a row and an up-to-date, or with a 503 thatonErrorretries. The run then stays open, gives up on a 503 whoseonErrorreturns no retry, or aborts the external signal, and is cleaned up. A run that starts after an abort starts with the signal already aborted. The grammar never answers a request after a give-up or an abort, because the client sends none.Production path and observation: a real Collection, the Electric adapter, the installed
ShapeStream, and controlled HTTP. A recorder wrapsdocument.addEventListenerandremoveEventListenerand counts livevisibilitychangelisteners. The count is compared with the model after each provider answer, give-up, abort, and cleanup. A calibration check at the end of each history shows that the recorder saw one listener per run.Reach (40 histories per campaign; the property fails if any state is missed):
cleanup-while-loadingcleanup-after-readycleanup-after-give-upcleanup-after-abortcleanup-of-restarted-runRED on
main: both campaigns fail at the first cleanup. The shrunk history is one eager run with no answers: one listener is still live after cleanup.GREEN: both campaigns pass.
Mutants (all assertion failures at a cleanup checkpoint):
main(nounsubscribeAll())Rotation witness: the existing case "discards an old SDK snapshot after managed cache rotation" now also checks that only the replacement stream's listener is live after the rotation, and none after cleanup. On
mainit fails with 2 live listeners after the rotation.Limits: the lane observes the browser listener in jsdom. It does not install a runtime visibility adapter, so React Native's
AppStatepath is not exercised here; the fix does not depend on which one the client uses. A live Electric service is not exercised.Replay:
TANSTACK_DB_ORACLE_SEED=<seed> TANSTACK_DB_ORACLE_PATH=<path> TANSTACK_DB_ORACLE_PROPERTY=electric.sdk-source-release pnpm --dir packages/electric-db-collection exec vitest run tests/electric-sdk-delivery-oracle.property.test.ts -t 'replay'The controlled
ShapeStreammocks in eight test files gainedunsubscribeAll: vi.fn(). The new property is registered inoracle-config.tsand the replay manifest, and the coverage map's Electric row describes it.Shipped size:
pnpm size:comparereports +9 B gzip (+15 B brotli) for@tanstack/electric-db-collectionand no change elsewhere.unsubscribeAll()also clears our subscription, sounsubscribeStream()in cleanup is now redundant. I kept it to keep this change small: removing it fails 13 existing tests that assert the per-subscription unsubscribe on a controlled stream, and those would need to move tounsubscribeAll(). Happy to make that change here if you prefer.Checklist
pnpm test.@tanstack/electric-db-collection: 1,126 tests pass, 1 skipped, no type errors.pnpm lintreports no errors in the package (existing warnings only),pnpm lint:changedis clean, and the replay manifest test passes.Release Impact
Summary by CodeRabbit