From 1adf22070b777805b92e4ffc1a68658020049d39 Mon Sep 17 00:00:00 2001 From: ecgang Date: Fri, 31 Jul 2026 08:47:10 -0700 Subject: [PATCH] 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(