From a27c705e392ec1e6b8d8be945127de9ff11d5db0 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:06:50 +0200 Subject: [PATCH] fix(backup): make backup settings actually apply (#871) (stable) (#875) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(backup): make backup settings actually apply (#871) (stable) - Wire the What-to-Backup toggles into the walker: honor backup_include_thumbnails / backup_include_photos (opt-out, default ON) and accept the UI's backup_include_archives spelling for the archived gate (the engine expected _archived, so the Archives checkbox silently never worked). - Fix the 167.6 TB dashboard size: file_size_bytes is a bigint that node-postgres returns as a string, and the S3 path concatenated it onto the byte counter; coerce to Number at the source. - Compute the real next scheduled run (cron-parser) and return it as nextBackup; the UI read a field the API never sent and rendered a hardcoded 'Not scheduled'. A named schedule label now beats the stray default cron the UI always sent, which silently turned weekly schedules into daily 03:00 runs. - Never back up filesystem noise (.nfs* silly-renames, .DS_Store, Thumbs.db) and honor backup_exclude_patterns in the walker (previously rsync-only). - Remove the compression/encryption toggles from the configuration UI: no backend implementation exists, and collecting an encryption passphrase while uploading plaintext is a false promise. * fix(backup): close the review gaps in the settings wiring (stable) - The UI's backup_include_archives now beats the migration-seeded backup_include_archived: every install has the singular key seeded true, so the alias-only-when-absent lookup made unchecking Archives a no-op. - rsync destinations now receive the de-selected What-to-Backup paths and the noise filters as anchored --exclude args; previously rsync synced the whole storage root and the walker's selection only shaped the manifest, which then misreported what was actually transferred. - Escape regex metacharacters in the walker's glob matcher: '.nfs*' compiled to /^.nfs.*$/ whose leading dot matched any character, so files like anfs-photo.jpg were silently dropped from backups. - The Backup Coverage report now uses the same gate as the walker (new 'skipped-by-setting' status) instead of re-implementing it without the opt-out toggles and the archives alias. * fix(backup): make the coverage diagnostics agree with the walker - The coverage table shows the alias-aware flag value the gate actually used, instead of the seeded backup_include_archived shadowed by the UI's plural key (true next to a 'Gated off' badge). - skipped-by-setting paths are now counted in the coverage summary (backend, TS contract, summary card, EN/DE locales) so the totals reconcile again when Photos or Thumbnails is unchecked. - The form's thumbnail default now matches the backend's never-saved fallback (include): the checkbox no longer shows 'off' while thumbnails are being backed up, and saving an unrelated setting no longer flips the backup scope. (stable) * fix(backup): keep custom crons, exclude disabled rows from rsync, normalize flag display - Saving a named schedule no longer wipes the stored custom cron: the backend already prefers the label, so the cron field stays inert for named schedules and is preserved for switching back to Custom. A custom schedule now validates the 5-field expression before saving (the backend silently fell back to daily 02:00 on a blank value). - resolveExcludedBackupPaths now also returns rows disabled via include_in_default, so rsync excludes them; the enabled-only loader hid them and rsync transferred their contents anyway. - The coverage table normalizes flag values like the walker does — Boolean('false') displayed true beside a gated-off badge. (stable) --- .../backupService.configurableWalker.test.js | 199 ++++++++++++++ backend/package-lock.json | 26 +- backend/package.json | 1 + backend/src/services/backupCoverageService.js | 16 +- backend/src/services/backupService.js | 260 +++++++++++++----- .../components/admin/BackupConfiguration.tsx | 80 +----- .../components/admin/BackupCoverageCard.tsx | 10 + frontend/src/i18n/locales/de.json | 2 + frontend/src/i18n/locales/en.json | 2 + frontend/src/pages/admin/BackupManagement.tsx | 2 +- frontend/src/services/admin.service.ts | 2 + 11 files changed, 456 insertions(+), 144 deletions(-) diff --git a/backend/__tests__/integration/backupService.configurableWalker.test.js b/backend/__tests__/integration/backupService.configurableWalker.test.js index 583ab504..80c71a56 100644 --- a/backend/__tests__/integration/backupService.configurableWalker.test.js +++ b/backend/__tests__/integration/backupService.configurableWalker.test.js @@ -177,4 +177,203 @@ describe('backupService — configurable walker (backup_paths)', () => { const filesOn = await backupService.getFilesToBackup(true); expect(filesOn.map((f) => f.relativePath)).toContain('events/archived/E3/legacy.jpg'); }); + + // Issue #871 — the "What to Backup" checkboxes were stored but never read. + describe('UI opt-out toggles (issue #871)', () => { + it('unchecking Thumbnails excludes thumbnails/', async () => { + seedFile('thumbnails/E1/thumb.jpg'); + seedFile('events/active/E1/photo.jpg'); + + const files = await backupService.getFilesToBackup({ + backup_include_thumbnails: false, + }); + const rels = files.map((f) => f.relativePath); + + expect(rels).toContain('events/active/E1/photo.jpg'); + expect(rels).not.toContain('thumbnails/E1/thumb.jpg'); + }); + + it('unchecking Photos excludes events/active', async () => { + seedFile('thumbnails/E1/thumb.jpg'); + seedFile('events/active/E1/photo.jpg'); + + const files = await backupService.getFilesToBackup({ + backup_include_photos: false, + }); + const rels = files.map((f) => f.relativePath); + + expect(rels).toContain('thumbnails/E1/thumb.jpg'); + expect(rels).not.toContain('events/active/E1/photo.jpg'); + }); + + it('defaults to including everything when the keys were never saved', async () => { + seedFile('thumbnails/E1/thumb.jpg'); + seedFile('events/active/E1/photo.jpg'); + + const files = await backupService.getFilesToBackup({}); + const rels = files.map((f) => f.relativePath); + + expect(rels).toContain('thumbnails/E1/thumb.jpg'); + expect(rels).toContain('events/active/E1/photo.jpg'); + }); + + it("accepts the UI's plural backup_include_archives for the archived gate", async () => { + seedFile('events/archived/E4/archived.jpg'); + + const files = await backupService.getFilesToBackup({ + backup_include_archives: true, + }); + expect(files.map((f) => f.relativePath)).toContain('events/archived/E4/archived.jpg'); + }); + + it('the UI plural key beats the migration-seeded singular key', async () => { + // Migration seeds backup_include_archived=true on every install; the + // form only ever writes the plural key, so unchecking Archives must + // win over the stale seeded value. + seedFile('events/archived/E5/archived.jpg'); + + const files = await backupService.getFilesToBackup({ + backup_include_archived: true, // seeded default + backup_include_archives: false, // what the admin actually chose + }); + expect(files.map((f) => f.relativePath)).not.toContain('events/archived/E5/archived.jpg'); + }); + + it('rsync gets the de-selected paths and noise filters as --exclude args', async () => { + const excluded = await backupService.resolveExcludedBackupPaths({ + backup_include_thumbnails: false, + backup_include_archives: false, + }); + expect(excluded.map((r) => r.path)).toEqual( + expect.arrayContaining(['thumbnails', 'events/archived']) + ); + + const args = backupService.buildRsyncArgs( + { backup_rsync_host: 'backup.example.com', backup_rsync_path: '/srv/backups' }, + excluded.map((r) => `/${r.path}/`) + ); + const excludes = args + .map((a, i) => (a === '--exclude' ? args[i + 1] : null)) + .filter(Boolean); + expect(excludes).toEqual(expect.arrayContaining([ + '.nfs*', + '/thumbnails/', + '/events/archived/', + ])); + }); + + it('rows toggled off via include_in_default also become rsync excludes', async () => { + // The enabled-only loader hides these rows from the walker, but rsync + // syncs the whole storage root, so they must still appear as excludes. + await db('backup_paths').where('path', 'previews').update({ + include_in_default: false, + }); + + const excluded = await backupService.resolveExcludedBackupPaths({}); + expect(excluded.map((r) => r.path)).toContain('previews'); + }); + }); + + // Issue #871 — .nfs* silly-rename artifacts were uploaded to S3. + it('never backs up filesystem noise (.nfs*, .DS_Store)', async () => { + seedFile('thumbnails/E1/.nfs000000000000006600000008'); + seedFile('events/active/E1/.DS_Store'); + seedFile('events/active/E1/photo.jpg'); + + const files = await backupService.getFilesToBackup({}); + const rels = files.map((f) => f.relativePath); + + expect(rels).toContain('events/active/E1/photo.jpg'); + expect(rels.some((r) => r.includes('.nfs'))).toBe(false); + expect(rels.some((r) => r.includes('.DS_Store'))).toBe(false); + }); + + it('the walker honors backup_exclude_patterns (previously rsync-only)', async () => { + seedFile('events/active/E1/photo.jpg'); + seedFile('events/active/E1/scratch.tmp'); + + const files = await backupService.getFilesToBackup({ + backup_exclude_patterns: ['*.tmp'], + }); + const rels = files.map((f) => f.relativePath); + + expect(rels).toContain('events/active/E1/photo.jpg'); + expect(rels).not.toContain('events/active/E1/scratch.tmp'); + }); + + it('glob patterns are literal outside the star (.nfs* must not eat anfs-…)', async () => { + seedFile('events/active/E1/anfs-photo.jpg'); + seedFile('events/active/E1/notes-tmp'); + + const files = await backupService.getFilesToBackup({ + backup_exclude_patterns: ['*.tmp'], + }); + const rels = files.map((f) => f.relativePath); + + // '.nfs*' used to compile to /^.nfs.*$/ whose dot matched any char; + // '*.tmp' used to compile to /^.*.tmp$/ which also matched 'notes-tmp'. + expect(rels).toContain('events/active/E1/anfs-photo.jpg'); + expect(rels).toContain('events/active/E1/notes-tmp'); + }); + + // Issue #871 — weekly schedules silently ran daily, and the dashboard's + // "next backup" was a hardcoded "tomorrow 02:00". + describe('schedule resolution + next run (issue #871)', () => { + it('a named label beats the stray default cron the UI used to send', () => { + expect(backupService.resolveScheduleCron({ + backup_schedule: 'weekly', + backup_schedule_cron: '0 3 * * *', // old UI default, sent unconditionally + })).toBe('0 3 * * 0'); + }); + + it('custom schedules use the cron field', () => { + expect(backupService.resolveScheduleCron({ + backup_schedule: 'custom', + backup_schedule_cron: '15 5 * * 2', + })).toBe('15 5 * * 2'); + }); + + it('falls back to the default daily cron', () => { + expect(backupService.resolveScheduleCron({})).toBe('0 2 * * *'); + }); + + it('getNextScheduledRun is null when backups are disabled', () => { + expect(backupService.getNextScheduledRun(null)).toBeNull(); + expect(backupService.getNextScheduledRun({ backup_enabled: false })).toBeNull(); + }); + + it('getNextScheduledRun returns the real next weekly fire time', () => { + const iso = backupService.getNextScheduledRun({ + backup_enabled: true, + backup_schedule: 'weekly', + backup_schedule_cron: '0 3 * * *', + }); + const next = new Date(iso); + expect(Number.isNaN(next.getTime())).toBe(false); + expect(next.getTime()).toBeGreaterThan(Date.now()); + expect(next.getDay()).toBe(0); // Sunday + expect(next.getHours()).toBe(3); // 03:00 + }); + }); + + // Issue #871 — "Backup Size: 167.6 TB": file_size_bytes is a bigInteger + // column, node-postgres returns int8 as a string, and the S3 path did + // `backedUpSize += size` — string concatenation. + it('getDatabaseBackupInfo coerces file_size_bytes to a number', async () => { + await db('database_backup_runs').del(); + await db('database_backup_runs').insert({ + backup_type: 'full', + status: 'completed', + file_path: '/backups/db/dump.sql.gz', + // Simulate the PG int8-as-string driver behaviour (sqlite stores + // whatever it is handed, so the string round-trips). + file_size_bytes: '421988', + started_at: new Date().toISOString(), + completed_at: new Date().toISOString(), + }); + + const info = await backupService.getDatabaseBackupInfo(); + expect(typeof info.size).toBe('number'); + expect(info.size).toBe(421988); + }); }); diff --git a/backend/package-lock.json b/backend/package-lock.json index 45fcca62..8d53a68e 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -1,12 +1,12 @@ { "name": "picpeak-backend", - "version": "3.45.4", + "version": "3.45.5", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "picpeak-backend", - "version": "3.45.4", + "version": "3.45.5", "dependencies": { "@aws-sdk/client-s3": "^3.850.0", "@aws-sdk/lib-storage": "^3.850.0", @@ -17,6 +17,7 @@ "chokidar": "4.0.3", "cookie-parser": "^1.4.7", "cors": "^2.8.5", + "cron-parser": "^4.9.0", "dotenv": "^16.0.3", "exifr": "^7.1.3", "express": "^4.18.2", @@ -5184,6 +5185,18 @@ "node": "^14.15.0 || ^16.10.0 || >=18.0.0" } }, + "node_modules/cron-parser": { + "version": "4.9.0", + "resolved": "https://registry.npmjs.org/cron-parser/-/cron-parser-4.9.0.tgz", + "integrity": "sha512-p0SaNjrHOnQeR8/VnfGbmg9te2kfyYSQ7Sc/j/6DtPL3JQvKxmjO9TSjNFpujqV3vEYYBvNNvXSxzyksBWAx1Q==", + "license": "MIT", + "dependencies": { + "luxon": "^3.2.1" + }, + "engines": { + "node": ">=12.0.0" + } + }, "node_modules/cross-fetch": { "version": "4.1.0", "resolved": "https://registry.npmjs.org/cross-fetch/-/cross-fetch-4.1.0.tgz", @@ -8505,6 +8518,15 @@ "yallist": "^3.0.2" } }, + "node_modules/luxon": { + "version": "3.7.2", + "resolved": "https://registry.npmjs.org/luxon/-/luxon-3.7.2.tgz", + "integrity": "sha512-vtEhXh/gNjI9Yg1u4jX/0YVPMvxzHuGgCm6tC5kZyb08yjGWGnqAjGJvcXbqQR2P3MyMEFnRbpcdFS6PBcLqew==", + "license": "MIT", + "engines": { + "node": ">=12" + } + }, "node_modules/mailparser": { "version": "3.9.14", "resolved": "https://registry.npmjs.org/mailparser/-/mailparser-3.9.14.tgz", diff --git a/backend/package.json b/backend/package.json index 875303c7..173a7579 100644 --- a/backend/package.json +++ b/backend/package.json @@ -26,6 +26,7 @@ "chokidar": "4.0.3", "cookie-parser": "^1.4.7", "cors": "^2.8.5", + "cron-parser": "^4.9.0", "dotenv": "^16.0.3", "exifr": "^7.1.3", "express": "^4.18.2", diff --git a/backend/src/services/backupCoverageService.js b/backend/src/services/backupCoverageService.js index 7af24cc4..4adcd344 100644 --- a/backend/src/services/backupCoverageService.js +++ b/backend/src/services/backupCoverageService.js @@ -212,17 +212,20 @@ async function buildConfiguredPathReport(configuredRows, config) { const includedInDefault = Boolean(row.include_in_default); let featureFlagValue = null; if (row.feature_flag) { - const v = config[row.feature_flag]; - featureFlagValue = v === undefined ? null : Boolean(v); + // Alias-aware: show the value the gate actually used, not a seeded + // canonical key shadowed by the UI's spelling. Normalize like the + // walker does — Boolean('false') is true. + const v = backupService.effectiveFlagValue(row, config); + featureFlagValue = v === undefined || v === null ? null : backupService.normalizeBoolean(v); } let coverage; if (!includedInDefault) { coverage = 'skipped-by-toggle'; - } else if (row.feature_flag && featureFlagValue !== true) { - // null (unset) and explicit false both gate the path off — matches - // the walker's normalizeBoolean semantics - coverage = 'skipped-by-feature-flag'; + } else if (!backupService.backupPathIncluded(row, config)) { + // Same gate the walker uses — feature flags (incl. the UI's + // backup_include_archives alias) and the What-to-Backup opt-outs. + coverage = row.feature_flag ? 'skipped-by-feature-flag' : 'skipped-by-setting'; } else if (!stat.exists) { coverage = 'missing-on-disk'; } else { @@ -313,6 +316,7 @@ async function getCoverageReport() { willScanCount: paths.filter((p) => p.coverage === 'will-scan').length, skippedByToggleCount: paths.filter((p) => p.coverage === 'skipped-by-toggle').length, skippedByFeatureFlagCount: paths.filter((p) => p.coverage === 'skipped-by-feature-flag').length, + skippedBySettingCount: paths.filter((p) => p.coverage === 'skipped-by-setting').length, missingOnDiskCount: paths.filter((p) => p.coverage === 'missing-on-disk').length, driftCount: unconfiguredOnDisk.length, tableMissingFallbackInUse: fallback, diff --git a/backend/src/services/backupService.js b/backend/src/services/backupService.js index 6d9580dc..88db2666 100644 --- a/backend/src/services/backupService.js +++ b/backend/src/services/backupService.js @@ -7,6 +7,7 @@ const os = require('os'); const { promisify } = require('util'); const cron = require('node-cron'); +const cronParser = require('cron-parser'); const { db } = require('../database/db'); const { queueEmail } = require('./emailProcessor'); const logger = require('../utils/logger'); @@ -348,7 +349,10 @@ async function getDatabaseBackupInfoInternal() { return { type: recent.backup_type || 'unknown', backupFile: recent.file_path, - size: recent.file_size_bytes, + // file_size_bytes is a bigInteger column — node-postgres returns int8 + // as a STRING, and `backedUpSize += size` then concatenates instead of + // adding (issue #871: "167.6 TB" dashboard size). Coerce at the source. + size: Number(recent.file_size_bytes) || 0, checksum: recent.checksum, hasChanged, backupTime: recent.completed_at, @@ -389,7 +393,11 @@ async function scanDirectory(dirPath, fileList, basePath, excludePatterns = []) const isExcluded = excludePatterns.some(pattern => { if (pattern.includes('*')) { - const regex = new RegExp(`^${pattern.replace(/\*/g, '.*')}$`); + // Escape regex metacharacters before expanding the glob star — the + // raw replace turned '.nfs*' into /^.nfs.*$/ whose leading dot + // matched any character (e.g. 'anfs-photo.jpg' was excluded too). + const escaped = pattern.replace(/[.+?^${}()|[\]\\]/g, '\\$&').replace(/\*/g, '.*'); + const regex = new RegExp(`^${escaped}$`); return regex.test(entry.name); } return entry.name === pattern; @@ -437,6 +445,26 @@ const LEGACY_BACKUP_PATHS = [ { path: 'business-docs', feature_flag: null }, ]; +// "What to Backup" opt-OUT toggles written by BackupConfiguration.tsx. +// Default-ON semantics: only an explicit false excludes the path, so +// installs that never saved the backup form keep backing up everything +// (issue #871: unchecking Thumbnails had no effect because these keys +// were stored but never read). +const OPT_OUT_FLAGS = { + 'events/active': 'backup_include_photos', + 'thumbnails': 'backup_include_thumbnails', +}; + +// The UI "Archives" checkbox writes backup_include_archives (plural) while +// the feature_flag rows use backup_include_archived — accept both. +const FLAG_ALIASES = { + backup_include_archived: 'backup_include_archives', +}; + +// Filesystem noise that must never land in a backup: NFS silly-rename +// artifacts (issue #871 showed .nfs* files uploaded to S3) and OS metadata. +const DEFAULT_EXCLUDE_PATTERNS = ['.nfs*', '.DS_Store', 'Thumbs.db']; + /** * Resolve the walker's target subdirectories from `backup_paths`. * @@ -456,34 +484,84 @@ const LEGACY_BACKUP_PATHS = [ * Used to evaluate feature_flag gates. * @returns {Promise>} */ -async function resolveBackupPaths(config) { - let rows; +async function loadBackupPathRows({ includeDisabled = false } = {}) { try { if (!(await db.schema.hasTable('backup_paths'))) { logger.warn('backup_paths table missing — falling back to LEGACY_BACKUP_PATHS'); - rows = LEGACY_BACKUP_PATHS; - } else { - rows = await db('backup_paths') - .where('include_in_default', formatBoolean(true)) - .orderBy('display_order', 'asc') - .select('path', 'feature_flag'); - if (!rows.length) { - logger.warn('backup_paths has no rows with include_in_default=true — falling back to LEGACY_BACKUP_PATHS'); - rows = LEGACY_BACKUP_PATHS; - } + return LEGACY_BACKUP_PATHS; } + let query = db('backup_paths') + .orderBy('display_order', 'asc') + .select('path', 'feature_flag', 'include_in_default'); + if (!includeDisabled) { + query = query.where('include_in_default', formatBoolean(true)); + } + const rows = await query; + if (!rows.filter((r) => normalizeBoolean(r.include_in_default)).length) { + logger.warn('backup_paths has no rows with include_in_default=true — falling back to LEGACY_BACKUP_PATHS'); + return LEGACY_BACKUP_PATHS; + } + return rows; } catch (err) { logger.warn(`Failed to query backup_paths (${err.message}) — falling back to LEGACY_BACKUP_PATHS`); - rows = LEGACY_BACKUP_PATHS; + return LEGACY_BACKUP_PATHS; } +} - // Apply feature_flag gating. A row with feature_flag='backup_include_archived' - // requires config.backup_include_archived to be truthy (same semantics as - // the historical `includeArchived` parameter). +// Per-row gate. Applies the UI opt-out toggles first, then feature_flag +// gating: a row with feature_flag='backup_include_archived' requires the +// corresponding config key to be truthy (same semantics as the historical +// `includeArchived` parameter). +function backupPathIncluded(row, config) { + const optOutKey = OPT_OUT_FLAGS[row.path]; + if (optOutKey && config) { + const optOutValue = config[optOutKey]; + if (optOutValue !== undefined && optOutValue !== null && normalizeBoolean(optOutValue) === false) { + return false; + } + } + if (!row.feature_flag) return true; + let flagValue; + if (config) { + // The alias (backup_include_archives) is what the current UI writes; + // the canonical singular key is seeded true by migration on every + // install, so the UI value must take precedence or the checkbox can + // never turn the flag off. + const alias = FLAG_ALIASES[row.feature_flag]; + if (alias && config[alias] !== undefined && config[alias] !== null) { + flagValue = config[alias]; + } else { + flagValue = config[row.feature_flag]; + } + } + return normalizeBoolean(flagValue); +} + +// The raw config value the gate actually consulted for a row's feature +// flag (alias-aware) — the coverage report shows it next to the status, +// so it must not display the shadowed seeded key. +function effectiveFlagValue(row, config) { + if (!row.feature_flag || !config) return undefined; + const alias = FLAG_ALIASES[row.feature_flag]; + if (alias && config[alias] !== undefined && config[alias] !== null) { + return config[alias]; + } + return config[row.feature_flag]; +} + +async function resolveBackupPaths(config) { + return (await loadBackupPathRows()).filter((row) => backupPathIncluded(row, config)); +} + +// The rows the admin de-selected — the rsync destination needs them as +// --exclude filters because it syncs the whole storage root rather than +// the walker's file list. Includes rows with include_in_default=false, +// which the enabled-only loader would otherwise hide from rsync entirely. +async function resolveExcludedBackupPaths(config) { + const rows = await loadBackupPathRows({ includeDisabled: true }); return rows.filter((row) => { - if (!row.feature_flag) return true; - const flagValue = config ? config[row.feature_flag] : undefined; - return normalizeBoolean(flagValue); + const disabled = row.include_in_default !== undefined && !normalizeBoolean(row.include_in_default); + return disabled || !backupPathIncluded(row, config); }); } @@ -574,6 +652,14 @@ async function getFilesToBackupInternal(configOrIncludeArchived = true) { const targets = await resolveBackupPaths(config); + // backup_exclude_patterns was only honored by the rsync destination + // (as --exclude args); the local/S3 walker ignored it. Merge it with + // the always-on noise filters here so every destination agrees. + const configuredExcludes = Array.isArray(config.backup_exclude_patterns) + ? config.backup_exclude_patterns + : []; + const excludePatterns = [...new Set([...DEFAULT_EXCLUDE_PATTERNS, ...configuredExcludes])]; + for (const target of targets) { // CRM document estate is special-cased in the comment block below // because it's the most expensive omission to recover from: @@ -589,7 +675,7 @@ async function getFilesToBackupInternal(configOrIncludeArchived = true) { // those values refer to do not, leaving every CRM *_path column a // broken FK. scanDirectory short-circuits on ENOENT so installs // that never used CRM features won't error. - await scanDirectory(path.join(storagePath, target.path), files, storagePath); + await scanDirectory(path.join(storagePath, target.path), files, storagePath, excludePatterns); } return files; @@ -686,7 +772,7 @@ function validateRsyncParam(value, label) { return value; } -function buildRsyncArgs(config) { +function buildRsyncArgs(config, extraExcludes = []) { const storagePath = getStoragePath(); const host = validateRsyncParam(config.backup_rsync_host, 'host'); const remotePath = validateRsyncParam(config.backup_rsync_path, 'remote path'); @@ -713,7 +799,14 @@ function buildRsyncArgs(config) { args.push('-e', `ssh -i ${sshKey} -o StrictHostKeyChecking=no`); } - const excludePatterns = config.backup_exclude_patterns || []; + // Same noise filters as the walker, plus the de-selected backup paths + // (extraExcludes) — rsync syncs the whole storage root, so this is the + // only place the What-to-Backup selection can take effect for rsync. + const excludePatterns = [...new Set([ + ...DEFAULT_EXCLUDE_PATTERNS, + ...(Array.isArray(config.backup_exclude_patterns) ? config.backup_exclude_patterns : []), + ...extraExcludes, + ])]; excludePatterns.forEach(pattern => args.push('--exclude', pattern)); const source = `${storagePath}/`; @@ -752,7 +845,11 @@ function parseRsyncStats(output) { async function performRsyncBackup(config, files) { const { spawnAsync } = require('../utils/safeExec'); - const rsyncArgs = buildRsyncArgs(config); + // Anchored excludes for the de-selected What-to-Backup paths; rsync + // otherwise transfers the whole storage root regardless of the walker's + // file list (which only feeds manifests and file state). + const excludedPaths = await resolveExcludedBackupPaths(config); + const rsyncArgs = buildRsyncArgs(config, excludedPaths.map((row) => `/${row.path}/`)); const { stdout } = await spawnAsync('rsync', rsyncArgs); const stats = parseRsyncStats(stdout); @@ -1175,6 +1272,50 @@ async function runBackupInternal(isManual = false) { } } +// Two settings cooperate here: +// - backup_schedule — UI label like "daily" / "weekly" / "custom" +// - backup_schedule_cron — actual cron expression (custom schedules) +// Older startup code 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 * *', +}; + +function resolveScheduleCron(config) { + 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 * * *'; + const cronCandidate = readSetting('backup_schedule_cron'); + const labelCandidate = readSetting('backup_schedule'); + // A named label wins over the cron field: the UI always used to send its + // default cron ('0 3 * * *') alongside e.g. backup_schedule='weekly', which + // silently turned weekly schedules into daily ones (issue #871). The cron + // field only applies for 'custom' (or when no known label is set). + if (labelCandidate && labelCandidate.toLowerCase() !== 'custom' && NAMED_SCHEDULES[labelCandidate.toLowerCase()]) { + schedule = NAMED_SCHEDULES[labelCandidate.toLowerCase()]; + } else if (cronCandidate && isCronExpression(cronCandidate)) { + schedule = cronCandidate; + } 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; + } + return schedule; +} + async function startBackupService() { try { const config = await resolveConfigWithFallback(); @@ -1192,42 +1333,7 @@ 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 * * *'; - 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; - } + const schedule = resolveScheduleCron(config); backupJob = cron.schedule(schedule, async () => { logger.info('Starting scheduled backup'); @@ -1319,6 +1425,8 @@ async function getBackupStatus(limit = 10) { // ago looked identical to a successful one. Same "silent failure // not surfaced" class Stage A was designed to fight. const lastSuccessful = runs.find(r => r.status === 'completed') || null; + const config = await getBackupConfigInternal(); + const nextRun = getNextScheduledRun(config); // Detect zombie running rows (started >30min ago, never updated) // — these are processes that died without writing a completed_at. // Surface them so the admin can tell at a glance vs a live run. @@ -1339,7 +1447,8 @@ async function getBackupStatus(limit = 10) { recentRuns: runs, recentBackups: runs, // Alias for frontend compatibility totalBackups: runs.filter(r => r.status === 'completed').length, - nextScheduledRun: getNextScheduledRun() + nextScheduledRun: nextRun, + nextBackup: nextRun // BackupManagement.tsx reads this name }; } catch (error) { logger.error('Failed to get backup status:', error); @@ -1351,12 +1460,19 @@ async function getBackupStatus(limit = 10) { } } -function getNextScheduledRun() { - const now = new Date(); - const next = new Date(now); - next.setDate(now.getDate() + 1); - next.setHours(2, 0, 0, 0); - return next.toISOString(); +function getNextScheduledRun(config) { + // null → the UI shows "Not scheduled". Only a real, enabled schedule + // produces a date (issue #871: this used to be a hardcoded "tomorrow + // 02:00" that ignored the configured schedule entirely). + if (!config || !normalizeBoolean(config.backup_enabled)) { + return null; + } + try { + return cronParser.parseExpression(resolveScheduleCron(config)).next().toISOString(); + } catch (error) { + logger.warn(`Could not compute next backup run: ${error.message}`); + return null; + } } async function cleanupOldBackupRuns(retentionDays = 30) { @@ -1569,5 +1685,13 @@ service.getBackupStatus = getBackupStatus; service.cleanupOldBackupRuns = cleanupOldBackupRuns; service.getBackupManifest = getBackupManifest; service.validateBackupManifest = validateBackupManifest; +service.resolveBackupPaths = resolveBackupPaths; +service.resolveExcludedBackupPaths = resolveExcludedBackupPaths; +service.backupPathIncluded = backupPathIncluded; +service.effectiveFlagValue = effectiveFlagValue; +service.normalizeBoolean = normalizeBoolean; +service.buildRsyncArgs = buildRsyncArgs; +service.resolveScheduleCron = resolveScheduleCron; +service.getNextScheduledRun = getNextScheduledRun; module.exports = service; diff --git a/frontend/src/components/admin/BackupConfiguration.tsx b/frontend/src/components/admin/BackupConfiguration.tsx index 19890291..752a52f1 100644 --- a/frontend/src/components/admin/BackupConfiguration.tsx +++ b/frontend/src/components/admin/BackupConfiguration.tsx @@ -5,7 +5,6 @@ import { Server, Cloud, HardDrive, - AlertCircle, Eye, EyeOff, Wifi, @@ -102,7 +101,9 @@ export const BackupConfiguration: React.FC = ({ config backup_include_database: true, backup_include_photos: true, backup_include_archives: true, - backup_include_thumbnails: false, + // Matches the backend never-saved fallback (include everything) so the + // form does not show "off" while thumbnails are in fact being backed up. + backup_include_thumbnails: true, backup_include_temp: false, backup_compression: true, backup_encryption: false, @@ -111,8 +112,7 @@ export const BackupConfiguration: React.FC = ({ config const [showSecrets, setShowSecrets] = useState({ s3_secret_key: false, - ssh_key: false, - encryption_passphrase: false + ssh_key: false }); const [testingConnection, setTestingConnection] = useState(false); @@ -152,6 +152,15 @@ export const BackupConfiguration: React.FC = ({ config return; } + // A custom schedule needs a real 5-field cron — the backend silently + // falls back to daily 02:00 otherwise. For named schedules the stored + // cron is kept (the backend prefers the label), so switching back to + // Custom keeps the previously saved expression. + if (formData.backup_schedule === 'custom' && !/^\s*\S+(\s+\S+){4}\s*$/.test(formData.backup_schedule_cron)) { + toast.error(t('backup.configuration.messages.invalidCron', 'Please enter a valid cron expression (5 fields)')); + return; + } + onSave(formData); }; @@ -546,69 +555,6 @@ export const BackupConfiguration: React.FC = ({ config - {/* Advanced Options */} - -

{t('backup.configuration.advancedOptions.title')}

- -
- - -
- - - {formData.backup_encryption && ( -
- -
- handleChange('backup_encryption_passphrase', e.target.value)} - placeholder={t('backup.configuration.advancedOptions.encryptionPassphraseHelp')} - required={formData.backup_encryption} - /> - -
-

- - {t('backup.configuration.advancedOptions.encryptionPassphraseHelp')} -

-
- )} -
-
- - {/* Save Button */}