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.
59 lines
3.6 KiB
Markdown
59 lines
3.6 KiB
Markdown
> **STATUS: FIXED** in controller v0.62.0 @ `6bab68b` (2026-06-14) — implemented trunk-based on `main` per the plan below, with a regression test. This note is retained for provenance.
|
|
|
|
# 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.
|