Files
felhom-controller/controller/internal/web/restore_wizard_test.go
T
admin 9d1b4983f5 v0.155.0 — the restore wizard read the wrong "is something running" flag
Fixes a defect shipped in v0.154.0, found by the operator on the first live
click-through of the new wizard.

backup.Manager carries TWO running booleans. `running` (read by IsRunning) is the
concurrency single-flight, acquired inside the background goroutine — and
RestoreOffboxScratch never acquires it at all. `opRunning` (read by RestoreStatus) is the
display flag, set synchronously by BeginRestoreOp in the handler.

The wizard sourced OpRunning from IsRunning(), so for „Ellenőrzés" and the full-restore
preparation — its two most-used and longest actions, both streaming from restic — the
execution step was unreachable: the page offered all three intents with live buttons
while a restore was running, and the progress banner contradicted the phase strip on the
same screen. Pressing anything there would have been refused by the handler, which is the
exact "offering a control guaranteed to fail" dishonesty R-48 exists to remove.

Fix: restoreOpInFlight(st) behind a documented seam, fed by a SINGLE RestoreStatus() read
per render so the strip, the suppression decision and the running-op name cannot diverge.

Why the tests missed it: the Scenario-E table proved deriveWizardStep behaves correctly
GIVEN OpRunning=true, but nothing proved the handler ever computes true — hollow at
exactly that seam. TestRestoreOpInFlight_UsesDisplayFlagNotConcurrencyFlag now drives a
real Manager through BeginRestoreOp and asserts the render suppresses every form.
Red-proofed against the v0.154.0 shape.

Also: „Eredmény" was a dead label. The strip's highlight is now its own derived Phase,
separate from Step — a finished restore returns to the intent step (everything available
again) while the strip reads „Eredmény" and an outcome card shows the result. Bounded by
restoreResultWindow (10 min) so a stale result cannot look fresh, and bound to the app so
a finished bookstack restore does not light immich's page with bookstack's message. The
card survives a reload; the redirect flash does not.

No new agent coupling — MinAgent stays 0.90.0.
2026-07-21 09:15:56 +02:00

472 lines
21 KiB
Go

