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")) + }) +})