diff --git a/internal/catalogue/seats.go b/internal/catalogue/seats.go index 8a70f70..9379370 100644 --- a/internal/catalogue/seats.go +++ b/internal/catalogue/seats.go @@ -184,17 +184,17 @@ func claimProblems(m Manifest) []string { } for _, c := range m.Claims { - if seat, known := SeatNamed(c.Name); known { - if c.At() != seat.Scope { - problems = append(problems, fmt.Sprintf( - "%s claims %s at scope %q, and %s is a %s seat", - m.Module, c.Name, c.At(), c.Name, seat.Scope)) - } - if seat.Delivers != "" && !providesAt(m, seat.Delivers, seat.Scope) { - problems = append(problems, fmt.Sprintf( - "%s claims %s, whose holder answers for %q, and %s does not provide %q at %s scope", - m.Module, c.Name, seat.Delivers, m.Module, seat.Delivers, seat.Scope)) - } + if _, known := SeatNamed(c.Name); known { + // **A seat's scope and what it delivers are not judged here** (novox/hq ADR 0122). + // This function runs wherever a manifest is parsed, and one of those places is the + // build machine, which has no store: there, `SeatNamed` answers from the set the + // binary shipped with, so a build would be refused for disagreeing with a compiled + // copy of data the control plane owns. Exactly that happened — a holder of the bus + // seat was refused for not providing what a stale compiled row said the seat + // delivered, while the store's own row said otherwise. + // + // Both checks moved to CatalogueProblems, which only ever runs in the control plane, + // after UseSeats has replaced the set with the store's. continue } if isSystemSeatName(c.Name) { diff --git a/internal/catalogue/seats_declared.go b/internal/catalogue/seats_declared.go index 5423c96..0188106 100644 --- a/internal/catalogue/seats_declared.go +++ b/internal/catalogue/seats_declared.go @@ -190,7 +190,22 @@ func CatalogueProblems(shelf Shelf) []string { } s, isModuleSeat := declared[c.Name] if !isModuleSeat { - continue // a mesh seat: already judged by claimProblems + // **A mesh seat is judged here and nowhere else** (novox/hq ADR 0122): the set is + // the store's, and this is the only place that runs with the store's set loaded. + // The parser cannot do it — it also runs on the build machine, against whatever + // set that binary was compiled with. + seat, _ := SeatNamed(c.Name) + if c.At() != seat.Scope { + problems = append(problems, fmt.Sprintf( + "%s claims %s at scope %q, and %s is a %s seat", + module, c.Name, c.At(), c.Name, seat.Scope)) + } + if seat.Delivers != "" && !providesAt(m, seat.Delivers, seat.Scope) { + problems = append(problems, fmt.Sprintf( + "%s claims %s, whose holder answers for %q, and %s does not provide %q at %s scope", + module, c.Name, seat.Delivers, module, seat.Delivers, seat.Scope)) + } + continue } if c.At() != s.At() { problems = append(problems, fmt.Sprintf( diff --git a/internal/catalogue/seats_declared_test.go b/internal/catalogue/seats_declared_test.go index 9c64d4b..8cdd2c3 100644 --- a/internal/catalogue/seats_declared_test.go +++ b/internal/catalogue/seats_declared_test.go @@ -91,7 +91,10 @@ func TestAClaimMustMatchTheDeclaredScope(t *testing.T) { // The mesh's own seats still work, and are not shadowed by the derived half. func TestTheMeshsOwnSeatsAreStillClaimable(t *testing.T) { - m := Manifest{Module: "nats", Claims: []Claim{{Name: "mesh-broker", Scope: ScopeMesh}}} + // It delivers the bus, so its holder provides the bus — the rule this check now enforces. + m := Manifest{Module: "nats", + Provides: []Offer{{Name: "mesh-bus", Scope: ScopeMesh}}, + Claims: []Claim{{Name: "mesh-broker", Scope: ScopeMesh}}} if got := problemsFor(t, Shelf{"nats": m}); got != "" { t.Fatalf("a mesh seat was refused by the derived check: %s", got) } @@ -106,3 +109,38 @@ func TestTheProblemsAreStable(t *testing.T) { t.Fatalf("unstable:\n%s\n%s", first, second) } } + +// **A build machine has no store, so it may not judge a seat.** The set is data the control plane +// owns (novox/hq ADR 0122), and `ParseManifest` runs on the build machine too, against whatever set +// that binary was compiled with. When the two disagreed, a valid holder of the bus seat was refused +// mid-rollout — the compiled row said the seat delivered one provision, the store's row said +// another, and the build failed on the copy rather than the truth. The parser judges the manifest; +// the seat set judges the claim, where it is loaded. +func TestTheParserDoesNotJudgeWhatOnlyTheStoreKnows(t *testing.T) { + was := Seats() + t.Cleanup(func() { UseSeats(was) }) + + // A store whose bus seat delivers something this module does provide. + UseSeats([]Seat{{Name: "mesh-broker", Scope: ScopeMesh, Delivers: "amqp", Decision: "test"}}) + raw := []byte(`{"module":"lavinmq","version":"1",` + + `"provides":[{"name":"amqp","scope":"mesh"}],` + + `"claims":[{"name":"mesh-broker","scope":"mesh"}]}`) + + m, err := ParseManifest(raw) + if err != nil { + t.Fatalf("the parser refused a claim only the seat set can judge: %v", err) + } + if got := CatalogueProblems(Shelf{m.Module: m}); len(got) != 0 { + t.Fatalf("a holder that provides what the store says the seat delivers was refused: %v", got) + } + + // And with the store saying the seat delivers something else, registration is what refuses it. + UseSeats([]Seat{{Name: "mesh-broker", Scope: ScopeMesh, Delivers: "mesh-bus", Decision: "test"}}) + if _, err := ParseManifest(raw); err != nil { + t.Fatalf("the parser judged it the second time: %v", err) + } + got := strings.Join(CatalogueProblems(Shelf{m.Module: m}), "; ") + if !strings.Contains(got, `does not provide "mesh-bus"`) { + t.Fatalf("registration did not refuse a holder that cannot answer for the seat: %q", got) + } +} diff --git a/internal/catalogue/seats_test.go b/internal/catalogue/seats_test.go index 030d043..1b33a72 100644 --- a/internal/catalogue/seats_test.go +++ b/internal/catalogue/seats_test.go @@ -138,26 +138,32 @@ func TestAModuleDefinesAndClaimsItsOwnSeat(t *testing.T) { } } +// **At registration, not in the parser** (novox/hq ADR 0122): a seat's scope is a property of the +// set, the set is the store's, and the parser also runs on a build machine that has no store. func TestASeatClaimedAtAnotherScopeIsRefused(t *testing.T) { - _, err := ParseManifest(claimed(`[{"name":"npm-package-registry","scope":"node"}]`)) - if err == nil { - t.Fatal("a mesh seat was held per node") + m, err := ParseManifest(claimed(`[{"name":"npm-package-registry","scope":"node"}]`)) + if err != nil { + t.Fatalf("the parser judged a scope it reads from data it may not have: %v", err) } - if !strings.Contains(err.Error(), "mesh seat") { - t.Fatalf("the refusal does not say which scope the seat is: %v", err) + got := strings.Join(CatalogueProblems(Shelf{m.Module: m}), "; ") + if !strings.Contains(got, "mesh seat") { + t.Fatalf("the refusal does not say which scope the seat is: %q", got) } } func TestADeliveringSeatIsOnlyHeldByAModuleThatProvides(t *testing.T) { // Holding it makes the module the mesh's answer for the provision. A module that cannot answer // would be the answer anyway, and every consumer would be sent to it. + // And refused at registration, where the seat set is the store's: what a seat delivers is + // data, so a compiled copy of it may not be what refuses a build (novox/hq ADR 0122). raw := []byte(`{"module":"thing","version":"1","claims":[{"name":"git","scope":"mesh"}]}`) - _, err := ParseManifest(raw) - if err == nil { - t.Fatal("a module holding the git seat need not provide git") + m, err := ParseManifest(raw) + if err != nil { + t.Fatalf("the parser judged what a seat delivers: %v", err) } - if !strings.Contains(err.Error(), `does not provide "git"`) { - t.Fatalf("the refusal does not say what is missing: %v", err) + got := strings.Join(CatalogueProblems(Shelf{m.Module: m}), "; ") + if !strings.Contains(got, `does not provide "git"`) { + t.Fatalf("the refusal does not say what is missing: %q", got) } }