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