From 2d1b20364f9bb7049c6b87b78d4f08e016831065 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 16 Oct 2026 14:35:58 +0200 Subject: [PATCH] mesh-delivery: an empty-head ready mark is cleared only after a minute, and not right after mark-ready; clearer wording (issue 913, issue 873) --- .../cmd/mesh-delivery/markready_test.go | 32 +++++++++++++++++-- .../mesh-delivery/cmd/mesh-delivery/ready.go | 22 +++++++++++-- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go b/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go index a99f24bc..703bdd78 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go @@ -4,6 +4,7 @@ import ( "errors" "strings" "testing" + "time" ) // **A reviewer's word marks the pull request ready in the order the forge counts** (novox/hq issue 913): the label, @@ -91,18 +92,45 @@ func TestMarkReadyWritesNoCommentWhenTheLabelFails(t *testing.T) { } } -func TestClearMarkWithAnEmptyHeadSaysWhatWasWrong(t *testing.T) { +func TestClearMarkWithAnEmptyHeadSaysWhatWasWrongOnceItStayedEmpty(t *testing.T) { w := newWorld(t) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")} w.h.SeeOpenPulls() w.settleAll() - if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "no \"ready at \" comment by a listed account") || + if len(w.forge.comments) != 0 || len(w.forge.unlabels) != 0 { + t.Fatalf("an empty-head mark seen once was cleared: %v", w.forge.comments) + } + w.later(readyTryEvery + time.Second) + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "no \"ready at\" comment by a listed account") || + !strings.Contains(w.forge.comments[0], "the head is now "+readyHead) || !strings.Contains(w.forge.comments[0], "mesh-delivery.mark-ready") || strings.Contains(w.forge.comments[0], "pushed after it") { t.Fatalf("comments: %v", w.forge.comments) } } +func TestAMarkReadyJustMadeIsNotClearedByAReadThatPredatesItsComment(t *testing.T) { + w := newWorld(t) + id := readyDelivery(t, w) + if _, err := w.h.MarkReady(id, "a@b", "ok"); err != nil { + t.Fatal(err) + } + w.later(readyTryEvery + time.Second) // the label alone was read long enough ago to count as stale... + w.forge.open = []OpenPull{markedPull("novox/app", 7, fullHead, "")} + w.h.SeeOpenPulls() + w.h.mu.Lock() + w.h.deliveries[id].Ready.Seen = w.now // ...but the mark was made just now + w.h.marks["novox/app#7"].firstEmpty = w.now.Add(-2 * readyTryEvery) + w.h.mu.Unlock() + before := len(w.forge.comments) + w.settleAll() + if len(w.forge.unlabels) != 0 || len(w.forge.comments) != before { + t.Fatalf("a mark just made was cleared: %v %v", w.forge.unlabels, w.forge.comments) + } +} + func TestClearMarkWithAnOlderHeadKeepsTheOldWording(t *testing.T) { w := newWorld(t) w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index 9743a664..f6f98672 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -88,6 +88,9 @@ type marked struct { pull OpenPull // cleared is when the mark was cleared, the label taken off: not cleared again within readyTryEvery. cleared time.Time + // firstEmpty is when a mark with no head (a label no "ready at" comment follows yet) was first read so: it is a + // mark not yet complete, and is cleared only once it has stayed so for readyTryEvery (novox/hq issue 873). + firstEmpty time.Time } // pullKey is a pull request as a key: owner/repo#number. @@ -109,6 +112,13 @@ func (h *Holder) seeMarks(open []OpenPull) { was.pull.HeadSHA == p.HeadSHA { m.cleared = was.cleared } + if p.Ready.Head == "" { + m.firstEmpty = h.Now() + if was := h.marks[k]; was != nil && was.pull.Ready != nil && was.pull.Ready.Head == "" && + was.pull.HeadSHA == p.HeadSHA && !was.firstEmpty.IsZero() { + m.firstEmpty = was.firstEmpty + } + } seen[k] = m } h.marks = seen @@ -129,6 +139,13 @@ func (h *Holder) readyAsks(now time.Time) []func() { p, mark := m.pull, *m.pull.Ready repository := strings.ToLower(p.Owner + "/" + p.Repo) if !sameFull(mark.Head, p.HeadSHA) { + if mark.Head == "" && now.Sub(m.firstEmpty) < readyTryEvery { + continue // the label first, its comment not yet: a mark not complete, not an old one + } + if d := h.deliveries[IDOf(repository, p.HeadSHA)]; d != nil && d.Ready != nil && + sameFull(d.Ready.Head, p.HeadSHA) && now.Sub(d.Ready.Seen) < readyTryEvery { + continue // marked through mark-ready a moment ago: this read may predate its comment + } if !m.cleared.IsZero() && now.Sub(m.cleared) < readyTryEvery { continue } @@ -297,8 +314,9 @@ func (h *Holder) clearMark(k string, p OpenPull, mark ReadyMark) func() { // comment of a listed account followed (novox/hq issue 873); one with a head names an older head than the pull's. func clearedSaying(mark ReadyMark, p OpenPull) string { if mark.Head == "" { - return "mesh-delivery: the label " + ReadyLabel + " is taken off: no \"ready at \" comment by a listed " + - "account was written after it. Mark it with `mesh-delivery.mark-ready`, which writes both in the order the forge counts." + return "mesh-delivery: the label " + ReadyLabel + " is taken off: no \"ready at\" comment by a listed account " + + "follows both the label's last addition and the last push; the head is now " + p.HeadSHA + ". Review it and " + + "mark that head with `mesh-delivery.mark-ready`, which writes label and comment in the order the forge counts." } return fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+ "the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA)