From 9c945688c07c14504013fb38fc937165cb1b1055 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 6 Oct 2026 15:11:00 +0200 Subject: [PATCH] =?UTF-8?q?R-638=20option=20A:=20a=20replay=20meets=20the?= =?UTF-8?q?=20copy's=20own=20schema=20on=20the=20two=20side=20paths=20(09?= =?UTF-8?q?=20=C2=A73=20decision=20154)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Slice 1 — the no-manifest fallback RestoreApp no longer starts the WHOLE stack at the current definition before the replay (a newer app could migrate the restored data underneath it). New order: resolve DB services from the live compose -> stop -> volumes -> DB-only start (StartStackServices, the same helper the unit restore uses, R-47) -> replay -> full start -> health wait. A dump with no identifiable DB service is refused before any mutation (same gate and message as the unit and off-site paths). A failed volume leg skips the replay. restoreDockerVolumes now goes through the existing volumeReplayFrom seam (nil in production) so the order is testable without Docker. Slice 2 — a unit restore whose volume leg failed no longer calls the importer. Everything else on that failure path is unchanged: dataErr is returned as "completed with data errors", the unit's definition is written and the app is fully started. No loader change, no new delete step. Red-proofs in felhom.eu documentation/audits/design-build-2026-10-06/B/ (red-slice1-fallback-order.txt, red-slice2-no-replay-after-volume-failure.txt). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- .../backup/r638_restore_order_test.go | 252 ++++++++++++++++++ controller/internal/backup/restore.go | 97 +++++-- controller/internal/backup/restore_unit.go | 11 +- 3 files changed, 342 insertions(+), 18 deletions(-) create mode 100644 controller/internal/backup/r638_restore_order_test.go diff --git a/controller/internal/backup/r638_restore_order_test.go b/controller/internal/backup/r638_restore_order_test.go new file mode 100644 index 0000000..fc54718 --- /dev/null +++ b/controller/internal/backup/r638_restore_order_test.go @@ -0,0 +1,252 @@ +package backup + +import ( + "context" + "errors" + "io" + "log" + "path/filepath" + "strings" + "testing" +) + +// R-638 option A (09-update-architecture §3 decision 154) — a replay must meet the COPY's own schema. +// +// The loader overlays a copy on the live database: it only removes what the copy knows about. So a +// replay is only safe when the database it lands in holds the copy's own files, not a newer app's +// migrated schema. Measured 2026-10-06 on scratch 9202: the unit restore after a docmost 0.95 → 0.96 +// migration put back exactly the copy's 42 tables — the main path is safe because it restores the +// volume FIRST and replays with ONLY the database up. These tests pin the two side paths that did not: +// +// - Slice 1, the no-manifest fallback RestoreApp: it started the WHOLE stack at the CURRENT +// definition before the replay, so a newer app could migrate the old data underneath it. +// - Slice 2, the unit restore whose volume leg failed: it replayed anyway, over a database volume +// that is not the copy's own. +// +// All Docker-reaching seams are injected (volumeReplayFrom, discoverDBs, importDBDump); no test here +// touches a daemon. + +// r638FallbackFixture builds a Manager whose app "app" has NO recovery unit (so RestoreFromRecoveryUnit +// takes the RestoreApp fallback), a LIVE compose with the given body, and — optionally — a replayable +// dump at the app's on-drive dump path, which is where RestoreApp replays from. +func r638FallbackFixture(t *testing.T, compose string, withDump bool) (*Manager, *fakeRecoveryProvider, *[]string) { + t.Helper() + tmp := t.TempDir() + drive := filepath.Join(tmp, "drive") + stackDir := filepath.Join(tmp, "stack") + mustWrite(t, filepath.Join(stackDir, "docker-compose.yml"), compose) + if withDump { + mustWrite(t, filepath.Join(AppDBDumpPath(drive, "app"), "app-postgres.sql"), pgDump(1)) + } + prov := &fakeRecoveryProvider{hdd: drive, running: true, info: RecoveryInfo{StackDir: stackDir}} + m := &Manager{ + logger: log.New(io.Discard, "", 0), + systemDataPath: filepath.Join(tmp, "sys"), // != drive ⇒ nsRoot = drive + stackProvider: prov, + } + // The volume leg is a seam here so the test can never reach Docker, even if a tar appears. + m.volumeReplayFrom = func(string, string) (int, error) { return 0, nil } + 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 +} + +// --- Slice 1: the no-manifest fallback ------------------------------------------------------------ + +// TestR638_FallbackReplaysBeforeTheAppStarts is the slice-1 assertion. It captures the provider's +// state AT THE MOMENT the importer fires: the database service must be up and the full stack must +// NOT be — a full start at the current definition is exactly the window in which a newer app +// migrates the restored data before the old copy is poured over it. +// +// COMPANION RED-PROOF: on the pre-fix RestoreApp (StartStack before reimportDBDumpsCtx) this fails on +// `the FULL stack was already up when the replay fired` — saved in +// audits/design-build-2026-10-06/B/red-slice1-fallback-order.txt. +func TestR638_FallbackReplaysBeforeTheAppStarts(t *testing.T) { + m, prov, imported := r638FallbackFixture(t, immichLikeCompose, true) + + var dbUpAtReplay, fullUpAtReplay bool + var callsAtReplay string + m.importDBDump = func(_ context.Context, _ DiscoveredDB, p string) error { + dbUpAtReplay = len(prov.gotServices) > 0 + fullUpAtReplay = prov.fullStarted + callsAtReplay = strings.Join(prov.calls, ",") + *imported = append(*imported, p) + return nil + } + + if err := m.RestoreApp("app", ""); err != nil { + t.Fatalf("fallback restore: %v", err) + } + if len(*imported) != 1 { + t.Fatalf("expected exactly one replay, got %v", *imported) + } + if fullUpAtReplay { + t.Fatalf("the FULL stack was already up when the replay fired (calls before the replay: %q) — a newer app can migrate the restored data before the copy is loaded (R-638)", callsAtReplay) + } + if !dbUpAtReplay { + t.Fatal("the database service was NOT started before the replay — the importer has no container to talk to") + } + 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 → volumes → db-only start → replay → full start", got) + } +} + +// TestR638_FallbackThroughUnitRestoreKeepsTheOrder reaches the fallback the way production does: a +// unit restore that finds no manifest. Same ordering, and the counts stay UNKNOWN (not zero). +func TestR638_FallbackThroughUnitRestoreKeepsTheOrder(t *testing.T) { + m, prov, imported := r638FallbackFixture(t, immichLikeCompose, true) + + res, err := m.RestoreFromRecoveryUnit("app") + if err != nil { + t.Fatalf("restore: %v", err) + } + if !res.CountsUnknown { + t.Error("the fallback's counts must stay UNKNOWN, not zero") + } + if len(*imported) != 1 { + t.Fatalf("expected exactly one replay, got %v", *imported) + } + 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) + } +} + +// TestR638_FallbackNoDumpTakesOneFullStart is the negative: with nothing to replay there is no +// DB-only window, and the flow is the old stop → volumes → full start. +func TestR638_FallbackNoDumpTakesOneFullStart(t *testing.T) { + m, prov, imported := r638FallbackFixture(t, immichLikeCompose, false) + + if err := m.RestoreApp("app", ""); err != nil { + t.Fatalf("fallback restore: %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,start" { + t.Fatalf("sequence = %q, want the unchanged stop → full start", got) + } + if len(*imported) != 0 { + t.Fatalf("nothing should have been replayed, got %v", *imported) + } +} + +// TestR638_FallbackRefusesWhenNoDBServiceIdentifiable: a dump exists but the live compose names no +// database service that could be started alone. The only alternative is the old full start + replay +// — the R-638 shape — so it refuses, with ZERO mutations, exactly as the unit and off-site paths do. +func TestR638_FallbackRefusesWhenNoDBServiceIdentifiable(t *testing.T) { + m, prov, imported := r638FallbackFixture(t, noDBCompose, true) + volTouched := false + m.volumeReplayFrom = func(string, string) (int, error) { volTouched = true; return 0, nil } + + err := m.RestoreApp("app", "") + 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 volTouched { + t.Fatal("volumes were replaced despite the refusal") + } + if len(*imported) != 0 { + t.Fatalf("a replay happened despite the refusal: %v", *imported) + } +} + +// TestR638_FallbackVolumeFailureSkipsTheReplay is slice 2's rule on the fallback path: when the +// volume leg failed, the database volume is not known to hold the copy's own files, so the copy is +// NOT poured over it. The failure is still returned and the app is still brought back up. +func TestR638_FallbackVolumeFailureSkipsTheReplay(t *testing.T) { + m, prov, imported := r638FallbackFixture(t, immichLikeCompose, true) + m.volumeReplayFrom = func(string, string) (int, error) { + return 0, errors.New("failed to restore 1 volume(s): [app_db]") + } + + err := m.RestoreApp("app", "") + if err == nil || !strings.Contains(err.Error(), "completed with data errors") || !strings.Contains(err.Error(), "app_db") { + t.Fatalf("the volume failure must surface as the data-error outcome naming the volume, got: %v", err) + } + if len(*imported) != 0 { + t.Fatalf("the importer ran after a failed volume leg: %v", *imported) + } + if !prov.fullStarted { + t.Fatal("the app was left down after the failure — today's failure path brings it back up") + } + if got := strings.Join(prov.calls, ","); got != "stop,start" { + t.Fatalf("sequence = %q, want stop → full start (no DB-only window, no replay)", got) + } +} + +// --- Slice 2: unit restore with a failed volume leg ----------------------------------------------- + +// TestR638_UnitVolumeFailureNeverCallsTheImporter is the slice-2 assertion. A volume leg that errors +// means the database's volume may still be the LIVE (possibly newer, migrated) one, or a half-filled +// one — the copy overlays it and leaves whatever it does not know about. So the importer must not run. +// +// What must NOT change from today's failure path: the error reaches the caller as "completed with data +// errors" (no silent success), the definition is the unit's, and the app is brought back up. +// +// COMPANION RED-PROOF: on the pre-fix restore_unit.go (dataErr set, then fall through to the replay) +// this fails on `the importer ran after a failed volume leg` — saved in +// audits/design-build-2026-10-06/B/red-slice2-no-replay-after-volume-failure.txt. +func TestR638_UnitVolumeFailureNeverCallsTheImporter(t *testing.T) { + m, prov, imported := r47UnitFixture(t, immichLikeCompose, true) + m.volumeReplayFrom = func(string, string) (int, error) { + return 1, errors.New("failed to restore 1 volume(s): [app_immich_postgres_data]") + } + + res, err := m.RestoreFromRecoveryUnit("app") + if len(*imported) != 0 { + t.Fatalf("the importer ran after a failed volume leg: %v", *imported) + } + if err == nil { + t.Fatal("a failed volume leg must be surfaced, not reported as success") + } + if !strings.Contains(err.Error(), "completed with data errors") || !strings.Contains(err.Error(), "app_immich_postgres_data") { + t.Fatalf("the pre-existing outcome must be preserved and name the failed volume, got: %v", err) + } + if res.VolumesReplayed != 1 { + t.Errorf("the partial volume count must still be reported, got %d", res.VolumesReplayed) + } + if res.DBsReplayed != 0 { + t.Errorf("no database was replayed, but the result says %d", res.DBsReplayed) + } + if prov.gotEnv == nil { + t.Error("the unit's definition must still be written, as on today's failure path") + } + if !prov.fullStarted { + t.Fatal("the app was left down — today's failure path brings it back up") + } + if got := strings.Join(prov.calls, ","); got != "stop,recreate,start" { + t.Fatalf("sequence = %q, want stop → recreate → full start (no DB-only window, no replay)", got) + } +} + +// TestR638_UnitVolumeSuccessStillReplays is the positive control for slice 2: the skip is keyed on the +// volume FAILURE, not on the presence of volumes — a clean volume leg still replays. +func TestR638_UnitVolumeSuccessStillReplays(t *testing.T) { + m, prov, imported := r47UnitFixture(t, immichLikeCompose, true) + m.volumeReplayFrom = func(string, string) (int, error) { return 1, nil } + + res, err := m.RestoreFromRecoveryUnit("app") + if err != nil { + t.Fatalf("restore: %v", err) + } + if len(*imported) != 1 || res.DBsReplayed != 1 { + t.Fatalf("a clean volume leg must still replay: imported=%v replayed=%d", *imported, res.DBsReplayed) + } + if got := strings.Join(prov.calls, ","); got != "stop,recreate,startsvc:immich-postgres,start" { + t.Fatalf("sequence = %q", got) + } +} diff --git a/controller/internal/backup/restore.go b/controller/internal/backup/restore.go index 2be92f5..9a91bb2 100644 --- a/controller/internal/backup/restore.go +++ b/controller/internal/backup/restore.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "gitea.dooplex.hu/admin/felhom-controller/internal/dockerexec" + "gitea.dooplex.hu/admin/felhom-controller/internal/util" "os" "strings" "time" @@ -16,6 +17,10 @@ import ( // (AppVolumeDumpPath) and relies on the DB dumps already present on the app's drive. // The stack is stopped before the volume import and restarted after. // +// R-638: the order is stop → volumes → DB-only start → replay → full start, the same as the unit +// restore (R-47). The replay is skipped when the volume leg failed, and a dump with no identifiable +// database service is refused before anything is touched. +// // snapshotID is retained for API/UI signature compatibility; with restic removed it // is only used for logging (the source of truth is now the on-disk volume tars). func (m *Manager) RestoreApp(stackName, snapshotID string) error { @@ -48,9 +53,35 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { m.logger.Printf("[INFO] [backup] Starting app-data restore for %s (drive=%s)", stackName, drivePath) + // R-638 (option A, 09 §3 decision 154): which compose service holds the database, and is there + // anything to replay? Resolved BEFORE the first mutation, from the LIVE compose — this fallback has no + // unit, so the live definition is the one that will run — exactly as the off-site path does for an app + // that keeps its definition (offbox_reconstitute.go, "WHICH SERVICE HOLDS THE DATABASE"). + dumpDir := AppDBDumpPath(m.namespaceRoot(drivePath), stackName) + hasDumps := hasReplayableDump(dumpDir) + var dbServices []string + if hasDumps { + if composePath, ok := m.stackProvider.GetStackComposePath(stackName); ok && composePath != "" { + svcs, dsErr := DBServiceNames(composePath) + if dsErr != nil { + // "cannot tell" is not "no database" — leave it empty and let the gate below refuse. + m.logger.Printf("[WARN] [backup] %s: could not read the live compose services: %v", stackName, dsErr) + } + dbServices = svcs + } + // Fail-closed, the same gate as the unit and off-site paths: the only alternative to refusing is + // to start the WHOLE stack and replay into it — the R-638 shape, where a newer app migrates the + // restored data before the old copy is poured over it. Refused with the live app untouched. + // Pinned by TestR638_FallbackRefusesWhenNoDBServiceIdentifiable. + if len(dbServices) == 0 { + m.logger.Printf("[ERROR] [backup] Restore REFUSED for %s: a .sql dump exists but no database service is identifiable in the live compose", stackName) + return util.MsgError("err.backup.az_adatbazis_szolgaltatas_nem_azonosithato_a", stackName) + } + } + // Stop the app before restore if m.isDebug() { - m.logger.Printf("[DEBUG] RestoreApp: step 1/3 — stopping app %s", stackName) + m.logger.Printf("[DEBUG] RestoreApp: step 1/4 — stopping app %s", stackName) } if err := m.stackProvider.StopStack(stackName); err != nil { m.logger.Printf("[WARN] RESTORE could not stop %s: %v (proceeding anyway)", stackName, err) @@ -62,30 +93,56 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { // Populate Docker volumes from restored tars if m.isDebug() { - m.logger.Printf("[DEBUG] RestoreApp: step 2/3 — restoring Docker volumes for %s", stackName) + m.logger.Printf("[DEBUG] RestoreApp: step 2/4 — restoring Docker volumes for %s", stackName) } - if _, err := m.restoreDockerVolumes(stackName, drivePath); err != nil { - m.logger.Printf("[ERROR] RESTORE volume restore failed for %s: %v", stackName, err) - dataErr = err + _, volErr := m.restoreDockerVolumes(stackName, drivePath) + if volErr != nil { + m.logger.Printf("[ERROR] RESTORE volume restore failed for %s: %v", stackName, volErr) + dataErr = volErr } - // Restart the app + // F17: replay the captured .sql dump (the legacy path never did this, so DB-resident data did not + // come back). Runs after the volume restore so the dump WINS over any tar copy. + // + // R-638: with ONLY the database service up — the same DB-only start the unit restore uses (R-47). + // This used to run after a FULL StartStack at the CURRENT definition, which gave a newer app the + // window to migrate the just-restored data; the loader then overlays the copy and removes only what + // the copy knows about, so the newer tables stayed behind (measured 2026-09-23, romm 5.0 → 5.3). + // Pinned by TestR638_FallbackReplaysBeforeTheAppStarts. + // + // And NOT when the volume leg failed: the database's volume is then not known to hold the copy's + // own files (it may still be the live, newer one), and the overlay would leave the difference + // behind. The failure is already in dataErr and is returned below. Pinned by + // TestR638_FallbackVolumeFailureSkipsTheReplay. + if hasDumps { + if volErr != nil { + m.logger.Printf("[ERROR] [backup] RESTORE %s: NOT replaying the database copy — the volume restore failed, so the database is not known to be the copy's own", stackName) + } else { + if m.isDebug() { + m.logger.Printf("[DEBUG] RestoreApp: step 3/4 — starting only %v and replaying the DB copy for %s", dbServices, stackName) + } + 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, m.namespaceRoot(drivePath)); err != nil { + m.logger.Printf("[ERROR] RESTORE DB re-import failed for %s: %v", stackName, err) + if dataErr == nil { + dataErr = err + } + } + } + } + + // Restart the app — every exit from the DB-only window ends in a full start. if m.isDebug() { - m.logger.Printf("[DEBUG] RestoreApp: step 3/3 — restarting app %s after restore", stackName) + m.logger.Printf("[DEBUG] RestoreApp: step 4/4 — starting app %s after restore", stackName) } if err := m.stackProvider.StartStack(stackName); err != nil { m.logger.Printf("[WARN] RESTORE could not restart %s after restore: %v", stackName, err) } - // F17: replay the captured .sql dump into the now-running DB (the legacy path never did this, so - // DB-resident data did not come back). Runs after volume restore so the dump WINS over any tar copy. - if _, err := m.reimportDBDumpsCtx(stackName, m.namespaceRoot(drivePath)); err != nil { - m.logger.Printf("[ERROR] RESTORE DB re-import failed for %s: %v", stackName, err) - if dataErr == nil { - dataErr = err - } - } - // Verify app started successfully if err := m.waitForHealthy(stackName, healthTimeout); err != nil { m.logger.Printf("[WARN] [backup] Restore completed but app health check failed: %v", err) @@ -109,7 +166,13 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { // re-derive: the caller cannot count volumes afterwards without re-reading the directory the restore // has already consumed. func (m *Manager) restoreDockerVolumes(stackName, drivePath string) (int, error) { - return m.restoreDockerVolumesFrom(stackName, AppVolumeDumpPath(m.namespaceRoot(drivePath), stackName)) + // R-638: through the same volume-replay seam the unit and off-site paths use (R-354), so the + // fallback's ordering can be tested without a Docker daemon. Nil in production. + replay := m.volumeReplayFrom + if replay == nil { + replay = m.restoreDockerVolumesFrom + } + return replay(stackName, AppVolumeDumpPath(m.namespaceRoot(drivePath), stackName)) } // restoreDockerVolumesFrom is restoreDockerVolumes with an EXPLICIT dump directory, and it returns how diff --git a/controller/internal/backup/restore_unit.go b/controller/internal/backup/restore_unit.go index a874cb8..8c20fec 100644 --- a/controller/internal/backup/restore_unit.go +++ b/controller/internal/backup/restore_unit.go @@ -449,7 +449,16 @@ func (m *Manager) RestoreFromRecoveryUnitAtWith(stackName, unitDir string, opt U // 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 { + // + // R-638 (option A, 09 §3 decision 154): NOT when the volume leg failed. The replay overlays the copy + // and removes only what the copy knows about, so it is safe only over the copy's OWN database files + // — which a failed volume leg does not guarantee (the volume may still be the live, newer one, or + // half-filled). The failure is already in dataErr and returned below; the definition is still the + // unit's and the app is still started, exactly as before. Pinned by + // TestR638_UnitVolumeFailureNeverCallsTheImporter. + if hasDumps && volErr != nil { + m.logger.Printf("[ERROR] [backup] %s: NOT replaying the database copy — the volume restore failed, so the database is not known to be the copy's own", stackName) + } else 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 {