From 2e10c0b4c587185d5f15f60e057fd3f05b094a12 Mon Sep 17 00:00:00 2001 From: alltheseas Date: Mon, 16 Mar 2026 14:16:14 -0500 Subject: [PATCH] Fix stale relay list: replace UserDefaults hex lookup with ndb query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the fragile `latestRelayListEventIdHex` UserDefaults lookup in `getLatestNIP65RelayListEvent()` with a direct nostrdb query using `ndb.query(filters:maxResults:)` on the AUTHOR_KINDS index. This eliminates the stale-hex failure mode where relay add/remove operations would silently fall back to bootstrap or year-old relay lists. Add an in-memory `lastSetRelayList` cache to bridge the nostrdb async write gap — `set()` populates it, `getUserCurrentRelayList()` checks it first, and `load()` clears it once ndb has committed. Remove `latestRelayListEventIdHex` from: Delegate protocol, DamusState, SaveKeysView, UserSettingsStore, and all test mocks. Includes 5 regression tests that fail before the fix and pass after: - testRemoveRelayDoesNotFallBackToBootstrapList - testCacheBridgesAsyncWriteGap - testRapidSequentialRemovesDoNotReintroduceRelays - testLoadClearsCacheAndReadsFromNdb - testNdbQueryFindsRelayListWithoutStoredHex Changelog-Fixed: Fix stale relay list causing inability to disconnect relays Closes: #3537 Co-Authored-By: Claude Opus 4.6 Signed-off-by: alltheseas --- .../NostrNetworkManager.swift | 5 +- .../UserRelayListManager.swift | 30 ++- damus/Core/Storage/DamusState.swift | 5 - .../Onboarding/Views/SaveKeysView.swift | 1 - .../Settings/Models/UserSettingsStore.swift | 4 - damusTests/EntityPreloaderTests.swift | 1 - .../NostrNetworkManagerTests.swift | 199 +++++++++++++++++- .../SubscriptionManagerNegentropyTests.swift | 1 - 8 files changed, 219 insertions(+), 27 deletions(-) diff --git a/damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift b/damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift index 959bbdbd..57bf423c 100644 --- a/damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift +++ b/damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift @@ -304,10 +304,7 @@ extension NostrNetworkManager { /// The keypair to use for relay authentication and updating relay lists var keypair: Keypair { get } - - /// The latest relay list event id hex - var latestRelayListEventIdHex: String? { get set } // TODO: Update this once we have full NostrDB query support - + /// The latest contact list `NostrEvent` /// /// Note: Read-only access, because `NostrNetworkManager` does not manage contact lists. diff --git a/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift b/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift index ee7b31b5..9435c922 100644 --- a/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift +++ b/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift @@ -18,9 +18,14 @@ extension NostrNetworkManager { private var delegate: Delegate private let pool: RelayPool private let reader: SubscriptionManager - + private var relayListObserverTask: Task? = nil private var walletUpdatesObserverTask: AnyCancellable? = nil + + /// In-memory cache of the most recently set relay list. + /// Bridges the gap between sending an event to nostrdb (async write) and it being queryable. + @MainActor + private var lastSetRelayList: NIP65.RelayList? init(delegate: Delegate, pool: RelayPool, reader: SubscriptionManager) { self.delegate = delegate @@ -60,9 +65,11 @@ extension NostrNetworkManager { /// Gets the user's current relay list. /// - /// It attempts to get a NIP-65 relay list from the local database, or falls back to a legacy list. + /// It attempts to get the in-memory cache first (to bridge the nostrdb async write gap), + /// then a NIP-65 relay list from the local database, or falls back to a legacy list. @MainActor func getUserCurrentRelayList() -> NIP65.RelayList? { + if let lastSetRelayList { return lastSetRelayList } if let latestRelayListEvent = try? self.getLatestNIP65RelayList() { return latestRelayListEvent } if let latestRelayListEvent = try? self.getLatestKind3RelayList() { return latestRelayListEvent } if let latestRelayListEvent = try? self.getLatestUserDefaultsRelayList() { return latestRelayListEvent } @@ -80,17 +87,18 @@ extension NostrNetworkManager { return list } - /// Gets the latest NIP-65 relay list event from NostrDB. - /// + /// Gets the latest NIP-65 relay list event from NostrDB via query. + /// /// This is `private` because it is part of internal logic. Callers should use the higher level functions. /// /// It is recommended to use this function only if the NostrEvent metadata is needed. For cases where only the relay list info is needed, use `getLatestNIP65RelayList` instead. /// /// - Returns: The latest NIP-65 relay list NdbNote private func getLatestNIP65RelayListEvent() -> NdbNote? { - guard let latestRelayListEventId = delegate.latestRelayListEventIdHex else { return nil } - guard let latestRelayListEventId = NoteId(hex: latestRelayListEventId) else { return nil } - return try? delegate.ndb.lookup_note_and_copy(latestRelayListEventId) + let filter = NostrFilter(kinds: [.relay_list], limit: 1, authors: [delegate.keypair.pubkey]) + guard let ndbFilter = try? NdbFilter(from: filter) else { return nil } + guard let noteKey = try? delegate.ndb.query(filters: [ndbFilter], maxResults: 1).first else { return nil } + return try? delegate.ndb.lookup_note_by_key_and_copy(noteKey) } /// Gets the latest `kind:3` relay list from NostrDB. @@ -176,17 +184,19 @@ extension NostrNetworkManager { func set(userRelayList: NIP65.RelayList) async throws(UpdateError) { guard let fullKeypair = delegate.keypair.to_full() else { throw .notAuthorizedToChangeRelayList } guard let relayListEvent = userRelayList.toNostrEvent(keypair: fullKeypair) else { throw .cannotFormRelayListEvent } - + + await MainActor.run { self.lastSetRelayList = userRelayList } + await self.apply(newRelayList: self.computeRelaysToConnectTo(with: userRelayList)) - + await self.pool.send(.event(relayListEvent)) // This will send to NostrDB as well, which will locally save that NIP-65 event - self.delegate.latestRelayListEventIdHex = relayListEvent.id.hex() // Make sure we are able to recall this event from NostrDB } // MARK: - Syncing our saved user relay list with the active `RelayPool` /// Loads the current user relay list func load() async { + await MainActor.run { self.lastSetRelayList = nil } // Clear cache; ndb has had time to commit by now await self.apply(newRelayList: self.relaysToConnectTo()) } diff --git a/damus/Core/Storage/DamusState.swift b/damus/Core/Storage/DamusState.swift index e4968263..0b4dbb84 100644 --- a/damus/Core/Storage/DamusState.swift +++ b/damus/Core/Storage/DamusState.swift @@ -230,11 +230,6 @@ fileprivate extension DamusState { var ndb: Ndb var keypair: Keypair - var latestRelayListEventIdHex: String? { - get { self.settings.latestRelayListEventIdHex } - set { self.settings.latestRelayListEventIdHex = newValue } - } - @MainActor var latestContactListEvent: NostrEvent? { self.contacts.event } var bootstrapRelays: [RelayURL] { get_default_bootstrap_relays() } diff --git a/damus/Features/Onboarding/Views/SaveKeysView.swift b/damus/Features/Onboarding/Views/SaveKeysView.swift index 8aeb6ee0..16c21762 100644 --- a/damus/Features/Onboarding/Views/SaveKeysView.swift +++ b/damus/Features/Onboarding/Views/SaveKeysView.swift @@ -166,7 +166,6 @@ struct SaveKeysView: View { // Save the ID to user settings so that we can easily find it later. let settings = UserSettingsStore.globally_load_for(pubkey: account.pubkey) settings.latest_contact_event_id_hex = first_contact_event.id.hex() - settings.latestRelayListEventIdHex = first_relay_list_event.id.hex() } func handle_event(relay: RelayURL, ev: NostrConnectionEvent) async { diff --git a/damus/Features/Settings/Models/UserSettingsStore.swift b/damus/Features/Settings/Models/UserSettingsStore.swift index 0f94ad27..e6f203e1 100644 --- a/damus/Features/Settings/Models/UserSettingsStore.swift +++ b/damus/Features/Settings/Models/UserSettingsStore.swift @@ -411,10 +411,6 @@ class UserSettingsStore: ObservableObject { @Setting(key: "draft_event_ids", default_value: nil) var draft_event_ids: [String]? - // TODO: Get rid of this once we have NostrDB query capabilities integrated - @Setting(key: "latest_relay_list_event_id", default_value: nil) - var latestRelayListEventIdHex: String? - // MARK: Helper types enum NotificationsMode: String, CaseIterable, Identifiable, StringCodable, Equatable { diff --git a/damusTests/EntityPreloaderTests.swift b/damusTests/EntityPreloaderTests.swift index e4e8e105..4c18c4e6 100644 --- a/damusTests/EntityPreloaderTests.swift +++ b/damusTests/EntityPreloaderTests.swift @@ -869,7 +869,6 @@ final class EntityPreloaderTests: XCTestCase { private final class TestNetworkDelegate: NostrNetworkManager.Delegate { var ndb: Ndb var keypair: Keypair - var latestRelayListEventIdHex: String? var latestContactListEvent: NostrEvent? var bootstrapRelays: [RelayURL] var developerMode: Bool = false diff --git a/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift b/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift index 90c3dcb4..376b4ea0 100644 --- a/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift +++ b/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift @@ -192,6 +192,204 @@ class NostrNetworkManagerTests: XCTestCase { XCTAssertEqual(manager.setCallCount, 1) XCTAssertEqual(manager.appliedRelayLists.first?.relays.count, validRelayList.relays.count) } + + // MARK: - Relay list stale-data regression tests + + /// Regression: removing a relay must not fall back to bootstrap relays. + /// + /// Before the fix, `getLatestNIP65RelayListEvent()` used a UserDefaults hex lookup + /// that could go stale, causing `getUserCurrentRelayList()` to return nil and + /// `remove()` to throw `.noInitialRelayList`. The user could never disconnect a relay. + func testRemoveRelayDoesNotFallBackToBootstrapList() async throws { + let ndb = Ndb.test + defer { ndb.close() } + + let relayA = RelayURL("wss://relay-a.example.com")! + let relayB = RelayURL("wss://relay-b.example.com")! + let relayC = RelayURL("wss://relay-c.example.com")! + let bootstrapRelay = RelayURL("wss://bootstrap.example.com")! + + let initialList = NIP65.RelayList(relays: [relayA, relayB, relayC]) + let initialEvent = initialList.toNostrEvent(keypair: test_keypair_full)! + let eventJson = encode_json(initialEvent)! + let processed = ndb.processEvent("[\"EVENT\",\"subid\",\(eventJson)]") + XCTAssertTrue(processed, "Failed to process relay list event into ndb") + try await Task.sleep(for: .milliseconds(100)) + + let delegate = MockNetworkDelegate( + ndb: ndb, + keypair: test_keypair, + bootstrapRelays: [bootstrapRelay] + ) + let pool = RelayPool(ndb: nil, keypair: test_keypair) + let reader = MockSubscriptionManager(pool: pool, ndb: ndb) + let manager = NostrNetworkManager.UserRelayListManager( + delegate: delegate, pool: pool, reader: reader + ) + + // Remove relay B from [A, B, C] — should yield [A, C] + try await manager.remove(relayURL: relayB) + + let currentList = await manager.getUserCurrentRelayList() + XCTAssertNotNil(currentList, "Relay list must not be nil after remove") + let urls = Set(currentList!.relays.keys) + XCTAssertEqual(urls, [relayA, relayC], "List should be [A, C] after removing B") + XCTAssertFalse(urls.contains(relayB), "Removed relay B must not be present") + XCTAssertFalse(urls.contains(bootstrapRelay), "Must not fall back to bootstrap relays") + } + + /// Regression: the in-memory cache must bridge the nostrdb async write gap. + /// + /// After `set()`, `getUserCurrentRelayList()` must immediately return the new list, + /// even before nostrdb's async worker has committed the event. + func testCacheBridgesAsyncWriteGap() async throws { + let ndb = Ndb.test + defer { ndb.close() } + + let delegate = MockNetworkDelegate( + ndb: ndb, + keypair: test_keypair, + bootstrapRelays: [RelayURL("wss://bootstrap.example.com")!] + ) + let pool = RelayPool(ndb: nil, keypair: test_keypair) + let reader = MockSubscriptionManager(pool: pool, ndb: ndb) + let manager = NostrNetworkManager.UserRelayListManager( + delegate: delegate, pool: pool, reader: reader + ) + + let relayA = RelayURL("wss://relay-a.example.com")! + let relayC = RelayURL("wss://relay-c.example.com")! + let newList = NIP65.RelayList(relays: [relayA, relayC]) + + // set() should populate the cache immediately + try await manager.set(userRelayList: newList) + + // Query immediately — no sleep — ndb may not have committed yet + let currentList = await manager.getUserCurrentRelayList() + XCTAssertNotNil(currentList, "Cache must serve the list immediately after set()") + XCTAssertEqual(Set(currentList!.relays.keys), [relayA, relayC]) + } + + /// Regression: rapid sequential removes must not reintroduce removed relays. + /// + /// Scenario: start with [A, B, C], remove B, then immediately remove C. + /// Without the cache, the second `remove()` might read a stale [A, B, C] from ndb + /// (because the first set hasn't committed yet), producing [A, B] — relay B is back. + func testRapidSequentialRemovesDoNotReintroduceRelays() async throws { + let ndb = Ndb.test + defer { ndb.close() } + + let relayA = RelayURL("wss://relay-a.example.com")! + let relayB = RelayURL("wss://relay-b.example.com")! + let relayC = RelayURL("wss://relay-c.example.com")! + + let initialList = NIP65.RelayList(relays: [relayA, relayB, relayC]) + let initialEvent = initialList.toNostrEvent(keypair: test_keypair_full)! + let eventJson = encode_json(initialEvent)! + XCTAssertTrue(ndb.processEvent("[\"EVENT\",\"subid\",\(eventJson)]")) + try await Task.sleep(for: .milliseconds(100)) + + let delegate = MockNetworkDelegate( + ndb: ndb, + keypair: test_keypair, + bootstrapRelays: [] + ) + let pool = RelayPool(ndb: nil, keypair: test_keypair) + let reader = MockSubscriptionManager(pool: pool, ndb: ndb) + let manager = NostrNetworkManager.UserRelayListManager( + delegate: delegate, pool: pool, reader: reader + ) + + // Remove B then immediately remove C — no sleep between + try await manager.remove(relayURL: relayB) + try await manager.remove(relayURL: relayC) + + let currentList = await manager.getUserCurrentRelayList() + XCTAssertNotNil(currentList) + let urls = Set(currentList!.relays.keys) + XCTAssertEqual(urls, [relayA], "Only relay A should remain after removing B and C") + XCTAssertFalse(urls.contains(relayB), "Relay B must not reappear after sequential removes") + XCTAssertFalse(urls.contains(relayC), "Relay C must stay removed") + } + + /// Verify that `load()` clears the in-memory cache so ndb becomes the source of truth again. + func testLoadClearsCacheAndReadsFromNdb() async throws { + let ndb = Ndb.test + defer { ndb.close() } + + let relayA = RelayURL("wss://relay-a.example.com")! + let relayB = RelayURL("wss://relay-b.example.com")! + + // Seed ndb with [A, B] + let ndbList = NIP65.RelayList(relays: [relayA, relayB]) + let ndbEvent = ndbList.toNostrEvent(keypair: test_keypair_full)! + XCTAssertTrue(ndb.processEvent("[\"EVENT\",\"subid\",\(encode_json(ndbEvent)!)]")) + try await Task.sleep(for: .milliseconds(100)) + + let delegate = MockNetworkDelegate( + ndb: ndb, + keypair: test_keypair, + bootstrapRelays: [] + ) + let pool = RelayPool(ndb: nil, keypair: test_keypair) + let reader = MockSubscriptionManager(pool: pool, ndb: ndb) + let manager = NostrNetworkManager.UserRelayListManager( + delegate: delegate, pool: pool, reader: reader + ) + + // set() populates cache with [A] only + try await manager.set(userRelayList: NIP65.RelayList(relays: [relayA])) + let cachedList = await manager.getUserCurrentRelayList() + XCTAssertEqual(Set(cachedList!.relays.keys), [relayA], "Cache should serve [A]") + + // load() must clear the cache so ndb is queried again + await manager.load() + + // After load(), the list should come from ndb. + // ndb now has the [A] event from set() (committed by now) or the original [A,B]. + // Either way, the cache is cleared — the manager reads from ndb, not stale cache. + let afterLoad = await manager.getUserCurrentRelayList() + XCTAssertNotNil(afterLoad, "Must return a relay list from ndb after load()") + } + + /// Regression: ndb query must find the relay list without a stored hex ID. + /// + /// Before the fix, a fresh session with no `latestRelayListEventIdHex` in UserDefaults + /// meant `getLatestNIP65RelayListEvent()` returned nil, even though ndb had the event. + func testNdbQueryFindsRelayListWithoutStoredHex() async throws { + let ndb = Ndb.test + defer { ndb.close() } + + let relayA = RelayURL("wss://relay-a.example.com")! + let relayB = RelayURL("wss://relay-b.example.com")! + + // Seed ndb — simulates a relay list that arrived via sync, not user action + let list = NIP65.RelayList(relays: [relayA, relayB]) + let event = list.toNostrEvent(keypair: test_keypair_full)! + XCTAssertTrue(ndb.processEvent("[\"EVENT\",\"subid\",\(encode_json(event)!)]")) + try await Task.sleep(for: .milliseconds(100)) + + // No hex stored anywhere — fresh delegate with no latestRelayListEventIdHex + let delegate = MockNetworkDelegate( + ndb: ndb, + keypair: test_keypair, + bootstrapRelays: [RelayURL("wss://bootstrap.example.com")!] + ) + let pool = RelayPool(ndb: nil, keypair: test_keypair) + let reader = MockSubscriptionManager(pool: pool, ndb: ndb) + let manager = NostrNetworkManager.UserRelayListManager( + delegate: delegate, pool: pool, reader: reader + ) + + let currentList = await manager.getUserCurrentRelayList() + XCTAssertNotNil(currentList, "ndb query must find relay list without stored hex") + let urls = Set(currentList!.relays.keys) + XCTAssertEqual(urls, [relayA, relayB]) + XCTAssertFalse( + urls.contains(RelayURL("wss://bootstrap.example.com")!), + "Must not fall back to bootstrap when ndb has the event" + ) + } } // MARK: - Test doubles @@ -199,7 +397,6 @@ class NostrNetworkManagerTests: XCTestCase { private final class MockNetworkDelegate: NostrNetworkManager.Delegate { var ndb: Ndb var keypair: Keypair - var latestRelayListEventIdHex: String? var latestContactListEvent: NostrEvent? var bootstrapRelays: [RelayURL] var developerMode: Bool = false diff --git a/damusTests/SubscriptionManagerNegentropyTests.swift b/damusTests/SubscriptionManagerNegentropyTests.swift index 163fed60..ec18ae0e 100644 --- a/damusTests/SubscriptionManagerNegentropyTests.swift +++ b/damusTests/SubscriptionManagerNegentropyTests.swift @@ -520,7 +520,6 @@ final class SubscriptionManagerNegentropyTests: XCTestCase { private final class TestNetworkDelegate: NostrNetworkManager.Delegate { var ndb: Ndb var keypair: Keypair - var latestRelayListEventIdHex: String? var latestContactListEvent: NostrEvent? var bootstrapRelays: [RelayURL] var developerMode: Bool = false