From 4cbdf8e046c4434d25f1d2d46734b49f6b059077 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 13 Jun 2026 23:00:56 +0200 Subject: [PATCH] =?UTF-8?q?fix/m18:=20notes=20=E2=80=94=20LIVE=20dump-reva?= =?UTF-8?q?lidation=20perf=20bug=20+=20implementable=20fix=20plan=20(not?= =?UTF-8?q?=20deployed)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- FIX-M18-NOTES.md | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 FIX-M18-NOTES.md diff --git a/FIX-M18-NOTES.md b/FIX-M18-NOTES.md new file mode 100644 index 0000000..ebd1486 --- /dev/null +++ b/FIX-M18-NOTES.md @@ -0,0 +1,49 @@ +# 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) + +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.