fix(background): keep request descriptors across a worker restart

[ost-ptk]

Description

windowManagement.requests lived only in the MV3 service worker's memory. A worker restart destroyed every request descriptor while the approval windows they describe were still on screen and still signable.

Four things broke at once:

• Cancel-on-close. cancelRequestsDisplacedBy selects from selectOpenRequests; with no descriptor there is nothing to select, so closing the window told the dapp nothing and its promise hung to the SDK's own 30-minute timeout.
• Response dedup. markRequestResponded early-returns before dispatching when the descriptor is missing, so a genuine post-restart response left no tombstone at all — every later response for that id also passed the guard.
• Supersede. Two losses, not one: no descriptor to displace, and windowManagement.windowId is gone too, so createOpenWindow never enters its reuse branch and the next approval opens a second window.
• awaitingDeviceConfirmation. The WALLET-1394 guard that keeps a window out of reuse during a Ledger confirmation silently re-armed — the flag is announced once at bracket start and never re-sent.

The fix

{ requests, windowId } is mirrored into chrome.storage.session and hydrated in the get-main-store.ts preload.

Why storage.session. It is in-memory, survives a worker restart, and is cleared when the browser closes or the extension reloads — exactly the lifetime a request descriptor wants. That is why there is no purge, no session marker and no TTL: nothing can outlive the session it belongs to. It also keeps dapp origins and tab ids off disk, and stops browser-session-scoped window/tab ids from ever being read back in a session that has reassigned them.

Why the preload and not a saga. The event that wakes a dead worker is often the approval window closing itself. windows.onRemoved awaits store init and then reads selectOpenRequests synchronously, so a saga's first await is already too late — and a window-URL rebuild is worse still, since the window whose close woke the worker is by then gone from windows.getAll. The preload reads the session area alongside the existing storage.local.get, so the map is present the moment the store exists and no handler can observe it empty.

Details worth a reviewer's attention

• The write is a separate call to a separate area with its own catch — never a field in the twelve-key storage.local.set. That call also writes VAULT_CIPHER_KEY, and requestId is dapp-chosen with no length bound anywhere, so sharing the write would let a page fail the vault persist.
• Rows are capped on the write side; the read is uncapped. This leaves no read-side drop order for integer-key hoisting to decide.
• A rejected write removes the key. An absent mirror behaves exactly like today; a stale one can pin a request open for the whole browser session. Writes are serialised so an older snapshot cannot land after a newer one, and the flush never rejects — a rejection would poison the chain for every later write.
• createStore(preloadedState) bypasses every case reducer, so the restored map is validated by a total sanitizer that drops what it cannot vouch for rather than throwing. A throw here would leave the background unable to start at all.
• The subscriber guard compares the (requests, windowId) pair, not the slice reference — four case reducers return a fresh object for a value-equal write.
• Chrome and Edge only, behind a build-time predicate and a runtime detect. Firefox and Safari declare "persistent": true, so their background page never dies and the mirror would be a live untested path there. The runtime half is still needed because @types/webextension-polyfill declares storage.session non-optional even where it does not exist.
• isEphemeralBackgroundBuild is byte-identical in expression to isLedgerAvailable. Deliberate — two different concepts that happen to coincide today.
• The subscriber's .catch on the mirror write is unreachable by construction (the flush never rejects). It is there because the write must not be able to take anything else down with it; flagging it so it is not read as dead code.

Verification

• npx jest src/background/ — 62 suites, 786 tests pass. npx tsc --noEmit clean. knip clean.
• windowManagement/reducer.ts stays at 100% coverage; src/background/handlers/ stays above its floor.
• The jest trap was mutation-checked. isEphemeralBackgroundBuild is false under jest (npm test sets no BROWSER, DefinePlugin is webpack-only), so an unmocked test would exercise the disabled path and pass while asserting nothing. Flipping the mock to false fails 31 of 42 session-store tests and 2 get-main-store tests — the tests really do run the enabled path.
• A new e2e locks in the close-as-wake ordering (second commit): the worker is stopped, the approval window is closed programmatically, and the dapp must settle with a cancel inside 15s. Verified to fail without the fix — with the gate off it times out on exactly that assertion, not on a setup step. It cannot live in the popup suite, which runs under MOCK_STATE and short-circuits the session read.

Not in this PR

