Files
felhom.eu/documentation/backlog/FIX-M18-NOTES.md
T
admin 98fa8b299a docs(backlog): mark M18 + M19 FIXED in controller v0.62.0
M18 (dump re-validation perf) @ f8afe5c and M19 (deriveStackName misattribution) @
6bab68b implemented trunk-based on controller main with regression tests + deployed.
Notes retained for provenance.
2026-06-14 14:17:59 +02:00

3.3 KiB

STATUS: FIXED in controller v0.62.0 @ f8afe5c (2026-06-14) — implemented trunk-based on main per the plan below, with a regression test. This note is retained for provenance.

fix/m18-dump-validation-cache — NOTES (pending review, NOT deployed)

Verdict: LIVE @ controller/internal/appbackup/dbdump.go:469 (commit 6953899). Class: performance (not correctness/security). Escape-hatch branch — the fix crosses the settingsappbackup package boundary and changes an exported signature; not forced unattended.

The bug (verified mechanism)

ListDumpFiles(dumpDir) (dbdump.go:425) unconditionally calls f.Validation = ValidateDump(fullPath, f.DBType) (line 469) for every .sql file on every call. ValidateDump (line 320) opens the file and scans it line-by-line (bufio loop). The caller chain is backup.RefreshCache (every ~5 min, scheduler) → listAllDumpFilesListDumpFiles for every drive/stack. So every dump is fully re-read every 5 minutes, even when unchanged.

A settings.DBValidationCache type exists (settings.go:159) and is written in RunDBDumps (backup.go:447), but it has only ValidatedAt/TableCount/HasHeader/Errorno size or modtime — and ListDumpFiles never consults it. DumpFileInfo already carries Size + ModTime (dbdump.go:451-453), so the inputs for a cheap skip-check are present; they're just not used.

Impact

On large DB dumps (hundreds of MB) this is wasted disk I/O + CPU every 5 minutes. Negligible on the demo (small dumps); real on a customer with big databases. No correctness impact — validation results are the same, just recomputed.

Fix plan (implementable, low-risk once reviewed)

  1. Add Size int64 and ModTime string (RFC3339) to settings.DBValidationCache.
  2. Change ListDumpFiles(dumpDir string)ListDumpFiles(dumpDir string, cached func(name string, size int64, mod time.Time) (DBValidationResult, bool)). Pass a plain lookup func, NOT the settings type — appbackup must not import settings (avoid an import cycle; keep appbackup leaf-like). The func returns the cached DBValidationResult + ok.
  3. Before line 469: if cached(e.Name(), info.Size(), info.ModTime()) returns ok, reuse it and skip ValidateDump; else validate and (caller-side) store the result keyed by name+size+modtime.
  4. Update the bridge re-export (backup/appbackup_bridge.go:66) and the backup.go call sites to build the lookup from settings's cache (now carrying size+modtime), and to write back fresh validations.
  5. Keep a nil-cached fast path (validate-always) so other callers/tests don't have to thread it.

Test plan (regression)

  • Unit test in appbackup: call ListDumpFiles twice with a cached func that records calls; assert ValidateDump is NOT re-run for a file whose size+modtime match the cache, and IS run when modtime changes. (A spy counter on a wrapped validator, or assert via a temp .sql whose mtime is bumped.)
  • This test fails on the pre-fix code (which always validates).

Why not tonight

Crosses a package boundary + changes an exported signature with multiple call sites (bridge + backup.go), and the cache type needs new fields — too entangled to land safely unattended. Hand to the supervised session: the plan above is complete and mechanical.