From 662014d544f835a075f0020e7d161fe7985d7959 Mon Sep 17 00:00:00 2001 From: Tom Brow Date: Mon, 28 Sep 2026 14:44:43 -0700 Subject: [PATCH] fix(mobile): stale community selection during mobile invite setup (#7951) Buzz sets up starter channels when a person accepts an invite. If the phone already had a saved community, the first attempt could fail. Retry could then succeed. A delayed read could replace the new active community with the old one. The read now leaves the saved selection unchanged. The fix also delays channel-status updates until the channel provider finishes initialization. ### Before and after Controlled Flutter app captures against local relays, with the same delayed storage read. The before capture restores the pre-fix active-community provider in the test harness. The after capture uses the fixed provider. These are separate from the native iOS validation below. | Before: first attempt fails | After: first attempt succeeds | | --- | --- | | ![Before: starter channel setup error with Retry setup button](https://github.com/user-attachments/assets/14d8ba45-6e53-40ae-bc10-f219f08d2f92) | ![After: joined community with Continue to welcome channel button](https://github.com/user-attachments/assets/7dc32d9d-3aae-4378-913c-8ac68cc7d930) | ### Validation - Regression tests reproduce the delayed-read ordering and passing timing control; removing the channel sequencing safeguards makes their regressions fail. - Production app against local relays passes the forced race on the first attempt. - Native iOS simulator with an existing saved community accepted the original invite using a fresh identity, joined on the first attempt, and opened the welcome channel without Retry. Signed-off-by: Tom Brow --- .../features/channels/channel_directory.dart | 21 ++- .../features/channels/channels_provider.dart | 2 +- .../shared/community/community_provider.dart | 24 +--- .../channels/channels_provider_test.dart | 129 ++++++++++-------- .../invite_active_community_race_test.dart | 112 +++++++++++++++ .../community/community_provider_test.dart | 14 ++ 6 files changed, 226 insertions(+), 76 deletions(-) create mode 100644 mobile/test/features/invites/invite_active_community_race_test.dart diff --git a/mobile/lib/features/channels/channel_directory.dart b/mobile/lib/features/channels/channel_directory.dart index dcff0a3569..f7701e0ef2 100644 --- a/mobile/lib/features/channels/channel_directory.dart +++ b/mobile/lib/features/channels/channel_directory.dart @@ -132,6 +132,7 @@ class _ChannelRefreshFence { /// Whether the refresh still owns the active scope and generation. bool get isCurrent => + _coordinator.isMounted() && _generation == _coordinator.generation && scope == _coordinator.currentScope(); @@ -303,6 +304,9 @@ Map _memberCountsByChannelId(Iterable memberEvents) { /// Lives in this part file because `channels_provider.dart` sits against the /// repository-wide 1200-line file ceiling enforced by `just file-size-check`. class _ChannelRefreshCoordinator { + /// Whether the owning provider can still read its current scope. + final bool Function() isMounted; + /// Resolves the relay-and-identity scope that is active right now. final String Function() currentScope; @@ -315,16 +319,18 @@ class _ChannelRefreshCoordinator { int get generation => _generation; _ChannelRefreshCoordinator({ + required this.isMounted, required this.currentScope, required this.loadStatus, }); /// Binds the fence to a notifier's [Ref] so the provider needs one line. /// - /// Both closures read rather than watch: the fence asks what the scope is - /// right now, and must not make the notifier depend on it. + /// The closures read rather than watch: the fence checks the current + /// lifecycle and scope without making the notifier depend on them. factory _ChannelRefreshCoordinator.forRef(Ref ref) => _ChannelRefreshCoordinator( + isMounted: () => ref.mounted, currentScope: () => channelDirectoryScope( ref.read(relayConfigProvider).baseUrl, ref.read(myPubkeyProvider), @@ -343,9 +349,16 @@ class _ChannelRefreshCoordinator { /// Used by callers that must carry the scope across later awaits even when /// they do not refresh discovery, so a membership-only refresh cannot install /// an old scope's list either. - _ChannelRefreshFence beginRefresh({required bool fetchesDirectory}) { + Future<_ChannelRefreshFence> beginRefresh({ + required bool fetchesDirectory, + }) async { final scope = currentScope(); final generation = ++_generation; + final fence = _ChannelRefreshFence(this, scope, generation); + // Acquire ownership synchronously, but let the provider finish building + // before changing another provider's directory status. + await Future.value(); + fence.ensureCurrent(); if (!fetchesDirectory) { final status = loadStatus(); if (status.isLoading(scope)) { @@ -359,7 +372,7 @@ class _ChannelRefreshCoordinator { status.markError(scope); } } - return _ChannelRefreshFence(this, scope, generation); + return fence; } /// Fetches the directory under [fence], or throws if the fence is retired. diff --git a/mobile/lib/features/channels/channels_provider.dart b/mobile/lib/features/channels/channels_provider.dart index 982e39fa2a..f63c271804 100644 --- a/mobile/lib/features/channels/channels_provider.dart +++ b/mobile/lib/features/channels/channels_provider.dart @@ -187,7 +187,7 @@ class ChannelsNotifier extends AsyncNotifier> { // Acquire request ownership before the first relay await. Every channel-list // path uses this fence so completion order cannot let an older ordinary, // directory, or reconnect refresh replace a newer membership list. - final fence = _refreshCoordinator.beginRefresh( + final fence = await _refreshCoordinator.beginRefresh( fetchesDirectory: fetchDirectory, ); diff --git a/mobile/lib/shared/community/community_provider.dart b/mobile/lib/shared/community/community_provider.dart index 525bc6956f..9cd24db0e8 100644 --- a/mobile/lib/shared/community/community_provider.dart +++ b/mobile/lib/shared/community/community_provider.dart @@ -618,21 +618,11 @@ final activeCommunityProvider = FutureProvider((ref) async { if (communities.isEmpty) return null; - if (activeId == null) { - // No active ID stored but communities exist — fall back to first. - await storage.saveActiveId(communities.first.id); - return communities.first; - } - - try { - return communities.firstWhere((w) => w.id == activeId); - } on StateError { - // Active ID points to a community that no longer exists. - // Fall back to first community. - if (communities.isNotEmpty) { - await storage.saveActiveId(communities.first.id); - return communities.first; - } - return null; - } + // This projection can outlive the list it captured above. Never persist a + // fallback here: a newer authentication may already have selected a community + // absent from that list. Authentication owns durable active-ID repair. + return communities.firstWhere( + (community) => community.id == activeId, + orElse: () => communities.first, + ); }); diff --git a/mobile/test/features/channels/channels_provider_test.dart b/mobile/test/features/channels/channels_provider_test.dart index e277a4dfb3..6f3d4ea514 100644 --- a/mobile/test/features/channels/channels_provider_test.dart +++ b/mobile/test/features/channels/channels_provider_test.dart @@ -293,67 +293,88 @@ void main() { ); }); - test( - 'ordinary refresh settles a superseded directory load for retry', - () async { - final session = _FakeRelaySession( - memberships: [_membership(_channelA, myPk)], - metadata: [ - _meta(id: _channelA, name: 'general'), - _meta(id: _channelB, name: 'discoverable'), - ], - ); - final container = _buildContainer(session: session); - addTearDown(container.dispose); + for (final rebuild in [false, true]) { + test( + '${rebuild ? 'Same-scope rebuild' : 'Ordinary refresh'} settles a superseded directory load for retry', + () async { + final session = _FakeRelaySession( + memberships: [_membership(_channelA, myPk)], + metadata: [ + _meta(id: _channelA, name: 'general'), + _meta(id: _channelB, name: 'discoverable'), + ], + ); + final container = _buildContainer(session: session); + addTearDown(container.dispose); - expect( - (await container.read( - channelsProvider.future, - )).map((channel) => channel.id), - [_channelA], - ); + expect( + (await container.read( + channelsProvider.future, + )).map((channel) => channel.id), + [_channelA], + ); - session.pauseNextDirectoryQuery(); - final directory = container - .read(channelsProvider.notifier) - .retryDirectory(); - await session.nextDirectoryQueryStarted; + session.pauseNextDirectoryQuery(); + final directory = container + .read(channelsProvider.notifier) + .retryDirectory(); + await session.nextDirectoryQueryStarted; - // A foreground, reconnect, pull-to-refresh, or membership update can - // start an ordinary refresh while Browse is still loading discovery. - await container.read(channelsProvider.notifier).refresh(); + if (rebuild) { + // A same-scope configuration rebuild can overlap an in-flight + // directory request when onboarding settles its community providers. + final config = container.read(relayConfigProvider); + container + .read(relayConfigProvider.notifier) + .update(baseUrl: config.baseUrl, nsec: config.nsec); + await container.read(channelsProvider.future); + } else { + await container.read(channelsProvider.notifier).refresh(); + } - session.resumePausedDirectoryQuery(); - await directory; - await _settle(); + session.resumePausedDirectoryQuery(); + await directory; + await _settle(); - expect( - container.read(channelDirectoryLoadStatusProvider).status, - ChannelDirectoryLoadStatus.error, - ); - expect( - container - .read(channelsProvider) - .requireValue - .map((channel) => channel.id), - [_channelA], - ); + expect( + container.read(channelDirectoryLoadStatusProvider).status, + ChannelDirectoryLoadStatus.error, + ); + expect( + container + .read(channelsProvider) + .requireValue + .map((channel) => channel.id), + [_channelA], + ); - await container.read(channelsProvider.notifier).retryDirectory(); + await container.read(channelsProvider.notifier).retryDirectory(); - expect( - container.read(channelDirectoryLoadStatusProvider).status, - ChannelDirectoryLoadStatus.loaded, - ); - expect( - container - .read(channelsProvider) - .requireValue - .map((channel) => channel.id), - unorderedEquals([_channelA, _channelB]), - ); - }, - ); + expect( + container.read(channelDirectoryLoadStatusProvider).status, + ChannelDirectoryLoadStatus.loaded, + ); + expect( + container + .read(channelsProvider) + .requireValue + .map((channel) => channel.id), + unorderedEquals([_channelA, _channelB]), + ); + }, + ); + } + + test('refresh disposed before directory settlement writes nothing', () async { + final session = _FakeRelaySession(memberships: const []); + final container = _buildContainer(session: session); + await container.read(channelsProvider.future); + + final refresh = container.read(channelsProvider.notifier).refresh(); + container.dispose(); + + await expectLater(refresh, completes); + }); test( 'community switch discards a stale directory success from the old relay', diff --git a/mobile/test/features/invites/invite_active_community_race_test.dart b/mobile/test/features/invites/invite_active_community_race_test.dart new file mode 100644 index 0000000000..b870788a6d --- /dev/null +++ b/mobile/test/features/invites/invite_active_community_race_test.dart @@ -0,0 +1,112 @@ +import 'dart:async'; + +import 'package:buzz/features/invites/invite_join_provider.dart'; +import 'package:buzz/shared/auth/auth.dart'; +import 'package:buzz/shared/deeplink/deep_link.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:http/http.dart' as http; +import 'package:http/testing.dart'; +import 'package:nostr/nostr.dart' as nostr; + +import '../../shared/community/community_storage_test.dart'; + +void main() { + for (final finishReadBeforeJoin in [false, true]) { + test('first invite setup succeeds when old active read finishes ' + '${finishReadBeforeJoin ? 'before' : 'after'} new selection', () async { + final storage = _PausedActiveReadStorage(); + final old = Community.create( + name: 'Existing', + relayUrl: 'wss://old.example.com', + nsec: nostr.Keys.generate().nsec, + ); + await storage.save(old); + await storage.saveActiveId(old.id); + var recoveryCalls = 0; + final container = ProviderContainer( + overrides: [ + communityStorageProvider.overrideWithValue(storage), + communitySnapshotWriterProvider.overrideWithValue((_) async {}), + inviteJoinHttpClientProvider.overrideWithValue( + MockClient( + (_) async => http.Response( + '{"status":"joined","host":"new.example.com","role":"member"}', + 200, + ), + ), + ), + inviteJoinRecoveryProvider.overrideWithValue((_) { + recoveryCalls++; + return _SuccessfulRecovery(); + }), + ], + ); + addTearDown(container.dispose); + // The app restores authentication before displaying the invite. + await container.read(authProvider.future); + storage.pauseNextRead = true; + final oldProjection = container.read(activeCommunityProvider.future); + await storage.readStarted.future; + if (finishReadBeforeJoin) { + storage.resumeRead.complete(); + await oldProjection; + } + final join = container.read(inviteJoinProvider.notifier); + await join.prepare( + const InviteDeepLink( + relayUrl: 'wss://new.example.com', + code: 'test-invite', + ), + ); + await join.confirmJoin(); + await oldProjection; + + final result = container.read(inviteJoinProvider); + expect(result.status, InviteJoinStatus.success); + expect(result.errorMessage, isNull); + expect(recoveryCalls, 1); + final communities = await storage.loadAll(); + final joined = communities.singleWhere((c) => c.id != old.id); + expect(await storage.loadActiveId(), joined.id); + expect(joined.starterSetupIncomplete, isFalse); + expect( + (await container.read(activeCommunityProvider.future))?.id, + joined.id, + ); + }); + } +} + +class _SuccessfulRecovery implements InviteJoinRecovery { + @override + Future ensureStarterChannels() async => 'welcome'; +} + +/// Delays the storage read after the provider captures its community list. +/// All reads and writes succeed; only their ordering changes. +class _PausedActiveReadStorage extends CommunityStorage { + _PausedActiveReadStorage() : super(secure: FakeSecureStorage()); + + bool pauseNextRead = false; + final readStarted = Completer(); + final resumeRead = Completer(); + + @override + Future loadActiveId() async { + if (pauseNextRead) { + pauseNextRead = false; + readStarted.complete(); + await resumeRead.future; + } + return super.loadActiveId(); + } + + @override + Future saveActiveId(String id) async { + await super.saveActiveId(id); + if (readStarted.isCompleted && !resumeRead.isCompleted) { + resumeRead.complete(); + } + } +} diff --git a/mobile/test/shared/community/community_provider_test.dart b/mobile/test/shared/community/community_provider_test.dart index 9052040c4b..fa977786f2 100644 --- a/mobile/test/shared/community/community_provider_test.dart +++ b/mobile/test/shared/community/community_provider_test.dart @@ -686,6 +686,19 @@ void main() { expect(active.name, 'Test'); }); + test('falls back without persisting when active ID is missing', () async { + final community = Community.create( + name: 'Fallback', + relayUrl: 'https://test.example.com', + ); + await communityStorage.save(community); + container = createContainer(); + + final active = await container.read(activeCommunityProvider.future); + expect(active?.id, community.id); + expect(await communityStorage.loadActiveId(), isNull); + }); + test('falls back to first community if active ID is invalid', () async { container = createContainer(); await container.read(communityListProvider.future); @@ -705,6 +718,7 @@ void main() { final active = await container.read(activeCommunityProvider.future); expect(active, isNotNull); expect(active!.id, ws.id); + expect(await communityStorage.loadActiveId(), 'nonexistent-id'); }); }); }