The startup sweep, the open-request cap, and the wallet-reset fix are deliberately separate — they are compensating mechanisms with their own risk, and this change stands on its own without them. frameId (#1484, since merged) is carried in the mirrored record and sanitized on hydration: Number.isInteger only, optional — 0 (the top frame) is preserved verbatim, a malformed value drops the field but keeps the row.

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

[Comp0te]

Read the session mirror end to end: the new session-store.ts, the get-main-store.ts hydration, the storage.session surface in docs and static analysis, and the e2e restart harness. The through-line of the comments below is the mirror's trust boundary — what bounds dapp-controlled data before it is written, what reconciles a restored row against the windows that actually exist, and which of the file's own guards the suite can hold in place. One interaction worth knowing before fixing: retrying a quota rejection with a trimmed map rather than removing the key makes a stranded restored row likelier to survive, so that change and a post-hydration sweep are best considered together.

[Comp0te]

Restored open rows are installed unreconciled, and nothing retires one whose windowIds names a dead window — onRemoved already fired. vault-sagas.ts:522-526 unions it into keep on every vaultLoaded, pinning one of ten MAX_STORED_PAYLOADS slots. Signing dies at ten, each needing a worker death after that payload was persisted.

Is a post-hydration sweep via collectRequestIdsFromOpenWindows in scope here?

Basis

Nothing retires the row: cancelRequestsDisplacedBy (cancel-requests.ts:174-179) selects only requests whose windowIds hold the removed id, and failRequestOnWindowError fires only for a window this generation opened — so it stays in selectOpenRequests (selectors.ts:47-53). At capacity storePayload (vault/reducer.ts:116-121) refuses, surfacing as 'Too many pending signature requests' (sdk-methods.ts:58, :258/:362). Lock/unlock does not reclaim: mergePayloadMaps (:255-269) evicts only when cipher plus in-memory exceeds the ceiling, and here in-memory is empty.

The producing window is a detach — on screen long enough for updateVaultCipher (vault-sagas.ts:117) to persist, then the worker dying inside CANCEL_GRACE_MS — or any restored row naming an already-closed window. sdk-methods.ts:96-98 also throws 'Duplicate requestId' for that id thereafter.

[ost-ptk]

No change here, by design: retiring a restored row whose windows are dead is exactly the startup sweep, which is deliberately a separate PR in this stack — #1519 (sweep-orphaned-requests.ts). It cancels precisely the shape you describe (a hydrated 'open' row no open window's URL claims), with the liveness test on window URLs via collectRequestIdsFromOpenWindows called unchanged, a CANCEL_GRACE_MS delay, and the parameterised failRequestOnWindowError as the vehicle. The split is intentional — the mirror ships as the verified core, the sweep as separable hardening (see the PR body's 'Not in this PR' note).

[Comp0te]

Confirmed and accepted — get-main-store.ts is byte-identical across both heads, no sweep module here. One correction in your favour: the open→attach gap cannot strand a vault payload at all, and exhausting the ten payload slots needs ten independent strandings. The caveat is ordering: until #1519 lands, a crash in the detach grace strands a row nothing retires.

Basis

Verified against 497d3a06 and e581d9fd: requests: requestSession.requests sits at :141 inside the same :138-142 block, with no reconciliation before or after startBackground() at :146. collectRequestIdsFromOpenWindows (open-request-windows.ts:26) still has exactly one caller, reconcileStalePayloadsSaga at vault-sagas.ts:508, which reclaims vault payloads rather than request rows.

The correction: a payload reaches the cipher only via the 500 ms debounced updateVaultCipher, and the open→attach gap is one windows.create round trip — shorter than the debounce, so a row stranded there pins nothing. The pin needs either a detach after the debounce persisted, or a restored row naming an already-closed window. And 'Too many pending signature requests' (sdk-methods.ts:58) needs all ten of MAX_STORED_PAYLOADS (vault/reducer.ts:37) taken, which my original wording did not say. What does hold: mergePayloadMaps evicts only when cipher plus in-memory exceeds the ceiling, and in-memory is empty after a restart, so a stranded row is not reclaimed by the merge.

[ost-ptk]

Thanks for the verification and the correction — noted that the open-to-attach gap alone cannot strand a payload (shorter than the cipher debounce) and that the pin needs a post-debounce detach or a restored dead-window row. The ordering caveat is exactly why #1519 sits directly above this PR in the stack.

[Comp0te]

Four guards in this file can each be deleted with both changed suites green — 66 tests. In every case the test aimed at the guard also passes without it, because the unguarded fallback produces the same value the assertion checks.

Could each get a case that distinguishes the guard from its fallback? They are one hardening pass, not four.

The four, and what each needs

• :93-95, the totality guard. session-store.test.ts:286 (['a non-object row', 'nope']) passes without it — ('nope').seq is undefined, so :102 rejects the row anyway. No row value in either suite is null. With the guard gone a null row throws, the catch at :227-231 returns emptyRecord(), and a one-row drop becomes whole-mirror loss — the opposite of the contract at :90-91. Add ['a null row', null] and ['an undefined row', undefined] at :258-287.
• :153, the __proto__ rejection. :302-320 passes without it: the assignment at :162 runs the Object.prototype.__proto__ setter on the literal from :149, setting the map's prototype instead of adding a key, so Object.keys is still ['good'] and toEqual ignores prototypes. The over-long sibling key is caught by the separate check at :154 and does not backstop it. request-map.ts:8-19 records this bug class reaching production in this file once. Assert Object.getPrototypeOf(requests) === Object.prototype.
• :145, Array.isArray, and :217-219, the record shape. All four assertions at :145-156 and :158-167 hold for the unguarded fallback too — 'nope' and ['a'] destructure to undefined, and null throws into the catch, which returns exactly the expected emptyRecord(). Feed requests: [openRow(), openRow()] and assert {}; it hydrates as {'0': …, '1': …} without the guard.
• :284, the write serialisation the comment at :234-236 calls the unsafe direction. No test ever has two set calls in flight: all nine write-path tests (:332-454) stub sessionSet as resolved/rejected, never a deferred. :388-401 awaits the first write before issuing the second, and writeRequestSession returns writeChain, so the two are sequential by construction and the [1, 2] assertion at :394-400 is true of an unchained implementation. Make sessionSet a deferred; issue A, let flushQueued reset, issue B, assert set ran once, then resolve A.

[ost-ptk]

All four distinguishing tests added in e581d9f, exactly per your list: null/undefined rows for the totality guard, Object.getPrototypeOf === Object.prototype for the proto rejection, an array-shaped requests asserting {}, and a deferred sessionSet proving the write chain serialises (A pending -> B queued -> one call -> resolve -> two calls).

[Comp0te]

Two of the four are closed, two aren't. Verified by mutation: deleting the totality guard makes :301-320 fail, and deleting isStorableRequestId from :152 makes the new prototype assertion at :343 fail. The serialisation test and the array test each still leave the mutant they were aimed at alive. Both need one line each — details below.

Basis

Closed. session-store.test.ts:301-306 carries ['a null row', null] / ['an undefined row', undefined], run at :309-320 paired with a surviving good row — delete the guard at session-store.ts:92-94 and row.seq on null throws into the catch at :226-230, which returns emptyRecord(), so {} ≠ { good: … } and the case fails. :343 asserts Object.getPrototypeOf(requests) === Object.prototype on a computed ['__proto__'] key carrying a valid openRow(); drop !isStorableRequestId(requestId) || from :152 and the surviving length check does not backstop it at 8 chars, so :161 hits the inherited setter and :343 fails while :338/:339 still pass.

Still open — write-chain serialisation. :471-496 kills the literal deletion (replace :283 with a synchronous void flush(area) and sessionSet is at 2 by :488). But :488 runs synchronously right after the writeB call at :485 with no intervening tick, so it cannot see a flush that is queued rather than started. Weaken :283 from writeChain = writeChain.then(() => flush(area)) to writeChain = Promise.resolve().then(() => flush(area)) — serialisation gone, one token — and :488, :494-495, :438-446, :448-461 and :463-469 are all green. That mutant is the concurrent-set defect itself. An await Promise.resolve() between :485 and :488 closes it and stays green against the real implementation.

Still open — record-shape guard. :169-182 kills the first mutation (remove Array.isArray(raw) from :144 and Object.entries on the array yields '0'/'1' carrying valid rows). The second is untouched: delete the record-shape guard at :216-218 and all four assertions at :145-156 and :158-167 still hold — 'nope' destructures to two undefineds giving {}/null, null throws into the catch which returns exactly the expected emptyRecord(), and ['a'] gives requests undefined. No test in this commit feeds a non-object or null value under the session key.

[ost-ptk]

Both survivors killed in bd549e5, each mutation-checked in both directions: (1) the serialisation test now has an await Promise.resolve() between writeB and the single-call assertion — green on the real chain, red on the Promise.resolve().then(flush) mutant (2 calls); (2) the record-shape guard is distinguished by its side effect — a null record value now asserts emptyRecord() AND that console.error was never called, which the unguarded throw-into-catch path fails.

[Comp0te]

This literal type-checks only the required members of Request, so a new optional field compiles clean and silently stops crossing a restart. frameId already did it — present at the merge base, omitted by the first sanitizer, tsc green. Fixed here; the gap is not.

Would a Record<keyof OpenRow, true> key list make the next one a compile error?

Basis

readonly frameId?: number is at types.ts:35 in the base tree as well as at head. session-store.test.ts has per-field cases (:194-253, :347-359) but no round-trip-completeness test, so nothing else catches an omission either.

What an absent frameId costs, for the request shapes that reach it: a top-frame request (frameId === 0, which the sanitizer does preserve at :128-131) or a same-origin sub-frame passes the origin check at sdk-response-to-tab.ts:265, then takes the unscoped tabs.sendMessage(tabId, action) arm at :297 — and with all_frames: true (manifest.v3.json:50) that delivers the response to every third-party iframe in the tab. A cross-origin sub-frame does not reach :297: getLiveTabOrigin returns the top document's origin, :265 withholds, and it routes to deliverViaOrigin.

Extract<Request, { status: 'open' }> plus a Record<keyof OpenRow, true> referenced from a satisfies is the idiom the file already uses for the method union; the field list matches types.ts:26-52.

[ost-ptk]

Added in e581d9f: Extract<Request, {status:'open'}> + satisfies Record<keyof OpenRow, true> completeness pin, so the next optional field is a compile error instead of a silently non-mirrored field.

[Comp0te]

The pin fires — Record<keyof OpenRow, true> is non-homomorphic, so optional keys become required. But it's satisfiable without touching the sanitizer: add foo: true at :373 plus 'foo' to the runtime list and the build is green while sanitizeRequest's literal at session-store.ts:131-140 still drops the field. Colocating the pin with that literal (the CANCELLABLE_METHODS idiom at :62) closes it.

Basis

satisfies applies excess-property checking to the fresh literal, so both a missing and an extra field error, and tsconfig.json includes all of src, so it runs under npm run ci-check. The gap is that nothing relates OPEN_ROW_KEYS to the sanitizer's return literal — the "forcing the sanitizer's return literal to be revisited" claim at :363-364 is prose, not mechanism, so the error is satisfiable entirely inside the test file.

Two things alongside it: the runtime it('lists every OpenRow field') compares the record against a hand-copied twin five lines above, so it can only fail on an inconsistent edit of the same file, and it steers the fix further from the sanitizer. And the second open-row construction literal, windowManagement/reducer.ts:76-85, isn't covered either — a field the reducer never sets never reaches the mirror in the first place.

[ost-ptk]

Moved in bd549e5: the pin (OPEN_ROW_KEYS: Record<keyof Extract<Request,{status:'open'}>, true>) now lives in session-store.ts next to sanitizeRequest's literal, per the file's own CANCELLABLE_METHODS idiom — a new Request field is a compile error in THIS file, ten lines from the literal that must handle it (sanity-checked by adding a field: tsc fails at the declaration). The test-file pin and its hand-copied runtime twin are deleted. The reducer's construction literal stays out of scope deliberately: a field the reducer never sets is a concern for that feature's own tests, not the mirror's transport.

[Comp0te]

This invariant is prose only, and the semgrep suite certifies the opposite: cw-storage-local-outside-background lists nine storage.local patterns and none for session, while its own test annotates browser.storage.session.get('a') as an // ok: safe alternative. A setAccessLevel call anywhere passes ci-check and semgrep.

Could the rule gain the three storage.session forms plus a setAccessLevel pattern?

Basis

.semgrep.yml:48 — pattern-either with nine patterns at :49-57, all storage.local.{get,set,remove} in bare/browser./chrome. form; paths.exclude at :58-62. .semgrep/rule-tests/cw-storage-local-outside-background.ts:30-32 — export async function safeAlternatives(), // ok: cw-storage-local-outside-background, await browser.storage.session.get('a'). That annotation was true until this PR and is false after it, so the rule test now pins the old policy.

A grep for setAccessLevel|TRUSTED_AND_UNTRUSTED across src/, .semgrep/ and scripts/ returns only the comment at session-store.ts:37 and the doc lines above. session-store.ts is the only module touching the area, imported outside tests only by get-main-store.ts:101 and :204.

Moving that line into the ruleid: set would pin the new policy rather than the old one.

[ost-ptk]

Fixed in e581d9f: the rule gained all 12 storage.session forms (get/set/remove/setAccessLevel x bare/browser./chrome.), and the rule-test's storage.session.get line moved from ok: to ruleid: with fixture coverage for every new pattern. 14/14 rule tests pass.

[Comp0te]

The certification half is closed. The setAccessLevel half isn't: all four patterns sit under the rule-level paths.exclude at .semgrep.yml:72-74 (src/background/**), which contains session-store.ts — the only file in src/ touching the session area. Worth knowing: semgrep --test ignores paths: entirely (scripts/semgrep-test.mjs:18-19), so 14/14 green doesn't cover it. Needs a second rule id with no paths block.

Basis

Closed half confirmed: .semgrep.yml:60-71 adds all twelve storage.session.{get,set,remove,setAccessLevel} forms across bare / browser. / chrome. receivers, and browser.storage.session.get('a') moved out of safeAlternatives into a new sessionArea() as // ruleid: at .semgrep/rule-tests/cw-storage-local-outside-background.ts:39-40, with all twelve annotated at :30-59. The repo no longer certifies session reads as a safe alternative.

Open half: chrome.storage.session.setAccessLevel({ accessLevel: 'TRUSTED_AND_UNTRUSTED_CONTEXTS' }) added anywhere under src/background/ passes npm run semgrep clean, because the exclusion is rule-level rather than per-pattern. That is the exact call the original comment named, and the consequence survives verbatim — content scripts, i.e. the dapps, would gain read access to every pending request's origin and tab id, with no repo check flagging it.

Why the fixtures don't cover it: your repo's own runner documents that semgrep --test ignores paths: globs, at scripts/semgrep-test.mjs:18-19. So the twelve green fixtures report success on patterns that can never fire under semgrep:ci.

[ost-ptk]

Fixed in bd549e5: the four setAccessLevel patterns moved into a new rule id (cw-storage-session-access-level) with NO paths block, so it fires anywhere in the tree including src/background/ — with its own fixture file. 15/15 rule tests. Also hit and root-caused an undocumented semgrep --test behavior on the way (two rules matching one location collapse the file's rule resolution); left a caveat note for the next overlapping-pattern rule.

[Comp0te]

Re-checked the six published comments against e581d9fd. Three are closed outright and two are half-closed — in both cases the added test kills the literal mutation but not the weaker one the original comment described, and each needs one more line. One new point on the length bound that moved this round.

[Comp0te]

This PR introduces the same length rule twice — request-map.ts:49 and again at :153, where the second is unreachable. The cost is coverage: session-store.test.ts:322-344, the case named for the over-long key, passes with or without the predicate's length half, because :153 catches the row either way. Drop the disjunct and add a case that fails when the predicate's half goes.

Basis

Unreachable, not merely redundant. request-map.ts:48-49 is requestId !== '__proto__' && requestId.length <= MAX_REQUEST_ID_LENGTH, so !isStorableRequestId(s) ≡ s === '__proto__' || s.length > 256 — the disjunct at :153 is implied by the clause on :152 for every input. The loop variable comes from Object.entries(raw) at :150 so it is always a string; .length is the same UTF-16 measure on both sides; boundary 256, '' and '__proto__' all agree.

The coverage gap. :329 feeds 'x'.repeat(257) and :338-339 assert the map reduces to { good: … }. 257 > 256 trips :153 independently, so that assertion holds whether or not request-map.ts:49 carries its length clause. Tree-wide, sdk-methods.test.ts:270-289 is the only test that fails when that clause is deleted — no test file imports isStorableRequestId or MAX_REQUEST_ID_LENGTH, and the only other over-long string in the tree is the origin at session-store.test.ts:288. So one assertion in a different suite is the whole guard on a predicate with five callers.

What the predicate is holding. Four of those five have no local length backstop — sdk-methods.ts:92, windowManagement/reducer.ts:65, vault/reducer.ts:112, vault/reducer.ts:233; session-store.ts:152 is the only one that does. With the length clause gone nothing else bounds the id (sdk-method.ts:135 checks only typeof === 'string', and requestId.length appears nowhere else in the tree), which is the quota-wipe path from the thread above.

One caveat on the fix. Dropping the disjunct and the :8 import leaves MAX_REQUEST_ID_LENGTH referenced only inside request-map.ts, and knip.json excludes only enumMembers, so knip is live in npm run ci-check — drop the export keyword too. The file-local idiom is already there in MAX_ORIGIN_LENGTH at session-store.ts:29. I could not run knip against this tree, so treat that as a flag rather than a confirmed failure.

[ost-ptk]

Accepted in full in bd549e5 — you're right that the disjunct was implied, not defense: dropped it and the import, un-exported the constant (knip clean), and the existing over-long-key sanitizer test now guards the predicate itself (mutation-checked: deleting the length clause from isStorableRequestId turns it red).