mounts: test the clearing loop, the look's own clearing and a disk's presence; clear only a taken-over unit's earlier failure; clear and re-arm an automount that gave up; refuse a transient unit of the same name (review of #162)

modules/mounts/README.md

@@ -48,7 +48,10 @@ off, or a node's share whose server is away, tried when someone opened the mount
 carried before the module wrote it (the unit fstab's generator makes until the line is adopted). The module
 says such a share as `absent` or `unreachable`; left failed, the unit would also be said as a failed unit of
 the machine's own, with a notification each time it is touched. A mount that failed while its source answers
-is left failed: that is a fault, and the share is said `failed`.
+is left failed: that is a fault, and the share is said `failed`. An automount that gave up after repeated
+failed mounts (`mount-start-limit-hit`) while its source was away is cleared and armed again the same way.
+Only a unit the module takes over has its earlier failure cleared regardless of the source; a pair of its
+own that it rewrites keeps its failure.
 
 A share is read-only when the setting says `:ro` or the sharing machine exports it read-only. A
 read-write share is mounted hard, so a write is never dropped while the server is away.
@@ -110,6 +113,8 @@ Each refusal is said as the share's state `failed`, with why, and nothing is wri
 - a mount point the system owns (`/`, `/etc`, `/usr`, `/boot`, `/proc`, `/sys`, `/dev`, `/run` and what
   is under them, and `/var`, `/home`, `/root`, `/tmp` themselves);
 - a mount point that something else is mounted at, or an automount that is not the module's;
+- a mount point whose unit someone made at run time (`/run/systemd/transient`, from `systemd-mount` or the
+  like): that is somebody's mount now;
 - a folder that holds files: a mount would hide them. Only a folder not in the kernel's mount table is
   opened, so a server that is away cannot hold the look;
 - a share that this machine serves itself.

modules/mounts/cmd/mounts/apply.go

@@ -97,6 +97,8 @@ type Applier struct {
 	// written is every pair the last pass wrote and armed, by its mount unit: what clearFailed looks at
 	// between passes. The acting process's own memory; nothing else reads it.
 	written map[string]Pair
+	// LookInterval and ClearInterval are LookEvery and ClearEvery, or a test's own when set.
+	LookInterval, ClearInterval time.Duration
 }
 
 // NewApplier is the process on this machine.
@@ -334,7 +336,9 @@ func (a *Applier) Look(ctx context.Context) Status {
 	own := a.ownUnits()
 	mounted := ParseMountinfo(func() string { t, _ := a.M.read("/proc/self/mountinfo"); return t }())
 	changed := false
-	fresh := map[string]bool{} // the mount units this pass wrote anew
+	// The mount units this pass wrote where no unit file of the module's was before: a unit of the same
+	// name that was there — the one fstab's generator makes until the line is adopted — is taken over.
+	takenOver := map[string]bool{}
 	var written []Pair
 	for _, p := range pairs {
 		ours := own[p.MountUnit] && own[p.AutoUnit]
@@ -371,9 +375,11 @@ func (a *Applier) Look(ctx context.Context) Status {
 				State: StateFailed, Why: fmt.Sprintf("its units could not be written: %v %v", err1, err2)}
 			continue
 		}
+		if c1 && !own[p.MountUnit] {
+			takenOver[p.MountUnit] = true
+		}
 		if c1 || c2 {
 			changed = true
-			fresh[p.MountUnit] = true
 			st.Changed = append(st.Changed, "the units of "+p.Name)
 		}
 		// A missing mount point is made; one that is there is left exactly as it is. An automount point
@@ -459,11 +465,12 @@ func (a *Applier) Look(ctx context.Context) Status {
 		}
 	}
 	if a.Act {
-		// A mount unit this pass wrote anew over one that had failed — the unit fstab's generator made for
-		// the same mount point, before the line was adopted — carries a failure that was not the module's
-		// pair's. It is cleared, so the pair starts armed and is judged on its own mounts (ADR 0263 rule 4).
+		// A mount unit the module wrote for the first time, over a unit of the same name that had already
+		// failed — the one fstab's generator makes until the line is adopted — carries a failure that is not
+		// the module's pair's. It is cleared, so the pair starts armed and is judged on its own mounts (ADR
+		// 0263 rule 4). A pair the module already had and rewrote keeps its failure: that one is its own.
 		for _, p := range written {
-			if fresh[p.MountUnit] && states[p.MountUnit]["ActiveState"] == "failed" {
+			if takenOver[p.MountUnit] && states[p.MountUnit]["ActiveState"] == "failed" {
 				if _, err := a.M.Run(ctx, "systemctl", "reset-failed", p.MountUnit); err == nil {
 					st.Changed = append(st.Changed, "cleared "+p.MountUnit+", which had failed before the module wrote it")
 				}
@@ -549,6 +556,12 @@ func (a *Applier) finish(st *Status, notSet map[string]ShareStatus) {
 // foreign refuses a pair whose unit file exists and is not the module's.
 func (a *Applier) foreign(p Pair, own map[string]bool) error {
 	for _, unit := range []string{p.MountUnit, p.AutoUnit} {
+		// A transient unit of the same name — made by `systemd-mount` or `mount` on the command line — is
+		// someone's mount now, whatever the module's own file says.
+		transient := filepath.Join(TransientDir, unit)
+		if _, err := os.Lstat(a.path(transient)); err == nil {
+			return fmt.Errorf("%s has a transient unit that is not the module's (%s); it is left as it is, and this share is not mounted here", p.Mountpoint, transient)
+		}
 		if own[unit] {
 			continue
 		}
@@ -559,6 +572,9 @@ func (a *Applier) foreign(p Pair, own map[string]bool) error {
 	return nil
 }
 
+// TransientDir is where the service manager keeps the units made at run time rather than from a file.
+const TransientDir = "/run/systemd/transient"
+
 // judge is one pair's state. The source is tried first, without touching the mount point.
 func (a *Applier) judge(p Pair, states map[string]map[string]string, mounted map[string]Mounted) (string, string) {
 	there := true
@@ -607,23 +623,37 @@ func (a *Applier) clearFailed(ctx context.Context) []string {
 	if len(a.written) == 0 {
 		return nil
 	}
-	var units []string
-	for unit := range a.written {
-		units = append(units, unit)
+	var mounts, units []string
+	for unit, p := range a.written {
+		mounts = append(mounts, unit)
+		units = append(units, unit, p.AutoUnit)
 	}
-	sort.Strings(units)
+	sort.Strings(mounts)
 	states := a.unitStates(ctx, units)
 	var cleared []string
-	for _, unit := range units {
-		if states[unit]["ActiveState"] != "failed" {
+	for _, unit := range mounts {
+		p := a.written[unit]
+		mountFailed := states[unit]["ActiveState"] == "failed"
+		// After repeated failed mounts the automount gives up too (mount-start-limit-hit) and stops
+		// answering its point: cleared and armed again, or the share stays dead until the next pass.
+		autoFailed := states[p.AutoUnit]["ActiveState"] == "failed"
+		if !mountFailed && !autoFailed {
 			continue
 		}
-		p := a.written[unit]
 		if a.sourceAnswers(p) {
 			continue
 		}
-		if _, err := a.M.Run(ctx, "systemctl", "reset-failed", unit); err == nil {
-			cleared = append(cleared, "cleared "+unit+": "+p.Name+" was not there to mount")
+		if mountFailed {
+			if _, err := a.M.Run(ctx, "systemctl", "reset-failed", unit); err == nil {
+				cleared = append(cleared, "cleared "+unit+": "+p.Name+" was not there to mount")
+			}
+		}
+		if autoFailed {
+			if _, err := a.M.Run(ctx, "systemctl", "reset-failed", p.AutoUnit); err == nil {
+				if _, err := a.M.Run(ctx, "systemctl", "start", p.AutoUnit); err == nil {
+					cleared = append(cleared, "cleared and armed "+p.AutoUnit+" again ("+orDefault(states[p.AutoUnit]["Result"], "failed")+"): "+p.Name+" was not there to mount")
+				}
+			}
 		}
 	}
 	return cleared
@@ -645,7 +675,7 @@ func RunApply(ctx context.Context, a *Applier) {
 		for _, c := range st.Changed {
 			fmt.Fprintln(os.Stderr, "mounts:", c)
 		}
-		next := time.After(LookEvery)
+		next := time.After(orInterval(a.LookInterval, LookEvery))
 	between:
 		for {
 			select {
@@ -653,7 +683,7 @@ func RunApply(ctx context.Context, a *Applier) {
 				return
 			case <-next:
 				break between
-			case <-time.After(ClearEvery):
+			case <-time.After(orInterval(a.ClearInterval, ClearEvery)):
 				if a.Act {
 					for _, c := range a.clearFailed(ctx) {
 						fmt.Fprintln(os.Stderr, "mounts:", c)
@@ -663,3 +693,10 @@ func RunApply(ctx context.Context, a *Applier) {
 		}
 	}
 }
+
+func orInterval(d, otherwise time.Duration) time.Duration {
+	if d > 0 {
+		return d
+	}
+	return otherwise
+}

modules/mounts/cmd/mounts/failed_test.go

@@ -99,3 +99,140 @@ func TestOnlyTheActingProcessClears(t *testing.T) {
 		t.Errorf("the judging process changes nothing: %q", f.calls)
 	}
 }
+
+
+// waitFor polls until cond holds or the deadline passes.
+func waitFor(t *testing.T, within time.Duration, cond func() bool) bool {
+	t.Helper()
+	deadline := time.Now().Add(within)
+	for time.Now().Before(deadline) {
+		if cond() {
+			return true
+		}
+		time.Sleep(5 * time.Millisecond)
+	}
+	return cond()
+}
+
+func TestTheActingProcessClearsBetweenItsLooks(t *testing.T) {
+	f := newSystem(t)
+	f.write(ConfigFile, libraryOnly)
+	f.write(ExportsFile, exportsAnswer(t, library))
+	a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC))
+	a.Judge = false
+	// The next look is an hour away: only the loop between looks can clear what fails now.
+	a.LookInterval, a.ClearInterval = time.Hour, 10*time.Millisecond
+	ctx, cancel := context.WithCancel(context.Background())
+	done := make(chan struct{})
+	go func() { RunApply(ctx, a); close(done) }()
+	defer func() { cancel(); <-done }()
+
+	if !waitFor(t, 2*time.Second, func() bool { return f.called("systemctl enable --now srv-library.automount") }) {
+		t.Fatal("the first look did not arm the pair")
+	}
+	// The server is away and someone opened the mount point: the mount failed after the look.
+	f.setUnit("srv-library.mount", map[string]string{"ActiveState": "failed", "Result": "timeout"})
+	if !waitFor(t, 2*time.Second, func() bool { return f.called("systemctl reset-failed srv-library.mount") }) {
+		t.Fatal("a mount that failed between looks, its server away, is cleared before the next look")
+	}
+}
+
+func TestALookClearsAMountOfItsOwnThatFailedWhileItsSourceWasAway(t *testing.T) {
+	f := newSystem(t)
+	f.write(ConfigFile, libraryOnly)
+	f.write(ExportsFile, exportsAnswer(t, library))
+	a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC))
+	a.Look(context.Background())
+	f.setUnit("srv-library.mount", map[string]string{"ActiveState": "failed", "Result": "timeout"})
+	f.calls = nil
+
+	// A second look, with nothing to write: the pair is the module's own already, so this is no takeover.
+	a.Look(context.Background())
+	if !f.called("systemctl reset-failed srv-library.mount") {
+		t.Errorf("the look itself clears it: %q", f.calls)
+	}
+}
+
+func TestAnAbsentDiskIsClearedAndAPresentOneIsNot(t *testing.T) {
+	f := newSystem(t)
+	f.write(ConfigFile, "sources=usb=disk:1234-ABCD@/mnt/usb\nnode=workstation\n")
+	a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC))
+	a.Look(context.Background())
+
+	f.setUnit("mnt-usb.mount", map[string]string{"ActiveState": "failed", "Result": "timeout"})
+	f.calls = nil
+	if a.clearFailed(context.Background()); !f.called("systemctl reset-failed mnt-usb.mount") {
+		t.Errorf("the disk is not plugged in: its failed mount is cleared: %q", f.calls)
+	}
+
+	// Plugged in, and the mount still failed: a fault, left as it is.
+	f.write("/dev/disk/by-uuid/1234-ABCD", "")
+	f.setUnit("mnt-usb.mount", map[string]string{"ActiveState": "failed", "Result": "exit-code"})
+	f.calls = nil
+	if a.clearFailed(context.Background()); f.called("systemctl reset-failed") {
+		t.Errorf("a disk that is there and failed to mount is left failed: %q", f.calls)
+	}
+}
+
+func TestARewrittenPairOfItsOwnKeepsItsFailure(t *testing.T) {
+	f := newSystem(t)
+	f.write(ConfigFile, libraryOnly)
+	f.write(ExportsFile, exportsAnswer(t, library))
+	f.reachable["192.0.2.10:2049"] = true
+	a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC))
+	a.Look(context.Background())
+
+	// The server moved the library and refuses the mount while it answers: the module's own failure.
+	f.setUnit("srv-library.mount", map[string]string{"ActiveState": "failed", "Result": "exit-code"})
+	moved := library
+	moved.Path = "/tank/library"
+	f.write(ExportsFile, exportsAnswer(t, moved))
+	f.calls = nil
+	a.Look(context.Background())
+	if !strings.Contains(f.read(UnitDir+"/srv-library.mount"), "What=192.0.2.10:/tank/library") {
+		t.Fatal("the pair was not rewritten")
+	}
+	if f.called("systemctl reset-failed") {
+		t.Errorf("only a unit the module takes over has its failure cleared, not one of its own it rewrites: %q", f.calls)
+	}
+}
+
+func TestAnAutomountThatGaveUpIsClearedAndArmedAgain(t *testing.T) {
+	f := newSystem(t)
+	f.write(ConfigFile, libraryOnly)
+	f.write(ExportsFile, exportsAnswer(t, library))
+	a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC))
+	a.Look(context.Background())
+
+	f.setUnit("srv-library.automount", map[string]string{"ActiveState": "failed", "Result": "mount-start-limit-hit", "UnitFileState": "enabled"})
+	f.calls = nil
+	cleared := a.clearFailed(context.Background())
+	if !f.called("systemctl reset-failed srv-library.automount") || !f.called("systemctl start srv-library.automount") {
+		t.Fatalf("the automount is cleared and started: %q", f.calls)
+	}
+	if !strings.Contains(strings.Join(cleared, ";"), "mount-start-limit-hit") {
+		t.Errorf("said with its result: %q", cleared)
+	}
+
+	// With the server answering, an automount that gave up is a fault: left as it is.
+	f.reachable["192.0.2.10:2049"] = true
+	f.setUnit("srv-library.automount", map[string]string{"ActiveState": "failed", "Result": "mount-start-limit-hit", "UnitFileState": "enabled"})
+	f.calls = nil
+	if a.clearFailed(context.Background()); f.called("systemctl reset-failed") || f.called("systemctl start") {
+		t.Errorf("left failed while the server answers: %q", f.calls)
+	}
+}
+
+func TestATransientUnitOfTheSameNameIsNotTheModules(t *testing.T) {
+	f := newSystem(t)
+	f.write(ConfigFile, libraryOnly)
+	f.write(ExportsFile, exportsAnswer(t, library))
+	f.write(TransientDir+"/srv-library.mount", "[Mount]\nWhat=elsewhere\n")
+	states := failedLook(t, f)
+	if s := states["library"]; s.State != StateFailed || !strings.Contains(s.Why, "transient") {
+		t.Errorf("a share whose unit someone made at run time is refused: %+v", s)
+	}
+	if f.called("systemctl enable --now srv-library.automount") {
+		t.Error("and not armed")
+	}
+}

modules/mounts/cmd/mounts/write_test.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 )
@@ -16,6 +17,8 @@ const testPassword = "s3cr3t-Pa55"
 
 // fakeSystem is a machine under a temporary root with a recorded service manager.
 type fakeSystem struct {
+	// mu guards calls, units and reachable, for a test that runs RunApply beside itself.
+	mu    sync.Mutex
 	t     *testing.T
 	root  string
 	calls []string
@@ -55,6 +58,8 @@ func (f *fakeSystem) read(p string) string {
 }
 
 func (f *fakeSystem) run(_ context.Context, name string, args ...string) (string, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
 	f.calls = append(f.calls, name+" "+strings.Join(args, " "))
 	if name != "systemctl" || len(args) == 0 {
 		return "", nil
@@ -77,6 +82,12 @@ func (f *fakeSystem) run(_ context.Context, name string, args ...string) (string
 	case "enable":
 		u := args[len(args)-1]
 		f.units[u] = map[string]string{"ActiveState": "active", "UnitFileState": "enabled"}
+	case "start":
+		u := args[len(args)-1]
+		if f.units[u] == nil {
+			f.units[u] = map[string]string{}
+		}
+		f.units[u]["ActiveState"], f.units[u]["Result"] = "active", "success"
 	case "reset-failed":
 		u := args[len(args)-1]
 		if f.units[u]["ActiveState"] == "failed" {
@@ -93,6 +104,8 @@ func (f *fakeSystem) machine() *Machine {
 func (f *fakeSystem) applier(now time.Time) *Applier {
 	return &Applier{M: f.machine(), Act: true, Judge: true,
 		Dial: func(addr string, _ time.Duration) error {
+			f.mu.Lock()
+			defer f.mu.Unlock()
 			if f.reachable[addr] {
 				return nil
 			}
@@ -104,6 +117,8 @@ func (f *fakeSystem) applier(now time.Time) *Applier {
 }
 
 func (f *fakeSystem) called(prefix string) bool {
+	f.mu.Lock()
+	defer f.mu.Unlock()
 	for _, c := range f.calls {
 		if strings.HasPrefix(c, prefix) {
 			return true
@@ -565,3 +580,10 @@ func TestApplyActsAndWatchJudgesAndNeitherDoesTheOthersWork(t *testing.T) {
 		t.Error("the password leaked")
 	}
 }
+
+// setUnit sets one unit's state as the service manager answers it, under the lock.
+func (f *fakeSystem) setUnit(unit string, state map[string]string) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.units[unit] = state
+}