mesh-delivery: a stopped duplicate's delivery follows the pull request that merges it (issue 783)

A delivery keyed by commit stayed stopped forever once an earlier, closed duplicate pull request with the same head had stopped it, even though a later pull request went on to merge that very commit: the merge only set merged_as on the stopped record, and the walk that carried it waited for this owner's word with no delivery left to give it. PullUpdated and PullMerged now start such a delivery over, under the same id, for the pull request that announces or merges it next, naming both pull requests in the transition's why. Any other stop (a person's, a group's, the same pull request's own close) is untouched.
2026-10-15 14:50:57 +02:00
parent 8c60604eb7
commit 9e8521afb5
2 changed files with 176 additions and 17 deletions
@@ -312,19 +312,25 @@ func (h *Holder) PullUpdated(p PullEvent) {
 	defer h.mu.Unlock()
 	id := IDOf(p.repository(), p.HeadSHA)
 	now := h.Now()
+	startedOver := ""
 	if d := h.deliveries[id]; d != nil {
-		// Announced again: nothing new — but for a delivery whose verdict is out of date, the announcement is its
-		// check asked again against the trunk's head (the merge tool's ask, novox/hq ADR 0323 §5), and its bound
-		// runs from now (§7). The base head it was announced with is kept, for a check this owner asks after it.
-		if p.BaseSHA != "" && d.BaseSHA != p.BaseSHA {
-			d.BaseSHA = p.BaseSHA
-			h.keep(d)
+		if startedOver = startedOverFor(d, p.Number); startedOver == "" {
+			// Announced again: nothing new — but for a delivery whose verdict is out of date, the announcement is its
+			// check asked again against the trunk's head (the merge tool's ask, novox/hq ADR 0323 §5), and its bound
+			// runs from now (§7). The base head it was announced with is kept, for a check this owner asks after it.
+			if p.BaseSHA != "" && d.BaseSHA != p.BaseSHA {
+				d.BaseSHA = p.BaseSHA
+				h.keep(d)
+			}
+			if d.State == Proposed && d.outOfDate() != "" && d.AskedAgain == nil {
+				d.AskedAgain = &now
+				h.keep(d)
+			}
+			return
 		}
-		if d.State == Proposed && d.outOfDate() != "" && d.AskedAgain == nil {
-			d.AskedAgain = &now
-			h.keep(d)
-		}
-		return
+		// A delivery stopped only because its pull request closed unmerged does not end its commit's delivery for
+		// another pull request with the same head (novox/hq issue 783): started over below, exactly as for a first
+		// announcement, under the same id — never a transition out of Stopped.
 	}
 	replaced := ""
 	for _, old := range h.deliveries {
@@ -338,11 +344,7 @@ func (h *Holder) PullUpdated(p PullEvent) {
 			}
 		}
 	}
-	d := &Delivery{ID: id, Repository: p.repository(), Commit: p.HeadSHA, Number: p.Number, Title: p.Title,
-		Base: p.Base, Branch: p.Head, HTMLURL: p.HTMLURL, CloneURL: p.CloneURL, Paths: p.Paths,
-		PathsTruncated: p.PathsTruncated, Removed: p.Removed, ModuleDirs: p.ModuleDirs, ModuleDirsSaid: p.ModuleDirsSaid,
-		After: AfterIn(p.Body), Created: now, BaseSHA: p.BaseSHA}
-	t, err := Apply(d, EvAnnounced, false, Facts{Now: now, Why: fmt.Sprintf("%s#%d's head announced", d.Repository, d.Number)})
+	d, t, err := h.freshAnnouncement(p, now, startedOver)
 	if err != nil {
 		h.refuse(err)
 		return
@@ -360,6 +362,34 @@ func (h *Holder) PullUpdated(p PullEvent) {
 	}
 }

+// startedOverFor is why a delivery already kept at a head's id is started over for a pull request's number rather
+// than taken as announced again: only when it was stopped solely by an earlier, closed duplicate pull request with
+// the same head — still Stopped, ClosedUnmerged, never merged, and under another number (novox/hq issue 783).
+// Empty for the ordinary 'announced again' path, or for any other stop (a person's, a group's, its own close).
+func startedOverFor(d *Delivery, number int) string {
+	if d != nil && d.State == Stopped && d.ClosedUnmerged && d.MergedAs == "" && d.Number != number {
+		return fmt.Sprintf("#%d with the same head closed unmerged", d.Number)
+	}
+	return ""
+}
+
+// freshAnnouncement makes a delivery for p's head exactly as a first announcement would: its own id, proposed.
+// startedOver, when not empty, is folded into the transition's Why, naming the pull request this replaces
+// (novox/hq issue 783); the delivery it replaces is swapped out by the caller keeping the new one under the same
+// id (keep), never by a transition out of its final state.
+func (h *Holder) freshAnnouncement(p PullEvent, now time.Time, startedOver string) (*Delivery, Transition, error) {
+	d := &Delivery{ID: IDOf(p.repository(), p.HeadSHA), Repository: p.repository(), Commit: p.HeadSHA, Number: p.Number,
+		Title: p.Title, Base: p.Base, Branch: p.Head, HTMLURL: p.HTMLURL, CloneURL: p.CloneURL, Paths: p.Paths,
+		PathsTruncated: p.PathsTruncated, Removed: p.Removed, ModuleDirs: p.ModuleDirs, ModuleDirsSaid: p.ModuleDirsSaid,
+		After: AfterIn(p.Body), Created: now, BaseSHA: p.BaseSHA}
+	why := fmt.Sprintf("%s#%d's head announced", d.Repository, d.Number)
+	if startedOver != "" {
+		why = fmt.Sprintf("%s; %s", why, startedOver)
+	}
+	t, err := Apply(d, EvAnnounced, false, Facts{Now: now, Why: why})
+	return d, t, err
+}
+
 // takePlaceOf puts a member's new head — a rebase, a push — in its group in the place of the head it superseded,
 // when the group is closed: a member of it already reached the trunk, and a closed group takes no new member. The
 // new head has its own check, and the group's composed check runs again for the new heads rather than holding the
@@ -491,7 +521,21 @@ func (h *Holder) PullMerged(p PullEvent) {
 	}
 	now := h.Now()
 	var ts []Transition
-	if d == nil {
+	if over := startedOverFor(d, p.Number); over != "" {
+		// A delivery stopped only by an earlier, closed duplicate pull request with the same head is not lost when
+		// its merge is heard without the open announcement either: it is started over for the merging pull request
+		// the same way an announcement would, before the merge is taken as usual (novox/hq issue 783).
+		fresh, t, err := h.freshAnnouncement(p, now, over)
+		if err != nil {
+			h.refuse(err)
+			return
+		}
+		d, ts = fresh, []Transition{t}
+		if v := decidedOnHead(p.HeadChecks, now); v != nil {
+			d.Check = v
+			ts = append(ts, settle(d, Facts{Now: now})...)
+		}
+	} else if d == nil {
 		head := p.HeadSHA
 		if head == "" {
 			head = p.MergeCommit
@@ -0,0 +1,115 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// novox/hq issue 783: mesh-catalog commit b5417c28 was pushed while #305 (from branch adr-359-mesh-mail-733) was
+// open, so this owner recorded its delivery as #305's. #305 then closed unmerged (its base branch had been
+// deleted), and #315, opened from the same branch and head, merged it as 0914a33f. The delivery stayed stopped
+// ("the pull request closed unmerged"), though it recorded merged_as 0914a33f: a rule did not hold — a delivery
+// keyed by commit follows the pull request that merges that commit, and a closed duplicate does not end it — and
+// the walk that carried the merge waited 64 minutes for this owner's word until a person's `plans go` released it.
+//
+// A delivery stopped only because its pull request closed unmerged, with no merge recorded, does not end its
+// commit's delivery for another pull request with the same head: it is started over, under the same id, for the
+// pull request that announces or merges it next.
+
+const (
+	head783, merge783   = "d305d305d305d305", "d315d315d315d315"
+	head783b, merge783b = "d306d306d306d306", "d316d316d316d316"
+	head783c            = "d307d307d307d307"
+	catalog783          = "novox/mesh-catalog"
+	branch783           = "adr-359-mesh-mail-733"
+)
+
+// TestADeliveryStoppedByAClosedDuplicateFollowsThePullRequestThatMergesItsCommit is the issue's own shape: #305
+// announced, closed unmerged (stopped); #315 announced from the same head, checked and readied; #315 merges it,
+// and the walk that answers that merge is let go — the delivery follows #315, never stuck on #305's stop.
+func TestADeliveryStoppedByAClosedDuplicateFollowsThePullRequestThatMergesItsCommit(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(catalog783, head783)
+
+	w.h.PullUpdated(pr(catalog783, 305, head783, branch783, "src/a.go"))
+	w.h.PullClosed(pr(catalog783, 305, head783, branch783))
+	if w.state(id) != Stopped {
+		t.Fatalf("#305 closed unmerged left it %s; want stopped", w.state(id))
+	}
+
+	w.h.PullUpdated(pr(catalog783, 315, head783, branch783, "src/a.go"))
+	if d := w.delivery(id); d.State == Stopped || d.Number != 315 {
+		t.Fatalf("#315, the same head, left it %s, Number %d; want started over for #315, not stopped", d.State, d.Number)
+	}
+	if d := w.delivery(id); !strings.Contains(d.Transitions[len(d.Transitions)-1].Why, "#305 with the same head closed unmerged") {
+		t.Fatalf("the start-over's transition names what it replaces: %q", d.Transitions[len(d.Transitions)-1].Why)
+	}
+
+	w.h.Checked(verdict(catalog783, 315, head783, "pass"))
+	if d := w.delivery(id); d.State != Ready {
+		t.Fatalf("a passing verdict for #315 left it %s", d.State)
+	}
+
+	walk := w.ctl.put(aWalk("plan-783", catalog783, merge783, true))
+	w.h.WalkMoved(walk)
+	w.h.PullMerged(merged(catalog783, 315, head783, merge783))
+	if d := w.delivery(id); d.State != Published || d.Number != 315 {
+		t.Fatalf("#315's merge left it %s, Number %d; want published under #315", d.State, d.Number)
+	}
+
+	w.settleAll()
+	if len(w.ctl.delivers) != 1 || w.ctl.delivers[0] != "plan-783" {
+		t.Fatalf("the walk that answers #315's merge was not let go: %v", w.ctl.delivers)
+	}
+	if d := w.delivery(id); d.State != Delivering {
+		t.Fatalf("let go, it is %s; want delivering", d.State)
+	}
+}
+
+// TestAMergeWithoutAnAnnouncementStartsOverAStoppedDuplicateToo is decision 4: the merging pull request is never
+// announced on its own (heard only as `pull.merged`) — the start-over happens in PullMerged itself, not only in
+// PullUpdated, so a merge heard without the open announcement is not lost either.
+func TestAMergeWithoutAnAnnouncementStartsOverAStoppedDuplicateToo(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(catalog783, head783b)
+
+	w.h.PullUpdated(pr(catalog783, 306, head783b, branch783+"-b", "src/b.go"))
+	w.h.PullClosed(pr(catalog783, 306, head783b, branch783+"-b"))
+	if w.state(id) != Stopped {
+		t.Fatalf("#306 closed unmerged left it %s; want stopped", w.state(id))
+	}
+
+	walk := w.ctl.put(aWalk("plan-783b", catalog783, merge783b, true))
+	w.h.WalkMoved(walk)
+	m := merged(catalog783, 316, head783b, merge783b)
+	m.HeadChecks = map[string]string{"mesh/merge-gate": "success", "mesh/repo-check": "success"}
+	w.h.PullMerged(m)
+	if d := w.delivery(id); d.State == Stopped || d.Number != 316 {
+		t.Fatalf("#316's merge, heard with no announcement of its own, left it %s, Number %d; want started over for #316",
+			d.State, d.Number)
+	}
+
+	w.settleAll()
+	if len(w.ctl.delivers) != 1 || w.ctl.delivers[0] != "plan-783b" {
+		t.Fatalf("the walk that answers #316's merge was not let go: %v", w.ctl.delivers)
+	}
+}
+
+// TestAStoppedDeliveryAnnouncedAgainByItsOwnPullRequestStaysStopped guards decision 3's "any other stopped
+// delivery ... is untouched": the same pull request announced again after its own close is the ordinary
+// 'announced again' path, never a start-over, and stays stopped.
+func TestAStoppedDeliveryAnnouncedAgainByItsOwnPullRequestStaysStopped(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(catalog783, head783c)
+
+	w.h.PullUpdated(pr(catalog783, 307, head783c, branch783+"-c", "src/c.go"))
+	w.h.PullClosed(pr(catalog783, 307, head783c, branch783+"-c"))
+	if w.state(id) != Stopped {
+		t.Fatalf("#307 closed unmerged left it %s; want stopped", w.state(id))
+	}
+
+	w.h.PullUpdated(pr(catalog783, 307, head783c, branch783+"-c", "src/c.go"))
+	if d := w.delivery(id); d.State != Stopped || d.Number != 307 {
+		t.Fatalf("#307 announced again by itself left it %s, Number %d; want it to stay stopped", d.State, d.Number)
+	}
+}