diff --git a/modules/mesh-issues/README.md b/modules/mesh-issues/README.md index 7ab47ab4..433efa0e 100644 --- a/modules/mesh-issues/README.md +++ b/modules/mesh-issues/README.md @@ -97,7 +97,7 @@ nothing. 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 -its bundle cannot raise a controller condition (hq ADR 0256), so the warning is the channel's, not the controller's. +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 c927ca85..579a7546 100644 --- a/modules/mesh-issues/cmd/mesh-issues/delivery.go +++ b/modules/mesh-issues/cmd/mesh-issues/delivery.go @@ -1,6 +1,6 @@ package main -// Whether an issue's fix is delivered (novox/hq ADR 0280). The tracker hears mesh-delivery's `transition` event +// Whether an issue's fix is delivered (novox/hq ADR 0281). The tracker hears mesh-delivery's `transition` event // for every pull request an issue's fixed-by names and keeps, per pull request, the state the delivery last said // — merged or not, held and why, covered by a later merge, delivered. From that it derives one line for `get` and // `list`: a measured fact, never set by a verb. Delivered is not resolved: when every fix of an issue not yet @@ -131,7 +131,14 @@ func (t *Tracker) recordFix(pr, state, why string, merged bool, at time.Time, on if (only != 0 && n != only) || !contains(pullRequests(is), pr) { continue } - if f := fixOf(is, pr); f != nil && f.State == state && f.Why == why && f.Merged == (merged || f.Merged) { + kept := cut(why, MostReasonKept) + if f := fixOf(is, pr); f != nil && (at.Before(f.Since) || (f.State == state && f.Why == kept && f.Merged == (merged || f.Merged))) { + // Older than what is kept, or nothing new: transitions can arrive out of order. + continue + } + if contains(is.Delivered, pr) && !reached(state) { + // Delivered stays delivered: a later state that has not reached the machines is a newer delivery of + // the same pull request at most, and an older one is history. continue } if _, err := t.change(n, func(i *Issue) error { @@ -140,7 +147,7 @@ func (t *Tracker) recordFix(pr, state, why string, merged bool, at time.Time, on i.Fixes = append(i.Fixes, Fix{PullRequest: pr}) f = &i.Fixes[len(i.Fixes)-1] } - f.State, f.Why, f.Since = state, cut(why, MostReasonKept), at + f.State, f.Why, f.Since = state, kept, at if merged && !f.Merged { f.Merged = true m := at @@ -167,43 +174,72 @@ func (t *Tracker) afterFix(n uint64) error { if err != nil { return err } + 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 (tried again at the next sweep): %v", n, err) + } if allDelivered(is) { - for _, f := range is.Fixes { - if f.StuckSaid && t.Notify != nil { - if err := t.Notify(stuckKey(n), "", "", "", "", "the fix of issue "+keyOf(n)+" is delivered", true); err != nil { - t.Logf("[mesh-issues] the stuck fix of %d could not be cleared on the operator's channel: %v", n, err) - } - break - } - } if unresolved(is) { prs := pullRequests(is) sort.Strings(prs) - claimed := false - if _, err := t.change(n, func(i *Issue) error { - claimed = false - if strings.Join(i.VerifyFor, ",") == strings.Join(prs, ",") { - return nil + if strings.Join(is.VerifyFor, ",") != strings.Join(prs, ",") { + // The note first, kept once by its words; then the claim; then the event. A failure before the claim + // is tried again at the next offer, and the note is not written twice. + prefix := fmt.Sprintf("Ready to verify: every fix of this issue is delivered (%s).", strings.Join(prs, ", ")) + if !noted(is, prefix) { + if _, err := t.note(GroomBy, n, prefix+" Delivered is not resolved: prove the acceptance live, then "+ + "resolve it. (novox/hq ADR 0281)"); err != nil { + return err + } } - i.VerifyFor, claimed = prs, true - return nil - }); err != nil { - return err - } - if claimed { - // At most once, as `ready`: a note that fails after the claim is not said again. - if _, err := t.note(GroomBy, n, fmt.Sprintf("Ready to verify: every fix of this issue is delivered (%s). "+ - "Delivered is not resolved: prove the acceptance live, then resolve it. (novox/hq ADR 0280)", - strings.Join(prs, ", "))); err != nil { + claimed := false + if _, err := t.change(n, func(i *Issue) error { + claimed = false + if strings.Join(i.VerifyFor, ",") == strings.Join(prs, ",") { + return nil + } + i.VerifyFor, claimed = prs, true + return nil + }); err != nil { return err } - t.emit(EventReadyToVerify, map[string]any{"number": n, "fixed-by": prs, "at": t.now()}) + if claimed { + t.emit(EventReadyToVerify, map[string]any{"number": n, "fixed-by": prs, "at": t.now()}) + } } } } return t.letGo(n) } +// stillStuck says whether an issue has a fix said stuck that is still named, not delivered, and the issue not +// closed as wontfix. +func stillStuck(is *Issue) bool { + if is.Status == StatusWontfix { + return false + } + named := pullRequests(is) + for _, f := range is.Fixes { + if f.StuckSaid && contains(named, f.PullRequest) && !contains(is.Delivered, f.PullRequest) { + return true + } + } + return false +} + +// 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. +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 { + return err + } + _, err := t.change(is.Number, func(i *Issue) error { i.StuckOpen = false; return nil }) + return err +} + func stuckKey(n uint64) string { return "issue-tracker.fix-stuck." + keyOf(n) } func prKey(repository string, number int) string { @@ -310,7 +346,9 @@ func (t *Tracker) OnMoved(e stdio.Envelope) error { } state := d.State if state == "" { - state = "unknown" + // The delivery knows nothing of it yet: not kept, so it is asked again at the next move, and its own + // events say it when they come. + continue } if _, err := t.recordFix(pr, state, d.Why, afterMerge(state), t.now(), body.Number); err != nil { return err @@ -359,10 +397,18 @@ func (t *Tracker) Sweep() (int, error) { said := 0 for n, is := range all { 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) + } + named := pullRequests(is) for _, f := range is.Fixes { - if !f.Merged || f.MergedAt == nil || reached(f.State) || f.StuckSaid || now.Sub(*f.MergedAt) < bound { + if !contains(named, f.PullRequest) || contains(is.Delivered, f.PullRequest) || !f.Merged || f.MergedAt == nil || + reached(f.State) || f.StuckSaid || now.Sub(*f.MergedAt) < bound { continue } state := f.State @@ -372,7 +418,7 @@ func (t *Tracker) Sweep() (int, error) { prefix := fmt.Sprintf("Stuck: the fix %s", f.PullRequest) if !noted(is, prefix) { if _, err := t.note(GroomBy, n, fmt.Sprintf("%s was merged %s ago and is not delivered (%s). A merged fix "+ - "that does not reach the machines is said on the operator's channel. (novox/hq ADR 0280)", prefix, + "that does not reach the machines is said on the operator's channel. (novox/hq ADR 0281)", prefix, now.Sub(*f.MergedAt).Round(time.Minute), state)); err != nil { return said, err } @@ -394,6 +440,7 @@ func (t *Tracker) Sweep() (int, error) { if g := fixOf(i, pr); g != nil { g.StuckSaid = true } + i.StuckOpen = t.Notify != nil return nil }); err != nil { return said, err diff --git a/modules/mesh-issues/cmd/mesh-issues/delivery_test.go b/modules/mesh-issues/cmd/mesh-issues/delivery_test.go index be72c1f2..b5c07800 100644 --- a/modules/mesh-issues/cmd/mesh-issues/delivery_test.go +++ b/modules/mesh-issues/cmd/mesh-issues/delivery_test.go @@ -188,3 +188,59 @@ func TestAFixIsNamedWhileTheIssueStaysLocated(t *testing.T) { t.Fatalf("%+v", is) } } + +// Transitions out of order never take a delivered fix back, and a delivered fix is never said stuck; a clear +// that fails is tried again; a fix no longer named is not swept; an empty answer from the delivery is asked again. +func TestDeliveredStaysDeliveredAndClearsAreRetried(t *testing.T) { + h := newHarness(t) + n := h.open(t, "x") + resolve(t, h, n, "mesh-catalog#9") + note := ¬ified{} + h.Notify = note.notify + merged := time.Date(2026, 10, 10, 9, 0, 0, 0, time.UTC) + transition(t, h, 9, "held", "waits", "abc", merged) + if said, _ := h.Sweep(); said != 1 { + t.Fatal("not said stuck") + } + note.err = errStuck + transition(t, h, 9, "delivered", "", "abc", merged.Add(time.Hour)) + transition(t, h, 9, "held", "an older word", "abc", merged.Add(30*time.Minute)) // out of order + if v, _ := h.View(n); v.Delivery != "delivered to every machine it should reach" { + t.Fatal(v.Delivery) + } + if len(note.clear) != 0 || !get(t, h, n).StuckOpen { + t.Fatal("cleared while the channel failed") + } + 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) + } + // A fix no longer named is not swept, and its warning is cleared. + h2 := newHarness(t) + m := h2.open(t, "y") + if _, err := h2.Move(MoveAsk{Number: m, Status: StatusLocated, LocatedIn: []string{"mesh-catalog modules/records"}, FixedBy: []string{"mesh-catalog#9"}}); err != nil { + t.Fatal(err) + } + n2 := ¬ified{} + h2.Notify = n2.notify + transition(t, h2, 9, "held", "waits", "abc", merged) + _, _ = h2.Sweep() + if _, err := h2.Move(MoveAsk{Number: m, Status: StatusLocated, FixedBy: []string{"mesh-catalog#10"}}); err != nil { + t.Fatal(err) + } + if said, _ := h2.Sweep(); said != 0 || len(n2.said) != 1 || len(n2.clear) != 1 { + t.Fatalf("said %d %q, cleared %q", said, n2.said, n2.clear) + } + if _, err := h2.Move(MoveAsk{Number: m, Status: StatusLocated, FixedBy: []string{"mesh-catalog#10"}}); err == nil { + t.Fatal("a move that changes nothing was taken") + } + // The delivery knows nothing yet: not kept, asked again at the next move. + asks := 0 + h2.Deliveries = func(string, string) (Delivery, error) { asks++; return Delivery{}, nil } + for i := 0; i < 2; i++ { + _ = h2.OnMoved(stdio.Envelope{Body: json.RawMessage(`{"number":365}`)}) + } + if v, _ := h2.View(m); asks != 2 || v.Delivery != "nothing heard from the delivery of mesh-catalog#10" { + t.Fatalf("%d asks, %q", asks, v.Delivery) + } +} diff --git a/modules/mesh-issues/cmd/mesh-issues/issue.go b/modules/mesh-issues/cmd/mesh-issues/issue.go index 2fc14b09..fcaa8b94 100644 --- a/modules/mesh-issues/cmd/mesh-issues/issue.go +++ b/modules/mesh-issues/cmd/mesh-issues/issue.go @@ -91,11 +91,13 @@ type Issue struct { SuggestedOwner string `json:"suggested-owner,omitempty"` // Delivered is the pull requests of FixedBy the delivery said are delivered. Delivered []string `json:"delivered,omitempty"` - // Fixes is each pull request of FixedBy as the delivery last said it (ADR 0280): measured, never set by a + // Fixes is each pull request of FixedBy as the delivery last said it (ADR 0281): measured, never set by a // verb. Fixes []Fix `json:"fixes,omitempty"` // VerifyFor is the pull requests it was last said ready to verify for, so it is said once. VerifyFor []string `json:"verify-for,omitempty"` + // StuckOpen is the warning on the operator's channel said for a stuck fix and not yet cleared there. + StuckOpen bool `json:"stuck-open,omitempty"` // ReadyFor is the issues it depended on when it was last said ready (ADR 0279 §3), so one spell of // readiness is said once however many dependencies let it go at one delivery. ReadyFor []uint64 `json:"ready-for,omitempty"` diff --git a/modules/mesh-issues/cmd/mesh-issues/main.go b/modules/mesh-issues/cmd/mesh-issues/main.go index 14cc9ce5..52ae5600 100644 --- a/modules/mesh-issues/cmd/mesh-issues/main.go +++ b/modules/mesh-issues/cmd/mesh-issues/main.go @@ -41,7 +41,7 @@ type Settings struct { // ForgeOwner is the owner on the forge of the repositories fixed-by names, for asking a pull request's // delivery (ADR 0279 §3). ForgeOwner string `json:"forge-owner,omitempty"` - // StuckAfter is how long a fix may be merged and not delivered before it is said stuck (ADR 0280), as a + // StuckAfter is how long a fix may be merged and not delivered before it is said stuck (ADR 0281), as a // duration such as 2h; DefaultStuckAfter when empty. StuckAfter string `json:"stuck-after,omitempty"` } @@ -122,7 +122,7 @@ func hearOpened(t *Tracker) { } } -// sweep says, every five minutes, every fix merged and not delivered past the bound (ADR 0280). +// sweep says, every five minutes, every fix merged and not delivered past the bound (ADR 0281). func sweep(t *Tracker) { for range time.Tick(5 * time.Minute) { if n, err := t.Sweep(); err != nil { diff --git a/modules/mesh-issues/cmd/mesh-issues/ready.go b/modules/mesh-issues/cmd/mesh-issues/ready.go index b74e593e..152093b4 100644 --- a/modules/mesh-issues/cmd/mesh-issues/ready.go +++ b/modules/mesh-issues/cmd/mesh-issues/ready.go @@ -268,7 +268,7 @@ type View struct { NeededBy []uint64 `json:"needed-by,omitempty"` WaitsFor []uint64 `json:"waits-for,omitempty"` Ready bool `json:"ready"` - // Delivery is whether its fix is delivered, in words (ADR 0280). + // Delivery is whether its fix is delivered, in words (ADR 0281). Delivery string `json:"delivery,omitempty"` } diff --git a/modules/mesh-issues/cmd/mesh-issues/tracker.go b/modules/mesh-issues/cmd/mesh-issues/tracker.go index 649d8afc..d1823798 100644 --- a/modules/mesh-issues/cmd/mesh-issues/tracker.go +++ b/modules/mesh-issues/cmd/mesh-issues/tracker.go @@ -30,9 +30,9 @@ type Tracker struct { Records Records // Judge is the triage's way to a model (ADR 0278, ADR 0279); nil judges nothing. Judge Judgement - // Deliveries answers a pull request's delivery (ADR 0279 §3, ADR 0280); nil asks nothing. + // Deliveries answers a pull request's delivery (ADR 0279 §3, ADR 0281); nil asks nothing. Deliveries DeliveryState - // Notify tells the operator's channel of a stuck fix (ADR 0280); nil says nothing there. + // Notify tells the operator's channel of a stuck fix (ADR 0281); nil says nothing there. Notify Notifier groom grooming } @@ -397,9 +397,14 @@ func (t *Tracker) move(by string, a MoveAsk) (*Moved, error) { is, err := t.change(a.Number, func(is *Issue) error { changed = nil from = is.Status + // A field counts as changed only when its value does, so a move that changes nothing is refused. set := func(name string, cond bool, apply func()) { - if cond { - apply() + if !cond { + return + } + before, _ := json.Marshal(is) + apply() + if after, _ := json.Marshal(is); string(before) != string(after) { changed = append(changed, name) } } @@ -626,7 +631,7 @@ type Row struct { Ready bool `json:"ready"` // SuggestedOwner is the triage's or a person's suggestion, for an issue located nowhere yet. SuggestedOwner string `json:"suggested-owner,omitempty"` - // Delivery is whether its fix is delivered, in words (ADR 0280); empty when fixed-by names no pull request. + // Delivery is whether its fix is delivered, in words (ADR 0281); empty when fixed-by names no pull request. Delivery string `json:"delivery,omitempty"` }