Uh oh!
There was an error while loading. Please reload this page.
chore: improve unsub failure and retry logic - #9678
Merged
sahar-fehri merged 12 commits intoAug 4, 2026
Merged
Conversation
sahar-fehri
marked this pull request as ready for review
July 28, 2026 13:39
sahar-fehritemporarily deployed
to
default-branch
July 28, 2026 13:39 — with 
GitHub Actions
Inactive
Uh oh!
There was an error while loading. Please reload this page.
Kriys94
reviewed
Jul 29, 2026
Uh oh!
There was an error while loading. Please reload this page.
Kriys94
reviewed
Jul 29, 2026
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Prithpal-Sooriya
previously approved these changes
Jul 29, 2026
Kriys94
previously approved these changes
Jul 29, 2026
Kriys94
left a comment
Contributor
There was a problem hiding this comment.
Slight preference for re-using TanStack, but I'm also good with the implementation
sahar-fehri
dismissed stale reviews from Kriys94 and Prithpal-Sooriya
via
August 3, 2026 08:03
5a03e1bsahar-fehri
commented
Aug 3, 2026
ContributorAuthor
@metamaskbot publish-preview |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5a03e1b. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
sahar-fehri
commented
Aug 3, 2026
ContributorAuthor
@metamaskbot publish-preview |
Contributor
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
sahar-fehri
enabled auto-merge
August 3, 2026 12:04
| const subscribePromise = service.subscribe(SUB_OPTS); | ||
| jest.advanceTimersByTime(3000); | ||
| await flushPromises(); |
Contributor
There was a problem hiding this comment.
If we see flakiness here, then let's replace with waitFor(). Not blocking
Comment on lines
+640
to
+648
| jest.advanceTimersByTime(3000); | ||
| await flushPromises(); | ||
| service.destroy(); | ||
| releaseUnsub(new Error('ws gone')); | ||
| await completeAsyncOperations(); | ||
| jest.advanceTimersByTime(1000); | ||
| await completeAsyncOperations(); |
Contributor
There was a problem hiding this comment.
Same here, not blocking but the test specs need a proper refactor
| // no-op | ||
| }); | ||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| this.#performUnsubscribe(channel); |
Contributor
There was a problem hiding this comment.
Can we add back the catch noop, protects from throwing?
| return; | ||
| } | ||
| entry.retryAbort?.abort(); |
Contributor
There was a problem hiding this comment.
Not - Hmm I wonder if this additional abort logic can be captured by a "debounce"?
ContributorAuthor
There was a problem hiding this comment.
The timing side is actually already a debounce: the grace-period timer waits GRACE_PERIOD_MS before tearing down and a fast re-subscribe cancels it. But the bit flagged here (retryAbort) is doing something a debounce can't: cancelling an in-flight retry loop mid-backoff (the 1s → 2s → 4s window before we fall back to forceReconnection).
A debounce only reacts to repeats of the same event and only delays the start. Here we need to interrupt work that's already running, triggered by three different external events:
1- User returns to the chart (#subscribeInner): abort the teardown and reuse the still-alive WS subscription instead of unsubscribing the channel they're now viewing.
2- Service teardown (destroy() → #clearChannelTimers): abort so an orphaned retry can't fire forceReconnection on the shared socket after teardown (the resurrection bug this PR fixes).
3- Reconnect cleanup (#resubscribeActiveChannels): abort a retry that's targeting the pre-reconnect subscription so we don't trigger a redundant reconnect storm.
| ): Promise<void> { | ||
| try { | ||
| await unsubRetryPolicy.execute(async () => { | ||
| const releaseLock = await this.#mutex.acquire(); |
Contributor
There was a problem hiding this comment.
Locking worries me.
- Do we need locks for this logic?
- Are we confident that we cover all possible unlocks?
- Is this shared mutex used on other operations. Does that mean if lock is "stuck" other operations are blocked?
ContributorAuthor
There was a problem hiding this comment.
Fair questions
1- Do we need locks? Yes; #channels and refCount are mutated from several concurrent async entrypoints (UI subscribe/unsubscribe, the grace-timer callback, the retry loop, and reconnect resubscribe). Without serialization we'd race on refCount and on WS subscribe/unsubscribe ordering (e.g. a double-subscribe, or unsubscribing a channel mid-subscribe).
2- Are all unlocks covered? Yes; every acquire() is paired with try { … } finally { releaseLock() }: subscribe, unsubscribe, #performUnsubscribe, the #runUnsubRetryLoop execute callback, and #resubscribeActiveChannels. Release always runs, even on throw.
3- Is it a shared mutex that could block other ops? It's private per OHLCVService instance (readonly #mutex = new Mutex()), not shared with BackendWebSocketService or any other service so worst case it only serializes OHLCV's own subscribe/unsubscribe, nothing else. Also worth noting the lock is released between retries: Cockatiel's backoff waits happen outside the execute callback, so the 1s/2s/4s delays don't hold the mutex.
Prithpal-Sooriya
approved these changes
Aug 4, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
OHLCVServiceunsubscribe cleanup so failed WebSocket unsubs no longer leak gateway subscription slots and block future OHLCV streams.1s → 2s → 4s) and callforceReconnectiononly after retries are exhausted.Explanation
Current state: When
#performUnsubscribefailed,OHLCVServiceremoved the channel from local tracking before the gateway unsub succeeded. That left orphaned server-side subscriptions, which could exhaust the gateway's 2market-datasubscription cap and cause later subscribes to fail (clients fall back to REST/latestpolling).Solution:
BackendWebSocketService:forceReconnectionto reset server subscription state.Same-channel grace (3s) is unchanged so users returning to the same token/interval within 3s can reuse the existing subscription.
Important:
forceReconnectionaffects the shared WebSocketBackendWebSocketServiceis a single shared connection on mobile, used by multiple services (not OHLCV-only):AccountActivityServiceOHLCVServiceAssetsController(indirect)AccountActivityService:balanceUpdatedCalling
forceReconnection()closes and reconnects that entire WebSocket. On disconnect it clears all local subscription state; on reconnect each service resubscribes what it needs:AccountActivityService— marks chains down onDISCONNECTED, then resubscribes the selected account onCONNECTED(this service already usesforceReconnectionfor its own cleanup today).OHLCVService— only resubscribes channels withrefCount > 0(active chart consumers). Failed-cleanup / grace entries (refCount === 0) are dropped on reconnect and are not resubscribed.When does OHLCV trigger it?
Only as a last resort, after unsub has failed 4 times total (initial grace-period attempt + 3 retries at 1s / 2s / 4s). Typical navigation (leave chart, switch token, return within grace) uses delete-on-success, flush, or grace reuse — not force reconnect.
Rough timeline after user leaves a chart with a stuck unsub: ~10s of background retries before reconnect fires.
User-visible impact
refCount === 0)Why we still use it
market-datasub hits the gateway 2-sub cap and can break all OHLCV streaming on that connection — worse than a one-time reconnect blip.forceReconnectionis an existing, shared primitive (introduced in fix(core-backend): reconnection logic #6861;AccountActivityServicealready relies on it).Open question for reviewers: If the cross-service reconnect blast radius is unacceptable, alternatives are (a) gateway-side sub cleanup API, or (b) OHLCV-only reconnect/isolation — neither exists today on the shared
BackendWebSocketService.Fixes
References
UNSUBSCRIBE_LEAK_FIX_PLAN.mdin metamask-mobileBackendWebSocketService.forceReconnection()— fix(core-backend): reconnection logic #6861Test plan
yarn workspace @metamask/core-backend run jest --no-coverage packages/core-backend/src/ws/ohlcv/OHLCVService.test.ts(43 tests)forceReconnection→ WS disconnected / connected cyclelimit would be exceeded)Checklist
forceReconnectionin code (JSDoc) — PR description above; happy to add JSDoc in follow-up if reviewers want it in-treeNote
High Risk
Changes real-time OHLCV subscription lifecycle and can trigger shared WebSocket
forceReconnection, affecting other consumers (e.g. account activity) after repeated unsub failures.Overview
Fixes OHLCV WebSocket unsubscribe leaks that could leave orphaned gateway
market-datasubscriptions and hit the 2-sub cap, breaking later chart streams.OHLCVServicenow keeps channel entries until the server unsub succeeds (delete-on-success). When subscribing to a different asset/interval, it flushes other channels still in grace or failed cleanup instead of holding two live subs for up to 3s; same-channel grace reuse is unchanged but also flushes other pending channels first. Failed unsubs are retried via cockatiel with 1s → 2s → 4s backoff; only after retries are exhausted does it callBackendWebSocketService:forceReconnectionon the shared connection. Retries are cancellable viaAbortController(resubscribe,destroy, reconnect dropsrefCount === 0entries).destroyclears grace and retry timers; reconnect no longer resurrects stuck cleanup channels.Adds
cockatielto@metamask/core-backendand expandsOHLCVService.test.tsfor flush, retry, destroy races, and connection-state edge cases.Reviewed by Cursor Bugbot for commit f44eea8. Bugbot is set up for automated code reviews on this repo. Configure here.