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(