admin
98fa8b299a
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.
52 lines
3.3 KiB
Markdown
52 lines
3.3 KiB
Markdown
> **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
|
|
`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.
|