ReviewOS
Reload

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

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/ts-cloud/src/deploy/dashboard-database.test.tsmodified+96-6
@@ -4,16 +4,19 @@ import {
44 buildBackupScript,
55 buildCreateDatabaseScript,
66 buildCreateUserScript,
7 buildListBackupsScript,
78 buildListScript,
89 buildVitessListScript,
10 DEFAULT_DB_BACKUP_RETENTION,
911 isExternalEngine,
1012 isValidDbIdentifier,
11 parseVitessTopology,
12 shardsMissingPrimary,
13 unhealthyTablets,
1413 parseBackups,
1514 parseDbList,
15 parseVitessTopology,
16 resolveDbBackupRetention,
1617 resolveDbEngine,
18 shardsMissingPrimary,
19 unhealthyTablets,
1720} from './dashboard-database'
1821
1922describe('resolveDbEngine', () => {
@@ -149,10 +152,12 @@ describe('buildListScript + parseDbList', () => {
149152 expect(script.filter(line => line.includes('$(date'))).toHaveLength(1)
150153 expect(script.some(line => line.startsWith('TS_CLOUD_BACKUP_STAMP='))).toBe(true)
151154
152 // The write, the report and the listing must all name the same thing.
153 const named = script.filter(line => line.includes('acme-'))
155 // The publish, the report and the listing must all name the same thing.
156 const named = script.filter(line => line.includes('acme-$TS_CLOUD_BACKUP_STAMP.sql.gz'))
154157 expect(named).toHaveLength(3)
155 for (const line of named) expect(line).toContain('acme-$TS_CLOUD_BACKUP_STAMP.sql.gz')
158 // And the scratch file is named once and consistently: written to,
159 // renamed from, removed on failure.
160 expect(script.filter(line => line.includes('.acme-$TS_CLOUD_BACKUP_STAMP.part'))).toHaveLength(3)
156161
157162 // And the stamp is set before anything uses it.
158163 expect(script.findIndex(l => l.startsWith('TS_CLOUD_BACKUP_STAMP=')))
@@ -161,6 +166,91 @@ describe('buildListScript + parseDbList', () => {
161166 })
162167
163168
169 /**
170 * Every on-demand dump used to be kept forever: `buildBackupScript` wrote a
171 * timestamped file and nothing pruned the directory, so each dashboard
172 * "Backup" click left a full dump on the box. The scheduled path has honoured
173 * `retentionCount`/`retentionDays` all along; this path had no bound at all.
174 */
175 describe('on-demand dump retention', () => {
176 it('keeps the newest N dumps of that database and drops the rest', () => {
177 const script = buildBackupScript('mysql', 'acme', '/var/backups/ts-cloud/databases', undefined, { count: 3 }).join('\n')
178 // Newest first, so `+4` is everything past the newest 3. The dump just
179 // taken is newest, so it always survives.
180 expect(script).toContain('ls -1t /var/backups/ts-cloud/databases/acme-*.sql.gz 2>/dev/null | tail -n +4 | xargs -r rm -f')
181 })
182
183 it('drops dumps older than the configured age', () => {
184 const script = buildBackupScript('mysql', 'acme', '/var/backups/ts-cloud/databases', undefined, { days: 7 }).join('\n')
185 expect(script).toContain('-name "acme-*.sql.gz" -mtime +7 -delete')
186 })
187
188 it('defaults to a bound rather than to unbounded', () => {
189 const script = buildBackupScript('mysql', 'acme').join('\n')
190 expect(script).toContain(`tail -n +${DEFAULT_DB_BACKUP_RETENTION.count + 1}`)
191 expect(script).toContain(`-mtime +${DEFAULT_DB_BACKUP_RETENTION.days} -delete`)
192 })
193
194 /** A shared box holds every database's dumps in one directory. */
195 it('never prunes another database\'s dumps', () => {
196 const script = buildBackupScript('mysql', 'acme').join('\n')
197 for (const line of script.split('\n').filter(l => l.includes('rm -f') || l.includes('-delete')))
198 expect(line).toContain('acme-')
199 })
200
201 it('can be turned off for a caller that owns the directory itself', () => {
202 const script = buildBackupScript('postgres', 'acme', '/tmp', undefined, false).join('\n')
203 expect(script).not.toContain('-delete')
204 expect(script).not.toContain('tail -n +')
205 })
206
207 /**
208 * `runDb` runs these under `set -uo pipefail` with NO `-e`, so a failed dump
209 * does not stop the script — and `gzip` exits 0 even when its upstream died.
210 * If retention ran anyway it would count a 20-byte gzip header as the newest
211 * backup and delete a good one to make room: adding a bound would then
212 * destroy data that accumulating never did.
213 */
214 it('prunes nothing when the dump fails', () => {
215 const script = buildBackupScript('mysql', 'acme')
216 const elseAt = script.indexOf('else')
217 expect(elseAt).toBeGreaterThan(-1)
218 const failureBranch = script.slice(elseAt).join('\n')
219 expect(failureBranch).not.toContain('-delete')
220 expect(failureBranch).not.toContain('tail -n +')
221 // And it reports the failure rather than exiting 0 with no dump.
222 expect(failureBranch).toContain('exit 1')
223 })
224
225 /**
226 * The stamp is only second-granular. Redirecting straight to the final name
227 * opens and truncates it before the dump is known to work, so a failed retry
228 * within the same second as a good dump destroyed that dump. Reproduced
229 * before the fix.
230 */
231 it('publishes by rename, so a failed retry cannot clobber a good dump', () => {
232 const script = buildBackupScript('mysql', 'acme')
233 const dumpLine = script.find(l => l.startsWith('if '))!
234 // The dump writes to the scratch name, never to the published one.
235 expect(dumpLine).toContain('.acme-$TS_CLOUD_BACKUP_STAMP.part"')
236 expect(dumpLine).not.toContain('acme-$TS_CLOUD_BACKUP_STAMP.sql.gz')
237 expect(script.join('\n')).toContain('mv -f "/var/backups/ts-cloud/databases/.acme-$TS_CLOUD_BACKUP_STAMP.part"')
238 // Scratch files are outside the `*.sql.gz` glob, so they are never listed
239 // as a backup — and stale ones get reaped.
240 expect(buildListBackupsScript()).toEqual([expect.stringContaining('*.sql.gz')])
241 expect(script.join('\n')).toContain('-name ".acme-*.part" -mmin +60 -delete')
242 })
243
244 it('reads its bounds from the same config the scheduled path uses', () => {
245 const base = { project: { name: 'acme', slug: 'acme' } } as any
246 expect(resolveDbBackupRetention(base)).toEqual(DEFAULT_DB_BACKUP_RETENTION)
247 expect(resolveDbBackupRetention({
248 ...base,
249 infrastructure: { compute: { backups: { retentionCount: 2, retentionDays: 3 } } },
250 })).toEqual({ count: 2, days: 3 })
251 })
252 })
253
164254 it('parses BACKUP= lines into database + file, deriving the db from the filename', () => {
165255 const parsed = parseBackups(
166256 'BACKUP=/var/backups/ts-cloud/databases/acme-20260702-101500.sql.gz\nnoise\nBACKUP=/var/backups/ts-cloud/databases/blog-20260701-090000.sql.gz',
packages/ts-cloud/src/deploy/dashboard-database.tsmodified+92-19
@@ -428,17 +428,71 @@ export async function createDatabaseUser(
428428export const DB_BACKUP_DIR = '/var/backups/ts-cloud/databases'
429429
430430/**
431 * Script that dumps a single database to a timestamped, gzipped file. The name
432 * is a validated SQL identifier (no shell metacharacters), so it is safe to
433 * embed directly. The timestamp is computed on the box. Postgres connects over
434 * the local unix socket for a co-located engine, or TCP with credentials for
435 * an external host (see {@link pgAdminCommand}).
431 * Local retention for on-box dumps, mirroring the bounds the scheduled path
432 * already honours ({@link ComputeBackupConfig.retentionCount} /
433 * {@link ComputeBackupConfig.retentionDays}).
434 */
435export interface DbBackupRetention {
436 /** Keep the newest N dumps of this database. @default 5 */
437 count?: number
438 /** Delete this database's dumps older than N days. @default 30 */
439 days?: number
440}
441
442/** Applied when `compute.backups` configures no bounds of its own. */
443export const DEFAULT_DB_BACKUP_RETENTION = { count: 5, days: 30 } as const
444
445/**
446 * Retention for an on-demand dump. Falls back to the defaults rather than to
447 * "unbounded" even when scheduled backups are disabled: `compute.backups` turns
448 * the *schedule* off, and a dashboard button that leaves dumps on the box
449 * forever is the bug this resolves, not a documented mode.
450 */
451export function resolveDbBackupRetention(config: CloudConfig): DbBackupRetention {
452 const backups = config.infrastructure?.compute?.backups
453 return {
454 count: backups?.retentionCount ?? DEFAULT_DB_BACKUP_RETENTION.count,
455 days: backups?.retentionDays ?? DEFAULT_DB_BACKUP_RETENTION.days,
456 }
457}
458
459/**
460 * Prune one database's dumps. Scoped to `<name>-*.sql.gz` so a shared box does
461 * not let one database's retention delete another's, and emitted only on the
462 * success branch of {@link buildBackupScript}.
463 */
464function buildDbBackupRetentionScript(destDir: string, name: string, retention: DbBackupRetention): string[] {
465 const count = Math.max(1, Math.floor(retention.count ?? DEFAULT_DB_BACKUP_RETENTION.count))
466 const days = Math.max(1, Math.floor(retention.days ?? DEFAULT_DB_BACKUP_RETENTION.days))
467 const pattern = `${name}-*.sql.gz`
468 return [
469 // Newest first, so `tail -n +N+1` is everything past the newest N. The dump
470 // just taken is the newest, so a count of at least 1 always keeps it.
471 ` ls -1t ${destDir}/${pattern} 2>/dev/null | tail -n +${count + 1} | xargs -r rm -f`,
472 ` find ${destDir} -xdev -maxdepth 1 -type f -name ${JSON.stringify(pattern)} -mtime +${days} -delete 2>/dev/null || true`,
473 // Scratch files from a dump whose process was killed before either branch
474 // ran. An hour bounds a concurrent dump that is still writing.
475 ` find ${destDir} -xdev -maxdepth 1 -type f -name ${JSON.stringify(`.${name}-*.part`)} -mmin +60 -delete 2>/dev/null || true`,
476 ]
477}
478
479/**
480 * Script that dumps a single database to a timestamped, gzipped file, then
481 * prunes older dumps of that database. The name is a validated SQL identifier
482 * (no shell metacharacters), so it is safe to embed directly. The timestamp is
483 * computed on the box. Postgres connects over the local unix socket for a
484 * co-located engine, or TCP with credentials for an external host (see
485 * {@link pgAdminCommand}).
486 *
487 * Pass `retention: false` for a caller that owns the destination directory's
488 * lifetime itself (a dump staged for a one-shot move, say).
436489 */
437490export function buildBackupScript(
438491 engine: DbEngine,
439492 name: string,
440493 destDir: string = DB_BACKUP_DIR,
441494 database?: DatabaseConfig,
495 retention: DbBackupRetention | false = {},
442496): string[] {
443497 // The timestamp is taken ONCE into a variable rather than embedded as a
444498 // command substitution. Written inline, `$(date …)` runs again in every
@@ -448,22 +502,41 @@ export function buildBackupScript(
448502 // find, which is the worst way for a backup to fail.
449503 const stamp = 'TS_CLOUD_BACKUP_STAMP="$(date +%Y%m%d-%H%M%S)"'
450504 const file = `${destDir}/${name}-$TS_CLOUD_BACKUP_STAMP.sql.gz`
451 const mkdir = `mkdir -p ${destDir}`
452 if (engine === 'postgres')
453 return [
454 mkdir,
455 stamp,
456 `${pgAdminCommand(database, 'pg_dump')} ${name} | gzip > "${file}"`,
457 `echo "BACKUP=${file}"`,
458 `ls -l "${file}"`,
459 ]
505 // Dump to a scratch name and rename only on success. Redirecting straight to
506 // `${file}` opens (and truncates) the final name before the dump is known to
507 // have worked, so two dumps of one database in the same second — the second
508 // failing — destroyed the first: same stamp, same path. The stamp is only
509 // second-granular, and a retry after a failure is exactly when that happens.
510 // `.part` is outside the `*.sql.gz` glob, so a scratch file is never listed
511 // as a backup nor counted by retention.
512 const part = `${destDir}/.${name}-$TS_CLOUD_BACKUP_STAMP.part`
460513 const sock = engine === 'mariadb' ? SOCKETS.mariadb : SOCKETS.mysql
514 const dump = engine === 'postgres'
515 ? `${pgAdminCommand(database, 'pg_dump')} ${name} | gzip > "${part}"`
516 : `mysqldump --socket=${sock} -u root ${name} | gzip > "${part}"`
517
461518 return [
462 mkdir,
519 `mkdir -p ${destDir}`,
463520 stamp,
464 `mysqldump --socket=${sock} -u root ${name} | gzip > "${file}"`,
465 `echo "BACKUP=${file}"`,
466 `ls -l "${file}"`,
521 // Branch explicitly on the dump rather than letting the script run on.
522 // `runDb` executes these under `set -uo pipefail` with NO `-e`, so a failed
523 // dump does not stop what follows — and `gzip` exits 0 even when its
524 // upstream died, so pipefail is the only thing that tells a real dump from
525 // a 20-byte gzip header. Without this gate, retention would count that
526 // header as the newest backup and delete a good one to make room for it:
527 // adding a bound would then destroy data that accumulating never did.
528 `if ${dump}; then`,
529 ` mv -f "${part}" "${file}"`,
530 ` echo "BACKUP=${file}"`,
531 ` ls -l "${file}"`,
532 ...(retention === false ? [] : buildDbBackupRetentionScript(destDir, name, retention)),
533 'else',
534 // A partial file is worse than none: `buildListBackupsScript` would report
535 // it as a backup, and `ls -1t` would rank it newest.
536 ` rm -f "${part}"`,
537 ` echo "[ts-cloud] backup failed for ${name}; no dump was kept" >&2`,
538 ' exit 1',
539 'fi',
467540 ]
468541}
469542
@@ -502,7 +575,7 @@ export async function backupDatabase(
502575 const r = await runDb(
503576 config,
504577 environment,
505 buildBackupScript(engine, name, DB_BACKUP_DIR, resolveAppDatabase(config)),
578 buildBackupScript(engine, name, DB_BACKUP_DIR, resolveAppDatabase(config), resolveDbBackupRetention(config)),
506579 `ts-cloud db:backup ${name}`,
507580 )
508581 return { ...r, database: name }