From 4baad15c93e0cf5f46b0c32ddb41ebc0c08e6f5b Mon Sep 17 00:00:00 2001 From: Zaldaryon <273555259+Zaldaryon@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:45:24 -0300 Subject: [PATCH] fix(backups): skip archives already being deleted in prune and delete-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 --- src/domain/installations/backup.ts | 10 ++++- src/domain/installations/delete.ts | 9 +++- .../features/installations/adapters/backup.ts | 2 +- .../features/installations/adapters/delete.ts | 2 +- tests/domain/installations/backup.test.ts | 43 ++++++++++++++++++- tests/domain/installations/delete.test.ts | 32 ++++++++++++++ 6 files changed, 91 insertions(+), 7 deletions(-) diff --git a/src/domain/installations/backup.ts b/src/domain/installations/backup.ts index b44baf48..39f71a11 100644 --- a/src/domain/installations/backup.ts +++ b/src/domain/installations/backup.ts @@ -29,7 +29,7 @@ export interface InstallationSnapshot { path: string backupsLimit: number compressionLevel: number - backups: readonly BackupRecord[] + backups: readonly (BackupRecord & { isDeleting?: boolean })[] isBackingUp: boolean isPlaying: boolean isRestoringBackup: boolean @@ -117,12 +117,18 @@ export async function makeInstallationBackup(ports: MakeInstallationBackupPorts, while (remaining > 0 && remaining >= installation.backupsLimit) { const oldest = installation.backups[remaining - 1] if (!oldest) break + remaining-- + + // Already on its way out through another operation (a manual delete in + // flight). Removing it here too would race the same file; counting it + // toward `remaining` without touching it is correct either way, since + // it will not be there once that other operation finishes. + if (oldest.isDeleting) continue if (!(await ports.fileSystem.remove(oldest.path))) return refuse("prune-failed", deletedBackupIds) deletedBackupIds.push(oldest.id) events.onBackupDeleted?.(oldest) - remaining-- } const date = ports.clock.now() diff --git a/src/domain/installations/delete.ts b/src/domain/installations/delete.ts index 45fb41d5..f60cce3c 100644 --- a/src/domain/installations/delete.ts +++ b/src/domain/installations/delete.ts @@ -4,7 +4,7 @@ import type { BackupRecord } from "./backup" /** The installation state a delete decision needs, copied out of wherever it lives. */ export interface InstallationDeleteSnapshot { path: string - backups: readonly Pick[] + backups: readonly (Pick & { isDeleting?: boolean })[] isPlaying: boolean isBackingUp: boolean isRestoringBackup: boolean @@ -80,6 +80,13 @@ export async function deleteInstallation(ports: DeleteInstallationPorts, input: const failedBackupPaths: string[] = [] for (const backup of installation.backups) { + // Already on its way out through another operation (a manual delete in + // flight). Neither this operation's success nor its failure, so it is + // reported as neither: removing it here would race the same file, and + // counting it as a failure would be misleading when the other operation + // is the one that actually succeeds. + if (backup.isDeleting) continue + if (await ports.fileSystem.remove(backup.path)) { events.onBackupDeleted?.(backup.path) } else { diff --git a/src/renderer/src/features/installations/adapters/backup.ts b/src/renderer/src/features/installations/adapters/backup.ts index 8e9b7e46..61278edd 100644 --- a/src/renderer/src/features/installations/adapters/backup.ts +++ b/src/renderer/src/features/installations/adapters/backup.ts @@ -58,7 +58,7 @@ export function toInstallationSnapshot(installation: InstallationType): Installa path: installation.path, backupsLimit: installation.backupsLimit, compressionLevel: installation.compressionLevel, - backups: installation.backups, + backups: installation.backups.map((backup) => ({ id: backup.id, date: backup.date, path: backup.path, isDeleting: backup._deleting ?? false })), isBackingUp: installation._backuping ?? false, isPlaying: installation._playing ?? false, isRestoringBackup: installation._restoringBackup ?? false diff --git a/src/renderer/src/features/installations/adapters/delete.ts b/src/renderer/src/features/installations/adapters/delete.ts index 7e463dfa..f0ba8954 100644 --- a/src/renderer/src/features/installations/adapters/delete.ts +++ b/src/renderer/src/features/installations/adapters/delete.ts @@ -10,7 +10,7 @@ export function createDeleteInstallationPorts(): DeleteInstallationPorts { export function toInstallationDeleteSnapshot(installation: InstallationType): InstallationDeleteSnapshot { return { path: installation.path, - backups: installation.backups.map((backup) => ({ path: backup.path })), + backups: installation.backups.map((backup) => ({ path: backup.path, isDeleting: backup._deleting ?? false })), isPlaying: installation._playing ?? false, isBackingUp: installation._backuping ?? false, isRestoringBackup: installation._restoringBackup ?? false diff --git a/tests/domain/installations/backup.test.ts b/tests/domain/installations/backup.test.ts index f2ac2924..6054b68a 100644 --- a/tests/domain/installations/backup.test.ts +++ b/tests/domain/installations/backup.test.ts @@ -65,8 +65,8 @@ function fakePorts(overrides: Partial = {}): MakeIn } } -function backup(id: string): BackupRecord { - return { id, date: 1, path: `/backups/${id}.zip` } +function backup(id: string, overrides: { isDeleting?: boolean } = {}): BackupRecord & { isDeleting?: boolean } { + return { id, date: 1, path: `/backups/${id}.zip`, ...overrides } } function snapshot(overrides: Partial = {}): InstallationSnapshot { @@ -193,6 +193,45 @@ describe("makeInstallationBackup pruning", () => { assert.equal(trace.at(-2), "guard-release") assert.equal(trace.at(-1), "finished") }) + + it("skips the oldest backup when it is already being deleted elsewhere, without touching its file", async () => { + const installation = snapshot({ backupsLimit: 2, backups: [backup("b1"), backup("b2"), backup("b3", { isDeleting: true })] }) + + const result = await makeInstallationBackup(fakePorts(), { installation, backupsFolder: "/backups" }, recordingEvents()) + + assert.equal(result.ok, true) + assert.deepEqual(result.deletedBackupIds, ["b2"]) + assert.deepEqual( + trace.filter((entry) => entry.startsWith("remove:") || entry.startsWith("deleted:")), + ["remove:/backups/b2.zip", "deleted:b2"] + ) + }) + + it("counts a skipped in-flight deletion toward the limit instead of also removing a newer backup", async () => { + const installation = snapshot({ backupsLimit: 2, backups: [backup("b1"), backup("b2", { isDeleting: true })] }) + + const result = await makeInstallationBackup(fakePorts(), { installation, backupsFolder: "/backups" }, recordingEvents()) + + assert.equal(result.ok, true) + assert.deepEqual(result.deletedBackupIds, []) + assert.equal( + trace.some((entry) => entry.startsWith("remove:")), + false + ) + }) + + it("keeps pruning past a skipped in-flight deletion to reach the limit", async () => { + const installation = snapshot({ backupsLimit: 2, backups: [backup("b1"), backup("b2"), backup("b3", { isDeleting: true }), backup("b4")] }) + + const result = await makeInstallationBackup(fakePorts(), { installation, backupsFolder: "/backups" }, recordingEvents()) + + assert.equal(result.ok, true) + assert.deepEqual(result.deletedBackupIds, ["b4", "b2"]) + assert.deepEqual( + trace.filter((entry) => entry.startsWith("remove:") || entry.startsWith("deleted:")), + ["remove:/backups/b4.zip", "deleted:b4", "remove:/backups/b2.zip", "deleted:b2"] + ) + }) }) describe("makeInstallationBackup archiving", () => { diff --git a/tests/domain/installations/delete.test.ts b/tests/domain/installations/delete.test.ts index 42d0cc99..6154a620 100644 --- a/tests/domain/installations/delete.test.ts +++ b/tests/domain/installations/delete.test.ts @@ -151,4 +151,36 @@ describe("deleteInstallation with data deletion", () => { assert.deepEqual(result, { ok: true, failedBackupPaths: [] }) }) + + it("skips a backup already being deleted elsewhere, without touching its file or reporting it as failed", async () => { + const installation = snapshot({ + backups: [{ path: "/backups/b1.zip" }, { path: "/backups/b2.zip", isDeleting: true }, { path: "/backups/b3.zip" }] + }) + + const result = await deleteInstallation(fakePorts(), { installation, deleteData: true }, recordingEvents()) + + assert.deepEqual(result, { ok: true, failedBackupPaths: [] }) + assert.deepEqual(trace, [ + "remove:/installations/my-install", + "data-deleted", + "remove:/backups/b1.zip", + "backup-deleted:/backups/b1.zip", + "remove:/backups/b3.zip", + "backup-deleted:/backups/b3.zip" + ]) + }) + + it("treats every backup as in flight the same way, reporting a clean success with none removed here", async () => { + const installation = snapshot({ + backups: [ + { path: "/backups/b1.zip", isDeleting: true }, + { path: "/backups/b2.zip", isDeleting: true } + ] + }) + + const result = await deleteInstallation(fakePorts(), { installation, deleteData: true }, recordingEvents()) + + assert.deepEqual(result, { ok: true, failedBackupPaths: [] }) + assert.deepEqual(trace, ["remove:/installations/my-install", "data-deleted"]) + }) })