Judge the grants step on the declaration it sends, so nothing composed between the check and the send slips through (review of #230)

The guard composed its own declaration ahead of the send; now sendToEach
judges the one it composed and stamped, and refuses it unsent.

cmd/mesh-controller/grants_step_store_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"encoding/json"
 	"strings"
 	"testing"
@@ -65,31 +66,50 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) {
 		}
 	}
 
-	// What anchor was last sent: everything at the builds of before the merge, kept as a send keeps it.
-	sendAnchor := func() {
+	wasEpoch := epochForActs
+	epochForActs = func(context.Context) (uint64, error) { return 7, nil }
+	t.Cleanup(func() { epochForActs = wasEpoch })
+
+	// A send to anchor composed as sendToEach composes it: numbered, planned, declared, stamped.
+	compose := func(under context.Context) (catalogue.Resolution, sendable) {
 		t.Helper()
-		plan, settings, err := planFor(keepingRecorded(ctx), open, "anchor")
+		numbered, err := allot(under, inv, "anchor")
 		if err != nil {
 			t.Fatal(err)
 		}
-		gens, err := generators(ctx, open)
+		plan, settings, err := planFor(under, open, "anchor")
 		if err != nil {
 			t.Fatal(err)
 		}
-		declared, err := declarationWith(keepingRecorded(ctx), open, "anchor", plan, settings, gens, Allocating)
+		gens, err := generators(under, open)
 		if err != nil {
 			t.Fatal(err)
 		}
+		declared, err := declarationWith(under, open, "anchor", plan, settings, gens, Allocating)
+		if err != nil {
+			t.Fatal(err)
+		}
+		numbered.stamp(&declared)
+		return plan, declared
+	}
+	record := func(declared sendable) {
+		t.Helper()
 		body, err := declared.Body()
 		if err != nil {
 			t.Fatal(err)
 		}
-		if _, err := recordSent(ctx, inv, "anchor", body, declared.Builds, 0); err != nil {
+		if _, err := recordSent(ctx, inv, "anchor", body, declared.Builds, declared.Epoch,
+			sentRecordOf(ctx, declared)); err != nil {
 			t.Fatal(err)
 		}
 	}
-	sendAnchor()
-	if only, err := grantsOnly(ctx, open, "anchor"); err != nil || only != "" {
+	kept := keepingEveryBuild(keepingRecorded(ctx))
+
+	// What anchor was last sent: everything at the builds of before the merge, as an ordinary send sends it.
+	_, before := compose(keepingRecorded(ctx))
+	record(before)
+	plan, declared := compose(kept)
+	if only, err := grantsOnlyIn(ctx, open, "anchor", plan, declared); err != nil || only != "" {
 		t.Fatalf("with nothing changed, the step reads %q, %v", only, err)
 	}
 
@@ -100,62 +120,53 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) {
 	}
 
 	// The step's send to anchor: restic as anchor runs it, c1, and the user list granting c2's tool.
-	kept := keepingEveryBuild(keepingRecorded(ctx))
-	plan, settings, err := planFor(kept, open, "anchor")
+	generation, err := inv.AssignmentGeneration(ctx)
 	if err != nil {
 		t.Fatal(err)
 	}
+	plan, declared = compose(kept)
 	for _, m := range plan.Modules {
 		if m.Module == "restic" && len(m.Tools) > 0 {
 			t.Fatalf("the grants step composed restic's new build on anchor: tools %v", m.Tools)
 		}
 	}
-	list, _, err := busUserList(ctx, inv, plan.Modules)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if !strings.Contains(list, "mesh.mod.restic.tool.backup_health.laptop") {
-		t.Errorf("the user list the step sends does not grant laptop's node-engine restic's new tool:\n%s", list)
-	}
-	gens, err := generators(kept, open)
-	if err != nil {
-		t.Fatal(err)
-	}
-	declared, err := declarationWith(kept, open, "anchor", plan, settings, gens, Allocating)
-	if err != nil {
-		t.Fatal(err)
-	}
 	if declared.Builds["restic"] != "c1" {
 		t.Fatalf("the step's send records it carries restic %q; want c1, which anchor runs", declared.Builds["restic"])
 	}
 	if !strings.Contains(declared.BusUsers, "mesh.mod.restic.tool.backup_health.laptop") {
-		t.Error("the step's declaration does not carry the new grant")
+		t.Errorf("the step's declaration does not grant laptop's node-engine restic's new tool:\n%s", declared.BusUsers)
 	}
-	// Recorded as a send records it: anchor still runs restic c1, and its gate is still to come there.
-	if err := inv.RecordSent(ctx, nodeID(t, open, "anchor"), "d-grants", declared.Builds); err != nil {
-		t.Fatal(err)
+	// The generation it carries is the current one (novox/hq issue 234), and the step does not raise it.
+	if declared.composedFrom != generation {
+		t.Errorf("the step's send was composed from generation %d; the mesh is at %d", declared.composedFrom, generation)
 	}
-	if sent, _, err := inv.SentBuilds(ctx, "anchor"); err != nil || sent["restic"] != "c1" {
-		t.Fatalf("after the step anchor reads as sent restic %q (%v); want c1", sent["restic"], err)
-	}
-	// The guard: only the user list differs from what anchor was last sent.
-	if only, err := grantsOnly(ctx, open, "anchor"); err != nil || only != "" {
+	// The guard, on the very declaration the send sends: only the user list differs from the last send.
+	if only, err := grantsOnlyIn(ctx, open, "anchor", plan, declared); err != nil || only != "" {
 		t.Fatalf("the step reads as changing more than the user list: %q, %v", only, err)
 	}
 	// And the gate's refusal still reads the step's send: it moves nothing there.
 	if names, err := ungatedIn(kept, open, []string{"anchor"}, ""); err != nil || strings.Join(names, ",") != "anchor" {
 		t.Fatalf("the step's send is refused as a move: %v, %v", names, err)
 	}
+	// Recorded as the send records it: anchor still runs restic c1, its gate still to come there.
+	record(declared)
+	if sent, _, err := inv.SentBuilds(ctx, "anchor"); err != nil || sent["restic"] != "c1" {
+		t.Fatalf("after the step anchor reads as sent restic %q (%v); want c1", sent["restic"], err)
+	}
+	if after, err := inv.AssignmentGeneration(ctx); err != nil || after != generation {
+		t.Fatalf("the step moved the assignment generation from %d to %d (%v)", generation, after, err)
+	}
 
-	// A module newly assigned to anchor: the step would install it, unjudged, so it is not sent.
+	// A module newly assigned to anchor: the step would install it, unjudged, so it is refused unsent.
 	if _, err := inv.Assign(ctx, "anchor", "wallpaper"); err != nil {
 		t.Fatal(err)
 	}
-	only, err := grantsOnly(ctx, open, "anchor")
+	plan, declared = compose(kept)
+	only, err := grantsOnlyIn(ctx, open, "anchor", plan, declared)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !strings.Contains(only, "wallpaper newly assigned") || !strings.Contains(only, "wallpaper") {
+	if !strings.Contains(only, "wallpaper newly assigned") || !strings.Contains(only, "+wallpaper") {
 		t.Fatalf("a new assignment on the bus's machine reads %q; want the step refused, naming it", only)
 	}
 }

cmd/mesh-controller/grants_step_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"errors"
+	"fmt"
 	"slices"
 	"strings"
 	"testing"
@@ -32,9 +33,8 @@ type aSend struct {
 func sendsRecorded(t *testing.T, holder string, behind bool, refuse error) *[]aSend {
 	t.Helper()
 	var sends []aSend
-	wasSend, wasBehind, wasOnly := sendRollout, brokerBehindOf, grantsOnlyOf
-	t.Cleanup(func() { sendRollout, brokerBehindOf, grantsOnlyOf = wasSend, wasBehind, wasOnly })
-	grantsOnlyOf = func(context.Context, *stores, string) (string, error) { return "", nil }
+	wasSend, wasBehind := sendRollout, brokerBehindOf
+	t.Cleanup(func() { sendRollout, brokerBehindOf = wasSend, wasBehind })
 	brokerBehindOf = func(_ context.Context, _ *stores, names []string) (string, bool, error) {
 		if slices.Contains(names, holder) {
 			return holder, false, nil
@@ -123,18 +123,17 @@ func TestAGrantsStepThatFailsIsSaidAndTheSendGoesOn(t *testing.T) {
 	}
 }
 
-// A step whose send would change more than the user list is not sent: it fails, said, and the walk goes on.
+// A step whose send would change more than the user list is refused unsent by the send (sendToEach): the
+// gate says it FAILED with what differed, and the walk goes on.
 func TestAGrantsStepThatWouldCarryMoreIsNotSent(t *testing.T) {
-	sends := sendsRecorded(t, "novox", true, nil)
-	grantsOnlyOf = func(context.Context, *stores, string) (string, error) {
-		return "its send would change more than the bus's user list: -postgres.login-n8n", nil
-	}
+	refusal := fmt.Errorf("%w: its send would change more than the bus's user list: -postgres.login-n8n", errNotGrantsOnly)
+	sends := sendsRecorded(t, "novox", true, refusal)
 	sent, grants, err := sendJudged(t.Context(), nil, "ace")
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(*sends) != 1 || (*sends)[0].keepsAll || strings.Join(sent, ",") != "ace" {
-		t.Errorf("the walk made %+v, want ace's judged send alone", *sends)
+	if strings.Join(sent, ",") != "ace" || len(*sends) != 2 {
+		t.Errorf("the walk made %+v, want the refused step and then ace's judged send", *sends)
 	}
 	if grants == nil || grants.At != nil || !strings.Contains(grants.Failed, "-postgres.login-n8n") {
 		t.Fatalf("the gate keeps %+v", grants)

cmd/mesh-controller/push.go

@@ -1236,6 +1236,17 @@ func sendToEach(ctx context.Context, open *stores, names []string) ([]string, er
 			continue
 		}
 		numbered.stamp(&declared)
+		// **The grants step sends the user list and nothing else** (novox/hq issue 490): judged on this very
+		// declaration, the one sent, so nothing composed between a check and the send can slip through.
+		if everyBuildKept(ctx) {
+			other, err := grantsOnlyIn(ctx, open, name, plan, declared)
+			if err != nil {
+				return nil, err
+			}
+			if other != "" {
+				return nil, fmt.Errorf("%w: %s", errNotGrantsOnly, other)
+			}
+		}
 		reportLeftOut(name, declared)
 		sending = append(sending, readyNode{name, declared})
 	}

cmd/mesh-controller/release.go

@@ -968,19 +968,6 @@ func grantsStep(ctx context.Context, open *stores, node string) *inventory.Grant
 		// list is the one already sent.
 		return nil
 	}
-	// **The user list and nothing else**: checked before it is sent, against what the machine was last sent.
-	// Kept builds are not the whole of a declaration — a new assignment, a settings change, an assignment
-	// taken away, a provision's logins are composed as the mesh holds them now — and a step that carried any
-	// of it would be the unjudged change this step exists to avoid.
-	if other, err := grantsOnlyOf(ctx, open, holder); err != nil || other != "" {
-		why := other
-		if err != nil {
-			why = err.Error()
-		}
-		fmt.Printf("grants step: %s is not sent its user list ahead of %s, which is sent without it: %s\n",
-			holder, node, why)
-		return &inventory.GrantsStep{Node: holder, Failed: why}
-	}
 	if _, err := sendRollout(keepingEveryBuild(withScope(ctx, sendScope{})), open, []string{holder}); err != nil {
 		fmt.Printf("grants step: the bus's user list could not be sent to %s ahead of %s, which is sent without "+
 			"it — the bus may refuse what the send newly grants: %v\n", holder, node, err)
@@ -992,21 +979,19 @@ func grantsStep(ctx context.Context, open *stores, node string) *inventory.Grant
 	return &inventory.GrantsStep{Node: holder, At: &now}
 }
 
-// grantsOnlyOf is grantsOnly: a variable so a test of the walk needs no store.
-var grantsOnlyOf = grantsOnly
+// errNotGrantsOnly is a grants step refused unsent: its declaration would change more than the user list.
+var errNotGrantsOnly = errors.New("the grants step would change more than the bus's user list, and is not sent")
 
-// grantsOnly is what the grants step's send to the machine holding the bus would change there besides its
-// user list, against what it was last sent (novox/hq issue 490); empty when nothing else. Composed as the
-// step's send composes it, without allocating anything (Foreseeing), and compared resource by resource
-// with the summary the last send kept (ADR 0217): exact, rather than a list of the cases that can differ.
-// A module there that was never sent is a new assignment, and refused too, whatever its resources.
-func grantsOnly(ctx context.Context, open *stores, holder string) (string, error) {
+// grantsOnlyIn is what a grants step's declaration, composed for sending, would change on the machine holding
+// the bus besides its user list, against what it was last sent (novox/hq issue 490); empty when nothing else.
+// Judged by sendToEach on the very declaration it then sends — one composition, judged, then sent or refused
+// unsent. Kept builds are not the whole of a declaration: a new assignment, a settings change, an assignment
+// taken away, a provision's logins are composed as the mesh holds them now, and a step that carried any of
+// it would be the unjudged change this step exists to avoid. Compared resource by resource with the summary
+// the last send kept (ADR 0217): exact, rather than a list of the cases that can differ. A module there that
+// was never sent is a new assignment, refused whatever its resources.
+func grantsOnlyIn(ctx context.Context, open *stores, holder string, plan catalogue.Resolution, declared sendable) (string, error) {
 	inv := open.inventory
-	kept := keepingEveryBuild(keepingRecorded(ctx))
-	plan, settings, err := planFor(kept, open, holder)
-	if err != nil {
-		return "", err
-	}
 	sent, known, err := inv.SentBuilds(ctx, holder)
 	if err != nil {
 		return "", err
@@ -1024,13 +1009,22 @@ func grantsOnly(ctx context.Context, open *stores, holder string) (string, error
 			listID = m.Module + "." + catalogue.BusUsersID()
 		}
 	}
-	gens, err := generators(kept, open)
-	if err != nil {
-		return "", err
-	}
-	declared, err := declarationWith(kept, open, holder, plan, settings, gens, Foreseeing)
-	if err != nil {
-		return "", err
+	var facts *moveFacts
+	for m, was := range sent {
+		now, carried := declared.Builds[m]
+		if !carried || sameCommit(now, was) {
+			continue
+		}
+		if facts == nil {
+			f, err := readMoveFacts(ctx, inv)
+			if err != nil {
+				return "", err
+			}
+			facts = &f
+		}
+		if !facts.identical(m, was, now) {
+			other = append(other, fmt.Sprintf("%s %s → %s", m, short(was), short(now)))
+		}
 	}
 	for _, f := range declared.foreseen {
 		other = append(other, f+" would be minted")
@@ -1067,6 +1061,7 @@ func grantsOnly(ctx context.Context, open *stores, holder string) (string, error
 		}
 		other = append(other, fmt.Sprintf("~%s (%s)", c.ID, strings.Join(c.Fields, ", ")))
 	}
+	sort.Strings(other)
 	if len(other) == 0 {
 		return "", nil
 	}