From dd9083ce7a3f0b89fff092feddb2f97d22c9d29d Mon Sep 17 00:00:00 2001 From: Nico <47644445+nicotsx@users.noreply.github.com> Date: Wed, 25 Feb 2026 19:02:44 +0100 Subject: [PATCH] fix: wrong cache key for snapshot retention tags (#574) ## Summary by CodeRabbit * **Bug Fixes** * Enhanced error handling in retention category calculations to gracefully handle missing or invalid repositories, ensuring the system returns safe defaults instead of failing * Optimized repository cache validation and lookup mechanisms for improved performance * Strengthened repository validation checks to prevent potential calculation failures --- .../__tests__/repositories.service.test.ts | 68 +++++++++++++++++++ .../repositories/repositories.service.ts | 26 +++---- 2 files changed, 81 insertions(+), 13 deletions(-) diff --git a/app/server/modules/repositories/__tests__/repositories.service.test.ts b/app/server/modules/repositories/__tests__/repositories.service.test.ts index 456414b3..730372e7 100644 --- a/app/server/modules/repositories/__tests__/repositories.service.test.ts +++ b/app/server/modules/repositories/__tests__/repositories.service.test.ts @@ -10,6 +10,8 @@ import { repositoriesTable } from "~/server/db/schema"; import { generateShortId } from "~/server/utils/id"; import { restic } from "~/server/utils/restic"; import { createTestSession } from "~/test/helpers/auth"; +import { createTestBackupSchedule } from "~/test/helpers/backup"; +import { cache, cacheKeys } from "~/server/utils/cache"; import { repositoriesService } from "../repositories.service"; describe("repositoriesService.createRepository", () => { @@ -303,3 +305,69 @@ describe("repositoriesService.dumpSnapshot", () => { }); }); }); + +describe("repositoriesService.getRetentionCategories", () => { + afterEach(() => { + mock.restore(); + }); + + test("recomputes retention categories after repository cache invalidation", async () => { + const { organizationId, user } = await createTestSession(); + const schedule = await createTestBackupSchedule({ organizationId, retentionPolicy: { keepLast: 1 } }); + + const repository = await db.query.repositoriesTable.findFirst({ where: { id: schedule.repositoryId } }); + + expect(repository).toBeTruthy(); + if (!repository) { + throw new Error("Repository should exist"); + } + + const oldSnapshotId = "snapshot-old"; + const newSnapshotId = "snapshot-new"; + const buildForgetResponse = (snapshotId: string) => ({ + success: true, + data: [ + { + tags: [schedule.shortId], + host: "host", + paths: ["/data"], + keep: [], + remove: null, + reasons: [ + { + snapshot: { + id: snapshotId, + short_id: snapshotId, + time: new Date().toISOString(), + tree: "tree", + paths: ["/data"], + hostname: "host", + }, + matches: ["last snapshot"], + }, + ], + }, + ], + }); + + const forgetSpy = spyOn(restic, "forget"); + forgetSpy.mockResolvedValueOnce(buildForgetResponse(oldSnapshotId)); + forgetSpy.mockResolvedValueOnce(buildForgetResponse(newSnapshotId)); + + const firstCategories = await withContext({ organizationId, userId: user.id }, () => + repositoriesService.getRetentionCategories(repository.shortId, schedule.shortId), + ); + + expect(firstCategories.get(oldSnapshotId)).toEqual(["last"]); + + cache.delByPrefix(cacheKeys.repository.all(repository.id)); + + const secondCategories = await withContext({ organizationId, userId: user.id }, () => + repositoriesService.getRetentionCategories(repository.shortId, schedule.shortId), + ); + + expect(secondCategories.get(newSnapshotId)).toEqual(["last"]); + expect(secondCategories.has(oldSnapshotId)).toBe(false); + expect(forgetSpy).toHaveBeenCalledTimes(2); + }); +}); diff --git a/app/server/modules/repositories/repositories.service.ts b/app/server/modules/repositories/repositories.service.ts index fc5a2a2a..9d0e7642 100644 --- a/app/server/modules/repositories/repositories.service.ts +++ b/app/server/modules/repositories/repositories.service.ts @@ -809,25 +809,25 @@ const getRetentionCategories = async (repositoryId: ShortId, scheduleId?: ShortI return new Map(); } - const cacheKey = cacheKeys.repository.retention(repositoryId, scheduleId); - const cached = cache.get>(cacheKey); - - if (cached && Object.keys(cached).length > 0) { - return new Map(Object.entries(cached)); - } - try { - const [schedule, repositoryResult] = await Promise.all([ - backupsService.getScheduleByShortId(scheduleId), - repositoriesService.getRepository(repositoryId), - ]); + const repository = await findRepository(repositoryId); + if (!repository) { + return new Map(); + } + + const cacheKey = cacheKeys.repository.retention(repository.id, scheduleId); + const cached = cache.get>(cacheKey); + + if (cached) { + return new Map(Object.entries(cached)); + } + + const schedule = await backupsService.getScheduleByShortId(scheduleId); if (!schedule?.retentionPolicy) { return new Map(); } - const { repository } = repositoryResult; - const dryRunResults = await restic.forget(repository.config, schedule.retentionPolicy, { tag: scheduleId, organizationId: getOrganizationId(),