diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index a256007a..d1dd7461 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -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 diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue783_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue783_test.go new file mode 100644 index 00000000..c8b11518 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue783_test.go @@ -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) + } +}