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")