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.
3.3 KiB
STATUS: FIXED in controller v0.62.0 @
f8afe5c(2026-06-14) — implemented trunk-based onmainper 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
settings↔appbackup 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)
→ listAllDumpFiles → ListDumpFiles 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/Error — no 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)
- Add
Size int64andModTime string(RFC3339) tosettings.DBValidationCache. - Change
ListDumpFiles(dumpDir string)→ListDumpFiles(dumpDir string, cached func(name string, size int64, mod time.Time) (DBValidationResult, bool)). Pass a plain lookup func, NOT thesettingstype —appbackupmust not importsettings(avoid an import cycle; keep appbackup leaf-like). The func returns the cachedDBValidationResult+ ok. - Before line 469: if
cached(e.Name(), info.Size(), info.ModTime())returns ok, reuse it and skipValidateDump; else validate and (caller-side) store the result keyed by name+size+modtime. - Update the bridge re-export (
backup/appbackup_bridge.go:66) and thebackup.gocall sites to build the lookup fromsettings's cache (now carrying size+modtime), and to write back fresh validations. - Keep a nil-
cachedfast path (validate-always) so other callers/tests don't have to thread it.
Test plan (regression)
- Unit test in
appbackup: callListDumpFilestwice with acachedfunc that records calls; assertValidateDumpis 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.sqlwhose 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.