mesh-issues: a learned merge is kept as of now, closed untimed issues get no fix time, the forge read is declared, the sweep rotates its asks (issue 1950)

2026-10-27 01:30:37 +01:00
parent b51f03c4ff
commit 3c6f2dc274
8 changed files with 115 additions and 25 deletions
@@ -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
 			}
 		}
@@ -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
@@ -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
@@ -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:
@@ -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 := &notified{}
+	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))
+	}
+}
@@ -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).
@@ -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)
@@ -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",