From d15ad105be09dc67fb1ca0f7d8afb7de3dd1a06e Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 5 Oct 2026 21:36:54 +0200 Subject: [PATCH] R-569: stack-lifecycle handlers pick their status by error KIND, not English words stop/start/restart/update, remove and delete matched "protected", "not found", "not deployed", "still running", "not orphaned" in err.Error(). New sentinels in internal/stacks (stack_errors.go) carried by the producers in manager.go/delete.go via util.KindErrorf (message bytes unchanged); api.stackOpStatusFor maps them. Tests: reworded-message table per family, wiring check over all three handlers, producers keep kind + words. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- REUSE.md | 1 + .../internal/api/r569_stack_op_status_test.go | 80 +++++++++++++++++++ controller/internal/api/router.go | 64 ++++++++------- controller/internal/stacks/delete.go | 20 ++--- controller/internal/stacks/manager.go | 13 +-- .../stacks/r569_stack_error_kinds_test.go | 51 ++++++++++++ controller/internal/stacks/stack_errors.go | 21 +++++ 7 files changed, 203 insertions(+), 47 deletions(-) create mode 100644 controller/internal/api/r569_stack_op_status_test.go create mode 100644 controller/internal/stacks/r569_stack_error_kinds_test.go create mode 100644 controller/internal/stacks/stack_errors.go diff --git a/REUSE.md b/REUSE.md index 2f80a82..afea11e 100644 --- a/REUSE.md +++ b/REUSE.md @@ -59,6 +59,7 @@ |---|---|---|---|---| | `util.KindErrorf` / `util.KindError` | controller/internal/util/errkind.go | `(kind error, format string, a ...interface{}) error` | ANY refusal a caller must tell apart: build the message exactly as `fmt.Errorf` would AND carry a sentinel for `errors.Is` | The message bytes are unchanged (pinned by tests); never `fmt.Errorf("%w: …")`, which would prepend the sentinel's own text to the customer's sentence | | `stacks.ErrAlreadyDeployed` / `ErrRequiredField` / `ErrPathMissing` / `ErrNotEnoughMemory` | controller/internal/stacks/deploy_errors.go | sentinels | the API's deploy status code (`api.deployStatusFor`) | 409 / 400 / 400 / 400. Do NOT add a text signature beside them | +| `stacks.ErrStackNotFound` / `ErrProtectedStack` / `ErrNotDeployed` / `ErrStillRunning` / `ErrNotOrphaned` | controller/internal/stacks/stack_errors.go | sentinels | the API's stop/start/restart/update, remove and delete status code (`api.stackOpStatusFor`, R-569) | 404 / 403 / 409 / 409 / 409. Only the producers in manager.go and delete.go carry them; a new refusal on those paths must carry one or it answers 500 | | `backup.ErrOffsiteQuota` | controller/internal/backup/offbox.go | sentinel | `ClassifyOffsiteFailure` telling a quota over-run apart | The other arms of that switch stay TEXT matches on purpose — they are restic's and ssh's own English output, which we neither write nor translate | | `monitor.WarnKind*` + `HealthReport.addWarning` / `WarningKindAt` | controller/internal/monitor/healthcheck.go | `(text, kind string)` | a health warning whose PLACEMENT the dashboard decides | Internal only: `internal/report/builder.go` copies Status/Issues/Warnings, so kinds never reach the hub (pinned) | | `settings.OffboxTarget.LastWarningKind` + `backup.OffboxWarnNoAppsSelected` | controller/internal/settings/settings.go | persisted string | the Távoli mentés page's stale-note substitution | Written and cleared with `LastWarning`; the text fallback in `offboxWarningDisplay` is LEGACY only (kind == "") and is removed when R-570 closes | diff --git a/controller/internal/api/r569_stack_op_status_test.go b/controller/internal/api/r569_stack_op_status_test.go new file mode 100644 index 0000000..d510aba --- /dev/null +++ b/controller/internal/api/r569_stack_op_status_test.go @@ -0,0 +1,80 @@ +package api + +import ( + "errors" + "fmt" + "net/http" + "os" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" + "gitea.dooplex.hu/admin/felhom-controller/internal/util" +) + +// R-569 — the stop/start/restart/update, remove and delete handlers pick their status by the +// refusal's KIND. Every row passes a REWORDED message: the words the handlers used to match +// ("protected", "not found", "not deployed", "still running", "not orphaned") are gone, only the +// kind remains. RED-PROOF (REPORT): restore any strings.Contains chain and its family's reworded rows +// answer 500. +func TestR569_StackOpStatus_SurvivesRewording(t *testing.T) { + cases := []struct { + family, name string + err error + want int + }{ + // action family (start/stop/restart/update) + {"action", "unknown app, reworded", util.KindErrorf(stacks.ErrStackNotFound, "no app called %q", "x"), http.StatusNotFound}, + {"action", "unknown app, wrapped by the caller", fmt.Errorf("restarting: %w", + util.KindErrorf(stacks.ErrStackNotFound, "no app called %q", "x")), http.StatusNotFound}, + {"action", "protected, reworded", util.KindErrorf(stacks.ErrProtectedStack, "%q is infrastructure", "traefik"), http.StatusForbidden}, + {"action", "update refusal", &stacks.UpdateRefusal{Reason: "busy", Message: "foglalt"}, http.StatusConflict}, + {"action", "update refusal: unknown app", &stacks.UpdateRefusal{Reason: "not_found", Message: "nincs ilyen"}, http.StatusNotFound}, + // remove family + {"remove", "not deployed, reworded", util.KindErrorf(stacks.ErrNotDeployed, "%q has nothing installed", "x"), http.StatusConflict}, + {"remove", "still running, reworded", util.KindErrorf(stacks.ErrStillRunning, "%q is up — stop it first", "x"), http.StatusConflict}, + {"remove", "protected, reworded", util.KindErrorf(stacks.ErrProtectedStack, "cannot take %q away", "traefik"), http.StatusForbidden}, + // delete family + {"delete", "not orphaned, reworded", util.KindErrorf(stacks.ErrNotOrphaned, "%q still belongs to the catalog", "x"), http.StatusConflict}, + {"delete", "still running, reworded", util.KindErrorf(stacks.ErrStillRunning, "%q is up", "x"), http.StatusConflict}, + {"delete", "unknown, reworded", util.KindErrorf(stacks.ErrStackNotFound, "nothing named %q", "x"), http.StatusNotFound}, + // the negative half: the old WORDS without a kind are not a refusal + {"any", "text that merely contains the old words", errors.New("docker: image not found; protected; still running"), http.StatusInternalServerError}, + {"any", "anything else", errors.New("compose exploded"), http.StatusInternalServerError}, + } + for _, c := range cases { + if got := stackOpStatusFor(c.err); got != c.want { + t.Errorf("%s / %s: stackOpStatusFor(%q) = %d, want %d", c.family, c.name, c.err.Error(), got, c.want) + } + } +} + +// The seam must be WIRED in all three handlers, and none may decide by reading the error text again. +func TestR569_HandlersUseTheKind(t *testing.T) { + src, err := os.ReadFile("router.go") + if err != nil { + t.Fatal(err) + } + body := string(src) + for _, fn := range []string{"func (r *Router) actionStack(", "func (r *Router) removeStack(", "func (r *Router) deleteStack("} { + i := strings.Index(body, fn) + if i < 0 { + t.Fatalf("%s not found — this test no longer reads what it thinks it reads", fn) + } + h := body[i:] + if j := strings.Index(h[10:], "\nfunc "); j > 0 { + h = h[:j+10] + } + if !strings.Contains(h, "stackOpStatusFor(err)") { + t.Errorf("%s no longer asks stackOpStatusFor for the status code", fn) + } + for _, line := range strings.Split(h, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "//") { + continue + } + if strings.Contains(line, "strings.Contains(err.Error()") { + t.Errorf("%s decides by reading the error text again (R-569): %s", fn, strings.TrimSpace(line)) + } + } + } +} diff --git a/controller/internal/api/router.go b/controller/internal/api/router.go index b5e0312..801068b 100644 --- a/controller/internal/api/router.go +++ b/controller/internal/api/router.go @@ -623,6 +623,36 @@ func desiredStateForAction(action string) (string, bool) { } } +// stackOpStatusFor maps a stack-lifecycle refusal (stop/start/restart/update, remove, delete) to its +// HTTP status by the refusal's KIND (R-569) — the R-553 rule one handler family over. +// +// Until R-569 these three handlers matched "protected", "not found", "not deployed", "still running" +// and "not orphaned" in err.Error(). Those strings are internal English, so localisation never moved +// them; a reworded internal message would have, silently turning a 404/403/409 into a 500. The kinds +// come from internal/stacks (stack_errors.go); the messages are unchanged. +// +// One function serves the three families because their kinds are disjoint: an action never returns +// ErrNotOrphaned, a remove never returns an UpdateRefusal. Pinned by +// TestR569_StackOpStatus_SurvivesRewording and TestR569_HandlersUseTheKind. +func stackOpStatusFor(err error) int { + var ref *stacks.UpdateRefusal + switch { + case errors.Is(err, stacks.ErrStackNotFound): + return http.StatusNotFound + case errors.As(err, &ref): + if ref.Reason == "not_found" { + return http.StatusNotFound + } + return http.StatusConflict + case errors.Is(err, stacks.ErrProtectedStack): + return http.StatusForbidden + case errors.Is(err, stacks.ErrNotDeployed), errors.Is(err, stacks.ErrStillRunning), + errors.Is(err, stacks.ErrNotOrphaned): + return http.StatusConflict + } + return http.StatusInternalServerError +} + func (r *Router) actionStack(w http.ResponseWriter, req *http.Request, action, name string) { r.logger.Printf("[INFO] [api] %s requested for stack: %s", action, name) r.dbg("actionStack: action=%s name=%s", action, name) @@ -760,17 +790,7 @@ func (r *Router) actionStack(w http.ResponseWriter, req *http.Request, action, n if err != nil { r.logger.Printf("[ERROR] [api] %s failed for %s: %v", action, name, err) - status := http.StatusInternalServerError - var ref *stacks.UpdateRefusal - if errors.As(err, &ref) { - status = http.StatusConflict - } - if strings.Contains(err.Error(), "protected") { - status = http.StatusForbidden - } - if strings.Contains(err.Error(), "not found") { - status = http.StatusNotFound - } + status := stackOpStatusFor(err) // R-569: by kind, never by the error's words writeJSON(w, status, apiResponse{OK: false, Error: r.errText(req, err)}) return } @@ -1052,16 +1072,7 @@ func (r *Router) removeStack(w http.ResponseWriter, req *http.Request, name stri writeJSON(w, http.StatusConflict, apiResponse{OK: false, Error: r.errText(req, err)}) return } - status := http.StatusInternalServerError - if strings.Contains(err.Error(), "protected") { - status = http.StatusForbidden - } - if strings.Contains(err.Error(), "not found") { - status = http.StatusNotFound - } - if strings.Contains(err.Error(), "not deployed") || strings.Contains(err.Error(), "still running") { - status = http.StatusConflict - } + status := stackOpStatusFor(err) // R-569: by kind, never by the error's words writeJSON(w, status, apiResponse{OK: false, Error: r.errText(req, err)}) return } @@ -1137,16 +1148,7 @@ func (r *Router) deleteStack(w http.ResponseWriter, req *http.Request, name stri writeJSON(w, http.StatusConflict, apiResponse{OK: false, Error: refused.Message}) return } - status := http.StatusInternalServerError - if strings.Contains(err.Error(), "protected") { - status = http.StatusForbidden - } - if strings.Contains(err.Error(), "not found") { - status = http.StatusNotFound - } - if strings.Contains(err.Error(), "not orphaned") || strings.Contains(err.Error(), "still running") { - status = http.StatusConflict - } + status := stackOpStatusFor(err) // R-569: by kind, never by the error's words writeJSON(w, status, apiResponse{OK: false, Error: r.errText(req, err)}) return } diff --git a/controller/internal/stacks/delete.go b/controller/internal/stacks/delete.go index e060ab8..b6a4ea7 100644 --- a/controller/internal/stacks/delete.go +++ b/controller/internal/stacks/delete.go @@ -269,12 +269,12 @@ func (m *Manager) DeleteStack(name string, removeHDDData bool) (*DeleteResponse, // Safety: never delete protected stacks if m.cfg.IsProtectedStack(name) { - return nil, fmt.Errorf("stack %q is protected and cannot be deleted", name) + return nil, util.KindErrorf(ErrProtectedStack, "stack %q is protected and cannot be deleted", name) } stack, ok := m.GetStack(name) if !ok { - return nil, fmt.Errorf("stack %q not found", name) + return nil, util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } if m.isDebug() { @@ -284,7 +284,7 @@ func (m *Manager) DeleteStack(name string, removeHDDData bool) (*DeleteResponse, // Must be orphaned if !stack.Orphaned { - return nil, fmt.Errorf("stack %q is not orphaned — only orphaned stacks can be deleted", name) + return nil, util.KindErrorf(ErrNotOrphaned, "stack %q is not orphaned — only orphaned stacks can be deleted", name) } // Must not be deploying (H2 fix) @@ -296,7 +296,7 @@ func (m *Manager) DeleteStack(name string, removeHDDData bool) (*DeleteResponse, // StateDegraded (R-51) counts as running here: a degraded stack still has LIVE containers, and // deleting its directory out from under them would leave orphans behind. if stack.State == StateRunning || stack.State == StateStarting || stack.State == StateRestarting || stack.State == StateDegraded { - return nil, fmt.Errorf("stack %q is still running — stop it first before deleting", name) + return nil, util.KindErrorf(ErrStillRunning, "stack %q is still running — stop it first before deleting", name) } stackDir := filepath.Dir(stack.ComposePath) @@ -417,7 +417,7 @@ func (m *Manager) DeleteStack(name string, removeHDDData bool) (*DeleteResponse, func (m *Manager) GetStackHDDData(name string) (*HDDDataResponse, error) { stack, ok := m.GetStack(name) if !ok { - return nil, fmt.Errorf("stack %q not found", name) + return nil, util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } // R-442: the app's own recorded HDD_PATH, not the global config (which no box sets). @@ -577,12 +577,12 @@ func (m *Manager) RemoveStack(name string, removeHDDData bool, backupPathsToRemo // Safety: never remove protected stacks if m.cfg.IsProtectedStack(name) { - return nil, fmt.Errorf("stack %q is protected and cannot be removed", name) + return nil, util.KindErrorf(ErrProtectedStack, "stack %q is protected and cannot be removed", name) } stack, ok := m.GetStack(name) if !ok { - return nil, fmt.Errorf("stack %q not found", name) + return nil, util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } if m.isDebug() { @@ -601,7 +601,7 @@ func (m *Manager) RemoveStack(name string, removeHDDData bool, backupPathsToRemo if !stack.Deployed { half, why := m.halfStateEvidence(name, stack) if !half { - return nil, fmt.Errorf("stack %q is not deployed", name) + return nil, util.KindErrorf(ErrNotDeployed, "stack %q is not deployed", name) } m.logger.Printf("[WARN] [stacks] RemoveStack %s: deployed=false but %s — removing what exists (R-634)", name, why) } @@ -632,7 +632,7 @@ func (m *Manager) RemoveStack(name string, removeHDDData bool, backupPathsToRemo // StateDegraded (R-51) counts as running here: a degraded stack still has LIVE containers, and // deleting its directory out from under them would leave orphans behind. if stack.State == StateRunning || stack.State == StateStarting || stack.State == StateRestarting || stack.State == StateDegraded { - return nil, fmt.Errorf("stack %q is still running — stop it first before removing", name) + return nil, util.KindErrorf(ErrStillRunning, "stack %q is still running — stop it first before removing", name) } stackDir := filepath.Dir(stack.ComposePath) @@ -830,7 +830,7 @@ func (m *Manager) RemoveStack(name string, removeHDDData bool, backupPathsToRemo func (m *Manager) GetStackBackupData(name string, drivePath string, mirrorDirs []string) (*BackupDataResponse, error) { _, ok := m.GetStack(name) if !ok { - return nil, fmt.Errorf("stack %q not found", name) + return nil, util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } resp := &BackupDataResponse{ diff --git a/controller/internal/stacks/manager.go b/controller/internal/stacks/manager.go index ab2496b..9feac13 100644 --- a/controller/internal/stacks/manager.go +++ b/controller/internal/stacks/manager.go @@ -21,6 +21,7 @@ import ( "gitea.dooplex.hu/admin/felhom-controller/internal/crypto" "gitea.dooplex.hu/admin/felhom-controller/internal/settings" "gitea.dooplex.hu/admin/felhom-controller/internal/system" + "gitea.dooplex.hu/admin/felhom-controller/internal/util" ) // ContainerState represents the current state of a container. @@ -1275,7 +1276,7 @@ func deepCopyStack(s *Stack) Stack { func (m *Manager) StartStack(name string) error { stack, ok := m.GetStack(name) if !ok { - return fmt.Errorf("stack %q not found", name) + return util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } if stack.Deploying { // R-634: a second `compose up -d` beside the deploy's own is the race that left apps running @@ -1338,7 +1339,7 @@ func (m *Manager) StartStackServices(name string, services []string) error { } stack, ok := m.GetStack(name) if !ok { - return fmt.Errorf("stack %q not found", name) + return util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } m.logger.Printf("[INFO] [stacks] Starting stack %s services only: %v", name, services) @@ -1358,12 +1359,12 @@ func (m *Manager) StartStackServices(name string, services []string) error { func (m *Manager) StopStack(name string) error { if m.cfg.IsProtectedStack(name) { - return fmt.Errorf("stack %q is protected and cannot be stopped", name) + return util.KindErrorf(ErrProtectedStack, "stack %q is protected and cannot be stopped", name) } stack, ok := m.GetStack(name) if !ok { - return fmt.Errorf("stack %q not found", name) + return util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } if stack.Deploying { // R-634, the backstop for EVERY caller (backup, quiesce, restore, export, storage, the Stop @@ -1393,7 +1394,7 @@ func (m *Manager) StopStack(name string) error { func (m *Manager) RestartStack(name string) error { stack, ok := m.GetStack(name) if !ok { - return fmt.Errorf("stack %q not found", name) + return util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } if m.isDebug() { @@ -1437,7 +1438,7 @@ func (m *Manager) RestartStack(name string) error { func (m *Manager) GetLogs(name string, lines int) (string, error) { stack, ok := m.GetStack(name) if !ok { - return "", fmt.Errorf("stack %q not found", name) + return "", util.KindErrorf(ErrStackNotFound, "stack %q not found", name) } if lines <= 0 { diff --git a/controller/internal/stacks/r569_stack_error_kinds_test.go b/controller/internal/stacks/r569_stack_error_kinds_test.go new file mode 100644 index 0000000..8df2a6a --- /dev/null +++ b/controller/internal/stacks/r569_stack_error_kinds_test.go @@ -0,0 +1,51 @@ +package stacks + +import ( + "errors" + "testing" +) + +// R-569 — the stack-lifecycle refusals the API maps to 404/403/409 carry their KIND, and their +// messages are byte-for-byte what they were. Every case below refuses BEFORE anything is executed +// (no compose, no docker): unknown name, protected name, or a scanned app that is not orphaned. +// +// RED-PROOF (REPORT): put any one producer back to plain fmt.Errorf and its errors.Is row fails while +// its message row still passes — the silent state the API's old text match lived in. +func TestR569_StackProducersCarryKindAndKeepTheirWords(t *testing.T) { + m := r553Manager(t, "display_name: R569\n") + cases := []struct { + name string + call func() error + kind error + wantMsg string + }{ + {"start unknown", func() error { return m.StartStack("nope") }, ErrStackNotFound, `stack "nope" not found`}, + {"stop unknown", func() error { return m.StopStack("nope") }, ErrStackNotFound, `stack "nope" not found`}, + {"restart unknown", func() error { return m.RestartStack("nope") }, ErrStackNotFound, `stack "nope" not found`}, + {"stop protected", func() error { return m.StopStack("samba") }, ErrProtectedStack, + `stack "samba" is protected and cannot be stopped`}, + {"remove unknown", func() error { _, err := m.RemoveStack("nope", false, nil); return err }, ErrStackNotFound, + `stack "nope" not found`}, + {"remove protected", func() error { _, err := m.RemoveStack("samba", false, nil); return err }, ErrProtectedStack, + `stack "samba" is protected and cannot be removed`}, + {"delete unknown", func() error { _, err := m.DeleteStack("nope", false); return err }, ErrStackNotFound, + `stack "nope" not found`}, + {"delete protected", func() error { _, err := m.DeleteStack("samba", false); return err }, ErrProtectedStack, + `stack "samba" is protected and cannot be deleted`}, + {"delete not orphaned", func() error { _, err := m.DeleteStack("r553app", false); return err }, ErrNotOrphaned, + `stack "r553app" is not orphaned — only orphaned stacks can be deleted`}, + } + for _, c := range cases { + err := c.call() + if err == nil { + t.Errorf("%s: no refusal", c.name) + continue + } + if !errors.Is(err, c.kind) { + t.Errorf("%s: errors.Is(err, %v) = false — the API would answer 500 (err=%q)", c.name, c.kind, err.Error()) + } + if err.Error() != c.wantMsg { + t.Errorf("%s: message moved:\n got %q\n want %q", c.name, err.Error(), c.wantMsg) + } + } +} diff --git a/controller/internal/stacks/stack_errors.go b/controller/internal/stacks/stack_errors.go new file mode 100644 index 0000000..cfa6b8c --- /dev/null +++ b/controller/internal/stacks/stack_errors.go @@ -0,0 +1,21 @@ +package stacks + +import "errors" + +// R-569 — the stack-lifecycle refusals carry a KIND, so the API's stop/start/restart/update, remove +// and delete handlers pick their status code with errors.Is instead of matching "protected", +// "not found", "not deployed", "still running" or "not orphaned" in the error text. The messages are +// unchanged byte for byte (util.KindErrorf); a reworded message no longer moves a status code. +// Pinned by TestR569_StackOpStatus_SurvivesRewording (internal/api). +var ( + // ErrStackNotFound — no stack by that name (API: 404). + ErrStackNotFound = errors.New("stack not found") + // ErrProtectedStack — an infrastructure stack the household may not stop/remove/delete (API: 403). + ErrProtectedStack = errors.New("stack is protected") + // ErrNotDeployed — remove asked of a stack with nothing deployed (API: 409). + ErrNotDeployed = errors.New("stack is not deployed") + // ErrStillRunning — remove/delete asked of a running stack (API: 409). + ErrStillRunning = errors.New("stack is still running") + // ErrNotOrphaned — delete asked of a stack that is not orphaned (API: 409). + ErrNotOrphaned = errors.New("stack is not orphaned") +)