From eed65e52fd080e93c3ff9e779ae7b7730badbaa4 Mon Sep 17 00:00:00 2001 From: automated-signal <37887102+automated-signal@users.noreply.github.com> Date: Tue, 20 Jan 2026 18:02:23 -0600 Subject: [PATCH] Cleanup after canceled local backup export Co-authored-by: trevor-signal <131492920+trevor-signal@users.noreply.github.com> --- ts/services/backups/export.preload.ts | 23 +++- ts/services/backups/index.preload.ts | 115 +++++++++++------- ts/services/backups/types.std.ts | 5 +- ts/services/backups/util/localBackup.node.ts | 30 ++++- ts/sql/main.main.ts | 5 +- .../backup/filePointer_test.preload.ts | 9 ++ ts/test-electron/backup/helpers.preload.ts | 1 + .../backup/integration_test.preload.ts | 1 + 8 files changed, 139 insertions(+), 50 deletions(-) diff --git a/ts/services/backups/export.preload.ts b/ts/services/backups/export.preload.ts index 55dcb23ae7..4d224ac311 100644 --- a/ts/services/backups/export.preload.ts +++ b/ts/services/backups/export.preload.ts @@ -293,12 +293,27 @@ export class BackupExportStream extends Readable { super(); } + async #cleanupAfterError() { + log.warn('Cleaning up after error...'); + await resumeWriteAccess(); + } + + override _destroy( + error: Error | null, + callback: (error?: Error | null) => void + ): void { + if (error) { + drop(this.#cleanupAfterError()); + } + callback(error); + } + public run(): void { drop( (async () => { - log.info('BackupExportStream: starting...'); + log.info('starting...'); drop(AttachmentBackupManager.stop()); - log.info('BackupExportStream: message migration starting...'); + log.info('message migration starting...'); await migrateAllMessages(); await pauseWriteAccess(); @@ -317,7 +332,7 @@ export class BackupExportStream extends Readable { this.#attachmentBackupJobs.map(job => { if (job.type === 'local') { log.error( - "BackupExportStream: Can't enqueue local backup jobs during remote backup, skipping" + "Can't enqueue local backup jobs during remote backup, skipping" ); return Promise.resolve(); } @@ -338,7 +353,7 @@ export class BackupExportStream extends Readable { } log.info('finished successfully'); } catch (error) { - await resumeWriteAccess(); + await this.#cleanupAfterError(); log.error('errored', toLogFormat(error)); this.emit('error', error); } finally { diff --git a/ts/services/backups/index.preload.ts b/ts/services/backups/index.preload.ts index e25151e868..b8da1d496a 100644 --- a/ts/services/backups/index.preload.ts +++ b/ts/services/backups/index.preload.ts @@ -22,10 +22,7 @@ import * as Bytes from '../../Bytes.std.js'; import { strictAssert } from '../../util/assert.std.js'; import { drop } from '../../util/drop.std.js'; import { TEMP_PATH } from '../../util/basePaths.preload.js'; -import { - getAbsoluteDownloadsPath, - saveAttachmentToDisk, -} from '../../util/migrations.preload.js'; +import { getAbsoluteDownloadsPath } from '../../util/migrations.preload.js'; import { waitForAllBatchers } from '../../util/batcher.std.js'; import { flushAllWaitBatchers } from '../../util/waitBatcher.std.js'; import { DelimitedStream } from '../../util/DelimitedStream.node.js'; @@ -51,7 +48,7 @@ import { } from '../../types/backups.node.js'; import { HTTPError } from '../../types/HTTPError.std.js'; import { constantTimeEqual } from '../../Crypto.node.js'; -import { measureSize } from '../../AttachmentCrypto.node.js'; +import { measureSize, safeUnlink } from '../../AttachmentCrypto.node.js'; import { signalProtocolStore } from '../../SignalProtocolStore.preload.js'; import { isTestOrMockEnvironment } from '../../environment.std.js'; import { runStorageServiceSyncJob } from '../storage.preload.js'; @@ -94,6 +91,8 @@ import { writeLocalBackupFilesList, readLocalBackupFilesList, validateLocalBackupStructure, + getAllPathsInLocalBackupFilesDirectory, + getLocalBackupFilesDirectory, } from './util/localBackup.node.js'; import { AttachmentPermanentlyMissingError, @@ -327,6 +326,7 @@ export class BackupsService { const { totalBytes } = await this.exportToDisk(filePath, { type: 'remote', level: backupLevel, + abortSignal: new AbortController().signal, }); await this.api.upload(filePath, totalBytes); @@ -358,37 +358,78 @@ export class BackupsService { options.backupsBaseDir, `signal-backup-${getTimestampForFolder()}` ); - await mkdir(snapshotDir, { recursive: true }); - const freeSpaceBytes = await getFreeDiskSpace(snapshotDir); + const freeSpaceBytes = await getFreeDiskSpace(options.backupsBaseDir); const bytesNeeded = MIMINUM_DISK_SPACE_FOR_LOCAL_EXPORT - freeSpaceBytes; if (bytesNeeded > 0) { fnLog.info( `Not enough storage; only ${freeSpaceBytes} available, ${MIMINUM_DISK_SPACE_FOR_LOCAL_EXPORT} is minimum needed` ); - throw new NotEnoughStorageError(bytesNeeded); } - const exportResult = await this.exportToDisk(join(snapshotDir, 'main'), { - type: 'local-encrypted', - snapshotDir, + const filesDir = getLocalBackupFilesDirectory({ + backupsBaseDir: options.backupsBaseDir, }); - const metadataArgs = { - snapshotDir, - backupId: getBackupId(), - metadataKey: getLocalBackupMetadataKey(), - }; - await writeLocalBackupMetadata(metadataArgs); - await verifyLocalBackupMetadata(metadataArgs); - await this.#runLocalAttachmentBackupJobs({ - attachmentBackupJobs: exportResult.attachmentBackupJobs, - baseDir: options.backupsBaseDir, - onProgress: options.onProgress, - abortSignal: options.abortSignal, - }); + await mkdir(filesDir, { recursive: true }); - return { ...exportResult, snapshotDir }; + const attachmentPathsWrittenBeforeThisBackup = + await getAllPathsInLocalBackupFilesDirectory({ + backupsBaseDir: options.backupsBaseDir, + }); + + try { + await mkdir(snapshotDir, { recursive: true }); + + const exportResult = await this.exportToDisk(join(snapshotDir, 'main'), { + type: 'local-encrypted', + snapshotDir, + abortSignal: options.abortSignal, + }); + + const metadataArgs = { + snapshotDir, + backupId: getBackupId(), + metadataKey: getLocalBackupMetadataKey(), + }; + await writeLocalBackupMetadata(metadataArgs); + await verifyLocalBackupMetadata(metadataArgs); + await this.#runLocalAttachmentBackupJobs({ + attachmentBackupJobs: exportResult.attachmentBackupJobs, + baseDir: options.backupsBaseDir, + onProgress: options.onProgress, + abortSignal: options.abortSignal, + }); + + return { ...exportResult, snapshotDir }; + } catch (e) { + if (options.abortSignal.aborted) { + log.warn('exportLocalBackup aborted', Errors.toLogFormat(e)); + } else { + log.error('exportLocalBackup encountered error', Errors.toLogFormat(e)); + } + + log.info('Deleting snapshot directory'); + await rm(snapshotDir, { recursive: true, force: true }); + log.info('Deleted Snapshot directory'); + + const attachmentPathsAfterBackup = + await getAllPathsInLocalBackupFilesDirectory({ + backupsBaseDir: options.backupsBaseDir, + }); + + const pathsAdded = new Set(attachmentPathsAfterBackup).difference( + new Set(attachmentPathsWrittenBeforeThisBackup) + ); + + if (pathsAdded.size > 0) { + log.info(`Deleting ${pathsAdded.size} newly written files`); + await Promise.all([...pathsAdded].map(safeUnlink)); + log.info(`Deleted ${pathsAdded.size} files`); + } + + throw e; + } } async #runLocalAttachmentBackupJobs({ @@ -600,6 +641,7 @@ export class BackupsService { join(exportDir, 'main.jsonl'), { type: 'plaintext-export', + abortSignal, } ); @@ -667,6 +709,7 @@ export class BackupsService { const recordStream = new BackupExportStream({ type: 'remote', level: BackupLevel.Free, + abortSignal: new AbortController().signal, }); recordStream.run(); @@ -688,19 +731,6 @@ export class BackupsService { } } - // Test harness - public async exportWithDialog(): Promise { - const { data } = await this.exportBackupData({ - type: 'remote', - level: BackupLevel.Free, - }); - - await saveAttachmentToDisk({ - name: 'backup.bin', - data, - }); - } - public async importFromDisk( backupFile: string, options: BackupImportOptions @@ -1146,7 +1176,8 @@ export class BackupsService { totalBytes = size; }, }), - sink + sink, + { signal: options.abortSignal } ); break; case 'cross-client-integration-test': @@ -1161,7 +1192,8 @@ export class BackupsService { totalBytes = size; }, }), - sink + sink, + { signal: options.abortSignal } ); break; case 'plaintext-export': @@ -1172,7 +1204,8 @@ export class BackupsService { totalBytes = size; }, }), - sink + sink, + { signal: options.abortSignal } ); break; default: diff --git a/ts/services/backups/types.std.ts b/ts/services/backups/types.std.ts index c735e67b08..a3460bfe5e 100644 --- a/ts/services/backups/types.std.ts +++ b/ts/services/backups/types.std.ts @@ -34,7 +34,7 @@ export type OnProgressCallback = ( totalBytes: number ) => void; -export type BackupExportOptions = +export type BackupExportOptions = { abortSignal: AbortSignal } & ( | { type: 'remote' | 'cross-client-integration-test'; level: BackupLevel; @@ -45,7 +45,8 @@ export type BackupExportOptions = | { type: 'local-encrypted'; snapshotDir: string; - }; + } +); export type BackupImportOptions = ( | { type: 'remote' | 'cross-client-integration-test' } diff --git a/ts/services/backups/util/localBackup.node.ts b/ts/services/backups/util/localBackup.node.ts index 7faeb75425..0573b250bc 100644 --- a/ts/services/backups/util/localBackup.node.ts +++ b/ts/services/backups/util/localBackup.node.ts @@ -3,7 +3,7 @@ import { randomBytes } from 'node:crypto'; import { dirname, join } from 'node:path'; -import { readFile, stat, writeFile } from 'node:fs/promises'; +import { readdir, readFile, stat, writeFile } from 'node:fs/promises'; import { createReadStream, createWriteStream } from 'node:fs'; import { Transform } from 'node:stream'; import { pipeline } from 'node:stream/promises'; @@ -25,6 +25,29 @@ const log = createLogger('localBackup'); const { Reader } = protobuf; +export function getLocalBackupFilesDirectory({ + backupsBaseDir, +}: { + backupsBaseDir: string; +}): string { + return join(backupsBaseDir, 'files'); +} + +export async function getAllPathsInLocalBackupFilesDirectory({ + backupsBaseDir, +}: { + backupsBaseDir: string; +}): Promise> { + const filesDir = getLocalBackupFilesDirectory({ backupsBaseDir }); + const allEntries = await readdir(filesDir, { + withFileTypes: true, + recursive: true, + }); + return allEntries + .filter(entry => entry.isFile()) + .map(entry => join(entry.parentPath, entry.name)); +} + export function getLocalBackupDirectoryForMediaName({ backupsBaseDir, mediaName, @@ -36,7 +59,10 @@ export function getLocalBackupDirectoryForMediaName({ throw new Error('Invalid mediaName input'); } - return join(backupsBaseDir, 'files', mediaName.substring(0, 2)); + return join( + getLocalBackupFilesDirectory({ backupsBaseDir }), + mediaName.substring(0, 2) + ); } export function getLocalBackupPathForMediaName({ diff --git a/ts/sql/main.main.ts b/ts/sql/main.main.ts index a955d27e51..25f4cc735e 100644 --- a/ts/sql/main.main.ts +++ b/ts/sql/main.main.ts @@ -204,7 +204,10 @@ export class MainSQL { public resumeWriteAccess(): void { const pauseWaiters = this.#pauseWaiters; - strictAssert(pauseWaiters != null, 'Not paused'); + if (pauseWaiters == null) { + return; + } + this.#pauseWaiters = undefined; for (const waiter of pauseWaiters) { diff --git a/ts/test-electron/backup/filePointer_test.preload.ts b/ts/test-electron/backup/filePointer_test.preload.ts index 983bf29f54..4f33f74f28 100644 --- a/ts/test-electron/backup/filePointer_test.preload.ts +++ b/ts/test-electron/backup/filePointer_test.preload.ts @@ -241,6 +241,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'remote', level: BackupLevel.Paid, + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -271,6 +272,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'remote', level: BackupLevel.Paid, + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -298,6 +300,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'remote', level: BackupLevel.Free, + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -326,6 +329,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'remote', level: BackupLevel.Free, + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -354,6 +358,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'remote', level: BackupLevel.Free, + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -378,6 +383,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'remote', level: BackupLevel.Paid, + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -422,6 +428,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'remote', level: BackupLevel.Paid, + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -459,6 +466,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'local-encrypted', snapshotDir: '/root/backups/signal-backup-12-12-12', + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, @@ -498,6 +506,7 @@ describe('getFilePointerForAttachment', () => { backupOptions: { type: 'local-encrypted', snapshotDir: '/root/backups/signal-backup-12-12-12', + abortSignal: new AbortController().signal, }, getBackupCdnInfo: notInBackupCdn, messageReceivedAt: 100, diff --git a/ts/test-electron/backup/helpers.preload.ts b/ts/test-electron/backup/helpers.preload.ts index 5522ea45d8..94890c95fd 100644 --- a/ts/test-electron/backup/helpers.preload.ts +++ b/ts/test-electron/backup/helpers.preload.ts @@ -245,6 +245,7 @@ export async function asymmetricRoundtripHarness( await backupsService.exportToDisk(targetOutputFile, { type: 'remote', level: options.backupLevel, + abortSignal: new AbortController().signal, }); await updateConvoIdToTitle(); diff --git a/ts/test-electron/backup/integration_test.preload.ts b/ts/test-electron/backup/integration_test.preload.ts index 4c211c8898..03f7b24363 100644 --- a/ts/test-electron/backup/integration_test.preload.ts +++ b/ts/test-electron/backup/integration_test.preload.ts @@ -59,6 +59,7 @@ describe('backup/integration', () => { const { data: exported } = await backupsService.exportBackupData({ type: 'cross-client-integration-test', level: BackupLevel.Paid, + abortSignal: new AbortController().signal, }); const actualStream = new MemoryStream(Buffer.from(exported));