From 623f3855c5fcd9736427060554b5db9ba3330460 Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 12:28:02 -0700 Subject: [PATCH 1/6] Flush the stable-key outbox on reconnect and authentication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-scoped to the half of #1408 that is still a real bug, as the review asked. A DM composed while the recipient is an offline favorite is queued in the router outbox under their stable 64-hex Noise key. `flushOutbox` is keyed by exactly the id it is handed, and the reconnect path only ever handed it the short 16-hex id, so that queue was never reached. #1462's retry does not cover it either — that filters on `sendAttempts > 0`, and this mail was never transmitted at all. Absent a courier it waited for an app relaunch, a favorite-status change, or the 24h TTL. Connect now resolves the stable key (unified peer, else the cache, else the Noise session key, mirroring the disconnect path) and flushes it alongside the short id. Authentication does the same, once the link is authenticated and the identity is known. Dropped from the original PR: the ChatPrivateConversationCoordinator send-path changes and the mutual-favorite guard on offline Nostr delivery. #1415 rewrote that block — "always hand the message to the router; it owns delivery" — and removed the very gate those commits preserved, so both are superseded rather than merged. Rebuilt on current main instead of resolving four conflicts in code that no longer needed to change. Three tests, and one existing expectation updated because the behaviour it pinned is the bug: a connect flushes both queues, the stable key still resolves when unified-peer state has not populated yet (the case the fix exists for), and an unresolvable peer issues no bogus second flush. Mutation-verified: removing the stable-key flush fails all three. Co-Authored-By: Claude Opus 5 (1M context) --- .../ChatTransportEventCoordinator.swift | 24 ++++++- .../ChatVerificationCoordinator.swift | 24 +++++-- ...ransportEventCoordinatorContextTests.swift | 72 ++++++++++++++++++- ...tVerificationCoordinatorContextTests.swift | 3 + 4 files changed, 116 insertions(+), 7 deletions(-) diff --git a/bitchat/ViewModels/ChatTransportEventCoordinator.swift b/bitchat/ViewModels/ChatTransportEventCoordinator.swift index ee2d8b6c..ce705fa8 100644 --- a/bitchat/ViewModels/ChatTransportEventCoordinator.swift +++ b/bitchat/ViewModels/ChatTransportEventCoordinator.swift @@ -274,13 +274,33 @@ final class ChatTransportEventCoordinator { context.registerEphemeralSession(peerID: peerID) context.notifyUIChanged() + // Resolve the stable key robustly: unified-peer state may not be + // populated yet at connect time, so fall back to the cache and then to + // the noise session key (mirroring the disconnect path). + var stablePeerID: PeerID? if let peer = context.unifiedPeer(for: peerID) { - let stablePeerID = PeerID(hexData: peer.noisePublicKey) - context.cacheStablePeerID(stablePeerID, for: peerID) + let resolved = PeerID(hexData: peer.noisePublicKey) + context.cacheStablePeerID(resolved, for: peerID) + stablePeerID = resolved + } else if let cached = context.cachedStablePeerID(for: peerID) { + stablePeerID = cached + } else if let key = context.noiseSessionPublicKeyData(for: peerID) { + let derived = PeerID(hexData: key) + context.cacheStablePeerID(derived, for: peerID) + stablePeerID = derived } context.flushRouterOutbox(for: peerID) context.retryCourierDeposits(via: peerID) + + // Also flush under the stable 64-hex key. `flushOutbox` is keyed by + // exactly the id it is handed, and a DM composed while the recipient + // was an offline favorite is queued under their stable key — so the + // short-id flush above never reaches it, and absent a courier it waits + // for a relaunch, a favorite-status change, or the 24h TTL. + if let stablePeerID, stablePeerID != peerID { + context.flushRouterOutbox(for: stablePeerID) + } } func didDisconnectFromPeer(_ peerID: PeerID) { diff --git a/bitchat/ViewModels/ChatVerificationCoordinator.swift b/bitchat/ViewModels/ChatVerificationCoordinator.swift index 2de291ff..5ab89d54 100644 --- a/bitchat/ViewModels/ChatVerificationCoordinator.swift +++ b/bitchat/ViewModels/ChatVerificationCoordinator.swift @@ -45,6 +45,9 @@ protocol ChatVerificationContext: AnyObject { func resolveNickname(for peerID: PeerID) -> String func cachedStablePeerID(for shortPeerID: PeerID) -> PeerID? func cacheStablePeerID(_ stablePeerID: PeerID, for shortPeerID: PeerID) + /// Flushes the message router's disk outbox for the given (stable) key so + /// mail queued while the peer was offline delivers once it authenticates. + func flushRouterOutbox(for peerID: PeerID) // MARK: Noise sessions & verification transport /// Installs the Noise service's session callbacks (single registration point). @@ -78,7 +81,8 @@ extension ChatViewModel: ChatVerificationContext { // `isVerifiedFingerprint(_:)`, `setEncryptionStatus(_:for:)`, // `resolveNickname(for:)`, `cachedStablePeerID(for:)`, // `cacheStablePeerID(_:for:)`, `noiseSessionPublicKeyData(for:)`, - // `hasEstablishedNoiseSession(with:)`, and `triggerHandshake(with:)` are + // `hasEstablishedNoiseSession(with:)`, `triggerHandshake(with:)`, and + // `flushRouterOutbox(for:)` (shared with `ChatTransportEventContext`) are // shared requirements with the other contexts or satisfied by existing // `ChatViewModel` members. The members below flatten nested service // accesses into intent-named calls. @@ -245,9 +249,9 @@ final class ChatVerificationCoordinator { // retry it now that this newly authenticated/replacement // session can actually decrypt it. var peerIDAliases = [peerID] - if let stablePeerID = authenticatedStablePeerID - ?? self.context.cachedStablePeerID(for: peerID), - stablePeerID != peerID { + let stablePeerID = authenticatedStablePeerID + ?? self.context.cachedStablePeerID(for: peerID) + if let stablePeerID, stablePeerID != peerID { // Conversations can migrate from the ephemeral BLE ID // to the authenticated Noise-key ID. Retry both aliases // because either may own the retained outbox entry. @@ -255,6 +259,18 @@ final class ChatVerificationCoordinator { } self.context.retrySecurePrivateMessagesAfterAuthentication(for: peerIDAliases) + // The retry above only reaches messages already transmitted + // through a secure session (it filters on `sendAttempts > 0`). + // A DM composed while this peer was offline was never + // transmitted at all — it sits in the outbox under the + // stable 64-hex key — so it is in neither that set nor the + // short-id flush on connect. Flush it here, now that the + // link is authenticated and the stable identity is known, + // rather than leaving it for the TTL. + if let stablePeerID, stablePeerID != peerID { + self.context.flushRouterOutbox(for: stablePeerID) + } + if var pending = self.pendingQRVerifications[peerID], pending.sent == false { self.context.sendVerifyChallenge( to: peerID, diff --git a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift index aa02b255..fc7d4f7d 100644 --- a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift +++ b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift @@ -343,7 +343,10 @@ struct ChatTransportEventCoordinatorContextTests { #expect(context.isConnected) #expect(context.registeredEphemeralSessions == [peerID]) #expect(context.stablePeerIDCache[peerID] == PeerID(hexData: noiseKey)) - #expect(context.flushedOutboxPeerIDs == [peerID]) + // Both queues: the short id, then the stable 64-hex key that offline- + // composed mail is queued under (#1408). Order matters only in that the + // short-id flush is not delayed behind the stable-key resolution. + #expect(context.flushedOutboxPeerIDs == [peerID, PeerID(hexData: noiseKey)]) #expect(context.notifyUIChangedCount == 1) // Their messages' read receipts are un-marked on disconnect so READ @@ -480,4 +483,71 @@ struct ChatTransportEventCoordinatorContextTests { #expect(context.handledPrivateMessages.count == 1) #expect(context.meshDeliveryAcks.count == 1) } + + /// #1408: a DM composed while the recipient was an offline favorite is + /// queued in the router outbox under the peer's STABLE 64-hex Noise key. + /// `flushOutbox` is keyed by exactly the id it is handed, so flushing only + /// the short 16-hex id on connect never reaches that queue — the mail waits + /// for an app relaunch, a favorite-status change, or the 24h TTL. + /// + /// Reconnect must flush both. + @Test @MainActor + func didConnectToPeer_flushesTheStableKeyOutboxAsWellAsTheShortID() { + let context = MockChatTransportEventContext() + let coordinator = ChatTransportEventCoordinator(context: context) + + let shortPeerID = PeerID(str: "1122334455667788") + let noiseKey = Data((0..<32).map { UInt8(0xB0 &+ $0) }) + let stablePeerID = PeerID(hexData: noiseKey) + #expect(stablePeerID != shortPeerID) + + // The peer is known by its Noise key, as it is after a handshake. + context.peersByID[shortPeerID] = BitchatPeer( + peerID: shortPeerID, + noisePublicKey: noiseKey, + nickname: "alice" + ) + + coordinator.didConnectToPeerSynchronously(shortPeerID) + + // Both queues are drained, and the stable key is the one that carries + // the offline-composed mail. + #expect(context.flushedOutboxPeerIDs.contains(shortPeerID)) + #expect( + context.flushedOutboxPeerIDs.contains(stablePeerID), + "offline-queued mail under the stable key was never flushed" + ) + } + + /// The stable key must still resolve when unified-peer state has not been + /// populated yet at connect time — otherwise the flush silently no-ops in + /// exactly the case it is for. Falls back to the cache, then to the Noise + /// session key. + @Test @MainActor + func didConnectToPeer_resolvesTheStableKeyWithoutUnifiedPeerState() { + let shortPeerID = PeerID(str: "1122334455667788") + let noiseKey = Data((0..<32).map { UInt8(0xC0 &+ $0) }) + let stablePeerID = PeerID(hexData: noiseKey) + + // Cache only. + let viaCache = MockChatTransportEventContext() + viaCache.cacheStablePeerID(stablePeerID, for: shortPeerID) + ChatTransportEventCoordinator(context: viaCache) + .didConnectToPeerSynchronously(shortPeerID) + #expect(viaCache.flushedOutboxPeerIDs.contains(stablePeerID)) + + // Noise session key only. + let viaSession = MockChatTransportEventContext() + viaSession.noiseSessionKeysByPeerID[shortPeerID] = noiseKey + ChatTransportEventCoordinator(context: viaSession) + .didConnectToPeerSynchronously(shortPeerID) + #expect(viaSession.flushedOutboxPeerIDs.contains(stablePeerID)) + + // Nothing resolvable: the short-id flush still happens, and no bogus + // second flush is issued. + let unresolvable = MockChatTransportEventContext() + ChatTransportEventCoordinator(context: unresolvable) + .didConnectToPeerSynchronously(shortPeerID) + #expect(unresolvable.flushedOutboxPeerIDs == [shortPeerID]) + } } diff --git a/bitchatTests/ChatVerificationCoordinatorContextTests.swift b/bitchatTests/ChatVerificationCoordinatorContextTests.swift index 83cc7296..d4502982 100644 --- a/bitchatTests/ChatVerificationCoordinatorContextTests.swift +++ b/bitchatTests/ChatVerificationCoordinatorContextTests.swift @@ -91,6 +91,9 @@ private final class MockChatVerificationContext: ChatVerificationContext { stablePeerIDCache[shortPeerID] = stablePeerID } + private(set) var flushedOutboxPeerIDs: [PeerID] = [] + func flushRouterOutbox(for peerID: PeerID) { flushedOutboxPeerIDs.append(peerID) } + // Noise sessions & verification transport var myNoiseStaticKey = Data(repeating: 0x42, count: 32) var establishedNoiseSessions: Set = [] From 9c473f38413b4ee423253ac9232d03a08aee09a2 Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 12:49:37 -0700 Subject: [PATCH 2/6] Merge the two outbox keys into one flush instead of draining them in turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flushing the short ID and then the stable key as two calls let newer mail overtake older mail: `flushOutbox(for:)` is keyed by exactly the ID it is handed, so the short-ID queue drained first and a message composed moments ago went out ahead of the offline-composed DM that had been waiting under the stable key. `retrySecurePrivateMessagesAfterAuthentication` already solved this shape for its own alias set; `flushOutbox(forAliases:)` now does the same for the flush — merge the queues, sort by timestamp, and send each message ID once no matter how many keys hold a copy. Two smaller repairs on the way: Prefer the live Noise session key over the stable-ID cache when unified-peer state is not populated yet. Short BLE IDs are ephemeral and get recycled, so a cache entry left by a previous owner of the same ID would name the wrong peer, and the flush would drain — and transmit under — their queue. The session key is the identity of the link we just brought up. Make the authentication-path flush disjoint from the retry that precedes it. On an authenticated link the flush is a strict superset of the retry, so without `skippingSecurelyTransmitted` every retried message went out twice back-to-back and burned two attempts against the cap. Each guard is mutation-proven: dropping the sort fails only the FIFO test (and shows the pre-fix order), neutralising the skip fails only the double-send test, preferring the cache fails only the recycled-ID test, and removing the authentication flush fails only its own test. 1953 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 245 +++++++++++++----- .../ChatTransportEventCoordinator.swift | 44 ++-- .../ChatVerificationCoordinator.swift | 35 +-- ...ransportEventCoordinatorContextTests.swift | 60 ++++- ...tVerificationCoordinatorContextTests.swift | 35 ++- .../Services/MessageRouterTests.swift | 84 ++++++ 6 files changed, 399 insertions(+), 104 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index a070c7bf..757fa8e5 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -697,77 +697,7 @@ final class MessageRouter { var outboxChanged = false for message in queued { - // A synchronous ack from an earlier send in this flush may have - // removed an entry from the live outbox. The snapshot is only an - // iteration order; never use it to recreate removed messages. - guard queuedMessage(message.messageID, for: peerID) != nil else { continue } - - // Skip expired messages (TTL exceeded) - if now.timeIntervalSince(message.timestamp) > Self.messageTTLSeconds { - SecureLogger.debug("⏰ Expired queued message for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))… (age: \(Int(now.timeIntervalSince(message.timestamp)))s)", category: .session) - if removeQueuedMessage(message.messageID, for: peerID) { - dropMessage(message.messageID, for: peerID) - outboxChanged = true - } - continue - } - - if let transport = connectedTransport(for: peerID), transport.canDeliverSecurely(to: peerID) { - // A secure session is meaningful enough to retry, but not - // proof that this particular ciphertext reached the peer: the - // remote app may have restarted while our old session still - // looked established. Retain until an ack, while bounding - // actual secure transmissions for peers that never ack. - guard message.sendAttempts < Self.maxSendAttempts else { - SecureLogger.warning("📤 Dropping unacked PM for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))… after \(message.sendAttempts) attempts", category: .session) - if removeQueuedMessage(message.messageID, for: peerID) { - dropMessage(message.messageID, for: peerID) - outboxChanged = true - } - continue - } - SecureLogger.debug("Outbox -> \(type(of: transport)) (connected) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session) - secureTransmissions.insert( - PeerMessageKey(peerID: peerID, messageID: message.messageID) - ) - transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) - metrics?.record(.outboxResent) - outboxChanged = incrementSendAttemptsIfQueued(message.messageID, for: peerID) || outboxChanged - } else if let transport = connectedTransport(for: peerID) { - // "Connected" without a secure session — possibly a stolen - // binding from a replayed announce: send (a genuine link - // finishes the handshake and delivers) but keep retaining - // until an ack clears it. These flushes do NOT count toward - // the attempt-cap drop: the message was transmitted over a - // live link, so a peer whose handshake stalls across - // reconnect flapping must not burn through the cap and lose - // the store-and-forward copy this retention exists to - // preserve. Retention stays bounded by the 24h outbox TTL - // and the per-peer FIFO cap. - SecureLogger.debug("Outbox -> \(type(of: transport)) (connected, no secure session) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session) - secureTransmissions.remove( - PeerMessageKey(peerID: peerID, messageID: message.messageID) - ) - transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) - metrics?.record(.outboxResent) - } else if let transport = reachableTransport(for: peerID) { - // Reachability without a connection is a freshness heuristic, - // so the send can silently go nowhere: send but keep retaining - // until an ack clears it, bounded by attempt count for peers - // that never ack. - guard message.sendAttempts < Self.maxSendAttempts else { - SecureLogger.warning("📤 Dropping unacked PM for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))… after \(message.sendAttempts) attempts", category: .session) - if removeQueuedMessage(message.messageID, for: peerID) { - dropMessage(message.messageID, for: peerID) - outboxChanged = true - } - continue - } - SecureLogger.debug("Outbox -> \(type(of: transport)) (reachable) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session) - transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) - metrics?.record(.outboxResent) - outboxChanged = incrementSendAttemptsIfQueued(message.messageID, for: peerID) || outboxChanged - } + outboxChanged = flushQueuedMessage(message, for: peerID, now: now) || outboxChanged } if outboxChanged { @@ -775,6 +705,179 @@ final class MessageRouter { } } + /// Flush several outbox keys belonging to the same peer as one + /// chronological stream. + /// + /// A conversation can leave retained mail split across the ephemeral BLE + /// ID and the stable Noise-key ID: a DM composed while the recipient was + /// an offline favorite is queued under the stable key, while mail composed + /// after they appeared is queued under the short one. `flushOutbox(for:)` + /// is keyed by exactly the ID it is handed, so draining the two keys one + /// after another would let newer mail on the first key go out ahead of + /// older mail on the second. Merge them the way + /// `retrySecurePrivateMessagesAfterAuthentication` already does, and send + /// each message ID once no matter how many keys hold a copy. + /// + /// Pass `skippingSecurelyTransmitted` when a caller has just run + /// `retrySecurePrivateMessagesAfterAuthentication` over the same aliases. + /// That retry covers exactly the entries in `secureTransmissions`, and on + /// an authenticated link this flush would otherwise be a strict superset + /// of it — re-sending every message the retry just put on the air and + /// burning a second attempt against the cap. Skipping that set makes the + /// two passes disjoint by construction rather than by comment. + func flushOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool = false) { + typealias Candidate = ( + peerID: PeerID, + message: QueuedMessage, + aliasOrder: Int, + queueOrder: Int + ) + + var visitedPeerIDs = Set() + var candidates: [Candidate] = [] + + for (aliasOrder, peerID) in peerIDAliases.enumerated() { + guard visitedPeerIDs.insert(peerID).inserted else { continue } + guard let queued = outbox[peerID], !queued.isEmpty else { continue } + for (queueOrder, message) in queued.enumerated() { + if skippingSecurelyTransmitted, + secureTransmissions.contains( + PeerMessageKey(peerID: peerID, messageID: message.messageID) + ) { + continue + } + candidates.append(( + peerID: peerID, + message: message, + aliasOrder: aliasOrder, + queueOrder: queueOrder + )) + } + } + + guard !candidates.isEmpty else { return } + + candidates.sort { lhs, rhs in + if lhs.message.timestamp != rhs.message.timestamp { + return lhs.message.timestamp < rhs.message.timestamp + } + if lhs.aliasOrder != rhs.aliasOrder { + return lhs.aliasOrder < rhs.aliasOrder + } + if lhs.queueOrder != rhs.queueOrder { + return lhs.queueOrder < rhs.queueOrder + } + return lhs.message.messageID < rhs.message.messageID + } + + SecureLogger.debug( + "Flushing merged outbox for \(peerIDAliases.count) alias(es) count=\(candidates.count)", + category: .session + ) + + let now = now() + var outboxChanged = false + var flushedMessageIDs = Set() + + for candidate in candidates { + guard flushedMessageIDs.insert(candidate.message.messageID).inserted else { continue } + outboxChanged = flushQueuedMessage( + candidate.message, + for: candidate.peerID, + now: now + ) || outboxChanged + } + + if outboxChanged { + persistOutbox() + } + } + + /// Send one queued message, or drop it if it is past TTL or the attempt + /// cap. Returns whether the outbox changed and needs persisting; the + /// caller owns the `persistOutbox()` so a whole flush costs one write. + private func flushQueuedMessage( + _ message: QueuedMessage, + for peerID: PeerID, + now: Date + ) -> Bool { + var outboxChanged = false + + // A synchronous ack from an earlier send in this flush may have + // removed an entry from the live outbox. The snapshot is only an + // iteration order; never use it to recreate removed messages. + guard queuedMessage(message.messageID, for: peerID) != nil else { return false } + + // Skip expired messages (TTL exceeded) + if now.timeIntervalSince(message.timestamp) > Self.messageTTLSeconds { + SecureLogger.debug("⏰ Expired queued message for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))… (age: \(Int(now.timeIntervalSince(message.timestamp)))s)", category: .session) + if removeQueuedMessage(message.messageID, for: peerID) { + dropMessage(message.messageID, for: peerID) + outboxChanged = true + } + return outboxChanged + } + + if let transport = connectedTransport(for: peerID), transport.canDeliverSecurely(to: peerID) { + // A secure session is meaningful enough to retry, but not + // proof that this particular ciphertext reached the peer: the + // remote app may have restarted while our old session still + // looked established. Retain until an ack, while bounding + // actual secure transmissions for peers that never ack. + guard message.sendAttempts < Self.maxSendAttempts else { + SecureLogger.warning("📤 Dropping unacked PM for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))… after \(message.sendAttempts) attempts", category: .session) + if removeQueuedMessage(message.messageID, for: peerID) { + dropMessage(message.messageID, for: peerID) + outboxChanged = true + } + return outboxChanged + } + SecureLogger.debug("Outbox -> \(type(of: transport)) (connected) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session) + secureTransmissions.insert( + PeerMessageKey(peerID: peerID, messageID: message.messageID) + ) + transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) + metrics?.record(.outboxResent) + outboxChanged = incrementSendAttemptsIfQueued(message.messageID, for: peerID) || outboxChanged + } else if let transport = connectedTransport(for: peerID) { + // "Connected" without a secure session — possibly a stolen + // binding from a replayed announce: send (a genuine link + // finishes the handshake and delivers) but keep retaining + // until an ack clears it. These flushes do NOT count toward + // the attempt-cap drop: the message was transmitted over a + // live link, so a peer whose handshake stalls across + // reconnect flapping must not burn through the cap and lose + // the store-and-forward copy this retention exists to + // preserve. Retention stays bounded by the 24h outbox TTL + // and the per-peer FIFO cap. + SecureLogger.debug("Outbox -> \(type(of: transport)) (connected, no secure session) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session) + secureTransmissions.remove( + PeerMessageKey(peerID: peerID, messageID: message.messageID) + ) + transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) + metrics?.record(.outboxResent) + } else if let transport = reachableTransport(for: peerID) { + // Reachability without a connection is a freshness heuristic, + // so the send can silently go nowhere: send but keep retaining + // until an ack clears it, bounded by attempt count for peers + // that never ack. + guard message.sendAttempts < Self.maxSendAttempts else { + SecureLogger.warning("📤 Dropping unacked PM for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))… after \(message.sendAttempts) attempts", category: .session) + if removeQueuedMessage(message.messageID, for: peerID) { + dropMessage(message.messageID, for: peerID) + outboxChanged = true + } + return outboxChanged + } + SecureLogger.debug("Outbox -> \(type(of: transport)) (reachable) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session) + transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) + metrics?.record(.outboxResent) + outboxChanged = incrementSendAttemptsIfQueued(message.messageID, for: peerID) || outboxChanged + } + + return outboxChanged + } + func flushAllOutbox() { for key in Array(outbox.keys) { flushOutbox(for: key) } } diff --git a/bitchat/ViewModels/ChatTransportEventCoordinator.swift b/bitchat/ViewModels/ChatTransportEventCoordinator.swift index ce705fa8..d693993c 100644 --- a/bitchat/ViewModels/ChatTransportEventCoordinator.swift +++ b/bitchat/ViewModels/ChatTransportEventCoordinator.swift @@ -56,7 +56,10 @@ protocol ChatTransportEventContext: AnyObject { func cachedStablePeerID(for shortPeerID: PeerID) -> PeerID? // MARK: Routing & acknowledgements - func flushRouterOutbox(for peerID: PeerID) + /// Drains the message router's disk outbox for every alias of one peer + /// as a single chronological stream, so mail split across the ephemeral + /// and stable keys cannot be delivered out of order. + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) /// Offer queued mail for *other* peers to this newly connected courier. func retryCourierDeposits(via peerID: PeerID) func sendMeshDeliveryAck(for messageID: String, to peerID: PeerID) @@ -114,8 +117,11 @@ extension ChatViewModel: ChatTransportEventContext { meshService.noiseSessionPublicKeyData(for: peerID) } - func flushRouterOutbox(for peerID: PeerID) { - messageRouter.flushOutbox(for: peerID) + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + messageRouter.flushOutbox( + forAliases: peerIDAliases, + skippingSecurelyTransmitted: skippingSecurelyTransmitted + ) } func retryCourierDeposits(via peerID: PeerID) { @@ -275,32 +281,38 @@ final class ChatTransportEventCoordinator { context.notifyUIChanged() // Resolve the stable key robustly: unified-peer state may not be - // populated yet at connect time, so fall back to the cache and then to - // the noise session key (mirroring the disconnect path). + // populated yet at connect time, so fall back to the live noise + // session key and only then to the cache. Order matters — short BLE + // IDs are ephemeral and get recycled, so a cache entry left by a + // previous owner of this ID would name the wrong peer, while the + // session key is the identity of the link we just brought up. var stablePeerID: PeerID? if let peer = context.unifiedPeer(for: peerID) { let resolved = PeerID(hexData: peer.noisePublicKey) context.cacheStablePeerID(resolved, for: peerID) stablePeerID = resolved - } else if let cached = context.cachedStablePeerID(for: peerID) { - stablePeerID = cached } else if let key = context.noiseSessionPublicKeyData(for: peerID) { let derived = PeerID(hexData: key) context.cacheStablePeerID(derived, for: peerID) stablePeerID = derived + } else if let cached = context.cachedStablePeerID(for: peerID) { + stablePeerID = cached } - context.flushRouterOutbox(for: peerID) - context.retryCourierDeposits(via: peerID) - - // Also flush under the stable 64-hex key. `flushOutbox` is keyed by - // exactly the id it is handed, and a DM composed while the recipient - // was an offline favorite is queued under their stable key — so the - // short-id flush above never reaches it, and absent a courier it waits - // for a relaunch, a favorite-status change, or the 24h TTL. + // Flush the short ID and the stable 64-hex key together. `flushOutbox` + // is keyed by exactly the ID it is handed, and a DM composed while the + // recipient was an offline favorite is queued under their stable key — + // so a short-ID flush alone never reaches it, and absent a courier it + // waits for a relaunch, a favorite-status change, or the 24h TTL. + // Passing both as aliases lets the router merge the two queues + // chronologically, so recent mail on the short ID cannot overtake the + // older mail that has been waiting under the stable key. + var aliases = [peerID] if let stablePeerID, stablePeerID != peerID { - context.flushRouterOutbox(for: stablePeerID) + aliases.append(stablePeerID) } + context.flushRouterOutbox(forAliases: aliases, skippingSecurelyTransmitted: false) + context.retryCourierDeposits(via: peerID) } func didDisconnectFromPeer(_ peerID: PeerID) { diff --git a/bitchat/ViewModels/ChatVerificationCoordinator.swift b/bitchat/ViewModels/ChatVerificationCoordinator.swift index 5ab89d54..c139a012 100644 --- a/bitchat/ViewModels/ChatVerificationCoordinator.swift +++ b/bitchat/ViewModels/ChatVerificationCoordinator.swift @@ -45,9 +45,9 @@ protocol ChatVerificationContext: AnyObject { func resolveNickname(for peerID: PeerID) -> String func cachedStablePeerID(for shortPeerID: PeerID) -> PeerID? func cacheStablePeerID(_ stablePeerID: PeerID, for shortPeerID: PeerID) - /// Flushes the message router's disk outbox for the given (stable) key so - /// mail queued while the peer was offline delivers once it authenticates. - func flushRouterOutbox(for peerID: PeerID) + /// Drains the message router's disk outbox for every alias of one peer so + /// mail queued while they were offline delivers once they authenticate. + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) // MARK: Noise sessions & verification transport /// Installs the Noise service's session callbacks (single registration point). @@ -81,8 +81,9 @@ extension ChatViewModel: ChatVerificationContext { // `isVerifiedFingerprint(_:)`, `setEncryptionStatus(_:for:)`, // `resolveNickname(for:)`, `cachedStablePeerID(for:)`, // `cacheStablePeerID(_:for:)`, `noiseSessionPublicKeyData(for:)`, - // `hasEstablishedNoiseSession(with:)`, `triggerHandshake(with:)`, and - // `flushRouterOutbox(for:)` (shared with `ChatTransportEventContext`) are + // `hasEstablishedNoiseSession(with:)`, `triggerHandshake(with:)`, + // and `flushRouterOutbox(forAliases:skippingSecurelyTransmitted:)` + // (shared with `ChatTransportEventContext`) are // shared requirements with the other contexts or satisfied by existing // `ChatViewModel` members. The members below flatten nested service // accesses into intent-named calls. @@ -260,16 +261,20 @@ final class ChatVerificationCoordinator { self.context.retrySecurePrivateMessagesAfterAuthentication(for: peerIDAliases) // The retry above only reaches messages already transmitted - // through a secure session (it filters on `sendAttempts > 0`). - // A DM composed while this peer was offline was never - // transmitted at all — it sits in the outbox under the - // stable 64-hex key — so it is in neither that set nor the - // short-id flush on connect. Flush it here, now that the - // link is authenticated and the stable identity is known, - // rather than leaving it for the TTL. - if let stablePeerID, stablePeerID != peerID { - self.context.flushRouterOutbox(for: stablePeerID) - } + // through a secure session. A DM composed while this peer + // was offline was never transmitted at all — it sits in + // the outbox under the stable 64-hex key — so it is in + // neither that set nor, when the stable key was still + // unresolvable at connect time, the flush on connect. + // Flush it here, now that the link is authenticated and + // the stable identity is known, rather than leaving it for + // the TTL. `skippingSecurelyTransmitted` keeps this + // disjoint from the retry above instead of re-sending + // everything the retry just put on the air. + self.context.flushRouterOutbox( + forAliases: peerIDAliases, + skippingSecurelyTransmitted: true + ) if var pending = self.pendingQRVerifications[peerID], pending.sent == false { self.context.sendVerifyChallenge( diff --git a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift index fc7d4f7d..16c7007c 100644 --- a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift +++ b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift @@ -124,7 +124,11 @@ private final class MockChatTransportEventContext: ChatTransportEventContext { private(set) var courierRetryPeerIDs: [PeerID] = [] private(set) var meshDeliveryAcks: [(messageID: String, peerID: PeerID)] = [] - func flushRouterOutbox(for peerID: PeerID) { flushedOutboxPeerIDs.append(peerID) } + private(set) var flushedSkippingSecurelyTransmitted: [Bool] = [] + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + flushedOutboxPeerIDs.append(contentsOf: peerIDAliases) + flushedSkippingSecurelyTransmitted.append(skippingSecurelyTransmitted) + } func retryCourierDeposits(via peerID: PeerID) { courierRetryPeerIDs.append(peerID) } func sendMeshDeliveryAck(for messageID: String, to peerID: PeerID) { meshDeliveryAcks.append((messageID, peerID)) @@ -550,4 +554,58 @@ struct ChatTransportEventCoordinatorContextTests { .didConnectToPeerSynchronously(shortPeerID) #expect(unresolvable.flushedOutboxPeerIDs == [shortPeerID]) } + + /// Both keys must be handed to the router in a *single* call. Two + /// sequential single-key flushes would drain the short-ID queue first, so + /// mail composed moments ago could be delivered ahead of the older mail + /// that has been waiting under the stable key. Only the merged call lets + /// the router order the two queues by timestamp. + @Test @MainActor + func didConnectToPeer_flushesBothKeysInOneMergedCall() { + let context = MockChatTransportEventContext() + let shortPeerID = PeerID(str: "1122334455667788") + let noiseKey = Data((0..<32).map { UInt8(0xD0 &+ $0) }) + let stablePeerID = PeerID(hexData: noiseKey) + + context.peersByID[shortPeerID] = BitchatPeer( + peerID: shortPeerID, + noisePublicKey: noiseKey, + nickname: "alice" + ) + + ChatTransportEventCoordinator(context: context) + .didConnectToPeerSynchronously(shortPeerID) + + #expect( + context.flushedSkippingSecurelyTransmitted.count == 1, + "the two keys must be merged into one flush, not drained in sequence" + ) + #expect(context.flushedOutboxPeerIDs == [shortPeerID, stablePeerID]) + // Connect has no preceding retry pass, so nothing may be skipped. + #expect(context.flushedSkippingSecurelyTransmitted == [false]) + } + + /// Short BLE IDs are ephemeral and get recycled. A cache entry left by a + /// previous owner of the same short ID must lose to the identity of the + /// link we just brought up, or the flush drains — and transmits under — + /// the wrong peer's queue. + @Test @MainActor + func didConnectToPeer_prefersTheLiveSessionKeyOverAStaleCacheEntry() { + let context = MockChatTransportEventContext() + let shortPeerID = PeerID(str: "1122334455667788") + + let staleKey = Data(repeating: 0xAA, count: 32) + let liveKey = Data(repeating: 0xBB, count: 32) + context.cacheStablePeerID(PeerID(hexData: staleKey), for: shortPeerID) + context.noiseSessionKeysByPeerID[shortPeerID] = liveKey + + ChatTransportEventCoordinator(context: context) + .didConnectToPeerSynchronously(shortPeerID) + + #expect(context.flushedOutboxPeerIDs == [shortPeerID, PeerID(hexData: liveKey)]) + #expect( + !context.flushedOutboxPeerIDs.contains(PeerID(hexData: staleKey)), + "a recycled short ID flushed the previous owner's outbox" + ) + } } diff --git a/bitchatTests/ChatVerificationCoordinatorContextTests.swift b/bitchatTests/ChatVerificationCoordinatorContextTests.swift index d4502982..e2dae274 100644 --- a/bitchatTests/ChatVerificationCoordinatorContextTests.swift +++ b/bitchatTests/ChatVerificationCoordinatorContextTests.swift @@ -92,7 +92,11 @@ private final class MockChatVerificationContext: ChatVerificationContext { } private(set) var flushedOutboxPeerIDs: [PeerID] = [] - func flushRouterOutbox(for peerID: PeerID) { flushedOutboxPeerIDs.append(peerID) } + private(set) var flushedSkippingSecurelyTransmitted: [Bool] = [] + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + flushedOutboxPeerIDs.append(contentsOf: peerIDAliases) + flushedSkippingSecurelyTransmitted.append(skippingSecurelyTransmitted) + } // Noise sessions & verification transport var myNoiseStaticKey = Data(repeating: 0x42, count: 32) @@ -303,6 +307,35 @@ struct ChatVerificationCoordinatorContextTests { #expect(context.encryptionStatuses[peerID] == .noiseHandshaking) } + /// A DM composed while the peer was offline sits in the outbox under their + /// stable 64-hex key and was never transmitted, so + /// `retrySecurePrivateMessagesAfterAuthentication` — which only covers + /// messages already sent through a secure session — cannot reach it. When + /// the stable key was still unresolvable at connect time, authentication is + /// the first moment it can be flushed at all. + @Test @MainActor + func peerAuthentication_flushesTheOutboxForBothAliases() async { + let context = MockChatVerificationContext() + let coordinator = ChatVerificationCoordinator(context: context) + let peerID = PeerID(str: "1122334455667788") + let noiseKey = Data(repeating: 0x55, count: 32) + let stablePeerID = PeerID(hexData: noiseKey) + context.noiseSessionKeysByPeerID[peerID] = noiseKey + + coordinator.setupNoiseCallbacks() + context.installedCallbacks?.onPeerAuthenticated(peerID, "fp-unverified") + await waitForMainQueue() + + #expect( + context.flushedOutboxPeerIDs == [peerID, stablePeerID], + "offline-queued mail under the stable key was never flushed on authentication" + ) + // The retry pass just transmitted everything in `secureTransmissions`; + // flushing that set again would double-send it and burn a second + // attempt against the cap. + #expect(context.flushedSkippingSecurelyTransmitted == [true]) + } + @Test @MainActor func handleVerifyChallengePayload_postsMutualVerificationToastOncePerMinute() async { let context = MockChatVerificationContext() diff --git a/bitchatTests/Services/MessageRouterTests.swift b/bitchatTests/Services/MessageRouterTests.swift index a34acd7e..593f4904 100644 --- a/bitchatTests/Services/MessageRouterTests.swift +++ b/bitchatTests/Services/MessageRouterTests.swift @@ -167,6 +167,90 @@ struct MessageRouterTests { #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) } + /// Reconnect drains both the ephemeral and the stable outbox key. Draining + /// them as two sequential `flushOutbox(for:)` calls would put the newer + /// short-ID mail on the air first; the merged flush must order by + /// timestamp, exactly as the authentication retry above does. + @Test @MainActor + func mergedFlush_preservesFIFOAcrossSplitAliases() async { + let shortPeerID = PeerID(str: "0000000000000023") + let stablePeerID = PeerID(hexData: Data(repeating: 0x23, count: 32)) + let transport = MockTransport() + let clock = MutableTestClock() + let router = MessageRouter(transports: [transport], now: { clock.now }) + + // Both queue while the peer is unreachable: the older one under the + // stable key (composed while they were an offline favorite), the newer + // under the ephemeral ID. + router.sendPrivate("Older", to: stablePeerID, recipientNickname: "Peer", messageID: "flush-old") + clock.now = clock.now.addingTimeInterval(1) + router.sendPrivate("Newer", to: shortPeerID, recipientNickname: "Peer", messageID: "flush-new") + #expect(transport.sentPrivateMessages.isEmpty) + + transport.connectedPeers = [shortPeerID, stablePeerID] + transport.securePeers = [shortPeerID, stablePeerID] + router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + + #expect(transport.sentPrivateMessages.map(\.messageID) == ["flush-old", "flush-new"]) + #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) + } + + /// The same message ID can sit under both keys after a conversation + /// migrates from the ephemeral ID to the stable one. The merged flush must + /// put it on the air once, not once per key. + @Test @MainActor + func mergedFlush_sendsAMessageHeldUnderTwoKeysOnlyOnce() async { + let shortPeerID = PeerID(str: "0000000000000024") + let stablePeerID = PeerID(hexData: Data(repeating: 0x24, count: 32)) + let transport = MockTransport() + let router = MessageRouter(transports: [transport]) + + router.sendPrivate("Migrated", to: shortPeerID, recipientNickname: "Peer", messageID: "dup-1") + router.sendPrivate("Migrated", to: stablePeerID, recipientNickname: "Peer", messageID: "dup-1") + #expect(transport.sentPrivateMessages.isEmpty) + + transport.connectedPeers = [shortPeerID, stablePeerID] + transport.securePeers = [shortPeerID, stablePeerID] + router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + + #expect(transport.sentPrivateMessages.map(\.messageID) == ["dup-1"]) + } + + /// On the authentication path the flush runs straight after + /// `retrySecurePrivateMessagesAfterAuthentication`. Without the skip, the + /// flush is a strict superset of that retry on an authenticated link, so + /// every retried message goes out twice and burns two attempts against the + /// cap. Never-transmitted mail must still go out. + @Test @MainActor + func mergedFlush_skippingSecurelyTransmitted_doesNotResendTheRetriedSet() async { + let peerID = PeerID(str: "0000000000000025") + let transport = MockTransport() + transport.connectedPeers = [peerID] + transport.securePeers = [peerID] + let router = MessageRouter(transports: [transport]) + + // Transmitted through a secure session: this is the retry's territory. + // The flush is what marks it as securely transmitted. + router.sendPrivate("Transmitted", to: peerID, recipientNickname: "Peer", messageID: "sec-1") + router.flushOutbox(for: peerID) + #expect(transport.sentPrivateMessages.allSatisfy { $0.messageID == "sec-1" }) + + // Never transmitted: queued while the peer was unreachable. + transport.connectedPeers = [] + transport.securePeers = [] + router.sendPrivate("Offline", to: peerID, recipientNickname: "Peer", messageID: "off-1") + transport.connectedPeers = [peerID] + transport.securePeers = [peerID] + transport.resetRecordings() + + router.flushOutbox(forAliases: [peerID], skippingSecurelyTransmitted: true) + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["off-1"], + "the flush re-sent mail the authentication retry already put on the air" + ) + } + @Test @MainActor func authenticationRetry_doesNotDuplicateNormalPendingHandshakeSend() async { let peerID = PeerID(str: "0000000000000020") From 0a6a343ccc628d2f9a1bee7fea656ca2e8691949 Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 12:59:08 -0700 Subject: [PATCH 3/6] Skip by message ID, not by peer/message pair, and claim only live candidates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes in the merged flush, both found by a second cross-model review pass. The `skippingSecurelyTransmitted` filter matched on the peer/message pair. A migrated conversation holds the same message ID under both the ephemeral and the stable key, but only the transmitted copy is in `secureTransmissions` — so the filter excluded that one and let its untransmitted twin through, putting the message on the air twice in the very pass meant to prevent it. The skip now collects the retried message IDs across the whole alias set first, which is the granularity the retry itself works at. `flushedMessageIDs` also claimed an ID before checking the candidate was still live. A synchronous ack fired by an earlier send in the same loop removes an entry the candidate list still holds; that dead copy claimed the ID and suppressed the live twin under the other alias, dropping the mail instead of deduping it. The liveness guard now runs first. Both are mutation-proven against tests that reproduce the real sequence — the second needed `MockTransport.onSendPrivateMessage` to fire the ack mid-flush, since acking beforehand never builds the dead candidate at all and left the test tautological. 1955 app tests and 122 package tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 31 ++++-- .../Services/MessageRouterTests.swift | 95 +++++++++++++++++++ 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index 757fa8e5..e3ed1f54 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -734,18 +734,30 @@ final class MessageRouter { ) var visitedPeerIDs = Set() + for peerID in peerIDAliases { visitedPeerIDs.insert(peerID) } + + // The retry that precedes a skipping flush covers a message ID once, + // under whichever alias holds the securely-transmitted copy. So the + // skip has to be by message ID across the whole alias set, not by + // peer/message pair: the same ID can also sit under the *other* alias + // without being in `secureTransmissions`, and filtering per pair would + // let that copy sail through and put the message on the air twice — + // precisely the double-send this flag exists to prevent. + var retriedMessageIDs = Set() + if skippingSecurelyTransmitted { + for key in secureTransmissions where visitedPeerIDs.contains(key.peerID) { + retriedMessageIDs.insert(key.messageID) + } + } + + visitedPeerIDs.removeAll() var candidates: [Candidate] = [] for (aliasOrder, peerID) in peerIDAliases.enumerated() { guard visitedPeerIDs.insert(peerID).inserted else { continue } guard let queued = outbox[peerID], !queued.isEmpty else { continue } for (queueOrder, message) in queued.enumerated() { - if skippingSecurelyTransmitted, - secureTransmissions.contains( - PeerMessageKey(peerID: peerID, messageID: message.messageID) - ) { - continue - } + guard !retriedMessageIDs.contains(message.messageID) else { continue } candidates.append(( peerID: peerID, message: message, @@ -780,6 +792,13 @@ final class MessageRouter { var flushedMessageIDs = Set() for candidate in candidates { + // Claim the ID only for a candidate that is still live. Marking it + // flushed first would let a copy removed by a synchronous ack + // earlier in this loop suppress the live copy under the other + // alias, which would silently drop mail rather than dedup it. + guard queuedMessage(candidate.message.messageID, for: candidate.peerID) != nil else { + continue + } guard flushedMessageIDs.insert(candidate.message.messageID).inserted else { continue } outboxChanged = flushQueuedMessage( candidate.message, diff --git a/bitchatTests/Services/MessageRouterTests.swift b/bitchatTests/Services/MessageRouterTests.swift index 593f4904..722d1287 100644 --- a/bitchatTests/Services/MessageRouterTests.swift +++ b/bitchatTests/Services/MessageRouterTests.swift @@ -214,6 +214,17 @@ struct MessageRouterTests { router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) #expect(transport.sentPrivateMessages.map(\.messageID) == ["dup-1"]) + + // The skipped copy must not resurface as a duplicate once the ack + // arrives: an ack scoped to the peer's aliases clears every key that + // holds the ID, so the copy the flush passed over goes with it. + router.markDelivered("dup-1", for: [shortPeerID, stablePeerID]) + transport.resetRecordings() + router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + #expect( + transport.sentPrivateMessages.isEmpty, + "the copy the merged flush skipped was re-sent after the ack" + ) } /// On the authentication path the flush runs straight after @@ -251,6 +262,90 @@ struct MessageRouterTests { ) } + /// The skip must be by message ID across the whole alias set, not by + /// peer/message pair. A migrated conversation holds the same ID under both + /// keys, but only the copy that was actually transmitted is in + /// `secureTransmissions` — so a per-pair filter excludes that one and lets + /// the untransmitted twin through, putting the message on the air twice in + /// the very pass that was meant to prevent it. + @Test @MainActor + func mergedFlush_skippingSecurelyTransmitted_coversTheTwinUnderTheOtherAlias() async { + let shortPeerID = PeerID(str: "0000000000000026") + let stablePeerID = PeerID(hexData: Data(repeating: 0x26, count: 32)) + let transport = MockTransport() + let router = MessageRouter(transports: [transport]) + + // The short-ID copy is transmitted securely, so it lands in + // `secureTransmissions` — the retry's territory. + transport.connectedPeers = [shortPeerID] + transport.securePeers = [shortPeerID] + router.sendPrivate("Migrated", to: shortPeerID, recipientNickname: "Peer", messageID: "twin-1") + router.flushOutbox(for: shortPeerID) + + // The same message also sits under the stable key, never transmitted. + transport.connectedPeers = [] + transport.securePeers = [] + router.sendPrivate("Migrated", to: stablePeerID, recipientNickname: "Peer", messageID: "twin-1") + + transport.connectedPeers = [shortPeerID, stablePeerID] + transport.securePeers = [shortPeerID, stablePeerID] + transport.resetRecordings() + + router.flushOutbox( + forAliases: [shortPeerID, stablePeerID], + skippingSecurelyTransmitted: true + ) + + #expect( + transport.sentPrivateMessages.isEmpty, + "the untransmitted twin was sent even though the retry already covered this ID" + ) + } + + /// A synchronous ack fired by an earlier send in the same flush removes an + /// entry from the live outbox. The merged flush must not let that dead + /// candidate claim the message ID, or the live copy under the other alias + /// is silently dropped instead of deduped. + @Test @MainActor + func mergedFlush_aDeadFirstCandidateDoesNotSuppressTheLiveTwin() async { + let shortPeerID = PeerID(str: "0000000000000027") + let stablePeerID = PeerID(hexData: Data(repeating: 0x27, count: 32)) + let transport = MockTransport() + let clock = MutableTestClock() + let router = MessageRouter(transports: [transport], now: { clock.now }) + + // Oldest, so it is flushed first and its ack lands mid-loop. + router.sendPrivate("First", to: shortPeerID, recipientNickname: "Peer", messageID: "first-1") + clock.now = clock.now.addingTimeInterval(1) + // "gone-1" queues under the short ID before the stable one, so it sorts + // ahead of its twin and is the candidate that would claim the ID. + router.sendPrivate("Gone", to: shortPeerID, recipientNickname: "Peer", messageID: "gone-1") + clock.now = clock.now.addingTimeInterval(1) + router.sendPrivate("Gone", to: stablePeerID, recipientNickname: "Peer", messageID: "gone-1") + + transport.connectedPeers = [shortPeerID, stablePeerID] + transport.securePeers = [shortPeerID, stablePeerID] + transport.resetRecordings() + + // Sending the first message synchronously acks the short-ID copy of + // "gone-1" — scoped to that alias alone, which is the deliberate + // behaviour for an ID that is also queued elsewhere. The candidate + // list was snapshotted before this, so it still holds the dead copy. + transport.onSendPrivateMessage = { messageID in + guard messageID == "first-1" else { return } + router.markDelivered("gone-1", for: [shortPeerID]) + } + + router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + transport.onSendPrivateMessage = nil + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["first-1", "gone-1"], + "a copy removed mid-flush claimed the message ID and suppressed the live twin" + ) + #expect(transport.sentPrivateMessages.map(\.peerID) == [shortPeerID, stablePeerID]) + } + @Test @MainActor func authenticationRetry_doesNotDuplicateNormalPendingHandshakeSend() async { let peerID = PeerID(str: "0000000000000020") From 7ba946b73466ab421c7c97a47b0037826348163e Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 13:23:54 -0700 Subject: [PATCH 4/6] Fix the same claim-before-liveness bug in the authentication retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retrySecurePrivateMessagesAfterAuthentication` carries the bug this branch just fixed three lines away in the merged flush. Its guard opens with `retriedMessageIDs.insert(...).inserted`, and a comma-guard short-circuits left to right — so the insert lands even when a later clause rejects the candidate. A synchronous ack from an earlier send in the same loop is peer-scoped, so it can clear one copy while the twin under the other alias stays live and eligible; the dead candidate claims the ID first and the live twin is skipped, silently missing a retry that was due. Pre-existing on main, not introduced here: the guard is from #1462 and the two-alias call it needs is already in `ChatVerificationCoordinator`. I fixed it anyway because shipping the fix for one function while its twin sits broken next door is worse than the scope it adds. Happy to split it out. Reordering to check-then-claim is enough — the insert now runs only once everything else about the candidate is confirmed. Mutation-proven: restoring the original clause order fails the new test and nothing else. Also, from the same review: `OutboxCandidate` and the four-key sort comparator were duplicated verbatim between the two functions and are now shared; `flushOutbox(forAliases:)` no longer reuses one set for two different jobs; and `skippingSecurelyTransmitted` loses its default, since the wrong value there means a double-send and every caller should have to say what it means. 1957 app tests and 122 package tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 89 ++++++++++--------- .../Services/MessageRouterTests.swift | 53 ++++++++++- 2 files changed, 94 insertions(+), 48 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index e3ed1f54..257cdd84 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -587,13 +587,37 @@ final class MessageRouter { /// undecryptable remotely. Normal pre-handshake sends are intentionally /// absent from `secureTransmissions` because BLE already queues /// and drains them when authentication completes. + /// One retained message paired with the alias whose queue holds it, plus + /// the tie-breakers that make a merge across aliases deterministic. + private typealias OutboxCandidate = ( + peerID: PeerID, + message: QueuedMessage, + aliasOrder: Int, + queueOrder: Int + ) + + /// Merge candidates drawn from several alias queues into one chronological + /// stream, so the order the aliases happen to arrive in cannot send newer + /// mail ahead of older mail for the same conversation. + private static func chronologically( + _ candidates: [OutboxCandidate] + ) -> [OutboxCandidate] { + candidates.sorted { lhs, rhs in + if lhs.message.timestamp != rhs.message.timestamp { + return lhs.message.timestamp < rhs.message.timestamp + } + if lhs.aliasOrder != rhs.aliasOrder { + return lhs.aliasOrder < rhs.aliasOrder + } + if lhs.queueOrder != rhs.queueOrder { + return lhs.queueOrder < rhs.queueOrder + } + return lhs.message.messageID < rhs.message.messageID + } + } + func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) { - typealias Candidate = ( - peerID: PeerID, - message: QueuedMessage, - aliasOrder: Int, - queueOrder: Int - ) + typealias Candidate = OutboxCandidate var visitedPeerIDs = Set() var retriedMessageIDs = Set() @@ -625,28 +649,22 @@ final class MessageRouter { // ephemeral and stable outbox keys. Merge both queues into one // chronological stream so callback alias order cannot send newer mail // ahead of older mail. - candidates.sort { lhs, rhs in - if lhs.message.timestamp != rhs.message.timestamp { - return lhs.message.timestamp < rhs.message.timestamp - } - if lhs.aliasOrder != rhs.aliasOrder { - return lhs.aliasOrder < rhs.aliasOrder - } - if lhs.queueOrder != rhs.queueOrder { - return lhs.queueOrder < rhs.queueOrder - } - return lhs.message.messageID < rhs.message.messageID - } + candidates = Self.chronologically(candidates) for candidate in candidates { let peerID = candidate.peerID let message = candidate.message let key = PeerMessageKey(peerID: peerID, messageID: message.messageID) - guard retriedMessageIDs.insert(message.messageID).inserted, - secureTransmissions.contains(key), + // Claim the ID last. A synchronous ack from an earlier send in + // this loop is peer-scoped, so it can clear this copy while the + // twin under the other alias stays live and eligible — and a + // claim made before these checks would let the dead candidate + // suppress that twin, silently skipping a retry that was due. + guard secureTransmissions.contains(key), queuedMessage(message.messageID, for: peerID) != nil, let transport = connectedTransport(for: peerID), - transport.canDeliverSecurely(to: peerID) else { + transport.canDeliverSecurely(to: peerID), + retriedMessageIDs.insert(message.messageID).inserted else { continue } @@ -725,16 +743,10 @@ final class MessageRouter { /// of it — re-sending every message the retry just put on the air and /// burning a second attempt against the cap. Skipping that set makes the /// two passes disjoint by construction rather than by comment. - func flushOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool = false) { - typealias Candidate = ( - peerID: PeerID, - message: QueuedMessage, - aliasOrder: Int, - queueOrder: Int - ) + func flushOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + typealias Candidate = OutboxCandidate - var visitedPeerIDs = Set() - for peerID in peerIDAliases { visitedPeerIDs.insert(peerID) } + let aliasSet = Set(peerIDAliases) // The retry that precedes a skipping flush covers a message ID once, // under whichever alias holds the securely-transmitted copy. So the @@ -745,12 +757,12 @@ final class MessageRouter { // precisely the double-send this flag exists to prevent. var retriedMessageIDs = Set() if skippingSecurelyTransmitted { - for key in secureTransmissions where visitedPeerIDs.contains(key.peerID) { + for key in secureTransmissions where aliasSet.contains(key.peerID) { retriedMessageIDs.insert(key.messageID) } } - visitedPeerIDs.removeAll() + var visitedPeerIDs = Set() var candidates: [Candidate] = [] for (aliasOrder, peerID) in peerIDAliases.enumerated() { @@ -769,18 +781,7 @@ final class MessageRouter { guard !candidates.isEmpty else { return } - candidates.sort { lhs, rhs in - if lhs.message.timestamp != rhs.message.timestamp { - return lhs.message.timestamp < rhs.message.timestamp - } - if lhs.aliasOrder != rhs.aliasOrder { - return lhs.aliasOrder < rhs.aliasOrder - } - if lhs.queueOrder != rhs.queueOrder { - return lhs.queueOrder < rhs.queueOrder - } - return lhs.message.messageID < rhs.message.messageID - } + candidates = Self.chronologically(candidates) SecureLogger.debug( "Flushing merged outbox for \(peerIDAliases.count) alias(es) count=\(candidates.count)", diff --git a/bitchatTests/Services/MessageRouterTests.swift b/bitchatTests/Services/MessageRouterTests.swift index 722d1287..497040eb 100644 --- a/bitchatTests/Services/MessageRouterTests.swift +++ b/bitchatTests/Services/MessageRouterTests.swift @@ -167,6 +167,51 @@ struct MessageRouterTests { #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) } + /// The retry claims each message ID once across the alias set. It must not + /// claim one for a candidate that turns out to be dead: a synchronous ack + /// from an earlier send in the same loop is peer-scoped, so it can clear + /// one copy while the twin under the other alias stays live and eligible. + /// A claim made before that check would silently skip a retry that was due. + @Test @MainActor + func authenticationRetry_aDeadCandidateDoesNotSuppressTheLiveTwin() async { + let shortPeerID = PeerID(str: "0000000000000028") + let stablePeerID = PeerID(hexData: Data(repeating: 0x28, count: 32)) + let transport = MockTransport() + transport.connectedPeers = [shortPeerID, stablePeerID] + transport.securePeers = [shortPeerID, stablePeerID] + let clock = MutableTestClock() + let router = MessageRouter(transports: [transport], now: { clock.now }) + + // Oldest, so it is retried first and its ack lands mid-loop. + router.sendPrivate("First", to: shortPeerID, recipientNickname: "Peer", messageID: "r-first") + clock.now = clock.now.addingTimeInterval(1) + router.sendPrivate("Twin", to: shortPeerID, recipientNickname: "Peer", messageID: "r-twin") + clock.now = clock.now.addingTimeInterval(1) + router.sendPrivate("Twin", to: stablePeerID, recipientNickname: "Peer", messageID: "r-twin") + + // Flushing marks all three as securely transmitted, which is what puts + // them in the retry's candidate set. + router.flushOutbox(for: shortPeerID) + router.flushOutbox(for: stablePeerID) + transport.resetRecordings() + + // Retrying the first message synchronously acks the short-ID copy of + // the twin, scoped to that alias alone. + transport.onSendPrivateMessage = { messageID in + guard messageID == "r-first" else { return } + router.markDelivered("r-twin", for: [shortPeerID]) + } + + router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID]) + transport.onSendPrivateMessage = nil + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["r-first", "r-twin"], + "a copy acked mid-retry claimed the message ID and suppressed the live twin" + ) + #expect(transport.sentPrivateMessages.map(\.peerID) == [shortPeerID, stablePeerID]) + } + /// Reconnect drains both the ephemeral and the stable outbox key. Draining /// them as two sequential `flushOutbox(for:)` calls would put the newer /// short-ID mail on the air first; the merged flush must order by @@ -189,7 +234,7 @@ struct MessageRouterTests { transport.connectedPeers = [shortPeerID, stablePeerID] transport.securePeers = [shortPeerID, stablePeerID] - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) #expect(transport.sentPrivateMessages.map(\.messageID) == ["flush-old", "flush-new"]) #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) @@ -211,7 +256,7 @@ struct MessageRouterTests { transport.connectedPeers = [shortPeerID, stablePeerID] transport.securePeers = [shortPeerID, stablePeerID] - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) #expect(transport.sentPrivateMessages.map(\.messageID) == ["dup-1"]) @@ -220,7 +265,7 @@ struct MessageRouterTests { // holds the ID, so the copy the flush passed over goes with it. router.markDelivered("dup-1", for: [shortPeerID, stablePeerID]) transport.resetRecordings() - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) #expect( transport.sentPrivateMessages.isEmpty, "the copy the merged flush skipped was re-sent after the ack" @@ -336,7 +381,7 @@ struct MessageRouterTests { router.markDelivered("gone-1", for: [shortPeerID]) } - router.flushOutbox(forAliases: [shortPeerID, stablePeerID]) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) transport.onSendPrivateMessage = nil #expect( From a4502046ffb389d748ceb516baa816cf4224ecc7 Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 13:53:08 -0700 Subject: [PATCH 5/6] Drop the periphery ignore the alias sweep made obsolete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PeerMessageKey.peerID` carried a `periphery:ignore` because it was read only through the synthesized `Hashable` conformance, which the indexer cannot attribute. The alias-scoped sweep in `flushOutbox(forAliases:)` now reads it directly, so the suppression is superfluous — and Periphery flags a superfluous ignore as an issue in its own right, which is what turned the Dead Code check red on this PR. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index 257cdd84..2837a6ea 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -35,10 +35,10 @@ final class MessageRouter { typealias QueuedMessage = MessageOutboxStore.QueuedMessage private struct PeerMessageKey: Hashable { - // periphery:ignore - read only via the synthesized Hashable - // conformance (dictionary-key identity), which the indexer - // cannot attribute; see retain_codable_properties in .periphery.yml - // for the same class of false positive. + // Both properties are read directly now — `peerID` by the alias-scoped + // sweep in `flushOutbox(forAliases:)`, `messageID` throughout — so the + // ignore directive this once carried (for reads visible only through + // the synthesized Hashable conformance) would itself be flagged. let peerID: PeerID let messageID: String } From 265028c712f16bdddb2504eef139e4696df67e83 Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Mon, 27 Jul 2026 02:27:36 +0200 Subject: [PATCH 6/6] fix: route favorite notifications through the MessageRouter outbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A favorite toggle sent while the peer was offline or mid-handshake was silently dropped — for mesh-only peers a missed npub exchange. Build the same [FAVORITED]: payload the transports built internally and route it through sendPrivate, so the outbox retains it until an ack and the alias flush drains it on reconnect/authentication under either PeerID form. Wire format unchanged: receivers already parse the prefix from PM content. Co-Authored-By: goose --- bitchat/Services/MessageRouter.swift | 23 +++++++++++++---- .../ChatViewModelBootstrapper.swift | 3 ++- .../Services/MessageRouterTests.swift | 25 +++++++++++++++++-- 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index 2837a6ea..234902d0 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -48,6 +48,9 @@ final class MessageRouter { private let courierDirectory: CourierDirectory private let outboxStore: MessageOutboxStore? private let metrics: StoreAndForwardMetrics? + // Stamps our npub onto favorite notifications so the recipient can learn + // our Nostr identity. Optional so tests can omit it. + private let idBridge: NostrIdentityBridge? /// Invoked whenever a retained private message is dropped without a /// delivery ack (attempt cap, TTL expiry, or per-peer overflow eviction) @@ -141,9 +144,11 @@ final class MessageRouter { now: @escaping () -> Date = Date.init, courierDirectory: CourierDirectory? = nil, outboxStore: MessageOutboxStore? = nil, - metrics: StoreAndForwardMetrics? = nil + metrics: StoreAndForwardMetrics? = nil, + idBridge: NostrIdentityBridge? = nil ) { self.transports = transports + self.idBridge = idBridge self.now = now self.courierDirectory = courierDirectory ?? .favoritesBacked() self.outboxStore = outboxStore @@ -572,11 +577,19 @@ final class MessageRouter { } func sendFavoriteNotification(to peerID: PeerID, isFavorite: Bool) { - if let transport = connectedTransport(for: peerID) { - transport.sendFavoriteNotification(to: peerID, isFavorite: isFavorite) - } else if let transport = reachableTransport(for: peerID) { - transport.sendFavoriteNotification(to: peerID, isFavorite: isFavorite) + // Route favorites through the outbox instead of fire-and-forget: a + // toggle sent while the peer is offline or mid-handshake used to be + // silently dropped, which for mesh-only peers meant a missed npub + // exchange. The outbox retains until an ack, and the alias flush + // drains the queue on reconnect/authentication no matter which PeerID + // form the toggle was keyed under. The payload is the same + // [FAVORITED]: content the transports built internally — the + // receiver parses the prefix from PM content and never displays it. + var content = isFavorite ? "[FAVORITED]" : "[UNFAVORITED]" + if let identity = try? idBridge?.getCurrentNostrIdentity() { + content += ":" + identity.npub } + sendPrivate(content, to: peerID, recipientNickname: "", messageID: UUID().uuidString) } /// Retries only messages that the router previously transmitted through diff --git a/bitchat/ViewModels/ChatViewModelBootstrapper.swift b/bitchat/ViewModels/ChatViewModelBootstrapper.swift index 99ca2886..3e2c7c78 100644 --- a/bitchat/ViewModels/ChatViewModelBootstrapper.swift +++ b/bitchat/ViewModels/ChatViewModelBootstrapper.swift @@ -33,7 +33,8 @@ struct ChatViewModelServiceBundle { let messageRouter = MessageRouter( transports: [meshService, nostrTransport], outboxStore: outboxStore, - metrics: sfMetrics + metrics: sfMetrics, + idBridge: idBridge ) self.commandProcessor = commandProcessor diff --git a/bitchatTests/Services/MessageRouterTests.swift b/bitchatTests/Services/MessageRouterTests.swift index 497040eb..b0e1d868 100644 --- a/bitchatTests/Services/MessageRouterTests.swift +++ b/bitchatTests/Services/MessageRouterTests.swift @@ -653,7 +653,7 @@ struct MessageRouterTests { } @Test @MainActor - func sendFavoriteNotification_usesConnectedOrReachable() async { + func sendFavoriteNotification_routesThroughOutboxAsPrivateMessage() async { let peerID = PeerID(str: "0000000000000004") let transport = MockTransport() transport.reachablePeers.insert(peerID) @@ -661,7 +661,28 @@ struct MessageRouterTests { let router = MessageRouter(transports: [transport]) router.sendFavoriteNotification(to: peerID, isFavorite: true) - #expect(transport.sentFavoriteNotifications.count == 1) + // Favorites ride the outbox (sendPrivate) so they survive offline / + // handshake gaps; the recipient parses the [FAVORITED] prefix. + #expect(transport.sentFavoriteNotifications.isEmpty) + #expect(transport.sentPrivateMessages.count == 1) + #expect(transport.sentPrivateMessages.first?.content.hasPrefix("[FAVORITED]") == true) + } + + @Test @MainActor + func sendFavoriteNotification_whileOffline_queuesAndFlushesOnReconnect() async { + let peerID = PeerID(str: "0000000000000009") + let transport = MockTransport() + // Peer is neither connected nor reachable: the favorite must be retained. + let router = MessageRouter(transports: [transport]) + router.sendFavoriteNotification(to: peerID, isFavorite: false) + #expect(transport.sentPrivateMessages.isEmpty) + + // Peer comes back: the queued favorite flushes. + transport.connectedPeers.insert(peerID) + router.flushOutbox(for: peerID) + + #expect(transport.sentPrivateMessages.count == 1) + #expect(transport.sentPrivateMessages.first?.content.hasPrefix("[UNFAVORITED]") == true) } // MARK: - Courier deposits