Conversation
A backup record whose file is no longer in the Backups folder could never be removed. The host refuses a deletion path it cannot find, since assertManagedDeletionPath asserts it with allowMissing false, and the domain read that single false as "the file is still there". The record then kept its slot under the Installation's limit. Once the records reached that limit the prune had to remove the oldest one, failed on the file that was not there, and refused the whole backup with a sentence about making room for the new one. Raising the limit above the record count skipped the prune entirely, which is why it looked like a limit that was too low. A file that is not on disk counts as deleted now, so the record comes off with it and stops taking a slot. Two other paths go through the same function and were wrong in the same way: the delete button on the Manage Backups page left the row there for good, and deleting an Installation with its data reported the missing archive as one it had to leave behind. The refusal that is left over is a file that really is there and really will not go, so it now says what to check, in en-US and fr-FR. Closes #507.
Translation statusen-US is the source and carries 874 keys.
The status page is out of date. Refresh it with Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
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.
What the player sees
An Installation with a Backups max amount of 6 refuses every backup:
Raising the limit to 7 makes the same backup go through. The player had four archives in their Backups folder, so the sentence reads as a limit that is one too low.
Why
A backup record whose archive is no longer on disk can never be deleted, and it keeps its slot under the limit.
DELETE_PATHruns the path throughassertManagedDeletionPath, which asserts it withallowMissing: false(src/ipc/pathPolicy.ts:196), so a file that is not there throws and the handler answersfalse(src/ipc/handlers/pathsHandlers.ts:278-285). The domain read that singlefalseas "the file is still there" (src/domain/installations/backupDeletion.ts:29before this change).The prune starts as soon as the record count reaches the limit (
src/domain/installations/backup.ts:110) and gives the whole backup up at the first refusal (src/domain/installations/backup.ts:124), which the renderer turns into the sentence above (src/renderer/src/features/installations/adapters/backupFailure.ts:64). Raising the limit above the record count skips the prune entirely, which is the part that made it look like a limit problem.Reproduction: six records with a limit of 6, the two oldest archives deleted from the Backups folder by hand, four archives left on disk. The backup is refused at 6 and made at 7.
Two other callers of the same function were wrong in the same way, so the fix sits in the function they share: the delete button on the Manage Backups page left the row there for good, and deleting an Installation with its data reported the missing archive as one it had to leave behind.
The refusal that is left over is a file that really is there and really will not go, so
pruneFailednow says what to check, in en-US and fr-FR.Testing
tests/domain/installations/backup.test.ts: the reported shape, six records at a limit of 6 with the two oldest archives gone, backup made and the stale record reported as deleted. Red before the change withprune-failed.tests/domain/installations/backupDeletion.test.ts: an archive that is no longer on disk counts as deleted. The existing case, a file that is there and will not go, still answersfile-delete-failed.tests/renderer-dom/useMakeInstallationBackup.test.tsx: the same situation end to end through the hook, asserting the record leaves the Installation and the player reads no refusal.installationsRestoreBackup.test.tsxnow says the archive is on disk in the case that pins a real delete failure, otherwise the refusal it tests is no longer a refusal.backupDeletion.tsalone turns all three new tests red, restoring it turns them green.npm run typecheck,npm run lint:ci(0 errors, 14 pre-existing warnings),npm run format:check,npm run test:coverage(242 files, 4465 passed).Closes #507. Reported on Discord by NekoJess.