diff --git a/src/lib/server/backups/helpers.ts b/src/lib/server/backups/helpers.ts index 9aed2f7..5f8c240 100644 --- a/src/lib/server/backups/helpers.ts +++ b/src/lib/server/backups/helpers.ts @@ -503,6 +503,25 @@ export function retentionToStore(retention: any, schedule: unknown): string | nu return scheduled ? JSON.stringify(DEFAULT_SCHEDULED_RETENTION) : null; } +/** + * Resolve the retention value to persist on a PARTIAL config update. `undefined` means + * "leave the stored retention untouched" (the caller must skip the field). A minimal PUT + * that only flips `enabled` (pause/resume) omits `retention`, so re-applying the scheduled + * default here would silently wipe the user's policy (#1462). The default is applied ONLY + * when the retention is actually provided, or when the schedule is being added/changed - + * the transition the default exists to cover. + */ +export function resolveRetentionForUpdate( + retention: unknown, + newSchedule: unknown, + existingSchedule: unknown +): string | null | undefined { + const retentionProvided = retention !== undefined; + const scheduleChanged = newSchedule !== undefined && newSchedule !== existingSchedule; + if (!retentionProvided && !scheduleChanged) return undefined; // leave stored value as-is + return retentionToStore(retention, newSchedule ?? existingSchedule); +} + /** * Decide the `enabled` flag when a backup config is updated. A "run once" backup persists a * MANUAL, paused config (schedule=null, enabled=false); adding a cron schedule later should make diff --git a/src/routes/api/backup/configs/[id]/+server.ts b/src/routes/api/backup/configs/[id]/+server.ts index aa6ec3f..2a80ea5 100644 --- a/src/routes/api/backup/configs/[id]/+server.ts +++ b/src/routes/api/backup/configs/[id]/+server.ts @@ -8,7 +8,7 @@ import { } from '$lib/server/db'; import { registerSchedule, unregisterSchedule, isValidCron } from '$lib/server/scheduler'; import { isBackupRunning } from '$lib/server/backups'; -import { validateRetention, retentionToStore, resolveEnabledOnScheduleChange } from '$lib/server/backups/helpers'; +import { validateRetention, resolveRetentionForUpdate, resolveEnabledOnScheduleChange } from '$lib/server/backups/helpers'; import { requireBackups, loadConfigGateEnv } from '$lib/server/backups/route-guards'; /** @@ -99,11 +99,11 @@ export const PUT: RequestHandler = async (event) => { selectedVolumes: body.selectedVolumes ? JSON.stringify(body.selectedVolumes) : body.selectedVolumes, stopBeforeBackup: body.stopBeforeBackup, schedule: body.schedule, - // Apply the default scheduled retention the same way create does, so a - // config edited to add a schedule with no explicit retention doesn't end - // up with pruning disabled and an unbounded-growth repo. Uses the effective - // schedule (the incoming one, or the existing one if unchanged). - retention: retentionToStore(body.retention, body.schedule ?? existing.schedule), + // Leave the stored retention untouched when the client didn't send it and the + // schedule isn't changing - a minimal pause/resume PUT (only `enabled`) must not + // wipe the user's policy (#1462). The scheduled default is still applied when + // retention is provided or a schedule is being added/changed. + retention: resolveRetentionForUpdate(body.retention, body.schedule, existing.schedule), options: body.options ? JSON.stringify(body.options) : body.options, tags: body.tags ? JSON.stringify(body.tags) : body.tags }); diff --git a/tests/backup-helpers.test.ts b/tests/backup-helpers.test.ts index c46a41b..a355d11 100644 --- a/tests/backup-helpers.test.ts +++ b/tests/backup-helpers.test.ts @@ -16,6 +16,7 @@ import { isRepoNotInitializedError, wouldDeleteAllSnapshots, retentionToStore, + resolveRetentionForUpdate, validateFlags, validatePolicySchedules, validateRetention, @@ -691,6 +692,43 @@ describe('retentionToStore', () => { }); }); +// ============================================================================ +// resolveRetentionForUpdate: partial-update guard. A minimal PUT that omits +// retention (e.g. pause/resume, which only sends `enabled`) must NOT re-apply +// the scheduled default and wipe the stored policy (#1462). +// ============================================================================ +describe('resolveRetentionForUpdate', () => { + const SCHED = '0 4 * * *'; + test('retention omitted and schedule unchanged -> undefined (leave stored value as-is)', () => { + // The pause/resume case: body has no retention, schedule not in the body. + expect(resolveRetentionForUpdate(undefined, undefined, SCHED)).toBeUndefined(); + }); + test('retention omitted but the SAME schedule resent -> still leaves it alone', () => { + expect(resolveRetentionForUpdate(undefined, SCHED, SCHED)).toBeUndefined(); + }); + test('an explicit retention is stored verbatim', () => { + expect(resolveRetentionForUpdate({ keepYearly: 3 }, undefined, SCHED)).toBe( + JSON.stringify({ keepYearly: 3 }) + ); + }); + test('adding a schedule with no retention applies the scheduled default', () => { + // none -> cron transition: the default-injection the helper exists for. + expect(resolveRetentionForUpdate(undefined, SCHED, null)).toBe( + JSON.stringify({ keepDaily: 7, keepWeekly: 4, keepMonthly: 6 }) + ); + }); + test('clearing to all-zero on a SCHEDULED config falls back to the default (no unbounded growth)', () => { + // retention is provided (so not skipped), but empty; with a schedule present the + // scheduled default applies rather than storing "no policy". + expect(resolveRetentionForUpdate({ keepLast: 0 }, undefined, SCHED)).toBe( + JSON.stringify({ keepDaily: 7, keepWeekly: 4, keepMonthly: 6 }) + ); + }); + test('clearing to all-zero on an UNSCHEDULED config stores null', () => { + expect(resolveRetentionForUpdate({ keepLast: 0 }, undefined, null)).toBeNull(); + }); +}); + // ============================================================================ // validateFlags: save-time wrapper over sanitizeResticFlags — surfaces the // throw as an error string (or null when the flags are acceptable / absent).