Keep the last good backup until there is a new one - #724
Merged
Conversation
Same ordering the export had: the previous archives were deleted before the new ones were written, so a backup that failed part-way — an unreadable table, a directory it could not archive, a full disk — took the last backup with it and left the installation with none. Delete afterwards instead, keeping anything named with this run's hash, which is what the archives it just wrote are named with whatever extension the archive handler settled on. A successful backup still replaces the previous one. FileSystem gains the variant that does the skipping; deleteByPattern is now that call with nothing kept. The test uses a real directory rather than the harness's virtual one, because the cleanup globs and glob does not see a stream wrapper. It fails against the old ordering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The same ordering the export had (#722), with more at stake:
BackupFile::doBackup()calleddeleteOldBackups()before writing the new archives.So a backup that failed part-way — an unreadable table, a directory it could not archive, a full disk — took the previous database dump and application archive with it, and left the installation with no backup at all. The existing failure test drove exactly that path; it just had nothing on disk to notice was gone.
The fix
Delete afterwards, keeping anything named with this run's hash — which is what the archives it just wrote are named with, whatever extension the archive handler settled on. A successful backup still replaces the previous one, so the directory does not accumulate.
FileSystemgains the variant that does the skipping;deleteByPattern()is now that call with nothing kept.Testing
A test that seeds a previous archive, makes the run fail, and asserts it is still there. It fails against the old ordering.
Like the export's, it uses a real directory: the cleanup globs, and
glob()does not see a stream wrapper, so against the harness's virtual filesystem it would pass while proving nothing.Unit suite green: 3074. Integration backup tests: 18 green.