jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-31 22:41:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(readState): gate NIP-RS clear transitions and observer fallback authority

Browse Source 
Four correctness defects in the 384ebc9d7 merge resolution:

1. [CRITICAL] markChannelRead/markAllChannelsRead bypassed the NIP-RS clear
   transition. Both paths deleted forced-unread state unconditionally without
   calling applyOverrideRead/applyMarkAllReadTransition. A refused clear left
   the authoritative override active while local presentation was wiped.
   Fix: advance frontier first, then call applyOverrideRead (C-bump); gate
   forcedUnread deletion and observed evidence removal on overrideCleared
   outcome. markAllChannelsRead calls applyMarkAllReadTransition per channel
   and routes observed deletion through observedPersistence.removeChannel.

2. [IMPORTANT] Sidebar projection used forcedUnreadRef (stale-prone local
   cache) instead of getOverrideLiveness (manager-authoritative). A remotely
   synced override could fail to light the sidebar.
   Fix: derive isForcedUnread from getOverrideLiveness(channel.id)?.active.

3. [IMPORTANT] Observer fallback (no complete projection) decoded fetched
   events with mergeReadStateEvents (frontier-only) and set override
   authority to an empty map, silently dropping active remote registers.
   Fix: use mergeReadStateEventsStructured in the fallback; use
   structured.overrides as authority; also check authoritative register in
   the channel evaluation loop before falling back to forcedUnreadMap.

4. [IMPORTANT] handleMarkMessageUnread mutated forcedUnreadMsgRef before
   calling markChannelUnread, ignoring the boolean result. A refused manager
   accept (budget_exhausted, load_incomplete) produced phantom unread state.
   Fix: call markChannelUnread first; mutate overlay only on true.

Also adds production-path witnesses:
- adversarial-7 (communityUnreadObserver): active fetched register with no
  complete projection lights the rail (positive fallback coverage).
- adversarial-5/6 (nip-rs-unread-ui): refused-clear contract for
  markChannelRead gate; spec-order frontier-advance-before-cbump.
- useUnreadChannels.test.mjs: updated markAllChannelsRead happy-path to
  reflect NIP-RS-gated semantics (no longer unconditional clearAll).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7
2026-08-04 18:53:07 -04:00
parent 384ebc9d75
commit dde0aac1a7
6 changed files with 216 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
desktop/src/features/channels/nip-rs-unread-ui.test.mjs
View File

@@ -620,3 +620,61 @@ test("adversarial-4: applyMarkAllReadTransition_both_branches", () => {
"refused: forcedMap preserved",
);
});
// ── Call-path witnesses (pass-4 corrective) ────────────────────────────────────
test("adversarial-5: markChannelRead_refused_clear_preserves_forced_unread_entry", () => {
// Call-path witness for the markChannelRead fix: the production hook calls
// applyOverrideRead (C-bump) and ONLY clears forcedUnread when the outcome
// is "overrideCleared". On a refused clear (uint32_overflow), the return value
// must be "overrideStillActive" so the hook's deletion gate preserves the entry.
//
// If the hook ever drops the applyOverrideRead call and unconditionally deletes,
// the existing type-checker catches the missing import. This test locks the
// overrideStillActive return contract that the gate depends on.
const liveness = { active: true, frontier: 100 };
const result = applyOverrideRead(
"ch-refused",
makeApis({
isReadStateReady: true,
markChannelRead: () => ({ success: false, reason: "uint32_overflow" }),
getOverrideLiveness: () => liveness,
}),
);
assert.equal(
result,
"overrideStillActive",
"refused clear must return overrideStillActive so the hook preserves forcedUnread",
);
});
test("adversarial-6: markChannelRead_spec_order_frontier_advance_before_cbump", () => {
// Spec-ordering witness: the production markChannelRead advances the frontier
// BEFORE the C-bump. This means applyOverrideRead sees a post-advance frontier.
// When F advances past B (B < F), active → inactive, so applyOverrideRead
// returns "overrideCleared" — the hook correctly gates the deletion.
//
// Simulation: start with F=50, B=100 (active). Manager markChannelRead advances
// frontier to 101 (B < F → inactive). getOverrideLiveness reflects the new state.
// applyOverrideRead must return "overrideCleared".
let frontier = 50; // initial value; B=100, so active (B ≥ F)
const result = applyOverrideRead(
"ch-advanced",
makeApis({
isReadStateReady: true,
markChannelRead: (_id) => {
frontier = 101; // frontier advance: now F=101 > B=100 → inactive
return { success: true };
},
getOverrideLiveness: () => ({
active: frontier <= 100, // reflects post-advance state
frontier,
}),
}),
);
assert.equal(
result,
"overrideCleared",
"after frontier advance past B, C-bump must clear the override",
);
});

