fix(sidebar): expose migrated legacy prefs only once the legacy key is gone

Thufir's r6 review found the legacy→scoped migration exposed legacy data
to bootstrap even when the scoped write threw or the legacy delete did not
take, wrapped in a swallowed try/catch. On any storage failure the legacy
key survived while the relay still received the legacy preferences and
could seed-publish them; a second relay later read the same legacy key and
seeded it too — so "globally one-time" held only on the happy path.

migrateLegacyStore now exposes migrated data only when ownership is proven:
write the scoped key, delete the legacy key, and confirm the delete took.
On a scoped-write throw, a legacy-delete throw, or a delete that silently
does not take, roll back the partial scoped copy and return DEFAULT_STORE,
leaving the migration retryable. An empty store cannot seed-publish
(bootstrap's non-empty-local gate), so DEFAULT is safe.

The identical flaw existed in all three lanes with a legacy primary-cache
migration — sections (pre-existing on main), stars, and mutes. Sort has no
such migration and is untouched. Concurrent claimants are safe: every
window resolves the same app-wide active community, so at any instant they
scope to the same relay and any race migrates the identical value to the
identical key; a second relay only enters play after a community switch,
which is strictly after the first scoped read consumed the legacy key.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs

@@ -640,3 +640,62 @@ test("readChannelMutesStore: scoped key takes precedence over legacy key", () =>
   writeChannelMutesStore(pubkey, scoped, relay);
   assert.deepEqual(readChannelMutesStore(pubkey, relay), scoped);
 });
+
+// Migration failure safety (Thufir r6 IMPORTANT): legacy data must be exposed
+// to bootstrap ONLY once the legacy key is provably gone. If the scoped write
+// throws, or the legacy delete does not take, the reader must roll back and
+// return DEFAULT_STORE so no relay publishes legacy prefs while the legacy key
+// still holds them (which would let a second relay import the same value).
+
+test("readChannelMutesStore: scoped-write failure returns DEFAULT and leaves neither relay able to seed the legacy value", () => {
+  const pubkey = "pk-mute-migrate-writefail";
+  const relayA = "wss://relay-a-writefail.example.com";
+  const relayB = "wss://relay-b-writefail.example.com";
+  const legacy = makeMuteStore({ chw: MUTE_E(true, 500, 2) });
+  writeChannelMutesStore(pubkey, legacy);
+  const scopedA = storageKey(pubkey, relayA);
+  const origSet = window.localStorage.setItem;
+  window.localStorage.setItem = (k, v) => {
+    if (k === scopedA) throw new Error("QuotaExceededError");
+    return origSet.call(window.localStorage, k, v);
+  };
+  try {
+    // Relay A cannot claim the legacy value — it must NOT be exposed to publish.
+    assert.deepEqual(readChannelMutesStore(pubkey, relayA), DEFAULT_STORE);
+    // Rolled back: no partial scoped copy left behind for A.
+    assert.equal(window.localStorage.getItem(scopedA), null);
+  } finally {
+    window.localStorage.setItem = origSet;
+  }
+  // Legacy key survived (migration is retryable), and relay B likewise cannot
+  // be seeded — the value is claimed by whichever relay first deletes it.
+  assert.notEqual(window.localStorage.getItem(storageKey(pubkey)), null);
+  assert.deepEqual(readChannelMutesStore(pubkey, relayB), legacy);
+  // Once B claims it, the legacy key is gone and A can no longer seed.
+  assert.equal(window.localStorage.getItem(storageKey(pubkey)), null);
+  assert.deepEqual(readChannelMutesStore(pubkey, relayA), DEFAULT_STORE);
+});
+
+test("readChannelMutesStore: a legacy delete that does not take rolls back and returns DEFAULT", () => {
+  const pubkey = "pk-mute-migrate-delfail";
+  const relay = "wss://relay-delfail.example.com";
+  const legacy = makeMuteStore({ chd: MUTE_E(true, 700, 3) });
+  writeChannelMutesStore(pubkey, legacy);
+  const legacyKey = storageKey(pubkey);
+  const scoped = storageKey(pubkey, relay);
+  const origRemove = window.localStorage.removeItem;
+  window.localStorage.removeItem = (k) => {
+    if (k === legacyKey) return; // silently no-op: delete "does not take"
+    return origRemove.call(window.localStorage, k);
+  };
+  try {
+    assert.deepEqual(readChannelMutesStore(pubkey, relay), DEFAULT_STORE);
+    // Partial scoped copy rolled back so the migration stays retryable.
+    assert.equal(window.localStorage.getItem(scoped), null);
+  } finally {
+    window.localStorage.removeItem = origRemove;
+  }
+  // With the delete working again, the migration completes cleanly.
+  assert.deepEqual(readChannelMutesStore(pubkey, relay), legacy);
+  assert.equal(window.localStorage.getItem(legacyKey), null);
+});

