v0.261.0 — the controller no longer swaps itself out from under an app update (R-608, R-609)

The controller self-updates daily at 04:30 by default, and after any hub report once a floor sits above the box. That swap restarts the controller container. The window proposed for automatic app updates is 02:30-05:00. It contains 04:30. R-608 — a two-way lock, wired in main.go (stacks never imports selfupdate): - stacks.Manager.AnyUpdating() -> Updater.SetAppUpdatingCheck, consulted in the same three places as the existing backupRunning gate. - Updater.IsUpdateRunning -> Manager.SetSelfUpdatingCheck; UpdatePreflight refuses `self_updating`. - MEASURED: the gap was narrower than assumed. The update's `backing-up` phase already takes the backup single-flight, so that one phase was covered. The other six were not, and `starting`/`verifying` are where data may have moved. - The lock must NOT latch: a held app does not block the controller's own updates, including the release that might fix the hold. R-609 — the 409 carries `data.reason`, additively. transient (busy, updating, deploying, migrating, self_updating) vs terminal (held, downgrade). Found while writing the test: the router refuses a HELD app on its own line before the preflight, so `held` would have been the one reason missing. Five red-proofs, each seen to fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS
2026-09-21 14:23:59 +02:00
parent d0d431b42b
commit 811f75736e
13 changed files with 591 additions and 2 deletions
@@ -600,7 +600,13 @@ func (r *Router) actionStack(w http.ResponseWriter, req *http.Request, action, n
 	// TestR439_UpdateOfAHeldAppIsRefused.
 	if action == "start" || action == "restart" || action == "update" {
 		if held, why := r.restoreHoldFor(name); held {
-			writeJSON(w, http.StatusConflict, apiResponse{OK: false, Error: why})
+			// `reason` (v0.261.0) — THIS LINE FIRES BEFORE UpdatePreflight, so without it a held app
+			// answers 409 with no machine-readable reason at all, and an unattended caller cannot
+			// tell it from a passing backup window: it would press a terminally-refused button for
+			// ever. Found while writing the reason test, not by reading. `held` is TERMINAL until a
+			// person acts, and it is the one reason that matters most to get right.
+			writeJSON(w, http.StatusConflict, apiResponse{OK: false, Error: why,
+				Data: map[string]string{"reason": "held"}})
 			return
 		}
 	}
@@ -652,7 +658,15 @@ func (r *Router) actionStack(w http.ResponseWriter, req *http.Request, action, n
 			// byte — which is why this line is safe to change for all of them at once, and why the
 			// R-524 downgrade refusal below is not a key nobody reads (the "seam built but never
 			// wired" class).
-			writeJSON(w, status, apiResponse{OK: false, Error: r.errText(req, ref)})
+			//
+			// `data.reason` (v0.261.0) is ADDITIVE and is for a caller that is not a person. The
+			// sentence says WHAT happened; only the reason says whether trying again can ever work —
+			// `busy`/`updating`/`deploying`/`migrating`/`self_updating` are TRANSIENT, `held` and
+			// `downgrade` are TERMINAL until a human acts. An unattended caller that cannot tell
+			// those apart either gives up on a passing backup window or presses a refused button for
+			// ever. The sentence is unchanged, so no page moves.
+			writeJSON(w, status, apiResponse{OK: false, Error: r.errText(req, ref),
+				Data: map[string]string{"reason": ref.Reason}})
 			return
 		}
 	}
