novox/mesh-catalog
13
0
Fork 0
Code Issues Pull Requests 3 Actions Packages Projects Releases Wiki Activity

Merge pull request 'A pull request at the same head as one closed unmerged takes its delivery over (issue 1114)' (#360) from fix/takeover-on-close into main

Browse Source
This commit was merged in pull request #360.
This commit is contained in:
mesh-tools
2026-10-18 09:59:02 +00:00
parent e40ec47058 30d3bb9466
commit 580fadb63e
3 changed files with 171 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
modules/mesh-delivery/cmd/mesh-delivery/holder.go
+73 -2
View File
@@ -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
+90
View File
@@ -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
+8 -5
View File
@@ -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).