Skip to content

Don't let one bad attachment fail local backups #6374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
deadcaf3 wants to merge 1 commit into from
Closed

Don't let one bad attachment fail local backups #6374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions SignalServiceKit/Backups/LocalFileBackup/LocalFileBackupExportJob.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ class LocalFileBackupExportJob {

let backupsRootDirectory = LocalFileBackupManager.FileStructure.rootDirectoryInFileLocation(resolvedURL)
let currentDirectoryName: String
let backupDate: Date
let backupSizeBytes: UInt64

switch resumptionPoint {
case nil:
Expand Down Expand Up @@ -153,23 +155,21 @@ class LocalFileBackupExportJob {
localBackupURL: resolvedURL,
)
currentDirectoryName = currentBackupDirectoryName
backupDate = metadata.exportStartDate
backupSizeBytes = UInt64(safeCast: metadata.encryptedDataLength) + metadata.localAttachmentByteSize

let backupFileSizeBytes = UInt64(safeCast: metadata.encryptedDataLength)
let backupMediaSizeBytes = metadata.localAttachmentByteSize
await db.awaitableWrite { tx in
localFileBackupStore.setLastBackupDetails(
date: metadata.exportStartDate,
backupSizeBytes: backupFileSizeBytes + backupMediaSizeBytes,
localFileBackupExportJobStore.setReachedResumptionPoint(
.postBackupFileCopy(directoryName: currentDirectoryName, backupDate: backupDate, backupSizeBytes: backupSizeBytes),
tx: tx,
)
}
await db.awaitableWrite { tx in
localFileBackupExportJobStore.setReachedResumptionPoint(.postBackupFileCopy(directoryName: currentDirectoryName), tx: tx)
}
case .postBackupFileCopy(let directoryName):
case .postBackupFileCopy(let directoryName, let resumedBackupDate, let resumedBackupSizeBytes):
await performWithDummyProgress(progress.child(for: .attachmentMetadataPrep), work: {})
await performWithDummyProgress(progress.child(for: .backupFileExport), work: {})
currentDirectoryName = directoryName
backupDate = resumedBackupDate
backupSizeBytes = resumedBackupSizeBytes
}

logger.info("Copying attachments to disk...")
Expand All @@ -183,6 +183,8 @@ class LocalFileBackupExportJob {
try await localFileBackupManager.cleanUpOldBackupsAndOrphanedAttachments(backupsRootDirectory: backupsRootDirectory)

await db.awaitableWrite { tx in
// Only record the backup once attachments are copied and old backups pruned.
localFileBackupStore.setLastBackupDetails(date: backupDate, backupSizeBytes: backupSizeBytes, tx: tx)
localFileBackupExportJobStore.setReachedResumptionPoint(nil, tx: tx)
}
logger.info("Done!")
Expand Down
23 changes: 18 additions & 5 deletions SignalServiceKit/Backups/LocalFileBackup/LocalFileBackupExportJobStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ public struct LocalFileBackupExportJobStore {
private enum Keys {
static let resumptionPoint = "resumptionPoint"
static let currentBackupDirectoryName = "currentBackupDirectoryName"
static let backupDate = "backupDate"
static let backupSizeBytes = "backupSizeBytes"
}

private let kvStore: NewKeyValueStore
Expand All @@ -21,6 +23,8 @@ public struct LocalFileBackupExportJobStore {
public func wipe(tx: DBWriteTransaction) {
kvStore.removeValue(forKey: Keys.resumptionPoint, tx: tx)
kvStore.removeValue(forKey: Keys.currentBackupDirectoryName, tx: tx)
kvStore.removeValue(forKey: Keys.backupDate, tx: tx)
kvStore.removeValue(forKey: Keys.backupSizeBytes, tx: tx)
}

// MARK: -
Expand All @@ -29,7 +33,8 @@ public struct LocalFileBackupExportJobStore {
/// resumed. If nil, the job should be resumed from the beginning.
public enum ResumptionPoint {
/// The job should be resumed after backup file was copied to disk.
case postBackupFileCopy(directoryName: String)
/// Carries the details to record as the last backup once the job completes.
case postBackupFileCopy(directoryName: String, backupDate: Date, backupSizeBytes: UInt64)
}

public func lastReachedResumptionPoint(tx: DBReadTransaction) -> ResumptionPoint? {
Expand All @@ -38,11 +43,15 @@ public struct LocalFileBackupExportJobStore {
}
switch raw {
case 0:
guard let name = kvStore.fetchValue(String.self, forKey: Keys.currentBackupDirectoryName, tx: tx) else {
owsFailDebug("postBackupFileCopy checkpoint missing directory name")
guard
let name = kvStore.fetchValue(String.self, forKey: Keys.currentBackupDirectoryName, tx: tx),
let backupDate = kvStore.fetchValue(Date.self, forKey: Keys.backupDate, tx: tx),
let backupSizeBytes = kvStore.fetchValue(UInt64.self, forKey: Keys.backupSizeBytes, tx: tx)
else {
owsFailDebug("postBackupFileCopy checkpoint missing directory name or backup details")
return nil
}
return .postBackupFileCopy(directoryName: name)
return .postBackupFileCopy(directoryName: name, backupDate: backupDate, backupSizeBytes: backupSizeBytes)
default:
owsFailDebug("Unexpected resumption point raw value: \(raw)")
return nil
Expand All @@ -54,9 +63,13 @@ public struct LocalFileBackupExportJobStore {
case nil:
kvStore.removeValue(forKey: Keys.resumptionPoint, tx: tx)
kvStore.removeValue(forKey: Keys.currentBackupDirectoryName, tx: tx)
case .postBackupFileCopy(let name):
kvStore.removeValue(forKey: Keys.backupDate, tx: tx)
kvStore.removeValue(forKey: Keys.backupSizeBytes, tx: tx)
case .postBackupFileCopy(let name, let backupDate, let backupSizeBytes):
kvStore.writeValue(Int64(0), forKey: Keys.resumptionPoint, tx: tx)
kvStore.writeValue(name, forKey: Keys.currentBackupDirectoryName, tx: tx)
kvStore.writeValue(backupDate, forKey: Keys.backupDate, tx: tx)
kvStore.writeValue(backupSizeBytes, forKey: Keys.backupSizeBytes, tx: tx)
}
}

Expand Down
27 changes: 20 additions & 7 deletions SignalServiceKit/Backups/LocalFileBackup/LocalFileBackupManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ public class LocalFileBackupManager: NSObject, UIDocumentPickerDelegate {
localFileBackupStore.totalUnencryptedByteCountOfQueuedExports(tx: tx)
}
let source = progressSink?.addSource(withLabel: ProgressLabel.writeQueuedAttachment, unitCount: totalByteCount)
var skippedAttachmentCount = 0

while true {
try Task.checkCancellation()
Expand Down Expand Up @@ -402,13 +403,22 @@ public class LocalFileBackupManager: NSObject, UIDocumentPickerDelegate {
continue
}

let encryptedFileHandle = try Cryptography.encryptedAttachmentFileHandle(
at: attachmentStream.fileURL,
plaintextLength: UInt64(safeCast: attachmentStream.unencryptedByteCount),
attachmentKey: AttachmentKey(combinedKey: attachmentStream.attachment.encryptionKey),
)

let attachmentKey = try AttachmentKey(combinedKey: localKey)
let encryptedFileHandle: EncryptedFileHandle
let attachmentKey: AttachmentKey
do {
encryptedFileHandle = try Cryptography.encryptedAttachmentFileHandle(
at: attachmentStream.fileURL,
plaintextLength: UInt64(safeCast: attachmentStream.unencryptedByteCount),
attachmentKey: AttachmentKey(combinedKey: attachmentStream.attachment.encryptionKey),
)
attachmentKey = try AttachmentKey(combinedKey: localKey)
} catch {
// One unreadable attachment must not fail every backup; skip it.
logger.error("Skipping attachment id \(attachmentWithMetadata.attachment.id), error: \(error)")
skippedAttachmentCount += 1
source?.incrementCompletedUnitCount(by: attachmentByteCount)
continue
}

let attachmentSubDirectory = backupsRootDirectory
.appendingPathComponent(FileStructure.attachmentDirectory.rawValue)
Expand Down Expand Up @@ -451,6 +461,9 @@ public class LocalFileBackupManager: NSObject, UIDocumentPickerDelegate {
}
}
}
if skippedAttachmentCount > 0 {
logger.warn("Skipped \(skippedAttachmentCount) unreadable attachments")
}
source?.complete()
}

Expand Down
2 changes: 1 addition & 1 deletion SignalServiceKit/tests/Backups/LocalFileBackupExportJobRunnerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ struct LocalFileBackupExportJobRunnerTests {

// Seed the store with a resumption point
await db.awaitableWrite { tx in
localFileBackupExportJobStore.setReachedResumptionPoint(.postBackupFileCopy(directoryName: currentBackupDir), tx: tx)
localFileBackupExportJobStore.setReachedResumptionPoint(.postBackupFileCopy(directoryName: currentBackupDir, backupDate: Date(), backupSizeBytes: 0), tx: tx)
}

_ = await jobRunner.resumeIfNecessary()?.result
Expand Down
56 changes: 56 additions & 0 deletions SignalServiceKit/tests/Backups/LocalFileBackupManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,62 @@ struct LocalFileBackupManagerTests {
#expect(FileManager.default.fileExists(atPath: mediaPath2.path))
}

@Test
func testCopyAttachmentsToDiskSkipsMissingFile() async throws {
let missingAttachment = try LocalFileBackupTestSupport.makeMockAttachmentWithRealFile()
let presentAttachment = try LocalFileBackupTestSupport.makeMockAttachmentWithRealFile()

let (missingId, presentId) = db.write { tx in
let missingId = LocalFileBackupTestSupport.insertMockAttachment(missingAttachment, tx: tx)
let presentId = LocalFileBackupTestSupport.insertMockAttachment(presentAttachment, tx: tx)
return (missingId, presentId)
}

try await localFileBackupAttachmentMetadataManager.ensureAttachmentMetadataExists(progressSink: nil)

let collector = LocalFileBackupAttachmentCollector()
collector.append(id: missingId)
collector.append(id: presentId)
try await localFileBackupManager.queueLocalBackupAttachmentsForExport(localFileBackupAttachmentCollector: collector)

try FileManager.default.removeItem(at: missingAttachment.asStream()!.fileURL)

let localBackupURL = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true)
.appendingPathComponent(UUID().uuidString, isDirectory: true)
defer {
try? FileManager.default.removeItem(at: localBackupURL)
}

let currentBackupDirectoryName = LocalFileBackupManager.FileStructure.backupDirectory(date: Date())
try FileManager.default.createDirectory(
at: localBackupURL.appendingPathComponent(currentBackupDirectoryName),
withIntermediateDirectories: true,
)

try await localFileBackupManager.writeQueuedAttachmentsToDisk(
backupsRootDirectory: localBackupURL,
currentBackupDirectoryName: currentBackupDirectoryName,
progress: nil,
)

let presentLocalKey = try db.read { tx in
try BackupLocalFileAttachmentMetadataRecord
.filter(key: presentId)
.fetchOne(tx.database)!.localKey
}
let presentMediaName = LocalFileBackupAttachmentNaming.mediaName(
localKey: presentLocalKey,
plaintextHash: presentAttachment.plaintextHash!,
)
let presentMediaPath = localBackupURL
.appendingPathComponent("files")
.appendingPathComponent(LocalFileBackupAttachmentNaming.subdirectoryName(mediaName: presentMediaName))
.appendingPathComponent(presentMediaName)

#expect(FileManager.default.fileExists(atPath: presentMediaPath.path))
#expect(db.read { tx in LocalFileBackupStore().exportRecords(batchSize: 10, tx: tx) }.isEmpty)
}

@Test
func testWriteQueuedAttachmentsAcrossMultipleBatches() async throws {
// Larger than batch count so it spans multiple manifestStream writes.
Expand Down