diff --git a/bitchat/Services/MessageRouter.swift b/bitchat/Services/MessageRouter.swift index 3a733029..0dc88fb9 100644 --- a/bitchat/Services/MessageRouter.swift +++ b/bitchat/Services/MessageRouter.swift @@ -37,15 +37,6 @@ final class MessageRouter { private struct PeerMessageKey: Hashable { let peerID: PeerID let messageID: String - - static func == (lhs: PeerMessageKey, rhs: PeerMessageKey) -> Bool { - lhs.peerID == rhs.peerID && lhs.messageID == rhs.messageID - } - - func hash(into hasher: inout Hasher) { - hasher.combine(peerID) - hasher.combine(messageID) - } } private let transports: [Transport] @@ -399,7 +390,7 @@ final class MessageRouter { /// every copy of the message. Authenticated remote receipts must use the /// peer-bound overload below instead. func markDelivered(_ messageID: String) { - clearRetainedMessage(messageID, allowedPeerIDs: nil) + clearRetainedMessage(messageID) } /// Stops retaining a message only for the authenticated conversation @@ -411,15 +402,9 @@ final class MessageRouter { _ = markDelivered(messageID, for: Array(peerIDs)) } - private func clearRetainedMessage( - _ messageID: String, - allowedPeerIDs: Set? - ) { + private func clearRetainedMessage(_ messageID: String) { var cleared = false for (peerID, queue) in outbox { - if let allowedPeerIDs, !allowedPeerIDs.contains(peerID) { - continue - } let filtered = queue.filter { $0.messageID != messageID } guard filtered.count != queue.count else { continue } outbox[peerID] = filtered.isEmpty ? nil : filtered diff --git a/bitchat/ViewModels/ChatDeliveryCoordinator.swift b/bitchat/ViewModels/ChatDeliveryCoordinator.swift index 7f85e3ac..dcaf95e6 100644 --- a/bitchat/ViewModels/ChatDeliveryCoordinator.swift +++ b/bitchat/ViewModels/ChatDeliveryCoordinator.swift @@ -42,8 +42,15 @@ protocol ChatDeliveryContext: AnyObject { /// Confirms receipt so the message router stops retaining the message for resend. func markMessageDelivered(_ messageID: String) /// Peer-bound form for authenticated remote receipts. Only the supplied - /// conversation aliases may have retained state terminalized. + /// conversation aliases may have retained state terminalized. This is the + /// router-side clear only: it is safe without a conversation lookup + /// because the router scopes removal to the acking peer's own queues. func markMessageDelivered(_ messageID: String, from peerIDs: Set) + /// Releases the media reconnect retry for a private transfer. Unlike the + /// router clear above this is keyed only by the stable media message ID, + /// so callers must first bind the receipt to one of our outgoing + /// conversations for the acking peer. + func confirmPrivateMediaDelivery(_ messageID: String) /// Returns true only when `messageID` is one of our outgoing messages in /// at least one of the authenticated peer's direct-conversation aliases. func isOutgoingPrivateMessage(_ messageID: String, toAny peerIDs: Set) -> Bool @@ -89,9 +96,9 @@ extension ChatViewModel: ChatDeliveryContext { func markMessageDelivered(_ messageID: String, from peerIDs: Set) { messageRouter.markDelivered(messageID, from: peerIDs) - // The caller has already bound this receipt to one of our outgoing - // conversations. Release the matching media retry only after that - // peer-scoped validation and accepted delivery-status transition. + } + + func confirmPrivateMediaDelivery(_ messageID: String) { mediaTransferCoordinator.confirmPrivateMediaDelivery( messageID: messageID ) @@ -165,10 +172,18 @@ final class ChatDeliveryCoordinator { return true } - /// Applies an authenticated remote delivery/read receipt only when it - /// belongs to one of our outgoing messages in that peer's conversation. - /// Retry state is cleared after, never before, the status transition is - /// accepted by the store. + /// Applies an authenticated remote delivery/read receipt. The durable + /// retry state is always released for the acking peer's own aliases + /// (`markMessageDelivered(_:from:)` is peer-scoped on the router side, so + /// a receipt can only terminalize messages queued for that peer). Only the + /// UI status transition and the media-retry release are gated on the + /// in-memory conversation holding the message as one of ours: after a + /// force-quit → relaunch the durable outbox is restored + /// while the conversation may not be, and discarding the ack there would + /// re-send an already-delivered message on every flush/auth event until + /// the attempt cap marks it failed. Mirrors the Nostr path + /// (`ChatPrivateConversationCoordinator.handleDelivered`), which clears + /// retained state unconditionally. @MainActor @discardableResult func updateAcknowledgedMessageDeliveryStatus( @@ -182,8 +197,9 @@ final class ChatDeliveryCoordinator { default: return false } - guard !peerIDAliases.isEmpty, - context.isOutgoingPrivateMessage(messageID, toAny: peerIDAliases), + guard !peerIDAliases.isEmpty else { return false } + context.markMessageDelivered(messageID, from: peerIDAliases) + guard context.isOutgoingPrivateMessage(messageID, toAny: peerIDAliases), context.setDeliveryStatus( status, forMessageID: messageID, @@ -191,7 +207,10 @@ final class ChatDeliveryCoordinator { ) else { return false } - context.markMessageDelivered(messageID, from: peerIDAliases) + // The receipt is now bound to one of our outgoing conversations for + // the acking peer; only then release the media reconnect retry, whose + // stable message ID is not peer-scoped. + context.confirmPrivateMediaDelivery(messageID) context.notifyUIChanged() return true } diff --git a/bitchatTests/ChatViewModelDeliveryStatusTests.swift b/bitchatTests/ChatViewModelDeliveryStatusTests.swift index 70c92e81..15a1edea 100644 --- a/bitchatTests/ChatViewModelDeliveryStatusTests.swift +++ b/bitchatTests/ChatViewModelDeliveryStatusTests.swift @@ -13,8 +13,11 @@ import BitFoundation // MARK: - Test Helpers @MainActor -private func makeTestableViewModel() -> (viewModel: ChatViewModel, transport: MockTransport) { - let keychain = MockKeychain() +private func makeTestableViewModel( + keychain injectedKeychain: MockKeychain? = nil, + outboxStore: MessageOutboxStore? = nil +) -> (viewModel: ChatViewModel, transport: MockTransport) { + let keychain = injectedKeychain ?? MockKeychain() let keychainHelper = MockKeychainHelper() let idBridge = NostrIdentityBridge(keychain: keychainHelper) let identityManager = MockIdentityManager(keychain) @@ -24,7 +27,8 @@ private func makeTestableViewModel() -> (viewModel: ChatViewModel, transport: Mo keychain: keychain, idBridge: idBridge, identityManager: identityManager, - transport: transport + transport: transport, + outboxStore: outboxStore ) return (viewModel, transport) @@ -454,6 +458,65 @@ struct ChatViewModelDeliveryStatusTests { #expect(viewModel.sentReadReceipts == ["keep-receipt"]) } + // MARK: - Relaunch-Restored Outbox Tests + + @Test @MainActor + func deliveryAckAfterRelaunchClearsRestoredOutboxWithoutConversation() async { + // Force-quit → relaunch: the durable outbox restores the retained DM, + // but the in-memory conversation store starts empty. The delivery ack + // must still clear the router's retained copy — gating it on the + // conversation lookup would leave the entry re-sending on every + // flush/auth event until the attempt cap marks it failed despite + // delivery. + let fileURL = FileManager.default.temporaryDirectory + .appendingPathComponent("relaunch-ack-\(UUID().uuidString).sealed") + defer { try? FileManager.default.removeItem(at: fileURL) } + let keychain = MockKeychain() + let peerID = PeerID(str: "0102030405060708") + let messageID = "relaunch-retained-dm" + + // Pre-quit state: one retained private message on disk. + MessageOutboxStore(keychain: keychain, fileURL: fileURL).save([ + peerID: [MessageOutboxStore.QueuedMessage( + content: "Sent before force-quit", + nickname: "Peer", + messageID: messageID, + timestamp: Date() + )] + ]) + + // Relaunch: fresh view model over the same durable store. + let (viewModel, transport) = makeTestableViewModel( + keychain: keychain, + outboxStore: MessageOutboxStore(keychain: keychain, fileURL: fileURL) + ) + #expect(viewModel.privateChats[peerID] == nil) + + // The peer's delivery ack arrives with no conversation in the store. + // No UI transition is possible (and none must crash), but the durable + // retry state has to clear. + let didUpdate = viewModel.deliveryCoordinator + .updateAcknowledgedMessageDeliveryStatus( + messageID, + status: .delivered(to: "Peer", at: Date()), + from: [peerID] + ) + #expect(!didUpdate) + #expect(viewModel.privateChats[peerID] == nil) + + // Neither a flush nor a replacement-handshake auth event may re-send + // the already-delivered message. + transport.connectedPeers.insert(peerID) + transport.securePeers = [peerID] + viewModel.messageRouter.flushOutbox(for: peerID) + viewModel.messageRouter.retrySecurePrivateMessagesAfterAuthentication(for: [peerID]) + #expect(transport.sentPrivateMessages.isEmpty) + + // The clear reached the durable snapshot: the next relaunch restores + // nothing. + #expect(MessageOutboxStore(keychain: keychain, fileURL: fileURL).load().isEmpty) + } + // MARK: - Public Timeline Status Tests @Test @MainActor @@ -709,6 +772,7 @@ private final class MockChatDeliveryContext: ChatDeliveryContext { private(set) var notifyUIChangedCount = 0 private(set) var markedDeliveredMessageIDs: [String] = [] private(set) var peerBoundDeliveredMessages: [(messageID: String, peerIDs: Set)] = [] + private(set) var confirmedMediaMessageIDs: [String] = [] @discardableResult func setDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool { @@ -754,6 +818,10 @@ private final class MockChatDeliveryContext: ChatDeliveryContext { peerBoundDeliveredMessages.append((messageID, peerIDs)) } + func confirmPrivateMediaDelivery(_ messageID: String) { + confirmedMediaMessageIDs.append(messageID) + } + func isOutgoingPrivateMessage(_ messageID: String, toAny peerIDs: Set) -> Bool { peerIDs.contains { peerID in contextMessages(for: peerID).contains { message in @@ -963,13 +1031,19 @@ struct ChatDeliveryCoordinatorContextTests { ) #expect(isSent(coordinator.deliveryStatus(for: messageID))) - #expect(context.peerBoundDeliveredMessages.isEmpty) + // The router-side clear runs, but bound only to the wrong peer's own + // aliases — a scoped no-op that cannot touch the intended peer's + // retained copy. Status, media retry, and UI stay untouched. + #expect(context.peerBoundDeliveredMessages.count == 1) + #expect(context.peerBoundDeliveredMessages[0].messageID == messageID) + #expect(context.peerBoundDeliveredMessages[0].peerIDs == [otherPeer]) #expect(context.markedDeliveredMessageIDs.isEmpty) + #expect(context.confirmedMediaMessageIDs.isEmpty) #expect(context.notifyUIChangedCount == 0) } @Test @MainActor - func rejectedStaleReceiptDoesNotTerminalizeRetryState() async { + func rejectedStaleReceiptDoesNotDowngradeStatusOrNotify() async { let context = MockChatDeliveryContext() let coordinator = ChatDeliveryCoordinator(context: context) let peerID = PeerID(str: "0102030405060708") @@ -990,8 +1064,14 @@ struct ChatDeliveryCoordinatorContextTests { #expect(!didUpdate) #expect(isRead(coordinator.deliveryStatus(for: messageID))) - #expect(context.peerBoundDeliveredMessages.isEmpty) + // The peer-scoped router clear re-runs (idempotent: the earlier read + // receipt already emptied this peer's retained copy), but the stale + // delivered ack must not downgrade the read status, release media, or + // notify the UI. + #expect(context.peerBoundDeliveredMessages.count == 1) + #expect(context.peerBoundDeliveredMessages[0].peerIDs == [peerID]) #expect(context.markedDeliveredMessageIDs.isEmpty) + #expect(context.confirmedMediaMessageIDs.isEmpty) #expect(context.notifyUIChangedCount == 0) } diff --git a/bitchatTests/Performance/PerformanceBaselineTests.swift b/bitchatTests/Performance/PerformanceBaselineTests.swift index 7875af0e..453a4a9a 100644 --- a/bitchatTests/Performance/PerformanceBaselineTests.swift +++ b/bitchatTests/Performance/PerformanceBaselineTests.swift @@ -750,6 +750,7 @@ private final class PerfDeliveryContext: ChatDeliveryContext { func notifyUIChanged() {} func markMessageDelivered(_ messageID: String) {} func markMessageDelivered(_ messageID: String, from peerIDs: Set) {} + func confirmPrivateMediaDelivery(_ messageID: String) {} func isOutgoingPrivateMessage(_ messageID: String, toAny peerIDs: Set) -> Bool { true }