diff --git a/desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs b/desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs index 59ee298644..4bc5d137b3 100644 --- a/desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs +++ b/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)); diff --git a/desktop/src/features/sidebar/lib/channelMutesStorage.ts b/desktop/src/features/sidebar/lib/channelMutesStorage.ts index 7f6f478dc9..7f85909081 100644 --- a/desktop/src/features/sidebar/lib/channelMutesStorage.ts +++ b/desktop/src/features/sidebar/lib/channelMutesStorage.ts @@ -50,20 +50,24 @@ export function parseMutePayload(json: unknown): ChannelMuteStore | null { v !== null && typeof (v as Record).muted === "boolean" && typeof (v as Record).updatedAt === "number" && - Number.isFinite( + Number.isSafeInteger( (v as Record).updatedAt as number, ) && ((v as Record).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; diff --git a/desktop/src/features/sidebar/lib/channelMutesSync.test.mjs b/desktop/src/features/sidebar/lib/channelMutesSync.test.mjs index 88ce402702..a5cc3bc06b 100644 --- a/desktop/src/features/sidebar/lib/channelMutesSync.test.mjs +++ b/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(); diff --git a/desktop/src/features/sidebar/lib/channelMutesSync.ts b/desktop/src/features/sidebar/lib/channelMutesSync.ts index eef18cd14b..7ad9fd8620 100644 --- a/desktop/src/features/sidebar/lib/channelMutesSync.ts +++ b/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 { try { const plaintext = await nip44DecryptFromSelf(event.content); @@ -209,27 +221,38 @@ export class ChannelMuteSyncManager { private async fetchOwnBlobBeforePublish( store: ChannelMuteStore, - ): Promise { + ): Promise { + 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; diff --git a/desktop/src/features/sidebar/lib/channelSectionsSync.test.mjs b/desktop/src/features/sidebar/lib/channelSectionsSync.test.mjs index 9116a280e0..a3a90139c2 100644 --- a/desktop/src/features/sidebar/lib/channelSectionsSync.test.mjs +++ b/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. diff --git a/desktop/src/features/sidebar/lib/channelSectionsSync.ts b/desktop/src/features/sidebar/lib/channelSectionsSync.ts index 373111472f..175f52508a 100644 --- a/desktop/src/features/sidebar/lib/channelSectionsSync.ts +++ b/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" }; } } diff --git a/desktop/src/features/sidebar/lib/channelSortSync.test.mjs b/desktop/src/features/sidebar/lib/channelSortSync.test.mjs index 7c499a83e4..a4eff10ff2 100644 --- a/desktop/src/features/sidebar/lib/channelSortSync.test.mjs +++ b/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(), diff --git a/desktop/src/features/sidebar/lib/channelSortSync.ts b/desktop/src/features/sidebar/lib/channelSortSync.ts index fde2f19a35..14a8a76404 100644 --- a/desktop/src/features/sidebar/lib/channelSortSync.ts +++ b/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" }; } } diff --git a/desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs b/desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs index 0c2bf22ea6..1cc25361dd 100644 --- a/desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs +++ b/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)); diff --git a/desktop/src/features/sidebar/lib/channelStarsStorage.ts b/desktop/src/features/sidebar/lib/channelStarsStorage.ts index 6cec7f377b..61dae1c10a 100644 --- a/desktop/src/features/sidebar/lib/channelStarsStorage.ts +++ b/desktop/src/features/sidebar/lib/channelStarsStorage.ts @@ -50,20 +50,24 @@ export function parseStarPayload(json: unknown): ChannelStarStore | null { v !== null && typeof (v as Record).starred === "boolean" && typeof (v as Record).updatedAt === "number" && - Number.isFinite( + Number.isSafeInteger( (v as Record).updatedAt as number, ) && ((v as Record).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; diff --git a/desktop/src/features/sidebar/lib/channelStarsSync.test.mjs b/desktop/src/features/sidebar/lib/channelStarsSync.test.mjs index 0d16817243..53ea9881d4 100644 --- a/desktop/src/features/sidebar/lib/channelStarsSync.test.mjs +++ b/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(); diff --git a/desktop/src/features/sidebar/lib/channelStarsSync.ts b/desktop/src/features/sidebar/lib/channelStarsSync.ts index bf1e5c046c..bec5a0eb34 100644 --- a/desktop/src/features/sidebar/lib/channelStarsSync.ts +++ b/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 { try { const plaintext = await nip44DecryptFromSelf(event.content); @@ -209,27 +221,38 @@ export class ChannelStarSyncManager { private async fetchOwnBlobBeforePublish( store: ChannelStarStore, - ): Promise { + ): Promise { + 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;