fix(backup): incremental backups against S3 + jsonb stats parsing
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.
This commit is contained in:
@@ -80,10 +80,12 @@ describe('S3 Backup Integration Tests', () => {
|
|||||||
await setupTestData();
|
await setupTestData();
|
||||||
|
|
||||||
// Mock logger to reduce noise
|
// Mock logger to reduce noise
|
||||||
|
if (process.env.UNMOCK_LOGGER !== 'true') {
|
||||||
logger.info = jest.fn();
|
logger.info = jest.fn();
|
||||||
logger.debug = jest.fn();
|
logger.debug = jest.fn();
|
||||||
logger.warn = jest.fn();
|
logger.warn = jest.fn();
|
||||||
logger.error = jest.fn();
|
logger.error = jest.fn();
|
||||||
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
afterAll(async () => {
|
afterAll(async () => {
|
||||||
@@ -281,13 +283,16 @@ describe('S3 Backup Integration Tests', () => {
|
|||||||
.first();
|
.first();
|
||||||
|
|
||||||
expect(secondRun.id).not.toBe(firstRun.id);
|
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) {
|
if (secondRun.manifest_path) {
|
||||||
const manifest = await backupService.getBackupManifest(secondRun.id);
|
const manifest = await backupService.getBackupManifest(secondRun.id);
|
||||||
expect(manifest.manifest.incremental).toBeDefined();
|
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);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -323,6 +323,12 @@ class BackupManifestGenerator {
|
|||||||
deleted_files: comparison.deleted_files.map(f => f.path)
|
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;
|
return fullManifest;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -268,6 +268,15 @@ async function getDatabaseBackupInfoInternal() {
|
|||||||
|
|
||||||
if (recent && recent.file_path) {
|
if (recent && recent.file_path) {
|
||||||
const hasChanged = await hasDatabaseChanged(recent.completed_at);
|
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 {
|
return {
|
||||||
type: recent.backup_type || 'unknown',
|
type: recent.backup_type || 'unknown',
|
||||||
backupFile: recent.file_path,
|
backupFile: recent.file_path,
|
||||||
@@ -275,8 +284,8 @@ async function getDatabaseBackupInfoInternal() {
|
|||||||
checksum: recent.checksum,
|
checksum: recent.checksum,
|
||||||
hasChanged,
|
hasChanged,
|
||||||
backupTime: recent.completed_at,
|
backupTime: recent.completed_at,
|
||||||
tables: recent.statistics ? JSON.parse(recent.statistics).tables : {},
|
tables: (stats && stats.tables) || {},
|
||||||
rowCounts: recent.table_checksums ? JSON.parse(recent.table_checksums) : {}
|
rowCounts: checksums || {}
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -826,7 +835,7 @@ async function runBackupInternal(isManual = false) {
|
|||||||
let manifest = await backupManifest.generateManifest(manifestOptions);
|
let manifest = await backupManifest.generateManifest(manifestOptions);
|
||||||
if (previousBackup && previousBackup.manifest_path) {
|
if (previousBackup && previousBackup.manifest_path) {
|
||||||
try {
|
try {
|
||||||
const parentManifest = await backupManifest.loadManifest(previousBackup.manifest_path);
|
const parentManifest = await loadManifestFromAnywhere(previousBackup.manifest_path, config);
|
||||||
manifest = await backupManifest.generateIncrementalManifest(manifestOptions, parentManifest);
|
manifest = await backupManifest.generateIncrementalManifest(manifestOptions, parentManifest);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
logger.warn('Failed to load parent manifest, generating full manifest:', 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) {
|
async function getBackupManifest(backupRunId) {
|
||||||
const run = await db('backup_runs')
|
const run = await db('backup_runs')
|
||||||
.where('id', backupRunId)
|
.where('id', backupRunId)
|
||||||
|
|||||||
Reference in New Issue
Block a user