diff --git a/CHANGELOG.md b/CHANGELOG.md index f689e8f..ef5bc88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,53 @@ +## 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** +`storage_disconnected`, while its return raised the **specific** `backup_target_restored` — an alarm +and an all-clear an operator cannot pair. `backup_target_absent` never fired at all. + +**The mechanism, and it is not what the Session-C audit first said.** `RoleForStorage` returns +`RoleSystem` whenever `backingDevice == ""` (`internal/storage/role.go:180-181`). When the device goes, +Observe's `exactMountDevice` fails, `t.BackingDevice` becomes `""`, the target row's role flips to +system and it **loses its guest path** — but keeps its `MountPath`. The union loop skips any drive whose +`MountPath` is already `seen`, so the registry row is **deduped away entirely**. `/disks` ends up with +**no row carrying that guest path**, so the controller's `isTarget[guestPath]` is a **missing key**, not +a `false`. The obvious fix — setting `BackupTarget` on the union row — **could not have worked**: that +row is not emitted in the state where the alarm is needed. The audit has been corrected. + +**The fix.** On the Observe row only, carry the guest path when the row **is** the backup target **and** +its role flipped because the device vanished: + +```go +if di.GuestPath == "" && di.BackupTarget && t.BackingDevice == "" { + di.GuestPath = StablePathForRaw(t.MountPath) +} +``` + +**Three gates, each verified rather than assumed:** + +- `t.BackingDevice == ""` restricts this to the vanished-device flip. A storage that is `RoleSystem` + because it is genuinely system-**backed** has a real device and is excluded — otherwise a dir storage + at `/mnt/` on the root disk would acquire a guest path. +- **Case B, the common fresh-box shape, is safe twice over.** Its target is the builtin `local` on + `/var/lib/vz`, and `StablePathForRaw` returns `""` for anything that is not exactly `/mnt/` + (`DriveNameFromRaw`, `intermediary.go:79-88`) — nothing is set even before the gates apply. +- **It cannot make the gate read an absent drive as PRESENT.** `BoundUnderParent` is assigned at exactly + two sites (`disks.go:222`, `:280`), both inside guest-path blocks a system-role row never enters, so + it stays `false` and `planDriveGates` computes `present[gp] = false || false`. Inert by construction — + pinned by `TestAbsentTargetRowDoesNotRegisterPresence`. Getting this wrong would have **silenced the + alarm this fix exists to raise**. + +**The `:213-214` boundary stands.** No system or backup mount gains a guest path; only the drive the +alarm is *about* keeps its identity while it is missing, and only while its device is gone. + +**Tests** +5 in `internal/localapi`, asserting the emitted `/disks` JSON through a faithful copy of the +controller's `driveTargetByPath`, because the failure class is "the value is on the wrong row" and a +hand-built fixture proves nothing about which row the handler emits. Red-proof: removing the block +fails with *"isTarget[…] is a MISSING KEY"*; reverted, byte-identical. + +**Known limitation, filed not closed:** the two-row shape that produced this survives. The flag and the +guest path still live on different rows in the healthy state, and nothing prevents a future consumer +keying on the wrong one. + ## v0.114.0 — R-113: drive presence means the DEVICE, not the bind (2026-07-29) **The bug, measured live in E-2d.** `BoundUnderParent` — the one field the controller's drive-absent diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index a019ab6..cd67bb5 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -223,6 +223,41 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { s.devicePresent(t.MountPath) } } + // R-116: carry the GUEST PATH on the backup-target row even when its role has flipped to + // system — but ONLY when that flip was caused by the device vanishing. + // + // WHY. The controller keys the drive-absent alarm on the registered StoragePath, which for an + // external drive is the GUEST path. When the device goes, Observe's exactMountDevice fails, so + // t.BackingDevice becomes "" and RoleForStorage returns RoleSystem (role.go:180-181) — the block + // above is skipped and this row loses its guest path. It keeps its MountPath, so the union loop + // below DEDUPES the registry row away (`seen[d.MountPath]`), and /disks ends up carrying NO row + // with that guest path at all. driveTargetByPath then has no entry, isTarget[guestPath] is a + // missing key, and the specific backup_target_absent alarm cannot fire — the generic one goes + // out instead, while the RETURN (rows rejoined) fires the specific recovery. An unmatchable + // pair. Measured live: felhom.eu audits/SESSION-C-2026-07-29.md §5. + // + // THE GATES, each load-bearing: + // di.GuestPath == "" — never touch the user-data path above; this is a fallback, not a rule. + // di.BackupTarget — only the target row. No other system/backup mount gains a guest path, + // so the boundary at :213-214 stands: this is not "system mounts now + // cross into the guest", it is "the drive the alarm is about keeps its + // identity while it is missing". + // t.BackingDevice == "" — ONLY the vanished-device flip. A storage that is RoleSystem because + // it is genuinely system-BACKED has a non-empty BackingDevice and is + // excluded. Without this gate a dir storage at /mnt/ living on the + // root disk would acquire a guest path. + // + // Case B (the COMMON fresh-box shape) is safe twice over: the target is the builtin `local` on + // /var/lib/vz, and StablePathForRaw returns "" for anything that is not exactly /mnt/ + // (DriveNameFromRaw, intermediary.go:79-88), so nothing is set even before the gates apply. + // + // This cannot make the gate read an absent drive as PRESENT: BoundUnderParent is assigned only + // 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. + if di.GuestPath == "" && di.BackupTarget && t.BackingDevice == "" { + di.GuestPath = StablePathForRaw(t.MountPath) + } // Inspect the backing device for the UI's data-bearing hint (the authoritative check // is re-run at format time on the actual device). if t.BackingDevice != "" { diff --git a/internal/localapi/disks_backup_target_row_test.go b/internal/localapi/disks_backup_target_row_test.go new file mode 100644 index 0000000..64b4338 --- /dev/null +++ b/internal/localapi/disks_backup_target_row_test.go @@ -0,0 +1,194 @@ +package localapi + +import ( + "context" + "encoding/json" + "io" + "log/slog" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// R-116 — the backup-target flag must be reachable from the row the CONTROLLER keys on. +// +// THE DEFECT. The controller resolves the drive-absent alarm by the registered StoragePath, which for +// an external drive is the GUEST path. When the device vanishes, Observe's exactMountDevice fails, so +// BackingDevice becomes "" and RoleForStorage returns RoleSystem (role.go:180-181) — the guest-path +// block is skipped and the flag-bearing row loses its guest path. It keeps its MountPath, so the union +// loop DEDUPES the registry row away, and /disks carries NO row with that guest path at all. +// driveTargetByPath then has no entry, isTarget[guestPath] is a MISSING KEY, and the specific +// backup_target_absent alarm cannot fire — the generic storage_disconnected goes out instead, while +// the RETURN (rows rejoined) fires the specific recovery. An operator gets a pair they cannot match. +// Measured live: felhom.eu audits/SESSION-C-2026-07-29.md §5. +// +// These tests exercise the REAL GET /disks response and assert the emitted JSON, because the failure +// class is "the value is on the wrong row" — a test that hand-builds rows proves nothing about which +// row the handler actually emits. + +// targetRowServer builds a /disks server whose primary backup tier is `primaryTarget`, over the given +// Observe targets. boundCheck/deviceCheck are pinned so the R-113 conjunction is not the variable +// under test here. +func targetRowServer(t *testing.T, primaryTarget string, targets []hub.StorageTarget) *Server { + t.Helper() + srv, err := NewServer(Options{ + 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 + // BackupTarget false and would make these tests pass for the wrong reason. + BackupTiers: []BackupTier{{TargetID: primaryTarget, Primary: true, Service: &fakeBackups{}}}, + Tokens: staticTokens{"A": 8200}, + Disks: &fakeDiskOps{probe: storage.DeviceProbe{Probed: true, HasFilesystem: true, FSType: "ext4"}}, + DiskGate: &fakeGate{}, HostReader: sysOnSDA(), + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatal(err) + } + srv.baseCtx = context.Background() + srv.boundCheck = func(string) bool { return true } + srv.deviceCheck = func(string) bool { return true } + return srv +} + +// wireDisks returns the decoded /disks rows exactly as the controller receives them. +func wireDisks(t *testing.T, srv *Server) []map[string]any { + t.Helper() + body := do(t, srv.Handler(), "GET", "/disks", "A", "").Body.Bytes() + var w struct { + Data struct { + Disks []map[string]any `json:"disks"` + } `json:"data"` + } + if err := json.Unmarshal(body, &w); err != nil { + t.Fatalf("decode /disks: %v (%s)", err, body) + } + return w.Data.Disks +} + +// isTargetByPath reproduces the controller's driveTargetByPath EXACTLY (intermediary.go:602-616): +// both keyings, value = backup_target. This is the map whose missing key is the whole defect, so the +// assertion is made against a faithful copy of it rather than against a field in isolation. +func isTargetByPath(disks []map[string]any) map[string]bool { + out := map[string]bool{} + for _, d := range disks { + bt, _ := d["backup_target"].(bool) + if gp, ok := d["guest_path"].(string); ok && gp != "" { + out[gp] = bt + } + if mp, ok := d["mount_path"].(string); ok && mp != "" { + out[mp] = bt + } + } + 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. +var theAbsentTarget = hub.StorageTarget{ + Name: "felhom-backup", Type: hub.StorageTypeLocalDir, + MountPath: "/mnt/mentes", BackingDevice: "", State: hub.StorageStateDisconnected, +} + +// ── the observable that must move ─────────────────────────────────────────────────────────────── + +// 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})) + isTarget := isTargetByPath(disks) + + const guestPath = "/mnt/felhom-drives/mentes" + got, present := isTarget[guestPath] + if !present { + t.Fatalf("isTarget[%q] is a MISSING KEY — the guest path the controller keys on is missing from "+ + "/disks entirely, so notifyDriveAbsent takes the generic branch and backup_target_absent "+ + "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") + } +} + +// ── V2: the new guest path must NOT make the gate read the drive as PRESENT ───────────────────── + +// This is the over-correction guard, in the exact component under test. planDriveGates computes +// present[gp] = present[gp] || d.BoundUnderParent. If the row we now emit carried a true +// BoundUnderParent, this fix would SILENCE the alarm it exists to raise. +func TestAbsentTargetRowDoesNotRegisterPresence(t *testing.T) { + srv := targetRowServer(t, "felhom-backup", []hub.StorageTarget{theAbsentTarget}) + // deviceCheck/boundCheck are pinned TRUE — the strongest possible case for a false positive. + // The row must still report bound_under_parent=false, because that field is only ever assigned + // inside the guest-path blocks a system-role row does not enter. + for _, d := range wireDisks(t, srv) { + if d["guest_path"] != "/mnt/felhom-drives/mentes" { + continue + } + if bup, _ := d["bound_under_parent"].(bool); bup { + t.Fatal("the absent backup-target row reports bound_under_parent=true — planDriveGates " + + "would compute present=true, the Stop branch would never run, and this fix would " + + "SUPPRESS the very alarm it exists to raise") + } + return + } + t.Fatal("the absent target row never reached the wire") +} + +// ── V1: the gates, each on its own ────────────────────────────────────────────────────────────── + +// Case B is the COMMON fresh-box shape, not an edge: the tier target is the builtin `local` on the +// root fs. It must never acquire a guest path. +func TestCaseBLocalTargetGetsNoGuestPath(t *testing.T) { + disks := wireDisks(t, targetRowServer(t, "local", []hub.StorageTarget{ + {Name: "local", Type: "local", MountPath: "/var/lib/vz", BackingDevice: "", State: hub.StorageStateAttached}, + })) + for _, d := range disks { + if gp, _ := d["guest_path"].(string); gp != "" { + t.Errorf("the Case B target on %v acquired guest path %q — a system-drive backup target "+ + "must not cross into the guest", d["mount_path"], gp) + } + } +} + +// A storage that is RoleSystem because it is genuinely system-BACKED (non-empty BackingDevice on the +// system disk) must be excluded — this is the case StablePathForRaw would NOT have filtered, since +// /mnt/ maps to a real stable path. The BackingDevice gate is what stops it. +func TestSystemBackedTargetUnderMntGetsNoGuestPath(t *testing.T) { + disks := wireDisks(t, targetRowServer(t, "sysbackup", []hub.StorageTarget{ + // sysOnSDA() makes /dev/sda the system disk, so this classifies RoleSystem with a REAL device. + {Name: "sysbackup", Type: hub.StorageTypeLocalDir, MountPath: "/mnt/sysbackup", + BackingDevice: "/dev/sda1", State: hub.StorageStateAttached}, + })) + for _, d := range disks { + if gp, _ := d["guest_path"].(string); gp != "" { + t.Errorf("a system-BACKED backup target acquired guest path %q — the BackingDevice gate "+ + "failed and the :213-214 boundary was widened", gp) + } + } +} + +// ── the negative ──────────────────────────────────────────────────────────────────────────────── + +// A drive that is NOT the target must not acquire the flag on any row, present or absent. +func TestNonTargetDriveNeverCarriesTheFlag(t *testing.T) { + disks := wireDisks(t, targetRowServer(t, "felhom-backup", []hub.StorageTarget{ + {Name: "adat", Type: hub.StorageTypeLocalDir, MountPath: "/mnt/adat", + BackingDevice: "", State: hub.StorageStateDisconnected}, + })) + for _, d := range disks { + if bt, _ := d["backup_target"].(bool); bt { + t.Errorf("non-target drive %v reports backup_target=true", d["name"]) + } + if gp, _ := d["guest_path"].(string); gp != "" { + t.Errorf("an absent NON-target drive acquired guest path %q via the R-116 fallback — the "+ + "BackupTarget gate failed", gp) + } + } +}