ReviewOS

also looking at this

stacks/ts-cloud

fix(db): bound on-demand database dumps, and publish them atomically

#200
Merged glennmichael123 wants to merge fix/db-backup-retention into main
2 files +188 -25
4 days ago

Closes #199.

The bug

buildBackupScript wrote a timestamped dump and nothing ever pruned DB_BACKUP_DIR — buildListBackupsScript only lists it. So every dashboard "Backup" action and every ts-cloud db:backup left a full dump on the box indefinitely. The scheduled path has honoured retentionCount/retentionDays via ts-backups all along; this path had no bound at all.

The fix

Retention runs after a successful dump, reading the same compute.backups.retentionCount / retentionDays the scheduled path uses, and falling back to 5/30 rather than to unbounded — backups turns the schedule off, it does not document "keep forever". Pruning is scoped to <name>-*.sql.gz so one database's retention cannot delete another's on a shared box, and a caller that owns the destination's lifetime can pass retention: false.

Two things the bound exposed

Both are fixed here, because adding retention without them would destroy data that accumulating never did.

1. Retention would have run after a failed dump. runDb executes these under set -uo pipefail with no -e, so a failed dump doesn't stop the script — and gzip exits 0 even when its upstream dies, so pipefail is the only thing distinguishing a real dump from a 20-byte gzip header. Naive retention would count that header as the newest backup and delete a good dump to make room for it. The dump is now an explicit if: retention and the BACKUP= report are on the success branch only, and a failure removes its own output and exits 1.

2. A failed retry could destroy a good dump. The stamp is only second-granular, and gzip > "$file" opens and truncates the final name before the dump is known to work. So a failed retry in the same second as a good dump destroyed that dump. Reproduced, then fixed: dumps land on a .<name>-<stamp>.part scratch name and publish with mv -f on success. .part is outside the *.sql.gz glob, so a scratch file is never listed as a backup nor counted by retention; stale ones are reaped after an hour.

Verification

Sandbox-verified against real files, not just string assertions:

  • count bound keeps exactly N and always keeps the fresh dump (newest-first + tail -n +N+1)
  • age bound fires independently of the count bound
  • a second database's dumps in the same directory are untouched
  • failure path: exits 1, prunes nothing, leaves no partial behind
  • same-second failed retry (both runs forced into one second, confirmed by stamp) leaves the good dump byte-identical and no .part orphan

Also: 8 new unit tests; bun run lint clean (0 errors / 909 files); tsc --noEmit clean; full suite 3298 pass / 1 fail.

That one failure is box-converge.test.ts > "a drain signal reaches the runner while its active job finishes normally" — a pre-existing load-sensitive flake, not from this change. It spawns real child processes with a hardcoded setTimeout(…, 800) and waits on signal delivery, has zero references to dashboard-database, and passes in isolation both with and without this change. Earlier in the same session I chased phantom failures that turned out to be my own concurrent test runs competing for dashboard server ports; this one reproduces the same way.

Rebased onto current main (0.16.70). Re-verified on that base that runDb still uses set -uo pipefail without -e, that retentionCount/retentionDays are unchanged, and that buildListBackupsScript still globs *.sql.gz — all three are load-bearing for this fix.

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.