@@ -0,0 +1,199 @@
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-controller/internal/settings"
+	"gitea.dooplex.hu/admin/felhom-controller/internal/stacks"
+)
+
+// R-609 (v0.261.0) — the 409 carries a machine-readable REASON, additively.
+//
+// WHY. `UpdateRefusal.Reason` has existed since v0.237.0 and never left the process: the body carried
+// only the translated sentence. A caller that is not a person cannot parse a Hungarian sentence, and
+// the distinction it needs is not decorative:
+//
+//	TRANSIENT — busy, updating, deploying, migrating, self_updating → try again later
+//	TERMINAL  — held, downgrade                                     → never press this again
+//
+// An unattended caller that cannot tell them apart either gives up on a passing backup window, or
+// presses a terminally-refused button on every pass for ever. `09` §6.2's caller reads exactly this.
+//
+// The SENTENCE is unchanged and stays where it was, so no page moves — this is purely additive.
+
+// reasonOf pulls `data.reason` out of a refusal body, or "" when absent.
+func reasonOf(t *testing.T, resp apiResponse) string {
+	t.Helper()
+	if resp.Data == nil {
+		return ""
+	}
+	m, ok := resp.Data.(map[string]interface{})
+	if !ok {
+		t.Fatalf("data is not an object: %#v", resp.Data)
+	}
+	s, _ := m["reason"].(string)
+	return s
+}
+
+// TestR609_EveryRefusalCarriesItsReason is table-driven over the refusal paths reachable without
+// docker, and it INCLUDES the router's own hold line — which fires before UpdatePreflight and was
+// the one that had no reason at all until this test was written.
+//
+// COMPANION RED-PROOF (run 2026-09-21): drop the `Data:` field from either writeJSON in actionStack's
+// update paths. The corresponding row fails with `reason = ""`. Reverted.
+func TestR609_EveryRefusalCarriesItsReason(t *testing.T) {
+	cases := []struct {
+		name       string
+		arrange    func(t *testing.T, r *Router, sett *settings.Settings, g *apiFakeGuards, dir string)
+		wantReason string
+		wantCode   int
+	}{
+		{
+			name: "held — the ROUTER's own line, before the preflight",
+			arrange: func(t *testing.T, r *Router, sett *settings.Settings, g *apiFakeGuards, dir string) {
+				if err := sett.SetRestoreHold(settings.RestoreHold{
+					Stack: "app", At: "2026-09-21T08:00:00Z",
+					Reason: settings.HoldReasonUpdateFailed, CopyDate: "2026-09-21T01:30:00Z",
+				}); err != nil {
+					t.Fatal(err)
+				}
+			},
+			wantReason: "held", wantCode: http.StatusConflict,
+		},
+		{
+			name: "no_backup — nothing on any tier and none can be taken",
+			arrange: func(_ *testing.T, _ *Router, _ *settings.Settings, g *apiFakeGuards, _ string) {
+				g.points, g.cannotBackUp = nil, true
+			},
+			wantReason: "no_backup", wantCode: http.StatusConflict,
+		},
+		{
+			name: "self_updating — the controller is swapping itself (v0.261.0)",
+			arrange: func(_ *testing.T, r *Router, _ *settings.Settings, _ *apiFakeGuards, _ string) {
+				r.stackMgr.SetSelfUpdatingCheck(func() bool { return true })
+			},
+			wantReason: "self_updating", wantCode: http.StatusConflict,
+		},
+		{
+			name: "downgrade — the box runs something newer than the catalog (R-524)",
+			arrange: func(t *testing.T, r *Router, _ *settings.Settings, _ *apiFakeGuards, dir string) {
+				setInstalledAndCatalog(t, r, dir, "nginx:1.28", "nginx:1.27")
+			},
+			wantReason: "downgrade", wantCode: http.StatusConflict,
+		},
+		{
+			name: "not_found — an app that exists nowhere",
+			arrange: func(_ *testing.T, _ *Router, _ *settings.Settings, _ *apiFakeGuards, _ string) {},
+			wantReason: "not_found", wantCode: http.StatusNotFound,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			r, sett, g, dir := newSlice4Router(t)
+			c.arrange(t, r, sett, g, dir)
+
+			name := "app"
+			if c.wantReason == "not_found" {
+				name = "no-such-app"
+			}
+			code, resp := postUpdateNamed(t, r, name)
+
+			if code != c.wantCode {
+				t.Errorf("code = %d, want %d (error %q)", code, c.wantCode, resp.Error)
+			}
+			if resp.OK {
+				t.Fatal("a refusal must not answer ok")
+			}
+			if got := reasonOf(t, resp); got != c.wantReason {
+				t.Errorf("reason = %q, want %q", got, c.wantReason)
+			}
+			// The sentence is the household's and must still be there, in words — never a bare key.
+			if resp.Error == "" {
+				t.Error("the reason is ADDITIVE: the sentence must still be present")
+			}
+		})
+	}
+}
+
+// TestR609_ASuccessfulActionCarriesNoReason — the field is for refusals only, so a caller cannot
+// mistake a normal answer for one.
+func TestR609_ASuccessfulActionCarriesNoReason(t *testing.T) {
+	r, _, _, _ := newSlice4Router(t)
+	_, resp := postStopNamed(t, r, "app")
+	if got := reasonOf(t, resp); got != "" {
+		t.Errorf("a non-refusal must carry no reason, got %q", got)
+	}
+}
+
+// ── helpers ─────────────────────────────────────────────────────────────────────────────────────
+
+// postUpdateNamed is postUpdate for an arbitrary app name (the not_found row needs one that is absent).
+func postUpdateNamed(t *testing.T, r *Router, name string) (int, apiResponse) {
+	t.Helper()
+	w := httptest.NewRecorder()
+	r.actionStack(w, httptest.NewRequest("POST", "/api/stacks/x/action", nil), "update", name)
+	var resp apiResponse
+	if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("non-JSON body %q: %v", w.Body.String(), err)
+	}
+	return w.Code, resp
+}
+
+// postStopNamed exercises a NON-update action, as the control that `reason` is refusal-only.
+func postStopNamed(t *testing.T, r *Router, name string) (int, apiResponse) {
+	t.Helper()
+	w := httptest.NewRecorder()
+	r.actionStack(w, httptest.NewRequest("POST", "/api/stacks/x/action", nil), "stop", name)
+	var resp apiResponse
+	if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("non-JSON body %q: %v", w.Body.String(), err)
+	}
+	return w.Code, resp
+}
+
+// setInstalledAndCatalog puts the app AHEAD of the catalog, which is R-524's Ahead arm.
+//
+// It writes the REAL files the real code reads — `installed_images` into the app's app.yaml and a
+// catalog template under <DataDir>/catalog-cache/templates/<app>/ — and then rescans. **No test-only
+// seam is added to production code for this**: the whole point of the api tests here is that they go
+// through the production handler over a real stacks.Manager, and a setter that only tests call would
+// be a second way to reach the state, which is how the two drift apart.
+func setInstalledAndCatalog(t *testing.T, r *Router, dir, installed, catalog string) {
+	t.Helper()
+	appYAML := slice4AppYAML + "installed_images:\n    app:\n        ref: " + installed +
+		"\n        digest: sha256:x\n        at: \"2026-09-01T00:00:00Z\"\n"
+	if err := os.WriteFile(filepath.Join(dir, "app.yaml"), []byte(appYAML), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	catDir := filepath.Join(r.cfg.Paths.DataDir, "catalog-cache", "templates", "app")
+	if err := os.MkdirAll(catDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(catDir, "docker-compose.yml"),
+		[]byte("services:\n  app:\n    image: "+catalog+"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := r.stackMgr.ScanStacks(); err != nil {
+		t.Fatal(err)
+	}
+	// Positive control: the state this helper claims to create must actually exist, or the row it
+	// serves would pass for the wrong reason (a different refusal firing first).
+	if got := stacks.CatalogOrder(mustStack(t, r, "app")); got != stacks.UpdateOrderAhead {
+		t.Fatalf("fixture did not produce the AHEAD state; CatalogOrder = %v", got)
+	}
+}
+
+func mustStack(t *testing.T, r *Router, name string) stacks.Stack {
+	t.Helper()
+	st, ok := r.stackMgr.GetStack(name)
+	if !ok {
+		t.Fatalf("stack %q missing", name)
+	}
+	return *st
+}