diff --git a/CHANGELOG.md b/CHANGELOG.md index 70b860a..e5403fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,43 @@ ## Changelog +### v0.155.0 — the restore wizard read the wrong "is something running" flag (2026-07-21) + +**No new agent coupling — MinAgent stays 0.90.0.** Fixes a defect shipped in v0.154.0 and found by +the operator on the first live click-through, plus the dead phase-strip label from the same release. + +**The bug.** `backup.Manager` carries two different booleans and v0.154.0 read the wrong one: + +| flag | read by | set by | covers the verification restore? | +|---|---|---|---| +| `running` | `IsRunning()` | `acquireRunning()`, **inside** the goroutine | **no — `RestoreOffboxScratch` never acquires it at all** | +| `opRunning` | `RestoreStatus()` | `BeginRestoreOp()`, in the handler, synchronously | yes, all four offsite actions | + +The wizard sourced `OpRunning` from `IsRunning()`. For „Ellenőrzés" and the full-restore preparation +— the wizard's two most-used actions, and the long ones, since they stream from restic — that flag is +false for the *entire* operation. So the execution step was unreachable: the page kept offering all +three intents with live buttons while a restore was downloading, and the progress banner (which polls +the op status) contradicted the phase strip on the same screen. Any button pressed there would have +been refused by the handler — which is exactly the "offering a control guaranteed to fail" dishonesty +R-48 exists to remove. + +**The fix** is one line of behaviour behind a named seam: `restoreOpInFlight(st)` takes the +`RestoreOpStatus` the handler already reads once, and its doc comment states which flag is which and +why. The handler now takes a single `RestoreStatus()` read, so the strip, the suppression decision +and the running-op name can no longer disagree with each other. + +**Why the v0.154.0 tests missed it.** The Scenario-E table proved `deriveWizardStep` behaves +correctly *given* `OpRunning=true`; nothing proved the handler ever computes `true`. Hollow at exactly +that seam. `TestRestoreOpInFlight_UsesDisplayFlagNotConcurrencyFlag` now drives a real `Manager` +through `BeginRestoreOp` and asserts the wizard suppresses every form — red-proofed against the +v0.154.0 shape. + +**„Eredmény" is now reachable.** The fourth phase label never lit up in v0.154.0. The strip's +highlight is now its own derived value (`Phase`), separate from `Step`: a finished restore is back on +the intent step — everything is available again — while the strip rightly reads „Eredmény" and an +outcome card shows the result. Bounded by `restoreResultWindow` (10 min) so a stale result cannot +claim to be fresh, and bound to the app, so a finished bookstack restore does not light up immich's +page with bookstack's message. The card survives a reload, which the redirect flash does not. + ### v0.154.0 — one restore entry per app, and the intent is a described choice (2026-07-21) Closes **R-48**. **No new agent coupling — MinAgent stays 0.90.0.** This is a UI-layer change: diff --git a/CONTEXT.md b/CONTEXT.md index 5b7f456..e2b91aa 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,27 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-07-21 (v0.154.0 — R-48: one restore entry per app, intent as a described choice) +Last updated: 2026-07-21 (v0.155.0 — R-48 follow-up: the wizard read the wrong running-flag) + +> **2026-07-21 — v0.155.0.** v0.154.0's wizard sourced "is an op running" from `Manager.IsRunning()` +> — the CONCURRENCY single-flight, acquired inside the goroutine, and **`RestoreOffboxScratch` never +> acquires it**. So the execution step was unreachable for „Ellenőrzés" and the full-restore +> preparation: live buttons while a restore downloaded, with the progress banner contradicting the +> phase strip on the same screen. Found by the operator on the first live click-through. +> +> **DECISION: display reads `RestoreStatus()` (the `opRunning` flag), never `IsRunning()`**, through +> the named `restoreOpInFlight` seam, and the handler reads the status ONCE per render so the strip, +> the suppression and the running-op name cannot diverge. The lesson generalises: `opstatus.go` is the +> DISPLAY surface and says so in its own header — the concurrency flag is not a substitute. +> +> **The test lesson:** a table test over a pure function proves the function, not the caller. Scenario +> E passed throughout because it injected `OpRunning=true` directly. The new test drives a real +> `Manager` through `BeginRestoreOp` and asserts the render. +> +> **DECISION: „Eredmény" earns its place.** The strip's highlight is now `Phase`, derived separately +> from `Step`: a finished restore is back on the intent step while the strip reads „Eredmény" and an +> outcome card shows the result — window-bounded (10 min) and app-bound. + > **2026-07-21 — v0.154.0 (R-48).** Collapses the offsite restore controls to a single > „Visszaállítás…" entry per app row plus a per-app wizard at `GET /backups/restore/app?name=`. diff --git a/REUSE.md b/REUSE.md index b9596cc..38e4e83 100644 --- a/REUSE.md +++ b/REUSE.md @@ -42,6 +42,7 @@ | `limitBody` | controller/internal/api/router.go | `(w, req)` | Bound request bodies (1MB) | Apply before decode on any new POST | | `offboxRedirect` | controller/internal/web/offbox_handlers.go | `(w, r, msg string, isErr bool)` | Flash-message redirects | Flash = `?flash=` / `?flash_error=` query params, read by page handlers | | `offboxRedirectTo` | controller/internal/web/offbox_handlers.go | `(w, r, page, msg string, isErr bool)` | Same, to an EXPLICIT page | **TRAP (fixed v0.154.0): the separator is chosen, not `"?"`.** Targets may already carry a query — the R-48 wizard is `/backups/restore/app?name=` — and a hardcoded `"?"` buries the flash inside the previous parameter's value | +| `restoreOpInFlight` + `hasRecentRestoreResult` | controller/internal/web/restore_wizard.go | `(backup.RestoreOpStatus) bool` / `(st, app, now) bool` | THE "is a restore running / did one just finish" display reads | **TRAP (v0.154.0 shipped this bug): `Manager` has TWO running flags.** `IsRunning()` reads the CONCURRENCY flag, acquired inside the goroutine — and `RestoreOffboxScratch` never acquires it, so it is false for the whole verification restore. Display must read `RestoreStatus().Running` (set synchronously by `BeginRestoreOp`). Read the status ONCE per render or the strip and the suppression can disagree. `hasRecentRestoreResult` is app-bound and window-bounded — a process-wide result must not light another app's „Eredmény" | | `restoreWizardPath` / `deriveWizardStep` / `resolveWizardApp` | controller/internal/web/restore_wizard.go | `(app) string` / `(restoreWizardInput) restoreWizardView` / `([]OffboxAppRow, name) *OffboxAppRow` | R-48 offsite restore wizard: URL builder + the PURE step/unlock derivation + the app-resolution refusals | The step is **never** taken from the request. Precedence is load-bearing: op-running outranks a stale `?full_prep=`, else a commit button reappears mid-restore. Truth table + red-proof: `restore_wizard_test.go`. Adding a form here that posts anywhere new breaks `TestRestoreWizard_NoNewMutationEndpoints` **by design** — R-48 adds no mutation surface | | `redirectTier2` | controller/internal/web/tier2_config_handler.go | `(w, r, name, flash, flashErr)` | Tier2 page flash redirects | Same convention | | `validStackName` | controller/internal/web/validate.go | `(name string) bool` | Any stack name from a request | Single-segment, no `/ \ ..` — blocks path traversal into stacks/userdata | diff --git a/controller/README.md b/controller/README.md index 1e0c8d8..d15f24e 100644 --- a/controller/README.md +++ b/controller/README.md @@ -326,6 +326,12 @@ Each app can define rich metadata in `.felhom.yml`: `?full_prep=`, so no commit button survives into a restore. While ANY op runs, every mutation form is suppressed server-side rather than offered and refused. No new endpoint, no job registry (that stays R-45), and the page works with JavaScript disabled. + **v0.155.0 fix:** the "is an op running" read must come from `RestoreStatus()` (the `opRunning` + display flag, set synchronously by `BeginRestoreOp`), NOT `Manager.IsRunning()` (the concurrency + single-flight, which `RestoreOffboxScratch` never acquires — so v0.154.0's execution step was + unreachable for the verification restore). The strip's highlight is its own derived `Phase`, so a + finished restore reads „Eredmény" while the intent step is available again; the outcome card is + window-bounded and app-bound. - **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` diff --git a/controller/internal/web/restore_wizard.go b/controller/internal/web/restore_wizard.go index bfb3a10..0c3335e 100644 --- a/controller/internal/web/restore_wizard.go +++ b/controller/internal/web/restore_wizard.go @@ -4,6 +4,7 @@ import ( "net/http" "net/url" "strings" + "time" "gitea.dooplex.hu/admin/felhom-controller/internal/backup" ) @@ -53,8 +54,12 @@ const ( type restoreWizardInput struct { // App is the app this wizard page is for. App string - // OpRunning is true when ANY backup/restore op is in flight — not just this app's. The - // single-flight is process-wide, so a restore running for app X must suppress app Y's controls. + // OpRunning is true when ANY backup/restore op is in flight — not just this app's. The op status + // is process-wide, so a restore running for app X must suppress app Y's controls. + // + // Source it via restoreOpInFlight (the DISPLAY flag), never Manager.IsRunning() — see the note + // on that helper. Reading the wrong flag makes this field silently always-false for the + // verification restore, which is the wizard's most-used path. OpRunning bool // ScratchReady is true when a completed full-restore scratch exists for App. Both the // missing-only merge and the true reconstitution require one. @@ -62,6 +67,10 @@ type restoreWizardInput struct { // FullPrepApp is the app the size-gate flash binds to (from ?full_prep=). It binds to ONE app: // a prepare for X must not reveal a confirm on Y's page. FullPrepApp string + // HasRecentResult is true when THIS app's restore finished a short while ago — see + // hasRecentRestoreResult. It moves the phase strip to „Eredmény"; it never changes what the + // customer may do (a finished restore leaves every intent available again). + HasRecentResult bool } // restoreWizardView is what the template renders. The enabled-flags are part of the derivation (not @@ -69,6 +78,11 @@ type restoreWizardInput struct { // one pure function with one test table. type restoreWizardView struct { Step restoreWizardStep + // Phase is which label the phase strip highlights. It is NOT the same as Step: a finished restore + // is back on the intent step (everything is offered again) while the strip rightly says + // „Eredmény". Keeping them separate is what stopped the strip from having to lie in one direction + // or the other. + Phase restoreWizardPhase // VerifyEnabled — intent 1: restore into a separate verification folder (mode=unit). Live data // is untouched, so this is the only intent available without a prepared scratch. VerifyEnabled bool @@ -85,6 +99,35 @@ type restoreWizardView struct { CommitPrepareEnabled bool } +// restoreWizardPhase is the phase-strip highlight. Four labels, all reachable. +type restoreWizardPhase string + +const ( + wizPhasePrepare restoreWizardPhase = "elokeszites" + wizPhaseConfirm restoreWizardPhase = "megerosites" + wizPhaseExecute restoreWizardPhase = "vegrehajtas" + wizPhaseResult restoreWizardPhase = "eredmeny" +) + +// restoreResultWindow bounds how long after a finished restore the strip still says „Eredmény". +// Without a bound the last result would light that phase forever — landing on the page a week later +// would claim you had just finished a restore. Same reasoning as escrowCeremonyGraceWindow; shorter, +// because this answers "what just happened", not "are we still waiting". +const restoreResultWindow = 10 * time.Minute + +// hasRecentRestoreResult reports whether THIS app has a just-finished restore to show. Pure (the +// clock is a parameter) so the boundary and the wrong-app case are table-testable. +// +// Bound to the app on purpose: the op status is process-wide, so a finished bookstack restore must +// not light „Eredmény" on immich's wizard and show bookstack's message there. +func hasRecentRestoreResult(st backup.RestoreOpStatus, app string, now time.Time) bool { + if st.Running || st.Last == nil || st.Last.Stack != app || st.Last.FinishedAt.IsZero() { + return false + } + d := now.Sub(st.Last.FinishedAt) + return d >= 0 && d < restoreResultWindow +} + // deriveWizardStep is the Scenario-B truth table: step and available intents are a PURE function of // the state, in strict precedence order. // @@ -98,13 +141,18 @@ type restoreWizardView struct { // never resurrect a commit button while a restore is mid-flight. func deriveWizardStep(in restoreWizardInput) restoreWizardView { if in.OpRunning { - return restoreWizardView{Step: wizStepExecution} + return restoreWizardView{Step: wizStepExecution, Phase: wizPhaseExecute} } if in.FullPrepApp != "" && in.FullPrepApp == in.App { - return restoreWizardView{Step: wizStepPrepareConfirm, CommitPrepareEnabled: true} + return restoreWizardView{Step: wizStepPrepareConfirm, Phase: wizPhaseConfirm, CommitPrepareEnabled: true} + } + phase := wizPhasePrepare + if in.HasRecentResult { + phase = wizPhaseResult } return restoreWizardView{ Step: wizStepIntent, + Phase: phase, VerifyEnabled: true, PrepareEnabled: !in.ScratchReady, PlaceEnabled: in.ScratchReady, @@ -129,6 +177,28 @@ func resolveWizardApp(rows []OffboxAppRow, name string) *OffboxAppRow { return nil } +// restoreOpInFlight reports whether a restore op is in flight, FOR DISPLAY. +// +// **Use this, not `Manager.IsRunning()`.** The Manager carries two different booleans and they are +// not interchangeable: +// +// - `m.running` (read by `IsRunning`) is the CONCURRENCY single-flight. It is acquired *inside* +// the restore function, on the background goroutine — and `RestoreOffboxScratch` never acquires +// it at all. So for the verification restore and the full-restore preparation — the wizard's two +// most-used actions, and the long ones, since they stream from restic — `IsRunning()` is false +// for the entire operation. +// - `m.opRunning` (read by `RestoreStatus`) is the DISPLAY flag, set synchronously by +// `BeginRestoreOp` in the handler *before* the goroutine launches and cleared by `EndRestoreOp`. +// It covers all four offsite actions with no start-up window. +// +// v0.154.0 shipped with `IsRunning()` here, which made the execution step unreachable for +// `RestoreOffboxScratch`: the page offered all three intents, with live buttons, while a restore was +// downloading — and the progress banner (which polls the op status) contradicted it on the same +// screen. Caught by the operator on the first live click-through. +func restoreOpInFlight(st backup.RestoreOpStatus) bool { + return st.Running +} + // backupsRestoreWizardHandler renders GET /backups/restore/app?name= — the single entry the // list page now offers per app. // @@ -154,11 +224,13 @@ func (s *Server) backupsRestoreWizardHandler(w http.ResponseWriter, r *http.Requ data := s.backupsCommonData("backups-restore", "Visszaállítás — "+row.DisplayName, r) + st := s.backupMgr.RestoreStatus() in := restoreWizardInput{ - App: app, - OpRunning: s.backupMgr.IsRunning(), - ScratchReady: s.backupMgr.OffboxFullScratchReady(app), - FullPrepApp: strings.TrimSpace(r.URL.Query().Get("full_prep")), + App: app, + OpRunning: restoreOpInFlight(st), + ScratchReady: s.backupMgr.OffboxFullScratchReady(app), + FullPrepApp: strings.TrimSpace(r.URL.Query().Get("full_prep")), + HasRecentResult: hasRecentRestoreResult(st, app, time.Now()), } view := deriveWizardStep(in) @@ -177,8 +249,12 @@ func (s *Server) backupsRestoreWizardHandler(w http.ResponseWriter, r *http.Requ } // The running op's identity, so the execution card can say WHAT is running rather than a bare // "please wait" — including the case where it belongs to a different app. - st := s.backupMgr.RestoreStatus() data["RunningStack"] = st.Stack + // The outcome card for the „Eredmény" phase — the same message the redirect flash carried, but it + // survives a reload, which the flash does not. + if in.HasRecentResult { + data["LastResult"] = st.Last + } s.executeTemplate(w, r, "backups_restore_wizard", data) } diff --git a/controller/internal/web/restore_wizard_test.go b/controller/internal/web/restore_wizard_test.go index 3d4c587..9ea13cc 100644 --- a/controller/internal/web/restore_wizard_test.go +++ b/controller/internal/web/restore_wizard_test.go @@ -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, "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, "

Eredmény

") { + t.Error("no result card may render without a recent result") + } +} diff --git a/controller/internal/web/templates/backups_restore_wizard.html b/controller/internal/web/templates/backups_restore_wizard.html index 5f9deaa..ccf2728 100644 --- a/controller/internal/web/templates/backups_restore_wizard.html +++ b/controller/internal/web/templates/backups_restore_wizard.html @@ -20,13 +20,24 @@ +{{$phase := printf "%s" .Wizard.Phase}}
- Előkészítés - Megerősítés - Végrehajtás - Eredmény + Előkészítés + Megerősítés + Végrehajtás + Eredmény
+{{with .LastResult}} + +
+

Eredmény

+
{{.Message}}
+

Befejezve: {{fmtTime .FinishedAt}}. Ha szeretnéd, alább újra indíthatsz egy visszaállítást.

+
+{{end}} + {{if eq (printf "%s" .Wizard.Step) "execution"}}