Repository navigation
fix(db): prune pre-migration backups so db_backups stops growing unbounded - #10423
Merged
Merged
Conversation
…unded createPreMigrationBackup() wrote a VACUUM INTO snapshot on every migration run and never pruned. On a long-lived instance db_backups/ reached 48.999 files / 204 GB against a 5,3 MB live database; a second devbox showed the same shape (5.711 files / 24 GB). The retention policy already existed in cleanupDbBackups() but nothing on the migration path reached it — its only callers are backup.ts and the /api/db-backups route, neither of which runs during a migration. migrationRunner.ts cannot import backup.ts: core.ts imports migrationRunner.ts and backup.ts imports core.ts, so that edge would close a cycle. The policy therefore moves to a new core-free module, backupRetention.ts, which both call sites share — cleanupDbBackups() now delegates to it rather than duplicating it. At the migration call site the operator's maxFiles/retentionDays are read through the adapter already open for the run; going through getDbInstance() would re-enter database initialization. Pruning never throws, so housekeeping cannot fail a migration. Closes #10421
…dule check:db-rules requires every src/lib/db/ module to be either re-exported by localDb.ts or listed in INTENTIONALLY_INTERNAL. backupRetention.ts is a shared primitive consumed only by db/backup.ts and db/migrationRunner.ts — the same category as the migrationRunner entry — so it belongs in the allowlist rather than in the public re-export surface.
…AL list check-db-rules-classification.test.ts freezes the exact membership of INTENTIONALLY_INTERNAL, so adding the 40th entry has to be reflected there too — the gate script and this test pin the same contract from opposite sides.
tkgo11
pushed a commit
to tkgo11/OmniRoute
that referenced
this pull request
Sep 23, 2026
…unded (diegosouzapw#10423) * fix(db): prune pre-migration backups so db_backups stops growing unbounded createPreMigrationBackup() wrote a VACUUM INTO snapshot on every migration run and never pruned. On a long-lived instance db_backups/ reached 48.999 files / 204 GB against a 5,3 MB live database; a second devbox showed the same shape (5.711 files / 24 GB). The retention policy already existed in cleanupDbBackups() but nothing on the migration path reached it — its only callers are backup.ts and the /api/db-backups route, neither of which runs during a migration. migrationRunner.ts cannot import backup.ts: core.ts imports migrationRunner.ts and backup.ts imports core.ts, so that edge would close a cycle. The policy therefore moves to a new core-free module, backupRetention.ts, which both call sites share — cleanupDbBackups() now delegates to it rather than duplicating it. At the migration call site the operator's maxFiles/retentionDays are read through the adapter already open for the run; going through getDbInstance() would re-enter database initialization. Pruning never throws, so housekeeping cannot fail a migration. Closes diegosouzapw#10421 * chore(db): declare backupRetention as an intentionally-internal db module check:db-rules requires every src/lib/db/ module to be either re-exported by localDb.ts or listed in INTENTIONALLY_INTERNAL. backupRetention.ts is a shared primitive consumed only by db/backup.ts and db/migrationRunner.ts — the same category as the migrationRunner entry — so it belongs in the allowlist rather than in the public re-export surface. * test(db): include backupRetention in the audited INTENTIONALLY_INTERNAL list check-db-rules-classification.test.ts freezes the exact membership of INTENTIONALLY_INTERNAL, so adding the 40th entry has to be reflected there too — the gate script and this test pin the same contract from opposite sides. --------- Co-authored-by: Xiangzhe <bakryun0718@proton.me>
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…unded (diegosouzapw#10423) * fix(db): prune pre-migration backups so db_backups stops growing unbounded createPreMigrationBackup() wrote a VACUUM INTO snapshot on every migration run and never pruned. On a long-lived instance db_backups/ reached 48.999 files / 204 GB against a 5,3 MB live database; a second devbox showed the same shape (5.711 files / 24 GB). The retention policy already existed in cleanupDbBackups() but nothing on the migration path reached it — its only callers are backup.ts and the /api/db-backups route, neither of which runs during a migration. migrationRunner.ts cannot import backup.ts: core.ts imports migrationRunner.ts and backup.ts imports core.ts, so that edge would close a cycle. The policy therefore moves to a new core-free module, backupRetention.ts, which both call sites share — cleanupDbBackups() now delegates to it rather than duplicating it. At the migration call site the operator's maxFiles/retentionDays are read through the adapter already open for the run; going through getDbInstance() would re-enter database initialization. Pruning never throws, so housekeeping cannot fail a migration. Closes diegosouzapw#10421 * chore(db): declare backupRetention as an intentionally-internal db module check:db-rules requires every src/lib/db/ module to be either re-exported by localDb.ts or listed in INTENTIONALLY_INTERNAL. backupRetention.ts is a shared primitive consumed only by db/backup.ts and db/migrationRunner.ts — the same category as the migrationRunner entry — so it belongs in the allowlist rather than in the public re-export surface. * test(db): include backupRetention in the audited INTENTIONALLY_INTERNAL list check-db-rules-classification.test.ts freezes the exact membership of INTENTIONALLY_INTERNAL, so adding the 40th entry has to be reflected there too — the gate script and this test pin the same contract from opposite sides. --------- Co-authored-by: Xiangzhe <bakryun0718@proton.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10421.
Problem
createPreMigrationBackup()writes aVACUUM INTOsnapshot on every migration run and never prunes. Two machines hit the same wall:db_backups/Every process start that opens the DB with a pending migration adds ~4,7 MB, forever. On the first host this took the disk to 97% full.
Root cause
The retention policy already existed and was complete —
cleanupDbBackups()insrc/lib/db/backup.ts, honoring the operator'smaxFiles/retentionDays. Nothing on the migration path ever reached it: its only callers arebackup.tsitself and the/api/db-backupsroute, neither of which runs during a migration. The pre-migration snapshot was the single backup producer with no retention at all — and by far the most frequent one.Fix
migrationRunner.tscannot simply importbackup.ts:core.tsimportsmigrationRunner.tsandbackup.tsimportscore.ts, so that edge would close a dependency cycle (npm run check:cycleswould fail).So the policy moves into a new
core-free module,src/lib/db/backupRetention.ts, shared by both call sites:cleanupDbBackups()now delegates to it instead of carrying its own copy — same behavior, no duplicated policy. It also accepts an optionalbackupDir.migrationRunner.tsprunes right after writing the snapshot.Two details at the migration call site:
backup.tsresolves them viagetDbInstance(); doing that here would re-enter database initialization, since this code runs from inside it. Precedence is unchanged (env override → persisted operator setting → default), and a DB too old to havekey_valuefalls back to the default.Validation (Hard Rule #18 — TDD)
tests/unit/db-pre-migration-backup-retention-10421.test.ts, written before the fix:The second test pins that the snapshot taken by the current run survives — pruning must never discard the backup protecting the migration it was taken for.
Also green locally:
npm run check:cycles— no cycles across 413 files (the constraint that shaped this fix)npm run typecheck:core— cleandb-backup-extended,db-backup-autobackup-setting-5871,db-backups-skills-3500,db-migration-runner,db-core-migrationDocs
docs/reference/ENVIRONMENT.md:DB_BACKUP_MAX_FILES/DB_BACKUP_RETENTION_DAYSnow also listmigrationRunner.tsas a read site and state that they apply to pre-migration snapshots.