From 758a0507871d595f28c901aab4f147fbe804c408 Mon Sep 17 00:00:00 2001 From: kenny lopez Date: Tue, 25 Aug 2026 20:13:54 +0100 Subject: [PATCH] fix(dm-visibility): harden NIP-DV hidden-DM resurface path against reviewer-flagged defects Address Wess/Jude review defects on the hidden-DM visibility/resurface path. Each is an in-scope correctness fix to the mechanism this PR builds; none change product intent or UI. The #6769 durable cross-process convergence invariant remains an intentional out-of-scope follow-up (bug #6316 stays open, tracked by #6769). A. Clock-skew fence (buzz-db/event.rs): received_at was assigned by the relay process clock (Utc::now()) while hide_dm's hidden_at uses the DB clock (NOW()), so the causal resurface fence compared two clocks. Insert now writes NOW() and RETURNs the value, sourcing both sides from the one database clock. Adds a PG-gated regression bracketing received_at between two DB NOW() reads. B. Mobile replay loop (dm_visibility.dart): the limit:1 snapshot replay could fire before subscribe() resolved, kicking refresh() -> version bump -> resubscribe -> identical replay, unbounded for any user with a stored snapshot. Dedup on snapshot event id (reset on relay/identity change and clear). Adds a regression. C. Huddle invites never resurfaced (buzz-core/kind.rs): KIND_HUDDLE_STARTED is rendered as visible content and treated as a notifiable DM invite but was absent from is_human_visible_message_kind, so it never resurfaced a hidden DM. Added to the trigger set; other huddle lifecycle kinds stay excluded. Test updated. D. Fail-open snapshot read (desktop fetch.rs, mobile channel_directory.dart): a transient kind:30622 read failure was treated as 'nothing hidden', re-exposing every hidden DM until the next good refresh. Now fails closed by propagating the error so the caller preserves the last rendered list. Adds a mobile regression. E. Desktop in-flight invalidation race (useMembershipNotifications.ts): direct channelsQueryKey invalidation could be silently undone by an in-flight get_channels, dropping the resurface signal. Routed through the idle-aware trailing debounce (refreshChannelsWhenIdle) as useLiveChannelUpdates does, with cleanup-time cancel(). Test updated plus an in-flight re-arm regression. Verified locally at this tree: cargo fmt --check, clippy (buzz-core, buzz-db), file-size-check; buzz-core kind tests, buzz-db event tests (incl. PG-gated), buzz-relay resurface integration tests; desktop node --test useMembershipNotifications; mobile flutter analyze + full channels_provider_test suite. All green. Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez --- crates/buzz-core/src/kind.rs | 23 +++- crates/buzz-db/src/event.rs | 90 +++++++++++-- .../src-tauri/src/commands/channels/fetch.rs | 43 ++++--- .../useMembershipNotifications.test.mjs | 99 ++++++++++++++- .../channels/useMembershipNotifications.ts | 31 ++++- .../features/channels/channel_directory.dart | 31 +++-- .../channels_provider/dm_visibility.dart | 19 +++ .../channels/channels_provider_test.dart | 120 ++++++++++++++++++ 8 files changed, 409 insertions(+), 47 deletions(-) diff --git a/crates/buzz-core/src/kind.rs b/crates/buzz-core/src/kind.rs index 8965150763..a775e28f79 100644 --- a/crates/buzz-core/src/kind.rs +++ b/crates/buzz-core/src/kind.rs @@ -794,10 +794,20 @@ pub const fn is_workflow_execution_kind(kind: u32) -> bool { /// /// Reactions, edits, deletions, and system events are deliberately excluded: /// they must not resurface a direct message without a new human-visible message. +/// +/// `KIND_HUDDLE_STARTED` is included: clients render the huddle-start card as +/// visible timeline content and desktop treats it as a notifiable DM invitation, +/// so a hidden DM must resurface to deliver the (time-sensitive) invite. The +/// other huddle lifecycle kinds (joined/left/ended/reaction) are not message +/// content and stay excluded. pub const fn is_human_visible_message_kind(kind: u32) -> bool { matches!( kind, - KIND_STREAM_MESSAGE | KIND_STREAM_MESSAGE_V2 | KIND_FORUM_POST | KIND_FORUM_COMMENT + KIND_STREAM_MESSAGE + | KIND_STREAM_MESSAGE_V2 + | KIND_FORUM_POST + | KIND_FORUM_COMMENT + | KIND_HUDDLE_STARTED ) } @@ -951,11 +961,20 @@ mod tests { KIND_STREAM_MESSAGE_V2, KIND_FORUM_POST, KIND_FORUM_COMMENT, + KIND_HUDDLE_STARTED, ] { assert!(is_human_visible_message_kind(kind), "kind {kind}"); } - for kind in [KIND_REACTION, KIND_STREAM_MESSAGE_EDIT, KIND_DELETION] { + for kind in [ + KIND_REACTION, + KIND_STREAM_MESSAGE_EDIT, + KIND_DELETION, + KIND_HUDDLE_PARTICIPANT_JOINED, + KIND_HUDDLE_PARTICIPANT_LEFT, + KIND_HUDDLE_ENDED, + KIND_HUDDLE_REACTION, + ] { assert!(!is_human_visible_message_kind(kind), "kind {kind}"); } } diff --git a/crates/buzz-db/src/event.rs b/crates/buzz-db/src/event.rs index dcfc97c844..01ea4c05a2 100644 --- a/crates/buzz-db/src/event.rs +++ b/crates/buzz-db/src/event.rs @@ -319,14 +319,19 @@ async fn insert_event_on( let created_at_secs = event.created_at.as_secs() as i64; let created_at = DateTime::from_timestamp(created_at_secs, 0) .ok_or(DbError::InvalidTimestamp(created_at_secs))?; - let received_at = Utc::now(); let d_tag = extract_d_tag(event); let not_before = extract_not_before(event); - let result = sqlx::query( + // `received_at` is assigned by the database clock (`NOW()`) and read back, + // not by the relay process clock. The DM resurface fence compares this + // value against `channel_members.hidden_at`, which is also written with + // `NOW()` (see `hide_dm`); sourcing both from the single database clock is + // what makes that causal comparison sound under relay/DB clock skew. + let inserted_received_at: Option> = sqlx::query_scalar( r#" INSERT INTO events (community_id, id, pubkey, created_at, kind, tags, content, sig, received_at, channel_id, d_tag, not_before) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, NOW(), $9, $10, $11) ON CONFLICT DO NOTHING + RETURNING received_at "#, ) .bind(community_id.as_uuid()) @@ -337,14 +342,17 @@ async fn insert_event_on( .bind(&tags_json) .bind(&event.content) .bind(sig_bytes.as_slice()) - .bind(received_at) .bind(channel_id) .bind(d_tag.as_deref()) .bind(not_before) - .execute(connection) + .fetch_optional(connection) .await?; - let was_inserted = result.rows_affected() > 0; + let was_inserted = inserted_received_at.is_some(); + // On a conflict (duplicate/replay) no row is returned; the value is only + // informational here because callers gate on `was_inserted`, and the + // resurface replay path re-reads the original event's `received_at`. + let received_at = inserted_received_at.unwrap_or_else(Utc::now); Ok(( StoredEvent::with_received_at(event.clone(), received_at, channel_id, true), @@ -1178,15 +1186,19 @@ pub(crate) async fn insert_event_with_thread_metadata_tx( let created_at_secs = event.created_at.as_secs() as i64; let created_at = DateTime::from_timestamp(created_at_secs, 0) .ok_or(DbError::InvalidTimestamp(created_at_secs))?; - let received_at = Utc::now(); let d_tag = extract_d_tag(event); let not_before = extract_not_before(event); - let result = sqlx::query( + // `received_at` is assigned by the database clock (`NOW()`) and read back, + // not by the relay process clock, so the DM resurface fence compares it + // against `channel_members.hidden_at` (also `NOW()`) under one clock. See + // `insert_event_on` for the full rationale. + let inserted_received_at: Option> = sqlx::query_scalar( r#" INSERT INTO events (community_id, id, pubkey, created_at, kind, tags, content, sig, received_at, channel_id, d_tag, not_before) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, NOW(), $9, $10, $11) ON CONFLICT DO NOTHING + RETURNING received_at "#, ) .bind(community_id.as_uuid()) @@ -1197,14 +1209,17 @@ pub(crate) async fn insert_event_with_thread_metadata_tx( .bind(&tags_json) .bind(&event.content) .bind(sig_bytes.as_slice()) - .bind(received_at) .bind(channel_id) .bind(d_tag.as_deref()) .bind(not_before) - .execute(&mut **tx) + .fetch_optional(&mut **tx) .await?; - let was_inserted = result.rows_affected() > 0; + let was_inserted = inserted_received_at.is_some(); + // On a conflict (duplicate/replay) no row is returned; callers gate on + // `was_inserted`, and the resurface replay path re-reads the original + // event's `received_at`. + let received_at = inserted_received_at.unwrap_or_else(Utc::now); if was_inserted { if let Some(ref meta) = thread_meta { @@ -1656,6 +1671,57 @@ mod tests { assert_eq!(persisted, 0); } + /// The DM resurface fence (`unhide_dm_recipients`) compares an event's + /// `received_at` against `channel_members.hidden_at`, which is written with + /// the database clock (`hide_dm` uses `NOW()`). For that comparison to be + /// causally sound under relay/DB clock skew, `received_at` must also come + /// from the database clock — not the relay process `Utc::now()`. + /// + /// This asserts the persisted `received_at` falls inside a `[before, after]` + /// window sampled from the *same database's* `NOW()`, and that the value the + /// insert returns matches the persisted column. A relay-process timestamp + /// would only satisfy this by accident of zero skew. + #[tokio::test] + #[ignore = "requires Postgres"] + async fn insert_event_sources_received_at_from_the_database_clock() { + let pool = setup_pool().await; + let community_uuid = make_test_community(&pool).await; + let community = CommunityId::from_uuid(community_uuid); + let event = make_text_event("db-clock received_at"); + + let before: DateTime = sqlx::query_scalar("SELECT NOW()") + .fetch_one(&pool) + .await + .expect("read db NOW() before insert"); + let (stored, was_inserted) = insert_event(&pool, community, &event, None) + .await + .expect("insert event"); + let after: DateTime = sqlx::query_scalar("SELECT NOW()") + .fetch_one(&pool) + .await + .expect("read db NOW() after insert"); + + assert!(was_inserted, "fresh event must insert"); + assert!( + stored.received_at >= before && stored.received_at <= after, + "received_at {} must fall within the database clock window [{before}, {after}]", + stored.received_at + ); + + let persisted: DateTime = sqlx::query_scalar( + "SELECT received_at FROM events WHERE community_id = $1 AND id = $2", + ) + .bind(community_uuid) + .bind(event.id.as_bytes().as_slice()) + .fetch_one(&pool) + .await + .expect("read persisted received_at"); + assert_eq!( + stored.received_at, persisted, + "the returned received_at must equal the persisted database value" + ); + } + #[tokio::test] #[ignore = "requires Postgres"] async fn event_insert_ttl_trigger_handles_permanent_ephemeral_duplicate_and_activation_race() { diff --git a/desktop/src-tauri/src/commands/channels/fetch.rs b/desktop/src-tauri/src/commands/channels/fetch.rs index 36c24a35b7..249e5af5b1 100644 --- a/desktop/src-tauri/src/commands/channels/fetch.rs +++ b/desktop/src-tauri/src/commands/channels/fetch.rs @@ -289,8 +289,12 @@ pub(super) async fn fetch_channels( DirectoryScope::MemberOnly => Ok(Vec::new()), } }, - // Step 6: NIP-DV hidden-DM snapshot. Tolerant — a failure means no DMs - // are hidden rather than aborting the whole fetch. + // Step 6: NIP-DV hidden-DM snapshot. A read *failure* must NOT be + // treated as "no DMs are hidden": that fail-open collapse would + // re-expose every hidden DM until the next successful refresh. Only a + // successful read of zero snapshots means nothing is hidden. Propagate + // the transport error so the whole refresh fails and the caller keeps + // the previously rendered (correctly filtered) channel list. async { let events = query_relay( state, @@ -300,21 +304,22 @@ pub(super) async fn fetch_channels( "limit": 1, })], ) - .await - .unwrap_or_default(); - events - .iter() - .max_by_key(|e| e.created_at.as_secs()) - .map(|e| { - e.tags - .iter() - .filter_map(|t| { - let s = t.as_slice(); - (s.len() >= 2 && s[0] == "h").then(|| s[1].clone()) - }) - .collect::>() - }) - .unwrap_or_default() + .await?; + Ok::, String>( + events + .iter() + .max_by_key(|e| e.created_at.as_secs()) + .map(|e| { + e.tags + .iter() + .filter_map(|t| { + let s = t.as_slice(); + (s.len() >= 2 && s[0] == "h").then(|| s[1].clone()) + }) + .collect::>() + }) + .unwrap_or_default(), + ) }, ); @@ -323,7 +328,9 @@ pub(super) async fn fetch_channels( let meta_events = member_chain_result?; let open_meta_events = open_meta_result?; - // hidden_dms is already a resolved HashSet (tolerant path above) + // A snapshot read failure fails the whole refresh (fail-closed): the caller + // preserves the last rendered list rather than showing every hidden DM. + let hidden_dms = hidden_dms?; // Merge: member channels (marked as member) + non-member channels (open // directory when included, else pending-owned) not already in the member set. diff --git a/desktop/src/features/channels/useMembershipNotifications.test.mjs b/desktop/src/features/channels/useMembershipNotifications.test.mjs index 8ebfa55013..bc1c11255d 100644 --- a/desktop/src/features/channels/useMembershipNotifications.test.mjs +++ b/desktop/src/features/channels/useMembershipNotifications.test.mjs @@ -48,6 +48,10 @@ test("DM visibility has a dedicated replay and refreshes only the channel list", queryClient.invalidateQueries = async ({ queryKey }) => { invalidations.push(queryKey); }; + // The channel-list invalidation is routed through an idle-aware trailing + // debounce; report the query idle so a fired debounce invalidates rather than + // re-arms. Detail/members keys are invalidated directly. + queryClient.isFetching = () => 0; const wrapper = ({ children }) => React.createElement(QueryClientProvider, { client: queryClient }, children); @@ -91,6 +95,10 @@ test("DM visibility has a dedicated replay and refreshes only the channel list", sig: "sig", }); }); + // The channel-list refresh is debounced (trailing). Let the timer fire. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 600)); + }); assert.deepEqual(invalidations, [["channels"]]); invalidations.length = 0; @@ -108,11 +116,19 @@ test("DM visibility has a dedicated replay and refreshes only the channel list", sig: "sig", }); }); + // Detail/members invalidate synchronously; the channel-list refresh trails. assert.deepEqual(invalidations, [ - ["channels"], ["channels", "new-channel", "detail"], ["channels", "new-channel", "members"], ]); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 600)); + }); + assert.deepEqual(invalidations, [ + ["channels", "new-channel", "detail"], + ["channels", "new-channel", "members"], + ["channels"], + ]); unmount(); } finally { @@ -121,3 +137,84 @@ test("DM visibility has a dedicated replay and refreshes only the channel list", relayClient.subscribeLive = originalSubscribeLive; } }); + +test("channel-list refresh re-arms instead of dropping the signal mid-fetch", async () => { + const React = await import("react"); + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { QueryClient, QueryClientProvider } = await import( + "@tanstack/react-query" + ); + const { relayClient } = await import("@/shared/api/relayClient"); + const { KIND_DM_VISIBILITY } = await import("@/shared/constants/kinds"); + const { useMembershipNotifications } = await import( + "./useMembershipNotifications.ts" + ); + + const originalSubscribeLive = relayClient.subscribeLive; + const subscriptions = []; + relayClient.subscribeLive = async (filter, listener) => { + subscriptions.push({ filter, listener }); + return async () => {}; + }; + + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + const invalidations = []; + queryClient.invalidateQueries = async ({ queryKey }) => { + invalidations.push(queryKey); + }; + // Simulate get_channels in flight: a direct invalidate now would be silently + // undone when the older (pre-event) response lands. The idle-aware routing + // must re-arm instead, so no invalidation lands while fetching, and exactly + // one lands once the query goes idle. + let fetching = 1; + queryClient.isFetching = () => fetching; + const wrapper = ({ children }) => + React.createElement(QueryClientProvider, { client: queryClient }, children); + + try { + renderHook(() => useMembershipNotifications("viewer-pubkey"), { wrapper }); + await act(async () => new Promise((resolve) => setImmediate(resolve))); + + const visibility = subscriptions.find( + (subscription) => + subscription.filter.kinds.length === 1 && + subscription.filter.kinds[0] === KIND_DM_VISIBILITY, + ); + + await act(async () => { + visibility.listener({ + id: "visibility", + pubkey: "relay", + created_at: 1, + kind: KIND_DM_VISIBILITY, + tags: [["p", "viewer-pubkey"]], + content: "", + sig: "sig", + }); + }); + // Debounce fires while a fetch is in flight -> must re-arm, not invalidate. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 600)); + }); + assert.deepEqual( + invalidations, + [], + "must not invalidate while get_channels is in flight", + ); + + // Query goes idle; the re-armed refresh now invalidates exactly once. + fetching = 0; + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 600)); + }); + assert.deepEqual(invalidations, [["channels"]]); + + cleanup(); + } finally { + cleanup(); + queryClient.clear(); + relayClient.subscribeLive = originalSubscribeLive; + } +}); diff --git a/desktop/src/features/channels/useMembershipNotifications.ts b/desktop/src/features/channels/useMembershipNotifications.ts index f66e672b71..371c2f781a 100644 --- a/desktop/src/features/channels/useMembershipNotifications.ts +++ b/desktop/src/features/channels/useMembershipNotifications.ts @@ -2,9 +2,14 @@ import * as React from "react"; import { useQueryClient } from "@tanstack/react-query"; import { channelsQueryKey } from "@/features/channels/hooks"; +import { refreshChannelsWhenIdle } from "@/features/channels/refreshChannelsWhenIdle"; import { getChannelIdFromTags } from "@/features/messages/lib/threading"; import { relayClient } from "@/shared/api/relayClient"; import type { RelayEvent } from "@/shared/api/types"; +import { + createTrailingDebounce, + type TrailingDebounce, +} from "@/shared/lib/trailingDebounce"; import { KIND_DM_VISIBILITY, KIND_MEMBER_ADDED_NOTIFICATION, @@ -13,16 +18,39 @@ import { const MEMBERSHIP_NOTIFICATION_RETRY_BASE_MS = 1_000; const MEMBERSHIP_NOTIFICATION_RETRY_MAX_MS = 30_000; +// Matches useLiveChannelUpdates: collapse a burst of notifications into a +// single trailing refresh once the channels query goes idle. +const CHANNELS_INVALIDATE_DEBOUNCE_MS = 500; export function useMembershipNotifications(currentPubkey?: string) { const queryClient = useQueryClient(); const normalizedCurrentPubkey = currentPubkey?.trim().toLowerCase() ?? ""; + // Route channel-list invalidation through the idle-aware trailing mechanism + // rather than invalidating directly. A direct invalidate while get_channels + // is mid-flight is silently undone when the older (pre-event) response lands + // and clears the dirty flag — dropping the resurface signal so an incoming + // DM stays out of navigation until the next poll. This mirrors ordinary live + // channel traffic (useLiveChannelUpdates), which invalidates the same key. + const channelsInvalidateRef = React.useRef(null); + if (channelsInvalidateRef.current === null) { + channelsInvalidateRef.current = createTrailingDebounce(() => { + refreshChannelsWhenIdle({ + isFetching: () => + queryClient.isFetching({ queryKey: channelsQueryKey }), + invalidate: () => { + void queryClient.invalidateQueries({ queryKey: channelsQueryKey }); + }, + reArm: () => channelsInvalidateRef.current?.trigger(), + }); + }, CHANNELS_INVALIDATE_DEBOUNCE_MS); + } + const handleMembershipNotification = React.useEffectEvent( (event: RelayEvent) => { const channelId = getChannelIdFromTags(event.tags); - void queryClient.invalidateQueries({ queryKey: channelsQueryKey }); + channelsInvalidateRef.current?.trigger(); if (event.kind === KIND_DM_VISIBILITY) { return; } @@ -134,6 +162,7 @@ export function useMembershipNotifications(currentPubkey?: string) { if (retryTimeout !== undefined) { window.clearTimeout(retryTimeout); } + channelsInvalidateRef.current?.cancel(); if (dispose) { void dispose().catch(() => {}); } diff --git a/mobile/lib/features/channels/channel_directory.dart b/mobile/lib/features/channels/channel_directory.dart index a99822ede5..db11b14a1e 100644 --- a/mobile/lib/features/channels/channel_directory.dart +++ b/mobile/lib/features/channels/channel_directory.dart @@ -225,24 +225,29 @@ Future> _resolveDmDisplayNames( return displayNames; } +/// Fetch the set of DM channel ids the viewer currently has hidden, from the +/// relay-signed NIP-DV snapshot. +/// +/// A read *failure* is propagated to the caller rather than swallowed: treating +/// a transient history-read error as "nothing is hidden" would fail open and +/// re-expose every hidden DM until the next successful refresh. An empty result +/// (`events.isEmpty`) is a legitimate "no DMs hidden" and returns `{}`. Callers +/// wrap this in `_fenced` and preserve the prior list on failure, matching the +/// sibling membership/huddle reads. Future> _fetchHiddenDmIds( RelaySessionNotifier session, String myPk, ) async { - try { - final events = await session.fetchHistory(NostrFilters.hiddenDms(myPk)); - if (events.isEmpty) return const {}; - NostrEvent latest = events.first; - for (final event in events.skip(1)) { - if (event.createdAt > latest.createdAt) latest = event; - } - return { - for (final tag in latest.tags) - if (tag.length >= 2 && tag[0] == 'h') tag[1], - }; - } catch (_) { - return const {}; + final events = await session.fetchHistory(NostrFilters.hiddenDms(myPk)); + if (events.isEmpty) return const {}; + NostrEvent latest = events.first; + for (final event in events.skip(1)) { + if (event.createdAt > latest.createdAt) latest = event; } + return { + for (final tag in latest.tags) + if (tag.length >= 2 && tag[0] == 'h') tag[1], + }; } Future> _fetchHuddleStarts( diff --git a/mobile/lib/features/channels/channels_provider/dm_visibility.dart b/mobile/lib/features/channels/channels_provider/dm_visibility.dart index 1820027666..a9ab412a43 100644 --- a/mobile/lib/features/channels/channels_provider/dm_visibility.dart +++ b/mobile/lib/features/channels/channels_provider/dm_visibility.dart @@ -4,6 +4,16 @@ mixin _DmVisibilitySubscription on AsyncNotifier> { void Function()? _unsubscribeDmVisibility; String? _dmVisibilityRelayBaseUrl; String? _dmVisibilityPubkey; + // Id of the last visibility snapshot we acted on. The `limit: 1` filter + // replays the latest stored kind:30622 snapshot on every (re)subscribe; if + // that replay fires before `subscribe()` resolves, `refresh()` bumps the + // subscription version, retires this install, and the next queued sync + // re-subscribes and replays the identical snapshot again — an unbounded + // refresh/subscribe loop for any user with an existing snapshot. Snapshots + // are parameterized-replaceable, so the stored event keeps a stable id; + // suppressing a re-delivered identical id breaks that loop while a genuinely + // new snapshot (new id) still triggers exactly one refresh. + String? _lastHandledDmVisibilityEventId; int get _subscriptionVersion; @@ -22,6 +32,9 @@ mixin _DmVisibilitySubscription on AsyncNotifier> { _unsubscribeDmVisibility = null; _dmVisibilityRelayBaseUrl = relayBaseUrl; _dmVisibilityPubkey = myPk; + // A different relay/identity owns entirely different snapshots; drop the + // dedup key so the new scope's first snapshot is honored. + _lastHandledDmVisibilityEventId = null; } if (_unsubscribeDmVisibility != null || myPk == null) return; @@ -61,6 +74,11 @@ mixin _DmVisibilitySubscription on AsyncNotifier> { void _handleDmVisibilityEvent(NostrEvent event) { if (event.kind != EventKind.dmVisibility) return; + // Suppress a re-delivered identical snapshot (see `_lastHandledDmVisibilityEventId`). + // The relay replays the latest snapshot on every resubscribe; only a new + // snapshot id should drive a refresh. + if (event.id == _lastHandledDmVisibilityEventId) return; + _lastHandledDmVisibilityEventId = event.id; unawaited(refresh()); } @@ -69,6 +87,7 @@ mixin _DmVisibilitySubscription on AsyncNotifier> { _unsubscribeDmVisibility = null; _dmVisibilityRelayBaseUrl = null; _dmVisibilityPubkey = null; + _lastHandledDmVisibilityEventId = null; } Set _mutedChannelIds() => { diff --git a/mobile/test/features/channels/channels_provider_test.dart b/mobile/test/features/channels/channels_provider_test.dart index cd65db5a42..4054e1d052 100644 --- a/mobile/test/features/channels/channels_provider_test.dart +++ b/mobile/test/features/channels/channels_provider_test.dart @@ -2085,6 +2085,42 @@ void main() { expect(session.visibilitySubscribeFilters.single.tags['#p'], [myPk]); }); + test('a failed hidden-DM read keeps DMs hidden (fail-closed)', () async { + // A transient hidden-DM snapshot read failure must NOT collapse to "no + // DMs are hidden": that fail-open behavior would re-expose every hidden + // DM until the next successful refresh. The read now propagates its error + // like the sibling membership/huddle reads, so a refresh that can't read + // the snapshot fails and preserves the previously rendered (correctly + // filtered) list instead of surfacing the hidden DM. + final session = _FakeRelaySession( + memberships: [_membership(_channelA, myPk), _membership(_channelB, myPk)], + metadata: [ + _meta(id: _channelA, name: 'Alice', channelType: 'dm'), + _meta(id: _channelB, name: 'Bob', channelType: 'dm'), + ], + hiddenDmEvents: [ + _hiddenDms([_channelA], pubkey: myPk), + ], + ); + final container = _buildContainer(session: session); + addTearDown(container.dispose); + + final initial = await container.read(channelsProvider.future); + expect(initial.map((c) => c.id), [_channelB]); + + // The next snapshot read fails. `retryDirectory` preserves the prior list + // on any refresh failure; the hidden DM must stay hidden, never resurface. + session.hiddenDmFailures = 1; + await container.read(channelsProvider.notifier).retryDirectory(); + + expect( + container.read(channelsProvider).requireValue.map((c) => c.id), + [_channelB], + reason: + 'a hidden-DM read failure must not re-expose the hidden DM (fail-closed)', + ); + }); + test('a DM visibility snapshot refreshes the hidden channel list', () async { final hiddenDmEvents = [ _hiddenDms([_channelA], pubkey: myPk), @@ -2131,6 +2167,85 @@ void main() { ); }); + test( + 'a re-delivered identical visibility snapshot does not re-refresh', + () async { + // The `limit: 1` DM-visibility subscription replays the latest stored + // snapshot on every (re)subscribe. If a replay that repeats the snapshot + // we already acted on triggered another refresh, the refresh would bump + // the subscription version, discard the in-flight subscription, resubscribe, + // and replay the same snapshot again — an unbounded refresh/resubscribe + // loop. Snapshots are parameterized-replaceable (stable id), so a repeated + // id must be suppressed while a genuinely new snapshot still refreshes. + final hiddenDmEvents = [ + _hiddenDms([_channelA], pubkey: myPk), + ]; + final session = _FakeRelaySession( + memberships: [ + _membership(_channelA, myPk), + _membership(_channelB, myPk), + ], + metadata: [ + _meta(id: _channelA, name: 'Alice', channelType: 'dm'), + _meta(id: _channelB, name: 'Bob', channelType: 'dm'), + ], + hiddenDmEvents: hiddenDmEvents, + ); + final container = _buildContainer(session: session); + addTearDown(container.dispose); + + expect( + (await container.read( + channelsProvider.future, + )).map((channel) => channel.id), + [_channelB], + ); + final refreshesAfterLoad = session.membershipRequestCount; + + // A new snapshot (channelA no longer hidden) drives exactly one refresh. + final unhide = _hiddenDms(const [], pubkey: myPk); + hiddenDmEvents + ..clear() + ..add(unhide); + session.emit(unhide); + await _waitUntil( + () => + container + .read(channelsProvider) + .value + ?.map((channel) => channel.id) + .toSet() + .length == + 2, + ); + final refreshesAfterNewSnapshot = session.membershipRequestCount; + expect( + refreshesAfterNewSnapshot, + greaterThan(refreshesAfterLoad), + reason: 'a new snapshot id must refresh', + ); + + // Re-delivering the SAME snapshot id (the replay) must NOT refresh again. + session.emit(unhide); + session.emit(unhide); + // Let any erroneously-scheduled refresh run. + await Future.delayed(const Duration(milliseconds: 50)); + expect( + session.membershipRequestCount, + refreshesAfterNewSnapshot, + reason: + 'a repeated identical snapshot must be suppressed, not re-refreshed', + ); + expect( + container + .read(channelsProvider) + .requireValue + .map((channel) => channel.id), + unorderedEquals([_channelA, _channelB]), + ); + }, + ); + test( 'archived kind:39000 metadata sets Channel.isArchived (covers TTL auto-archive)', () async { @@ -2489,6 +2604,7 @@ class _FakeRelaySession extends RelaySessionNotifier { List recentMessages; int membershipFailures; int directoryFailures = 0; + int hiddenDmFailures = 0; bool failClaimedMemberCountQuery = false; bool failClaimedUnreadCatchUpQuery = false; int membershipRequestCount = 0; @@ -2742,6 +2858,10 @@ class _FakeRelaySession extends RelaySessionNotifier { _hiddenDmStarted = null; await paused.future; } + if (hiddenDmFailures > 0) { + hiddenDmFailures--; + throw Exception('hidden-DM snapshot fetch failed'); + } return hiddenDmEvents; } if (filter.kinds.contains(EventKind.huddleStarted)) {