Skip by message ID, not by peer/message pair, and claim only live candidates

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) <noreply@anthropic.com>

bitchat/Services/MessageRouter.swift

@@ -734,18 +734,30 @@ final class MessageRouter {
         )
 
         var visitedPeerIDs = Set<PeerID>()
+        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<String>()
+        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<String>()
 
         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,

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