From f067fab9db3b624f871bf301a474b11b2051599f Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 13:31:51 +0200 Subject: [PATCH] mesh-issues: end a fix-stuck warning when its issue is resolved, and never say a closed issue's fix stuck (issue 547) A resolved issue's worker proved the acceptance live, so a fix whose own walk failed and that a later merge carried is not waited for. Only wontfix ended the warning, so 16 warnings stayed open on the operator's channel and the sweep raised new ones for resolved issues. --- modules/mesh-issues/README.md | 9 +- .../mesh-issues/cmd/mesh-issues/delivery.go | 35 ++++-- .../cmd/mesh-issues/delivery_test.go | 103 ++++++++++++++++-- 3 files changed, 125 insertions(+), 22 deletions(-) diff --git a/modules/mesh-issues/README.md b/modules/mesh-issues/README.md index 7b36ef7a..11628ff1 100644 --- a/modules/mesh-issues/README.md +++ b/modules/mesh-issues/README.md @@ -134,9 +134,12 @@ carries `abandoned` on `get` and its `list` row, beside its `worker`. A move by `take-over`'s reason, as any stale worker; the answer, the note and the `moved` event say it was abandoned (`abandoned`). -Every five minutes the module looks for a fix merged and not delivered after `stuck-after` (default `2h`): it is -said once — a note on the issue and a warning on the operator's channel (`seat:operator-channel.notify`, key -`issue-tracker.fix-stuck.`), cleared there when every fix of the issue is delivered. A module that runs only +Every five minutes the module looks for a fix merged and not delivered after `stuck-after` (default `2h`) of an +issue not closed: it is said once — a note on the issue and a warning on the operator's channel +(`seat:operator-channel.notify`, key `issue-tracker.fix-stuck.`), cleared there when every fix of the issue +is delivered or the issue is closed. Resolved is the end of the warning as wontfix is: its worker proved the +acceptance live, so a fix whose own walk failed and that a later merge carried is not waited for (hq issue 547). An +issue resolved or wontfix is never said stuck. A module that runs only its bundle cannot raise a controller condition (hq ADR 0240 rule 8, ADR 0256), so the warning is the channel's, not the controller's. ## The import of hq's issues (slice 3) diff --git a/modules/mesh-issues/cmd/mesh-issues/delivery.go b/modules/mesh-issues/cmd/mesh-issues/delivery.go index 45e6bfa4..443c9b2e 100644 --- a/modules/mesh-issues/cmd/mesh-issues/delivery.go +++ b/modules/mesh-issues/cmd/mesh-issues/delivery.go @@ -266,9 +266,11 @@ func (t *Tracker) afterFix(n uint64) error { } // stillStuck says whether an issue has a fix said stuck that is still named, not delivered, and the issue not -// closed as wontfix. +// closed. A closed issue is the end of its warning: resolved means its worker proved the acceptance live, so a fix +// the tracker never heard delivered is not waited for any more (a failed walk that a later merge carried stays +// failed, novox/hq issue 547), and wontfix waits for nothing. func stillStuck(is *Issue) bool { - if is.Status == StatusWontfix { + if closedStatus(is.Status) { return false } named := liveFixes(is) @@ -281,19 +283,30 @@ func stillStuck(is *Issue) bool { } // clearStuck clears the warning said for an issue on the operator's channel once nothing it was said for is -// stuck — delivered, no longer named, or the issue closed as wontfix; one that fails is tried again at the next -// sweep. +// stuck — delivered, no longer named, or the issue closed (resolved or wontfix); one that fails is tried again at +// the next sweep. func (t *Tracker) clearStuck(is *Issue) error { if !is.StuckOpen || stillStuck(is) || t.Notify == nil { return nil } - if err := t.Notify(stuckKey(is.Number), "", "", "", "", "the fix of issue "+keyOf(is.Number)+" is delivered or no longer waited for", true); err != nil { + if err := t.Notify(stuckKey(is.Number), "", "", "", "", stuckEnded(is), true); err != nil { return err } _, err := t.change(is.Number, Act{Op: "warned", By: Seat}, func(i *Issue) error { i.StuckOpen = false; return nil }) return err } +// stuckEnded is what the operator's channel says when a stuck warning ends. +func stuckEnded(is *Issue) string { + switch is.Status { + case StatusResolved: + return "resolved: issue " + keyOf(is.Number) + " was proven live by its worker" + case StatusWontfix: + return "issue " + keyOf(is.Number) + " was closed as wontfix: its fix is no longer waited for" + } + return "the fix of issue " + keyOf(is.Number) + " is delivered or no longer waited for" +} + func stuckKey(n uint64) string { return "issue-tracker.fix-stuck." + keyOf(n) } func prKey(repository string, number int) string { @@ -453,7 +466,7 @@ func (t *Tracker) stuckAfter() time.Duration { return DefaultStuckAfter } -// Sweep says every fix merged and not delivered past the bound: a note on its issue and a warning on the +// Sweep says every fix of an issue not closed merged and not delivered past the bound: a note on its issue and a warning on the // operator's channel, once; and every issue in fixing whose fixes were all delivered past its own bound and is not // resolved (novox/hq issue 483). First it asks the delivery of a few fixes never heard of. It answers how many // warnings it said. Run every few minutes. @@ -474,15 +487,13 @@ func (t *Tracker) Sweep() (int, error) { } else if ok { said++ } - if is.Status == StatusWontfix { - if err := t.clearStuck(is); err != nil { - t.Logf("[mesh-issues] the stuck fix of %d could not be cleared on the operator's channel: %v", n, err) - } - continue - } if err := t.clearStuck(is); err != nil { t.Logf("[mesh-issues] the stuck fix of %d could not be cleared on the operator's channel: %v", n, err) } + if closedStatus(is.Status) { + // A resolved or wontfix issue waits for no fix: never said stuck (novox/hq issue 547). + continue + } named := liveFixes(is) for _, f := range is.Fixes { if !contains(named, f.PullRequest) || contains(is.Delivered, f.PullRequest) || !f.Merged || f.MergedAt == nil || diff --git a/modules/mesh-issues/cmd/mesh-issues/delivery_test.go b/modules/mesh-issues/cmd/mesh-issues/delivery_test.go index b5c07800..5ad2ec44 100644 --- a/modules/mesh-issues/cmd/mesh-issues/delivery_test.go +++ b/modules/mesh-issues/cmd/mesh-issues/delivery_test.go @@ -11,10 +11,22 @@ import ( ) type notified struct { - mu sync.Mutex - said []string - clear []string - err error + mu sync.Mutex + said []string + clear []string + resolved []string + err error +} + +// fixedAtFixing names a fix while the issue stays at fixing, as an agent does when the fix merges (ADR 0281 §3, +// ADR 0284): a fix stuck is said only for an issue not closed (issue 547). +func fixedAtFixing(t *testing.T, h *harness, n uint64, fixedBy ...string) { + t.Helper() + h.fixing(t, n) + if _, err := h.Move(MoveAsk{Number: n, Status: StatusFixing, By: "fix@g14", LocatedIn: []string{"mesh-catalog modules/records"}, + FixedBy: fixedBy}); err != nil { + t.Fatal(err) + } } func (n *notified) notify(key, severity, summary, headline, explanation, resolved string, clear bool) error { @@ -25,6 +37,7 @@ func (n *notified) notify(key, severity, summary, headline, explanation, resolve } if clear { n.clear = append(n.clear, key) + n.resolved = append(n.resolved, resolved) } else { n.said = append(n.said, key+" "+severity+" "+summary) } @@ -121,7 +134,7 @@ func TestAnIssueKnowsWhetherItsFixIsDelivered(t *testing.T) { func TestAStuckFixIsSaidOnceAndClearedWhenDelivered(t *testing.T) { h := newHarness(t) n := h.open(t, "the counter skips a number") - resolve(t, h, n, "mesh-catalog#9") + fixedAtFixing(t, h, n, "mesh-catalog#9") note := ¬ified{} h.Notify = note.notify merged := time.Date(2026, 10, 10, 9, 0, 0, 0, time.UTC) // three hours before the harness's clock @@ -152,7 +165,7 @@ func TestAStuckFixIsSaidOnceAndClearedWhenDelivered(t *testing.T) { // Under the bound nothing is said. h2 := newHarness(t) m := h2.open(t, "y") - resolve(t, h2, m, "mesh-catalog#9") + fixedAtFixing(t, h2, m, "mesh-catalog#9") n2 := ¬ified{} h2.Notify = n2.notify transition(t, h2, 9, "delivering", "", "abc", time.Date(2026, 10, 10, 11, 30, 0, 0, time.UTC)) @@ -194,7 +207,8 @@ func TestAFixIsNamedWhileTheIssueStaysLocated(t *testing.T) { func TestDeliveredStaysDeliveredAndClearsAreRetried(t *testing.T) { h := newHarness(t) n := h.open(t, "x") - resolve(t, h, n, "mesh-catalog#9") + fixedAtFixing(t, h, n, "mesh-catalog#9") + h.settings.DeliveredNotResolvedAfter = "24h" // only the stuck warning is under test here note := ¬ified{} h.Notify = note.notify merged := time.Date(2026, 10, 10, 9, 0, 0, 0, time.UTC) @@ -244,3 +258,78 @@ func TestDeliveredStaysDeliveredAndClearsAreRetried(t *testing.T) { t.Fatalf("%d asks, %q", asks, v.Delivery) } } + +// A resolved issue is the end of its stuck warning, as wontfix is: resolving clears it on the operator's channel, +// saying so, and the sweep never says a fix stuck for an issue resolved or wontfix, even one whose delivery failed +// and never reads delivered (issue 547). +func TestAResolvedIssueEndsItsStuckWarningAndIsNeverSaidStuck(t *testing.T) { + merged := time.Date(2026, 10, 10, 9, 0, 0, 0, time.UTC) // three hours before the harness's clock + moved := func(t *testing.T, h *harness, n uint64) { + t.Helper() + raw, _ := json.Marshal(map[string]any{"number": n}) + if err := h.OnMoved(stdio.Envelope{Body: raw}); err != nil { + t.Fatal(err) + } + } + for _, to := range []string{StatusResolved, StatusWontfix} { + t.Run(to, func(t *testing.T) { + h := newHarness(t) + n := h.open(t, "the counter skips a number") + fixedAtFixing(t, h, n, "mesh-catalog#9") + note := ¬ified{} + h.Notify = note.notify + transition(t, h, 9, "failed", "the gate failed on one machine", "abc", merged) + if said, err := h.Sweep(); err != nil || said != 1 || len(note.said) != 1 { + t.Fatalf("not said stuck: %d %v %q", said, err, note.said) + } + ask := MoveAsk{Number: n, Status: to, FixedBy: []string{"mesh-catalog#9"}, ReplayNone: "proven live"} + if to == StatusWontfix { + ask = MoveAsk{Number: n, Status: to, Reason: "not worth it"} + } + if _, err := h.Move(ask); err != nil { + t.Fatal(err) + } + moved(t, h, n) + if len(note.clear) != 1 || note.clear[0] != stuckKey(n) || get(t, h, n).StuckOpen { + t.Fatalf("not cleared at the move: %q", note.clear) + } + if to == StatusResolved && !strings.HasPrefix(note.resolved[0], "resolved: ") { + t.Fatalf("%q", note.resolved) + } + if said, _ := h.Sweep(); said != 0 || len(note.said) != 1 || len(note.clear) != 1 { + t.Fatalf("said again after %s: %d %q, cleared %q", to, said, note.said, note.clear) + } + }) + } + // A warning left open by a tracker that kept it for a resolved issue is cleared at the first sweep, and an + // issue resolved before its fix was ever said stuck is never said stuck. + h := newHarness(t) + n := h.open(t, "x") + fixedAtFixing(t, h, n, "mesh-catalog#9") + note := ¬ified{err: errStuck} + h.Notify = note.notify + transition(t, h, 9, "failed", "the gate failed", "abc", merged) + if _, err := h.change(n, Act{Op: "warned", By: Seat}, func(i *Issue) error { + i.Fixes[0].StuckSaid, i.StuckOpen = true, true + return nil + }); err != nil { + t.Fatal(err) + } + if _, err := h.Move(MoveAsk{Number: n, Status: StatusResolved, FixedBy: []string{"mesh-catalog#9"}}); err != nil { + t.Fatal(err) + } + moved(t, h, n) // the channel does not answer: kept open, tried at the sweep + note.err = nil + if said, _ := h.Sweep(); said != 0 || len(note.clear) != 1 || get(t, h, n).StuckOpen { + t.Fatalf("said %d, cleared %q", said, note.clear) + } + m := h.open(t, "y") + fixedAtFixing(t, h, m, "mesh-catalog#10") + transition(t, h, 10, "failed", "the gate failed", "abc", merged) + if _, err := h.Move(MoveAsk{Number: m, Status: StatusResolved, FixedBy: []string{"mesh-catalog#10"}}); err != nil { + t.Fatal(err) + } + if said, _ := h.Sweep(); said != 0 || len(note.said) != 0 { + t.Fatalf("a resolved issue said stuck: %q", note.said) + } +}