From aaf9969734da3a91c9a982f77edf9675523c938f Mon Sep 17 00:00:00 2001 From: Pixnop <77785313+Pixnop@users.noreply.github.com> Date: Fri, 18 Sep 2026 18:48:01 +0200 Subject: [PATCH] Count an archive that is already gone as deleted 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. --- src/domain/installations/backupDeletion.ts | 16 ++++++- src/domain/installations/delete.ts | 2 +- src/renderer/src/locales/en-US.json | 2 +- src/renderer/src/locales/fr-FR.json | 2 +- tests/domain/installations/backup.test.ts | 29 ++++++++++++- .../installations/backupDeletion.test.ts | 9 +++- tests/domain/installations/delete.test.ts | 5 ++- .../installationsRestoreBackup.test.tsx | 3 ++ tests/renderer-dom/launchPlayGame.test.tsx | 2 +- .../useMakeInstallationBackup.test.tsx | 42 +++++++++++++++++++ 10 files changed, 103 insertions(+), 9 deletions(-) diff --git a/src/domain/installations/backupDeletion.ts b/src/domain/installations/backupDeletion.ts index 83af3338..f038cc9c 100644 --- a/src/domain/installations/backupDeletion.ts +++ b/src/domain/installations/backupDeletion.ts @@ -7,7 +7,7 @@ export type DeleteInstallationBackupFailure = "backup-in-use" | "file-delete-fai export type DeleteInstallationBackupResult = { ok: true } | { ok: false; reason: DeleteInstallationBackupFailure } export interface DeleteInstallationBackupPorts { - fileSystem: Pick + fileSystem: Pick } export interface DeleteInstallationBackupInput { @@ -18,6 +18,15 @@ export interface DeleteInstallationBackupInput { * Deletes one archive off disk. Dropping it from the installation record is the * caller's job, and only once this says the file is gone. * + * A record whose archive is not on disk any more counts as deleted. The host + * refuses to delete a path it cannot find (assertManagedDeletionPath runs with + * `allowMissing: false`, see ipc/pathPolicy.ts), so an archive the player + * removed from the Backups folder by hand used to answer "file-delete-failed" + * for good: the record could never be dropped, the manual delete kept failing + * and, once enough records piled up, the prune in makeInstallationBackup + * refused every new backup (#507). The file is gone either way, which is what + * the caller asked for. + * * @param ports Host capabilities the work runs on. * @param input The archive to delete. * @returns Success, or the reason the file is still there. @@ -26,7 +35,10 @@ export async function deleteInstallationBackup(ports: DeleteInstallationBackupPo const { backup } = input if (backup.isRestoring || backup.isDeleting) return { ok: false, reason: "backup-in-use" } - if (!(await ports.fileSystem.remove(backup.path))) return { ok: false, reason: "file-delete-failed" } + + if (!(await ports.fileSystem.remove(backup.path))) { + if (await ports.fileSystem.exists(backup.path)) return { ok: false, reason: "file-delete-failed" } + } return { ok: true } } diff --git a/src/domain/installations/delete.ts b/src/domain/installations/delete.ts index f048615f..ecdc0cc0 100644 --- a/src/domain/installations/delete.ts +++ b/src/domain/installations/delete.ts @@ -23,7 +23,7 @@ export type DeleteInstallationResult = | { ok: false; reason: DeleteInstallationFailure } export interface DeleteInstallationPorts { - fileSystem: Pick + fileSystem: Pick } export interface DeleteInstallationInput { diff --git a/src/renderer/src/locales/en-US.json b/src/renderer/src/locales/en-US.json index 24788d47..45550784 100644 --- a/src/renderer/src/locales/en-US.json +++ b/src/renderer/src/locales/en-US.json @@ -724,7 +724,7 @@ "compressUnsafeEntry": "No backup made: this Installation's folder contains a symlink or a special file that cannot be archived.", "compressTooManyFiles": "No backup made: this Installation's folder has too many files to archive.", "compressWriteFailed": "No backup made: the backup archive could not be written. Check that the Backups folder is on a writable drive with free space.", - "pruneFailed": "No backup made: an old backup could not be removed to make room for the new one.", + "pruneFailed": "No backup made: an old backup file could not be deleted to make room for the new one. Check that it is not open in another program, then try again.", "errorRestoringBackup": "The Backup could not be restored, so your Installation was left as it was.", "restoreLeftDataAside": "The Backup could not be restored and your old Installation data is now in {{path}}. Move that folder back yourself before playing.", "backupsAmount": "Backups limit", diff --git a/src/renderer/src/locales/fr-FR.json b/src/renderer/src/locales/fr-FR.json index 57888ec8..78888f0f 100644 --- a/src/renderer/src/locales/fr-FR.json +++ b/src/renderer/src/locales/fr-FR.json @@ -724,7 +724,7 @@ "compressUnsafeEntry": "Aucune sauvegarde faite : le dossier de cette installation contient un lien symbolique ou un fichier spécial qui ne peut pas être archivé.", "compressTooManyFiles": "Aucune sauvegarde faite : le dossier de cette installation contient trop de fichiers à archiver.", "compressWriteFailed": "Aucune sauvegarde faite : l'archive de sauvegarde n'a pas pu être écrite. Vérifiez que le dossier des sauvegardes est sur un disque accessible en écriture et qu'il y reste de la place.", - "pruneFailed": "Aucune sauvegarde faite : une ancienne sauvegarde n'a pas pu être retirée pour faire de la place à la nouvelle.", + "pruneFailed": "Aucune sauvegarde faite : un ancien fichier de sauvegarde n'a pas pu être supprimé pour faire de la place à la nouvelle. Vérifiez qu'il n'est pas ouvert dans un autre programme, puis réessayez.", "errorRestoringBackup": "La sauvegarde n'a pas pu être restaurée, votre installation est restée telle quelle.", "restoreLeftDataAside": "La sauvegarde n'a pas pu être restaurée et vos anciennes données d'installation se trouvent maintenant dans {{path}}. Remettez ce dossier en place vous-même avant de jouer.", "backupsAmount": "Nombre maximum", diff --git a/tests/domain/installations/backup.test.ts b/tests/domain/installations/backup.test.ts index 93bb6a43..8d15d4ba 100644 --- a/tests/domain/installations/backup.test.ts +++ b/tests/domain/installations/backup.test.ts @@ -13,15 +13,23 @@ const FIXED_NOW = new Date(2025, 7, 16, 1, 20, 0).getTime() /** Everything the fakes wrote down, in the order it happened. */ let trace: string[] = [] -function fakeFileSystem(options: { exists?: boolean; removals?: Record } = {}): FileSystem { +/** + * `missing` holds archive paths that are no longer on disk: the host refuses to + * delete a path it cannot find (assertManagedDeletionPath in ipc/pathPolicy.ts + * runs with allowMissing false), so those answer false to both calls. + */ +function fakeFileSystem(options: { exists?: boolean; removals?: Record; missing?: readonly string[] } = {}): FileSystem { const removals = options.removals ?? {} + const missing = new Set(options.missing ?? []) return { exists: async (path: string): Promise => { trace.push(`exists:${path}`) + if (missing.has(path)) return false return options.exists ?? true }, remove: async (path: string): Promise => { trace.push(`remove:${path}`) + if (missing.has(path)) return false return removals[path] ?? true }, move: async (from: string, to: string): Promise => { @@ -237,6 +245,25 @@ describe("makeInstallationBackup pruning", () => { ) }) + it("makes the backup when the oldest record's archive is already gone from disk", async () => { + // Reported on Discord: six records, the two oldest deleted from the Backups + // folder by hand, so the player counted four archives. The prune could not + // remove a file that was not there and the whole backup was refused. + const installation = snapshot({ backupsLimit: 6, backups: [backup("b1"), backup("b2"), backup("b3"), backup("b4"), backup("b5"), backup("b6")] }) + const ports = fakePorts({ fileSystem: fakeFileSystem({ missing: ["/backups/b5.tar.gz", "/backups/b6.tar.gz"] }) }) + + const result = await makeInstallationBackup(ports, { installation, backupsFolder: "/backups" }, recordingEvents()) + + assert.equal(result.ok, true) + // The stale record comes off with the archives: reporting it as deleted is + // what drops it from the installation, so it stops taking a slot. + assert.deepEqual(result.deletedBackupIds, ["b6"]) + assert.equal( + trace.some((entry) => entry.startsWith("compress:")), + true + ) + }) + it("skips the oldest backup when it is being restored, without touching its file", async () => { const installation = snapshot({ backupsLimit: 2, backups: [backup("b1"), backup("b2"), backup("b3", { isRestoring: true })] }) diff --git a/tests/domain/installations/backupDeletion.test.ts b/tests/domain/installations/backupDeletion.test.ts index dc39763f..ebf71330 100644 --- a/tests/domain/installations/backupDeletion.test.ts +++ b/tests/domain/installations/backupDeletion.test.ts @@ -8,10 +8,11 @@ function backup(overrides: Partial = {}): BackupSnapshot { return { id: "backup-1", path: "/backups/my-install.zip", isRestoring: false, isDeleting: false, ...overrides } } -function ports(options: { removed?: boolean } = {}): { fileSystem: { remove: (path: string) => Promise }; removals: string[] } { +function ports(options: { removed?: boolean; onDisk?: boolean } = {}): { fileSystem: { exists: (path: string) => Promise; remove: (path: string) => Promise }; removals: string[] } { const removals: string[] = [] return { fileSystem: { + exists: async (): Promise => options.onDisk ?? true, remove: async (path: string): Promise => { removals.push(path) return options.removed ?? true @@ -51,4 +52,10 @@ describe("deleteInstallationBackup", () => { assert.deepEqual(result, { ok: false, reason: "file-delete-failed" }) }) + + it("treats an archive that is no longer on disk as deleted, so its record can go", async () => { + const result = await deleteInstallationBackup(ports({ removed: false, onDisk: false }), { backup: backup() }) + + assert.deepEqual(result, { ok: true }) + }) }) diff --git a/tests/domain/installations/delete.test.ts b/tests/domain/installations/delete.test.ts index b1f1630d..48911870 100644 --- a/tests/domain/installations/delete.test.ts +++ b/tests/domain/installations/delete.test.ts @@ -9,12 +9,15 @@ type BackupFixture = InstallationDeleteSnapshot["backups"][number] /** Everything the fakes wrote down, in the order it happened. */ let trace: string[] = [] -function fakePorts(options: { removals?: Record } = {}): DeleteInstallationPorts { +function fakePorts(options: { removals?: Record; missing?: readonly string[] } = {}): DeleteInstallationPorts { const removals = options.removals ?? {} + const missing = new Set(options.missing ?? []) return { fileSystem: { + exists: async (path: string): Promise => !missing.has(path), remove: async (path: string): Promise => { trace.push(`remove:${path}`) + if (missing.has(path)) return false return removals[path] ?? true } } diff --git a/tests/renderer-dom/installationsRestoreBackup.test.tsx b/tests/renderer-dom/installationsRestoreBackup.test.tsx index 3c10fb30..a645a371 100644 --- a/tests/renderer-dom/installationsRestoreBackup.test.tsx +++ b/tests/renderer-dom/installationsRestoreBackup.test.tsx @@ -170,6 +170,9 @@ describe("ManageInstallationBackups", () => { installMockWindowApi({ configManager: { getConfig: vi.fn(async () => createMockConfig({ installations: [anInstallationWithBackup()] })) }, pathsManager: { + // The archive is still on disk, so the refusal is a real one: a path + // the host cannot find counts as deleted (see backupDeletion.ts). + checkPathExists: vi.fn(async () => true), deletePath, extractOnPath: vi.fn(async () => true) } diff --git a/tests/renderer-dom/launchPlayGame.test.tsx b/tests/renderer-dom/launchPlayGame.test.tsx index 8418fba7..3cba6bd6 100644 --- a/tests/renderer-dom/launchPlayGame.test.tsx +++ b/tests/renderer-dom/launchPlayGame.test.tsx @@ -100,7 +100,7 @@ async function clickPlay(user: ReturnType): Promise { expect(result.current.notifications.history.map((notification) => notification.body)).not.toContain(GENERIC_WRITE_FAILURE) }) }) + +/** + * Issue #507, reported on Discord. An archive removed from the Backups folder + * outside the launcher leaves its record behind, and the host refuses to delete + * a path it cannot find (assertManagedDeletionPath in ipc/pathPolicy.ts). Once + * the records reach the Installation's limit the prune has to remove one, so + * every backup was refused with a sentence about making room, which reads as a + * limit the player has to raise. + */ +describe("useMakeInstallationBackup with an archive missing from the Backups folder", () => { + const PRUNE_FAILED = "No backup made: an old backup file could not be deleted to make room for the new one. Check that it is not open in another program, then try again." + const MISSING_ARCHIVE = "/backups/a/backup-6.tar.gz" + + function anInstallationAtItsLimit(): InstallationType { + return { + ...anInstallation(), + backupsLimit: 6, + // Newest first, the order the config keeps them in. + backups: Array.from({ length: 6 }, (_, index) => ({ id: `backup-${index + 1}`, date: 1_700_000_000_000 - index, path: `/backups/a/backup-${index + 1}.tar.gz` })) + } + } + + it("makes the backup and drops the record whose archive is gone", async () => { + installMockWindowApi({ + configManager: { getConfig: vi.fn(async () => createMockConfig({ backupsFolder: "/backups", installations: [anInstallationAtItsLimit()] })) }, + pathsManager: { + checkPathExists: vi.fn(async (path: string) => path !== MISSING_ARCHIVE), + deletePath: vi.fn(async (path: string) => path !== MISSING_ARCHIVE), + compressOnPath: vi.fn(async () => true) + } + }) + + const { result } = renderHook(() => ({ makeBackup: useMakeInstallationBackup(), installations: useInstallations(), notifications: useNotificationsContext() }), { wrapper }) + await waitFor(() => expect(result.current.installations).toHaveLength(1)) + + const outcome = await result.current.makeBackup("install-a") + + expect(outcome).toEqual({ ok: true }) + expect(result.current.notifications.history.map((notification) => notification.body)).not.toContain(PRUNE_FAILED) + await waitFor(() => expect(result.current.installations[0]?.backups.map((backup) => backup.id)).not.toContain("backup-6")) + }) +})