fix(db): resolve backup retention from persisted setting on health-check path (#13773)

* fix(db): resolve backup retention from persisted setting on health-check path

#13404 fixed the missing prune call after health-check-repair backups but
only resolved maxFiles/retentionDays from env vars, so the persisted
Storage-page setting (honored for manual/API/auto backups via
getDbBackupMaxFiles/getDbBackupRetentionDays) was silently ignored on this
path. Extract that env->persisted->default precedence into
resolveDbBackupRetention() in backupRetention.ts and share it between
backup.ts and core.ts's createManagedDbBackup().

* docs: add changelog fragment for #13308 persisted-setting follow-up

* fix(db): re-point backup retention fix at managedBackup.ts's prune call

The base drifted since this branch was opened: the health-check-repair backup
path (createManagedDbBackup) moved from core.ts into managedBackup.ts
(writeManagedDbBackup), taking its env-only maxFiles/retentionDays resolution
along with it. This branch's resolveDbBackupRetention() extraction and
backup.ts delegation were already correct and unaffected; only the wiring
that used to live in core.ts needed to move to managedBackup.ts's prune call
so the persisted Storage-page setting is honored on this path too (#13308).

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

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))

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() {

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<SqliteAdapter, "prepare">,
+  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<SqliteAdapter, "prepare">,
+  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

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.
     }

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

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);
+});