From 623f3855c5fcd9736427060554b5db9ba3330460 Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 12:28:02 -0700 Subject: [PATCH 1/9] 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/9] 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/9] 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/9] 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/9] 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 1adf22070b777805b92e4ffc1a68658020049d39 Mon Sep 17 00:00:00 2001 From: ecgang Date: Fri, 31 Jul 2026 08:47:10 -0700 Subject: [PATCH 6/9] Skip what the retry sent, not what it merely knew about MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auth pass and the flush that follows it were meant to be disjoint. They were, but by over-skipping: flushOutbox built its skip set from every secureTransmissions entry under these aliases, with no liveness check, while the retry abandons a whole alias when that alias has no connected transport or no secure session. A message under such an alias is in secureTransmissions and was never sent, so it was neither retried nor flushed. It waited for the next reconnect or the 24h TTL — the exact delay this change exists to remove. Have retrySecurePrivateMessagesAfterAuthentication return the message IDs it actually put on the air, and hand that set to flushOutbox. The doc comment's "disjoint by construction" is now true by construction rather than by narration. The same mistake sat one level down in the merged-flush loop, which claimed an ID before flushQueuedMessage ran, so an alias that sent nothing still suppressed the live twin under the other alias. Claiming after the call needed a return value the old Bool could not give: it meant "the outbox changed", which is true when a message is dropped past TTL without being sent, and false when a send over a connected link with no secure session deliberately leaves the attempt count alone. Wrong in both directions. FlushAttempt reports the two facts separately — the claim keys on sent, persistence keys on outboxChanged. Both regression tests were mutation-proved: reverting either fix turns the matching test red. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 115 +++++++++++------- .../ChatTransportEventCoordinator.swift | 8 +- .../ChatVerificationCoordinator.swift | 19 +-- ...ransportEventCoordinatorContextTests.swift | 10 +- ...tVerificationCoordinatorContextTests.swift | 22 ++-- .../Services/MessageRouterTests.swift | 94 ++++++++++++-- 6 files changed, 189 insertions(+), 79 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index 2837a6ea..410b73e4 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -616,11 +616,18 @@ final class MessageRouter { } } - func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) { + /// Returns the message IDs this pass actually put on the air, so a flush + /// that follows can skip exactly those and nothing more. Deriving that set + /// from `secureTransmissions` instead would over-skip: an alias with no + /// live secure transport is abandoned wholesale below, and its entries are + /// in that set while never having been sent. + @discardableResult + func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) -> Set { typealias Candidate = OutboxCandidate var visitedPeerIDs = Set() var retriedMessageIDs = Set() + var transmittedMessageIDs = Set() var outboxChanged = false let currentDate = now() var candidates: [Candidate] = [] @@ -699,12 +706,15 @@ final class MessageRouter { messageID: message.messageID ) metrics?.record(.outboxResent) + transmittedMessageIDs.insert(message.messageID) outboxChanged = incrementSendAttemptsIfQueued(message.messageID, for: peerID) || outboxChanged } if outboxChanged { persistOutbox() } + + return transmittedMessageIDs } func flushOutbox(for peerID: PeerID) { @@ -715,7 +725,7 @@ final class MessageRouter { var outboxChanged = false for message in queued { - outboxChanged = flushQueuedMessage(message, for: peerID, now: now) || outboxChanged + outboxChanged = flushQueuedMessage(message, for: peerID, now: now).outboxChanged || outboxChanged } if outboxChanged { @@ -736,32 +746,25 @@ final class MessageRouter { /// `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) { + /// Pass the set returned by `retrySecurePrivateMessagesAfterAuthentication` + /// as `skippingMessageIDs` when a caller has just run it over the same + /// aliases. Those messages are already on the air, and re-sending them here + /// would burn a second attempt against the cap. + /// + /// It has to be the IDs that retry *transmitted*, not the entries in + /// `secureTransmissions` matching these aliases. Those differ: the retry + /// abandons an alias wholesale when it has no live secure transport, so a + /// message under that alias is in `secureTransmissions` yet was never sent. + /// Skipping the wider set left such a message neither retried nor flushed, + /// waiting on the next reconnect or the 24h TTL — the exact delay this + /// flush exists to remove. + /// + /// The skip is by message ID across the whole alias set rather than by + /// peer/message pair, because the same ID can sit under both aliases and + /// filtering per pair would put it on the air twice. + func flushOutbox(forAliases peerIDAliases: [PeerID], skippingMessageIDs: Set) { typealias Candidate = OutboxCandidate - 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 - // 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 aliasSet.contains(key.peerID) { - retriedMessageIDs.insert(key.messageID) - } - } - var visitedPeerIDs = Set() var candidates: [Candidate] = [] @@ -769,7 +772,7 @@ final class MessageRouter { guard visitedPeerIDs.insert(peerID).inserted else { continue } guard let queued = outbox[peerID], !queued.isEmpty else { continue } for (queueOrder, message) in queued.enumerated() { - guard !retriedMessageIDs.contains(message.messageID) else { continue } + guard !skippingMessageIDs.contains(message.messageID) else { continue } candidates.append(( peerID: peerID, message: message, @@ -793,19 +796,24 @@ 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( + // Claim the ID only once this alias actually put the message on a + // transport. Claiming any earlier — before the liveness check, or + // merely because the candidate was still queued — lets an alias + // that sends nothing suppress the live twin under the other alias, + // silently dropping mail instead of deduping it. A copy removed by + // a synchronous ack earlier in this loop, or an alias with no + // transport at all, both fail to send and so leave the twin + // eligible. + guard !flushedMessageIDs.contains(candidate.message.messageID) else { continue } + let attempt = flushQueuedMessage( candidate.message, for: candidate.peerID, now: now - ) || outboxChanged + ) + outboxChanged = attempt.outboxChanged || outboxChanged + if attempt.sent { + flushedMessageIDs.insert(candidate.message.messageID) + } } if outboxChanged { @@ -813,20 +821,34 @@ final class MessageRouter { } } + /// What one flush attempt did. The two facts are independent and neither + /// implies the other: a message past TTL is dropped without being sent + /// (`outboxChanged`, not `sent`), while a send over a connected link with + /// no secure session deliberately does not touch the attempt count + /// (`sent`, not `outboxChanged`). A caller deduping by message ID needs + /// `sent`; a caller deciding whether to persist needs `outboxChanged`. + private struct FlushAttempt { + let sent: Bool + let outboxChanged: Bool + } + /// 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. + /// cap. The caller owns the `persistOutbox()` so a whole flush costs one + /// write. private func flushQueuedMessage( _ message: QueuedMessage, for peerID: PeerID, now: Date - ) -> Bool { + ) -> FlushAttempt { var outboxChanged = false + var sent = 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 } + guard queuedMessage(message.messageID, for: peerID) != nil else { + return FlushAttempt(sent: false, outboxChanged: false) + } // Skip expired messages (TTL exceeded) if now.timeIntervalSince(message.timestamp) > Self.messageTTLSeconds { @@ -835,7 +857,7 @@ final class MessageRouter { dropMessage(message.messageID, for: peerID) outboxChanged = true } - return outboxChanged + return FlushAttempt(sent: false, outboxChanged: outboxChanged) } if let transport = connectedTransport(for: peerID), transport.canDeliverSecurely(to: peerID) { @@ -850,7 +872,7 @@ final class MessageRouter { dropMessage(message.messageID, for: peerID) outboxChanged = true } - return outboxChanged + return FlushAttempt(sent: false, outboxChanged: outboxChanged) } SecureLogger.debug("Outbox -> \(type(of: transport)) (connected) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session) secureTransmissions.insert( @@ -858,6 +880,7 @@ final class MessageRouter { ) transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) metrics?.record(.outboxResent) + sent = true outboxChanged = incrementSendAttemptsIfQueued(message.messageID, for: peerID) || outboxChanged } else if let transport = connectedTransport(for: peerID) { // "Connected" without a secure session — possibly a stolen @@ -876,6 +899,7 @@ final class MessageRouter { ) transport.sendPrivateMessage(message.content, to: peerID, recipientNickname: message.nickname, messageID: message.messageID) metrics?.record(.outboxResent) + sent = true } 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 @@ -887,15 +911,16 @@ final class MessageRouter { dropMessage(message.messageID, for: peerID) outboxChanged = true } - return outboxChanged + return FlushAttempt(sent: false, outboxChanged: 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) + sent = true outboxChanged = incrementSendAttemptsIfQueued(message.messageID, for: peerID) || outboxChanged } - return outboxChanged + return FlushAttempt(sent: sent, outboxChanged: outboxChanged) } func flushAllOutbox() { diff --git a/bitchat/ViewModels/ChatTransportEventCoordinator.swift b/bitchat/ViewModels/ChatTransportEventCoordinator.swift index d693993c..07647128 100644 --- a/bitchat/ViewModels/ChatTransportEventCoordinator.swift +++ b/bitchat/ViewModels/ChatTransportEventCoordinator.swift @@ -59,7 +59,7 @@ protocol ChatTransportEventContext: AnyObject { /// 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) + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingMessageIDs: Set) /// Offer queued mail for *other* peers to this newly connected courier. func retryCourierDeposits(via peerID: PeerID) func sendMeshDeliveryAck(for messageID: String, to peerID: PeerID) @@ -117,10 +117,10 @@ extension ChatViewModel: ChatTransportEventContext { meshService.noiseSessionPublicKeyData(for: peerID) } - func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingMessageIDs: Set) { messageRouter.flushOutbox( forAliases: peerIDAliases, - skippingSecurelyTransmitted: skippingSecurelyTransmitted + skippingMessageIDs: skippingMessageIDs ) } @@ -311,7 +311,7 @@ final class ChatTransportEventCoordinator { if let stablePeerID, stablePeerID != peerID { aliases.append(stablePeerID) } - context.flushRouterOutbox(forAliases: aliases, skippingSecurelyTransmitted: false) + context.flushRouterOutbox(forAliases: aliases, skippingMessageIDs: []) context.retryCourierDeposits(via: peerID) } diff --git a/bitchat/ViewModels/ChatVerificationCoordinator.swift b/bitchat/ViewModels/ChatVerificationCoordinator.swift index c139a012..c0c4eadf 100644 --- a/bitchat/ViewModels/ChatVerificationCoordinator.swift +++ b/bitchat/ViewModels/ChatVerificationCoordinator.swift @@ -47,7 +47,7 @@ protocol ChatVerificationContext: AnyObject { func cacheStablePeerID(_ stablePeerID: PeerID, for shortPeerID: 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) + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingMessageIDs: Set) // MARK: Noise sessions & verification transport /// Installs the Noise service's session callbacks (single registration point). @@ -65,7 +65,7 @@ protocol ChatVerificationContext: AnyObject { /// Retries only private messages previously transmitted through a secure /// session and still pending an ack. Both ephemeral and stable aliases /// are supplied because either can own the outbox entry. - func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) + func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) -> Set func sendVerifyChallenge(to peerID: PeerID, noiseKeyHex: String, nonceA: Data) func sendVerifyResponse(to peerID: PeerID, noiseKeyHex: String, nonceA: Data) @@ -82,7 +82,7 @@ extension ChatViewModel: ChatVerificationContext { // `resolveNickname(for:)`, `cachedStablePeerID(for:)`, // `cacheStablePeerID(_:for:)`, `noiseSessionPublicKeyData(for:)`, // `hasEstablishedNoiseSession(with:)`, `triggerHandshake(with:)`, - // and `flushRouterOutbox(forAliases:skippingSecurelyTransmitted:)` + // and `flushRouterOutbox(forAliases:skippingMessageIDs:)` // (shared with `ChatTransportEventContext`) are // shared requirements with the other contexts or satisfied by existing // `ChatViewModel` members. The members below flatten nested service @@ -130,7 +130,7 @@ extension ChatViewModel: ChatVerificationContext { mediaTransferCoordinator.peerDidAuthenticate(peerID.toShort()) } - func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) { + func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) -> Set { messageRouter.retrySecurePrivateMessagesAfterAuthentication(for: peerIDAliases) } @@ -258,7 +258,8 @@ final class ChatVerificationCoordinator { // because either may own the retained outbox entry. peerIDAliases.append(stablePeerID) } - self.context.retrySecurePrivateMessagesAfterAuthentication(for: peerIDAliases) + let retried = self.context + .retrySecurePrivateMessagesAfterAuthentication(for: peerIDAliases) // The retry above only reaches messages already transmitted // through a secure session. A DM composed while this peer @@ -268,12 +269,12 @@ final class ChatVerificationCoordinator { // 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. + // the TTL. Skipping exactly what the retry transmitted + // keeps the two passes disjoint without stranding a + // message the retry skipped for want of a live transport. self.context.flushRouterOutbox( forAliases: peerIDAliases, - skippingSecurelyTransmitted: true + skippingMessageIDs: retried ) if var pending = self.pendingQRVerifications[peerID], pending.sent == false { diff --git a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift index 16c7007c..f9a8ac41 100644 --- a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift +++ b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift @@ -124,10 +124,10 @@ private final class MockChatTransportEventContext: ChatTransportEventContext { private(set) var courierRetryPeerIDs: [PeerID] = [] private(set) var meshDeliveryAcks: [(messageID: String, peerID: PeerID)] = [] - private(set) var flushedSkippingSecurelyTransmitted: [Bool] = [] - func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + private(set) var flushedSkippingMessageIDs: [Set] = [] + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingMessageIDs: Set) { flushedOutboxPeerIDs.append(contentsOf: peerIDAliases) - flushedSkippingSecurelyTransmitted.append(skippingSecurelyTransmitted) + flushedSkippingMessageIDs.append(skippingMessageIDs) } func retryCourierDeposits(via peerID: PeerID) { courierRetryPeerIDs.append(peerID) } func sendMeshDeliveryAck(for messageID: String, to peerID: PeerID) { @@ -577,12 +577,12 @@ struct ChatTransportEventCoordinatorContextTests { .didConnectToPeerSynchronously(shortPeerID) #expect( - context.flushedSkippingSecurelyTransmitted.count == 1, + context.flushedSkippingMessageIDs.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]) + #expect(context.flushedSkippingMessageIDs == [[]]) } /// Short BLE IDs are ephemeral and get recycled. A cache entry left by a diff --git a/bitchatTests/ChatVerificationCoordinatorContextTests.swift b/bitchatTests/ChatVerificationCoordinatorContextTests.swift index e2dae274..c0b0ecb3 100644 --- a/bitchatTests/ChatVerificationCoordinatorContextTests.swift +++ b/bitchatTests/ChatVerificationCoordinatorContextTests.swift @@ -92,10 +92,10 @@ private final class MockChatVerificationContext: ChatVerificationContext { } private(set) var flushedOutboxPeerIDs: [PeerID] = [] - private(set) var flushedSkippingSecurelyTransmitted: [Bool] = [] - func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingSecurelyTransmitted: Bool) { + private(set) var flushedSkippingMessageIDs: [Set] = [] + func flushRouterOutbox(forAliases peerIDAliases: [PeerID], skippingMessageIDs: Set) { flushedOutboxPeerIDs.append(contentsOf: peerIDAliases) - flushedSkippingSecurelyTransmitted.append(skippingSecurelyTransmitted) + flushedSkippingMessageIDs.append(skippingMessageIDs) } // Noise sessions & verification transport @@ -126,8 +126,10 @@ private final class MockChatVerificationContext: ChatVerificationContext { privateMediaAuthenticatedPeers.append(peerID) } - func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) { + var securePrivateMessageRetryResult: Set = [] + func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) -> Set { securePrivateMessageRetryAliases.append(peerIDAliases) + return securePrivateMessageRetryResult } func sendVerifyChallenge(to peerID: PeerID, noiseKeyHex: String, nonceA: Data) { @@ -321,6 +323,7 @@ struct ChatVerificationCoordinatorContextTests { let noiseKey = Data(repeating: 0x55, count: 32) let stablePeerID = PeerID(hexData: noiseKey) context.noiseSessionKeysByPeerID[peerID] = noiseKey + context.securePrivateMessageRetryResult = ["retried-1"] coordinator.setupNoiseCallbacks() context.installedCallbacks?.onPeerAuthenticated(peerID, "fp-unverified") @@ -330,10 +333,13 @@ struct ChatVerificationCoordinatorContextTests { 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]) + // The flush must skip exactly what the retry reported transmitting. + // Re-deriving that set from `secureTransmissions` would also skip mail + // the retry passed over for want of a live secure session, stranding it. + #expect( + context.flushedSkippingMessageIDs == [["retried-1"]], + "the flush did not skip exactly the set the retry transmitted" + ) } @Test @MainActor diff --git a/bitchatTests/Services/MessageRouterTests.swift b/bitchatTests/Services/MessageRouterTests.swift index 497040eb..c147595a 100644 --- a/bitchatTests/Services/MessageRouterTests.swift +++ b/bitchatTests/Services/MessageRouterTests.swift @@ -234,7 +234,7 @@ struct MessageRouterTests { transport.connectedPeers = [shortPeerID, stablePeerID] transport.securePeers = [shortPeerID, stablePeerID] - router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingMessageIDs: []) #expect(transport.sentPrivateMessages.map(\.messageID) == ["flush-old", "flush-new"]) #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) @@ -256,7 +256,7 @@ struct MessageRouterTests { transport.connectedPeers = [shortPeerID, stablePeerID] transport.securePeers = [shortPeerID, stablePeerID] - router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingMessageIDs: []) #expect(transport.sentPrivateMessages.map(\.messageID) == ["dup-1"]) @@ -265,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], skippingSecurelyTransmitted: false) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingMessageIDs: []) #expect( transport.sentPrivateMessages.isEmpty, "the copy the merged flush skipped was re-sent after the ack" @@ -278,7 +278,7 @@ struct MessageRouterTests { /// 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 { + func mergedFlush_skippingRetriedIDs_doesNotResendTheRetriedSet() async { let peerID = PeerID(str: "0000000000000025") let transport = MockTransport() transport.connectedPeers = [peerID] @@ -299,7 +299,7 @@ struct MessageRouterTests { transport.securePeers = [peerID] transport.resetRecordings() - router.flushOutbox(forAliases: [peerID], skippingSecurelyTransmitted: true) + router.flushOutbox(forAliases: [peerID], skippingMessageIDs: ["sec-1"]) #expect( transport.sentPrivateMessages.map(\.messageID) == ["off-1"], @@ -314,7 +314,7 @@ struct MessageRouterTests { /// 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 { + func mergedFlush_skippingRetriedIDs_coversTheTwinUnderTheOtherAlias() async { let shortPeerID = PeerID(str: "0000000000000026") let stablePeerID = PeerID(hexData: Data(repeating: 0x26, count: 32)) let transport = MockTransport() @@ -338,7 +338,7 @@ struct MessageRouterTests { router.flushOutbox( forAliases: [shortPeerID, stablePeerID], - skippingSecurelyTransmitted: true + skippingMessageIDs: ["twin-1"] ) #expect( @@ -347,6 +347,84 @@ struct MessageRouterTests { ) } + /// A message ID must be claimed only once an alias actually put it on a + /// transport. The same ID can sit under both the ephemeral and the stable + /// key; if the first alias visited has no transport at all, its flush is a + /// no-op, and claiming the ID there would suppress the twin under the + /// alias that *can* deliver — dropping the message rather than deduping it. + @Test @MainActor + func mergedFlush_deadAliasDoesNotSuppressTheDeliverableTwin() async { + let deadPeerID = PeerID(str: "0000000000000028") + let livePeerID = PeerID(hexData: Data(repeating: 0x28, count: 32)) + let transport = MockTransport() + let router = MessageRouter(transports: [transport]) + + // Queue the same message ID under both aliases while neither is + // reachable, so nothing is sent yet. + transport.connectedPeers = [] + transport.securePeers = [] + router.sendPrivate("Twin", to: deadPeerID, recipientNickname: "Peer", messageID: "twin-2") + router.sendPrivate("Twin", to: livePeerID, recipientNickname: "Peer", messageID: "twin-2") + + // Only the stable alias comes up. The ephemeral one stays dark, so its + // flush sends nothing. + transport.connectedPeers = [livePeerID] + transport.securePeers = [livePeerID] + transport.resetRecordings() + + router.flushOutbox(forAliases: [deadPeerID, livePeerID], skippingMessageIDs: []) + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["twin-2"], + "the dead alias claimed the ID and suppressed the twin that could deliver" + ) + } + + /// The skip set must be what the retry *transmitted*, not every entry in + /// `secureTransmissions` under these aliases. Those differ whenever an + /// alias is connected but has no live secure session: the retry abandons + /// that alias wholesale, so its messages are in `secureTransmissions` + /// having never been sent. Deriving the skip set from that map left them + /// neither retried nor flushed — stranded until the next reconnect or the + /// 24h TTL, which is the exact delay this flush exists to remove. + @Test @MainActor + func mergedFlush_deliversMailTheRetrySkippedForWantOfASecureSession() async { + let peerID = PeerID(str: "0000000000000027") + let transport = MockTransport() + transport.connectedPeers = [peerID] + transport.securePeers = [peerID] + let router = MessageRouter(transports: [transport]) + + // Transmitted securely, so it lands in `secureTransmissions` and stays + // queued pending an ack. + router.sendPrivate("Stranded", to: peerID, recipientNickname: "Peer", messageID: "stranded-1") + router.flushOutbox(for: peerID) + #expect(transport.sentPrivateMessages.allSatisfy { $0.messageID == "stranded-1" }) + + // The link comes back without a secure session: still connected, but + // `canDeliverSecurely` is false, so the retry abandons this alias. + transport.securePeers = [] + transport.resetRecordings() + + let retried = router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + + #expect( + retried.isEmpty, + "the retry reported transmitting a message it never sent" + ) + #expect( + transport.sentPrivateMessages.isEmpty, + "the retry sent over a link that cannot deliver securely" + ) + + router.flushOutbox(forAliases: [peerID], skippingMessageIDs: retried) + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["stranded-1"], + "the flush skipped a message the retry never transmitted, stranding it until TTL" + ) + } + /// 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 @@ -381,7 +459,7 @@ struct MessageRouterTests { router.markDelivered("gone-1", for: [shortPeerID]) } - router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingSecurelyTransmitted: false) + router.flushOutbox(forAliases: [shortPeerID, stablePeerID], skippingMessageIDs: []) transport.onSendPrivateMessage = nil #expect( From f99d0e85ab545ebdee865e258b126370c71e214d Mon Sep 17 00:00:00 2001 From: ecgang Date: Fri, 31 Jul 2026 08:48:00 -0700 Subject: [PATCH 7/9] Drop the cache fallback the comment above it argues against MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit didConnectToPeerSynchronously explained that short BLE IDs get recycled, so a cache entry left by a previous owner of an ID would name the wrong peer — then read exactly that cache when both live sources were absent. Reaching the fallback means unified-peer state and the noise session key both had nothing to say about this link, which is precisely when nothing is left to catch the mistake. Flushing [shortID, wrongStableID] drains a stranger's queue and silently skips the right one. Resolve from evidence about this link only. When neither live source has resolved yet the short-ID flush still runs, and authentication flushes both aliases once the identity is known, so nothing is lost by waiting. Mutation-proved: restoring the fallback turns didConnectToPeer_resolvesTheStableKeyWithoutUnifiedPeerState red. Co-Authored-By: Claude Opus 5 (1M context) --- .../ChatTransportEventCoordinator.swift | 20 +++++++++++-------- ...ransportEventCoordinatorContextTests.swift | 13 ++++++++---- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/bitchat/ViewModels/ChatTransportEventCoordinator.swift b/bitchat/ViewModels/ChatTransportEventCoordinator.swift index 07647128..9cd39bbd 100644 --- a/bitchat/ViewModels/ChatTransportEventCoordinator.swift +++ b/bitchat/ViewModels/ChatTransportEventCoordinator.swift @@ -280,12 +280,18 @@ 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 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. + // Resolve the stable key from evidence about *this* link only: + // unified-peer state, or the live noise session key. Both name the peer + // we just brought up. + // + // Deliberately no cache fallback. Short BLE IDs are ephemeral and get + // recycled, so a cache entry left by a previous owner of this ID names + // the wrong peer — and reaching the fallback means both live sources + // were absent, which is exactly when there is nothing to catch the + // mistake. Flushing `[shortID, wrongStableID]` would drain a stranger's + // queue and silently skip the right one. When neither live source has + // resolved yet, the short-ID flush below still runs, and authentication + // flushes both aliases once the identity is known. var stablePeerID: PeerID? if let peer = context.unifiedPeer(for: peerID) { let resolved = PeerID(hexData: peer.noisePublicKey) @@ -295,8 +301,6 @@ final class ChatTransportEventCoordinator { let derived = PeerID(hexData: key) context.cacheStablePeerID(derived, for: peerID) stablePeerID = derived - } else if let cached = context.cachedStablePeerID(for: peerID) { - stablePeerID = cached } // Flush the short ID and the stable 64-hex key together. `flushOutbox` diff --git a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift index f9a8ac41..692e754d 100644 --- a/bitchatTests/ChatTransportEventCoordinatorContextTests.swift +++ b/bitchatTests/ChatTransportEventCoordinatorContextTests.swift @@ -525,20 +525,25 @@ struct ChatTransportEventCoordinatorContextTests { /// 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. + /// exactly the case it is for. It resolves from the live Noise session key, + /// never from the cache alone. @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. + // Cache only, with no live evidence for this link. Short BLE IDs are + // recycled, so the entry may belong to a previous owner of this ID — + // flushing it would drain a stranger's queue and skip the right one. let viaCache = MockChatTransportEventContext() viaCache.cacheStablePeerID(stablePeerID, for: shortPeerID) ChatTransportEventCoordinator(context: viaCache) .didConnectToPeerSynchronously(shortPeerID) - #expect(viaCache.flushedOutboxPeerIDs.contains(stablePeerID)) + #expect( + viaCache.flushedOutboxPeerIDs == [shortPeerID], + "a cache entry with no live corroboration named the stable peer" + ) // Noise session key only. let viaSession = MockChatTransportEventContext() From 7ccaac556eec4a5fb3efad2663bb86395b90d27a Mon Sep 17 00:00:00 2001 From: ecgang Date: Fri, 31 Jul 2026 09:32:49 -0700 Subject: [PATCH 8/9] Claim in the retry only past the checks that can drop the candidate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the same claim-before-outcome mistake a third time, in the retry pass this time. retriedMessageIDs was claimed inside the guard chain, above both the TTL check and the attempt cap, so a copy that was expired or capped took the ID, dropped itself, and suppressed the live twin under the other alias for the rest of the pass. The message still got out — the flush that follows never saw the ID in the transmitted set, so it picked the twin up. What did not survive is the status: dropMessage fires onMessageDropped, and the no-downgrade guard there only protects an already delivered or read message, so the UI marked the message failed while it was in fact about to deliver. Move the claim below both checks, matching what the flush loop now does. Also pin the retry's return value on the path where it actually sends. Nothing did: every other test either ignores the result or exercises the empty case, so deleting the line that records a transmission left all 68 tests green while reopening the double-send this PR exists to prevent. Verified by mutation before writing the test, and again after. Drop @discardableResult for the same reason. The whole point of returning the set is that the caller must forward it to flushOutbox; an unused-result warning is what enforces that on the next call site added, so the tests that genuinely do not care now spell it `_ =`. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 28 ++++-- .../ChatViewModelDeliveryStatusTests.swift | 2 +- .../Services/MessageRouterTests.swift | 94 +++++++++++++++++-- 3 files changed, 105 insertions(+), 19 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index 410b73e4..1577153e 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -621,7 +621,12 @@ final class MessageRouter { /// from `secureTransmissions` instead would over-skip: an alias with no /// live secure transport is abandoned wholesale below, and its entries are /// in that set while never having been sent. - @discardableResult + /// + /// Deliberately not `@discardableResult`. A caller that drops this set and + /// passes an empty one to `flushOutbox` re-sends everything this pass just + /// put on the air and burns a second attempt against the cap. An unused + /// result is the one thing that catches that at compile time, so the tests + /// that genuinely do not care spell it `_ =`. func retrySecurePrivateMessagesAfterAuthentication(for peerIDAliases: [PeerID]) -> Set { typealias Candidate = OutboxCandidate @@ -662,16 +667,13 @@ final class MessageRouter { let peerID = candidate.peerID let message = candidate.message let key = PeerMessageKey(peerID: peerID, messageID: message.messageID) - // 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. + // 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. guard secureTransmissions.contains(key), queuedMessage(message.messageID, for: peerID) != nil, let transport = connectedTransport(for: peerID), - transport.canDeliverSecurely(to: peerID), - retriedMessageIDs.insert(message.messageID).inserted else { + transport.canDeliverSecurely(to: peerID) else { continue } @@ -695,6 +697,16 @@ final class MessageRouter { continue } + // Claim the ID only now — past every check that can drop this + // candidate instead of sending it. Claiming in the guard chain + // above let a copy that was expired or past the attempt cap take + // the ID, drop itself, and suppress the live twin under the other + // alias, which then went unsent for this whole pass. The flush + // that follows still recovered it, but the drop had already + // reported the message failed to the UI while it was in fact + // about to deliver. + guard retriedMessageIDs.insert(message.messageID).inserted else { continue } + SecureLogger.debug( "Auth retry -> \(type(of: transport)) for \(peerID.id.prefix(8))… id=\(message.messageID.prefix(8))…", category: .session diff --git a/bitchatTests/ChatViewModelDeliveryStatusTests.swift b/bitchatTests/ChatViewModelDeliveryStatusTests.swift index 15a1edea..61308544 100644 --- a/bitchatTests/ChatViewModelDeliveryStatusTests.swift +++ b/bitchatTests/ChatViewModelDeliveryStatusTests.swift @@ -509,7 +509,7 @@ struct ChatViewModelDeliveryStatusTests { transport.connectedPeers.insert(peerID) transport.securePeers = [peerID] viewModel.messageRouter.flushOutbox(for: peerID) - viewModel.messageRouter.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + _ = viewModel.messageRouter.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) #expect(transport.sentPrivateMessages.isEmpty) // The clear reached the durable snapshot: the next relaunch restores diff --git a/bitchatTests/Services/MessageRouterTests.swift b/bitchatTests/Services/MessageRouterTests.swift index c147595a..98e0c536 100644 --- a/bitchatTests/Services/MessageRouterTests.swift +++ b/bitchatTests/Services/MessageRouterTests.swift @@ -119,11 +119,11 @@ struct MessageRouterTests { // A newly authenticated/replacement session retries the retained // message instead of losing the first ciphertext to a stale session. - router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) #expect(transport.sentPrivateMessages.count == 2) router.markDelivered("m7") - router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) #expect(transport.sentPrivateMessages.count == 2) } @@ -138,7 +138,7 @@ struct MessageRouterTests { let router = MessageRouter(transports: [transport]) router.sendPrivate("Hello", to: stablePeerID, recipientNickname: "Peer", messageID: "alias-retry") - router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID, stablePeerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID, stablePeerID]) #expect(transport.sentPrivateMessages.map(\.messageID) == ["alias-retry", "alias-retry"]) #expect(transport.sentPrivateMessages.allSatisfy { $0.peerID == stablePeerID }) @@ -161,7 +161,7 @@ struct MessageRouterTests { router.sendPrivate("Newer", to: shortPeerID, recipientNickname: "Peer", messageID: "fifo-new") transport.resetRecordings() - router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID]) #expect(transport.sentPrivateMessages.map(\.messageID) == ["fifo-old", "fifo-new"]) #expect(transport.sentPrivateMessages.map(\.peerID) == [stablePeerID, shortPeerID]) @@ -202,7 +202,7 @@ struct MessageRouterTests { router.markDelivered("r-twin", for: [shortPeerID]) } - router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID]) transport.onSendPrivateMessage = nil #expect( @@ -425,6 +425,80 @@ struct MessageRouterTests { ) } + /// The returned set is the entire contract with the flush that follows, so + /// it has to be pinned on the path where the retry actually sends. Every + /// other test either ignores the return or exercises the empty case, which + /// left the line that records a transmission unguarded: deleting it kept + /// the suite green while reopening the double-send it exists to prevent. + @Test @MainActor + func authenticationRetry_reportsExactlyTheIDsItPutOnTheAir() async { + let peerID = PeerID(str: "0000000000000029") + let transport = MockTransport() + let router = MessageRouter(transports: [transport]) + + // Composed while the peer was offline: queued, never transmitted, so + // never in `secureTransmissions` and never this pass's business. It is + // here so the assertion below pins the set exactly rather than merely + // proving it is non-empty — an over-inclusive bug would name this one. + transport.connectedPeers = [] + transport.securePeers = [] + router.sendPrivate("Offline", to: peerID, recipientNickname: "Peer", messageID: "off-1") + + // Transmitted securely, so it lands in `secureTransmissions` and stays + // queued pending an ack — exactly what the retry pass re-sends. + transport.connectedPeers = [peerID] + transport.securePeers = [peerID] + router.sendPrivate("On the air", to: peerID, recipientNickname: "Peer", messageID: "air-1") + transport.resetRecordings() + + let retried = router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["air-1"], + "the retry sent mail that was never securely transmitted" + ) + #expect( + retried == ["air-1"], + "the retry must report exactly what it put on the air: under-reporting makes the flush send it again and burn a second attempt against the cap, over-reporting strands the message it wrongly named" + ) + } + + /// The retry must claim a message ID only past every check that can drop + /// the candidate rather than send it. Claiming in the guard chain let an + /// expired copy take the ID, drop itself, and suppress the live twin under + /// the other alias — which then went unsent for the whole pass. The flush + /// afterwards still recovered it, but the drop had already reported the + /// message failed to the UI while it was in fact about to deliver. + @Test @MainActor + func authenticationRetry_anExpiredCandidateDoesNotSuppressTheLiveTwin() async { + let shortPeerID = PeerID(str: "0000000000000030") + let stablePeerID = PeerID(hexData: Data(repeating: 0x30, count: 32)) + let transport = MockTransport() + transport.connectedPeers = [shortPeerID, stablePeerID] + transport.securePeers = [shortPeerID, stablePeerID] + let clock = MutableTestClock() + let router = MessageRouter(transports: [transport], now: { clock.now }) + + // The ephemeral copy ages past the TTL. The stable copy of the same ID + // is composed fresh, so only one of the twins is droppable. + router.sendPrivate("Twin", to: shortPeerID, recipientNickname: "Peer", messageID: "ttl-twin") + clock.now = clock.now.addingTimeInterval(25 * 60 * 60) + router.sendPrivate("Twin", to: stablePeerID, recipientNickname: "Peer", messageID: "ttl-twin") + transport.resetRecordings() + + let retried = router.retrySecurePrivateMessagesAfterAuthentication(for: [shortPeerID, stablePeerID]) + + #expect( + transport.sentPrivateMessages.map(\.messageID) == ["ttl-twin"], + "the expired copy claimed the ID and suppressed the twin that was still deliverable" + ) + #expect( + transport.sentPrivateMessages.map(\.peerID) == [stablePeerID], + "the retry sent under the expired alias rather than the live one" + ) + #expect(retried == ["ttl-twin"]) + } + /// 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 @@ -484,7 +558,7 @@ struct MessageRouterTests { // the session becomes secure, the router's targeted auth retry must // stay silent instead of producing a second copy. transport.securePeers = [peerID] - router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) #expect(transport.sentPrivateMessages.count == 1) router.markDelivered("normal-handshake") @@ -533,10 +607,10 @@ struct MessageRouterTests { transport.resetRecordings() transport.securePeers = [securePeer, pendingPeer] - router.retrySecurePrivateMessagesAfterAuthentication(for: [pendingPeer]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [pendingPeer]) #expect(transport.sentPrivateMessages.isEmpty) - router.retrySecurePrivateMessagesAfterAuthentication(for: [securePeer]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [securePeer]) #expect(transport.sentPrivateMessages.count == 2) #expect(Set(transport.sentPrivateMessages.map(\.messageID)) == [promotedID, clearedID]) #expect(transport.sentPrivateMessages.allSatisfy { $0.peerID == securePeer }) @@ -561,7 +635,7 @@ struct MessageRouterTests { #expect(transport.sentPrivateMessages.count == 2) transport.securePeers = [peerID] - router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) #expect(transport.sentPrivateMessages.count == 2) router.markDelivered("session-lost") @@ -920,7 +994,7 @@ struct MessageRouterTests { router.sendPrivate("Hello", to: peerID, recipientNickname: "Peer", messageID: "secure-retry") for _ in 0..<10 { - router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + _ = router.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) } #expect(dropped == ["secure-retry"]) From 9e95374bca5f2d278469fc57075a54bb1453593f Mon Sep 17 00:00:00 2001 From: ecgang Date: Fri, 31 Jul 2026 10:07:39 -0700 Subject: [PATCH 9/9] Restore the periphery ignore the skip-set fix made necessary again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing the alias-scoped sweep from flushOutbox took away the only direct read of PeerMessageKey.peerID, so Periphery flags it assign-only and the Dead Code check goes red. That sweep is exactly what commit a450204 cited when it dropped this suppression, and exactly what the Critical fix in 1adf220 had to delete: it derived the flush's skip set from every secureTransmissions entry under the aliases with no liveness check. The property is not dead. It is what scopes secureTransmissions and dropMessage per peer, and it is read through the synthesized Hashable conformance, which the indexer cannot attribute. Deleting it to satisfy the scanner would let one alias's drop clear its twin under the other — the bug this PR spent three commits closing. Verified locally with periphery scan --strict: the MessageRouter warning is gone and the restored directive is not itself reported superfluous, which is the failure mode that turned this red the last time round. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/Services/MessageRouter.swift | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index 1577153e..06ecc4f8 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -35,10 +35,19 @@ final class MessageRouter { typealias QueuedMessage = MessageOutboxStore.QueuedMessage private struct PeerMessageKey: Hashable { - // 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. + // 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. + // + // The alias-scoped sweep in `flushOutbox(forAliases:)` used to read + // this directly, which is why the suppression was dropped in a450204. + // That sweep was the bug: it derived the flush's skip set from every + // `secureTransmissions` entry under the aliases with no liveness + // check, so removing it brought the false positive back. Do not delete + // the property to satisfy the scanner — it is what scopes + // `secureTransmissions` and `dropMessage` per peer, and collapsing it + // would let one alias's drop clear its twin under the other. let peerID: PeerID let messageID: String }