Skip to content

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

Closed
deadcaf3 wants to merge 1 commit into
signalapp:mainfrom
deadcaf3:worktree-fix-flaky-backups
Closed

deadcaf3 wants to merge 1 commit into
signalapp:mainfrom
deadcaf3:worktree-fix-flaky-backups

Conversation

@deadcaf3

Copy link
Copy Markdown
Contributor

Contributor checklist

Administrative:

Commits and testing:

  • My commits are rebased on the latest main branch.
  • My commits are well-structured for review.
  • My change has been thoroughly tested, and I am not aware of any regressions to existing features or behaviors.
  • I have tested my contribution on these devices:
  • iPhone Duo Simulator, iOS 27.1

Description

This fixes local file backups failing every night because of one unreadable attachment, while the UI still shows a fresh backup.

LocalFileBackupExportJob recorded the last backup details before copying attachments. If one attachment's source file was missing, writeQueuedAttachmentsToDisk threw, and since that attachment's export record was never deleted, every later run failed the same way. Old backups were never pruned, a new backup directory piled up each night, and the settings screen kept reporting a recent backup.

This change:

  • skips, counts and logs attachments whose source file or local key can't be read, matching how the restore path already handles a bad attachment. Errors writing to the backup location still fail the job.
  • records the last backup details only after attachments are copied and old backups are cleaned up.
  • stores the backup date and size in the .postBackupFileCopy resumption point, so a resumed job can record them when it finishes. A job that is mid-resume when this ships has a checkpoint without these keys and starts over once.

Testing: I added LocalFileBackupManagerTests.testCopyAttachmentsToDiskSkipsMissingFile, which deletes one queued attachment's file and checks that the copy doesn't throw, the other attachment is written and the export queue is drained. I also updated LocalFileBackupExportJobRunnerTests.localFileBackup_resumedAfterCopy for the new resumption point fields. To check for regressions, I ran all of LocalFileBackupManagerTests and LocalFileBackupExportJobRunnerTests (15 tests), and they pass. I did not run the full test suite.

A local file backup recorded "last backup" before copying attachments.
If one attachment's file was missing, the copy threw on every run: old
backups were never pruned, a new backup directory piled up each night,
and the UI still reported a fresh backup.

Skip and count attachments whose source can't be read, instead of
aborting. Failures writing to the backup location still abort.

Record the last backup details only after attachments are copied and
old backups cleaned up. The resumption point now carries the backup
date and size so a resumed job can record them when it finishes.
@kate-signal

Copy link
Copy Markdown
Contributor

This merged internally as 778f0e7, which will be available with an upcoming release! Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants