Merge pull request 'pacman's mirror list keeps a fallback tail; a failed check leads the merge ask's headline (ADR 0586, issues 1876, 1166)' (#589) from fix/mirror-tail-headline into main

2026-11-01 18:04:05 +00:00
parent 3483f603ae 90f7f87c2a
commit ce3bedd584
7 changed files with 132 additions and 22 deletions
@@ -169,6 +169,16 @@ func approveCaveat(c checkSaid) string {
 	return ""
 }

+// mergeHeadline opens the ask's headline: "Merge into main: ", and "Check failed - merge into main: " once the check failed,
+// since a merge is not gated on the check and the headline is the phone's only warning (issue 1166). A passed or still
+// running check keeps the plain headline. The prefix leads, so phone's cut of a long headline never drops it.
+func mergeHeadline(c checkSaid) string {
+	if c.Key == checkSaidFail {
+		return "Check failed \u2014 merge into main: "
+	}
+	return "Merge into main: "
+}
+
 // composeMergeAsk is the ask for a delivery, or for a group's members together.
 // check is the state of the check at the heads it covers (checkAt): the ask says "passed its check" only when it did, and
 // otherwise that Approve merges before the check ends, or although it failed (issue 1733).
@@ -193,7 +203,7 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery,
 			pullSaid(m), m.Title, shortOf(m.Commit, 12), issuesSaid([]*Delivery{m}), words, touches))
 	}
 	if group == "" {
-		a.Headline = phone("Merge into main: "+pullSaid(d)+" "+d.Title, asks.HeadlineLength)
+		a.Headline = phone(mergeHeadline(check)+pullSaid(d)+" "+d.Title, asks.HeadlineLength)
 		summary := fmt.Sprintf("%s %s%s. Approve merges it into main at %s%s; Decline leaves it unmerged.",
 			pullSaid(d), d.Title, checkClause(check, " "), shortOf(d.Commit, 12), approveCaveat(check))
 		if unreviewed != "" {
@@ -210,7 +220,7 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery,
 		for _, m := range members {
 			names = append(names, pullSaid(m))
 		}
-		a.Headline = phone("Merge into main: group "+group+" ("+strings.Join(names, ", ")+")", asks.HeadlineLength)
+		a.Headline = phone(mergeHeadline(check)+"group "+group+" ("+strings.Join(names, ", ")+")", asks.HeadlineLength)
 		caveat := approveCaveat(check)
 		if caveat != "" {
 			caveat = "," + caveat
@@ -3,6 +3,7 @@ package main
 import (
 	"strings"
 	"testing"
+	"time"

 	"git.novox.be/novox/mesh-sdk/go/asks"
 	sdk "git.novox.be/novox/mesh-sdk/go/delivery"
@@ -42,6 +43,17 @@ func TestAReadyGroupWritesOneAskKeyWithTodaysWordsAndCallsNoRouter(t *testing.T)
 		!strings.Contains(ask.Ask.Details, "mesh-catalog#2") || len(ask.Ask.Options) != 2 || ask.Ask.Who != asks.Operator {
 		t.Fatalf("the ask is not today's merge ask: %+v", ask.Ask)
 	}
+	// A group's headline says a failed check first (issue 1166); a passed or a running one keeps the plain words.
+	members := []*Delivery{{Repository: "novox/mesh-controller", Number: 1}, {Repository: "novox/mesh-catalog", Number: 2}}
+	for _, c := range []struct{ key, head string }{
+		{checkSaidFail, "Check failed \u2014 merge into main: group feat"},
+		{checkSaidPass, "Merge into main: group feat"},
+		{"running", "Merge into main: group feat"},
+	} {
+		if h := composeMergeAsk("id", members[0], "feat/g", members, nil, checkSaid{Key: c.key, Words: "w"}, time.Time{}).Headline; !strings.HasPrefix(h, c.head) {
+			t.Fatalf("a %s check gives the headline %q, wanted it to start %q", c.key, h, c.head)
+		}
+	}
 	// The act the Approve binds is the one the teller checks and the controller verifies: this ask's router id, the option, and
 	// the heads as `owner/repo@<first 12 characters>` to the whole commit.
 	approve, _ := ask.Ask.Option(OptionApprove)
@@ -80,8 +80,9 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) {
 		after                     time.Duration
 		asks, open                int
 		want, not                 []string
+		head                      string // the ask's headline starts with it
 	}{
-		{name: "gated, passed", gated: true, state: "pass", asks: 1,
+		{name: "gated, passed", gated: true, state: "pass", asks: 1, head: "Merge into main: app#7 Fix it",
 			want: []string{"app#7 Fix it passed its check. Approve merges it into main at " + readyHead[:12] + ";"},
 			not:  []string{"before its check ends", "no verdict yet"}},
 		{name: "ungated, no verdict", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet"}, not: []string{"anyway"}},
@@ -98,6 +99,7 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) {
 		{name: "ungated, passed, its verdict gone", state: "pass", then: "gone", asks: 1, open: -1,
 			want: []string{waitsCheckBeforeAsking + "its check has no verdict yet"}, not: []string{"anyway"}},
 		{name: "ungated, failed", state: "fail", failed: "TestThing: wanted 1, got 2", asks: 1,
+			head: "Check failed \u2014 merge into main: app#7 Fix it",
 			want: []string{"its check failed: TestThing: wanted 1, got 2", "although its check failed"}, not: []string{"passed", "anyway"}},
 	} {
 		t.Run(row.name, func(t *testing.T) {
@@ -143,6 +145,9 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) {
 				}
 				return
 			}
+			if h := v.op.ask(open[0]).Headline; row.head != "" && !strings.HasPrefix(h, row.head) {
+				t.Fatalf("the headline is %q; wanted it to start %q", h, row.head)
+			}
 			e := v.op.ask(open[0]).Explanation
 			for _, w := range row.want {
 				if !strings.Contains(e, w) {
@@ -35,7 +35,7 @@ var busFields = map[string][]string{
 	"Record":           {"node", "run", "at", "installed", "kernel", "failed_units", "dkms"},
 	"DKMSModule":       {"module", "version", "kernels"},
 	"Round":            {"round", "opened", "opened_by", "order", "mirrors", "mirrors_note"},
-	"MirrorList":       {"servers", "ranked_at", "ranked_by", "options", "newest_sync", "round"},
+	"MirrorList":       {"servers", "ranked_at", "ranked_by", "options", "newest_sync", "freshest", "round"},
 	"RoundNode":        {"round", "node", "run", "pass"},
 	"Pass":             {"reason", "ask", "asked", "answer", "answered"},
 	"Ask":              {"id", "node", "act", "at", "of", "remove_added", "round", "pass", "reason"},
@@ -24,6 +24,7 @@ import (
 	"fmt"
 	"os"
 	"regexp"
+	"sort"
 	"strings"
 	"time"

@@ -165,20 +166,48 @@ func ParseReflectorInfo(text string) []rankedMirror {
 	return out
 }

-// Freshest keeps, in their order, only the mirrors whose last sync is no older than the newest found (ADR 0585 §2), so
-// a mirror lagging behind the others is never in the list; one whose last sync is unknown is left out.
-func Freshest(ranked []rankedMirror) (servers []string, newest time.Time) {
+// TailLength is how many of the next freshest mirrors follow the freshest ones in the list (ADR 0586 §1).
+const TailLength = 4
+
+// Freshest orders the mirrors by last sync, newest first, in reflector's order among equals, and keeps every mirror
+// whose last sync equals the newest found (the freshest, count of them) followed by up to TailLength of the next
+// freshest (ADR 0586 §1, which changes ADR 0585 §2); one whose last sync is unknown (zero) is left out.
+func Freshest(ranked []rankedMirror) (servers []string, newest time.Time, freshest int) {
+	var known []rankedMirror
 	for _, m := range ranked {
-		if m.LastSync.After(newest) {
-			newest = m.LastSync
+		if !m.LastSync.IsZero() {
+			known = append(known, m)
 		}
 	}
-	for _, m := range ranked {
-		if !m.LastSync.IsZero() && !m.LastSync.Before(newest) {
-			servers = append(servers, m.URL)
+	sort.SliceStable(known, func(i, j int) bool { return known[i].LastSync.After(known[j].LastSync) })
+	if len(known) == 0 {
+		return nil, newest, 0
+	}
+	newest = known[0].LastSync
+	for _, m := range known {
+		if m.LastSync.Equal(newest) {
+			freshest++
+		}
+	}
+	for i, m := range known {
+		if i >= freshest+TailLength {
+			break
+		}
+		servers = append(servers, m.URL)
+	}
+	return servers, newest, freshest
+}
+
+// withoutUnknownSync zeroes the last sync of every mirror that reflector printed as the moment it ran: --info writes an
+// unknown last sync (None) through gmtime(None), which is the current time, so a last sync at or after start (the
+// second the run began) cannot be a real one, since the status reflector reads is older than the run.
+func withoutUnknownSync(ranked []rankedMirror, start time.Time) {
+	start = start.UTC().Truncate(time.Second)
+	for i := range ranked {
+		if !ranked[i].LastSync.Before(start) {
+			ranked[i].LastSync = time.Time{}
 		}
 	}
-	return servers, newest
 }

 // reflectorRun runs reflector with args on this machine, within RankTimeout; its cache in the temporary directory, since
@@ -197,11 +226,12 @@ func RankMirrors(conf []byte, node string, now time.Time, round string) (MirrorL
 		return MirrorList{}, failure("reflector", "reflector", r)
 	}
 	ranked := ParseReflectorInfo(r.Stdout)
-	servers, newest := Freshest(ranked)
+	withoutUnknownSync(ranked, now)
+	servers, newest, freshest := Freshest(ranked)
 	if err := checkServers(servers); err != nil {
 		return MirrorList{}, fmt.Errorf("reflector ranked no list the mesh can use (%d mirrors read): %w", len(ranked), err)
 	}
-	return MirrorList{Servers: servers, RankedAt: now.UTC(), RankedBy: node, Options: opts, Newest: newest, Round: round}, nil
+	return MirrorList{Servers: servers, RankedAt: now.UTC(), RankedBy: node, Options: opts, Newest: newest, Freshest: freshest, Round: round}, nil
 }

 // readMirrorList is the shared list and its revision; nil when there is none yet.
@@ -379,6 +409,10 @@ func (m *Machine) MirrorsView(refresh bool) (map[string]any, error) {
 		return nil, err
 	}
 	out["shared"], out["revision"] = list, rev
+	if list != nil {
+		n := min(max(list.Freshest, 1), len(list.Servers))
+		out["freshest"], out["tail"] = list.Servers[:n], list.Servers[n:]
+	}
 	text, ferr := m.ReadFile(Mirrorlist)
 	if ferr != nil {
 		out["file_error"] = ferr.Error()
@@ -2,6 +2,7 @@ package main

 import (
 	"context"
+	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -306,7 +307,10 @@ https://fr.mirror.example/archlinux/$repo/os/$arch
 last_sync       : None
 `

-// ADR 0585 §2, §4: the ranking keeps only mirrors as fresh as the newest, in reflector's order, with the module's options
+// rankedAt is after every last sync in reflectorInfo: reflector ran then.
+var rankedAt = time.Date(2026, 11, 1, 15, 0, 0, 0, time.UTC)
+
+// ADR 0586 §1, ADR 0585 §4: the ranking keeps the mirrors at the newest sync and a tail of the next freshest, in reflector's order, with the module's options
 // and never its --save; a ranking that fails or keeps none is no list. pacman_mirrors answers the shared list, its
 // ranker and age and whether this node's file matches it (the file as found when not); with refresh it asks this node's
 // keeper to rank again, refused while a round runs, naming it.
@@ -320,18 +324,19 @@ func TestTheMeshsMirrorListIsRankedFreshAndAnsweredWithThisNodesFile(t *testing.
 	saved := reflectorRun
 	reflectorRun = func(_ context.Context, args ...string) Ran { asked = args; return answer }
 	defer func() { reflectorRun = saved }()
-	l, err := RankMirrors([]byte(conf), "node-a", t0, "2026-W43")
-	if err != nil || strings.Join(l.Servers, " ") != "https://nl.mirror.example/archlinux/$repo/os/$arch https://de.mirror.example/$repo/os/$arch" ||
+	l, err := RankMirrors([]byte(conf), "node-a", rankedAt, "2026-W43")
+	if err != nil || strings.Join(l.Servers, " ") != "https://nl.mirror.example/archlinux/$repo/os/$arch https://de.mirror.example/$repo/os/$arch https://be.mirror.example/archlinux/$repo/os/$arch" || l.Freshest != 2 ||
 		!l.Newest.Equal(time.Date(2026, 11, 1, 14, 32, 1, 0, time.UTC)) || l.RankedBy != "node-a" || l.Round != "2026-W43" ||
 		strings.Join(asked, " ") != "--country Belgium,Netherlands --sort rate --info" {
 		t.Fatalf("ranked %+v (%v), reflector asked %v", l, err, asked)
 	}
 	for _, a := range []Ran{{Status: 1, Stderr: "error: no mirrors"}, {Stdout: "https://x.example/$repo/os/$arch\nlast_sync : None\n"}} {
 		answer = a
-		if _, err := RankMirrors([]byte(conf), "node-a", t0, ""); err == nil {
+		if _, err := RankMirrors([]byte(conf), "node-a", rankedAt, ""); err == nil {
 			t.Errorf("a ranking from %+v is a list", a)
 		}
 	}
+	checkFreshest(t)
 	if checkServers([]string{"https://ok.example/$repo/os/$arch\nServer = https://evil.example/$repo/os/$arch"}) == nil {
 		t.Error("a server carrying a second line is taken")
 	}
@@ -343,7 +348,7 @@ func TestTheMeshsMirrorListIsRankedFreshAndAnsweredWithThisNodesFile(t *testing.
 		return Ran{Stdout: "LoadState=loaded\nUnitFileState=disabled\nActiveState=inactive\n"}
 	}, 1000)
 	m.Node, m.States = "node-a", readerOf(states)
-	m.Now = func() time.Time { return t0.Add(90 * time.Minute) }
+	m.Now = func() time.Time { return rankedAt.Add(90 * time.Minute) }
 	m.ReadFile = func(string) ([]byte, error) { return file, nil }
 	v, err := m.MirrorsView(false)
 	if err != nil || v["file_matches"] != true || v["ranked_by"] != "node-a" || v["age"] != "1h30m0s" || v["file"] != nil ||
@@ -382,3 +387,46 @@ func TestConfigIsReadAsPacmanConfResolvesIt(t *testing.T) {
 		t.Fatalf("%+v", c)
 	}
 }
+
+// ADR 0586 §1: the freshest first, then up to four of the next freshest, newest first; an unknown last sync is left out,
+// whether reflector wrote None or the moment it ran (gmtime(None)).
+func checkFreshest(t *testing.T) {
+	t.Helper()
+	at := func(m int) time.Time { return time.Date(2026, 11, 1, 14, m, 0, 0, time.UTC) }
+	mk := func(syncs ...int) []rankedMirror {
+		var r []rankedMirror
+		for i, m := range syncs {
+			mm := rankedMirror{URL: fmt.Sprintf("m%d", i)}
+			if m >= 0 {
+				mm.LastSync = at(m)
+			}
+			r = append(r, mm)
+		}
+		return r
+	}
+	for _, c := range []struct {
+		name    string
+		ranked  []rankedMirror
+		servers string
+		fresh   int
+	}{
+		{"all fresh", mk(30, 30, 30), "m0 m1 m2", 3},
+		{"one fresh and a tail of four", mk(10, 30, 20, 25, 5, 1), "m1 m3 m2 m0 m4", 1},
+		{"more than four candidates", mk(30, 29, 28, 27, 26, 25, 24), "m0 m1 m2 m3 m4", 1},
+		{"two fresh and the tail", mk(30, 20, 30, 10, 11, 12, 13), "m0 m2 m1 m6 m5 m4", 2},
+		{"unknown last sync left out", mk(30, -1, 20, -1), "m0 m2", 1},
+		{"none known", mk(-1, -1), "", 0},
+	} {
+		got, _, fresh := Freshest(c.ranked)
+		if strings.Join(got, " ") != c.servers || fresh != c.fresh {
+			t.Errorf("%s: kept %v, %d freshest; want %q, %d", c.name, got, fresh, c.servers, c.fresh)
+		}
+	}
+	// reflector prints an unknown last sync as the moment it ran: at or after the start of the run is no real sync.
+	start := time.Date(2026, 11, 1, 15, 0, 0, 500, time.UTC)
+	r := []rankedMirror{{"a", at(30)}, {"b", start.Truncate(time.Second)}, {"c", start.Add(time.Second)}}
+	withoutUnknownSync(r, start)
+	if r[0].LastSync.IsZero() || !r[1].LastSync.IsZero() || !r[2].LastSync.IsZero() {
+		t.Errorf("unknown syncs not detected: %+v", r)
+	}
+}
@@ -334,14 +334,15 @@ type Round struct {
 }

 // MirrorList is `mirrors.list`, the mesh's one mirror list (ADR 0585 §1): the servers in order, when and by which
-// node they were ranked, the options reflector ranked them with, the newest last sync found among them (only mirrors as
-// fresh as it are kept, §2), and the round it was ranked for (none for a refresh).
+// node they were ranked, the options reflector ranked them with, the newest last sync found among them (the mirrors at it come first,
+// then a tail of up to four fresher-than-the-rest, ADR 0586), and the round it was ranked for (none for a refresh).
 type MirrorList struct {
 	Servers  []string  `json:"servers"`
 	RankedAt time.Time `json:"ranked_at"`
 	RankedBy string    `json:"ranked_by"`
 	Options  []string  `json:"options"`
 	Newest   time.Time `json:"newest_sync"`
+	Freshest int       `json:"freshest"` // how many of the first servers are at the newest sync; the rest are the fallback tail (ADR 0586)
 	Round    string    `json:"round,omitempty"`
 }