Dropped storage keys should not cause upload

ts/services/storage.ts

@@ -41,6 +41,11 @@ import { SignalService as Proto } from '../protobuf';
 import * as log from '../logging/log';
 import { singleProtoJobQueue } from '../jobs/singleProtoJobQueue';
 import * as Errors from '../types/errors';
+import type {
+  ExtendedStorageID,
+  RemoteRecord,
+  UnknownRecord,
+} from '../types/StorageService.d';
 
 type IManifestRecordIdentifier = Proto.ManifestRecord.IIdentifier;
 
@@ -80,23 +85,10 @@ function redactStorageID(
   return `${version ?? '?'}:${storageID.substring(0, 3)}${convoId}`;
 }
 
-type RemoteRecord = {
+function redactExtendedStorageID({
-  itemType: number;
-  storageID: string;
-  storageVersion?: number;
-};
-
-type UnknownRecord = RemoteRecord;
-
-type ProcessRemoteRecordsResultType = Readonly<{
-  conflictCount: number;
-  deleteKeys: Array<string>;
-}>;
-
-function unknownRecordToRedactedID({
   storageID,
   storageVersion,
-}: UnknownRecord): string {
+}: ExtendedStorageID): string {
   return redactStorageID(storageID, storageVersion);
 }
 
@@ -148,12 +140,11 @@ type GeneratedManifestType = {
 async function generateManifest(
   version: number,
   previousManifest?: Proto.IManifestRecord,
-  isNewManifest = false,
+  isNewManifest = false
-  extraDeleteKeys = new Array<string>()
 ): Promise<GeneratedManifestType> {
   log.info(
     `storageService.upload(${version}): generating manifest ` +
-      `new=${isNewManifest} extraDeleteKeys=${extraDeleteKeys.length}`
+      `new=${isNewManifest}`
   );
 
   await window.ConversationController.checkForConflicts();
@@ -166,10 +157,6 @@ async function generateManifest(
   const manifestRecordKeys: Set<IManifestRecordIdentifier> = new Set();
   const newItems: Set<Proto.IStorageItem> = new Set();
 
-  for (const key of extraDeleteKeys) {
-    deleteKeys.push(Bytes.fromBase64(key));
-  }
-
   const conversations = window.getConversations();
   for (let i = 0; i < conversations.length; i += 1) {
     const conversation = conversations.models[i];
@@ -300,7 +287,7 @@ async function generateManifest(
     window.storage.get('storage-service-unknown-records') || []
   ).filter((record: UnknownRecord) => !validRecordTypes.has(record.itemType));
 
-  const redactedUnknowns = unknownRecordsArray.map(unknownRecordToRedactedID);
+  const redactedUnknowns = unknownRecordsArray.map(redactExtendedStorageID);
 
   log.info(
     `storageService.upload(${version}): adding unknown ` +
@@ -322,7 +309,7 @@ async function generateManifest(
     'storage-service-error-records',
     new Array<UnknownRecord>()
   );
-  const redactedErrors = recordsWithErrors.map(unknownRecordToRedactedID);
+  const redactedErrors = recordsWithErrors.map(redactExtendedStorageID);
 
   log.info(
     `storageService.upload(${version}): adding error ` +
@@ -339,6 +326,24 @@ async function generateManifest(
     manifestRecordKeys.add(identifier);
   });
 
+  // Delete keys that we wanted to drop during the processing of the manifest.
+  const storedPendingDeletes = window.storage.get(
+    'storage-service-pending-deletes',
+    []
+  );
+  const redactedPendingDeletes = storedPendingDeletes.map(
+    redactExtendedStorageID
+  );
+  log.info(
+    `storageService.upload(${version}): ` +
+      `deleting extra keys=${JSON.stringify(redactedPendingDeletes)} ` +
+      `count=${redactedPendingDeletes.length}`
+  );
+
+  for (const { storageID } of storedPendingDeletes) {
+    deleteKeys.push(Bytes.fromBase64(storageID));
+  }
+
   // Validate before writing
 
   const rawDuplicates = new Set();
@@ -827,7 +832,7 @@ async function mergeRecord(
 async function processManifest(
   manifest: Proto.IManifestRecord,
   version: number
-): Promise<ProcessRemoteRecordsResultType> {
+): Promise<number> {
   if (!window.textsecure.messaging) {
     throw new Error('storageService.processManifest: We are offline!');
   }
@@ -907,12 +912,8 @@ async function processManifest(
   });
 
   let conflictCount = 0;
-  let deleteKeys = new Array<string>();
   if (remoteOnlyRecords.size) {
-    ({ conflictCount, deleteKeys } = await processRemoteRecords(
+    conflictCount = await processRemoteRecords(version, remoteOnlyRecords);
-      version,
-      remoteOnlyRecords
-    ));
   }
 
   // Post-merge, if our local records contain any storage IDs that were not
@@ -940,17 +941,16 @@ async function processManifest(
   });
 
   log.info(
-    `storageService.process(${version}): conflictCount=${conflictCount} ` +
+    `storageService.process(${version}): conflictCount=${conflictCount}`
-      `deleteKeys=${deleteKeys.length}`
   );
 
-  return { conflictCount, deleteKeys };
+  return conflictCount;
 }
 
 async function processRemoteRecords(
   storageVersion: number,
   remoteOnlyRecords: Map<string, RemoteRecord>
-): Promise<ProcessRemoteRecordsResultType> {
+): Promise<number> {
   const storageKeyBase64 = window.storage.get('storageKey');
   if (!storageKeyBase64) {
     throw new Error('No storage key');
@@ -1102,7 +1102,7 @@ async function processRemoteRecords(
         });
       }
 
-      if (mergedRecord.hasConflict || mergedRecord.shouldDrop) {
+      if (mergedRecord.hasConflict) {
         conflictCount += 1;
       }
 
@@ -1116,7 +1116,7 @@ async function processRemoteRecords(
     );
     log.info(
       `storageService.process(${storageVersion}): ` +
-        `droppedKeys=${JSON.stringify(redactedDroppedKeys)} ` +
+        `dropped keys=${JSON.stringify(redactedDroppedKeys)} ` +
         `count=${redactedDroppedKeys.length}`
     );
 
@@ -1124,9 +1124,7 @@ async function processRemoteRecords(
     const newUnknownRecords = Array.from(unknownRecords.values()).filter(
       (record: UnknownRecord) => !validRecordTypes.has(record.itemType)
     );
-    const redactedNewUnknowns = newUnknownRecords.map(
+    const redactedNewUnknowns = newUnknownRecords.map(redactExtendedStorageID);
-      unknownRecordToRedactedID
-    );
 
     log.info(
       `storageService.process(${storageVersion}): ` +
@@ -1139,7 +1137,7 @@ async function processRemoteRecords(
     );
 
     const redactedErrorRecords = newRecordsWithErrors.map(
-      unknownRecordToRedactedID
+      redactExtendedStorageID
     );
     log.info(
       `storageService.process(${storageVersion}): ` +
@@ -1154,14 +1152,25 @@ async function processRemoteRecords(
       newRecordsWithErrors
     );
 
+    // Store/overwrite keys pending deletion, but use them only when we have to
+    // upload a new manifest to avoid oscillation.
+    const pendingDeletes = [...missingKeys, ...droppedKeys].map(storageID => ({
+      storageID,
+      storageVersion,
+    }));
+    const redactedPendingDeletes = pendingDeletes.map(redactExtendedStorageID);
+    log.info(
+      `storageService.process(${storageVersion}): ` +
+        `pending deletes=${JSON.stringify(redactedPendingDeletes)} ` +
+        `count=${redactedPendingDeletes.length}`
+    );
+    await window.storage.put('storage-service-pending-deletes', pendingDeletes);
+
     if (conflictCount === 0) {
       conflictBackOff.reset();
     }
 
-    return {
+    return conflictCount;
-      conflictCount,
-      deleteKeys: [...missingKeys, ...droppedKeys],
-    };
   } catch (err) {
     log.error(
       `storageService.process(${storageVersion}): ` +
@@ -1170,7 +1179,8 @@ async function processRemoteRecords(
     );
   }
 
-  return { conflictCount: 0, deleteKeys: [] };
+  // conflictCount
+  return 0;
 }
 
 async function sync(
@@ -1218,21 +1228,18 @@ async function sync(
         `version=${localManifestVersion}`
     );
 
-    const { conflictCount, deleteKeys } = await processManifest(
+    const conflictCount = await processManifest(manifest, version);
-      manifest,
-      version
-    );
 
     log.info(
       `storageService.sync: updated to version=${version} ` +
-        `conflicts=${conflictCount} deleteKeys=${deleteKeys.length}`
+        `conflicts=${conflictCount}`
     );
 
     await window.storage.put('manifestVersion', version);
 
-    const hasConflicts = conflictCount !== 0 || deleteKeys.length !== 0;
+    const hasConflicts = conflictCount !== 0;
     if (hasConflicts && !ignoreConflicts) {
-      await upload(true, deleteKeys);
+      await upload(true);
     }
 
     // We now know that we've successfully completed a storage service fetch
@@ -1249,10 +1256,7 @@ async function sync(
   return manifest;
 }
 
-async function upload(
+async function upload(fromSync = false): Promise<void> {
-  fromSync = false,
-  extraDeleteKeys = new Array<string>()
-): Promise<void> {
   if (!window.textsecure.messaging) {
     throw new Error('storageService.upload: We are offline!');
   }
@@ -1322,8 +1326,7 @@ async function upload(
     const generatedManifest = await generateManifest(
       version,
       previousManifest,
-      false,
+      false
-      extraDeleteKeys
     );
     await uploadManifest(version, generatedManifest);
   } catch (err) {

ts/services/storageRecordOps.ts

@@ -558,7 +558,9 @@ export async function mergeGroupV1Record(
     details.push('GV1 record for GV2 group, dropping');
 
     return {
-      hasConflict: true,
+      // Note: conflicts cause immediate uploads, but we should upload
+      // only in response to user's action.
+      hasConflict: false,
       shouldDrop: true,
       conversation,
       oldStorageID,
@@ -771,11 +773,11 @@ export async function mergeContactRecord(
 
   // All contacts must have UUID
   if (!uuid) {
-    return { hasConflict: false, details: ['no uuid'] };
+    return { hasConflict: false, shouldDrop: true, details: ['no uuid'] };
   }
 
   if (!isValidUuid(uuid)) {
-    return { hasConflict: false, details: ['invalid uuid'] };
+    return { hasConflict: false, shouldDrop: true, details: ['invalid uuid'] };
   }
 
   const id = window.ConversationController.ensureContactIds({

ts/test-both/util/normalizeUuid_test.ts

@@ -3,19 +3,36 @@
 
 import { assert } from 'chai';
 import { v4 as generateUuid } from 'uuid';
+import * as sinon from 'sinon';
 
+import type { LoggerType } from '../../types/Logging';
 import { normalizeUuid } from '../../util/normalizeUuid';
 
 describe('normalizeUuid', () => {
-  it('converts uuid to lower case', () => {
+  let warn: sinon.SinonStub;
-    const uuid = generateUuid();
+  let logger: Pick<LoggerType, 'warn'>;
-    assert.strictEqual(normalizeUuid(uuid, 'context 1'), uuid);
+
-    assert.strictEqual(normalizeUuid(uuid.toUpperCase(), 'context 2'), uuid);
+  beforeEach(() => {
+    warn = sinon.stub();
+    logger = { warn };
   });
 
-  it("throws if passed a string that's not a UUID", () => {
+  it('converts uuid to lower case', () => {
-    assert.throws(
+    const uuid = generateUuid();
-      () => normalizeUuid('not-UUID-at-all', 'context 3'),
+    assert.strictEqual(normalizeUuid(uuid, 'context 1', logger), uuid);
+    assert.strictEqual(
+      normalizeUuid(uuid.toUpperCase(), 'context 2', logger),
+      uuid
+    );
+
+    sinon.assert.notCalled(warn);
+  });
+
+  it("warns if passed a string that's not a UUID", () => {
+    normalizeUuid('not-UUID-at-all', 'context 3', logger);
+    sinon.assert.calledOnce(warn);
+    sinon.assert.calledWith(
+      warn,
       'Normalizing invalid uuid: not-UUID-at-all to not-uuid-at-all in ' +
         'context "context 3"'
     );

ts/types/Storage.d.ts

@@ -11,7 +11,12 @@ import type { PhoneNumberDiscoverability } from '../util/phoneNumberDiscoverabil
 import type { PhoneNumberSharingMode } from '../util/phoneNumberSharingMode';
 import type { RetryItemType } from '../util/retryPlaceholders';
 import type { ConfigMapType as RemoteConfigType } from '../RemoteConfig';
-import { SystemTraySetting } from './SystemTraySetting';
+import type { SystemTraySetting } from './SystemTraySetting';
+import type {
+  ExtendedStorageID,
+  RemoteRecord,
+  UnknownRecord,
+} from './StorageService';
 
 import type { GroupCredentialType } from '../textsecure/WebAPI';
 import type {
@@ -105,14 +110,9 @@ export type StorageAccessType = {
   avatarUrl: string;
   manifestVersion: number;
   storageCredentials: StorageServiceCredentials;
-  'storage-service-error-records': Array<{
+  'storage-service-error-records': Array<UnknownRecord>;
-    itemType: number;
+  'storage-service-unknown-records': Array<UnknownRecord>;
-    storageID: string;
+  'storage-service-pending-deletes': Array<ExtendedStorageID>;
-  }>;
-  'storage-service-unknown-records': Array<{
-    itemType: number;
-    storageID: string;
-  }>;
   'preferred-video-input-device': string;
   'preferred-audio-input-device': AudioDevice;
   'preferred-audio-output-device': AudioDevice;

ts/types/StorageService.d.ts

@@ -0,0 +1,13 @@
+// Copyright 2020-2022 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+export type ExtendedStorageID = {
+  storageID: string;
+  storageVersion?: number;
+};
+
+export type RemoteRecord = ExtendedStorageID & {
+  itemType: number;
+};
+
+export type UnknownRecord = RemoteRecord;

ts/util/normalizeUuid.ts

@@ -3,15 +3,24 @@
 
 import type { UUIDStringType } from '../types/UUID';
 import { isValidUuid } from '../types/UUID';
-import { assert } from './assert';
+import type { LoggerType } from '../types/Logging';
+import * as log from '../logging/log';
 
-export function normalizeUuid(uuid: string, context: string): UUIDStringType {
+export function normalizeUuid(
+  uuid: string,
+  context: string,
+  logger: Pick<LoggerType, 'warn'> = log
+): UUIDStringType {
   const result = uuid.toLowerCase();
 
-  assert(
+  if (!isValidUuid(uuid) || !isValidUuid(result)) {
-    isValidUuid(uuid) && isValidUuid(result),
+    logger.warn(
-    `Normalizing invalid uuid: ${uuid} to ${result} in context "${context}"`
+      `Normalizing invalid uuid: ${uuid} to ${result} in context "${context}"`
-  );
+    );
+
+    // Cast anyway we don't want to throw here
+    return result as unknown as UUIDStringType;
+  }
 
   return result;
 }