diff --git a/backend/src/services/__tests__/databaseBackup.test.js b/backend/src/services/__tests__/databaseBackup.test.js index 12e73972..27a9d382 100644 --- a/backend/src/services/__tests__/databaseBackup.test.js +++ b/backend/src/services/__tests__/databaseBackup.test.js @@ -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 = [ diff --git a/backend/src/services/backupService.js b/backend/src/services/backupService.js index 1731a5ba..afb6f13a 100644 --- a/backend/src/services/backupService.js +++ b/backend/src/services/backupService.js @@ -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'); diff --git a/backend/src/services/databaseBackup.js b/backend/src/services/databaseBackup.js index db9832fd..f16209bc 100644 --- a/backend/src/services/databaseBackup.js +++ b/backend/src/services/databaseBackup.js @@ -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); }