Local backup validation improvements

Co-authored-by: trevor-signal <131492920+trevor-signal@users.noreply.github.com>

ts/components/Preferences.dom.stories.tsx

@@ -115,6 +115,7 @@ const availableSpeakers = [
 
 const validateBackupResult: ExportResultType = {
   attachmentBackupJobs: [],
+  mediaNames: [],
   totalBytes: 100,
   duration: 10000,
   stats: {

ts/services/backups/export.preload.ts

@@ -291,7 +291,11 @@ export class BackupExportStream extends Readable {
   // array.
   #customColorIdByUuid = new Map<string, Long>();
 
-  constructor(private readonly options: Readonly<BackupExportOptions>) {
+  constructor(
+    private readonly options: Readonly<BackupExportOptions> & {
+      validationRun?: boolean;
+    }
+  ) {
     super();
   }
 
@@ -365,8 +369,8 @@ export class BackupExportStream extends Readable {
     );
   }
 
-  public getMediaNamesIterator(): MapIterator<string> {
-    return this.#mediaNamesToFilePointers.keys();
+  public getMediaNames(): Array<string> {
+    return [...this.#mediaNamesToFilePointers.keys()];
   }
 
   public getStats(): Readonly<StatsType> {
@@ -826,6 +830,11 @@ export class BackupExportStream extends Readable {
               chatItem,
             });
 
+            if (this.options.validationRun) {
+              // flush every chatItem to expose all validation errors
+              await this.#flush();
+            }
+
             this.#stats.messages += 1;
           },
           { concurrency: MAX_CONCURRENCY }

ts/services/backups/index.preload.ts

@@ -354,10 +354,8 @@ export class BackupsService {
       fnLog.info('offline; skipping wait for empty queues');
     }
 
-    const snapshotDir = join(
-      options.backupsBaseDir,
-      `signal-backup-${getTimestampForFolder()}`
-    );
+    // Just in case it's been deleted, ensure the backup dir exists
+    await mkdir(options.backupsBaseDir, { recursive: true });
 
     const freeSpaceBytes = await getFreeDiskSpace(options.backupsBaseDir);
     const bytesNeeded = MIMINUM_DISK_SPACE_FOR_LOCAL_EXPORT - freeSpaceBytes;
@@ -378,15 +376,31 @@ export class BackupsService {
         backupsBaseDir: options.backupsBaseDir,
       });
 
+    const snapshotDir = join(
+      options.backupsBaseDir,
+      `signal-backup-${getTimestampForFolder()}`
+    );
+
     try {
       await mkdir(snapshotDir, { recursive: true });
 
       const exportResult = await this.exportToDisk(join(snapshotDir, 'main'), {
         type: 'local-encrypted',
-        snapshotDir,
         abortSignal: options.abortSignal,
       });
 
+      log.info('exportLocalBackup: writing local backup files list');
+      const filesWritten = await writeLocalBackupFilesList({
+        snapshotDir,
+        mediaNames: exportResult.mediaNames,
+      });
+      const filesRead = await readLocalBackupFilesList(snapshotDir);
+      strictAssert(
+        isEqual(filesWritten, filesRead),
+        'exportBackup: Local backup files proto must match files written'
+      );
+
+      log.info('exportLocalBackup: writing metadata');
       const metadataArgs = {
         snapshotDir,
         backupId: getBackupId(),
@@ -394,6 +408,7 @@ export class BackupsService {
       };
       await writeLocalBackupMetadata(metadataArgs);
       await verifyLocalBackupMetadata(metadataArgs);
+
       await this.#runLocalAttachmentBackupJobs({
         attachmentBackupJobs: exportResult.attachmentBackupJobs,
         baseDir: options.backupsBaseDir,
@@ -702,14 +717,19 @@ export class BackupsService {
   }
 
   // Test harness
-  public async _internalValidate(): Promise<ValidationResultType> {
+  public async _internalValidate(
+    exportOptions: BackupExportOptions = {
+      type: 'local-encrypted',
+      abortSignal: new AbortController().signal,
+    }
+  ): Promise<ValidationResultType> {
     try {
+      log.info('internal validation: starting');
       const start = Date.now();
 
       const recordStream = new BackupExportStream({
-        type: 'remote',
-        level: BackupLevel.Free,
-        abortSignal: new AbortController().signal,
+        ...exportOptions,
+        validationRun: true,
       });
 
       recordStream.run();
@@ -718,15 +738,21 @@ export class BackupsService {
 
       const duration = Date.now() - start;
 
+      log.info('internal validation: succeeded');
       return {
         result: {
           attachmentBackupJobs: recordStream.getAttachmentBackupJobs(),
+          mediaNames: recordStream.getMediaNames(),
           duration,
           stats: recordStream.getStats(),
           totalBytes,
         },
       };
     } catch (error) {
+      log.warn(
+        'internal validation: failed with errors',
+        Errors.toLogFormat(error)
+      );
       return { error: Errors.toLogFormat(error) };
     }
   }
@@ -1212,22 +1238,10 @@ export class BackupsService {
           throw missingCaseError(type);
       }
 
-      if (type === 'local-encrypted') {
-        log.info('exportBackup: writing local backup files list');
-        const filesWritten = await writeLocalBackupFilesList({
-          snapshotDir: options.snapshotDir,
-          mediaNamesIterator: recordStream.getMediaNamesIterator(),
-        });
-        const filesRead = await readLocalBackupFilesList(options.snapshotDir);
-        strictAssert(
-          isEqual(filesWritten, filesRead),
-          'exportBackup: Local backup files proto must match files written'
-        );
-      }
-
       const duration = Date.now() - start;
       return {
         attachmentBackupJobs: recordStream.getAttachmentBackupJobs(),
+        mediaNames: recordStream.getMediaNames(),
         totalBytes,
         stats: recordStream.getStats(),
         duration,

ts/services/backups/types.std.ts

@@ -39,13 +39,8 @@ export type BackupExportOptions = { abortSignal: AbortSignal } & (
       type: 'remote' | 'cross-client-integration-test';
       level: BackupLevel;
     }
-  | {
-      type: 'plaintext-export';
-    }
-  | {
-      type: 'local-encrypted';
-      snapshotDir: string;
-    }
+  | { type: 'plaintext-export' }
+  | { type: 'local-encrypted' }
 );
 
 export type BackupImportOptions = (
@@ -86,6 +81,7 @@ export type ExportResultType = Readonly<{
   attachmentBackupJobs: ReadonlyArray<
     CoreAttachmentBackupJobType | CoreAttachmentLocalBackupJobType
   >;
+  mediaNames: Array<string>;
   totalBytes: number;
   duration: number;
   stats: Readonly<StatsType>;

ts/services/backups/util/localBackup.node.ts

@@ -153,10 +153,10 @@ export async function verifyLocalBackupMetadata({
 
 export async function writeLocalBackupFilesList({
   snapshotDir,
-  mediaNamesIterator,
+  mediaNames,
 }: {
   snapshotDir: string;
-  mediaNamesIterator: MapIterator<string>;
+  mediaNames: Array<string>;
 }): Promise<ReadonlyArray<string>> {
   const { promise, resolve, reject } = explodePromise<ReadonlyArray<string>>();
 
@@ -167,7 +167,7 @@ export async function writeLocalBackupFilesList({
   });
 
   const files: Array<string> = [];
-  for (const mediaName of mediaNamesIterator) {
+  for (const mediaName of mediaNames) {
     const data = Signal.backup.local.FilesFrame.encodeDelimited({
       mediaName,
     }).finish();

ts/services/backups/validator.preload.ts

@@ -62,6 +62,8 @@ export async function validateBackupStream(
 
   let totalBytes = 0;
   let frameCount = 0;
+
+  const allErrorMessages: Array<string> = [];
   readable.on('data', delimitedFrame => {
     totalBytes += delimitedFrame.byteLength;
     frameCount += 1;
@@ -79,12 +81,24 @@ export async function validateBackupStream(
     }
 
     strictAssert(validator != null, 'validator must be already created');
-    validator.addFrame(frame);
+    try {
+      validator.addFrame(frame);
+    } catch (error) {
+      allErrorMessages.push(error.message);
+    }
   });
 
   await once(readable, 'end');
   strictAssert(validator != null, 'no frames');
-  validator.finalize();
 
+  try {
+    validator.finalize();
+  } catch (error) {
+    allErrorMessages.push(error.message);
+  }
+
+  if (allErrorMessages.length) {
+    throw new Error(allErrorMessages.join('\n'));
+  }
   return totalBytes;
 }

ts/test-electron/backup/filePointer_test.preload.ts

@@ -467,7 +467,6 @@ describe('getFilePointerForAttachment', () => {
           attachment: defaultAttachment,
           backupOptions: {
             type: 'local-encrypted',
-            snapshotDir: '/root/backups/signal-backup-12-12-12',
             abortSignal: new AbortController().signal,
           },
           getBackupCdnInfo: notInBackupCdn,
@@ -507,7 +506,6 @@ describe('getFilePointerForAttachment', () => {
           attachment: { ...defaultAttachment, path: 'no/file/here' },
           backupOptions: {
             type: 'local-encrypted',
-            snapshotDir: '/root/backups/signal-backup-12-12-12',
             abortSignal: new AbortController().signal,
           },
           getBackupCdnInfo: notInBackupCdn,