From 0b007eee1ae1d81c797ce0b5ca233f7267e1f5ab Mon Sep 17 00:00:00 2001 From: jack Date: Thu, 11 Jun 2026 21:29:08 +0200 Subject: [PATCH] Close the final re-grade findings: parked EOSE fallback, panic atomicity, overflow visibility EOSE callbacks parked while Tor is bootstrapping now get a fallback unblock at the standard 10s EOSE timeout (via the injected scheduler, generation-guarded, single-fire) instead of waiting up to ~225s for Tor-readiness retry exhaustion. The identity swap in refreshPeerIdentity runs inside a messageQueue barrier with re-entrancy guard, so a panic reset can no longer race in-flight packet builds (deadlock analysis documented; both call paths verified off-queue). Relay send-queue overflow drops now log a sampled warning. Co-Authored-By: Claude Fable 5 --- bitchat/Nostr/NostrRelayManager.swift | 42 ++++++++- bitchat/Services/BLE/BLEService.swift | 30 ++++-- bitchat/Services/TransportConfig.swift | 3 + bitchatTests/BLEServiceCoreTests.swift | 17 ++++ .../Services/NostrRelayManagerTests.swift | 93 +++++++++++++++++++ 5 files changed, 178 insertions(+), 7 deletions(-) diff --git a/bitchat/Nostr/NostrRelayManager.swift b/bitchat/Nostr/NostrRelayManager.swift index aa9562c4..91cfaec4 100644 --- a/bitchat/Nostr/NostrRelayManager.swift +++ b/bitchat/Nostr/NostrRelayManager.swift @@ -198,6 +198,9 @@ final class NostrRelayManager: ObservableObject { } private var messageQueue: [PendingSend] = [] private let messageQueueLock = NSLock() + // Total pending sends dropped at the queue cap; drives the sampled + // overflow warning (first + every Nth drop). + private var pendingSendDropCount = 0 private let encoder = JSONEncoder() private var shouldUseTor: Bool { dependencies.userTorEnabled() } @@ -352,6 +355,18 @@ final class NostrRelayManager: ObservableObject { messageQueue.removeFirst(overflow) } messageQueueLock.unlock() + guard overflow > 0 else { return } + // Dropped events are ephemeral (presence/geo), so no status surfacing + // is needed — but the drops should be visible. Sampled so a sustained + // relay stall can't flood the log. + pendingSendDropCount += overflow + if pendingSendDropCount == 1 || + pendingSendDropCount.isMultiple(of: TransportConfig.nostrPendingSendDropLogInterval) { + SecureLogger.warning( + "📤 Relay send queue full — dropped \(pendingSendDropCount) oldest event(s)", + category: .session + ) + } } /// Try to flush any queued messages for relays that are now connected. @@ -452,7 +467,7 @@ final class NostrRelayManager: ObservableObject { if urls.isEmpty { onEOSE() } else if shouldWaitForTorBeforeConnecting { - pendingEOSECallbacks[id] = onEOSE + parkEOSECallbackUntilTorReady(id: id, callback: onEOSE) } else { startEOSETracking(id: id, relayURLs: Set(urls), callback: onEOSE) } @@ -623,6 +638,31 @@ final class NostrRelayManager: ObservableObject { } } + /// Park an EOSE callback while Tor is not yet ready, and schedule the same + /// fallback timeout `startEOSETracking` uses. Without it, a parked callback + /// would only be unblocked by Tor-readiness retry exhaustion (several + /// awaitReady timeouts, i.e. minutes), leaving callers hanging far past the + /// normal EOSE fallback. If Tor recovers first the callback is promoted to + /// a real EOSE tracker (`startPendingEOSETrackingIfNeeded`), and if retry + /// exhaustion fires first it is drained by `unblockPendingEOSECallbacks`; + /// either way it leaves `pendingEOSECallbacks` and this timer is a no-op. + private func parkEOSECallbackUntilTorReady(id: String, callback: @escaping () -> Void) { + pendingEOSECallbacks[id] = callback + let generation = connectionGeneration + dependencies.scheduleAfter(TransportConfig.nostrSubscriptionEOSEFallbackSeconds) { [weak self] in + Task { @MainActor [weak self] in + guard let self else { return } + // Stale timers from a previous connection generation are void. + guard generation == self.connectionGeneration else { return } + // Already fired (unsubscribe, retry-exhaustion unblock) or + // promoted to a real EOSE tracker: nothing to do. + guard let callback = self.pendingEOSECallbacks.removeValue(forKey: id) else { return } + SecureLogger.warning("Unblocking Tor-parked EOSE callback for \(id) after fallback timeout", category: .session) + callback() + } + } + } + /// Fire and clear all EOSE callbacks that are parked waiting for Tor. /// Callers treat EOSE as "initial fetch finished"; firing with no data is /// safe and prevents indefinite hangs when Tor cannot bootstrap. diff --git a/bitchat/Services/BLE/BLEService.swift b/bitchat/Services/BLE/BLEService.swift index 596415f4..f761d88d 100644 --- a/bitchat/Services/BLE/BLEService.swift +++ b/bitchat/Services/BLE/BLEService.swift @@ -74,6 +74,8 @@ final class BLEService: NSObject { private let identityManager: SecureIdentityStateManagerProtocol private let keychain: KeychainManagerProtocol private let idBridge: NostrIdentityBridge + /// Binary form of `myPeerID`; same contract — mutated only inside a + /// `messageQueue` barrier via `refreshPeerIdentity()`. private var myPeerIDData: Data = Data() // MARK: - Advertising Privacy @@ -407,8 +409,10 @@ final class BLEService: NSObject { // MARK: Identity /// Derived from the Noise identity fingerprint; rotated only via - /// `refreshPeerIdentity()` (e.g. panic reset). Externally read-only — - /// no out-of-band mutation may bypass that derivation. + /// `refreshPeerIdentity()` (e.g. panic reset), which performs the swap + /// inside a `messageQueue` barrier so concurrent queue work never sees a + /// half-updated identity. Externally read-only — no out-of-band mutation + /// may bypass that derivation. private(set) var myPeerID = PeerID(str: "") /// Externally read-only; mutate via `setNickname(_:)`, which also /// broadcasts the change to peers. @@ -2370,11 +2374,25 @@ extension BLEService { } } + /// Swaps `myPeerID`/`myPeerIDData` to match the current Noise identity. + /// The swap runs as a `messageQueue` barrier so in-flight work items that + /// read the identity (e.g. `sendMessage` building packets) complete + /// against the old value and everything after sees the new one atomically. + /// Callers (init, panic reset on the main thread) are never on + /// `messageQueue`; the re-entrancy check keeps any future on-queue caller + /// from deadlocking. private func refreshPeerIdentity() { - let fingerprint = noiseService.getIdentityFingerprint() - myPeerID = PeerID(str: fingerprint.prefix(16)) - myPeerIDData = Data(hexString: myPeerID.id) ?? Data() - meshTopology.reset() + let swap = { + let fingerprint = self.noiseService.getIdentityFingerprint() + self.myPeerID = PeerID(str: fingerprint.prefix(16)) + self.myPeerIDData = Data(hexString: self.myPeerID.id) ?? Data() + self.meshTopology.reset() + } + if DispatchQueue.getSpecific(key: messageQueueKey) != nil { + swap() + } else { + messageQueue.sync(flags: .barrier, execute: swap) + } } diff --git a/bitchat/Services/TransportConfig.swift b/bitchat/Services/TransportConfig.swift index 02ccfe4e..606185bd 100644 --- a/bitchat/Services/TransportConfig.swift +++ b/bitchat/Services/TransportConfig.swift @@ -167,6 +167,9 @@ enum TransportConfig { // bootstrap deadline) to attempt before unblocking pending EOSE callers. static let nostrTorReadyMaxWaitAttempts: Int = 3 static let nostrPendingSendQueueCap: Int = 200 + // Sample interval for the send-queue overflow warning (first + every Nth + // dropped event). Drops are ephemeral presence/geo traffic — log-only. + static let nostrPendingSendDropLogInterval: Int = 10 // Pending (not-yet-flushed) REQs are bounded per relay: oldest-by-insertion // eviction at the cap, plus an age sweep on connect attempts. Durable // subscription intent survives in subscriptionRequestState either way. diff --git a/bitchatTests/BLEServiceCoreTests.swift b/bitchatTests/BLEServiceCoreTests.swift index 9b5155b2..992eb647 100644 --- a/bitchatTests/BLEServiceCoreTests.swift +++ b/bitchatTests/BLEServiceCoreTests.swift @@ -164,6 +164,23 @@ struct BLEServiceCoreTests { #expect(!ble._test_recordIngressIfNew(packet: packet, linkID: "central-b")) } + @Test + func panicReset_rotatesPeerIDDerivedFromNewNoiseFingerprint() async throws { + let ble = makeService() + let originalPeerID = ble.myPeerID + let originalFingerprint = ble.noiseIdentityFingerprint() + #expect(originalPeerID == PeerID(str: originalFingerprint.prefix(16))) + + ble.resetIdentityForPanic(currentNickname: "anon") + + // The Noise identity is regenerated and the peer ID swaps with it + // (atomically, behind a messageQueue barrier). + let newFingerprint = ble.noiseIdentityFingerprint() + #expect(newFingerprint != originalFingerprint) + #expect(ble.myPeerID != originalPeerID) + #expect(ble.myPeerID == PeerID(str: newFingerprint.prefix(16))) + } + @Test func modifiedServices_rediscoverWhenBitChatServiceIsInvalidated() async throws { let otherService = CBUUID(string: "0000180F-0000-1000-8000-00805F9B34FB") diff --git a/bitchatTests/Services/NostrRelayManagerTests.swift b/bitchatTests/Services/NostrRelayManagerTests.swift index c6271973..8184564a 100644 --- a/bitchatTests/Services/NostrRelayManagerTests.swift +++ b/bitchatTests/Services/NostrRelayManagerTests.swift @@ -133,6 +133,99 @@ final class NostrRelayManagerTests: XCTestCase { XCTAssertTrue(context.sessionFactory.requestedURLs.isEmpty) } + func test_subscribe_parkedEOSEFiresAfterFallbackTimeoutWhenTorNeverBecomesReady() async { + let relayURL = "wss://tor-eose-parked-fallback.example" + let context = makeContext(permission: .denied, userTorEnabled: true, torEnforced: true, torIsReady: false) + var eoseCount = 0 + + context.manager.subscribe( + filter: makeFilter(), + id: "tor-parked-fallback", + relayUrls: [relayURL], + handler: { _ in }, + onEOSE: { eoseCount += 1 } + ) + + // Parking the callback schedules the normal EOSE fallback, not just + // the Tor retry-exhaustion unblock (~minutes later). + XCTAssertEqual(context.scheduler.scheduled.first?.delay, TransportConfig.nostrSubscriptionEOSEFallbackSeconds) + XCTAssertEqual(eoseCount, 0) + + context.scheduler.runNext() + let unblocked = await waitUntil { eoseCount == 1 } + XCTAssertTrue(unblocked) + XCTAssertTrue(context.sessionFactory.requestedURLs.isEmpty) + + // A later retry-exhaustion unblock must not fire the callback again. + for _ in 0..