From e232f9f2cf54aeba1e16d769397428206a0f1801 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 28 Apr 2026 10:52:49 +0200 Subject: [PATCH] fix(backup): incremental backups against S3 + jsonb stats parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes uncovered while bringing the backup-s3 integration suite to 12/12 against MinIO + Postgres: - backupService.getDatabaseBackupInfo: pg's jsonb driver auto-parses `statistics` / `table_checksums` to objects; the old JSON.parse() then threw "[object Object]" is not valid JSON and the manifest dropped database info silently. Accept both string and object inputs. - backupService.runBackup: incremental path called backupManifest.loadManifest() with an s3:// URI directly, which falls through to fs.readFile() and ENOENTs — every "incremental" backup silently downgraded to a full one. Added loadManifestFromAnywhere() helper that downloads s3:// to a tmp file before delegating. - backupManifest.generateIncrementalManifest: attached the `incremental` section AFTER generateManifest() had already stamped verification.total_checksum, so every incremental manifest failed validateManifest() on read-back. Recompute the checksum after. Test side: updated assertions to the current manifest shape (`incremental.changes.modified_files_count`), Number()-coerce bigint columns from pg, and gate the logger mock on UNMOCK_LOGGER for diagnosing similar silent-failure modes in the future. --- .../__tests__/integration/backup-s3.test.js | 19 +++-- backend/src/services/backupManifest.js | 6 ++ backend/src/services/backupService.js | 80 ++++++++++++++++++- 3 files changed, 95 insertions(+), 10 deletions(-) diff --git a/backend/__tests__/integration/backup-s3.test.js b/backend/__tests__/integration/backup-s3.test.js index d8f26d5a..afe1bb3c 100644 --- a/backend/__tests__/integration/backup-s3.test.js +++ b/backend/__tests__/integration/backup-s3.test.js @@ -80,10 +80,12 @@ describe('S3 Backup Integration Tests', () => { await setupTestData(); // Mock logger to reduce noise - logger.info = jest.fn(); - logger.debug = jest.fn(); - logger.warn = jest.fn(); - logger.error = jest.fn(); + if (process.env.UNMOCK_LOGGER !== 'true') { + logger.info = jest.fn(); + logger.debug = jest.fn(); + logger.warn = jest.fn(); + logger.error = jest.fn(); + } }); afterAll(async () => { @@ -281,13 +283,16 @@ describe('S3 Backup Integration Tests', () => { .first(); expect(secondRun.id).not.toBe(firstRun.id); - expect(secondRun.files_backed_up).toBe(1); // Only modified file + expect(Number(secondRun.files_backed_up)).toBe(1); // Only modified file - // Check manifest indicates incremental + // Check manifest indicates incremental. The current manifest schema + // groups counts under `incremental.changes.*` (added/modified/deleted/ + // unchanged + size_difference) — see backupManifest.generateIncrementalManifest. if (secondRun.manifest_path) { const manifest = await backupService.getBackupManifest(secondRun.id); expect(manifest.manifest.incremental).toBeDefined(); - expect(manifest.manifest.incremental.modified_files_count).toBe(1); + expect(manifest.manifest.incremental.changes).toBeDefined(); + expect(manifest.manifest.incremental.changes.modified_files_count).toBe(1); } }); diff --git a/backend/src/services/backupManifest.js b/backend/src/services/backupManifest.js index d69fb548..41bf0370 100644 --- a/backend/src/services/backupManifest.js +++ b/backend/src/services/backupManifest.js @@ -323,6 +323,12 @@ class BackupManifestGenerator { deleted_files: comparison.deleted_files.map(f => f.path) }; + // Recalculate the checksum after attaching the incremental section, + // otherwise validateManifest() rejects the loaded manifest because + // generateManifest() stamped a checksum that did NOT include this + // section. + fullManifest.verification.total_checksum = this.calculateManifestChecksum(fullManifest); + return fullManifest; } diff --git a/backend/src/services/backupService.js b/backend/src/services/backupService.js index 29c6c372..74720bc1 100644 --- a/backend/src/services/backupService.js +++ b/backend/src/services/backupService.js @@ -268,6 +268,15 @@ async function getDatabaseBackupInfoInternal() { if (recent && recent.file_path) { const hasChanged = await hasDatabaseChanged(recent.completed_at); + // Postgres jsonb columns come back already parsed; sqlite TEXT comes + // back as a JSON string. Accept both. + const parseField = (v) => { + if (v == null) return null; + if (typeof v === 'object') return v; + try { return JSON.parse(v); } catch { return null; } + }; + const stats = parseField(recent.statistics); + const checksums = parseField(recent.table_checksums); return { type: recent.backup_type || 'unknown', backupFile: recent.file_path, @@ -275,8 +284,8 @@ async function getDatabaseBackupInfoInternal() { checksum: recent.checksum, hasChanged, backupTime: recent.completed_at, - tables: recent.statistics ? JSON.parse(recent.statistics).tables : {}, - rowCounts: recent.table_checksums ? JSON.parse(recent.table_checksums) : {} + tables: (stats && stats.tables) || {}, + rowCounts: checksums || {} }; } @@ -826,7 +835,7 @@ async function runBackupInternal(isManual = false) { let manifest = await backupManifest.generateManifest(manifestOptions); if (previousBackup && previousBackup.manifest_path) { try { - const parentManifest = await backupManifest.loadManifest(previousBackup.manifest_path); + const parentManifest = await loadManifestFromAnywhere(previousBackup.manifest_path, config); manifest = await backupManifest.generateIncrementalManifest(manifestOptions, parentManifest); } catch (error) { logger.warn('Failed to load parent manifest, generating full manifest:', error); @@ -1097,6 +1106,71 @@ async function cleanupOldBackupRuns(retentionDays = 30) { } } +/** + * Load a backup manifest regardless of whether it lives on the local + * filesystem or in S3. Used by both the public getBackupManifest API + * and the incremental-manifest path in runBackupInternal — previously + * the latter called loadManifest() with an s3:// URI directly, which + * tried fs.readFile on the literal string and threw ENOENT, silently + * downgrading every incremental backup to a full manifest. + */ +async function loadManifestFromAnywhere(manifestPath, config) { + if (!manifestPath) { + throw new Error('Manifest path is required'); + } + if (!manifestPath.startsWith('s3://')) { + return backupManifest.loadManifest(manifestPath); + } + + const cfg = config || (await resolveConfigWithFallback()); + const accessKey = cfg?.backup_s3_access_key + ?? (cfg?.__raw && Object.prototype.hasOwnProperty.call(cfg.__raw, 'backup_s3_access_key') + ? parseSettingValue(cfg.__raw.backup_s3_access_key) + : undefined) + ?? process.env.BACKUP_S3_ACCESS_KEY; + const secretKey = cfg?.backup_s3_secret_key + ?? (cfg?.__raw && Object.prototype.hasOwnProperty.call(cfg.__raw, 'backup_s3_secret_key') + ? parseSettingValue(cfg.__raw.backup_s3_secret_key) + : undefined) + ?? process.env.BACKUP_S3_SECRET_KEY; + + if (!accessKey || !secretKey) { + throw new Error('S3 credentials not configured for manifest retrieval'); + } + + const match = manifestPath.match(/^s3:\/\/([^\/]+)\/(.+)$/); + if (!match) { + throw new Error('Invalid S3 manifest path'); + } + const [, bucket, key] = match; + + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'backup-manifest-')); + // Preserve the original extension so loadManifest's format detection + // picks the right parser. + const ext = path.extname(key) || '.json'; + const tempPath = path.join(tempDir, `manifest-${Date.now()}${ext}`); + + const s3Client = new S3StorageAdapter({ + bucket, + region: (cfg && cfg.backup_s3_region) || 'us-east-1', + endpoint: cfg && cfg.backup_s3_endpoint, + accessKeyId: accessKey, + secretAccessKey: secretKey, + forcePathStyle: cfg ? normalizeBoolean(cfg.backup_s3_force_path_style) : false, + sslEnabled: cfg && cfg.backup_s3_ssl_enabled !== undefined + ? normalizeBoolean(cfg.backup_s3_ssl_enabled) + : true, + }); + + try { + await s3Client.download(key, tempPath); + return await backupManifest.loadManifest(tempPath); + } finally { + await fs.unlink(tempPath).catch(() => {}); + await fs.rm(tempDir, { recursive: true, force: true }).catch(() => {}); + } +} + async function getBackupManifest(backupRunId) { const run = await db('backup_runs') .where('id', backupRunId)