Rework outgoing message state.

// FREEBIE

src/Devices/OWSRecordTranscriptJob.m

@@ -1,5 +1,6 @@
-//  Created by Michael Kirk on 9/23/16.
-//  Copyright © 2016 Open Whisper Systems. All rights reserved.
+//
+//  Copyright (c) 2017 Open Whisper Systems. All rights reserved.
+//
 
 #import "OWSRecordTranscriptJob.h"
 #import "OWSAttachmentsProcessor.h"
@@ -85,8 +86,7 @@ NS_ASSUME_NONNULL_BEGIN
                                                                         attachmentIds:[NSMutableArray new]
                                                                      expiresInSeconds:transcript.expirationDuration
                                                                       expireStartedAt:transcript.expirationStartedAt];
-        textMessage.messageState = TSOutgoingMessageStateDelivered;
-        [textMessage save];
+        [textMessage updateWithWasDelivered];
     }
 }
 

src/Messages/Interactions/TSOutgoingMessage.h

@@ -19,11 +19,11 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) {
     TSOutgoingMessageStateAttemptingOut,
     // The failure state.
     TSOutgoingMessageStateUnsent,
-    // The message has been sent to the service, but not received by any recipient client.
-    TSOutgoingMessageStateSent,
-    // The message has been sent to the service and received by at least one recipient client.
-    // A recipient may have more than one client, and group message may have more than one recipient.
-    TSOutgoingMessageStateDelivered
+    // These two enum values have been combined into TSOutgoingMessageStateSentToService.
+    TSOutgoingMessageStateSent_OBSOLETE,
+    TSOutgoingMessageStateDelivered_OBSOLETE,
+    // The message has been sent to the service.
+    TSOutgoingMessageStateSentToService,
 };
 
 - (instancetype)initWithTimestamp:(uint64_t)timestamp;
