v0.117.0 — R-117: the liveness signal now tests liveness

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 parse a mountinfo line and then test only fields[4], the mount POINT. Field 3 — major:minor — sat in the same parsed slice and was discarded. 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 and emailing backup_target_restored with no alarm on any channel. BoundUnderParent gains a third term at both /disks construction sites. The new bindLiveness reads /proc only and asks two questions: the bind must name the same device as the raw mount, and the filesystem must not have aborted (ext4 `shutdown` or `emergency_ro`). The second check is not optional. A device that fails WITHOUT disappearing gives 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 and nothing is emitted on any channel, indefinitely (R-117a). A devno-only fix would have passed every payload test. Three states, never a bool: {Unknown, Live, StaleDevice, Aborted}, read through Usable(), where Unknown counts as PRESENT — reporting absent stops a working customer's apps. No new recovery path; the existing one was unblocked. AttachDrive's normalize leg already did the repair and three call sites already invoked it, including the controller's Return branch before it restarts apps. All three died on `if n == 1 && GuestSeesMount(...)` returning early. Now: StaleDevice ⇒ re-bind (repairs live, guest never restarts); Aborted ⇒ quiet no-op, because a re-bind lands on the same dead superblock and this runs every 20s — an infinite silent retry that masks the state; it surfaces via BoundUnderParent=false instead. 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. The abort flag is read off the RAW mount in the stale case. Tests 849 → 863, 29/29 packages green. 6 red-proofs, each verified to have landed. A hollow test was caught during them: 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.
2026-07-30 12:26:42 +02:00
parent 6be168d1a0
commit 966d8f41ff
7 changed files with 947 additions and 46 deletions
@@ -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/<guest-init-pid>/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/<pid>/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/<name>). 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
 }