From bb96f4d1637d6c74538dc3ef9d52a239c3381e54 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 13:44:43 +0200 Subject: [PATCH 1/2] messenger: every message still shown keeps its own Details, bounded in number and age (issue 371) A newer message about the same problem (an escalation, a reminder, a re-send) replaced the Details ticket of the one before it on that channel kind, so tapping Details on any but the newest was refused "it belongs to no open message". Each message now keeps a ticket of its own, used once, at most MostDetails per record and kind and none older than TokenFor; one pushed out is refused in words that send the operator to the newest message. A change of the condition's actions no longer drops Details, which names no action. --- modules/messenger/cmd/messenger/answers.go | 51 +++++- .../messenger/cmd/messenger/details_test.go | 166 ++++++++++++++++++ modules/messenger/cmd/messenger/holder.go | 9 +- 3 files changed, 218 insertions(+), 8 deletions(-) create mode 100644 modules/messenger/cmd/messenger/details_test.go diff --git a/modules/messenger/cmd/messenger/answers.go b/modules/messenger/cmd/messenger/answers.go index 4c81b708..76a9cbcb 100644 --- a/modules/messenger/cmd/messenger/answers.go +++ b/modules/messenger/cmd/messenger/answers.go @@ -15,9 +15,14 @@ package main // TokenFor, used once; refused for a record cleared or silenced, used, expired, from another kind or // unknown — and the refusal is shown where the answer came from. One answer is acted on once, // whatever its redelivery. +// - **Every message still shown keeps its Details** (issue 371): a newer message about the same problem +// makes a ticket of its own and leaves the older ones standing, so Details works on each message once. +// A record keeps at most MostDetails of them on a kind, and none older than TokenFor; a ticket dropped +// by that bound is refused in words that say to tap Details on the newest message. import ( "encoding/json" + "sort" "strings" "time" @@ -48,10 +53,18 @@ const ( MostActions = 4 // TokenFor is how long a Details ticket is good for. TokenFor = 24 * time.Hour + // MostDetails bounds the Details tickets one record keeps on one channel kind: one per message about it + // still shown there; beyond it the oldest is forgotten (issue 371). + MostDetails = 8 // KeptEvents is how many answers are remembered, so a redelivered one is acted on once. KeptEvents = 200 ) +// NoOpenMessage is the refusal of a ticket no open message holds: its problem ended, or so many newer +// messages about it were sent that this older one's Details was forgotten. +const NoOpenMessage = "it belongs to no open message; the problem may have ended, or this message is older " + + "than the last few about it: tap Details on the newest one" + func sameActions(a, b []Action) bool { x, _ := json.Marshal(a) y, _ := json.Marshal(b) @@ -65,7 +78,9 @@ func needsNothing(r *Record) bool { } // choices are the answers a record's next message offers on one kind: the open ask's options offered -// there, and Details for a condition, with a fresh ticket replacing that kind's earlier one. +// there, and Details for a condition, with a ticket of its own. The tickets of the record's earlier messages +// stand (issue 371): each message keeps a Details of its own, used once, until it is TokenFor old or +// MostDetails newer ones on that kind push it out. func (h *Holder) choices(r *Record, kind string) []Choice { var out []Choice if rec := h.openAsk(r); rec != nil { @@ -80,16 +95,38 @@ func (h *Holder) choices(r *Record, kind string) []Choice { if r.Tokens == nil { r.Tokens = map[string]Grant{} } - for t, g := range r.Tokens { - if g.Kind == kind { - delete(r.Tokens, t) - } - } + pruneGrants(r.Tokens, kind, now, MostDetails-1) t := ticket() r.Tokens[t] = Grant{Index: -1, Kind: kind, Issued: now} return append(out, Choice{Token: t, Label: DetailsLabel}) } +// pruneGrants forgets every ticket older than TokenFor, and keeps at most keep of kind's, the newest. +func pruneGrants(grants map[string]Grant, kind string, now time.Time, keep int) { + var mine []string + for t, g := range grants { + switch { + case now.Sub(g.Issued) > TokenFor: + delete(grants, t) + case g.Kind == kind: + mine = append(mine, t) + } + } + if len(mine) <= keep { + return + } + sort.Slice(mine, func(i, j int) bool { + a, b := grants[mine[i]], grants[mine[j]] + if !a.Issued.Equal(b.Issued) { + return a.Issued.Before(b.Issued) + } + return mine[i] < mine[j] + }) + for _, t := range mine[:len(mine)-keep] { + delete(grants, t) + } +} + // Chosen takes an answer a channel of a kind said it was given: an ask's option is judged and, when it // stands, closes the ask with a warrant for its asker (asks.go); Details are shown where they were asked. // Anything else is refused, and said there. The same answer is acted on once. @@ -139,7 +176,7 @@ func (h *Holder) Chosen(kind string, c asks.Choice) (string, error) { } switch { case r == nil: - return refuse("it belongs to no open message; the problem may have ended, or a newer message replaced it") + return refuse(NoOpenMessage) case !r.Cleared.IsZero(): return refuse("the problem it answered has ended") case r.silenced(now): diff --git a/modules/messenger/cmd/messenger/details_test.go b/modules/messenger/cmd/messenger/details_test.go new file mode 100644 index 00000000..8dba0d35 --- /dev/null +++ b/modules/messenger/cmd/messenger/details_test.go @@ -0,0 +1,166 @@ +package main + +import ( + "fmt" + "strings" + "testing" + "time" +) + +// Issue 371: tapping Details on any but the newest message about a problem was refused "it belongs to no +// open message", because each new message replaced the ticket of the one before it. Every message still +// shown keeps a Details of its own, used once, and Details never acts. + +func detailsOf(t *testing.T, m Message) string { + t.Helper() + tok := ticketFor(m, DetailsLabel) + if tok == "" { + t.Fatalf("no Details on %q: %+v", m.Title, m.Actions) + } + return tok +} + +// The acceptance of issue 371: three warnings in a row, Details works on each of them, once each. +func TestDetailsWorksOnEachOfThreeWarningsOnceEach(t *testing.T) { + for _, same := range []bool{true, false} { + t.Run(fmt.Sprintf("same problem %v", same), func(t *testing.T) { + h, tg, _, c, _ := newHolder(t) + h.Desk = nil + var toks []string + if same { + // One problem said three times: raised, escalated, and reminded. + k := cond("machine.novox.silent", Warning, "the anchor is silent") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + k.Severity = Urgent + h.Condition(EventChanged, k) + settle(h, c) + c.pass(RemindUrgent + time.Minute) + h.Tick() + settle(h, c) + } else { + for _, key := range []string{"machine.ace.silent", "machine.shanks.units", "plan.41.stalled"} { + k := cond(key, Warning, key+" needs a look") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + } + } + if len(tg.sends) != 3 { + t.Fatalf("%d messages, want 3", len(tg.sends)) + } + for _, m := range tg.sends { + toks = append(toks, detailsOf(t, m)) + } + if toks[0] == toks[1] || toks[1] == toks[2] || toks[0] == toks[2] { + t.Fatalf("a ticket is shared: %v", toks) + } + // The oldest first, as the operator scrolls back. + for i, tok := range toks { + before := len(tg.sends) + out, err := h.Chosen("telegram", choiceOf(tok, "42", c.now())) + if err != nil || out != "shown" || len(tg.sends) != before+1 || + !strings.HasPrefix(tg.sends[len(tg.sends)-1].Title, "Details: ") { + t.Fatalf("message %d: Details %q %v", i+1, out, err) + } + } + // Once each: a second tap (a new answer, not a redelivery) is refused in words, and shows nothing. + for i, tok := range toks { + c2 := choiceOf(tok, "42", c.now()) + c2.ID += "-again" + before := len(tg.sends) + out, err := h.Chosen("telegram", c2) + if err == nil || !strings.Contains(err.Error(), "already chosen once") || + strings.HasPrefix(tg.sends[len(tg.sends)-1].Title, "Details: ") || len(tg.sends) > before+1 { + t.Errorf("message %d: replayed Details %q %v", i+1, out, err) + } + } + }) + } +} + +// Tickets are bounded: at most MostDetails per record and kind, none older than TokenFor; one pushed out +// is refused in words that send the operator to the newest message. +func TestDetailsTicketsAreBoundedInNumberAndTime(t *testing.T) { + h, tg, _, c, _ := newHolder(t) + h.Desk = nil + k := cond("machine.ace.silent", Warning, "the home server is silent") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + r := h.open[k.Key] + first := detailsOf(t, tg.sends[0]) + var toks []string + for i := 0; i < MostDetails; i++ { + c.pass(time.Second) + toks = append(toks, ticketFor(Message{Actions: h.choices(r, "telegram")}, DetailsLabel)) + } + if n := len(r.Tokens); n != MostDetails { + t.Fatalf("%d tickets kept, want %d", n, MostDetails) + } + if _, err := h.Chosen("telegram", choiceOf(first, "42", c.now())); err == nil || + !strings.Contains(err.Error(), "tap Details on the newest one") { + t.Errorf("the pushed-out ticket: %v", err) + } + if out, err := h.Chosen("telegram", choiceOf(toks[0], "42", c.now())); err != nil || out != "shown" { + t.Errorf("the oldest kept ticket: %q %v", out, err) + } + // A day on, the rest are refused as old, and forgotten at the next message. + c.pass(TokenFor + time.Minute) + if _, err := h.Chosen("telegram", choiceOf(toks[1], "42", c.now())); err == nil || + !strings.Contains(err.Error(), "more than a day old") { + t.Errorf("an expired ticket: %v", err) + } + h.choices(r, "desktop") + if n := len(r.Tokens); n != 1 { + t.Errorf("%d tickets kept after a day, want the one just made", n) + } +} + +// A change of the condition's actions leaves every shown message its Details: Details names no action. +func TestDetailsOutlivesAChangeOfTheActions(t *testing.T) { + h, tg, _, c, _ := newHolder(t) + h.Desk = nil + k := cond("machine.ace.silent", Warning, "the home server is silent") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + tok := detailsOf(t, tg.sends[0]) + k.Actions = []Action{{Label: "Restart", Verb: "node-service-manager.restart"}} + h.Condition(EventChanged, k) + if out, err := h.Chosen("telegram", choiceOf(tok, "42", c.now())); err != nil || out != "shown" { + t.Errorf("Details after the actions changed: %q %v", out, err) + } +} + +// Details is never an approval: chosen on a message whose ask is open, it shows and performs nothing, and +// the ask stays open with no warrant; an older message's Details does the same. +func TestDetailsOnAMessageWithAnOpenAskNeverAnswersIt(t *testing.T) { + h, _, dt, c, store, w := newRouter(t) + link(h, store, c, "42", c.now().Add(-2*time.Hour)) + k := cond("delivery.hq.held", Warning, "the delivery of hq is held") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + older := detailsOf(t, dt.sends[0]) + a := ackAsk(c, "d-1") + a.About = "delivery.hq.held" + submit(t, h, "mesh-controller", a) + settle(h, c) + var shown Message + for _, m := range dt.edits { + shown = m + } + for _, tok := range []string{older, detailsOf(t, shown)} { + if out, err := h.Chosen("desktop", choiceOf(tok, "g14", c.now())); err != nil || out != "shown" { + t.Fatalf("Details: %q %v", out, err) + } + } + if got := w.of("mesh-controller"); len(got) != 0 { + t.Errorf("Details issued a warrant: %+v", got) + } + if st := store.asks["mesh-controller.d-1"].State; st != StateOpen { + t.Errorf("Details ended the ask: %s", st) + } +} diff --git a/modules/messenger/cmd/messenger/holder.go b/modules/messenger/cmd/messenger/holder.go index 9351544b..f72279b3 100644 --- a/modules/messenger/cmd/messenger/holder.go +++ b/modules/messenger/cmd/messenger/holder.go @@ -186,7 +186,14 @@ func recordOf(c Condition) Record { func (r *Record) takeWords(from Record) { r.Headline, r.Explanation, r.Resolved, r.Needs = from.Headline, from.Explanation, from.Resolved, from.Needs if !sameActions(r.Actions, from.Actions) { - r.Actions, r.Tokens = from.Actions, nil + // Details performs nothing and names no action, so its tickets outlive a change of the actions: + // every message still shown keeps its Details (issue 371). Any other ticket dies with its action. + r.Actions = from.Actions + for t, g := range r.Tokens { + if g.Index != -1 { + delete(r.Tokens, t) + } + } } } From 6a8c718f440cda2e6c86deec821c60e0b135ea02 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 13:46:34 +0200 Subject: [PATCH 2/2] messenger: ask whether a channel is ready before composing its messages, so failed tries make no Details tickets (issue 371) The review found that each try at an unready channel composed its held messages and so made a Details ticket for each, and enough tries pushed out the tickets of messages already shown. --- .../messenger/cmd/messenger/details_test.go | 33 +++++++++++++++++++ modules/messenger/cmd/messenger/holder.go | 2 +- modules/messenger/cmd/messenger/outbox.go | 14 ++++---- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/modules/messenger/cmd/messenger/details_test.go b/modules/messenger/cmd/messenger/details_test.go index 8dba0d35..def1cb06 100644 --- a/modules/messenger/cmd/messenger/details_test.go +++ b/modules/messenger/cmd/messenger/details_test.go @@ -164,3 +164,36 @@ func TestDetailsOnAMessageWithAnOpenAskNeverAnswersIt(t *testing.T) { t.Errorf("Details ended the ask: %s", st) } } + +// A channel that stays unready makes no ticket at its tries: composing a message makes its Details ticket, +// and tickets made at every try would push out those of the messages already shown. +func TestAnUnreadyChannelPushesOutNoShownDetails(t *testing.T) { + h, tg, _, c, _ := newHolder(t) + h.Desk = nil + k := cond("machine.ace.silent", Warning, "the home server is silent") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + tok := detailsOf(t, tg.sends[0]) + k.Severity = Urgent + h.Condition(EventChanged, k) + tg.notReady = fmt.Errorf("telegram is away") + for i := 0; i < 3*MostDetails; i++ { + c.pass(2 * time.Minute) + h.Flush() + } + if n := len(h.open[k.Key].Tokens); n != 1 { + t.Errorf("%d tickets after the tries, want the one shown", n) + } + tg.notReady = nil + c.pass(2 * time.Minute) + h.Flush() + if len(tg.sends) != 2 { + t.Fatalf("%d messages, want 2", len(tg.sends)) + } + for _, tk := range []string{tok, detailsOf(t, tg.sends[1])} { + if out, err := h.Chosen("telegram", choiceOf(tk, "42", c.now())); err != nil || out != "shown" { + t.Errorf("Details: %q %v", out, err) + } + } +} diff --git a/modules/messenger/cmd/messenger/holder.go b/modules/messenger/cmd/messenger/holder.go index f72279b3..4ef00ac2 100644 --- a/modules/messenger/cmd/messenger/holder.go +++ b/modules/messenger/cmd/messenger/holder.go @@ -142,7 +142,7 @@ type Record struct { Explanation string `json:"explanation,omitempty"` Resolved string `json:"resolved,omitempty"` // Needs is what the operator does, empty for nothing; Actions the answers offered, each by a token - // (Tokens: token -> the action's index, or -1 for the details) a channel hands back when chosen. + // (Tokens: token -> its grant; Index -1 is Details, one per message shown) a channel hands back when chosen. Needs string `json:"needs,omitempty"` Actions []Action `json:"actions,omitempty"` // Kept as "grants": the build before ADR 0258 kept "tokens" as token -> index, which this form cannot diff --git a/modules/messenger/cmd/messenger/outbox.go b/modules/messenger/cmd/messenger/outbox.go index d3c473a3..1b264e4d 100644 --- a/modules/messenger/cmd/messenger/outbox.go +++ b/modules/messenger/cmd/messenger/outbox.go @@ -243,6 +243,14 @@ func (h *Holder) flushChannel(ch Channel, now time.Time, force bool) { if len(list) == 0 || (!force && !h.due(ch, list, now)) { return } + taken := len(list) + // Asked before the messages are composed: composing one makes its Details ticket, and a channel that + // stays unready would make one at every try, pushing out the tickets of messages already shown (issue 371). + if err := ch.Ready(); err != nil { + h.noteChannel(name, err) + h.failed(ch, list, taken, now, err) + return + } var entries []entry for _, it := range list { r, m := h.resolve(it, now, name) @@ -251,16 +259,10 @@ func (h *Holder) flushChannel(ch Channel, now time.Time, force bool) { } entries = append(entries, entry{it, r, *m}) } - taken := len(list) if len(entries) == 0 { h.drop(name, taken) return } - if err := ch.Ready(); err != nil { - h.noteChannel(name, err) - h.failed(ch, list, taken, now, err) - return - } if !h.allow(name) { // The last line of defence: said once, in a message of its own, and the rest held. h.mu.Lock()