agent v0.37.0: re-assert mounted-but-DISABLED units (live felhom-usb fix)

The skip-if-mounted optimization defeated the actual root cause: felhom-usb is
mounted now but its unit is `disabled`, so a host reboot would not auto-mount
it. ReassertEnrolledMounts now skips ONLY the durable steady state (mounted AND
enabled) via the pure shouldReassertMount; a mounted-but-disabled unit is
re-asserted so enable --now re-creates the wants-symlink. Enabled-state read by
privilege-free Lstat of the multi-user.target.wants symlink (unitEnabled) — no
systemctl is-enabled subprocess, no new sudoers entry.

Tests: TestShouldReassertMount (4 combos), TestUnitEnabled (wants-symlink).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -18,16 +18,21 @@ already correct (by-UUID), but nothing re-asserted the unit at startup.
 - `storage.parseFelhomMountUnit` — pure inverse of `renderMountUnit` (Name/UUID/Where/Type/Options) keyed
   on a `Managed by felhom-agent` marker; ignores any foreign `.mount` unit.
 - `(*SudoHostOps).ReassertEnrolledMounts(ctx)` — at startup (BEFORE binding into the guest) and on the
-  periodic 20s tick: for each enrolled `.mount` unit not currently mounted (per `/proc/mounts`), re-resolve
+  periodic 20s tick: for each enrolled `.mount` unit, re-resolve by UUID and re-run `EnsureMount`
-  by UUID and re-run `EnsureMount` (idempotent `systemctl enable --now`) — re-enables a disabled unit AND
+  (idempotent `systemctl enable --now`) — re-enables a disabled unit AND mounts the CURRENT device by
-  mounts the CURRENT device by UUID, so a `/dev/sdX` reshuffle is a no-op. Already-mounted drives are
+  UUID, so a `/dev/sdX` reshuffle is a no-op. Skips ONLY the durable steady state (mounted AND enabled),
-  skipped (no daemon-reload churn); an absent UUID is skipped (re-asserts on a later tick).
+  via the pure `shouldReassertMount`; a **mounted-but-DISABLED** unit (the exact live felhom-usb bug — it
+  serves now but a reboot would not auto-mount it) is still re-asserted to re-create the wants-symlink.
+  Enabled-state is read with a privilege-free `os.Lstat` of the `multi-user.target.wants` symlink
+  (`unitEnabled`) — no `systemctl is-enabled` subprocess, no new sudoers entry. An absent UUID is skipped
+  (re-asserts on a later tick).
 - Wired in `main.go` ahead of `ReassertGuestBinds` so mounts are live before the guest binds re-assert.
 - Tests (Linux, seam the device-resolution): `TestResolveStorageDevice_ToleratesDeviceLetterMove`
   (UUID symlink moved sdb→sdc → resolves sdc; companion asserts the cached enroll-time node differs from
   the freshly-resolved one — a node-based remount would target the wrong device), `..._AbsentAndScheme`
   (absent UUID errors; only the `uuid:` scheme resolvable), `TestParseFelhomMountUnit` (render→parse
-  round-trip + rejects a foreign unit).
+  round-trip + rejects a foreign unit), `TestShouldReassertMount` (the four mounted/enabled combos — pins
+  the mounted-but-disabled re-assert), `TestUnitEnabled` (wants-symlink detection).
 
 **TASK A2 — verdict: enrolling a NEW drive does NOT need an LXC restart.** The enroll path lands on the
 live intermediary-mount `AttachDrive` (`/disks/guest-attach` → `handleDiskGuestAttach` → `AttachDrive`,

REPORT.md

@@ -28,10 +28,13 @@ rewrite the unit.
   errors when the UUID is absent so a caller skips a gone drive instead of fail-mounting.
 - `storage.parseFelhomMountUnit` (`mountunit.go`) — pure inverse of `renderMountUnit`, gated on a
   `Managed by felhom-agent` marker; extracts Name/UUID/Where/Type/Options, ignores foreign units.
-- `(*SudoHostOps).ReassertEnrolledMounts(ctx)` (`hostops.go`) — for each enrolled `.mount` unit
+- `(*SudoHostOps).ReassertEnrolledMounts(ctx)` (`hostops.go`) — for each enrolled `.mount` unit,
-  not in `/proc/mounts`, re-resolve by UUID and re-run `EnsureMount` (idempotent
+  re-resolve by UUID and re-run `EnsureMount` (idempotent `systemctl enable --now`). Skips ONLY the
-  `systemctl enable --now`). Re-enables a disabled unit AND mounts the current device by UUID;
+  durable steady state (mounted AND enabled), via the pure `shouldReassertMount`; a
-  already-mounted drives skipped (no daemon-reload churn), absent UUIDs skipped (retry next tick).
+  **mounted-but-DISABLED** unit (the exact live felhom-usb state — serves now, but a reboot would not
+  auto-mount it) is still re-asserted so the wants-symlink is re-created. Enabled-state read with a
+  privilege-free `os.Lstat` of the `multi-user.target.wants` symlink (`unitEnabled`) — no
+  `systemctl is-enabled` subprocess / no new sudoers entry. Absent UUIDs skipped (retry next tick).
 - `main.go` — runs `ReassertEnrolledMounts` at startup **before** `ReassertGuestBinds`, and again
   ahead of the bind re-assert on the periodic 20s tick, so host mounts are live before the guest
   binds re-establish.
@@ -46,6 +49,9 @@ rewrite the unit.
   (`/dev/sdb1`, `store:`, `byid:`, `""` all rejected).
 - `TestParseFelhomMountUnit` — `renderMountUnit`→`parseFelhomMountUnit` round-trip + rejects a
   non-felhom unit.
+- `TestShouldReassertMount` — the four mounted/enabled combinations; pins that mounted-but-disabled
+  (the live bug) re-asserts and only mounted+enabled is skipped.
+- `TestUnitEnabled` — wants-symlink presence ⇒ enabled (privilege-free `Lstat`).
 
 ## Task A2 — verdict: enrolling a NEW drive does NOT need an LXC restart
 

internal/storage/hostops.go

@@ -250,8 +250,15 @@ func (h *SudoHostOps) ReassertEnrolledMounts(ctx context.Context) {
 			continue
 		}
 		spec, ok := parseFelhomMountUnit(string(data))
-		if !ok || mounted[spec.Where] {
+		if !ok {
-			continue // not ours, or already mounted → nothing to do (no churn)
+			continue // not one of ours
+		}
+		// Re-assert unless the drive is BOTH mounted AND its unit enabled (the durable steady state).
+		// A mounted-but-DISABLED unit (the live felhom-usb bug: a prior detach left it disabled, so it
+		// would NOT auto-mount on the next host reboot) is re-asserted too — EnsureMount's enable --now
+		// re-creates the wants-symlink. Skipping a mounted-but-disabled unit would leave the reboot fragile.
+		if !shouldReassertMount(mounted[spec.Where], h.unitEnabled(e.Name())) {
+			continue // mounted + enabled → no churn
 		}
 		dev, derr := ResolveStorageDevice("uuid:" + spec.UUID)
 		if derr != nil {
@@ -262,10 +269,27 @@ func (h *SudoHostOps) ReassertEnrolledMounts(ctx context.Context) {
 			h.logger.Warn("storage: re-assert enrolled mount failed", "name", spec.Name, "where", spec.Where, "err", err)
 			continue
 		}
-		h.logger.Info("storage: re-asserted enrolled mount by UUID (enable --now)", "name", spec.Name, "where", spec.Where, "uuid", spec.UUID, "device", dev)
+		h.logger.Info("storage: re-asserted enrolled mount by UUID (enable --now)", "name", spec.Name, "where", spec.Where, "uuid", spec.UUID, "device", dev, "wasMounted", mounted[spec.Where])
 	}
 }
 
+// shouldReassertMount decides whether an enrolled mount needs re-asserting. Re-assert unless it is
+// BOTH currently mounted AND its unit enabled — the durable steady state. The mounted-but-disabled
+// case is the load-bearing one: the drive serves now but its unit has no wants-symlink, so a host
+// reboot would not auto-mount it; re-asserting re-enables it. Pure → unit-tested.
+func shouldReassertMount(mounted, enabled bool) bool {
+	return !(mounted && enabled)
+}
+
+// unitEnabled reports whether a WantedBy=multi-user.target unit is enabled, by checking for its
+// wants-symlink. A privilege-free os.Lstat (the systemd dirs are world-readable) — no subprocess and
+// no sudoers entry, consistent with the durable-id reads. A missing symlink (or any stat error) ==
+// not enabled, so the re-assert errs toward re-enabling rather than leaving a reboot fragile.
+func (h *SudoHostOps) unitEnabled(unitName string) bool {
+	_, err := os.Lstat(filepath.Join(h.unitDir, "multi-user.target.wants", unitName))
+	return err == nil
+}
+
 // mountedSet returns the set of currently-active mountpoints from /proc/mounts (mountpoint is field 2).
 // Best-effort: a read failure yields an empty set (every enrolled drive is then considered for
 // re-assert, which EnsureMount makes idempotent).

internal/storage/hostops_test.go

@@ -192,3 +192,52 @@ type errExitT int
 
 func (e errExitT) Error() string { return "exit status nonzero" }
 func errExit(code int) error     { return errExitT(code) }
+
+// TestShouldReassertMount pins the host-reboot re-assert decision. The load-bearing case is
+// mounted-but-DISABLED (the live felhom-usb bug): the drive serves now, but with no wants-symlink a
+// host reboot would not auto-mount it, so it MUST still be re-asserted (enable --now re-creates the
+// symlink). Only mounted+enabled is the durable steady state we skip.
+func TestShouldReassertMount(t *testing.T) {
+	cases := []struct {
+		mounted, enabled, want bool
+		name                   string
+	}{
+		{true, true, false, "mounted+enabled → durable, skip"},
+		{true, false, true, "mounted+DISABLED → re-assert (the live bug: reboot would not auto-mount)"},
+		{false, true, true, "unmounted+enabled → re-assert (mount it now)"},
+		{false, false, true, "unmounted+disabled → re-assert (enable + mount)"},
+	}
+	for _, c := range cases {
+		if got := shouldReassertMount(c.mounted, c.enabled); got != c.want {
+			t.Errorf("%s: shouldReassertMount(%v,%v)=%v want %v", c.name, c.mounted, c.enabled, got, c.want)
+		}
+	}
+}
+
+// TestUnitEnabled detects the WantedBy=multi-user.target wants-symlink with a privilege-free Lstat
+// (no systemctl subprocess), so the re-assert can tell a disabled unit from an enabled one.
+func TestUnitEnabled(t *testing.T) {
+	unitDir := t.TempDir()
+	ops := NewSudoHostOps(SudoHostOpsConfig{
+		Runner:   &recordingRunner{},
+		Bins:     Binaries{Systemctl: "/usr/bin/systemctl", Install: "/usr/bin/install"},
+		UnitDir:  unitDir,
+		StageDir: t.TempDir(),
+		Logger:   quietLogger(),
+	})
+	const unit = "mnt-felhom-usb.mount"
+	if ops.unitEnabled(unit) {
+		t.Fatal("a unit with no wants-symlink must report NOT enabled (the felhom-usb disabled state)")
+	}
+	wants := filepath.Join(unitDir, "multi-user.target.wants")
+	if err := os.MkdirAll(wants, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	// systemd points the wants-symlink at the real unit file; the target need not exist for Lstat.
+	if err := os.Symlink(filepath.Join(unitDir, unit), filepath.Join(wants, unit)); err != nil {
+		t.Skipf("symlink unsupported here (Windows privilege): %v", err)
+	}
+	if !ops.unitEnabled(unit) {
+		t.Fatal("a unit WITH a wants-symlink must report enabled")
+	}
+}