@@ -53,9 +53,14 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) {
 
 - (instancetype)initWithCoder:(NSCoder *)coder NS_DESIGNATED_INITIALIZER;
 
-@property (nonatomic) TSOutgoingMessageState messageState;
-@property BOOL hasSyncedTranscript;
-@property NSString *customMessage;
+@property (atomic, readonly) TSOutgoingMessageState messageState;
+
+// The message has been sent to the service and received by at least one recipient client.
+// A recipient may have more than one client, and group message may have more than one recipient.
+@property (atomic, readonly) BOOL wasDelivered;
+
+@property (atomic, readonly) BOOL hasSyncedTranscript;
+@property (atomic, readonly) NSString *customMessage;
 @property (atomic, readonly) NSString *mostRecentFailureText;
 // A map of attachment id-to-filename.
 @property (nonatomic, readonly) NSMutableDictionary<NSString *, NSString *> *attachmentFilenameMap;
@@ -67,8 +72,6 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) {
  */
 @property (nonatomic, readonly) BOOL isLegacyMessage;
 
-- (void)setSendingError:(NSError *)error;
-
 /**
  * Signal Identifier (e.g. e164 number) or nil if in a group thread.
  */
@@ -105,6 +108,29 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) {
 - (OWSSignalServiceProtosAttachmentPointer *)buildAttachmentProtoForAttachmentId:(NSString *)attachmentId
                                                                         filename:(nullable NSString *)filename;
 
+// TSOutgoingMessage are updated from many threads. We don't want to save
+// our local copy since it may be out of date.  Instead, we use these
+// "updateWith..." methods to:
+//
+// a) Update a property of the local copy.
+// b) Load an up-to-date instance of this model from from the data store.
+// c) Update and save that fresh instance.
+//
+// This isn't a perfect arrangement, but in practice this will prevent
+// data loss.
+- (void)updateWithMessageState:(TSOutgoingMessageState)messageState;
+- (void)updateWithSendingError:(NSError *)error;
+- (void)updateWithHasSyncedTranscript:(BOOL)hasSyncedTranscript;
+- (void)updateWithCustomMessage:(NSString *)customMessage;
+- (void)updateWithWasDeliveredWithTransaction:(YapDatabaseReadWriteTransaction *)transaction;
+- (void)updateWithWasDelivered;
+
+#pragma mark - Sent Recipients
+
+- (BOOL)wasSentToRecipient:(NSString *)contactId;
+- (void)updateWithSentRecipient:(NSString *)contactId transaction:(YapDatabaseReadWriteTransaction *)transaction;
+- (void)updateWithSentRecipient:(NSString *)contactId;
+
 @end
 
 NS_ASSUME_NONNULL_END

src/Messages/Interactions/TSOutgoingMessage.m

@@ -8,19 +8,56 @@
 #import "TSAttachmentStream.h"
 #import "TSContactThread.h"
 #import "TSGroupThread.h"
+#import <YapDatabase/YapDatabase.h>
+#import <YapDatabase/YapDatabaseTransaction.h>
 
 NS_ASSUME_NONNULL_BEGIN
 
+NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRecipientAll";
+
+@interface TSOutgoingMessage ()
+
+@property (atomic) TSOutgoingMessageState messageState;
+@property (atomic) BOOL hasSyncedTranscript;
+@property (atomic) NSString *customMessage;
+@property (atomic) NSString *mostRecentFailureText;
+@property (atomic) BOOL wasDelivered;
+// For outgoing, non-legacy group messages sent from this client, this
+// contains the list of recipients to whom the message has been sent.
+//
+// This collection can also be tested to avoid repeat delivery to the
+// same recipient.
+@property (atomic) NSArray<NSString *> *sentRecipients;
+
+@end
+
+#pragma mark -
+
 @implementation TSOutgoingMessage
 
+@synthesize sentRecipients = _sentRecipients;
+
 - (instancetype)initWithCoder:(NSCoder *)coder
 {
     self = [super initWithCoder:coder];
+
     if (self) {
         if (!_attachmentFilenameMap) {
             _attachmentFilenameMap = [NSMutableDictionary new];
         }
+        
+        // Migrate message state.
+        if (_messageState == TSOutgoingMessageStateSent_OBSOLETE) {
+            _messageState = TSOutgoingMessageStateSentToService;
+        } else if (_messageState == TSOutgoingMessageStateDelivered_OBSOLETE) {
+            _messageState = TSOutgoingMessageStateSentToService;
+            _wasDelivered = YES;
+        }
+        if (!_sentRecipients) {
+            _sentRecipients = [NSArray new];
+        }
     }
+
     return self;
 }
 
@@ -85,6 +122,7 @@ NS_ASSUME_NONNULL_BEGIN
     }
 
     _messageState = TSOutgoingMessageStateAttemptingOut;
