diff --git a/bitchat/App/ConversationStore.swift b/bitchat/App/ConversationStore.swift index 4e752b9b..f7fff03a 100644 --- a/bitchat/App/ConversationStore.swift +++ b/bitchat/App/ConversationStore.swift @@ -64,10 +64,25 @@ final class Conversation: ObservableObject, Identifiable { return messages[index] } + /// All message IDs currently in this conversation (unordered). + var messageIDs: Dictionary.Keys { + indexByMessageID.keys + } + // MARK: Store-internal mutations + /// Result of an ordered insert. `trimmedMessageIDs` reports messages + /// evicted by the cap so the store can keep its message-ID → + /// conversation map exact. + fileprivate struct InsertResult { + let inserted: Bool + let trimmedMessageIDs: [String] + + static let duplicate = InsertResult(inserted: false, trimmedMessageIDs: []) + } + fileprivate enum UpsertOutcome { - case appended + case appended(trimmedMessageIDs: [String]) case updated } @@ -75,9 +90,9 @@ final class Conversation: ObservableObject, Identifiable { /// Fast path appends when the timestamp is >= the current tail; /// otherwise a binary search finds the upper-bound insertion point so /// arrival order is preserved among equal timestamps. - /// Returns `false` if a message with the same ID already exists. - fileprivate func insert(_ message: BitchatMessage) -> Bool { - guard indexByMessageID[message.id] == nil else { return false } + /// Reports `inserted: false` if a message with the same ID already exists. + fileprivate func insert(_ message: BitchatMessage) -> InsertResult { + guard indexByMessageID[message.id] == nil else { return .duplicate } if let last = messages.last, message.timestamp < last.timestamp { let index = insertionIndex(for: message.timestamp) @@ -88,8 +103,7 @@ final class Conversation: ObservableObject, Identifiable { indexByMessageID[message.id] = messages.count - 1 } - trimIfNeeded() - return true + return InsertResult(inserted: true, trimmedMessageIDs: trimIfNeeded()) } /// Replace-or-append by message ID. An existing message keeps its @@ -100,14 +114,14 @@ final class Conversation: ObservableObject, Identifiable { messages[index] = message return .updated } - _ = insert(message) - return .appended + let result = insert(message) + return .appended(trimmedMessageIDs: result.trimmedMessageIDs) } /// Applies a delivery status keyed by message ID, honoring the - /// no-downgrade rule (mirrors `ChatDeliveryCoordinator.shouldSkipUpdate`): - /// equal statuses are skipped, and `.read` is never downgraded to - /// `.delivered` or `.sent`. + /// no-downgrade rule (the SOLE enforcement point — every delivery + /// update flows through the store): equal statuses are skipped, and + /// `.read` is never downgraded to `.delivered` or `.sent`. /// Returns `true` when the status was applied. fileprivate func applyDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool { guard let index = indexByMessageID[messageID] else { return false } @@ -197,14 +211,17 @@ final class Conversation: ObservableObject, Identifiable { } } - private func trimIfNeeded() { - guard messages.count > cap else { return } + /// Trims oldest messages over the cap; returns the trimmed message IDs. + private func trimIfNeeded() -> [String] { + guard messages.count > cap else { return [] } let overflow = messages.count - cap - for message in messages.prefix(overflow) { - indexByMessageID.removeValue(forKey: message.id) + let trimmedIDs = messages.prefix(overflow).map(\.id) + for id in trimmedIDs { + indexByMessageID.removeValue(forKey: id) } messages.removeFirst(overflow) reindex(from: 0) + return trimmedIDs } } @@ -242,6 +259,19 @@ final class ConversationStore: ObservableObject { private(set) var conversationsByID: [ConversationID: Conversation] = [:] + /// Store-level message-ID → conversation-membership map for ID-only + /// lookups (delivery receipts arrive with a message ID, not a + /// conversation). Maintained incrementally at every mutation point — + /// all mutation is centralized in the intent API below, so the map is + /// exact, never scanned or rebuilt. + /// + /// The value is a `Set` because a private message can legitimately live + /// in TWO direct conversations: step 2's raw per-peer keying mirrors a + /// message into both the stable-key and ephemeral-peer chats + /// (`mirrorToEphemeralIfNeeded`). A delivery update must reach both + /// copies. + private var conversationIDsByMessageID: [String: Set] = [:] + let changes = PassthroughSubject() // MARK: Intent API @@ -264,7 +294,10 @@ final class ConversationStore: ObservableObject { @discardableResult func append(_ message: BitchatMessage, to id: ConversationID) -> Bool { let conversation = conversation(for: id) - guard conversation.insert(message) else { return false } + let result = conversation.insert(message) + guard result.inserted else { return false } + registerMessageID(message.id, in: id) + unregisterMessageIDs(result.trimmedMessageIDs, from: id) changes.send(.appended(id, message)) return true } @@ -273,7 +306,9 @@ final class ConversationStore: ObservableObject { func upsertByID(_ message: BitchatMessage, in id: ConversationID) { let conversation = conversation(for: id) switch conversation.upsert(message) { - case .appended: + case .appended(let trimmedMessageIDs): + registerMessageID(message.id, in: id) + unregisterMessageIDs(trimmedMessageIDs, from: id) changes.send(.appended(id, message)) case .updated: changes.send(.updated(id, messageID: message.id)) @@ -293,6 +328,44 @@ final class ConversationStore: ObservableObject { return true } + /// Applies a delivery status to EVERY conversation containing + /// `messageID` (ID-only — delivery receipts don't know conversations; + /// mirrored private copies live in two direct chats). Returns `false` + /// when the message is unknown or no copy changed (equal status or + /// downgrade — read beats delivered beats sent). + /// + /// `BitchatMessage` is a reference type, so mirrored copies sharing one + /// instance are updated by the first apply; subsequent conversations + /// skip as already-equal (state stays correct everywhere, the + /// `.statusChanged` event fires for the conversation that applied). + @discardableResult + func setDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool { + guard let ids = conversationIDsByMessageID[messageID] else { return false } + var applied = false + for id in ids where setDeliveryStatus(status, forMessageID: messageID, in: id) { + applied = true + } + return applied + } + + /// Current delivery status of `messageID` in whichever conversation + /// holds it (mirrored copies share status — see `setDeliveryStatus`). + func deliveryStatus(forMessageID messageID: String) -> DeliveryStatus? { + guard let ids = conversationIDsByMessageID[messageID] else { return nil } + for id in ids { + if let status = conversationsByID[id]?.message(withID: messageID)?.deliveryStatus { + return status + } + } + return nil + } + + /// Every conversation currently containing `messageID` (empty when the + /// message is unknown). + func conversationIDs(forMessageID messageID: String) -> Set { + conversationIDsByMessageID[messageID] ?? [] + } + func markRead(_ id: ConversationID) { guard unreadConversations.contains(id) else { return } unreadConversations.remove(id) @@ -329,7 +402,13 @@ final class ConversationStore: ObservableObject { let destinationConversation = conversation(for: destination) for message in sourceConversation.messages { - _ = destinationConversation.insert(message) + let result = destinationConversation.insert(message) + guard result.inserted else { continue } + registerMessageID(message.id, in: destination) + unregisterMessageIDs(result.trimmedMessageIDs, from: destination) + } + for messageID in sourceConversation.messageIDs { + unregisterMessageID(messageID, from: source) } let wasUnread = unreadConversations.contains(source) @@ -359,6 +438,7 @@ final class ConversationStore: ObservableObject { let removed = conversation.remove(messageID: messageID) else { return nil } + unregisterMessageID(messageID, from: id) changes.send(.messageRemoved(id, messageID: messageID)) return removed } @@ -368,7 +448,9 @@ final class ConversationStore: ObservableObject { /// conversation is consistent. No-op for unknown conversations. func removeMessages(from id: ConversationID, where predicate: (BitchatMessage) -> Bool) { guard let conversation = conversationsByID[id] else { return } - for messageID in conversation.removeAll(where: predicate) { + let removedIDs = conversation.removeAll(where: predicate) + unregisterMessageIDs(removedIDs, from: id) + for messageID in removedIDs { changes.send(.messageRemoved(id, messageID: messageID)) } } @@ -377,6 +459,9 @@ final class ConversationStore: ObservableObject { /// its unread/selection state) alive. func clear(_ id: ConversationID) { guard let conversation = conversationsByID[id] else { return } + for messageID in conversation.messageIDs { + unregisterMessageID(messageID, from: id) + } conversation.clearMessages() changes.send(.cleared(id)) } @@ -384,7 +469,10 @@ final class ConversationStore: ObservableObject { /// Removes a conversation entirely, including unread state; clears the /// selection if it pointed at the removed conversation. func removeConversation(_ id: ConversationID) { - guard conversationsByID.removeValue(forKey: id) != nil else { return } + guard let conversation = conversationsByID.removeValue(forKey: id) else { return } + for messageID in conversation.messageIDs { + unregisterMessageID(messageID, from: id) + } conversationIDs.removeAll { $0 == id } unreadConversations.remove(id) if selectedConversationID == id { @@ -400,6 +488,7 @@ final class ConversationStore: ObservableObject { conversationsByID.removeAll() conversationIDs.removeAll() unreadConversations.removeAll() + conversationIDsByMessageID.removeAll() if selectedConversationID != nil { selectedConversationID = nil } @@ -411,6 +500,26 @@ final class ConversationStore: ObservableObject { // MARK: Internals + private func registerMessageID(_ messageID: String, in id: ConversationID) { + conversationIDsByMessageID[messageID, default: []].insert(id) + } + + private func unregisterMessageID(_ messageID: String, from id: ConversationID) { + guard var ids = conversationIDsByMessageID[messageID] else { return } + ids.remove(id) + if ids.isEmpty { + conversationIDsByMessageID.removeValue(forKey: messageID) + } else { + conversationIDsByMessageID[messageID] = ids + } + } + + private func unregisterMessageIDs(_ messageIDs: [String], from id: ConversationID) { + for messageID in messageIDs { + unregisterMessageID(messageID, from: id) + } + } + private static func cap(for id: ConversationID) -> Int { switch id { case .mesh: @@ -471,13 +580,23 @@ extension ConversationStore { } /// `true` when any direct conversation contains a message with `messageID` - /// (O(1) per conversation via the incremental ID index). + /// (O(1) via the store-level message-ID → conversation map). func directConversationsContainMessage(withID messageID: String) -> Bool { + conversationIDs(forMessageID: messageID).contains { id in + if case .direct = id { return true } + return false + } + } + + /// Message IDs across all direct conversations (read-receipt pruning + /// keeps only receipts whose messages still exist). + func directMessageIDs() -> Set { + var messageIDs = Set() for (id, conversation) in conversationsByID { guard case .direct = id else { continue } - if conversation.containsMessage(withID: messageID) { return true } + messageIDs.formUnion(conversation.messageIDs) } - return false + return messageIDs } /// Removes every direct conversation (panic clear). @@ -501,12 +620,10 @@ extension ConversationStore { /// message, if any. @discardableResult func removePublicMessage(withID messageID: String) -> BitchatMessage? { - for (id, conversation) in conversationsByID { + for id in conversationIDs(forMessageID: messageID) { switch id { case .mesh, .geohash: - if conversation.containsMessage(withID: messageID) { - return removeMessage(withID: messageID, from: id) - } + return removeMessage(withID: messageID, from: id) case .direct: continue } diff --git a/bitchat/ViewModels/ChatDeliveryCoordinator.swift b/bitchat/ViewModels/ChatDeliveryCoordinator.swift index 8acf7bec..f2fc13c0 100644 --- a/bitchat/ViewModels/ChatDeliveryCoordinator.swift +++ b/bitchat/ViewModels/ChatDeliveryCoordinator.swift @@ -12,18 +12,19 @@ import Foundation /// coordinators off their `unowned let viewModel: ChatViewModel` back-refs. @MainActor protocol ChatDeliveryContext: AnyObject { - /// Get-only derived views of the `ConversationStore`. `BitchatMessage` - /// is a reference type, so the public-timeline status patch below writes - /// through the shared message objects; step 4 replaces this with - /// `setDeliveryStatus(for:in:)` store intents. - var messages: [BitchatMessage] { get } - var privateChats: [PeerID: [BitchatMessage]] { get } var isStartupPhase: Bool { get } - /// Applies a delivery status to a private message by ID (single-writer - /// store intent; full delivery migration is step 4). Returns `false` - /// when the message is unknown or the update would downgrade the status. + /// Applies a delivery status to every copy of the message across + /// conversations (`ConversationStore` intent, ID-only: the store's + /// message-ID → conversation map resolves which conversations hold the + /// message, including mirrored ephemeral/stable private copies). The + /// no-downgrade rule is enforced in the store. Returns `false` when the + /// message is unknown or no copy changed. @discardableResult - func setPrivateDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String, peerID: PeerID) -> Bool + func setDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool + /// Current delivery status of the message in whichever conversation holds it. + func deliveryStatus(forMessageID messageID: String) -> DeliveryStatus? + /// Message IDs across all direct conversations (read-receipt pruning). + func privateMessageIDs() -> Set /// Drops every recorded read receipt whose message ID is not in `validMessageIDs`. /// Returns the number of receipts removed. (Single mutation path for the /// owner's `sentReadReceipts`; this coordinator never reads the raw set.) @@ -35,6 +36,19 @@ protocol ChatDeliveryContext: AnyObject { } extension ChatViewModel: ChatDeliveryContext { + @discardableResult + func setDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool { + conversations.setDeliveryStatus(status, forMessageID: messageID) + } + + func deliveryStatus(forMessageID messageID: String) -> DeliveryStatus? { + conversations.deliveryStatus(forMessageID: messageID) + } + + func privateMessageIDs() -> Set { + conversations.directMessageIDs() + } + func notifyUIChanged() { objectWillChange.send() } @@ -44,14 +58,12 @@ extension ChatViewModel: ChatDeliveryContext { } } +/// Thin mapper from delivery events (read receipts, transport delivery +/// callbacks) onto `ConversationStore` delivery intents, plus read-receipt +/// retention cleanup. The store's message-ID → conversation map replaces the +/// positional `messageLocationIndex` this coordinator used to maintain. final class ChatDeliveryCoordinator { private unowned let context: any ChatDeliveryContext - private var messageLocationIndex: [String: Set] = [:] - private var indexedPublicMessageCount = 0 - private var indexedPublicTailMessageID: String? - private var indexedPrivateMessageCounts: [PeerID: Int] = [:] - private var indexedPrivateTailMessageIDs: [PeerID: String] = [:] - private var hasBuiltMessageLocationIndex = false init(context: any ChatDeliveryContext) { self.context = context @@ -59,15 +71,9 @@ final class ChatDeliveryCoordinator { @MainActor func cleanupOldReadReceipts() { - guard !context.isStartupPhase, !context.privateChats.isEmpty else { - return - } - - let validMessageIDs = Set( - context.privateChats.values.flatMap { messages in - messages.map(\.id) - } - ) + guard !context.isStartupPhase else { return } + let validMessageIDs = context.privateMessageIDs() + guard !validMessageIDs.isEmpty else { return } let removedCount = context.pruneSentReadReceipts(keeping: validMessageIDs) if removedCount > 0 { @@ -90,9 +96,7 @@ final class ChatDeliveryCoordinator { @MainActor func deliveryStatus(for messageID: String) -> DeliveryStatus? { - withValidLocations(for: messageID) { locations in - locations.lazy.compactMap { self.deliveryStatus(at: $0) }.first - } + context.deliveryStatus(forMessageID: messageID) } @MainActor @@ -106,222 +110,10 @@ final class ChatDeliveryCoordinator { break } - var didUpdateStatus = false - let locations = withValidLocations(for: messageID) { $0 } - guard !locations.isEmpty else { return false } - - for location in locations { - guard case .publicTimeline(let index) = location, - index < context.messages.count, - context.messages[index].id == messageID else { - continue - } - - let currentStatus = context.messages[index].deliveryStatus - if !shouldSkipUpdate(currentStatus: currentStatus, newStatus: status) { - context.messages[index].deliveryStatus = status - didUpdateStatus = true - } - } - - for location in locations { - guard case .privateChat(let peerID, let index) = location, - let chatMessages = context.privateChats[peerID], - index < chatMessages.count, - chatMessages[index].id == messageID else { - continue - } - - let currentStatus = chatMessages[index].deliveryStatus - guard !shouldSkipUpdate(currentStatus: currentStatus, newStatus: status) else { continue } - - if context.setPrivateDeliveryStatus(status, forMessageID: messageID, peerID: peerID) { - didUpdateStatus = true - } - } - - if didUpdateStatus { - context.notifyUIChanged() - } - - return didUpdateStatus - } -} - -private extension ChatDeliveryCoordinator { - enum MessageLocation: Hashable { - case publicTimeline(index: Int) - case privateChat(peerID: PeerID, index: Int) - } - - func shouldSkipUpdate(currentStatus: DeliveryStatus?, newStatus: DeliveryStatus) -> Bool { - guard let currentStatus else { return false } - if currentStatus == newStatus { return true } - - switch (currentStatus, newStatus) { - case (.read, .delivered), (.read, .sent): - return true - default: + guard context.setDeliveryStatus(status, forMessageID: messageID) else { return false } - } - - @MainActor - func withValidLocations( - for messageID: String, - _ body: (Set) -> T - ) -> T { - let didRebuildIndex = refreshMessageLocationIndexForGrowth() - - if let locations = messageLocationIndex[messageID], - locations.allSatisfy({ isLocation($0, validFor: messageID) }) { - return body(locations) - } - - guard !didRebuildIndex else { - return body(messageLocationIndex[messageID] ?? []) - } - - if messageLocationIndex[messageID] == nil { - return body([]) - } - - rebuildMessageLocationIndex() - return body(messageLocationIndex[messageID] ?? []) - } - - @MainActor - func deliveryStatus(at location: MessageLocation) -> DeliveryStatus? { - switch location { - case .publicTimeline(let index): - guard index < context.messages.count else { return nil } - return context.messages[index].deliveryStatus - case .privateChat(let peerID, let index): - guard let messages = context.privateChats[peerID], - index < messages.count else { - return nil - } - return messages[index].deliveryStatus - } - } - - @MainActor - func isLocation(_ location: MessageLocation, validFor messageID: String) -> Bool { - switch location { - case .publicTimeline(let index): - return index < context.messages.count - && context.messages[index].id == messageID - case .privateChat(let peerID, let index): - guard let messages = context.privateChats[peerID], - index < messages.count else { - return false - } - return messages[index].id == messageID - } - } - - @MainActor - @discardableResult - func refreshMessageLocationIndexForGrowth() -> Bool { - guard hasBuiltMessageLocationIndex else { - rebuildMessageLocationIndex() - return true - } - - if context.messages.count < indexedPublicMessageCount { - rebuildMessageLocationIndex() - return true - } - - if context.messages.count == indexedPublicMessageCount, - context.messages.last?.id != indexedPublicTailMessageID { - rebuildMessageLocationIndex() - return true - } - - if context.messages.count > indexedPublicMessageCount { - // Growth is only a pure append if the previously indexed tail kept - // its position; a middle insertion (out-of-order timestamp arrival) - // shifts it and invalidates every indexed location after the - // insertion point. - if indexedPublicMessageCount > 0, - context.messages[indexedPublicMessageCount - 1].id != indexedPublicTailMessageID { - rebuildMessageLocationIndex() - return true - } - for index in indexedPublicMessageCount.. indexedCount else { continue } - // Same append-only check as the public timeline above. - if indexedCount > 0, - messages[indexedCount - 1].id != indexedPrivateTailMessageIDs[peerID] { - rebuildMessageLocationIndex() - return true - } - for index in indexedCount.. = [] var isStartupPhase = false private(set) var notifyUIChangedCount = 0 private(set) var markedDeliveredMessageIDs: [String] = [] + @discardableResult + func setDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool { + store.setDeliveryStatus(status, forMessageID: messageID) + } + + func deliveryStatus(forMessageID messageID: String) -> DeliveryStatus? { + store.deliveryStatus(forMessageID: messageID) + } + + func privateMessageIDs() -> Set { + store.directMessageIDs() + } + func pruneSentReadReceipts(keeping validMessageIDs: Set) -> Int { let oldCount = sentReadReceipts.count sentReadReceipts = sentReadReceipts.intersection(validMessageIDs) @@ -406,29 +421,19 @@ private final class MockChatDeliveryContext: ChatDeliveryContext { func markMessageDelivered(_ messageID: String) { markedDeliveredMessageIDs.append(messageID) } - - @discardableResult - func setPrivateDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String, peerID: PeerID) -> Bool { - guard var chat = privateChats[peerID], - let index = chat.firstIndex(where: { $0.id == messageID }) else { - return false - } - if Conversation.shouldSkipStatusUpdate(current: chat[index].deliveryStatus, new: status) { - return false - } - chat[index].deliveryStatus = status - privateChats[peerID] = chat - return true - } } @MainActor -private func makePrivateMessage(id: String, status: DeliveryStatus) -> BitchatMessage { +private func makePrivateMessage( + id: String, + status: DeliveryStatus, + timestamp: Date = Date() +) -> BitchatMessage { BitchatMessage( id: id, sender: "me", content: "Test message", - timestamp: Date(), + timestamp: timestamp, isRelay: false, isPrivate: true, recipientNickname: "Peer", @@ -437,10 +442,28 @@ private func makePrivateMessage(id: String, status: DeliveryStatus) -> BitchatMe ) } +@MainActor +private func makePublicMessage( + id: String, + status: DeliveryStatus, + timestamp: Date = Date() +) -> BitchatMessage { + BitchatMessage( + id: id, + sender: "me", + content: "Public message", + timestamp: timestamp, + isRelay: false, + isPrivate: false, + deliveryStatus: status + ) +} + // MARK: - Coordinator Tests Against Mock Context /// Exercises `ChatDeliveryCoordinator` against `MockChatDeliveryContext` — -/// the exemplar for the narrow-dependency coordinator pattern. +/// the exemplar for the narrow-dependency coordinator pattern. State is +/// seeded into and asserted against the mock's `ConversationStore`. struct ChatDeliveryCoordinatorContextTests { @Test @MainActor @@ -449,7 +472,7 @@ struct ChatDeliveryCoordinatorContextTests { let coordinator = ChatDeliveryCoordinator(context: context) let peerID = PeerID(str: "0102030405060708") let messageID = "mock-msg-1" - context.privateChats[peerID] = [makePrivateMessage(id: messageID, status: .sent)] + context.store.append(makePrivateMessage(id: messageID, status: .sent), to: .directPeer(peerID)) let didUpdate = coordinator.updateMessageDeliveryStatus( messageID, @@ -457,7 +480,7 @@ struct ChatDeliveryCoordinatorContextTests { ) #expect(didUpdate) - #expect(isDelivered(context.privateChats[peerID]?.first?.deliveryStatus)) + #expect(isDelivered(context.store.conversation(for: .directPeer(peerID)).message(withID: messageID)?.deliveryStatus)) #expect(context.notifyUIChangedCount == 1) #expect(context.markedDeliveredMessageIDs == [messageID]) } @@ -468,13 +491,16 @@ struct ChatDeliveryCoordinatorContextTests { let coordinator = ChatDeliveryCoordinator(context: context) let peerID = PeerID(str: "0102030405060708") let messageID = "mock-msg-2" - context.privateChats[peerID] = [makePrivateMessage(id: messageID, status: .delivered(to: "Peer", at: Date()))] + context.store.append( + makePrivateMessage(id: messageID, status: .delivered(to: "Peer", at: Date())), + to: .directPeer(peerID) + ) coordinator.didReceiveReadReceipt( ReadReceipt(originalMessageID: messageID, readerID: peerID, readerNickname: "Peer") ) - #expect(isRead(context.privateChats[peerID]?.first?.deliveryStatus)) + #expect(isRead(coordinator.deliveryStatus(for: messageID))) #expect(context.notifyUIChangedCount == 1) #expect(context.markedDeliveredMessageIDs == [messageID]) } @@ -483,22 +509,12 @@ struct ChatDeliveryCoordinatorContextTests { func sentStatus_doesNotMarkDeliveredAndUnknownMessageDoesNotNotify() async { let context = MockChatDeliveryContext() let coordinator = ChatDeliveryCoordinator(context: context) - context.messages = [ - BitchatMessage( - id: "public-mock-1", - sender: "me", - content: "Public message", - timestamp: Date(), - isRelay: false, - isPrivate: false, - deliveryStatus: .sending - ) - ] + context.store.append(makePublicMessage(id: "public-mock-1", status: .sending), to: .mesh) // .sent is not a confirmed receipt — must not reach markMessageDelivered. let didUpdate = coordinator.updateMessageDeliveryStatus("public-mock-1", status: .sent) #expect(didUpdate) - #expect(isSent(context.messages.first?.deliveryStatus)) + #expect(isSent(context.store.conversation(for: .mesh).message(withID: "public-mock-1")?.deliveryStatus)) #expect(context.markedDeliveredMessageIDs.isEmpty) #expect(context.notifyUIChangedCount == 1) @@ -508,57 +524,98 @@ struct ChatDeliveryCoordinatorContextTests { #expect(context.notifyUIChangedCount == 1) } + // The old positional `messageLocationIndex` could go stale when a late + // arrival was inserted mid-array (count grew but indexed locations + // shifted). The store's per-conversation ID index is reindexed inside the + // same mutation, so staleness is structurally impossible — these tests + // pin the equivalent behavior through the new path: after out-of-order + // insertion, updates keyed by ID still land on the right messages. + @Test @MainActor - func middleInsertedMessage_isFoundAfterIndexWasBuilt() async { + func middleInsertedMessage_isStillUpdatableByID() async { let context = MockChatDeliveryContext() let coordinator = ChatDeliveryCoordinator(context: context) - let makePublic = { (id: String) in - BitchatMessage( - id: id, - sender: "me", - content: "Public message", - timestamp: Date(), - isRelay: false, - isPrivate: false, - deliveryStatus: .sending - ) - } - context.messages = [makePublic("public-a"), makePublic("public-b")] + context.store.append( + makePublicMessage(id: "public-a", status: .sending, timestamp: Date(timeIntervalSince1970: 10)), + to: .mesh + ) + context.store.append( + makePublicMessage(id: "public-b", status: .sending, timestamp: Date(timeIntervalSince1970: 30)), + to: .mesh + ) - // Build the incremental index. #expect(coordinator.updateMessageDeliveryStatus("public-a", status: .sent)) - // Out-of-order arrival: PublicMessagePipeline inserts by timestamp, - // so the count grows while the tail ID stays the same. - context.messages.insert(makePublic("public-mid"), at: 1) + // Out-of-order arrival: the store inserts by timestamp, shifting the + // tail's position. + context.store.append( + makePublicMessage(id: "public-mid", status: .sending, timestamp: Date(timeIntervalSince1970: 20)), + to: .mesh + ) + let mesh = context.store.conversation(for: .mesh) + #expect(mesh.messages.map(\.id) == ["public-a", "public-mid", "public-b"]) - // The inserted message must be locatable, and the shifted tail must - // not be updated through a stale index entry. + // Both the inserted message and the shifted tail stay updatable. #expect(coordinator.updateMessageDeliveryStatus("public-mid", status: .sent)) - #expect(isSent(context.messages[1].deliveryStatus)) + #expect(isSent(mesh.message(withID: "public-mid")?.deliveryStatus)) #expect(coordinator.updateMessageDeliveryStatus("public-b", status: .sent)) - #expect(isSent(context.messages[2].deliveryStatus)) + #expect(isSent(mesh.message(withID: "public-b")?.deliveryStatus)) } @Test @MainActor - func middleInsertedPrivateMessage_isFoundAfterIndexWasBuilt() async { + func middleInsertedPrivateMessage_isStillUpdatableByID() async { let context = MockChatDeliveryContext() let coordinator = ChatDeliveryCoordinator(context: context) let peerID = PeerID(str: "0102030405060708") - context.privateChats[peerID] = [ - makePrivateMessage(id: "pm-a", status: .sending), - makePrivateMessage(id: "pm-b", status: .sending) - ] + let conversationID = ConversationID.directPeer(peerID) + context.store.append( + makePrivateMessage(id: "pm-a", status: .sending, timestamp: Date(timeIntervalSince1970: 10)), + to: conversationID + ) + context.store.append( + makePrivateMessage(id: "pm-b", status: .sending, timestamp: Date(timeIntervalSince1970: 30)), + to: conversationID + ) #expect(coordinator.updateMessageDeliveryStatus("pm-a", status: .sent)) - // Timestamp re-sort (sanitizeChat) can place a late arrival mid-array. - context.privateChats[peerID]?.insert(makePrivateMessage(id: "pm-mid", status: .sending), at: 1) + // A late arrival with an older timestamp lands mid-array. + context.store.append( + makePrivateMessage(id: "pm-mid", status: .sending, timestamp: Date(timeIntervalSince1970: 20)), + to: conversationID + ) + let chat = context.store.conversation(for: conversationID) + #expect(chat.messages.map(\.id) == ["pm-a", "pm-mid", "pm-b"]) #expect(coordinator.updateMessageDeliveryStatus("pm-mid", status: .sent)) - #expect(isSent(context.privateChats[peerID]?[1].deliveryStatus)) + #expect(isSent(chat.message(withID: "pm-mid")?.deliveryStatus)) #expect(coordinator.updateMessageDeliveryStatus("pm-b", status: .sent)) - #expect(isSent(context.privateChats[peerID]?[2].deliveryStatus)) + #expect(isSent(chat.message(withID: "pm-b")?.deliveryStatus)) + } + + @Test @MainActor + func mirroredPrivateCopies_bothReceiveDeliveryUpdate() async { + let context = MockChatDeliveryContext() + let coordinator = ChatDeliveryCoordinator(context: context) + let stablePeerID = PeerID(str: String(repeating: "ab", count: 32)) + let ephemeralPeerID = PeerID(str: "0102030405060708") + let messageID = "mirrored-mock-1" + + // Step 2's keying mirrors a private message into both the stable-key + // and ephemeral-peer conversations (distinct copies here to prove + // per-conversation application, not shared-reference aliasing). + context.store.append(makePrivateMessage(id: messageID, status: .sent), to: .directPeer(stablePeerID)) + context.store.append(makePrivateMessage(id: messageID, status: .sent), to: .directPeer(ephemeralPeerID)) + + let didUpdate = coordinator.updateMessageDeliveryStatus( + messageID, + status: .delivered(to: "Peer", at: Date()) + ) + + #expect(didUpdate) + #expect(isDelivered(context.store.conversation(for: .directPeer(stablePeerID)).message(withID: messageID)?.deliveryStatus)) + #expect(isDelivered(context.store.conversation(for: .directPeer(ephemeralPeerID)).message(withID: messageID)?.deliveryStatus)) + #expect(context.markedDeliveredMessageIDs == [messageID]) } @Test @MainActor @@ -566,7 +623,7 @@ struct ChatDeliveryCoordinatorContextTests { let context = MockChatDeliveryContext() let coordinator = ChatDeliveryCoordinator(context: context) let peerID = PeerID(str: "0102030405060708") - context.privateChats[peerID] = [makePrivateMessage(id: "keep-receipt", status: .sent)] + context.store.append(makePrivateMessage(id: "keep-receipt", status: .sent), to: .directPeer(peerID)) context.sentReadReceipts = ["keep-receipt", "drop-receipt"] // Startup phase: cleanup must be a no-op. diff --git a/bitchatTests/ConversationStoreTests.swift b/bitchatTests/ConversationStoreTests.swift index 8535cc57..3cd0f225 100644 --- a/bitchatTests/ConversationStoreTests.swift +++ b/bitchatTests/ConversationStoreTests.swift @@ -562,4 +562,159 @@ struct ConversationStoreTests { #expect(!conversation.containsMessage(withID: "one")) #expect(store.append(makeMessage(id: "one", timestamp: 2000), to: id)) } + + // MARK: - Store-level message-ID → conversation map (ID-only delivery) + + @Test("ID map tracks append, upsert, and removal") + @MainActor + func messageIDMapTracksAppendAndRemoval() { + let store = ConversationStore() + let direct = makeDirectConversationID("aa") + + #expect(store.conversationIDs(forMessageID: "m1").isEmpty) + + store.append(makeMessage(id: "m1", timestamp: 1), to: .mesh) + #expect(store.conversationIDs(forMessageID: "m1") == [.mesh]) + + // Upsert-as-append registers; upsert-in-place does not duplicate. + store.upsertByID(makeMessage(id: "d1", timestamp: 1, isPrivate: true), in: direct) + store.upsertByID(makeMessage(id: "d1", timestamp: 1, isPrivate: true, deliveryStatus: .sent), in: direct) + #expect(store.conversationIDs(forMessageID: "d1") == [direct]) + + store.removeMessage(withID: "m1", from: .mesh) + #expect(store.conversationIDs(forMessageID: "m1").isEmpty) + + store.removeMessages(from: direct, where: { $0.id == "d1" }) + #expect(store.conversationIDs(forMessageID: "d1").isEmpty) + } + + @Test("ID map handles multi-conversation membership (mirrored private copies)") + @MainActor + func messageIDMapHandlesMirroredCopies() { + let store = ConversationStore() + let stable = makeDirectConversationID("stable") + let ephemeral = makeDirectConversationID("ephemeral") + + // Step 2's keying mirrors one private message into the stable-key + // AND ephemeral-peer conversations (distinct copies here to prove + // per-conversation bookkeeping). + store.upsertByID(makeMessage(id: "dm-1", timestamp: 1, isPrivate: true, deliveryStatus: .sent), in: stable) + store.upsertByID(makeMessage(id: "dm-1", timestamp: 1, isPrivate: true, deliveryStatus: .sent), in: ephemeral) + #expect(store.conversationIDs(forMessageID: "dm-1") == [stable, ephemeral]) + + // An ID-only delivery update reaches BOTH copies. + #expect(store.setDeliveryStatus(.delivered(to: "bob", at: Date()), forMessageID: "dm-1")) + for id in [stable, ephemeral] { + guard case .delivered = store.conversation(for: id).message(withID: "dm-1")?.deliveryStatus else { + Issue.record("expected .delivered in \(id)") + return + } + } + + // Removing one copy keeps the other resolvable. + store.removeMessage(withID: "dm-1", from: ephemeral) + #expect(store.conversationIDs(forMessageID: "dm-1") == [stable]) + } + + @Test("ID-only setDeliveryStatus enforces no-downgrade and unknown IDs") + @MainActor + func idOnlyDeliveryStatusRules() { + let store = ConversationStore() + let direct = makeDirectConversationID("aa") + store.append( + makeMessage(id: "dm-1", timestamp: 1, isPrivate: true, deliveryStatus: .read(by: "bob", at: Date())), + to: direct + ) + + // Unknown message. + #expect(!store.setDeliveryStatus(.sent, forMessageID: "missing")) + #expect(store.deliveryStatus(forMessageID: "missing") == nil) + + // Downgrade from .read is refused; status is readable by ID alone. + #expect(!store.setDeliveryStatus(.delivered(to: "bob", at: Date()), forMessageID: "dm-1")) + guard case .read = store.deliveryStatus(forMessageID: "dm-1") else { + Issue.record("expected .read to survive the downgrade attempt") + return + } + } + + @Test("ID map follows migration between conversations") + @MainActor + func messageIDMapFollowsMigration() { + let store = ConversationStore() + let source = makeDirectConversationID("ephemeral") + let destination = makeDirectConversationID("stable") + store.append(makeMessage(id: "dm-1", timestamp: 1, isPrivate: true), to: source) + store.append(makeMessage(id: "dm-2", timestamp: 2, isPrivate: true), to: source) + // Already present in the destination: migration dedups, and the map + // must not retain a stale source membership. + store.append(makeMessage(id: "dm-2", timestamp: 2, isPrivate: true), to: destination) + + store.migrateConversation(from: source, to: destination) + + #expect(store.conversationIDs(forMessageID: "dm-1") == [destination]) + #expect(store.conversationIDs(forMessageID: "dm-2") == [destination]) + // Delivery updates keep flowing after the handoff. + #expect(store.setDeliveryStatus(.sent, forMessageID: "dm-1")) + } + + @Test("ID map drops trimmed messages") + @MainActor + func messageIDMapDropsTrimmedMessages() { + let store = ConversationStore() + let id = ConversationID.geohash("u4pruyd") + let conversation = store.conversation(for: id) + store.append(makeMessage(id: "first", timestamp: 1), to: id) + for index in 0.. delivered so every call - /// performs a real update (never the skip path). + /// `ChatDeliveryCoordinator.updateMessageDeliveryStatus` over the + /// `ConversationStore`'s message-ID → conversation map: 2000 public + /// (split mesh + geohash to stay under the per-conversation cap) + 50x40 + /// private messages, 500 status updates per pass. Statuses alternate + /// sent <-> delivered so every call performs a real update (never the + /// skip path). func testDeliveryStatusIncrementalUpdates() { let context = PerfDeliveryContext.makeCorpus(publicCount: 2000, peerCount: 50, messagesPerPeer: 40) let coordinator = ChatDeliveryCoordinator(context: context) - // Warm the index so the measure block exercises the incremental path. - XCTAssertTrue(coordinator.updateMessageDeliveryStatus(context.messages[0].id, status: .sent)) - - // 250 public + 250 private IDs spread across the corpus. - var targetIDs: [String] = [] - for i in stride(from: 0, to: 2000, by: 8) { targetIDs.append(context.messages[i].id) } - let peerIDs = context.privateChats.keys.sorted { $0.id < $1.id } - for (offset, peer) in peerIDs.enumerated() where offset < 25 { - guard let chat = context.privateChats[peer] else { continue } - for i in stride(from: 0, to: chat.count, by: 4) { targetIDs.append(chat[i].id) } - } + let targetIDs = context.makeTargetIDs(publicTargets: 250, privateTargets: 250) XCTAssertEqual(targetIDs.count, 500) let fixedDate = Date(timeIntervalSince1970: 1_700_000_000) @@ -235,46 +227,38 @@ final class PerformanceBaselineTests: XCTestCase { reportThroughput("delivery.incrementalUpdate", samples: samples, operations: targetIDs.count, unit: "updates") } - // MARK: - 4b. Delivery location index (rebuild path) + // MARK: - 4b. Delivery status updates against the store directly - /// The expensive path: a non-append insertion (out-of-order arrival at - /// the head of the timeline) invalidates the index, so the next status - /// update triggers a full rebuild over ~4000 message locations. - func testDeliveryStatusIndexRebuild() { + /// `ConversationStore.setDeliveryStatus(_:forMessageID:)` at the same + /// scale as 4a, without the coordinator/context wrapping — the store-side + /// cost of an ID-only delivery update (map lookup + per-conversation + /// ID-index apply + change emission). Replaces the deleted + /// `delivery.indexRebuild` benchmark: the positional location index and + /// its rebuild path no longer exist; the store's ID indexes are + /// maintained inside each mutation, so there is no rebuild to measure. + func testDeliveryStatusStoreUpdates() { let context = PerfDeliveryContext.makeCorpus(publicCount: 2000, peerCount: 50, messagesPerPeer: 40) - let coordinator = ChatDeliveryCoordinator(context: context) - // Warm the index before the first insertion invalidates it. - XCTAssertTrue(coordinator.updateMessageDeliveryStatus(context.messages[0].id, status: .sent)) + let store = context.store + let targetIDs = context.makeTargetIDs(publicTargets: 250, privateTargets: 250) + XCTAssertEqual(targetIDs.count, 500) - // Pre-built head insertions, one consumed per measure pass. - let insertions: [BitchatMessage] = (0..<32).map { i in - BitchatMessage( - id: "insert-\(i)", - sender: "alice", - content: "out-of-order arrival \(i)", - timestamp: Date(timeIntervalSince1970: 1_690_000_000), - isRelay: false - ) - } - let probeID = context.messages[1000].id let fixedDate = Date(timeIntervalSince1970: 1_700_000_000) - var pass = 0 var toggle = false var samples: [TimeInterval] = [] measure { - precondition(pass < insertions.count, "add more pre-built insertions") - context.messages.insert(insertions[pass], at: 0) - pass += 1 toggle.toggle() let status: DeliveryStatus = toggle ? .delivered(to: "peer", at: fixedDate) : .sent let start = Date() - let updated = coordinator.updateMessageDeliveryStatus(probeID, status: status) + var updated = 0 + for id in targetIDs where store.setDeliveryStatus(status, forMessageID: id) { + updated += 1 + } samples.append(Date().timeIntervalSince(start)) - XCTAssertTrue(updated) + XCTAssertEqual(updated, targetIDs.count) } - reportThroughput("delivery.indexRebuild", samples: samples, operations: 1, unit: "rebuilds") + reportThroughput("delivery.storeUpdate", samples: samples, operations: targetIDs.count, unit: "updates") } // MARK: - 5. Message formatting @@ -638,50 +622,63 @@ private final class PerfNostrContext: ChatNostrContext { // MARK: - Mock ChatDeliveryContext +/// Minimal `ChatDeliveryContext` over a real `ConversationStore` (the +/// coordinator is a thin mapper onto store intents, so the measured cost is +/// the store's ID-map delivery path). @MainActor private final class PerfDeliveryContext: ChatDeliveryContext { - var messages: [BitchatMessage] = [] - var privateChats: [PeerID: [BitchatMessage]] = [:] + let store = ConversationStore() var sentReadReceipts: Set = [] var isStartupPhase: Bool { false } + private var publicIDs: [String] = [] + private var privateIDsByPeer: [(peerID: PeerID, messageIDs: [String])] = [] + func notifyUIChanged() {} func markMessageDelivered(_ messageID: String) {} + @discardableResult + func setDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool { + store.setDeliveryStatus(status, forMessageID: messageID) + } + + func deliveryStatus(forMessageID messageID: String) -> DeliveryStatus? { + store.deliveryStatus(forMessageID: messageID) + } + + func privateMessageIDs() -> Set { + store.directMessageIDs() + } + func pruneSentReadReceipts(keeping validMessageIDs: Set) -> Int { let oldCount = sentReadReceipts.count sentReadReceipts = sentReadReceipts.intersection(validMessageIDs) return oldCount - sentReadReceipts.count } - @discardableResult - func setPrivateDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String, peerID: PeerID) -> Bool { - guard var chat = privateChats[peerID], - let index = chat.firstIndex(where: { $0.id == messageID }) else { - return false - } - chat[index].deliveryStatus = status - privateChats[peerID] = chat - return true - } - - /// 2000 public + `peerCount` x `messagesPerPeer` private messages with - /// deterministic IDs and timestamps. + /// `publicCount` public messages (split evenly between mesh and one + /// geohash conversation so the corpus stays under the per-conversation + /// cap) + `peerCount` x `messagesPerPeer` private messages, seeded into + /// the store with deterministic IDs and timestamps. static func makeCorpus(publicCount: Int, peerCount: Int, messagesPerPeer: Int) -> PerfDeliveryContext { let context = PerfDeliveryContext() let base = Date(timeIntervalSince1970: 1_700_000_000) - context.messages = (0.. [String] { + var targetIDs: [String] = [] + let publicStride = max(1, publicIDs.count / publicTargets) + for i in stride(from: 0, to: publicIDs.count, by: publicStride) where targetIDs.count < publicTargets { + targetIDs.append(publicIDs[i]) + } + var privateCount = 0 + outer: for (_, messageIDs) in privateIDsByPeer { + for i in stride(from: 0, to: messageIDs.count, by: 4) { + guard privateCount < privateTargets else { break outer } + targetIDs.append(messageIDs[i]) + privateCount += 1 + } + } + return targetIDs + } } // MARK: - End-to-end pipeline fixture diff --git a/docs/CONVERSATION-STORE-DESIGN.md b/docs/CONVERSATION-STORE-DESIGN.md index 7f63342e..aa34b406 100644 --- a/docs/CONVERSATION-STORE-DESIGN.md +++ b/docs/CONVERSATION-STORE-DESIGN.md @@ -1,9 +1,11 @@ # Conversation Store: Single Source of Truth -**Status:** Steps 1–3 implemented (additive store, private cutover, public -cutover; `PublicTimelineStore` deleted). Baselines recorded in +**Status:** Steps 1–4 implemented (additive store, private cutover, public +cutover, delivery via store; `PublicTimelineStore` and +`ChatDeliveryCoordinator.messageLocationIndex` deleted). Baselines recorded in `bitchatTests/Performance/PerformanceBaselineTests.swift` (`pipeline.privateIngest`, -`pipeline.publicIngest`, `store.append`). +`pipeline.publicIngest`, `store.append`, `delivery.incrementalUpdate`, +`delivery.storeUpdate`). ---