docs(audits/backlog): preserve live-drive findings+fixspec and M18/M19 fix-plans

Relocates the 2026-06-14 live-drive findings + fixspec from the felhom-controller
repo root into documentation/audits/ (alongside the bughunt-reconcile/deep-sweep
records), and preserves the M18/M19 implementable fix-plans (from the deleted
controller fix/m18 + fix/m19 branches) into a new documentation/backlog/. Part of
the trunk-based no-branches reconciliation.
This commit is contained in:
2026-06-14 11:00:26 +02:00
parent 4c0eb2f5d4
commit 751941ca6a
5 changed files with 879 additions and 0 deletions
+49
View File
@@ -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.
+56
View File
@@ -0,0 +1,56 @@
# fix/m19-stackname-crossref — NOTES (pending review, NOT deployed)
**Verdict:** LIVE @ `controller/internal/appbackup/dbdump.go:536-551`, used at `:115` (commit `6953899`).
**Class:** correctness edge — **low real-world incidence** with the current catalog. **Escape-hatch branch**
— the clean fix injects the deployed-stack list into `appbackup` (cross-package), not forced unattended.
## The bug (verified mechanism)
`deriveStackName(containerName)` pure-suffix-strips: it splits on `-` and, if the last segment is in
`{postgres,db,mariadb,mysql,database,redis,cache}`, returns the join of the remaining parts. It never
cross-references actual deployed stack names. `DiscoverDatabases` assigns `StackName: deriveStackName(name)`
directly (line 115).
So a stack whose real name **ends** in one of those tokens is misattributed:
- a stack literally named `my-cache` → its DB container `my-cache` (or `my-cache-postgres`) derives to
`my` / `my-cache`, attributing the dump to the wrong (or a non-existent) stack.
- worse, `a-db` and `a` could collide.
## Impact
A DB dump is filed under the wrong stack name → that stack's backup/restore accounting is wrong, and the
restore-by-stack path could miss or cross-wire the dump. **Incidence is effectively zero in the current
felhom catalog** (stack slugs are `romm`, `nextcloud`, `paperless-ngx`, `immich`, `adventurelog`,
`actualbudget`, `mealie`, `vikunja`, … — none ends in a DB-role token; DB containers are `<stack>-postgres`
etc., which strip correctly). It becomes real only if a future app slug ends in a role token.
## Fix plan (implementable)
1. Thread the set of **known deployed stack names** into discovery:
`DiscoverDatabases(ctx, logger, debug, knownStacks []string)` and
`deriveStackName(containerName string, known map[string]bool)`.
Source the list from the caller in `backup.go` (it holds the `StackDataProvider` — expose/known stack
names via a lookup func to avoid importing `stacks` into `appbackup` and creating a cycle).
2. New `deriveStackName` logic:
- candidate := current suffix-strip result.
- if `known[candidate]` → use it (the suffix was a real DB-role suffix of a real stack).
- else if `known[containerName]` → the container name IS the stack (don't strip).
- else → longest `known` stack name that is a prefix of `containerName` (handles `<stack>_postgres`,
`<stack>-1`, compose-suffixed names); tie-break to the longest match.
- else → fall back to the current suffix-strip (preserve today's behaviour when the stack list is
unavailable/empty, so nothing regresses).
3. Keep a nil/empty-`known` fast path = today's behaviour (back-compat for other callers/tests).
## Test plan (regression)
- Table test for `deriveStackName` with `known = {romm, my-cache}`:
- `romm-postgres``romm` (suffix is a role; `romm` is known).
- `my-cache``my-cache` (known as-is; must NOT strip to `my`). ← fails on pre-fix code.
- `my-cache-postgres``my-cache` (strip role, result known).
- `unknown-db` with no matching known → falls back to `unknown` (today's behaviour).
## Why not tonight
Requires plumbing the deployed-stack list across the `backup``appbackup` boundary (cycle-avoidance via a
lookup func) and touching `DiscoverDatabases`'s signature + caller. Low-incidence, so not worth a risky
unattended change. The plan + tests above are complete for the supervised session.
+14
View File
@@ -0,0 +1,14 @@
# documentation/backlog/
Verified-LIVE findings with implementable fix plans that are **not yet implemented**. Preserved here
(instead of on git branches) per the trunk-based, no-branches rule — the fix itself is implemented later
**directly on `main`**, during a normal/supervised session.
- **FIX-M18-NOTES.md** — dump re-validation runs every 5 min (perf); implementable fix plan. (was on the
deleted `felhom-controller` branch `fix/m18-dump-validation-cache`.)
- **FIX-M19-NOTES.md** — `deriveStackName` misattribution edge (low-incidence correctness); fix plan.
(was on the deleted `felhom-controller` branch `fix/m19-stackname-crossref`.)
Related: the live-drive fixspec (`../audits/live-drive-fixspec-2026-06-14.md`) carries the **deferred
supervised items** F9 (HDD provisioning/guest-attach), F20-BUG2 (durable_id scheme), F20-BUG3 (async
mkfs) — to be implemented in the agent/golden supervised session.