Name a filesystem by its kernel id, never its device name (hq ADR 0325 fourth review)

An ext4 or xfs library mounted by UUID can come back from a reboot as another device
(/dev/sdb1 as /dev/sdc1); naming the filesystem by mountinfo's source read that as a
move, stripped the warranted library's entries and asked for a new Approve. A
filesystem is now named by statfs's f_fsid (from the UUID, or the ZFS dataset), its
type and its mount point. Entries left on the filesystem a folder is on now are
merged back or held, never walked away, and a TakenOn naming a mount point is not
taken.

internal/apply/keep.go

@@ -256,7 +256,7 @@ func assessFolder(ctx context.Context, path string, rec store.KeptFolder, taken
 	}
 	// The controller's word that the folder was taken holds for the filesystem the warrant was carried out on: a
 	// folder moved to another is not taken, and asks again (§8).
-	takenHere := taken && (takenOnID(rec.TakenOn) == here || (rec.TakenOn == "" && !moved))
+	takenHere := taken && (rec.TakenOn == here || (rec.TakenOn == "" && !moved))
 	switch {
 	case rec.Kept && !moved:
 		return folderVerdict{kind: folderKeep, mount: pl.Mount, how: rec.How}
@@ -279,24 +279,16 @@ func assessFolder(ctx context.Context, path string, rec store.KeptFolder, taken
 func movedRecord(f store.KeptFolder, mount string) store.KeptFolder {
 	left := f.Left
 	if f.How == store.KeptByWarrant && len(f.Users) > 0 && f.Mount != "" {
-		left = addLeft(left, f.Mount, f.Users)
+		left = addLeft(left, f.Mount, f.Placed, f.Users)
 	}
 	return store.KeptFolder{Path: f.Path, Mount: mount, Placed: PlacedID(mount), Want: f.Want, IDs: f.IDs,
 		Act: f.Act, TakenOn: f.TakenOn, Left: left}
 }
 
-// takenOnID is a record's TakenOn as a filesystem's name: one written before names held the mount point itself.
-func takenOnID(takenOn string) string {
-	if strings.HasPrefix(takenOn, "/") {
-		return PlacedID(takenOn)
-	}
-	return takenOn
-}
-
 // addLeft adds users' entries left on mount, once per mount and uid.
-func addLeft(left []store.KeptLeft, mount string, users []store.KeptUser) []store.KeptLeft {
+func addLeft(left []store.KeptLeft, mount, placed string, users []store.KeptUser) []store.KeptLeft {
 	for i := range left {
-		if left[i].Mount == mount {
+		if left[i].Mount == mount && left[i].Placed == placed {
 			have := usersOf(left[i].Users)
 			for _, u := range users {
 				if _, had := have[u.UID]; !had {
@@ -306,20 +298,37 @@ func addLeft(left []store.KeptLeft, mount string, users []store.KeptUser) []stor
 			return left
 		}
 	}
-	return append(left, store.KeptLeft{Mount: mount, Users: append([]store.KeptUser(nil), users...)})
+	return append(left, store.KeptLeft{Mount: mount, Placed: placed, Users: append([]store.KeptUser(nil), users...)})
 }
 
 // PlacedID names the filesystem at mount for a wait, with no path in it: the controller asks again when the
-// folder's filesystem is not the one a warrant was given for.
+// folder's filesystem is not the one a warrant was given for. It is built from what stays the same across reboots
+// for one filesystem — the kernel's filesystem id (statfs f_fsid: ext4 and xfs derive it from the filesystem's
+// UUID, ZFS from the dataset), the filesystem's type and its mount point — and never from the device name
+// mountinfo gives as its source, which a reboot may renumber (/dev/sdb1 becoming /dev/sdc1).
 func PlacedID(mount string) string {
 	if mount == "" {
 		return ""
 	}
-	source, fstype := mountSourceOf(mount)
-	sum := sha256.Sum256([]byte(mount + "\x00" + source + "\x00" + fstype))
+	_, fstype := mountSourceOf(mount)
+	fsid := fsidWithin(mount)
+	sum := sha256.Sum256([]byte(mount + "\x00" + fsid + "\x00" + fstype))
 	return "fs-" + hex.EncodeToString(sum[:6])
 }
 
+// fsidWithin is MountFsid within statBound: empty when it does not answer, so a mount that is down is named by its
+// point and type alone, never held on.
+func fsidWithin(mount string) string {
+	done := make(chan string, 1)
+	go func() { done <- MountFsid(mount) }()
+	select {
+	case id := <-done:
+		return id
+	case <-time.After(statBound):
+		return ""
+	}
+}
+
 // statBound is how long a read of a folder may take before it is said not to answer: a hard network mount whose
 // server is down blocks a stat for ever, and one access must never hold the apply.
 var statBound = 5 * time.Second
@@ -534,7 +543,6 @@ func applyAccessFolder(ctx context.Context, r *declaration.Access, in inputs) (O
 			// The controller says it was taken, and this engine has no record of where: here, now.
 			f.TakenOn = PlacedID(v.mount)
 		}
-		f.TakenOn = takenOnID(f.TakenOn)
 		if first {
 			f.How, f.At = v.how, time.Now().UTC()
 			// A folder that held nothing has nothing under it to walk; one the controller says was taken

internal/apply/keeper_linux.go

@@ -87,14 +87,24 @@ func dropLeft(path string, f store.KeptFolder) []string {
 	if _, err := lstatWithin(path); err == nil {
 		holding = mountHolding(path, MountPoints())
 	}
+	holdingID := ""
+	if holding != "" {
+		holdingID = PlacedID(holding)
+	}
 	var said []string
 	var still []store.KeptLeft
 	var mine []store.KeptUser
 	for _, l := range f.Left {
+		same := l.Mount == f.Mount && (l.Placed == "" || l.Placed == f.Placed)
 		switch {
-		case f.Kept && l.Mount == f.Mount:
+		case f.Kept && same:
+			// Left on the filesystem the folder is kept on now: the folder's own entries.
 			mine = append(mine, l.Users...)
-		case l.Mount != holding:
+		case same:
+			// On the filesystem the folder is on now, not kept yet: never walked away; they are the folder's
+			// once it is kept there again.
+			still = append(still, l)
+		case l.Mount != holding || (l.Placed != "" && l.Placed != holdingID):
 			still = append(still, l)
 		default:
 			drop := map[uint32]bool{}

internal/apply/kept_test.go

@@ -1028,24 +1028,31 @@ func TestALeftoverWithNoAccessIdsIsSaidUnderAPathFreeName(t *testing.T) {
 	}
 }
 
-// Review (third) low: another filesystem mounted at the same point is not the one the warrant was for.
-func TestAnotherFilesystemAtTheSameMountPointIsNotTaken(t *testing.T) {
+// placedPool is a folder of a library on a pool the test says is mounted, with the pool's device name and filesystem
+// id as the test sets them, taken under a warrant.
+func placedPool(t *testing.T, source, fsid *string) (pool, lib, taken string) {
+	t.Helper()
 	dir := aclFolder(t)
-	pool := filepath.Join(dir, "pool")
-	lib := filepath.Join(pool, "media")
+	pool = filepath.Join(dir, "pool")
+	lib = filepath.Join(pool, "media")
 	if err := os.MkdirAll(filepath.Join(lib, "old"), 0o755); err != nil {
 		t.Fatal(err)
 	}
 	keeping(t, []string{pool}, nil)
-	source := "tank/a"
-	was := MountSource
+	wasSource, wasFsid := MountSource, MountFsid
 	MountSource = func(point string) (string, string) {
 		if point == pool {
-			return source, "zfs"
+			return *source, "ext4"
 		}
 		return "", ""
 	}
-	defer func() { MountSource = was }()
+	MountFsid = func(point string) string {
+		if point == pool {
+			return *fsid
+		}
+		return ""
+	}
+	t.Cleanup(func() { MountSource, MountFsid = wasSource, wasFsid })
 	notTaken := accessJSON("radarr.media", lib, "read-write", false, "mesh-radarr:61001:read-write:container")
 	if _, _, err := applyAccesses(t, store.State{}, notTaken); err != nil {
 		t.Fatal(err)
@@ -1055,11 +1062,18 @@ func TestAnotherFilesystemAtTheSameMountPointIsNotTaken(t *testing.T) {
 		t.Fatal(err)
 	}
 	<-done
-	taken := accessJSON("radarr.media", lib, "read-write", true, "mesh-radarr:61001:read-write:container")
+	taken = accessJSON("radarr.media", lib, "read-write", true, "mesh-radarr:61001:read-write:container")
 	if report, _, _ := applyAccesses(t, store.State{}, taken); outcomeOf(report, "radarr.media").Action == "waiting" {
 		t.Fatal("the folder taken waits on its own filesystem")
 	}
-	source = "tank/b" // another pool imported at the same mount point
+	return pool, lib, taken
+}
+
+// Review (third) low: another filesystem mounted at the same point is not the one the warrant was for.
+func TestAnotherFilesystemAtTheSameMountPointIsNotTaken(t *testing.T) {
+	source, fsid := "/dev/sdb1", "0a0b0c0d01020304"
+	_, lib, taken := placedPool(t, &source, &fsid)
+	fsid = "1111222233334444" // another filesystem mounted at the same point
 	report, _, err := applyAccesses(t, store.State{}, taken)
 	if err != nil {
 		t.Fatal(err)
@@ -1072,6 +1086,28 @@ func TestAnotherFilesystemAtTheSameMountPointIsNotTaken(t *testing.T) {
 	}
 }
 
+// Review (fourth): the same filesystem whose device the kernel renamed across a reboot (/dev/sdb1 to /dev/sdc1, one
+// filesystem id) is the same filesystem: still taken, still kept, and none of its entries walked away.
+func TestADeviceRenamedIsTheSameFilesystem(t *testing.T) {
+	source, fsid := "/dev/sdb1", "0a0b0c0d01020304"
+	_, lib, taken := placedPool(t, &source, &fsid)
+	source = "/dev/sdc1"
+	report, _, err := applyAccesses(t, store.State{}, taken)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if o := outcomeOf(report, "radarr.media"); o.Action == "waiting" {
+		t.Fatalf("a device renamed reads as another filesystem: %+v", o)
+	}
+	ReconcileKept(context.Background())
+	if got := effectiveOn(t, filepath.Join(lib, "old"), radarr); got != 7 {
+		t.Fatalf("the library's entries were walked away: the identity may %s", permWords(got))
+	}
+	if left := Leftovers(); len(left) != 0 {
+		t.Fatalf("a device renamed leaves entries said as left: %+v", left)
+	}
+}
+
 // Review L4: the controller's word that a folder was taken holds for the filesystem the warrant was carried out on;
 // moved to another, it is not taken, the wait names the new filesystem, and nothing is walked.
 func TestAMovedFolderIsNotTakenOnTheOldWarrant(t *testing.T) {
@@ -1148,3 +1184,48 @@ func TestAHungReadIsSharedByEveryoneWhoAsks(t *testing.T) {
 		t.Fatalf("%d reads of one path were started while the first hung", started)
 	}
 }
+
+// Review (fourth): entries left on the filesystem the folder is on now are never walked away — merged back into
+// the folder's users when it is kept there, held while it is not.
+func TestEntriesLeftOnTheFilesystemTheFolderIsOnAreNeverWalkedAway(t *testing.T) {
+	source, fsid := "/dev/sdb1", "0a0b0c0d01020304"
+	pool, lib, _ := placedPool(t, &source, &fsid)
+	rec, _ := keptRecord(lib)
+	users := rec.Users
+	for _, kept := range []bool{false, true} {
+		_ = KeptFolders.Update(lib, func(f *store.KeptFolder, _ bool) bool {
+			f.Kept, f.Users = kept, nil
+			f.Left = []store.KeptLeft{{Mount: pool, Placed: f.Placed, Users: users}}
+			return true
+		})
+		ReconcileKept(context.Background())
+		if got := effectiveOn(t, filepath.Join(lib, "old"), radarr); got != 7 {
+			t.Fatalf("kept=%v: entries on the folder's own filesystem were walked away", kept)
+		}
+		r, _ := keptRecord(lib)
+		if kept && (len(r.Left) != 0 || len(r.Users) != 1) {
+			t.Errorf("kept: not merged back into the folder's users: %+v", r)
+		}
+		if !kept && len(r.Left) != 1 {
+			t.Errorf("not kept: the entries' record was dropped: %+v", r)
+		}
+	}
+}
+
+// Review (fourth): a TakenOn naming a mount point (an unmerged build's record) is not taken on whatever is mounted
+// there now.
+func TestATakenOnOfTheOldFormIsNotTaken(t *testing.T) {
+	dir := aclFolder(t)
+	keeping(t, nil, nil)
+	lib := filepath.Join(dir, "media")
+	if err := os.MkdirAll(filepath.Join(lib, "old"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	_ = KeptFolders.Update(lib, func(f *store.KeptFolder, _ bool) bool {
+		f.Mount, f.TakenOn = "/", "/"
+		return true
+	})
+	if v := AccessVerdict(context.Background(), lib, true, true); v.Kind != VerdictWaitKeep {
+		t.Fatalf("an old-form record is read as taken: %s", v.Kind)
+	}
+}

internal/apply/walk_linux.go

@@ -538,3 +538,13 @@ func createPath(path string, stats walkStats) (int, error) {
 	unix.Close(fd)
 	return made, nil
 }
+
+// MountFsid is the kernel's id of the filesystem at point (statfs f_fsid), in hex; empty when it cannot be read.
+// A variable so a test can stand in for a filesystem it cannot mount.
+var MountFsid = func(point string) string {
+	var st unix.Statfs_t
+	if err := unix.Statfs(point, &st); err != nil {
+		return ""
+	}
+	return fmt.Sprintf("%08x%08x", uint32(st.Fsid.Val[0]), uint32(st.Fsid.Val[1]))
+}

internal/apply/walk_other.go

@@ -53,3 +53,6 @@ type Watcher struct{}
 func NewWatcher() (*Watcher, error)              { return nil, errNoKeeping }
 func (*Watcher) Set(map[string][]store.KeptUser) {}
 func (*Watcher) Run(context.Context)             {}
+
+// MountFsid reads no filesystem id where a folder is not kept.
+var MountFsid = func(string) string { return "" }

internal/store/kept.go

@@ -66,7 +66,7 @@ type KeptFolder struct {
 	Placed string `json:"placed,omitempty"`
 	// TakenOn names, as Placed does, the filesystem the operator's warrant was carried out on (ADR 0325 §8): the
 	// controller's word that the folder was taken holds for that filesystem alone, and a folder moved to another
-	// asks again. A record written before held the mount point here, and is read as that mount's name.
+	// asks again. One that names no filesystem this way (a mount point, from an unmerged build) is not taken.
 	TakenOn string `json:"taken_on,omitempty"`
 	// Left is the entries the mesh added on a filesystem no longer at the folder (a disk mounted over it, or
 	// one gone): taken out by a drop walk once that filesystem holds the folder again, said until then.
@@ -83,8 +83,11 @@ type KeptFolder struct {
 
 // KeptLeft is the mesh's entries left on one filesystem: its mount point and whose entries they are.
 type KeptLeft struct {
-	Mount string     `json:"mount"`
-	Users []KeptUser `json:"users"`
+	Mount string `json:"mount"`
+	// Placed names that filesystem as KeptFolder.Placed does: entries left on the filesystem the folder is on
+	// now are the folder's own, never walked away.
+	Placed string     `json:"placed,omitempty"`
+	Users  []KeptUser `json:"users"`
 }
 
 // KeptAct is one walk under a kept folder, as the engine records it. Counts only: never a path.