diff --git a/changelog.d/fixes/13773-db-backup-retention-persisted.md b/changelog.d/fixes/13773-db-backup-retention-persisted.md new file mode 100644 index 0000000000..e3077a6b3d --- /dev/null +++ b/changelog.d/fixes/13773-db-backup-retention-persisted.md @@ -0,0 +1 @@ +- **fix(db):** Health-check-repair backup pruning now resolves `maxFiles`/`retentionDays` from the persisted Storage-page setting (not just env vars), matching manual/API/auto backups. ([#13308](https://github.com/diegosouzapw/OmniRoute/issues/13308)) diff --git a/src/lib/db/backup.ts b/src/lib/db/backup.ts index d170a5b764..b3d9b7e994 100644 --- a/src/lib/db/backup.ts +++ b/src/lib/db/backup.ts @@ -15,11 +15,11 @@ import { } from "./core"; import { resetAllDbModuleState } from "./stateReset"; import { - MAX_DB_BACKUPS, - DEFAULT_DB_BACKUP_RETENTION_DAYS, - parsePositiveInt, - parseNonNegativeInt, + DB_BACKUP_SETTINGS_NAMESPACE, + DB_BACKUP_MAX_FILES_KEY, + DB_BACKUP_RETENTION_DAYS_KEY, pruneBackupDirectory, + resolveDbBackupRetention, } from "./backupRetention"; import { isAutomatedTestProcess } from "@/shared/utils/testProcess"; @@ -31,30 +31,6 @@ let _lastBackupAt = 0; const BACKUP_THROTTLE_MS = 60 * 60 * 1000; // 60 minutes — high-churn pre-write (models.dev pricing) must not copy the whole SQLite file every call (#10351) const TRUE_ENV_VALUES = new Set(["1", "true", "yes", "on"]); -// #3834: the "Keep latest backups" UI value is persisted here so it survives a page -// refresh / the loadStorageHealth() refetch. A dedicated namespace avoids any -// cross-talk with the databaseSettings key_value store (which rewrites all of its own -// keys on every update). It is intentionally separate from the orphan -// `databaseSettings.backup.keepLastNBackups` (default 5) so existing installs keep the -// historical default of 20 until an operator explicitly changes it here. -const DB_BACKUP_SETTINGS_NAMESPACE = "dbBackup"; -const DB_BACKUP_MAX_FILES_KEY = "maxFiles"; -const DB_BACKUP_RETENTION_DAYS_KEY = "retentionDays"; - -function getStoredDbBackupInteger(key: string, options: { min: number }): number | undefined { - try { - const db = getDbInstance(); - const row = db - .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") - .get(DB_BACKUP_SETTINGS_NAMESPACE, key) as { value?: string } | undefined; - if (!row?.value) return undefined; - const parsed = JSON.parse(row.value); - return Number.isInteger(parsed) && parsed >= options.min ? parsed : undefined; - } catch { - return undefined; - } -} - function setStoredDbBackupInteger(key: string, value: number, options: { min: number }): void { if (!Number.isInteger(value) || value < options.min) return; const db = getDbInstance(); @@ -71,11 +47,7 @@ export function setDbBackupMaxFiles(value: number): void { } export function getDbBackupMaxFiles() { - // Precedence: DB_BACKUP_MAX_FILES env override (ops) → persisted UI value → default. - if (process.env.DB_BACKUP_MAX_FILES) { - return parsePositiveInt(process.env.DB_BACKUP_MAX_FILES, MAX_DB_BACKUPS); - } - return getStoredDbBackupInteger(DB_BACKUP_MAX_FILES_KEY, { min: 1 }) ?? MAX_DB_BACKUPS; + return resolveDbBackupRetention(getDbInstance()).maxFiles; } /** Persist the operator-chosen age-based backup retention window. */ @@ -84,17 +56,7 @@ export function setDbBackupRetentionDays(value: number): void { } export function getDbBackupRetentionDays() { - // Precedence: DB_BACKUP_RETENTION_DAYS env override (ops) → persisted UI value → default. - if (process.env.DB_BACKUP_RETENTION_DAYS) { - return parseNonNegativeInt( - process.env.DB_BACKUP_RETENTION_DAYS, - DEFAULT_DB_BACKUP_RETENTION_DAYS - ); - } - return ( - getStoredDbBackupInteger(DB_BACKUP_RETENTION_DAYS_KEY, { min: 0 }) ?? - DEFAULT_DB_BACKUP_RETENTION_DAYS - ); + return resolveDbBackupRetention(getDbInstance()).retentionDays; } function getBackupDir() { diff --git a/src/lib/db/backupRetention.ts b/src/lib/db/backupRetention.ts index 150bfe07d7..2bda886b28 100644 --- a/src/lib/db/backupRetention.ts +++ b/src/lib/db/backupRetention.ts @@ -12,8 +12,16 @@ import fs from "fs"; import path from "path"; +import type { SqliteAdapter } from "./adapters/types"; + export const MAX_DB_BACKUPS = 20; export const DEFAULT_DB_BACKUP_RETENTION_DAYS = 0; +// #3834: the "Keep latest backups" UI value is persisted here so it survives a page +// refresh. A dedicated namespace avoids cross-talk with the databaseSettings key_value +// store (which rewrites all of its own keys on every update). +export const DB_BACKUP_SETTINGS_NAMESPACE = "dbBackup"; +export const DB_BACKUP_MAX_FILES_KEY = "maxFiles"; +export const DB_BACKUP_RETENTION_DAYS_KEY = "retentionDays"; export function parsePositiveInt(value: string | undefined, fallback: number) { if (!value) return fallback; @@ -27,6 +35,44 @@ export function parseNonNegativeInt(value: string | undefined, fallback: number) return Number.isInteger(parsed) && parsed >= 0 ? parsed : fallback; } +function getStoredInteger( + db: Pick, + key: string, + min: number +): number | undefined { + try { + const row = db + .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") + .get(DB_BACKUP_SETTINGS_NAMESPACE, key) as { value?: string } | undefined; + if (!row?.value) return undefined; + const parsed = JSON.parse(row.value); + return Number.isInteger(parsed) && parsed >= min ? parsed : undefined; + } catch { + return undefined; + } +} + +/** + * Resolve the operator's backup retention settings with the same precedence + * `backup.ts` uses for manual/API/auto backups: env override (ops) → persisted + * Storage-page setting → default. Callers that only had the env-only fallback + * (e.g. the health-check-repair path in `core.ts`) silently ignored the + * persisted setting — this is the single source of truth for both (#13308). + */ +export function resolveDbBackupRetention( + db: Pick, + env: NodeJS.ProcessEnv = process.env +): { maxFiles: number; retentionDays: number } { + return { + maxFiles: env.DB_BACKUP_MAX_FILES + ? parsePositiveInt(env.DB_BACKUP_MAX_FILES, MAX_DB_BACKUPS) + : (getStoredInteger(db, DB_BACKUP_MAX_FILES_KEY, 1) ?? MAX_DB_BACKUPS), + retentionDays: env.DB_BACKUP_RETENTION_DAYS + ? parseNonNegativeInt(env.DB_BACKUP_RETENTION_DAYS, DEFAULT_DB_BACKUP_RETENTION_DAYS) + : (getStoredInteger(db, DB_BACKUP_RETENTION_DAYS_KEY, 0) ?? DEFAULT_DB_BACKUP_RETENTION_DAYS), + }; +} + /** * A backup "family" is the primary `.sqlite` file plus its SQLite sidecars * (`-wal` / `-shm` / `-journal`). Retention operates on families so a sidecar is never diff --git a/src/lib/db/managedBackup.ts b/src/lib/db/managedBackup.ts index 8213aa0e10..8a9cc2cf7d 100644 --- a/src/lib/db/managedBackup.ts +++ b/src/lib/db/managedBackup.ts @@ -1,13 +1,7 @@ import fs from "node:fs"; import path from "node:path"; import type { SqliteAdapter } from "./adapters/types"; -import { - MAX_DB_BACKUPS, - DEFAULT_DB_BACKUP_RETENTION_DAYS, - parsePositiveInt, - parseNonNegativeInt, - pruneBackupDirectory, -} from "./backupRetention"; +import { pruneBackupDirectory, resolveDbBackupRetention } from "./backupRetention"; interface SqlJsSnapshotDatabase { constructor: unknown; @@ -79,18 +73,12 @@ export function createManagedDbBackup( // Prune old backups to prevent the directory from growing without bound. // This mirrors the post-backup pruning in backup.ts but avoids a circular - // dependency by importing directly from backupRetention.ts. + // dependency by importing directly from backupRetention.ts. Resolving + // through resolveDbBackupRetention() (not an env-only inline fallback) + // means the persisted Storage-page setting is honored here too, not just + // for manual/API/auto backups (#13308). try { - const maxFiles = process.env.DB_BACKUP_MAX_FILES - ? parsePositiveInt(process.env.DB_BACKUP_MAX_FILES, MAX_DB_BACKUPS) - : MAX_DB_BACKUPS; - const retentionDays = process.env.DB_BACKUP_RETENTION_DAYS - ? parseNonNegativeInt( - process.env.DB_BACKUP_RETENTION_DAYS, - DEFAULT_DB_BACKUP_RETENTION_DAYS - ) - : DEFAULT_DB_BACKUP_RETENTION_DAYS; - pruneBackupDirectory({ backupDir, maxFiles, retentionDays }); + pruneBackupDirectory({ backupDir, ...resolveDbBackupRetention(db) }); } catch { // Retention is best-effort; never let a pruning failure obscure the backup result. } diff --git a/tests/unit/db-backup-managed-retention-persisted-13308.test.ts b/tests/unit/db-backup-managed-retention-persisted-13308.test.ts new file mode 100644 index 0000000000..c4c99d46ad --- /dev/null +++ b/tests/unit/db-backup-managed-retention-persisted-13308.test.ts @@ -0,0 +1,58 @@ +// #13308 follow-up — the health-check-repair backup path lives in managedBackup.ts's +// createManagedDbBackup() (core.ts just delegates to it), and until this fix that prune +// call resolved maxFiles/retentionDays from env vars only, ignoring the persisted +// Storage-page setting that backup.ts honors for manual/API/auto backups via +// resolveDbBackupRetention(). This seeds a backup directory beyond the *persisted* +// maxFiles (well under the hardcoded MAX_DB_BACKUPS default of 20) and asserts the +// actual health-check-repair write path (managedBackup.ts) prunes down to the +// persisted limit, not the env-only default. + +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-managed-backup-retention-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const { setDbBackupMaxFiles } = await import("../../src/lib/db/backup.ts"); +const { createManagedDbBackup } = await import("../../src/lib/db/managedBackup.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +function seedFamilies(dir: string, count: number) { + fs.mkdirSync(dir, { recursive: true }); + for (let i = 0; i < count; i++) { + const ts = new Date(Date.now() - (i + 1) * 1000).toISOString().replace(/[:.]/g, "-"); + fs.writeFileSync(path.join(dir, `db_${ts}_health-check-repair.sqlite`), `fake-${i}`); + } +} + +test("createManagedDbBackup() health-check-repair path honors the persisted maxFiles setting (#13308)", () => { + const db = core.getDbInstance(); + + // Persist a much stricter limit than the MAX_DB_BACKUPS (20) env-only default. + setDbBackupMaxFiles(3); + + const backupDir = path.join(TEST_DATA_DIR, "db_backups_managed"); + seedFamilies(backupDir, 6); + const beforeCount = fs.readdirSync(backupDir).filter((f) => f.endsWith(".sqlite")).length; + assert.equal(beforeCount, 6, "seeded 6 pre-existing families"); + + const created = createManagedDbBackup(db, "health-check-repair", backupDir); + assert.equal(created, true, "backup write should succeed"); + + const afterCount = fs.readdirSync(backupDir).filter((f) => f.endsWith(".sqlite")).length; + // maxFiles=3 caps the whole family set (the 6 seeded + the 1 just created), newest kept. + assert.equal( + afterCount, + 3, + `expected pruning down to the persisted maxFiles=3, got ${afterCount} — ` + + "if this is 7, the health-check-repair path is still using the env-only MAX_DB_BACKUPS=20 default" + ); +}); diff --git a/tests/unit/db-backup-retention-persisted-13308.test.ts b/tests/unit/db-backup-retention-persisted-13308.test.ts new file mode 100644 index 0000000000..e4dfbfbdc2 --- /dev/null +++ b/tests/unit/db-backup-retention-persisted-13308.test.ts @@ -0,0 +1,54 @@ +// #13308 follow-up — #13404 fixed the health-check-repair prune call itself but only +// resolved maxFiles/retentionDays from env vars, ignoring the persisted Storage-page +// setting that backup.ts honors for manual/API/auto backups. resolveDbBackupRetention() +// is the single precedence chain (env override → persisted setting → default) both +// paths now share. + +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-backup-retention-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const { resolveDbBackupRetention } = await import("../../src/lib/db/backupRetention.ts"); +const { setDbBackupMaxFiles, setDbBackupRetentionDays } = + await import("../../src/lib/db/backup.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +test("resolveDbBackupRetention() falls back to defaults with nothing set (#13308)", () => { + const db = core.getDbInstance(); + const result = resolveDbBackupRetention(db, {}); + assert.equal(result.maxFiles, 20); + assert.equal(result.retentionDays, 0); +}); + +test("resolveDbBackupRetention() honors the persisted Storage-page setting (#13308)", () => { + const db = core.getDbInstance(); + setDbBackupMaxFiles(3); + setDbBackupRetentionDays(7); + + const result = resolveDbBackupRetention(db, {}); + assert.equal(result.maxFiles, 3); + assert.equal(result.retentionDays, 7); +}); + +test("resolveDbBackupRetention() lets an env override win over the persisted setting (#13308)", () => { + const db = core.getDbInstance(); + setDbBackupMaxFiles(3); + setDbBackupRetentionDays(7); + + const result = resolveDbBackupRetention(db, { + DB_BACKUP_MAX_FILES: "5", + DB_BACKUP_RETENTION_DAYS: "14", + }); + assert.equal(result.maxFiles, 5); + assert.equal(result.retentionDays, 14); +});