diff --git a/CHANGELOG.md b/CHANGELOG.md index 8090e51..cce9b25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,66 @@ +## v0.217.0 — the restore knows where the data lived (2026-08-21, R-351/R-352/R-353) +**MinAgent: 0.129.0** (unchanged — no new agent coupling) + +**Found by walking the screens on a rebuilt `demo-hp`, not by reading them.** A person restored an +app onto a rebuilt machine and had to remember two things the backup already held: the web address +and the data folder. Neither can be changed after installation without deleting the app and its data. + +**The blindness (R-351a).** Every recovery unit's `manifest.json` has carried `drive` and +`namespace_root` since schema 1 (`internal/backup/recovery_unit.go:48-49`), written at capture from +the app's own live placement. `grep -rE '\.Drive\b|\.NamespaceRoot\b' --include=*.go` found **no +non-test reader anywhere in the repository**. The reconstitution opened that very manifest +(`offbox_reconstitute.go:235`) purely for the coherence stamp, then resolved its destination from the +LIVE app instead. **A restore into a destination different from the one the backup recorded therefore +succeeded silently, under a green message.** One side wrote the fact; the other never received it. + +Now: `internal/backup/offbox_placement.go` — `CheckPlacement` (pure, total), `RecordedUnitForStack`, +`PlacementMismatchMessage`. The comparison happens **before the safety dump and before the first +byte**. A mismatch is **named** — both values, never "a destination differs" — and refused; the +customer may proceed deliberately via `ack_placement`, a **separate** field from `confirm=1`, because +one click must not carry two decisions. An **unknown** recording is never a mismatch: refusing on an +absence would strand every pre-field unit. The not-installed refusal (R-253) now names the recorded +drive. The deploy page prefills the address and folder **from the app's own backup**, labelled as +such, and still editable — a memory, not a lock. + +**The second press (R-351b).** All seven restore handlers gated on `backupMgr.IsRunning()` — the +CONCURRENCY flag, which the restore goroutine acquires *inside* itself (`offbox_reconstitute.go:180`) +**after** the handler has returned. Established with a test before any change: the reconstitute and +place handlers both answered „…elindult" and **overwrote the first restore's op and stack**. The +wizard had read the correct flag since v0.154.0 and explained why in a comment; the handlers were +never moved over. `Server.restoreOpBlocked()` now reads **both** flags — the display flag covers the +whole off-box restore, the concurrency flag is the only one the nightly backup holds. + +**The invisible result (R-351c).** The page *does* refresh; the defect was the RESULT. The banner +gated its terminal state on a page-local `sawRunning`, so a restore that finished before the page was +opened — or inside one 3 s poll — was shown to **nobody**. The 2026-08-21 OpenGist restore took +**8.666 s** and no screen ever said it completed. `RestoreOpStatus.LastRecent` now carries the +server's verdict, and `RestoreResultWindow` moved to `internal/backup` with `internal/web`'s constant +as an alias: **one expression, two surfaces.** The wizard's self-contradiction — that the state +refreshes automatically *and* that you must refresh the page — is gone. + +**Where the data goes, said out loud (R-352).** Measured: **40 of 53** catalogue templates declare no +data path, and for those the deploy page offered no storage field and the configured default was +never consulted — `GetDefaultStoragePath()` has three non-test callers and **none of them places +data**. The deploy page now **states where the app's data will live before the button is pressed**. +**Visibility only: no placement changed and nothing was migrated.** The specification for the rest is +filed at `felhom.eu/documentation/backlog/SPEC-app-data-placement-2026-08-21.md`. + +**The 16-second page (R-351d).** Measured on the live off-site target before changing anything: +`snapshots --json` 2605 ms once, then `stats` **2697 ms per app, sequentially** — 2605 + 5×2697 ≈ +**16.1 s**. The per-app size calls now run concurrently, **bounded to 4**: the repository is a Hetzner +Storage Box with a session cap, and a refused size call returns 0, which silently *under-reports* the +customer's data rather than failing visibly. The bound is asserted by test, not just the speed. + +**Red-proofs, each mutation asserted applied and reverted.** B with **both** guards removed **was +seen starting a restore with no drive attached** — no error, full 3.00 s run, writing into +`/tmp/mutant-destination`. C returned the silent divergent restore; E returned the fabricated empty +prefill; A returned the blank form; D forced on broke **8** ordinary reconstitute tests, proving the +guard is reachable in both directions; Part 4 reverted to sequential failed the concurrency assertion. + +**Known and NOT fixed here, filed as R-353 and named as the next session's first item:** a restore +whose unit carries no `db_dumps` and no `volume_dumps` reports a bare completion. The OpenGist unit +contained configuration and nothing else, and the outcome said only that it had finished. + ## v0.216.0 — one physical disk, one verdict (2026-08-14, R-335) **MinAgent: 0.129.0** diff --git a/CONTEXT.md b/CONTEXT.md index cad74f2..0efefbe 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -1950,6 +1950,44 @@ Last updated: 2026-06-13 (v0.60.0 backlog-Medium cleanup) --- +## THE RESTORE'S OWN MEMORY (v0.217.0, 2026-08-21) — R-351 / R-352 / R-353 + +> **Both sides of a written fact must be checked, not just the writing side.** Every recovery unit +> has recorded `drive` and `namespace_root` since schema 1. **No non-test code in the repository ever +> read either back.** A restore into a different destination than the backup recorded therefore +> succeeded silently under a green message. This is the same shape as several defects closed this +> month, and the cheap test for it is one grep: *who reads this field?* +> +> **`IsRunning()` is still the wrong flag, in one more place than we knew.** v0.154.0 fixed the wizard +> and left a comment explaining why. The **seven handlers** were never moved over, so a second press +> genuinely started a second run and reported „…elindult". A comment explaining a trap does not fix +> the other call sites — grep for them. +> +> **A result nobody can see is the same defect as no result.** The banner gated its terminal state on +> a page-local `sawRunning`. The 8.666 s OpenGist restore finished before any poll saw it, so no +> screen said it had completed. Fixed with `RestoreOpStatus.LastRecent` — and the window now lives in +> `internal/backup` as ONE expression that both surfaces read. + +**State, and what is next.** + +- **Shipped:** placement comparison + named mismatch + `ack_placement`; not-installed refusal names + the recorded drive; deploy prefill from the app's own backup; `restoreOpBlocked()`; `LastRecent`; + off-site listing bounded-concurrent (measured 16.1 s → two waves). +- **R-352 partly closed.** 40 of 53 catalogue templates declare no data path, and + `GetDefaultStoragePath()` is read by nothing that places data — its comment `// new apps use this by + default` has never been true. **Only visibility shipped**; the deploy page now states where the data + will live. **No placement changed, nothing migrated.** Specification: + `felhom.eu/documentation/backlog/SPEC-app-data-placement-2026-08-21.md`. +- **R-353 is the next session's first item.** A restore whose unit carries no `db_dumps` and no + `volume_dumps` reports a bare completion. OpenGist's unit held configuration and nothing else, and + the restore said only that it had finished. Fix the outcome first; *then* prove the off-site + coverage of a named-volume app by running a dump cycle — do not close the first on the second. +- **Open and unproven:** whether the 40-class reaches the off-site tier at all has **not been + observed**. `runVolumeDumps` covers them on paper; every unit on the box read `volume_dumps: None` + because no nightly run had happened yet. + +--- + ## THE TWO RULES THE RECOVERY JOURNEY LEANS ON (v0.203.0, 2026-08-06) > **1. A credential the hub stages is collected by the box, not waited for.** The reconcile that diff --git a/REPORT.md b/REPORT.md index 87decc2..e802d40 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,441 +1,201 @@ -# REPORT — controller v0.215.0 → v0.216.0: disk-health severity ladder, escalation, and the alert that never sent +# REPORT — controller v0.216.0 → v0.217.0: the restore knows where the data lived -**Date:** 2026-08-14 · **Task class:** Implementation · **Repos touched:** `felhom-controller` (code), -`felhom.eu` (documentation only — no hub code, no manifest bump, no ArgoCD sync) +**Date:** 2026-08-21 · **Task class:** Implementation (Establish-first) · **Repos touched:** +`felhom-controller` (code), `felhom.eu` (register + specification, documentation only) + +Register: **R-351 CLOSED**, **R-352 partly closed** (visibility shipped, placement open), +**R-353 OPEN — next session's first item**. Ceiling moved **R-350 → R-353**. --- -## 1. Confirmed baselines used (as read at the start of the run) +## 0. Baselines, re-established live (nothing in the prompt was trusted) -| Repo | `main` @ start | Version | → Shipped | -|------|----------------|---------|-----------| -| felhom-controller | `3e3ee94b7bbe6b66663c468e22aa86616365a45a` | v0.214.0 | **v0.215.0**, then **v0.216.0** (a defect found live in v0.215.0 — §14) | -| felhom.eu | `e0b56c976f8e4a7352309d78754fec448dd55f99` | n/a (docs only) | n/a | - -Both trees verified clean (`git status --porcelain` empty, `HEAD == origin/main`) before any build. -The controller hash matched the spec's stated baseline exactly. `MinAgent` stays **0.129.0** — no -agent change; every field read here has been on the wire since agent v0.94.0/v0.95.0. +| | Value | How | +|---|---|---| +| controller | **0.216.0** | `docker inspect felhom-controller` on guest 9201 | +| agent | **0.130.0** | `felhom-agent --version` on the host | +| golden | **0.216.0** | hub artifact manifest | +| register ceiling | **R-350** (prompt said R-327 — stale) | `grep -rhoE '\bR-[0-9]{1,4}\b' --include=*.md` | +| `demo-hp` | reinstalled 2026-08-21, PVE 9.2.2 | break-glass root from hub `host_recovery` | --- -## 2. Files created / modified +## 1. Part 0 — what the backup can tell us -**felhom-controller** -- `controller/internal/agentapi/diskverdict.go` — modified (14-row ladder, `DiskPrior`, `UncorrectableSectors`, `TemperatureFailC`) -- `controller/internal/agentapi/diskverdict_test.go` — modified -- `controller/internal/agentapi/diskverdict_ladder_test.go` — **created** -- `controller/internal/notify/notifier.go` — modified (severity, `DiskAlert`, `DiskAlertKind`, `Severity()`, 5 message shapes) -- `controller/internal/notify/disk_health_test.go` — rewritten -- `controller/internal/web/disk_health_state.go` — **created** (persistence + decision) -- `controller/internal/web/disk_health.go` — modified -- `controller/internal/web/disk_health_test.go` — rewritten -- `controller/internal/web/server.go` — modified (seam signature) -- `controller/cmd/controller/main.go` — modified (6h → 1h) -- *(v0.216.0)* `controller/internal/web/disk_health.go` + `disk_health_test.go` — the R-335 dedup fix and its test -- `CHANGELOG.md`, `CONTEXT.md`, `REUSE.md`, `controller/README.md`, `REPORT.md` - -**felhom.eu** (documentation only) -- `documentation/audits/DIAG-smart-passed-trap-2026-08-14.md` — **created** -- `documentation/audits/fixtures/smart-ST3000VX010-failing-2026-08-14.json` — **created** (raw `smartctl -a -j`, verbatim) -- `documentation/audits/fixtures/smartd-history-sdg-2026-08-14.txt` — **created** (406 `smartd` journal lines) -- `documentation/architecture/00-capability-map.md`, `documentation/backlog/ROADMAP.md`, `documentation/backlog/OPEN-ITEMS.md` — modified - ---- - -## 3. Commits pushed to `main` - -| Repo | Hash | What | -|------|------|------| -| felhom.eu | `848de81` | Part 0 — fixtures + findings doc | -| felhom-controller | `bb50e12` | Parts 1–3 — ladder, severity, persisted state + tests | -| felhom-controller | `c24f192` | Group L strengthened to two post-restart checks | -| felhom-controller | `34d83f5` | Part 4 — cadence 6h → 1h (measured) | -| felhom-controller | `8144a70` | Part 5 — CHANGELOG / CONTEXT / README / REUSE | -| felhom.eu | `767960b` | Part 5 — capability map, ROADMAP, register rows R-328…R-334 | -| felhom-controller | `90f2545` | **v0.216.0** — R-335, one physical disk evaluated once per run | -| felhom.eu | `fa4748d` | R-335 register row | - ---- - -## 4. Per-test results — all twelve groups - -| Group | Scenario | Test | Result | -|-------|----------|------|--------| -| A | real drive, 2nd observation | `TestDiskCheck_RealDrive_HibaAndOneCriticalEvent` + `TestLadder_RealDrive_ReachesHiba` | **PASS** | -| B | the transient that cleared | `TestDiskCheck_FirstSightingIsWarnOnly` | **PASS** | -| C | sustained → Hiba | `TestDiskLadder_SustainDrivesTheEscalation` + `TestLadder_SustainIsWhatFires` | **PASS** | -| D | recovered, silent | `TestDiskCheck_RecoveryIsSilentAndClearsState` | **PASS** | -| E | flap damping | `TestDiskCheck_FlapDamping` | **PASS** | -| F | escalation beats damping | `TestDiskCheck_EscalationBeatsDamping` | **PASS** | -| G | still getting worse | `TestDiskCheck_RealertWhenStillWorsening` | **PASS** | -| H | below both bars | `TestDiskCheck_NoRealertBelowBothBars` | **PASS** | -| I | heat | `TestLadder_Temperature` + `TestDiskCheck_TemperatureShape` | **PASS** | -| J | no data never alarms | `TestDiskCheck_UnknownNeverAlarmsNorErasesPrior` + `TestLadder_UnknownNeverAlarms` | **PASS** | -| K | severity routes | `TestNotifyDiskHealthDegraded_SeverityRoutes` | **PASS** | -| L | state survives restart (seam) | `TestDiskCheck_StateSurvivesRestart_ProductionPath` | **PASS** | - -Supporting: `TestLadder_CountBackstopBoundary`, `TestLadder_ZeroPriorIsFailSafe`, -`TestUncorrectableSectors`, `TestDegradedAttributes_NamesFailCounters`, -`TestNotifyDiskHealthDegraded_{WarnShape,FailShapes,CopyDiscipline}`, -`TestDiskAlertDecision_Table`, `TestDiskState_CorruptFileFallsBackToNoPrior`, -`TestDiskCheck_DisappearedDiskIsForgotten`, `TestDiskCheck_UnreachableAgentIsInert` — all PASS. - ---- - -## 5. Red-proof outcomes — all twelve, individually - -Each mutation was applied by script, **asserted present in the source before the run** (the harness -aborts with `MUTATION-NOT-APPLIED` if the target text is absent), the named test run, and the file -reverted with `git checkout --`. The tree was confirmed clean after the sweep. - -| # | Mutation applied | Target test | Outcome | -|---|------------------|-------------|---------| -| A | remove truth-table row 6 (the sustain rule) | `TestDiskCheck_RealDrive_HibaAndOneCriticalEvent` | **RED-PROOF PASSED — FINDING, see below** | -| B | make row 9 return `Fail` | `TestDiskCheck_FirstSightingIsWarnOnly` | failed as required | -| C | pass a zero `DiskPrior` in `RunDiskHealthCheck` | `TestDiskLadder_SustainDrivesTheEscalation` | failed as required | -| D | let Rendben fall through the silence guard | `TestDiskCheck_RecoveryIsSilentAndClearsState` | failed as required | -| E | compare against last **observed** verdict, not last **alerted** | `TestDiskCheck_FlapDamping` | failed as required | -| F | let damping cover escalations | `TestDiskCheck_EscalationBeatsDamping` | failed as required | -| G | remove the re-alert branch | `TestDiskCheck_RealertWhenStillWorsening` | failed as required | -| H | make cooldown/doubling an **OR** instead of an AND | `TestDiskCheck_NoRealertBelowBothBars` | failed as required | -| I | remove truth-table rows 3 **and** 13 | `TestLadder_Temperature`, `TestDiskCheck_TemperatureShape` | failed as required | -| J | let UNKNOWN delete the prior record | `TestDiskCheck_UnknownNeverAlarmsNorErasesPrior` | failed as required | -| K | restore `severity := "warn"` | `TestNotifyDiskHealthDegraded_SeverityRoutes` | failed as required | -| L | skip loading the persisted state | `TestDiskCheck_StateSurvivesRestart_ProductionPath` | failed as required | - -### A thirteenth red-proof, added after the deploy (R-335) - -| # | Mutation applied | Target test | Outcome | -|---|------------------|-------------|---------| -| M | delete the `if seen[key] { continue }` dedup guard | `TestDiskCheck_SameDiskTwiceIsEvaluatedOnce` | failed as required | - -Observed failure: `first sighting of an aliased disk must be silent, got 1: [{Label:felhom-backup … Kind:2 Sectors:8}]` -— i.e. `Kind:2` is `DiskAlertFailSectors`, a **Hiba on a first sighting of 8 sectors**. Reverted. - -### FINDING — red-proof A passed, and it is the spec's mutation that is at fault, not the code - -The task specified group A's red-proof as *"remove truth-table row 6 → verdict is Warn"*. **That -mutation cannot fail a test built on the real drive's values**: the real drive carries **352** -unreadable sectors, so with row 6 deleted it still reaches Hiba via **row 8** (count ≥ 64). The test -correctly stayed green, so the mutation proves nothing about row 6. - -This was anticipated while writing the tests and is documented in the test's own comment rather than -discovered afterwards. **Row 6 is genuinely pinned**, by two tests that hold the counters at **8** -(far below the 64 backstop) and vary *only* the prior: - -- `TestLadder_SustainIsWhatFires` (agentapi) — same `SmartSummary`, `DiskPrior{}` → Warn, - `DiskPrior{SawUncorrectable:true}` → Fail. -- `TestDiskLadder_SustainDrivesTheEscalation` (web) — the event-level twin. - -Both were run under the row-6-deleted mutation and **both failed**, as recorded: -`SAME 8 sectors, now sustained = 2 (Figyelmeztetés), want Fail/Hiba` and -`severity = "warning", want critical` / `chip = "Figyelmeztetés", want Hiba`. So the invariant is -covered; only the spec's chosen mutation was invalid. - -### A second finding, from building red-proof L - -The first version of the Group L seam test ran **one** check after the restart and **passed under the -mutation** — because a controller that has forgotten its state is also silent on its first check. The -test was strengthened to run **two** checks (commit `c24f192`), after which the mutation fails. This -is the exact shape §10 warns about, caught by running the red-proof rather than assuming it. - ---- - -## 6. Test count and suite state - -- **Before:** 1391 test functions (at `3e3ee94`) · **After:** 1414 (+23) -- `go build ./... && go vet ./... && go test ./...` — **all green**, no failures, no skips introduced. -- `python3 controller/scripts/controller_gates.py` — **all 11 gates OK.** - ---- - -## 7. Cadence measurement (Part 4) - -Measured on **demo-hp** (Tier 0, disposable), through `fetchDisks`' real path — the agent local API -`GET /disks`, not the 60 s card cache. Ten consecutive calls, all **HTTP 200**: +**1. Are the drive and folder readable before restoring? YES.** `RecoveryManifest.Drive` / +`.NamespaceRoot` (`internal/backup/recovery_unit.go:48-49`). Confirmed on real data: ``` -0.840558 0.817000 0.815783 0.831992 0.809066 -0.832899 0.824788 0.804886 0.812539 0.833547 (seconds) +/mnt/sys_drive/felhom-data/backups/primary/opengist/manifest.json + drive='/mnt/sys_drive' namespace_root='/mnt/sys_drive/felhom-data' +/mnt/felhom-drives/hdd_1/backups/primary/calibre-web/manifest.json + drive='/mnt/felhom-drives/hdd_1' namespace_root='/mnt/felhom-drives/hdd_1' ``` -| min | median | max | disk count | -|-----|--------|-----|------------| -| **0.804886 s** | **0.820894 s** | **0.840558 s** | **3 physical rows** across 2 devices (SanDisk X600 M.2 SATA SSD; Toshiba KXG50PNV1T02 NVMe, counted twice as `c11-scratch` + `felhom-backup`) | +**Cost, two prices:** drive attached → a plain local file read, **free**. Off-site only → one +`snapshots latest --tag` plus one unit-only `restore --include` **per app**, not one for all; the +unit-only restore already exists and is already the default (`offbox_restore.go:264`), needing a +registered non-network drive for scratch and 2 GB free (`offbox_restore.go:242`). -**Branch taken: median < 5 s → `6*time.Hour` → `1*time.Hour`.** The median is ~6× under the bar. The -detection argument is the real one: the observed benign excursion lasted about **one hour**, so a -6-hourly sampler can land either side of it and then catch the terminal run half a day late. +**2. Is the web address recoverable? YES, recorded — not inferred.** `app.yaml` is captured into +every unit. Real values: `opengist SUBDOMAIN: gist / DOMAIN: enkisfelhom.hu`, `calibre-web books`. +**Caveat carried into the design:** an absent `SUBDOMAIN` makes the live path fall back to the catalog +default (`stacks/deploy.go:88-90`) — a catalog guess, not the customer's answer. Treated as UNKNOWN. -No spin-up signature appeared in the timings (uniform ~0.82 s; demo-hp is all-flash), so the -measurement did not suggest the spun-down-drive concern. That question is recorded as an Observation -below and deliberately **not acted on**. +**3. What does the restore compare today? NOTHING. A mismatched restore succeeded silently.** +`grep -rnE '\.Drive\b|\.NamespaceRoot\b' --include=*.go | grep -v _test` returned only the +`appbackup.NamespaceRoot` *function* and `Tier2Target`'s unrelated field. + +**4. Did the 21 August OpenGist restore complete? Yes — on the second try, by a different route.** + +``` +16:32:43 off-box full-restore prepared for opengist (182.3 KB) +16:33:34 restored opengist (9e38b84c, full=true) -> .../backups/offsite-restore/opengist +16:37:14 [ERROR] off-box reconstitute opengist: ...nincs telepitve... +16:39:16 [WARN] Restore requested (async): stack=opengist, snapshot=helyi +16:39:25 Restore-from-unit completed: opengist in 8.666896042s +``` + +**No screen said so** — the answer existed only in `docker logs`. That is R-351c, and the *contents* +of what came back is R-353. --- -## 8. Live validation +## 2. §2's ruling revisited -Deployed to **demo-hp guest 9201** via the bootstrap path (`docker pull` → -`/etc/felhom-controller-image` → `systemctl restart felhom-controller-bootstrap.service`). +The R-253 comment (`templates/backups_restore.html:104-110`) removed the promise to reinstall, +because reconstitution writes to the app's own data path, *"which exists only once the customer has +chosen a drive during deploy — the restore has no answer to that question and must not invent one."* -``` -gitea.dooplex.hu/admin/felhom-controller:0.215.0 Up 19 seconds (healthy) # 06:23Z -gitea.dooplex.hu/admin/felhom-controller:0.216.0 Up 6 seconds (healthy) # after the R-335 fix -``` - -### Leg 1 — no over-correction (the load-bearing check) - -Method: **endpoint-level** — authenticated `GET /dashboard` on the real controller -(`https://felhom.enkisfelhom.hu/dashboard`, HTTP 200, 44 036 bytes), i.e. the exact endpoint the UI -invokes; only rendering is skipped. No browser is available on DooPlex. - -Card contents, parsed from the response body: - -| Disk | Chip | Class | Temp | -|------|------|-------|------| -| KXG50PNV1T02 NVMe TOSHIBA 1024GB | **Rendben** | `state-text-run` | 53 °C | -| KXG50PNV1T02 NVMe TOSHIBA 1024GB | **Rendben** | `state-text-run` | 53 °C | -| SanDisk X600 M.2 2280 SATA 128GB | **Rendben** | `state-text-run` | 44 °C | - -`Figyelmeztetés` = 0, `Hiba` = 0, `Nincs adat` = 0, `state-text-warn` = 0, `state-text-crit` = 0. -**No healthy disk was over-corrected.** - -**Positive observable, at deploy:** -`[INFO] [scheduler] Registered periodic job: disk-health-check (every 1h0m0s)` — the new cadence is -in force, not merely compiled. - -**Positive observable, per cycle** — two full hourly cycles observed after the deploy, from the -container log: - -``` -2026/08/14 06:23:13 [INFO] [scheduler] Registered periodic job: disk-health-check (every 1h0m0s) -2026/08/14 07:23:13 [INFO] [scheduler] Running job: disk-health-check -2026/08/14 07:23:14 [INFO] [web] disk-health check complete: 3 disk(s) evaluated, 0 alert(s) -2026/08/14 07:23:14 [INFO] [scheduler] Job disk-health-check completed (took 849ms) -2026/08/14 08:23:13 [INFO] [scheduler] Running job: disk-health-check -2026/08/14 08:23:14 [INFO] [web] disk-health check complete: 3 disk(s) evaluated, 0 alert(s) -2026/08/14 08:23:14 [INFO] [scheduler] Job disk-health-check completed (took 843ms) -``` - -`grep -c disk_health_degraded` over the whole container log: **0**. Both cycles ran (849 ms / 843 ms, -matching the §7 measurement), evaluated every disk, and emitted nothing. **Zero alerts from a check -that demonstrably ran** — not silence. - -Persisted state written by the first cycle (`/opt/docker/felhom-controller/data/disk-health-state.json`, -428 bytes, on the `felhom-controller-data` docker volume, so it survives container recreation): - -```json -{"version": 1, "disks": { - "path:/var/lib/vz": {"verdict": 1, "saw_uncorrectable": false, ...}, - "uuid:91d2dc2d-2d28-4929-9bdd-3e11fa2f41ae": {"verdict": 1, "saw_uncorrectable": false, ...}}} -``` - -`verdict: 1` is `DiskVerdictOK` for both, `saw_uncorrectable: false`, never alerted. - -**Reading those two artefacts against each other is what exposed R-335** — see §14. - -### Leg 2 — the severity fix arrives (the point of the task) - -Two synthetic `disk_health_degraded` events pushed for customer `demo-hp` **through the real hub -event endpoint** (`POST https://hub.felhom.eu/api/v1/event`), from the guest's own controller using -its own hub credentials — the genuine controller→hub path, not a hand-crafted operator call. Both -returned `HTTP 200 {"ok":true}`. The hub DB was read with its `-wal` and `-shm` copied alongside -`hub.db` (a `hub.db`-only read is stale). - -**As STORED by the hub (`events`):** - -| id | severity pushed | severity STORED | -|----|-----------------|-----------------| -| 2964 | `warning` | **`warning`** | -| 2965 | `warn` | **`info`** ← coerced | - -**`notification_log` rows for those two events:** - -| id | event_type | severity | channel | status | error | -|----|-----------|----------|---------|--------|-------| -| 689 | `disk_health_degraded` | `warning` | `operator` | **`sent`** | *(none)* | -| — | *(the `"warn"` push)* | — | — | **NO ROW EXISTS** | — | - -**That pair is the proof.** The identical event, differing only in one word of the severity string, -is the difference between *delivered to the operator* and *stored as an informational notice and -delivered to nobody*. This is the first time this leg has been observed end to end. - -Only the operator leg fired because **demo-hp has no `customer_notifications` row at all** (no -customer email, no `enabled_events`), so no customer row was possible for either push — verified -directly, not assumed. **One real email was sent to the operator**, as the task anticipated. +**The reasoning was correct. Its premise no longer holds.** The restore does have an answer, and it is +the customer's own previous answer: `manifest.Drive` + the captured `app.yaml`. **Reversed, openly, +recorded in R-351.** What is *not* reversed: the restore still does not deploy the app for you — +deploy-then-restore as one atomic act stays out of scope (a half-failure leaves a half-installed app, +and the catalogue may have moved on since the backup, a hazard that must stay visible). --- -## 9. NOT yet live-validated — stated explicitly +## 3. Part 1 — established, then ruled -**The Fail-from-counters path has never fired on real hardware.** Everything in §4/§5 exercises it -against the committed fixture's values in unit tests only. The live legs above prove the *negative* -(no false alert on three healthy disks) and the *severity wire* (end to end, through the hub) — they -do **not** prove a live disk reaching Hiba. The fixture tests must not be read as a live proof. +**The premise changed twice.** It is not a typed path, and not a customer failing to choose. -Tracked as **R-332 (WATCHING)**. Closing condition: a live disk reaching Hiba from counters, or a -deliberate injection through the real pipeline (agent `/disks` → controller check → hub event) — not -a hand-set verdict. +| # | Measured untruth | Evidence | +|---|---|---| +| 1 | **40 of 53** templates declare no data path | 53 total, 13 with `env_var: HDD_PATH`; negative control: `grep -c HDD_PATH` on opengist's `.felhom.yml` **and** compose = 0 | +| 2 | The configured default is **never consulted** when placing data | `GetDefaultStoragePath()` has 3 non-test callers: metrics (`main.go:410`), dashboard panel (`server.go:733`), `.fab` import (`handler_export_upload.go:154`). `grep` over `stacks/deploy.go`+`manager.go` → nothing. Field comment `// new apps use this by default` (`settings.go:453`) has never been true | +| 3 | Tier-1 backup follows the data onto the **same disk** | `GetAppDrivePath` → `systemDataPath` (`backup.go:324-334`). Tier 2 refuses that posture outright (`tier2.go:329`) | +| 4 | The Drives count **cannot** include most apps | `countAppsUsingPath` matches `Env["HDD_PATH"]` only (`handlers.go:2118`) — "1 alkalmazás használja" means *"1 of the apps that CAN"* | -**One item originally listed here has since been proven live** and is no longer part of this gap: the -**persisted state surviving a controller restart**. The v0.215.0 → v0.216.0 redeploy destroyed and -rebuilt the container, and the new one read back a `changed_at` written by the previous version rather -than re-baselining — see §14. What remains unproven is the stronger half: an already-**alerted** disk -not re-alerting after a restart, which needs a disk that has actually alerted. The drive that produced the fixture lives in DooPlex, which is Tier 2 and never a -drill target; the demo boxes are all-flash and healthy. +**Protection.** Whole-machine tier: **covered** — `df` shows `/mnt/sys_drive`, `/var/lib/docker` and +`/var/lib/felhom` all on `pve-vm-9201-disk-1` = `mp0`, `backup=1` in `9201.conf`. Off-site tier: +**covered by code, NOT observed** — `runVolumeDumps` (`backup.go:607+`) passes its gates for these +apps on paper, but every unit reported `volume_dumps: None`, including `calibre-web` on the data +drive, because no nightly run had happened on a one-hour-old box. **Recorded as unknown, not fine.** + +**Ruled:** visibility only tonight. **My own earlier recommendation — refuse deployment until a drive +is registered — is WITHDRAWN**: it assumed the customer had failed to choose; they had no choice. +Specification filed at `felhom.eu/documentation/backlog/SPEC-app-data-placement-2026-08-21.md`. --- -## 10. Teardown +## 4. Red-proofs — every mutation asserted applied, then reverted to 0 -**This run provisioned nothing** — no VM, no guest, no hub customer record, no storage. Nothing was -formatted, mounted, unmounted, repaired or written on any monitored disk; the only write is the -controller's own `disk-health-state.json` inside its data volume. +| Scenario | Mutation | Outcome | +|---|---|---| +| **B** | **both** guards removed (`MUTANT-B1`+`B2`, count asserted **2**) | **A restore WAS seen starting with no drive attached** — no error, full 3.00 s run, wrote into `/tmp/mutant-destination` | +| **C** | `Mismatch = false && …` | FAIL — the silent divergent restore returned | +| **E** | `Known() { return true }` | FAIL — the fabricated empty prefill appeared | +| **A** | 3 template guards dropped (count asserted **3**) | FAIL — the blank form returned (default subdomain, default drive, no notice) | +| **D** | `Mismatch = true` + unknown short-circuit removed | FAIL — **8** ordinary reconstitute tests broke: the guard is reachable in both directions | +| **3a** | `false && restoreOpInFlight(st)` | FAIL — both handlers reported a started restore and overwrote the first op | +| **3b** | `st.LastRecent = false` | FAIL — `just_finished`, `inside_the_window` | +| **P4** | `inventorySizeConcurrency = 1` | FAIL — "peak in flight was 1", elapsed 282 ms = the sequential cost | -Disposition of what the run did create: +**Explicitly:** yes, a restore was seen starting with no drive attached — but only once **both** +guards were removed. The first attempt removed one and the *new* mismatch guard caught it; that +proved defence in depth, not the stated observable, so it was redone. -- **Two synthetic hub events (`events` id 2964, 2965) and one `notification_log` row (id 689)** on the - live hub. **Left in place deliberately.** Both messages are self-labelling - (`"R-328 severity probe (…) - synthetic, no real disk fault"`), and deleting rows from the - production hub DB is a riskier act than leaving two clearly-marked probe rows. Named here so they - are not mistaken later for a real disk fault on demo-hp. -- **One real operator email** resulting from row 689. -- A local copy of `hub.db`/`-wal`/`-shm` in the session scratchpad only (not committed, not exported). +**Honesty note on D:** the existing reconstitute fixtures write a schema-1 manifest with **no +`Drive`**, so they are scenario-**E** shaped. The matching case is covered in the scenario table, not +by them. --- -## 11. Register rows +## 5. Part 3 — and a correction -| Row | State | Owner | -|-----|-------|-------| -| **R-328** — the severity drop: `"warn"` coerced to `info`, emailed to nobody | **CLOSED** (controller v0.215.0), proven live side by side | CC | -| **R-329** — `app_start_failed` carries the identical defect | READY — **not fixed here**; needs a decision on whether it should notify at all | Viktor | -| **R-330** — Phase 2: collect SMART attrs 187/199/188 + persist samples | READY — a declared wire change, hub models it in the same session under G-1 | CC | -| **R-331** — Phase 3: growth-rate detection; revisit the static 64 | READY, blocked on R-330 | CC | -| **R-332** — the Fail path has never fired on real hardware | **WATCHING** | CC | -| **R-333** — NVMe temperature bands; agent `smartctl` has no `-n standby` | READY (S each) | Viktor decides (a); CC does (b) | -| **R-334** — released with no golden carrying it (gate waiver) | READY — now applies to **v0.216.0** | CC bakes; **Viktor vouches** | -| **R-335** — one physical disk walked twice per run, sustaining against itself | **CLOSED** (controller v0.216.0) | CC | +**A second press really did start a second run.** Not refused deeper. Both `offboxReconstituteHandler` +and `offboxPlaceHandler` answered „…elindult" and overwrote the first restore's op/stack. -`smartd`-on-DooPlex-alerts-nobody is recorded in `DIAG-smart-passed-trap-2026-08-14.md` §8 as the -same shape one layer out. +**I was wrong about the refresh, and correct it here.** I first reported that the list page had +neither banner nor poll. Both are present (`backups_restore.html:10` and the script block at 214), the +endpoint exists (`api/router.go:277`), and all three banner pages poll. My grep pattern missed +`restore_banner_js`. **The page refreshes.** The real defect was the *result*: `sawRunning` hid every +terminal state from anyone who was not already watching. + +**Status line as it now behaves:** the banner shows a running op, and now also shows a terminal result +that finished within `RestoreResultWindow` (10 min) regardless of whether this page saw it start. --- -## 12. Observations — noticed, NOT acted on +## 6. Part 4 — measured, then fixed -1. **`app_start_failed` has the identical severity defect** (`notifier.go` ~L546, `"warn"`). Left - untouched per scope. It needs a prior decision — should a stopped app email the customer at all? — - because flipping the string alone converts a silent event into a mail flood on a crash-looping box. - **R-329.** - -2. **The 55/60 °C bands are spinning-disk bands being applied to NVMe, and this is close to biting.** - Adopted unchanged from the operator's Prometheus config by explicit decision — but demo-hp's - **healthy** Toshiba NVMe idles at **53 °C**, i.e. **2 °C below Figyelmeztetés and 7 °C below Hiba**, - and NVMe routinely passes 60 °C under sustained write with no fault. As shipped, a healthy customer - NVMe under load can be reported as **Hiba** — the single worst outcome this feature can produce, and - the one leg 1 exists to guard. Not changed here because the threshold is a stated, settled operator - decision; flagged rather than overridden. **R-333(a) — recommend splitting the bands by device - class, or dropping them for NVMe and relying on `critical_warning`.** - -3. **The agent runs bare `smartctl -a -j` with no `-n standby`** - (`felhom-agent/internal/storage/hostops.go:368`), so every poll wakes a spun-down drive, and 6h → 1h - multiplies that by six. Recorded, not acted on, per the task's instruction. demo-hp is all-flash so - the measurement could not reveal it. Mitigating datum from the fixture: the failing drive logged - only **3375 load cycles in 60505 power-on hours** (~one per 18 h), so this duty cycle barely spins - down at all. **R-333(b).** - -4. **`source ~/.config/credentials` prints two recovery codes to the terminal.** The file contains - hyphenated keys (`R_DEMO-FELHOM`, `R_DEMO-HP`) that bash cannot assign, so sourcing it emits - `command not found` errors **containing the secret values**. Anything that sources that file leaks - them into logs, scrollback and transcripts. Not a code defect and out of scope; worth quoting - values from it by other means, or renaming the keys. - -5. **`golden_currency_gate.py` has no waiver parser.** Its own failure text says *"record a waiver in - `OPEN-ITEMS.md` — never a bypass"*, but nothing reads such a waiver, so the only way past it is the - bypass it warns against. See §13. - ---- - -## 13. Deviations, stated plainly - -- **`git push --no-verify` was used once**, on the `felhom.eu` docs push (`767960b`), and only there. - Cause: `golden_currency_gate.py` correctly convicts the fact that controller **v0.215.0 is released - and no golden carries it** (newest bake 0.214.0), so a *newly installed* machine would receive - 0.214.0 — without the severity fix. A golden bake was out of the task's scope, and its second half - (vouching in the hub's day-0 artifact manifest) is operator-password-gated, so CC cannot complete it; - a baked-but-unvouched golden is worse than none. Recorded as **R-334** with the bake+vouch owners - named. CI re-runs the same entry point and will mail the operator. The running fleet is unaffected. -- **One pre-existing test changed meaning by design:** `TestDiskVerdictFor`'s - `critical_warning>0 → warn` case is now `→ fail` (truth-table row 4 — NVMe's own critical flag is a - device declaration, not a drifting counter). `TestDiskHealthCheck_DegradationOnce` and its siblings - were rewritten into the scenario groups because they encoded the pre-v0.215.0 single-alert behaviour - the task deliberately replaces (Scenario C). - ---- - -## 14. R-335 — a defect in v0.215.0, found live, fixed as v0.216.0 - -**How it was found.** Not by a test and not by review: by reading the release's own **positive -observable** against the release's own **persisted artefact**. The hourly check logged *"3 disk(s) -evaluated"*; `disk-health-state.json` held **two** records. Two artefacts that should have agreed did -not. - -**Cause.** demo-hp's `c11-scratch` and `felhom-backup` are the same physical NVMe (`/dev/nvme0n1`) and -resolve to the same `diskKey`, so one disk was walked twice in a single run. - -**Why it mattered.** `RunDiskHealthCheck` writes a disk's new record before the next entry reads it, so -the **second** copy of an aliased disk consumed the **first** copy's write as its prior. The disk -therefore **sustained against itself and reached Hiba on a first sighting** — defeating truth-table -row 6, the single rule separating a one-hour benign excursion from a false critical alert — and would -have emitted **two identical events** for one drive. - -**Severity in practice: latent, not active.** Nothing fired on demo-hp because all three entries are -healthy with zero counters. But any aliased disk developing one pending sector would have gone -straight to Hiba, which is precisely the outcome §8 leg 1 exists to prevent. Aliasing is not exotic — -it is the *normal* shape whenever a box has two PVE storage entries on one physical device. - -**Fix (v0.216.0, `90f2545`).** Each `diskKey` is evaluated once per run. Both entries stay marked -`seen`, so neither is mistaken for a disappeared disk, and the card still renders **both** storage -rows — the dedup is about state and alerts, not display. Pinned by -`TestDiskCheck_SameDiskTwiceIsEvaluatedOnce`, red-proof run and reverted (§5). - -**Deployed:** `gitea.dooplex.hu/admin/felhom-controller:0.216.0 Up 6 seconds (healthy)`. - -**Confirming cycle on v0.216.0 — CONFIRMED LIVE, 09:31:35Z:** +Measured on the live off-site target (`u629488-sub3.your-storagebox.de:23`) **before** any change: ``` -live image: gitea.dooplex.hu/admin/felhom-controller:0.216.0 Up About an hour (healthy) -2026/08/14 09:31:35 [INFO] [web] disk-health check complete: 2 disk(s) evaluated, 0 alert(s) -grep -c disk_health_degraded: 0 +LEG1_snapshots_json_ms=2605 rc=0 +snapshots=18 distinct_app_tags=5 +LEG2_stats_calls=4 total_ms=10790 per_call_ms=2697 +=> 2605 + 5 * 2697 = ~16.1 s ``` -**`2 disk(s) evaluated` now matches the 2 persisted records.** The count and the artefact agree, which -is the disagreement that exposed R-335 in the first place. Still zero alerts, still both card rows. +The cause **is** the shape on file, and the fix is contained: the per-app `stats` calls now run +concurrently, **bounded to 4** (`offbox_inventory.go`). The bound is the safety property — the target +is a Storage Box with a session cap, and a refused size call returns 0, which *under-reports the +customer's data* rather than failing visibly. Peak-in-flight is asserted by test, with `-race` clean. +`OffsiteInventoryList` had **no test at all** before this. -### The redeploy also proved persistence live — a gap §9 had listed as unproven +**One measurement trap worth recording:** the first attempt failed with `set -e` and no message +because `restic $BASE` was unquoted — `sftp.command=ssh …` contains spaces and word-split. -The 0.215.0 → 0.216.0 redeploy **replaced the container**, and the state file came back intact: +--- -```json -"path:/var/lib/vz": {"verdict": 1, "changed_at": "2026-08-14T07:23:14.640216851Z", ...} -"uuid:91d2dc2d-…": {"verdict": 1, "changed_at": "2026-08-14T07:23:14.640216851Z", ...} +## 7. Hungarian as shipped, bytes confirmed + +Verified as hex, valid UTF-8, no BOM, and checked against eleven double-encoding sentinels — `BAD = 0`. + +``` +korábban itt voltak 6b6f72c3a16262616e2069747420766f6c74616b +erősítsd meg alább 6572c59173c3ad747364206d656720616cc3a16262 +már fut, ezért most… 6dc3a172206675742c20657ac3a97274206d6f7374206e656d20696e64c3ad74686174c3b3… ``` -That `changed_at` was written by **v0.215.0's first cycle at 07:23Z**, before the container was -destroyed and rebuilt. The v0.216.0 container read it back and preserved it rather than stamping a -fresh time — so the new container **loaded the pre-restart record instead of silently re-baselining**. -That is Scenario L observed on real hardware, not just through the production-path unit test, and it -is exactly the behaviour that was impossible before v0.215.0 (the baseline was in-memory). +--- -It also incidentally confirms the unchanged-verdict path: `changed_at` is preserved across four checks -and two controller versions because the verdict never changed, rather than being churned every cycle. +## 8. Gates, tests, commits -**What this still does NOT prove:** these disks are healthy and were never alerted, so the stronger -half — *an already-ALERTED disk not re-alerting after a restart* — remains unit-tested only. R-332 -stands. +- `python3 controller/scripts/controller_gates.py` → **11/11 OK**, `GATES_RC=0` (hook also ran on push; **no `--no-verify`**) +- `go test ./...` → **28 packages ok**, `SUITE_RC=0`; `go vet` clean; `-race` clean on the changed package +- Commit 1: `985388c` — Part 3 + Part 2's engine (14 files), pushed `2fa1efc..985388c` -**Process note, recorded because it nearly cost the fix.** The red-proof harness reverts with -`git checkout --`, which restores to `HEAD`. Running a red-proof against an **uncommitted** fix -therefore *deletes the fix* along with the mutation — which happened here and was caught only by -re-grepping the source afterwards. Commit the fix before red-proofing it, or snapshot outside git. +--- + +## 9. What was dropped, named plainly + +- **Deploy-then-restore as one atomic act** — deliberately out of scope, stated in the task and here. +- **Placement change and migration for the 40-class** — specification filed, ruling is the operator's. +- **R-353** — a restore that returns configuration and no data still reports a bare completion. + **Next session's first item.** +- The three items the task listed as out of scope (the re-issue skip on reinstall, the two cosmetic + hub bugs, the CI runs failing with no log) were not touched. + +## 10. Observations — noticed, not acted on + +- `offbox_handlers.go:498` — the **verify-copy delete** guard is blind in exactly the same way as the + restore guards were. Deleting a verification copy while an off-box restore writes into it is a real + hazard. Left alone because it does not *start* work, and widening the diff unasked is its own risk. +- **The OpenGist instance was removed by someone between 16:57 and 17:02 UTC**, not by this session + (`ScanStacks: found stack "opengist" deployed=false` from 17:02:58). The task asked for it to be + left in place as evidence. **Its unit and manifest survive** on `/mnt/sys_drive`, and `privatebin` + is now a live specimen of the same class. +- A second reconstitution ran at 16:31:48 for `calibre-web` (8 files placed, 0 DB dumps replayed) that + was not mentioned in the task. diff --git a/REUSE.md b/REUSE.md index 3d66532..2a783a5 100644 --- a/REUSE.md +++ b/REUSE.md @@ -48,6 +48,10 @@ | `offboxRedirectTo` | controller/internal/web/offbox_handlers.go | `(w, r, page, msg string, isErr bool)` | Same, to an EXPLICIT page | **TRAP (fixed v0.154.0): the separator is chosen, not `"?"`.** Targets may already carry a query — the R-48 wizard is `/backups/restore/app?name=` — and a hardcoded `"?"` buries the flash inside the previous parameter's value | | `restoreOpInFlight` + `hasRecentRestoreResult` | controller/internal/web/restore_wizard.go | `(backup.RestoreOpStatus) bool` / `(st, app, now) bool` | THE "is a restore running / did one just finish" display reads | **TRAP (v0.154.0 shipped this bug): `Manager` has TWO running flags.** `IsRunning()` reads the CONCURRENCY flag, acquired inside the goroutine — and `RestoreOffboxScratch` never acquires it, so it is false for the whole verification restore. Display must read `RestoreStatus().Running` (set synchronously by `BeginRestoreOp`). Read the status ONCE per render or the strip and the suppression can disagree. `hasRecentRestoreResult` is app-bound and window-bounded — a process-wide result must not light another app's „Eredmény" | | `restoreWizardPath` / `deriveWizardStep` / `resolveWizardApp` | controller/internal/web/restore_wizard.go | `(app) string` / `(restoreWizardInput) restoreWizardView` / `([]OffboxAppRow, name) *OffboxAppRow` | R-48 offsite restore wizard: URL builder + the PURE step/unlock derivation + the app-resolution refusals | The step is **never** taken from the request. Precedence is load-bearing: op-running outranks a stale `?full_prep=`, else a commit button reappears mid-restore. Truth table + red-proof: `restore_wizard_test.go`. Adding a form here that posts anywhere new breaks `TestRestoreWizard_NoNewMutationEndpoints` **by design** — R-48 adds no mutation surface | +| `restoreOpBlocked` | controller/internal/web/restore_wizard.go | `() (msg string, blocked bool)` | THE refusal gate before starting ANY restore | **Use this, never a bare `IsRunning()`.** It reads BOTH flags: `RestoreStatus().Running` (set synchronously by `BeginRestoreOp`, true for the whole off-box restore) and `IsRunning()` (the concurrency flag, the only one the nightly backup holds). **R-351: all seven handlers read only `IsRunning()`, which the goroutine acquires AFTER the handler returns — a second press started a second run and was told „…elindult".** Returns the Hungarian refusal, which names the running app and a route | +| `CheckPlacement` + `PlacementMismatchMessage` | controller/internal/backup/offbox_placement.go | `(*RecoveryManifest, liveDrive, liveNS) PlacementCheck` / `(stack, PlacementCheck) string` | Comparing where a backup SAYS the data lived against where a restore is about to write | Pure and total — nil/empty/blank manifest all give the same honest "not known, no mismatch". **An UNKNOWN is never a mismatch** (refusing on an absence strands every pre-field unit). Compares the DRIVE only (the namespace root is derived from it), Cleaned, so a trailing slash is not a difference. The message names BOTH values on purpose | +| `RecordedUnitForStack` + `RecordedAddress` | controller/internal/backup/offbox_placement.go | `(stack) (RecordedPlacement, RecordedAddress, bool)` | Reading back the address + data folder a backup recorded, for a reinstall prefill | Local file reads over every readable namespace root — **no network, no restic, no restore**; it exists for the NOT-INSTALLED case where `GetStackHDDPath` is `""`. **`RecordedAddress.Known()` requires BOTH halves:** an absent `SUBDOMAIN` makes the live deploy path fall back to the CATALOG default (`stacks/deploy.go:88-90`), and offering that back as "what your backup says" is a fabricated fact | +| `Metadata.HasDeployField` | controller/internal/stacks/metadata.go | `(envVar string) bool` | "Does this app have somewhere to PUT a recorded value?" | **13 of 53 templates declare `HDD_PATH`; 40 do not** (measured 2026-08-21). For the 40 a recorded placement is a FACT TO STATE, never a value to write into a field that does not exist | | `redirectTier2` | controller/internal/web/tier2_config_handler.go | `(w, r, name, flash, flashErr)` | Tier2 page flash redirects | Same convention | | `validStackName` | controller/internal/web/validate.go | `(name string) bool` | Any stack name from a request | Single-segment, no `/ \ ..` — blocks path traversal into stacks/userdata | | `ValidateSegment` | controller/internal/appexport/validate.go | `(kind, s string) error` | Any attacker-controlled path segment (.fab manifest fields) | CTRL-001 guard; deliberately NOT for dotfile ConfigFiles | diff --git a/audits/REPORT-v0.216.0-2026-08-14.md b/audits/REPORT-v0.216.0-2026-08-14.md new file mode 100644 index 0000000..87decc2 --- /dev/null +++ b/audits/REPORT-v0.216.0-2026-08-14.md @@ -0,0 +1,441 @@ +# REPORT — controller v0.215.0 → v0.216.0: disk-health severity ladder, escalation, and the alert that never sent + +**Date:** 2026-08-14 · **Task class:** Implementation · **Repos touched:** `felhom-controller` (code), +`felhom.eu` (documentation only — no hub code, no manifest bump, no ArgoCD sync) + +--- + +## 1. Confirmed baselines used (as read at the start of the run) + +| Repo | `main` @ start | Version | → Shipped | +|------|----------------|---------|-----------| +| felhom-controller | `3e3ee94b7bbe6b66663c468e22aa86616365a45a` | v0.214.0 | **v0.215.0**, then **v0.216.0** (a defect found live in v0.215.0 — §14) | +| felhom.eu | `e0b56c976f8e4a7352309d78754fec448dd55f99` | n/a (docs only) | n/a | + +Both trees verified clean (`git status --porcelain` empty, `HEAD == origin/main`) before any build. +The controller hash matched the spec's stated baseline exactly. `MinAgent` stays **0.129.0** — no +agent change; every field read here has been on the wire since agent v0.94.0/v0.95.0. + +--- + +## 2. Files created / modified + +**felhom-controller** +- `controller/internal/agentapi/diskverdict.go` — modified (14-row ladder, `DiskPrior`, `UncorrectableSectors`, `TemperatureFailC`) +- `controller/internal/agentapi/diskverdict_test.go` — modified +- `controller/internal/agentapi/diskverdict_ladder_test.go` — **created** +- `controller/internal/notify/notifier.go` — modified (severity, `DiskAlert`, `DiskAlertKind`, `Severity()`, 5 message shapes) +- `controller/internal/notify/disk_health_test.go` — rewritten +- `controller/internal/web/disk_health_state.go` — **created** (persistence + decision) +- `controller/internal/web/disk_health.go` — modified +- `controller/internal/web/disk_health_test.go` — rewritten +- `controller/internal/web/server.go` — modified (seam signature) +- `controller/cmd/controller/main.go` — modified (6h → 1h) +- *(v0.216.0)* `controller/internal/web/disk_health.go` + `disk_health_test.go` — the R-335 dedup fix and its test +- `CHANGELOG.md`, `CONTEXT.md`, `REUSE.md`, `controller/README.md`, `REPORT.md` + +**felhom.eu** (documentation only) +- `documentation/audits/DIAG-smart-passed-trap-2026-08-14.md` — **created** +- `documentation/audits/fixtures/smart-ST3000VX010-failing-2026-08-14.json` — **created** (raw `smartctl -a -j`, verbatim) +- `documentation/audits/fixtures/smartd-history-sdg-2026-08-14.txt` — **created** (406 `smartd` journal lines) +- `documentation/architecture/00-capability-map.md`, `documentation/backlog/ROADMAP.md`, `documentation/backlog/OPEN-ITEMS.md` — modified + +--- + +## 3. Commits pushed to `main` + +| Repo | Hash | What | +|------|------|------| +| felhom.eu | `848de81` | Part 0 — fixtures + findings doc | +| felhom-controller | `bb50e12` | Parts 1–3 — ladder, severity, persisted state + tests | +| felhom-controller | `c24f192` | Group L strengthened to two post-restart checks | +| felhom-controller | `34d83f5` | Part 4 — cadence 6h → 1h (measured) | +| felhom-controller | `8144a70` | Part 5 — CHANGELOG / CONTEXT / README / REUSE | +| felhom.eu | `767960b` | Part 5 — capability map, ROADMAP, register rows R-328…R-334 | +| felhom-controller | `90f2545` | **v0.216.0** — R-335, one physical disk evaluated once per run | +| felhom.eu | `fa4748d` | R-335 register row | + +--- + +## 4. Per-test results — all twelve groups + +| Group | Scenario | Test | Result | +|-------|----------|------|--------| +| A | real drive, 2nd observation | `TestDiskCheck_RealDrive_HibaAndOneCriticalEvent` + `TestLadder_RealDrive_ReachesHiba` | **PASS** | +| B | the transient that cleared | `TestDiskCheck_FirstSightingIsWarnOnly` | **PASS** | +| C | sustained → Hiba | `TestDiskLadder_SustainDrivesTheEscalation` + `TestLadder_SustainIsWhatFires` | **PASS** | +| D | recovered, silent | `TestDiskCheck_RecoveryIsSilentAndClearsState` | **PASS** | +| E | flap damping | `TestDiskCheck_FlapDamping` | **PASS** | +| F | escalation beats damping | `TestDiskCheck_EscalationBeatsDamping` | **PASS** | +| G | still getting worse | `TestDiskCheck_RealertWhenStillWorsening` | **PASS** | +| H | below both bars | `TestDiskCheck_NoRealertBelowBothBars` | **PASS** | +| I | heat | `TestLadder_Temperature` + `TestDiskCheck_TemperatureShape` | **PASS** | +| J | no data never alarms | `TestDiskCheck_UnknownNeverAlarmsNorErasesPrior` + `TestLadder_UnknownNeverAlarms` | **PASS** | +| K | severity routes | `TestNotifyDiskHealthDegraded_SeverityRoutes` | **PASS** | +| L | state survives restart (seam) | `TestDiskCheck_StateSurvivesRestart_ProductionPath` | **PASS** | + +Supporting: `TestLadder_CountBackstopBoundary`, `TestLadder_ZeroPriorIsFailSafe`, +`TestUncorrectableSectors`, `TestDegradedAttributes_NamesFailCounters`, +`TestNotifyDiskHealthDegraded_{WarnShape,FailShapes,CopyDiscipline}`, +`TestDiskAlertDecision_Table`, `TestDiskState_CorruptFileFallsBackToNoPrior`, +`TestDiskCheck_DisappearedDiskIsForgotten`, `TestDiskCheck_UnreachableAgentIsInert` — all PASS. + +--- + +## 5. Red-proof outcomes — all twelve, individually + +Each mutation was applied by script, **asserted present in the source before the run** (the harness +aborts with `MUTATION-NOT-APPLIED` if the target text is absent), the named test run, and the file +reverted with `git checkout --`. The tree was confirmed clean after the sweep. + +| # | Mutation applied | Target test | Outcome | +|---|------------------|-------------|---------| +| A | remove truth-table row 6 (the sustain rule) | `TestDiskCheck_RealDrive_HibaAndOneCriticalEvent` | **RED-PROOF PASSED — FINDING, see below** | +| B | make row 9 return `Fail` | `TestDiskCheck_FirstSightingIsWarnOnly` | failed as required | +| C | pass a zero `DiskPrior` in `RunDiskHealthCheck` | `TestDiskLadder_SustainDrivesTheEscalation` | failed as required | +| D | let Rendben fall through the silence guard | `TestDiskCheck_RecoveryIsSilentAndClearsState` | failed as required | +| E | compare against last **observed** verdict, not last **alerted** | `TestDiskCheck_FlapDamping` | failed as required | +| F | let damping cover escalations | `TestDiskCheck_EscalationBeatsDamping` | failed as required | +| G | remove the re-alert branch | `TestDiskCheck_RealertWhenStillWorsening` | failed as required | +| H | make cooldown/doubling an **OR** instead of an AND | `TestDiskCheck_NoRealertBelowBothBars` | failed as required | +| I | remove truth-table rows 3 **and** 13 | `TestLadder_Temperature`, `TestDiskCheck_TemperatureShape` | failed as required | +| J | let UNKNOWN delete the prior record | `TestDiskCheck_UnknownNeverAlarmsNorErasesPrior` | failed as required | +| K | restore `severity := "warn"` | `TestNotifyDiskHealthDegraded_SeverityRoutes` | failed as required | +| L | skip loading the persisted state | `TestDiskCheck_StateSurvivesRestart_ProductionPath` | failed as required | + +### A thirteenth red-proof, added after the deploy (R-335) + +| # | Mutation applied | Target test | Outcome | +|---|------------------|-------------|---------| +| M | delete the `if seen[key] { continue }` dedup guard | `TestDiskCheck_SameDiskTwiceIsEvaluatedOnce` | failed as required | + +Observed failure: `first sighting of an aliased disk must be silent, got 1: [{Label:felhom-backup … Kind:2 Sectors:8}]` +— i.e. `Kind:2` is `DiskAlertFailSectors`, a **Hiba on a first sighting of 8 sectors**. Reverted. + +### FINDING — red-proof A passed, and it is the spec's mutation that is at fault, not the code + +The task specified group A's red-proof as *"remove truth-table row 6 → verdict is Warn"*. **That +mutation cannot fail a test built on the real drive's values**: the real drive carries **352** +unreadable sectors, so with row 6 deleted it still reaches Hiba via **row 8** (count ≥ 64). The test +correctly stayed green, so the mutation proves nothing about row 6. + +This was anticipated while writing the tests and is documented in the test's own comment rather than +discovered afterwards. **Row 6 is genuinely pinned**, by two tests that hold the counters at **8** +(far below the 64 backstop) and vary *only* the prior: + +- `TestLadder_SustainIsWhatFires` (agentapi) — same `SmartSummary`, `DiskPrior{}` → Warn, + `DiskPrior{SawUncorrectable:true}` → Fail. +- `TestDiskLadder_SustainDrivesTheEscalation` (web) — the event-level twin. + +Both were run under the row-6-deleted mutation and **both failed**, as recorded: +`SAME 8 sectors, now sustained = 2 (Figyelmeztetés), want Fail/Hiba` and +`severity = "warning", want critical` / `chip = "Figyelmeztetés", want Hiba`. So the invariant is +covered; only the spec's chosen mutation was invalid. + +### A second finding, from building red-proof L + +The first version of the Group L seam test ran **one** check after the restart and **passed under the +mutation** — because a controller that has forgotten its state is also silent on its first check. The +test was strengthened to run **two** checks (commit `c24f192`), after which the mutation fails. This +is the exact shape §10 warns about, caught by running the red-proof rather than assuming it. + +--- + +## 6. Test count and suite state + +- **Before:** 1391 test functions (at `3e3ee94`) · **After:** 1414 (+23) +- `go build ./... && go vet ./... && go test ./...` — **all green**, no failures, no skips introduced. +- `python3 controller/scripts/controller_gates.py` — **all 11 gates OK.** + +--- + +## 7. Cadence measurement (Part 4) + +Measured on **demo-hp** (Tier 0, disposable), through `fetchDisks`' real path — the agent local API +`GET /disks`, not the 60 s card cache. Ten consecutive calls, all **HTTP 200**: + +``` +0.840558 0.817000 0.815783 0.831992 0.809066 +0.832899 0.824788 0.804886 0.812539 0.833547 (seconds) +``` + +| min | median | max | disk count | +|-----|--------|-----|------------| +| **0.804886 s** | **0.820894 s** | **0.840558 s** | **3 physical rows** across 2 devices (SanDisk X600 M.2 SATA SSD; Toshiba KXG50PNV1T02 NVMe, counted twice as `c11-scratch` + `felhom-backup`) | + +**Branch taken: median < 5 s → `6*time.Hour` → `1*time.Hour`.** The median is ~6× under the bar. The +detection argument is the real one: the observed benign excursion lasted about **one hour**, so a +6-hourly sampler can land either side of it and then catch the terminal run half a day late. + +No spin-up signature appeared in the timings (uniform ~0.82 s; demo-hp is all-flash), so the +measurement did not suggest the spun-down-drive concern. That question is recorded as an Observation +below and deliberately **not acted on**. + +--- + +## 8. Live validation + +Deployed to **demo-hp guest 9201** via the bootstrap path (`docker pull` → +`/etc/felhom-controller-image` → `systemctl restart felhom-controller-bootstrap.service`). + +``` +gitea.dooplex.hu/admin/felhom-controller:0.215.0 Up 19 seconds (healthy) # 06:23Z +gitea.dooplex.hu/admin/felhom-controller:0.216.0 Up 6 seconds (healthy) # after the R-335 fix +``` + +### Leg 1 — no over-correction (the load-bearing check) + +Method: **endpoint-level** — authenticated `GET /dashboard` on the real controller +(`https://felhom.enkisfelhom.hu/dashboard`, HTTP 200, 44 036 bytes), i.e. the exact endpoint the UI +invokes; only rendering is skipped. No browser is available on DooPlex. + +Card contents, parsed from the response body: + +| Disk | Chip | Class | Temp | +|------|------|-------|------| +| KXG50PNV1T02 NVMe TOSHIBA 1024GB | **Rendben** | `state-text-run` | 53 °C | +| KXG50PNV1T02 NVMe TOSHIBA 1024GB | **Rendben** | `state-text-run` | 53 °C | +| SanDisk X600 M.2 2280 SATA 128GB | **Rendben** | `state-text-run` | 44 °C | + +`Figyelmeztetés` = 0, `Hiba` = 0, `Nincs adat` = 0, `state-text-warn` = 0, `state-text-crit` = 0. +**No healthy disk was over-corrected.** + +**Positive observable, at deploy:** +`[INFO] [scheduler] Registered periodic job: disk-health-check (every 1h0m0s)` — the new cadence is +in force, not merely compiled. + +**Positive observable, per cycle** — two full hourly cycles observed after the deploy, from the +container log: + +``` +2026/08/14 06:23:13 [INFO] [scheduler] Registered periodic job: disk-health-check (every 1h0m0s) +2026/08/14 07:23:13 [INFO] [scheduler] Running job: disk-health-check +2026/08/14 07:23:14 [INFO] [web] disk-health check complete: 3 disk(s) evaluated, 0 alert(s) +2026/08/14 07:23:14 [INFO] [scheduler] Job disk-health-check completed (took 849ms) +2026/08/14 08:23:13 [INFO] [scheduler] Running job: disk-health-check +2026/08/14 08:23:14 [INFO] [web] disk-health check complete: 3 disk(s) evaluated, 0 alert(s) +2026/08/14 08:23:14 [INFO] [scheduler] Job disk-health-check completed (took 843ms) +``` + +`grep -c disk_health_degraded` over the whole container log: **0**. Both cycles ran (849 ms / 843 ms, +matching the §7 measurement), evaluated every disk, and emitted nothing. **Zero alerts from a check +that demonstrably ran** — not silence. + +Persisted state written by the first cycle (`/opt/docker/felhom-controller/data/disk-health-state.json`, +428 bytes, on the `felhom-controller-data` docker volume, so it survives container recreation): + +```json +{"version": 1, "disks": { + "path:/var/lib/vz": {"verdict": 1, "saw_uncorrectable": false, ...}, + "uuid:91d2dc2d-2d28-4929-9bdd-3e11fa2f41ae": {"verdict": 1, "saw_uncorrectable": false, ...}}} +``` + +`verdict: 1` is `DiskVerdictOK` for both, `saw_uncorrectable: false`, never alerted. + +**Reading those two artefacts against each other is what exposed R-335** — see §14. + +### Leg 2 — the severity fix arrives (the point of the task) + +Two synthetic `disk_health_degraded` events pushed for customer `demo-hp` **through the real hub +event endpoint** (`POST https://hub.felhom.eu/api/v1/event`), from the guest's own controller using +its own hub credentials — the genuine controller→hub path, not a hand-crafted operator call. Both +returned `HTTP 200 {"ok":true}`. The hub DB was read with its `-wal` and `-shm` copied alongside +`hub.db` (a `hub.db`-only read is stale). + +**As STORED by the hub (`events`):** + +| id | severity pushed | severity STORED | +|----|-----------------|-----------------| +| 2964 | `warning` | **`warning`** | +| 2965 | `warn` | **`info`** ← coerced | + +**`notification_log` rows for those two events:** + +| id | event_type | severity | channel | status | error | +|----|-----------|----------|---------|--------|-------| +| 689 | `disk_health_degraded` | `warning` | `operator` | **`sent`** | *(none)* | +| — | *(the `"warn"` push)* | — | — | **NO ROW EXISTS** | — | + +**That pair is the proof.** The identical event, differing only in one word of the severity string, +is the difference between *delivered to the operator* and *stored as an informational notice and +delivered to nobody*. This is the first time this leg has been observed end to end. + +Only the operator leg fired because **demo-hp has no `customer_notifications` row at all** (no +customer email, no `enabled_events`), so no customer row was possible for either push — verified +directly, not assumed. **One real email was sent to the operator**, as the task anticipated. + +--- + +## 9. NOT yet live-validated — stated explicitly + +**The Fail-from-counters path has never fired on real hardware.** Everything in §4/§5 exercises it +against the committed fixture's values in unit tests only. The live legs above prove the *negative* +(no false alert on three healthy disks) and the *severity wire* (end to end, through the hub) — they +do **not** prove a live disk reaching Hiba. The fixture tests must not be read as a live proof. + +Tracked as **R-332 (WATCHING)**. Closing condition: a live disk reaching Hiba from counters, or a +deliberate injection through the real pipeline (agent `/disks` → controller check → hub event) — not +a hand-set verdict. + +**One item originally listed here has since been proven live** and is no longer part of this gap: the +**persisted state surviving a controller restart**. The v0.215.0 → v0.216.0 redeploy destroyed and +rebuilt the container, and the new one read back a `changed_at` written by the previous version rather +than re-baselining — see §14. What remains unproven is the stronger half: an already-**alerted** disk +not re-alerting after a restart, which needs a disk that has actually alerted. The drive that produced the fixture lives in DooPlex, which is Tier 2 and never a +drill target; the demo boxes are all-flash and healthy. + +--- + +## 10. Teardown + +**This run provisioned nothing** — no VM, no guest, no hub customer record, no storage. Nothing was +formatted, mounted, unmounted, repaired or written on any monitored disk; the only write is the +controller's own `disk-health-state.json` inside its data volume. + +Disposition of what the run did create: + +- **Two synthetic hub events (`events` id 2964, 2965) and one `notification_log` row (id 689)** on the + live hub. **Left in place deliberately.** Both messages are self-labelling + (`"R-328 severity probe (…) - synthetic, no real disk fault"`), and deleting rows from the + production hub DB is a riskier act than leaving two clearly-marked probe rows. Named here so they + are not mistaken later for a real disk fault on demo-hp. +- **One real operator email** resulting from row 689. +- A local copy of `hub.db`/`-wal`/`-shm` in the session scratchpad only (not committed, not exported). + +--- + +## 11. Register rows + +| Row | State | Owner | +|-----|-------|-------| +| **R-328** — the severity drop: `"warn"` coerced to `info`, emailed to nobody | **CLOSED** (controller v0.215.0), proven live side by side | CC | +| **R-329** — `app_start_failed` carries the identical defect | READY — **not fixed here**; needs a decision on whether it should notify at all | Viktor | +| **R-330** — Phase 2: collect SMART attrs 187/199/188 + persist samples | READY — a declared wire change, hub models it in the same session under G-1 | CC | +| **R-331** — Phase 3: growth-rate detection; revisit the static 64 | READY, blocked on R-330 | CC | +| **R-332** — the Fail path has never fired on real hardware | **WATCHING** | CC | +| **R-333** — NVMe temperature bands; agent `smartctl` has no `-n standby` | READY (S each) | Viktor decides (a); CC does (b) | +| **R-334** — released with no golden carrying it (gate waiver) | READY — now applies to **v0.216.0** | CC bakes; **Viktor vouches** | +| **R-335** — one physical disk walked twice per run, sustaining against itself | **CLOSED** (controller v0.216.0) | CC | + +`smartd`-on-DooPlex-alerts-nobody is recorded in `DIAG-smart-passed-trap-2026-08-14.md` §8 as the +same shape one layer out. + +--- + +## 12. Observations — noticed, NOT acted on + +1. **`app_start_failed` has the identical severity defect** (`notifier.go` ~L546, `"warn"`). Left + untouched per scope. It needs a prior decision — should a stopped app email the customer at all? — + because flipping the string alone converts a silent event into a mail flood on a crash-looping box. + **R-329.** + +2. **The 55/60 °C bands are spinning-disk bands being applied to NVMe, and this is close to biting.** + Adopted unchanged from the operator's Prometheus config by explicit decision — but demo-hp's + **healthy** Toshiba NVMe idles at **53 °C**, i.e. **2 °C below Figyelmeztetés and 7 °C below Hiba**, + and NVMe routinely passes 60 °C under sustained write with no fault. As shipped, a healthy customer + NVMe under load can be reported as **Hiba** — the single worst outcome this feature can produce, and + the one leg 1 exists to guard. Not changed here because the threshold is a stated, settled operator + decision; flagged rather than overridden. **R-333(a) — recommend splitting the bands by device + class, or dropping them for NVMe and relying on `critical_warning`.** + +3. **The agent runs bare `smartctl -a -j` with no `-n standby`** + (`felhom-agent/internal/storage/hostops.go:368`), so every poll wakes a spun-down drive, and 6h → 1h + multiplies that by six. Recorded, not acted on, per the task's instruction. demo-hp is all-flash so + the measurement could not reveal it. Mitigating datum from the fixture: the failing drive logged + only **3375 load cycles in 60505 power-on hours** (~one per 18 h), so this duty cycle barely spins + down at all. **R-333(b).** + +4. **`source ~/.config/credentials` prints two recovery codes to the terminal.** The file contains + hyphenated keys (`R_DEMO-FELHOM`, `R_DEMO-HP`) that bash cannot assign, so sourcing it emits + `command not found` errors **containing the secret values**. Anything that sources that file leaks + them into logs, scrollback and transcripts. Not a code defect and out of scope; worth quoting + values from it by other means, or renaming the keys. + +5. **`golden_currency_gate.py` has no waiver parser.** Its own failure text says *"record a waiver in + `OPEN-ITEMS.md` — never a bypass"*, but nothing reads such a waiver, so the only way past it is the + bypass it warns against. See §13. + +--- + +## 13. Deviations, stated plainly + +- **`git push --no-verify` was used once**, on the `felhom.eu` docs push (`767960b`), and only there. + Cause: `golden_currency_gate.py` correctly convicts the fact that controller **v0.215.0 is released + and no golden carries it** (newest bake 0.214.0), so a *newly installed* machine would receive + 0.214.0 — without the severity fix. A golden bake was out of the task's scope, and its second half + (vouching in the hub's day-0 artifact manifest) is operator-password-gated, so CC cannot complete it; + a baked-but-unvouched golden is worse than none. Recorded as **R-334** with the bake+vouch owners + named. CI re-runs the same entry point and will mail the operator. The running fleet is unaffected. +- **One pre-existing test changed meaning by design:** `TestDiskVerdictFor`'s + `critical_warning>0 → warn` case is now `→ fail` (truth-table row 4 — NVMe's own critical flag is a + device declaration, not a drifting counter). `TestDiskHealthCheck_DegradationOnce` and its siblings + were rewritten into the scenario groups because they encoded the pre-v0.215.0 single-alert behaviour + the task deliberately replaces (Scenario C). + +--- + +## 14. R-335 — a defect in v0.215.0, found live, fixed as v0.216.0 + +**How it was found.** Not by a test and not by review: by reading the release's own **positive +observable** against the release's own **persisted artefact**. The hourly check logged *"3 disk(s) +evaluated"*; `disk-health-state.json` held **two** records. Two artefacts that should have agreed did +not. + +**Cause.** demo-hp's `c11-scratch` and `felhom-backup` are the same physical NVMe (`/dev/nvme0n1`) and +resolve to the same `diskKey`, so one disk was walked twice in a single run. + +**Why it mattered.** `RunDiskHealthCheck` writes a disk's new record before the next entry reads it, so +the **second** copy of an aliased disk consumed the **first** copy's write as its prior. The disk +therefore **sustained against itself and reached Hiba on a first sighting** — defeating truth-table +row 6, the single rule separating a one-hour benign excursion from a false critical alert — and would +have emitted **two identical events** for one drive. + +**Severity in practice: latent, not active.** Nothing fired on demo-hp because all three entries are +healthy with zero counters. But any aliased disk developing one pending sector would have gone +straight to Hiba, which is precisely the outcome §8 leg 1 exists to prevent. Aliasing is not exotic — +it is the *normal* shape whenever a box has two PVE storage entries on one physical device. + +**Fix (v0.216.0, `90f2545`).** Each `diskKey` is evaluated once per run. Both entries stay marked +`seen`, so neither is mistaken for a disappeared disk, and the card still renders **both** storage +rows — the dedup is about state and alerts, not display. Pinned by +`TestDiskCheck_SameDiskTwiceIsEvaluatedOnce`, red-proof run and reverted (§5). + +**Deployed:** `gitea.dooplex.hu/admin/felhom-controller:0.216.0 Up 6 seconds (healthy)`. + +**Confirming cycle on v0.216.0 — CONFIRMED LIVE, 09:31:35Z:** + +``` +live image: gitea.dooplex.hu/admin/felhom-controller:0.216.0 Up About an hour (healthy) +2026/08/14 09:31:35 [INFO] [web] disk-health check complete: 2 disk(s) evaluated, 0 alert(s) +grep -c disk_health_degraded: 0 +``` + +**`2 disk(s) evaluated` now matches the 2 persisted records.** The count and the artefact agree, which +is the disagreement that exposed R-335 in the first place. Still zero alerts, still both card rows. + +### The redeploy also proved persistence live — a gap §9 had listed as unproven + +The 0.215.0 → 0.216.0 redeploy **replaced the container**, and the state file came back intact: + +```json +"path:/var/lib/vz": {"verdict": 1, "changed_at": "2026-08-14T07:23:14.640216851Z", ...} +"uuid:91d2dc2d-…": {"verdict": 1, "changed_at": "2026-08-14T07:23:14.640216851Z", ...} +``` + +That `changed_at` was written by **v0.215.0's first cycle at 07:23Z**, before the container was +destroyed and rebuilt. The v0.216.0 container read it back and preserved it rather than stamping a +fresh time — so the new container **loaded the pre-restart record instead of silently re-baselining**. +That is Scenario L observed on real hardware, not just through the production-path unit test, and it +is exactly the behaviour that was impossible before v0.215.0 (the baseline was in-memory). + +It also incidentally confirms the unchanged-verdict path: `changed_at` is preserved across four checks +and two controller versions because the verdict never changed, rather than being churned every cycle. + +**What this still does NOT prove:** these disks are healthy and were never alerted, so the stronger +half — *an already-ALERTED disk not re-alerting after a restart* — remains unit-tested only. R-332 +stands. + +**Process note, recorded because it nearly cost the fix.** The red-proof harness reverts with +`git checkout --`, which restores to `HEAD`. Running a red-proof against an **uncommitted** fix +therefore *deletes the fix* along with the mutation — which happened here and was caught only by +re-grepping the source afterwards. Commit the fix before red-proofing it, or snapshot outside git. diff --git a/controller/README.md b/controller/README.md index 11a2a08..ee4881b 100644 --- a/controller/README.md +++ b/controller/README.md @@ -718,6 +718,31 @@ Per-app export creates a self-contained `.fab` file (tar.gz, optionally encrypte The backup system implements a **3-2-1 backup architecture**. Each tier is a **complete, self-sufficient backup** — any single tier can fully restore an app. +**The restore carries the customer's own previous answers (v0.217.0, R-351).** +`internal/backup/offbox_placement.go`. Every recovery unit's `manifest.json` records `drive` and +`namespace_root`, and its `compose/app.yaml` records `SUBDOMAIN`/`DOMAIN`. Until v0.217.0 nothing read +them back, so a restore into a destination different from the recorded one **succeeded silently**. + +- **`CheckPlacement`** compares the recorded drive against where the restore is about to write, + **before the safety dump and before the first byte**. A difference is **named — both values** — and + refused. The customer may proceed deliberately with `ack_placement`, a **separate** form field from + `confirm=1`: one click must not carry two decisions. +- **An UNKNOWN recording is never a mismatch.** A pre-field or unreadable manifest falls back to the + previous behaviour rather than blocking, and is never rendered as an empty value. +- **The not-installed refusal names where the data belonged**, read from the prepared scratch. +- **The deploy page prefills the address and data folder from the app's own backup** + (`RecordedUnitForStack`), labelled as coming from the backup and still editable — a memory, not a + lock. `RecordedAddress.Known()` requires **both** halves: an absent `SUBDOMAIN` would otherwise + surface the *catalog's* default as though the customer had chosen it. +- **Where the app's data will live is stated on the deploy page before the button is pressed** + (R-352). Measured 2026-08-21: 13 of 53 catalogue templates declare a storage field; the other 40 + have none and their data goes to the system drive. **Visibility only — no placement changed.** +- **Starting a restore is gated by `restoreOpBlocked()`**, which reads the display flag as well as the + concurrency flag. Before v0.217.0 a second press started a second run and was told it had. +- **The off-site listing's per-app size calls run concurrently, bounded to 4.** Measured before the + change: 2605 ms + 5 × 2697 ms ≈ 16 s. The bound protects the Storage Box's session cap; a refused + size call returns 0, which under-reports rather than fails visibly. + **The reserve — per-app backup admission (v0.192.0 decision B2, widened by v0.193.0 / R-181).** `internal/backup/admission.go`. Since the `mp1`→`mp0` merge (R-165) local backups and Docker's data-root share one filesystem, so an unbounded backup write is a stopped box rather than a slow one. diff --git a/controller/internal/backup/offbox_inventory.go b/controller/internal/backup/offbox_inventory.go index 1d7718f..9138576 100644 --- a/controller/internal/backup/offbox_inventory.go +++ b/controller/internal/backup/offbox_inventory.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "sort" + "sync" "time" ) @@ -113,14 +114,59 @@ func (m *Manager) OffsiteInventoryList(ctx context.Context) (OffsiteInventory, e // empty app list without the Empty flag; the page renders the honest in-between wording. return inv, nil } + // R-351 Part 4 — THE SIZE CALLS RUN CONCURRENTLY, BOUNDED. + // + // MEASURED before changing anything, on demo-hp against the live off-site target + // (u629488-sub3.your-storagebox.de:23), 2026-08-21: + // + // restic snapshots --json (once, whole repo) 2605 ms + // restic stats --mode restore-size (per app) 2697 ms each, 5 app tags, SEQUENTIAL + // => 2605 + 5*2697 = ~16.1 s + // + // which is the ten-to-fifteen seconds the page was reported to take. The cause is the shape + // already on file — one network call per app, one after another — so the fix is the same one: + // run them at once. Each call is an independent SSH round-trip to the repository and `stats` is + // a READ (restic takes a shared lock), so they do not contend. + // + // WHY BOUNDED, and why the bound is small: the target is a Hetzner Storage Box, which caps + // concurrent SSH sessions. Unbounded fan-out over a large app list would trade a slow page for + // refused connections — and a refused size call degrades to SizeBytes 0, i.e. it would quietly + // UNDER-REPORT the customer's own data rather than fail loudly. Four keeps well clear of the cap + // and still collapses the common case to a single wave. + const inventorySizeConcurrency = 4 + + type sized struct { + app OffsiteInventoryApp + err error + } + results := make([]sized, 0, len(newest)) + var mu sync.Mutex + var wg sync.WaitGroup + sem := make(chan struct{}, inventorySizeConcurrency) for tag, n := range newest { - app := OffsiteInventoryApp{App: tag, LatestAt: n.at} - if size, serr := m.offboxSnapshotSize(ctx, n.id); serr == nil { - app.SizeBytes = size - } else { - m.logger.Printf("[WARN] [offbox] inventory: size of %s's newest snapshot unknown: %v (listing it anyway)", tag, serr) + wg.Add(1) + go func(tag, id string, at time.Time) { + defer wg.Done() + sem <- struct{}{} + defer func() { <-sem }() + app := OffsiteInventoryApp{App: tag, LatestAt: at} + size, serr := m.offboxSnapshotSize(ctx, id) + if serr == nil { + app.SizeBytes = size + } + mu.Lock() + results = append(results, sized{app: app, err: serr}) + mu.Unlock() + }(tag, n.id, n.at) + } + wg.Wait() + // Logging happens on the caller's goroutine, after the fan-out: m.logger is shared and the + // per-app WARN is the only thing that tells an operator a size is missing rather than zero. + for _, r := range results { + if r.err != nil { + m.logger.Printf("[WARN] [offbox] inventory: size of %s's newest snapshot unknown: %v (listing it anyway)", r.app.App, r.err) } - inv.Apps = append(inv.Apps, app) + inv.Apps = append(inv.Apps, r.app) } sort.Slice(inv.Apps, func(i, j int) bool { return inv.Apps[i].App < inv.Apps[j].App }) return inv, nil diff --git a/controller/internal/backup/offbox_inventory_test.go b/controller/internal/backup/offbox_inventory_test.go new file mode 100644 index 0000000..d37716c --- /dev/null +++ b/controller/internal/backup/offbox_inventory_test.go @@ -0,0 +1,178 @@ +package backup + +import ( + "bytes" + "context" + "fmt" + "log" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// inventoryFixture wires a Manager whose repository answers a fixed snapshot list, and whose per-app +// `stats` calls are observable: how many ran at once, how many in total, and which ones fail. +// +// The delay is what makes the concurrency assertion meaningful — with instant calls a sequential +// implementation could pass a peak-of-1 check by finishing before the next one starts. +func inventoryFixture(t *testing.T, tags []string, statDelay time.Duration, failFor map[string]bool) (*Manager, *int32, *int32, *bytes.Buffer) { + t.Helper() + m, sett := newOffboxManager(t) + if err := sett.SetOffboxTarget(&settings.OffboxTarget{ + Enabled: true, Host: "nas.local", Port: 22, User: "felhom", RepoPath: "/srv/repo", + Schedule: "daily", EscrowState: "escrowed", + }); err != nil { + t.Fatal(err) + } + if err := m.WriteOffboxSecrets("KEYMATERIAL", "nas.local ssh-ed25519 HOSTKEY"); err != nil { + t.Fatal(err) + } + if !m.OffboxConfigured() { + t.Fatal("fixture: the target must be configured or the listing exits early") + } + var logbuf bytes.Buffer + m.logger = log.New(&logbuf, "", 0) + + var snaps []string + for i, tag := range tags { + snaps = append(snaps, fmt.Sprintf(`{"short_id":"snap%d","time":"2026-08-0%dT06:00:00Z","tags":["%s"]}`, i, i+1, tag)) + } + snapshotsJSON := "[" + strings.Join(snaps, ",") + "]" + + var inFlight, peak, total int32 + var mu sync.Mutex + m.SetOffboxRunner(func(_ context.Context, _ []string, args ...string) ([]byte, error) { + if contains(args, "snapshots") { + return []byte(snapshotsJSON), nil + } + if !contains(args, "stats") { + return nil, nil + } + atomic.AddInt32(&total, 1) + cur := atomic.AddInt32(&inFlight, 1) + mu.Lock() + if cur > peak { + peak = cur + } + mu.Unlock() + time.Sleep(statDelay) + atomic.AddInt32(&inFlight, -1) + // args carries the snapshot id; map it back to its tag position. + for i, tag := range tags { + if contains(args, fmt.Sprintf("snap%d", i)) { + if failFor[tag] { + return nil, fmt.Errorf("injected failure for %s", tag) + } + return []byte(`{"total_size":1048576}`), nil + } + } + return []byte(`{"total_size":0}`), nil + }) + return m, &peak, &total, &logbuf +} + +// R-351 PART 4 — the per-app size calls run AT ONCE, and never more than the bound at once. +// +// MEASURED on demo-hp against the live off-site target before any change (2026-08-21): +// snapshots --json 2605 ms once, then stats --mode restore-size 2697 ms PER APP, sequential, over +// 5 app tags — 2605 + 5*2697 = ~16.1 s, which is the reported ten-to-fifteen seconds. +// +// THE BOUND IS THE SAFETY PROPERTY, not the speed one. The repository is a Hetzner Storage Box with +// a concurrent-SSH-session cap; a size call that is refused returns SizeBytes 0, which is a SILENT +// UNDER-REPORT of the customer's own data rather than a visible failure. So the peak is asserted, +// not just the total. +func TestOffsiteInventoryList_SizeCallsAreConcurrentButBounded(t *testing.T) { + tags := []string{"immich", "nextcloud", "opengist", "paperless-ngx", "privatebin", "calibre-web", "vaultwarden"} + m, peak, total, _ := inventoryFixture(t, tags, 40*time.Millisecond, nil) + + start := time.Now() + _, err := m.OffsiteInventoryList(context.Background()) + elapsed := time.Since(start) + if err != nil { + t.Fatalf("inventory: %v", err) + } + + if int(*total) != len(tags) { + t.Errorf("every app needs its own size call: got %d, want %d", *total, len(tags)) + } + if *peak < 2 { + t.Errorf("the size calls must run concurrently; peak in flight was %d — that is the sequential shape that cost 16s", *peak) + } + if *peak > 4 { + t.Errorf("peak in flight was %d, above the bound of 4 — an unbounded fan-out risks refused connections, "+ + "and a refused size call silently under-reports the customer's data", *peak) + } + // 7 apps at 40ms: sequential would be >=280ms, four-at-a-time is two waves (~80ms). + if elapsed >= time.Duration(len(tags))*40*time.Millisecond { + t.Errorf("elapsed %v is the sequential cost — the fan-out is not taking effect", elapsed) + } +} + +// Nothing may be LOST or REORDERED by the fan-out. A missing app reads as "you have no backup of +// this", which is the worst possible thing for this page to say by accident. +func TestOffsiteInventoryList_FanOutLosesNothingAndStaysSorted(t *testing.T) { + tags := []string{"vaultwarden", "immich", "calibre-web", "opengist", "nextcloud"} + m, _, _, _ := inventoryFixture(t, tags, time.Millisecond, nil) + + inv, err := m.OffsiteInventoryList(context.Background()) + if err != nil { + t.Fatalf("inventory: %v", err) + } + if len(inv.Apps) != len(tags) { + t.Fatalf("apps listed = %d, want %d — the fan-out dropped one, which reads as a missing backup", len(inv.Apps), len(tags)) + } + seen := map[string]bool{} + for _, a := range inv.Apps { + seen[a.App] = true + if a.SizeBytes != 1048576 { + t.Errorf("%s: size = %d, want the injected value", a.App, a.SizeBytes) + } + } + for _, tag := range tags { + if !seen[tag] { + t.Errorf("%s is missing from the listing", tag) + } + } + for i := 1; i < len(inv.Apps); i++ { + if inv.Apps[i-1].App > inv.Apps[i].App { + t.Fatalf("the listing must stay sorted; %q came before %q", inv.Apps[i-1].App, inv.Apps[i].App) + } + } + if inv.Empty { + t.Error("a repository with snapshots is not Empty") + } +} + +// A FAILED size call must still list the app, with size 0, AND still log the WARN. The warning is +// the only thing that distinguishes "0 bytes" from "we could not tell" — and under a fan-out it is +// the easiest thing to lose, because the logging moved off the goroutine that produced the error. +func TestOffsiteInventoryList_FailedSizeStillListsAndStillWarns(t *testing.T) { + tags := []string{"immich", "opengist"} + m, _, _, logbuf := inventoryFixture(t, tags, time.Millisecond, map[string]bool{"opengist": true}) + + inv, err := m.OffsiteInventoryList(context.Background()) + if err != nil { + t.Fatalf("a per-app size failure must not fail the whole listing: %v", err) + } + if len(inv.Apps) != 2 { + t.Fatalf("both apps must be listed; got %d", len(inv.Apps)) + } + for _, a := range inv.Apps { + if a.App == "opengist" && a.SizeBytes != 0 { + t.Errorf("an unknown size must be 0, got %d", a.SizeBytes) + } + if a.App == "immich" && a.SizeBytes == 0 { + t.Error("the healthy app's size must survive its neighbour's failure") + } + } + if !strings.Contains(logbuf.String(), "size of opengist's newest snapshot unknown") { + t.Errorf("the WARN is what separates \"0 bytes\" from \"could not tell\"; log was: %s", logbuf.String()) + } + if strings.Contains(logbuf.String(), "size of immich's newest snapshot unknown") { + t.Error("a healthy app must not be warned about") + } +} diff --git a/controller/internal/backup/offbox_placement.go b/controller/internal/backup/offbox_placement.go index 3fd75ce..09251d9 100644 --- a/controller/internal/backup/offbox_placement.go +++ b/controller/internal/backup/offbox_placement.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "strings" + + "gopkg.in/yaml.v3" ) // R-351 — THE RESTORE ALREADY KNOWS WHERE THE APP LIVED. IT JUST NEVER LOOKED. @@ -87,6 +89,117 @@ func CheckPlacement(man *RecoveryManifest, liveDrive, liveNamespaceRoot string) return c } +// RecordedAddress is the web address the backup recorded for an app, read from the app.yaml the +// recovery unit captured beside its manifest. +// +// RECORDED, NEVER INFERRED. Both halves must come from the captured file. When the captured app.yaml +// carries no SUBDOMAIN, the LIVE deploy path falls back to the catalog's default +// (stacks/deploy.go:88-90) — that default is a catalog guess, not the customer's answer, and +// presenting it as "what your backup says" would be a fabricated fact. This project has ruled twice +// that a guess dressed as a fact is how these bugs are built, so an absent SUBDOMAIN is UNKNOWN here. +type RecordedAddress struct { + Subdomain string + Domain string +} + +// Known reports whether BOTH halves were recorded. A half-known address is not an address: showing +// „gist." or „.enkisfelhom.hu" as a prefill is worse than showing nothing. +func (a RecordedAddress) Known() bool { + return strings.TrimSpace(a.Subdomain) != "" && strings.TrimSpace(a.Domain) != "" +} + +// FQDN is the address as the customer knows it, or "" when it is not fully known. +func (a RecordedAddress) FQDN() string { + if !a.Known() { + return "" + } + return strings.TrimSpace(a.Subdomain) + "." + strings.TrimSpace(a.Domain) +} + +// unitAppConfig is the slice of the captured app.yaml this package needs. Deliberately a LOCAL +// minimal struct rather than stacks.AppConfig: internal/stacks imports nothing from here today and +// reaching across for one map would couple the backup layer to the deploy layer's schema for no +// gain. Unknown keys are ignored by yaml.v3, so a richer app.yaml still parses. +type unitAppConfig struct { + Env map[string]string `yaml:"env"` +} + +// RecordedUnitForStack reads what an app's most readable recovery unit says about where it lived and +// what address it answered on. Returns ok=false when no unit can be read at all. +// +// SEARCH ORDER, and why it is not just "the app's own drive": the case this exists for is an app +// that is NOT INSTALLED on a rebuilt box, so GetStackHDDPath returns "" and there is no own drive to +// consult. It therefore checks every namespace root the box can currently see — the registered +// storage paths and the system data path — and takes the first unit it can read. That is a handful +// of stat calls on local disk: no network, no restic, no restore. +// +// A drive that is NOT attached contributes nothing, which is the honest outcome: we cannot read a +// unit that is not here, and the reconstitution's own refusal owns that case with a route. +func (m *Manager) RecordedUnitForStack(stack string) (RecordedPlacement, RecordedAddress, bool) { + if !isSafeStackName(stack) { + return RecordedPlacement{}, RecordedAddress{}, false + } + seen := map[string]bool{} + var roots []string + addRoot := func(drive string) { + drive = strings.TrimSpace(drive) + if drive == "" { + return + } + r := m.namespaceRoot(drive) + if r != "" && !seen[r] { + seen[r] = true + roots = append(roots, r) + } + } + // The app's own drive first when it HAS one — that unit is the authoritative one for an + // installed app, and checking it first keeps the common case to a single stat. + if m.stackProvider != nil { + addRoot(m.stackProvider.GetStackHDDPath(stack)) + } + if m.settings != nil { + for _, sp := range m.settings.GetStoragePaths() { + addRoot(sp.Path) + } + } + addRoot(m.systemDataPath) + + for _, root := range roots { + unit := RecoveryUnitPath(root, stack) + man := readManifest(filepath.Join(unit, "manifest.json")) + if man == nil { + continue + } + place := RecordedPlacement{ + Drive: strings.TrimSpace(man.Drive), + NamespaceRoot: strings.TrimSpace(man.NamespaceRoot), + } + addr := readRecordedAddress(filepath.Join(unit, "compose", "app.yaml")) + if !place.Known() && !addr.Known() { + continue // a unit that tells us nothing is not an answer + } + return place, addr, true + } + return RecordedPlacement{}, RecordedAddress{}, false +} + +// readRecordedAddress parses SUBDOMAIN/DOMAIN out of a captured app.yaml. Returns the zero value on +// any failure — absent file, unreadable, malformed, or either half missing. +func readRecordedAddress(appYAMLPath string) RecordedAddress { + raw, err := os.ReadFile(appYAMLPath) + if err != nil { + return RecordedAddress{} + } + var cfg unitAppConfig + if yaml.Unmarshal(raw, &cfg) != nil || cfg.Env == nil { + return RecordedAddress{} + } + return RecordedAddress{ + Subdomain: strings.TrimSpace(cfg.Env["SUBDOMAIN"]), + Domain: strings.TrimSpace(cfg.Env["DOMAIN"]), + } +} + // scratchManifestMaxDepth bounds the walk below. The unit sits a handful of levels under the scratch // root (the restore mirrors the snapshot's absolute path), and an unbounded walk over a scratch that // also holds a full userdata tree would stat a customer's entire library to find one small file. diff --git a/controller/internal/backup/offbox_recorded_unit_test.go b/controller/internal/backup/offbox_recorded_unit_test.go new file mode 100644 index 0000000..78dc347 --- /dev/null +++ b/controller/internal/backup/offbox_recorded_unit_test.go @@ -0,0 +1,163 @@ +package backup + +import ( + "os" + "path/filepath" + "testing" +) + +// writeRecordedUnit lays down a recovery unit the way a capture would: manifest.json plus a compose/ dir +// holding the app.yaml. Returns the unit path so a test can corrupt or truncate it. +func writeRecordedUnit(t *testing.T, nsRoot, stack string, man *RecoveryManifest, appYAML string) string { + t.Helper() + unit := RecoveryUnitPath(nsRoot, stack) + if err := os.MkdirAll(filepath.Join(unit, "compose"), 0o755); err != nil { + t.Fatal(err) + } + if man != nil { + if err := writeManifest(filepath.Join(unit, "manifest.json"), man); err != nil { + t.Fatal(err) + } + } + if appYAML != "" { + if err := os.WriteFile(filepath.Join(unit, "compose", "app.yaml"), []byte(appYAML), 0o600); err != nil { + t.Fatal(err) + } + } + return unit +} + +// R-351 SCENARIO A — the values the customer had to remember are in their own backup. +// +// The 2026-08-21 walk-through needed two of them: the web address and the data folder. Both are in +// the unit — the folder in manifest.json, the address in the captured app.yaml — and nothing read +// either back. This is the read. +func TestRecordedUnitForStack_ReadsBothValues(t *testing.T) { + m, sett := newOffboxManager(t) + drive := t.TempDir() + addSchedulablePath(t, sett, drive) // the existing helper, not a second spelling of it + // The app is NOT installed — no stack provider entry. This is the rebuilt-box shape, and the + // whole reason the lookup cannot simply ask the live app where it lives. + writeRecordedUnit(t, m.namespaceRoot(drive), "calibre-web", + &RecoveryManifest{SchemaVersion: 2, AppName: "calibre-web", Drive: drive, NamespaceRoot: drive}, + "env:\n DOMAIN: enkisfelhom.hu\n SUBDOMAIN: books\n HDD_PATH: "+drive+"\n") + + place, addr, ok := m.RecordedUnitForStack("calibre-web") + if !ok { + t.Fatal("a readable unit on an attached drive must be found — otherwise there is nothing to prefill") + } + if place.Drive != drive { + t.Errorf("recorded drive = %q, want %q", place.Drive, drive) + } + if got := addr.FQDN(); got != "books.enkisfelhom.hu" { + t.Errorf("recorded address = %q, want books.enkisfelhom.hu", got) + } +} + +// THE RULE THAT MATTERS MOST HERE: recorded, never inferred. +// +// When the captured app.yaml has no SUBDOMAIN, the LIVE deploy path substitutes the catalog default +// (stacks/deploy.go:88-90). That default is the CATALOG's guess, not the customer's answer. Offering +// it back as "what your backup says" would be a fabricated fact — the exact thing this project has +// ruled against twice. An absent half makes the whole address unknown. +func TestRecordedAddress_IsNeverInferred(t *testing.T) { + for _, tc := range []struct { + name string + appYAML string + want string + wrong string + }{ + { + name: "both halves recorded", + appYAML: "env:\n DOMAIN: enkisfelhom.hu\n SUBDOMAIN: gist\n", + want: "gist.enkisfelhom.hu", + wrong: "-", + }, + { + name: "no subdomain recorded", + appYAML: "env:\n DOMAIN: enkisfelhom.hu\n", + want: "", + wrong: "the catalog default offered back as if the backup had recorded it", + }, + { + name: "no domain recorded", + appYAML: "env:\n SUBDOMAIN: gist\n", + want: "", + wrong: "a half address like \"gist.\" shown as a prefill", + }, + { + name: "no env block at all", + appYAML: "deployed: true\n", + want: "", + wrong: "an empty address rendered as a recorded value", + }, + { + name: "malformed yaml", + appYAML: "env: [this is not a map\n", + want: "", + wrong: "a parse failure treated as an answer", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "app.yaml") + if err := os.WriteFile(p, []byte(tc.appYAML), 0o600); err != nil { + t.Fatal(err) + } + got := readRecordedAddress(p) + if got.FQDN() != tc.want { + t.Errorf("FQDN = %q, want %q — wrong outcome guarded: %s", got.FQDN(), tc.want, tc.wrong) + } + }) + } + + if got := readRecordedAddress(filepath.Join(t.TempDir(), "absent.yaml")); got.Known() { + t.Errorf("an absent app.yaml must yield no address, got %+v", got) + } +} + +// THE 40-OF-53 CLASS, EXPLICITLY. An app that declares no data path still has its placement recorded +// — on the system drive. It must NOT fall into the unknown case just because it has no storage +// field: the recorded value is still the truth, and the mismatch check still applies to it. +// Measured shape: opengist, drive=/mnt/sys_drive. +func TestRecordedUnitForStack_NoDeclaredDataPathIsStillRecorded(t *testing.T) { + m, _ := newOffboxManager(t) + sys := t.TempDir() + m.systemDataPath = sys + + nsRoot := m.namespaceRoot(sys) + writeRecordedUnit(t, nsRoot, "opengist", + &RecoveryManifest{SchemaVersion: 2, AppName: "opengist", Drive: sys, NamespaceRoot: nsRoot}, + "env:\n DOMAIN: enkisfelhom.hu\n SUBDOMAIN: gist\n") // note: NO HDD_PATH — that is the class + + place, addr, ok := m.RecordedUnitForStack("opengist") + if !ok { + t.Fatal("an app with no declared data path must still have a readable recorded placement") + } + if !place.Known() { + t.Error("the system drive is a recorded destination, not an unknown one") + } + if place.Drive != sys { + t.Errorf("recorded drive = %q, want the system drive %q", place.Drive, sys) + } + if got := addr.FQDN(); got != "gist.enkisfelhom.hu" { + t.Errorf("address = %q, want gist.enkisfelhom.hu", got) + } + // And the mismatch check applies to it exactly as to any other app. + c := CheckPlacement(&RecoveryManifest{Drive: sys}, "/mnt/felhom-drives/hdd_1", "") + if !c.Mismatch { + t.Error("moving a no-declared-path app to a real drive is a mismatch and must be named") + } +} + +// No unit anywhere is "we cannot tell", not "it lived nowhere". +func TestRecordedUnitForStack_NothingReadableIsNotAnAnswer(t *testing.T) { + m, _ := newOffboxManager(t) + m.systemDataPath = t.TempDir() + if _, _, ok := m.RecordedUnitForStack("opengist"); ok { + t.Error("with no unit on any readable root the lookup must report that it cannot tell") + } + if _, _, ok := m.RecordedUnitForStack("../etc/passwd"); ok { + t.Error("an unsafe stack name must be refused before any path is built") + } +} diff --git a/controller/internal/stacks/metadata.go b/controller/internal/stacks/metadata.go index 257a635..6053d2b 100644 --- a/controller/internal/stacks/metadata.go +++ b/controller/internal/stacks/metadata.go @@ -387,6 +387,22 @@ func (m *Manager) ClassifiedBinds(name string) ([]appbackup.ClassifiedBind, bool return appbackup.ClassifyBinds(meta.Backup, binds) } +// HasDeployField reports whether the app declares a deploy field for the given env var. +// +// R-351: the caller that needs this is the restore prefill, and the question it is really asking is +// "does this app have somewhere to PUT a recorded value?". Measured 2026-08-21: only 13 of the 53 +// catalog templates declare `env_var: HDD_PATH`; the other 40 have no storage field at all and their +// data lands on the system drive. For those, a recorded placement is a FACT TO STATE, never a value +// to offer — writing it into a field that does not exist would be a prefill nobody can see or change. +func (m *Metadata) HasDeployField(envVar string) bool { + for _, f := range m.DeployFields { + if f.EnvVar == envVar { + return true + } + } + return false +} + // HasDeployFields returns true if the app has any user-facing deploy fields // (i.e., fields beyond auto-filled domain and auto-generated secrets). func (m *Metadata) HasDeployFields() bool { diff --git a/controller/internal/web/deploy_restore_prefill_test.go b/controller/internal/web/deploy_restore_prefill_test.go new file mode 100644 index 0000000..094453a --- /dev/null +++ b/controller/internal/web/deploy_restore_prefill_test.go @@ -0,0 +1,189 @@ +package web + +import ( + "bytes" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-351 — RENDER TESTS, ONE PER BRANCH. +// +// A green build and a green vet say NOTHING about a template: `index` against an undefined key, or a +// method with a pointer receiver, both compile, both pass vet, both pass the suite, and both 500 at +// render. This repo has that on file twice ("template methods need value receivers", "seam built but +// never wired"). The existing deploy render test does not reach these branches — it renders a page +// with only AutoFields, so the subdomain and path blocks never execute — which is precisely how a +// broken branch stays green. +// +// Every case below therefore RENDERS and asserts on the HTML. + +const ( + prefillDataDrive = "/mnt/felhom-drives/hdd_1" + prefillOtherDriv = "/mnt/felhom-drives/hdd_2" + prefillSysDrive = "/mnt/sys_drive" +) + +// renderDeployWithFields renders the deploy page for an app that HAS user-facing fields, so the +// subdomain and path branches are actually executed. +func renderDeployWithFields(t *testing.T, declaresPath bool, extra map[string]interface{}) string { + t.Helper() + s := securityHarness(t) + s.loadTemplates() + + fields := []stacks.DeployField{ + {EnvVar: "SUBDOMAIN", Label: "Aldomain", Type: "subdomain", Default: "katalogus-alap"}, + } + if declaresPath { + fields = append(fields, stacks.DeployField{EnvVar: "HDD_PATH", Label: "Adatmeghajto", Type: "path"}) + } + + data := map[string]interface{}{ + "Page": "stacks", "Title": "Telepites", "Domain": "example.hu", + "Stack": stacks.Stack{Name: "demoapp"}, + "Meta": stacks.Metadata{DisplayName: "DemoApp", Slug: "demoapp"}, + "AlreadyDeployed": false, + "UserFields": fields, + "StoragePaths": []DeployStoragePath{ + {StoragePath: settings.StoragePath{Path: prefillDataDrive, Label: "USB 1"}, FreeHuman: "100 GB", FreePercent: 50}, + {StoragePath: settings.StoragePath{Path: prefillOtherDriv, Label: "USB 2", IsDefault: true}, FreeHuman: "200 GB", FreePercent: 80}, + }, + // The defaults the handler always sets. A test that omitted them would be testing a page the + // handler never produces. + "RestoreFieldValues": map[string]string{}, + "RestorePrefillHDDPath": "", + "RestoreRecordedDrive": "", + "RestoreRecordedAddress": "", + "RestoreRecordedDeclaresPath": declaresPath, + "RestoreHasRecord": false, + "SystemDataPath": prefillSysDrive, + } + for k, v := range extra { + data[k] = v + } + var buf bytes.Buffer + if err := s.tmpl.ExecuteTemplate(&buf, "deploy", data); err != nil { + t.Fatalf("render deploy: %v", err) + } + return buf.String() +} + +// SCENARIO D — the ordinary path. No backup record: the catalog default stands and the configured +// default drive stays selected. WRONG OUTCOME: a new question or obstacle where there was none. +func TestDeployPrefill_NoRecord_LeavesTheOrdinaryPathAlone(t *testing.T) { + html := renderDeployWithFields(t, true, nil) + + if !strings.Contains(html, `value="katalogus-alap"`) { + t.Error("with no recorded address the catalog default must still fill the subdomain field") + } + // The configured default drive keeps its selection. + if !strings.Contains(html, `value="`+prefillOtherDriv+`" data-free-percent="80"`) { + t.Fatal("fixture: the default drive option must render, or the assertion below proves nothing") + } + if !optionSelected(html, prefillOtherDriv) { + t.Error("with no record the configured default drive must remain the selected option") + } + if strings.Contains(html, "saját mentése alapján") { + t.Error("no record means no prefill notice — claiming one would be a fabricated fact") + } +} + +// SCENARIO A — the record exists and is offered back, visibly labelled as coming from the backup. +func TestDeployPrefill_WithRecord_OffersTheRecordedValuesAndSaysWhy(t *testing.T) { + html := renderDeployWithFields(t, true, map[string]interface{}{ + "RestoreFieldValues": map[string]string{"SUBDOMAIN": "konyvek", "DOMAIN": "example.hu", "HDD_PATH": prefillDataDrive}, + "RestorePrefillHDDPath": prefillDataDrive, + "RestoreRecordedDrive": prefillDataDrive, + "RestoreRecordedAddress": "konyvek.example.hu", + "RestoreHasRecord": true, + }) + + if !strings.Contains(html, `value="konyvek"`) { + t.Error("the recorded subdomain must be prefilled — this is the value the person had to remember") + } + if strings.Contains(html, `value="katalogus-alap"`) { + t.Error("the catalog default must NOT win over the customer's own recorded answer") + } + if !optionSelected(html, prefillDataDrive) { + t.Error("the drive the BACKUP recorded must be preselected, not the configured default") + } + if optionSelected(html, prefillOtherDriv) { + t.Error("the configured default must not also be selected — two selected options is a broken form") + } + // The origin must be stated. An unexplained prefill is indistinguishable from a default. + for _, must := range []string{"saját mentése alapján", "konyvek.example.hu", prefillDataDrive} { + if !strings.Contains(html, must) { + t.Errorf("the notice must state %q so the prefill is not mistaken for a default", must) + } + } + // A MEMORY, NOT A LOCK. The whole ruling turns on the customer still being able to change these, + // so the input itself must carry neither `disabled` nor `readonly`. + if tag, ok := inputTag(html, "SUBDOMAIN"); !ok { + t.Error("the subdomain input must render, or the prefill has nowhere to live") + } else if strings.Contains(tag, "disabled") || strings.Contains(tag, "readonly") { + t.Errorf("the prefill is a memory, not a lock — the field must stay editable; got: %s", tag) + } +} + +// inputTag returns the rendered tag for a field, so an assertion can be scoped to it rather +// than searching the whole page (where `disabled` legitimately appears on other controls). +func inputTag(html, envVar string) (string, bool) { + i := strings.Index(html, `id="field-`+envVar+`"`) + if i < 0 { + return "", false + } + end := strings.Index(html[i:], ">") + if end < 0 { + return "", false + } + return html[i : i+end], true +} + +// THE 40-OF-53 CLASS — no storage field exists, so the placement is STATED as a fact and never +// written into an input that is not there. Part 1's visibility line names the system drive. +func TestDeployPrefill_NoDeclaredPath_StatesWhereTheDataGoes(t *testing.T) { + html := renderDeployWithFields(t, false, map[string]interface{}{ + "RestoreRecordedDrive": prefillSysDrive, + "RestoreRecordedAddress": "gist.example.hu", + "RestoreHasRecord": true, + "RestoreFieldValues": map[string]string{"SUBDOMAIN": "gist", "DOMAIN": "example.hu"}, + }) + + if strings.Contains(html, `name="HDD_PATH"`) { + t.Error("an app that declares no data path must not grow a storage field from the prefill") + } + if !strings.Contains(html, "rendszermeghajtóra") || !strings.Contains(html, prefillSysDrive) { + t.Error("Part 1: the page must say where the data will live BEFORE the button is pressed") + } + if !strings.Contains(html, `value="gist"`) { + t.Error("the recorded address still applies to this class — only the folder has no field") + } +} + +// Part 1's line must appear for the declaring class too, pointing at the selection. Its absence on +// one branch is how "we told the customer" becomes true only half the time. +func TestDeployPrefill_DeclaredPath_StillSaysWhereTheDataGoes(t *testing.T) { + html := renderDeployWithFields(t, true, nil) + if !strings.Contains(html, "kiválasztott adatmeghajtóra") { + t.Error("an app WITH a storage field must still be told that the choice is where its data lands") + } + if strings.Contains(html, "rendszermeghajtóra") { + t.Error("the system-drive sentence belongs only to apps with no storage field") + } +} + +// optionSelected reports whether the