+    _sentRecipients = [NSArray new];
     _hasSyncedTranscript = NO;
 
     if ([thread isKindOfClass:[TSGroupThread class]]) {
@@ -117,20 +155,152 @@ NS_ASSUME_NONNULL_BEGIN
 - (BOOL)shouldStartExpireTimer
 {
     switch (self.messageState) {
-        case TSOutgoingMessageStateSent:
-        case TSOutgoingMessageStateDelivered:
+        case TSOutgoingMessageStateSentToService:
             return self.isExpiringMessage;
         case TSOutgoingMessageStateAttemptingOut:
         case TSOutgoingMessageStateUnsent:
             return NO;
+        case TSOutgoingMessageStateSent_OBSOLETE:
+        case TSOutgoingMessageStateDelivered_OBSOLETE:
+            OWSAssert(0);
+            return self.isExpiringMessage;
+    }
+}
+
+#pragma mark - Update Methods
+
+- (void)applyChangeToSelfAndLatest:(YapDatabaseReadWriteTransaction *)transaction
+                       changeBlock:(void (^)(TSOutgoingMessage *))changeBlock
+{
+    OWSAssert(transaction);
+
+    changeBlock(self);
+
+    TSOutgoingMessage *latestMessage =
+        [transaction objectForKey:self.uniqueId inCollection:[TSOutgoingMessage collection]];
+    if (latestMessage) {
+        changeBlock(latestMessage);
+        [latestMessage saveWithTransaction:transaction];
+    } else {
+        // This message has not yet been saved.
+        [self saveWithTransaction:transaction];
+    }
+}
+
+- (void)updateWithSendingError:(NSError *)error
+{
+    [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
+        [self applyChangeToSelfAndLatest:transaction
+                             changeBlock:^(TSOutgoingMessage *message) {
+                                 [message setMessageState:TSOutgoingMessageStateUnsent];
+                                 [message setMostRecentFailureText:error.localizedDescription];
+                             }];
+    }];
+}
+
+- (void)updateWithMessageState:(TSOutgoingMessageState)messageState
+{
+    [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
+        [self applyChangeToSelfAndLatest:transaction
+                             changeBlock:^(TSOutgoingMessage *message) {
+                                 [message setMessageState:messageState];
+                             }];
+    }];
+}
+
+- (void)updateWithHasSyncedTranscript:(BOOL)hasSyncedTranscript
+{
+    [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
+        [self applyChangeToSelfAndLatest:transaction
+                             changeBlock:^(TSOutgoingMessage *message) {
+                                 [message setHasSyncedTranscript:hasSyncedTranscript];
+                             }];
+    }];
+}
+
+- (void)updateWithCustomMessage:(NSString *)customMessage
+{
+    [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
+        [self applyChangeToSelfAndLatest:transaction
+                             changeBlock:^(TSOutgoingMessage *message) {
+                                 [message setCustomMessage:customMessage];
+                             }];
+    }];
+}
+
+- (void)updateWithWasDeliveredWithTransaction:(YapDatabaseReadWriteTransaction *)transaction
+{
+    OWSAssert(transaction);
+    [self applyChangeToSelfAndLatest:transaction
+                         changeBlock:^(TSOutgoingMessage *message) {
+                             [message setWasDelivered:YES];
+                         }];
+}
+
+- (void)updateWithWasDelivered
+{
+    [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
+        [self updateWithWasDeliveredWithTransaction:transaction];
+    }];
+}
+
+#pragma mark - Sent Recipients
+
+- (NSArray<NSString *> *)sentRecipients
+{
+    @synchronized(self)
+    {
+        return _sentRecipients;
+    }
+}
+
+- (void)setSentRecipients:(NSArray<NSString *> *)sentRecipients
+{
+    @synchronized(self)
+    {
+        _sentRecipients = [sentRecipients copy];
+    }
+}
+
+- (void)addSentRecipient:(NSString *)contactId
+{
+    @synchronized(self)
+    {
+        OWSAssert(_sentRecipients);
+        OWSAssert(contactId.length > 0);
+
+        NSMutableArray *sentRecipients = [_sentRecipients mutableCopy];
+        [sentRecipients addObject:contactId];
+        _sentRecipients = [sentRecipients copy];
     }
 }
 
-- (void)setSendingError:(NSError *)error
+- (BOOL)wasSentToRecipient:(NSString *)contactId
 {
-    _mostRecentFailureText = error.localizedDescription;
+    OWSAssert(self.sentRecipients);
+    OWSAssert(contactId.length > 0);
+
+    return [self.sentRecipients containsObject:contactId];
 }
 
+- (void)updateWithSentRecipient:(NSString *)contactId transaction:(YapDatabaseReadWriteTransaction *)transaction
+{
+    OWSAssert(transaction);
+    [self applyChangeToSelfAndLatest:transaction
+                         changeBlock:^(TSOutgoingMessage *message) {
+                             [message addSentRecipient:contactId];
+                         }];
+}
+
+- (void)updateWithSentRecipient:(NSString *)contactId
+{
+    [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
+        [self updateWithSentRecipient:contactId transaction:transaction];
+    }];
+}
+
+#pragma mark -
+
 - (OWSSignalServiceProtosDataMessageBuilder *)dataMessageBuilder
 {
     TSThread *thread = self.thread;

src/Messages/OWSFailedMessagesJob.m

@@ -84,8 +84,7 @@ static NSString *const OWSFailedMessagesJobMessageStateIndex = @"index_outoing_m
         }
 
         DDLogDebug(@"%@ marking message as unsent", self.tag);
-        message.messageState = TSOutgoingMessageStateUnsent;
-        [message save];
+        [message updateWithMessageState:TSOutgoingMessageStateUnsent];
         count++;
     }];
 

src/Messages/OWSMessageSender.m

