fix(sdk): close balance and pubkeysubscriptions on auth state changes#232
fix(sdk): close balance and pubkeysubscriptions on auth state changes#232
Conversation
reactivating existing subscriptions causes a bunch of side effects alongside assets active in the background that are not always present in the currently signed in wallet (consuming unnecessary resources etc. alongside a memory leak possibility)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors reset flows in BalanceManager and PubkeyManager to perform concurrent cancellation and cleanup of watchers/controllers, add elapsed-time logging, and remove automatic watcher restart/auto-activation post-reset. Substantial new tests validate concurrent cleanup, error resilience, performance, memory integrity, and disposal behaviors for both managers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Manager as BalanceManager/PubkeyManager
participant Watchers as Active Watchers
participant Controllers as Stream Controllers
participant Log as Logger
App->>Manager: resetState()
activate Manager
Manager->>Manager: Capture active watchers & controllers
par Cancel watchers (concurrent)
Manager->>Watchers: cancel() (non-blocking)
Watchers-->>Manager: completion or error (logged)
and Close controllers (concurrent)
Manager->>Controllers: addError(disconnected)\nclose()
Controllers-->>Manager: completion or error (logged)
end
Manager->>Manager: Clear caches/internal maps
Manager->>Log: log(duration, counts)
deactivate Manager
Note over Manager,App: No auto-restart/auto-activation after reset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses critical issues in the auth state change handling for PubkeyManager and BalanceManager by replacing the previous reactivation approach with proper controller closure and implementing concurrent cleanup operations using Future.wait().
- Replaced controller reactivation with proper closure during auth state changes
- Implemented concurrent cleanup operations using
Future.wait()to improve performance - Added comprehensive error handling and performance logging for cleanup operations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart | Updated _resetState() method to close controllers concurrently instead of reactivating them |
| packages/komodo_defi_sdk/lib/src/balances/balance_manager.dart | Updated _resetState() method to close controllers concurrently with improved error handling |
| packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart | Added extensive test suites for concurrent cleanup, memory leak prevention, and performance benchmarking |
| packages/komodo_defi_sdk/test/balances/balance_manager_test.dart | Added comprehensive test coverage for concurrent operations and performance measurements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Visit the preview URL for this PR (updated for commit bfb0677): https://kdf-sdk--pr232-bugfix-auto-restart-vrf6yb7g.web.app (expires Fri, 10 Oct 2025 14:25:09 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
|
Visit the preview URL for this PR (updated for commit bfb0677): https://komodo-playground--pr232-bugfix-auto-restart-irhydrhk.web.app (expires Fri, 10 Oct 2025 14:24:41 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart (2)
194-197: Stream chain formatting nit (already discussed elsewhere)Chained take(1).toList() is fine; previous reviews flagged formatting preference. No action needed.
351-354: Formatting nit (already discussed elsewhere)Same prior nit about chaining .timeout(...).take(1).toList() formatting. Fine to keep as-is.
🧹 Nitpick comments (3)
packages/komodo_defi_sdk/lib/src/balances/balance_manager.dart (1)
151-160: Consider signaling with a specific error typeUsing a generic StateError works, but a custom exception (e.g., WalletChangedDisconnect) would let downstream distinguish auth-cycle disconnects from real failures.
packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart (1)
373-391: Align disposal logging with BalanceManager (catch per future)Dispose currently relies on a single Future.wait + outer try/catch. For consistent diagnostics and resilience, attach catchError to each cancel/close (like BalanceManager) so one failure is logged with context and doesn’t mask others.
Apply this diff:
- final List<StreamSubscription<dynamic>> watcherSubs = _activeWatchers.values - .toList(); + final List<StreamSubscription<dynamic>> watcherSubs = _activeWatchers.values.toList(); _activeWatchers.clear(); - for (final StreamSubscription<dynamic> subscription in watcherSubs) { - pending.add(subscription.cancel()); - } + for (final StreamSubscription<dynamic> subscription in watcherSubs) { + pending.add( + subscription.cancel().catchError((Object e, StackTrace s) { + _logger.warning('Error cancelling pubkey watcher', e, s); + }), + ); + } - final List<StreamController<AssetPubkeys>> controllers = _pubkeysControllers - .values - .toList(); + final List<StreamController<AssetPubkeys>> controllers = _pubkeysControllers.values.toList(); _pubkeysControllers.clear(); - for (final StreamController<AssetPubkeys> controller in controllers) { - pending.add(controller.close()); - } + for (final StreamController<AssetPubkeys> controller in controllers) { + pending.add( + controller.close().catchError((Object e, StackTrace s) { + _logger.warning('Error closing pubkey controller', e, s); + }), + ); + }Also applies to: 380-387
packages/komodo_defi_sdk/test/balances/balance_manager_test.dart (1)
208-751: Solid functional/perf coverage; minor flakiness riskThe suite robustly validates concurrent cleanup, resilience, and cache clearing. Some waits use real timers (e.g., 150–300ms). Consider FakeAsync or event-driven completers to reduce flakiness on slow CI.
If you see intermittent CI timing failures, I can help adapt key tests to FakeAsync where feasible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/komodo_defi_sdk/lib/src/balances/balance_manager.dart(1 hunks)packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart(2 hunks)packages/komodo_defi_sdk/test/balances/balance_manager_test.dart(2 hunks)packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart(4 hunks)
🔇 Additional comments (7)
packages/komodo_defi_sdk/lib/src/balances/balance_manager.dart (2)
131-142: Good concurrent teardown with per-operation error handlingCollecting subscriptions/controllers first, clearing maps, then cancelling/closing concurrently with catchError per future is solid and avoids unhandled errors. Nice.
Also applies to: 149-167
168-173: Useful telemetryStopwatch-based reset duration logging with counts is helpful for perf diagnostics. LGTM.
packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart (2)
351-356: Great: duration + counts logStopwatch telemetry helps validate concurrent cleanup behavior. 👍
322-347: No action needed:_watchedAssetsis already cleared on reset
The reset/dispose logic atpubkey_manager.dart:395–401includes_watchedAssets.clear().Likely an incorrect or invalid review comment.
packages/komodo_defi_sdk/test/balances/balance_manager_test.dart (1)
23-54: Fallback registrations: good coverage for complex mocksGlobal registerFallbackValue for AssetId/Asset avoids mocktail type issues across tests. LGTM.
packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart (2)
22-54: Fallback registrations for deep typesGood global fallback registrations to stabilize mocktail behavior across suites. LGTM.
654-2016: Excellent concurrency and performance coverageStrong end-to-end tests for concurrent cleanup, memory integrity across auth cycles, and perf baselines. Use of FakeAsync where applicable is great. Approved.
takenagain
force-pushed
the
bugfix/auto-restart-watchers
branch
from
a63991f to
bfb0677
Compare
This PR addresses duplicate background activation attempts in the auth state change handling for
PubkeyManagerandBalanceManager, which result in IndexedDB lock errors for subsequent ZHTLC activation requests from komodo-wallet (accompanying improvement to flush KW watchers in #3168).This fixes the issue where signing out and back into the same account results in ARRR activation attempts failing in the foreground while activation succeeds from the SDK without means of "watching" existing activations.
Further improvements to
SharedActivationCoordinatorare required to gracefully handle duplicate activation requests and to return existing activation futures/streams to the duplicate requests rather than creating new activation requests as it does now.Problem Description (Generated with Claude Sonnet 4.0)
Previous Implementation Issues
The previous implementation had several critical problems when users signed out and back into the same wallet:
activateIfNeeded=trueSide Effects of Previous Implementation
Solution
Key Changes
Future.wait()to perform cleanup operations in parallelImplementation Details
_resetState()to close all controllers and cancel subscriptions concurrently_resetState()to close all controllers and cancel subscriptions concurrentlycatchError()for each cleanup operationFlow Comparison
Before: Sequential Cleanup with Reactivation
sequenceDiagram participant User as User participant Auth as Auth System participant PM as PubkeyManager participant BM as BalanceManager participant Controllers as Stream Controllers participant SAC as SharedActivationCoordinator User->>Auth: Sign Out Auth->>PM: handleAuthStateChanged(null) Auth->>BM: handleAuthStateChanged(null) PM->>PM: _resetState() loop Sequential Subscription Cleanup PM->>PM: await subscription.cancel() end Note over PM,Controllers: Controllers remain open PM->>Controllers: addError("Wallet changed") loop Sequential Controller Restart PM->>Controllers: restart watcher Controllers->>SAC: activateAsset() [DUPLICATE] end BM->>BM: _resetState() loop Sequential Subscription Cleanup BM->>BM: await subscription.cancel() end Note over BM,Controllers: Controllers remain open BM->>Controllers: addError("Wallet changed") loop Sequential Controller Restart BM->>Controllers: restart watcher Controllers->>SAC: activateAsset() [DUPLICATE] end User->>Auth: Sign In (same wallet) Note over SAC: Multiple activation attempts conflict SAC-->>Controllers: Activation failuresProblems with Previous Flow:
After: Concurrent Cleanup with Controller Closure
sequenceDiagram participant User as User participant Auth as Auth System participant PM as PubkeyManager participant BM as BalanceManager participant Controllers as Stream Controllers participant SAC as SharedActivationCoordinator User->>Auth: Sign Out Auth->>PM: handleAuthStateChanged(null) Auth->>BM: handleAuthStateChanged(null) PM->>PM: _resetState() PM->>Controllers: addError("Wallet changed") par Concurrent Cleanup PM->>PM: Future.wait([subscription cancellations]) and PM->>Controllers: Future.wait([controller.close()]) end PM->>PM: Clear caches and maps Note over PM: All resources properly disposed BM->>BM: _resetState() BM->>Controllers: addError("Wallet changed") par Concurrent Cleanup BM->>BM: Future.wait([subscription cancellations]) and BM->>Controllers: Future.wait([controller.close()]) end BM->>BM: Clear caches and maps Note over BM: All resources properly disposed User->>Auth: Sign In (same wallet) Note over PM,BM: Clean slate - new controllers created as needed PM->>Controllers: Create new controllers on demand BM->>Controllers: Create new controllers on demand Controllers->>SAC: Fresh activation requests (no conflicts)Benefits of New Flow:
Summary by CodeRabbit
Performance
Bug Fixes
Stability
Tests