7
desktop/src/features/channels/ui/useChannelUnreadState.ts
View File

@@ -501,15 +501,16 @@ export function useChannelUnreadState({
// cleared on channel-leave; the channel-level force survives until reopen.
const handleMarkMessageUnread = React.useCallback(
(messageId: string) => {
// Call the manager-backed channel mark first; only add messages to the
// forced-unread overlay if the manager accepted (returns true). A refusal
// (budget_exhausted, load_incomplete) must not produce phantom unread state.
if (activeChannelId && !markChannelUnread(activeChannelId)) return;
for (const id of [
messageId,
...getReplyDescendantIdsForMessage(messageId),
]) {
forcedUnreadMsgRef.current.add(id);
}
if (activeChannelId) {
markChannelUnread(activeChannelId);
}
forceUnreadRender();
},
[activeChannelId, getReplyDescendantIdsForMessage, markChannelUnread],

14
desktop/src/features/channels/useUnreadChannels.test.mjs
View File

@@ -117,7 +117,7 @@ test("markChannelRead stale: scope-A callback rejects after scope B loads — B
await harness.unmount();
});
test("markAllChannelsRead happy path: current scope clears all observed refs and clears storage bucket", async () => {
test("markAllChannelsRead happy path: current scope only removes channels that had active observed unread state", async () => {
installFreshStorage();
const PUBKEY = "pubkey-happy-mar";
@@ -125,15 +125,21 @@ test("markAllChannelsRead happy path: current scope clears all observed refs and
const harness = await mountUnreadChannels({ pubkey: PUBKEY });
// clearAll cancels any pending write and removes the storage bucket.
// markAllChannelsRead iterates unreadChannelIdsRef.current. Since no channels
// were observed as unread in this harness (hook just mounted, no live messages
// ingested, isReadStateReady is false), the loop body never executes and
// seeded storage from a prior session is not disturbed.
// Scope-safety (that stale scope-A callbacks cannot corrupt scope-B's refs)
// is verified in the stale companion test below.
await act(async () => {
harness.markAllChannelsRead();
});
// No channels were in the observed-unread set, so storage is unchanged.
const stored = readObservedUnreadFromStorage(PUBKEY, RELAY);
assert.ok(
stored === null || stored.size === 0,
"storage bucket must be empty after markAllChannelsRead under current scope",
stored?.has("channel-1"),
"seeded channel-1 must be undisturbed: markAllChannelsRead only clears channels with observed unread state",
);
await harness.unmount();

72
desktop/src/features/channels/useUnreadChannels.ts
View File

@@ -24,6 +24,8 @@ import {
useForcedUnreadActions,
} from "@/features/channels/forcedUnreadStore";
import {
applyMarkAllReadTransition,
applyOverrideRead,
applyOverrideUnread,
type OverrideAPIs,
} from "@/features/channels/readStateOverride";
@@ -333,16 +335,8 @@ export function useUnreadChannels(
topLevelOnly?: boolean;
} = {},
) => {
if (
!preserveForcedUnread &&
Object.hasOwn(forcedUnreadRef.current, channelId)
) {
delete forcedUnreadRef.current[channelId];
if (pubkey) {
forcedUnreadStore.write(pubkey, forcedUnreadRef.current);
}
bumpLatestVersion();
}
// Frontier advance first (spec order: advance → C-bump → inspect liveness).
// The advance may make F > B, so applyOverrideRead sees an inactive override.
const observedLatest = topLevelOnly
? undefined
: latestByChannelRef.current.get(channelId);
@@ -350,19 +344,35 @@ export function useUnreadChannels(
readAt,
observedLatest,
);
if (markAt === null) return;
markContextRead(channelId, markAt);
if (markAt !== null) {
markContextRead(channelId, markAt);
}
// C-bump; clear local presentation only when liveness is confirmed inactive
// AND the caller has not opted to preserve the forced-unread state.
// null ≠ inactive — pre-init fails closed.
if (!preserveForcedUnread) {
const outcome = applyOverrideRead(channelId, overrideApis);
if (outcome === "overrideCleared") {
if (Object.hasOwn(forcedUnreadRef.current, channelId)) {
delete forcedUnreadRef.current[channelId];
if (pubkey) {
forcedUnreadStore.write(pubkey, forcedUnreadRef.current);
}
bumpLatestVersion();
}
}
}
// Delegate destructive observed-ref removal to the fenced owner operation —
// the parent must not delete from latestByChannelRef or
// observedUnreadEventsByChannelRef directly on the clear-observed 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.)
if (clearObserved) {
if (markAt !== null && clearObserved) {
observedPersistence.removeChannel(channelId);
bumpLatestVersion();
}
},
[markContextRead, observedPersistence, pubkey],
[markContextRead, observedPersistence, overrideApis, pubkey],
);
const {
@@ -889,10 +899,10 @@ export function useUnreadChannels(
let unreadChannelNotificationCount = 0;
for (const channel of channels) {
const isForcedUnread = Object.hasOwn(
forcedUnreadRef.current,
channel.id,
);
const isForcedUnread =
// Manager liveness is authoritative (isReadStateReady). Use
// getOverrideLiveness, not forcedUnreadRef which may be stale.
getOverrideLiveness(channel.id)?.active === true;
if (channel.id === activeChannelId && !isForcedUnread) continue;
const observedEvents = observedUnreadEventsByChannelRef.current.get(
@@ -984,7 +994,6 @@ export function useUnreadChannels(
const markAllChannelsRead = React.useCallback(() => {
for (const channelId of unreadChannelIdsRef.current) {
delete forcedUnreadRef.current[channelId];
const unixSeconds =
latestByChannelRef.current.get(channelId) ??
getEffectiveTimestamp(channelId) ??
@@ -992,17 +1001,30 @@ export function useUnreadChannels(
if (unixSeconds !== null) {
markContextRead(channelId, unixSeconds);
}
// Gate evidence clearing on manager outcome — a refused clear (budget_exhausted,
// load_incomplete) must not wipe local forced-unread or observed state for
// channels whose override is still active. applyMarkAllReadTransition handles
// the forcedUnreadRef mutation; route observed deletion through the fenced API.
const outcome = applyMarkAllReadTransition(channelId, overrideApis, {
forcedUnread: forcedUnreadRef.current,
latestByChannel: latestByChannelRef.current,
observedUnreadEvents: observedUnreadEventsByChannelRef.current,
});
if (outcome === "overrideCleared") {
observedPersistence.removeChannel(channelId);
}
}
if (pubkey) {
forcedUnreadStore.write(pubkey, forcedUnreadRef.current);
}
// Delegate destructive observed-ref clearing to the fenced owner operation —
// 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();
bumpLatestVersion();
}, [getEffectiveTimestamp, markContextRead, observedPersistence, pubkey]);
}, [
getEffectiveTimestamp,
markContextRead,
observedPersistence,
overrideApis,
pubkey,
]);
// Identity-stable snapshots of the membership sets for the notify gate.
// Re-derived only when membershipVersion bumps (a set actually changed), so

77
desktop/src/features/communities/communityUnreadObserver.test.mjs
View File

@@ -1189,3 +1189,80 @@ test("fetchCommunityUnread adversarial-6: torn fetched register (ov_s+ov_c witho
// no message events — rail must remain dark.
assert.deepEqual(result, { hasUnread: false, mentionCount: 0 });
});
test("fetchCommunityUnread adversarial-7: active fetched register, no complete projection → rail lights", async () => {
// Positive fallback witness: when getProjection() is absent (null), the observer
// must decode the fetched read-state events with mergeReadStateEventsStructured
// and use the resulting `overrides` map as override authority. An active register
// (S > C, B ≥ F) in the fetched state must light the rail.
//
// This locks the fallback branch: before the fix, the fallback set authoritative
// to an empty Map(), so any remote NIP-RS override older than the seven-day
// horizon would be silently ignored. Adversarial-1 covers the tombstone (dark)
// case; this witness covers the live-register (lit) case.
const SLOT = "c".repeat(32); // valid 32-hex slot, distinct from other tests
const relay = relayFor([
// 1. member events
() => [
event({
tags: [
["d", CHANNEL_ID],
["p", PUBKEY],
],
}),
],
// 2. metadata events
() => [
event({
tags: [
["d", CHANNEL_ID],
["t", "stream"],
],
}),
],
// 3. visibility events
() => [],
// 4. read-state: live register S=5,C=0,B=100; frontier F=50 (B ≥ F, S > C → active)
() => [
event({
pubkey: PUBKEY,
created_at: 300,
tags: [
["d", `read-state:${SLOT}`],
["t", "read-state"],
],
content: JSON.stringify({
v: 1,
client_id: "client",
contexts: {
[CHANNEL_ID]: 50,
[`ov_s:${CHANNEL_ID}`]: 5,
[`ov_c:${CHANNEL_ID}`]: 0,
[`ov_b:${CHANNEL_ID}`]: 100,
},
}),
}),
],
// 5. mutes
() => [],
// 6. unread events — none (liveness from override register only)
() => [],
// 7. mention events — none
() => [],
]);
// No getProjection — fallback path must use fetched structured overrides.
const result = await fetchCommunityUnread({
client: relay,
pubkey: PUBKEY,
nowSeconds: 400,
decryptReadState: async (v) => v,
decryptMutes: async (v) => v,
readThreadRelationships: readRelationships(),
// Intentionally no getProjection.
});
// Fetched active register (S=5 > C=0, B=100 ≥ F=50) must light the rail.
assert.deepEqual(result, { hasUnread: true, mentionCount: 0 });
});

35
desktop/src/features/communities/communityUnreadObserver.ts
View File

@@ -5,10 +5,7 @@ import {
type ForcedUnreadMap,
} from "@/features/channels/forcedUnreadStore";
import { DM_NOTIFIABLE_EVENT_KINDS } from "@/features/channels/isDmNotifiableKind";
import {
mergeReadStateEvents,
mergeReadStateEventsStructured,
} from "@/features/channels/readState/readStateSnapshot";
import { mergeReadStateEventsStructured } from "@/features/channels/readState/readStateSnapshot";
import { deduplicateByCoordinate } from "@/features/channels/readState/readStateFencedLoader";
import {
isOverrideActive,
@@ -242,13 +239,15 @@ export async function fetchCommunityUnread(args: {
}
readStateMap = merged;
} else {
readStateMap = await mergeReadStateEvents(
readStateEvents,
const structured = await mergeReadStateEventsStructured(
deduplicateByCoordinate(readStateEvents),
pubkey,
args.decryptReadState,
);
// No complete projection — override authority is the forced-unread store.
authoritative = new Map<string, OverrideRegister>();
// No complete projection — use fetched-deduped structured state as the
// override authority so active remote registers are not silently dropped.
authoritative = structured.overrides;
readStateMap = structured.frontiers;
}
let mutedIds = new Set<string>();
@@ -295,13 +294,19 @@ export async function fetchCommunityUnread(args: {
hasUnread = true;
}
} else {
// Forced-unread lights the dot without a relay fetch, but only if the
// synced read marker has NOT advanced past the stored baseline. This
// prevents stale forced-unread from lighting the rail after a cross-device
// read has covered the channel (the drain path in useUnreadChannels only
// runs while the community is active, so the store may not be pruned for
// inactive communities).
if (Object.hasOwn(forcedUnreadMap, channel.id)) {
// No complete projection — check fetched-deduped override registers first
// (remote NIP-RS marks), then fall back to the locally-stored forced-unread
// map for marks not yet synced to the relay.
const reg = authoritative.get(channel.id);
if (reg !== undefined && isOverrideActive(reg, readAt ?? 0)) {
hasUnread = true;
} else if (Object.hasOwn(forcedUnreadMap, channel.id)) {
// Forced-unread lights the dot without a relay fetch, but only if the
// synced read marker has NOT advanced past the stored baseline. This
// prevents stale forced-unread from lighting the rail after a cross-device
// read has covered the channel (the drain path in useUnreadChannels only
// runs while the community is active, so the store may not be pruned for
// inactive communities).
const markerAtWhenForced = forcedUnreadMarker(
forcedUnreadMap[channel.id],
);