diff --git a/CHANGELOG.md b/CHANGELOG.md index c875476..6e0ad3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,66 @@ +## v0.117.0 — R-117: the liveness signal now tests liveness (2026-07-30) + +**`BoundUnderParent` reported a namespace that returned `EIO` on every read and write as healthy, and the +gate restarted the customer's apps onto it.** Both existing terms — `GuestSeesMount` and +`isHostMountpoint` — parse a mountinfo line and then test only `fields[4]`, the mount POINT. Field 3, the +`major:minor`, sat in the same parsed slice and was discarded. So after a drive was detached and returned, +the raw host mount healed onto the NEW device via its fs-UUID-keyed unit while the bind still named the +OLD one, and both terms stayed true. Measured on hardware: raw `8:32 /dev/sdc`, bind `8:16 /dev/sdb` with +`shutdown`, `bound_under_parent: true`, `EIO` both directions — and the controller taking its `Return` +branch, restarting the gate-stopped apps and emailing `backup_target_restored`, with no alarm on any +channel (`felhom.eu/documentation/audits/SPIKE-r117-bind-liveness-2026-07-30.md`). + +**THE FIX is one predicate.** `BoundUnderParent` gains a third term, `bindUsable`, at both /disks +construction sites (`disks.go`). The new `bindLiveness` (`intermediary.go`) reads `/proc` only and asks +two questions: + +- **the bind must name the same device as the raw mount** — sound rather than heuristic, because a stale + bind pins the dead superblock, which keeps the old device index allocated, which FORCES the returning + device onto a different number (measured both ways: releasing the bind let the letter be reused); +- **the filesystem must not have aborted** — ext4's `shutdown` (device removed) or `emergency_ro` + (`errors=remount-ro` fired in place). Both tokens were measured and both are load-bearing. + +**The second check is not optional, and this is the substantive part.** R-117 was filed as a +detach/return defect, but a device that fails WITHOUT disappearing produces the identical +all-signals-healthy state with the devnos EQUAL and the drive never `Disconnected` — so the gate produces +neither a `Stop` nor a `Return` action and **nothing is emitted on any channel, indefinitely** (R-117a). +The device comparison alone cannot see it. A devno-only fix would have passed every payload test. + +**THREE states, never a bool.** `BindLiveness` is `{Unknown, Live, StaleDevice, Aborted}` and every +caller reads it through `Usable()`, where `Unknown` counts as PRESENT — unreadable `/proc`, no raw mount +to compare against, or a filesystem whose abort vocabulary we have not measured all degrade to unknown, +because reporting absent stops a working customer's apps. The workspace's false-invariant table records +`newestArchiveOn` promising exactly this over a `(value, bool)` shape that could not express it. + +**NO NEW RECOVERY PATH — the existing one was unblocked.** `AttachDrive`'s normalize leg already performed +the needed repair, and three call sites already invoked it: the 20 s reconcile ticker +(`cmd/felhom-agent/main.go`), agent startup, and **the controller's `Return` branch, before it restarts +the apps**. All three were defeated by `if n == 1 && b.GuestSeesMount(...)` returning early and logging +*"fully live, no-op"* about a dead namespace. The arm now switches on the verdict: +**`BindStaleDevice` ⇒ re-bind** (repairs live, guest never restarts — proven, init PID identical); +**`BindAborted` ⇒ quiet no-op**, because a re-bind lands on the SAME aborted superblock and this runs +every 20 s, so re-binding would be an infinite silent retry that also masks the state — it surfaces via +`BoundUnderParent=false` instead, and clearing an aborted filesystem needs a remount or a fsck, which is +an operator decision. **Ordering trap, caught by a test:** reading the abort flag before comparing devices +classifies the real return state as aborted (its stale bind carries `shutdown` too) and refuses the repair +while still reporting correctly — so the abort flag is read off the RAW mount in the stale case. + +**Also:** `isHostMountpoint` and `countHostMounts` are now one-liners over the single new parser +`hostMountEntries`, which yields devno, root, fstype and super options (REUSE.md's +three-independent-readers note narrowed). The three comments that promised `BoundUnderParent` meant +"live + usable in the guest" — true for three releases only as a wish — now name the tests that pin them. +`CLAUDE.md` gained a standing rule: **a health check issues no block I/O** (a probe on a wedged device +survives `SIGKILL`; measured). + +Tests **849 → 863** (+14 top-level, verified against a temp worktree at HEAD — not inferred from a count taken mid-edit, which is how the first draft of this line read 858 → 873), all green: 29/29 packages, `go build`/`go vet`/`go test` each with `rc=0` read separately. **6 red-proofs, each verified to have landed**: term 3 removed at +each construction site; the abort check neutered (the P1-only fix — fails the aborted tests ALONE); +`emergency_ro` dropped from the token list; unknown reported as not-live; and the aborted arm falling +through to a re-bind. **A hollow test was caught and fixed during the red-proofs**: the aborted fixture +first used a `/dev/mapper` device, for which `RoleForStorage` derives `role="system"` — a system row has +no `GuestPath`, never runs the conjunction, and reports `false` by default, so the assertion passed +vacuously and NO mutation could fail it. Found because RP1 failed to fail. The fixtures now assert the +production row shape before asserting the field. + ## 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 diff --git a/REUSE.md b/REUSE.md index 8939651..607e698 100644 --- a/REUSE.md +++ b/REUSE.md @@ -110,8 +110,10 @@ | Anti-retarget durable-id binding | internal/localapi/wipe_reresolve.go | resolve id → re-derive + exact match → re-inspect expected state → act on RE-RESOLVED device only | | Atomic single-file JSON store | internal/storage/intent.go | `Open*` loads (missing=empty, corrupt=fail-loud), mutex, tmp+rename 0600, idempotent set | | Durable append-only log + index | internal/authz/noncestore.go (`FileNonceStore`) | fsync before returning "new"; replay into index on open; expiry-only compaction | -| Injectable seam funcs on Server | internal/localapi/server.go (`reresolveWipe`, `deviceDurableID`, `boundCheck`, `deviceCheck`, net-verify: `netTrigger`/`netMounted`/`netJournal`/`netReachable`) | prod default wired in `NewServer`; tests override — no real /dev, /proc/mounts, journalctl or TCP in tests | +| Injectable seam funcs on Server | internal/localapi/server.go (`reresolveWipe`, `deviceDurableID`, `boundCheck`, `deviceCheck`, `livenessCheck`, net-verify: `netTrigger`/`netMounted`/`netJournal`/`netReachable`) | prod default wired in `NewServer`; tests override — no real /dev, /proc/mounts, journalctl or TCP in tests. **For mount-table predicates prefer the DATA seams `procSelfMountinfo` / `procGuestMountinfo` (internal/localapi/intermediary.go) over `boundCheck`/`livenessCheck`**: pointing them at a captured fixture runs the real parser, the real predicate and the real handler, so the test cannot go hollow the way R-116's did | | `Server.devicePresent` (R-113, v0.114.0) | internal/localapi/disks.go | `devicePresent(rawMountPath) bool`; seam `deviceCheck`, default `isHostMountpoint` | the agent's DEVICE-presence signal — asks whether the drive's RAW mount is still mounted | **Use this, never the bind, to answer "is the drive there".** The raw mount is a device-bound systemd unit and dies with its device; the agent's own bind under the shared parent is NOT device-bound and outlives it as a stale shell. `BoundUnderParent` is now `boundUnderParent(...) && devicePresent(...)` at BOTH /disks construction sites — dropping either half is a regression with its own red-proof. Empty path ⇒ **true** (unknown is never absent: absent stops a customer's apps) | +| `bindLiveness` + `BindLiveness` (R-117, v0.117.0) | internal/localapi/intermediary.go | `bindLiveness(stable, raw) BindLiveness`; seam `livenessCheck`; read verdicts ONLY via `.Usable()` | the agent's bind-LIVENESS signal — the third term of `BoundUnderParent` | **`devicePresent` and `boundUnderParent` are both PATH-PRESENCE tests and neither is liveness.** They compare only mountinfo field 5, so both stay true over a bind that names the drive that went away while the raw mount healed onto the returning one (measured: raw 8:32 /dev/sdc, bind 8:16 /dev/sdb `shutdown`, EIO both ways, payload healthy). Two dead states, and a fix needs BOTH checks: devno mismatch (the detach/return case) AND the ext4 abort tokens `shutdown`/`emergency_ro` (the steady-state case, where the devnos AGREE because the device never left). **THREE states, never a bool** — `BindUnknown` must exist and `Usable()` treats it as PRESENT (absent stops a customer's apps). **Order matters:** compare devices first and read the abort flag off the RAW mount in the stale case — abort-first classifies the real return state as aborted and refuses the re-bind that repairs it. **NO BLOCK I/O, ever** (CLAUDE.md rule; a probe on a wedged device survives SIGKILL). 6 red-proofs | +| `AttachDrive` repair ruling (R-117, v0.117.0) | internal/localapi/intermediary.go | the `switch bindLiveness(...)` inside the `n == 1 && GuestSeesMount` arm | decides whether the existing self-heal runs | `BindStaleDevice` ⇒ **re-bind** (the raw mount is a healthy new superblock; repairs live, no guest restart). `BindAborted` ⇒ **quiet no-op** — a re-bind lands on the SAME dead superblock and this runs every 20 s, so re-binding is an infinite silent retry that also masks the state; it must surface via `BoundUnderParent=false`. `BindLive`/`BindUnknown` ⇒ no-op, unchanged. **Do not return an error for the aborted case** — the reconcile loop would log a failure every 20 s | | Detached IN-MEMORY verify job (single slot, deliberately unpersisted) | internal/localapi/netverifyjob.go | claim slot sync (single-flight 409) → detached pipeline off baseCtx → auto-rollback on fail; restart ⇒ slot empty ⇒ the CALLER rolls back (Scenario F) — contrast formatjob (persisted+recovered) | | Optional dependency degradation | internal/localapi/server.go (`Options`) | nil dep ⇒ endpoint answers "not configured" (503), never a crash | | Version channel (v0.82.0) | internal/localapi/server.go (`Options.AgentVersion`; `Handler()` mux wrap) | sets `X-Felhom-Agent-Version` on EVERY response (all routes/statuses, incl. auth-fail/404) — the controller's capability-comparison source; empty version ⇒ header omitted | @@ -183,6 +185,6 @@ - **SMART device resolution (v0.95.0):** `smartDeviceFor` (internal/storage/observe.go) resolves partition→disk AND dm/LVM→disk (`dmWholeDisk` in internal/storage/smartdev.go, via `/sys/block//slaves`, `sysBlockRoot` test seam). `storage.SmartReader.SMARTForBacking` is the shared read the localapi `/disks` union path uses (Fix B) — do NOT re-implement smartctl parsing. The builtin-`local` SMART device comes from `containingMountDevice` (SMART-only; never feeds backing/durable_id). - Atomic tmp+rename JSON store implemented 3×: `IntentStore.saveLocked` (internal/storage/intent.go), `FormatJobStore.save` (internal/localapi/formatjob.go), `GuestBindStore.saveLocked` (internal/localapi/guestbindstore.go) — comments say "mirrors", no shared helper. - `run(ctx, name, args...) error` stderr-wrapping helper duplicated 4×: `SudoHostOps.run`, `Privileged.run`, `BackHalf.run` (internal/provision/backhalf.go), `GuestBinder.run` (internal/localapi/guestbind.go). -- Several independent /proc mount-table readers: `SudoHostOps.mountedSet` (internal/storage/hostops.go), `ProcHostReader.Mounts` (internal/storage/hostread.go), `isHostMountpoint` + `countHostMounts` (internal/localapi/intermediary.go). +- Several independent /proc mount-table readers: `SudoHostOps.mountedSet` (internal/storage/hostops.go), `ProcHostReader.Mounts` (internal/storage/hostread.go). **In localapi they were unified in v0.117.0**: `isHostMountpoint` and `countHostMounts` are now one-liners over `hostMountEntries`, the single parser that also yields devno/fstype/super-options for `bindLiveness`. - Deliberate mirror: `antiRetargetResolveExpect` (internal/localapi/wipe_reresolve.go) duplicates `WipeExecutor.Execute` steps 1–3 (internal/signedjobs/wipe.go) across packages. - `stableParentDir` literal duplicated in internal/provision/backhalf.go to avoid a provision→localapi import edge (commented as intentional); `trim` (internal/storage/hostops.go) vs `trimBody` (internal/proxmox/errors.go) output-truncation twins. diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index ddf8d47..969521c 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -79,8 +79,10 @@ type GuestAttacher interface { DetachDrive(ctx context.Context, where string) error // EnsureSharedParent makes the host stable parent shared + installs the boot-persistence unit. EnsureSharedParent(ctx context.Context) error - // GuestSeesMount reports whether vmid's guest sees `path` as a mount in its own namespace (the - // guest-usable signal — distinct from the host having the bind). Backs BoundUnderParent. + // GuestSeesMount reports whether vmid's guest sees `path` as a mount in its own namespace — the + // guest-VISIBILITY signal, distinct from the host having the bind. One of the three terms behind + // BoundUnderParent. It is a path-presence test and NOT a liveness signal (R-117): a stale bind over a + // dead device is still "seen". Liveness is bindLiveness's job. GuestSeesMount(ctx context.Context, vmid int, path string) bool // GuestBootID returns a token that changes on every guest boot (host or guest) but is stable across a // controller-only restart — the deterministic guest-reboot signal the controller recreates apps on. @@ -155,14 +157,30 @@ type DiskInfo struct { // BoundUnderParent reports whether the drive is live + usable in the guest in the intermediary model. // The controller's drive-absent gate + auto-restart key on this (and State). // - // It is a CONJUNCTION of two facts, and both halves are load-bearing (R-113, v0.114.0): + // It is a CONJUNCTION of THREE facts, and all three are load-bearing: // 1. felhom-data is bound under the shared parent at GuestPath (the guest-visible mount check), and - // 2. the drive's RAW host mount is still mounted — i.e. the DEVICE is still there. - // Half 1 alone was the bug: the raw mount is device-bound and dies with its device, but the agent's - // own bind is not, so half 1 stays true over a stale shell after the device is pulled. The controller - // read that survivor as "present" and the drive-absent alarm could never fire — measured live in E-2d - // (felhom.eu audits/E2D-fresh-vm-2026-07-29.md §5.2). Half 2 alone would regress boot ordering, where - // the raw mounts early and the bind lands ~18s later; the conjunction keeps that window reading absent. + // 2. the drive's RAW host mount is still mounted — i.e. the DEVICE is still there (R-113), and + // 3. the bind actually WORKS: it names the same device as the raw mount, and that filesystem has + // not aborted (R-117, v0.117.0 — see bindLiveness). + // Half 1 alone was the bug R-113 fixed: the raw mount is device-bound and dies with its device, but + // the agent's own bind is not, so half 1 stays true over a stale shell after the device is pulled. The + // controller read that survivor as "present" and the drive-absent alarm could never fire — measured + // live in E-2d (felhom.eu audits/E2D-fresh-vm-2026-07-29.md §5.2). Half 2 alone would regress boot + // ordering, where the raw mounts early and the bind lands ~18s later; the conjunction keeps that + // window reading absent. + // + // Terms 1 and 2 TOGETHER were still not liveness, which is R-117: both are path-presence tests + // comparing only field 5 of a mountinfo line, so both stay true over a bind that names the drive that + // went away while the raw mount healed onto the returning one via its fs-UUID-keyed unit. Measured + // live: raw on 8:32 /dev/sdc, bind on 8:16 /dev/sdb with `shutdown`, this field TRUE, EIO on every + // read and write, and the gate restarting the customer's apps onto it with no alarm on any channel + // (felhom.eu audits/SPIKE-r117-bind-liveness-2026-07-30.md §5.2). + // + // THE TESTS THAT PIN THIS COMMENT, because for three releases it promised a property nothing tested + // (spike §5.3): disks_bind_liveness_test.go — TestDisks_BindLiveness_StaleBindReadsAbsent (term 3, + // case a), _AbortedFilesystemReadsAbsent (term 3, case b, the steady-state case that emits nothing + // today), _UnknownIsTreatedAsPresent (the cannot-tell rule) and _HealthyReadsPresent (no false + // negative). Each asserts the CONSEQUENCE — what this field reads — not the mechanism. BoundUnderParent bool `json:"bound_under_parent"` // Smart is the already-computed per-disk SMART health summary (v0.94.0), serialized here so the // controller can render a disk-health card + degradation alert WITHOUT any new smartctl load — the @@ -219,8 +237,12 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { // boot-ordering behaviour the controller's gate depends on exactly as it was // (raw mounted early, bind not yet ⇒ still absent) while closing the case the // gate could never see (bind outlived the device ⇒ now absent). + // R-117: AND in bind LIVENESS. The two terms above are both path-presence tests, so + // both stay true over a bind that names the drive that went away while the raw mount + // healed onto the returning one — EIO on every call, payload healthy. di.BoundUnderParent = s.boundUnderParent(r.Context(), vmid, gp) && - s.devicePresent(t.MountPath) + s.devicePresent(t.MountPath) && + s.bindUsable(gp, t.MountPath) } } // R-116: carry the GUEST PATH on the backup-target row even when its role has flipped to @@ -351,12 +373,13 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { // exactly like the Observe path — else the controller reads a registry drive as "Leválasztva". if gp := StablePathForRaw(d.MountPath); gp != "" { di.GuestPath = gp - // R-113: same conjunction as the Observe path. This path matters MORE, not less — - // a registry drive with no PVE dir-storage is exactly the shape E-2d detached, and - // its State is hardcoded "attached" below, so the raw-mount check is the only - // device truth this row carries. + // R-113 + R-117: same three-term conjunction as the Observe path. This path matters + // MORE, not less — a registry drive with no PVE dir-storage is exactly the shape + // E-2d detached, and its State is hardcoded "attached" below, so these checks are + // the only device truth this row carries. di.BoundUnderParent = s.boundUnderParent(r.Context(), vmid, gp) && - s.devicePresent(d.MountPath) + s.devicePresent(d.MountPath) && + s.bindUsable(gp, d.MountPath) } // A registry drive has no PVE `pvesm status` snapshot, so fill backing device + capacity // from the host directly: resolve the device by fs-UUID, and statfs the mount for size — @@ -943,9 +966,12 @@ func (s *Server) handleDiskFormat(w http.ResponseWriter, r *http.Request, vmid i } // boundUnderParent reports whether a drive's felhom-data is bound at its stable guest path AND visible -// inside the guest (the usable-in-guest signal the controller's gate keys on — a guest reboot leaves the -// host bind in place but invisible to the guest until re-propagated). Injectable via s.boundCheck for -// tests; defaults to the guest-namespace mount check. +// inside the guest — a guest reboot leaves the host bind in place but invisible to the guest until +// re-propagated. Injectable via s.boundCheck for tests; defaults to the guest-namespace mount check. +// +// This is VISIBILITY, not liveness (R-117). It compares only the mount point, so it stays true over a +// bind whose device has gone; do not read it as "usable in the guest" — that is the whole three-term +// conjunction at the two /disks construction sites, whose third term is bindUsable. func (s *Server) boundUnderParent(ctx context.Context, vmid int, stablePath string) bool { if s.boundCheck != nil { return s.boundCheck(stablePath) @@ -979,6 +1005,21 @@ func (s *Server) devicePresent(rawMountPath string) bool { return isHostMountpoint(rawMountPath) } +// bindUsable is the THIRD term of the BoundUnderParent conjunction (R-117): the bind must not only exist +// and be guest-visible, it must actually WORK. The first two terms are path-presence tests and are both +// satisfied by a bind that names the drive that went away — measured live, with EIO on every read and +// write while the payload read healthy and the gate restarted the customer's apps onto it. +// +// UNKNOWN counts as usable, via BindLiveness.Usable — the same "cannot tell → never absent" rule +// devicePresent applies above, and for the same reason: a false absent stops a working customer's apps. +// Injectable via s.livenessCheck; the default reads /proc only and issues NO block I/O (CLAUDE.md). +func (s *Server) bindUsable(stable, rawMountPath string) bool { + if s.livenessCheck != nil { + return s.livenessCheck(stable, rawMountPath).Usable() + } + return bindLiveness(stable, rawMountPath).Usable() +} + // guestBoundPaths returns the set of guest mountpoint paths (the `mp=` of each entry in the guest's // config) — i.e. the host drives actually BOUND into the guest. F9: this is the guest-attached signal // (`GuestAttached`) that distinguishes a guest-usable drive from one merely present on the host. A bind diff --git a/internal/localapi/disks_bind_liveness_test.go b/internal/localapi/disks_bind_liveness_test.go new file mode 100644 index 0000000..2e55443 --- /dev/null +++ b/internal/localapi/disks_bind_liveness_test.go @@ -0,0 +1,397 @@ +package localapi + +import ( + "context" + "io" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// R-117 — BoundUnderParent must mean THE BIND ACTUALLY WORKS, not "a mount by that name exists". +// +// THE BUG THESE PIN. GuestSeesMount (intermediary.go) and isHostMountpoint both parse a mountinfo line +// and then test only fields[4], the mount POINT. Field 3 — major:minor — sits in the same parsed slice +// and was discarded. So after a drive is detached and returned, the raw host mount heals onto the NEW +// device via its fs-UUID-keyed unit while the bind still names the OLD one, and BOTH existing terms stay +// true. Measured live: raw on 8:32 /dev/sdc, bind on 8:16 /dev/sdb with `shutdown`, BoundUnderParent +// TRUE, EIO on every read and write, and the controller's gate taking its Return branch — restarting the +// customer's apps onto that namespace and emailing backup_target_restored, with no alarm on any channel +// (felhom.eu audits/SPIKE-r117-bind-liveness-2026-07-30.md §3.3, §5.2). +// +// AND THE HALF THAT EMITS NOTHING AT ALL (spike §9, filed R-117a): a device that fails WITHOUT +// disappearing leaves the raw mount active, the devnos EQUAL, and the drive never Disconnected — so the +// gate produces neither a Stop nor a Return action and nothing is emitted, indefinitely. A devno +// comparison alone reads stale-device=false there, which is why term 3 checks the filesystem's own abort +// flags too. TestDisks_BindLiveness_AbortedFilesystemReadsAbsent is that case; a fix that shipped only +// the devno comparison would pass every other test in this file. +// +// WHY THE FIXTURES ARE REAL. Each mountinfo body below is the captured output of the spike run, not a +// hand-written line. R-116's fix shipped green and inert because its fixture supplied a MountPath +// production never supplies. These tests redirect procSelfMountinfo at a fixture file, so the REAL +// parser (hostMountEntries), the REAL predicate (bindLiveness) and the REAL /disks handler all run — +// the data is injected, the logic is not. +// +// RED-PROOFS (each verified to land, see REPORT.md): dropping `&& s.bindUsable(...)` from either /disks +// construction site fails StaleBindReadsAbsent / UnionPath_StaleBindReadsAbsent and +// AbortedFilesystemReadsAbsent; dropping the abort check so only the device comparison remains fails +// AbortedFilesystemReadsAbsent and AbortedWins_WhenDevnosAgree ALONE — that is the P1-only fix, and it is +// the one worth fearing; dropping `emergency_ro` from abortTokensByFS fails only the emergency_ro subtest; +// and returning BindLive instead of BindUnknown for an unreadable table fails UnknownIsTreatedAsPresent. +// +// A THIRD ordering trap, caught by TestBindLiveness_Verdicts during development and worth naming because +// it reports correctly while breaking the repair: reading the abort flag BEFORE comparing devices +// classifies the real return state as BindAborted, since its stale bind carries `shutdown` as well as a +// different device. BoundUnderParent still reads absent — every test in Group A still passes — but +// AttachDrive then refuses the re-bind that actually repairs it, so the self-heal never runs. The verdict +// must answer "would a re-bind help", which means reading the abort flag of the RAW mount (the re-bind's +// target) in the stale case, and of the bind itself only when the devices already agree. + +// ── fixtures, from the spike's captures ───────────────────────────────────────────────────────── +// +// Devices, super options, optional-field tags and root paths are verbatim. ONE substitution: the spike +// ran against a SCRATCH shared parent (/mnt/r117-drives) so it could not disturb the live +// /mnt/felhom-drives peer group, whereas the code under test derives the stable path itself, as +// StableParentDir + "/" + DriveNameFromRaw(raw). So /mnt/r117-drives/sd becomes +// /mnt/felhom-drives/r117sd. Substituting anything else would make these fixtures describe a path +// production never produces — which is precisely how R-116's fix shipped green and inert. + +// mountinfoHealthy is the HEALTHY state (spike §3.3): raw and bind on the SAME device, no abort token. +const mountinfoHealthy = `748 33 8:16 / /mnt/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw,stripe=512 +755 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw,stripe=512 +` + +// mountinfoStaleBind is the R-117 RETURN state (spike §3.3 / §5.2): the drive came back as /dev/sdc +// (8:32) and the raw mount healed onto it, while the bind still names /dev/sdb (8:16) and carries +// `shutdown`. Both pre-R-117 terms read true here. +const mountinfoStaleBind = `755 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw,stripe=512,shutdown +814 33 8:32 / /mnt/r117sd rw,relatime shared:485 - ext4 /dev/sdc rw,stripe=512 +` + +// mountinfoAborted is the STEADY-STATE state (spike §9): the device errored in place, so it NEVER LEFT. +// Raw and bind are the SAME device — the device comparison cannot see this — and ext4 has done an +// emergency remount-ro. Today this state emits nothing on any channel. +// +// SECOND substitution, and it is the one that nearly made this test decoration. The spike produced this +// state on a dm device (dm is the only mechanism that can make a device error WITHOUT disappearing), so +// the capture reads 252:11 /dev/mapper/r117cel. Transposed here onto the USB drive shape, because +// RoleForStorage derives role="system" for a /dev/mapper backing device — and a system-role row never +// enters the block that computes BoundUnderParent, so the field stays false by DEFAULT and the assertion +// below passes without term 3 ever running. It did exactly that until RP1 failed to fail (see REPORT.md). +// An in-place abort on a USB drive is the realistic customer case anyway (a link reset that recovers the +// link after ext4 has already given up); only the super options and the matching devnos carry the claim. +const mountinfoAborted = `748 33 8:16 / /mnt/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw,stripe=512,emergency_ro +755 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw,stripe=512,emergency_ro +` + +// mountinfoAbortedShutdown is the same in-place shape with the OTHER ext4 abort token, the one a device +// removal sets. Both were measured; a check for only `shutdown` passes mountinfoAborted and a check for +// only `emergency_ro` passes this — which is why abortTokensByFS carries both, and why RP4 exists. +const mountinfoAbortedShutdown = `748 33 8:16 / /mnt/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw,stripe=512,shutdown +755 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw,stripe=512,shutdown +` + +// mountinfoUnknownFS is healthy-looking but on a filesystem whose abort vocabulary we have not measured. +// The honest verdict is UNKNOWN — which must be treated as PRESENT, not as live and not as absent. +const mountinfoUnknownFS = `748 33 8:16 / /mnt/r117sd rw,relatime shared:450 - btrfs /dev/sdb rw +755 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:450 - btrfs /dev/sdb rw +` + +// useMountinfo points the REAL parsers at a fixture for the duration of one test. +func useMountinfo(t *testing.T, body string) { + t.Helper() + p := filepath.Join(t.TempDir(), "mountinfo") + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + prev := procSelfMountinfo + procSelfMountinfo = p + t.Cleanup(func() { procSelfMountinfo = prev }) +} + +// livenessServer builds a /disks server over one Observe target (or one registry drive) whose raw mount +// is `raw` and stable guest path derives from it. The two PRE-R-117 terms are forced TRUE — that is the +// whole point: they were both true in the measured defect, so term 3 is the only thing that can save us. +func livenessServer(t *testing.T, obs []hub.StorageTarget, known []storage.KnownTarget) *Server { + t.Helper() + opts := Options{ + ListenAddr: "127.0.0.1:0", + Guests: &fakeGuestsCfg{}, + Backups: &fakeBackups{}, + Store: &fakeStore{}, + Storage: fakeStorage{targets: obs}, + 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 known != nil { + opts.DriveTargets = fakeKnownTargets{drives: known} + } + srv, err := NewServer(opts) + if err != nil { + t.Fatal(err) + } + srv.baseCtx = context.Background() + // Terms 1 and 2 TRUE — the measured defect's own conditions. livenessCheck is left nil so the real + // bindLiveness runs against the fixture. + srv.boundCheck = func(string) bool { return true } + srv.deviceCheck = func(string) bool { return true } + srv.resolveStorageDevice = func(string) (string, error) { return "/dev/sdb1", nil } + return srv +} + +var obsSD = []hub.StorageTarget{ + {Name: "sd", Type: hub.StorageTypeUSB, BackingDevice: "/dev/sdb", MountPath: "/mnt/r117sd", State: hub.StorageStateAttached}, +} +var knownSD = []storage.KnownTarget{ + {Name: "sd", Type: hub.StorageTypeUSB, MountPath: "/mnt/r117sd", DurableID: "uuid:71e1", UUID: "71e1"}, +} + +// ── Group A — the consequence: a dead namespace reads ABSENT ──────────────────────────────────── + +// TestDisks_BindLiveness_StaleBindReadsAbsent is R-117 case (a), through the real /disks handler. +// It asserts the CONSEQUENCE — what the controller reads off the wire — not that a comparison happened. +func TestDisks_BindLiveness_StaleBindReadsAbsent(t *testing.T) { + useMountinfo(t, mountinfoStaleBind) + di := diskByMount(t, livenessServer(t, obsSD, nil), "/mnt/r117sd") + if di.BoundUnderParent { + t.Error("BoundUnderParent reports PRESENT over a stale bind (R-117). The bind names 8:16 /dev/sdb " + + "while the raw mount is 8:32 /dev/sdc; every access through it returns EIO. The controller's " + + "gate would take its Return branch (controller intermediary.go:258,:299) and restart the " + + "customer's apps onto a dead namespace, then email backup_target_restored.") + } +} + +// TestDisks_BindLiveness_AbortedFilesystemReadsAbsent is R-117a, the steady-state half — and the test a +// devno-only fix would fail. The device NEVER LEFT, so raw and bind agree on 252:11. +func TestDisks_BindLiveness_AbortedFilesystemReadsAbsent(t *testing.T) { + for _, c := range []struct{ name, body string }{ + {"emergency_ro (errors=remount-ro fired in place)", mountinfoAborted}, + {"shutdown (forced abort)", mountinfoAbortedShutdown}, + } { + t.Run(c.name, func(t *testing.T) { + useMountinfo(t, c.body) + // GUARD, earned: assert the row is the shape production emits BEFORE asserting the field. + // A system-role row has no GuestPath, never runs the conjunction, and reports + // BoundUnderParent=false by default — passing this test while proving nothing. + di := diskByMount(t, livenessServer(t, obsSD, nil), "/mnt/r117sd") + if di.Role != "user-data" || di.GuestPath == "" { + t.Fatalf("fixture does not reproduce the production row shape: role=%q guest_path=%q — "+ + "the conjunction never runs on such a row, so any assertion below is vacuous", + di.Role, di.GuestPath) + } + if di.BoundUnderParent { + t.Error("BoundUnderParent reports PRESENT over an ABORTED filesystem (R-117a). The devnos " + + "MATCH (the device never disappeared), so the device comparison cannot see this — only " + + "the filesystem's own abort token can. Today this state emits NOTHING on any channel: " + + "the drive is never Disconnected, so the gate produces neither a Stop nor a Return.") + } + }) + } +} + +// TestDisks_BindLiveness_UnionPath_AbortedReadsAbsent — the union path for the steady-state half. It +// matters more than the Observe one here: this row's Role is hardcoded user-data and its State hardcoded +// attached, so the conjunction is the ONLY thing on the row that can report the abort. +func TestDisks_BindLiveness_UnionPath_AbortedReadsAbsent(t *testing.T) { + useMountinfo(t, mountinfoAborted) + di := diskByMount(t, livenessServer(t, nil, knownSD), "/mnt/r117sd") + if di.BoundUnderParent { + t.Error("union-path drive reports PRESENT over an ABORTED filesystem (R-117a) — and its Role and " + + "State are both hardcoded on this row, so nothing else can contradict it") + } +} + +// The union path carries no PVE dir-storage and hardcodes State:"attached", so these terms are the only +// device truth on the row — R-113's reasoning, and it applies to term 3 identically. +func TestDisks_BindLiveness_UnionPath_StaleBindReadsAbsent(t *testing.T) { + useMountinfo(t, mountinfoStaleBind) + di := diskByMount(t, livenessServer(t, nil, knownSD), "/mnt/r117sd") + if di.BoundUnderParent { + t.Error("union-path drive reports PRESENT over a stale bind (R-117) — and its State is hardcoded " + + "attached, so nothing else on the row can contradict it") + } +} + +// ── Group B — no false negatives ──────────────────────────────────────────────────────────────── + +func TestDisks_BindLiveness_HealthyReadsPresent(t *testing.T) { + useMountinfo(t, mountinfoHealthy) + for _, c := range []struct { + name string + obs []hub.StorageTarget + known []storage.KnownTarget + }{ + {"observe", obsSD, nil}, + {"union", nil, knownSD}, + } { + t.Run(c.name, func(t *testing.T) { + di := diskByMount(t, livenessServer(t, c.obs, c.known), "/mnt/r117sd") + if !di.BoundUnderParent { + t.Error("a healthy drive reads ABSENT — a false absent STOPS a working customer's apps, " + + "which is strictly worse than the bug being fixed") + } + }) + } +} + +// ── Group C — cannot tell must never mean absent ──────────────────────────────────────────────── + +// TestDisks_BindLiveness_UnknownIsTreatedAsPresent pins the rule in every way it can be reached. The +// workspace's false-invariant table records newestArchiveOn promising exactly this over a signature that +// could not express it; Usable() is the one place it lives, so this is the test that keeps it honest. +func TestDisks_BindLiveness_UnknownIsTreatedAsPresent(t *testing.T) { + t.Run("unreadable mount table", func(t *testing.T) { + prev := procSelfMountinfo + procSelfMountinfo = filepath.Join(t.TempDir(), "does-not-exist") + t.Cleanup(func() { procSelfMountinfo = prev }) + if got := bindLiveness("/mnt/felhom-drives/r117sd", "/mnt/r117sd"); got != BindUnknown { + t.Errorf("unreadable /proc gave %v, want BindUnknown", got) + } + di := diskByMount(t, livenessServer(t, obsSD, nil), "/mnt/r117sd") + if !di.BoundUnderParent { + t.Error("an unreadable mount table made the drive read ABSENT — cannot-tell must never stop apps") + } + }) + + t.Run("no raw mount entry to compare against", func(t *testing.T) { + // Only the bind is in the table. devicePresent is the term that answers device absence; this one + // must abstain rather than double-count it. + useMountinfo(t, `755 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:450 - ext4 /dev/sdb rw +`) + if got := bindLiveness("/mnt/felhom-drives/r117sd", "/mnt/r117sd"); got != BindUnknown { + t.Errorf("missing raw entry gave %v, want BindUnknown", got) + } + }) + + t.Run("empty paths", func(t *testing.T) { + if got := bindLiveness("", "/mnt/r117sd"); got != BindUnknown { + t.Errorf("empty stable gave %v, want BindUnknown", got) + } + if got := bindLiveness("/mnt/felhom-drives/r117sd", ""); got != BindUnknown { + t.Errorf("empty raw gave %v, want BindUnknown", got) + } + }) + + t.Run("filesystem whose abort vocabulary is unmeasured", func(t *testing.T) { + useMountinfo(t, mountinfoUnknownFS) + if got := bindLiveness("/mnt/felhom-drives/r117sd", "/mnt/r117sd"); got != BindUnknown { + t.Errorf("btrfs bind gave %v, want BindUnknown — we cannot read its abort state, so we must "+ + "not claim LIVE either", got) + } + di := diskByMount(t, livenessServer(t, obsSD, nil), "/mnt/r117sd") + if !di.BoundUnderParent { + t.Error("an unmeasured filesystem read ABSENT — that would stop apps on every non-ext4 drive") + } + }) + + t.Run("Usable is the single place the rule lives", func(t *testing.T) { + for _, c := range []struct { + l BindLiveness + want bool + }{ + {BindLive, true}, + {BindUnknown, true}, // the rule + {BindStaleDevice, false}, + {BindAborted, false}, + } { + if got := c.l.Usable(); got != c.want { + t.Errorf("%v.Usable() = %v, want %v", c.l, got, c.want) + } + } + }) +} + +// ── Group D — the verdict itself, including the ordering that matters ─────────────────────────── + +func TestBindLiveness_Verdicts(t *testing.T) { + for _, c := range []struct { + name, body string + stable, raw string + want BindLiveness + }{ + {"healthy", mountinfoHealthy, "/mnt/felhom-drives/r117sd", "/mnt/r117sd", BindLive}, + {"stale device (case a)", mountinfoStaleBind, "/mnt/felhom-drives/r117sd", "/mnt/r117sd", BindStaleDevice}, + {"aborted in place, emergency_ro (case b)", mountinfoAborted, "/mnt/felhom-drives/r117sd", "/mnt/r117sd", BindAborted}, + {"aborted in place, shutdown", mountinfoAbortedShutdown, "/mnt/felhom-drives/r117sd", "/mnt/r117sd", BindAborted}, + } { + t.Run(c.name, func(t *testing.T) { + useMountinfo(t, c.body) + if got := bindLiveness(c.stable, c.raw); got != c.want { + t.Errorf("bindLiveness = %v, want %v", got, c.want) + } + }) + } +} + +// TestBindLiveness_AbortedWins pins the ORDERING, which is load-bearing and not obvious: in the measured +// stale-bind state the filesystem ALSO carries `shutdown`, so both P1 and P2 apply. The verdict must be +// BindAborted-or-BindStaleDevice — never live — but more importantly the in-place state, where ONLY P2 +// applies, must not fall through to a devno comparison that reads equal. This test fails if P1 is checked +// before P2 and returns early. +func TestBindLiveness_AbortedWins_WhenDevnosAgree(t *testing.T) { + useMountinfo(t, mountinfoAborted) + got := bindLiveness("/mnt/felhom-drives/r117sd", "/mnt/r117sd") + if got.Usable() { + t.Fatalf("bindLiveness = %v (usable) — the devnos agree because the device never left, so a "+ + "P1-first implementation reads this as LIVE and ships R-117's silent half intact", got) + } + if got != BindAborted { + t.Errorf("bindLiveness = %v, want BindAborted (the abort token is the only signal here)", got) + } +} + +// ── Group E — the parser, on a real captured table ────────────────────────────────────────────── + +// TestHostMountEntries_ParsesDevnoAndSuperOpts pins the field extraction R-117 turned on. The optional +// fields run (shared:NNN master:NNN) is variable-length, so the " - " separator — not a fixed index — is +// what locates fstype and the super options. +func TestHostMountEntries_ParsesDevnoAndSuperOpts(t *testing.T) { + // A guest-side line with BOTH optional-field tags, the longest real shape (spike §5.2). + useMountinfo(t, `759 1176 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:459 master:450 - ext4 /dev/sdb rw,stripe=512,shutdown +`) + got := hostMountEntries("/mnt/felhom-drives/r117sd") + if len(got) != 1 { + t.Fatalf("got %d entries, want 1", len(got)) + } + e := got[0] + if e.Devno != "8:16" { + t.Errorf("Devno = %q, want 8:16 — this is the field R-117 was lost for want of reading", e.Devno) + } + if e.Root != "/felhom-data" { + t.Errorf("Root = %q, want /felhom-data", e.Root) + } + if e.FSType != "ext4" { + t.Errorf("FSType = %q, want ext4 (located via the ' - ' separator, not a fixed index)", e.FSType) + } + if !strings.Contains(e.SuperOpts, "shutdown") { + t.Errorf("SuperOpts = %q, want it to carry `shutdown`", e.SuperOpts) + } +} + +// countHostMounts and isHostMountpoint were rewritten onto hostMountEntries; the double-bind convergence +// AttachDrive depends on must survive that (REUSE.md: a boolean could not converge stacked binds). +func TestHostMountEntries_CountsStackedBinds(t *testing.T) { + useMountinfo(t, `755 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime - ext4 /dev/sdb rw +756 118 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime - ext4 /dev/sdb rw +`) + if n := countHostMounts("/mnt/felhom-drives/r117sd"); n != 2 { + t.Errorf("countHostMounts = %d, want 2 — AttachDrive's normalize leg needs the count, not a bool", n) + } + if !isHostMountpoint("/mnt/felhom-drives/r117sd") { + t.Error("isHostMountpoint = false over two stacked binds") + } + if isHostMountpoint("/mnt/nope") { + t.Error("isHostMountpoint = true for a path with no entry") + } + if n := countHostMounts("/mnt/nope"); n != 0 { + t.Errorf("countHostMounts = %d for an absent path, want 0", n) + } +} diff --git a/internal/localapi/intermediary.go b/internal/localapi/intermediary.go index 42f30a3..fb4ce58 100644 --- a/internal/localapi/intermediary.go +++ b/internal/localapi/intermediary.go @@ -233,7 +233,33 @@ func (b *GuestBinder) AttachDrive(ctx context.Context, vmid int, where string) ( // makes this converge a double-bind to one (the old umount-one+mount-one never did). n := countHostMounts(stable) if n == 1 && b.GuestSeesMount(ctx, vmid, stable) { - return stable, nil // exactly one bind + guest-visible → fully live, no-op + // R-117: "one bind + the guest sees it" is NOT liveness. Both of those are path-presence tests, so + // this early return declared a namespace that EIO'd on every call "fully live" and defeated the + // three call sites that already invoke this repair — the 20 s reconcile ticker, agent startup, and + // the controller's Return branch BEFORE it restarts the apps (spike §8.2). The verdict decides: + switch lv := bindLiveness(stable, where); lv { + case BindStaleDevice: + // Case (a). The raw mount has healed onto the returning device; re-binding this stale shell + // onto it REPAIRS the namespace live, with no guest restart (proven, spike §8.1). Fall through + // to the normalize+rebind below. WARN not INFO-per-tick: this fires once, then it is fixed. + b.logger.Warn("guest-attach: bind is STALE — it names a different device than the raw mount; re-binding", + "vmid", vmid, "where", where, "stable", stable, "verdict", lv.String()) + case BindAborted: + // Case (b), the Q7 steady-state case. The raw mount is the SAME aborted superblock, so a + // re-bind produces a fresh bind to a still-dead filesystem — and because this runs every 20 s + // it would be an infinite silent retry: exactly the silence Q7 found, with more CPU. Leave the + // mount alone and let the truth travel in BoundUnderParent, which now reads false, so the + // drive gate stops the apps and raises the alarm. Clearing an aborted filesystem needs a + // remount or a fsck — an operator decision, never an automatic one (R-117a). + // + // DEBUG, not WARN: this repeats every tick, and the operator-facing signal is the /disks + // payload plus the customer alarm. Per logging-conventions, INFO is for state changes. + b.logger.Debug("guest-attach: filesystem under the bind has ABORTED — not re-binding (a re-bind cannot clear it); reported not-live instead", + "vmid", vmid, "where", where, "stable", stable, "verdict", lv.String()) + return stable, nil + default: // BindLive, BindUnknown — genuinely live, or we cannot tell. Unchanged behaviour. + return stable, nil + } } for i := 0; i < 16 && countHostMounts(stable) > 0; i++ { if err := b.run(ctx, "umount", stable); err != nil { @@ -250,23 +276,7 @@ func (b *GuestBinder) AttachDrive(ctx context.Context, vmid int, where string) ( // countHostMounts returns how many times `path` appears as a mount target in /proc/self/mountinfo (i.e. // how many stacked binds are at it). 0 = not mounted; >1 = stacked duplicates. Used to normalize to one. -func countHostMounts(path string) int { - f, err := os.Open("/proc/self/mountinfo") - if err != nil { - return 0 - } - defer f.Close() - n := 0 - sc := bufio.NewScanner(f) - sc.Buffer(make([]byte, 0, 64*1024), 1024*1024) - for sc.Scan() { - fields := strings.Fields(sc.Text()) - if len(fields) >= 5 && fields[4] == path { - n++ - } - } - return n -} +func countHostMounts(path string) int { return len(hostMountEntries(path)) } // GuestSeesMount reports whether vmid's guest currently has `path` as a mount target in ITS mount // namespace (read from /proc//mountinfo). This is the GUEST-side truth the host-side @@ -278,7 +288,7 @@ func (b *GuestBinder) GuestSeesMount(ctx context.Context, vmid int, path string) if pid == "" { return false } - data, err := os.ReadFile("/proc/" + pid + "/mountinfo") + data, err := os.ReadFile(procGuestMountinfo(pid)) if err != nil { return false } @@ -391,20 +401,212 @@ func (b *GuestBinder) DetachDrive(ctx context.Context, where string) error { // isHostMountpoint reports whether path is currently a mount target in the host's mount table // (/proc/self/mountinfo). Pure read — used for idempotency (skip re-binding) and the BoundUnderParent // report. A read error → false (treat as not-mounted; AttachDrive then (re)binds, which is safe). -func isHostMountpoint(path string) bool { - f, err := os.Open("/proc/self/mountinfo") +func isHostMountpoint(path string) bool { return len(hostMountEntries(path)) > 0 } + +// procSelfMountinfo is the host mount table every predicate in this file reads. It is a package var +// ONLY so a test can point the REAL parsers at a captured fixture — production never reassigns it, and a +// test that does must restore it (t.Cleanup). Injecting the DATA rather than the verdict is what keeps +// the R-117 tests non-hollow: the parser, the predicate and the /disks handler all run for real. +var procSelfMountinfo = "/proc/self/mountinfo" + +// procGuestMountinfo resolves a guest's init PID to its mount-table path. A package var for the same +// single reason as procSelfMountinfo: so a test can point the REAL GuestSeesMount at a captured guest +// mount table. Production never reassigns it. +var procGuestMountinfo = func(pid string) string { return "/proc/" + pid + "/mountinfo" } + +// mountEntry is the parsed subset of a mountinfo line the liveness predicate needs. Field numbers are +// the kernel's 1-based numbering (proc(5) "/proc//mountinfo"): 3 = major:minor, 4 = root within the +// filesystem, 5 = mount point; after the " - " separator come fstype, source and the per-superblock +// options. Mount points containing spaces are octal-escaped by the kernel, so strings.Fields is safe. +type mountEntry struct { + // Devno is field 3, the backing device as major:minor. THIS is the field R-117 was lost for want of + // reading: it sat in the same parsed slice as the mount point and was discarded. + Devno string + // Root is field 4 — which subtree of the filesystem is mounted (e.g. /felhom-data for our binds). + Root string + // FSType is the filesystem driver, needed to know whether SuperOpts' vocabulary is one we can read. + FSType string + // SuperOpts is the per-superblock option list — where ext4 records that it has stopped serving I/O. + SuperOpts string +} + +// hostMountEntries returns every entry in the host mount table whose mount point is `path`. There is +// more than one when binds are stacked (the double-bind case AttachDrive normalizes). A read error +// yields nil — callers treat that as "not mounted"/"cannot tell", never as a positive. +// +// Pure /proc read, NO BLOCK I/O, per CLAUDE.md's health-check rule: a probe that touches a wedged +// device enters uninterruptible sleep and survives SIGKILL (measured, R-117 spike §6.3). +func hostMountEntries(path string) []mountEntry { + f, err := os.Open(procSelfMountinfo) if err != nil { - return false + return nil } defer f.Close() + var out []mountEntry sc := bufio.NewScanner(f) sc.Buffer(make([]byte, 0, 64*1024), 1024*1024) for sc.Scan() { - // mountinfo field 5 (0-indexed 4) is the mount point. fields := strings.Fields(sc.Text()) - if len(fields) >= 5 && fields[4] == path { - return true + if len(fields) < 5 || fields[4] != path { + continue + } + e := mountEntry{Devno: fields[2], Root: fields[3]} + // The optional-fields run is variable-length; the " - " separator terminates it. + for i := 5; i < len(fields); i++ { + if fields[i] != "-" { + continue + } + if len(fields) > i+1 { + e.FSType = fields[i+1] + } + if len(fields) > i+3 { + e.SuperOpts = fields[i+3] + } + break + } + out = append(out, e) + } + return out +} + +// BindLiveness is the THREE-state answer to "is the bind at the stable path actually usable?". +// +// Three states and not a bool, deliberately. The R-117 fix must be able to say "cannot tell", and the +// cost of getting that wrong is asymmetric: reporting a live drive absent STOPS a customer's apps. The +// workspace's false-invariant table records `newestArchiveOn` promising "errors degrade to unknown, +// never to no-backup" over a (value, bool) signature that made it unrepresentable — the comment was a +// wish. Read every verdict through Usable() and no caller can repeat that. +type BindLiveness int + +const ( + // BindUnknown — liveness could not be established (unreadable /proc, no raw mount to compare + // against, or a filesystem whose abort vocabulary we have not measured). TREATED AS PRESENT by + // Usable(), the same rule devicePresent applies to an empty path (disks.go). + BindUnknown BindLiveness = iota + // BindLive — the bind names the same device as the raw mount and its filesystem has not aborted. + BindLive + // BindStaleDevice — R-117 case (a), the detach/return case. The bind still references the superblock + // of the drive that went away, while the raw mount has healed onto the returning device via its + // fs-UUID-keyed unit. Every access through the bind fails. RE-BINDING REPAIRS THIS. + BindStaleDevice + // BindAborted — R-117 case (b), the Q7 steady-state case. The filesystem under the bind has given up: + // ext4 sets `shutdown` when its device vanished, `emergency_ro` when errors=remount-ro fired in place. + // The raw mount is the SAME aborted superblock, so RE-BINDING CANNOT REPAIR THIS — it must surface as + // not-live so the drive gate stops the apps and alarms. See AttachDrive's switch. + BindAborted +) + +// Usable is the ONLY sanctioned way to turn a verdict into a yes/no, so the unknown-is-present rule +// lives in exactly one place. Pinned by TestBindLiveness_UnknownIsTreatedAsPresent. +func (l BindLiveness) Usable() bool { return l == BindLive || l == BindUnknown } + +func (l BindLiveness) String() string { + switch l { + case BindLive: + return "live" + case BindStaleDevice: + return "stale-device" + case BindAborted: + return "filesystem-aborted" + default: + return "unknown" + } +} + +// abortTokensByFS maps a filesystem driver to the per-superblock option tokens it sets when it has +// stopped serving I/O. BOTH ext4 tokens are load-bearing and BOTH were measured (R-117 spike §4): +// `shutdown` when the device was removed, `emergency_ro` when errors=remount-ro fired with the device +// still present. A check for only `shutdown` passes the ENTIRE Q7 state, which is the silent half. +// +// ext2/ext3 are served by the ext4 driver on this kernel, so they emit the same tokens. Anything else is +// a customer-supplied filesystem whose vocabulary we have not measured — it yields UNKNOWN, never LIVE +// (the agent itself only ever formats ext4). +var abortTokensByFS = map[string][]string{ + "ext4": {"shutdown", "emergency_ro"}, + "ext3": {"shutdown", "emergency_ro"}, + "ext2": {"shutdown", "emergency_ro"}, +} + +// fsAborted reports whether the entry's filesystem has aborted, and whether we could tell at all. +// `known` false means the fstype is not in abortTokensByFS — the caller must degrade to BindUnknown +// rather than infer health from the absence of a token it does not know how to look for. +func fsAborted(e mountEntry) (aborted, known bool) { + toks, ok := abortTokensByFS[e.FSType] + if !ok { + return false, false + } + for _, opt := range strings.Split(e.SuperOpts, ",") { + for _, t := range toks { + if opt == t { + return true, true + } } } - return false + return false, true +} + +// bindLiveness is the R-117 predicate: does the bind at `stable` actually work? `raw` is the drive's RAW +// host mount (/mnt/). Reads /proc only — NO block I/O, per CLAUDE.md's health-check rule. +// +// WHY THIS EXISTS. GuestSeesMount and isHostMountpoint both compare only field 5 (the mount point) of a +// mountinfo line, so both answer "does a mount by that name exist" and neither can see that the bind and +// the raw mount name DIFFERENT devices. Measured live: raw on 8:32 /dev/sdc while the bind read +// 8:16 /dev/sdb with `shutdown`, BoundUnderParent true, EIO on every read and write, and the gate +// restarting the customer's apps onto it (R-117 spike §5.2). +// +// HOST-SIDE ONLY, deliberately: the host bind and the guest's view of it are the same mount in one +// propagation peer group and carry identical devno and super options (measured, spike §5.2), so this +// needs no lxc-info fork. Guest VISIBILITY is a different question and stays with GuestSeesMount. +func bindLiveness(stable, raw string) BindLiveness { + if stable == "" || raw == "" { + return BindUnknown // nothing to compare — never claim absent + } + binds := hostMountEntries(stable) + if len(binds) == 0 { + return BindUnknown // nothing bound here; that is isHostMountpoint's question, not this one + } + rawEntries := hostMountEntries(raw) + if len(rawEntries) == 0 { + return BindUnknown // the raw mount is gone — devicePresent already reports that as absent + } + // The two cases are distinguished by WHETHER THE DEVICES AGREE, and the abort flag is read off a + // DIFFERENT entry in each. Getting this backwards is a live trap, caught here by + // TestBindLiveness_Verdicts: in the real return state the stale bind carries `shutdown` AND names a + // different device, so an abort-first rule classifies it BindAborted — which reports correctly but + // refuses the re-bind that actually repairs it. The question a verdict must answer for AttachDrive is + // not "is something aborted" but "would a re-bind help". + rawEntry := rawEntries[0] + for _, b := range binds { + if b.Devno != rawEntry.Devno { + // P1 — case (a). The bind references a superblock that is NOT the one the raw mount now has: + // the drive went away and came back, and the raw mount healed onto it via its fs-UUID-keyed + // unit. Sound rather than heuristic — a stale bind pins the dead superblock, which keeps the + // old device index allocated, which FORCES the returning device onto a different number + // (measured both ways, including the control test where releasing the bind let the letter be + // reused, spike §3.4). + // + // Whether a re-bind repairs it depends on the RAW mount, which is what a re-bind would point + // at — not on the stale bind's own abort flag. + if aborted, known := fsAborted(rawEntry); known && aborted { + return BindAborted // re-binding would land on another dead filesystem + } + return BindStaleDevice // re-binding lands on the healthy returning device: repairable + } + } + // Same superblock on both sides, so a re-bind is a no-op by construction. Only the filesystem's own + // abort state can tell us anything — and this is R-117's steady-state half (spike §9), where the + // device NEVER LEFT so the devnos above agree and P1 alone reads healthy. + for _, b := range binds { + if aborted, known := fsAborted(b); known && aborted { + return BindAborted + } + } + // Devices agree and nothing aborted. If we cannot read this filesystem's abort vocabulary we must not + // call it live — say unknown, which Usable() treats as present. + for _, b := range binds { + if _, known := fsAborted(b); !known { + return BindUnknown + } + } + return BindLive } diff --git a/internal/localapi/intermediary_attach_liveness_test.go b/internal/localapi/intermediary_attach_liveness_test.go new file mode 100644 index 0000000..bd94bc6 --- /dev/null +++ b/internal/localapi/intermediary_attach_liveness_test.go @@ -0,0 +1,187 @@ +package localapi + +import ( + "context" + "io" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" +) + +// R-117 §2.2 — WHAT AttachDrive DOES with each verdict, which is a RULING and not a detail. +// +// The two dead states have DIFFERENT repairs, and the existing self-heal only fits one: +// +// - STALE DEVICE (the detach/return case). The raw mount has healed onto the returning device via its +// fs-UUID-keyed unit, so umount + re-bind lands the namespace on a HEALTHY superblock. Repair is +// correct, and it happens live with no guest restart (proven on hardware, spike §8.1). It MUST run: +// three call sites already invoke it — the 20 s reconcile ticker, agent startup, and the controller's +// Return branch BEFORE it restarts the apps (spike §8.2) — and before v0.117.0 all three were +// short-circuited by `if n == 1 && GuestSeesMount(...)` declaring the dead namespace "fully live". +// +// - ABORTED FILESYSTEM (the steady-state case, R-117a). The raw mount is the SAME aborted superblock, +// so a re-bind produces a fresh bind to a still-dead filesystem. It MUST NOT run: AttachDrive is +// called every 20 s, so re-binding here is an infinite silent retry — the exact silence R-117a +// found, with more CPU — and it would mask the state instead of surfacing it. The truth travels in +// BoundUnderParent (now false), so the drive gate stops the apps and alarms. Clearing an aborted +// filesystem needs a remount or a fsck; that is an operator decision, never an automatic one. +// +// These tests assert the CONSEQUENCE — which privileged commands were issued — not that a verdict was +// computed. RED-PROOF: make the BindAborted arm fall through to the re-bind instead of returning, and +// TestAttachDrive_AbortedFilesystem_DoesNotRebind fails on the recorded umount/mount calls. + +// attachRecorder is a proxmox.Runner that records every privileged call and answers `lxc-info -p` with a +// fixed PID, so the REAL GuestSeesMount runs against a captured guest mount table. +type attachRecorder struct { + calls [][]string + pid string +} + +func (r *attachRecorder) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + if name == "lxc-info" { + return []byte(r.pid + "\n"), nil, nil + } + return nil, nil, nil +} + +func (r *attachRecorder) RunStdin(ctx context.Context, _ io.Reader, name string, args ...string) ([]byte, []byte, error) { + return r.Run(ctx, name, args...) +} + +// mountOps returns just the mount-table-mutating calls — the ones that constitute "a repair ran". +func (r *attachRecorder) mountOps() []string { + var out []string + for _, c := range r.calls { + switch c[0] { + case "umount", "mount": + out = append(out, strings.Join(c, " ")) + } + } + return out +} + +// attachFixture points BOTH mount tables at fixtures: the host one (procSelfMountinfo, which +// countHostMounts and bindLiveness read) and the guest one (procGuestMountinfo, which the REAL +// GuestSeesMount reads). Only the data is injected — every predicate runs for real. +func attachFixture(t *testing.T, hostBody, guestBody string) *attachRecorder { + t.Helper() + dir := t.TempDir() + hp := filepath.Join(dir, "host-mountinfo") + gp := filepath.Join(dir, "guest-mountinfo") + if err := os.WriteFile(hp, []byte(hostBody), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(gp, []byte(guestBody), 0o600); err != nil { + t.Fatal(err) + } + prevHost, prevGuest := procSelfMountinfo, procGuestMountinfo + procSelfMountinfo = hp + procGuestMountinfo = func(string) string { return gp } + t.Cleanup(func() { procSelfMountinfo, procGuestMountinfo = prevHost, prevGuest }) + return &attachRecorder{pid: "9301"} +} + +// guestSeesStale / guestSeesHealthy / guestSeesAborted are the GUEST-side captures — note `master:450`, +// the slave-of-the-shared-parent tag that proves propagation was wired (spike §3.1). The guest carries the +// same devno and super options as the host bind, because it IS the same mount. +const guestSeesStale = `759 1176 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:459 master:450 - ext4 /dev/sdb rw,stripe=512,shutdown +` +const guestSeesHealthy = `759 1176 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:459 master:450 - ext4 /dev/sdb rw,stripe=512 +` +const guestSeesAborted = `759 1176 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:459 master:450 - ext4 /dev/sdb rw,stripe=512,emergency_ro +` + +func attachBinder(rec *attachRecorder) *GuestBinder { + return NewGuestBinder(rec, slog.New(slog.NewTextHandler(io.Discard, nil))) +} + +// TestAttachDrive_StaleBind_Rebinds is Q6's consequence: the repair that was short-circuited for three +// releases now runs. Exactly the state measured on hardware — bind on 8:16, raw healed onto 8:32. +func TestAttachDrive_StaleBind_Rebinds(t *testing.T) { + rec := attachFixture(t, mountinfoStaleBind, guestSeesStale) + b := attachBinder(rec) + + got, err := b.AttachDrive(context.Background(), 9301, "/mnt/r117sd") + if err != nil { + t.Fatalf("AttachDrive: %v", err) + } + if got != "/mnt/felhom-drives/r117sd" { + t.Errorf("stable path = %q", got) + } + ops := rec.mountOps() + if len(ops) == 0 { + t.Fatal("NO repair ran over a stale bind — this is the R-117 short-circuit: `n == 1 && " + + "GuestSeesMount` declared an EIO namespace \"fully live\", so the 20 s ticker, agent startup " + + "and the controller's pre-restart re-attach all did nothing") + } + var sawUmount, sawBind bool + for _, o := range ops { + if strings.HasPrefix(o, "umount /mnt/felhom-drives/r117sd") { + sawUmount = true + } + if o == "mount --bind /mnt/r117sd/felhom-data /mnt/felhom-drives/r117sd" { + sawBind = true + } + } + if !sawUmount || !sawBind { + t.Errorf("repair did not umount-then-rebind; ops=%v", ops) + } +} + +// TestAttachDrive_AbortedFilesystem_DoesNotRebind is the §2.2 ruling. A re-bind here cannot repair +// anything (the raw mount is the same aborted superblock) and AttachDrive runs every 20 s, so re-binding +// would be an infinite silent retry that also masks the state. +func TestAttachDrive_AbortedFilesystem_DoesNotRebind(t *testing.T) { + rec := attachFixture(t, mountinfoAborted, guestSeesAborted) + b := attachBinder(rec) + + got, err := b.AttachDrive(context.Background(), 9301, "/mnt/r117sd") + if err != nil { + t.Fatalf("AttachDrive returned an error for an aborted filesystem: %v — it must be a quiet no-op; "+ + "an error here would log `reconcile: AttachDrive failed` every 20 s", err) + } + if got != "/mnt/felhom-drives/r117sd" { + t.Errorf("stable path = %q", got) + } + if ops := rec.mountOps(); len(ops) != 0 { + t.Errorf("AttachDrive re-bound an ABORTED filesystem: %v\n"+ + "A re-bind lands on the SAME dead superblock, and this runs every 20 s — an infinite silent "+ + "retry, which is R-117a's silence with more CPU. The state must SURFACE via "+ + "BoundUnderParent=false so the gate stops the apps and alarms.", ops) + } +} + +// TestAttachDrive_Healthy_IsStillANoOp — the idempotency the reconcile ticker depends on. If this +// regressed, every tick would umount and re-bind a working drive, re-firing propagation into the guest +// 4320 times a day. +func TestAttachDrive_Healthy_IsStillANoOp(t *testing.T) { + rec := attachFixture(t, mountinfoHealthy, guestSeesHealthy) + b := attachBinder(rec) + + if _, err := b.AttachDrive(context.Background(), 9301, "/mnt/r117sd"); err != nil { + t.Fatalf("AttachDrive: %v", err) + } + if ops := rec.mountOps(); len(ops) != 0 { + t.Errorf("a healthy bind was disturbed: %v — the 20 s reconcile must stay a no-op", ops) + } +} + +// TestAttachDrive_UnknownLiveness_IsANoOp — cannot-tell must not trigger churn either. An unreadable +// mount table making the agent umount and re-bind every 20 s would be a self-inflicted outage. +func TestAttachDrive_UnknownLiveness_IsANoOp(t *testing.T) { + // Host table healthy (so n == 1) but on a filesystem whose abort vocabulary we cannot read. + rec := attachFixture(t, mountinfoUnknownFS, + `759 1176 8:16 /felhom-data /mnt/felhom-drives/r117sd rw,relatime shared:459 master:450 - btrfs /dev/sdb rw +`) + b := attachBinder(rec) + + if _, err := b.AttachDrive(context.Background(), 9301, "/mnt/r117sd"); err != nil { + t.Fatalf("AttachDrive: %v", err) + } + if ops := rec.mountOps(); len(ops) != 0 { + t.Errorf("an UNKNOWN verdict caused a re-bind: %v — cannot-tell must never churn a live mount", ops) + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 4a5e652..8e93f2c 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -324,6 +324,15 @@ type Server struct { // Optional — nil defaults to isHostMountpoint; tests inject a fake. deviceCheck func(string) bool + // livenessCheck answers whether the bind at a stable guest path is USABLE, not merely present — the + // third term of the BoundUnderParent conjunction (R-117). Deliberately separate from boundCheck and + // deviceCheck because it is the only one of the three that compares them: boundCheck asks "does the + // guest see a mount by that name", deviceCheck asks "is the raw mount still there", and BOTH are + // satisfied by a bind that names the drive that went away while the raw mount healed onto the + // returning one. Optional — nil defaults to bindLiveness. Prefer redirecting procSelfMountinfo at a + // captured fixture over injecting here: that exercises the real parser and predicate. + livenessCheck func(stable, raw string) BindLiveness + jobsMu sync.Mutex // jobs is per-guest-PER-TARGET backup job state (slice 8B; keyed by target too since R-82). // Keying by vmid alone would let a PBS backup started inside the same quiesce window collide