From a3181e356ba4f5f974ca49566f91522d89a24ad8 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 19:41:39 +0200 Subject: [PATCH 1/3] Ask only once the bus enforces the controller's own grant, judged for real and failing closed (review of hq issue 353) change held asking back, a list sent was taken for a list the bus enforces, a plan that could not be worked out read as not behind, and no test held the real judgement. Now the asker asks only when the composition grants the controller itself to publish its ask, and the bus's own module says that grant is in force on the running server (nats_user_can's in_force, mesh-catalog); each fact not known is not held, said with why. brokerBehind takes a list nobody could compose as behind. --- cmd/mesh-controller/asker_test.go | 95 +++++++++++ cmd/mesh-controller/asker_wire.go | 173 ++++++++++++++++++--- cmd/mesh-controller/delivery_order_test.go | 7 + cmd/mesh-controller/proposals.go | 2 +- cmd/mesh-controller/push.go | 23 ++- 5 files changed, 271 insertions(+), 29 deletions(-) diff --git a/cmd/mesh-controller/asker_test.go b/cmd/mesh-controller/asker_test.go index 96654618..fc56f9af 100644 --- a/cmd/mesh-controller/asker_test.go +++ b/cmd/mesh-controller/asker_test.go @@ -12,6 +12,7 @@ import ( "git.novox.be/novox/mesh-sdk/go/asks" + "github.com/novox/mesh-controller/internal/broker" "github.com/novox/mesh-controller/internal/conditions" "github.com/novox/mesh-controller/internal/link" ) @@ -808,3 +809,97 @@ func TestARefusalIsAskedAgainAndTheUndeliveredConditionClearsOnceTaken(t *testin t.Errorf("the explanation says its verdict twice: %q", e) } } + +// hq issue 353, the review: the bus holds the controller's grant to ask only when both facts hold — the mesh +// composes the controller's own grant, and the bus's module says it is in force on the running server — and +// each fact not known is not held, said with why. +func TestTheGrantIsHeldOnlyWhenComposedAndInForceOnTheBus(t *testing.T) { + yes := func(context.Context) (bool, string, error) { return true, "", nil } + no := func(why string) grantJudge { + return func(context.Context) (bool, string, error) { return false, why, nil } + } + broken := func(context.Context) (bool, string, error) { return false, "", errors.New("the store is away") } + for _, c := range []struct { + name string + composed, inForce grantJudge + held bool + says string + }{ + {"composed and in force", yes, yes, true, ""}, + {"not composed", no("no router is assigned"), yes, false, "no router is assigned"}, + {"composed, written, not reloaded", yes, no("the server has not reloaded"), false, "has not reloaded"}, + {"the composition unknown", broken, yes, false, "could not be worked out: the store is away"}, + {"the bus unknown", yes, broken, false, "could not be worked out: the store is away"}, + {"nothing judges the bus", yes, nil, false, "not judged"}, + } { + held, why, err := grantHeldBy(c.composed, c.inForce, time.Now)(context.Background()) + if err != nil || held != c.held || !strings.Contains(why, c.says) { + t.Errorf("%s: held %v (%q, %v)", c.name, held, why, err) + } + } + // Remembered for grantLookEvery, then judged again. + now := time.Date(2026, 10, 9, 18, 0, 0, 0, time.UTC) + inForce := false + judge := grantHeldBy(yes, func(context.Context) (bool, string, error) { return inForce, "not yet", nil }, + func() time.Time { return now }) + if held, _, _ := judge(context.Background()); held { + t.Fatal("held before the bus enforces it") + } + inForce = true + if held, _, _ := judge(context.Background()); held { + t.Error("judged again inside grantLookEvery") + } + now = now.Add(grantLookEvery) + if held, _, _ := judge(context.Background()); !held { + t.Error("not judged again after grantLookEvery") + } +} + +// The controller's own grant, as composed: present while a router that takes asks is assigned, absent +// otherwise — whatever else in the user list changes. +func TestTheControllersOwnGrantToAskIsReadFromTheComposition(t *testing.T) { + router := broker.Declared{Module: "messenger", Holds: []broker.Seat{{Name: broker.AsksSeat, Scope: "mesh", + Accepts: []string{"ask", "cancel"}, Emits: []string{"decided"}, ByCaller: []string{"ask", "cancel", "decided"}}}} + other := broker.Declared{Module: "dunst"} + with := broker.Records{Nodes: []string{"anchor"}, Assigned: map[string][]broker.Declared{"anchor": {router, other}}} + without := broker.Records{Nodes: []string{"anchor"}, Assigned: map[string][]broker.Declared{"anchor": {other}}} + if may, err := controllerMayAsk(with); err != nil || !may { + t.Errorf("with a router assigned: %v %v", may, err) + } + if may, err := controllerMayAsk(without); err != nil || may { + t.Errorf("with no router assigned: %v %v", may, err) + } +} + +// The bus module's answer: in force only when it says so in so many words. +func TestTheBusModulesAnswerIsInForceOnlyWhenItSaysSo(t *testing.T) { + for _, c := range []struct { + answer string + want bool + says string + }{ + {`{"allowed":true,"in_force":true}`, true, ""}, + {`{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, false, "not reloaded"}, + {`{"allowed":false,"in_force":false}`, false, "not in force"}, + {`{"allowed":true}`, false, "older than that answer"}, + {`{"allowed":false,"in_force":true}`, false, "not allowed"}, + {`not json`, false, "could not be read"}, + } { + if got, why := grantInForce(json.RawMessage(c.answer)); got != c.want || !strings.Contains(why, c.says) { + t.Errorf("%s: %v %q", c.answer, got, why) + } + } +} + +// The serving asker is wired to the real judgement: with records and a bus it cannot read, the grant is not +// held, and so nothing is asked (a stub saying yes, or no judgement at all, would ask). +func TestTheServingAskerJudgesTheGrantForReal(t *testing.T) { + a := servingAsker(&stores{}, nil, nil, nil) + if a.grantHeld == nil { + t.Fatal("the serving asker does not judge the bus's grant") + } + held, why, err := a.grantHeld(context.Background()) + if err != nil || held || !strings.Contains(why, "could not be worked out") { + t.Errorf("held %v (%q, %v) with nothing to read", held, why, err) + } +} diff --git a/cmd/mesh-controller/asker_wire.go b/cmd/mesh-controller/asker_wire.go index 345cf5e5..6c34aa9d 100644 --- a/cmd/mesh-controller/asker_wire.go +++ b/cmd/mesh-controller/asker_wire.go @@ -235,12 +235,27 @@ func routerHereIn(inv *inventory.Inventory) func(ctx context.Context) (bool, err } } -// grantHeldIn says whether the bus holds the controller's grant to ask (novox/hq issue 353): the user list the -// machine holding the bus was last sent is the one the mesh composes now (brokerBehind, the same judgement a -// push makes to send that machine first). While it is behind, the controller's ask is refused by the bus, -// whatever the record says of the router, so nothing is asked and the operator is told to push that machine. -// Judged at most every grantLookEvery: composing the list resolves the bus's machine whole. -func grantHeldIn(open *stores) func(ctx context.Context) (bool, string, error) { +// grantHeldIn says whether the bus holds the controller's grant to ask (novox/hq issue 353), judged on the two +// facts that make it so, and failing closed on each: +// +// 1. **composed**: the user list the mesh composes now grants the controller itself to publish its ask +// (controllerMayAsk) — the controller's own grant, not whether any user of the list changed; +// 2. **in force**: the bus's own module, on the machine holding the bus, says the user list there allows it +// and the server reloaded after that list was written (the nats module's `nats_user_can`, `in_force`) — a +// list sent is not a list the bus enforces until it reloads. +// +// Anything that cannot be worked out is not held, with why, so the conditions that need the operator are said +// as undelivered rather than asked into a refusal. +func grantHeldIn(open *stores, conn *nats.Conn) func(ctx context.Context) (bool, string, error) { + return grantHeldBy(composedGrantIn(open), busGrantIn(open, conn), time.Now) +} + +// grantJudge is one of the two facts: whether it holds, and why not. +type grantJudge func(ctx context.Context) (bool, string, error) + +// grantHeldBy is the judgement over the two facts, remembered for grantLookEvery: composing the list resolves +// the bus's machine whole, and asking the bus's module is a call. +func grantHeldBy(composed, inForce grantJudge, now func() time.Time) func(ctx context.Context) (bool, string, error) { var mu sync.Mutex var at time.Time var held bool @@ -248,22 +263,132 @@ func grantHeldIn(open *stores) func(ctx context.Context) (bool, string, error) { return func(ctx context.Context) (bool, string, error) { mu.Lock() defer mu.Unlock() - if !at.IsZero() && time.Since(at) < grantLookEvery { + if !at.IsZero() && now().Sub(at) < grantLookEvery { return held, why, nil } - machine, behind, err := brokerBehind(ctx, open, nil) + held, why = judgeGrant(ctx, composed, inForce) + at = now() + return held, why, nil + } +} + +// judgeGrant is held only when both facts hold; an error is a fact not known, never a yes. +func judgeGrant(ctx context.Context, composed, inForce grantJudge) (bool, string) { + for _, fact := range []struct { + judge grantJudge + what string + }{{composed, "whether the mesh composes the controller a grant to ask"}, {inForce, "whether the bus enforces it"}} { + if fact.judge == nil { + return false, fact.what + " is not judged here" + } + ok, why, err := fact.judge(ctx) + if err != nil { + return false, fact.what + " could not be worked out: " + err.Error() + } + if !ok { + return false, why + } + } + return true, "" +} + +// controllerMayAsk says whether the controller's own grant, as composed from these records, lets it publish its +// ask (ADR 0259 §3: composed while a router that takes asks under its asker's name is assigned). +func controllerMayAsk(records broker.Records) (bool, error) { + users, err := broker.Users(records) + if err != nil { + return false, err + } + for _, u := range users { + if u.Kind != broker.KindController { + continue + } + perms, err := broker.PermissionsFor(u) + if err != nil { + return false, err + } + return broker.MayPublish(perms, asks.AskSubject(askerName)), nil + } + return false, errors.New("the composition holds no controller") +} + +// composedGrantIn is the first fact, from the mesh's records. +func composedGrantIn(open *stores) grantJudge { + return func(ctx context.Context) (bool, string, error) { + if open == nil || open.inventory == nil { + return false, "", errors.New("the mesh's records are not open") + } + records, err := open.inventory.BusRecords(ctx) if err != nil { return false, "", err } - at, held, why = time.Now(), !behind, "" - if behind { - why = fmt.Sprintf("the bus's user list on %s is behind what the mesh composes, so the bus has not been "+ - "given the controller's grant to ask; `push %s` carries it", machine, machine) + may, err := controllerMayAsk(records) + if err != nil || may { + return may, "", err } - return held, why, nil + return false, "the mesh composes the controller no grant to ask: no router that takes asks under its " + + "asker's name is assigned", nil } } +// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus. +func busGrantIn(open *stores, conn *nats.Conn) grantJudge { + return func(ctx context.Context) (bool, string, error) { + if open == nil || open.inventory == nil || conn == nil { + return false, "", errors.New("the mesh's records or the bus are not open") + } + holders, err := seatHolders(ctx, open.inventory) + if err != nil { + return false, "", err + } + h, held := holders[theBrokerSeat] + if !held || h.Node == "" || h.Module == "" { + return false, "no module holds the bus, so nothing says what it enforces", nil + } + a, err := link.AskModuleToolOn(ctx, conn, h.Module, busUserCanTool, h.Node, map[string]any{ + "user": broker.ControllerName, "action": "publish", "subject": asks.AskSubject(askerName)}, 15*time.Second) + if err != nil { + return false, "", err + } + if a.Error != "" { + return false, "", errors.New(a.Error) + } + inForce, why := grantInForce(a.Result) + if !inForce { + why = fmt.Sprintf("the bus on %s does not enforce the controller's grant to ask yet: %s; `push %s` "+ + "carries it, and the bus reloads it in place", h.Node, why, h.Node) + } + return inForce, why, nil + } +} + +// busUserCanTool is the bus module's answer to whether a user may do something, and whether it is in force. +const busUserCanTool = "nats_user_can" + +// grantInForce reads the bus module's answer: in force only when it says so, in so many words. An answer +// without `in_force` is a bus module older than the question, and is not a yes. +func grantInForce(raw json.RawMessage) (bool, string) { + var answer struct { + Allowed bool `json:"allowed"` + InForce *bool `json:"in_force"` + Why string `json:"why"` + } + if err := json.Unmarshal(raw, &answer); err != nil { + return false, "the bus module's answer could not be read" + } + switch { + case answer.InForce == nil: + return false, "the bus module does not say whether a grant is in force (it is older than that answer)" + case !*answer.InForce && answer.Why != "": + return false, answer.Why + case !*answer.InForce: + return false, "the bus module says it is not in force" + case !answer.Allowed: + return false, "the bus module says it is in force and not allowed" + } + return true, "" +} + // grantLookEvery is how often the bus's user list is judged against the one its machine was last sent. const grantLookEvery = 30 * time.Second @@ -302,7 +427,19 @@ func startAsking(ctx context.Context, open *stores, server *link.Server, conn *n fmt.Printf("the operator cannot be asked: %v\n", err) return } - a := &asker{ + a := servingAsker(open, conn, js, keeper) + if err := server.Decides(a); err != nil { + fmt.Printf("the operator's answers cannot be heard, so nothing is asked: %v\n", err) + return + } + askerFrom = a + go a.keep(ctx) +} + +// servingAsker is the serving controller's asker, wired to the mesh: what it reads, publishes, performs and +// records, and the judgements it asks by — the bus's grant to ask among them (novox/hq issue 353). +func servingAsker(open *stores, conn *nats.Conn, js jetstream.JetStream, keeper *conditions.Keeper) *asker { + return &asker{ open: keeper.Open, silence: func(ctx context.Context, key string, d time.Duration, by, why string) error { _, err := keeper.Silence(ctx, key, d, by, why) @@ -322,7 +459,7 @@ func startAsking(ctx context.Context, open *stores, server *link.Server, conn *n }, routerRecord: routerRecordOf(conn, open.inventory), routerHere: routerHereIn(open.inventory), - grantHeld: grantHeldIn(open), + grantHeld: grantHeldIn(open, conn), channels: channelsIn(open.inventory), raise: func(ctx context.Context, obs []conditions.Observation) error { return keeper.Reconcile(ctx, sourceAsker, obs) @@ -330,10 +467,4 @@ func startAsking(ctx context.Context, open *stores, server *link.Server, conn *n now: time.Now, logf: func(format string, args ...any) { fmt.Printf(format+"\n", args...) }, } - if err := server.Decides(a); err != nil { - fmt.Printf("the operator's answers cannot be heard, so nothing is asked: %v\n", err) - return - } - askerFrom = a - go a.keep(ctx) } diff --git a/cmd/mesh-controller/delivery_order_test.go b/cmd/mesh-controller/delivery_order_test.go index 878a2016..e5776e50 100644 --- a/cmd/mesh-controller/delivery_order_test.go +++ b/cmd/mesh-controller/delivery_order_test.go @@ -119,6 +119,13 @@ func TestTheBusMachineIsBehindByItsUserListAlone(t *testing.T) { if userListBehind("", "") { t.Fatal("a machine sent no list reads as behind") } + // A list that could not be composed is behind: not known is never "not behind" (the review of hq issue 353). + if !listBehind("", errors.New("the plan cannot be worked out"), digestOf([]byte(list))) { + t.Fatal("a list nobody could compose reads as current") + } + if listBehind(list, nil, digestOf([]byte(list))) || !listBehind(list, nil, "") { + t.Fatal("listBehind does not read the digest as userListBehind does") + } } // The machine holding the bus goes first: its declaration carries the user list the new grants are diff --git a/cmd/mesh-controller/proposals.go b/cmd/mesh-controller/proposals.go index 124aee3d..6dd4d000 100644 --- a/cmd/mesh-controller/proposals.go +++ b/cmd/mesh-controller/proposals.go @@ -591,7 +591,7 @@ func proposeCommand(ctx context.Context, module, node, valuesArg string, clear, return err }, routerHere: routerHereIn(open.inventory), - grantHeld: grantHeldIn(open), + grantHeld: grantHeldIn(open, conn), routerRecord: func(ctx context.Context, id string) (string, error) { bucket, err := asksRecords(ctx, open.inventory) if err != nil || bucket == "" { diff --git a/cmd/mesh-controller/push.go b/cmd/mesh-controller/push.go index 723eb7bc..236b9020 100644 --- a/cmd/mesh-controller/push.go +++ b/cmd/mesh-controller/push.go @@ -1106,20 +1106,29 @@ func brokerBehind(ctx context.Context, open *stores, names []string) (string, bo return h.Node, false, nil } } + // **What cannot be worked out is behind** (the review of hq issue 353). The list it would be sent cannot be + // composed, so whether it carries the grants a send relies on is not known; it is taken as behind, so a push + // sends that machine first and says why it cannot (`plan` says it too), and a rollout that needs the list + // refuses rather than sending code whose grants the bus may not hold. It said "not behind" until then: a + // send went ahead as if the bus held a list nobody could compose. + list, composeErr := "", error(nil) plan, _, err := planFor(ctx, open, h.Node) if err != nil { - // It cannot be worked out: sending it would refuse the whole send, and `plan` says why. - return h.Node, false, nil - } - list, _, err := busUserList(ctx, inv, plan.Modules) - if err != nil { - return h.Node, false, nil + composeErr = err + } else if list, _, err = busUserList(ctx, inv, plan.Modules); err != nil { + composeErr = err } sent, err := inv.SentBusUsers(ctx, h.Node) if err != nil { return "", false, err } - return h.Node, userListBehind(list, sent), nil + return h.Node, listBehind(list, composeErr, sent), nil +} + +// listBehind is whether the bus's machine is behind: the list composed now is not the one last sent, or it +// could not be composed at all — not known is behind, never "not behind". +func listBehind(list string, composeErr error, sentDigest string) bool { + return composeErr != nil || userListBehind(list, sentDigest) } // userListBehind is whether the user list composed now is not the one last sent, by its digest. An From bcfcfaaaf371d2a5f62b2ff549254e1a86fabaed Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 12:34:07 +0200 Subject: [PATCH 2/3] Keep asking on an older bus module and name the real remedy for each grant not held (review of hq issue 353) A bus module older than its in_force answer read as not held, so a merge before mesh-catalog #166 is live would silence every ask until a person's bus upgrade. Such an answer now falls back to the check main makes (is the list sent to the bus's machine the one composed now), says the reload is not confirmed, and each case names its own remedy: push for a list behind or not allowing it, a ~5 s wait for a reload, the bus upgrade when an older module's list cannot be judged. --- cmd/mesh-controller/asker.go | 9 ++ cmd/mesh-controller/asker_test.go | 104 ++++++++++++++++++++--- cmd/mesh-controller/asker_wire.go | 137 ++++++++++++++++++++++-------- 3 files changed, 201 insertions(+), 49 deletions(-) diff --git a/cmd/mesh-controller/asker.go b/cmd/mesh-controller/asker.go index 8a7233dd..4bf3b117 100644 --- a/cmd/mesh-controller/asker.go +++ b/cmd/mesh-controller/asker.go @@ -213,6 +213,8 @@ type asker struct { saidNoRouter bool saidNoGrant bool + // saidGrantCaveat is whether a grant held with a caveat was said, so it is said once. + saidGrantCaveat bool mu sync.Mutex nudged chan struct{} @@ -317,6 +319,13 @@ func (a *asker) reconcile(ctx context.Context) error { } return a.sayUnasked(ctx, unasked, "the bus does not hold the controller's grant to ask yet: "+why) } + if why != "" && !a.saidGrantCaveat { + // Held, with a caveat worth one line (an older bus module cannot confirm the reload). + a.logf("the controller asks: %s", why) + a.saidGrantCaveat = true + } else if why == "" { + a.saidGrantCaveat = false + } a.saidNoGrant = false } channels := "" diff --git a/cmd/mesh-controller/asker_test.go b/cmd/mesh-controller/asker_test.go index fc56f9af..6fe315fb 100644 --- a/cmd/mesh-controller/asker_test.go +++ b/cmd/mesh-controller/asker_test.go @@ -871,24 +871,102 @@ func TestTheControllersOwnGrantToAskIsReadFromTheComposition(t *testing.T) { } } -// The bus module's answer: in force only when it says so in so many words. -func TestTheBusModulesAnswerIsInForceOnlyWhenItSaysSo(t *testing.T) { +// The bus module's answer, judged with the check before it as the floor (the review of PR 188): an answer +// without in_force is an older module, judged by whether the list sent to its machine is the one composed now; +// each way it is not held names its own remedy. +func TestTheBusModulesAnswerNamesTheRemedyForEachCase(t *testing.T) { for _, c := range []struct { - answer string - want bool - says string + name, answer string + behind bool + behindErr error + held bool + says, not string }{ - {`{"allowed":true,"in_force":true}`, true, ""}, - {`{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, false, "not reloaded"}, - {`{"allowed":false,"in_force":false}`, false, "not in force"}, - {`{"allowed":true}`, false, "older than that answer"}, - {`{"allowed":false,"in_force":true}`, false, "not allowed"}, - {`not json`, false, "could not be read"}, + {"in force", `{"allowed":true,"in_force":true}`, false, nil, true, "", "push"}, + {"in force, list behind by the record", `{"allowed":true,"in_force":true}`, true, nil, true, "", "push"}, + {"written, not reloaded", `{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, + false, nil, false, "within about 5 s", "push"}, + {"not allowed", `{"allowed":false,"in_force":false}`, false, nil, false, "`push anchor` carries it", "5 s"}, + {"in force and not allowed", `{"allowed":false,"in_force":true}`, false, nil, false, "`push anchor`", "5 s"}, + {"older module, list current", `{"allowed":true}`, false, nil, true, "not confirmed", "push"}, + {"older module, list behind", `{"allowed":true}`, true, nil, false, "`push anchor` carries it", "5 s"}, + {"older module, list not allowed", `{"allowed":false}`, false, nil, false, "`push anchor` carries it", "5 s"}, + {"older module, list unknown", `{"allowed":true}`, false, errors.New("the store is away"), false, "`bus upgrade`", "push"}, + {"not json", `not json`, false, nil, false, "could not be read", ""}, } { - if got, why := grantInForce(json.RawMessage(c.answer)); got != c.want || !strings.Contains(why, c.says) { - t.Errorf("%s: %v %q", c.answer, got, why) + judge := busGrantBy( + func(context.Context) (string, json.RawMessage, error) { + return "anchor", json.RawMessage(c.answer), nil + }, + func(context.Context) (string, bool, error) { return "anchor", c.behind, c.behindErr }) + held, why, _ := judgeGrantOnce(judge) + if held != c.held || !strings.Contains(why, c.says) || (c.not != "" && strings.Contains(why, c.not)) { + t.Errorf("%s: held %v (%q)", c.name, held, why) } } + none := busGrantBy(func(context.Context) (string, json.RawMessage, error) { return "", nil, nil }, nil) + if held, why, _ := judgeGrantOnce(none); held || !strings.Contains(why, "no module holds the bus") { + t.Errorf("no bus: held %v (%q)", held, why) + } +} + +// judgeGrantOnce is the whole judgement with the composed grant held, as the asker makes it. +func judgeGrantOnce(bus grantJudge) (bool, string, error) { + yes := func(context.Context) (bool, string, error) { return true, "", nil } + return grantHeldBy(yes, bus, time.Now)(context.Background()) +} + +// The order the review of PR 188 found: this controller live before the bus module that answers in_force. An +// older module with the list sent current asks; with the list behind it asks nothing and the condition names the +// push; a new module that has not reloaded names the wait, not a push. +func TestAnOlderBusModuleNeitherSilencesTheAsksNorNamesTheWrongRemedy(t *testing.T) { + for _, c := range []struct { + name, answer string + behind bool + asks bool + says, not string + }{ + {"older module, list current", `{"allowed":true}`, false, true, "", ""}, + {"older module, list behind", `{"allowed":true}`, true, false, "`push anchor` carries it", "5 s"}, + {"new module, not reloaded", `{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, + false, false, "within about 5 s", "push"}, + } { + t.Run(c.name, func(t *testing.T) { + r := newAskerRig(t) + var raised [][]conditions.Observation + r.a.raise = func(_ context.Context, obs []conditions.Observation) error { + raised = append(raised, obs) + return nil + } + yes := func(context.Context) (bool, string, error) { return true, "", nil } + r.a.grantHeld = grantHeldBy(yes, busGrantBy( + func(context.Context) (string, json.RawMessage, error) { + return "anchor", json.RawMessage(c.answer), nil + }, + func(context.Context) (string, bool, error) { return "anchor", c.behind, nil }), + func() time.Time { return r.now }) + r.open = []conditions.Condition{heldCondition()} + _ = r.a.reconcile(context.Background()) + sent := r.asksSent(t) + if c.asks { + if len(sent) != 1 || sent[0].About != heldCondition().Key { + t.Fatalf("not asked: %+v", sent) + } + return + } + if len(sent) != 0 { + t.Fatalf("asked while the bus does not hold the grant: %+v", sent) + } + if len(raised) == 0 || len(raised[len(raised)-1]) != 1 || + raised[len(raised)-1][0].Kind != "asks-undelivered" { + t.Fatalf("not said as undelivered: %+v", raised) + } + summary := raised[len(raised)-1][0].Summary + if !strings.Contains(summary, c.says) || (c.not != "" && strings.Contains(summary, c.not)) { + t.Errorf("the condition names the wrong remedy: %q", summary) + } + }) + } } // The serving asker is wired to the real judgement: with records and a bus it cannot read, the grant is not diff --git a/cmd/mesh-controller/asker_wire.go b/cmd/mesh-controller/asker_wire.go index 6c34aa9d..4b711d54 100644 --- a/cmd/mesh-controller/asker_wire.go +++ b/cmd/mesh-controller/asker_wire.go @@ -272,8 +272,10 @@ func grantHeldBy(composed, inForce grantJudge, now func() time.Time) func(ctx co } } -// judgeGrant is held only when both facts hold; an error is a fact not known, never a yes. +// judgeGrant is held only when both facts hold; an error is a fact not known, never a yes. A fact that holds +// with a caveat (a reload not confirmed) gives that caveat as the why of a yes. func judgeGrant(ctx context.Context, composed, inForce grantJudge) (bool, string) { + var caveat []string for _, fact := range []struct { judge grantJudge what string @@ -288,8 +290,11 @@ func judgeGrant(ctx context.Context, composed, inForce grantJudge) (bool, string if !ok { return false, why } + if why != "" { + caveat = append(caveat, why) + } } - return true, "" + return true, strings.Join(caveat, "; ") } // controllerMayAsk says whether the controller's own grant, as composed from these records, lets it publish its @@ -331,62 +336,122 @@ func composedGrantIn(open *stores) grantJudge { } } -// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus. +// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus, with the +// judgement main made before it (novox/hq issue 353, PR 186: is the user list sent to the bus's machine the one +// composed now) as the floor for a bus module too old to say whether a grant is in force. func busGrantIn(open *stores, conn *nats.Conn) grantJudge { - return func(ctx context.Context) (bool, string, error) { + ask := func(ctx context.Context) (string, json.RawMessage, error) { if open == nil || open.inventory == nil || conn == nil { - return false, "", errors.New("the mesh's records or the bus are not open") + return "", nil, errors.New("the mesh's records or the bus are not open") } holders, err := seatHolders(ctx, open.inventory) if err != nil { - return false, "", err + return "", nil, err } h, held := holders[theBrokerSeat] if !held || h.Node == "" || h.Module == "" { - return false, "no module holds the bus, so nothing says what it enforces", nil + return "", nil, nil } a, err := link.AskModuleToolOn(ctx, conn, h.Module, busUserCanTool, h.Node, map[string]any{ "user": broker.ControllerName, "action": "publish", "subject": asks.AskSubject(askerName)}, 15*time.Second) if err != nil { - return false, "", err + return h.Node, nil, err } if a.Error != "" { - return false, "", errors.New(a.Error) + return h.Node, nil, errors.New(a.Error) } - inForce, why := grantInForce(a.Result) - if !inForce { - why = fmt.Sprintf("the bus on %s does not enforce the controller's grant to ask yet: %s; `push %s` "+ - "carries it, and the bus reloads it in place", h.Node, why, h.Node) + return h.Node, a.Result, nil + } + behind := func(ctx context.Context) (string, bool, error) { + if open == nil || open.inventory == nil { + return "", false, errors.New("the mesh's records are not open") } - return inForce, why, nil + return brokerBehind(ctx, open, nil) + } + return busGrantBy(ask, behind) +} + +// busGrantBy judges the bus's word, and names for each way it is not held the one thing that makes it held: +// +// - **the list there does not grant it**: `push ` sends the list composed now; +// - **written, not reloaded**: the bus reloads it in place within about 5 s, so the remedy is to wait; +// - **the module is older than `in_force`**: the bus cannot confirm a reload, so the floor is main's check +// before it — held while the list sent there is the one composed now, `push ` while it is behind, and +// the bus upgrade (which carries a module that answers `in_force`) when even that cannot be worked out. A +// bus module too old is not a reason to ask nothing: an order of merges that leaves it so would silence +// every ask until a person's bus upgrade (the review of PR 188). +// +// ask answers the machine holding the bus and its module's answer ("" when nothing holds the bus); behind is +// whether the list sent to that machine is behind what the mesh composes. +func busGrantBy(ask func(ctx context.Context) (string, json.RawMessage, error), + behind func(ctx context.Context) (string, bool, error)) grantJudge { + return func(ctx context.Context) (bool, string, error) { + node, raw, err := ask(ctx) + if err != nil { + return false, "", err + } + if node == "" { + return false, "no module holds the bus, so nothing says what it enforces", nil + } + answer, err := readBusAnswer(raw) + if err != nil { + return false, "", fmt.Errorf("the bus module's answer on %s could not be read: %w", node, err) + } + pushIt := func(what string) string { + return fmt.Sprintf("%s, so the bus has not been given the controller's grant to ask; `push %s` carries it", + what, node) + } + switch { + case answer.InForce == nil: + const older = "the bus module there is older than the answer whether a grant is in force, so its " + + "reload is not confirmed" + if !answer.Allowed { + return false, pushIt(fmt.Sprintf("the user list on %s does not allow the controller to ask", node)) + + " (" + older + ")", nil + } + machine, late, err := behind(ctx) + if err != nil { + return false, "", fmt.Errorf("%s, and whether the list sent to %s is the one composed now could "+ + "not be worked out (%v); the bus upgrade (`bus upgrade`) carries a module that says", older, + node, err) + } + if late { + if machine == "" { + machine = node + } + return false, fmt.Sprintf("the bus's user list on %s is behind what the mesh composes, so the bus has "+ + "not been given the controller's grant to ask; `push %s` carries it (%s)", machine, machine, older), nil + } + return true, fmt.Sprintf("the user list on %s is the one composed now and allows the controller to ask; "+ + "%s, and the bus upgrade (`bus upgrade`) carries a module that confirms it", node, older), nil + case !*answer.InForce && answer.Allowed: + why := "the server has not reloaded since it was written" + if answer.Why != "" { + why = answer.Why + } + return false, fmt.Sprintf("the bus on %s has the controller's grant to ask written but not in force yet "+ + "(%s); the bus reloads it in place within about 5 s, so nothing is to be done but wait", node, why), nil + case !answer.Allowed: + return false, pushIt(fmt.Sprintf("the user list on %s does not allow the controller to ask", node)), nil + } + return true, "", nil } } // busUserCanTool is the bus module's answer to whether a user may do something, and whether it is in force. const busUserCanTool = "nats_user_can" -// grantInForce reads the bus module's answer: in force only when it says so, in so many words. An answer -// without `in_force` is a bus module older than the question, and is not a yes. -func grantInForce(raw json.RawMessage) (bool, string) { - var answer struct { - Allowed bool `json:"allowed"` - InForce *bool `json:"in_force"` - Why string `json:"why"` - } - if err := json.Unmarshal(raw, &answer); err != nil { - return false, "the bus module's answer could not be read" - } - switch { - case answer.InForce == nil: - return false, "the bus module does not say whether a grant is in force (it is older than that answer)" - case !*answer.InForce && answer.Why != "": - return false, answer.Why - case !*answer.InForce: - return false, "the bus module says it is not in force" - case !answer.Allowed: - return false, "the bus module says it is in force and not allowed" - } - return true, "" +// busAnswer is the bus module's answer to busUserCanTool; InForce is nil from a module older than that field. +type busAnswer struct { + Allowed bool `json:"allowed"` + InForce *bool `json:"in_force"` + Why string `json:"why"` +} + +func readBusAnswer(raw json.RawMessage) (busAnswer, error) { + var answer busAnswer + err := json.Unmarshal(raw, &answer) + return answer, err } // grantLookEvery is how often the bus's user list is judged against the one its machine was last sent. From f996cf7b6b9bfa230efc5aec36ae96bb166e721d Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 12:55:46 +0200 Subject: [PATCH 3/3] Take an older bus module's allowed as held, and name only remedies that work (review 31 of PR 188, hq issue 353) After mesh-catalog #166 merges and before the bus upgrade, the bus's machine cannot be pushed, so any change to the composed list made the list check read behind for good: every ask was withheld and the condition named a push that is refused. An older module's allowed now counts as held, the unconfirmed reload said once; the worst case is one ask refused in the seconds before a reload, which is asked again (issue 369). The list check stays for a module that says not allowed or cannot answer, and its remedy is the plan's own reason when the list cannot be composed, the bus upgrade while sends there wait on it, and a push otherwise. An in_force false is a wait only when the module gives both times; otherwise what is not known is named. --- cmd/mesh-controller/asker_test.go | 86 ++++++++++----- cmd/mesh-controller/asker_wire.go | 178 +++++++++++++++++++----------- cmd/mesh-controller/push.go | 21 ++-- 3 files changed, 189 insertions(+), 96 deletions(-) diff --git a/cmd/mesh-controller/asker_test.go b/cmd/mesh-controller/asker_test.go index 6fe315fb..554214d9 100644 --- a/cmd/mesh-controller/asker_test.go +++ b/cmd/mesh-controller/asker_test.go @@ -871,34 +871,49 @@ func TestTheControllersOwnGrantToAskIsReadFromTheComposition(t *testing.T) { } } -// The bus module's answer, judged with the check before it as the floor (the review of PR 188): an answer -// without in_force is an older module, judged by whether the list sent to its machine is the one composed now; -// each way it is not held names its own remedy. +// The bus module's answer, with the record's word on the list sent as the check only where the answer does +// not settle it (reviews 22 and 31 of PR 188); each way it is not held names its own remedy. func TestTheBusModulesAnswerNamesTheRemedyForEachCase(t *testing.T) { + const ( + reloaded = `{"allowed":true,"in_force":true,"file_written":"2026-10-09T15:55:56Z","server_loaded":"2026-10-09T15:56:05Z"}` + notLoaded = `{"allowed":true,"in_force":false,"why":"the user list on the machine allows it, and the server has not reloaded since it was written","file_written":"2026-10-09T15:55:56Z","server_loaded":"2026-10-09T15:50:00Z"}` + noVarz = `{"allowed":true,"in_force":false,"why":"the server's last configuration load could not be read: refused"}` + noStat = `{"allowed":true,"in_force":false,"why":"when the bus's user list was written could not be read inside mesh-broker-nats: exit 1"}` + notAllowed = `{"allowed":false,"in_force":false,"why":"the user list on the machine does not allow it"}` + ) + current := busListState{Machine: "anchor"} + behind := busListState{Machine: "anchor", Behind: true} + stepped := busListState{Machine: "anchor", Behind: true, BusStep: "sending anchor would replace the bus; `bus upgrade --why …`"} + uncomposed := busListState{Machine: "anchor", Behind: true, Uncomposed: errors.New("module foo requires a provision nobody provides")} for _, c := range []struct { name, answer string - behind bool - behindErr error + askErr error + list busListState + listErr error held bool says, not string }{ - {"in force", `{"allowed":true,"in_force":true}`, false, nil, true, "", "push"}, - {"in force, list behind by the record", `{"allowed":true,"in_force":true}`, true, nil, true, "", "push"}, - {"written, not reloaded", `{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, - false, nil, false, "within about 5 s", "push"}, - {"not allowed", `{"allowed":false,"in_force":false}`, false, nil, false, "`push anchor` carries it", "5 s"}, - {"in force and not allowed", `{"allowed":false,"in_force":true}`, false, nil, false, "`push anchor`", "5 s"}, - {"older module, list current", `{"allowed":true}`, false, nil, true, "not confirmed", "push"}, - {"older module, list behind", `{"allowed":true}`, true, nil, false, "`push anchor` carries it", "5 s"}, - {"older module, list not allowed", `{"allowed":false}`, false, nil, false, "`push anchor` carries it", "5 s"}, - {"older module, list unknown", `{"allowed":true}`, false, errors.New("the store is away"), false, "`bus upgrade`", "push"}, - {"not json", `not json`, false, nil, false, "could not be read", ""}, + {"in force", reloaded, nil, behind, nil, true, "", "push"}, + {"written, not reloaded", notLoaded, nil, current, nil, false, "within about 5 s", "push"}, + {"in force not known: varz", noVarz, nil, current, nil, false, "is not known: the server's last configuration load", "wait"}, + {"in force not known: stat", noStat, nil, current, nil, false, "is not known: when the bus's user list was written", "wait"}, + {"not allowed, list behind", notAllowed, nil, behind, nil, false, "`push anchor` carries it", "5 s"}, + {"not allowed, bus step pending", notAllowed, nil, stepped, nil, false, "`bus upgrade", "`push anchor`"}, + {"not allowed, list uncomposed", notAllowed, nil, uncomposed, nil, false, "`plan anchor`", "`push anchor`"}, + {"older module, allowed, list current", `{"allowed":true}`, nil, current, nil, true, "not confirmed", "push"}, + {"older module, allowed, list behind", `{"allowed":true}`, nil, behind, nil, true, "not confirmed", "push"}, + {"older module, allowed, list unknown", `{"allowed":true}`, nil, current, errors.New("the store is away"), true, "`bus upgrade`", "push"}, + {"older module, not allowed, list behind", `{"allowed":false}`, nil, behind, nil, false, "`push anchor` carries it", "5 s"}, + {"module cannot answer, list current", "", errors.New("no responders"), current, nil, true, "could not answer", "push"}, + {"module cannot answer, list behind", "", errors.New("no responders"), behind, nil, false, "`push anchor` carries it", ""}, + {"module cannot answer, list unknown", "", errors.New("no responders"), current, errors.New("the store is away"), false, "could not be worked out", ""}, + {"answer unreadable, list behind", `not json`, nil, behind, nil, false, "could not be read", ""}, } { judge := busGrantBy( func(context.Context) (string, json.RawMessage, error) { - return "anchor", json.RawMessage(c.answer), nil + return "anchor", json.RawMessage(c.answer), c.askErr }, - func(context.Context) (string, bool, error) { return "anchor", c.behind, c.behindErr }) + func(context.Context) (busListState, error) { return c.list, c.listErr }) held, why, _ := judgeGrantOnce(judge) if held != c.held || !strings.Contains(why, c.says) || (c.not != "" && strings.Contains(why, c.not)) { t.Errorf("%s: held %v (%q)", c.name, held, why) @@ -916,23 +931,30 @@ func judgeGrantOnce(bus grantJudge) (bool, string, error) { return grantHeldBy(yes, bus, time.Now)(context.Background()) } -// The order the review of PR 188 found: this controller live before the bus module that answers in_force. An -// older module with the list sent current asks; with the list behind it asks nothing and the condition names the -// push; a new module that has not reloaded names the wait, not a push. +// The orders reviews 22 and 31 of PR 188 found: this controller live before the bus module that answers +// in_force, with the bus's machine behind on its list — and, after mesh-catalog #166 merged, unpushable until a +// person's bus upgrade. An older module that allows the ask asks (once, with the caveat said); a new module that +// has not reloaded names the wait, not a push; one that cannot say whether it reloaded names what is not known. func TestAnOlderBusModuleNeitherSilencesTheAsksNorNamesTheWrongRemedy(t *testing.T) { for _, c := range []struct { name, answer string - behind bool + list busListState asks bool says, not string }{ - {"older module, list current", `{"allowed":true}`, false, true, "", ""}, - {"older module, list behind", `{"allowed":true}`, true, false, "`push anchor` carries it", "5 s"}, - {"new module, not reloaded", `{"allowed":true,"in_force":false,"why":"the server has not reloaded since it was written"}`, - false, false, "within about 5 s", "push"}, + {"older module, list current", `{"allowed":true}`, busListState{Machine: "anchor"}, true, "not confirmed", ""}, + {"older module, list behind, bus step pending", `{"allowed":true}`, + busListState{Machine: "anchor", Behind: true, BusStep: "a planned step"}, true, "not confirmed", ""}, + {"new module, not reloaded", + `{"allowed":true,"in_force":false,"why":"not reloaded","file_written":"2026-10-09T15:55:56Z","server_loaded":"2026-10-09T15:50:00Z"}`, + busListState{Machine: "anchor"}, false, "within about 5 s", "push"}, + {"new module, reload not known", `{"allowed":true,"in_force":false,"why":"the server's last configuration load could not be read: refused"}`, + busListState{Machine: "anchor"}, false, "is not known", "wait"}, } { t.Run(c.name, func(t *testing.T) { r := newAskerRig(t) + var said []string + r.a.logf = func(f string, a ...any) { said = append(said, fmt.Sprintf(f, a...)) } var raised [][]conditions.Observation r.a.raise = func(_ context.Context, obs []conditions.Observation) error { raised = append(raised, obs) @@ -943,15 +965,25 @@ func TestAnOlderBusModuleNeitherSilencesTheAsksNorNamesTheWrongRemedy(t *testing func(context.Context) (string, json.RawMessage, error) { return "anchor", json.RawMessage(c.answer), nil }, - func(context.Context) (string, bool, error) { return "anchor", c.behind, nil }), + func(context.Context) (busListState, error) { return c.list, nil }), func() time.Time { return r.now }) r.open = []conditions.Condition{heldCondition()} _ = r.a.reconcile(context.Background()) + _ = r.a.reconcile(context.Background()) sent := r.asksSent(t) if c.asks { if len(sent) != 1 || sent[0].About != heldCondition().Key { t.Fatalf("not asked: %+v", sent) } + n := 0 + for _, s := range said { + if strings.Contains(s, c.says) { + n++ + } + } + if n != 1 { + t.Errorf("the caveat said %d times: %q", n, said) + } return } if len(sent) != 0 { diff --git a/cmd/mesh-controller/asker_wire.go b/cmd/mesh-controller/asker_wire.go index 4b711d54..aadff952 100644 --- a/cmd/mesh-controller/asker_wire.go +++ b/cmd/mesh-controller/asker_wire.go @@ -336,9 +336,9 @@ func composedGrantIn(open *stores) grantJudge { } } -// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus, with the -// judgement main made before it (novox/hq issue 353, PR 186: is the user list sent to the bus's machine the one -// composed now) as the floor for a bus module too old to say whether a grant is in force. +// busGrantIn is the second fact, asked of the bus's own module on the machine holding the bus, with the check +// main made before it (novox/hq issue 353, PR 186: is the user list sent to the bus's machine the one composed +// now) kept for the cases the module's word does not settle. func busGrantIn(open *stores, conn *nats.Conn) grantJudge { ask := func(ctx context.Context) (string, json.RawMessage, error) { if open == nil || open.inventory == nil || conn == nil { @@ -362,90 +362,144 @@ func busGrantIn(open *stores, conn *nats.Conn) grantJudge { } return h.Node, a.Result, nil } - behind := func(ctx context.Context) (string, bool, error) { + list := func(ctx context.Context) (busListState, error) { if open == nil || open.inventory == nil { - return "", false, errors.New("the mesh's records are not open") + return busListState{}, errors.New("the mesh's records are not open") } - return brokerBehind(ctx, open, nil) + s, err := brokerListState(ctx, open, nil) + if err != nil || s.Machine == "" || !s.Behind { + return s, err + } + step, err := busHeld(ctx, open.inventory, []string{s.Machine}) + if err != nil { + return busListState{}, err + } + s.BusStep = step[s.Machine] + return s, nil } - return busGrantBy(ask, behind) + return busGrantBy(ask, list) +} + +// busListState is the user list on the bus's machine as the controller's record knows it: whether the list +// sent there is behind the one composed now, why it could not be composed, and the bus step a send there waits +// on (that machine cannot be pushed until a person's `bus upgrade`). +type busListState struct { + Machine string + Behind bool + Uncomposed error + BusStep string +} + +// remedy is the one thing that sends the bus's machine the list composed now: the plan's own reason when it +// cannot be composed, the bus upgrade while sends there wait on it, and otherwise a push of that machine. +func (s busListState) remedy(node string) string { + if s.Machine != "" { + node = s.Machine + } + switch { + case s.Uncomposed != nil: + return fmt.Sprintf("the user list for %s cannot be composed (%v); `plan %s` says what to fix first", + node, s.Uncomposed, node) + case s.BusStep != "": + return fmt.Sprintf("sends to %s wait on the bus's planned step, so a push will not carry it: %s", node, s.BusStep) + } + return fmt.Sprintf("`push %s` carries it", node) } // busGrantBy judges the bus's word, and names for each way it is not held the one thing that makes it held: // -// - **the list there does not grant it**: `push ` sends the list composed now; -// - **written, not reloaded**: the bus reloads it in place within about 5 s, so the remedy is to wait; -// - **the module is older than `in_force`**: the bus cannot confirm a reload, so the floor is main's check -// before it — held while the list sent there is the one composed now, `push ` while it is behind, and -// the bus upgrade (which carries a module that answers `in_force`) when even that cannot be worked out. A -// bus module too old is not a reason to ask nothing: an order of merges that leaves it so would silence -// every ask until a person's bus upgrade (the review of PR 188). +// - **in force**: held; +// - **allowed, from a module older than `in_force`**: held, with the caveat that the reload is not confirmed. +// The file allows the publish, and the bus reloads within about 5 s of a change to it, so the worst case is +// one ask refused in those seconds, which is asked again (issue 369). The list check is not consulted: +// while a bus step is pending the bus's machine cannot be pushed, so any change to the composed list would +// read as behind for good and silence every ask until a person's bus upgrade (the review of PR 188); +// - **allowed, written, not reloaded** (the module gives both times): wait about 5 s; +// - **allowed, in force not known** (the module could not read a time): what is not known, said; +// - **not allowed, or the module cannot answer**: the list check decides — the remedy that sends the list +// when it is behind (busListState.remedy), and, for a module that cannot answer, held while the list sent +// is current, as main judged it. // -// ask answers the machine holding the bus and its module's answer ("" when nothing holds the bus); behind is -// whether the list sent to that machine is behind what the mesh composes. +// ask answers the machine holding the bus and its module's answer ("" when nothing holds the bus); list is the +// record's word on the list sent to that machine. func busGrantBy(ask func(ctx context.Context) (string, json.RawMessage, error), - behind func(ctx context.Context) (string, bool, error)) grantJudge { + list func(ctx context.Context) (busListState, error)) grantJudge { return func(ctx context.Context) (bool, string, error) { - node, raw, err := ask(ctx) - if err != nil { - return false, "", err - } - if node == "" { + node, raw, askErr := ask(ctx) + if askErr == nil && node == "" { return false, "no module holds the bus, so nothing says what it enforces", nil } - answer, err := readBusAnswer(raw) - if err != nil { - return false, "", fmt.Errorf("the bus module's answer on %s could not be read: %w", node, err) + var answer busAnswer + if askErr == nil { + if answer, askErr = readBusAnswer(raw); askErr != nil { + askErr = fmt.Errorf("its answer could not be read: %w", askErr) + } } - pushIt := func(what string) string { - return fmt.Sprintf("%s, so the bus has not been given the controller's grant to ask; `push %s` carries it", - what, node) + if askErr != nil { + if node == "" { + node = "the bus's machine" + } + s, err := list(ctx) + if err != nil { + return false, "", fmt.Errorf("the bus module on %s could not answer (%v), and whether the list sent "+ + "there is the one composed now could not be worked out: %w", node, askErr, err) + } + if s.Behind { + return false, fmt.Sprintf("the bus module on %s could not answer (%v), and the user list sent there is "+ + "behind what the mesh composes; %s", node, askErr, s.remedy(node)), nil + } + return true, fmt.Sprintf("the bus module on %s could not answer (%v); judged by the user list sent there, "+ + "which is the one composed now", node, askErr), nil + } + notAllowed := func(older string) (bool, string, error) { + s, err := list(ctx) + if err != nil { + return false, "", fmt.Errorf("the user list on %s does not allow the controller to ask%s, and whether "+ + "the list sent there is the one composed now could not be worked out: %w", node, older, err) + } + if s.Behind { + return false, fmt.Sprintf("the user list on %s does not allow the controller to ask%s: it is behind "+ + "what the mesh composes; %s", node, older, s.remedy(node)), nil + } + return false, fmt.Sprintf("the user list on %s does not allow the controller to ask%s, though the list "+ + "sent there is the one composed now; %s again", node, older, s.remedy(node)), nil } switch { + case answer.InForce == nil && answer.Allowed: + return true, fmt.Sprintf("the user list on %s allows the controller to ask, but its reload is not "+ + "confirmed: the bus module there is older than the answer whether a grant is in force, and the bus "+ + "upgrade (`bus upgrade`) carries one that confirms it", node), nil case answer.InForce == nil: - const older = "the bus module there is older than the answer whether a grant is in force, so its " + - "reload is not confirmed" - if !answer.Allowed { - return false, pushIt(fmt.Sprintf("the user list on %s does not allow the controller to ask", node)) + - " (" + older + ")", nil - } - machine, late, err := behind(ctx) - if err != nil { - return false, "", fmt.Errorf("%s, and whether the list sent to %s is the one composed now could "+ - "not be worked out (%v); the bus upgrade (`bus upgrade`) carries a module that says", older, - node, err) - } - if late { - if machine == "" { - machine = node - } - return false, fmt.Sprintf("the bus's user list on %s is behind what the mesh composes, so the bus has "+ - "not been given the controller's grant to ask; `push %s` carries it (%s)", machine, machine, older), nil - } - return true, fmt.Sprintf("the user list on %s is the one composed now and allows the controller to ask; "+ - "%s, and the bus upgrade (`bus upgrade`) carries a module that confirms it", node, older), nil - case !*answer.InForce && answer.Allowed: - why := "the server has not reloaded since it was written" - if answer.Why != "" { - why = answer.Why - } - return false, fmt.Sprintf("the bus on %s has the controller's grant to ask written but not in force yet "+ - "(%s); the bus reloads it in place within about 5 s, so nothing is to be done but wait", node, why), nil + return notAllowed(" (the bus module there is older than the answer whether a grant is in force)") case !answer.Allowed: - return false, pushIt(fmt.Sprintf("the user list on %s does not allow the controller to ask", node)), nil + return notAllowed("") + case *answer.InForce: + return true, "", nil + case answer.FileWritten != "" && answer.ServerLoaded != "": + return false, fmt.Sprintf("the bus on %s has the controller's grant to ask written (%s) but has not "+ + "reloaded since (last load %s); it reloads in place within about 5 s, so nothing is to be done but wait", + node, answer.FileWritten, answer.ServerLoaded), nil } - return true, "", nil + why := answer.Why + if why == "" { + why = "it gives no reason" + } + return false, fmt.Sprintf("the user list on %s allows the controller to ask, but whether the bus enforces it "+ + "is not known: %s", node, why), nil } } // busUserCanTool is the bus module's answer to whether a user may do something, and whether it is in force. const busUserCanTool = "nats_user_can" -// busAnswer is the bus module's answer to busUserCanTool; InForce is nil from a module older than that field. +// busAnswer is the bus module's answer to busUserCanTool; InForce is nil from a module older than that field, +// and FileWritten and ServerLoaded are given only when it could read both times (mesh-catalog's nats InForce). type busAnswer struct { - Allowed bool `json:"allowed"` - InForce *bool `json:"in_force"` - Why string `json:"why"` + Allowed bool `json:"allowed"` + InForce *bool `json:"in_force"` + Why string `json:"why"` + FileWritten string `json:"file_written"` + ServerLoaded string `json:"server_loaded"` } func readBusAnswer(raw json.RawMessage) (busAnswer, error) { diff --git a/cmd/mesh-controller/push.go b/cmd/mesh-controller/push.go index 236b9020..8aef75c2 100644 --- a/cmd/mesh-controller/push.go +++ b/cmd/mesh-controller/push.go @@ -1084,26 +1084,33 @@ func brokerFirst(names []string, holder string, behind bool) []string { // send anywhere, and a module running there always put it in its first wave. A digest of the list // last sent is kept for this (ADR 0043: the list is composed on each push, never kept itself). func brokerBehind(ctx context.Context, open *stores, names []string) (string, bool, error) { + s, err := brokerListState(ctx, open, names) + return s.Machine, s.Behind, err +} + +// brokerListState is brokerBehind with why the list could not be composed, when it could not: the plan's own +// reason, which a remedy names rather than a push that would fail on it (the review of hq issue 353). +func brokerListState(ctx context.Context, open *stores, names []string) (busListState, error) { inv := open.inventory holders, err := seatHolders(ctx, inv) if err != nil { - return "", false, err + return busListState{}, err } h, held := holders[theBrokerSeat] if !held || h.Node == "" { - return "", false, nil + return busListState{}, nil } shelf, err := inv.Catalogue(ctx) if err != nil { - return "", false, err + return busListState{}, err } if m, known := shelf[h.Module]; !known || m.BusUsers == "" { // A holder that is sent no user list carries no grant: nothing to send first. - return "", false, nil + return busListState{}, nil } for _, n := range names { if n == h.Node { - return h.Node, false, nil + return busListState{Machine: h.Node}, nil } } // **What cannot be worked out is behind** (the review of hq issue 353). The list it would be sent cannot be @@ -1120,9 +1127,9 @@ func brokerBehind(ctx context.Context, open *stores, names []string) (string, bo } sent, err := inv.SentBusUsers(ctx, h.Node) if err != nil { - return "", false, err + return busListState{}, err } - return h.Node, listBehind(list, composeErr, sent), nil + return busListState{Machine: h.Node, Behind: listBehind(list, composeErr, sent), Uncomposed: composeErr}, nil } // listBehind is whether the bus's machine is behind: the list composed now is not the one last sent, or it