Fix issue with storage key intersections.

- When doing the intersection, ignore keys that have type mismatches (same storageId, different types)
- If we detect that scenario, schedule a force push to happen afterwards
- Also schedule a force push afterwards if we detect that there's keys in the manifest that don't have any storage item on the service

app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java

@@ -131,6 +131,7 @@ public class StorageSyncJob extends BaseJob {
     StorageKey                  storageServiceKey  = SignalStore.storageServiceValues().getOrCreateStorageKey();
 
     boolean                         needsMultiDeviceSync  = false;
+    boolean                         needsForcePush        = false;
     long                            localManifestVersion  = TextSecurePreferences.getStorageManifestVersion(context);
     Optional<SignalStorageManifest> remoteManifest        = accountManager.getStorageManifestIfDifferentVersion(storageServiceKey, localManifestVersion);
     long                            remoteManifestVersion = remoteManifest.transform(SignalStorageManifest::getVersion).or(localManifestVersion);
@@ -143,6 +144,11 @@ public class StorageSyncJob extends BaseJob {
       List<StorageId>     allLocalStorageKeys = getAllLocalStorageIds(context, Recipient.self().fresh());
       KeyDifferenceResult keyDifference       = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), allLocalStorageKeys);
 
+      if (keyDifference.hasTypeMismatches()) {
+        Log.w(TAG, "Found type mismatches in the key sets! Scheduling a force push after this sync completes.");
+        needsForcePush = true;
+      }
+
       if (!keyDifference.isEmpty()) {
         Log.i(TAG, "[Remote Newer] There's a difference in keys. Local-only: " + keyDifference.getLocalOnlyKeys().size() + ", Remote-only: " + keyDifference.getRemoteOnlyKeys().size());
 
@@ -152,6 +158,11 @@ public class StorageSyncJob extends BaseJob {
         MergeResult               mergeResult          = StorageSyncHelper.resolveConflict(remoteOnly, localOnly);
         WriteOperationResult      writeOperationResult = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), allLocalStorageKeys, mergeResult);
 
+        if (remoteOnly.size() != keyDifference.getRemoteOnlyKeys().size()) {
+          Log.w(TAG, "Could not find all remote-only records! Requested: " + keyDifference.getRemoteOnlyKeys().size() + ", Found: " + remoteOnly.size() + ". Scheduling a force push after this sync completes.");
+          needsForcePush = true;
+        }
+
         StorageSyncValidations.validate(writeOperationResult);
 
         Log.i(TAG, "[Remote Newer] MergeResult :: " + mergeResult);
@@ -248,6 +259,11 @@ public class StorageSyncJob extends BaseJob {
       Log.i(TAG, "[Local Changes] No local changes.");
     }
 
+    if (needsForcePush) {
+      Log.w(TAG, "Scheduling a force push.");
+      ApplicationDependencies.getJobManager().add(new StorageForcePushJob());
+    }
+
     return needsMultiDeviceSync;
   }
 

app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java

@@ -6,6 +6,7 @@ import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 
+import com.annimon.stream.Collectors;
 import com.annimon.stream.Stream;
 
 import org.thoughtcrime.securesms.database.DatabaseFactory;
@@ -19,6 +20,7 @@ import org.thoughtcrime.securesms.keyvalue.SignalStore;
 import org.thoughtcrime.securesms.logging.Log;
 import org.thoughtcrime.securesms.recipients.Recipient;
 import org.thoughtcrime.securesms.recipients.RecipientId;
+import org.thoughtcrime.securesms.util.Base64;
 import org.thoughtcrime.securesms.util.SetUtil;
 import org.thoughtcrime.securesms.util.TextSecurePreferences;
 import org.thoughtcrime.securesms.util.Util;
