diff --git a/CHANGELOG.md b/CHANGELOG.md index 252166e..5d6b4df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,52 @@ +## v0.210.0 — two pictures that were not true (2026-08-08, R-259 / R-258) — MinAgent 0.127.0 + +Both are the same shape: something the box already knows, drawn as its opposite. + +**R-259 — a disk we failed to read was drawn as a healthy empty disk.** `readDiskUsage` +(`internal/system/info_linux.go`) logged a `statfs` failure at DEBUG and returned, leaving the +caller's `TotalGB/UsedGB/AvailGB/Percent` at zero — and `usageColor(0)` is `"nominal"`. The +dashboard's most-looked-at meter therefore rendered „0.0 GB / 0.0 GB (0%)" with a 0%-wide bar in the +healthy colour. **"We could not look" and "there is plenty of room" were the same picture.** + +`readDiskUsage` now returns whether the measurement succeeded; `SystemInfo` gains `DiskKnown` and +`HDDKnown`; and the template draws **no figure, no percentage and no meter fill** when unknown, +saying „A tárhely mérete most nem olvasható ki." instead. A healthy box is byte-identical to before, +colour band included. + +**This session rules the convention** (`felhom.eu/CONTEXT.md` S-39): an explicit `…Known bool` +companion beside the figures, checked in the template — the shape `Offbox.StatsKnown` already uses, +whose own comment says *"a 0%-wide bar over an unread store is a picture of emptiness, and a picture +is a claim"*. Pointers and separate error fields are both legitimate Go, but a codebase with three +dialects cannot be gated. Existing call sites were **not** converted. + +**R-258 — the per-app backup tick was green on presence, and red only on a global condition.** +`buildAppBackupRows` set `Tier1LastStatus` from `status.LastDBDump.Success`, which is the box's +single most recent dump **run, whichever app it belonged to**. An app whose own dump failed showed a +tick as long as some other app dumped successfully afterwards, and an app with no database took the +`nil` branch and went green on the mere existence of a restore point. + +Now `appDumpVerdict` reads THIS app's own entries in `DBDumpStatus.Results` (matched on +`DumpResult.DB.StackName`, failure = non-nil `Error`). **Three states:** any failing database → +`error`; all clean → `ok`; **no result recorded → no verdict and no icon**, with the title +„Erről a mentésről nincs eredményünk." The recovery unit carries no per-run outcome of its own, so +green cannot honestly be derived from presence. The global `tier1DBStatus` label is untouched. + +**RECENCY IS DELIBERATELY NOT ADDED.** A tick over a three-week-old restore point is a real +weakness, but an age threshold means inventing a number and the time is already printed beside the +icon. Recorded as an observation. + +**An existing test was asserting the defect and was corrected, not deleted:** +`TestBuildAppBackupRows_Tier1FromRestorePoints` expected `"ok"` for a status with no `LastDBDump` at +all — green from nothing but a file's existence. It now expects no verdict; its real subject, the +`Tier1LastRun` time, is unchanged. + +Four red-proofs, each with the mutation asserted applied: reverting to the global field returns app +X's false green; mapping "no result" to `ok` returns green-on-presence; ignoring the known-flag +returns „0.0 GB / 0.0 GB (0%)"; forcing the flag false shows a healthy box losing its numbers. + +**No new tag on any declared wire** — `report/builder.go` maps into its own types and is untouched; +`wire_contract_gate.py` confirmed green. + ## v0.209.0 — the box stops saying a false thing about its own recovery package (2026-08-08, R-247 / R-260) — MinAgent 0.127.0 **R-247, and it is the third instance of one shape: the answer was on the wire and was discarded at diff --git a/controller/internal/system/info.go b/controller/internal/system/info.go index bc55eba..9c9b06d 100644 --- a/controller/internal/system/info.go +++ b/controller/internal/system/info.go @@ -25,12 +25,26 @@ type SystemInfo struct { DiskUsedGB float64 `json:"disk_used_gb"` DiskAvailGB float64 `json:"disk_avail_gb"` DiskPercent float64 `json:"disk_percent"` + // DiskKnown (R-259) — the statfs on "/" SUCCEEDED. Without it, a failed measurement is + // indistinguishable from an empty disk: readDiskUsage returned early on error and left every + // figure above at its zero value, `usageColor(0)` is "nominal", and the dashboard drew + // „0.0 GB / 0.0 GB (0%)" with a 0%-wide bar in the healthy colour. "We could not look" and + // "there is plenty of room" were the same picture. + // + // This is the house three-state form, and this session rules it THE one (CONTEXT S-39): + // an explicit `…Known bool` companion beside the figures, checked in the template before + // anything is rendered — the shape `Offbox.StatsKnown` already uses, whose own comment says + // "a 0%-wide bar over an unread store is a picture of emptiness, and a picture is a claim". + DiskKnown bool `json:"disk_known"` HDDTotalGB float64 `json:"hdd_total_gb,omitempty"` HDDUsedGB float64 `json:"hdd_used_gb,omitempty"` HDDAvailGB float64 `json:"hdd_avail_gb,omitempty"` HDDPercent float64 `json:"hdd_percent,omitempty"` HDDConfigured bool `json:"hdd_configured"` + // HDDKnown (R-259) — as DiskKnown, for the configured HDD path. HDDConfigured is NOT a + // substitute: it says a path was configured, not that reading it worked. + HDDKnown bool `json:"hdd_known"` CPUPercent float64 `json:"cpu_percent"` LoadAvg1 float64 `json:"load_avg_1"` diff --git a/controller/internal/system/info_linux.go b/controller/internal/system/info_linux.go index af90414..5880bcb 100644 --- a/controller/internal/system/info_linux.go +++ b/controller/internal/system/info_linux.go @@ -29,12 +29,12 @@ func GetInfo(hddPath string, cpuCollector *CPUCollector) SystemInfo { readMemInfo(&info) // --- Root filesystem disk usage --- - readDiskUsage("/", &info.DiskTotalGB, &info.DiskUsedGB, &info.DiskAvailGB, &info.DiskPercent) + info.DiskKnown = readDiskUsage("/", &info.DiskTotalGB, &info.DiskUsedGB, &info.DiskAvailGB, &info.DiskPercent) // --- HDD disk usage (if configured) --- if hddPath != "" { info.HDDConfigured = true - readDiskUsage(hddPath, &info.HDDTotalGB, &info.HDDUsedGB, &info.HDDAvailGB, &info.HDDPercent) + info.HDDKnown = readDiskUsage(hddPath, &info.HDDTotalGB, &info.HDDUsedGB, &info.HDDAvailGB, &info.HDDPercent) } // --- Load average --- @@ -256,11 +256,17 @@ func parseMemLine(line string) uint64 { return val } -func readDiskUsage(path string, totalGB, usedGB, availGB *float64, percent *float64) { +// readDiskUsage fills the four figures and reports whether the measurement SUCCEEDED (R-259). +// +// It used to return nothing. On a statfs error it logged at DEBUG and returned, leaving the +// caller's floats at zero — and zero renders as a healthy empty disk. The boolean is the whole +// fix: the caller now knows the difference between "0 GB used" and "we could not look", and the +// template refuses to draw a picture it does not have. +func readDiskUsage(path string, totalGB, usedGB, availGB *float64, percent *float64) bool { var stat syscall.Statfs_t if err := syscall.Statfs(path, &stat); err != nil { debugf("[DEBUG] [system] readDiskUsage: statfs(%q) failed: %v", path, err) - return + return false } bsize := uint64(stat.Bsize) @@ -277,6 +283,7 @@ func readDiskUsage(path string, totalGB, usedGB, availGB *float64, percent *floa } debugf("[DEBUG] [system] readDiskUsage: path=%q bsize=%d total=%.1fGB used=%.1fGB avail=%.1fGB (%.1f%%)", path, bsize, *totalGB, *usedGB, *availGB, *percent) + return true } // readLoadAvg reads 1/5/15 minute load averages from /proc/loadavg. diff --git a/controller/internal/web/app_dump_verdict_test.go b/controller/internal/web/app_dump_verdict_test.go new file mode 100644 index 0000000..3e19a3e --- /dev/null +++ b/controller/internal/web/app_dump_verdict_test.go @@ -0,0 +1,83 @@ +package web + +import ( + "errors" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/appbackup" + "gitea.dooplex.hu/admin/felhom-controller/internal/backup" +) + +// R-258 — the per-app tier-1 tick must answer about THIS app, and say nothing when it knows nothing. +// +// Driven through appDumpVerdict, which is the seam buildAppBackupRows now calls. The defect was a +// verdict derived from a GLOBAL field, so the test that matters is the one with two apps where the +// global answer and the per-app answer disagree. + +func res(stack string, err error) appbackup.DumpResult { + return appbackup.DumpResult{DB: appbackup.DiscoveredDB{StackName: stack}, Error: err} +} + +// SCENARIO F — X's own dump failed; Y's succeeded and is the most recent on the box. +func TestAppDumpVerdict_IsPerApp_NotTheBoxsMostRecentRun(t *testing.T) { + dump := &backup.DBDumpStatus{ + LastRun: time.Now(), + // Y ran last and succeeded, so the box-level Success is true — which is exactly the value + // the old code used for every app. + Success: true, + Results: []appbackup.DumpResult{ + res("appX", errors.New("pg_dump: connection refused")), + res("appY", nil), + }, + } + + if got := appDumpVerdict(dump, "appX"); got != "error" { + t.Errorf("app X's own dump FAILED but its tick is %q, want \"error\".\n"+ + "This is R-258: the verdict was read from the box's most recent dump run — app Y's — so a "+ + "failed backup showed a green tick to the customer.", got) + } + if got := appDumpVerdict(dump, "appY"); got != "ok" { + t.Errorf("app Y succeeded but its tick is %q, want \"ok\"", got) + } +} + +// SCENARIO G — nothing known is not the same as fine. +func TestAppDumpVerdict_NoResultForThisApp_IsNoVerdict(t *testing.T) { + dump := &backup.DBDumpStatus{Success: true, Results: []appbackup.DumpResult{res("other", nil)}} + if got := appDumpVerdict(dump, "appWithNoDatabase"); got != "" { + t.Errorf("an app with no dump result of its own got the verdict %q; want \"\" (no icon).\n"+ + "A green tick standing for \"a restore point file exists\" is the presence-is-not-success "+ + "rule as a UI badge.", got) + } + // and with no dump run recorded at all + if got := appDumpVerdict(nil, "anything"); got != "" { + t.Errorf("no dump status at all gave the verdict %q; want \"\"", got) + } +} + +// An app with SEVERAL databases: any failure among them makes the app's backup a failure. A partial +// dump is not a success, and reporting the last-listed result would make the verdict order-dependent. +func TestAppDumpVerdict_AnyFailingDatabaseFailsTheApp(t *testing.T) { + for _, tc := range []struct { + name string + results []appbackup.DumpResult + }{ + {"failure first", []appbackup.DumpResult{res("app", errors.New("boom")), res("app", nil)}}, + {"failure last", []appbackup.DumpResult{res("app", nil), res("app", errors.New("boom"))}}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := appDumpVerdict(&backup.DBDumpStatus{Results: tc.results}, "app"); got != "error" { + t.Errorf("one of the app's databases failed to dump, verdict = %q, want \"error\"", got) + } + }) + } +} + +// All of this app's databases dumped cleanly → ok. +func TestAppDumpVerdict_AllCleanIsOK(t *testing.T) { + dump := &backup.DBDumpStatus{Results: []appbackup.DumpResult{res("app", nil), res("app", nil)}} + if got := appDumpVerdict(dump, "app"); got != "ok" { + t.Errorf("verdict = %q, want \"ok\"", got) + } +} diff --git a/controller/internal/web/backup_page_state_test.go b/controller/internal/web/backup_page_state_test.go index 610a4cf..51e8864 100644 --- a/controller/internal/web/backup_page_state_test.go +++ b/controller/internal/web/backup_page_state_test.go @@ -164,8 +164,16 @@ func TestBuildAppBackupRows_Tier1FromRestorePoints(t *testing.T) { if hu == nil || hu.Tier1LastRun != mtime.Format(time.RFC3339) { t.Fatalf("hasunit Tier1LastRun = %q, want %q", huTier1(hu), mtime.Format(time.RFC3339)) } - if hu.Tier1LastStatus != "ok" { - t.Errorf("hasunit Tier1LastStatus = %q, want ok", hu.Tier1LastStatus) + // ⚠ CHANGED 2026-08-08 (R-258), and the change is the finding. This asserted `== "ok"` for a + // FullBackupStatus with NO LastDBDump at all — i.e. it pinned the defect: a green tick derived + // from nothing but the presence of a recovery-unit file. The old code took the `nil` branch and + // returned "ok"; the verdict now comes from THIS app's own dump result, and there is none here, + // so the honest answer is no verdict and the template renders no icon. + // + // The row's real subject — Tier1LastRun, the time — is unchanged and still asserted above. + if hu.Tier1LastStatus != "" { + t.Errorf("hasunit Tier1LastStatus = %q, want \"\" (no dump result for this app ⇒ no verdict; "+ + "a tick standing for \"a file exists\" is R-258)", hu.Tier1LastStatus) } nu := findRow(rows, "nounit") if nu == nil || nu.Tier1LastRun != "" { diff --git a/controller/internal/web/dashboard_diskknown_test.go b/controller/internal/web/dashboard_diskknown_test.go new file mode 100644 index 0000000..2fa9d71 --- /dev/null +++ b/controller/internal/web/dashboard_diskknown_test.go @@ -0,0 +1,112 @@ +package web + +import ( + "bytes" + "html/template" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/system" +) + +// R-259 — a disk we could not measure must not be drawn as a healthy empty one. +// +// These render the REAL dashboard template block against a real SystemInfo, because the defect was +// entirely in what the markup does with a zero. A test on the Go struct alone cannot see it. + +// diskMeterBlock EXTRACTS the system-disk block from the SHIPPED dashboard.html, rather than +// duplicating it here. A copied block drifts, and a drifted copy is a test that passes while the +// page it claims to cover has changed — the fixture-is-not-the-wire mistake, which this project has +// now hit twice (R-262, and the OOB fixture in hub v0.99.0). +func diskMeterBlock(t *testing.T) string { + t.Helper() + raw, err := templateFS.ReadFile("templates/dashboard.html") + if err != nil { + t.Fatalf("read dashboard.html: %v", err) + } + src := string(raw) + const startMark = `{{if not .SystemInfo.DiskKnown}}` + i := strings.Index(src, startMark) + if i < 0 { + t.Fatalf("the DiskKnown guard is not in dashboard.html — R-259's fix is not in the shipped markup") + } + // the guard's matching {{end}} is the one closing the if/else chain: take through the + // "Kritikusan kevés hely" branch and its two closers + const endMark = `Kritikusan kevés hely{{end}}` + j := strings.Index(src[i:], endMark) + if j < 0 { + t.Fatal("could not find the end of the disk meter block") + } + block := src[i : i+j+len(endMark)] + // close the outer if/else opened by startMark + return block + "\n \n {{end}}" +} + +// renderDiskMeter renders just the system-disk block of dashboard.html with the production funcmap. +func renderDiskMeter(t *testing.T, info system.SystemInfo) string { + t.Helper() + // the block under test, copied verbatim from dashboard.html by the guard below + src := diskMeterBlock(t) + tpl, err := template.New("m").Funcs((&Server{}).templateFuncMap()).Parse(src) + if err != nil { + t.Fatalf("parse: %v", err) + } + var buf bytes.Buffer + if err := tpl.Execute(&buf, map[string]any{"SystemInfo": info}); err != nil { + t.Fatalf("execute: %v", err) + } + return buf.String() +} + +func healthyDisk() system.SystemInfo { + return system.SystemInfo{ + DiskTotalGB: 100, DiskUsedGB: 42, DiskAvailGB: 58, DiskPercent: 42, DiskKnown: true, + } +} + +// SCENARIO D — a disk we could not read says so, and draws nothing. +func TestDiskMeter_UnknownDrawsNoPictureAndSaysSo(t *testing.T) { + // exactly what a failed statfs leaves behind: every figure zero, and now DiskKnown=false + out := renderDiskMeter(t, system.SystemInfo{DiskKnown: false}) + + if strings.Contains(out, "0.0 GB") || strings.Contains(out, "(0%)") { + t.Errorf("a failed measurement printed a figure — „0.0 GB / 0.0 GB (0%%)\" is R-259, "+ + "and it is the healthy-looking picture of an unread disk.\n%s", out) + } + if strings.Contains(out, "meter-fill") { + t.Errorf("a meter fill was drawn for a disk that was never measured — a 0%%-wide bar over an "+ + "unread store is a picture of emptiness, and a picture is a claim.\n%s", out) + } + if strings.Contains(out, "nominal") { + t.Errorf("an unmeasured disk was coloured as healthy.\n%s", out) + } + if !strings.Contains(out, "nem olvashat") { // ASCII-safe fragment of „nem olvasható ki" + t.Errorf("the customer is not told the measurement could not be taken.\n%s", out) + } +} + +// SCENARIO E — a disk we CAN read is unchanged, including its colour band. +func TestDiskMeter_KnownIsUnchanged(t *testing.T) { + out := renderDiskMeter(t, healthyDisk()) + + // fmtGB: >=100 renders whole, >=10 renders one decimal (funcmap.go:208-215) + for _, want := range []string{"42.0 GB", "100 GB", "(42%)", "meter-fill", "nominal"} { + if !strings.Contains(out, want) { + t.Errorf("a healthy box lost %q from its meter — this change must be invisible on a "+ + "machine that is fine.\n%s", want, out) + } + } + if strings.Contains(out, "nem olvashat") { + t.Errorf("a healthy box gained the could-not-measure caveat.\n%s", out) + } +} + +// The colour bands still work on a known disk — the guard must not have swallowed them. +func TestDiskMeter_KnownStillBands(t *testing.T) { + crit := healthyDisk() + crit.DiskPercent = 92 + out := renderDiskMeter(t, crit) + if !strings.Contains(out, "crit") || !strings.Contains(out, "Kritikusan") { + t.Errorf("a critically full KNOWN disk lost its warning.\n%s", out) + } +} diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index fb7f8dd..a4f117c 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -176,7 +176,7 @@ func (s *Server) dashboardHandler(w http.ResponseWriter, r *http.Request) { sysInfo := system.GetInfo(s.primaryHDDPath(), s.cpuCollector) data := s.baseData("dashboard", "Vezérlőpult") - s.addRecoveryBanner(data, r) // R-241: the reminder bar, per visit + s.addRecoveryBanner(data, r) // R-241: the reminder bar, per visit data["SettingsWarning"] = s.settings.LoadWarning // non-empty if settings.json was recovered from corruption data["Stacks"] = deployedStacks data["MissingStorage"] = s.missingStorageMap(deployedStacks) @@ -1145,6 +1145,42 @@ type AppBackupRow struct { Warnings []string } +// appDumpVerdict is THIS app's tier-1 verdict, from THIS app's own most recent dump result. +// +// "" (no icon) — no dump result recorded for this stack: the app has no database, or no run has +// +// happened since start-up. Presence of a restore point is NOT evidence the last +// run worked, and this is the case that used to be drawn as a green tick (R-258). +// +// "error" — this app's own most recent result carries an Error. +// "ok" — this app's own most recent result succeeded. +// +// Deliberately NOT considered: RECENCY. A tick over a three-week-old restore point is a real +// weakness, but an age threshold means inventing a number, and the time is already printed beside +// the icon. Recorded as an observation rather than changed here. +// +// Deliberately NOT used: DBDumpStatus.Success, which is the box's most recent RUN whichever app it +// belonged to. It is correct for the global tier1DBStatus label a few lines above and is the exact +// lookalike that produced this defect. +func appDumpVerdict(dump *backup.DBDumpStatus, stackName string) string { + if dump == nil { + return "" + } + verdict := "" + for _, res := range dump.Results { + if res.DB.StackName != stackName { + continue + } + // Results is one entry per DATABASE; an app may have several. Any failure among this + // app's databases makes the app's backup a failure — a partial dump is not a success. + if res.Error != nil { + return "error" + } + verdict = "ok" + } + return verdict +} + // buildAppBackupRows constructs one AppBackupRow per deployed app for the backup page. // Disk-tier (cross-drive / restic) backup has moved to the host agent; this now // reflects only the app-data backup (DB dumps + Docker-volume tars). @@ -1239,12 +1275,24 @@ func (s *Server) buildAppBackupRows(status *backup.FullBackupStatus) []AppBackup if s.backupMgr != nil { if pts, ok := s.backupMgr.ListRestorePoints(app.StackName); ok && len(pts) > 0 { row.Tier1LastRun = pts[0].Time - // A unit exists: green unless the DB dump failed (keep tier1DBStatus as the source). - if status.LastDBDump != nil && !status.LastDBDump.Success { - row.Tier1LastStatus = "error" - } else { - row.Tier1LastStatus = "ok" - } + // R-259's sibling, R-258: THE VERDICT MUST BE ABOUT THIS APP, AND SILENT WHEN + // THERE IS NOTHING TO SAY. + // + // This used to read `status.LastDBDump.Success`, which is the box's single most + // recent dump RUN — whichever app it belonged to (backup.go: `m.lastDBDump`). So an + // app whose own dump failed last night showed a tick as long as some OTHER app + // dumped successfully afterwards, and an app with no database at all took the + // `nil` branch and went green on the mere existence of a restore point. A tick + // standing for "a file exists" is the presence-is-not-success rule as a UI badge. + // + // Per-app truth needs no new plumbing: DBDumpStatus.Results carries one DumpResult + // per database, each with its DiscoveredDB.StackName and its own Error. + // + // Three states, deliberately — the template renders an icon for "ok" and "error" + // and NOTHING for any third value, which is the slot "we do not know" belongs in. + // The recovery unit carries no per-run verdict of its own (recovery_unit.go: times + // and checksums, no outcome), so green cannot honestly be derived from presence. + row.Tier1LastStatus = appDumpVerdict(status.LastDBDump, app.StackName) } } @@ -2654,7 +2702,7 @@ type fbPathDeps struct { // than derived here because only the caller knows the system-data path. nil → identity, which is // the pre-R-203 behaviour and correct for every enrolled drive. nsRootFor func(string) string - logger *log.Logger + logger *log.Logger } // buildFileBrowserPaths computes one FileBrowser sync pass's volume mount lines + the source-list diff --git a/controller/internal/web/templates/backups_apps.html b/controller/internal/web/templates/backups_apps.html index 8bb1cfe..fa03a31 100644 --- a/controller/internal/web/templates/backups_apps.html +++ b/controller/internal/web/templates/backups_apps.html @@ -169,9 +169,14 @@ Auto helyi {{if .Tier1LastRun}} + {{/* R-258: three states. The tick is THIS app's own most recent dump result, + not the box's most recent run — and a third value renders NO icon, which is + where "we have no result for this backup" belongs. A tick standing for + "a file exists" is what this replaced. */}} Utolsó: {{timeAgoStr .Tier1LastRun}} {{if eq .Tier1LastStatus "ok"}} - {{else if eq .Tier1LastStatus "error"}}{{end}} + {{else if eq .Tier1LastStatus "error"}} + {{else}}—{{end}} {{end}} {{.BackupContents}} diff --git a/controller/internal/web/templates/dashboard.html b/controller/internal/web/templates/dashboard.html index 3a92b1e..debf79c 100644 --- a/controller/internal/web/templates/dashboard.html +++ b/controller/internal/web/templates/dashboard.html @@ -53,6 +53,21 @@ {{end}}