desktop/src/features/sidebar/lib/channelMutesStorage.ts

@@ -118,11 +118,21 @@ function parseRaw(raw: string | null): ChannelMuteStore | null {
  * Read the mute store for `pubkey` scoped to `relayUrl`.
  *
  * On first access for a scoped key, migrates any existing data from the legacy
- * pubkey-only key so users don't lose their mutes on upgrade. After a
- * successful migration write the legacy key is deleted, making this a globally
- * one-time operation — a subsequent empty scoped-key read on a DIFFERENT relay
- * won't see legacy data, so a non-empty store from relay A can never seed onto
- * a first-visited relay B (Carl P1).
+ * pubkey-only key so users don't lose their mutes on upgrade. Migrated data is
+ * exposed to the caller (and thus to publish) ONLY once the legacy key is
+ * provably gone: we write the scoped key, delete the legacy key, and confirm
+ * the delete took. On any write/delete failure we roll back the partial scoped
+ * copy and return `DEFAULT_STORE`, leaving the migration safely retryable — so
+ * legacy preferences can never be exposed while still importable by a second
+ * relay, which would let relay A's mutes seed-publish onto relay B (Carl P1).
+ *
+ * This makes the migration first-scope-only, not merely "usually deletes
+ * afterward." Concurrent claimants are safe: every window (main and huddle)
+ * resolves the same app-wide active community (`buzz-active-community-id`), so
+ * at any instant they scope to the same relay and any race migrates the
+ * identical legacy value to the identical scoped key (idempotent). A second
+ * relay only enters play after a community switch, which happens strictly after
+ * the first scoped read consumed the legacy key.
  */
 export function readChannelMutesStore(
   pubkey: string,
@@ -143,15 +153,7 @@ export function readChannelMutesStore(
       const legacyKey = storageKey(pubkey);
       const migrated = parseRaw(window.localStorage.getItem(legacyKey));
       if (migrated && Object.keys(migrated.channels).length > 0) {
-        // Persist under the scoped key and remove the legacy key so this
-        // migration cannot fire again for any other relay scope.
-        try {
-          window.localStorage.setItem(key, JSON.stringify(migrated));
-          window.localStorage.removeItem(legacyKey);
-        } catch {
-          // Ignore write failures — we still return the migrated value.
-        }
-        return migrated;
+        return migrateLegacyStore(key, legacyKey, migrated);
       }
     }
 
@@ -161,6 +163,38 @@ export function readChannelMutesStore(
   }
 }
 
+/**
+ * Claim the legacy value for the scoped key: persist it, delete the legacy key,
+ * and expose it only if the legacy key is confirmed gone. Any failure rolls
+ * back the partial scoped copy and returns `DEFAULT_STORE` so the migration is
+ * retryable and no relay imports legacy data the legacy key still holds.
+ */
+function migrateLegacyStore(
+  key: string,
+  legacyKey: string,
+  migrated: ChannelMuteStore,
+): ChannelMuteStore {
+  try {
+    window.localStorage.setItem(key, JSON.stringify(migrated));
+    window.localStorage.removeItem(legacyKey);
+    if (window.localStorage.getItem(legacyKey) !== null) {
+      // Delete did not take — do not expose data another relay could import.
+      window.localStorage.removeItem(key);
+      return DEFAULT_STORE;
+    }
+    return migrated;
+  } catch {
+    // Scoped write or legacy delete threw — roll back the partial scoped copy
+    // so the scoped key stays empty and the migration can retry later.
+    try {
+      window.localStorage.removeItem(key);
+    } catch {
+      // Best effort; a stuck partial copy self-heals on the next read.
+    }
+    return DEFAULT_STORE;
+  }
+}
+
 export function boundMuteStore(
   store: ChannelMuteStore,
   preservedKey?: string,

desktop/src/features/sidebar/lib/channelSectionsStorage.test.mjs

@@ -357,6 +357,71 @@ test("readChannelSectionsStore: scoped key takes precedence over legacy key afte
   assert.deepEqual(result, newStore);
 });
 
+// Migration failure safety (Thufir r6 IMPORTANT): legacy data must be exposed
+// to bootstrap ONLY once the legacy key is provably gone. If the scoped write
+// throws, or the legacy delete does not take, the reader must roll back and
+// return DEFAULT_STORE so no relay publishes legacy prefs while the legacy key
+// still holds them (which would let a second relay import the same value).
+
+test("readChannelSectionsStore: scoped-write failure returns DEFAULT and leaves neither relay able to seed the legacy value", () => {
+  const pubkey = "pk-migrate-writefail";
+  const relayA = "wss://relay-a-writefail.example.com";
+  const relayB = "wss://relay-b-writefail.example.com";
+  const legacy = makeStore({
+    sections: [makeSection({ id: "sw", name: "Legacy", order: 0 })],
+    assignments: {},
+  });
+  writeChannelSectionsStore(pubkey, legacy);
+  const scopedA = storageKey(pubkey, relayA);
+  const origSet = window.localStorage.setItem;
+  window.localStorage.setItem = (k, v) => {
+    if (k === scopedA) throw new Error("QuotaExceededError");
+    return origSet.call(window.localStorage, k, v);
+  };
+  try {
+    // Relay A cannot claim the legacy value — it must NOT be exposed to publish.
+    assert.deepEqual(readChannelSectionsStore(pubkey, relayA), DEFAULT_STORE);
+    // Rolled back: no partial scoped copy left behind for A.
+    assert.equal(window.localStorage.getItem(scopedA), null);
+  } finally {
+    window.localStorage.setItem = origSet;
+  }
+  // Legacy key survived (migration is retryable), and relay B likewise cannot
+  // be seeded — the value is claimed by whichever relay first deletes it.
+  assert.notEqual(window.localStorage.getItem(storageKey(pubkey)), null);
+  assert.deepEqual(readChannelSectionsStore(pubkey, relayB), legacy);
+  // Once B claims it, the legacy key is gone and A can no longer seed.
+  assert.equal(window.localStorage.getItem(storageKey(pubkey)), null);
+  assert.deepEqual(readChannelSectionsStore(pubkey, relayA), DEFAULT_STORE);
+});
+
+test("readChannelSectionsStore: a legacy delete that does not take rolls back and returns DEFAULT", () => {
+  const pubkey = "pk-migrate-delfail";
+  const relay = "wss://relay-delfail.example.com";
+  const legacy = makeStore({
+    sections: [makeSection({ id: "sd", name: "Legacy", order: 0 })],
+    assignments: {},
+  });
+  writeChannelSectionsStore(pubkey, legacy);
+  const legacyKey = storageKey(pubkey);
+  const scoped = storageKey(pubkey, relay);
+  const origRemove = window.localStorage.removeItem;
+  window.localStorage.removeItem = (k) => {
+    if (k === legacyKey) return; // silently no-op: delete "does not take"
+    return origRemove.call(window.localStorage, k);
+  };
+  try {
+    assert.deepEqual(readChannelSectionsStore(pubkey, relay), DEFAULT_STORE);
+    // Partial scoped copy rolled back so the migration stays retryable.
+    assert.equal(window.localStorage.getItem(scoped), null);
+  } finally {
+    window.localStorage.removeItem = origRemove;
+  }
+  // With the delete working again, the migration completes cleanly.
+  assert.deepEqual(readChannelSectionsStore(pubkey, relay), legacy);
+  assert.equal(window.localStorage.getItem(legacyKey), null);
+});
+
 test("parseChannelSectionPayload: preserves icon field when present", () => {
   const payload = {
     version: 1,

desktop/src/features/sidebar/lib/channelSectionsStorage.ts

@@ -141,12 +141,22 @@ function parseRaw(raw: string | null): ChannelSectionStore | null {
 /**
  * Read the section store for `pubkey` scoped to `relayUrl`.
  *
- * On first access for a scoped key, migrates any existing data from the
- * legacy pubkey-only key so users don't lose their sections on upgrade.
- * After a successful migration write the legacy key is deleted, making
- * this a globally one-time operation — subsequent empty scoped-key reads
- * (i.e. a different relay) won't see legacy data and won't trigger
- * cross-relay contamination via the seed-publish path.
+ * On first access for a scoped key, migrates any existing data from the legacy
+ * pubkey-only key so users don't lose their sections on upgrade. Migrated data
+ * is exposed to the caller (and thus to publish) ONLY once the legacy key is
+ * provably gone: we write the scoped key, delete the legacy key, and confirm
+ * the delete took. On any write/delete failure we roll back the partial scoped
+ * copy and return `DEFAULT_STORE`, leaving the migration safely retryable — so
+ * legacy sections can never be exposed while still importable by a second
+ * relay, which would let relay A's sections seed-publish onto relay B (Carl P1).
+ *
+ * This makes the migration first-scope-only, not merely "usually deletes
+ * afterward." Concurrent claimants are safe: every window (main and huddle)
+ * resolves the same app-wide active community (`buzz-active-community-id`), so
+ * at any instant they scope to the same relay and any race migrates the
+ * identical legacy value to the identical scoped key (idempotent). A second
+ * relay only enters play after a community switch, which happens strictly after
+ * the first scoped read consumed the legacy key.
  */
 export function readChannelSectionsStore(
   pubkey: string,
@@ -168,15 +178,7 @@ export function readChannelSectionsStore(
       const legacyRaw = window.localStorage.getItem(legacyKey);
       const migrated = parseRaw(legacyRaw);
       if (migrated && migrated.sections.length > 0) {
-        // Persist under the scoped key and remove the legacy key so this
-        // migration cannot fire again for any other relay scope.
-        try {
-          window.localStorage.setItem(key, JSON.stringify(migrated));
-          window.localStorage.removeItem(legacyKey);
-        } catch {
-          // Ignore write failures — we still return the migrated value.
-        }
-        return migrated;
+        return migrateLegacyStore(key, legacyKey, migrated);
       }
     }
 
@@ -186,6 +188,38 @@ export function readChannelSectionsStore(
   }
 }
 
+/**
+ * Claim the legacy value for the scoped key: persist it, delete the legacy key,
+ * and expose it only if the legacy key is confirmed gone. Any failure rolls
+ * back the partial scoped copy and returns `DEFAULT_STORE` so the migration is
+ * retryable and no relay imports legacy data the legacy key still holds.
+ */
+function migrateLegacyStore(
+  key: string,
+  legacyKey: string,
+  migrated: ChannelSectionStore,
+): ChannelSectionStore {
+  try {
+    window.localStorage.setItem(key, JSON.stringify(migrated));
+    window.localStorage.removeItem(legacyKey);
+    if (window.localStorage.getItem(legacyKey) !== null) {
+      // Delete did not take — do not expose data another relay could import.
+      window.localStorage.removeItem(key);
+      return DEFAULT_STORE;
+    }
+    return migrated;
+  } catch {
+    // Scoped write or legacy delete threw — roll back the partial scoped copy
+    // so the scoped key stays empty and the migration can retry later.
+    try {
+      window.localStorage.removeItem(key);
+    } catch {
+      // Best effort; a stuck partial copy self-heals on the next read.
+    }
+    return DEFAULT_STORE;
+  }
+}
+
 export function writeChannelSectionsStore(
   pubkey: string,
   store: ChannelSectionStore,

desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs

@@ -643,3 +643,62 @@ test("readChannelStarsStore: scoped key takes precedence over legacy key", () =>
   writeChannelStarsStore(pubkey, scoped, relay);
   assert.deepEqual(readChannelStarsStore(pubkey, relay), scoped);
 });
+
+// Migration failure safety (Thufir r6 IMPORTANT): legacy data must be exposed
+// to bootstrap ONLY once the legacy key is provably gone. If the scoped write
+// throws, or the legacy delete does not take, the reader must roll back and
+// return DEFAULT_STORE so no relay publishes legacy prefs while the legacy key
+// still holds them (which would let a second relay import the same value).
+
+test("readChannelStarsStore: scoped-write failure returns DEFAULT and leaves neither relay able to seed the legacy value", () => {
+  const pubkey = "pk-star-migrate-writefail";
+  const relayA = "wss://relay-a-writefail.example.com";
+  const relayB = "wss://relay-b-writefail.example.com";
+  const legacy = makeStarStore({ chw: STAR_E(true, 500, 2) });
+  writeChannelStarsStore(pubkey, legacy);
+  const scopedA = storageKey(pubkey, relayA);
+  const origSet = window.localStorage.setItem;
+  window.localStorage.setItem = (k, v) => {
+    if (k === scopedA) throw new Error("QuotaExceededError");
+    return origSet.call(window.localStorage, k, v);
+  };
+  try {
+    // Relay A cannot claim the legacy value — it must NOT be exposed to publish.
+    assert.deepEqual(readChannelStarsStore(pubkey, relayA), DEFAULT_STORE);
+    // Rolled back: no partial scoped copy left behind for A.
+    assert.equal(window.localStorage.getItem(scopedA), null);
+  } finally {
+    window.localStorage.setItem = origSet;
+  }
+  // Legacy key survived (migration is retryable), and relay B likewise cannot
+  // be seeded — the value is claimed by whichever relay first deletes it.
+  assert.notEqual(window.localStorage.getItem(storageKey(pubkey)), null);
+  assert.deepEqual(readChannelStarsStore(pubkey, relayB), legacy);
+  // Once B claims it, the legacy key is gone and A can no longer seed.
+  assert.equal(window.localStorage.getItem(storageKey(pubkey)), null);
+  assert.deepEqual(readChannelStarsStore(pubkey, relayA), DEFAULT_STORE);
+});
+
+test("readChannelStarsStore: a legacy delete that does not take rolls back and returns DEFAULT", () => {
+  const pubkey = "pk-star-migrate-delfail";
+  const relay = "wss://relay-delfail.example.com";
+  const legacy = makeStarStore({ chd: STAR_E(true, 700, 3) });
+  writeChannelStarsStore(pubkey, legacy);
+  const legacyKey = storageKey(pubkey);
+  const scoped = storageKey(pubkey, relay);
+  const origRemove = window.localStorage.removeItem;
+  window.localStorage.removeItem = (k) => {
+    if (k === legacyKey) return; // silently no-op: delete "does not take"
+    return origRemove.call(window.localStorage, k);
+  };
+  try {
+    assert.deepEqual(readChannelStarsStore(pubkey, relay), DEFAULT_STORE);
+    // Partial scoped copy rolled back so the migration stays retryable.
+    assert.equal(window.localStorage.getItem(scoped), null);
+  } finally {
+    window.localStorage.removeItem = origRemove;
+  }
+  // With the delete working again, the migration completes cleanly.
+  assert.deepEqual(readChannelStarsStore(pubkey, relay), legacy);
+  assert.equal(window.localStorage.getItem(legacyKey), null);
+});

desktop/src/features/sidebar/lib/channelStarsStorage.ts

@@ -118,11 +118,21 @@ function parseRaw(raw: string | null): ChannelStarStore | null {
  * Read the star store for `pubkey` scoped to `relayUrl`.
  *
  * On first access for a scoped key, migrates any existing data from the legacy
- * pubkey-only key so users don't lose their stars on upgrade. After a
- * successful migration write the legacy key is deleted, making this a globally
- * one-time operation — a subsequent empty scoped-key read on a DIFFERENT relay
- * won't see legacy data, so a non-empty store from relay A can never seed onto
- * a first-visited relay B (Carl P1).
+ * pubkey-only key so users don't lose their stars on upgrade. Migrated data is
+ * exposed to the caller (and thus to publish) ONLY once the legacy key is
+ * provably gone: we write the scoped key, delete the legacy key, and confirm
+ * the delete took. On any write/delete failure we roll back the partial scoped
+ * copy and return `DEFAULT_STORE`, leaving the migration safely retryable — so
+ * legacy preferences can never be exposed while still importable by a second
+ * relay, which would let relay A's stars seed-publish onto relay B (Carl P1).
+ *
+ * This makes the migration first-scope-only, not merely "usually deletes
+ * afterward." Concurrent claimants are safe: every window (main and huddle)
+ * resolves the same app-wide active community (`buzz-active-community-id`), so
+ * at any instant they scope to the same relay and any race migrates the
+ * identical legacy value to the identical scoped key (idempotent). A second
+ * relay only enters play after a community switch, which happens strictly after
+ * the first scoped read consumed the legacy key.
  */
 export function readChannelStarsStore(
   pubkey: string,
@@ -143,15 +153,7 @@ export function readChannelStarsStore(
       const legacyKey = storageKey(pubkey);
       const migrated = parseRaw(window.localStorage.getItem(legacyKey));
       if (migrated && Object.keys(migrated.channels).length > 0) {
-        // Persist under the scoped key and remove the legacy key so this
-        // migration cannot fire again for any other relay scope.
-        try {
-          window.localStorage.setItem(key, JSON.stringify(migrated));
-          window.localStorage.removeItem(legacyKey);
-        } catch {
-          // Ignore write failures — we still return the migrated value.
-        }
-        return migrated;
+        return migrateLegacyStore(key, legacyKey, migrated);
       }
     }
 
@@ -161,6 +163,38 @@ export function readChannelStarsStore(
   }
 }
 
+/**
+ * Claim the legacy value for the scoped key: persist it, delete the legacy key,
+ * and expose it only if the legacy key is confirmed gone. Any failure rolls
+ * back the partial scoped copy and returns `DEFAULT_STORE` so the migration is
+ * retryable and no relay imports legacy data the legacy key still holds.
+ */
+function migrateLegacyStore(
+  key: string,
+  legacyKey: string,
+  migrated: ChannelStarStore,
+): ChannelStarStore {
+  try {
+    window.localStorage.setItem(key, JSON.stringify(migrated));
+    window.localStorage.removeItem(legacyKey);
+    if (window.localStorage.getItem(legacyKey) !== null) {
+      // Delete did not take — do not expose data another relay could import.
+      window.localStorage.removeItem(key);
+      return DEFAULT_STORE;
+    }
+    return migrated;
+  } catch {
+    // Scoped write or legacy delete threw — roll back the partial scoped copy
+    // so the scoped key stays empty and the migration can retry later.
+    try {
+      window.localStorage.removeItem(key);
+    } catch {
+      // Best effort; a stuck partial copy self-heals on the next read.
+    }
+    return DEFAULT_STORE;
+  }
+}
+
 export function boundStarStore(
   store: ChannelStarStore,
   preservedKey?: string,