@@ -41,6 +43,7 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -213,10 +216,30 @@ public final class StorageSyncHelper {
   public static @NonNull KeyDifferenceResult findKeyDifference(@NonNull Collection<StorageId> remoteKeys,
                                                                @NonNull Collection<StorageId> localKeys)
   {
-    Set<StorageId> remoteOnlyKeys = SetUtil.difference(remoteKeys, localKeys);
+    Map<String, StorageId> remoteByRawId = Stream.of(remoteKeys).collect(Collectors.toMap(id -> Base64.encodeBytes(id.getRaw()), id -> id));
-    Set<StorageId> localOnlyKeys  = SetUtil.difference(localKeys, remoteKeys);
+    Map<String, StorageId> localByRawId  = Stream.of(localKeys).collect(Collectors.toMap(id -> Base64.encodeBytes(id.getRaw()), id -> id));
 
-    return new KeyDifferenceResult(new ArrayList<>(remoteOnlyKeys), new ArrayList<>(localOnlyKeys));
+    boolean hasTypeMismatch = remoteByRawId.size() != remoteKeys.size() || localByRawId.size() != localKeys.size();
+
+    Set<String> remoteOnlyRawIds = SetUtil.difference(remoteByRawId.keySet(), localByRawId.keySet());
+    Set<String> localOnlyRawIds  = SetUtil.difference(localByRawId.keySet(), remoteByRawId.keySet());
+    Set<String> sharedRawIds     = SetUtil.intersection(localByRawId.keySet(), remoteByRawId.keySet());
+
+    for (String rawId : sharedRawIds) {
+      StorageId remote = Objects.requireNonNull(remoteByRawId.get(rawId));
+      StorageId local  = Objects.requireNonNull(localByRawId.get(rawId));
+
+      if (remote.getType() != local.getType()) {
+        remoteOnlyRawIds.remove(rawId);
+        localOnlyRawIds.remove(rawId);
+        hasTypeMismatch = true;
+      }
+    }
+
+    List<StorageId> remoteOnlyKeys = Stream.of(remoteOnlyRawIds).map(remoteByRawId::get).toList();
+    List<StorageId> localOnlyKeys  = Stream.of(localOnlyRawIds).map(localByRawId::get).toList();
+
+    return new KeyDifferenceResult(remoteOnlyKeys, localOnlyKeys, hasTypeMismatch);
   }
 
   /**
@@ -459,12 +482,15 @@ public final class StorageSyncHelper {
   public static final class KeyDifferenceResult {
     private final List<StorageId> remoteOnlyKeys;
     private final List<StorageId> localOnlyKeys;
+    private final boolean         hasTypeMismatches;
 
     private KeyDifferenceResult(@NonNull List<StorageId> remoteOnlyKeys,
-                                @NonNull List<StorageId> localOnlyKeys)
+                                @NonNull List<StorageId> localOnlyKeys,
+                                boolean hasTypeMismatches)
     {
       this.remoteOnlyKeys    = remoteOnlyKeys;
       this.localOnlyKeys     = localOnlyKeys;
+      this.hasTypeMismatches = hasTypeMismatches;
     }
 
     public @NonNull List<StorageId> getRemoteOnlyKeys() {
@@ -475,6 +501,14 @@ public final class StorageSyncHelper {
       return localOnlyKeys;
     }
 
+    /**
+     * @return True if there exist some keys that have matching raw ID's but different types,
+     *         otherwise false.
+     */
+    public boolean hasTypeMismatches() {
+      return hasTypeMismatches;
+    }
+
     public boolean isEmpty() {
       return remoteOnlyKeys.isEmpty() && localOnlyKeys.isEmpty();
     }

app/src/test/java/org/thoughtcrime/securesms/storage/StorageSyncHelperTest.java

@@ -28,9 +28,11 @@ import org.whispersystems.signalservice.api.storage.StorageId;
 import org.whispersystems.signalservice.api.util.UuidUtil;
 
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
 
@@ -85,20 +87,57 @@ public final class StorageSyncHelperTest {
     KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(1, 2, 3), keyListOf(1, 2, 3));
     assertTrue(result.getLocalOnlyKeys().isEmpty());
     assertTrue(result.getRemoteOnlyKeys().isEmpty());
+    assertFalse(result.hasTypeMismatches());
   }
 
   @Test
   public void findKeyDifference_noOverlap() {
     KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(1, 2, 3), keyListOf(4, 5, 6));
