diff --git a/CHANGELOG.md b/CHANGELOG.md index 91f2ef93..5c66a0bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,11 @@ hub answers 400 to both and nothing else changes. runs `psql -v ON_ERROR_STOP=1`. Tests `internal/appexport/r1187_abdfg_test.go` (6), red-proved with the old behaviour behind the new function shapes (link packing error, a 10-s hang on the FIFO, link not restored, import not refused, bad paths accepted, postgis missed). +- **R-1298:** the hold report skips an install's hold (`unhealthy_stop`, `update_failed`, `putback_pending`) of an app that + is not installed (`holdreport.Reporter.SetInstalled`: the app's `app.yaml` exists — an unreadable one still counts, + R-1220) and reports it cleared if it was reported; a restore hold is always reported (R-379). 9202's four orphan holds + from a hand removal would otherwise have mailed weekly. Test `TestR1298_OnlyHoldsOfInstalledAppsAreReported`, + red-proved; the wiring test pins the `SetInstalled` line. - `settings.RestoreHold.Why()` / `WireReason()`: the hold sentence moved out of `cmd/controller` (`holdWhy` calls it), so `--restore-holds` and the hub's list say the same thing. - Tests `internal/holdreport/holdreport_test.go` (8): the report field, the daily repeat, cleared once, cleared while diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index fea6d5e4..5789f771 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1068,6 +1068,12 @@ func main() { // when one is gone, every hold again once a day and after each start. One observer of the hold store, so every // writer is covered. Pinned by internal/holdreport. holdReporter := holdreport.New(sett, notifier.PushEvent, logger) + // R-1298: „installed" = the app's app.yaml file exists (a product removal deletes it; an unreadable one still counts, + // the R-1220 rule). An install's hold of an app without one is not reported. + holdReporter.SetInstalled(func(stack string) bool { + _, err := os.Stat(filepath.Join(cfg.Paths.StacksDir, stack, "app.yaml")) + return err == nil + }) sched.Every("hold-report", time.Minute, func(ctx context.Context) error { holdReporter.Tick(time.Now()) return nil diff --git a/controller/internal/holdreport/holdreport.go b/controller/internal/holdreport/holdreport.go index 3ab060c0..b94a79ea 100644 --- a/controller/internal/holdreport/holdreport.go +++ b/controller/internal/holdreport/holdreport.go @@ -60,6 +60,9 @@ type Reporter struct { push Push logger *log.Logger lastFull time.Time + // installed (R-1298) — nil = every app counts as installed. + installed func(stack string) bool + skipLogged map[string]bool } // New builds a reporter. The first Tick re-sends every hold (a controller start). @@ -69,13 +72,42 @@ func New(store Store, push Push, logger *log.Logger) *Reporter { func key(h settings.RestoreHold) string { return h.WireReason() + "|" + h.At } +// SetInstalled wires the „is this app installed?" check (R-1298). nil = every app counts as installed. +func (r *Reporter) SetInstalled(f func(stack string) bool) { r.installed = f } + +// installHold is a hold that belongs to an app's INSTALL (R-704): a removal ends it. A restore hold (restore_failed, +// restore_mixed) does not — a person clears it, so it stays reported whatever the app's state (R-379). +func installHold(h settings.RestoreHold) bool { + switch h.Reason { + case settings.HoldReasonUnhealthyStop, settings.HoldReasonUpdateFailed, settings.HoldReasonPutBackPending: + return true + } + return false +} + // Tick sends what changed since the last tick, and every hold again when a day has passed. func (r *Reporter) Tick(now time.Time) { if r.store == nil || r.push == nil { return } - holds := r.store.ListRestoreHolds() - sort.Slice(holds, func(i, j int) bool { return holds[i].Stack < holds[j].Stack }) + all := r.store.ListRestoreHolds() + sort.Slice(all, func(i, j int) bool { return all[i].Stack < all[j].Stack }) + // R-1298: an install's hold of an app that is not installed any more (removed outside the product) is not a held + // app — it is skipped, and reported cleared if it was reported before. Logged once per stack per process. + holds := all[:0:0] + for _, h := range all { + if r.installed != nil && installHold(h) && !r.installed(h.Stack) { + if r.skipLogged == nil { + r.skipLogged = map[string]bool{} + } + if !r.skipLogged[h.Stack] { + r.skipLogged[h.Stack] = true + r.logf("[INFO] [holdreport] %s: a %s hold of an app that is not installed — not reported (R-1298)", h.Stack, h.WireReason()) + } + continue + } + holds = append(holds, h) + } prev := r.store.GetReportedHolds() full := r.lastFull.IsZero() || now.Sub(r.lastFull) >= ResendEvery diff --git a/controller/internal/holdreport/holdreport_test.go b/controller/internal/holdreport/holdreport_test.go index 5cf5e3bf..36f3b3be 100644 --- a/controller/internal/holdreport/holdreport_test.go +++ b/controller/internal/holdreport/holdreport_test.go @@ -155,7 +155,7 @@ func TestD53_TheReporterIsWiredInMain(t *testing.T) { } } src := strings.Join(live, "\n") - for _, want := range []string{"holdreport.New(sett, notifier.PushEvent, logger)", `sched.Every("hold-report"`, "holdReporter.Tick("} { + for _, want := range []string{"holdreport.New(sett, notifier.PushEvent, logger)", `sched.Every("hold-report"`, "holdReporter.Tick(", "holdReporter.SetInstalled("} { if !strings.Contains(src, want) { t.Errorf("main.go lacks %q — the hold report is not wired", want) } @@ -180,3 +180,32 @@ func TestD53_TheReportedRecordSurvivesAReload(t *testing.T) { t.Fatalf("after a reload the record reads %q", got) } } + +// R-1298: an install's hold (crash-loop stop, failed update, waiting put-back) of an app that is NOT installed any more is +// not reported — on 9202 four such holds from a hand removal would have mailed the operator weekly about apps that do not +// exist. A restore hold stays reported whatever the app's state (R-379: a person clears it). A hold reported earlier and +// now skipped is reported cleared. COMPANION RED-PROOF: without the installed check all three holds are reported. +func TestR1298_OnlyHoldsOfInstalledAppsAreReported(t *testing.T) { + st := &fakeStore{holds: map[string]settings.RestoreHold{ + "gone": {Stack: "gone", At: "2026-09-24T12:16:24Z", Reason: settings.HoldReasonUnhealthyStop}, + "gonedb": {Stack: "gonedb", At: "2026-09-24T12:16:24Z", Reason: settings.HoldReasonRestoreFailed}, + "present": {Stack: "present", At: "2026-10-26T17:37:52Z", Reason: settings.HoldReasonUpdateFailed}, + }, reported: map[string]string{"gone": "unhealthy_stop|2026-09-24T12:16:24Z"}} + var out []sent + r := New(st, rec(&out), nil) + r.SetInstalled(func(stack string) bool { return stack == "present" }) + r.Tick(t0) + got := map[string]string{} + for _, s := range out { + got[s.d.StackName] = s.typ + } + if got["gone"] != EventCleared { + t.Errorf("the hold of an app that is not installed must be reported cleared, got %q", got["gone"]) + } + if got["gonedb"] != EventHeld { + t.Errorf("a restore hold must stay reported even when the app is gone, got %q", got["gonedb"]) + } + if got["present"] != EventHeld { + t.Errorf("an installed app's hold must be reported, got %q", got["present"]) + } +}