package web
import (
"io"
"log"
"net/http/httptest"
"path/filepath"
"regexp"
"sort"
"strings"
"testing"
"time"
"gitea.dooplex.hu/admin/felhom-controller/internal/backup"
"gitea.dooplex.hu/admin/felhom-controller/internal/config"
"gitea.dooplex.hu/admin/felhom-controller/internal/settings"
)
// R-48 — the offsite restore wizard.
//
// What these tests actually guard is the finding's RULE, not the page: two adjacent controls whose
// difference is "your data comes back" vs "your data cannot come back" must not be distinguishable
// only by layout. Scenario A is therefore asserted STRUCTURALLY (the mutation forms are absent from
// the list page and present only in the wizard), not by eyeballing copy.
// wizardData builds the wizard template's data map. Mirrors what backupsRestoreWizardHandler puts
// there — the handler's own construction is exercised separately by the redirect tests.
func wizardData(app string, view restoreWizardView, pair backup.OffsitePairInfo) map[string]interface{} {
return map[string]interface{}{
"Page": "backups-restore", "Title": "Visszaállítás",
"Backup": &backup.FullBackupStatus{},
"App": app,
"AppDisplayName": strings.ToUpper(app[:1]) + app[1:],
"AppSlug": app,
"Wizard": view,
"Pair": pair,
"FullPrepSize": "1,2 GB",
"RunningStack": "",
}
}
func renderWizard(t *testing.T, data map[string]interface{}) string {
t.Helper()
return renderBackupPage(t, "backups_restore_wizard", data)
}
// --- Group B (Scenario B): the step derivation is server truth, and PURE ---------------------------
//
// COMPANION RED-PROOF (run + recorded in REPORT.md): replace the body of deriveWizardStep with the
// trivial `return restoreWizardView{Step: wizStepIntent, VerifyEnabled: true}` — the op-running,
// prepare-confirm and scratch-ready rows all FAIL. Restore → green.
func TestDeriveWizardStep_Table(t *testing.T) {
cases := []struct {
name string
in restoreWizardInput
want restoreWizardView
}{
{
name: "no scratch, no op → intent; only verification is offered, full restore must be prepared first",
in: restoreWizardInput{App: "immich"},
want: restoreWizardView{Step: wizStepIntent, Phase: wizPhasePrepare, VerifyEnabled: true, PrepareEnabled: true},
},
{
name: "scratch ready → intent, and BOTH data-touching intents unlock; preparation is done",
in: restoreWizardInput{App: "immich", ScratchReady: true},
want: restoreWizardView{Step: wizStepIntent, Phase: wizPhasePrepare, VerifyEnabled: true, PlaceEnabled: true, RestoreEnabled: true},
},
{
name: "full_prep flash for THIS app → prepare-confirm; only the commit is offered",
in: restoreWizardInput{App: "immich", FullPrepApp: "immich"},
want: restoreWizardView{Step: wizStepPrepareConfirm, Phase: wizPhaseConfirm, CommitPrepareEnabled: true},
},
{
name: "full_prep flash for ANOTHER app → this app keeps its own intent step",
in: restoreWizardInput{App: "immich", FullPrepApp: "bookstack"},
want: restoreWizardView{Step: wizStepIntent, Phase: wizPhasePrepare, VerifyEnabled: true, PrepareEnabled: true},
},
{
name: "op running (this app) → execution; nothing offered",
in: restoreWizardInput{App: "immich", OpRunning: true},
want: restoreWizardView{Step: wizStepExecution, Phase: wizPhaseExecute},
},
{
name: "op running for ANOTHER app still suppresses THIS app (the single-flight is process-wide)",
in: restoreWizardInput{App: "immich", OpRunning: true, ScratchReady: true},
want: restoreWizardView{Step: wizStepExecution, Phase: wizPhaseExecute},
},
{
name: "a just-finished restore returns to intent, but the strip says Eredmény",
in: restoreWizardInput{App: "immich", ScratchReady: true, HasRecentResult: true},
want: restoreWizardView{Step: wizStepIntent, Phase: wizPhaseResult, VerifyEnabled: true, PlaceEnabled: true, RestoreEnabled: true},
},
{
name: "a running op outranks a recent result — Végrehajtás, not Eredmény",
in: restoreWizardInput{App: "immich", OpRunning: true, HasRecentResult: true},
want: restoreWizardView{Step: wizStepExecution, Phase: wizPhaseExecute},
},
{
name: "op running OUTRANKS a stale full_prep flash — no commit button mid-restore",
in: restoreWizardInput{App: "immich", OpRunning: true, FullPrepApp: "immich"},
want: restoreWizardView{Step: wizStepExecution, Phase: wizPhaseExecute},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := deriveWizardStep(tc.in)
if got != tc.want {
t.Errorf("deriveWizardStep(%+v)\n got %+v\n want %+v", tc.in, got, tc.want)
}
})
}
}
// --- Group C (Scenario B error rows): refusals resolve, never 500 ----------------------------------
func TestResolveWizardApp_Refusals(t *testing.T) {
rows := []OffboxAppRow{
{Name: "immich", DisplayName: "Immich", Enabled: true},
{Name: "radarr", DisplayName: "Radarr", Enabled: false},
}
if got := resolveWizardApp(rows, "immich"); got == nil || got.DisplayName != "Immich" {
t.Fatalf("toggled app must resolve, got %+v", got)
}
if got := resolveWizardApp(rows, "radarr"); got != nil {
t.Errorf("an app that is NOT toggled for offsite has no snapshot to restore from — want nil, got %+v", got)
}
if got := resolveWizardApp(rows, "does-not-exist"); got != nil {
t.Errorf("unknown app must not resolve, got %+v", got)
}
if got := resolveWizardApp(nil, "immich"); got != nil {
t.Errorf("empty set must not resolve, got %+v", got)
}
}
// The customer-visible URL must redirect, not 500, when the offsite target is not configured at all.
func TestRestoreWizardHandler_UnconfiguredRedirects(t *testing.T) {
s := testServer(t)
rec := httptest.NewRecorder()
req := httptest.NewRequest("GET", "/backups/restore/app?name=immich", nil)
s.backupsRestoreWizardHandler(rec, req)
if rec.Code != 302 {
t.Fatalf("want 302 redirect, got %d", rec.Code)
}
loc := rec.Header().Get("Location")
if !strings.HasPrefix(loc, "/backups/restore?flash_error=") {
t.Errorf("must redirect to the list with an error flash, got %q", loc)
}
}
// --- Group A + C (Scenarios A and C): one entry on the list, three described cards in the wizard ----
// Scenario A — the list page carries EXACTLY ONE restore control per app and ZERO offsite mutation
// forms. This is the finding itself: the five inline forms are gone from the row.
func TestRestoreList_SingleEntryPerApp(t *testing.T) {
data := splitTestData()
data["OffboxScratchReady"] = map[string]bool{"calibre-web": true}
data["OffboxPairInfo"] = map[string]backup.OffsitePairInfo{
"calibre-web": {Ready: true, HasDump: true, DumpsAt: time.Now().Add(-2 * time.Hour)},
}
html := renderBackupPage(t, "backups_restore", data)
// The scratch-ready fixture is precisely the state in which the OLD page rendered place and
// reconstitute as adjacent siblings. None of them may appear here now.
for _, banned := range []string{
`action="/backup/offbox/restore"`,
`action="/backup/offbox/place"`,
`action="/backup/offbox/reconstitute"`,
"Helyreállítás az élő adatok közé",
"Teljes visszaállítás (fájlok + adatbázis)",
"Teljes visszaállítás előkészítése",
} {
if strings.Contains(html, banned) {
t.Errorf("R-48 violated: the list page still renders %q", banned)
}
}
if n := strings.Count(html, `href="/backups/restore/app?name=calibre-web"`); n != 1 {
t.Errorf("want exactly ONE wizard entry for the app, got %d", n)
}
}
// Scenario C — the three intents are CARDS with their own consequence sentence, the dangerous one is
// styled as such, and the honesty panel is bound to the real pair info.
func TestRestoreWizard_ThreeIntentCards(t *testing.T) {
pair := backup.OffsitePairInfo{
Ready: true, HasDump: true,
DumpsAt: time.Date(2026, 7, 19, 3, 15, 0, 0, time.UTC),
Skewed: true, LooksEmpty: true,
}
view := deriveWizardStep(restoreWizardInput{App: "immich", ScratchReady: true})
html := renderWizard(t, wizardData("immich", view, pair))
// Each intent must state its CONSEQUENCE, not just its name.
for _, want := range []string{
"Ellenőrzés külön mappába",
"az élő adataid nem változnak",
"Hiányzó fájlok visszahozása",
"törölt tartalom ettől nem jelenik meg újra",
"Teljes visszaállítás (fájlok + adatbázis)",
"az adatbázist is visszatölti",
} {
if !strings.Contains(html, want) {
t.Errorf("intent card copy missing: %q", want)
}
}
// The dangerous intent is marked structurally, not only by wording/position.
if !strings.Contains(html, "restore-danger-card") {
t.Error("the full-restore card must carry the danger styling hook")
}
// Pair honesty is BOUND to the fixture, not hardcoded prose.
if !strings.Contains(html, "eltérő időpontból származnak") {
t.Error("Skewed pair must surface the skew warning")
}
if !strings.Contains(html, "üresnek tűnik") {
t.Error("LooksEmpty pair must surface the empty-dump warning")
}
if !strings.Contains(html, `data-restore-skewed="1"`) || !strings.Contains(html, `data-restore-empty="1"`) {
t.Error("the double-confirm must receive the pair facts it repeats back")
}
// The confirm copy moved VERBATIM — it is the good part of the old surface.
if !strings.Contains(html, "UTOLSÓ MEGERŐSÍTÉS") || !strings.Contains(html, "confirmFullRestore") {
t.Error("the double-confirm did not move with the action")
}
// A pair WITHOUT the warnings must not inherit them (guards a hardcoded-prose regression).
// NOTE: the confirm helper's JS repeats both sentences as string literals, so the assertion has
// to be on the RENDERED markup — the warning banners and the data-attributes that drive them —
// not on a bare substring, which is always present via the script block.
clean := renderWizard(t, wizardData("immich", view, backup.OffsitePairInfo{Ready: true, HasDump: true}))
// The two banners are identified by their opening markup, not by a bare substring: the layout's
// shared confirm helper also contains alert-warning literals.
const skewBanner = `alert alert-warning" style="margin-bottom:.75rem">Az adatbázis-mentés régebbi`
const emptyBanner = `alert alert-warning" style="margin-bottom:.75rem">A mentett adatbázis üresnek tűnik`
if !strings.Contains(html, skewBanner) || !strings.Contains(html, emptyBanner) {
t.Error("a skewed + empty-looking pair must render BOTH warning banners")
}
if strings.Contains(clean, skewBanner) || strings.Contains(clean, emptyBanner) {
t.Error("a clean pair must render neither warning banner")
}
if !strings.Contains(clean, `data-restore-skewed=""`) || !strings.Contains(clean, `data-restore-empty=""`) {
t.Error("a clean pair must pass empty skew/empty flags to the confirm helper")
}
}
// Without a prepared scratch the two data-touching intents are NOT offered — the card explains what
// has to happen first instead of showing a button that would fail.
func TestRestoreWizard_NoScratchLocksDataIntents(t *testing.T) {
view := deriveWizardStep(restoreWizardInput{App: "immich"})
html := renderWizard(t, wizardData("immich", view, backup.OffsitePairInfo{}))
if strings.Contains(html, `action="/backup/offbox/place"`) {
t.Error("missing-only merge must not be offered without a prepared scratch")
}
if strings.Contains(html, `action="/backup/offbox/reconstitute"`) {
t.Error("full restore must not be offered without a prepared scratch")
}
if !strings.Contains(html, "Teljes visszaállítás előkészítése") {
t.Error("the full-restore card must offer preparation instead")
}
}
// --- Group E (Scenario B, execution row): every mutation form suppressed while an op runs ----------
func TestRestoreWizard_OpRunningSuppressesAllMutations(t *testing.T) {
view := deriveWizardStep(restoreWizardInput{App: "immich", OpRunning: true, ScratchReady: true, FullPrepApp: "immich"})
data := wizardData("immich", view, backup.OffsitePairInfo{Ready: true, HasDump: true})
data["RunningStack"] = "bookstack"
html := renderWizard(t, data)
if strings.Contains(html, "<form") {
t.Error("the execution step must render NO mutation form — the single-flight would refuse it")
}
if !strings.Contains(html, "bookstack") {
t.Error("the execution card must name what is actually running")
}
}
// --- Group D (Scenario D): real flows only — no new mutation surface ------------------------------
var formActionRe = regexp.MustCompile(`action="([^"]+)"`)
// Every form in every wizard state posts to a PRE-EXISTING endpoint. If this test has to be updated
// to add a path, a new mutation endpoint was introduced — which R-48 explicitly does not do.
func TestRestoreWizard_NoNewMutationEndpoints(t *testing.T) {
preExisting := map[string]bool{
"/backup/offbox/restore": true,
"/backup/offbox/place": true,
"/backup/offbox/reconstitute": true,
}
states := []restoreWizardInput{
{App: "immich"},
{App: "immich", ScratchReady: true},
{App: "immich", FullPrepApp: "immich"},
{App: "immich", OpRunning: true},
}
seen := map[string]bool{}
for _, in := range states {
html := renderWizard(t, wizardData(in.App, deriveWizardStep(in), backup.OffsitePairInfo{Ready: true, HasDump: true}))
for _, m := range formActionRe.FindAllStringSubmatch(html, -1) {
seen[m[1]] = true
if !preExisting[m[1]] {
t.Errorf("wizard posts to a NON-pre-existing endpoint %q — R-48 adds no mutation surface", m[1])
}
}
}
var got []string
for p := range seen {
got = append(got, p)
}
sort.Strings(got)
if len(got) != 3 {
t.Errorf("expected all three existing endpoints to be reachable across the states, got %v", got)
}
}
// The field NAMES the wizard sends must match what the handlers read — a renamed field would make
// every action a silent no-op that still redirects with a success-shaped flash.
func TestRestoreWizard_FieldContract(t *testing.T) {
// intent step, scratch ready: unit-verify + place + reconstitute(confirm=1)
html := renderWizard(t, wizardData("immich",
deriveWizardStep(restoreWizardInput{App: "immich", ScratchReady: true}),
backup.OffsitePairInfo{Ready: true, HasDump: true}))
for _, want := range []string{
`<input type="hidden" name="app" value="immich">`,
`<input type="hidden" name="mode" value="unit">`,
`<input type="hidden" name="confirm" value="1">`,
} {
if !strings.Contains(html, want) {
t.Errorf("field contract broken, missing: %s", want)
}
}
// prepare-confirm step: mode=full AND confirm=1 together (the size-gated commit)
confirmHTML := renderWizard(t, wizardData("immich",
deriveWizardStep(restoreWizardInput{App: "immich", FullPrepApp: "immich"}),
backup.OffsitePairInfo{}))
if !strings.Contains(confirmHTML, `name="mode" value="full"`) || !strings.Contains(confirmHTML, `name="confirm" value="1"`) {
t.Error("the size-gated commit must post mode=full together with confirm=1")
}
if !strings.Contains(confirmHTML, "1,2 GB") {
t.Error("the confirm step must show the measured size before the customer commits")
}
}
// --- The v0.154.0 escape: the handler read the WRONG "is something running" flag -----------------
//
// The Scenario-E table test above proves deriveWizardStep behaves correctly GIVEN OpRunning=true.
// Nothing proved the handler ever COMPUTES OpRunning=true — and it didn't, for the wizard's most-used
// action. `Manager` carries two booleans: `running` (concurrency, acquired inside the goroutine, and
// `RestoreOffboxScratch` never acquires it at all) and `opRunning` (display, set synchronously by
// `BeginRestoreOp`). v0.154.0 read the first via `IsRunning()`, so during a verification restore the
// page offered all three intents with live buttons while the progress banner on the same screen said
// the restore was in progress. Found by the operator on the first live click-through.
//
// COMPANION RED-PROOF (run + recorded in REPORT.md): point restoreOpInFlight at m.IsRunning() —
// this test FAILS with inFlight=false while a restore op is live.
func TestRestoreOpInFlight_UsesDisplayFlagNotConcurrencyFlag(t *testing.T) {
tmp := t.TempDir()
lg := log.New(io.Discard, "", 0)
sett, err := settings.Load(filepath.Join(tmp, "settings.json"), lg)
if err != nil {
t.Fatal(err)
}
cfg := &config.Config{}
cfg.Paths.DataDir = tmp
m := backup.NewManager(cfg, sett, lg)
if restoreOpInFlight(m.RestoreStatus()) {
t.Fatal("idle manager must not report an op in flight")
}
// EXACTLY what offboxRestoreHandler does for a verification restore: mark the op, then launch.
// RestoreOffboxScratch never acquires the concurrency flag, so IsRunning() stays false here —
// which is precisely why reading it was wrong.
m.BeginRestoreOp("offbox-restore", "immich")
if m.IsRunning() {
t.Fatal("precondition changed: BeginRestoreOp now sets the concurrency flag too — revisit this test")
}
if !restoreOpInFlight(m.RestoreStatus()) {
t.Fatal("a started restore op MUST read as in-flight for display (this is the v0.154.0 bug)")
}
// …and the wizard must therefore suppress every mutation form.
view := deriveWizardStep(restoreWizardInput{App: "immich", OpRunning: restoreOpInFlight(m.RestoreStatus()), ScratchReady: true})
if view.Step != wizStepExecution {
t.Fatalf("wizard must render the execution step during a restore, got %q", view.Step)
}
html := renderWizard(t, wizardData("immich", view, backup.OffsitePairInfo{Ready: true, HasDump: true}))
if strings.Contains(html, "<form") {
t.Error("no mutation form may render while a restore op is in flight")
}
m.EndRestoreOp(true, "kész")
if restoreOpInFlight(m.RestoreStatus()) {
t.Error("a finished op must clear the in-flight display state")
}
}
// hasRecentRestoreResult decides whether „Eredmény" lights up. Two ways it could lie: showing a
// stale result forever (no bound), and showing ANOTHER app's result on this app's page (the op
// status is process-wide). Both are asserted here.
func TestHasRecentRestoreResult(t *testing.T) {
now := time.Date(2026, 7, 21, 12, 0, 0, 0, time.UTC)
res := func(stack string, ago time.Duration) *backup.RestoreOpResult {
return &backup.RestoreOpResult{Op: "offbox-restore", Stack: stack, OK: true,
Message: "kész", FinishedAt: now.Add(-ago)}
}
cases := []struct {
name string
st backup.RestoreOpStatus
app string
want bool
}{
{"just finished, this app", backup.RestoreOpStatus{Last: res("immich", time.Minute)}, "immich", true},
{"finished long ago — the strip must not claim a fresh result",
backup.RestoreOpStatus{Last: res("immich", 2*time.Hour)}, "immich", false},
{"ANOTHER app's result must not light this app's page",
backup.RestoreOpStatus{Last: res("bookstack", time.Minute)}, "immich", false},
{"still running — Végrehajtás owns the strip, not Eredmény",
backup.RestoreOpStatus{Running: true, Last: res("immich", time.Minute)}, "immich", false},
{"no result at all", backup.RestoreOpStatus{}, "immich", false},
{"zero FinishedAt is not a result", backup.RestoreOpStatus{
Last: &backup.RestoreOpResult{Stack: "immich", OK: true}}, "immich", false},
{"exactly at the window boundary is stale (half-open)",
backup.RestoreOpStatus{Last: res("immich", restoreResultWindow)}, "immich", false},
{"one tick inside the window is fresh",
backup.RestoreOpStatus{Last: res("immich", restoreResultWindow-time.Second)}, "immich", true},
{"a clock skew into the future must not count as recent",
backup.RestoreOpStatus{Last: res("immich", -time.Minute)}, "immich", false},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
if got := hasRecentRestoreResult(tc.st, tc.app, now); got != tc.want {
t.Errorf("hasRecentRestoreResult = %v, want %v", got, tc.want)
}
})
}
}
// The result card renders the outcome and is bound to the real result — and a FAILED restore must
// not borrow the success styling.
func TestRestoreWizard_ResultCard(t *testing.T) {
view := deriveWizardStep(restoreWizardInput{App: "immich", ScratchReady: true, HasRecentResult: true})
data := wizardData("immich", view, backup.OffsitePairInfo{Ready: true, HasDump: true})
data["LastResult"] = &backup.RestoreOpResult{
Op: "offbox-restore", Stack: "immich", OK: true,
Message: "A(z) immich visszaállítva ellenőrző mappába: /mnt/felhom-drives/hdd_1/backups/offsite-restore/immich",
FinishedAt: time.Date(2026, 7, 21, 9, 12, 0, 0, time.UTC),
}
html := renderWizard(t, data)
if !strings.Contains(html, "offsite-restore/immich") {
t.Error("the result card must show the real outcome message, naming where the copy landed")
}
if !strings.Contains(html, "alert alert-info") {
t.Error("a successful result must render in the neutral/info tone")
}
data["LastResult"] = &backup.RestoreOpResult{Op: "offbox-restore", Stack: "immich", OK: false,
Message: "A visszaállítás sikertelen: nincs elég hely", FinishedAt: time.Date(2026, 7, 21, 9, 12, 0, 0, time.UTC)}
fail := renderWizard(t, data)
if !strings.Contains(fail, "alert alert-error") {
t.Error("a FAILED restore must render in the error tone, not the success one")
}
if strings.Contains(fail, `alert alert-info">A visszaállítás sikertelen`) {
t.Error("failure message rendered with success styling")
}
// No recent result -> no card at all.
plain := renderWizard(t, wizardData("immich",
deriveWizardStep(restoreWizardInput{App: "immich", ScratchReady: true}),
backup.OffsitePairInfo{Ready: true, HasDump: true}))
if strings.Contains(plain, "<h3>Eredmény</h3>") {
t.Error("no result card may render without a recent result")
}
}