From 78ff991f1cd4d5917132ab20ed995370d256ee41 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 20 Jul 2026 17:01:52 +0200 Subject: [PATCH] =?UTF-8?q?v0.153.0=20=E2=80=94=20R-47:=20the=20DB=20repla?= =?UTF-8?q?y=20no=20longer=20races=20the=20app,=20on=20BOTH=20restore=20pa?= =?UTF-8?q?ths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 80 ++++ CONTEXT.md | 33 +- REPORT.md | 226 +++++------ REUSE.md | 4 + controller/README.md | 27 +- controller/cmd/controller/main.go | 16 +- .../internal/api/backup_snapshots_test.go | 3 +- controller/internal/appbackup/appdata.go | 16 +- controller/internal/appbackup/dbdump.go | 10 +- controller/internal/appbackup/dbservices.go | 79 ++++ .../internal/appbackup/dbservices_test.go | 195 +++++++++ .../internal/backup/appbackup_bridge.go | 6 + controller/internal/backup/offbox_3a_test.go | 25 +- .../internal/backup/offbox_reconstitute.go | 44 ++- .../backup/offbox_reconstitute_test.go | 43 +- .../internal/backup/r47_replay_order_test.go | 374 ++++++++++++++++++ .../internal/backup/recovery_unit_test.go | 34 +- controller/internal/backup/restore_unit.go | 65 ++- .../internal/backup/tier2_restore_test.go | 3 +- controller/internal/backup/tier2_v2_test.go | 30 +- .../internal/backup/volume_dumps_test.go | 3 +- controller/internal/stacks/deploy.go | 33 +- controller/internal/stacks/manager.go | 38 ++ .../internal/stacks/start_services_test.go | 134 +++++++ controller/internal/web/async_restore_test.go | 3 +- 25 files changed, 1323 insertions(+), 201 deletions(-) create mode 100644 controller/internal/appbackup/dbservices.go create mode 100644 controller/internal/appbackup/dbservices_test.go create mode 100644 controller/internal/backup/r47_replay_order_test.go create mode 100644 controller/internal/stacks/start_services_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index fcebb7f..2135435 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,85 @@ ## Changelog +### v0.153.0 — the database replay no longer races the application, on BOTH restore paths (2026-07-20) + +Closes **R-47**. **No new agent coupling — MinAgent stays 0.90.0.** Nothing in this release talks to +the host agent; the whole change is inside the controller's own compose orchestration. + +**The defect, measured to the second.** On 2026-07-19 the offsite reconstitution was run deliberately +and correctly (`felhom.eu/documentation/audits/DIAG-immich-restore-round2-2026-07-19.md`, finding +**H4**). It executed 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" <- the app recreates it +10:58:35 controller: ERROR relation "clip_index" already exists - exit status 3 +``` + +The replay needs a running database container, so the code started the WHOLE stack first. That gave +immich-server an eight-second window in which to rebuild the very schema objects the dump was about +to create, and under `ON_ERROR_STOP=1` the collision aborted the script. The photos came back anyway +**by accident**: `pg_dump` emits COPY data before CREATE INDEX, so the abort landed after the rows. A +collision earlier in the script would have left a genuinely half-restored database and reported it +identically. The operation reported failure and immich then reported schema drift. + +**The fix: a DB-only window.** After the files are placed, only the stack's database service(s) come +up; the dump is replayed into them with the application still stopped; the rest of the stack starts +only once the replay has exited 0. Nothing about the replay itself changed — `--clean --if-exists` +and `ON_ERROR_STOP=1` were always correct. The bug was the window, not the flags. + +**This was a class defect and both paths carried it.** The local `RestoreFromRecoveryUnit` had the +same start-then-replay shape, hidden inside `RecreateStackFromUnit` (which ended in a full +`compose up -d`). Fixing only the offsite path would have left the identical race one button away. +Both are re-sequenced here. + +**What changed** + +- `appbackup.DBServiceNames(composePath)` names the compose SERVICE(s) whose `image:` identifies a + database — `docker compose up -d` takes service names, not container names. It is a yaml.v3 + `services:` map parse, deliberately not a line scan: immich's real template carries top-level + `immich_ml_cache:` and `immich_postgres_data:` volume keys that sit at exactly the indentation a + service name does. +- The image heuristic that `DiscoverDatabases` had inline is extracted to `dbTypeForImage` and shared + by both. That sharing is what makes the safety argument hold: a `.sql` dump can only exist because + discovery matched the running container's image, and the compose `image:` value IS that image + string — so "a dump exists" and "a service can be named" are answered by one predicate. +- `stacks.Manager.StartStackServices(name, services)` runs the scoped `up -d`. It **refuses an empty + service list**: an argument-less `up -d` is a full start, which is precisely the behaviour the + window exists to avoid, and a silent fall-through would have reintroduced the race at the one call + site that most needs it not to. +- `RedeployFromEnv` is split. Its persist half is now `PersistUnitRedeployConfig` (app.yaml, locked + fields, in-memory flags — starting nothing); `RedeployFromEnv` is that plus its unchanged + up-and-report tail, so its public behaviour is byte-identical. The split is what lets the restore + path put the DB-only window between persisting the definition and starting the app. +- `StackDataProvider.RecreateStackFromUnit` becomes `RecreateStackDefinitionFromUnit` (files + + persist, no start), and gains `StartStackServices`. The rename is deliberate: the old name promised + less than the method did, and the hidden `up -d` inside it is what carried the defect on the local + path. + +**Fail-closed, both paths.** If a `.sql` dump exists but no database service can be identified in the +compose, the restore **refuses before the first mutation** — no stop, no file overwrite, no volume +restore. The alternative would be to start everything and replay into the race. Given the shared +predicate this should be structurally unreachable; it is the belt for template drift, not an expected +path. + +**Every exit from the window still starts the app.** A failed replay, or a failed DB-only start, is +surfaced as before — but a best-effort full `StartStack` runs first. The DB-only state is a +deliberate half-started one, and leaving a customer with a running database and no application would +turn a failed restore into an outage. + +**Tests.** 19 new (Groups A–G): ordering plus **state-at-replay-time** on both paths (a recording +provider captures whether the full stack was up at the moment the import fired — asserting "no error" +would have passed on the pre-fix shape, which is how this shipped), the no-DB negatives, the +zero-mutation fail-closed effects, the replay-failure bring-up, the compose-parser decoys built from +the catalog's real immich template, and the empty-list refusal. Three companion red-proofs run and +reverted: the pre-fix full start on the offsite path, the pre-fix full start on the local path, and +deletion of both fail-closed gates — each failing on the intended assertion. 23/23 packages green. + +**Not yet live-validated.** The supervised reconstitute on the demo box (STOP-1) and Viktor's C6 +customer-restore UI run remain outstanding. + + ### v0.152.0 — Megosztás on a Mac: mDNS in the image, and the page stops giving Mac users a dead form (2026-07-20) Closes **S-3** of `felhom.eu/documentation/audits/DIAG-sharing-2026-07-20.md`, and fixes a copy diff --git a/CONTEXT.md b/CONTEXT.md index 36cdda8..4fbaad4 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,38 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-07-20 (v0.152.0 + samba 1.1.0 — mDNS for macOS; S-3 closed) +Last updated: 2026-07-20 (v0.153.0 — R-47: the DB replay no longer races the app, both paths) + +> **2026-07-20 — v0.153.0 (R-47).** Closes the H4 race on **BOTH** restore paths. The replay needs a +> running DB container, so both paths started the WHOLE stack first — giving the application a window +> to rebuild the schema objects the dump was about to create. Measured at 8 s on 2026-07-19 +> (`DIAG-immich-restore-round2-2026-07-19`): 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 +> then reported schema drift. The photos came back **by accident** — `pg_dump` emits COPY before +> CREATE INDEX, so the abort landed after the rows; a collision earlier in the script would have left +> a genuinely half-restored database, reported identically. +> +> **DECISION: the DB-only bring-up is done by compose SERVICE scoping**, not by container tricks — +> `StartStackServices(name, []string{svc})` → `compose up -d `. Every catalog template's +> dependency direction is app→db, so naming the DB starts the DB and nothing else. `docker start +> ` was never an option: `StopStack` is `compose down`, so the containers no longer exist. +> `RestartStack`/`RedeployFromEnv` are traps here — both end in a full `up -d`. +> +> **DECISION: fail-closed.** A `.sql` dump with no identifiable DB service refuses BEFORE the first +> mutation, on both paths (one Hungarian string, shared). The alternative would be to start everything +> and replay into the race. It should be structurally unreachable — `dbTypeForImage` is now shared by +> `DiscoverDatabases` and `DBServiceNames`, and a dump can only exist because discovery matched the +> container's image, which IS the compose `image:` value — so this is the belt for template drift. +> +> Enablers: `RedeployFromEnv` split into `PersistUnitRedeployConfig` (persist, starts nothing) + the +> unchanged tail; `StackDataProvider.RecreateStackFromUnit` renamed to +> `RecreateStackDefinitionFromUnit` because the old name promised less than the method did — the +> hidden `up -d` inside it is what carried the defect on the local path. `StartStackServices` REFUSES +> an empty list (argument-less `up -d` is a full start). **No agent coupling — MinAgent stays 0.90.0.** +> 19 new tests, 3 red-proofs, 23/23 green. **NOT live-validated yet:** STOP-1 supervised reconstitute, +> golden 0.153.0 bake (P3 registry-reachability probe from the vacation site is load-bearing), Viktor's +> two hub saves, and his C6 customer-restore UI run. + > **2026-07-20 — v0.152.0 + felhom-samba 1.1.0 (Megosztás on a Mac).** Closes **S-3**. **A capture > on the box overturned the earlier guess:** macOS DOES send a correct NBNS query for `<20>` and diff --git a/REPORT.md b/REPORT.md index 8dc34cc..1cd247e 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,153 +1,133 @@ -# REPORT — v0.152.0 + felhom-samba 1.1.0: mDNS for macOS (S-3), and the connect card stops offering a dead form +# 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.151.0 → **v0.152.0**) + **felhom-samba -1.0.0 → 1.1.0** · Trunk, pushed to `main`. -**Origin:** operator report — `smb://FELHOM` still failing from a Mac after v0.151.0; S-3 of -`felhom.eu/documentation/audits/DIAG-sharing-2026-07-20.md`. +**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) -## Baselines +--- -| Repo / artifact | start | end | -|---|---|---| -| felhom-controller | `5c105fb`, v0.151.0 live on 9201 | `37e12c8` (+ this docs commit), **v0.152.0** | -| felhom-samba | `1.0.0` | **`1.1.0`**, digest `sha256:1c17c094…` | -| felhom.eu | `a7d9837` | `1d1d60a` | +## 1. What was wrong -## The finding that redirected the fix - -My earlier conclusion — "macOS no longer does NetBIOS" — **was wrong**, and a packet capture on the -box disproved it. On a bare `smb://FELHOM` from the operator's Mac (`192.168.0.11`): +`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: ``` -11:18:30.222080 .11:52844 > 192.168.0.255:137 NBNS query, 50 B - name → "FELHOM" + 9 pad + suffix 0x20 (File Server Service — correct for SMB) -11:18:30.222220 .104:137 > .11:52844 NBNS response, 62 B, +140 µs - flags 0x8580 = response, AUTHORITATIVE, RCODE=0 · ANCOUNT 1 - TTL 259200 · NB_FLAGS 0x0000 (unique, B-node) · RDATA 192.168.0.104 +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 ``` -**macOS asked correctly, was answered correctly in 140 microseconds, and never opened a TCP -connection.** It retried once, was answered again, gave up. Sixteen seconds later the same Mac -queried `FELHOM.local` over mDNS and went straight to port 445. +`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. -So NetBIOS on macOS feeds legacy browsing, not `smb://` URL resolution: **the bare `smb://` -cannot be made to work from a Mac by any change on our side**, and nmbd — which the R-6/S4b spike -was right to insist on — was never the broken part. It is precisely what serves Windows. +**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. -Investigated and dismissed: the box answers **twice** per broadcast (nmbd holds `0.0.0.0:137`, -`:137` and `:137`; a broadcast lands on two). Standard Samba, and a duplicated correct -answer is still a correct answer that macOS declined to use. +## 2. What was built -### Proven matrix +**Part 1 — the seams** -| Client | Working form | Served by | -|---|---|---| -| Windows | `\\` | nmbd (+ wsdd for the Network view) | -| macOS | **`smb://.local`** | avahi/mDNS — new in 1.1.0 | -| Any | `smb://` | direct | - -## Method — spike before publish (operator's call, and it paid) - -avahi was installed by hand into the *running* container and configured, then proven from a second -machine before any image was built. The operator confirmed `smb://FELHOM.local` connects. Only then -was 1.1.0 built. The spike also chose the design: a **static avahi service file** beats smbd's own -`multicast dns register`, because `smb.conf` is bind-mounted READ-ONLY and owned by the controller's -renderer, and a static file additionally publishes `_device-info._tcp`. - -## Changes - -| File | Change | +| Change | File | |---|---| -| `controller/infra-images/samba/Dockerfile` | `avahi` + `dbus` added; `rm` of packaged service files; header documents the captured NBNS proof and that sidebar discovery is NOT claimed | -| `controller/infra-images/samba/entrypoint.sh` | templates `avahi-daemon.conf` + `_smb._tcp`/`_device-info._tcp` service file **from `FELHOM_SERVER_NAME`**; starts dbus + avahi, both **non-fatal** | -| `controller/internal/infra/infra.go` | `SambaImage` pin → `1.1.0` (`Images()` and the golden bake follow automatically) | -| `controller/internal/web/templates/sharing.html` | Mac line `smb://` → **`smb://.local`**; Windows flat name untouched | -| `controller/internal/web/sharing_connect_card_test.go` | assertions updated + new `TestSharingConnectCard_MacLineIsDotLocalNotBareName` | -| `controller/internal/infra/samba_test.go` | tag assertion derives from `SambaImage`; adds an explicit non-`:latest` assertion | -| `controller/internal/web/handler_export_upload_test.go` | async-race fix (below) | -| `CHANGELOG.md`, `controller/README.md`, `REPORT.md` | docs | +| `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` | -## Tests +**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. -**23/23 packages green, run twice with `-count=1`.** +**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. -Red-proofs, both mutated → FAILED → restored: +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. -| Mutation | Result | -|---|---| -| revert the template's Mac line to the bare `smb://{{.SMBServerName}}` | `TestSharingConnectCard_MacLineIsDotLocalNotBareName` **FAILED** on both the missing `.local` and the present bare form, for both configured names (`FELHOM`, `OTTHON`) | -| delete `os.Remove(job.partPath)` from `expireIdleUpload` | `TestFabUpload_GCAndIdleTimeout` **FAILED** — `idle-expired .part must be deleted` | +## 3. Tests — 19 new, Groups A–G -### Two test bugs found, neither a production defect +| 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 | -1. **`TestRenderSambaCompose` pinned the literal tag `1.0.0`**, so a routine image bump read as a - renderer regression. Now derives from `SambaImage`, plus a separate assertion for what actually - matters — the tag is explicit and never `:latest`. -2. **`TestFabUpload_GCAndIdleTimeout` raced.** `expireIdleUpload` nils the slot, releases the mutex, - and only *then* closes and unlinks the `.part` — so "the slot is free" does not yet mean "the file - is gone", and the test stat-ed immediately. It passed in isolation and failed in the full package - once this release's new render tests made `web` heavier. **Not caused by this change and not a - production bug** (a new upload mints a fresh random `.part`, so the gap is harmless); the test was - asserting an async post-condition synchronously. It now waits on the same 3 s deadline with the - assertion unchanged. +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. -Design gates `template_id_gate` / `emoji_gate` / `native_confirm_gate` / `offbox_rename_gate`: **OK**. +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. -## Build, publish, deploy +### Companion red-proofs — three run, all reverted, tree clean -``` -./controller/scripts/build-samba-image.sh 1.1.0 --push → sha256:1c17c09422be… -smoke test (DooPlex, DEFAULT BRIDGE — never --network host on this host, it would bind 445/5353) - → smbd · nmbd · wsdd · dbus-daemon · avahi-daemon: running [SMOKETEST.local] (all 5 up) - → host-name=SMOKETEST, allow-interfaces=eth0, _smb._tcp/445 in the service file -anonymous pull from guest 9201 (no registry creds) → OK -./build.sh 0.152.0 --push → deployed to 9201 -``` +| # | 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…"* | -Live: `felhom-controller:0.152.0 Up (healthy)` · `felhom-samba:1.1.0 Up` — the controller's reconcile -recreated the samba container onto the real image, discarding the hand-patched spike. +### Green gate -## Live validation +`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. -**Method: on-the-wire from a second machine (felhom-pve) + endpoint-level for the page.** +## 4. Deployment -``` -mDNS from felhom-pve, against the SHIPPED image: - FELHOM.local (A) -> A 192.168.0.104 (from 192.168.0.104) - _smb._tcp (browse) -> PTR FELHOM._smb._tcp.local (from 192.168.0.104) +Recorded on completion of Phase B — see the CHANGELOG entry for the shipped version. -daemons in felhom-samba:1.1.0: smbd · nmbd · wsdd · dbus-daemon · avahi-daemon: running [FELHOM.local] +## 5. NOT yet live-validated — remaining human/supervised work -GET /sharing -> smb://FELHOM.local smb://192.168.0.104 \\FELHOM -GET /sharing/status ×2 -> phase:"idle", running:true (v0.151.0 contract still holding) -``` +- **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). -Operator, from the Mac: **`smb://FELHOM.local` connects and prompts for credentials.** +## 6. Observations (out of scope, recorded not acted on) -## Still open - -- **Finder-sidebar discovery — NOT shipped, NOT claimed.** The `_smb._tcp` record is published and - answers browse queries on the wire, but FELHOM did not appear in the operator's Finder sidebar. - That window showed no Network/Bonjour section at all, which points at **Finder Settings → Sidebar - → Locations** rather than at the box — unverified either way. Next: that setting, and - `dns-sd -B _smb._tcp` on the Mac. Zero-typing discovery was the R-6 spike's original ambition and - is still not demonstrated. -- **Windows was not retested** this session. `\\FELHOM` is served by nmbd, which this release does - not touch, and the capture proves nmbd answers correctly — but no Windows client was exercised. - -## Observations — noticed, not acted on - -- **The username trap.** macOS prefills the local account name (`Viktor.Nagyfenyvesi`) in the SMB - credential dialog; the household account is `felhom`. The page states this in the password card, - but not next to the connect addresses where the customer is looking at that moment. Worth a - sentence in the connect card — deliberately not added mid-session without a design pass. -- **nmbd's duplicate answer** is harmless here but would look alarming in any future capture. Noted - in the DIAG so the next person does not re-investigate it. -- **The spike left avahi installed by hand** in the old container. It was discarded when the - reconcile recreated the container on 1.1.0 — worth remembering that hand-patching a *managed* - infra container is always temporary by construction, which is a safety property, not a limitation. -- **`dns-sd`/`avahi-browse` were not available** anywhere on the box side, so both the NBNS and mDNS - verifications were done with small hand-written UDP probes run from felhom-pve. That turned out to - be the more valuable method anyway: querying from a *second machine* is what distinguishes "the - daemon answers itself" from "the daemon answers the network", and it is what proved the box - innocent before any code changed. +- **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 + `-.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. diff --git a/REUSE.md b/REUSE.md index 34a8681..519c29b 100644 --- a/REUSE.md +++ b/REUSE.md @@ -72,6 +72,9 @@ |---|---|---|---|---| | `Manager.DeployStack` | controller/internal/stacks/deploy.go | `(req DeployRequest) (string, error)` | Full deploy flow | Sets in-memory `Deployed` BEFORE compose up (slow-pull race), reverts on failure | | `Manager.RedeployFromEnv` | controller/internal/stacks/deploy.go | `(name, env map[string]string) error` | Re-up with changed env (migration flip, config edits) | `compose up -d`, never `restart` (restart won't pick up images/env) | +| `Manager.PersistUnitRedeployConfig` (R-47, v0.153.0) | controller/internal/stacks/deploy.go | `(name, env map[string]string) error` | the PERSIST half of `RedeployFromEnv` — app.yaml + locked fields + in-memory flags, **starts nothing** | **TRAP: the restore paths must use THIS, never `RedeployFromEnv`.** RedeployFromEnv ends in a full `up -d`, which before the replay IS the H4 race. RedeployFromEnv is now literally this + the unchanged up-and-report tail | +| `Manager.StartStackServices` (R-47, v0.153.0) | controller/internal/stacks/manager.go | `(name string, services []string) error` | scoped `compose up -d ...` — the DB-only window a dump is replayed in | **REFUSES an empty list** (argument-less `up -d` is a FULL start — the one silent fall-through that would reintroduce the race). No `logPostStartStatus`: the app containers are absent on purpose. Never `RestartStack` here — it is a full up in disguise | +| `appbackup.DBServiceNames` / `dbTypeForImage` (R-47, v0.153.0) | controller/internal/appbackup/dbservices.go | `(composePath string) ([]string, error)` | naming the compose SERVICE(s) holding a database, sorted | yaml.v3 `services:` MAP parse — **never a line scan** (immich's top-level `immich_ml_cache:` / `immich_postgres_data:` volume keys look exactly like services). `dbTypeForImage` is shared with `DiscoverDatabases`, which is what makes "a dump exists ⇒ a service can be named" hold. An error means CANNOT-TELL, never "no database" — callers refuse when a dump exists | | `Manager.StartStack/StopStack/RestartStack/UpdateStack` | controller/internal/stacks/manager.go | `(name string) error` | Lifecycle | Protected stacks refuse stop; all funnel through composeExec | | `Manager.DeleteStack` / `RemoveStack` | controller/internal/stacks/delete.go | `(name, removeHDDData[, backupPaths])` | THE guarded removal paths | Orphan/protected/deploying/running checks + ProtectedHDDPaths filter before any RemoveAll | | `resolveContainerState` / `aggregateState` | controller/internal/stacks/manager.go | `(dockerState, dockerStatus)` / `([]ContainerInfo)` | State classification | `.State` says "running" even when unhealthy — `.Status` parse is the fix | @@ -213,6 +216,7 @@ | `Manager.offboxFullPlaceCopier` + `SetOffboxFullPlaceCopier` (R-43) | controller/internal/backup/offbox_reconstitute.go | nil → `rsyncRestoreOverwrite` (`-a --itemize-changes`; **no** `--ignore-existing`, **no** `--delete`) | **TRAP: do NOT reuse `offboxPlaceCopier` here.** The two copiers have OPPOSITE semantics for an existing file — `--ignore-existing` is exactly what a full restore must not do, and conflating them is how a missing-only merge came to be labelled a restore. Never `rsyncMirror` (`--delete`) in any restore direction | | `Manager.safetyDumpFn` + `SetSafetyDumpFn` (R-43) | controller/internal/backup/offbox_reconstitute.go | nil → `DumpOne` | the pre-restore undo. Invariant: the `pre-restore-`-prefixed dump must be verified ON DISK before anything is stopped/overwritten/replayed; failure ⇒ refuse with zero changes. Red-proof requires removing BOTH guards (the `err != nil` return and the `os.Stat`) — removing one leaves the other holding | | `reimportDBDumpsFrom(ctx, stack, dumpDir)` | controller/internal/backup/restore_db.go | explicit-dir sibling of `reimportDBDumps` (which passes `AppDBDumpPath`) | offsite reconstitution replays from the SCRATCH unit: the live unit is deliberately never overwritten, so replaying from it would replay the current DB over itself and restore nothing | +| The DB-only replay window (R-47, v0.153.0) | controller/internal/backup/{offbox_reconstitute,restore_unit}.go | both restore paths: stop → place/volumes → `StartStackServices(dbServices)` → replay → `StartStack` (full) | **THE ordering invariant.** Replaying while the whole stack is up lets the app's own schema management race the dump — measured at 2 s on 2026-07-19 (H4), replay aborted `already exists`. Fail-closed: a dump with NO identifiable DB service refuses BEFORE the first mutation. Every exit from the window (replay error, DB-only start error) MUST still do a best-effort full start, or a failed restore becomes an outage. `hasReplayableDump` excludes `pre-restore-` safety dumps — counting them would arm the window for an app with nothing to replay | | `Manager.OffsiteScratchPair` / `OffsitePairInfo` | controller/internal/backup/offbox_reconstitute.go | reads the restored scratch unit's manifest (`offsite_run_id` / `dumps_at`) + the R-44 sniff | the confirm-dialog honesty surface. All warn-level: a pre-v0.148 (unstamped) pair and an empty-looking dump are SURFACED, never blocked — a false positive that refused a legitimate restore would be worse than the skew | | `appbackup.DumpValidation.LooksEmpty` (R-44 sniff) | controller/internal/appbackup/dbdump.go | computed in ValidateDump's existing single pass; `userTableNames` is EXACT-match | size and table count are both useless as emptiness heuristics (the 2026-07-19 dump: 52MB, 60+ tables, zero users — all geodata). **TRAP: never widen to a substring match on "user"** — it would flag `user_metadata` / `album_user` / `user_audit` on every healthy single-user box. A row wider than the read buffer still counts as a row | | `report.SetPendingControllerLog` / `SetControllerLogSource` | controller/internal/report/selftail.go | ACK-armed consume-once self-log pull (the logtail.go shape) | selftail_test.go; source = `logBuffer.Lines`, wired once in main.go | diff --git a/controller/README.md b/controller/README.md index 0a39a22..554e709 100644 --- a/controller/README.md +++ b/controller/README.md @@ -307,14 +307,24 @@ Each app can define rich metadata in `.felhom.yml`: - **Offsite reconstitution (v0.148.0, R-43 — `offbox_reconstitute.go`):** the leg that was missing. `ReconstituteFromOffsite` (`/backup/offbox/reconstitute`, „Teljes visszaállítás (fájlok + adatbázis)") makes the live app equal to the chosen snapshot: **safety dump → stop → files - overwritten (`rsyncRestoreOverwrite`: no `--ignore-existing`, no `--delete`) → start → the - snapshot's dump replayed (`reimportDBDumpsFrom`, reading the SCRATCH unit) → health wait**. + overwritten (`rsyncRestoreOverwrite`: no `--ignore-existing`, no `--delete`) → the DATABASE + SERVICE ONLY started (`StartStackServices`, v0.153.0) → the snapshot's dump replayed + (`reimportDBDumpsFrom`, reading the SCRATCH unit) → the full stack started → health wait**. Two invariants: nothing is ever deleted (post-snapshot files survive as extras), and the `pre-restore-` safety dump is verified on disk BEFORE anything is stopped or overwritten — if it cannot be taken the operation refuses with zero changes. Safety dumps appear in `ListDumpFiles` (they are the undo). The live recovery unit is still never overwritten, which is why the replay source is the scratch. Honesty surfaces (`OffsiteScratchPair`): dump age, an unstamped-pair warning, and the R-44 empty-dump sniff — all warn-level, none of them gates. + - **The DB-only replay window (v0.153.0, R-47).** Until v0.153.0 the whole stack was started before + the replay, so the application's own schema management raced the dump: measured live on + 2026-07-19 (H4), immich-server rebuilt `clip_index` two seconds before the dump's `CREATE INDEX` + and the replay aborted `already exists` under `ON_ERROR_STOP=1`. The DB service is now brought up + alone (`appbackup.DBServiceNames` reads the LIVE compose's `services:` map to name it), the dump + is replayed with the app still down, and only then does the full start run. **Fail-closed:** a + dump with no identifiable DB service refuses before the first mutation. Every exit from the window + — replay failure, DB-only start failure — still does a best-effort full start, so a failed restore + never leaves the box with a database and no application. The `/apps/{slug}` page renders hero section, screenshots, setup guide, and optional config form. @@ -595,7 +605,7 @@ backups/primary// **Resettable secrets (O4, v0.99.0):** an unrecoverable resettable secret (DB password etc.) gets a **generated replacement** from its catalog `generate` spec (`stacks.GenerateSecretForField` via the `backup.SetSecretGenerator` seam) instead of redeploying blank (which failed compose-up); the new - value persists encrypted through the normal `RecreateStackFromUnit` → `SaveAppConfig` path. Fields + value persists encrypted through the normal `RecreateStackDefinitionFromUnit` → `SaveAppConfig` path. Fields with no `generate` spec still proceed with a loud "may fail to start" WARN. Residual case: a restored volume tar carrying the OLD internal credential hash may still need a manual in-DB reset. - Helpers: `RecoveryUnitPath` / `RecoveryUnitComposePath` / `RecoveryUnitManifestPath` @@ -604,9 +614,14 @@ backups/primary// env comes from `StackDataProvider.GetStackRecoveryInfo` (excludes secret-named + encrypted values, so the capture never touches a secret). `data_key` fields are marked in `.felhom.yml` (`DeployField.DataKey`). -- **Restore replays the DB dump (F17, v0.61.0).** `RestoreFromRecoveryUnit` (and the `RestoreApp` - fallback) stops the app → restores named-volume tars → recreates the compose definition + redeploys - with the recovered env → **replays each `db-dumps/*.sql` into the now-running DB** via +- **Restore replays the DB dump (F17, v0.61.0; re-sequenced v0.153.0, R-47).** `RestoreFromRecoveryUnit` + (and the `RestoreApp` fallback) stops the app → restores named-volume tars → recreates the compose + definition and persists the recovered env (`RecreateStackDefinitionFromUnit` — **starts nothing**) + → starts the DATABASE SERVICE ONLY (`StartStackServices`, named from the unit's compose) → + **replays each `db-dumps/*.sql` into that DB** → starts the full stack → health wait. Before + v0.153.0 `RecreateStackFromUnit` ended in a full `compose up -d`, so this path carried the same + H4 race as the offsite one (see the reconstitution section above), with the same fail-closed rule + and the same guarantee that every exit still brings the app back up. The replay itself uses `backup.reimportDBDumps` → `appbackup.ImportDump` (psql / mariadb client, using the live container's own discovered credentials). The DB replay runs AFTER the volume restore, so the **logical SQL dump wins** over any volume-tar copy of the database (the dumps use DROP/CREATE — `pg_dump --clean --if-exists`, diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index e96373f..85c410f 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1328,10 +1328,11 @@ func (a *stackAdapter) RecoverStackSecrets(name string, names []string) map[stri return out } -// RecreateStackFromUnit restores the app definition from the unit's compose dir into the stack dir, -// then redeploys with the reconstructed full env (re-pulling the pinned image). Secrets in fullEnv were -// recovered from the guest, never regenerated. -func (a *stackAdapter) RecreateStackFromUnit(name, composeSrcDir string, fullEnv map[string]string) error { +// RecreateStackDefinitionFromUnit restores the app definition from the unit's compose dir into the +// stack dir and persists app.yaml from the reconstructed full env. Secrets in fullEnv were recovered +// from the guest, never regenerated. It starts NOTHING — the restore flow brings the database service +// up alone for the dump replay and only then starts the whole stack (R-47). +func (a *stackAdapter) RecreateStackDefinitionFromUnit(name, composeSrcDir string, fullEnv map[string]string) error { s, ok := a.mgr.GetStack(name) if !ok { return fmt.Errorf("stack %q not found", name) @@ -1347,7 +1348,12 @@ func (a *stackAdapter) RecreateStackFromUnit(name, composeSrcDir string, fullEnv return fmt.Errorf("restoring %s from unit: %w", fname, err) } } - return a.mgr.RedeployFromEnv(name, fullEnv) + return a.mgr.PersistUnitRedeployConfig(name, fullEnv) +} + +// StartStackServices brings up only the named compose services (the DB-only replay window, R-47). +func (a *stackAdapter) StartStackServices(name string, services []string) error { + return a.mgr.StartStackServices(name, services) } // RefreshAndIsRunning forces a docker ps scan before checking state. diff --git a/controller/internal/api/backup_snapshots_test.go b/controller/internal/api/backup_snapshots_test.go index e918dcd..b6bff93 100644 --- a/controller/internal/api/backup_snapshots_test.go +++ b/controller/internal/api/backup_snapshots_test.go @@ -38,9 +38,10 @@ func (p *snapshotsStubProvider) GetStackClassifiedBinds(string) ([]backup.Classi return nil, false } func (p *snapshotsStubProvider) RecoverStackSecrets(string, []string) map[string]string { return nil } -func (p *snapshotsStubProvider) RecreateStackFromUnit(string, string, map[string]string) error { +func (p *snapshotsStubProvider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { return nil } +func (p *snapshotsStubProvider) StartStackServices(string, []string) error { return nil } // newSnapshotsRouter wires a Router with a real backup.Manager over a tempdir drive. func newSnapshotsRouter(t *testing.T) (*Router, string) { diff --git a/controller/internal/appbackup/appdata.go b/controller/internal/appbackup/appdata.go index 67adae7..c5e54cb 100644 --- a/controller/internal/appbackup/appdata.go +++ b/controller/internal/appbackup/appdata.go @@ -39,10 +39,18 @@ type StackDataProvider interface { // fail-closed gate decides what to do. The unit is never the source of secrets. RecoverStackSecrets(name string, names []string) map[string]string - // RecreateStackFromUnit restores an app's definition from the unit's compose dir into the stack - // dir, writes app.yaml from fullEnv (encrypting secret fields), and (re-)deploys it via - // `docker compose up -d`, which re-pulls the pinned image. Secrets are NEVER regenerated. - RecreateStackFromUnit(name, composeSrcDir string, fullEnv map[string]string) error + // RecreateStackDefinitionFromUnit restores an app's DEFINITION from the unit's compose dir into + // the stack dir and writes app.yaml from fullEnv (encrypting secret fields). Secrets are NEVER + // regenerated. It starts NOTHING: the caller owns the bring-up order, because a DB-bearing app + // must have its database service started alone for the dump replay (R-47). It was + // `RecreateStackFromUnit` until v0.153.0 and ended in a full `docker compose up -d` — that full + // start before the replay IS the H4 race. + RecreateStackDefinitionFromUnit(name, composeSrcDir string, fullEnv map[string]string) error + + // StartStackServices brings up ONLY the named compose services, leaving the rest of the stack + // down — the DB-only window in which a dump is replayed without the application racing it. + // Implementations must REFUSE an empty list (an argument-less `up -d` is a full start). + StartStackServices(name string, services []string) error // GetStackClassifiedBinds returns the app's backup-classified compose binds + whether it carries a // (valid) backup block (Task 2, referential coupling). INERT — no tier consumes it yet; wired now diff --git a/controller/internal/appbackup/dbdump.go b/controller/internal/appbackup/dbdump.go index 9fbf6ec..81a7607 100644 --- a/controller/internal/appbackup/dbdump.go +++ b/controller/internal/appbackup/dbdump.go @@ -115,12 +115,10 @@ func DiscoverDatabases(ctx context.Context, logger *log.Logger, debug bool, know id, name, image := parts[0], parts[1], strings.ToLower(parts[2]) - var dbType DBType - if strings.Contains(image, "postgres") { - dbType = DBTypePostgres - } else if strings.Contains(image, "mariadb") || strings.Contains(image, "mysql") { - dbType = DBTypeMariaDB - } else { + // R-47: the same predicate that DBServiceNames applies to compose `image:` values, so a dump + // that exists is always attributable to a startable service (see dbservices.go). + dbType, isDB := dbTypeForImage(image) + if !isDB { if debug { logger.Printf("[DEBUG] DiscoverDatabases: skipping container %s (image=%s, not a database)", name, image) } diff --git a/controller/internal/appbackup/dbservices.go b/controller/internal/appbackup/dbservices.go new file mode 100644 index 0000000..1218318 --- /dev/null +++ b/controller/internal/appbackup/dbservices.go @@ -0,0 +1,79 @@ +package appbackup + +import ( + "fmt" + "os" + "sort" + "strings" + + "gopkg.in/yaml.v3" +) + +// R-47 — naming the database SERVICE, not just the running container. +// +// A dump replay must never race the application's own schema management. Proven live on 2026-07-19 +// (DIAG-immich-restore-round2-2026-07-19, H4): the reconstitution started the whole stack before +// replaying, immich-server rebuilt `clip_index` two seconds before the dump's own CREATE INDEX, and +// the replay aborted `already exists` under ON_ERROR_STOP=1 — leaving a half-applied schema that the +// app itself then reported as drift. The fix is to bring up ONLY the database service(s) for the +// replay, which requires knowing their compose SERVICE names (docker `up -d ` takes service +// names, not container names). +// +// The symmetry that makes this safe: a `.sql` dump can only exist because DiscoverDatabases matched +// the running container's image string, and the compose `image:` value IS that image string. So the +// same predicate — dbTypeForImage — decides both "is there a dump" and "which service holds it". + +// dbTypeForImage maps a container/compose image reference to the database engine the backup code +// supports, or ok=false for anything else (redis/valkey/app images — never started in the DB-only +// phase). Extracted from DiscoverDatabases so the discovery heuristic and the compose heuristic can +// never drift apart; behaviour is byte-equivalent to the inline form it replaced. +func dbTypeForImage(image string) (DBType, bool) { + img := strings.ToLower(image) + switch { + case strings.Contains(img, "postgres"): + return DBTypePostgres, true + case strings.Contains(img, "mariadb"), strings.Contains(img, "mysql"): + return DBTypeMariaDB, true + } + return DBType(""), false +} + +// composeServicesDoc is the minimal view of a compose file needed here: the `services:` MAP and each +// service's `image:`. Deliberately a real YAML parse and not a line scan — a top-level `volumes:` +// block (immich's `immich_ml_cache:`) has exactly the shape a naive scan misreads as a service, and +// starting a phantom service, or missing the real one, both land in the wrong branch. +type composeServicesDoc struct { + Services map[string]struct { + Image string `yaml:"image"` + } `yaml:"services"` +} + +// DBServiceNames returns the sorted compose SERVICE names in composePath whose `image:` identifies a +// supported database engine — the exact argument list for `docker compose up -d ...`. +// +// A file with no (or an empty) `services:` key returns (nil, nil): an app with no identifiable DB +// service is a legitimate, common case and the caller decides what it means. An unreadable or +// unparseable file returns an error, because "cannot tell" must never silently read as "no database" +// — the callers turn that into a refusal when a dump exists. +// +// Image values are matched literally. Catalog templates pin their images literally (enforced since +// Campaign 7), so an interpolated `${...}` image simply does not match and lands in the caller's +// fail-closed branch by design, rather than being guessed at. +func DBServiceNames(composePath string) ([]string, error) { + data, err := os.ReadFile(composePath) + if err != nil { + return nil, fmt.Errorf("reading compose file: %w", err) + } + var doc composeServicesDoc + if err := yaml.Unmarshal(data, &doc); err != nil { + return nil, fmt.Errorf("parsing compose file %s: %w", composePath, err) + } + var names []string + for name, svc := range doc.Services { + if _, ok := dbTypeForImage(svc.Image); ok { + names = append(names, name) + } + } + sort.Strings(names) + return names, nil +} diff --git a/controller/internal/appbackup/dbservices_test.go b/controller/internal/appbackup/dbservices_test.go new file mode 100644 index 0000000..557581d --- /dev/null +++ b/controller/internal/appbackup/dbservices_test.go @@ -0,0 +1,195 @@ +package appbackup + +import ( + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) + +// R-47 (v0.153.0) — the DB-service resolver. +// +// These exist because a dump replay that starts the WHOLE stack races the application's own schema +// management: proven live on 2026-07-19 (DIAG-immich-restore-round2-2026-07-19, H4) when +// immich-server rebuilt `clip_index` two seconds before the dump's CREATE INDEX and the replay +// aborted `already exists`. Closing that window means bringing up ONLY the database service, which +// means naming it correctly — every case below is a way of naming it wrongly. + +// writeCompose drops a compose file in a temp dir and returns its path. +func writeCompose(t *testing.T, body string) string { + t.Helper() + p := filepath.Join(t.TempDir(), "docker-compose.yml") + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +// TestDBTypeForImage pins the shared heuristic. It is the SAME predicate DiscoverDatabases applies to +// a running container's image, which is what makes "a dump exists ⇒ a service can be named" hold: +// the compose `image:` value IS the container's image string. The table reproduces the inline form +// this function replaced, byte for byte, including the redis/valkey negatives that must never be +// started in the DB-only window. +func TestDBTypeForImage(t *testing.T) { + cases := []struct { + image string + want DBType + ok bool + }{ + {"docker.io/library/postgres:16-alpine", DBTypePostgres, true}, + // immich's real pin — a vector-extended postgres whose REPO segment carries the substring. + {"ghcr.io/immich-app/postgres:16-vectorchord0.4.3-pgvectors0.2.0", DBTypePostgres, true}, + {"postgres", DBTypePostgres, true}, + {"POSTGRES:16", DBTypePostgres, true}, // the discovery path lowercases; so does this + {"mariadb:11", DBTypeMariaDB, true}, + {"mysql:8.4", DBTypeMariaDB, true}, + {"docker.io/library/MySQL:8", DBTypeMariaDB, true}, + {"redis:7-alpine", "", false}, + {"valkey/valkey:8", "", false}, + {"ghcr.io/immich-app/immich-server:v1.119.0", "", false}, + {"", "", false}, + } + for _, c := range cases { + got, ok := dbTypeForImage(c.image) + if ok != c.ok || (ok && got != c.want) { + t.Errorf("dbTypeForImage(%q) = (%q, %v), want (%q, %v)", c.image, got, ok, c.want, c.ok) + } + } +} + +func TestDBServiceNames(t *testing.T) { + cases := []struct { + name string + body string + want []string + }{ + { + name: "postgres service is named", + body: "services:\n app:\n image: ghcr.io/x/app:1\n database:\n image: postgres:16\n", + want: []string{"database"}, + }, + { + name: "mariadb service is named", + body: "services:\n db:\n image: mariadb:11\n web:\n image: nextcloud:30\n", + want: []string{"db"}, + }, + { + name: "mysql service is named", + body: "services:\n mysql:\n image: mysql:8.4\n", + want: []string{"mysql"}, + }, + { + name: "redis-only app has no database service", + body: "services:\n app:\n image: ghcr.io/x/app:1\n redis:\n image: redis:7-alpine\n", + want: nil, + }, + { + name: "multiple databases are returned SORTED (one up -d carries them all)", + body: "services:\n zdb:\n image: postgres:16\n adb:\n image: mariadb:11\n app:\n image: x:1\n", + want: []string{"adb", "zdb"}, + }, + { + name: "no services key at all", + body: "volumes:\n data:\n", + want: nil, + }, + { + name: "empty services map", + body: "services:\n", + want: nil, + }, + { + name: "an interpolated image is not guessed at", + body: "services:\n db:\n image: ${DB_IMAGE}\n", + want: nil, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := DBServiceNames(writeCompose(t, c.body)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !reflect.DeepEqual(got, c.want) { + t.Errorf("DBServiceNames = %v, want %v", got, c.want) + } + }) + } +} + +// TestDBServiceNames_TopLevelKeysAreNotServices is the decoy test, and the reason this is a YAML +// parse rather than a line scan. immich's real compose carries a top-level `volumes:` block whose +// entry (`immich_ml_cache:`) sits at exactly the indentation a service name does, and a top-level +// `networks:` block does the same. A scanner that collected "indented keys followed by image-ish +// lines" would either invent a service that `docker compose up -d` cannot start, or — worse — match +// the wrong one and leave the real database down while the app came up around the replay. +func TestDBServiceNames_TopLevelKeysAreNotServices(t *testing.T) { + // The service/volume/network names and the image pins are the catalog's real immich template. + // `immich_postgres_data` is the trap made concrete: a top-level VOLUME key whose name contains + // "postgres" and which no `up -d` could ever start. + body := `services: + immich-server: + image: ghcr.io/immich-app/immich-server:v3.0.3 + immich-machine-learning: + image: ghcr.io/immich-app/immich-machine-learning:v3.0.3 + immich-postgres: + image: ghcr.io/immich-app/postgres:16-vectorchord0.4.3-pgvectors0.2.0 + immich-redis: + image: redis:7-alpine +volumes: + immich_ml_cache: + immich_postgres_data: + immich_redis_data: +networks: + traefik-public: + external: true + immich-internal: +` + got, err := DBServiceNames(writeCompose(t, body)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !reflect.DeepEqual(got, []string{"immich-postgres"}) { + t.Fatalf("DBServiceNames = %v, want [immich-postgres] — a top-level volume/network key was mistaken for a service", got) + } +} + +// TestDBServiceNames_UnreadableAndUnparseableError proves the fail-closed direction: "cannot tell" +// must surface as an ERROR, never as the empty (= "this app has no database") answer. The callers +// turn an empty result into a refusal only when a dump exists; if a read failure silently produced +// the same empty slice for an app with no dump, a genuinely broken compose would flow on unnoticed. +func TestDBServiceNames_UnreadableAndUnparseableError(t *testing.T) { + if _, err := DBServiceNames(filepath.Join(t.TempDir(), "nope.yml")); err == nil { + t.Fatal("a missing compose file must be an error, not an empty service list") + } + // Valid YAML scalar where a map is required, plus outright broken YAML. + if _, err := DBServiceNames(writeCompose(t, "services: [1, 2, 3\n broken")); err == nil { + t.Fatal("an unparseable compose file must be an error, not an empty service list") + } +} + +// TestDiscoverAndComposeAgreeOnTheSameImages is the SYMMETRY guard: whatever image string makes +// DiscoverDatabases produce a dump must also make DBServiceNames name a service. They now share one +// predicate; this asserts the property that sharing is FOR, so a future edit to either side that +// breaks it fails here rather than in a customer's restore. +func TestDiscoverAndComposeAgreeOnTheSameImages(t *testing.T) { + images := []string{"postgres:16", "mariadb:11", "mysql:8.4", "redis:7", "ghcr.io/x/app:1"} + var body strings.Builder + body.WriteString("services:\n") + var wantDB []string + for i, img := range images { + svc := string(rune('a' + i)) + body.WriteString(" " + svc + ":\n image: " + img + "\n") + if _, ok := dbTypeForImage(img); ok { + wantDB = append(wantDB, svc) + } + } + got, err := DBServiceNames(writeCompose(t, body.String())) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(got, wantDB) { + t.Fatalf("compose resolver named %v but the discovery predicate says %v — the two sides have drifted", got, wantDB) + } +} diff --git a/controller/internal/backup/appbackup_bridge.go b/controller/internal/backup/appbackup_bridge.go index ba9c18c..bbe3ae4 100644 --- a/controller/internal/backup/appbackup_bridge.go +++ b/controller/internal/backup/appbackup_bridge.go @@ -100,6 +100,12 @@ func ParseComposeImages(composePath string) []string { return appbackup.ParseComposeImages(composePath) } +// DBServiceNames forwards to appbackup.DBServiceNames — the compose SERVICE names holding a database, +// i.e. the argument list for the DB-only bring-up both restore paths use before a dump replay (R-47). +func DBServiceNames(composePath string) ([]string, error) { + return appbackup.DBServiceNames(composePath) +} + // humanizeBytes forwards to appbackup.HumanizeBytes; kept unexported so the // many in-package call sites (backup.go, crossdrive.go, restore code) need no edit. func humanizeBytes(b int64) string { diff --git a/controller/internal/backup/offbox_3a_test.go b/controller/internal/backup/offbox_3a_test.go index 35afdb0..a02f4fa 100644 --- a/controller/internal/backup/offbox_3a_test.go +++ b/controller/internal/backup/offbox_3a_test.go @@ -25,17 +25,22 @@ type offbox3aProvider struct { has map[string]bool } -func (p *offbox3aProvider) GetStackComposePath(string) (string, bool) { return "", false } -func (p *offbox3aProvider) ListDeployedStacks() []StackSummary { return nil } -func (p *offbox3aProvider) GetStackHDDMounts(string) []string { return nil } -func (p *offbox3aProvider) GetStackHDDPath(n string) string { return p.hdd[n] } -func (p *offbox3aProvider) GetDockerVolumes(string) []string { return nil } -func (p *offbox3aProvider) StopStack(string) error { return nil } -func (p *offbox3aProvider) StartStack(string) error { return nil } -func (p *offbox3aProvider) RefreshAndIsRunning(string) bool { return false } -func (p *offbox3aProvider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { return RecoveryInfo{}, false } +func (p *offbox3aProvider) GetStackComposePath(string) (string, bool) { return "", false } +func (p *offbox3aProvider) ListDeployedStacks() []StackSummary { return nil } +func (p *offbox3aProvider) GetStackHDDMounts(string) []string { return nil } +func (p *offbox3aProvider) GetStackHDDPath(n string) string { return p.hdd[n] } +func (p *offbox3aProvider) GetDockerVolumes(string) []string { return nil } +func (p *offbox3aProvider) StopStack(string) error { return nil } +func (p *offbox3aProvider) StartStack(string) error { return nil } +func (p *offbox3aProvider) RefreshAndIsRunning(string) bool { return false } +func (p *offbox3aProvider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { + return RecoveryInfo{}, false +} func (p *offbox3aProvider) RecoverStackSecrets(string, []string) map[string]string { return nil } -func (p *offbox3aProvider) RecreateStackFromUnit(_, _ string, _ map[string]string) error { return nil } +func (p *offbox3aProvider) RecreateStackDefinitionFromUnit(_, _ string, _ map[string]string) error { + return nil +} +func (p *offbox3aProvider) StartStackServices(string, []string) error { return nil } func (p *offbox3aProvider) GetStackClassifiedBinds(n string) ([]ClassifiedBind, bool) { return p.binds[n], p.has[n] } diff --git a/controller/internal/backup/offbox_reconstitute.go b/controller/internal/backup/offbox_reconstitute.go index 5882be3..d02f06d 100644 --- a/controller/internal/backup/offbox_reconstitute.go +++ b/controller/internal/backup/offbox_reconstitute.go @@ -239,6 +239,20 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string) (Of res.Skewed = res.OffsiteRunID == "" res.LooksEmpty = m.sniffScratchDump(scratchDumpDir, stack) + // --- WHICH SERVICE HOLDS THE DATABASE (R-47) ------------------------------------------------ + // Read from the LIVE compose, not the scratch one: reconstitution never overwrites the stack dir, + // so the live file is what `docker compose up` will actually act on. Resolved BEFORE the first + // mutation so the refusal below costs nothing. + var dbServices []string + if composePath, cOK := m.stackProvider.GetStackComposePath(stack); cOK && composePath != "" { + svcs, dsErr := DBServiceNames(composePath) + if dsErr != nil { + // "cannot tell" is not "no database" — leave dbServices empty and let the gate refuse. + m.logger.Printf("[WARN] [offbox] %s: could not read the live compose services: %v", stack, dsErr) + } + dbServices = svcs + } + // --- THE UNDO, BEFORE THE ACT --------------------------------------------------------------- // Taken while the stack is still UP (a stopped database cannot be dumped) and before a single // byte is overwritten, so a failure here aborts with the live app completely untouched. @@ -253,6 +267,12 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string) (Of // Fail-closed: never replay when the undo is not verifiably on disk. return res, fmt.Errorf("a biztonsági mentés nem található a lemezen — a visszaállítás biztonsági okból nem indult el") } + // Fail-closed (R-47): the app HAS a database but no compose service can be identified to + // start alone for the replay. The only alternative would be to start everything and replay + // into the race that produced H4 — refusing with the live app untouched is the better outcome. + if len(dbServices) == 0 { + return res, fmt.Errorf("Az adatbázis-szolgáltatás nem azonosítható a(z) %s alkalmazásban — a visszaállítás biztonsági okból nem indult el.", stack) + } } // --- FILES ---------------------------------------------------------------------------------- @@ -280,19 +300,31 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string) (Of } // --- DATABASE ------------------------------------------------------------------------------- - // The stack must be UP for the replay: ImportDump talks to the running container using its own - // discovered credentials (the same precedence RestoreFromRecoveryUnit uses — the logical dump - // wins over whatever the file copy just laid down for the DB's own data dir). - if err := m.stackProvider.StartStack(stack); err != nil { - return res, fmt.Errorf("a(z) %s újraindítása sikertelen a fájlok visszaállítása után: %w", stack, err) - } + // The DB container must be UP for the replay (ImportDump talks to it with its own discovered + // credentials), but NOTHING ELSE may be — R-47. Until v0.153.0 this was a full StartStack, which + // gave the application a window to rebuild the very schema objects the dump was about to create: + // measured at 2 s on 2026-07-19, and the replay aborted `relation "clip_index" already exists` + // under ON_ERROR_STOP=1 (H4). Starting only the database service closes that window entirely. if hasDB { + if err := m.stackProvider.StartStackServices(stack, dbServices); err != nil { + // Best-effort bring-up: a failed restore must not also be an outage. + if sErr := m.stackProvider.StartStack(stack); sErr != nil { + m.logger.Printf("[WARN] [offbox] %s: full start after failed DB-only start also failed: %v", stack, sErr) + } + return res, fmt.Errorf("a(z) %s adatbázis-szolgáltatásának indítása sikertelen: %w", stack, err) + } n, iErr := m.reimportDBDumpsFrom(ctx, stack, scratchDumpDir) res.DBsReplayed = n if iErr != nil { + if sErr := m.stackProvider.StartStack(stack); sErr != nil { + m.logger.Printf("[WARN] [offbox] %s: full start after failed replay also failed: %v", stack, sErr) + } return res, fmt.Errorf("az adatbázis visszaállítása sikertelen: %w — a korábbi állapot mentése megvan: %s", iErr, filepath.Base(safety)) } } + if err := m.stackProvider.StartStack(stack); err != nil { + return res, fmt.Errorf("a(z) %s újraindítása sikertelen a fájlok visszaállítása után: %w", stack, err) + } if err := m.waitForHealthy(stack, 90*time.Second); err != nil { m.logger.Printf("[WARN] [offbox] %s reconstituted but health check failed: %v", stack, err) } diff --git a/controller/internal/backup/offbox_reconstitute_test.go b/controller/internal/backup/offbox_reconstitute_test.go index d794e5b..ce2abbd 100644 --- a/controller/internal/backup/offbox_reconstitute_test.go +++ b/controller/internal/backup/offbox_reconstitute_test.go @@ -20,13 +20,34 @@ import ( // rather than a description of the current implementation. // recordingProvider records stop/start call ORDER so the reconstitution sequence can be asserted. +// +// R-47 widened it: it now also records the DB-ONLY bring-up and, critically, whether a FULL start +// has happened yet — the state the replay must observe as `false`. That single flag is what +// separates the fixed sequence from the one that produced H4, in which the whole stack was already +// up (and rebuilding its own schema) when the dump replay began. type recordingProvider struct { offbox3aProvider - calls []string + calls []string + composePath string // the LIVE compose the DB-service resolver reads + gotServices []string // services passed to StartStackServices + fullStarted bool // a FULL StartStack has happened + startSvcErr error // injected StartStackServices failure } -func (p *recordingProvider) StopStack(string) error { p.calls = append(p.calls, "stop"); return nil } -func (p *recordingProvider) StartStack(string) error { p.calls = append(p.calls, "start"); return nil } +func (p *recordingProvider) StopStack(string) error { p.calls = append(p.calls, "stop"); return nil } +func (p *recordingProvider) StartStack(string) error { + p.fullStarted = true + p.calls = append(p.calls, "start") + return nil +} +func (p *recordingProvider) StartStackServices(_ string, services []string) error { + p.gotServices = append([]string(nil), services...) + p.calls = append(p.calls, "startsvc:"+strings.Join(services, ",")) + return p.startSvcErr +} +func (p *recordingProvider) GetStackComposePath(string) (string, bool) { + return p.composePath, p.composePath != "" +} // The app really is up again after StartStack, so the post-restore health wait returns at once. // Leaving it false would make each test sit through the full 90s deadline. @@ -78,6 +99,15 @@ func reconFixture(t *testing.T, runID, dumpsAt string, dumpBody string) (*Manage t.Fatal(err) } + // The LIVE compose the reconstitution reads to learn WHICH service holds the database (R-47). + // Immich-shaped on purpose: an app service, a redis service that must never be mistaken for a + // database, and a top-level `volumes:` key whose entry looks exactly like a service to a line scan. + liveStackDir := t.TempDir() + prov.composePath = filepath.Join(liveStackDir, "docker-compose.yml") + if err := os.WriteFile(prov.composePath, []byte(immichLikeCompose), 0o644); err != nil { + t.Fatal(err) + } + scratch, liveNs, err := m.offboxRestoreScratchDir("immich") if err != nil { t.Fatal(err) @@ -163,9 +193,10 @@ func TestReconstituteReplaysDBAndOrdersOperations(t *testing.T) { if res.FilesPlaced != 3 { t.Fatalf("expected the userdata placement to be counted, got %d", res.FilesPlaced) } - // stop BEFORE the file copy, start BEFORE the replay (ImportDump needs a live container). - if got := strings.Join(prov.calls, ","); got != "stop,start" { - t.Fatalf("expected stop then start around the restore, got %q", got) + // stop BEFORE the file copy; then ONLY the database service up for the replay (R-47 — a full + // start here is the H4 race); the full start comes last. + if got := strings.Join(prov.calls, ","); got != "stop,startsvc:immich-postgres,start" { + t.Fatalf("expected stop → db-only start → full start around the restore, got %q", got) } if res.SafetyDump == "" { t.Fatal("no safety dump recorded — the undo must exist") diff --git a/controller/internal/backup/r47_replay_order_test.go b/controller/internal/backup/r47_replay_order_test.go new file mode 100644 index 0000000..62751d9 --- /dev/null +++ b/controller/internal/backup/r47_replay_order_test.go @@ -0,0 +1,374 @@ +package backup + +import ( + "context" + "io" + "log" + "os" + "path/filepath" + "strings" + "testing" +) + +// R-47 (v0.153.0) — the DB replay must not race the app, on BOTH restore paths. +// +// Every test here is a regression guard for a measured incident, not a description of the code. +// On 2026-07-19 (DIAG-immich-restore-round2-2026-07-19, H4) the offsite reconstitution started the +// WHOLE stack before replaying the dump. immich-server used the window to rebuild `clip_index` two +// seconds before the dump's own CREATE INDEX; the replay aborted `relation "clip_index" already +// exists` under ON_ERROR_STOP=1, and immich then reported schema drift. The data survived only by +// accident of pg_dump's ordering (COPY before CREATE INDEX) — a collision earlier in the script +// would have left a genuinely half-restored database, reported identically. +// +// The property under test is therefore an ORDERING plus a STATE-AT-REPLAY-TIME: at the moment the +// import fires, the database service must be up and the full stack must NOT be. Asserting only +// "no error" would pass on the pre-fix shape, which is exactly how this shipped. + +// immichLikeCompose is the catalog's immich template reduced to what the resolver reads: the app +// services, the DB service, a redis that must never be mistaken for a database, and the top-level +// `volumes:`/`networks:` keys (including `immich_postgres_data`) that a line scan would misread. +const immichLikeCompose = `services: + immich-server: + image: ghcr.io/immich-app/immich-server:v3.0.3 + immich-machine-learning: + image: ghcr.io/immich-app/immich-machine-learning:v3.0.3 + immich-postgres: + image: ghcr.io/immich-app/postgres:16-vectorchord0.4.3-pgvectors0.2.0 + immich-redis: + image: redis:7-alpine +volumes: + immich_ml_cache: + immich_postgres_data: +networks: + traefik-public: + external: true +` + +// noDBCompose is a DB-free app: nothing here may ever trigger the DB-only phase. +const noDBCompose = `services: + app: + image: ghcr.io/x/app:1 + cache: + image: redis:7-alpine +` + +// --- Group A: offsite reconstitute, DB-bearing app (the H4 killer) -------------------------------- + +// TestReconstituteReplaysWithOnlyTheDBServiceUp is the core R-47 assertion for the offsite path. +// It does not merely check the call ORDER — it captures the provider's state AT THE MOMENT the +// import fires, because that is what H4 was: the sequence looked right, and the app was up. +// +// COMPANION RED-PROOF: replacing the DB-only bring-up in ReconstituteFromOffsite with the pre-fix +// full StartStack makes this fail on `full stack was ALREADY UP when the replay fired`. +func TestReconstituteReplaysWithOnlyTheDBServiceUp(t *testing.T) { + m, prov, imported := reconFixture(t, "20260719T060000Z", "2026-07-19T06:00:00Z", pgDump(1)) + + var dbUpAtReplay, fullUpAtReplay bool + m.importDBDump = func(_ context.Context, _ DiscoveredDB, p string) error { + dbUpAtReplay = len(prov.gotServices) > 0 + fullUpAtReplay = prov.fullStarted + *imported = append(*imported, p) + return nil + } + + res, err := m.ReconstituteFromOffsite(context.Background(), "immich") + if err != nil { + t.Fatalf("reconstitute: %v", err) + } + if res.DBsReplayed != 1 { + t.Fatalf("expected exactly one replay, got %d", res.DBsReplayed) + } + if !dbUpAtReplay { + t.Fatal("the database service was NOT started before the replay — ImportDump has no container to talk to") + } + if fullUpAtReplay { + t.Fatal("the FULL stack was already up when the replay fired — this is H4 exactly: the app races the dump's schema") + } + if got := strings.Join(prov.gotServices, ","); got != "immich-postgres" { + t.Fatalf("DB-only phase started %q, want only the database service immich-postgres", got) + } + if got := strings.Join(prov.calls, ","); got != "stop,startsvc:immich-postgres,start" { + t.Fatalf("sequence = %q, want stop → db-only start → replay → full start", got) + } + // The undo must have existed before any of it. + if res.SafetyDump == "" { + t.Fatal("no safety dump recorded") + } + if _, sErr := os.Stat(res.SafetyDump); sErr != nil { + t.Fatalf("safety dump not on disk before the mutation: %v", sErr) + } +} + +// --- Group B: offsite reconstitute, no-DB app (flow unchanged) ------------------------------------ + +// TestReconstituteNoDBAppNeverStartsServicesOnly asserts the NEGATIVE: an app with no database must +// take exactly one full start and must never enter the DB-only phase. Without this, a bug that +// armed the phase for every app would show up first as a customer's stack half-started. +func TestReconstituteNoDBAppNeverStartsServicesOnly(t *testing.T) { + m, prov, imported := reconFixture(t, "run1", "2026-07-19T06:00:00Z", "") + prov.composePath = writeLiveCompose(t, noDBCompose) + m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { return nil, nil } + + res, err := m.ReconstituteFromOffsite(context.Background(), "immich") + if err != nil { + t.Fatalf("a no-DB app must restore unchanged, got: %v", err) + } + if len(prov.gotServices) != 0 { + t.Fatalf("the DB-only phase ran for an app with no database: %v", prov.gotServices) + } + if got := strings.Join(prov.calls, ","); got != "stop,start" { + t.Fatalf("sequence = %q, want the unchanged stop → full start", got) + } + if len(*imported) != 0 || res.DBsReplayed != 0 { + t.Fatalf("a no-DB app must not replay anything: imported=%v replayed=%d", *imported, res.DBsReplayed) + } +} + +// --- Group C: fail-closed, both paths ------------------------------------------------------------- + +// TestReconstituteRefusesWhenNoDBServiceIdentifiable is the security-adjacent gate. A dump exists and +// a live database was discovered, but the live compose names no startable database service. The only +// alternative to refusing would be to start everything and replay into the H4 race, so this must +// refuse — and it must refuse with ZERO mutations, which is what the effect assertions below prove. +// Asserting `err != nil` alone would pass even if the app had already been stopped and overwritten. +// +// COMPANION RED-PROOF: deleting the `len(dbServices) == 0` gate makes this fail on +// `the app was stopped despite the refusal`. +func TestReconstituteRefusesWhenNoDBServiceIdentifiable(t *testing.T) { + m, prov, imported := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) + // A live compose whose services are all app/cache images — nothing to start alone. + prov.composePath = writeLiveCompose(t, noDBCompose) + var copied bool + m.SetOffboxFullPlaceCopier(func(_, _ string) (int, error) { copied = true; return 1, nil }) + + _, err := m.ReconstituteFromOffsite(context.Background(), "immich") + if err == nil { + t.Fatal("expected a refusal: a dump exists but no database service can be started for it") + } + if !strings.Contains(err.Error(), "nem azonosítható") { + t.Fatalf("refusal must say the database service could not be identified, got: %v", err) + } + if len(prov.calls) != 0 { + t.Fatalf("ZERO mutations required, but the provider was called: %v", prov.calls) + } + if copied { + t.Fatal("files were overwritten despite the refusal") + } + if len(*imported) != 0 { + t.Fatalf("a replay happened despite the refusal: %v", *imported) + } +} + +// TestRestoreFromUnitRefusesWhenNoDBServiceIdentifiable is the local path's sibling gate, with the +// same zero-mutation requirement: no stop, no volume restore, no definition recreate. +func TestRestoreFromUnitRefusesWhenNoDBServiceIdentifiable(t *testing.T) { + m, prov, _ := r47UnitFixture(t, noDBCompose, true) + + err := m.RestoreFromRecoveryUnit("app") + if err == nil { + t.Fatal("expected a refusal: the unit carries a dump but names no startable database service") + } + if !strings.Contains(err.Error(), "nem azonosítható") { + t.Fatalf("refusal must say the database service could not be identified, got: %v", err) + } + if len(prov.calls) != 0 { + t.Fatalf("ZERO mutations required, but the provider was called: %v", prov.calls) + } + if prov.stopped { + t.Fatal("the app was stopped despite the refusal") + } + if prov.gotEnv != nil { + t.Fatal("the definition was recreated despite the refusal") + } +} + +// --- Group D: local restore-from-unit ordering ---------------------------------------------------- + +// TestRestoreFromUnitReplaysWithOnlyTheDBServiceUp is Group A's twin on the local path — the SAME +// class defect lived here, in the shape `RecreateStackFromUnit` (which ended in a full `up -d`) +// followed by the replay. Splitting the persist from the start is what makes this orderable at all. +// +// COMPANION RED-PROOF: restoring the pre-fix shape (RecreateStackDefinitionFromUnit performing a +// full start, replay after) makes this fail on `full stack was ALREADY UP when the replay fired`. +func TestRestoreFromUnitReplaysWithOnlyTheDBServiceUp(t *testing.T) { + m, prov, imported := r47UnitFixture(t, immichLikeCompose, true) + + var dbUpAtReplay, fullUpAtReplay, definitionPersisted bool + m.importDBDump = func(_ context.Context, _ DiscoveredDB, p string) error { + dbUpAtReplay = len(prov.gotServices) > 0 + fullUpAtReplay = prov.fullStarted + definitionPersisted = prov.gotEnv != nil + *imported = append(*imported, p) + return nil + } + + if err := m.RestoreFromRecoveryUnit("app"); err != nil { + t.Fatalf("restore-from-unit: %v", err) + } + if len(*imported) != 1 { + t.Fatalf("expected exactly one replay, got %v", *imported) + } + if !definitionPersisted { + t.Fatal("the app definition was not persisted before the replay — the DB service could not have been started from it") + } + if !dbUpAtReplay { + t.Fatal("the database service was NOT started before the replay") + } + if fullUpAtReplay { + t.Fatal("the FULL stack was already up when the replay fired — the H4 race, on the local path") + } + if got := strings.Join(prov.calls, ","); got != "stop,recreate,startsvc:immich-postgres,start" { + t.Fatalf("sequence = %q, want stop → recreate(definition only) → db-only start → replay → full start", got) + } +} + +// TestRestoreFromUnitNoDumpsTakesOneFullStart is the local no-DB negative: without a replayable dump +// there is no DB-only window at all, just the definition and one full start. +func TestRestoreFromUnitNoDumpsTakesOneFullStart(t *testing.T) { + m, prov, imported := r47UnitFixture(t, noDBCompose, false) + + if err := m.RestoreFromRecoveryUnit("app"); err != nil { + t.Fatalf("restore-from-unit: %v", err) + } + if len(prov.gotServices) != 0 { + t.Fatalf("the DB-only phase ran with nothing to replay: %v", prov.gotServices) + } + if got := strings.Join(prov.calls, ","); got != "stop,recreate,start" { + t.Fatalf("sequence = %q, want stop → recreate → full start", got) + } + if len(*imported) != 0 { + t.Fatalf("nothing should have been replayed, got %v", *imported) + } +} + +// TestRestoreFromUnitIgnoresSafetyDumpsWhenDecidingToReplay guards the one file-naming trap in the +// gate: `pre-restore-*.sql` safety dumps live in the SAME directory as the real dumps (deliberately — +// an undo the customer cannot see is not much of one) but are never a replay source. Counting them +// would arm the DB-only phase, and its refusal, for an app that has nothing to replay. +func TestRestoreFromUnitIgnoresSafetyDumpsWhenDecidingToReplay(t *testing.T) { + m, prov, _ := r47UnitFixture(t, noDBCompose, false) + // A safety dump present for a DB-less app must not arm anything — including the refusal. + mustWrite(t, filepath.Join(AppDBDumpPath(prov.hdd, "app"), + preRestoreDumpPrefix+"20260720T101010Z-app-postgres.sql"), pgDump(1)) + + if err := m.RestoreFromRecoveryUnit("app"); err != nil { + t.Fatalf("a lone safety dump must not turn into a refusal: %v", err) + } + if len(prov.gotServices) != 0 { + t.Fatalf("a safety dump armed the DB-only phase: %v", prov.gotServices) + } +} + +// --- Group E: a failed replay never strands the box DB-only --------------------------------------- + +// TestReconstituteReplayFailureStillBringsTheStackUp: the DB-only window is a deliberate half-started +// state, so EVERY exit from it must end in a full start. Otherwise a failed restore leaves the +// customer with a running database and no application — an outage caused by the recovery tool. +func TestReconstituteReplayFailureStillBringsTheStackUp(t *testing.T) { + m, prov, _ := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) + m.importDBDump = func(context.Context, DiscoveredDB, string) error { + return context.DeadlineExceeded + } + + res, err := m.ReconstituteFromOffsite(context.Background(), "immich") + if err == nil { + t.Fatal("a failed replay must be surfaced, not swallowed") + } + if !prov.fullStarted { + t.Fatal("the stack was left DB-ONLY after a failed replay — the app is down and nothing will bring it up") + } + if got := strings.Join(prov.calls, ","); got != "stop,startsvc:immich-postgres,start" { + t.Fatalf("sequence = %q, want the best-effort full start after the failure", got) + } + // The existing message shape stays: the operator needs the undo's filename. + if !strings.Contains(err.Error(), filepath.Base(res.SafetyDump)) { + t.Fatalf("the error must name the safety dump so the operator can undo, got: %v", err) + } +} + +// TestReconstituteDBOnlyStartFailureStillBringsTheStackUp covers the other exit from the window: the +// DB-only start itself failing. Same requirement — the app must not be left down. +func TestReconstituteDBOnlyStartFailureStillBringsTheStackUp(t *testing.T) { + m, prov, imported := reconFixture(t, "run1", "2026-07-19T06:00:00Z", pgDump(1)) + prov.startSvcErr = context.DeadlineExceeded + + if _, err := m.ReconstituteFromOffsite(context.Background(), "immich"); err == nil { + t.Fatal("a failed DB-only start must be surfaced") + } + if !prov.fullStarted { + t.Fatal("the stack was left down after a failed DB-only start") + } + if len(*imported) != 0 { + t.Fatalf("nothing may be replayed when the database never came up: %v", *imported) + } +} + +// TestRestoreFromUnitReplayFailureStillBringsTheStackUp is the local path's version, and it also +// pins the pre-existing semantics: a replay error becomes a dataErr and surfaces as the "completed +// with data errors" outcome, with the app back up. +func TestRestoreFromUnitReplayFailureStillBringsTheStackUp(t *testing.T) { + m, prov, _ := r47UnitFixture(t, immichLikeCompose, true) + m.importDBDump = func(context.Context, DiscoveredDB, string) error { + return context.DeadlineExceeded + } + + err := m.RestoreFromRecoveryUnit("app") + if err == nil { + t.Fatal("a failed replay must be surfaced, not swallowed") + } + if !strings.Contains(err.Error(), "completed with data errors") { + t.Fatalf("the pre-existing outcome semantics must be preserved, got: %v", err) + } + if !prov.fullStarted { + t.Fatal("the stack was left DB-ONLY after a failed replay") + } + if got := strings.Join(prov.calls, ","); got != "stop,recreate,startsvc:immich-postgres,start" { + t.Fatalf("sequence = %q, want the full start to follow the failed replay", got) + } +} + +// --- fixtures ------------------------------------------------------------------------------------- + +// writeLiveCompose drops a compose file in its own temp dir and returns the path. +func writeLiveCompose(t *testing.T, body string) string { + t.Helper() + p := filepath.Join(t.TempDir(), "docker-compose.yml") + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +// r47UnitFixture builds a Manager whose local recovery unit for "app" carries the given compose and, +// optionally, a replayable `app-postgres.sql` dump. The provider records call order; the DB +// discovery/import seams are injected so no Docker is touched. +func r47UnitFixture(t *testing.T, compose string, withDump bool) (*Manager, *fakeRecoveryProvider, *[]string) { + t.Helper() + drive := filepath.Join(t.TempDir(), "drive") + composeDir := RecoveryUnitComposePath(drive, "app") + mustWrite(t, filepath.Join(composeDir, "app.yaml"), "deployed: true\nenv:\n SUBDOMAIN: app\n") + mustWrite(t, filepath.Join(composeDir, "docker-compose.yml"), compose) + man := &RecoveryManifest{SchemaVersion: 1, AppName: "app", ControllerVer: "v"} + if err := writeManifest(RecoveryUnitManifestPath(drive, "app"), man); err != nil { + t.Fatal(err) + } + if withDump { + mustWrite(t, filepath.Join(AppDBDumpPath(drive, "app"), "app-postgres.sql"), pgDump(1)) + } + + prov := &fakeRecoveryProvider{hdd: drive, running: true} + m := &Manager{ + logger: log.New(io.Discard, "", 0), + systemDataPath: filepath.Join(drive, "..", "sys"), + stackProvider: prov, + } + + db := DiscoveredDB{StackName: "app", ContainerName: "immich-postgres", DBType: DBTypePostgres} + m.discoverDBs = func(context.Context) ([]DiscoveredDB, error) { return []DiscoveredDB{db}, nil } + var imported []string + m.importDBDump = func(_ context.Context, _ DiscoveredDB, p string) error { + imported = append(imported, p) + return nil + } + return m, prov, &imported +} diff --git a/controller/internal/backup/recovery_unit_test.go b/controller/internal/backup/recovery_unit_test.go index 912a128..62ab30c 100644 --- a/controller/internal/backup/recovery_unit_test.go +++ b/controller/internal/backup/recovery_unit_test.go @@ -12,13 +12,23 @@ import ( ) // fakeRecoveryProvider is a configurable StackDataProvider for the capture + restore tests. +// +// It records the ORDER of every mutating call (R-47): the local restore path's correctness is an +// ordering property — definition persisted, then the DB service alone, then the replay, then the +// full start — and an ordering guarantee nothing observes is one refactor from silently reverting to +// the shape that produced H4. type fakeRecoveryProvider struct { info RecoveryInfo hdd string secrets map[string]string // returned by RecoverStackSecrets - gotEnv map[string]string // captured by RecreateStackFromUnit + gotEnv map[string]string // captured by RecreateStackDefinitionFromUnit running bool // returned by RefreshAndIsRunning stopped bool + + calls []string // ordered log: stop / recreate / startsvc: / start + gotServices []string // services passed to StartStackServices + startSvcErr error // injected StartStackServices failure + fullStarted bool // a FULL StartStack happened } func (f *fakeRecoveryProvider) GetStackComposePath(string) (string, bool) { @@ -28,9 +38,17 @@ func (f *fakeRecoveryProvider) ListDeployedStacks() []StackSummary { return nil func (f *fakeRecoveryProvider) GetStackHDDMounts(string) []string { return nil } func (f *fakeRecoveryProvider) GetStackHDDPath(string) string { return f.hdd } func (f *fakeRecoveryProvider) GetDockerVolumes(string) []string { return nil } -func (f *fakeRecoveryProvider) StopStack(string) error { f.stopped = true; return nil } -func (f *fakeRecoveryProvider) StartStack(string) error { return nil } -func (f *fakeRecoveryProvider) RefreshAndIsRunning(string) bool { return f.running } +func (f *fakeRecoveryProvider) StopStack(string) error { + f.stopped = true + f.calls = append(f.calls, "stop") + return nil +} +func (f *fakeRecoveryProvider) StartStack(string) error { + f.fullStarted = true + f.calls = append(f.calls, "start") + return nil +} +func (f *fakeRecoveryProvider) RefreshAndIsRunning(string) bool { return f.running } func (f *fakeRecoveryProvider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { return f.info, true } @@ -40,10 +58,16 @@ func (f *fakeRecoveryProvider) GetStackClassifiedBinds(string) ([]ClassifiedBind func (f *fakeRecoveryProvider) RecoverStackSecrets(string, []string) map[string]string { return f.secrets } -func (f *fakeRecoveryProvider) RecreateStackFromUnit(_, _ string, fullEnv map[string]string) error { +func (f *fakeRecoveryProvider) RecreateStackDefinitionFromUnit(_, _ string, fullEnv map[string]string) error { f.gotEnv = fullEnv + f.calls = append(f.calls, "recreate") return nil } +func (f *fakeRecoveryProvider) StartStackServices(_ string, services []string) error { + f.gotServices = append([]string(nil), services...) + f.calls = append(f.calls, "startsvc:"+strings.Join(services, ",")) + return f.startSvcErr +} // TestCaptureRecoveryUnitIsSecretFree proves the captured unit (a) contains compose+config+manifest, // (b) enumerates the existing dumps, and (c) is SECRET-FREE: a secret value present in the SOURCE diff --git a/controller/internal/backup/restore_unit.go b/controller/internal/backup/restore_unit.go index 4092a80..e24d681 100644 --- a/controller/internal/backup/restore_unit.go +++ b/controller/internal/backup/restore_unit.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "gopkg.in/yaml.v3" @@ -64,6 +65,26 @@ func readStrippedEnv(path string) map[string]string { return s.Env } +// hasReplayableDump reports whether dumpDir holds a .sql dump that the replay could actually use. +// The `pre-restore-` safety dumps are EXCLUDED: they live in the same directory (deliberately — an +// undo the customer cannot see is not much of one) but are never a replay source, so counting them +// would arm the DB-only phase, and its fail-closed gate, for an app that has nothing to replay. +func hasReplayableDump(dumpDir string) bool { + entries, err := os.ReadDir(dumpDir) + if err != nil { + return false + } + for _, e := range entries { + if e.IsDir() || filepath.Ext(e.Name()) != ".sql" { + continue + } + if !strings.HasPrefix(e.Name(), preRestoreDumpPrefix) { + return true + } + } + return false +} + // RestoreFromRecoveryUnit recreates an app from its on-drive recovery unit + the guest's own secrets. // // It reads the unit manifest, recovers the secret values from the guest's live app.yaml, applies the @@ -148,7 +169,22 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { m.logger.Printf("[INFO] [backup] Restoring %s from recovery unit: images=%d, secrets recovered=%d/%d, data_keys=%d", stackName, len(manifest.ImagePins), len(manifest.SecretEnvVars)-len(missing), len(manifest.SecretEnvVars), len(manifest.DataKeyEnvVars)) - // Stop, restore named-volume data, then recreate the definition + redeploy with the recovered env. + // R-47: which compose service holds the database, and is there anything to replay? Resolved from + // the UNIT's compose, because that file is about to BECOME the live one. Both answers are needed + // BEFORE the first mutation, so the refusal below leaves the live app completely untouched. + dbServices, dsErr := DBServiceNames(filepath.Join(composeDir, "docker-compose.yml")) + if dsErr != nil { + // "cannot tell" is not "no database" — leave it empty and let the gate decide. + m.logger.Printf("[WARN] [backup] %s: could not read the unit's compose services: %v", stackName, dsErr) + } + hasDumps := hasReplayableDump(AppDBDumpPath(nsRoot, stackName)) + if hasDumps && len(dbServices) == 0 { + m.logger.Printf("[ERROR] [backup] Restore REFUSED for %s: a .sql dump exists but no database service is identifiable in the unit's compose", stackName) + return fmt.Errorf("Az adatbázis-szolgáltatás nem azonosítható a(z) %s alkalmazásban — a visszaállítás biztonsági okból nem indult el.", stackName) + } + + // Stop, restore named-volume data, recreate the definition, replay the DB with ONLY the database + // service running, and only then start the whole stack. // F17: surface a data-restore failure instead of swallowing it (we still bring the app back up). var dataErr error if err := m.stackProvider.StopStack(stackName); err != nil { @@ -158,17 +194,30 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { m.logger.Printf("[ERROR] [backup] volume restore for %s: %v", stackName, err) dataErr = err } - if err := m.stackProvider.RecreateStackFromUnit(stackName, composeDir, fullEnv); err != nil { + if err := m.stackProvider.RecreateStackDefinitionFromUnit(stackName, composeDir, fullEnv); err != nil { return fmt.Errorf("recreating %s from unit: %w", stackName, err) } - // F17: the captured .sql dump is the authoritative logical DB state — replay it into the now-running - // DB container AFTER the volume restore, so the dump WINS over any volume-tar copy of the database. - if _, err := m.reimportDBDumpsCtx(stackName, nsRoot); err != nil { - m.logger.Printf("[ERROR] [backup] DB re-import for %s: %v", stackName, err) - if dataErr == nil { - dataErr = err + // F17: the captured .sql dump is the authoritative logical DB state — replay it AFTER the volume + // restore, so the dump WINS over any volume-tar copy of the database. + // R-47: the replay happens with ONLY the database service up. This used to run after + // RecreateStackFromUnit had already brought the WHOLE stack up, letting the application rebuild + // schema objects underneath the replay (H4, DIAG-immich-restore-round2-2026-07-19). + if hasDumps { + if err := m.stackProvider.StartStackServices(stackName, dbServices); err != nil { + m.logger.Printf("[ERROR] [backup] DB-only start for %s: %v", stackName, err) + if dataErr == nil { + dataErr = err + } + } else if _, err := m.reimportDBDumpsCtx(stackName, nsRoot); err != nil { + m.logger.Printf("[ERROR] [backup] DB re-import for %s: %v", stackName, err) + if dataErr == nil { + dataErr = err + } } } + if err := m.stackProvider.StartStack(stackName); err != nil { + return fmt.Errorf("starting %s after restore from unit: %w", stackName, err) + } if err := m.waitForHealthy(stackName, 90*time.Second); err != nil { m.logger.Printf("[WARN] [backup] %s restored but health check failed: %v", stackName, err) } diff --git a/controller/internal/backup/tier2_restore_test.go b/controller/internal/backup/tier2_restore_test.go index 3d4c521..ff76d32 100644 --- a/controller/internal/backup/tier2_restore_test.go +++ b/controller/internal/backup/tier2_restore_test.go @@ -44,9 +44,10 @@ func (f *t2rFakeProvider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { } func (f *t2rFakeProvider) GetStackClassifiedBinds(string) ([]ClassifiedBind, bool) { return nil, false } func (f *t2rFakeProvider) RecoverStackSecrets(string, []string) map[string]string { return nil } -func (f *t2rFakeProvider) RecreateStackFromUnit(string, string, map[string]string) error { +func (f *t2rFakeProvider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { return nil } +func (f *t2rFakeProvider) StartStackServices(string, []string) error { return nil } // newT2RManager builds a Manager with a RECORDED Tier-2 copy for "app": live drive + a populated // /backups/secondary/app/appdata dir, and a CrossDriveBackup entry pointing at dest. diff --git a/controller/internal/backup/tier2_v2_test.go b/controller/internal/backup/tier2_v2_test.go index 56531fb..cfcd5c9 100644 --- a/controller/internal/backup/tier2_v2_test.go +++ b/controller/internal/backup/tier2_v2_test.go @@ -21,17 +21,22 @@ type t2v2Provider struct { has map[string]bool } -func (p *t2v2Provider) GetStackComposePath(string) (string, bool) { return "", false } -func (p *t2v2Provider) ListDeployedStacks() []StackSummary { return nil } -func (p *t2v2Provider) GetStackHDDMounts(n string) []string { return p.mounts[n] } -func (p *t2v2Provider) GetStackHDDPath(string) string { return p.hdd } -func (p *t2v2Provider) GetDockerVolumes(string) []string { return nil } -func (p *t2v2Provider) StopStack(string) error { return nil } -func (p *t2v2Provider) StartStack(string) error { return nil } -func (p *t2v2Provider) RefreshAndIsRunning(string) bool { return true } -func (p *t2v2Provider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { return RecoveryInfo{}, false } +func (p *t2v2Provider) GetStackComposePath(string) (string, bool) { return "", false } +func (p *t2v2Provider) ListDeployedStacks() []StackSummary { return nil } +func (p *t2v2Provider) GetStackHDDMounts(n string) []string { return p.mounts[n] } +func (p *t2v2Provider) GetStackHDDPath(string) string { return p.hdd } +func (p *t2v2Provider) GetDockerVolumes(string) []string { return nil } +func (p *t2v2Provider) StopStack(string) error { return nil } +func (p *t2v2Provider) StartStack(string) error { return nil } +func (p *t2v2Provider) RefreshAndIsRunning(string) bool { return true } +func (p *t2v2Provider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { + return RecoveryInfo{}, false +} func (p *t2v2Provider) RecoverStackSecrets(string, []string) map[string]string { return nil } -func (p *t2v2Provider) RecreateStackFromUnit(string, string, map[string]string) error { return nil } +func (p *t2v2Provider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { + return nil +} +func (p *t2v2Provider) StartStackServices(string, []string) error { return nil } func (p *t2v2Provider) GetStackClassifiedBinds(n string) ([]ClassifiedBind, bool) { return p.binds[n], p.has[n] } @@ -299,7 +304,10 @@ func TestTier2V2_RestoreRefusesOldLayout(t *testing.T) { if err := os.Remove(filepath.Join(destDrive, "backups", "secondary", "app", tier2LayoutMarker)); err != nil { t.Fatal(err) } - m.restoreFilesCopier = func(string, string) (int, error) { t.Fatal("copier must not run on an old-layout refusal"); return 0, nil } + m.restoreFilesCopier = func(string, string) (int, error) { + t.Fatal("copier must not run on an old-layout refusal") + return 0, nil + } if _, err := m.RestoreTier2Files("app"); err == nil || !strings.Contains(err.Error(), "régi formátumú") { t.Fatalf("restore must refuse a pre-v2 copy with the marker-refusal string, got %v", err) } diff --git a/controller/internal/backup/volume_dumps_test.go b/controller/internal/backup/volume_dumps_test.go index 2429a81..f3c4ccf 100644 --- a/controller/internal/backup/volume_dumps_test.go +++ b/controller/internal/backup/volume_dumps_test.go @@ -39,9 +39,10 @@ func (f *volDumpFakeProvider) GetStackClassifiedBinds(string) ([]ClassifiedBind, return nil, false } func (f *volDumpFakeProvider) RecoverStackSecrets(string, []string) map[string]string { return nil } -func (f *volDumpFakeProvider) RecreateStackFromUnit(string, string, map[string]string) error { +func (f *volDumpFakeProvider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { return nil } +func (f *volDumpFakeProvider) StartStackServices(string, []string) error { return nil } // TestRunVolumeDumps_GatesPrecedeDump proves Scenario D/E's gating: the dump is invoked ONLY for // a volume-bearing, unprotected stack on a writable drive. The negatives are the point — diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index af1a08d..82aa22d 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -480,6 +480,32 @@ func (m *Manager) UpdateStackConfig(name string, values map[string]string) error // flow (Phase 2b): unlike UpdateStackConfig it sets the full env INCLUDING locked secrets — which were // recovered from the guest's own app.yaml, never regenerated. Caller is responsible for the gate. func (m *Manager) RedeployFromEnv(name string, env map[string]string) error { + if err := m.PersistUnitRedeployConfig(name, env); err != nil { + return err + } + stack, ok := m.GetStack(name) + if !ok { + return fmt.Errorf("stack %q not found", name) + } + stackDir := filepath.Dir(stack.ComposePath) + deployEnv := m.stackEnv(stackDir) // decrypts secrets back for compose + if _, err := m.composeExecCustomEnv(stackDir, deployEnv, "up", "-d"); err != nil { + return fmt.Errorf("compose up: %w", err) + } + m.logPostStartStatus(name, stackDir, deployEnv) + return m.RefreshStatus() +} + +// PersistUnitRedeployConfig is the PERSIST half of RedeployFromEnv: it writes app.yaml from the full +// env (encrypting secret fields, recording locked fields) and marks the stack deployed in memory — +// and starts NOTHING. +// +// Split out for R-47. The restore paths must place the app's definition and then bring up only the +// database service for the dump replay; calling RedeployFromEnv there would end in a full +// `compose up -d` BEFORE the replay, which is exactly the race (H4) this work removes. +// RedeployFromEnv itself is this function plus the unchanged up-and-report tail, so its public +// behaviour is identical to before the split. +func (m *Manager) PersistUnitRedeployConfig(name string, env map[string]string) error { stack, ok := m.GetStack(name) if !ok { return fmt.Errorf("stack %q not found", name) @@ -509,12 +535,7 @@ func (m *Manager) RedeployFromEnv(name string, env map[string]string) error { m.mu.Unlock() m.logger.Printf("[INFO] [stacks] Redeploying %s from recovery unit with %d env vars", name, len(env)) - deployEnv := m.stackEnv(stackDir) // decrypts secrets back for compose - if _, err := m.composeExecCustomEnv(stackDir, deployEnv, "up", "-d"); err != nil { - return fmt.Errorf("compose up: %w", err) - } - m.logPostStartStatus(name, stackDir, deployEnv) - return m.RefreshStatus() + return nil } // composeExecWithEnv runs a compose command with custom env vars injected. Used by the initial deploy diff --git a/controller/internal/stacks/manager.go b/controller/internal/stacks/manager.go index 12d7ad1..88b72d8 100644 --- a/controller/internal/stacks/manager.go +++ b/controller/internal/stacks/manager.go @@ -767,6 +767,44 @@ func (m *Manager) StartStack(name string) error { return m.RefreshStatus() } +// StartStackServices brings up ONLY the named compose services (`docker compose up -d ...`), +// leaving the rest of the stack down. It exists for R-47: a database dump must be replayed into a +// running DB container while the application that owns the schema is still stopped, otherwise the +// app's own schema management races the replay (proven live — H4, +// DIAG-immich-restore-round2-2026-07-19). Every catalog template's dependency direction is app→db, +// so naming the DB service starts the DB and nothing else. +// +// An EMPTY service list is refused rather than passed through: `up -d` with no arguments is a FULL +// start, which is precisely the behaviour this function exists to avoid — a silent fall-through +// would reintroduce the race at the one call site that most needs it not to. +// +// Deliberately no logPostStartStatus: the app containers are absent ON PURPOSE here, and it would +// WARN about every one of them. The full StartStack that always follows logs the real post-start +// state. +func (m *Manager) StartStackServices(name string, services []string) error { + if len(services) == 0 { + return fmt.Errorf("starting services of stack %s: empty service list", name) + } + stack, ok := m.GetStack(name) + if !ok { + return fmt.Errorf("stack %q not found", name) + } + + m.logger.Printf("[INFO] [stacks] Starting stack %s services only: %v", name, services) + start := time.Now() + + dir := filepath.Dir(stack.ComposePath) + env := m.stackEnv(dir) + + if _, err := m.composeExecCustomEnv(dir, env, append([]string{"up", "-d"}, services...)...); err != nil { + m.logger.Printf("[ERROR] [stacks] Stack %s service start failed after %.1fs: %v", name, time.Since(start).Seconds(), err) + return fmt.Errorf("starting services %v of stack %s: %w", services, name, err) + } + + m.logger.Printf("[INFO] [stacks] Stack %s services %v started (took %.1fs)", name, services, time.Since(start).Seconds()) + return m.RefreshStatus() +} + func (m *Manager) StopStack(name string) error { if m.cfg.IsProtectedStack(name) { return fmt.Errorf("stack %q is protected and cannot be stopped", name) diff --git a/controller/internal/stacks/start_services_test.go b/controller/internal/stacks/start_services_test.go new file mode 100644 index 0000000..3dc44c7 --- /dev/null +++ b/controller/internal/stacks/start_services_test.go @@ -0,0 +1,134 @@ +package stacks + +import ( + "io" + "log" + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) + +// R-47 (v0.153.0) — the two seams the restore paths need in order to replay a DB dump without the +// application racing it: a scoped bring-up, and a persist-without-start. + +// newR47Manager builds a Manager with one stack whose .felhom.yml declares a locked data-key and a +// plain field, so the persist half's locked-field and encryption behaviour is observable. +func newR47Manager(t *testing.T) (*Manager, string) { + t.Helper() + stackDir := filepath.Join(t.TempDir(), "app") + if err := os.MkdirAll(stackDir, 0o755); err != nil { + t.Fatal(err) + } + meta := `display_name: App +deploy_fields: + - env_var: SECRET_KEY + type: secret + locked_after_deploy: true + - env_var: SUBDOMAIN + type: subdomain + - env_var: TIMEZONE + type: text +` + if err := os.WriteFile(filepath.Join(stackDir, ".felhom.yml"), []byte(meta), 0o644); err != nil { + t.Fatal(err) + } + m := &Manager{ + logger: log.New(io.Discard, "", 0), + encKey: []byte("0123456789abcdef0123456789abcdef"), // 32 bytes → AES-256 + stacks: map[string]*Stack{ + "app": {Name: "app", ComposePath: filepath.Join(stackDir, "docker-compose.yml")}, + }, + } + return m, stackDir +} + +// TestStartStackServicesRefusesEmptyList is the whole reason this function is not a thin wrapper. +// `docker compose up -d` with no service arguments is a FULL start — the exact behaviour the DB-only +// window exists to avoid. So an empty list must be an ERROR, never a silent pass-through: a caller +// that computed zero DB services has, by definition, nothing it may safely start. +// +// The refusal is also asserted to happen WITHOUT reaching compose: it fires for an unknown stack +// too, which proves nothing was executed (a real `up` would need a stack dir and a docker daemon). +func TestStartStackServicesRefusesEmptyList(t *testing.T) { + m, _ := newR47Manager(t) + + for _, svcs := range [][]string{nil, {}} { + err := m.StartStackServices("app", svcs) + if err == nil { + t.Fatalf("an empty service list (%v) must be refused — argument-less `up -d` is a FULL start", svcs) + } + if !strings.Contains(err.Error(), "empty service list") { + t.Fatalf("refusal must name the cause, got: %v", err) + } + } + // Unknown stack: refused at the lookup, still without touching compose. + if err := m.StartStackServices("nope", []string{"db"}); err == nil { + t.Fatal("an unknown stack must be refused") + } +} + +// TestPersistUnitRedeployConfigPersistsWithoutStarting is the split's contract. RedeployFromEnv used +// to be persist+start in one call, which is why the local restore path could not put the DB-only +// window between them. This asserts the persist half is COMPLETE on its own — app.yaml written with +// the deployed marker, the locked field recorded, the secret encrypted at rest and decryptable, and +// the in-memory stack flipped to deployed — so RedeployFromEnv's public behaviour is unchanged by +// being expressed as this function plus the untouched up-and-report tail. +func TestPersistUnitRedeployConfigPersistsWithoutStarting(t *testing.T) { + m, stackDir := newR47Manager(t) + const secret = "s3cr3t-data-key-value" + env := map[string]string{"SECRET_KEY": secret, "SUBDOMAIN": "app", "TIMEZONE": "Europe/Budapest", "HDD_PATH": "/mnt/drv"} + + if err := m.PersistUnitRedeployConfig("app", env); err != nil { + t.Fatalf("PersistUnitRedeployConfig: %v", err) + } + + cfgPath := filepath.Join(stackDir, "app.yaml") + raw, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("app.yaml was not written — the persist half is incomplete: %v", err) + } + // Secrets safety: the plaintext must not be at rest in app.yaml. + if strings.Contains(string(raw), secret) { + t.Fatal("SECRET LEAK: the secret value is stored in plaintext in app.yaml") + } + + got := LoadAppConfigDecrypted(stackDir, m.encKey) + if got == nil { + t.Fatal("app.yaml does not load back") + } + if !got.Deployed || got.DeployedAt == "" { + t.Errorf("app must be marked deployed with a timestamp, got deployed=%v at=%q", got.Deployed, got.DeployedAt) + } + if got.Env["SECRET_KEY"] != secret { + t.Errorf("SECRET_KEY did not round-trip through encryption, got %q", got.Env["SECRET_KEY"]) + } + if got.Env["SUBDOMAIN"] != "app" || got.Env["HDD_PATH"] != "/mnt/drv" { + t.Errorf("non-secret env did not persist verbatim: %v", got.Env) + } + // Secrets and subdomains are implicitly locked-after-deploy; a plain text field is not. Recording + // exactly those is part of the persist half, and losing it in the split would silently unlock a + // data-key field on the next config edit. + if !reflect.DeepEqual(got.LockedFields, []string{"SECRET_KEY", "SUBDOMAIN"}) { + t.Errorf("locked fields = %v, want [SECRET_KEY SUBDOMAIN] (TIMEZONE must NOT be locked)", got.LockedFields) + } + + // In-memory state must agree, because StartStack (the caller's next step) reads it. + s, ok := m.GetStack("app") + if !ok || !s.Deployed { + t.Fatalf("in-memory stack not marked deployed (ok=%v), so the follow-up start would treat it as undeployed", ok) + } + if s.AppConfig == nil || s.AppConfig.Env["SECRET_KEY"] == "" { + t.Error("in-memory AppConfig not populated by the persist half") + } +} + +// TestPersistUnitRedeployConfigRejectsUnknownStack keeps the failure direction the same as the +// unsplit function's: an unknown stack is an error, not a silently-created app.yaml somewhere. +func TestPersistUnitRedeployConfigRejectsUnknownStack(t *testing.T) { + m, _ := newR47Manager(t) + if err := m.PersistUnitRedeployConfig("nope", map[string]string{"A": "b"}); err == nil { + t.Fatal("an unknown stack must be refused") + } +} diff --git a/controller/internal/web/async_restore_test.go b/controller/internal/web/async_restore_test.go index 92b52a2..9c15288 100644 --- a/controller/internal/web/async_restore_test.go +++ b/controller/internal/web/async_restore_test.go @@ -45,9 +45,10 @@ func (p *blockProvider) GetStackClassifiedBinds(string) ([]backup.ClassifiedBind return nil, false } func (p *blockProvider) RecoverStackSecrets(string, []string) map[string]string { return nil } -func (p *blockProvider) RecreateStackFromUnit(string, string, map[string]string) error { +func (p *blockProvider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { return nil } +func (p *blockProvider) StartStackServices(string, []string) error { return nil } func newAsyncRestoreServer(t *testing.T) (*Server, *blockProvider, *backup.Manager) { t.Helper()