fix(sidebar): retain over failed/unreadable heads and bound rev parse

Extends the retain-on-unreadable pattern to the two remaining overwrite
paths and hardens the rev/timestamp parse boundary.

- All four lanes tri-state the pre-publish read: a THROWN fetch (timeout /
  auth / socket) now retains and retries instead of falling through to
  publish, so an edit during a transient outage can no longer sign above a
  stale watermark and erase an unseen newer head.
- The merge lanes (stars/mutes) retain when the existing head fails
  decryption/JSON/schema parsing rather than publishing the local store
  over it — a max-merge is only safe once both operands were read, matching
  the whole-blob lanes.
- The rev/timestamp parsers require Number.isSafeInteger: an unsafe rev is
  normalized to 0 and an unsafe timestamp entry is dropped, so a malformed
  huge-rev true entry can no longer wedge later false toggles forever.

Retain reuses the existing bounded-backoff retry and generation CAS, so a
later readable/successful head resumes normal resolution. Regressions cover
all four lanes for the failed-fetch and unreadable-head paths and the
rev-wedge P2 case in both storage suites.

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

@@ -34,7 +34,7 @@ test("parseMutePayload: missing rev normalizes to 0 (old-build blob, entry kept)
   });
 });
 
-test("parseMutePayload: malformed rev (string / negative / non-integer / NaN) normalizes to 0", () => {
+test("parseMutePayload: malformed rev (string / negative / non-integer / NaN / unsafe) normalizes to 0", () => {
   const result = parseMutePayload({
     version: 1,
     channels: {
@@ -42,9 +42,13 @@ test("parseMutePayload: malformed rev (string / negative / non-integer / NaN) no
       neg: { muted: true, updatedAt: 1, rev: -2 },
       frac: { muted: true, updatedAt: 1, rev: 1.5 },
       nan: { muted: true, updatedAt: 1, rev: NaN },
+      // Above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past it, so a
+      // malformed entry at this magnitude would wedge later toggles forever
+      // unless rejected here (Carl P2).
+      huge: { muted: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER + 1 },
     },
   });
-  for (const id of ["str", "neg", "frac", "nan"]) {
+  for (const id of ["str", "neg", "frac", "nan", "huge"]) {
     assert.equal(result.channels[id].rev, 0, `${id} rev normalized to 0`);
     assert.equal(result.channels[id].muted, true, `${id} entry kept`);
   }
@@ -94,7 +98,7 @@ test("parseMutePayload: malformed channel entries missing muted/updatedAt are fi
   });
 });
 
-test("parseMutePayload: NaN/Infinity/negative updatedAt entries are filtered out", () => {
+test("parseMutePayload: NaN/Infinity/negative/unsafe updatedAt entries are filtered out", () => {
   const result = parseMutePayload({
     version: 1,
     channels: {
@@ -102,6 +106,10 @@ test("parseMutePayload: NaN/Infinity/negative updatedAt entries are filtered out
       inf: { muted: true, updatedAt: Infinity },
       "neg-inf": { muted: true, updatedAt: -Infinity },
       neg: { muted: true, updatedAt: -1 },
+      // Beyond Number.MAX_SAFE_INTEGER: an unrepresentable second is not a
+      // trustworthy watermark, so the entry is dropped rather than kept (Carl
+      // P2 — same bound as `rev`).
+      unsafe: { muted: true, updatedAt: Number.MAX_SAFE_INTEGER + 1 },
       valid: { muted: true, updatedAt: 100, rev: 2 },
     },
   });
@@ -181,6 +189,26 @@ test("mergeStores: same-second old-build click (rev 0) loses to an earlier new-b
   );
 });
 
+test("mergeStores: a huge (unsafe) rev entry cannot wedge a later false toggle", () => {
+  // Carl P2 regression. A malformed blob carries a same-second rev far above
+  // Number.MAX_SAFE_INTEGER on a `muted:true` entry. Before the parse bound,
+  // this survived as-is and — since a real later click mints rev = maxSeen + 1,
+  // which cannot advance past an unsafe integer — the true entry won the
+  // same-second tie forever, suppressing every later unmute. The parser now
+  // normalizes the unsafe rev to 0, so a same-second later click (rev 1) wins.
+  const wedged = parseMutePayload({
+    version: 1,
+    channels: { c: { muted: true, updatedAt: 100, rev: Number.MAX_VALUE } },
+  });
+  assert.equal(wedged.channels.c.rev, 0, "unsafe rev is normalized to 0");
+  const laterUnmute = S(E(false, 100, 1));
+  assert.deepEqual(
+    mergeStores(wedged, laterUnmute).channels.c,
+    E(false, 100, 1),
+    "the same-second later unmute (rev 1) wins — no permanent wedge",
+  );
+});
+
 test("mergeStores: unmute with higher updatedAt overrides mute", () => {
   const result = mergeStores(S(E(true, 100, 9)), S(E(false, 999, 1)));
   assert.deepEqual(result.channels.c, E(false, 999, 1));

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

@@ -50,20 +50,24 @@ export function parseMutePayload(json: unknown): ChannelMuteStore | null {
                 v !== null &&
                 typeof (v as Record<string, unknown>).muted === "boolean" &&
                 typeof (v as Record<string, unknown>).updatedAt === "number" &&
-                Number.isFinite(
+                Number.isSafeInteger(
                   (v as Record<string, unknown>).updatedAt as number,
                 ) &&
                 ((v as Record<string, unknown>).updatedAt as number) >= 0
               );
             })
-            // Normalize `rev`: accept a non-negative integer, otherwise 0. An
-            // entry is never dropped solely because `rev` is absent (older
-            // build) or malformed — absence is a valid mergeable value.
+            // Normalize `rev`: accept a safe non-negative integer, otherwise 0.
+            // An entry is never dropped solely because `rev` is absent (older
+            // build) or malformed — absence is a valid mergeable value. `rev`
+            // above `Number.MAX_SAFE_INTEGER` is rejected here: at that
+            // magnitude `maxRev + 1` no longer advances, so a malformed entry
+            // could wedge every later toggle. Treating it as rev 0 lets a real
+            // later click win the same-second tie instead (Carl P2).
             .map(([id, v]) => {
               const rawRev = v.rev;
               const rev =
                 typeof rawRev === "number" &&
-                Number.isInteger(rawRev) &&
+                Number.isSafeInteger(rawRev) &&
                 rawRev >= 0
                   ? rawRev
                   : 0;

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

@@ -6,7 +6,6 @@ import { readChannelMutesOutbox } from "./channelMutesStorage.ts";
 import { ChannelMuteSyncManager } from "./channelMutesSync.ts";
 import {
   installFakeWindow,
-  installTauriMock,
   installEchoTauri,
   makeFakeWindow,
 } from "./sidebarSyncTestHelpers.mjs";
@@ -490,6 +489,145 @@ test("revert-fix: relay-A watermark does not suppress first-sync seed on relay-B
   }
 });
 
+// ─── Failed pre-publish fetch: retain, never publish (Carl P1) ────────────────
+
+// The pre-publish fetch THROWS (timeout / auth / socket) — NOT proof that no
+// head exists. Publishing the local store here could erase an unseen newer head
+// during a transient outage. Fix: `retain` — keep the durable outbox and retry.
+// Mutation: reverting the merge-lane catch to `publish` fires publishEvent over
+// the unseen head.
+test("failed pre-publish fetch retains the pending edit and retries, never publishing", async () => {
+  mock.method(relayClient, "fetchEvents", () =>
+    Promise.reject(new Error("socket timeout")),
+  );
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installEchoTauri("pk-fetchfail");
+  try {
+    const manager = new ChannelMuteSyncManager("pk-fetchfail", RELAY);
+    manager.publishMutes(makeStore({ a: E(true, 100, 1) }));
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish when the pre-publish fetch failed",
+    );
+    assert.ok(
+      manager.getPendingMuteStore() !== null,
+      "a failed fetch must retain the pending edit",
+    );
+    assert.ok(
+      readChannelMutesOutbox("pk-fetchfail", RELAY),
+      "durable outbox must survive a failed fetch",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
+// ─── Unreadable head: retain, never max-merge over it (Carl P1) ───────────────
+
+// A pre-publish head event exists but cannot be decrypted (transient keychain
+// fault, future NIP-44 scheme, malformed/unsupported payload). A max-merge is
+// only safe once both operands were actually read, so publishing the local
+// store over an uninspectable head would drop its unread entries. Fix: `retain`
+// — keep the durable outbox and retry, never publish. Mutation: reverting the
+// `!remote` branch to `publish` fires publishEvent and clobbers the unread head.
+test("unreadable head (decrypt failure) retains the pending edit and retries, never publishing", async () => {
+  // A head event exists but its ciphertext is not registered in the echo map,
+  // so decrypt rejects → decryptAndParse returns null → retain.
+  mock.method(relayClient, "fetchEvents", () =>
+    Promise.resolve([
+      {
+        pubkey: "pk-undec",
+        content: "unregistered-cipher",
+        created_at: 500,
+        id: "evt-undec",
+      },
+    ]),
+  );
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installEchoTauri("pk-undec");
+  try {
+    const manager = new ChannelMuteSyncManager("pk-undec", RELAY);
+    manager.publishMutes(makeStore({ a: E(true, 100, 1) }));
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish over a head we could not read",
+    );
+    assert.ok(
+      manager.getPendingMuteStore() !== null,
+      "unreadable head must retain the pending edit",
+    );
+    assert.ok(
+      readChannelMutesOutbox("pk-undec", RELAY),
+      "durable outbox must survive an unreadable head",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
+// A readable head whose decrypted payload carries an unsupported schema version
+// (parseMutePayload rejects it → decryptAndParse returns null). Distinct from a
+// decrypt fault: the ciphertext decrypts fine, but we still cannot trust the
+// contents, so retain rather than max-merge over it.
+test("unsupported head payload schema retains the pending edit, never publishing", async () => {
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installEchoTauri("pk-badver");
+  // mintHead stringifies the given object as the head plaintext; a future schema
+  // version decrypts cleanly but parseMutePayload returns null.
+  const head = tauri.mintHead({ version: 2, channels: {} }, 500);
+  mock.method(relayClient, "fetchEvents", () => Promise.resolve([head]));
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  try {
+    const manager = new ChannelMuteSyncManager("pk-badver", RELAY);
+    manager.publishMutes(makeStore({ a: E(true, 100, 1) }));
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish over a head whose schema we do not support",
+    );
+    assert.ok(
+      manager.getPendingMuteStore() !== null,
+      "unsupported schema must retain the pending edit",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
 // ─── Timestamp clamp (Carl P2): a far-future remote head must not push our
 //     published createdAt past the relay's ±15min future-drift window.
 // Mutation test: removing the clamp lets createdAt = lastRemote+1 (~now+3600),
@@ -497,24 +635,16 @@ test("revert-fix: relay-A watermark does not suppress first-sync seed on relay-B
 test("timestamp clamp: published createdAt stays inside the relay future window", async () => {
   const nowSecs = Math.floor(Date.now() / 1000);
   const farFutureHead = nowSecs + 3_600; // 1h ahead — beyond the ±15min window
-  let call = 0;
-  mock.method(relayClient, "fetchEvents", () => {
-    call++;
-    // First call primes lastRemoteCreatedAt to farFutureHead (undecryptable so
-    // it does not merge into the store); pre-publish call returns created_at=0.
-    return Promise.resolve([
-      {
-        pubkey: "pk-clamp",
-        content: "bad-cipher",
-        created_at: call === 1 ? farFutureHead : 0,
-        id: "evt-clamp",
-      },
-    ]);
-  });
-  let signedCreatedAt = null;
   const fw = makeFakeWindow();
   const restore = installFakeWindow(fw);
-  const tauri = installTauriMock(JSON.stringify({ version: 1, channels: {} }));
+  const tauri = installEchoTauri("pk-clamp");
+  // A readable far-future head: primes lastRemoteCreatedAt to farFutureHead on
+  // the priming fetch and is max-merged on the pre-publish fetch. It must be
+  // decryptable — an unreadable head now retains rather than publishes, so the
+  // clamp is only exercised via a head we actually read.
+  const head = tauri.mintHead(makeStore({}), farFutureHead);
+  mock.method(relayClient, "fetchEvents", () => Promise.resolve([head]));
+  let signedCreatedAt = null;
   mock.method(relayClient, "publishEvent", (evt) => {
     signedCreatedAt = evt.created_at;
     return Promise.resolve();

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

@@ -38,6 +38,18 @@ export type RemoteMutes = {
   eventId: string;
 };
 
+/**
+ * Pre-publish decision for the merge lane. `publish` carries the max-merged
+ * store to write; `retain` keeps the durable pending edit and retries. Retain
+ * covers two unreadable cases where publishing would blindly overwrite state we
+ * could not inspect (Carl P1): a pre-publish fetch that THREW (timeout / auth /
+ * socket — not proof no head exists), and an existing head that failed
+ * decryption/parsing (a max-merge is only safe once both operands were read).
+ */
+type MergePublishDecision =
+  | { kind: "publish"; store: ChannelMuteStore }
+  | { kind: "retain" };
+
 async function decryptAndParse(event: RelayEvent): Promise<RemoteMutes | null> {
   try {
     const plaintext = await nip44DecryptFromSelf(event.content);
@@ -209,27 +221,38 @@ export class ChannelMuteSyncManager {
 
   private async fetchOwnBlobBeforePublish(
     store: ChannelMuteStore,
-  ): Promise<ChannelMuteStore> {
+  ): Promise<MergePublishDecision> {
+    let events: RelayEvent[];
     try {
-      const events = await relayClient.fetchEvents({
+      events = await relayClient.fetchEvents({
         kinds: [KIND_CHANNEL_MUTES],
         authors: [this.pubkey],
         "#d": [D_TAG],
         limit: 1,
       });
-      if (events.length === 0 || events[0].pubkey !== this.pubkey) return store;
-      const event = events[0];
-      // Record the raw head before decrypt on the pre-publish path too.
-      this.recordRemoteHead(event.created_at);
-      const remote = await decryptAndParse(event);
-      if (!remote) return store;
-      this.observe(remote.store);
-      // Max-merge: the local edit's per-entry winners survive by construction
-      // and any newer remote entries fold in, so no adopt step is needed.
-      return mergeStores(store, remote.store);
     } catch {
-      return store;
+      // The fetch itself failed (timeout / auth / socket) — NOT proof that no
+      // head exists. Publishing the local store here could erase an unseen
+      // newer head during a transient outage, so retain and retry (Carl P1).
+      return { kind: "retain" };
     }
+    // A successful fetch that proves no head exists: publish the local store.
+    if (events.length === 0 || events[0].pubkey !== this.pubkey)
+      return { kind: "publish", store };
+    const event = events[0];
+    // Record the raw head before decrypt on the pre-publish path too.
+    this.recordRemoteHead(event.created_at);
+    const remote = await decryptAndParse(event);
+    // A head exists but could not be read (decrypt fault / malformed / future
+    // schema). A max-merge is only safe once both operands were actually read,
+    // so retain rather than publish the local store over an uninspectable head
+    // (Carl P1). The whole-blob lanes retain here too; the durable outbox and
+    // bounded retry resume normal resolution once a readable head returns.
+    if (!remote) return { kind: "retain" };
+    this.observe(remote.store);
+    // Max-merge: the local edit's per-entry winners survive by construction and
+    // any newer remote entries fold in, so no adopt step is needed.
+    return { kind: "publish", store: mergeStores(store, remote.store) };
   }
 
   /**
@@ -317,7 +340,7 @@ export class ChannelMuteSyncManager {
     // pending state and will publish the latest store — abandon this stale run.
     if (gen !== this.pendingGeneration) return;
     try {
-      const merged = await this.fetchOwnBlobBeforePublish(store);
+      const decision = await this.fetchOwnBlobBeforePublish(store);
       // Guard: manager may have been destroyed while fetchOwnBlobBeforePublish
       // was awaited (community switch during in-flight fetch).
       if (this.destroyed) return;
@@ -325,6 +348,14 @@ export class ChannelMuteSyncManager {
       // convergence now; the serialized cycle re-drives for it once this run
       // unwinds.
       if (gen !== this.pendingGeneration) return;
+      // The pre-publish read failed or the head was unreadable — keep the
+      // durable pending edit and retry rather than publish over uninspectable
+      // state (Carl P1).
+      if (decision.kind === "retain") {
+        this.scheduleRetry(gen);
+        return;
+      }
+      const merged = decision.store;
       if (this.isIdenticalToLastPublished(merged)) {
         this.discardPending(gen);
         return;

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

@@ -436,6 +436,54 @@ test("malformed (non-JSON) head payload retains the pending edit, never publishi
   }
 });
 
+// ─── Failed pre-publish fetch: retain, never publish (Carl P1) ────────────────
+
+// The pre-publish fetch THROWS (timeout / auth / socket) — this is NOT proof
+// that no head exists. Publishing here would sign above a stale watermark and
+// could erase an unseen newer head during a transient outage. Fix: `retain` —
+// keep the durable pending edit and retry, never publish on a failed fetch.
+// Mutation: reverting the catch to `publish` fires publishEvent over the unseen
+// head.
+test("failed pre-publish fetch retains the pending edit and retries, never publishing", async () => {
+  mock.method(relayClient, "fetchEvents", () =>
+    Promise.reject(new Error("socket timeout")),
+  );
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installTauriMock("{}");
+  try {
+    const manager = new ChannelSectionSyncManager("pk-fetchfail", RELAY);
+    manager.publishSections(
+      makeSectionsStore([{ id: "s1", name: "Work", order: 0 }]),
+    );
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish when the pre-publish fetch failed",
+    );
+    assert.ok(
+      manager.getPendingStore() !== null,
+      "a failed fetch must retain the pending edit",
+    );
+    assert.ok(
+      readChannelSectionsOutbox("pk-fetchfail", RELAY) !== null,
+      "durable outbox must survive a failed fetch",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
 // 5. live-sub: undecryptable event on live path records head before decrypt
 // Mutation test: removing recordRemoteHead before decrypt in the live callback
 // leaves watermark at 0 after a live event.

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

@@ -412,7 +412,12 @@ export class ChannelSectionSyncManager {
       }
       return { kind: "publish", store };
     } catch {
-      return { kind: "publish", store };
+      // The pre-publish fetch itself failed (timeout / auth / socket) — this is
+      // NOT proof that no head exists. Publishing here would sign above a stale
+      // watermark and could erase an unseen newer head during a transient
+      // outage. Retain the durable pending edit and retry rather than overwrite
+      // state we could not read (Carl P1).
+      return { kind: "retain" };
     }
   }
 

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

@@ -388,6 +388,52 @@ test("unsupported head payload version retains the pending edit, never publishin
   }
 });
 
+// ─── Failed pre-publish fetch: retain, never publish (Carl P1) ────────────────
+
+// The pre-publish fetch THROWS (timeout / auth / socket) — this is NOT proof
+// that no head exists. Publishing here would sign above a stale watermark and
+// could erase an unseen newer head during a transient outage. Fix: `retain` —
+// keep the durable pending edit and retry, never publish on a failed fetch.
+// Mutation: reverting the catch to `publish` fires publishEvent over the unseen
+// head.
+test("failed pre-publish fetch retains the pending edit and retries, never publishing", async () => {
+  mock.method(relayClient, "fetchEvents", () =>
+    Promise.reject(new Error("socket timeout")),
+  );
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installTauriMock("{}");
+  try {
+    const manager = new ChannelSortSyncManager("pk-fetchfail", RELAY);
+    manager.publishSortPrefs(makeStore({ channels: "recent" }));
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish when the pre-publish fetch failed",
+    );
+    assert.ok(
+      manager.getPendingStore() !== null,
+      "a failed fetch must retain the pending edit",
+    );
+    assert.ok(
+      readChannelSortOutbox("pk-fetchfail", RELAY) !== null,
+      "durable outbox must survive a failed fetch",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
 // ─── Reconnect keeps the frozen baseline (Carl P1) ────────────────────────────
 
 // The reconnect handler re-drives a pending edit through retryPendingPublish(),

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

@@ -394,7 +394,12 @@ export class ChannelSortSyncManager {
       }
       return { kind: "publish", store };
     } catch {
-      return { kind: "publish", store };
+      // The pre-publish fetch itself failed (timeout / auth / socket) — this is
+      // NOT proof that no head exists. Publishing here would sign above a stale
+      // watermark and could erase an unseen newer head during a transient
+      // outage. Retain the durable pending edit and retry rather than overwrite
+      // state we could not read (Carl P1).
+      return { kind: "retain" };
     }
   }
 

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

@@ -34,7 +34,7 @@ test("parseStarPayload: missing rev normalizes to 0 (old-build blob, entry kept)
   });
 });
 
-test("parseStarPayload: malformed rev (string / negative / non-integer / NaN) normalizes to 0", () => {
+test("parseStarPayload: malformed rev (string / negative / non-integer / NaN / unsafe) normalizes to 0", () => {
   const result = parseStarPayload({
     version: 1,
     channels: {
@@ -42,9 +42,13 @@ test("parseStarPayload: malformed rev (string / negative / non-integer / NaN) no
       neg: { starred: true, updatedAt: 1, rev: -2 },
       frac: { starred: true, updatedAt: 1, rev: 1.5 },
       nan: { starred: true, updatedAt: 1, rev: NaN },
+      // Above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past it, so a
+      // malformed entry at this magnitude would wedge later toggles forever
+      // unless rejected here (Carl P2).
+      huge: { starred: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER + 1 },
     },
   });
-  for (const id of ["str", "neg", "frac", "nan"]) {
+  for (const id of ["str", "neg", "frac", "nan", "huge"]) {
     assert.equal(result.channels[id].rev, 0, `${id} rev normalized to 0`);
     assert.equal(result.channels[id].starred, true, `${id} entry kept`);
   }
@@ -94,7 +98,7 @@ test("parseStarPayload: malformed channel entries missing starred/updatedAt are
   });
 });
 
-test("parseStarPayload: NaN/Infinity/negative updatedAt entries are filtered out", () => {
+test("parseStarPayload: NaN/Infinity/negative/unsafe updatedAt entries are filtered out", () => {
   const result = parseStarPayload({
     version: 1,
     channels: {
@@ -102,6 +106,10 @@ test("parseStarPayload: NaN/Infinity/negative updatedAt entries are filtered out
       inf: { starred: true, updatedAt: Infinity },
       "neg-inf": { starred: true, updatedAt: -Infinity },
       neg: { starred: true, updatedAt: -1 },
+      // Beyond Number.MAX_SAFE_INTEGER: an unrepresentable second is not a
+      // trustworthy watermark, so the entry is dropped rather than kept (Carl
+      // P2 — same bound as `rev`).
+      unsafe: { starred: true, updatedAt: Number.MAX_SAFE_INTEGER + 1 },
       valid: { starred: true, updatedAt: 100, rev: 2 },
     },
   });
@@ -181,6 +189,26 @@ test("mergeStores: same-second old-build click (rev 0) loses to an earlier new-b
   );
 });
 
+test("mergeStores: a huge (unsafe) rev entry cannot wedge a later false toggle", () => {
+  // Carl P2 regression. A malformed blob carries a same-second rev far above
+  // Number.MAX_SAFE_INTEGER on a `starred:true` entry. Before the parse bound,
+  // this survived as-is and — since a real later click mints rev = maxSeen + 1,
+  // which cannot advance past an unsafe integer — the true entry won the
+  // same-second tie forever, suppressing every later unstar. The parser now
+  // normalizes the unsafe rev to 0, so a same-second later click (rev 1) wins.
+  const wedged = parseStarPayload({
+    version: 1,
+    channels: { c: { starred: true, updatedAt: 100, rev: Number.MAX_VALUE } },
+  });
+  assert.equal(wedged.channels.c.rev, 0, "unsafe rev is normalized to 0");
+  const laterUnstar = S(E(false, 100, 1));
+  assert.deepEqual(
+    mergeStores(wedged, laterUnstar).channels.c,
+    E(false, 100, 1),
+    "the same-second later unstar (rev 1) wins — no permanent wedge",
+  );
+});
+
 test("mergeStores: unstar with higher updatedAt overrides star", () => {
   const result = mergeStores(S(E(true, 100, 9)), S(E(false, 999, 1)));
   assert.deepEqual(result.channels.c, E(false, 999, 1));

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

@@ -50,20 +50,24 @@ export function parseStarPayload(json: unknown): ChannelStarStore | null {
                 v !== null &&
                 typeof (v as Record<string, unknown>).starred === "boolean" &&
                 typeof (v as Record<string, unknown>).updatedAt === "number" &&
-                Number.isFinite(
+                Number.isSafeInteger(
                   (v as Record<string, unknown>).updatedAt as number,
                 ) &&
                 ((v as Record<string, unknown>).updatedAt as number) >= 0
               );
             })
-            // Normalize `rev`: accept a non-negative integer, otherwise 0. An
-            // entry is never dropped solely because `rev` is absent (older
-            // build) or malformed — absence is a valid mergeable value.
+            // Normalize `rev`: accept a safe non-negative integer, otherwise 0.
+            // An entry is never dropped solely because `rev` is absent (older
+            // build) or malformed — absence is a valid mergeable value. `rev`
+            // above `Number.MAX_SAFE_INTEGER` is rejected here: at that
+            // magnitude `maxRev + 1` no longer advances, so a malformed entry
+            // could wedge every later toggle. Treating it as rev 0 lets a real
+            // later click win the same-second tie instead (Carl P2).
             .map(([id, v]) => {
               const rawRev = v.rev;
               const rev =
                 typeof rawRev === "number" &&
-                Number.isInteger(rawRev) &&
+                Number.isSafeInteger(rawRev) &&
                 rawRev >= 0
                   ? rawRev
                   : 0;

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

@@ -6,7 +6,6 @@ import { readChannelStarsOutbox } from "./channelStarsStorage.ts";
 import { ChannelStarSyncManager } from "./channelStarsSync.ts";
 import {
   installFakeWindow,
-  installTauriMock,
   installEchoTauri,
   makeFakeWindow,
 } from "./sidebarSyncTestHelpers.mjs";
@@ -490,6 +489,145 @@ test("revert-fix: relay-A watermark does not suppress first-sync seed on relay-B
   }
 });
 
+// ─── Failed pre-publish fetch: retain, never publish (Carl P1) ────────────────
+
+// The pre-publish fetch THROWS (timeout / auth / socket) — NOT proof that no
+// head exists. Publishing the local store here could erase an unseen newer head
+// during a transient outage. Fix: `retain` — keep the durable outbox and retry.
+// Mutation: reverting the merge-lane catch to `publish` fires publishEvent over
+// the unseen head.
+test("failed pre-publish fetch retains the pending edit and retries, never publishing", async () => {
+  mock.method(relayClient, "fetchEvents", () =>
+    Promise.reject(new Error("socket timeout")),
+  );
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installEchoTauri("pk-fetchfail");
+  try {
+    const manager = new ChannelStarSyncManager("pk-fetchfail", RELAY);
+    manager.publishStars(makeStore({ a: E(true, 100, 1) }));
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish when the pre-publish fetch failed",
+    );
+    assert.ok(
+      manager.getPendingStarStore() !== null,
+      "a failed fetch must retain the pending edit",
+    );
+    assert.ok(
+      readChannelStarsOutbox("pk-fetchfail", RELAY),
+      "durable outbox must survive a failed fetch",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
+// ─── Unreadable head: retain, never max-merge over it (Carl P1) ───────────────
+
+// A pre-publish head event exists but cannot be decrypted (transient keychain
+// fault, future NIP-44 scheme, malformed/unsupported payload). A max-merge is
+// only safe once both operands were actually read, so publishing the local
+// store over an uninspectable head would drop its unread entries. Fix: `retain`
+// — keep the durable outbox and retry, never publish. Mutation: reverting the
+// `!remote` branch to `publish` fires publishEvent and clobbers the unread head.
+test("unreadable head (decrypt failure) retains the pending edit and retries, never publishing", async () => {
+  // A head event exists but its ciphertext is not registered in the echo map,
+  // so decrypt rejects → decryptAndParse returns null → retain.
+  mock.method(relayClient, "fetchEvents", () =>
+    Promise.resolve([
+      {
+        pubkey: "pk-undec",
+        content: "unregistered-cipher",
+        created_at: 500,
+        id: "evt-undec",
+      },
+    ]),
+  );
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installEchoTauri("pk-undec");
+  try {
+    const manager = new ChannelStarSyncManager("pk-undec", RELAY);
+    manager.publishStars(makeStore({ a: E(true, 100, 1) }));
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish over a head we could not read",
+    );
+    assert.ok(
+      manager.getPendingStarStore() !== null,
+      "unreadable head must retain the pending edit",
+    );
+    assert.ok(
+      readChannelStarsOutbox("pk-undec", RELAY),
+      "durable outbox must survive an unreadable head",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
+// A readable head whose decrypted payload carries an unsupported schema version
+// (parseStarPayload rejects it → decryptAndParse returns null). Distinct from a
+// decrypt fault: the ciphertext decrypts fine, but we still cannot trust the
+// contents, so retain rather than max-merge over it.
+test("unsupported head payload schema retains the pending edit, never publishing", async () => {
+  const fw = makeFakeWindow();
+  const restore = installFakeWindow(fw);
+  const tauri = installEchoTauri("pk-badver");
+  // mintHead stringifies the given object as the head plaintext; a future schema
+  // version decrypts cleanly but parseStarPayload returns null.
+  const head = tauri.mintHead({ version: 2, channels: {} }, 500);
+  mock.method(relayClient, "fetchEvents", () => Promise.resolve([head]));
+  const publishCalls = [];
+  mock.method(relayClient, "publishEvent", (...args) => {
+    publishCalls.push(args);
+    return Promise.resolve();
+  });
+  try {
+    const manager = new ChannelStarSyncManager("pk-badver", RELAY);
+    manager.publishStars(makeStore({ a: E(true, 100, 1) }));
+    fw._fireTimer();
+    await new Promise((r) => setTimeout(r, 20));
+    assert.equal(
+      publishCalls.length,
+      0,
+      "must not publish over a head whose schema we do not support",
+    );
+    assert.ok(
+      manager.getPendingStarStore() !== null,
+      "unsupported schema must retain the pending edit",
+    );
+    assert.ok(fw._hasTimer(), "a retry must be scheduled");
+  } finally {
+    tauri.restore();
+    restore();
+    mock.reset();
+  }
+});
+
 // ─── Timestamp clamp (Carl P2): a far-future remote head must not push our
 //     published createdAt past the relay's ±15min future-drift window.
 // Mutation test: removing the clamp lets createdAt = lastRemote+1 (~now+3600),
@@ -497,24 +635,16 @@ test("revert-fix: relay-A watermark does not suppress first-sync seed on relay-B
 test("timestamp clamp: published createdAt stays inside the relay future window", async () => {
   const nowSecs = Math.floor(Date.now() / 1000);
   const farFutureHead = nowSecs + 3_600; // 1h ahead — beyond the ±15min window
-  let call = 0;
-  mock.method(relayClient, "fetchEvents", () => {
-    call++;
-    // First call primes lastRemoteCreatedAt to farFutureHead (undecryptable so
-    // it does not merge into the store); pre-publish call returns created_at=0.
-    return Promise.resolve([
-      {
-        pubkey: "pk-clamp",
-        content: "bad-cipher",
-        created_at: call === 1 ? farFutureHead : 0,
-        id: "evt-clamp",
-      },
-    ]);
-  });
-  let signedCreatedAt = null;
   const fw = makeFakeWindow();
   const restore = installFakeWindow(fw);
-  const tauri = installTauriMock(JSON.stringify({ version: 1, channels: {} }));
+  const tauri = installEchoTauri("pk-clamp");
+  // A readable far-future head: primes lastRemoteCreatedAt to farFutureHead on
+  // the priming fetch and is max-merged on the pre-publish fetch. It must be
+  // decryptable — an unreadable head now retains rather than publishes, so the
+  // clamp is only exercised via a head we actually read.
+  const head = tauri.mintHead(makeStore({}), farFutureHead);
+  mock.method(relayClient, "fetchEvents", () => Promise.resolve([head]));
+  let signedCreatedAt = null;
   mock.method(relayClient, "publishEvent", (evt) => {
     signedCreatedAt = evt.created_at;
     return Promise.resolve();

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

@@ -38,6 +38,18 @@ export type RemoteStars = {
   eventId: string;
 };
 
+/**
+ * Pre-publish decision for the merge lane. `publish` carries the max-merged
+ * store to write; `retain` keeps the durable pending edit and retries. Retain
+ * covers two unreadable cases where publishing would blindly overwrite state we
+ * could not inspect (Carl P1): a pre-publish fetch that THREW (timeout / auth /
+ * socket — not proof no head exists), and an existing head that failed
+ * decryption/parsing (a max-merge is only safe once both operands were read).
+ */
+type MergePublishDecision =
+  | { kind: "publish"; store: ChannelStarStore }
+  | { kind: "retain" };
+
 async function decryptAndParse(event: RelayEvent): Promise<RemoteStars | null> {
   try {
     const plaintext = await nip44DecryptFromSelf(event.content);
@@ -209,27 +221,38 @@ export class ChannelStarSyncManager {
 
   private async fetchOwnBlobBeforePublish(
     store: ChannelStarStore,
-  ): Promise<ChannelStarStore> {
+  ): Promise<MergePublishDecision> {
+    let events: RelayEvent[];
     try {
-      const events = await relayClient.fetchEvents({
+      events = await relayClient.fetchEvents({
         kinds: [KIND_CHANNEL_STARS],
         authors: [this.pubkey],
         "#d": [D_TAG],
         limit: 1,
       });
-      if (events.length === 0 || events[0].pubkey !== this.pubkey) return store;
-      const event = events[0];
-      // Record the raw head before decrypt on the pre-publish path too.
-      this.recordRemoteHead(event.created_at);
-      const remote = await decryptAndParse(event);
-      if (!remote) return store;
-      this.observe(remote.store);
-      // Max-merge: the local edit's per-entry winners survive by construction
-      // and any newer remote entries fold in, so no adopt step is needed.
-      return mergeStores(store, remote.store);
     } catch {
-      return store;
+      // The fetch itself failed (timeout / auth / socket) — NOT proof that no
+      // head exists. Publishing the local store here could erase an unseen
+      // newer head during a transient outage, so retain and retry (Carl P1).
+      return { kind: "retain" };
     }
+    // A successful fetch that proves no head exists: publish the local store.
+    if (events.length === 0 || events[0].pubkey !== this.pubkey)
+      return { kind: "publish", store };
+    const event = events[0];
+    // Record the raw head before decrypt on the pre-publish path too.
+    this.recordRemoteHead(event.created_at);
+    const remote = await decryptAndParse(event);
+    // A head exists but could not be read (decrypt fault / malformed / future
+    // schema). A max-merge is only safe once both operands were actually read,
+    // so retain rather than publish the local store over an uninspectable head
+    // (Carl P1). The whole-blob lanes retain here too; the durable outbox and
+    // bounded retry resume normal resolution once a readable head returns.
+    if (!remote) return { kind: "retain" };
+    this.observe(remote.store);
+    // Max-merge: the local edit's per-entry winners survive by construction and
+    // any newer remote entries fold in, so no adopt step is needed.
+    return { kind: "publish", store: mergeStores(store, remote.store) };
   }
 
   /**
@@ -317,7 +340,7 @@ export class ChannelStarSyncManager {
     // pending state and will publish the latest store — abandon this stale run.
     if (gen !== this.pendingGeneration) return;
     try {
-      const merged = await this.fetchOwnBlobBeforePublish(store);
+      const decision = await this.fetchOwnBlobBeforePublish(store);
       // Guard: manager may have been destroyed while fetchOwnBlobBeforePublish
       // was awaited (community switch during in-flight fetch).
       if (this.destroyed) return;
@@ -325,6 +348,14 @@ export class ChannelStarSyncManager {
       // convergence now; the serialized cycle re-drives for it once this run
       // unwinds.
       if (gen !== this.pendingGeneration) return;
+      // The pre-publish read failed or the head was unreadable — keep the
+      // durable pending edit and retry rather than publish over uninspectable
+      // state (Carl P1).
+      if (decision.kind === "retain") {
+        this.scheduleRetry(gen);
+        return;
+      }
+      const merged = decision.store;
       if (this.isIdenticalToLastPublished(merged)) {
         this.discardPending(gen);
         return;