From 7b83aaa9495f76e99b84be58663f340078a91ad1 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Mon, 19 Oct 2026 21:40:27 +0000 Subject: [PATCH] Tell a refusal as one and an unanswered call as not known; ask a plan that fails again within the hour (issues 1316, 1314, review round 1) --- cmd/mesh-controller/asker.go | 62 +++++++++++++++------- cmd/mesh-controller/asker_test.go | 82 +++++++++++++++++++++++++++++- cmd/mesh-controller/asker_wire.go | 8 ++- cmd/mesh-controller/plan_failed.go | 7 ++- 4 files changed, 136 insertions(+), 23 deletions(-) diff --git a/cmd/mesh-controller/asker.go b/cmd/mesh-controller/asker.go index 76ac9a1f..b2081d9d 100644 --- a/cmd/mesh-controller/asker.go +++ b/cmd/mesh-controller/asker.go @@ -409,7 +409,9 @@ func (a *asker) reconcile(ctx context.Context) error { for _, r := range all { k := partKey(r.Condition, r.Part) if r.State == string(asks.OutcomeChosen) && now.Sub(r.Ended) < askAgainAfterAnswer { - answered[k] = r + if prior, has := answered[k]; !has || r.Ended.After(prior.Ended) { + answered[k] = r + } } if r.State == string(asks.OutcomeRefused) { if prior, has := refused[k]; !has || r.Opened.After(prior.Opened) { @@ -500,7 +502,9 @@ func (a *asker) reconcile(ctx context.Context) error { continue // refused lately, and nothing it was refused for has changed: asked again after the wait } } - if r, done := answered[key]; done && sameAsked(r.Actions, p.actions) { + // **A condition raised after the answered ask ended is a new one** (novox/hq ADR 0449 §2): a retried + // plan that fails again is asked again, not held back for the hour its earlier answer holds. + if r, done := answered[key]; done && sameAsked(r.Actions, p.actions) && !c.Raised.After(r.Ended) { if _, held := byCondition[key]; !held { continue } @@ -924,33 +928,53 @@ func (a *asker) Decided(ctx context.Context, body []byte) error { return nil } -// tellRefused tells the operator that the answer they gave to a plan's ask was refused (novox/hq ADR 0449 §3): the -// controller's `plans` verb refuses a Retry, Start or Stop when the plan moved on between the ask and the answer, -// and the refusal was only recorded, so the operator who pressed the answer heard nothing. One plain notice with the -// ask's headline and the refusal's words, beside the hand act the caller still records. An answer to any other ask -// is not told here. A notice that cannot be told is said in the log and changes nothing. -func (a *asker) tellRefused(ctx context.Context, r asked, act conditions.Action, refusal error) { +// tellRefused tells the operator what became of the answer they gave to a plan's ask, when it was not done (novox/hq +// ADR 0449 §3): the controller's `plans` verb refuses a Retry, Start or Stop when the plan moved on between the ask and +// the answer, and the refusal was only recorded, so the operator who pressed the answer heard nothing. **A refusal is +// told as one, and nothing else is**: a verb that said no changed nothing, but a call that got no answer (a timeout, a +// refused grant) may have run, and is told as not known. One plain notice with the ask's headline, beside the hand act +// the caller still records. An answer to any other ask is not told here. A notice that cannot be told is said in the +// log and changes nothing. +func (a *asker) tellRefused(ctx context.Context, r asked, act conditions.Action, failure error) { if a.tell == nil || act.Verb != broker.ControllerSeat+".plans" { return } - lines := []string{ - fmt.Sprintf("Your answer, %s, could not be done: %s.", act.Label, strings.TrimRight(refusalWords(refusal), ".")), - "Nothing was changed.", + var lines []string + var refused verbRefused + if errors.As(failure, &refused) { + lines = []string{ + fmt.Sprintf("Your answer, %s, could not be done: %s.", act.Label, strings.TrimRight(refusalWords(refused.words, act), ".")), + "Nothing was changed.", + } + } else { + lines = []string{ + fmt.Sprintf("Your answer, %s, was given, and it is not known whether it was done.", act.Label), + "Look at the delivery before you answer again.", + } } if err := a.tell(ctx, r.Ask.Headline, lines); err != nil { a.logf("the operator could NOT be told that %s was refused (ask %s): %v", act.Label, r.ID, err) } } -// refusalWords is a refused verb's words as the operator reads them: without the verb's address in front, with the -// plans named as "the plan" (plainRefusal), and, where they still carry an identifier the operator's channel -// withholds, only that the controller refused it. -func refusalWords(err error) string { - text := err.Error() - if i := strings.Index(text, " refused: "); i >= 0 { - text = text[i+len(" refused: "):] +// refusalWords is a verb's refusal as the operator reads it: the plans named as "the plan" (plainRefusal), and, where +// the words still carry an identifier the operator's channel withholds, one sentence of the answer's own. +func refusalWords(words string, act conditions.Action) string { + return plainRefusalOr(words, refusalFallback(act)) +} + +// refusalFallback is what an answer's refusal says when its own words cannot be said: one sentence for each answer +// to a plan's ask, never another answer's. +func refusalFallback(act conditions.Action) string { + switch { + case act.Arguments["retry"] != "": + return "a retry would not run now; the details say why" + case act.Arguments["go"] != "": + return "the delivery could not be started now; the details say why" + case act.Arguments["stop"] != "": + return "the delivery could not be stopped now; the details say why" } - return plainRefusal(text) + return "the controller refused it; the details say why" } // handActWarrant is the verb an act the operator chose on a warrant is recorded under: a person's decision, diff --git a/cmd/mesh-controller/asker_test.go b/cmd/mesh-controller/asker_test.go index 031ad89d..40604d7e 100644 --- a/cmd/mesh-controller/asker_test.go +++ b/cmd/mesh-controller/asker_test.go @@ -1105,7 +1105,7 @@ func TestARefusedAnswerToAPlansAskIsToldOnceAndStillRecorded(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { r := newAskerRig(t) - r.refuses = errors.New("mesh-controller.plans refused: " + plan + " is done; there is nothing to retry") + r.refuses = verbRefused{verb: "mesh-controller.plans", words: plan + " is done; there is nothing to retry"} r.open = []conditions.Condition{tc.c} _ = r.a.reconcile(context.Background()) w := r.warrantFor(t, tc.c.Key, tc.label) @@ -1147,7 +1147,7 @@ func TestADoneAnswerTellsNothingAndOnlyAPlansRefusalIsTold(t *testing.T) { } r = newAskerRig(t) - r.refuses = errors.New("node-service-manager.restart refused: unit not found") + r.refuses = verbRefused{verb: "node-service-manager.restart", words: "unit not found"} module := conditions.Condition{Key: "module.openrazer.g14.unhealthy", Severity: conditions.Warning, Headline: "openrazer not working on g14", Needs: "restart its service openrazer-daemon on g14.", Explanation: "Needs you: restart it.", Actions: []conditions.Action{{Label: "Restart", @@ -1160,3 +1160,81 @@ func TestADoneAnswerTellsNothingAndOnlyAPlansRefusalIsTold(t *testing.T) { t.Fatalf("a refused restart told %+v, acts %+v", r.told, r.acts) } } + +// 1316, review round 1: a call that got no answer may have run, so it is told as not known, never as "nothing was +// changed"; and the sentence a refusal falls back to fits the answer given. +func TestAnAnswerThatGotNoAnswerIsToldAsNotKnownNotAsRefused(t *testing.T) { + plan := "plan-1792433168676870614" + r := newAskerRig(t) + r.refuses = errors.New("mesh-controller did not answer plans within 30s") + c := failedPlanCondition(plan) + r.open = []conditions.Condition{c} + _ = r.a.reconcile(context.Background()) + answerWith(t, r, r.warrantFor(t, c.Key, "Retry")) + if len(r.told) != 1 { + t.Fatalf("told %+v", r.told) + } + said := strings.Join(r.told[0].lines, " ") + if !strings.Contains(said, "not known whether") || strings.Contains(said, "Nothing was changed") { + t.Errorf("a call that may have run is told as %q", said) + } + if why, ok := conditions.Plain(said); !ok { + t.Errorf("not plain: %s: %q", why, said) + } +} + +func TestEachAnswersRefusalFallsBackToItsOwnSentence(t *testing.T) { + plan := "plan-1792433168676870614" + for _, tc := range []struct { + act conditions.Action + want string + }{ + {retryAction(plan), "a retry would not run now"}, + {conditions.Action{Label: "Start", Verb: "mesh-controller.plans", Arguments: map[string]string{"go": plan}}, "could not be started now"}, + {conditions.Action{Label: "Stop", Verb: "mesh-controller.plans", Arguments: map[string]string{"stop": plan}}, "could not be stopped now"}, + } { + // Words the operator's channel withholds: a hash and a dotted name. + got := refusalWords("build 48bda475dunst at mesh.seat.x refused", tc.act) + if !strings.Contains(got, tc.want) { + t.Errorf("%s: %q, want %q", tc.act.Label, got, tc.want) + } + if _, ok := conditions.Plain(got); !ok { + t.Errorf("%s: the fallback is not plain: %q", tc.act.Label, got) + } + } +} + +// 1314/AC6, ADR 0449 §2: a retried plan that fails again within the hour of the answer is asked again, because the +// condition that comes back is a new one, raised after the answered ask ended; the same condition still open is not. +func TestARetriedPlanThatFailsAgainWithinTheHourIsAskedAgain(t *testing.T) { + r := newAskerRig(t) + c := failedPlanCondition("plan-1792433168676870614") + c.Raised = r.now.Add(-time.Minute) + r.open = []conditions.Condition{c} + _ = r.a.reconcile(context.Background()) + answerWith(t, r, r.warrantFor(t, c.Key, "Retry")) + // Still open right after the answer: not asked again at once. + r.now = r.now.Add(time.Minute) + _ = r.a.reconcile(context.Background()) + if n := len(r.asksSent(t)); n != 1 { + t.Fatalf("asked again while the same condition stayed open: %d", n) + } + // The retry ran, the plan left failed and the condition ended. + r.now = r.now.Add(time.Minute) + r.open = nil + _ = r.a.reconcile(context.Background()) + // It fails again, ten minutes after the answer: a condition raised after the answered ask ended. + r.now = r.now.Add(8 * time.Minute) + again := failedPlanCondition("plan-1792433168676870614") + again.Raised = r.now + r.open = []conditions.Condition{again} + if err := r.a.reconcile(context.Background()); err != nil { + t.Fatal(err) + } + if n := len(r.asksSent(t)); n != 2 { + t.Fatalf("the second failure was asked %d times in all, want 2", n) + } + if len(r.called) != 1 { + t.Fatalf("something retried without an answer: %v", r.called) + } +} diff --git a/cmd/mesh-controller/asker_wire.go b/cmd/mesh-controller/asker_wire.go index ba057308..66651dc5 100644 --- a/cmd/mesh-controller/asker_wire.go +++ b/cmd/mesh-controller/asker_wire.go @@ -135,6 +135,12 @@ func (b busAsked) All(ctx context.Context) ([]asked, error) { return out, nil } +// verbRefused is a verb's own refusal of an act: the seat answered, and said no. Any other error of callAction (a bus +// that did not answer, a grant that is refused) is not one: the verb may have run. +type verbRefused struct{ verb, words string } + +func (e verbRefused) Error() string { return e.verb + " refused: " + e.words } + // callAction performs an action's verb as the controller, through the grant that names it. func callAction(conn *nats.Conn) func(ctx context.Context, a conditions.Action, args map[string]string) error { return func(ctx context.Context, a conditions.Action, args map[string]string) error { @@ -168,7 +174,7 @@ func callAction(conn *nats.Conn) func(ctx context.Context, a conditions.Action, return err } if answer.Error != "" { - return fmt.Errorf("%s refused: %s", a.Verb, answer.Error) + return verbRefused{verb: a.Verb, words: answer.Error} } return nil } diff --git a/cmd/mesh-controller/plan_failed.go b/cmd/mesh-controller/plan_failed.go index 16853324..bd82b38c 100644 --- a/cmd/mesh-controller/plan_failed.go +++ b/cmd/mesh-controller/plan_failed.go @@ -92,12 +92,17 @@ var ( // delivery. Whatever still breaks the plain rule (a hash, a dotted name) is left to the condition's evidence, and the // words say only that a retry would not run. func plainRefusal(refusal string) string { + return plainRefusalOr(refusal, "a retry would not run now; the details say why") +} + +// plainRefusalOr is plainRefusal with the sentence to say when the refusal's own words cannot be said plainly. +func plainRefusalOr(refusal, fallback string) string { r := planIDWord.ReplaceAllString(refusal, "the plan") r = walkWd.ReplaceAllString(r, "deliveries") r = walkOne.ReplaceAllString(r, "delivery") r = strings.TrimSpace(r) if _, ok := conditions.Plain(r); !ok { - return "a retry would not run now; the details say why" + return fallback } return r }