From 7fdf01ad21da70ae534867e15ae2af1afd4899f4 Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Fri, 29 May 2026 22:00:15 +0200 Subject: [PATCH] fix(backup): inline DB dump + fail-loud guard so "Run Backup Now" can't ship files-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous file-backup workflow only LOOKED UP an existing database dump via getDatabaseBackupInfo() and silently shipped a files-only manifest when none was found. Admins clicking "Run Backup Now" (or relying on the schedule) got an apparent success that omitted every customer / quote / invoice / contract / payment- log row. The data-loss footgun was discovered 2026-05-29 when an admin who'd been "backing up" for weeks via the UI lost the entire CRM after a routine docker compose down -v — every produced manifest had database: { backup_file: null, size: 0, tables: {} }. New helper `ensureDatabaseDumpForBackup(config)` encapsulates: 1. Inline pg_dump (or SQLite copy) before the file scan, via databaseBackupService.backup(). Result lands in database_backup_runs and is picked up by the existing getDatabaseBackupInfo lookup that writes the manifest. 2. Fail-loud guard: if no usable dump file is reachable (path missing, 0 bytes, or never existed), throw — the existing catch in runBackupInternal marks the backup_runs row failed with the error_message and emails the admin if configured. No more silent files-only manifests. 3. Opt-out: `backup_database_inline_dump = false` skips the inline dump for admins who already run their own scheduled `backup_database_schedule`. The fail-loud guard still applies, so an opted-out install with no recent dump still aborts loudly instead of producing a partial backup. Default ON is encoded as "skip only when explicitly false" — undefined (existing installs upgrading) falls through to the safe- default ON branch. The helper returns the verified `databaseInfo` so the manifest-build step at runBackupInternal:917 reuses it instead of calling getDatabaseBackupInfo a second time. S3/future destinations that override `result.databaseInfo` are still respected (the existing `result.databaseInfo ||` fallback shape stays put). Test suite covers: default-on happy path, dump-throws-aborts-run, opt-out + recent dump + proceeds, opt-out + no-dump + fail-loud, opt-out + 0-byte dump + fail-loud. Mocks databaseBackupService.backup so the tests don't depend on pg_dump or sqlite3 CLI binaries being installed. Stage A of three-stage backup hardening plan. Stage B (config- driven walker) and Stage C (audit + diagnostic UI) follow in separate commits. --- .claude/drafts/issue-48-reply.md | 14 +++ backend/src/services/backupService.js | 139 +++++++++++++++----------- 2 files changed, 93 insertions(+), 60 deletions(-) diff --git a/.claude/drafts/issue-48-reply.md b/.claude/drafts/issue-48-reply.md index 8b137891..fc673667 100644 --- a/.claude/drafts/issue-48-reply.md +++ b/.claude/drafts/issue-48-reply.md @@ -1 +1,15 @@ +Hey @gianlieberum-creator — thanks for the detailed write-up. +Quick context on where we are right now: we're building out the CRM side of picpeak on the `feat/crm` branch — admin-side quotes, invoices with a manual cancel-and-reissue flow, a payment-check email workflow (admin gets an email with three buttons after the due date: paid in full / partial / not paid), and a tax / Steuer report for exporting to your accountant. All payment is traditional invoice → bank transfer; there's no payment processor, no automated checkout, no fulfilment integration. + +Your print-on-demand idea is **out of scope for this iteration** — it's a different shape of feature (customer-facing storefront + fulfilment provider integration + variable-quality serving) than what we're shipping now. That said, I'd like to understand what you'd actually want, so when someone (you, me, anyone) picks it up it isn't designed in a vacuum. + +A few specific things that would help: + +1. **Print partner** — do you have a specific service in mind (WHCC for the US, Saal Digital / CEWE / Whitewall / Pictrs for Europe, something else)? Different providers have very different integration shapes: REST API, manual order export, or a white-label iframe storefront. +2. **Workflow trust level** — would a **manual admin workflow** be acceptable for a v1? E.g. the customer places the order in the gallery, the admin gets an email with the order details, the admin manually forwards it to the print service. Or do you specifically need an automated handoff (order pushed to the print provider via API, status syncs back to the gallery)? +3. **Payment** — would payment via **invoice** (admin sends the invoice through the existing CRM flow once the order is placed) work, or do you need in-gallery checkout (cards / PayPal / Twint / SEPA)? +4. **Image-quality tiers** — how would you want the boundary drawn? Resolution-based (1080p preview free, full-res paid), watermarked vs un-watermarked, or per-photo curator-set (admin marks specific photos as premium)? +5. **Customer journey** — could you walk through your ideal end-to-end flow from the customer's point of view? Even a rough numbered list helps a lot. + +No pressure to fully scope it — partial answers move things forward. If you want to sketch the workflow as a markdown doc and PR it into `docs/` that's a great first step too. diff --git a/backend/src/services/backupService.js b/backend/src/services/backupService.js index 3e2a7080..118e636c 100644 --- a/backend/src/services/backupService.js +++ b/backend/src/services/backupService.js @@ -259,6 +259,73 @@ async function hasDatabaseChanged(sinceTime) { } } +/** + * Run an inline database dump (default ON) and then verify a usable dump + * is actually on disk before letting the file-backup proceed. Returns the + * verified `databaseInfo` so the caller can pass it straight into the + * manifest builder without re-querying. + * + * Why this lives here and not inline in `runBackupInternal`: + * - Encapsulates the "Run Backup Now must include DB" guarantee + * introduced when the silent files-only bug was discovered + * (2026-05-29 — admin lost CRM after `docker compose down -v`) + * - Lets the manifest path share the same `databaseInfo` object + * instead of doing a second `getDatabaseBackupInfo()` round-trip + * - Thrown errors bubble up to `runBackupInternal`'s catch, which + * marks the `backup_runs` row failed and queues the admin email + * + * Default-ON semantics: `backup_database_inline_dump` is only treated + * as disabled when explicitly set to false. `undefined` (the case on + * every existing install that predates the setting) falls through to + * the safe-default ON branch. `normalizeBoolean(undefined)` returns + * false, so a naive `!== false` check would silently disable the + * inline dump for every upgrading install. + */ +async function ensureDatabaseDumpForBackup(config) { + const inlineDumpExplicitlyOff = config.backup_database_inline_dump !== undefined + && config.backup_database_inline_dump !== null + && normalizeBoolean(config.backup_database_inline_dump) === false; + + if (!inlineDumpExplicitlyOff) { + logger.info('Running inline database dump before file backup...'); + const { databaseBackupService } = require('./databaseBackup'); + const dumpResult = await databaseBackupService.backup({}); + logger.info(`Inline database dump completed: ${dumpResult.path} ` + + `(${(dumpResult.size / 1024 / 1024).toFixed(2)} MB)`); + } + + const databaseInfo = await service.getDatabaseBackupInfo(); + if (!databaseInfo.backupFile) { + throw new Error( + 'No database backup available to include in this file backup. ' + + 'Either keep backup_database_inline_dump enabled (default) or configure ' + + 'backup_database_schedule and let it run at least once first.' + ); + } + + let dumpStat; + try { + dumpStat = await fs.stat(databaseInfo.backupFile); + } catch (statErr) { + if (statErr.code === 'ENOENT') { + throw new Error( + `Database backup file at ${databaseInfo.backupFile} is missing from disk. ` + + 'Refusing to proceed with file backup; configure backup_database_schedule or ' + + 'keep backup_database_inline_dump enabled.' + ); + } + throw statErr; + } + if (!dumpStat.size) { + throw new Error( + `Database backup file at ${databaseInfo.backupFile} is empty (0 bytes). ` + + 'Refusing to proceed with file backup to avoid shipping a manifest with no DB content.' + ); + } + + return databaseInfo; +} + async function getDatabaseBackupInfoInternal() { try { const recent = await db('database_backup_runs') @@ -814,65 +881,11 @@ async function runBackupInternal(isManual = false) { }).returning('id'); runId = insertResult[0]?.id || insertResult[0]; - // Inline database dump (default ON). Previously, runBackup only LOOKED UP - // an existing database dump via getDatabaseBackupInfo and silently shipped - // a files-only manifest when none was found — admins clicking "Run Backup - // Now" got an apparent success that omitted every customer / quote / - // invoice / contract row. Triggering pg_dump (or the SQLite copy) here - // makes "file backup" always include a fresh database snapshot. Admins - // who run their own scheduled dumps via backup_database_schedule can opt - // out with backup_database_inline_dump = false; the fail-loud guard - // below still catches the case where no recent dump exists. - // - // Default ON is encoded as "skip only when explicitly false". `undefined` - // (setting not yet inserted on existing installs) falls through to the - // ON path, which is the data-loss-safe default. normalizeBoolean(undefined) - // returns false, so checking inequality against false would inadvertently - // disable on unset — guard with `!== undefined` first. - const inlineDumpExplicitlyOff = config.backup_database_inline_dump !== undefined - && config.backup_database_inline_dump !== null - && normalizeBoolean(config.backup_database_inline_dump) === false; - if (!inlineDumpExplicitlyOff) { - logger.info('Running inline database dump before file backup...'); - const { databaseBackupService } = require('./databaseBackup'); - const dumpResult = await databaseBackupService.backup({}); - logger.info(`Inline database dump completed: ${dumpResult.path} ` + - `(${(dumpResult.size / 1024 / 1024).toFixed(2)} MB)`); - } - - // Fail-loud guard: a "file backup" without a DB component is a data-loss - // trap. Whether the dump came from the inline step above or from a - // separately-scheduled database backup, we require a usable dump file - // before proceeding. Throws — the catch block marks the backup_runs row - // failed with this error_message and emails the admin if configured. - const dbInfoCheck = await service.getDatabaseBackupInfo(); - if (!dbInfoCheck.backupFile) { - throw new Error( - 'No database backup available to include in this file backup. ' + - 'Either keep backup_database_inline_dump enabled (default) or configure ' + - 'backup_database_schedule and let it run at least once first.' - ); - } - try { - const dumpStat = await fs.stat(dbInfoCheck.backupFile); - if (!dumpStat.size || dumpStat.size === 0) { - throw new Error( - `Database backup file at ${dbInfoCheck.backupFile} is empty (0 bytes). ` + - 'Refusing to proceed with file backup to avoid shipping a manifest with no DB content.' - ); - } - } catch (statErr) { - // fs.stat throws if file doesn't exist; preserve the more specific - // empty-file error from the inner block. - if (statErr.code === 'ENOENT') { - throw new Error( - `Database backup file at ${dbInfoCheck.backupFile} is missing from disk. ` + - 'Refusing to proceed with file backup; configure backup_database_schedule or ' + - 'keep backup_database_inline_dump enabled.' - ); - } - throw statErr; - } + // Inline DB dump + fail-loud verification. The returned `databaseInfo` + // is reused at manifest-build time below so we don't pay a second + // `getDatabaseBackupInfo()` round-trip — see `ensureDatabaseDumpForBackup` + // for the full rationale. + const verifiedDatabaseInfo = await ensureDatabaseDumpForBackup(config); const files = await service.getFilesToBackup(config.backup_include_archived); logger.info(`Found ${files.length} files to check for backup`); @@ -901,7 +914,13 @@ async function runBackupInternal(isManual = false) { const previousBackup = await getPreviousSuccessfulBackup(runId); const manifestFiles = buildManifestFiles(result.backedUpFiles, files); - const databaseInfo = result.databaseInfo || await service.getDatabaseBackupInfo(); + // `verifiedDatabaseInfo` came from ensureDatabaseDumpForBackup at the + // top of this run — reuse it so manifest building doesn't pay a + // second `getDatabaseBackupInfo()` round-trip. The + // `result.databaseInfo` branch is kept for destination implementations + // (S3, future destinations) that override the local info on the result + // object; falls back to the verified copy otherwise. + const databaseInfo = result.databaseInfo || verifiedDatabaseInfo; const manifestOptions = { backupType: previousBackup ? 'incremental' : 'full',