A pull request announced at a live head takes its delivery over when the other one closes or is edited (issue 1114)

modules/mesh-delivery/cmd/mesh-delivery/holder.go

@@ -329,6 +329,11 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) {
 	}
 	h.mu.Lock()
 	defer h.mu.Unlock()
+	return h.pullUpdated(p)
+}
+
+// pullUpdated is PullUpdated under the lock.
+func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 	id := IDOf(p.repository(), p.HeadSHA)
 	isNew = h.deliveries[id] == nil
 	now := h.Now()
@@ -356,6 +361,12 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) {
 				d.AskedAgain = &now
 				h.keep(d)
 			}
+			// Another pull request at this head, announced while this one is live (issue 1114): kept, so this one's close
+			// starts the record over for it.
+			if d.Number != p.Number && !d.State.Final() && d.MergedAs == "" {
+				d.Others = withOther(d.Others, p)
+				h.keep(d)
+			}
 			return
 		}
 		// A delivery stopped only because its pull request closed unmerged does not end its commit's delivery for
@@ -405,12 +416,32 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) {
 // approved, so no Approve ended. An answered ask is left as it is. The record is never a gate (ADR 0421 §5). An edit with no
 // description, or of a pull request with no open delivery at that head, changes nothing.
 func (h *Holder) PullEdited(p PullEvent) {
-	if p.HeadSHA == "" || p.Number == 0 || p.Body == "" {
+	if p.HeadSHA == "" || p.Number == 0 {
 		return
 	}
 	h.mu.Lock()
 	defer h.mu.Unlock()
 	d := h.deliveries[IDOf(p.repository(), p.HeadSHA)]
+	if d != nil && startedOverFor(d, p.Number) != "" {
+		// The record at this head is stopped under another, closed pull request (issue 1114: this one was announced while
+		// that one was live and never heard again): the edit takes it over, as an announcement would. The forge's edit
+		// event carries no paths; the record's, for the same head, are the ones.
+		if p.Paths == nil {
+			p.Paths, p.PathsTruncated, p.Removed = d.Paths, d.PathsTruncated, d.Removed
+			p.ModuleDirs, p.ModuleDirsSaid = d.ModuleDirs, d.ModuleDirsSaid
+		}
+		if p.CloneURL == "" {
+			p.CloneURL = d.CloneURL
+		}
+		if p.BaseSHA == "" {
+			p.BaseSHA = d.BaseSHA
+		}
+		h.pullUpdated(p)
+		return
+	}
+	if p.Body == "" {
+		return
+	}
 	if d == nil || d.Number != p.Number || d.State.Final() || d.MergedAs != "" {
 		return
 	}
@@ -431,6 +462,23 @@ func (h *Holder) PullEdited(p PullEvent) {
 	}
 }
 
+// withOther is others with p put in, replacing an earlier announcement of the same pull request.
+func withOther(others []PullEvent, p PullEvent) []PullEvent {
+	out := withoutOther(others, p.Number)
+	return append(out, p)
+}
+
+// withoutOther is others without the pull request number.
+func withoutOther(others []PullEvent, number int) []PullEvent {
+	var out []PullEvent
+	for _, o := range others {
+		if o.Number != number {
+			out = append(out, o)
+		}
+	}
+	return out
+}
+
 // 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: it is Stopped, never merged, and under another number. Stopped by an earlier, closed
 // duplicate pull request with the same head (novox/hq issue 783), or by a person or a group before its pull request
@@ -748,15 +796,38 @@ func verdictOfState(s string) string {
 func (h *Holder) PullClosed(p PullEvent) {
 	h.mu.Lock()
 	h.forgetUnchecked(p.Owner, p.Repo, p.Number)
+	var over *PullEvent
 	for _, d := range h.deliveries {
-		if d.Repository == p.repository() && d.Number == p.Number && !d.State.Final() && d.MergedAs == "" {
+		if d.Repository != p.repository() {
+			continue
+		}
+		if d.Number != p.Number {
+			// The other pull request at this head closed first: it is no longer one to take the record over.
+			if len(d.Others) > 0 {
+				if rest := withoutOther(d.Others, p.Number); len(rest) != len(d.Others) {
+					d.Others = rest
+					h.keep(d)
+				}
+			}
+			continue
+		}
+		if !d.State.Final() && d.MergedAs == "" {
 			d.ClosedUnmerged = true
 			h.settleAndKeep(d)
 			if d.Group != "" {
 				h.leaveGroup(d)
 			}
+			// Another open pull request carries this head (issue 1114): the record starts over for it at once, as a first
+			// announcement of it would, so it gets its check, its review record and its Approve ask.
+			if len(d.Others) > 0 {
+				next := d.Others[0]
+				over = &next
+			}
 		}
 	}
+	if over != nil {
+		h.pullUpdated(*over)
+	}
 	h.mu.Unlock()
 	if h.Controller == nil || p.Owner == "" || p.Repo == "" || p.Number <= 0 {
 		return

modules/mesh-delivery/cmd/mesh-delivery/issue1114_test.go

@@ -0,0 +1,90 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// novox/hq issue 1114: mesh-controller #379 was opened at the head of the still-open #378. Its announcement read as the
+// same delivery announced again, so when #378 closed unmerged the shared record went Stopped and #379 had no delivery.
+// The other open pull request at the head is kept on the record; a close of the record's own pull request starts the
+// record over for it at once (as issue 1083's take-over does for a later announcement), and an edit of it does so too,
+// so a case stuck before the fix frees itself. A merged record is never taken over.
+
+const head1114 = "0149a3d70149a3d70149a3d70149a3d70149a3d7"
+
+func TestClosingThePullRequestOfASharedHeadStartsTheOtherOnesDeliveryOver(t *testing.T) {
+	w := newWorld(t)
+	w.op.auto = false
+	id := IDOf(gCtl, head1114)
+	w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go"))
+	b := pr(gCtl, 379, head1114, "feat/chain-2", "internal/a.go")
+	b.Body = "Reviewed-by: reviewer@shanks at " + head1114
+	w.h.PullUpdated(b) // announced while #378 is live: the same delivery heard again
+	if d := w.delivery(id); d.Number != 378 {
+		t.Fatalf("setup: the record is #%d, wanted #378 still", d.Number)
+	}
+	w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain"))
+	d := w.delivery(id)
+	if d.Number != 379 || d.State.Final() || d.ClosedUnmerged {
+		t.Fatalf("after #378 closed the record is #%d %s (closed %v); wanted #379, proposed", d.Number, d.State, d.ClosedUnmerged)
+	}
+	if d.ReviewedBy == "" {
+		t.Fatalf("#379's review record was not read: %+v", d)
+	}
+	w.h.Checked(verdict(gCtl, 379, head1114, "pass"))
+	w.settleAll()
+	if open := w.op.open(); w.state(id) != Ready || len(open) != 1 ||
+		!strings.Contains(w.op.ask(open[0]).Explanation, "reviewed by reviewer") {
+		t.Fatalf("#379's delivery is %s with asks %v; wanted ready and one Approve ask with its review record", w.state(id), w.op.open())
+	}
+}
+
+func TestClosingTheOtherPullRequestOfASharedHeadLeavesTheRecordAlone(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(gCtl, head1114)
+	w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go"))
+	w.h.PullUpdated(pr(gCtl, 379, head1114, "feat/chain-2", "internal/a.go"))
+	w.h.PullClosed(pr(gCtl, 379, head1114, "feat/chain-2"))
+	w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain"))
+	if d := w.delivery(id); d.Number != 378 || !d.ClosedUnmerged || !d.State.Final() {
+		t.Fatalf("with #379 closed first, #378's close left %d %s; wanted it stopped under #378", d.Number, d.State)
+	}
+}
+
+func TestAnEditOfAPullRequestWhoseSharedHeadsRecordIsStoppedUnderAnotherTakesItOver(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(gCtl, head1114)
+	w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go"))
+	w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain")) // #379 was never heard: the stuck case
+	if d := w.delivery(id); !d.State.Final() {
+		t.Fatalf("setup: %s", d.State)
+	}
+	e := pr(gCtl, 379, head1114, "feat/chain-2")
+	e.Body = "Reviewed-by: reviewer@shanks at " + head1114
+	w.h.PullEdited(e)
+	d := w.delivery(id)
+	if d.Number != 379 || d.State != Proposed || d.ReviewedBy == "" || len(d.Paths) == 0 {
+		t.Fatalf("after #379's edit the record is #%d %s reviewed %q paths %v; wanted #379 proposed, read, with the paths", d.Number,
+			d.State, d.ReviewedBy, d.Paths)
+	}
+	w.h.PullEdited(e) // an edit heard again changes nothing
+	if again := w.delivery(id); again.Number != 379 || again.State != Proposed {
+		t.Fatalf("a second edit moved it: #%d %s", again.Number, again.State)
+	}
+}
+
+func TestAMergedRecordIsNeverTakenOverByTheOtherPullRequestOfItsHead(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(gCtl, head1114)
+	w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go"))
+	w.h.PullUpdated(pr(gCtl, 379, head1114, "feat/chain-2", "internal/a.go"))
+	w.h.PullMerged(merged(gCtl, 378, head1114, "f378f378f378f378"))
+	w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain"))
+	e := pr(gCtl, 379, head1114, "feat/chain-2")
+	e.Body = "text"
+	w.h.PullEdited(e)
+	if d := w.delivery(id); d.Number != 378 || d.MergedAs == "" {
+		t.Fatalf("a merged record was taken over: #%d merged as %q", d.Number, d.MergedAs)
+	}
+}

modules/mesh-delivery/cmd/mesh-delivery/model.go

@@ -69,11 +69,14 @@ type Delivery struct {
 
 	// The facts the table's guards read. NewerHead is the head that superseded it; ClosedUnmerged the pull
 	// request closed; MergedAs the commit it landed on the trunk as; HeldWhy what holds it for a person.
-	NewerHead      string    `json:"newer_head,omitempty"`
-	ClosedUnmerged bool      `json:"closed_unmerged,omitempty"`
-	MergedAs       string    `json:"merged_as,omitempty"`
-	MergedAt       time.Time `json:"merged_at"`
-	HeldWhy        string    `json:"held_why,omitempty"`
+	NewerHead      string `json:"newer_head,omitempty"`
+	ClosedUnmerged bool   `json:"closed_unmerged,omitempty"`
+	// Others are the other open pull requests announced at this head while this one was live (issue 1114), as they were
+	// announced: the record is theirs to take over when this one's pull request closes unmerged.
+	Others   []PullEvent `json:"others,omitempty"`
+	MergedAs string      `json:"merged_as,omitempty"`
+	MergedAt time.Time   `json:"merged_at"`
+	HeldWhy  string      `json:"held_why,omitempty"`
 	// BackupsAgainAfter is the revision of its walk when a person released it for a failed backup: a record at that
 	// revision or before still holds the failure the controller was asked to clear, and does not hold it again
 	// (novox/hq ADR 0404, issue 1068).