fix(backup): stop ignoring the configured database-backup destination path
databaseBackupService.getBackupConfig() returns the raw database_backup_*-prefixed setting keys, but backup() and startScheduledBackups() destructured unprefixed names off that object (destinationPath, compress, enabled, schedule, retentionDays, emailOnSuccess/Failure). None of those keys ever existed on the config object, so every read silently fell through to its hardcoded default. The visible symptom (reported in issue 1365): the inline database dump that runs before every file backup (default ON) always tried to create /backup/database, regardless of what an admin configured, and died with EACCES on the read-only default path — before the file backup's own (correctly wired) backup_destination_path was ever reached. The standalone scheduled database-backup runner had the same bug: config.enabled was always undefined, so it silently never started regardless of database_backup_enabled. Also fixes saveManifestToLocal's manifest-directory fallback, which hardcoded /backup instead of matching the sane getStoragePath()/backups default used everywhere else for a missing backup_destination_path. Relates to issue 1365
This commit is contained in:
@@ -1,4 +1,3 @@
|
||||
const { DatabaseBackupService } = require('../databaseBackup');
|
||||
const { db } = require('../../database/db');
|
||||
const fs = require('fs').promises;
|
||||
const path = require('path');
|
||||
@@ -9,6 +8,10 @@ jest.mock('../../database/db');
|
||||
jest.mock('../../utils/logger');
|
||||
jest.mock('../emailProcessor');
|
||||
jest.mock('child_process');
|
||||
jest.mock('node-cron', () => ({ schedule: jest.fn(() => ({ stop: jest.fn() })) }));
|
||||
|
||||
const { DatabaseBackupService, startScheduledBackups } = require('../databaseBackup');
|
||||
const cron = require('node-cron');
|
||||
|
||||
describe('DatabaseBackupService', () => {
|
||||
let service;
|
||||
@@ -188,6 +191,76 @@ describe('DatabaseBackupService', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('backup() destination path resolution (#1365)', () => {
|
||||
// getBackupConfig() returns database_backup_*-prefixed keys.
|
||||
// Regression: backup() used to destructure the unprefixed names
|
||||
// (`destinationPath`, ...) straight off that object, which never
|
||||
// matched, so the configured path was silently ignored and every
|
||||
// run tried to create the hardcoded /backup/database default.
|
||||
it('creates the directory from database_backup_destination_path when configured', async () => {
|
||||
db.mockReturnValue({
|
||||
where: jest.fn().mockReturnThis(),
|
||||
select: jest.fn().mockResolvedValue([
|
||||
{ setting_key: 'database_backup_destination_path', setting_value: JSON.stringify('/data/db-backups') }
|
||||
])
|
||||
});
|
||||
|
||||
const stop = new Error('stop after mkdir — nothing past it matters for this test');
|
||||
const mkdirSpy = jest.spyOn(fs, 'mkdir').mockRejectedValue(stop);
|
||||
|
||||
await expect(service.backup({})).rejects.toThrow(stop.message);
|
||||
|
||||
expect(mkdirSpy).toHaveBeenCalledWith('/data/db-backups', { recursive: true });
|
||||
});
|
||||
|
||||
it('falls back to /backup/database only when nothing is configured', async () => {
|
||||
db.mockReturnValue({
|
||||
where: jest.fn().mockReturnThis(),
|
||||
select: jest.fn().mockResolvedValue([])
|
||||
});
|
||||
|
||||
const stop = new Error('stop after mkdir');
|
||||
const mkdirSpy = jest.spyOn(fs, 'mkdir').mockRejectedValue(stop);
|
||||
|
||||
await expect(service.backup({})).rejects.toThrow(stop.message);
|
||||
|
||||
expect(mkdirSpy).toHaveBeenCalledWith('/backup/database', { recursive: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('startScheduledBackups (#1365)', () => {
|
||||
// Same key-mismatch bug as backup(): getBackupConfig() returns
|
||||
// database_backup_*-prefixed keys, but this read `config.enabled` /
|
||||
// `config.schedule` / `config.retentionDays` — always undefined, so
|
||||
// the scheduler silently treated every install as disabled.
|
||||
it('does not start the schedule while database_backup_enabled is false', async () => {
|
||||
db.mockReturnValue({
|
||||
where: jest.fn().mockReturnThis(),
|
||||
select: jest.fn().mockResolvedValue([
|
||||
{ setting_key: 'database_backup_enabled', setting_value: 'false' }
|
||||
])
|
||||
});
|
||||
|
||||
await startScheduledBackups();
|
||||
|
||||
expect(cron.schedule).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('starts the schedule with the configured cron when database_backup_enabled is true', async () => {
|
||||
db.mockReturnValue({
|
||||
where: jest.fn().mockReturnThis(),
|
||||
select: jest.fn().mockResolvedValue([
|
||||
{ setting_key: 'database_backup_enabled', setting_value: 'true' },
|
||||
{ setting_key: 'database_backup_schedule', setting_value: JSON.stringify('0 4 * * *') }
|
||||
])
|
||||
});
|
||||
|
||||
await startScheduledBackups();
|
||||
|
||||
expect(cron.schedule).toHaveBeenCalledWith('0 4 * * *', expect.any(Function));
|
||||
});
|
||||
});
|
||||
|
||||
describe('cleanupOldBackups', () => {
|
||||
it('should delete old backup files and records', async () => {
|
||||
const oldBackups = [
|
||||
|
||||
@@ -1043,7 +1043,7 @@ function buildManifestFiles(backedUpFiles, allFiles) {
|
||||
|
||||
async function saveManifestToLocal(manifest, manifestFileName, config) {
|
||||
const manifestDir = config.backup_manifest_path
|
||||
|| path.join(config.backup_destination_path || '/backup', 'manifests');
|
||||
|| path.join(config.backup_destination_path || path.join(getStoragePath(), 'backups'), 'manifests');
|
||||
await fs.mkdir(manifestDir, { recursive: true });
|
||||
const manifestPath = path.join(manifestDir, manifestFileName);
|
||||
await backupManifest.saveManifest(manifest, manifestPath, config.backup_manifest_format || 'json');
|
||||
|
||||
@@ -296,14 +296,26 @@ class DatabaseBackupService {
|
||||
let backupRun = null;
|
||||
|
||||
try {
|
||||
// Get configuration
|
||||
// Get configuration. getBackupConfig() returns the raw
|
||||
// database_backup_*-prefixed setting keys, not the unprefixed
|
||||
// names used internally below — map them explicitly rather than
|
||||
// spreading `config` straight into the destructure, which silently
|
||||
// matched nothing and always fell through to the hardcoded
|
||||
// defaults (notably `/backup/database`, regardless of what was
|
||||
// configured).
|
||||
const config = await this.getBackupConfig();
|
||||
const {
|
||||
destinationPath = '/backup/database',
|
||||
compress = true,
|
||||
validateIntegrity = true,
|
||||
includeChecksums = true
|
||||
} = { ...config, ...options };
|
||||
} = {
|
||||
destinationPath: config.database_backup_destination_path,
|
||||
compress: config.database_backup_compress,
|
||||
validateIntegrity: config.database_backup_validate_integrity,
|
||||
includeChecksums: config.database_backup_include_checksums,
|
||||
...options
|
||||
};
|
||||
|
||||
// Create backup directory
|
||||
await fs.mkdir(destinationPath, { recursive: true });
|
||||
@@ -423,7 +435,7 @@ class DatabaseBackupService {
|
||||
logger.info(`Database backup completed: ${finalFile} (${(finalStats.size / 1024 / 1024).toFixed(2)} MB) in ${durationSeconds}s`);
|
||||
|
||||
// Send success notification if configured
|
||||
if (config.emailOnSuccess) {
|
||||
if (config.database_backup_email_on_success) {
|
||||
await this.sendBackupNotification('success', {
|
||||
duration: durationSeconds,
|
||||
size: finalStats.size,
|
||||
@@ -457,7 +469,7 @@ class DatabaseBackupService {
|
||||
|
||||
// Send failure notification
|
||||
const config = await this.getBackupConfig();
|
||||
if (config.emailOnFailure) {
|
||||
if (config.database_backup_email_on_failure) {
|
||||
await this.sendBackupNotification('failure', {
|
||||
error: error.message
|
||||
});
|
||||
@@ -686,25 +698,25 @@ async function startScheduledBackups() {
|
||||
|
||||
try {
|
||||
const config = await databaseBackupService.getBackupConfig();
|
||||
|
||||
if (!config.enabled) {
|
||||
|
||||
if (!config.database_backup_enabled) {
|
||||
logger.info('Database backup service is disabled');
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
// Stop existing schedule
|
||||
if (backupSchedule) {
|
||||
backupSchedule.stop();
|
||||
}
|
||||
|
||||
|
||||
// Default schedule: 3 AM daily (offset from file backups at 2 AM)
|
||||
const schedule = config.schedule || '0 3 * * *';
|
||||
|
||||
const schedule = config.database_backup_schedule || '0 3 * * *';
|
||||
|
||||
backupSchedule = cron.schedule(schedule, async () => {
|
||||
logger.info('Starting scheduled database backup');
|
||||
try {
|
||||
await databaseBackupService.backup();
|
||||
await databaseBackupService.cleanupOldBackups(config.retentionDays || 30);
|
||||
await databaseBackupService.cleanupOldBackups(config.database_backup_retention_days || 30);
|
||||
} catch (error) {
|
||||
logger.error('Scheduled database backup failed:', error);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user