diff --git a/server/services/backupScheduler.js b/server/services/backupScheduler.js index cb7e3872cc..c54b9ae2e1 100644 --- a/server/services/backupScheduler.js +++ b/server/services/backupScheduler.js @@ -16,10 +16,9 @@ import { resolveBackupConfig } from '../lib/backupConfig.js'; const EVENT_ID = 'backup-daily'; -// Registration state, so an unrelated settings save is a cheap no-op and a -// cancel only fires when a cron is actually registered. -let registered = false; -let lastSignature = null; +// Only confirmed disabled or runnable configurations are cached. +// null means stopped or failed, so identical inputs can retry. +let reconciliationState = null; /** * The registration-affecting slice of settings: `null` when backup scheduling @@ -47,31 +46,32 @@ export async function syncBackupSchedule(settings) { // Only `cron` + `timezone` + active/inactive are baked into the registration; // destPath and the exclude lists are re-read by the handler on every run. const signature = JSON.stringify({ active: Boolean(inputs), cron: inputs?.cron ?? null, tz: timezone }); - if (signature === lastSignature) return registered; + if (signature === reconciliationState?.signature) return reconciliationState.kind === 'scheduled'; if (!inputs) { - lastSignature = signature; - if (registered) { + if (reconciliationState?.kind === 'scheduled') { cancel(EVENT_ID); - registered = false; console.log('💾 Backup scheduler: disabled or destPath cleared — cron cancelled'); } else { console.log('💾 Backup scheduler: disabled or no destPath configured — nothing scheduled'); } + reconciliationState = { kind: 'disabled', signature }; return false; } + reconciliationState = attemptRegistration(inputs, timezone, signature); + return reconciliationState?.kind === 'scheduled'; +} + +function attemptRegistration(inputs, timezone, signature) { // `schedule()` replaces an event with the same id, so a changed cron // expression cleanly re-registers. destPath, excludePaths and // disabledDefaultExcludes are re-read inside the handler so toggles saved in // the Settings UI take effect on the next scheduled run. - // - // try/catch (allowed here — this runs on the settings event bus / at boot, - // outside the request lifecycle): schedule() CANCELS the existing event - // before it validates the new cron, so a malformed expression tears down a - // working timer and throws. Leave `lastSignature` unset in that case so the - // next save — even one that re-submits the same value — retries instead of - // short-circuiting on a registration that never happened. + // schedule() cancels the old event before validating its replacement. + // A throw or missing next run leaves no confirmed state to cache. + // This catch owns failures at boot / on the settings event bus, outside + // the request lifecycle where errors would otherwise propagate to a caller. let event; try { event = schedule({ @@ -98,10 +98,8 @@ export async function syncBackupSchedule(settings) { metadata: { source: 'backupScheduler' } }); } catch (err) { - registered = false; - lastSignature = null; console.error(`❌ Backup scheduler: cron "${inputs.cron}" rejected — no backup scheduled: ${err.message}`); - return false; + return null; } // Not every bad expression throws: a five-field cron with an out-of-range @@ -110,16 +108,11 @@ export async function syncBackupSchedule(settings) { // retry once the user corrects it. if (!event?.nextRunAt) { cancel(EVENT_ID); - registered = false; - lastSignature = null; console.error(`❌ Backup scheduler: cron "${inputs.cron}" has no next run time — no backup scheduled`); - return false; + return null; } - - registered = true; - lastSignature = signature; console.log(`💾 Backup scheduler: registered daily backup at cron "${inputs.cron}"`); - return true; + return { kind: 'scheduled', signature }; } // Re-sync on every settings save rather than from the settings route — keeps @@ -144,7 +137,6 @@ export async function startBackupScheduler() { */ export function stopBackupScheduler() { cancel(EVENT_ID); - registered = false; - lastSignature = null; + reconciliationState = null; console.log('💾 Backup scheduler: stopped'); } diff --git a/server/services/backupScheduler.test.js b/server/services/backupScheduler.test.js index 4250c6576d..58df30eac0 100644 --- a/server/services/backupScheduler.test.js +++ b/server/services/backupScheduler.test.js @@ -50,7 +50,7 @@ vi.mock('./userTimezone.js', () => ({ import { schedule, cancel } from './eventScheduler.js'; import { getSettings } from './settings.js'; import { runBackup } from './backup.js'; -import { startBackupScheduler, stopBackupScheduler } from './backupScheduler.js'; +import { startBackupScheduler, stopBackupScheduler, syncBackupSchedule } from './backupScheduler.js'; describe('startBackupScheduler', () => { beforeEach(() => { @@ -294,3 +294,37 @@ describe('backup schedule defaults (#6632)', () => { }); }); }); + +describe('confirmed backup schedule lifecycle', () => { + beforeEach(() => { + stopBackupScheduler(); + vi.clearAllMocks(); + }); + + it('applies disable once and retries the same schedule after stopping', async () => { + const settings = { backup: { enabled: true, destPath: '/dest' } }; + getSettings.mockResolvedValue(settings); + expect(await startBackupScheduler()).toBe(true); + expect(await syncBackupSchedule({ backup: { enabled: false } })).toBe(false); + expect(await syncBackupSchedule({ backup: { enabled: false } })).toBe(false); + expect(cancel).toHaveBeenCalledTimes(1); + expect(await startBackupScheduler()).toBe(true); + stopBackupScheduler(); + expect(await startBackupScheduler()).toBe(true); + expect(schedule).toHaveBeenCalledTimes(3); + }); + + it.each(['throw', 'no next run'])('forgets a successful signature after replacement fails with %s', async (failure) => { + const original = { backup: { enabled: true, destPath: '/dest', cronExpression: '0 1 * * *' } }; + expect(await syncBackupSchedule(original)).toBe(true); + schedule.mockImplementationOnce(() => { + if (failure === 'throw') throw new Error('Rejected replacement'); + return { id: 'backup-daily', nextRunAt: null }; + }); + expect(await syncBackupSchedule({ + backup: { ...original.backup, cronExpression: '0 2 * * *' } + })).toBe(false); + expect(await syncBackupSchedule(original)).toBe(true); + expect(schedule).toHaveBeenCalledTimes(3); + }); +});