fix(background): cancel open requests and clear the mirror on wallet reset - #1520
Conversation
df1d196 to
eb0d0fb
Compare
Comp0te
left a comment
There was a problem hiding this comment.
Read through the reset path this adds — the pre-reset snapshot, deliverResetCancels, the windowManagementReseted reducer and the direct session-mirror clear — along with the tests covering them. Most of the comments below share one through-line: the set of windows the reset closes is derived solely from windowIds on the snapshotted request rows, which makes it at once too wide (it can include the window the reset UI is itself running in) and too narrow (the export-keys window, and any request still between registration and window attach). One edit to what gets snapshotted and what gets excluded would cover all three. The last comment is about the test harness over the same code.
| const windowIds = [...new Set(openRequests.flatMap(r => r.windowIds))]; | ||
|
|
||
| for (const windowId of windowIds) { | ||
| windows.remove(windowId).catch(error => { |
There was a problem hiding this comment.
windows.remove here can close the window the reset was issued from: LockedRouter renders ResetVaultPage inside the signature-request and connect-to-app approval windows, and that window is attached to the open request. Killing it kills the page's own continuation — no onboarding tab, and on Firefox/Safari no runtime.reload(). The user recovers via the toolbar icon, but nothing does it for them.
Exclude the originating window from the removal set — its id is available to handleReduxAction — or open the onboarding UI from the background in the resetVault branch.
Basis
signature-request/app-router.tsx:27andconnect-to-app/app-router.tsx:34renderLockedRouter, which routes/reset-vaulttoResetVaultPage(locked-router/index.tsx:23-26) from the button atunlock-vault/index.tsx:142-149.sdk-methods.ts:266-284registers the request with no lock gate;open-window.ts:111attaches that window id to it.- No background code opens onboarding on reset. Both continuations that do live in a page this removal can destroy:
reset-vault/index.tsx:30-36(the only caller ofcloseWindowByReloadExtension) anderror/window-page.tsx:66-71. redux-actions.ts:343-346dispatches, then awaitsenableOnboardingFlow()before responding, so the ack trails the synchronous removal by at least one awaited call. Which IPC message lands first is not settleable from source.
There was a problem hiding this comment.
Fixed in bd7b960: the background's resetVault forwarding branch threads sender.tab?.windowId into the action payload (taken from MessageSender, so the page cannot self-report it; action type unchanged, UI dispatches untouched), and the saga excludes that id from the removal set. The exclusion skips only the removal, never the cancel delivery — and the excluded window converges on its own (Chrome closes itself, Firefox/Safari die with runtime.reload). Pinned by tests on both the forwarding branch and the saga (excluded even when the id also appears in a row's windowIds).
| // the reset flow does not rely on the subscriber's write guard to clear | ||
| // the session mirror for this slice — it clears it directly instead (see | ||
| // session-store.ts). | ||
| windowManagementReseted: () => initialState |
There was a problem hiding this comment.
windowManagementReseted nulls exportKeysWindowId, but deliverResetCancels closes only windows found in open requests. Reset with the Download-account-keys window open and it stays on screen with nothing left that will close it, and the single-export-window guard is defeated for the rest of that service worker's life — the next export skips reuse and calls windows.create.
Snapshot selectExportKeysWindowId and selectWindowId alongside selectOpenRequests and remove those windows too — or, if leaving them open is the intended trade, record it in a comment and a test. Same root cause as the removal set at onboarding-sagas.ts:75.
Basis
- No key material is duplicated. Post-reset the surviving window shows the error page, not keys:
sessionResetedrestoresisLocked, sounlock-vault/index.tsx:47-49throwsPasswordDoesNotExistErroranderror-boundary.tsx:33-59rendersWindowErrorPage. Only a newly opened window would render keys. - The guard is the loss:
export-keys-window-saga.ts:42-53enters its reuse branch only on a non-null id, andwindow-removed.ts:39-45— the only other writer of that id — fires on a close that now goes unrecorded. vault-sagas.ts:198-237: lock neither closes nor clears the export window, so it can still be on screen at reset time.
There was a problem hiding this comment.
Fixed in bd7b960: selectWindowId and selectExportKeysWindowId are snapshotted synchronously alongside the requests, and both (non-null, deduped, minus the originating window) join the removal set — the export-keys window is closed and its single-window guard survives reset. Test: {windowId: 7, exportKeysWindowId: 8} with no requests removes exactly the two.
|
|
||
| for (const windowId of windowIds) { | ||
| windows.remove(windowId).catch(error => { | ||
| console.error( |
There was a problem hiding this comment.
If this windows.remove rejects for any reason other than the window already being gone, the zombie approval window this PR exists to close persists with nothing able to find it — the requests map is wiped and the mirror cleared, so sweepOrphanedRequests has no descriptor to walk and never calls windows.remove anyway. Its likelihood I could not establish from source.
Route the failure through the sagaError channel this saga already uses, or retain the descriptor for a later sweep, so a failed close is not silent.
Basis
sweep-orphaned-requests.ts:26-35walks only the hydrated request snapshot, whichclearRequestSessionempties atonboarding-sagas.ts:126; its only action isfailRequestOnWindowError(:89-105), which never toucheswindows.*(cancel-requests.ts:270-311).- No other
windows.removecall site is a retry for this case —close-ledger-flow-windows.ts:82,close-windows-on-response.ts:79,open-export-keys-surface.ts:13,close-current-window.ts:14andexport-keys-window-saga.ts:134are all differently scoped.
There was a problem hiding this comment.
Fixed in bd7b960: window removal is now a forked saga (removeResetWindow) — a bare .catch() has no route to dispatch without closing the get-main-store -> root-saga -> onboarding-sagas cycle — and a rejection both logs (windowId + redacted error) and put(sagaError({source: 'resetVaultSaga', ...})). The synchronous-resets invariant is untouched: the fork sits after the resets and its only blocking call lives in the child task; the lands-synchronously test still pins it.
| }); | ||
| } | ||
|
|
||
| const windowIds = [...new Set(openRequests.flatMap(r => r.windowIds))]; |
There was a problem hiding this comment.
The removal set comes only from windowIds already on the snapshot rows, so a request still between windowRequestOpened and attachWindowToRequest contributes none — its window opens seconds after the reset and sits over a wallet that no longer exists. Not a regression — the base closed no windows at all — but a gap in the guarantee this PR adds.
Also remove selectWindowId(state) when non-null, or have openWindow's success arm close the window it just created when attachWindowToRequest finds no live request row. Same snapshot-derivation root cause as the comments on line 78 and windowManagement/reducer.ts:229.
Basis
reducer.ts:71-86creates the row withwindowIds: [], andsdk-methods.ts:266-284→open-window.ts:71-111fires the create and forgets it. After the wipewindowRequestWindowAttachedno-ops on the missing row (reducer.ts:123-131).- The liveness probe does not compensate:
attach-window-to-request.ts:48-52runsrepair()only for a non-extension page or a dead window, and it callscancelRequestsDisplacedBy, which closes nothing. - The window is not untracked —
windowIdChanged(create-open-window.ts:154-155→open-window.ts:73) writes the live id into the freshly reset slice, so a later approval will reuse it rather than leaving it forever.
There was a problem hiding this comment.
Partially fixed, partially accepted in bd7b960: selectWindowId (and exportKeysWindowId) now join the removal set, which covers every tracked window. The registration-to-attach race itself is kept as a documented residual (comment at the snapshot): the late-opening window is tracked via windowIdChanged into the fresh slice and reused by the next approval, exactly as you traced — the openWindow-closes-itself arm felt like cross-cutting machinery for a seconds-wide, self-limiting window.
| }); | ||
| }); | ||
|
|
||
| it('delivers the cancel for an open request at reset time, from the pre-reset snapshot', async () => { |
There was a problem hiding this comment.
Three properties this change is load-bearing on are pinned by no test. Two named mutations were run and left the whole suite green; the third is deductive from the fixtures. onboarding-sagas.test.ts is the only file in the repo that exercises this saga, so nothing else covers them either.
Worth one pass over this file: assert delivery in the real-store test, put frameId in the fixture, and add a two-request case.
The three
1. The pre-reset ordering — this test, :196. It cannot observe the "pre-reset" half its name claims: it runs on .withState(...) (:98-104), so no put changes what the select at onboarding-sagas.ts:96 reads. Moving that select to after put(windowManagementReseted()) — which reopens the leak this PR closes — kept 11/11 passing. The one real-reducer test (:163-194) mocks deliverCancelResponse and windows.remove to never-resolving promises and asserts neither. With the real reducer already wired there, resolving that mock and asserting deliverCancelResponse was called with requestId: 'r1' pins it.
2. frameId reaching deliverCancelResponse — :205. The fixture (:88-96) carries no frameId, and the delivery assertion is an objectContaining without it, so nothing pins that the saga passes the whole row. Narrowing the argument to the declared CancelDeliveryRow shape — which mirrors the sibling call site at cancel-requests.ts:285-288 and reads like a tidy-up — left 381 tests passing, and it defeats two guards: the targeted send at cancel-requests.ts:234-236, and deliver-via-origin.ts:28-34, whose sub-frame refusal is keyed on frameId != null, so an omitted id resumes the unscoped broadcast to every active same-origin tab. fail-request-on-window-error.test.ts:173-193 pins this for the other caller.
3. The loop, the flatten and the Set — onboarding-sagas.ts:65. Every test drives one request with one window (:88-104) or an empty map (:226-256), so openRequests.slice(0, 1) and flatMap(...) → map(r => r.windowIds[0]) are both behaviourally identical under the entire suite. Two simultaneous requests are reachable: open-window.ts:64-69 nulls reusableWindowId while a device is busy, and windowManagement/types.ts:11-15,39-47 documents the Ledger flow attaching a second window to one requestId. One case with r1: { windowIds: [42, 43] }, r2: { tabId: 9, windowIds: [42] } asserting two deliveries and removals of exactly 42 and 43 covers all three.
There was a problem hiding this comment.
All three pinned in bd7b960: (1) the real-store test now resolves the deliverCancelResponse mock and asserts the call with the snapshot row — your mutation (select moved after the reset put) goes red against it (verified by running it); (2) frameId: 5 is in the fixture and asserted through to delivery; (3) the two-request/multi-window case (r1 [42,43], r2 [42] -> two deliveries, removals exactly 42 and 43) plus a combined widened-set case (windowId 44, exportKeysWindowId 45, origin 43 excluded -> exactly {42,44,45}).
eb0d0fb to
d79df2b
Compare
bd7b960 to
e3c1dc2
Compare
e3c1dc2 to
8e7efd3
Compare
8e7efd3 to
21bf6dd
Compare
21bf6dd to
57f262a
Compare
57f262a to
df8b17d
Compare
df8b17d to
1ecadeb
Compare
Description
A
developdefect, separable from the mirror but shipped in its stack:resetVaultSagadispatches twelve slice resets plusstorage.local.clear(), andwindowManagementis not among them — no reset action existed for it. Worse,resetVaultis reachable from three onboarding pages and from inside the approval window itself (ResetVaultPageviaLockedRouter), and nothing in the reset flow answered the dapp or closed that window: the reset left a zombie signing prompt for a wallet that no longer exists, and the dapp's promise hung.A blind "also clear windowManagement" would be worse than the leak — wipe the descriptors and the later window close finds nothing to cancel. And a naive cancel-then-clear breaks reset outright on Firefox and Safari: today the twelve
puts complete synchronously insidestore.dispatch(resetVault()), before the UI'scloseWindowByReloadExtension()runsruntime.reload(). Any awaited I/O before the resets suspends the saga, the reload kills it, and the resets andstorage.local.clear()never execute — "Reset wallet" silently resets nothing over an intact vault.The fix — order is the whole point
Snapshot
selectOpenRequestssynchronously.Reset synchronously — all slice resets, now including the new
windowManagementreset action, still complete inside thedispatchcall; the mirror is cleared via the session store's own serialised write chain (not left to the subscriber guard, which suppresses identity-returns).Deliver afterwards, from the snapshot: each open request gets its cancel through a store-free delivery helper (a request's
method/requestId/tabId/origin/frameIdare all in the snapshot row), and the removal set is closed overwindows.remove. Deliveries are fire-and-forget — a slow or rejecting delivery cannot delay or break the resets.The removal set (review round): the snapshot's
windowIdsplus the shared approvalwindowIdandexportKeysWindowId(both snapshotted synchronously alongside the requests — otherwise the Download-account-keys window survives reset with its single-window guard defeated), minus the window the reset was issued from —ResetVaultPageruns inside approval windows, and closing the originating window would kill the page's own continuation (on Firefox/Safari that window dies withruntime.reload()anyway; on Chrome it closes itself). The background threadssender.tab.windowIdinto the action payload for that exclusion; the exclusion skips only the removal, never the cancel delivery. A failedwindows.removeis no longer silent: with the descriptors already wiped nothing could ever find that window again, so the failure is routed throughsagaError(a forked saga — a bare catch has no dispatch without closing an import cycle). Accepted residual, documented in code: a request still between registration and window-attach contributes no window; its late-opening window is tracked viawindowIdChangedinto the fresh slice and reused by the next approval.The delivery helper is extracted from
failRequestOnWindowError's delivery path (PR D), which now delegates to it — its existing callers' behaviour is unchanged, and the helper carries #1484's frameId semantics.failRequestOnWindowErroritself could not be reused here: it needs the store object, and importing the store singleton intoonboarding-sagas.tswould close a runtime import cycle.Wiring notes
EXCLUSIONStail ofredux-actions.parity.test.ts.runtime.reload(), so the cancel is usually lost there. That is no worse than today (nothing was delivered at all), and the alternative — awaiting delivery before the resets — is exactly the "reset silently resets nothing" hazard this PR closes.Verification
npx jest src/background/— 69 suites, 925 tests pass;npx tsc --noEmitclean; eslint/prettier clean; circular-dependency count unchanged.windowManagement/reducer.ts100/100/100/100 with the new action; sagas and handlers directories above their floors.Linked tickets
WALLET-1419
Checklist
Make sure this PR title follows semantic release conventions: https://semantic-release.gitbook.io/semantic-release/#commit-message-format
If the PR adds any new text to the UI, make sure they are localized — no UI text added
Include a screenshot or recording if implementing significant UI or user flow change — background-only, no UI change
When this PR affects architecture changes wait for review from Dmytro before merging