fix(backups): skip archives already being deleted in prune and delete-installation - #158
Open
Zaldaryon wants to merge 1 commit into
Open
fix(backups): skip archives already being deleted in prune and delete-installation#158Zaldaryon wants to merge 1 commit into
Zaldaryon wants to merge 1 commit into
Conversation
…-installation PR #136 wired the per-backup _deleting flag into the manual-delete button's own guard, but two other paths that remove backup archives never learned about it: the auto-prune loop in makeInstallationBackup (backupsLimit enforcement, runs before every new backup) and the backup cleanup inside deleteInstallation. Starting a manual delete on an archive, then triggering either of those before it finishes, raced the same file: the prune loop could report prune-failed for a file another operation had already removed correctly, and deleteInstallation could land a bogus entry in failedBackupPaths for the same reason. Both InstallationSnapshot.backups and InstallationDeleteSnapshot.backups now carry an optional isDeleting flag, filled in by the adapters from the config-owned _deleting field. The prune loop skips a candidate that is isDeleting without touching its file, still counting it toward the target since it is on its way out through the other operation either way. deleteInstallation does the same: skips it, reports it as neither removed nor failed, since its actual fate belongs to whichever operation is handling it. Fixes #142
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
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.
PR #136 wired the per-backup
_deletingflag into the manual-delete button's own guard, but two other paths that remove backup archives never learned about it. The auto-prune loop inmakeInstallationBackup(backupsLimitenforcement, runs before every new backup) removes the oldest archive directly and structurally could not check the flag, sinceInstallationSnapshot.backupscarried onlyid,dateandpath.deleteInstallation's backup cleanup had the same gap:InstallationDeleteSnapshot.backupswas narrowed to justpath.Starting a manual delete on an archive, then triggering either of those before it finishes, races the same file: the prune loop can report
prune-failedfor a file another operation already removed correctly, anddeleteInstallationcan land a bogus entry infailedBackupPathsfor the same reason, producing the misleading "some backups survived" warning even when nothing actually survived.Both
InstallationSnapshot.backupsandInstallationDeleteSnapshot.backupsnow carry an optionalisDeletingflag, filled in by the adapters from the config-owned_deletingfield. The prune loop skips a candidate that isisDeletingwithout touching its file, still counting it toward the target since it is on its way out through the other operation either way.deleteInstallationdoes the same: skips it, reports it as neither removed nor failed, since its actual fate belongs to whichever operation is handling it.Six tests added: three for the prune loop (skips the oldest when it is in flight, counts a skip toward the limit instead of reaching for a newer backup, keeps pruning past a skip to reach the limit) and two for
deleteInstallation(skips one in-flight backup among others, treats every backup being in flight as a clean success with none removed here), plus the existing suites verified unaffected.Checks:
npm run typechecknpm run lint:cinpm run format:checknpm run test:coverage(98 files, 1140 passed, 2 skipped; 90.31% statements, 87.47% branches, both domain files this PR touches at 100% and 97.82% statement coverage)npm run build:unpackFixes #142