From e7d187bbb32d828671a4f96127c3d3845c901744 Mon Sep 17 00:00:00 2001 From: Taksh Date: Sun, 16 Aug 2026 22:51:07 +0530 Subject: [PATCH 1/5] fix(desktop): stop a future-dated message from marking a channel read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit created_at is self-asserted by the sending client, and the relay bounds it for moderation commands but not for ordinary messages. resolveChannelReadMarker took the newest timestamp verbatim, so one event dated ahead of the clock landed in the read marker and every genuinely new message afterwards failed createdAt > readAt — no badge, no divider, and since #5983 no thread resume — until wall-clock time caught up with the bad value. Both inputs are event-derived, so the ceiling covers both: callerReadAt comes from a message's own created_at (channel open, the Esc shortcut's lastMessageAt, mark-all-read) and observedLatest from live events. A tolerance rather than a hard now ceiling: ordinary skew between two machines is seconds, and clamping that hard would leave a just-received message unread until this clock caught up. 120s is the relay's own MAX_COMMAND_SKEW_SECS. clearObserved now reports false when the marker was clamped below the observed event, so the observed refs survive — that event really is still unread, and dropping them would clear the sidebar dot this change exists to keep. Refs #6046 Signed-off-by: Taksh --- .../features/channels/useUnreadChannels.ts | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/desktop/src/features/channels/useUnreadChannels.ts b/desktop/src/features/channels/useUnreadChannels.ts index 0464a00fe0b..f239a4b647c 100644 --- a/desktop/src/features/channels/useUnreadChannels.ts +++ b/desktop/src/features/channels/useUnreadChannels.ts @@ -98,6 +98,20 @@ function toUnixSeconds(isoOrMs: string | null | undefined): number | null { return ms === null ? null : Math.floor(ms / 1_000); } +// How far ahead of this machine's clock a read marker may land. `created_at` +// is self-asserted by the sending client and the relay does not bound it for +// ordinary messages, so an unbounded marker lets one future-dated event mark +// every later message in the channel as already read — no badge, no divider, +// no thread resume — until wall-clock time catches up with it. +// +// A tolerance rather than a hard `now` ceiling: ordinary skew between two +// machines is seconds, and clamping that hard would leave a just-received +// message unread until this clock caught up. 120s matches the relay's own +// `MAX_COMMAND_SKEW_SECS` (`handlers/moderation_commands.rs`), which is the +// house number for "clock difference we accept"; NIP-AB already says clients +// MUST NOT set `created_at` in the future at all. +const MAX_READ_MARKER_SKEW_SECONDS = 120; + // Resolve where the read marker should land when a channel is marked read. // Folds the caller's timeline position together with the newest event this // client has observed live (`observedLatest`), so an explicit "mark read" still @@ -107,17 +121,29 @@ function toUnixSeconds(isoOrMs: string | null | undefined): number | null { // the resulting marker covers the observed timestamp, signalling the caller to // drop its observed refs so the unread memo sees `latest === undefined` until a // genuinely newer event arrives. +// +// Both inputs are event-derived, so both are clamped: `callerReadAt` comes from +// a message's own `created_at` (channel-open, the Esc shortcut's +// `lastMessageAt`, mark-all-read) and `observedLatest` from live events. +// +// `nowSeconds` is injectable so the ceiling is testable; it must stay a +// parameter rather than a captured constant. export function resolveChannelReadMarker( callerReadAt: string | null | undefined, observedLatest: number | undefined, + nowSeconds: number = Date.now() / 1_000, ): { markAt: number | null; clearObserved: boolean } { const callerUnix = toUnixSeconds(callerReadAt); - const markAt = Math.max(callerUnix ?? 0, observedLatest ?? 0) || null; + const requested = Math.max(callerUnix ?? 0, observedLatest ?? 0) || null; + const ceiling = Math.floor(nowSeconds) + MAX_READ_MARKER_SKEW_SECONDS; + const markAt = requested === null ? null : Math.min(requested, ceiling); return { markAt, clearObserved: markAt !== null && observedLatest !== undefined && + // A clamped marker does not cover a future-dated observed event, so the + // observed refs must survive: that event really is still unread. observedLatest <= markAt, }; } From e6bb0e9d3c386beecd10d25ae84f3c2d573de90e Mon Sep 17 00:00:00 2001 From: Taksh Date: Sun, 16 Aug 2026 22:51:07 +0530 Subject: [PATCH 2/5] test(desktop): cover the clamped read marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five cases: a caller timestamp a year ahead, an observed event a year ahead (asserting the observed refs are kept), the end-to-end shape — poison, mark read, then a genuine message still counted unread by computeChannelUnreadMarker — plus 30s of ordinary skew passing through unclamped, and the past-timestamp paths unchanged. Reverting the clamp turns the first three red. Refs #6046 Signed-off-by: Taksh --- .../channels/unreadReadMarker.test.mjs | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/desktop/src/features/channels/unreadReadMarker.test.mjs b/desktop/src/features/channels/unreadReadMarker.test.mjs index 660d4f5c61b..a704c0b61b9 100644 --- a/desktop/src/features/channels/unreadReadMarker.test.mjs +++ b/desktop/src/features/channels/unreadReadMarker.test.mjs @@ -513,3 +513,80 @@ test("addThreadActivityItems keeps newest items when input is newest-first", () assert.equal(result.items[0].id, "reply-1"); assert.equal(result.items.at(-1).id, "reply-100"); }); + +// --- A future-dated created_at must not mark later messages read --- +// +// `created_at` is self-asserted by the sending client and the relay does not +// bound it for ordinary messages. Before the clamp, one event dated a year out +// landed in the read marker verbatim, so every genuinely new message failed +// `createdAt > readAt` and was classified read: no badge, no divider, no +// thread resume, until wall-clock time caught up. + +const NOW = 1_780_000_000; +const YEAR_AHEAD = NOW + 365 * 24 * 60 * 60; + +test("resolveChannelReadMarker_futureCallerReadAt_clampsToSkewCeiling", () => { + const result = resolveChannelReadMarker( + new Date(YEAR_AHEAD * 1_000).toISOString(), + undefined, + NOW, + ); + + assert.equal(result.markAt, NOW + 120); + assert.ok( + result.markAt < YEAR_AHEAD, + "a future-dated message must not push the marker past it", + ); +}); + +test("resolveChannelReadMarker_futureObservedLatest_clampsAndKeepsObserved", () => { + const result = resolveChannelReadMarker(null, YEAR_AHEAD, NOW); + + assert.equal(result.markAt, NOW + 120); + // The observed refs must survive: that event really is still unread, so + // clearing them would drop the sidebar dot the clamp exists to preserve. + assert.equal(result.clearObserved, false); +}); + +test("resolveChannelReadMarker_afterClamp_aLaterRealMessageIsStillUnread", () => { + // The end-to-end shape of the bug, through the same comparison the divider + // uses: poison arrives, the channel is marked read, then a genuine message. + const { markAt } = resolveChannelReadMarker( + new Date(YEAR_AHEAD * 1_000).toISOString(), + undefined, + NOW, + ); + const genuineMessage = topLevel("later", NOW + 300); + + const marker = computeChannelUnreadMarker([genuineMessage], markAt); + + assert.equal(marker.firstUnreadMessageId, "later"); + assert.equal(marker.unreadCount, 1); +}); + +test("resolveChannelReadMarker_ordinarySkewInsideTolerance_isNotClamped", () => { + // A sender 30s ahead of this machine is normal. Clamping that would leave a + // just-received message unread until this clock caught up. + const slightlyAhead = NOW + 30; + + const result = resolveChannelReadMarker( + new Date(slightlyAhead * 1_000).toISOString(), + undefined, + NOW, + ); + + assert.equal(result.markAt, slightlyAhead); +}); + +test("resolveChannelReadMarker_pastReadAt_isUnaffectedByTheCeiling", () => { + // Comfortably before NOW: the ceiling must only ever bite upwards. + const readAt = new Date((NOW - 86_400) * 1_000).toISOString(); + const expected = NOW - 86_400; + + assert.equal( + resolveChannelReadMarker(readAt, undefined, NOW).markAt, + expected, + ); + assert.equal(resolveChannelReadMarker(null, 200, NOW).markAt, 200); + assert.equal(resolveChannelReadMarker(null, undefined, NOW).markAt, null); +}); From a828199aab7163a8294e2eae48b0849da9f12d4b Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 17 Aug 2026 07:44:41 +0530 Subject: [PATCH 3/5] fix(desktop): repair an implausible read marker to the present, don't clamp it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding (P1, themiguelamador on #6063): mapping an outlier to `now + 120` manufactures a future read frontier. A correct message arriving at `now + 1` still failed `createdAt > readAt`, so the clamp went on hiding legitimate messages for two minutes — a smaller version of the bug it was meant to fix. The tolerance decides whether a timestamp is *plausible*; it is not a value to clamp to. `isPlausibleReadMarker` now names that policy, and `resolveChannelReadMarker` discards each implausible input rather than pulling it down to the ceiling, keeping any plausible input beside it — a real caller position is no longer thrown away because the observed timestamp next to it is poison. Only when nothing survives is the marker repaired to the present: the mark-read gesture is real, so it still takes effect, and just the future-dated event stays unread. The policy lives in `readState/readStateFormat.ts` because every route a marker can enter by has to share it; the other routes follow in the next two commits. Splits `resolveChannelReadMarkerUnix` out of the ISO-string wrapper so callers that already hold unix seconds can use the same funnel. The two tests that asserted `now + 120` encoded the wrong contract and now assert the repair, plus the case that motivates it: a message one second from now is still unread against a repaired marker. Signed-off-by: Taksh --- .../channels/readState/readStateFormat.ts | 35 ++++++++ .../channels/unreadReadMarker.test.mjs | 65 ++++++++++++--- .../features/channels/useUnreadChannels.ts | 80 +++++++++++-------- 3 files changed, 134 insertions(+), 46 deletions(-) diff --git a/desktop/src/features/channels/readState/readStateFormat.ts b/desktop/src/features/channels/readState/readStateFormat.ts index 4cd61e9f43e..cbcc37f487c 100644 --- a/desktop/src/features/channels/readState/readStateFormat.ts +++ b/desktop/src/features/channels/readState/readStateFormat.ts @@ -37,6 +37,41 @@ export const THREAD_PREFIX = "thread:"; const EVENT_ID_PATTERN = /^[0-9a-f]{64}$/; +// How far ahead of this machine's clock a read marker may plausibly land. +// `created_at` is self-asserted by the sending client and the relay does not +// bound it for ordinary messages, so an unbounded marker lets one future-dated +// event mark every later message in the channel as already read — no badge, no +// divider, no thread resume — until wall-clock time catches up with it. +// +// A tolerance rather than a hard `now` ceiling: ordinary skew between two +// machines is seconds, and rejecting that would drop a marker a sibling device +// legitimately wrote. 120s matches the relay's own `MAX_COMMAND_SKEW_SECS` +// (`handlers/moderation_commands.rs`), the house number for "clock difference +// we accept"; NIP-AB already says clients MUST NOT set `created_at` in the +// future at all. +export const MAX_READ_MARKER_SKEW_SECONDS = 120; + +export function nowUnixSeconds(): number { + return Math.floor(Date.now() / 1_000); +} + +/** + * Whether a read marker at `unixSeconds` could have been written by a clock + * this one agrees with. + * + * The single skew policy for read state. Every route a marker can enter by + * consults it, because read markers are monotonic and persisted: a marker + * accepted once from a live event, from local storage, or from an NIP-RS + * event synced by another (possibly unpatched) desktop stays effective, and + * a year-ahead one is unrecoverable from the UI. + */ +export function isPlausibleReadMarker( + unixSeconds: number, + now: number = nowUnixSeconds(), +): boolean { + return unixSeconds <= now + MAX_READ_MARKER_SKEW_SECONDS; +} + export function maxReadAt(...markers: Array): number | null { return markers.reduce((latest, marker) => { if (marker === null) return latest; diff --git a/desktop/src/features/channels/unreadReadMarker.test.mjs b/desktop/src/features/channels/unreadReadMarker.test.mjs index a704c0b61b9..8bcdba83968 100644 --- a/desktop/src/features/channels/unreadReadMarker.test.mjs +++ b/desktop/src/features/channels/unreadReadMarker.test.mjs @@ -517,38 +517,77 @@ test("addThreadActivityItems keeps newest items when input is newest-first", () // --- A future-dated created_at must not mark later messages read --- // // `created_at` is self-asserted by the sending client and the relay does not -// bound it for ordinary messages. Before the clamp, one event dated a year out -// landed in the read marker verbatim, so every genuinely new message failed -// `createdAt > readAt` and was classified read: no badge, no divider, no -// thread resume, until wall-clock time caught up. +// bound it for ordinary messages. Before the skew policy, one event dated a +// year out landed in the read marker verbatim, so every genuinely new message +// failed `createdAt > readAt` and was classified read: no badge, no divider, +// no thread resume, until wall-clock time caught up. +// +// The tolerance decides whether a timestamp is *plausible*; it is not a value +// to clamp to. Clamping an outlier to `now + 120` would manufacture a read +// frontier two minutes into the future and hide every legitimate message that +// arrives before the clock reaches it. An implausible timestamp is discarded +// and the marker repaired to the present instead. const NOW = 1_780_000_000; const YEAR_AHEAD = NOW + 365 * 24 * 60 * 60; -test("resolveChannelReadMarker_futureCallerReadAt_clampsToSkewCeiling", () => { +test("resolveChannelReadMarker_futureCallerReadAt_repairsToThePresent", () => { const result = resolveChannelReadMarker( new Date(YEAR_AHEAD * 1_000).toISOString(), undefined, NOW, ); - assert.equal(result.markAt, NOW + 120); + assert.equal(result.markAt, NOW); assert.ok( result.markAt < YEAR_AHEAD, "a future-dated message must not push the marker past it", ); }); -test("resolveChannelReadMarker_futureObservedLatest_clampsAndKeepsObserved", () => { +test("resolveChannelReadMarker_repairedMarker_doesNotHideTheNextTwoMinutes", () => { + // The regression the ceiling-clamp version had: a correct message arriving a + // second from now must still be unread against the repaired marker. + const { markAt } = resolveChannelReadMarker( + new Date(YEAR_AHEAD * 1_000).toISOString(), + undefined, + NOW, + ); + + const marker = computeChannelUnreadMarker( + [topLevel("soon", NOW + 1)], + markAt, + ); + + assert.equal(marker.firstUnreadMessageId, "soon"); + assert.equal(marker.unreadCount, 1); +}); + +test("resolveChannelReadMarker_futureObservedLatest_repairsAndKeepsObserved", () => { const result = resolveChannelReadMarker(null, YEAR_AHEAD, NOW); - assert.equal(result.markAt, NOW + 120); + assert.equal(result.markAt, NOW); // The observed refs must survive: that event really is still unread, so - // clearing them would drop the sidebar dot the clamp exists to preserve. + // clearing them would drop the sidebar dot the policy exists to preserve. + assert.equal(result.clearObserved, false); +}); + +test("resolveChannelReadMarker_keepsThePlausibleInputWhenTheOtherIsPoison", () => { + // Discarding is per-input, not on the max: a real caller position must not be + // thrown away just because the observed timestamp beside it is implausible. + const realRead = NOW - 3_600; + + const result = resolveChannelReadMarker( + new Date(realRead * 1_000).toISOString(), + YEAR_AHEAD, + NOW, + ); + + assert.equal(result.markAt, realRead); assert.equal(result.clearObserved, false); }); -test("resolveChannelReadMarker_afterClamp_aLaterRealMessageIsStillUnread", () => { +test("resolveChannelReadMarker_afterRepair_aLaterRealMessageIsStillUnread", () => { // The end-to-end shape of the bug, through the same comparison the divider // uses: poison arrives, the channel is marked read, then a genuine message. const { markAt } = resolveChannelReadMarker( @@ -564,9 +603,9 @@ test("resolveChannelReadMarker_afterClamp_aLaterRealMessageIsStillUnread", () => assert.equal(marker.unreadCount, 1); }); -test("resolveChannelReadMarker_ordinarySkewInsideTolerance_isNotClamped", () => { - // A sender 30s ahead of this machine is normal. Clamping that would leave a - // just-received message unread until this clock caught up. +test("resolveChannelReadMarker_ordinarySkewInsideTolerance_isKept", () => { + // A sender 30s ahead of this machine is normal. Discarding that would move + // the marker back to now and leave a message the user just read unread. const slightlyAhead = NOW + 30; const result = resolveChannelReadMarker( diff --git a/desktop/src/features/channels/useUnreadChannels.ts b/desktop/src/features/channels/useUnreadChannels.ts index f239a4b647c..4f06a56a661 100644 --- a/desktop/src/features/channels/useUnreadChannels.ts +++ b/desktop/src/features/channels/useUnreadChannels.ts @@ -16,6 +16,7 @@ import { type ObservedUnreadEvent, } from "@/features/channels/unreadChannelCounts"; import { useReadState } from "@/features/channels/readState/useReadState"; +import { isPlausibleReadMarker } from "@/features/channels/readState/readStateFormat"; import { makeRootIdStore } from "@/features/channels/unreadRootIdStore"; import { forcedUnreadStore, @@ -98,56 +99,69 @@ function toUnixSeconds(isoOrMs: string | null | undefined): number | null { return ms === null ? null : Math.floor(ms / 1_000); } -// How far ahead of this machine's clock a read marker may land. `created_at` -// is self-asserted by the sending client and the relay does not bound it for -// ordinary messages, so an unbounded marker lets one future-dated event mark -// every later message in the channel as already read — no badge, no divider, -// no thread resume — until wall-clock time catches up with it. -// -// A tolerance rather than a hard `now` ceiling: ordinary skew between two -// machines is seconds, and clamping that hard would leave a just-received -// message unread until this clock caught up. 120s matches the relay's own -// `MAX_COMMAND_SKEW_SECS` (`handlers/moderation_commands.rs`), which is the -// house number for "clock difference we accept"; NIP-AB already says clients -// MUST NOT set `created_at` in the future at all. -const MAX_READ_MARKER_SKEW_SECONDS = 120; - -// Resolve where the read marker should land when a channel is marked read. -// Folds the caller's timeline position together with the newest event this -// client has observed live (`observedLatest`), so an explicit "mark read" still -// covers messages that arrived faster than channel metadata — this fold is -// load-bearing for the Esc shortcut, sidebar mark-read, and empty-channel open, -// all of which pass a null/stale caller value. `clearObserved` reports whether -// the resulting marker covers the observed timestamp, signalling the caller to -// drop its observed refs so the unread memo sees `latest === undefined` until a +// Resolve where the read marker should land when a channel is marked read, +// from timestamps already in unix seconds. Folds the caller's timeline position +// together with the newest event this client has observed live +// (`observedLatest`), so an explicit "mark read" still covers messages that +// arrived faster than channel metadata — this fold is load-bearing for the Esc +// shortcut, sidebar mark-read, empty-channel open and mark-all-read, all of +// which pass a null/stale caller value. `clearObserved` reports whether the +// resulting marker covers the observed timestamp, signalling the caller to drop +// its observed refs so the unread memo sees `latest === undefined` until a // genuinely newer event arrives. // -// Both inputs are event-derived, so both are clamped: `callerReadAt` comes from -// a message's own `created_at` (channel-open, the Esc shortcut's -// `lastMessageAt`, mark-all-read) and `observedLatest` from live events. +// Both inputs are event-derived — `callerUnix` from a message's own +// `created_at`, `observedLatest` from live events — so `isPlausibleReadMarker` +// decides for each of them whether it could have come from a clock we agree +// with. An implausible input is *discarded*, not clamped: clamping it to the +// tolerance ceiling would manufacture a read frontier at `now + 120` and hide +// every legitimate message for the next two minutes. If no input survives, the +// marker is repaired to the present — the mark-read gesture is real, so it +// still takes effect, and only the future-dated event itself stays unread. // -// `nowSeconds` is injectable so the ceiling is testable; it must stay a +// `nowSeconds` is injectable so the policy is testable; it must stay a // parameter rather than a captured constant. -export function resolveChannelReadMarker( - callerReadAt: string | null | undefined, +export function resolveChannelReadMarkerUnix( + callerUnix: number | null, observedLatest: number | undefined, nowSeconds: number = Date.now() / 1_000, ): { markAt: number | null; clearObserved: boolean } { - const callerUnix = toUnixSeconds(callerReadAt); const requested = Math.max(callerUnix ?? 0, observedLatest ?? 0) || null; - const ceiling = Math.floor(nowSeconds) + MAX_READ_MARKER_SKEW_SECONDS; - const markAt = requested === null ? null : Math.min(requested, ceiling); + if (requested === null) return { markAt: null, clearObserved: false }; + + const now = Math.floor(nowSeconds); + const plausible = [callerUnix, observedLatest].filter( + (value): value is number => + value !== null && + value !== undefined && + value > 0 && + isPlausibleReadMarker(value, now), + ); + const markAt = plausible.length > 0 ? Math.max(...plausible) : now; return { markAt, clearObserved: - markAt !== null && observedLatest !== undefined && - // A clamped marker does not cover a future-dated observed event, so the + // A repaired marker does not cover a future-dated observed event, so the // observed refs must survive: that event really is still unread. observedLatest <= markAt, }; } +// String-timestamp front door for `resolveChannelReadMarkerUnix`, for the +// callers that hold a message's ISO `created_at`. +export function resolveChannelReadMarker( + callerReadAt: string | null | undefined, + observedLatest: number | undefined, + nowSeconds: number = Date.now() / 1_000, +): { markAt: number | null; clearObserved: boolean } { + return resolveChannelReadMarkerUnix( + toUnixSeconds(callerReadAt), + observedLatest, + nowSeconds, + ); +} + export function resolveObservedUnreadRootId(tags: string[][]): string | null { return isBroadcastReply(tags) ? null : getThreadReference(tags).rootId; } From 9dcc67d090d84c116b0591612a08a7e5eeaa4999 Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 17 Aug 2026 07:46:53 +0530 Subject: [PATCH 4/5] fix(desktop): route mark-all-read through the read-marker funnel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding (P1, themiguelamador on #6063): `markAllChannelsRead` wrote `latestByChannelRef` straight through `markContextRead`, bypassing `resolveChannelReadMarker` entirely, and then called `clearAll()` — which deleted the observed evidence. A future-dated event therefore still poisoned the marker through the Esc shortcut and the community rail's mark-all action, and lost its unread dot on the way out. Mark-all now resolves each channel through `resolveChannelReadMarkerUnix`, the same funnel as `markChannelRead`, so there is one place a marker can be written and one skew policy behind it. Folding the effective timestamp and the observed timestamp with `Math.max` also subsumes the old `??` preference between them. Because a repaired marker does not cover the future-dated event, that channel's observed evidence has to survive the clear: `clearAll` takes a retain set and rebuilds both refs from it, writing the survivors back instead of wiping the bucket. With no retain set it wipes as before. Signed-off-by: Taksh --- .../useObservedUnreadPersistence.test.mjs | 61 ++++++++++++++++ .../channels/useObservedUnreadPersistence.ts | 73 +++++++++++++------ .../features/channels/useUnreadChannels.ts | 25 +++++-- 3 files changed, 129 insertions(+), 30 deletions(-) diff --git a/desktop/src/features/channels/useObservedUnreadPersistence.test.mjs b/desktop/src/features/channels/useObservedUnreadPersistence.test.mjs index a9d422a4cf2..32e09b266a1 100644 --- a/desktop/src/features/channels/useObservedUnreadPersistence.test.mjs +++ b/desktop/src/features/channels/useObservedUnreadPersistence.test.mjs @@ -511,3 +511,64 @@ test("unrelated rerenders do not change API object identity (catch-up stability) await harness.unmount(); }); + +test("clearAll retains the channels mark-all-read could not cover", async () => { + installFreshStorage(); + + // Two channels observed unread. Mark-all-read repairs an implausible marker + // to the present, so channel-2's future-dated event stays uncovered and its + // evidence must survive the clear — otherwise its unread dot is lost for good. + const seedMap = new Map(); + const ch1 = new Map(); + ch1.set("evt-1", makeObservedEvent({ id: "evt-1", createdAt: NOW_S })); + seedMap.set("channel-1", ch1); + const ch2 = new Map(); + ch2.set("evt-2", makeObservedEvent({ id: "evt-2", createdAt: NOW_S + 1 })); + seedMap.set("channel-2", ch2); + writeObservedUnreadToStorage(PUBKEY, RELAY, seedMap); + + const refs = { + eventsRef: { current: new Map() }, + latestRef: { current: new Map() }, + }; + const harness = await mountDefaultHook(refs); + + harness.api.clearAll(new Set(["channel-2"])); + + assert.deepEqual( + [...refs.eventsRef.current.keys()], + ["channel-2"], + "the retained channel's observed events must stay in memory", + ); + assert.deepEqual([...refs.latestRef.current.keys()], ["channel-2"]); + + await act(async () => { + globalThis.dispatchEvent({ type: "pagehide" }); + }); + + const persisted = readObservedUnreadFromStorage(PUBKEY, RELAY); + assert.ok(persisted, "a partial clear must write survivors, not wipe them"); + assert.deepEqual([...persisted.keys()], ["channel-2"]); + + await harness.unmount(); +}); + +test("clearAll with an empty retain set still wipes the bucket", async () => { + installFreshStorage(); + const refs = makeRefs(); + const harness = await mountDefaultHook(refs); + + harness.api.schedule(harness.api.currentScope); + harness.api.clearAll(new Set()); + + refs.eventsRef.current = new Map(); + refs.latestRef.current = new Map(); + + await act(async () => { + globalThis.dispatchEvent({ type: "pagehide" }); + }); + + assert.equal(readObservedUnreadFromStorage(PUBKEY, RELAY), null); + + await harness.unmount(); +}); diff --git a/desktop/src/features/channels/useObservedUnreadPersistence.ts b/desktop/src/features/channels/useObservedUnreadPersistence.ts index c1a94570b44..35dfb9ccc64 100644 --- a/desktop/src/features/channels/useObservedUnreadPersistence.ts +++ b/desktop/src/features/channels/useObservedUnreadPersistence.ts @@ -29,8 +29,15 @@ export type ObservedUnreadPersistence = { schedule: (scope: string) => void; /** Remove a single channel from the persisted cache (clearObserved path). */ removeChannel: (channelId: string) => void; - /** Clear the entire persisted cache (mark-all-read path). */ - clearAll: () => void; + /** + * Clear the persisted cache (mark-all-read path). + * + * `retainChannelIds` keeps those channels' observed events and latest + * timestamps: mark-all-read repairs an implausible marker to the present, so + * a future-dated observed event stays uncovered and is genuinely still + * unread. Dropping its evidence here would lose its unread dot for good. + */ + clearAll: (retainChannelIds?: ReadonlySet) => void; }; /** @@ -163,27 +170,47 @@ export function useObservedUnreadPersistence( [currentScope, observedUnreadEventsByChannelRef, latestByChannelRef], ); - const clearAll = React.useCallback(() => { - // Reject if the loaded scope has drifted — a stale callback must not - // cancel the new scope's pending snapshot or clear the wrong bucket. - if (scopeLoadedRef.current !== currentScope) return; - // Cancel any pending snapshot and clear both in-memory refs before touching - // storage — the parent no longer resets the refs directly, so this is the - // single transactional clear path for mark-all-read. - if (timerRef.current !== null) { - clearTimeout(timerRef.current); - timerRef.current = null; - } - observedUnreadEventsByChannelRef.current = new Map(); - latestByChannelRef.current = new Map(); - clearObservedUnreadStorage(normalizedPubkey ?? "", normalizedRelayUrl); - }, [ - currentScope, - normalizedPubkey, - normalizedRelayUrl, - observedUnreadEventsByChannelRef, - latestByChannelRef, - ]); + const clearAll = React.useCallback( + (retainChannelIds?: ReadonlySet) => { + // Reject if the loaded scope has drifted — a stale callback must not + // cancel the new scope's pending snapshot or clear the wrong bucket. + if (scopeLoadedRef.current !== currentScope) return; + // Cancel any pending snapshot and clear both in-memory refs before touching + // storage — the parent no longer resets the refs directly, so this is the + // single transactional clear path for mark-all-read. + if (timerRef.current !== null) { + clearTimeout(timerRef.current); + timerRef.current = null; + } + if (retainChannelIds && retainChannelIds.size > 0) { + // Partial clear: rebuild both refs from the retained channels and write + // the survivors back, rather than wiping the bucket. Assigning fresh + // Maps is safe — `persistRefs` holds the ref objects, not the Maps. + observedUnreadEventsByChannelRef.current = new Map( + [...observedUnreadEventsByChannelRef.current].filter(([channelId]) => + retainChannelIds.has(channelId), + ), + ); + latestByChannelRef.current = new Map( + [...latestByChannelRef.current].filter(([channelId]) => + retainChannelIds.has(channelId), + ), + ); + scheduleObservedUnreadWrite(currentScope, persistRefs.current); + return; + } + observedUnreadEventsByChannelRef.current = new Map(); + latestByChannelRef.current = new Map(); + clearObservedUnreadStorage(normalizedPubkey ?? "", normalizedRelayUrl); + }, + [ + currentScope, + normalizedPubkey, + normalizedRelayUrl, + observedUnreadEventsByChannelRef, + latestByChannelRef, + ], + ); // isScopeLoaded reads the ref at call time — always fresh, never a stale // snapshot from a closed-over useMemo value. diff --git a/desktop/src/features/channels/useUnreadChannels.ts b/desktop/src/features/channels/useUnreadChannels.ts index 4f06a56a661..4155c85a754 100644 --- a/desktop/src/features/channels/useUnreadChannels.ts +++ b/desktop/src/features/channels/useUnreadChannels.ts @@ -954,14 +954,25 @@ export function useUnreadChannels( unreadChannelIdsRef.current = unreadChannelIds; const markAllChannelsRead = React.useCallback(() => { + // Channels whose observed evidence must outlive the clear: their newest + // observed event is future-dated, so the repaired marker does not cover it + // and it is genuinely still unread. Clearing them here would delete the + // only record of that event and silently drop its unread dot. + const retainObserved = new Set(); for (const channelId of unreadChannelIdsRef.current) { delete forcedUnreadRef.current[channelId]; - const unixSeconds = - latestByChannelRef.current.get(channelId) ?? - getEffectiveTimestamp(channelId) ?? - null; - if (unixSeconds !== null) { - markContextRead(channelId, unixSeconds); + const observedLatest = latestByChannelRef.current.get(channelId); + // Same funnel as markChannelRead — mark-all must not be a second, + // unbounded way to write a marker. + const { markAt, clearObserved } = resolveChannelReadMarkerUnix( + getEffectiveTimestamp(channelId), + observedLatest, + ); + if (markAt !== null) { + markContextRead(channelId, markAt); + } + if (observedLatest !== undefined && !clearObserved) { + retainObserved.add(channelId); } } if (pubkey) { @@ -971,7 +982,7 @@ export function useUnreadChannels( // the parent must not reset the observed Maps directly on this path, or a // stale scope-A callback could corrupt scope B before the fence rejects. // (Fenced record writes in handleChannelMessage and catch-up remain in the parent.) - observedPersistence.clearAll(); + observedPersistence.clearAll(retainObserved); bumpLatestVersion(); }, [getEffectiveTimestamp, markContextRead, observedPersistence, pubkey]); From 8eb0165caea75b2de8997474b31e88e0547f8c6a Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 17 Aug 2026 07:48:50 +0530 Subject: [PATCH 5/5] fix(desktop): disarm a poisoned read marker on hydration and on sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding (P1, themiguelamador on #6063): constraining new local `markChannelRead` calls is not enough. Read markers are monotonic and are loaded from local storage and from encrypted NIP-RS events, so a marker written before this change — or by another desktop that still lacks it — stays year-ahead and is unrecoverable from the UI. Both entry points now apply the same `isPlausibleReadMarker` policy: - `readStoredReadState` skips an implausible persisted marker. Hydration is the last place it can be disarmed, because the next write persists whatever was loaded. - `sanitizeContexts` skips one in a decrypted NIP-RS blob, alongside the malformed-value checks it already made. Both *drop* rather than clamp. Clamping to the present would assert a read position nobody reached; dropping restores the channel to unread, which the user can see and act on. Markers inside the tolerance are untouched, so ordinary cross-device skew still merges normally. Signed-off-by: Taksh --- .../readState/readStateFormat.test.mjs | 51 +++++++++++++++++++ .../channels/readState/readStateFormat.ts | 12 +++++ .../readState/readStateStorage.test.mjs | 47 +++++++++++++++++ .../channels/readState/readStateStorage.ts | 15 +++++- 4 files changed, 123 insertions(+), 2 deletions(-) diff --git a/desktop/src/features/channels/readState/readStateFormat.test.mjs b/desktop/src/features/channels/readState/readStateFormat.test.mjs index 417e40b02d3..714d9d199ba 100644 --- a/desktop/src/features/channels/readState/readStateFormat.test.mjs +++ b/desktop/src/features/channels/readState/readStateFormat.test.mjs @@ -3,9 +3,12 @@ import test from "node:test"; import { isMsgContextKey, + isPlausibleReadMarker, isThreadContextKey, + MAX_READ_MARKER_SKEW_SECONDS, maxReadAt, msgContextKey, + sanitizeContexts, } from "./readStateFormat.ts"; const EVENT_ID = "a".repeat(64); @@ -58,3 +61,51 @@ test("isThreadContextKey_shortId_returnsFalse", () => { test("msgContextKey_output_roundTripsThroughValidator", () => { assert.equal(isMsgContextKey(msgContextKey(EVENT_ID)), true); }); + +// --- Synced read state carries the same skew policy ----------------------- +// +// A NIP-RS blob may have been published by another desktop that predates the +// policy. Admitting a year-ahead context there would poison this device too, +// and monotonic merging means it would never expire. + +test("sanitizeContexts_dropsAnImplausiblyFutureMarker", () => { + const now = 1_780_000_000; + + const result = sanitizeContexts( + { + "channel-real": now - 3_600, + "channel-skewed": now + 30, + "channel-poisoned": now + 365 * 24 * 60 * 60, + }, + now, + ); + + assert.deepEqual(result, { + "channel-real": now - 3_600, + "channel-skewed": now + 30, + }); +}); + +test("sanitizeContexts_stillDropsTheOldMalformedShapes", () => { + const now = 1_780_000_000; + + assert.deepEqual( + sanitizeContexts( + { a: "12", b: 1.5, c: -1, d: 4_294_967_296, e: now - 1 }, + now, + ), + { e: now - 1 }, + ); +}); + +test("isPlausibleReadMarker_boundaryIsInclusive", () => { + const now = 1_780_000_000; + assert.equal( + isPlausibleReadMarker(now + MAX_READ_MARKER_SKEW_SECONDS, now), + true, + ); + assert.equal( + isPlausibleReadMarker(now + MAX_READ_MARKER_SKEW_SECONDS + 1, now), + false, + ); +}); diff --git a/desktop/src/features/channels/readState/readStateFormat.ts b/desktop/src/features/channels/readState/readStateFormat.ts index cbcc37f487c..15d12a92298 100644 --- a/desktop/src/features/channels/readState/readStateFormat.ts +++ b/desktop/src/features/channels/readState/readStateFormat.ts @@ -129,14 +129,26 @@ export function isValidBlob(obj: unknown): obj is ReadStateBlob { return true; } +/** + * Validate a decrypted blob's context map. + * + * Implausible markers are dropped rather than clamped. Markers are monotonic + * and this blob may have been written by another desktop that predates the + * skew policy, so a year-ahead entry admitted here would silently mark every + * later message read and never expire. Dropping it restores the channel to + * unread, which the user can see and act on; clamping it to the present would + * assert a read position nobody ever reached. + */ export function sanitizeContexts( contexts: Record, + now: number = nowUnixSeconds(), ): Record { const result: Record = {}; for (const [key, value] of Object.entries(contexts)) { if (new TextEncoder().encode(key).length > 256) continue; if (typeof value !== "number" || !Number.isInteger(value)) continue; if (value < 0 || value > 4294967295) continue; + if (!isPlausibleReadMarker(value, now)) continue; result[key] = value; } return result; diff --git a/desktop/src/features/channels/readState/readStateStorage.test.mjs b/desktop/src/features/channels/readState/readStateStorage.test.mjs index ad7eb031ebc..cafe34dc7f1 100644 --- a/desktop/src/features/channels/readState/readStateStorage.test.mjs +++ b/desktop/src/features/channels/readState/readStateStorage.test.mjs @@ -153,3 +153,50 @@ test("writeStoredReadState survives a throwing localStorage.setItem", () => { ); }); }); + +// --- A poisoned marker must not survive a restart ------------------------- +// +// Read markers are monotonic and persisted. A marker written before the skew +// policy existed — or by a sibling desktop that still lacks it — is year-ahead +// and unrecoverable from the UI, so hydration is the last place it can be +// disarmed: the next write persists whatever readStoredReadState loaded. + +test("readStoredReadState drops a persisted year-ahead marker", () => { + installLocalStorage(); + const pubkey = "f".repeat(64); + const yearAhead = NOW + 365 * 24 * 60 * 60; + + localStorage.setItem( + localReadStateKey(pubkey), + JSON.stringify({ + "channel-real": new Date((NOW - 3_600) * 1_000).toISOString(), + "channel-poisoned": new Date(yearAhead * 1_000).toISOString(), + }), + ); + + const stored = readStoredReadState(pubkey, NOW); + + assert.equal(stored.contexts.get("channel-real"), NOW - 3_600); + assert.equal( + stored.contexts.has("channel-poisoned"), + false, + "an implausible marker must not be hydrated back into effect", + ); +}); + +test("readStoredReadState keeps a marker inside the skew tolerance", () => { + installLocalStorage(); + const pubkey = "1".repeat(64); + const slightlyAhead = NOW + 30; + + localStorage.setItem( + localReadStateKey(pubkey), + JSON.stringify({ + "channel-skewed": new Date(slightlyAhead * 1_000).toISOString(), + }), + ); + + const stored = readStoredReadState(pubkey, NOW); + + assert.equal(stored.contexts.get("channel-skewed"), slightlyAhead); +}); diff --git a/desktop/src/features/channels/readState/readStateStorage.ts b/desktop/src/features/channels/readState/readStateStorage.ts index f5ac8996134..6be6d0173ff 100644 --- a/desktop/src/features/channels/readState/readStateStorage.ts +++ b/desktop/src/features/channels/readState/readStateStorage.ts @@ -1,11 +1,13 @@ import { isPlainRecord, + isPlausibleReadMarker, localIsoToUnixSeconds, localPublishableContextKey, localReadStateKey, localSourceCreatedAtKey, LOCAL_MAX_PRUNABLE_CONTEXTS, MSG_PREFIX, + nowUnixSeconds, READ_STATE_HORIZON_SECONDS, THREAD_PREFIX, } from "@/features/channels/readState/readStateFormat"; @@ -20,6 +22,7 @@ export type StoredReadState = { function mergeLocalStorageKey( contexts: Map, key: string, + now: number, ): void { const raw = localStorage.getItem(key); if (!raw) return; @@ -31,6 +34,11 @@ function mergeLocalStorageKey( for (const [channelId, value] of Object.entries(parsed)) { const unixSeconds = localIsoToUnixSeconds(value); if (unixSeconds === null) continue; + // A marker persisted before the skew policy existed — or written by a + // sibling desktop that still lacks it — is dropped on the way in, not + // clamped: hydration is the last chance to disarm it, since markers are + // monotonic and the next write persists whatever we load here. + if (!isPlausibleReadMarker(unixSeconds, now)) continue; const current = contexts.get(channelId) ?? 0; if (unixSeconds > current) { contexts.set(channelId, unixSeconds); @@ -92,9 +100,12 @@ function readContextSourceCreatedAt(pubkey: string): Map { return result; } -export function readStoredReadState(pubkey: string): StoredReadState { +export function readStoredReadState( + pubkey: string, + now: number = nowUnixSeconds(), +): StoredReadState { const contexts = new Map(); - mergeLocalStorageKey(contexts, localReadStateKey(pubkey)); + mergeLocalStorageKey(contexts, localReadStateKey(pubkey), now); return { contexts,