fix!: release retained resources on dispose - #1668
Open
spydon wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR extracts rename dialogs into stateful widgets, improves Supabase singleton disposal and client access checks, and makes JSON isolate disposal safe for unused and repeated calls. Tests cover the updated lifecycle behavior. ChangesRename dialog ownership
Supabase singleton cleanup
JSON isolate disposal
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses resource-retention leaks across the Supabase Flutter monorepo by making disposal paths actually release references (so objects can be GC’d) and by adding leak_tracker-based regression coverage in unit/widget/integration tests.
Changes:
- Make teardown paths safe and leak-free (notably
YAJsonIsolate.dispose()andSupabasesingleton disposal semantics). - Add leak-tracking harnesses and targeted leak tests for
supabaseandsupabase_flutter. - Enable leak tracking in the Flutter test suites (including example integration tests) via
flutter_test_config.dart, with necessary ignore rules for global Flutter caches.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/yet_another_json_isolate/test/yet_another_json_isolate_io_test.dart | Adds regression tests ensuring YAJsonIsolate.dispose() completes when unused and when called twice. |
| packages/yet_another_json_isolate/lib/src/_isolates_io.dart | Makes dispose() idempotent and non-blocking when the isolate was never spawned (closes ReceivePort). |
| packages/supabase/test/leak_utils.dart | Introduces a test-side leak tracking harness (LeakScope, collectLeaks, expectNoLeaks). |
| packages/supabase/test/leak_utils_test.dart | Adds self-tests verifying the harness actually reports notDisposed and notGCed leaks. |
| packages/supabase/test/leak_test.dart | Adds leak tests covering SupabaseClient, sub-clients, realtime channels, and stream/auth subscriptions. |
| packages/supabase/pubspec.yaml | Adds leak_tracker as a dev dependency for leak tests. |
| packages/supabase_flutter/test/widget_test.dart | Updates widget test to use tester.runAsync for isolate spawning and disposes the singleton to satisfy leak tracking. |
| packages/supabase_flutter/test/widget_test_stubs.dart | Stores/cancels auth subscription to avoid retaining listeners across tests/leak tracking. |
| packages/supabase_flutter/test/leak_utils.dart | Adds a Flutter test leak harness (similar to supabase package) for non-widget leak scenarios. |
| packages/supabase_flutter/test/leak_test.dart | Adds leak tests validating Supabase.initialize()/dispose() cycles release client/auth references. |
| packages/supabase_flutter/test/flutter_test_config.dart | Enables LeakTesting and tracks all classes for the supabase_flutter test suite. |
| packages/supabase_flutter/pubspec.yaml | Adds leak_tracker and leak_tracker_flutter_testing dev dependencies. |
| packages/supabase_flutter/lib/src/supabase.dart | Converts client to a getter over a nullable backing field and clears singleton references in dispose(). |
| examples/storage_transforms/pubspec.yaml | Adds leak_tracker_flutter_testing dev dependency for integration tests. |
| examples/storage_transforms/integration_test/flutter_test_config.dart | Enables leak tracking and ignores Flutter image-cache classes to avoid false positives. |
| examples/realtime_room/pubspec.yaml | Adds leak_tracker_flutter_testing dev dependency for integration tests. |
| examples/realtime_room/integration_test/flutter_test_config.dart | Enables leak tracking for integration tests. |
| examples/passkeys/pubspec.yaml | Adds leak_tracker_flutter_testing dev dependency for integration tests. |
| examples/passkeys/lib/main.dart | Refactors rename dialog to own/dispose its TextEditingController. |
| examples/passkeys/integration_test/flutter_test_config.dart | Enables leak tracking for integration tests. |
| examples/edge_functions/pubspec.yaml | Adds leak_tracker_flutter_testing dev dependency for integration tests. |
| examples/edge_functions/integration_test/flutter_test_config.dart | Enables leak tracking for integration tests. |
| examples/database_crud/pubspec.yaml | Adds leak_tracker_flutter_testing dev dependency for integration tests. |
| examples/database_crud/lib/main.dart | Refactors rename dialog to own/dispose its TextEditingController. |
| examples/database_crud/integration_test/flutter_test_config.dart | Enables leak tracking for integration tests. |
| examples/authentication/pubspec.yaml | Adds leak_tracker_flutter_testing dev dependency for integration tests. |
| examples/authentication/integration_test/flutter_test_config.dart | Enables leak tracking for integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/passkeys/lib/main.dart`:
- Around line 199-201: Update the FilledButton callback in the rename flow to
trim _name.text before passing it to Navigator.pop. Ensure the value checked by
_rename for emptiness and saved as the passkey name is the trimmed input, while
preserving the existing Save behavior.
In `@packages/supabase_flutter/test/widget_test.dart`:
- Around line 18-41: Register singleton cleanup immediately after the successful
Supabase.initialize call by adding an addTearDown callback that disposes
Supabase.instance via tester.runAsync. Remove the later assertion-dependent
disposal call, while preserving the existing initialization and test flow.
In `@packages/supabase/test/leak_utils_test.dart`:
- Line 15: Update the test teardown around _retained so every retained
SupabaseClient is disposed before the collection is cleared. Preserve the
existing teardown behavior while ensuring the notDisposed scenario releases its
JSON isolate and owned resources.
In `@packages/yet_another_json_isolate/lib/src/_isolates_io.dart`:
- Line 23: Add a shared _ensureNotDisposed() guard that throws
StateError('YAJsonIsolate has been disposed') when _isDisposed is true, and
invoke it at the start of initialize(), decode(), and encode(). Ensure calls
after dispose are rejected before initialization or isolate work begins,
including decode() and encode().
- Around line 49-51: Update dispose() to store the future representing the first
cleanup operation, and have subsequent calls return that same future instead of
completing immediately. Ensure the shared future covers awaiting
_createdIsolate.future and cancelling _events, while preserving the existing
idempotent behavior; add a test that invokes dispose() twice before awaiting
either result and verifies both calls complete together.
In
`@packages/yet_another_json_isolate/test/yet_another_json_isolate_io_test.dart`:
- Around line 29-37: Update the “dispose completes when called twice” test to
bound the first isolate.dispose() await with the same five-second timeout as the
second call, or apply a shared deadline covering both disposal calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a33b15f2-73f5-46cf-b5e9-cc3522fc0975
📒 Files selected for processing (27)
examples/authentication/integration_test/flutter_test_config.dartexamples/authentication/pubspec.yamlexamples/database_crud/integration_test/flutter_test_config.dartexamples/database_crud/lib/main.dartexamples/database_crud/pubspec.yamlexamples/edge_functions/integration_test/flutter_test_config.dartexamples/edge_functions/pubspec.yamlexamples/passkeys/integration_test/flutter_test_config.dartexamples/passkeys/lib/main.dartexamples/passkeys/pubspec.yamlexamples/realtime_room/integration_test/flutter_test_config.dartexamples/realtime_room/pubspec.yamlexamples/storage_transforms/integration_test/flutter_test_config.dartexamples/storage_transforms/pubspec.yamlpackages/supabase/pubspec.yamlpackages/supabase/test/leak_test.dartpackages/supabase/test/leak_utils.dartpackages/supabase/test/leak_utils_test.dartpackages/supabase_flutter/lib/src/supabase.dartpackages/supabase_flutter/pubspec.yamlpackages/supabase_flutter/test/flutter_test_config.dartpackages/supabase_flutter/test/leak_test.dartpackages/supabase_flutter/test/leak_utils.dartpackages/supabase_flutter/test/widget_test.dartpackages/supabase_flutter/test/widget_test_stubs.dartpackages/yet_another_json_isolate/lib/src/_isolates_io.dartpackages/yet_another_json_isolate/test/yet_another_json_isolate_io_test.dart
Fixes three leaks found by auditing the packages and the example apps with leak_tracker. The tracking itself was a one-off investigation and is not part of this change. The Supabase singleton disposed everything it owned but never dropped the references, so the disposed SupabaseClient and its whole graph stayed reachable from a static field for the rest of the process. `client` is now a getter over a nullable field and `dispose()` clears every reference it holds. YAJsonIsolate.dispose() awaited a completer that only completes inside initialize(), so disposing an instance that was never used hung forever and left its ReceivePort open. It now returns early when nothing was spawned, and is idempotent. Both example rename dialogs created a TextEditingController in the calling method and never disposed it. Each is now a stateful dialog widget that owns its controller. BREAKING CHANGE: Supabase.instance.client is now a getter rather than a field, and throws after dispose() instead of returning the disposed client.
spydon
force-pushed
the
lukasklingsbo/sdk-1441-leak-tracker-audit
branch
from
August 7, 2026 09:40
75c53d9 to
03800ca
Compare
spydon
changed the title
Tr00d
approved these changes
Aug 7, 2026
`Supabase.client` threw a bare null check error in release builds, where the assert is stripped. It now throws a StateError with the same message in every mode. `Supabase.dispose()` was not safe to call twice: the second call went through the `client` getter, which throws once the reference is cleared. It now returns early when there is nothing to dispose, and drops its references before tearing anything down so a throwing step cannot leave the singleton pinning a half-disposed client. `YAJsonIsolate` rejected nothing after disposal, so a later decode would spawn a replacement isolate onto a closed receive port and then wait for a reply that never arrives. It now throws a StateError, and overlapping dispose() calls share one shutdown future rather than the second reporting completion early. Also registers the widget test's singleton teardown with addTearDown so a failing assertion cannot leak it into later tests, and trims the submitted name in the passkeys example.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/supabase_flutter/lib/src/supabase.dart`:
- Around line 229-247: Update the dispose flow to dispose the captured
lifecycleListener immediately after clearing _targetLifecycleState, before any
asynchronous teardown. Capture the existing _pendingLifecycleOperation and await
it after cancelling restoreSession and logSubscription but before disposing
currentClient, ensuring queued lifecycle work completes before
currentClient.dispose().
- Around line 243-247: Update the cleanup sequence around
restoreSession.cancel(), logSubscription.cancel(), currentClient.dispose(),
supabaseAuth.dispose(), and lifecycleListener.dispose() so every captured
resource cleanup is attempted even if an earlier operation throws. Use
try/finally or equivalent error aggregation while preserving propagation of
cleanup failures.
In `@packages/yet_another_json_isolate/lib/src/_isolates_io.dart`:
- Around line 33-35: Wrap the overlong documentation line in the isolate
initialization comment so every Dart source line stays within 80 characters,
preserving the existing wording and formatting the file with dart format.
In
`@packages/yet_another_json_isolate/test/yet_another_json_isolate_io_test.dart`:
- Around line 43-51: Update the concurrent dispose test around isolate.dispose()
to assert that the futures returned by the first and second calls are identical
before awaiting completion. Keep the existing Future.wait timeout assertion to
verify successful shutdown, ensuring the test covers both Future identity and
completion behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc5ef80a-2423-44c3-9ecf-1c2d9dbb6595
📒 Files selected for processing (6)
examples/passkeys/lib/main.dartpackages/supabase_flutter/lib/src/supabase.dartpackages/supabase_flutter/test/initialization_test.dartpackages/supabase_flutter/test/widget_test.dartpackages/yet_another_json_isolate/lib/src/_isolates_io.dartpackages/yet_another_json_isolate/test/yet_another_json_isolate_io_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/supabase_flutter/test/widget_test.dart
- examples/passkeys/lib/main.dart
The lifecycle listener is now disposed before anything is awaited, so no event can be queued while the client is being torn down. Operations queued earlier already abort as stale against the cleared target state. Each teardown step now runs even if an earlier one throws, so a failure part way through no longer leaves the rest of the graph alive. The first error is rethrown once everything has been attempted. Also asserts that overlapping YAJsonIsolate.dispose() calls return the same future, and wraps an over long doc comment.
Clearing the target lifecycle state stops a queued operation from rejoining channels, but an operation that already passed that check can be half way through `realtime.connect()`. That connect raced the `realtime.disconnect()` inside the client teardown and could reopen the socket after it closed. Dispose now waits for the captured lifecycle operation before disposing the client, so the two are serialized.
Keeps the dartdoc, including the wrapped `initialize()` line, and removes the `//` comments explaining the reasoning. Also drops the reset of `_pendingLifecycleOperation`: the field holds a completed chain that captures nothing beyond the already permanent singleton, and it is chained onto harmlessly by the next cycle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three leaks found by auditing the packages and the example apps with
leak_tracker. The tracking itself was a one-off investigation and is not part of this change.Closes SDK-1441.
Leaks fixed
The
Supabasesingleton pinned the disposed client graph.dispose()tore everything down but never dropped its references, so the disposedSupabaseClientand everything under it (auth, realtime, PostgREST, storage, functions, the http clients) stayed reachable from a static field for the rest of the process.clientis now a getter over a nullable backing field, anddispose()clears_client,_supabaseAuth,_lifecycleListener,_restoreSessionCancellableOperation,_logSubscriptionand the pending lifecycle operation. Clearing happens before teardown, so a step that throws cannot leave the singleton pinning a half-disposed client, anddispose()returns early when there is nothing to dispose so it is safe to call more than once.YAJsonIsolate.dispose()hung forever when the isolate was never used. It awaited_createdIsolate.future, which only completes insideinitialize(), so an instance that was never exercised never finished disposing and left itsReceivePortopen. Reachable throughPostgrestClientandFunctionsClientwhen they are handed a custom isolate that never runs.It now returns early after closing the port when nothing was spawned. Overlapping
dispose()calls share a single shutdown future, andinitialize(),decode()andencode()throw aStateErrorafterwards rather than spawning a replacement isolate onto a closed receive port and waiting for a reply that never arrives.This one also blocked the fix above: making the existing widget test dispose the singleton turned it into a 10 minute timeout until the isolate teardown was fixed.
Undisposed
TextEditingControllerin thedatabase_crudandpasskeysexamples. Both rename dialogs built a controller in the calling method and never disposed it.Each is now a
_RenameDialogstateful widget that owns and disposes its controller. Disposing in the caller does not work:whenCompleteon theshowDialogfuture fires while the route is still animating out, and theTextFieldthen rebuilds against a disposed controller.Tests
initialization_test.dartgains cases asserting the singleton drops its client reference on dispose and that a seconddispose()completes. The first captures the instance before disposing, becauseSupabase.instanceitself refuses to hand out a disposed instance.yet_another_json_isolategains cases for disposing an unused isolate, disposing twice, overlapping disposals, and using the isolate after disposal. Every disposal in that suite is bounded by a timeout so a regression fails rather than hanging.addTearDown, which also covers the isolate fix in a widget test context. It needstester.runAsyncaround both initialize and dispose, see the caveat below.MockWidgetstub now cancels its auth subscription indispose().Verification
supabase_flutter: 65 tests pass.supabase: 134 tests pass.yet_another_json_isolate: 27,postgrest: 196,functions_client: 48,realtime_clientandstorage_clientall pass.dcm analyze packagesis clean.database_crudwent from one reported leak to none.passkeysneeds real platform passkey support and does not run on an emulator, so its identical fix comes from inspection.Breaking change
Supabase.instance.clientis a getter rather than a field, so it can no longer be assigned, and it throws aStateErrorafterdispose()instead of returning the disposed client. Reads on an initialized instance are unchanged.Found but not fixed here
SupabaseClient.dispose()cannot complete insidetestWidgets. The constructor eagerly spawns the JSON isolate, andIsolate.spawnnever resolves under the fake clock, so consumers writing widget tests have to wrap bothSupabase.initializeanddisposeintester.runAsync. Makingdispose()non-blocking would weaken its contract, so it is a design call rather than a drive-by fix.The other finding from this audit,
RealtimeClient.remove()dropping unrelated channels, is fixed separately in #1669.Summary by CodeRabbit
New Features
Bug Fixes