Skip to content

fix(db): prune old backups after health-check-repair snapshots - #12992

Closed
hartmark wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
hartmark:fix/health-check-backup-retention
Closed

hartmark wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
hartmark:fix/health-check-backup-retention

Conversation

@hartmark

@hartmark hartmark commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What this fixes

createManagedDbBackup() (src/lib/db/core.ts) — the writer behind health-check-repair backups, taken every time the periodic health check runs autoRepair — never called into the shared retention policy at all. backup.ts's manual/auto path already resolves the operator's settings and prunes right after writing; this was the one backup producer with no retention call whatsoever.

Observed live: db_backups/ grew to 570 GB / 60 files against a small live database, all health-check-repair snapshots, none ever pruned.

What changed

  • Extracted the duplicated "read persisted dbBackup setting" logic (previously copy-pasted between backup.ts's getStoredDbBackupInteger and migrationRunner.ts's now-removed readStoredBackupSetting) into backupRetention.ts as readStoredDbBackupSetting() / resolveDbBackupRetentionSettings().
  • Added pruneManagedDbBackups() to backupRetention.ts as the shared resolve+prune+log sequence.
  • core.ts's health-check-repair path now calls pruneManagedDbBackups() right after every successful VACUUM INTO, the same way backup.ts's backupDbFile() already does.

What this deliberately does NOT touch

The pre-migration backup path (migrationRunner/preMigrationBackup.ts). db-pre-migration-backup-retention-10421.test.ts explicitly asserts retention stays outside the concurrent migration window — that's an intentional design decision (content-addressed snapshots, reused for an identical DB state), not a gap. I initially assumed the missing prune call there (also dropped when this file was extracted from migrationRunner.ts) was an unrelated regression and tried to fix it too; running the full existing suite caught that immediately (successful migrations retain existing backups and do not prune inside the migration window failed), so I reverted that part. Only the health-check-repair path — which has no such constraint — is touched here.

Evidence

  • New test: tests/unit/db-backup-retention-shared.test.ts — proves pruneManagedDbBackups caps a seeded 30-file backup directory at the configured maxFiles (Pruned 25 old backup file(s) (5 kept...)), never throws when pruning itself fails, and that resolveDbBackupRetentionSettings's env-override precedence holds.
  • createManagedDbBackup() itself is gated behind isAutomatedTestProcess() (an intentional, pre-existing production-safety check) and cannot be exercised end-to-end from this test runner — which is also exactly why the missing prune call went unnoticed by the existing suite for as long as it did.
  • Full existing backup + health-check suite: 60/60 pass (db-pre-migration-backup-retention-10421, db-backup-extended, db-backup-autobackup-setting-5871, db-backups-skills-3500, cli-backup-command, db-health-check, db-health-driver, plus the new file).
  • eslint clean on all touched files (3 pre-existing unused-var errors in core.ts are unrelated to this change and present on unmodified release/v3.8.51 too).

Note for maintainer

Also relevant to my own deployment: I have DB_BACKUP_MAX_FILES=5 set via env, which this fix now actually enforces for health-check-repair backups too.

createManagedDbBackup() (core.ts) -- the writer behind health-check-repair
backups, taken every time the periodic health check runs autoRepair -- never
called into the shared retention policy at all. backup.ts's manual/auto path
already resolved settings and pruned after writing; this was the one backup
producer with no retention call whatsoever.

Observed live: db_backups/ grew to 570 GB / 60 files against a small live
database, all health-check-repair snapshots, none ever pruned.

Extracted the duplicated "read persisted dbBackup setting" logic (previously
copy-pasted between backup.ts's getStoredDbBackupInteger and
migrationRunner.ts's now-removed readStoredBackupSetting) into
backupRetention.ts as readStoredDbBackupSetting() / resolveDbBackupRetentionSettings(),
and added pruneManagedDbBackups() as the shared resolve+prune+log sequence.
core.ts's health-check-repair path now calls it right after every successful
VACUUM INTO, the same way backup.ts's backupDbFile() already does.

Does NOT touch the pre-migration backup path (migrationRunner/preMigrationBackup.ts):
db-pre-migration-backup-retention-10421.test.ts explicitly asserts retention stays
outside the migration window, by design -- confirmed by running the full existing
suite before finalizing this change.

Added tests/unit/db-backup-retention-shared.test.ts proving pruneManagedDbBackups
actually caps a seeded 30-file backup directory at the configured maxFiles, never
throws when pruning itself fails, and that resolveDbBackupRetentionSettings'
env-override precedence holds. createManagedDbBackup() itself is gated behind
isAutomatedTestProcess() (an intentional, pre-existing production-safety check)
and cannot be exercised end-to-end from this test runner -- which is also exactly
why the missing prune call here went unnoticed by the existing suite.

60/60 backup + health-check tests pass. eslint clean on all touched files.
@hartmark
hartmark marked this pull request as ready for review September 7, 2026 21:26
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 7, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 7, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 8, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 8, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 9, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 11, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 11, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 13, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 13, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 14, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 14, 2026
…check-repair snapshots) into dev/omniroute-dev-combined
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for the thorough writeup and the 570 GB / 60-file evidence — this was a
real, unbounded-growth bug. It's since been fixed and merged separately (#13404,
closing #13308), so I'm recommending we close this rather than rebase a
duplicate. Your pruneManagedDbBackups() (never-throws, shared resolve+prune+log)
and the 148-line test suite are genuinely more thorough than what landed — worth
knowing the merged fix only reads maxFiles/retentionDays from env vars, not
from the persisted Storage-page setting your version also handles. If you'd like
to open a narrower follow-up for just that gap, this branch would be a great
starting point.

Triage note: this is the review recommendation — the close itself happens only after the maintainer's per-PR sign-off (and, where a superseding PR is named, after it has landed). Nothing is being closed by this comment.

@hartmark

Copy link
Copy Markdown
Contributor Author

Confirmed #13404 (closing #13308) merged, so per your own sequencing that's the go — closing this in its favor. Thanks for the flag on the gap: the merged fix reads maxFiles/retentionDays only from env vars, not the persisted Storage-page setting this branch also handled. Might come back and open that as a narrower follow-up off this branch.

@hartmark hartmark closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants