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
parent 70cb21b058
commit 9d1b4983f5
7 changed files with 320 additions and 21 deletions
@@ -1,7 +1,10 @@
 package web

 import (
+	"io"
+	"log"
 	"net/http/httptest"
+	"path/filepath"
 	"regexp"
 	"sort"
 	"strings"
@@ -9,6 +12,8 @@ import (
 	"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.
@@ -53,37 +58,47 @@ func TestDeriveWizardStep_Table(t *testing.T) {
 		{
 			name: "no scratch, no op → intent; only verification is offered, full restore must be prepared first",
 			in:   restoreWizardInput{App: "immich"},
-			want: restoreWizardView{Step: wizStepIntent, VerifyEnabled: true, PrepareEnabled: true},
+			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, VerifyEnabled: true, PlaceEnabled: true, RestoreEnabled: 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, CommitPrepareEnabled: true},
+			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, VerifyEnabled: true, PrepareEnabled: true},
+			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},
+			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},
+			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},
+			want: restoreWizardView{Step: wizStepExecution, Phase: wizPhaseExecute},
 		},
 	}
 	for _, tc := range cases {
@@ -322,3 +337,135 @@ func TestRestoreWizard_FieldContract(t *testing.T) {
 		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")
+	}
+}