diff --git a/modules/mesh-issues/cmd/mesh-issues/claims.go b/modules/mesh-issues/cmd/mesh-issues/claims.go index d11f1942..a3847f97 100644 --- a/modules/mesh-issues/cmd/mesh-issues/claims.go +++ b/modules/mesh-issues/cmd/mesh-issues/claims.go @@ -121,31 +121,52 @@ func (t *Tracker) askUnheard() error { return err } // Issues not resolved first, then the others: a resolved issue's fix merged before the tracker heard deliveries is learned - // too, so its delivery line stops saying nothing was heard (issue 1950). The asks are bounded; the rest wait for the next sweep. - numbers := make([]uint64, 0, len(all)) - for n := range all { - numbers = append(numbers, n) + // too, so its delivery line stops saying nothing was heard (issue 1950). The asks are bounded, and the pull request asked + // longest ago goes first, so ones that cannot be learned (still open, gone) never starve the rest. + type cand struct { + n uint64 + pr string + open bool + at time.Time } - sort.Slice(numbers, func(i, j int) bool { - a, b := unresolved(all[numbers[i]]), unresolved(all[numbers[j]]) - if a != b { - return a + var cs []cand + t.askedMu.Lock() + for n, is := range all { + for _, pr := range unheardOf(is) { + cs = append(cs, cand{n, pr, unresolved(is), t.askedAt[pr]}) } - return numbers[i] < numbers[j] + } + t.askedMu.Unlock() + sort.Slice(cs, func(i, j int) bool { + a, b := cs[i], cs[j] + if a.open != b.open { + return a.open + } + if !a.at.Equal(b.at) { + return a.at.Before(b.at) + } + if a.n != b.n { + return a.n < b.n + } + return a.pr < b.pr }) - left := MostSweepAsks - for _, n := range numbers { - if left == 0 { - return nil + for i, c := range cs { + if i == MostSweepAsks { + break } - is := all[n] - asked, kept, err := t.learnUnheard(n, is, left) - left -= asked + t.askedMu.Lock() + if t.askedAt == nil { + t.askedAt = map[string]time.Time{} + } + t.askedAt[c.pr] = t.now() + t.askedMu.Unlock() + kept, err := t.learnFix(c.n, c.pr) if err != nil { - return err + t.Logf("[mesh-issues] the delivery of %s (issue %d) could not be asked (the next sweep asks the others first): %v", c.pr, c.n, err) + continue } if kept { - if err := t.afterFix(n); err != nil { + if err := t.afterFix(c.n); err != nil { return err } } diff --git a/modules/mesh-issues/cmd/mesh-issues/delivery.go b/modules/mesh-issues/cmd/mesh-issues/delivery.go index 7e64daff..fd461b2a 100644 --- a/modules/mesh-issues/cmd/mesh-issues/delivery.go +++ b/modules/mesh-issues/cmd/mesh-issues/delivery.go @@ -791,7 +791,7 @@ func (t *Tracker) Sweep() (int, error) { named := liveFixes(is) for _, f := range is.Fixes { 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 { + reached(f.State) || f.StuckSaid || now.Sub(*f.MergedAt) < bound || now.Sub(f.Since) < bound { continue } state := f.State diff --git a/modules/mesh-issues/cmd/mesh-issues/fixtime.go b/modules/mesh-issues/cmd/mesh-issues/fixtime.go index fd078578..9d6f5c96 100644 --- a/modules/mesh-issues/cmd/mesh-issues/fixtime.go +++ b/modules/mesh-issues/cmd/mesh-issues/fixtime.go @@ -320,6 +320,11 @@ func (t *Tracker) timeFix(n uint64) error { if err != nil { return err } + if closedStatus(is.Status) && is.FixTime == nil { + // An issue closed before its fix was timed keeps none: measuring now would end its fix time at a delivery learned + // late ("fixed in N weeks") and could raise a false over-budget warning (issue 1950). + return nil + } ft, _ := t.fixTimeOf(is) if ft == nil || sameFixTime(ft, is.FixTime) { return nil diff --git a/modules/mesh-issues/cmd/mesh-issues/learn.go b/modules/mesh-issues/cmd/mesh-issues/learn.go index 6f47c4bf..f53f1765 100644 --- a/modules/mesh-issues/cmd/mesh-issues/learn.go +++ b/modules/mesh-issues/cmd/mesh-issues/learn.go @@ -111,11 +111,9 @@ func (t *Tracker) learnFix(n uint64, pr string) (bool, error) { var h heardFix switch { case d.Merged: - at := t.now() - if d.MergedAt != nil { - at = *d.MergedAt - } - h = heardFix{State: "merged", Merged: true, At: at, MergedAt: d.MergedAt, Repository: d.Repository, MergedAs: d.MergedAs} + // Kept as of now, the moment it was learned: the fix's state did not change days ago for the tracker, and the stuck + // sweep measures from it. MergedAt stays the forge's own moment. + h = heardFix{State: "merged", Merged: true, At: t.now(), MergedAt: d.MergedAt, Repository: d.Repository, MergedAs: d.MergedAs} case d.Closed: h = heardFix{State: "closed", Why: "closed without merging", At: t.now()} default: diff --git a/modules/mesh-issues/cmd/mesh-issues/learn_test.go b/modules/mesh-issues/cmd/mesh-issues/learn_test.go index 7a074f97..b73b27b5 100644 --- a/modules/mesh-issues/cmd/mesh-issues/learn_test.go +++ b/modules/mesh-issues/cmd/mesh-issues/learn_test.go @@ -124,3 +124,65 @@ func TestTheForgesRealPullAnswerIsReadAsMerged(t *testing.T) { t.Fatalf("%+v", d) } } + +// A merge learned late is kept as of the moment it was learned: the stuck bound runs from then, so an old merge raises no +// false warning in the sweep that learned it; and an issue closed before its fix was timed keeps no fix time when a late +// delivery covers it. +func TestALearnedOldMergeRaisesNoStuckWarningAndTimesNoClosedIssue(t *testing.T) { + h := newHarness(t) + now := clocked(h) + note := ¬ified{} + h.Notify = note.notify + old := now.Add(-10 * 24 * time.Hour) + mergedAs := "1325c8b452c8fa65c478af6815887870a1f42e7b" + h.Pull = func(string, string) (Delivery, error) { + return Delivery{Pulled: true, Merged: true, MergedAs: mergedAs, MergedAt: &old, Repository: "novox/mesh-catalog"}, nil + } + h.Contains = func(_, _, commit, _ string) (bool, error) { return commit == mergedAs, nil } + open := fixIssue(t, h, "mesh-catalog#60") + closed := fixIssue(t, h, "mesh-catalog#61") + if _, err := h.Move(MoveAsk{Number: closed, Status: StatusResolved, FixedBy: []string{"mesh-catalog#61"}}); err != nil { + t.Fatal(err) + } + if _, err := h.Sweep(); err != nil { + t.Fatal(err) + } + if len(note.said) != 0 || noted(get(t, h, open), "Stuck: the fix") { + t.Fatalf("a learned old merge was said stuck in the sweep that learned it: %q", note.said) + } + if f := fixOf(get(t, h, open), "mesh-catalog#60"); f == nil || f.MergedAt == nil || !f.MergedAt.Equal(old) { + t.Fatalf("the forge's merge moment is kept: %+v", f) + } + *now = now.Add(3 * time.Hour) + if _, err := h.Sweep(); err != nil || len(note.said) != 1 { + t.Fatalf("not delivered 3 hours after it was learned: %q %v", note.said, err) + } + viewSays(t, h, 40, "", map[string]string{"novox/mesh-catalog": "ffffffffffff0000"}) + if v, _ := h.View(closed); v.Delivery != "delivered to every machine it should reach" || get(t, h, closed).FixTime != nil { + t.Fatalf("closed issue: %q, fix time %+v", v.Delivery, get(t, h, closed).FixTime) + } +} + +// Pull requests that cannot be learned (still open) do not starve the others: the one asked longest ago goes first. +func TestTheSweepAsksTheLeastRecentlyAskedPullRequestFirst(t *testing.T) { + h := newHarness(t) + var stuck []uint64 + for i := 0; i < MostSweepAsks+2; i++ { + stuck = append(stuck, fixIssue(t, h, "mesh-catalog#"+itoa(70+i))) + } + last := fixIssue(t, h, "mesh-catalog#99") + h.Pull = func(_, number string) (Delivery, error) { + if number == "99" { + return Delivery{Pulled: true, Merged: true, MergedAs: "1325c8b452c8fa65c478af6815887870a1f42e7b"}, nil + } + return Delivery{Pulled: true}, nil // still open + } + for i := 0; i < 4; i++ { + if _, err := h.Sweep(); err != nil { + t.Fatal(err) + } + } + if v, _ := h.View(last); !strings.HasPrefix(v.Delivery, "fix merged") { + t.Fatalf("starved by pull requests that cannot be learned: %q (%d of them)", v.Delivery, len(stuck)) + } +} diff --git a/modules/mesh-issues/cmd/mesh-issues/manifest_test.go b/modules/mesh-issues/cmd/mesh-issues/manifest_test.go index fa116e3f..d0619782 100644 --- a/modules/mesh-issues/cmd/mesh-issues/manifest_test.go +++ b/modules/mesh-issues/cmd/mesh-issues/manifest_test.go @@ -112,7 +112,7 @@ func TestItDeclaresAndHoldsTheIssueTrackerSeatOnceForTheMesh(t *testing.T) { // the import and a reservation read the record through the records module (issue 424). if !reflect.DeepEqual(m.Consumes, []string{"mesh-issues." + EventOpened, "mesh-issues." + EventMoved, "mesh-delivery-gate.transition", DeliveredEvent}) || !reflect.DeepEqual(m.Invokes, []string{"claude-code.claude_code_judge", "seat:mesh-delivery.checks", "seat:operator-channel.notify", - "gitea.gitea_pull_reviews", "gitea.gitea_contains", "records.records_status", "records.records_list", "records.records_read", "mesh-knowledge.knowledge_highest"}) { + "gitea.gitea_pull_reviews", "gitea.gitea_contains", "gitea.gitea_get_pull_request", "records.records_status", "records.records_list", "records.records_read", "mesh-knowledge.knowledge_highest"}) { t.Fatalf("consumes %v, invokes %v", m.Consumes, m.Invokes) } // The tools bundle, and the archive the build packs files/ into for the templates (novox/hq ADR 0349 ยง2). diff --git a/modules/mesh-issues/cmd/mesh-issues/tracker.go b/modules/mesh-issues/cmd/mesh-issues/tracker.go index b136571d..c125cd3d 100644 --- a/modules/mesh-issues/cmd/mesh-issues/tracker.go +++ b/modules/mesh-issues/cmd/mesh-issues/tracker.go @@ -39,6 +39,9 @@ type Tracker struct { Deliveries DeliveryState // Pull reads one pull request from the forge (gitea_get_pull_request) for a fix the delivery cannot say anything of; nil asks nothing. Pull DeliveryState + // askedAt is when each pull request without a record was last asked by the sweep, so the least recently asked goes first. + askedMu sync.Mutex + askedAt map[string]time.Time // Contains asks the forge whether a commit is contained in another (gitea_contains); nil asks nothing, and a delivered // revision then marks only the fixes it names (OnDelivered). Contains func(owner, repo, commit, in string) (bool, error) diff --git a/modules/mesh-issues/module.json b/modules/mesh-issues/module.json index 3836c89b..a6c1b9d1 100644 --- a/modules/mesh-issues/module.json +++ b/modules/mesh-issues/module.json @@ -76,6 +76,7 @@ "seat:operator-channel.notify", "gitea.gitea_pull_reviews", "gitea.gitea_contains", + "gitea.gitea_get_pull_request", "records.records_status", "records.records_list", "records.records_read",