diff --git a/CHANGELOG.md b/CHANGELOG.md index ef5bc88..50f617d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,72 @@ +## v0.116.0 — R-116: the flag and the key finally share a row (2026-07-30) + +**Closes the mechanism `v0.115.0` guessed at.** The absent-drive alarm was the generic +`storage_disconnected` while its recovery was the specific `backup_target_restored` — a pair an +operator cannot match. The cause is now measured, not reasoned: `felhom.eu` +`audits/DIAG-r116-disks-payload-2026-07-30.md`. + +**What the captured payload showed.** With the device gone `/disks` returns **4 rows, not 3** — the +drive appears **twice**, and the two facts the controller needs are on **different rows**: + +| row | source | `mount_path` | `guest_path` | `backup_target` | +|---|---|---|---|---| +| `felhom-backup` | Observe | `""` | `""` | **`true`** | +| `694034cc-…` | registry union | `/mnt/cel` | `/mnt/felhom-drives/cel` | field absent ⇒ **`false`** | + +`driveTargetByPath` (controller `intermediary.go:602-618`) registers a key only from a **non-empty** +field, so the flag-bearing row contributes **no key at all**, while the registry row owns +`/mnt/felhom-drives/cel` — the key the gate looks up — and says `false`. + +**Why v0.115.0 was inert, and it was not subtle.** Its fallback computed +`StablePathForRaw(t.MountPath)` on the Observe row, and in the absent state `t.MountPath` is `""` — +emptied by the same `exactMount` failure that empties `BackingDevice` (`observe.go:184-190`). So it was +`StablePathForRaw("")` == `""` and it assigned nothing. **Its test passed because the fixture supplied a +`MountPath` the real absent state does not have.** That fixture is corrected in this release, and +red-proof 1 replays v0.115.0's exact code against the corrected shape: it fails. + +**THE JOIN — the actual hard part.** With the device gone the two records of one drive share **no +runtime field**: no mount, no backing device, and the Observe row's `DurableID` has already degraded off +the fs-UUID (`path:/mnt/cel`). What they do share is **configuration** — the storage's `path` from +`storage.cfg` on one side, the `.mount` unit's `Where` on the other. Both yield the same stable guest +path, so that is the key the dedup now uses. + +- `hub.StorageTarget.ConfigPath` — the configured path, carried verbatim, **`json:"-"`**. That struct is + a cross-repo contract pinned by `testdata/host-report.golden.json` + `contract_test.go`'s key-set + comparison; a wire-visible field would have required a matching `felhom.eu/hub` change to stay + non-drifting, and nothing off-box needs the value. +- `observe.go` sets it from `s.Path`. **This is NOT the fallthrough the comment at `:176-183` forbids** — + that prohibition is about resolving a *device or UUID* from the **containing** filesystem, which would + hand back root's identity and mis-target a DR re-attach. `s.Path` is the storage's own declaration, + identifies nothing but itself, and is used for no resolution. `MountPath` stays empty, which is true. +- `disks.go` tries `MountPath` first (so the present-state path and v0.115.0's behaviour are + byte-identical) and falls back to `ConfigPath` only when the mount is genuinely gone. +- The union loop now dedups on guest path as well as mount path, so exactly **one** row carries the drive. + +**THE REGRESSION THIS AVOIDED, and it is why the two obvious fixes were both rejected.** The controller +reads `d.BackupTarget && d.MountPath != ""` as *"a real drive with its own mountpoint — healthy"* and +returns immediately (`backup_target_offer.go:79`). So **back-filling `MountPath` onto the Observe row** +(the smallest change) and **teaching the registry row the flag** (its `MountPath` is non-empty, read from +the by-then-stale unit file) both produce a row satisfying that predicate while the drive is missing — +either would have silently flipped **R-114**, which shipped 2026-07-29, back to a false *healthy*. +**R-114's correctness currently rests on the absent-state rows not combining the flag with a mount +path**; that coupling was invisible until the payload existed. Pinned by +`TestAbsentTargetKeepsR114DegradedSignal`, red-proofed by feeding the handler exactly what the rejected +option would have produced. + +**Deliberately unchanged:** `Role` (the absent target still reads `system`), the `BoundUnderParent` +conjunction, and every wire field. Suppressing the registry row in the absent state removes its +`uuid:` `durable_id`, its hardcoded — and false — `state: "attached"`, and its `total_bytes` copied from +the **root** filesystem (**R-118's symptom, incidentally gone; R-118 itself is NOT fixed and stays +open**). No consumer of those was found: wipe/decommission need the drive present, `firstOfferableDrive` +already excludes it, and the disk-health baseline excludes UNKNOWN verdicts. + +**Tests 845 → 849.** Four red-proofs, each mutation verified to have landed before the run: +(1) v0.115.0's MountPath-only fallback → `isTarget[guestPath] = FALSE, rows=2`; (2) drop the guest-path +dedup → `carried by 2 rows`; (3) give the absent row a MountPath → the R-114 guard fires; (4) over-broad +dedup → the non-target drive loses its own row (and two pre-existing R-113 tests fail too). +`TestPresentTargetPayloadUnchanged` pins the healthy payload field-for-field — the state the whole fleet +is in. + ## v0.115.0 — R-116: the backup-target flag reaches the row the controller keys on (2026-07-29) **The defect, measured live in Session C.** A drive whose device vanished raised the **generic** diff --git a/REPORT.md b/REPORT.md index df3366b..75b6042 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,98 +1,118 @@ -# REPORT — R-113: drive presence means the DEVICE, not the bind (agent v0.114.0, 2026-07-29) +# REPORT — v0.116.0: R-116, the flag and the key share a row (2026-07-30) -**Overwritten** per the standing rule. R-113, Session A of three (R-113 → R-114+R-112 → one drill-VM -validating all three). **Repos touched:** `felhom-agent` only; **`felhom-controller` deliberately -UNCHANGED** — see the ruling. **Status: code + tests complete, NOT LIVE-VALIDATED.** No drive was pulled; -that is Session C. +**Overwritten** per the standing rule. Agent-only fix: the controller was **not** touched (one fix, one +place) and `v0.115.0` was **not** reverted. Mechanism evidence: `felhom.eu` +`audits/DIAG-r116-disks-payload-2026-07-30.md`. -## Phase 0 ruling — shape (a), the agent tells the truth +## Baselines as actually running (not as on `main`) -`BoundUnderParent` becomes `bound && devicePresent`. Controller unchanged, no `MinAgent` bump. +| | `main` | running | a FRESH box gets (hub Day-0 manifest) | +|---|---|---|---| +| agent | 0.115.0 → **0.116.0** | felhom-pve **0.115.0**, demo-hp **0.113.0** | **0.115.0** | +| controller | 0.186.0 | felhom-pve **0.186.0**, demo-hp **0.185.1** | golden bakes **0.185.1** | +| hub | 0.81.0 | 0.81.0 | `min_agent` 0.113.0, `min_controller` 0.156.0 | +| `felhom.eu` | `1aa1bd1` | — | — | -| Q | Answer | -|---|---| -| 1. the symbol | `ResolveStorageDevice` (`internal/storage/durable_device.go:82`, `EvalSymlinks` of `/dev/disk/by-uuid/`) behind `hostops.go:298`; `mountByDurable` (`localapi/disks.go:960-969`, **raw mounts only** — binds skipped at `:962`) behind `disks.go:986` | -| 2. available at `/disks`? | Yes at both sites. Observe path has `t.MountPath`/`t.State`; union path has `d.MountPath`/`d.UUID` and already resolves that UUID eight lines later | -| 3. cost | `os.Stat` / `EvalSymlinks` — at most two syscalls per drive per poll. No exec, no shell-out | -| 4. `State == "attached"` | **Split verdict.** Observe path: truthful (`reachable()`, `storage/observe.go:321-330` = `exactMount && DeviceExists`). Union path: **hardcoded `"attached"`** (`disks.go:257`) — so `planDriveGates:229`'s legacy raw branch carries the same defect. **Filed as an observation, not fixed** | +**The fleet is not uniform on either component.** The golden baking controller **0.185.1** while +demo-felhom runs **0.186.0** is load-bearing for the ruling below: a controller-side fix would not reach +a fresh box without a re-bake, whereas the agent channel already serves the newest published version. -**Why (a) and not (b):** `BoundUnderParent` has exactly **one** functional consumer, `planDriveGates` -(`intermediary.go:226`); everything else across both repos is a comment or a test, and boot convergence -deliberately moved off it (`intermediary.go:362`). Tightening it changes one decision — the broken one. -Shape (b) is actively dangerous: a new `bool` absent from a pre-0.114.0 agent's JSON decodes to `false`, -so ANDing it in would make **every drive on an older agent read ABSENT and stop its apps** — the exact -Scenario D failure the spec forbids. Defending it needs `*bool` + feature detection: more machinery, worse -failure mode. +## The ruling: neither (a) nor (b) — a hybrid, because both regress R-114 -## The signal chosen, and why it is the right one +`primaryTargetID` is a PVE storage name (`felhom-backup`); the registry row's `Name` is the drive UUID. +With the device gone the two records of one drive share **no runtime field** — no mount, no backing +device, and the Observe row's `DurableID` has degraded off the fs-UUID to `path:/mnt/cel`. -Not a UUID probe — the **raw mount**. E-2d measured the asymmetry directly: with the device pulled, -`/mnt/mentes2` was NOT mounted while `/mnt/felhom-drives/mentes2` still read `/dev/sdb[/felhom-data]`. -The raw mount is a device-bound systemd unit; the agent's bind is not. So "is the raw mount still -mounted" **is** device presence, it is the same check on both construction paths, and it rests on a live -measurement rather than inference. +**Rejected (a) — back-fill `MountPath` on the Observe row.** The spec's lean, and it does re-activate +v0.115.0. Rejected on source evidence: `backup_target_offer.go:79` reads +`d.BackupTarget && d.MountPath != ""` as *"a real drive with its own mountpoint — healthy"* and returns +**before** its `TargetAbsent` branch. Option (a) manufactures exactly that row while the drive is +missing, so it would have silently flipped **R-114** — shipped 2026-07-29, proven live — back to a false +*healthy* banner. Also `MountPath`'s own contract is "the RAW host PVE mount" (`disks.go:152-153`); a path +that is not mounted is not that, so the downstream reading is correct and the change would be the bug. -## Files modified +**Rejected (b) — carry `BackupTarget` on the registry row.** A join **does** exist (below), so this was +available — but the registry row's `MountPath` is non-empty, read from the by-then-stale `.mount` unit +file. It trips `:79` identically. Same regression, same reason. -| File | Change | -|---|---| -| `internal/localapi/server.go` | new `deviceCheck func(string) bool` seam (mirrors `boundCheck`) | -| `internal/localapi/disks.go` | new `devicePresent(rawMountPath)`; conjunction at BOTH construction sites (`:210` Observe, `:263` union); `BoundUnderParent`'s doc restated to name both halves | -| `internal/localapi/intermediary_test.go` | existing fixture pinned `deviceCheck=true` — its subject is the bind half; unchanged otherwise | -| `internal/localapi/disks_device_presence_test.go` | **new** — 6 tests, groups A–D + the wire contract | -| `REUSE.md` | `devicePresent`/`deviceCheck` registered, with the "use this, never the bind" rule | +**Rejected (c) — resolve controller-side.** The mapping is not there to find, and the *one state, one +owner* ruling stands: the agent owns drive identity. -## Tests: 208 → 214 in `internal/localapi` (+6). Full agent suite green. +**Taken (d) — give the Observe row the guest path (and NO MountPath), then dedup the registry row on +guest path.** The gate keys on **GuestPath**; R-114 keys on **MountPath**. Separating them satisfies both: +the surviving row carries the flag and the guest path with `mount_path: ""`, so `isTarget[guestPath]` is +`true` **and** `:79` does not match, leaving R-114's `TargetAbsent` branch reachable. -| Group | Test | Pins | +**The join:** with the device gone the shared identity is **configuration**, not state — the storage's +`path` from `storage.cfg` (Observe side) and the `.mount` unit's `Where` (registry side) are the same +path, so both derive the same stable guest path. That is the key both sides can still compute. + +**What (d)'s dedup removes in the absent state**, enumerated: the registry row's `uuid:` `durable_id`, its +hardcoded and **false** `state: "attached"`, its `role: user-data`, and its `total_bytes`/`used_bytes` +copied from the **root** filesystem. No consumer found — wipe/decommission require the drive present; +`firstOfferableDrive` (`:138`) already requires `MountPath != ""` **and** `Role == "user-data"`; `diskKey` +(`disk_health.go:130`) changes key transiently but the health check excludes UNKNOWN verdicts, so no false +degradation. **R-118's symptom disappears incidentally — R-118 is NOT fixed and stays open.** + +**Deliberately unchanged:** `Role` (the absent target still reads `system`), the `BoundUnderParent` +conjunction, and every wire field. + +## Files + +`internal/hub/report.go` (`ConfigPath`, **`json:"-"`** — that struct is a cross-repo contract pinned by +`testdata/host-report.golden.json` + `contract_test.go`'s key-set comparison, so a wire-visible field +would have needed a matching `felhom.eu/hub` change; nothing off-box needs the value) · +`internal/storage/observe.go` (set from `s.Path`) · `internal/localapi/disks.go` (ConfigPath fallback + +guest-path dedup) · `internal/localapi/disks_backup_target_row_test.go` (fixture corrected + 4 tests) · +`CHANGELOG.md`. + +`observe.go`'s change is **not** the fallthrough its `:176-183` comment forbids: that prohibition is about +resolving a *device or UUID* from the **containing** filesystem, which would hand back root's identity and +mis-target a DR re-attach. `s.Path` is the storage's own declaration, identifies nothing but itself, and is +used for no resolution. `MountPath` stays empty, which is the truth. + +## §2 reconfirmation + +| Cited | Line now | Holds | |---|---|---| -| A | `..._ObservePath_DeviceLossReadsAbsent` | the E-2d shape: bind survives, device gone ⇒ absent | -| A | `..._UnionPath_DeviceLossReadsAbsent` | same on the registry path — the shape E-2d actually detached | -| B | `..._HealthyReadsPresent` (observe+union) | a healthy bound drive reads PRESENT — no false absent | -| C | `..._BootWindowStillReadsAbsent` | raw mounted early, bind not yet ⇒ still absent (no regression) | -| D | `..._UnknownIsNotAbsent` | `devicePresent("")` is TRUE — unknown never means gone | -| wire | `..._WireFieldIsFalseOnDeviceLoss` | the ENCODED `bound_under_parent`, which is what crosses | +| `mountPath` assigned only on exactMount success | `observe.go:184-190` | **HOLDS** verbatim | +| empty `backingDevice` ⇒ `RoleSystem` | `role.go:180-181` | **HOLDS** verbatim | +| guest-path guard skipped for non-user-data | `disks.go:215-217` | **HOLDS** | +| `Known()` reads unit FILES, never the mount table | `registry_known.go:40-75`, `MountPath: spec.Where` at `:70` | **HOLDS** | +| `driveTargetByPath` keys only from non-empty fields | `intermediary.go:602-618` (`:611`, `:614`) | **HOLDS** | +| the dedup keys on `d.MountPath` at `:263` | `:263` is `if t.BackingDevice != ""`. Actual: `seen` built `:290-295`, dedup `:298` | **HOLDS; the line number in the spec is wrong** | +| "`seen[""]` is set, so the union row is not skipped" | **DOES NOT HOLD as stated** — `seen[d.MountPath] = true` is guarded by `if d.MountPath != ""`, so `seen` never gains `""`. The union row survives because `/mnt/` is *absent* from `seen`, not because `""` is present. Same outcome, different reason | corrected | -**Seam wiring:** every test drives `NewServer` → `GET /disks` through `srv.Handler()` → the real JSON. -Only the two lowest-level mount reads are injected (a unit test cannot create real mounts); nothing above -them is faked. The wire test asserts the encoded field rather than the struct, so a regression that -dropped the field from the wire would fail it. +## Tests — 845 → 849, suite green (`go build ./... && go vet ./... && go test ./...` rc=0) -**Consumer contract already pinned:** `TestPlanDriveGates` (controller) asserts `BoundUnderParent:false` -⇒ exactly one `Stop`, a healthy drive ⇒ zero actions, with an exact action count. Verified green on the -untouched controller tree, so no redundant case was added. +All PASS: `TestAbsentBackupTargetIsResolvableByGuestPath` (rewritten — real shape **and** the union loop), +`TestAbsentTargetAppearsExactlyOnce` (new, the join), `TestAbsentTargetKeepsR114DegradedSignal` (new, the +regression guard), `TestPresentTargetPayloadUnchanged` (new, fleet-wide parity), +`TestAbsentNonTargetKeepsItsRegistryRowAndNoFlag` (new, negative + over-suppression), +`TestAbsentTargetRowDoesNotRegisterPresence`, `TestCaseBLocalTargetGetsNoGuestPath`, +`TestSystemBackedTargetUnderMntGetsNoGuestPath`, `TestNonTargetDriveNeverCarriesTheFlag`. -## Red-proofs — all four went red with the expected text, all reverted +**Red-proofs — each mutation asserted to have landed before the run:** -| # | Mutation | Failure text produced | -|---|---|---| -| 1 | drop `&& devicePresent` from the **Observe** path | `BoundUnderParent reports present — the bind outlived the device (R-113). The controller's gate would emit no Stop action, so no alarm can fire.` | -| 2 | drop it from the **union** path | `union-path drive reports present — the bind outlived the device (R-113)` **and** `wire bound_under_parent = true (want false)` | -| 3 | make unknown mean absent (`return false`) | `devicePresent("") = false — an unanswerable question was reported as ABSENT` | -| 4 | drop the **bind** half (device only) | `boot window reports present before the bind landed — this regresses the reboot convergence…` | +1. v0.115.0's MountPath-only fallback → `isTarget[…] = FALSE … rows=2`. **The empirical proof v0.115.0 + was inert**, rather than an argument for it. +2. Drop the guest-path dedup → `the absent drive is carried by 2 rows, want exactly 1`. +3. Give the absent row a `MountPath` (what option (a) produces) → the R-114 guard fires. +4. Over-broad dedup (`gp != ""` without `seenGuest`) → the non-target drive loses its own row, and two + pre-existing R-113 union tests fail as well. -After the last revert `disks.go` was verified **byte-identical** to the pre-mutation file (`diff -q`). +**The corrected fixture is the headline test change.** `theAbsentTarget` supplied +`MountPath: "/mnt/mentes"`, which production never supplies, and `targetRowServer` left `DriveTargets` +**nil**, so the union loop never ran and the two-row shape was invisible to the entire suite. Both fixed — +that combination is why an inert fix shipped green. -## NOT LIVE-VALIDATED — awaiting Session C +## Process note -The leg not exercised: **device loss → gate `Stop` → `SetDisconnected` → `backup_target_absent` on the -wire.** Deployment proves the service starts, nothing more. +One red-proof was restored with `git checkout `, which discarded the whole implementation rather +than the one-line mutation. Re-applied and re-verified; later red-proofs restored from pristine copies +kept outside the repo. `git checkout` is not an undo for a mutation on a work-in-progress file. -⚠️ **Session C blocker to settle first:** a throwaway box installs the agent from the hub's Day-0 artifact -manifest, which currently vouches **0.113.0**. Unless 0.114.0 is published (`scripts/publish-agent.sh`) -and vouched, Session C's fresh box runs the agent **without this fix** and validates nothing for R-113. -This is R-111's trap in the same shape. +## NOT yet live-validated at the time of this commit -## Observations — filed, not fixed - -1. **The legacy raw branch carries the same defect.** `disks.go:257` hardcodes `State:"attached"` on the - union path, and `planDriveGates:229` uses `d.State == "attached"` as presence for legacy raw paths — so - a registry-only drive registered at its raw path can never read absent either. Phase 0 Q4; out of scope - per the spec. -2. **`intermediary.go:220-224` is now incomplete, not wrong.** Its claim (presence is the bind, not merely - `State==attached`) still holds; it no longer says the bind now also implies the device. That clause - belongs there — add it in Session B when the controller is legitimately open, rather than touching a - repo Phase 0 ruled out. -3. **R-114's second half is expected to resolve as a side effect.** `SetDisconnected` is called only from - the `Stop` branch, and `firstOfferableDrive` skips `sp.Disconnected` — so once the gate fires, the - vanished drive should stop being offered. **Not acted on, not verified**; Session B/C. +Publish + vouch, C5 pairing, discrimination, and the over-correction guard — the session's live half. +Outcome recorded in the audit under `felhom.eu/documentation/audits/`. diff --git a/internal/hub/report.go b/internal/hub/report.go index baca996..65ca10a 100644 --- a/internal/hub/report.go +++ b/internal/hub/report.go @@ -280,6 +280,16 @@ type StorageTarget struct { MountPath string `json:"mount_path"` // host mountpoint (dir/usb); "" for network/lvm BackingDevice string `json:"backing_device"` // resolved block device (e.g. /dev/sdb1); "" for network + // ConfigPath is the storage's CONFIGURED path from storage.cfg (proxmox.Storage.Path) — not a + // resolved mount. It is the only identity a dir storage keeps when its device is gone: MountPath + // and BackingDevice both empty out (observe.go's exactMount block) and DurableID degrades off the + // fs-UUID, so the configured path is what still says WHICH drive this row is about (R-116). + // + // `json:"-"` DELIBERATELY. This struct is a cross-repo contract duplicated in felhom.eu/hub and + // pinned by testdata/host-report.golden.json + contract_test.go's key-set comparison; a wire-visible + // field here would need a matching change in the other repo to stay non-drifting. Nothing off-box + // needs this value — its only consumer is the agent's own /disks construction, in-process. + ConfigPath string `json:"-"` // ClassHint is a fast|slow HINT derived from the backing disk's rotational flag — a // hint only; the authoritative class is hub-owned (locked decision). "" when not // derivable (network targets have no local rotational flag). diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index cd67bb5..ddf8d47 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -255,8 +255,24 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { // inside the two guest-path blocks a system-role row never enters, so it stays false, and // planDriveGates computes present[gp] = present[gp] || d.BoundUnderParent. Inert by construction // — pinned by TestAbsentTargetRowDoesNotRegisterPresence. + // + // v0.116.0 — WHY v0.115.0 (the MountPath-only form) WAS INERT, measured not reasoned. In the + // absent state t.MountPath is ALSO "" — the same exactMount failure that emptied BackingDevice + // empties it — so StablePathForRaw("") returned "" and this assigned nothing. Captured payload: + // felhom.eu audits/DIAG-r116-disks-payload-2026-07-30.md §6.2. + // + // t.ConfigPath is the fix: the storage's CONFIGURED path from storage.cfg, which is configuration + // and therefore survives the device. MountPath is tried FIRST so the present-state path and + // v0.115.0's tested behaviour are byte-identical; ConfigPath is consulted only when the mount is + // genuinely gone. MountPath is deliberately NOT back-filled from ConfigPath — see the union-dedup + // note below for the consumer that would break, and because a path that is not mounted is not a + // "host mountpoint" (this field's own contract, :152-153). if di.GuestPath == "" && di.BackupTarget && t.BackingDevice == "" { - di.GuestPath = StablePathForRaw(t.MountPath) + if gp := StablePathForRaw(t.MountPath); gp != "" { + di.GuestPath = gp + } else { + di.GuestPath = StablePathForRaw(t.ConfigPath) + } } // Inspect the backing device for the UI's data-bearing hint (the authoritative check // is re-run at format time on the actual device). @@ -288,16 +304,43 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { // NOT duplicated, and no Observe row is dropped (so this can never regress the current view). if s.driveTargets != nil { seen := make(map[string]bool, len(out)) + // R-116: dedup ALSO by guest path. `seen` keys on MountPath, the one field the absent state + // empties, so with the device gone /mnt/ is absent from `seen` and the registry row was NOT + // skipped — /disks carried the drive TWICE, the Observe row holding BackupTarget with no key and + // the registry row holding both keys with BackupTarget defaulted false. driveTargetByPath + // (controller intermediary.go:602-618) assigns rather than ORs, and the registry row is appended + // LAST, so its false won on both keys. Measured, 4 rows vs 3: + // felhom.eu audits/DIAG-r116-disks-payload-2026-07-30.md §6.2. + // + // THE JOIN, and it is the whole point: with the device gone the two records of one drive share NO + // runtime field — no mount, no backing device, and the Observe row's DurableID has degraded off the + // fs-UUID. What they DO share is CONFIGURATION: the Observe row's storage path (storage.cfg) and the + // registry row's unit `Where` (the .mount unit) are the same path, so both derive the same stable + // guest path. That is the key both sides can still compute, which is why the dedup keys on it. + seenGuest := make(map[string]bool, len(out)) for _, d := range out { if d.MountPath != "" { seen[d.MountPath] = true } + if d.GuestPath != "" { + seenGuest[d.GuestPath] = true + } } if drives, derr := s.driveTargets.Known(r.Context()); derr == nil { for _, d := range drives { if d.MountPath == "" || seen[d.MountPath] { continue } + // Same drive as an Observe row that already carries this guest path — skip it. Suppressing + // it rather than teaching it BackupTarget is deliberate: the registry row has a non-empty + // MountPath (from the unit file, stale by then), and the controller reads + // `d.BackupTarget && d.MountPath != ""` as "a real drive with its own mountpoint — HEALTHY" + // (backup_target_offer.go:79). Putting the flag on a row with a stale MountPath would have + // silently regressed R-114, telling the customer the backup target is fine while its drive + // is missing. Pinned by TestAbsentTargetKeepsR114DegradedSignal. + if gp := StablePathForRaw(d.MountPath); gp != "" && seenGuest[gp] { + continue + } di := DiskInfo{ Name: d.Name, Type: d.Type, State: "attached", MountPath: d.MountPath, DurableID: d.DurableID, diff --git a/internal/localapi/disks_backup_target_row_test.go b/internal/localapi/disks_backup_target_row_test.go index 64b4338..224e938 100644 --- a/internal/localapi/disks_backup_target_row_test.go +++ b/internal/localapi/disks_backup_target_row_test.go @@ -32,9 +32,23 @@ import ( // under test here. func targetRowServer(t *testing.T, primaryTarget string, targets []hub.StorageTarget) *Server { t.Helper() + return targetRowServerWithDrives(t, primaryTarget, targets, nil) +} + +// targetRowServerWithDrives additionally wires the REGISTRY union source. v0.115.0's tests left +// DriveTargets nil, so the union loop never ran and the two-row absent shape — the actual defect — was +// invisible to the whole suite. Any test about which row carries what MUST populate this. +func targetRowServerWithDrives(t *testing.T, primaryTarget string, targets []hub.StorageTarget, + drives []storage.KnownTarget) *Server { + t.Helper() + var known storage.KnownTargets + if drives != nil { + known = fakeKnownTargets{drives: drives} + } srv, err := NewServer(Options{ - ListenAddr: "127.0.0.1:0", - Guests: &fakeGuestsCfg{}, Backups: &fakeBackups{}, Store: &fakeStore{}, + DriveTargets: known, + ListenAddr: "127.0.0.1:0", + Guests: &fakeGuestsCfg{}, Backups: &fakeBackups{}, Store: &fakeStore{}, Storage: fakeStorage{targets: targets}, // Service is REQUIRED: normalizeBackupTiers (backup_tiers.go:21-22) drops any tier with a nil // Service, and the legacy fallback then yields TargetID "" — which silently makes every @@ -86,11 +100,32 @@ func isTargetByPath(disks []map[string]any) map[string]bool { return out } -// theAbsentTarget is the Session-C shape: the felhom-backup storage whose device has gone, so Observe -// reports no backing device — which is what flips its role to system and drops its guest path. +// theAbsentTarget is the absent-target Observe row, CORRECTED in v0.116.0 to the shape the live box +// actually produces. +// +// THIS FIXTURE IS WHY AN INERT FIX SHIPPED GREEN. As written for v0.115.0 it supplied +// `MountPath: "/mnt/mentes"` — a field the real absent state does NOT have. The same exactMount failure +// that empties BackingDevice empties MountPath (observe.go:184-190), so on the live box this row carries +// `mount_path: ""`, and v0.115.0's `StablePathForRaw(t.MountPath)` was therefore +// `StablePathForRaw("")` == "". The fixture handed the code a value production never supplies, the test +// went green, and the fix was inert on real hardware — twice. +// +// Captured payload this now mirrors, field for field: +// felhom.eu audits/DIAG-r116-disks-payload-2026-07-30.md §6.2. var theAbsentTarget = hub.StorageTarget{ Name: "felhom-backup", Type: hub.StorageTypeLocalDir, - MountPath: "/mnt/mentes", BackingDevice: "", State: hub.StorageStateDisconnected, + MountPath: "", BackingDevice: "", ConfigPath: "/mnt/mentes", + State: hub.StorageStateDisconnected, + // DurableID degrades off the fs-UUID exactly as the live payload showed (`path:/mnt/cel` there). + DurableID: "path:/mnt/mentes", +} + +// theAbsentRegistryRow is the OTHER half of the live absent payload — the registry/union row. Its +// MountPath comes from the systemd .mount unit FILE (registry_known.go:40-75), which never consults the +// mount table, so it survives the device intact. Its presence is what made /disks carry the drive TWICE. +var theAbsentRegistryRow = []storage.KnownTarget{ + {Name: "9303-uuid", Type: hub.StorageTypeUSB, MountPath: "/mnt/mentes", + DurableID: "uuid:9303", UUID: "9303"}, } // ── the observable that must move ─────────────────────────────────────────────────────────────── @@ -98,7 +133,8 @@ var theAbsentTarget = hub.StorageTarget{ // RED-PROOF: delete the `di.GuestPath == "" && di.BackupTarget && t.BackingDevice == ""` block and // this fails with "the guest path the controller keys on is MISSING from /disks entirely". func TestAbsentBackupTargetIsResolvableByGuestPath(t *testing.T) { - disks := wireDisks(t, targetRowServer(t, "felhom-backup", []hub.StorageTarget{theAbsentTarget})) + disks := wireDisks(t, targetRowServerWithDrives(t, "felhom-backup", + []hub.StorageTarget{theAbsentTarget}, theAbsentRegistryRow)) isTarget := isTargetByPath(disks) const guestPath = "/mnt/felhom-drives/mentes" @@ -109,11 +145,9 @@ func TestAbsentBackupTargetIsResolvableByGuestPath(t *testing.T) { "can never fire (R-116)", guestPath) } if !got { - t.Errorf("isTarget[%q] = false; the row carrying the guest path does not carry the flag", guestPath) - } - // The host-path key was never the broken one — it must stay true. - if !isTarget["/mnt/mentes"] { - t.Error("isTarget by host path regressed to false") + t.Fatalf("isTarget[%q] = FALSE. Both rows for this drive reached the wire and the registry row — "+ + "appended last, BackupTarget defaulted false — overwrote the flag-bearing row's true. This is "+ + "the measured live defect, not a hypothetical: rows=%d", guestPath, len(disks)) } } @@ -192,3 +226,117 @@ func TestNonTargetDriveNeverCarriesTheFlag(t *testing.T) { } } } + +// ── v0.116.0 — the join, and the regression it must not cause ─────────────────────────────────── + +// THE JOIN. With the device gone the two records of one drive share no runtime field, so the dedup has +// to key on the one thing both can still derive: the CONFIGURED path (storage.cfg's `path` on the +// Observe side, the .mount unit's `Where` on the registry side), expressed as the stable guest path. +// This pins that exactly one row survives — because driveTargetByPath ASSIGNS rather than ORs, so two +// rows disagreeing on the flag is decided by append order, which is not a contract anyone should rely on. +// +// RED-PROOF: delete the `seenGuest[gp]` skip in the union loop and this fails with rows=2. +func TestAbsentTargetAppearsExactlyOnce(t *testing.T) { + disks := wireDisks(t, targetRowServerWithDrives(t, "felhom-backup", + []hub.StorageTarget{theAbsentTarget}, theAbsentRegistryRow)) + + const guestPath = "/mnt/felhom-drives/mentes" + var rows []map[string]any + for _, d := range disks { + if gp, _ := d["guest_path"].(string); gp == guestPath { + rows = append(rows, d) + } + } + if len(rows) != 1 { + t.Fatalf("the absent drive is carried by %d rows, want exactly 1 — with two rows the flag the "+ + "controller reads is decided by append order, not by the fix. rows=%v", len(rows), rows) + } + if bt, _ := rows[0]["backup_target"].(bool); !bt { + t.Error("the surviving row does not carry backup_target=true") + } +} + +// THE REGRESSION THIS FIX MUST NOT CAUSE, and the reason neither obvious option was taken. +// +// The controller reads `d.BackupTarget && d.MountPath != ""` as "a real drive with its own mountpoint — +// HEALTHY" and returns immediately (backup_target_offer.go:79). So the two candidate fixes that look +// smallest — back-filling MountPath onto the Observe row, or teaching the registry row the flag (its +// MountPath is non-empty, read from the stale unit file) — BOTH produce a row satisfying that predicate +// while the drive is missing. Either would have silently regressed R-114, which shipped 2026-07-29 and +// tells the customer „A rendszermentés meghajtója nem érhető el" in exactly this state, flipping it back +// to a false healthy. +// +// R-114's correctness currently rests on the absent-state rows NOT combining the flag with a mount path. +// That coupling was invisible until the payload was captured, and it is what this test pins. +// +// RED-PROOF: set `MountPath: "/mnt/mentes"` on theAbsentTarget (v0.115.0's fixture value) and this fails. +func TestAbsentTargetKeepsR114DegradedSignal(t *testing.T) { + disks := wireDisks(t, targetRowServerWithDrives(t, "felhom-backup", + []hub.StorageTarget{theAbsentTarget}, theAbsentRegistryRow)) + + for _, d := range disks { + bt, _ := d["backup_target"].(bool) + mp, _ := d["mount_path"].(string) + if bt && mp != "" { + t.Fatalf("row %v carries backup_target=true AND mount_path=%q while the drive is ABSENT. "+ + "resolveBackupTargetState (backup_target_offer.go:79) reads that as \"a real drive with "+ + "its own mountpoint — healthy\" and returns before its TargetAbsent branch, so the "+ + "customer is told the backup target is fine while its drive is gone. That is R-114, "+ + "regressed.", d["name"], mp) + } + } +} + +// PRESENT-STATE PARITY. The fix must change nothing when the drive is there. Present state is the +// state every healthy box is in, so a change here reaches the whole fleet; absent state reaches only a +// box with a problem. Both rows are supplied, exactly as on a live present box, and the pre-existing +// MountPath dedup must still collapse them to one COMPLETE row. +func TestPresentTargetPayloadUnchanged(t *testing.T) { + present := hub.StorageTarget{ + Name: "felhom-backup", Type: hub.StorageTypeLocalDir, + MountPath: "/mnt/mentes", BackingDevice: "/dev/sdb", ConfigPath: "/mnt/mentes", + State: hub.StorageStateAttached, DurableID: "uuid:9303", + } + disks := wireDisks(t, targetRowServerWithDrives(t, "felhom-backup", + []hub.StorageTarget{present}, theAbsentRegistryRow)) + + var rows []map[string]any + for _, d := range disks { + if d["name"] == "felhom-backup" || d["mount_path"] == "/mnt/mentes" { + rows = append(rows, d) + } + } + if len(rows) != 1 { + t.Fatalf("present state carries the drive on %d rows, want 1 (the MountPath dedup): %v", len(rows), rows) + } + r := rows[0] + for field, want := range map[string]any{ + "mount_path": "/mnt/mentes", "guest_path": "/mnt/felhom-drives/mentes", + "backing_device": "/dev/sdb", "role": "user-data", "state": "attached", + "backup_target": true, "bound_under_parent": true, "durable_id": "uuid:9303", + } { + if got := r[field]; got != want { + t.Errorf("present-state %s = %v, want %v — the fix altered the healthy payload", field, got, want) + } + } +} + +// The negative, with the union loop actually running: a non-target absent drive gains the flag on no row +// and keeps its own registry row (nothing to dedup against, since no Observe row claims its guest path). +func TestAbsentNonTargetKeepsItsRegistryRowAndNoFlag(t *testing.T) { + disks := wireDisks(t, targetRowServerWithDrives(t, "felhom-backup", + []hub.StorageTarget{{Name: "adat", Type: hub.StorageTypeLocalDir, MountPath: "", + BackingDevice: "", ConfigPath: "/mnt/adat", State: hub.StorageStateDisconnected}}, + []storage.KnownTarget{{Name: "adat-uuid", Type: hub.StorageTypeUSB, + MountPath: "/mnt/adat", DurableID: "uuid:1111", UUID: "1111"}})) + + isTarget := isTargetByPath(disks) + for k, v := range isTarget { + if v { + t.Errorf("isTarget[%q] = true for a NON-target drive — the BackupTarget gate failed", k) + } + } + if _, ok := isTarget["/mnt/felhom-drives/adat"]; !ok { + t.Error("the non-target drive lost its guest-path key entirely — the union row was over-suppressed") + } +} diff --git a/internal/storage/observe.go b/internal/storage/observe.go index e9d2589..b7b4bb1 100644 --- a/internal/storage/observe.go +++ b/internal/storage/observe.go @@ -235,9 +235,16 @@ func (o *Observer) build(s proxmox.Storage, mounts []Mount) observed { Content: s.Content, MountPath: mountPath, BackingDevice: backingDevice, - ClassHint: classHint, - Role: "", // hub-owned; not derivable from a Proxmox def (slice 10) - Smart: hub.SmartSummary{Health: hub.SmartUnknown}, + // R-116: the CONFIGURED path, carried verbatim and never resolved. This is emphatically NOT the + // fallthrough the comment above forbids — that prohibition is about resolving a device or a UUID + // from the CONTAINING filesystem when the target is not its own mount, which would hand back + // root's identity and mis-target a DR re-attach. `s.Path` is the storage's own declaration of + // where it lives; it identifies nothing but itself, and it is not used for device or UUID + // resolution anywhere. MountPath stays empty when the mount is gone, which is the truth. + ConfigPath: s.Path, + ClassHint: classHint, + Role: "", // hub-owned; not derivable from a Proxmox def (slice 10) + Smart: hub.SmartSummary{Health: hub.SmartUnknown}, } // Thin-pool DATA fill: surfaced prominently for lvmthin (metadata fill is Phase B/lvs).