From 35c102aa988a26148cd91bb91fa9d6bda2eae652 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Tue, 13 Apr 2021 11:32:24 -0400 Subject: [PATCH] Fix issues with StorageSyncV2 bookkeeping. 1. I screwed up the comparators in the record processor. Pretty bad, glad this was caught. 2. Previously I was sort of keeping track of which local-only records were accounted for while I was merging, and then hoping everything worked out in the end. Now I just very directly take some set differences and retrieve the appropriate records, so it's clear that we should never fail certain validations. 3. Rev's the feature flag so we don't turn on something broken. --- .../securesms/database/RecipientDatabase.java | 6 +- .../securesms/jobs/StorageSyncJobV2.java | 60 ++++++++++++++----- .../storage/ContactRecordProcessor.java | 2 +- .../DefaultStorageRecordProcessor.java | 5 +- .../storage/GroupV1RecordProcessor.java | 2 +- .../storage/GroupV2RecordProcessor.java | 2 +- .../storage/StorageRecordProcessor.java | 10 +--- .../securesms/storage/StorageSyncHelper.java | 9 ++- .../storage/StorageSyncValidations.java | 5 ++ .../securesms/util/FeatureFlags.java | 2 +- .../api/storage/SignalAccountRecord.java | 5 ++ .../api/storage/SignalContactRecord.java | 5 ++ .../api/storage/SignalGroupV1Record.java | 4 ++ .../api/storage/SignalGroupV2Record.java | 4 ++ 14 files changed, 81 insertions(+), 40 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java index c1622e69a..033d780c1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java @@ -2737,7 +2737,7 @@ public class RecipientDatabase extends Database { } } - public void clearDirtyStateForRecords(@NonNull List records) { + public void clearDirtyStateForStorageIds(@NonNull Collection ids) { SQLiteDatabase db = databaseHelper.getWritableDatabase(); Preconditions.checkArgument(db.inTransaction(), "Database should already be in a transaction."); @@ -2747,8 +2747,8 @@ public class RecipientDatabase extends Database { String query = STORAGE_SERVICE_ID + " = ?"; - for (SignalRecord record : records) { - String[] args = SqlUtil.buildArgs(Base64.encodeBytes(record.getId().getRaw())); + for (StorageId id : ids) { + String[] args = SqlUtil.buildArgs(Base64.encodeBytes(id.getRaw())); db.update(TABLE_NAME, values, query, args); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java index 294d4b194..5a7f455d4 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java @@ -34,6 +34,7 @@ import org.thoughtcrime.securesms.storage.StorageSyncHelper.WriteOperationResult import org.thoughtcrime.securesms.storage.StorageSyncModels; import org.thoughtcrime.securesms.storage.StorageSyncValidations; import org.thoughtcrime.securesms.transport.RetryLaterException; +import org.thoughtcrime.securesms.util.SetUtil; import org.thoughtcrime.securesms.util.TextSecurePreferences; import org.thoughtcrime.securesms.util.Util; import org.whispersystems.libsignal.InvalidKeyException; @@ -54,10 +55,12 @@ import org.whispersystems.signalservice.internal.storage.protos.ManifestRecord; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.Locale; +import java.util.Set; import java.util.concurrent.TimeUnit; /** @@ -222,7 +225,6 @@ public class StorageSyncJobV2 extends BaseJob { List localStorageIdsBeforeMerge = getAllLocalStorageIds(context, Recipient.self().fresh()); KeyDifferenceResult keyDifference = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), localStorageIdsBeforeMerge); - if (keyDifference.hasTypeMismatches()) { Log.w(TAG, "[Remote Sync] Found type mismatches in the key sets! Scheduling a force push after this sync completes."); needsForcePush = true; @@ -232,7 +234,6 @@ public class StorageSyncJobV2 extends BaseJob { Log.i(TAG, "[Remote Sync] Retrieving records for key difference: " + keyDifference); List remoteOnly = accountManager.readStorageRecords(storageServiceKey, keyDifference.getRemoteOnlyKeys()); - List localOnly = buildLocalStorageRecords(context, self, keyDifference.getLocalOnlyKeys()); if (remoteOnly.size() != keyDifference.getRemoteOnlyKeys().size()) { Log.w(TAG, "[Remote Sync] Could not find all remote-only records! Requested: " + keyDifference.getRemoteOnlyKeys().size() + ", Found: " + remoteOnly.size() + ". Scheduling a force push after this sync completes."); @@ -290,17 +291,26 @@ public class StorageSyncJobV2 extends BaseJob { Log.i(TAG, "IDs : " + localStorageIdsBeforeMerge.size() + " IDs before merge, " + localStorageIdsAfterMerge.size() + " IDs after merge"); } - localOnly.removeAll(contactResult.getLocalMatches()); - localOnly.removeAll(gv1Result.getLocalMatches()); - localOnly.removeAll(gv2Result.getLocalMatches()); - localOnly.removeAll(accountResult.getLocalMatches()); - - recipientDatabase.clearDirtyStateForRecords(localOnly); - - Log.i(TAG, "[Remote Sync] After the conflict resolution, there are " + localOnly.size() + " local-only records remaining."); - //noinspection unchecked Stop yelling at my beautiful method signatures - mergeWriteOperation = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), localStorageIdsAfterMerge, localOnly, contactResult, gv1Result, gv2Result, accountResult); + mergeWriteOperation = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), localStorageIdsAfterMerge, contactResult, gv1Result, gv2Result, accountResult); + + List postMergeLocalOnlyIds = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), mergeWriteOperation.getManifest().getStorageIds()).getLocalOnlyKeys(); + List remoteInsertIds = Stream.of(mergeWriteOperation.getInserts()).map(SignalStorageRecord::getId).toList(); + Set unhandledLocalOnlyIds = SetUtil.difference(postMergeLocalOnlyIds, remoteInsertIds); + + if (unhandledLocalOnlyIds.size() > 0) { + Log.i(TAG, "[Remote Sync] After the conflict resolution, there are " + unhandledLocalOnlyIds.size() + " local-only records remaining that weren't otherwise inserted. Adding them as inserts."); + + List localOnly = buildLocalStorageRecords(context, self, unhandledLocalOnlyIds); + + mergeWriteOperation = new WriteOperationResult(mergeWriteOperation.getManifest(), + Util.concatenatedList(mergeWriteOperation.getInserts(), localOnly), + mergeWriteOperation.getDeletes()); + + recipientDatabase.clearDirtyStateForStorageIds(unhandledLocalOnlyIds); + } else { + Log.i(TAG, "[Remote Sync] After the conflict resolution, there are no local-only records remaining."); + } StorageSyncValidations.validate(mergeWriteOperation, remoteManifest, needsForcePush); db.setTransactionSuccessful(); @@ -409,7 +419,11 @@ public class StorageSyncJobV2 extends BaseJob { DatabaseFactory.getStorageKeyDatabase(context).getAllKeys()); } - private static @NonNull List buildLocalStorageRecords(@NonNull Context context, @NonNull Recipient self, @NonNull List ids) { + private static @NonNull List buildLocalStorageRecords(@NonNull Context context, @NonNull Recipient self, @NonNull Collection ids) { + if (ids.isEmpty()) { + return Collections.emptyList(); + } + RecipientDatabase recipientDatabase = DatabaseFactory.getRecipientDatabase(context); StorageKeyDatabase storageKeyDatabase = DatabaseFactory.getStorageKeyDatabase(context); @@ -423,12 +437,12 @@ public class StorageSyncJobV2 extends BaseJob { RecipientSettings settings = recipientDatabase.getByStorageId(id.getRaw()); if (settings != null) { if (settings.getGroupType() == RecipientDatabase.GroupType.SIGNAL_V2 && settings.getSyncExtras().getGroupMasterKey() == null) { - Log.w(TAG, "Missing master key on gv2 recipient"); + throw new MissingGv2MasterKeyError(); } else { records.add(StorageSyncModels.localToRemoteRecord(settings)); } } else { - Log.w(TAG, "Missing local recipient model! Type: " + id.getType()); + throw new MissingRecipientModelError("Missing local recipient model! Type: " + id.getType()); } break; case ManifestRecord.Identifier.Type.ACCOUNT_VALUE: @@ -442,7 +456,7 @@ public class StorageSyncJobV2 extends BaseJob { if (unknown != null) { records.add(unknown); } else { - Log.w(TAG, "Missing local unknown model! Type: " + id.getType()); + throw new MissingUnknownModelError("Missing local unknown model! Type: " + id.getType()); } break; } @@ -451,6 +465,20 @@ public class StorageSyncJobV2 extends BaseJob { return records; } + private static final class MissingGv2MasterKeyError extends Error {} + + private static final class MissingRecipientModelError extends Error { + public MissingRecipientModelError(String message) { + super(message); + } + } + + private static final class MissingUnknownModelError extends Error { + public MissingUnknownModelError(String message) { + super(message); + } + } + public static final class Factory implements Job.Factory { @Override public @NonNull StorageSyncJobV2 create(@NonNull Parameters parameters, @NonNull Data data) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java index c4e0f2d94..71c73eacf 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java @@ -141,7 +141,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor implements public @NonNull Result process(@NonNull Collection remoteRecords, @NonNull StorageKeyGenerator keyGenerator) throws IOException { List remoteDeletes = new LinkedList<>(); List> remoteUpdates = new LinkedList<>(); - List localMatches = new LinkedList<>(); Set matchedRecords = new TreeSet<>(this); for (E remote : remoteRecords) { @@ -57,8 +56,6 @@ abstract class DefaultStorageRecordProcessor implements if (local.isPresent()) { E merged = merge(remote, local.get(), keyGenerator); - localMatches.add(local.get()); - if (matchedRecords.contains(local.get())) { Log.w(TAG, "Multiple remote records map to the same local record! Marking this one for deletion. (Type: " + local.get().getClass().getSimpleName() + ")"); remoteDeletes.add(remote); @@ -79,7 +76,7 @@ abstract class DefaultStorageRecordProcessor implements } } - return new Result<>(remoteUpdates, remoteDeletes, localMatches); + return new Result<>(remoteUpdates, remoteDeletes); } /** diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java index 2f0f66672..a3f99104e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java @@ -117,7 +117,7 @@ public final class GroupV1RecordProcessor extends DefaultStorageRecordProcessor< if (Arrays.equals(lhs.getGroupId(), rhs.getGroupId())) { return 0; } else { - return lhs.getGroupId()[0] - rhs.getGroupId()[0]; + return 1; } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java index 19fbf8f78..8b65dbcb1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java @@ -117,7 +117,7 @@ public final class GroupV2RecordProcessor extends DefaultStorageRecordProcessor< if (Arrays.equals(lhs.getMasterKeyBytes(), rhs.getMasterKeyBytes())) { return 0; } else { - return lhs.getMasterKeyBytes()[0] - rhs.getMasterKeyBytes()[0]; + return 1; } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java index 054ffabdb..33876ed73 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java @@ -23,12 +23,10 @@ public interface StorageRecordProcessor { final class Result { private final Collection> remoteUpdates; private final Collection remoteDeletes; - private final Collection localMatches; - Result(@NonNull Collection> remoteUpdates, @NonNull Collection remoteDeletes, @NonNull Collection localMatches) { + Result(@NonNull Collection> remoteUpdates, @NonNull Collection remoteDeletes) { this.remoteDeletes = remoteDeletes; this.remoteUpdates = remoteUpdates; - this.localMatches = Stream.of(localMatches).map(SignalRecord::asStorageRecord).toList(); } public @NonNull Collection getRemoteDeletes() { @@ -39,10 +37,6 @@ public interface StorageRecordProcessor { return remoteUpdates; } - public @NonNull Collection getLocalMatches() { - return localMatches; - } - public boolean isLocalOnly() { return remoteUpdates.isEmpty() && remoteDeletes.isEmpty(); } @@ -61,7 +55,7 @@ public interface StorageRecordProcessor { builder.append("- ").append(update.toString()).append("\n"); } - return super.toString(); + return builder.toString(); } } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java index 5678e6d38..5e820ca62 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java @@ -355,10 +355,9 @@ public final class StorageSyncHelper { */ public static @NonNull WriteOperationResult createWriteOperation(long currentManifestVersion, @NonNull List allStorageKeys, - @NonNull List localOnlyRecords, @NonNull StorageRecordProcessor.Result... results) { - Set inserts = new LinkedHashSet<>(localOnlyRecords); + Set inserts = new LinkedHashSet<>(); Set deletes = new LinkedHashSet<>(); for (StorageRecordProcessor.Result result : results) { @@ -682,9 +681,9 @@ public final class StorageSyncHelper { private final List inserts; private final List deletes; - private WriteOperationResult(@NonNull SignalStorageManifest manifest, - @NonNull List inserts, - @NonNull List deletes) + public WriteOperationResult(@NonNull SignalStorageManifest manifest, + @NonNull List inserts, + @NonNull List deletes) { this.manifest = manifest; this.inserts = inserts; diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java index 713d744dd..fca159833 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java @@ -7,6 +7,7 @@ import com.annimon.stream.Stream; import org.signal.core.util.logging.Log; import org.thoughtcrime.securesms.recipients.Recipient; +import org.thoughtcrime.securesms.transport.RetryLaterException; import org.thoughtcrime.securesms.util.Base64; import org.thoughtcrime.securesms.util.SetUtil; import org.whispersystems.libsignal.util.guava.Optional; @@ -66,10 +67,12 @@ public final class StorageSyncValidations { Set writeDeletes = Stream.of(result.getDeletes()).map(ByteBuffer::wrap).collect(Collectors.toSet()); if (writeInserts.size() > insertedIds.size()) { + Log.w(TAG, "WriteInserts: " + writeInserts.size() + ", InsertedIds: " + insertedIds.size()); throw new MoreInsertsThanExpectedError(); } if (writeInserts.size() < insertedIds.size()) { + Log.w(TAG, "WriteInserts: " + writeInserts.size() + ", InsertedIds: " + insertedIds.size()); throw new LessInsertsThanExpectedError(); } @@ -78,10 +81,12 @@ public final class StorageSyncValidations { } if (writeDeletes.size() > deletedIds.size()) { + Log.w(TAG, "WriteDeletes: " + writeDeletes.size() + ", DeletedIds: " + deletedIds.size()); throw new MoreDeletesThanExpectedError(); } if (writeDeletes.size() < deletedIds.size()) { + Log.w(TAG, "WriteDeletes: " + writeDeletes.size() + ", DeletedIds: " + deletedIds.size()); throw new LessDeletesThanExpectedError(); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java index 49b44b2ec..05408416a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java @@ -74,7 +74,7 @@ public final class FeatureFlags { private static final String ANIMATED_STICKER_MIN_TOTAL_MEMORY = "android.animatedStickerMinTotalMemory"; private static final String MESSAGE_PROCESSOR_ALARM_INTERVAL = "android.messageProcessor.alarmIntervalMins"; private static final String MESSAGE_PROCESSOR_DELAY = "android.messageProcessor.foregroundDelayMs"; - private static final String STORAGE_SYNC_V2 = "android.storageSyncV2"; + private static final String STORAGE_SYNC_V2 = "android.storageSyncV2.2"; /** * We will only store remote values for flags in this set. If you want a flag to be controllable diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java index 4d4dfa2e9..034512c61 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java @@ -12,6 +12,7 @@ import org.whispersystems.signalservice.api.util.UuidUtil; import org.whispersystems.signalservice.internal.storage.protos.AccountRecord; import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -64,6 +65,10 @@ public final class SignalAccountRecord implements SignalRecord { SignalAccountRecord that = (SignalAccountRecord) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Objects.equals(this.givenName, that.givenName)) { diff.add("GivenName"); } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java index d86ebe03e..3fa3586aa 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java @@ -10,6 +10,7 @@ import org.whispersystems.signalservice.api.util.UuidUtil; import org.whispersystems.signalservice.internal.storage.protos.ContactRecord; import org.whispersystems.signalservice.internal.storage.protos.ContactRecord.IdentityState; +import java.util.Arrays; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -56,6 +57,10 @@ public final class SignalContactRecord implements SignalRecord { SignalContactRecord that = (SignalContactRecord) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Objects.equals(this.getAddress().getNumber(), that.getAddress().getNumber())) { diff.add("E164"); } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java index 3c9bb4a4c..15e92cb42 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java @@ -40,6 +40,10 @@ public final class SignalGroupV1Record implements SignalRecord { SignalGroupV1Record that = (SignalGroupV1Record) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Arrays.equals(this.groupId, that.groupId)) { diff.add("MasterKey"); } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java index 32cd6a2bf..91adb9aa8 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java @@ -42,6 +42,10 @@ public final class SignalGroupV2Record implements SignalRecord { SignalGroupV2Record that = (SignalGroupV2Record) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Arrays.equals(this.getMasterKeyBytes(), that.getMasterKeyBytes())) { diff.add("MasterKey"); }