From bcfcfaaaf371d2a5f62b2ff549254e1a86fabaed Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 12:34:07 +0200 Subject: [PATCH] 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.