fix(icons): serialize icon-cache writes, and stop a test stub outliving its test

Two more from PR review.

The whole icon cache lives under one storage key, and every update reads it,
changes an entry, and writes it back. Resolving icons concurrently means those
updates overlap, and storage hands each handler its own deserialized copy — so
two handlers read the same map, add different assets, and the later write
discards the earlier one's icon, or undoes a clear. Updates are now chained so
each works from the result of the last.

The regression test needed care to be honest: a fake store that hands back the
same object reference, or one that merely staggers reads with timers, both hide
the race. It holds every read open until all handlers have read, and returns a
copy per read, the way browser.storage does.

The Account icon test stubbed the probe and never restored it. The suite's
afterEach calls clearAllMocks, which drops call history but keeps the
implementation, so the stub stood in for the real thing in every test after it
and made the file order-dependent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: piyalbasu

extension/src/background/messageListener/handlers/__tests__/cacheAssetIcon.test.ts

@@ -65,3 +65,99 @@ describe("cacheAssetIcon", () => {
     });
   });
 });
+
+describe("cacheAssetIcon concurrency", () => {
+  /**
+   * localStore whose reads are held open until released, so every handler is
+   * guaranteed to have read before any of them writes — the interleaving that
+   * concurrent icon resolution produces, made deterministic.
+   *
+   * Reads hand back a copy, as browser.storage.local does: one handler's
+   * mutation is invisible to another that already read.
+   */
+  const makeGatedLocalStore = (
+    assetIconCache: Record<string, unknown> = {},
+  ) => {
+    const store: Record<string, unknown> = {
+      [CACHED_ASSET_ICONS_ID]: assetIconCache,
+    };
+    let release!: () => void;
+    const gate = new Promise<void>((resolve) => {
+      release = resolve;
+    });
+
+    return {
+      localStore: {
+        getItem: jest.fn(async (key: string) => {
+          await gate;
+          return store[key] === undefined
+            ? undefined
+            : JSON.parse(JSON.stringify(store[key]));
+        }),
+        setItem: jest.fn(async (key: string, value: unknown) => {
+          store[key] = value;
+        }),
+        remove: jest.fn(),
+        clear: jest.fn(),
+      } as any,
+      releaseReads: () => release(),
+      read: () => store,
+    };
+  };
+
+  it("keeps every icon when writes overlap", async () => {
+    // getAssetIcons resolves icons concurrently, so several CACHE_ASSET_ICON
+    // messages are in flight at once. A read-modify-write of one shared object
+    // loses all but the last unless the writes are serialized.
+    const { localStore, releaseReads, read } = makeGatedLocalStore();
+
+    const writes = Promise.all(
+      ["A:G1", "B:G2", "C:G3"].map((assetCanonical) =>
+        cacheAssetIcon({
+          request: {
+            assetCanonical,
+            iconUrl: `https://example.com/${assetCanonical}.png`,
+          } as CacheAssetIconMessage,
+          localStore,
+        }),
+      ),
+    );
+    releaseReads();
+    await writes;
+
+    expect(read()[CACHED_ASSET_ICONS_ID]).toEqual({
+      "A:G1": "https://example.com/A:G1.png",
+      "B:G2": "https://example.com/B:G2.png",
+      "C:G3": "https://example.com/C:G3.png",
+    });
+  });
+
+  it("does not let an overlapping write resurrect a cleared entry", async () => {
+    const { localStore, releaseReads, read } = makeGatedLocalStore({
+      [CANONICAL]: ICON_URL,
+    });
+
+    const writes = Promise.all([
+      cacheAssetIcon({
+        request: {
+          assetCanonical: CANONICAL,
+          iconUrl: null,
+        } as CacheAssetIconMessage,
+        localStore,
+      }),
+      cacheAssetIcon({
+        request: {
+          assetCanonical: OTHER_CANONICAL,
+          iconUrl: OTHER_ICON_URL,
+        } as CacheAssetIconMessage,
+        localStore,
+      }),
+    ]);
+    releaseReads();
+    await writes;
+
+    expect(read()[CACHED_ASSET_ICONS_ID]).toEqual({
+      [OTHER_CANONICAL]: OTHER_ICON_URL,
+    });
+  });
+});

