fix(backup): cron schedule mapping + manifest format detection + bigint coerce
Three pre-existing bugs surfaced by re-running the backup-s3 integration
suite. backup-s3 went 0/12 → 7/12 (storage-refactor session bootstrap
fixes) → 10/12 with this commit.
1. Backup service crashes on backend startup with
`TypeError: Cannot read properties of undefined (reading 'replace')`
from node-cron's expression parser.
Root cause: `backup_schedule` stores a UI label like "weekly", while
`backup_schedule_cron` stores the actual cron expression. Startup
code read the label and passed it straight to cron.schedule() —
"weekly" is not a cron expression.
Fix in startBackupService(): read backup_schedule_cron first; fall
back to mapping known labels (hourly/daily/weekly/monthly) to cron
expressions; back-compat for deployments that wrote a cron expression
into the legacy backup_schedule field.
2. Backup manifest retrieval fails with
`SyntaxError: Unexpected token 'a', "applicatio"...` when the
manifest format is YAML.
Root cause: getBackupManifest() downloads the s3:// manifest to a
tmp file hardcoded as `manifest-N.json`. loadManifest() then
detects format from extension only — sees .json, runs JSON.parse on
YAML content (which starts with "application: …"), fails.
Fix in backupManifest.loadManifest(): detect format from BOTH the
extension AND the content's first non-whitespace character. JSON
starts with { or [; anything else falls through to yaml.load.
Backwards compatible — extension is still authoritative when present
AND content matches.
3. Test assertion `expect(backupRun.total_size_bytes).toBeGreaterThan(0)`
fails with "received value must be a number or bigint" because pg
driver returns bigint columns as strings. Coerce via Number() in
the test.
Remaining 2 failures (out of scope here, both are spec-level drift):
- "should include database backup" expects the runBackup() flow to
upload the database backup file at S3 key `database/db-backup.sql`.
Current implementation reads db backup metadata for the manifest but
does not upload the file itself. Missing feature, not a test bug.
- "should only upload changed files" expects manifest.incremental.
modified_files_count. Implementation writes backupType: 'incremental'
on the run row but no per-run incremental subobject in the manifest.
Field shape mismatch.
This commit is contained in:
@@ -176,8 +176,9 @@ describe('S3 Backup Integration Tests', () => {
|
||||
.first();
|
||||
|
||||
expect(backupRun.status).toBe('completed');
|
||||
expect(backupRun.files_backed_up).toBeGreaterThan(0);
|
||||
expect(backupRun.total_size_bytes).toBeGreaterThan(0);
|
||||
// pg driver returns bigint columns as strings; coerce for the size assertion.
|
||||
expect(Number(backupRun.files_backed_up)).toBeGreaterThan(0);
|
||||
expect(Number(backupRun.total_size_bytes)).toBeGreaterThan(0);
|
||||
|
||||
// Verify files in S3
|
||||
const s3Objects = await listS3Objects();
|
||||
|
||||
@@ -181,8 +181,13 @@ class BackupManifestGenerator {
|
||||
const content = await fs.readFile(filePath, 'utf8');
|
||||
let manifest;
|
||||
|
||||
// Detect format and parse
|
||||
if (filePath.endsWith('.yaml') || filePath.endsWith('.yml')) {
|
||||
// Detect format from BOTH the extension and the content. Earlier code
|
||||
// trusted the extension alone, which broke when callers stored a YAML
|
||||
// manifest under a .json temp name (getBackupManifest does this when
|
||||
// downloading the s3:// path to a tmp file).
|
||||
const looksLikeJson = content.trimStart().startsWith('{')
|
||||
|| content.trimStart().startsWith('[');
|
||||
if (filePath.endsWith('.yaml') || filePath.endsWith('.yml') || !looksLikeJson) {
|
||||
manifest = yaml.load(content);
|
||||
} else {
|
||||
manifest = JSON.parse(content);
|
||||
|
||||
@@ -936,17 +936,41 @@ async function startBackupService() {
|
||||
backupJob = null;
|
||||
}
|
||||
|
||||
// Two settings cooperate here:
|
||||
// - backup_schedule — UI label like "daily" / "weekly" / "custom"
|
||||
// - backup_schedule_cron — actual cron expression
|
||||
// The frontend writes both (BackupConfiguration.jsx). Older startup code
|
||||
// here read backup_schedule and crashed when it found a label instead of
|
||||
// a cron expression. Resolution order: explicit cron field, then map known
|
||||
// labels, then fall back to default.
|
||||
const NAMED_SCHEDULES = {
|
||||
hourly: '0 * * * *',
|
||||
daily: '0 2 * * *',
|
||||
weekly: '0 3 * * 0', // Sunday 03:00
|
||||
monthly: '0 4 1 * *',
|
||||
};
|
||||
const isCronExpression = (s) => typeof s === 'string' && /^\s*\S+(\s+\S+){4}\s*$/.test(s);
|
||||
const readSetting = (key) => {
|
||||
if (config && Object.prototype.hasOwnProperty.call(config, key)) {
|
||||
return String(config[key] ?? '').trim();
|
||||
}
|
||||
if (config?.__raw && Object.prototype.hasOwnProperty.call(config.__raw, key)) {
|
||||
return String(parseSettingValue(config.__raw[key]) ?? '').trim();
|
||||
}
|
||||
return '';
|
||||
};
|
||||
|
||||
let schedule = '0 2 * * *';
|
||||
if (Object.prototype.hasOwnProperty.call(config, 'backup_schedule')) {
|
||||
const candidate = String(config.backup_schedule ?? '').trim();
|
||||
if (candidate.length) {
|
||||
schedule = candidate;
|
||||
}
|
||||
} else if (config.__raw && Object.prototype.hasOwnProperty.call(config.__raw, 'backup_schedule')) {
|
||||
const candidate = String(parseSettingValue(config.__raw.backup_schedule) ?? '').trim();
|
||||
if (candidate.length) {
|
||||
schedule = candidate;
|
||||
}
|
||||
const cronCandidate = readSetting('backup_schedule_cron');
|
||||
const labelCandidate = readSetting('backup_schedule');
|
||||
if (cronCandidate && isCronExpression(cronCandidate)) {
|
||||
schedule = cronCandidate;
|
||||
} else if (labelCandidate && NAMED_SCHEDULES[labelCandidate.toLowerCase()]) {
|
||||
schedule = NAMED_SCHEDULES[labelCandidate.toLowerCase()];
|
||||
} else if (labelCandidate && isCronExpression(labelCandidate)) {
|
||||
// Back-compat: a deployment that wrote a cron expression directly into
|
||||
// backup_schedule (no _cron field) still works.
|
||||
schedule = labelCandidate;
|
||||
}
|
||||
|
||||
backupJob = cron.schedule(schedule, async () => {
|
||||
|
||||
Reference in New Issue
Block a user