-    assertEquals(keyListOf(1, 2, 3), result.getRemoteOnlyKeys());
+    assertContentsEqual(keyListOf(1, 2, 3), result.getRemoteOnlyKeys());
-    assertEquals(keyListOf(4, 5, 6), result.getLocalOnlyKeys());
+    assertContentsEqual(keyListOf(4, 5, 6), result.getLocalOnlyKeys());
+    assertFalse(result.hasTypeMismatches());
   }
 
   @Test
   public void findKeyDifference_someOverlap() {
     KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(1, 2, 3), keyListOf(2, 3, 4));
-    assertEquals(keyListOf(1), result.getRemoteOnlyKeys());
+    assertContentsEqual(keyListOf(1), result.getRemoteOnlyKeys());
-    assertEquals(keyListOf(4), result.getLocalOnlyKeys());
+    assertContentsEqual(keyListOf(4), result.getLocalOnlyKeys());
+    assertFalse(result.hasTypeMismatches());
+  }
+
+  @Test
+  public void findKeyDifference_typeMismatch_allOverlap() {
+    KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(new HashMap<Integer, Integer>() {{
+                                                                                 put(100, 1);
+                                                                                 put(200, 2);
+                                                                               }}),
+                                                                     keyListOf(new HashMap<Integer, Integer>() {{
+                                                                                 put(100, 1);
+                                                                                 put(200, 1);
+                                                                               }}));
+
+    assertTrue(result.getLocalOnlyKeys().isEmpty());
+    assertTrue(result.getRemoteOnlyKeys().isEmpty());
+    assertTrue(result.hasTypeMismatches());
+  }
+
+  @Test
+  public void findKeyDifference_typeMismatch_someOverlap() {
+    KeyDifferenceResult result = StorageSyncHelper.findKeyDifference(keyListOf(new HashMap<Integer, Integer>() {{
+                                                                       put(100, 1);
+                                                                       put(200, 2);
+                                                                       put(300, 1);
+                                                                     }}),
+                                                                     keyListOf(new HashMap<Integer, Integer>() {{
+                                                                       put(100, 1);
+                                                                       put(200, 1);
+                                                                       put(400, 1);
+                                                                     }}));
+
+    assertContentsEqual(Arrays.asList(StorageId.forType(byteArray(300), 1)), result.getRemoteOnlyKeys());
+    assertContentsEqual(Arrays.asList(StorageId.forType(byteArray(400), 1)), result.getLocalOnlyKeys());
+    assertTrue(result.hasTypeMismatches());
   }
 
   @Test
@@ -488,6 +527,10 @@ public final class StorageSyncHelperTest {
     return Stream.of(byteListOf(vals)).map(b -> StorageId.forType(b, 1)).toList();
   }
 
+  private static List<StorageId> keyListOf(Map<Integer, Integer> vals) {
+    return Stream.of(vals).map(e -> StorageId.forType(byteArray(e.getKey()), e.getValue())).toList();
+  }
+
   private static StorageId contactKey(int val) {
     return StorageId.forContact(byteArray(val));
   }

libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java

@@ -470,10 +470,6 @@ public class SignalServiceAccountManager {
     String       authToken = this.pushServiceSocket.getStorageAuth();
     StorageItems items     = this.pushServiceSocket.readStorageItems(authToken, operation.build());
 
-    if (items.getItemsCount() != storageKeys.size()) {
-      Log.w(TAG, "Failed to find all remote keys! Requested: " + storageKeys.size() + ", Found: " + items.getItemsCount());
-    }
-
     for (StorageItem item : items.getItemsList()) {
       Integer type = typeMap.get(item.getKey());
       if (type != null) {