@@ -87,8 +87,6 @@ typedef NS_ENUM(NSInteger, OWSSendMessageOperationState) {
                      success:(void (^)())successHandler
                      failure:(RetryableFailureHandler)failureHandler;
 
-- (void)saveMessage:(TSOutgoingMessage *)message withError:(NSError *)error;
-
 @end
 
 #pragma mark -
@@ -136,6 +134,9 @@ NSUInteger const OWSSendMessageOperationMaxRetries = 4;
             OWSCAssert(NO);
             return;
         }
+
+        [message updateWithMessageState:TSOutgoingMessageStateSentToService];
+
         DDLogDebug(@"%@ succeeded.", strongSelf.tag);
         aSuccessHandler();
         [strongSelf markAsComplete];
@@ -148,7 +149,7 @@ NSUInteger const OWSSendMessageOperationMaxRetries = 4;
             return;
         }
 
-        [strongSelf.messageSender saveMessage:strongSelf.message withError:error];
+        [strongSelf.message updateWithSendingError:error];
 
         DDLogDebug(@"%@ failed with error: %@", strongSelf.tag, error);
         aFailureHandler(error);
@@ -355,7 +356,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
 {
     AssertIsOnMainThread();
 
-    [self saveMessage:message withState:TSOutgoingMessageStateAttemptingOut];
+    [message updateWithMessageState:TSOutgoingMessageStateAttemptingOut];
     OWSSendMessageOperation *sendMessageOperation = [[OWSSendMessageOperation alloc] initWithMessage:message
                                                                                        messageSender:self
                                                                                              success:successHandler
@@ -491,36 +492,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
     //  2.) fail and create a new identical error message in the thread.
     [errorMessage remove];
 
-    if ([errorMessage.thread isKindOfClass:[TSContactThread class]]) {
-        return [self sendMessage:message success:successHandler failure:failureHandler];
-    }
-
-    // else it's a GroupThread
-    dispatch_async([OWSDispatch sendingQueue], ^{
-
-        // Avoid spamming entire group when resending failed message.
-        SignalRecipient *failedRecipient = [SignalRecipient fetchObjectWithUniqueID:errorMessage.recipientId];
-
-        // Normally marking as unsent is handled in sendMessage happy path, but beacuse we're skipping the common entry
-        // point to message sending in order to send to a single recipient, we have to handle it ourselves.
-        RetryableFailureHandler markAndFailureHandler = ^(NSError *error, BOOL isRetryable) {
-            [self saveMessage:message withError:error];
-            if (isRetryable) {
-                // FIXME: Fixing this will require a larger refactor. In the meanwhile, we don't
-                // retry message sending from accepting key-changes in a group. (Except for the inner retry logic w/ the
-                // messages API)
-                DDLogWarn(@"%@ Skipping retry for group-message failure during %s.", self.tag, __PRETTY_FUNCTION__);
-            }
-
-            failureHandler(error);
-        };
-
-        [self groupSend:@[ failedRecipient ]
-                message:message
-                 thread:message.thread
-                success:successHandler
-                failure:markAndFailureHandler];
-    });
+    return [self sendMessage:message success:successHandler failure:failureHandler];
 }
 
 - (NSArray<SignalRecipient *> *)getRecipients:(NSArray<NSString *> *)identifiers error:(NSError **)error
@@ -664,6 +636,8 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
         thread:thread
         attempts:OWSMessageSenderRetryAttempts
         success:^{
+            DDLogInfo(@"Marking group message as sent to recipient: %@", recipient.uniqueId);
+            [message updateWithSentRecipient:recipient.uniqueId];
             [futureSource trySetResult:@1];
         }
         failure:^(NSError *error, BOOL isRetryable) {
@@ -683,14 +657,20 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
     [self saveGroupMessage:message inThread:thread];
     NSMutableArray<TOCFuture *> *futures = [NSMutableArray array];
 
-    for (SignalRecipient *rec in recipients) {
+    for (SignalRecipient *recipient in recipients) {
         // We don't need to send the message to ourselves...
-        if ([rec.uniqueId isEqualToString:[TSStorageManager localNumber]]) {
+        if ([recipient.uniqueId isEqualToString:[TSStorageManager localNumber]]) {
+            continue;
+        }
+        if ([message wasSentToRecipient:recipient.uniqueId]) {
+            // Skip recipients we have already sent this message to (on an
+            // earlier retry, perhaps).
+            DDLogInfo(@"Skipping group message recipient; already sent: %@", recipient.uniqueId);
             continue;
         }
 
         // ...otherwise we send.
-        [futures addObject:[self sendMessageFuture:message recipient:rec thread:thread]];
+        [futures addObject:[self sendMessageFuture:message recipient:recipient thread:thread]];
     }
 
     TOCFuture *completionFuture = futures.toc_thenAll;
@@ -938,12 +918,10 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
 
 - (void)handleMessageSentLocally:(TSOutgoingMessage *)message
 {
-    [self saveMessage:message withState:TSOutgoingMessageStateSent];
     if (message.shouldSyncTranscript) {
         // TODO: I suspect we shouldn't optimistically set hasSyncedTranscript.
         //       We could set this in a success handler for [sendSyncTranscriptForMessage:].
-
-        message.hasSyncedTranscript = YES;
+        [message updateWithHasSyncedTranscript:YES];
         [self sendSyncTranscriptForMessage:message];
     }
 
@@ -952,7 +930,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
 
 - (void)handleMessageSentRemotely:(TSOutgoingMessage *)message sentAt:(uint64_t)sentAt
 {
-    [self saveMessage:message withState:TSOutgoingMessageStateDelivered];
+    [message updateWithWasDelivered];
     [self becomeConsistentWithDisappearingConfigurationForMessage:message];
     [self.disappearingMessagesJob setExpirationForMessage:message expirationStartedAt:sentAt];
 }
@@ -1177,23 +1155,11 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
     return TSUnknownMessageType;
 }
 
-- (void)saveMessage:(TSOutgoingMessage *)message withState:(TSOutgoingMessageState)state
-{
-    message.messageState = state;
-    [message save];
-}
-
-- (void)saveMessage:(TSOutgoingMessage *)message withError:(NSError *)error
-{
-    message.messageState = TSOutgoingMessageStateUnsent;
-    [message setSendingError:error];
-    [message save];
-}
-
 - (void)saveGroupMessage:(TSOutgoingMessage *)message inThread:(TSThread *)thread
 {
     if (message.groupMetaMessage == TSGroupMessageDeliver) {
-        [self saveMessage:message withState:message.messageState];
+        // TODO: Why is this necessary?
+        [message save];
     } else if (message.groupMetaMessage == TSGroupMessageQuit) {
         [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp
                                          inThread:thread
@@ -1239,13 +1205,14 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
         // TODO: This error message is never created?
         TSErrorMessage *errorMessage;
 
-        if (message.groupMetaMessage == TSGroupMessageNone) {
-            // Only update this with exception if it is not a group message as group
-            // messages may except for one group
-            // send but not another and the UI doesn't know how to handle that
-            [message setMessageState:TSOutgoingMessageStateUnsent];
-            [message saveWithTransaction:transaction];
-        }
+        // TODO: Is this necessary?
+        //        if (message.groupMetaMessage == TSGroupMessageNone) {
+        //            // Only update this with exception if it is not a group message as group
+        //            // messages may except for one group
+        //            // send but not another and the UI doesn't know how to handle that
+        //            [message setMessageState:TSOutgoingMessageStateUnsent];
+        //            [message saveWithTransaction:transaction];
+        //        }
 
         [errorMessage saveWithTransaction:transaction];
     }];

src/Messages/TSMessagesManager.m

@@ -237,9 +237,7 @@ NS_ASSUME_NONNULL_BEGIN
             [TSInteraction interactionForTimestamp:envelope.timestamp withTransaction:transaction];
         if ([interaction isKindOfClass:[TSOutgoingMessage class]]) {
             TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)interaction;
-            outgoingMessage.messageState = TSOutgoingMessageStateDelivered;
-
-            [outgoingMessage saveWithTransaction:transaction];
+            [outgoingMessage updateWithWasDeliveredWithTransaction:transaction];
         }
     }];
 }