admin
78ff991f1c
Closes R-47. No new agent coupling — MinAgent stays 0.90.0. The replay needs a running DB container, so both restore paths started the WHOLE stack first, giving the application a window to rebuild the very schema objects the dump was about to create. Measured live on 2026-07-19 (H4, DIAG-immich-restore-round2): immich-server rebuilt clip_index two seconds before the dump's CREATE INDEX, the replay aborted "already exists" under ON_ERROR_STOP=1, and immich reported schema drift. The data survived only because pg_dump emits COPY before CREATE INDEX. Both paths now open a DB-ONLY window: only the stack's database service(s) come up, the dump is replayed with the app still down, and the full start runs only after the replay exits 0. Fail-closed: a dump with no identifiable DB service refuses BEFORE the first mutation. Every exit from the window still does a best-effort full start, so a failed restore never leaves a box with a database and no application. New: appbackup.DBServiceNames (yaml.v3 services-map parse — never a line scan; immich's top-level volume keys are the decoy) sharing dbTypeForImage with DiscoverDatabases; stacks.Manager.StartStackServices (refuses an empty list — argument-less `up -d` is a full start); RedeployFromEnv split into PersistUnitRedeployConfig + its unchanged tail. StackDataProvider's RecreateStackFromUnit becomes RecreateStackDefinitionFromUnit — the hidden `up -d` inside the old name is what carried the defect on the local path. 19 new tests (ordering plus state-at-replay-time, zero-mutation fail-closed effects, replay-failure bring-up, parser decoys, empty-list refusal); three companion red-proofs run and reverted. 23/23 packages green. Not yet live-validated: STOP-1 supervised reconstitute, golden 0.153.0.
134 lines
8.8 KiB
Markdown
134 lines
8.8 KiB
Markdown
# REPORT — R-47: the DB replay must not race the app (both restore paths) · felhom-controller v0.153.0
|
||
|
||
**Date:** 2026-07-20 · **Repo:** `felhom-controller` (v0.152.0 → **v0.153.0**) · Trunk, pushed to
|
||
`main`. · **Baseline:** `main` @ `fd40b29` (clean, equal to `origin/main` at session start)
|
||
|
||
---
|
||
|
||
## 1. What was wrong
|
||
|
||
`felhom.eu/documentation/audits/DIAG-immich-restore-round2-2026-07-19.md`, finding **H4**. The
|
||
offsite reconstitution ran its designed sequence — safety dump → stop → start → replay — and the
|
||
replay aborted:
|
||
|
||
```
|
||
10:58:25 controller: replaying DB dump into immich-postgres
|
||
10:58:33 immich-server: "Reindexing clip_index" -> "Reindexed clip_index"
|
||
10:58:35 controller: ERROR relation "clip_index" already exists - exit status 3
|
||
```
|
||
|
||
`ImportDump` needs a running database container, so the code started the WHOLE stack first. That gave
|
||
the application an eight-second window to rebuild the very schema objects the dump was about to
|
||
create; under `ON_ERROR_STOP=1` the collision aborted the script. The data survived only because
|
||
`pg_dump` emits COPY before CREATE INDEX — a collision earlier in the script would have left a
|
||
genuinely half-restored database and reported it identically.
|
||
|
||
**Class defect.** The local `RestoreFromRecoveryUnit` had the same start-then-replay shape, hidden
|
||
inside `RecreateStackFromUnit` (which ended in a full `compose up -d`). Both are fixed here.
|
||
|
||
## 2. What was built
|
||
|
||
**Part 1 — the seams**
|
||
|
||
| Change | File |
|
||
|---|---|
|
||
| `dbTypeForImage` extracted from `DiscoverDatabases` (behaviour byte-equivalent) and shared | `internal/appbackup/dbdump.go`, `internal/appbackup/dbservices.go` (new) |
|
||
| `DBServiceNames(composePath)` — sorted compose SERVICE names holding a DB; yaml.v3 `services:` map parse | `internal/appbackup/dbservices.go` (new) |
|
||
| `Manager.StartStackServices(name, services)` — scoped `up -d`, **refuses an empty list** | `internal/stacks/manager.go` |
|
||
| `RedeployFromEnv` split; persist half is `PersistUnitRedeployConfig` (starts nothing) | `internal/stacks/deploy.go` |
|
||
| `StackDataProvider`: `RecreateStackFromUnit` → `RecreateStackDefinitionFromUnit` (+ `StartStackServices`) | `internal/appbackup/appdata.go` |
|
||
| Adapter: definition-only recreate + delegation | `cmd/controller/main.go` |
|
||
| `DBServiceNames` forwarder | `internal/backup/appbackup_bridge.go` |
|
||
|
||
**Part 2 — offsite** (`internal/backup/offbox_reconstitute.go`): DB services resolved from the LIVE
|
||
compose before any mutation; fail-closed refusal when a DB exists but no service is identifiable;
|
||
sequence is now **stop → files → `StartStackServices(dbServices)` → replay → `StartStack` (full) →
|
||
health wait**; both failure exits from the window do a best-effort full start.
|
||
|
||
**Part 3 — local** (`internal/backup/restore_unit.go`): DB services resolved from the UNIT's compose
|
||
(it is about to become the live one) plus `hasReplayableDump` (excludes `pre-restore-` safety dumps);
|
||
same fail-closed gate before the first mutation; sequence is now **stop → volumes →
|
||
`RecreateStackDefinitionFromUnit` → `StartStackServices` → replay → `StartStack` (full) → health
|
||
wait**, with the pre-existing `dataErr` / "completed with data errors" semantics preserved.
|
||
|
||
Untouched, as specified: `restore_db.go`, `ImportDump`, `waitDBReady`, the dump flags
|
||
(`--clean --if-exists`, `ON_ERROR_STOP=1`), `mapOffsiteRestorePaths`, the copiers, the honesty
|
||
surfaces, `IsDownState`/alerting (R-51), and the agent/hub.
|
||
|
||
## 3. Tests — 19 new, Groups A–G
|
||
|
||
| Group | Test | Result |
|
||
|---|---|---|
|
||
| A | `TestReconstituteReplaysWithOnlyTheDBServiceUp` — order **plus state-at-replay-time** | PASS |
|
||
| A | `TestReconstituteReplaysDBAndOrdersOperations` (existing, sequence assertion updated) | PASS |
|
||
| B | `TestReconstituteNoDBAppNeverStartsServicesOnly` — negative, zero scoped starts | PASS |
|
||
| C | `TestReconstituteRefusesWhenNoDBServiceIdentifiable` — zero-mutation effect | PASS |
|
||
| C | `TestRestoreFromUnitRefusesWhenNoDBServiceIdentifiable` — zero-mutation effect | PASS |
|
||
| D | `TestRestoreFromUnitReplaysWithOnlyTheDBServiceUp` | PASS |
|
||
| D | `TestRestoreFromUnitNoDumpsTakesOneFullStart` | PASS |
|
||
| D | `TestRestoreFromUnitIgnoresSafetyDumpsWhenDecidingToReplay` | PASS |
|
||
| E | `TestReconstituteReplayFailureStillBringsTheStackUp` | PASS |
|
||
| E | `TestReconstituteDBOnlyStartFailureStillBringsTheStackUp` | PASS |
|
||
| E | `TestRestoreFromUnitReplayFailureStillBringsTheStackUp` | PASS |
|
||
| F | `TestDBTypeForImage`, `TestDBServiceNames` (8 sub-cases), `TestDBServiceNames_TopLevelKeysAreNotServices`, `TestDBServiceNames_UnreadableAndUnparseableError`, `TestDiscoverAndComposeAgreeOnTheSameImages` | PASS |
|
||
| G | `TestStartStackServicesRefusesEmptyList`, `TestPersistUnitRedeployConfigPersistsWithoutStarting`, `TestPersistUnitRedeployConfigRejectsUnknownStack` | PASS |
|
||
|
||
The core assertion is deliberately not "no error": a recording provider captures whether the FULL
|
||
stack had been started at the moment the import fired. Asserting only `err == nil` passes on the
|
||
pre-fix shape — which is exactly how this shipped.
|
||
|
||
The compose-parser decoys use the catalog's REAL immich template shape (`immich_ml_cache:`,
|
||
`immich_postgres_data:` as top-level `volumes:` keys, `ghcr.io/immich-app/postgres:16-vectorchord…`
|
||
as the pin) — the exact input a line scan would misread.
|
||
|
||
### Companion red-proofs — three run, all reverted, tree clean
|
||
|
||
| # | Pre-fix shape restored | Failure observed |
|
||
|---|---|---|
|
||
| 1 | offsite: `StartStackServices` → full `StartStack` before the replay | `TestReconstituteReplaysWithOnlyTheDBServiceUp`: *"the database service was NOT started before the replay"*; `TestReconstituteReplaysDBAndOrdersOperations`: sequence `"stop,start,start"` |
|
||
| 2 | local: full `StartStack` inserted before the replay | `TestRestoreFromUnitReplaysWithOnlyTheDBServiceUp`: *"the FULL stack was already up when the replay fired — the H4 race, on the local path"* |
|
||
| 3 | both fail-closed gates deleted | both `RefusesWhenNoDBServiceIdentifiable` tests: *"expected a refusal…"* |
|
||
|
||
### Green gate
|
||
|
||
`go build ./... && go vet ./... && go test ./...` — **23/23 packages green**, exit 0
|
||
(`internal/backup` 174 s). New tests by package: backup +11, appbackup +5, stacks +3.
|
||
|
||
## 4. Deployment
|
||
|
||
Recorded on completion of Phase B — see the CHANGELOG entry for the shipped version.
|
||
|
||
## 5. NOT yet live-validated — remaining human/supervised work
|
||
|
||
- **STOP-1 (supervised, Viktor present):** prepare a full offsite restore scratch for immich through
|
||
the real endpoints, then fire `/backup/offbox/reconstitute` and verify through the system's own
|
||
surfaces — controller log showing stop → db-only up → replay rc-0 → full up, no `already exists`,
|
||
no drift, immich healthy with content visible. Timestamps to be recorded here afterwards.
|
||
- **Phase C (golden 0.153.0):** probes P1–P3 first. **P3 is load-bearing** — the drill environment is
|
||
at the vacation site and must be proven able to pull
|
||
`gitea.dooplex.hu/admin/felhom-controller:0.153.0` BEFORE any bake. If unreachable: stop and
|
||
report; change no routing/DNS/nft.
|
||
- **STOP-2 (Viktor, password-gated):** Day-0 manifest Golden → 0.153.0 (Agent 0.90.1 / MinAgent
|
||
0.90.0 unchanged — the CHANGELOG's no-coupling declaration is the authority), then floor →
|
||
v0.153.0 saved **LAST**. Watching the demo box wake during the manifest save banks the **R-23(a)**
|
||
operator-UI save→apply evidence — log the timestamps if observed.
|
||
- **Viktor's C6 customer-restore UI run** (note the empty-the-trash method).
|
||
|
||
## 6. Observations (out of scope, recorded not acted on)
|
||
|
||
- **The `pre-restore-` prefix is load-bearing in three separate places** (the replay's exact-name
|
||
match, `OffsiteScratchPair`'s dump sniff, and now `hasReplayableDump`) with no shared predicate
|
||
deciding "is this file a replay source". A fourth consumer that forgets the exclusion would arm the
|
||
DB-only window for an app that has nothing to replay. Worth a single helper at some point.
|
||
- **`reimportDBDumpsFrom`'s own `hasDump` scan does NOT exclude the safety prefix** (unchanged here —
|
||
`restore_db.go` was explicitly out of scope). Harmless today because the per-DB lookup is an exact
|
||
`<stack>-<dbtype>.sql` match, so a directory holding only safety dumps merely produces the
|
||
"no matching running DB container" WARN instead of a clean zero.
|
||
- **`dbTypeForImage` maps both `mysql` and `mariadb` to `DBTypeMariaDB`.** Pre-existing and correct
|
||
for the current catalog (the `mariadb` client speaks to both), but it is an assumption, not an
|
||
invariant, and it is now written down in one place instead of two.
|
||
- **`RedeployFromEnv` has no end-to-end test** (it shells out to compose), so the split's equivalence
|
||
is asserted on the persist half only. That is the half the split could break; the tail is
|
||
byte-identical code that was moved, not rewritten.
|
||
- **R-29a (`estimate.go` gate finding)** remains open and was not touched.
|