Merge pull request 'Grants compose each membership from the build that runs as well as the trunk's, so a running build keeps its reads (issue 1966, recovery)' (#178) from fix/1966-membership-keeps-running-reads into main

modules/mesh-grants/grants_test.go

@@ -4000,3 +4000,87 @@ func (g *seatGate) MayAct(seats ...string) bool {
 func short(d string) string {
 	return strings.TrimPrefix(d, "sha256:")[:min(12, len(strings.TrimPrefix(d, "sha256:")))]
 }
+
+// A membership is composed from the build each machine runs now, the want's and the trunk's, so none loses a read (issue 1966:
+// the vault build running on the node-engines read mesh-delivery-assigner.placement, which the trunk's manifest no longer does).
+func TestMembershipKeepsWhatTheRunningBuildReads(t *testing.T) {
+	reader := func(reads ...string) catalogue.Manifest {
+		return catalogue.Manifest{Module: "mesh-vault", Version: "1", RunsAs: "mesh-vault", Reads: reads,
+			OwnSecrets: catalogue.OwnSecrets{"broker": {Path: "/var/lib/mesh-vault/broker"}}}
+	}
+	owner := catalogue.Manifest{Module: "mesh-delivery-assigner", Version: "1", RunsAs: "mesh-delivery-assigner",
+		OwnSecrets: catalogue.OwnSecrets{"broker": {Path: "/var/lib/a/broker"}}}
+	rows := []struct {
+		name         string
+		had, running map[string]catalogue.Manifest
+		reads        []string
+		want         []string
+	}{
+		{"the trunk alone", nil, nil, []string{"mesh-config.values"}, []string{"mesh-config.values"}},
+		{"the build run reads what the trunk dropped", map[string]catalogue.Manifest{"a.mesh-vault": reader("mesh-delivery-assigner.placement")}, nil, []string{"mesh-config.values"}, []string{"mesh-config.values", "mesh-delivery-assigner.placement"}},
+		{"the want's build reads what the trunk dropped", nil, map[string]catalogue.Manifest{"a.mesh-vault": reader("mesh-delivery-assigner.placement")}, []string{"mesh-config.values"}, []string{"mesh-config.values", "mesh-delivery-assigner.placement"}},
+	}
+	for _, row := range rows {
+		t.Run(row.name, func(t *testing.T) {
+			in := grants.Inputs{Nodes: []string{"a"}, Assigned: map[string][]string{"a": {"mesh-vault", "mesh-delivery-assigner"}},
+				Manifests: map[string]catalogue.Manifest{"mesh-vault": reader(row.reads...), "mesh-delivery-assigner": owner},
+				Running:   map[string]catalogue.Manifest{}, Had: map[string]catalogue.Manifest{}}
+			maps.Copy(in.Running, row.running)
+			maps.Copy(in.Had, row.had)
+			recs, _ := grants.ListRecords(in)
+			var got []string
+			for _, s := range grants.Memberships(recs)["a.mesh-vault"].Membership.State {
+				if !s.Writes {
+					got = append(got, s.Name)
+				}
+			}
+			slices.Sort(got)
+			slices.Sort(row.want)
+			if !slices.Equal(got, row.want) {
+				t.Errorf("reads = %v, want %v", got, row.want)
+			}
+		})
+	}
+}
+
+// A pass reads each machine's have for the build it runs (issue 1966): its membership keeps what that older build reads, a
+// have or build that cannot be read holds the pass back, and the seats held are the ones that build claims (ADR 0660 §2).
+func TestPassComposesMembershipFromTheHaveBuild(t *testing.T) {
+	const old = `{"module":"mod-y","version":"1","own-secrets":{"broker":"/var/lib/y/broker"},"reads":["mod-x.kept","mod-x.old"]}`
+	runs := func(b *fakeBus) {
+		b.manifestOf("mod-y", yManifest)
+		digest := meshcatalogclient.ManifestDigest([]byte(old))
+		b.put(grants.ReadManifests, digest, meshcatalog.ManifestsByDigestValue{V: keys.Version, Module: "mod-y", Manifest: old})
+		fp := "src2:" + strings.Repeat("cd", 32)
+		b.put(grants.ReadHave, "b.mod-y", nodeengine.HaveByNodeModuleValue{V: keys.Version, Phase: nodeengineclient.PhaseRunning, Health: nodeengineclient.HealthOK, Fp: fp})
+		key, _ := builds.BuilderBuildKey("mod-y", fp)
+		b.put(grants.ReadBuilds, key, map[string]any{"v": keys.Version, "state": "done", "manifest": digest})
+	}
+	t.Run("the published membership keeps a read of the build the have names", func(t *testing.T) {
+		b := fixture()
+		runs(b)
+		partOn(b).Pass()
+		if got := string(b.own[grants.StateMemberships]["b.mod-y"]); !strings.Contains(got, "mod-x.old") || !strings.Contains(got, "mod-x.kept") {
+			t.Errorf("b.mod-y's membership lacks the running build's read: %s", got)
+		}
+	})
+	t.Run("a pass that cannot read the build a have names keeps the membership last published", func(t *testing.T) {
+		b := fixture()
+		runs(b)
+		part := partOn(b)
+		part.Pass()
+		b.refused[grants.ReadBuilds] = true
+		part.Pass()
+		if got := string(b.own[grants.StateMemberships]["b.mod-y"]); !strings.Contains(got, "mod-x.old") {
+			t.Errorf("b.mod-y's membership lost the running build's read when its build could not be read: %s", got)
+		}
+	})
+	t.Run("a build that cannot be read is a failed read", func(t *testing.T) {
+		b := fixture()
+		runs(b)
+		b.refused[grants.ReadBuilds] = true
+		if f := grants.ReadInputs(b, &grants.Manifests{}).ReadFailed(); !strings.Contains(strings.Join(f, "\n"), grants.ReadBuilds) {
+			t.Errorf("a build read refused is not a failed read: %v", f)
+		}
+	})
+}

modules/mesh-grants/internal/grants/inputs.go

@@ -151,6 +151,12 @@ type Inputs struct {
 	// Running is the manifest a machine is wanted to run, by `<node>.<module>`: the one the builder built at the
 	// fingerprint the assigner's want names. An assignment with none is composed from the trunk's manifest.
 	Running map[string]catalogue.Manifest
+	// Had is the manifest of the build each machine runs now (its have's fingerprint, the builder's build of it), by
+	// `<node>.<module>`; the want's Running may already be a newer build. A membership is composed from both and the trunk's.
+	Had map[string]catalogue.Manifest
+	// HadFailed is why the build a machine runs could not be read this pass (a have or a build read that failed), by
+	// `<node>.<module>`: its membership is then composed without that build's reads, and the part keeps the last published.
+	HadFailed map[string]string
 	// Values is each resolved config value, by `<node>/<module>/<key>`.
 	Values map[string]any
 	// BusNode is the machine running the bus's module, and Loaded what its node agent says it loaded.
@@ -322,7 +328,7 @@ func ReadInputs(r Reader, cache *Manifests) Inputs {
 	gate := cache.gate
 	cache.mu.Unlock()
 	in := Inputs{Assigned: map[string][]string{}, Manifests: map[string]catalogue.Manifest{}, Digests: map[string]string{}, Fingerprints: map[string]string{},
-		NoFingerprint: map[string]string{}, Running: map[string]catalogue.Manifest{},
+		NoFingerprint: map[string]string{}, Running: map[string]catalogue.Manifest{}, Had: map[string]catalogue.Manifest{}, HadFailed: map[string]string{},
 		Values: map[string]any{}, Gaps: append([]Gap(nil), standingGaps...)}
 	in.SeatListUnread = gate != nil && !gate.MayAct()
 	nodes := map[string]bool{}
@@ -515,11 +521,14 @@ func ReadInputs(r Reader, cache *Manifests) Inputs {
 	wants := map[string][]byte{}
 	if in.ByHaves == "" {
 		wants = each(r, ReadWants, &in.Gaps)
+		// What each machine runs now is its have's build, which the want may already have left (issue 1966): the
+		// memberships keep what either build reads, calls and consumes.
+		runningByHaves(r, &in, cache, in.Had)
 	} else {
 		// Placed by the haves, what runs is what each have's fingerprint names, not a want: the assigner's wants are its
 		// view too, and not read in such a pass.
 		in.Gaps = append(in.Gaps, Gap{Input: ReadWants, Why: "not read: the modules are placed by the node-engines' haves this pass, so what runs is composed from each have's fingerprint"})
-		fromTrunk = runningByHaves(r, &in, cache)
+		fromTrunk = runningByHaves(r, &in, cache, in.Running)
 	}
 	for key, raw := range wants {
 		node, module, err := meshwantsclient.ParseWantKey(key)
@@ -743,11 +752,16 @@ func placeByHaves(r Reader, in *Inputs, haveKeys []string) {
 
 // runningByHaves is, in a pass placed by the haves, the manifest each placed module runs: its have's fingerprint, the
 // builder's build of it, the manifest it built; the placed modules with none, `<node>.<module>`, to compose from the trunk.
-func runningByHaves(r Reader, in *Inputs, cache *Manifests) []string {
+func runningByHaves(r Reader, in *Inputs, cache *Manifests, into map[string]catalogue.Manifest) []string {
 	var fromTrunk []string
 	for node, modules := range in.Assigned {
 		for _, module := range modules {
 			raw, found, err := r.Get(ReadHave, node+"."+module)
+			if err != nil {
+				in.Gaps = append(in.Gaps, Gap{Input: ReadHave + " " + node + "." + module, Why: "cannot be read, so the build it runs is unknown: " + err.Error(), Failed: true})
+				in.HadFailed[node+"."+module] = ReadHave + " cannot be read"
+
+			}
 			if err != nil || !found {
 				fromTrunk = append(fromTrunk, node+"."+module)
 				continue
@@ -763,6 +777,10 @@ func runningByHaves(r Reader, in *Inputs, cache *Manifests) []string {
 				continue
 			}
 			braw, found, err := r.Get(ReadBuilds, bk)
+			if err != nil {
+				in.HadFailed[node+"."+module] = ReadBuilds + " cannot be read"
+				in.Gaps = append(in.Gaps, Gap{Input: ReadBuilds + " " + bk, Why: "cannot be read, so the manifest of the build " + node + "." + module + " runs is unknown: " + err.Error(), Failed: true})
+			}
 			if err != nil || !found {
 				fromTrunk = append(fromTrunk, node+"."+module)
 				continue
@@ -773,7 +791,7 @@ func runningByHaves(r Reader, in *Inputs, cache *Manifests) []string {
 				continue
 			}
 			if m, err := cache.get(r, b.Manifest); err == nil {
-				in.Running[node+"."+module] = m
+				into[node+"."+module] = m
 			} else {
 				fromTrunk = append(fromTrunk, node+"."+module)
 			}

modules/mesh-grants/internal/grants/part.go

@@ -407,7 +407,7 @@ func (p *Part) Compose() Result {
 		}
 		p.writeSealed(&res, list, n, leftOut, dropped, in, bus, lost, users, loaded)
 	}
-	ms := Memberships(recs)
+	ms := Memberships(listRecs)
 	res.Memberships = len(ms)
 	members := make([]membership.Membership, 0, len(ms))
 	for _, m := range ms {
@@ -436,7 +436,11 @@ func (p *Part) Compose() Result {
 		// A pass that composes part of the mesh only grows a published membership: it publishes the union of the one last
 		// published and the one composed (keeploaded.go, issue 1966), so a tool runner or a seat holder goes on serving what it
 		// serves and still receives what the pass that ends the hold needs.
-		if why := in.keepsLoaded(); found && why != "" {
+		why := in.keepsLoaded()
+		if why == "" && in.HadFailed[key] != "" {
+			why = "the build it runs is unread (" + in.HadFailed[key] + ")"
+		}
+		if found && why != "" {
 			if bare, err := membership.BareGrantsMembership(was); err == nil {
 				if body, err := json.Marshal(ms[key].Membership); err == nil {
 					if grown, kept, err := UnionMembership(bare, body); err == nil && len(kept) > 0 {

modules/mesh-grants/internal/grants/records.go

@@ -21,7 +21,9 @@ func Records(in Inputs) (buscompose.Records, []Gap) { return records(in, false)
 // ListRecords is Records for the user list and the grants (issue 774): a module whose wanted build's manifest is not the
 // trunk's is declared with what both grant (unionDeclared), so the list carries the trunk manifest's subjects before the
 // assigner moves the want to it, and a grant at the trunk's fingerprint waits for nothing the want's move would bring.
-// Memberships and consumers stay with Records: they say what runs. Pure.
+// Memberships are composed from it too, and from the build each machine runs now (Inputs.Had): while a machine's build
+// differs from the want's or the trunk's, neither loses a read, call or consume (ADR 0660 §2 applied to memberships,
+// issue 1966). Consumers stay with Records. Pure.
 func ListRecords(in Inputs) (buscompose.Records, []Gap) { return records(in, true) }
 
 func records(in Inputs, union bool) (buscompose.Records, []Gap) {
@@ -106,6 +108,9 @@ func records(in Inputs, union bool) (buscompose.Records, []Gap) {
 			if union && onTrunk && !reflect.DeepEqual(m, trunk) {
 				d = unionDeclared(d, declare(trunk))
 			}
+			if had, ok := in.Had[node+"."+module]; union && ok && !reflect.DeepEqual(m, had) {
+				d = unionDeclared(d, declare(had))
+			}
 			out.Assigned[node] = append(out.Assigned[node], d)
 			if m.Instances == catalogue.InstancesInterchangeable || union && onTrunk && trunk.Instances == catalogue.InstancesInterchangeable {
 				out.Interchangeable[m.Module] = true

modules/mesh-grants/internal/grants/tools.go

@@ -111,7 +111,7 @@ func (p *Part) MembershipOf(node, module string) (any, error) {
 		return nil, errors.New("membership names a node and a module, or neither for every membership held back")
 	}
 	in := p.inputs()
-	recs, left := Records(in)
+	recs, left := ListRecords(in)
 	notFree := WithRootFree(&recs, p.Call)
 	m, ok := Memberships(recs)[node+"."+module]
 	out := map[string]any{"node": node, "module": module, "gaps": MembershipGaps, "root_free": recs.RootFree, "root_not_free": notFree}