extension/src/background/messageListener/handlers/cacheAssetIcon.ts

@@ -2,6 +2,22 @@ import { CacheAssetIconMessage } from "@shared/api/types/message-request";
 import { DataStorageAccess } from "background/helpers/dataStorageAccess";
 import { CACHED_ASSET_ICONS_ID } from "constants/localStorageTypes";
 
+/**
+ * Serializes the icon cache's read-modify-write.
+ *
+ * The whole cache lives under one storage key, and every update reads it,
+ * changes one entry, and writes it back. Icon resolution runs several lookups
+ * concurrently, so these messages arrive overlapping — and storage hands each
+ * handler its own deserialized copy, so without a queue two handlers read the
+ * same map, add different assets, and the later write discards the earlier
+ * one's icon (or undoes a clear). Chaining keeps each update working from the
+ * result of the last.
+ *
+ * A failed update still lets the queue continue: the rejection is passed to
+ * whoever asked for that write, not to the writes behind it.
+ */
+let pendingCacheWrite: Promise<void> = Promise.resolve();
+
 export const cacheAssetIcon = async ({
   request,
   localStore,
@@ -11,19 +27,25 @@ export const cacheAssetIcon = async ({
 }) => {
   const { assetCanonical, iconUrl } = request;
 
-  const assetIconCache =
-    (await localStore.getItem(CACHED_ASSET_ICONS_ID)) || {};
+  const applyUpdate = async () => {
+    const assetIconCache =
+      (await localStore.getItem(CACHED_ASSET_ICONS_ID)) || {};
+
+    if (iconUrl) {
+      assetIconCache[assetCanonical] = iconUrl;
+    } else {
+      // A falsy iconUrl means "forget what we cached" (retryAssetIcon sends
+      // null to drop a url that failed to load). Delete the entry rather than
+      // storing the null: getAssetIcons treats a cached null as "already
+      // tried, never look again", and this cache is persisted to disk — so a
+      // single transient image failure would blacklist the asset's icon for
+      // good.
+      delete assetIconCache[assetCanonical];
+    }
 
-  if (iconUrl) {
-    assetIconCache[assetCanonical] = iconUrl;
-  } else {
-    // A falsy iconUrl means "forget what we cached" (retryAssetIcon sends null
-    // to drop a url that failed to load). Delete the entry rather than storing
-    // the null: getAssetIcons treats a cached null as "already tried, never
-    // look again", and this cache is persisted to disk — so a single transient
-    // image failure would blacklist the asset's icon for good.
-    delete assetIconCache[assetCanonical];
-  }
+    await localStore.setItem(CACHED_ASSET_ICONS_ID, assetIconCache);
+  };
 
-  await localStore.setItem(CACHED_ASSET_ICONS_ID, assetIconCache);
+  pendingCacheWrite = pendingCacheWrite.then(applyUpdate, applyUpdate);
+  return pendingCacheWrite;
 };

extension/src/popup/views/__tests__/Account.test.tsx

@@ -285,10 +285,18 @@ jest.mock("popup/ducks/accountServices", () => {
 });
 
 describe("Account view", () => {
+  // Only the icon test stubs the probe. clearAllMocks wipes call history but
+  // keeps the implementation, so without an explicit restore that stub would
+  // outlive its test and quietly stand in for the real thing in every case
+  // after it, making the suite order-dependent.
+  let probeSpy: jest.SpyInstance | undefined;
+
   afterAll(() => {
     jest.clearAllMocks();
   });
   afterEach(() => {
+    probeSpy?.mockRestore();
+    probeSpy = undefined;
     jest.useRealTimers();
     jest.clearAllMocks();
   });
@@ -504,7 +512,7 @@ describe("Account view", () => {
     // jsdom images never fire load or error, so stand in for the browser and
     // say the first candidate renders. This test is about which source an icon
     // comes from; iconProbe.test.ts covers the loading itself.
-    jest
+    probeSpy = jest
       .spyOn(IconProbe, "firstLoadableIconUrl")
       .mockImplementation(async (urls: string[]) => urls[0]);
     const getIconUrlFromIssuerSpy = jest