From 811f75736ec895fe56c486922db74ad22e2ef139 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 21 Sep 2026 14:23:59 +0200 Subject: [PATCH] =?UTF-8?q?v0.261.0=20=E2=80=94=20the=20controller=20no=20?= =?UTF-8?q?longer=20swaps=20itself=20out=20from=20under=20an=20app=20updat?= =?UTF-8?q?e=20(R-608,=20R-609)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- CHANGELOG.md | 43 ++++ REUSE.md | 1 + controller/cmd/controller/main.go | 14 ++ controller/internal/api/router.go | 18 +- controller/internal/api/update_reason_test.go | 199 ++++++++++++++++++ controller/internal/i18n/locales/en.json | 2 + controller/internal/i18n/locales/hu.json | 2 + .../selfupdate/appupdate_lock_test.go | 89 ++++++++ controller/internal/selfupdate/updater.go | 35 +++ controller/internal/stacks/manager.go | 4 + controller/internal/stacks/update.go | 59 ++++++ controller/internal/stacks/updatelock_test.go | 125 +++++++++++ controller/scripts/i18n_go_keys.json | 2 + 13 files changed, 591 insertions(+), 2 deletions(-) create mode 100644 controller/internal/api/update_reason_test.go create mode 100644 controller/internal/selfupdate/appupdate_lock_test.go create mode 100644 controller/internal/stacks/updatelock_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d018cef..86d23c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,46 @@ +## v0.261.0 — the controller no longer swaps itself out from under an app update (2026-09-21, R-608/R-609) + +**MinAgent: 0.131.0** (unchanged). Hungarian pages byte-identical; the two new sentences are BORN AS +KEYS in both bundles and accounted for by the Go-parity gate. + +**The collision was found by reading the clock, not by a failure.** The controller updates itself +daily at `self_update.auto_update_time` — **04:30 by default** — and again after any hub report once +a floor sits above the box, so at any hour. That swap restarts the controller container. The window +`09` §3b Q1 proposes for automatic app updates is **02:30–05:00**. It contains 04:30. + +- **`stacks.Manager.AnyUpdating()`** — is a guarded update in flight for ANY app. +- **`Updater.SetAppUpdatingCheck`** — a sibling of `SetBackupRunningCheck`, deliberately the same + shape, consulted in the SAME three places: the dry run, `TriggerUpdate`, and `maybeAutoUpdate`. + One busy-gate pattern in that file, not two. +- **`Manager.SetSelfUpdatingCheck`** — the reverse direction. `UpdatePreflight` now refuses with + reason `self_updating`: „A vezérlő éppen frissül. Próbáld újra néhány perc múlva." Both halves are + wired in `main.go`, the only place holding both objects; `stacks` never imports `selfupdate`. +- **The gap was NARROWER than assumed, and measuring it is why the comment is right.** The update's + `backing-up` phase takes the backup single-flight (`RunAppBackupNow` → `acquireRunning`), so + `backupMgr.IsRunning()` ALREADY covered that one phase. It covered none of `checking`, + `safety-dump`, `pinning`, `pulling`, `starting` or `verifying` — and the last two are exactly where + the new version may already have touched the customer's data. +- **The lock must NOT latch, and that is the test that matters.** `Stack.Updating` is cleared on done, + failed AND held, so a held app does not block the controller's own updates — including the release + that might fix whatever held it. A latching gate would be a worse failure than the one prevented, + and a silent one. + +**R-609 — the 409 carries a machine-readable `reason`, additively.** `UpdateRefusal.Reason` has +existed since v0.237.0 and never left the process. The body now carries `data: {"reason": "..."}` +beside the unchanged sentence, because the distinction is not decorative: `busy`, `updating`, +`deploying`, `migrating`, `self_updating` are **transient**; `held` and `downgrade` are **terminal** +until a person acts. A caller that cannot tell them apart either gives up on a passing backup window +or presses a refused button for ever. + +- **Found while writing the test, not by reading: the router refuses a HELD app on its own line, + BEFORE `UpdatePreflight`** — so `held`, the reason that matters most, would have been the one + missing. That line now carries it too. + +**Five red-proofs, each SEEN to fail.** `AnyUpdating` always false → the in-flight test fails; +`AnyUpdating` also true for a held app → the release-after-hold test fails; delete the preflight's +`self_updating` block → the refusal test fails with `ref = `; delete `TriggerUpdate`'s app-update +block → the swap proceeds over a live app update; drop `Data` from the refusal → `reason = ""`. + ## v0.260.0 — a box AHEAD of the catalog reads „Naprakész", and the pin never moves backwards (2026-09-21, R-524) **MinAgent: 0.131.0** (unchanged). Hungarian pages byte-identical; the two new sentences are BORN AS diff --git a/REUSE.md b/REUSE.md index 0a8e376..4ee4c7d 100644 --- a/REUSE.md +++ b/REUSE.md @@ -144,6 +144,7 @@ | `Stack.CatalogImages` vs `Stack.TemplateImages` (v0.235.0) | controller/internal/stacks/manager.go | both `map[string]string` | badge input vs "what the next `up -d` gives this app" | **THE TRAP: same type, same shape, opposite meaning after the freeze.** `TemplateImages` reads the LIVE (possibly frozen) compose file; `CatalogImages` reads the syncer's clone. `web.compareInstalledToTemplate` MUST use `CatalogImages` or it answers „Naprakész" on exactly the apps that are behind, with every test green. Red-proved | | `Manager.BackfillInstalledImages` (v0.234.0) | controller/internal/stacks/installed.go | `() int` | seeding `installed_images` for apps that have NO record — call ONCE at startup | Beside `BackfillDesiredState` in `cmd/controller/main.go`, after it and BEFORE the boot reconciler (pinned by an AST-walking test that asserts the ORDER). **READS only** — starts nothing, writes no compose file. **Never overwrites an existing record** (an app that has one is not even observed). **REFUSES a partial observation** (`observationCoversTemplate`): `web.compareInstalledToTemplate` reads a service-count mismatch as BEHIND, so seeding a degraded app from what is visible renders „Frissítés elérhető" over an app that is current. The bring-up paths may write a partial because they follow a SUCCESSFUL `up -d` where a gap is real news; a backfill meets any state and must be stricter | | `stacks.ParseComposeImages` (v0.233.0) | controller/internal/stacks/installed.go | `(composePath string) (map[string]string, error)` | compose SERVICE name -> the image the FILE pins; feeds `Stack.TemplateImages` and the update badge | yaml.v3 `services:` MAP parse, never a line scan (same reason as `DBServiceNames`). An error means CANNOT-TELL — `ScanStacks` leaves `TemplateImages` nil and the badge renders NOTHING, never "current" | +| `stacks.Manager.AnyUpdating` / `SetSelfUpdatingCheck` + `selfupdate.Updater.SetAppUpdatingCheck` (v0.261.0, R-608) | controller/internal/stacks/update.go, internal/selfupdate/updater.go, wired in cmd/controller/main.go | `() bool` callbacks, both directions | THE two-way lock between the CONTROLLER's own swap and a guarded APP update | **Wire BOTH halves or neither** — they are wired together in `main.go`, the only place holding both objects; **`stacks` must never import `selfupdate`**. The app-update side is a sibling of `SetBackupRunningCheck` and is consulted in the SAME three places (dry run, `TriggerUpdate`, `maybeAutoUpdate`) — do not add a fourth pattern. **`AnyUpdating` MUST answer false for a HELD app** (`Stack.Updating` is cleared on done/failed/held): a latching gate would block the controller's own updates for ever, including the one that fixes the hold. Nil callbacks are SAFE and mean pre-v0.261.0 behaviour — never fail closed on an unwired gate | | `stacks.CatalogOrder` / `CompareImageRefs` (v0.260.0, R-524) | controller/internal/stacks/updateorder.go | `(Stack) UpdateOrder` — Unknown/Current/Behind/**Ahead** | THE one "how does this app stand against the catalog?" verdict | **Both the badge AND `Manager.UpdatePreflight`'s `downgrade` refusal read it — never re-implement the comparison.** `web.compareInstalledToTemplate` is a thin wrapper. Ahead is NARROW: every differing service must be orderable AND newer, else Behind. Ordering is `util.Version.Compare` behind a tag normaliser (`X.Y`/`X.Y.Z`, optional `v`, suffix must be IDENTICAL on both sides) — **never add a second comparator**. Queries NO registry; absent record = Unknown, never „Naprakész" | | `web.updateBadge` / `updateBadgeAt` / `Metadata.CatalogSince` + `CatalogSinceAge` (v0.233.0) | controller/internal/web/updatebadge.go, controller/internal/stacks/metadata.go | `(stacks.Stack) *MetaBadge` | THE "is this app current?" label — „Naprakész" / „Frissítés elérhető — N napja" | The SECOND `*MetaBadge` user the type was built for: existing `meta_badge` partial, **no new markup or CSS**. **NO RECORD RENDERS NOTHING — absent means UNKNOWN, never current** (R-166 applied to an observation; red-proved). **No version number reaches the customer** and **no registry is queried**. `catalog_since` is tolerant in the `lifecycle` style — absent/empty/malformed/**future** all degrade to a badge with no age + one WARN. LIMITATION: for the **10** floating pins (recounted 2026-09-21; the old "23" matched no definition the catalog supports) the ref can match while the image has moved — 6 of the 7 measurable ones HAVE moved — so those read „Naprakész" when they may not be | | `Manager.logPostStartStatus` | controller/internal/stacks/manager.go | `(name, stackDir, env)` | Async post-start verification | compose up exits 0 on crash-loops; this is the detection. Goroutine + 3s, never blocks | diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index 718703b..73f92a3 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -659,6 +659,20 @@ func main() { updater.SetBackupRunningCheck(func() bool { return backupMgr != nil && backupMgr.IsRunning() }) + // v0.261.0 — the two-way lock between the controller's own update and a guarded app update. + // BOTH directions are wired here because this is the only place that holds both objects, and + // the dependency must not exist in either package (stacks never imports selfupdate). + // + // The app-update side is NOT redundant with the backup check above: the update's `backing-up` + // phase does take the backup single-flight, but `checking`, `safety-dump`, `pinning`, + // `pulling`, `starting` and `verifying` do not — and the last two are where the new version + // may already have touched the customer's data. + updater.SetAppUpdatingCheck(func() bool { + return stackMgr != nil && stackMgr.AnyUpdating() + }) + if stackMgr != nil { + stackMgr.SetSelfUpdatingCheck(updater.IsUpdateRunning) + } // Check for post-update state (did a previous update succeed or fail?) if state := updater.VerifyStartup(); state != nil { notifier.NotifyControllerUpdated(state.PreviousVersion, state.TargetVersion, state.Status == "success") diff --git a/controller/internal/api/router.go b/controller/internal/api/router.go index 3a29b67..a4ada04 100644 --- a/controller/internal/api/router.go +++ b/controller/internal/api/router.go @@ -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 } } diff --git a/controller/internal/api/update_reason_test.go b/controller/internal/api/update_reason_test.go new file mode 100644 index 0000000..b8eaaaf --- /dev/null +++ b/controller/internal/api/update_reason_test.go @@ -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 /catalog-cache/templates// — 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 +} diff --git a/controller/internal/i18n/locales/en.json b/controller/internal/i18n/locales/en.json index 6a0ab0f..28d7fea 100644 --- a/controller/internal/i18n/locales/en.json +++ b/controller/internal/i18n/locales/en.json @@ -2018,6 +2018,8 @@ "disk.err.attach_unavailable": "This drive cannot be attached right now — it may already be registered, or it may have been unplugged. Reload the page and look at Storage → Drives.", "err.stacks.migracio_mar_folyamatban": "a migration is already running", "err.stacks.update_downgrade": "This version is newer than the one in the catalog — moving back needs the operator.", + "err.stacks.update_self_updating": "The controller is updating. Try again in a few minutes.", + "err.selfupdate.alkalmazas_frissites_folyamatban": "An app update is in progress. The controller update can start when it has finished.", "err.stacks.alkalmazas_nem_talalhato": "app not found: %s", "err.stacks.ismeretlen_migracios_hatokor": "unknown migration scope: %s", "err.stacks.migracios_naplo_irasa": "writing the migration log: %s", diff --git a/controller/internal/i18n/locales/hu.json b/controller/internal/i18n/locales/hu.json index 8f177f7..4eb252c 100644 --- a/controller/internal/i18n/locales/hu.json +++ b/controller/internal/i18n/locales/hu.json @@ -2015,6 +2015,8 @@ "disk.err.attach_unavailable": "Ez a meghajtó most nem csatolható — lehet, hogy már regisztrálva van, vagy időközben lecsatolódott. Frissítsd az oldalt, és nézd meg a Tárhely → Meghajtók listát.", "err.stacks.migracio_mar_folyamatban": "migráció már folyamatban", "err.stacks.update_downgrade": "Ez a változat újabb a katalógusban lévőnél — visszalépés csak az üzemeltető kérésére.", + "err.stacks.update_self_updating": "A vezérlő éppen frissül. Próbáld újra néhány perc múlva.", + "err.selfupdate.alkalmazas_frissites_folyamatban": "Egy alkalmazás frissítése éppen folyamatban van. A vezérlő frissítése utána indítható.", "err.stacks.alkalmazas_nem_talalhato": "alkalmazás nem található: %s", "err.stacks.ismeretlen_migracios_hatokor": "ismeretlen migrációs hatókör: %s", "err.stacks.migracios_naplo_irasa": "migrációs napló írása: %s", diff --git a/controller/internal/selfupdate/appupdate_lock_test.go b/controller/internal/selfupdate/appupdate_lock_test.go new file mode 100644 index 0000000..9b581ad --- /dev/null +++ b/controller/internal/selfupdate/appupdate_lock_test.go @@ -0,0 +1,89 @@ +package selfupdate + +import ( + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/util" +) + +// R-608 (v0.261.0) — the self-updater's half of the lock. +// +// The controller swaps itself daily at `self_update.auto_update_time` (04:30 by default) AND after +// any hub report once a floor sits above this box — so at any hour. That swap restarts this process. +// Until v0.261.0 the only busy gate was `backupRunning`, which covers the app update's `backing-up` +// phase (it takes the backup single-flight) and none of the others. + +// TestR608_TriggerUpdateRefusedWhileAnAppUpdates is a CONSEQUENCE test: does the swap actually +// refuse, and does it say so in a sentence the household can read in its own language? +// +// COMPANION RED-PROOF (run 2026-09-21): delete the `u.appUpdating` block from TriggerUpdate. The +// refusal assertion then fails — the swap proceeds on top of a live app update. +func TestR608_TriggerUpdateRefusedWhileAnAppUpdates(t *testing.T) { + agent := &fakeAgent{} + u := newTestUpdater(t, "0.260.0", agent) + u.queryFn = func() (string, error) { return "0.261.0", nil } + u.pullFn = func(string) error { return nil } + + appUpdating := true + u.SetAppUpdatingCheck(func() bool { return appUpdating }) + + err := u.TriggerUpdate("manual") + if err == nil { + t.Fatal("while a guarded app update is in flight the controller swap must be REFUSED") + } + if m, ok := util.AsMsg(err); !ok { + t.Error("the refusal must carry a bundle key, or an English household reads Hungarian") + } else if m.Key() != "err.selfupdate.alkalmazas_frissites_folyamatban" { + t.Errorf("key = %q", m.Key()) + } + if !strings.Contains(err.Error(), "alkalmaz") { + t.Errorf("the Hungarian fallback must name the app update, got %q", err.Error()) + } + if len(agent.swapCalls()) != 0 { + t.Fatalf("NOTHING may reach the agent on a refusal, got %v", agent.swapCalls()) + } + + // TRANSIENT, not latching: the moment the app update ends, the same trigger goes through with no + // human action in between. A gate that latches would strand the box on an old controller. + appUpdating = false + if err := u.TriggerUpdate("manual"); err != nil { + t.Fatalf("once the app update has finished the swap must proceed, got %v", err) + } + waitDone(t, u) + if len(agent.swapCalls()) != 1 { + t.Errorf("expected exactly one swap after the gate cleared, got %v", agent.swapCalls()) + } +} + +// TestR608_DryRunReportsTheAppUpdate — the operator's dry run must SAY why an update will not run. +// A dry run that reports "available" while the trigger would refuse is a confident wrong answer. +func TestR608_DryRunReportsTheAppUpdate(t *testing.T) { + u := newTestUpdater(t, "0.260.0", &fakeAgent{}) + u.queryFn = func() (string, error) { return "0.261.0", nil } + + if got := u.DryRun().AppUpdating; got { + t.Error("control: with no app update in flight the dry run must report false") + } + u.SetAppUpdatingCheck(func() bool { return true }) + if got := u.DryRun().AppUpdating; !got { + t.Error("the dry run must report that an app update is holding the swap") + } +} + +// TestR608_NilAppUpdatingCheckIsSafe — unwired means the pre-v0.261.0 behaviour, never fail-closed. +// Failing closed on an UNWIRED gate would strand every box whose construction path misses it. +func TestR608_NilAppUpdatingCheckIsSafe(t *testing.T) { + agent := &fakeAgent{} + u := newTestUpdater(t, "0.260.0", agent) + u.queryFn = func() (string, error) { return "0.261.0", nil } + u.pullFn = func(string) error { return nil } + + if err := u.TriggerUpdate("manual"); err != nil { + t.Fatalf("an unwired app-update gate must not refuse, got %v", err) + } + waitDone(t, u) + if u.DryRun().AppUpdating { + t.Error("an unwired gate must report false, not true") + } +} diff --git a/controller/internal/selfupdate/updater.go b/controller/internal/selfupdate/updater.go index 8e08891..985026c 100644 --- a/controller/internal/selfupdate/updater.go +++ b/controller/internal/selfupdate/updater.go @@ -59,6 +59,11 @@ type Updater struct { lastCheck *CheckResult updateRunning bool backupRunning func() bool + // appUpdating (v0.261.0) — is a GUARDED APP UPDATE in flight? The controller's own swap restarts + // this process, and an app update that is mid-`starting`/`verifying` may already have let the new + // version touch the customer's data. Same shape as backupRunning deliberately: one more callback + // consulted in the SAME three places, so there is one busy-gate pattern in this file, not two. + appUpdating func() bool // Phase 2 managed updates: the operator-enforced minimum version (FLOOR), learned from the hub // report ACK, and the last floor we already auto-attempted (no flapping within this process; the @@ -105,6 +110,16 @@ func (u *Updater) SetBackupRunningCheck(fn func() bool) { u.backupRunning = fn } +// SetAppUpdatingCheck sets the callback that reports whether a guarded APP update is in progress. +// +// Wired in main.go beside SetBackupRunningCheck, to stacks.Manager.AnyUpdating. Nil is safe and means +// "no app-update gate" — the pre-v0.261.0 behaviour — so a Server built without it still runs. +func (u *Updater) SetAppUpdatingCheck(fn func() bool) { + u.mu.Lock() + defer u.mu.Unlock() + u.appUpdating = fn +} + // IsUpdateRunning returns true if an update is currently in progress. func (u *Updater) IsUpdateRunning() bool { u.mu.Lock() @@ -459,6 +474,7 @@ type DryRunResult struct { PullCapable bool `json:"pull_capable"` // in-guest pull path available (full creds OR anonymous; false = half-configured creds) TargetImage string `json:"target_image"` // what we would pull + swap to BackupRunning bool `json:"backup_running"` + AppUpdating bool `json:"app_updating"` // v0.261.0: a guarded app update is in flight Error string `json:"error,omitempty"` } @@ -487,6 +503,9 @@ func (u *Updater) DryRun() *DryRunResult { if u.backupRunning != nil { result.BackupRunning = u.backupRunning() } + if u.appUpdating != nil { + result.AppUpdating = u.appUpdating() + } return result } @@ -514,6 +533,14 @@ func (u *Updater) TriggerUpdate(initiatedBy string) error { return util.MsgError("err.selfupdate.mentes_fut_probalja_kesobb") } + // App update running check (v0.261.0). The controller's swap restarts this process; an app update + // in `starting`/`verifying` may already have let the new version touch the customer's data. + if u.appUpdating != nil && u.appUpdating() { + u.mu.Unlock() + u.logger.Printf("[INFO] [selfupdate] TriggerUpdate refused: a guarded app update is in flight") + return util.MsgError("err.selfupdate.alkalmazas_frissites_folyamatban") + } + // Agent reachable check — the host agent performs the swap; without it there is no update path // (the old in-container docker-compose flow is removed). if u.agent == nil { @@ -662,6 +689,14 @@ func (u *Updater) MaybeAutoUpdate() { u.dbg("maybeAutoUpdate: backup running — defer floor %s (will retry next report)", floor) return } + if u.appUpdating != nil && u.appUpdating() { + u.mu.Unlock() + // INFO, not DEBUG: an app update can run for minutes and this defers a FLEET floor. The + // backup line above is DEBUG because a backup window is routine and bounded; a deferred + // floor is the kind of silence that reads as "the floor never arrived" (R-604's shape). + u.logger.Printf("[INFO] [selfupdate] auto-update: a guarded app update is in flight — deferring floor %s (retries on the next report)", floor) + return + } u.updateRunning = true u.lastAutoFloorAttempt = floor u.mu.Unlock() diff --git a/controller/internal/stacks/manager.go b/controller/internal/stacks/manager.go index 05bfbc8..3747c66 100644 --- a/controller/internal/stacks/manager.go +++ b/controller/internal/stacks/manager.go @@ -189,6 +189,10 @@ type Stack struct { // Manager handles all docker compose stack operations. type Manager struct { + // selfUpdating (v0.261.0) reports whether the CONTROLLER is swapping itself. Set by + // SetSelfUpdatingCheck; nil means no gate. See update.go. + selfUpdating func() bool + cfg *config.Config logger *log.Logger composeCmd string diff --git a/controller/internal/stacks/update.go b/controller/internal/stacks/update.go index a8b6e07..8f454c6 100644 --- a/controller/internal/stacks/update.go +++ b/controller/internal/stacks/update.go @@ -227,6 +227,27 @@ func (m *Manager) SetUpdateGuards(g UpdateGuards) { m.mu.Unlock() } +// SetSelfUpdatingCheck wires the OTHER half of the v0.261.0 lock: is the CONTROLLER swapping itself? +// +// A plain callback rather than a member of UpdateGuards, for two reasons. UpdateGuards is the BACKUP +// side's interface and this has nothing to do with backups; and `stacks` must never import +// `selfupdate` (selfupdate reaches the agent, and the import would run the wrong way), so the +// dependency is inverted here and satisfied in main.go with `updater.IsUpdateRunning`. +// +// Nil is safe and means the pre-v0.261.0 behaviour: no self-update gate. +func (m *Manager) SetSelfUpdatingCheck(fn func() bool) { + m.mu.Lock() + m.selfUpdating = fn + m.mu.Unlock() +} + +func (m *Manager) selfUpdatingNow() bool { + m.mu.RLock() + fn := m.selfUpdating + m.mu.RUnlock() + return fn != nil && fn() +} + func (m *Manager) guards() UpdateGuards { m.mu.RLock() defer m.mu.RUnlock() @@ -323,6 +344,14 @@ func (m *Manager) UpdatePreflight(name string) *UpdateRefusal { if m.IsMigrating() { return m.refuseUpdate(name, "migrating", MsgUpdateMigrating, "a data migration is running") } + // v0.261.0 — the other half of the self-update lock. The controller's swap restarts this process; + // starting an app update into that is how an update loses its own supervisor mid-flight. TRANSIENT: + // the household is told to try again in a few minutes, and the caller in `09` §6.2 reads the + // machine-readable `downgrade`-style reason and retries rather than giving up. + if m.selfUpdatingNow() { + return m.refuseUpdateErr(name, "self_updating", util.MsgError("err.stacks.update_self_updating"), + "the controller is swapping itself — refusing to start an app update into a restart") + } // R-524 — THE PIN NEVER MOVES BACKWARDS WITHOUT THE OPERATOR. MEASURED 2026-09-15 (BIGNIGHT // Phase 6): privatebin was updated 2.0.5 → 2.0.6, the catalog was reverted to 2.0.5, and the // „Frissítés" button behind the badge would have advanced the pin to the OLDER image — on a @@ -439,6 +468,36 @@ func (m *Manager) IsUpdating(name string) bool { return ok && s.Updating } +// AnyUpdating reports whether a guarded update is in flight for ANY app. +// +// ── WHY IT EXISTS (v0.261.0) ──────────────────────────────────────────────────────────────────── +// +// The CONTROLLER updates itself too — daily at `self_update.auto_update_time` (04:30 by default) and, +// once the hub serves a floor above this box, after any report, at any hour. That swap restarts the +// controller container. Until v0.261.0 its ONLY busy gate was `backupRunning`, so a self-update could +// land in the middle of a guarded app update. +// +// MEASURED, and the gap is narrower than it looks but real: the update's `backing-up` phase takes the +// backup single-flight (`RunAppBackupNow` → `acquireRunning`), so `backupMgr.IsRunning()` ALREADY +// covered that one phase. It covers none of the others — `checking`, `safety-dump`, `pinning`, +// `pulling`, `starting`, `verifying` — and `starting`/`verifying` are exactly where the new version +// may already have touched the app's data. +// +// ⚠ IT MUST ANSWER FALSE FOR A HELD APP. `Stack.Updating` is cleared by `finishUpdate` on done, +// failed AND held, so a held app does not hold this lock — otherwise one app that cannot come up +// would block the controller's own updates for ever, which is a worse failure than the one this +// prevents. TestR608_LockReleasesAfterHold pins that. +func (m *Manager) AnyUpdating() bool { + m.mu.RLock() + defer m.mu.RUnlock() + for _, s := range m.stacks { + if s.Updating { + return true + } + } + return false +} + // UpdatingStacks is the set of apps an update is currently moving — for the dead-app alarm, which // must not count an app the update itself is recreating (R-330's class, a third mechanism). func (m *Manager) UpdatingStacks() map[string]bool { diff --git a/controller/internal/stacks/updatelock_test.go b/controller/internal/stacks/updatelock_test.go new file mode 100644 index 0000000..c7fb6af --- /dev/null +++ b/controller/internal/stacks/updatelock_test.go @@ -0,0 +1,125 @@ +package stacks + +import ( + "context" + "testing" + "time" +) + +// R-608 (v0.261.0) — the two-way lock between the controller's own update and a guarded app update. +// +// THE GAP THIS CLOSES, measured rather than assumed: the self-updater's only busy gate was +// `backupRunning`. The app update's `backing-up` phase DOES take the backup single-flight +// (`RunAppBackupNow` → `acquireRunning`), so that one phase was already covered. `checking`, +// `safety-dump`, `pinning`, `pulling`, `starting` and `verifying` were not — and the last two are +// where the new version may already have touched the customer's data. The controller's swap restarts +// this process, and 04:30 (the default `self_update.auto_update_time`) sits inside the 02:30–05:00 +// window `09` §3b Q1 proposes for automatic app updates. + +// TestR608_AnyUpdatingSeesAnUpdateInFlight is the mechanism half. +// +// COMPANION RED-PROOF (run 2026-09-21): make AnyUpdating always return false. The "while updating" +// sub-test then fails — and with it the whole gate, because every caller reads this one answer. +func TestR608_AnyUpdatingSeesAnUpdateInFlight(t *testing.T) { + m, _, _, _ := newSlice4Manager(t) + + if m.AnyUpdating() { + t.Fatal("no update has started; AnyUpdating must be false") + } + + release := make(chan struct{}) + m.updateHealthFn = func(context.Context, string, time.Duration) (bool, string) { + <-release + return true, "released" + } + if err := m.StartGuardedUpdate("nextcloud"); err != nil { + t.Fatalf("start: %v", err) + } + deadline := time.Now().Add(3 * time.Second) + for !m.AnyUpdating() && time.Now().Before(deadline) { + time.Sleep(2 * time.Millisecond) + } + if !m.AnyUpdating() { + t.Fatal("an update is in flight and AnyUpdating says false — the controller could swap under it") + } + close(release) + waitUpdateDone(t, m, "nextcloud") + if m.AnyUpdating() { + t.Error("the update finished; the lock must not still be held") + } +} + +// TestR608_LockReleasesAfterHold is the one that matters most, and it is a CONSEQUENCE test. +// +// A held app is an app that could not come up. If it kept this lock, the box would never update its +// own controller again — including the release that might FIX whatever held the app. That is a worse +// failure than the one the lock prevents, and it is the kind that is silent for weeks. +// +// COMPANION RED-PROOF (run 2026-09-21): in AnyUpdating, return true when `s.updateHeld` is set as +// well as when `s.Updating` is — the plausible "a held update is still an update" reading. This test +// then fails with the lock still held after the hold. +func TestR608_LockReleasesAfterHold(t *testing.T) { + m, _, _, _ := newSlice4Manager(t) + m.updateHealthFn = func(context.Context, string, time.Duration) (bool, string) { return false, "crash loop" } + + if err := m.StartGuardedUpdate("nextcloud"); err != nil { + t.Fatalf("start: %v", err) + } + st := waitUpdateDone(t, m, "nextcloud") + if st.UpdatePhase != UpdatePhaseFailed { + t.Fatalf("this fixture must end HELD, or the test proves nothing; phase=%q", st.UpdatePhase) + } + if m.AnyUpdating() { + t.Error("a HELD app must not hold the self-update lock for ever") + } +} + +// TestR608_PreflightRefusesWhileTheControllerSwaps is the reverse direction, and a CONSEQUENCE test: +// the question is not "is the callback wired" but "does the button refuse". +// +// COMPANION RED-PROOF (run 2026-09-21): delete the `m.selfUpdatingNow()` block from UpdatePreflight. +// The "while swapping" sub-test then fails with `ref = ` — an app update is allowed to start +// into a controller restart. +func TestR608_PreflightRefusesWhileTheControllerSwaps(t *testing.T) { + m, _, _, _ := newSlice4Manager(t) + + if ref := m.UpdatePreflight("nextcloud"); ref != nil { + t.Fatalf("control: with no self-update running the app update must be allowed, got %q", ref.Reason) + } + + swapping := true + m.SetSelfUpdatingCheck(func() bool { return swapping }) + + ref := m.UpdatePreflight("nextcloud") + if ref == nil { + t.Fatal("while the controller swaps itself the app update must be REFUSED") + } + if ref.Reason != "self_updating" { + t.Errorf("reason = %q, want %q", ref.Reason, "self_updating") + } + // It must carry its bundle key, or an English household reads a Hungarian refusal — R-589's + // failure in a new place, and the reason v0.260.0 routed these through errText. + if ref.Cause == nil { + t.Error("the refusal must carry its key as a Cause, not only a Hungarian literal") + } + + // And it must be TRANSIENT: once the swap ends the same app update is allowed, with no human + // action in between. A gate that latches is an outage. + swapping = false + if ref := m.UpdatePreflight("nextcloud"); ref != nil { + t.Errorf("after the swap the update must be allowed again, got %q", ref.Reason) + } +} + +// TestR608_NilChecksAreSafe — both halves default to the pre-v0.261.0 behaviour when unwired. A +// Manager built by a test fixture or a future construction path must not panic or fail closed here: +// failing closed on an UNWIRED gate would refuse every update on such a box. +func TestR608_NilChecksAreSafe(t *testing.T) { + m, _, _, _ := newSlice4Manager(t) + if m.selfUpdatingNow() { + t.Error("an unwired self-update check must read false") + } + if ref := m.UpdatePreflight("nextcloud"); ref != nil { + t.Errorf("an unwired gate must not refuse an update, got %q", ref.Reason) + } +} diff --git a/controller/scripts/i18n_go_keys.json b/controller/scripts/i18n_go_keys.json index b7962b4..0efc11f 100644 --- a/controller/scripts/i18n_go_keys.json +++ b/controller/scripts/i18n_go_keys.json @@ -1,6 +1,8 @@ { "_comment": "Localisation slice 2 (R-557). key -> the base-commit Go literal it replaced, or the ORDERED list of literals a concatenation joined. Checked by scripts/i18n_go_parity.py against scripts/i18n_go_base.json, which is frozen at 736f54b49610 (the base commit of release v0.252.0). A key whose Hungarian text is not byte-identical to what the Go code said fails the gate.", "_preexisting": { + "err.selfupdate.alkalmazas_frissites_folyamatban": "BORN AS A KEY, v0.261.0 (R-608) -- the controller refuses to swap itself while a guarded app update is in flight. A NEW sentence, never a Go literal. Pinned by TestR608_TriggerUpdateRefusedWhileAnAppUpdates, which asserts the key itself.", + "err.stacks.update_self_updating": "BORN AS A KEY, v0.261.0 (R-608) -- the app update refuses to start while the controller is swapping itself. A NEW sentence, never a Go literal. Pinned by TestR608_PreflightRefusesWhileTheControllerSwaps, which asserts the refusal carries a Cause so errText can render it.", "badge.update.ahead.title": "BORN AS A KEY, v0.260.0 (R-524) -- a NEW sentence, never a Go literal, so there is nothing in the base capture to measure it against. Pinned in both languages by TestUpdateBadgeFollowsTheLanguage.", "err.stacks.update_downgrade": "BORN AS A KEY, v0.260.0 (R-524) -- the downgrade refusal. A NEW sentence, never a Go literal. Pinned by TestR524_PreflightRefusesDowngrade, which asserts the refusal carries a Cause so api.Router.errText can render it.", "func.state.running": "slice 0 -- pinned by TestLocaleFuncsHungarianBundleMatchesFuncMap",