Fix group state loss during concurrent updates.

Storage sync and a message process can both attempt to create a group at the same time. Message processing caches the local group state and performs networking which allows the cached state to become stale. The state was being used to decide to call create instead of update and the create would fail silently as the group record already exists. This would cause state to not be persisted and result in odd double events.

app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt

@@ -60,6 +60,7 @@ import java.security.SecureRandom
 import java.util.Optional
 import java.util.UUID
 import java.util.stream.Collectors
+import javax.annotation.CheckReturnValue
 
 class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseTable(context, databaseHelper), RecipientIdDatabaseReference {
 
@@ -649,20 +650,23 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT
     }
   }
 
-  fun create(groupId: GroupId.V1, title: String?, members: Collection<RecipientId>, avatar: SignalServiceAttachmentPointer?, relay: String?) {
+  @CheckReturnValue
+  fun create(groupId: GroupId.V1, title: String?, members: Collection<RecipientId>, avatar: SignalServiceAttachmentPointer?, relay: String?): Boolean {
     if (groupExists(groupId.deriveV2MigrationGroupId())) {
       throw LegacyGroupInsertException(groupId)
     }
 
-    create(groupId, title, members, avatar, relay, null, null)
+    return create(groupId, title, members, avatar, relay, null, null)
   }
 
-  fun create(groupId: GroupId.Mms, title: String?, members: Collection<RecipientId>) {
+  @CheckReturnValue
-    create(groupId, if (title.isNullOrEmpty()) null else title, members, null, null, null, null)
+  fun create(groupId: GroupId.Mms, title: String?, members: Collection<RecipientId>): Boolean {
+    return create(groupId, if (title.isNullOrEmpty()) null else title, members, null, null, null, null)
   }
 
   @JvmOverloads
-  fun create(groupMasterKey: GroupMasterKey, groupState: DecryptedGroup, force: Boolean = false): GroupId.V2 {
+  @CheckReturnValue
+  fun create(groupMasterKey: GroupMasterKey, groupState: DecryptedGroup, force: Boolean = false): GroupId.V2? {
     val groupId = GroupId.v2(groupMasterKey)
 
     if (!force && getGroupV1ByExpectedV2(groupId).isPresent) {
@@ -671,9 +675,11 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT
       Log.w(TAG, "Forcing the creation of a group even though we already have a V1 ID!")
     }
 
-    create(groupId, groupState.title, emptyList(), null, null, groupMasterKey, groupState)
+    return if (create(groupId = groupId, title = groupState.title, memberCollection = emptyList(), avatar = null, relay = null, groupMasterKey = groupMasterKey, groupState = groupState)) {
-
+      groupId
-    return groupId
+    } else {
+      null
+    }
   }
 
   /**
@@ -714,6 +720,7 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT
   /**
    * @param groupMasterKey null for V1, must be non-null for V2 (presence dictates group version).
    */
+  @CheckReturnValue
   private fun create(
     groupId: GroupId,
     title: String?,
@@ -722,7 +729,7 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT
     relay: String?,
     groupMasterKey: GroupMasterKey?,
     groupState: DecryptedGroup?
-  ) {
+  ): Boolean {
     val membershipValues = mutableListOf<ContentValues>()
     val groupRecipientId = recipients.getOrInsertFromGroupId(groupId)
     val members: List<RecipientId> = memberCollection.toSet().sorted()
@@ -777,16 +784,20 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT
       }
     }
 
-    writableDatabase.withinTransaction { db ->
+    writableDatabase.beginTransaction()
-      db.insert(TABLE_NAME, null, values)
+    try {
-      SqlUtil.buildBulkInsert(
+      val result: Long = writableDatabase.insert(TABLE_NAME, null, values)
-        MembershipTable.TABLE_NAME,
+      if (result < 1) {
-        arrayOf(MembershipTable.GROUP_ID, MembershipTable.RECIPIENT_ID),
+        Log.w(TAG, "Unable to create group, group record already exists")
-        membershipValues
+        return false
-      )
+      }
-        .forEach {
+
-          db.execSQL(it.where, it.whereArgs)
+      for (query in SqlUtil.buildBulkInsert(MembershipTable.TABLE_NAME, arrayOf(MembershipTable.GROUP_ID, MembershipTable.RECIPIENT_ID), membershipValues)) {
-        }
+        writableDatabase.execSQL(query.where, query.whereArgs)
+      }
+      writableDatabase.setTransactionSuccessful()
+    } finally {
+      writableDatabase.endTransaction()
     }
 
     if (groupState != null && groupState.hasDisappearingMessagesTimer()) {
@@ -799,6 +810,8 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT
 
     Recipient.live(groupRecipientId).refresh()
     notifyConversationListListeners()
+
+    return true
   }
 
   fun update(groupId: GroupId.V1, title: String?, avatar: SignalServiceAttachmentPointer?) {

app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt

@@ -915,13 +915,17 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da
     val recipient = Recipient.externalGroupExact(groupId)
 
     Log.i(TAG, "Creating restore placeholder for $groupId")
-    groups.create(
+    val createdId = groups.create(
       masterKey,
       DecryptedGroup.newBuilder()
         .setRevision(GroupsV2StateProcessor.RESTORE_PLACEHOLDER_REVISION)
         .build()
     )
 
+    if (createdId == null) {
+      Log.w(TAG, "Unable to create restore placeholder for $groupId, group already exists")
+    }
+
     groups.setShowAsStoryState(groupId, insert.storySendMode.toShowAsStoryState())
     updateExtras(recipient.id) {
       it.setHideStory(insert.shouldHideStory())

app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java

@@ -294,10 +294,15 @@ final class GroupManagerV2 {
         throw new GroupChangeFailedException(e);
       }
 
-      GroupMasterKey masterKey        = groupSecretParams.getMasterKey();
+      GroupMasterKey masterKey = groupSecretParams.getMasterKey();
-      GroupId.V2     groupId          = groupDatabase.create(masterKey, decryptedGroup);
+      GroupId.V2     groupId   = groupDatabase.create(masterKey, decryptedGroup);
-      RecipientId    groupRecipientId = SignalDatabase.recipients().getOrInsertFromGroupId(groupId);
+
-      Recipient      groupRecipient   = Recipient.resolved(groupRecipientId);
+      if (groupId == null) {
+        throw new GroupChangeFailedException("Unable to create group, group already exists");
+      }
+
+      RecipientId              groupRecipientId = SignalDatabase.recipients().getOrInsertFromGroupId(groupId);
+      Recipient                groupRecipient   = Recipient.resolved(groupRecipientId);
 
       AvatarHelper.setAvatar(context, groupRecipientId, avatar != null ? new ByteArrayInputStream(avatar) : null);
       groupDatabase.onAvatarUpdated(groupId, avatar != null);
@@ -946,8 +951,20 @@ final class GroupManagerV2 {
           }
         }
       } else {
-        groupDatabase.create(groupMasterKey, decryptedGroup);
+        GroupId.V2 groupId = groupDatabase.create(groupMasterKey, decryptedGroup);
-        Log.i(TAG, "Created local group with placeholder");
+        if (groupId != null) {
+          Log.i(TAG, "Created local group with placeholder");
+        } else {
+          Log.i(TAG, "Create placeholder failed, group suddenly present locally, attempting to apply change");
+          if (decryptedChange != null) {
+            try {
+              groupsV2StateProcessor.forGroup(SignalStore.account().getServiceIds(), groupMasterKey)
+                                    .updateLocalGroupToRevision(decryptedChange.getRevision(), System.currentTimeMillis(), decryptedChange);
+            } catch (GroupNotAMemberException e) {
+              Log.w(TAG, "Unable to apply join change to existing group", e);
+            }
+          }
+        }
       }
 
       RecipientId groupRecipientId = SignalDatabase.recipients().getOrInsertFromGroupId(groupId);

app/src/main/java/org/thoughtcrime/securesms/groups/v2/processing/GroupsV2StateProcessor.java

@@ -21,11 +21,11 @@ import org.signal.storageservice.protos.groups.local.DecryptedPendingMember;
 import org.signal.storageservice.protos.groups.local.DecryptedPendingMemberRemoval;
 import org.signal.storageservice.protos.groups.local.DecryptedRequestingMember;
 import org.thoughtcrime.securesms.database.GroupTable;
-import org.thoughtcrime.securesms.database.model.GroupRecord;
 import org.thoughtcrime.securesms.database.MessageTable;
 import org.thoughtcrime.securesms.database.RecipientTable;
 import org.thoughtcrime.securesms.database.SignalDatabase;
 import org.thoughtcrime.securesms.database.ThreadTable;
+import org.thoughtcrime.securesms.database.model.GroupRecord;
 import org.thoughtcrime.securesms.database.model.databaseprotos.DecryptedGroupV2Context;
 import org.thoughtcrime.securesms.dependencies.ApplicationDependencies;
 import org.thoughtcrime.securesms.groups.GroupDoesNotExistException;
@@ -554,7 +554,11 @@ public class GroupsV2StateProcessor {
       boolean needsAvatarFetch;
 
       if (inputGroupState.getLocalState() == null) {
-        groupDatabase.create(masterKey, newLocalState);
+        GroupId.V2 groupId = groupDatabase.create(masterKey, newLocalState);
+        if (groupId == null) {
+          Log.w(TAG, "Group create failed, trying to update");
+          groupDatabase.update(masterKey, newLocalState);
+        }
         needsAvatarFetch = !TextUtils.isEmpty(newLocalState.getAvatar());
       } else {
         groupDatabase.update(masterKey, newLocalState);