From 35c7662f895f57b0092939f67e6a48ac88386ed8 Mon Sep 17 00:00:00 2001 From: mesh-admin Date: Wed, 14 Oct 2026 01:26:29 +0000 Subject: [PATCH] Check the mirrors as ADR 0356 says, and prove a removal asks for the lease Reviews 251 of mesh-controller#315 and ADR 0356 v4 (hq issue 652): a removal without the lease removes nothing; a module no longer registered loses its declared-config key with its event; a listed mirror is never reported as undeclared state; only the audit and a mirror's own writer name a mirror's bucket; and the mirrors' list and the writers table agree. The store test's line moves where it merges with the integration branch's own new lines. Co-Authored-By: Claude Opus 5.5 --- internal/broker/controller_buckets_test.go | 53 ++++++++++++++ internal/recordmirror/mirror_store_test.go | 20 ++++++ internal/recordmirror/mirror_test.go | 25 +++++++ internal/recordmirror/readers_test.go | 82 ++++++++++++++++++++++ testdata/store-tests | 2 +- 5 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 internal/recordmirror/readers_test.go diff --git a/internal/broker/controller_buckets_test.go b/internal/broker/controller_buckets_test.go index 1e8c7a69..194227e2 100644 --- a/internal/broker/controller_buckets_test.go +++ b/internal/broker/controller_buckets_test.go @@ -125,3 +125,56 @@ func TestWhatTheControllerAskedIsBounded(t *testing.T) { t.Errorf("history %d, age %s, bytes %d", status.History(), status.TTL(), info.Config.MaxBytes) } } + +// **A listed mirror is no undeclared bucket** (ADR 0356 §3, ADR 0201 §16): the controller's list of its mirrors +// stands for their declaration, so the report of state nothing declares never names one. +func TestAListedMirrorIsNotReportedAsUndeclared(t *testing.T) { + names := []string{"gone_state"} + for _, m := range ControllerMirrors() { + names = append(names, BucketName(ControllerSeat, m)) + } + undeclared, err := RaiseBuckets(fakeBuckets{names: names}, nil) + if err != nil { + t.Fatal(err) + } + if !slices.Equal(undeclared, []string{"gone_state"}) { + t.Fatalf("the report of undeclared state names %v", undeclared) + } +} + +// **The mirrors' list and the writers table agree** (ADR 0356 §3, to-be 45 §1): every listed mirror is a row of +// the table whose one writer is the controller, on its bucket alone; and every row that writes a bucket of the +// controller's named as a mirror is on the list. +func TestTheMirrorsListAndTheWritersTableAgree(t *testing.T) { + controller := Principal{Kind: KindController} + module := Principal{Kind: KindModule, Module: ControllerSeat, Node: "one"} + rowed := map[string]bool{} + for _, row := range WritersTable { + if !strings.HasSuffix(row.State, " mirror") { + continue + } + for _, subject := range row.Subjects { + bucket := strings.TrimSuffix(strings.TrimPrefix(subject, "$KV."), ".>") + rowed[bucket] = true + if row.Writes == nil || !row.Writes(controller, subject) || row.Writes(module, subject) { + t.Errorf("%q is not written by the controller alone", row.State) + } + } + } + listed := map[string]bool{} + for _, m := range ControllerMirrors() { + bucket := BucketName(ControllerSeat, m) + listed[bucket] = true + if !rowed[bucket] { + t.Errorf("the mirror %s is listed and no row of the writers table writes it", bucket) + } + if !IsControllerBucket(bucket) || !slices.Contains(ControllerBuckets(), bucket) { + t.Errorf("the mirror %s is listed and not asserted as the controller's own", bucket) + } + } + for bucket := range rowed { + if !listed[bucket] { + t.Errorf("the writers table writes the mirror %s, which the list does not name", bucket) + } + } +} diff --git a/internal/recordmirror/mirror_store_test.go b/internal/recordmirror/mirror_store_test.go index 11e8de47..391f6b93 100644 --- a/internal/recordmirror/mirror_store_test.go +++ b/internal/recordmirror/mirror_store_test.go @@ -155,4 +155,24 @@ func TestNatsAnAssignmentAndARegistrationEachChangeOneKeyAndRaiseOneEvent(t *tes if len(said) != 1 || said[0].Key != "anchor.ui" || said[0].Change != Removed { t.Errorf("an unassignment raised %+v", said) } + + // **A module no longer registered** has its key removed, said once as `removed` (ADR 0356 §4). + if err := inv.ForgetModule(ctx, "notes"); err != nil { + t.Fatal(err) + } + declaredWrites = writes(t, api, DeclaredConfig.Bucket) + declaredSaid = len(heardDeclared.all()) + if _, rd := pass(); rd != (Result{Removed: 1}) { + t.Errorf("a module forgotten did %+v", rd) + } + if got := writes(t, api, DeclaredConfig.Bucket) - declaredWrites; got != 1 { + t.Errorf("a module forgotten changed %d keys of the declared config", got) + } + if _, err := declared.KV.Get(ctx, "notes"); err == nil { + t.Error("a module forgotten keeps its key") + } + said = settle(heardDeclared, declaredSaid+1)[declaredSaid:] + if len(said) != 1 || said[0].Key != "notes" || said[0].Change != Removed { + t.Errorf("a module forgotten raised %+v", said) + } } diff --git a/internal/recordmirror/mirror_test.go b/internal/recordmirror/mirror_test.go index 31f67594..9d987e37 100644 --- a/internal/recordmirror/mirror_test.go +++ b/internal/recordmirror/mirror_test.go @@ -312,6 +312,31 @@ func TestNatsAWriterWithoutTheLeaseWritesNothing(t *testing.T) { } } +// **A removal asks for the lease too** (ADR 0289 §9): a writer that held the lease when its pass began and lost +// it before its first removal purges nothing, and says it does not hold the lease. +func TestNatsARemovalWithoutTheLeaseRemovesNothing(t *testing.T) { + js, api := aMirrorBus(t) + ctx := t.Context() + held := aWriter(t, js, api, Assignments) + if _, err := held.Sync(ctx, map[string][]byte{"anchor.gitea": []byte(`{"a":1}`)}); err != nil { + t.Fatal(err) + } + before := writes(t, api, Assignments.Bucket) + asked := 0 + w := aWriter(t, js, api, Assignments) + w.Holds = func() bool { asked++; return asked == 1 } + r, err := w.Sync(ctx, map[string][]byte{}) + if !errors.Is(err, ErrNotHeld) || r != (Result{}) || asked != 2 { + t.Errorf("a removal without the lease did %+v, %v (the lease asked %d times)", r, err, asked) + } + if got := writes(t, api, Assignments.Bucket) - before; got != 0 { + t.Errorf("a removal without the lease wrote the bucket %d times", got) + } + if _, err := w.KV.Get(ctx, "anchor.gitea"); err != nil { + t.Errorf("the key was removed without the lease: %v", err) + } +} + // **A key the bus cannot hold is refused alone**, and the others are written. func TestAKeyTheBusCannotHoldIsRefusedAlone(t *testing.T) { if validKey("anchor.gitea") != nil || validKey("bad key") == nil || validKey(".lead") == nil || diff --git a/internal/recordmirror/readers_test.go b/internal/recordmirror/readers_test.go new file mode 100644 index 00000000..c57a7b4c --- /dev/null +++ b/internal/recordmirror/readers_test.go @@ -0,0 +1,82 @@ +package recordmirror + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/novox/mesh-controller/internal/broker" +) + +// mayNameAMirror are the files of the controller that may name a record mirror's bucket outside their tests: +// where the buckets are defined, asserted and granted (internal/broker), the list a reader names in `reads` +// (internal/catalogue/state.go), and each mirror's own writer (this package and the loop that runs it). The +// audit of a mirror against its store (ADR 0289 §4, issue 419) joins this list when it is built. +var mayNameAMirror = []string{ + "cmd/mesh-controller/record_mirrors.go", + "internal/broker/controller_buckets.go", + "internal/broker/writers.go", + "internal/catalogue/state.go", +} + +// **Nothing in the controller reads a mirror to decide anything** (ADR 0289 §19 as ADR 0356 §2 changes it): no +// file of the controller's packages outside its tests names a mirror's bucket, by its Go name or its bus name, +// but the audit and a mirror's own writer. A reader elsewhere fails here, naming the file and what it named. +func TestOnlyTheAuditAndAMirrorsWriterReadAMirror(t *testing.T) { + names := []string{"AssignmentsBucket", "DeclaredConfigBucket", "MirrorAssignments", "MirrorDeclaredConfig", + "ControllerMirrors", "Assignments", "DeclaredConfig", "Mirrors"} + var buckets []string + for _, m := range broker.ControllerMirrors() { + buckets = append(buckets, broker.BucketName(broker.ControllerSeat, m)) + } + root := filepath.Join("..", "..") + var found []string + for _, dir := range []string{"cmd", "internal"} { + err := filepath.WalkDir(filepath.Join(root, dir), func(path string, e fs.DirEntry, err error) error { + if err != nil || e.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return err + } + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + rel = filepath.ToSlash(rel) + if strings.HasPrefix(rel, "internal/recordmirror/") || slices.Contains(mayNameAMirror, rel) { + return nil + } + f, err := parser.ParseFile(token.NewFileSet(), path, nil, 0) + if err != nil { + return err + } + ast.Inspect(f, func(n ast.Node) bool { + switch n := n.(type) { + case *ast.SelectorExpr: + pkg, ok := n.X.(*ast.Ident) + if ok && (pkg.Name == "broker" || pkg.Name == "recordmirror") && slices.Contains(names, n.Sel.Name) { + found = append(found, rel+": "+pkg.Name+"."+n.Sel.Name) + } + case *ast.BasicLit: + for _, b := range buckets { + if n.Kind == token.STRING && strings.Contains(n.Value, b) { + found = append(found, rel+": "+n.Value) + } + } + } + return true + }) + return nil + }) + if err != nil { + t.Fatal(err) + } + } + if len(found) > 0 { + t.Fatalf("a package of the controller reads a record mirror, which only the audit and the mirror's "+ + "writer may (ADR 0289 §19, ADR 0356 §2):\n %s", strings.Join(found, "\n ")) + } +} diff --git a/testdata/store-tests b/testdata/store-tests index b7dc4922..2213aab7 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -49,6 +49,7 @@ ./internal/inventory TestACredentialIssuedOutsideIsNeverMade ./internal/inventory TestOneAcceptForAProviderIsSealedToEveryConsumer ./internal/inventory TestASharedOwnSecretIssuedOutsideIsNeverMade +./internal/recordmirror TestNatsAnAssignmentAndARegistrationEachChangeOneKeyAndRaiseOneEvent ./cmd/mesh-controller TestOneAcceptForAProviderIsPlacedForEveryConsumer ./cmd/mesh-controller TestReplay164 ./cmd/mesh-controller TestAnApprovedLayerOpensTheWalkThatSendsIt @@ -61,4 +62,3 @@ ./cmd/mesh-controller TestAProviderElsewhereServesItsOwnComposedName ./cmd/mesh-controller TestAReusedCheckBuildIsRegisteredByItsDigestAndGatedAsABuiltOne ./internal/inventory TestAReportOfABusyContainerIsKeptAndReadBack -./internal/recordmirror TestNatsAnAssignmentAndARegistrationEachChangeOneKeyAndRaiseOneEvent