diff --git a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go index 87d37458..a9562208 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go @@ -531,8 +531,14 @@ func TestAGroupOfTwoWithGreenChecksGetsOneAskAndOnItsApproveBothMergeInOnePassOv b.publish(t, "mesh.seat."+ControllerSeat+".event.checked", judgedAt("novox/mesh-catalog", 2, catHead, "pass", "t1t1t1t1t1t1")) // Both members green: the composed check is asked, once, for both. + // Each member's own check is asked at once (issue 1115), before the group's: wait for the one carrying the group. rt.until("the composed check to be asked", 10*time.Second, func() bool { - return len(rt.asksOf("seat:"+ControllerSeat+".delivery-check")) >= 1 + for _, c := range rt.asksOf("seat:" + ControllerSeat + ".delivery-check") { + if c.Body["group"] == "feat/x" { + return true + } + } + return false }) if n := len(rt.asksOf("seat:" + ControllerSeat + ".delivery-order")); n != 0 { t.Fatalf("the controller's delivery-order was asked %d times: no order is asked for", n) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/checkask_test.go b/modules/mesh-delivery/cmd/mesh-delivery/checkask_test.go index 7cae99d1..506035c8 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/checkask_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/checkask_test.go @@ -18,12 +18,12 @@ func TestAProposedDeliveryAdoptedWithNoVerdictHasItsCheckAsked(t *testing.T) { id := IDOf("novox/hq", head) w.h.PullUpdated(pr("novox/hq", 94, head, "issues/245-246", "04-ISSUES/245/00-report.md")) w.settleAll() - if len(w.ctl.checks) != 0 { - t.Fatalf("a head just announced was asked at once, before the controller's own check had time: %v", w.ctl.checks) + if len(w.ctl.checks) != 1 { + t.Fatalf("a head just announced was not asked at once: %v", w.ctl.checks) } - // A restart, hours later: read back proposed, no verdict, nothing asked. - again := newWorldOver(t, w.store, w.ctl, w.forge, w.now.Add(10*time.Hour)) + // A restart, a minute later: read back proposed with its ask kept; it is not asked again. + again := newWorldOver(t, w.store, w.ctl, w.forge, w.now.Add(time.Minute)) if s := again.state(id); s != Proposed { t.Fatalf("read back, it is %s", s) } @@ -55,18 +55,12 @@ func TestAProposedDeliveryAdoptedWithNoVerdictHasItsCheckAsked(t *testing.T) { } } -// The same head adopted at a first start with no restart: the grace passes, and its check is asked. -func TestAMissedAnnouncementHasItsCheckAskedAfterTheGrace(t *testing.T) { +// The same head adopted at a first start with no restart: its check is asked at once, with no grace (issue 1115). +func TestAMissedAnnouncementHasItsCheckAskedAtOnce(t *testing.T) { w := newWorld(t) id := IDOf("novox/app", head) w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) - w.later(checkGrace - time.Minute) - w.settleAll() - if len(w.ctl.checks) != 0 { - t.Fatalf("asked within the grace: %v", w.ctl.checks) - } // The controller does not answer: nothing is kept as asked, and it is tried again a minute on. - w.later(2 * time.Minute) w.ctl.down = true w.settleAll() if d := w.delivery(id); d.CheckAsk != nil { @@ -80,7 +74,7 @@ func TestAMissedAnnouncementHasItsCheckAskedAfterTheGrace(t *testing.T) { w.later(time.Minute) w.settleAll() if len(w.ctl.checks) != 1 || w.delivery(id).CheckAsk == nil { - t.Fatalf("a missed announcement was not asked once the grace passed: %v", w.ctl.checks) + t.Fatalf("a missed announcement was not asked: %v", w.ctl.checks) } // A verdict that arrived in the meantime, or a delivery past its proposal, is never asked. other := IDOf("novox/app", "bbbbbbbbbbbb2222") @@ -116,7 +110,6 @@ func TestH2ReasksAStalledCheckOnceThenItIsTheOperators(t *testing.T) { w := newWorld(t) id := IDOf("novox/app", head) w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) - w.later(checkGrace + time.Minute) w.settleAll() if len(w.ctl.checks) != 1 { t.Fatalf("asked %v", w.ctl.checks) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index f48b1a76..c2f355bc 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -109,6 +109,8 @@ type fakeController struct { moves map[string][]string plans int down bool + // emptyCheck makes Check answer with no id and no error, as the port does when the answer holds none. + emptyCheck bool // walkReads counts the reads of the controller's walks (Walks). walkReads int // checkReady is the ready flag of each member of each check asked. @@ -185,6 +187,9 @@ func (c *fakeController) Check(group string, ms []Member) (string, error) { } c.asked++ id := fmt.Sprintf("check-%d", c.asked) + if c.emptyCheck { + id = "" + } var heads []string for _, m := range ms { heads = append(heads, m.ID) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go index f508a157..c55936fd 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go @@ -101,8 +101,13 @@ func TestAMemberTheForgeRefusesIsMergedAgainWhileTheOtherMergedInTheSamePass(t * w.forge.refuse = nil w.later(2 * time.Minute) w.settleAll() + if len(w.forge.merges) != 2 { + t.Fatalf("a refused member is asked again with no event, on a timer or at each look: %v", w.forge.merges) + } + w.h.Checked(CheckedEvent{Group: "feat/g", ID: w.h.groups["feat/g"].Check.Asked, Verdict: "pass", Summary: "composed again"}) + w.settleAll() if len(w.forge.merges) != 4 { - t.Fatalf("a refused member is asked again a minute on: %v", w.forge.merges) + t.Fatalf("a refused member is asked again on the next composed verdict: %v", w.forge.merges) } if d := w.delivery(IDOf(gCat, gHk)); d.Ready.Merged == nil { t.Fatalf("not merged: %+v", d.Ready) @@ -263,7 +268,7 @@ func TestAClosedGroupsMemberWhoseMergeWasRefusedAsksTheComposedCheckAgainAtOnce( // Issue 1082: a refusal loop is not silent. A merge refused past ten minutes in a closed group (ADR 0418 §3) is a stalled // line naming the member and the merge tool's last answer, which the controller raises as a condition. -func TestARefusalLoopInAClosedGroupIsSaidPastTenMinutes(t *testing.T) { +func TestARefusalLoopInAClosedGroupIsSaidAtOnce(t *testing.T) { w := approvedGroup(t) w.forge.refuseRepo = map[string]error{gCtl: errAskedAgain} w.op.answer(w.op.open()[0], OptionApprove) @@ -276,12 +281,9 @@ func TestARefusalLoopInAClosedGroupIsSaidPastTenMinutes(t *testing.T) { g := w.h.groups["feat/g"] w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed again"}) w.settleAll() - if lines := w.h.Stalled(); len(lines) != 0 && w.now.Sub(*w.delivery(IDOf(gCtl, gHc)).Ready.RefusedSince) < 10*time.Minute { - t.Fatalf("said before its bound: %+v", lines) - } } lines := w.h.Stalled() - if len(lines) != 1 || lines[0].ID != IDOf(gCtl, gHc) || lines[0].Bound != "10m0s" || + if len(lines) != 1 || lines[0].ID != IDOf(gCtl, gHc) || lines[0].Bound != "0s" || !strings.Contains(lines[0].Says, errAskedAgain.Error()) || !strings.Contains(lines[0].Says, "group feat/g") { t.Fatalf("the loop was not said, naming the member, its group and the tool's answer: %+v", lines) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index adb84b6f..41aaf4aa 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -1314,12 +1314,10 @@ func (h *Holder) plan() []func() { } } - // Proposed, with no verdict and no check asked past the grace: its check is asked — a head adopted while - // it already waited, or one whose announcement the controller missed. - for _, d := range h.deliveries { - if a := h.askCheck(d, now); a != nil { - asks = append(asks, a) - } + // Proposed, with no verdict and no check asked: its check is asked at once (issue 1115), or the verdict that + // stands on its head at the forge is adopted. + if a := h.askChecks(now); a != nil { + asks = append(asks, a) } // Merged with no walk yet: tied to the newest walk heard that answers it — by name, by its own commit, or by @@ -1848,48 +1846,119 @@ func heldForGroup(d *Delivery, g *Group) bool { return d.State == Held && strings.HasPrefix(d.HeldWhy, fmt.Sprintf("its group %s's composed check", g.ID)) } -// askCheck is the ask of a proposed delivery's own check, when nothing else will bring its verdict: no verdict, -// no check this owner asked, and proposed past the grace the controller's own check of its announcement is -// given. Nil otherwise — once asked, a late verdict is the bound's and H2's to speak for. -func (h *Holder) askCheck(d *Delivery, now time.Time) func() { - if d.State != Proposed || d.Check != nil || d.CheckAsk != nil || d.MergedAs != "" || d.Number == 0 { +// standingVerdict is the passing verdict an open pull request's head holds at the forge, when every merge check and +// every context its base requires stands as success or warning; nil otherwise, a missing context included (issue 1115). +func standingVerdict(p OpenPull, now time.Time) *Verdict { + if p.Checks == nil { return nil } - if now.Sub(d.Since) < checkGrace && d.Ready == nil { // an Approve asked makes the check worth asking at once - h.lookAt(d.Since.Add(checkGrace), "a proposed delivery's check, asked once the grace has passed with no verdict") + contexts := slices.Clone(mergeChecks) + for _, c := range p.Required { + if !slices.Contains(contexts, c) { + contexts = append(contexts, c) + } + } + for _, c := range contexts { + if st := p.Checks[c].State; st != "success" && st != "warning" { + return nil + } + } + return &Verdict{At: now, Gate: verdictOfState(p.Checks["mesh/merge-gate"].State), + Summary: "the head's mesh/merge-gate, as the forge holds it", Repo: verdictOfState(p.Checks["mesh/repo-check"].State)} +} + +// askedCheck is whether a proposed delivery's own check stands asked: a kept ask with no id is none. +func askedCheck(d *Delivery) bool { return d.CheckAsk != nil && d.CheckAsk.ID != "" } + +// askChecks are the asks of proposed deliveries with no verdict, at each look, at once (issue 1115): the forge's open +// pull requests are read once, a passing verdict standing on a head is adopted (the delivery goes on to ready with no +// new check), and a head with no check asked is asked its check. Under the lock; the asks run outside it. +func (h *Holder) askChecks(now time.Time) func() { + type want struct { + id, owner, repo, head string + number int + member Member + ask bool + why string + } + var wants []want + for _, d := range h.deliveries { + if d.State != Proposed || d.Check != nil || d.MergedAs != "" || d.Number == 0 { + continue + } + w := want{id: d.ID, head: d.Commit, number: d.Number, member: MemberOf(d)} + w.owner, w.repo, _ = strings.Cut(d.Repository, "/") + if !askedCheck(d) { + if tried, was := h.askTried[d.ID]; was && now.Sub(tried) < time.Minute { + h.lookAt(tried.Add(time.Minute), "a proposed delivery's check that could not be asked") + } else { + w.ask = true + h.askTried[d.ID] = now + h.lookAt(now.Add(time.Minute), "a proposed delivery's check that could not be asked") + w.why = fmt.Sprintf("proposed for %s with no verdict and no check asked", now.Sub(d.Since).Round(time.Second)) + } + } + wants = append(wants, w) + } + if len(wants) == 0 { return nil } - if tried, was := h.askTried[d.ID]; was && now.Sub(tried) < time.Minute { - h.lookAt(tried.Add(time.Minute), "a proposed delivery's check that could not be asked") - return nil - } - h.askTried[d.ID] = now - h.lookAt(now.Add(time.Minute), "a proposed delivery's check that could not be asked") - id, m := d.ID, MemberOf(d) - why := fmt.Sprintf("proposed for %s with no verdict and no check asked: its announcement was heard from the "+ - "bus's history, or the controller missed it", now.Sub(d.Since).Round(time.Minute)) return func() { - asked, err := h.Controller.Check("", []Member{m}) - h.mu.Lock() - defer h.mu.Unlock() - if err != nil { - h.Logf("[mesh-delivery] %s's check cannot be asked yet: %v", id, err) - return + open, err := h.Forge.OpenPulls() + for _, w := range wants { + var standing *Verdict + if err == nil { + for _, p := range open { + if p.Number == w.number && strings.EqualFold(p.Owner, w.owner) && strings.EqualFold(p.Repo, w.repo) && + sameCommit(p.HeadSHA, w.head) { + standing = standingVerdict(p, h.Now()) + } + } + } + if standing != nil { + h.mu.Lock() + if d := h.deliveries[w.id]; d != nil && d.State == Proposed && d.Check == nil { + delete(h.askTried, w.id) + d.Check = standing + h.Logf("[mesh-delivery] %s: a passing verdict already stands on its head at the forge; adopted, no check asked", w.id) + h.settleAndKeep(d) + } + h.mu.Unlock() + continue + } + if w.ask { + h.askOwnCheck(w.id, w.member, w.why) + } } - delete(h.askTried, id) - d := h.deliveries[id] - if d == nil || d.State != Proposed || d.Check != nil { - return - } - d.CheckAsk = &CheckAsk{ID: asked, At: h.Now(), By: byOwner, Why: why} - h.Logf("[mesh-delivery] %s: its check asked of the controller as %s — %s", id, asked, why) - d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.Commit, Since: h.Now(), - Line: oneLine(fmt.Sprintf("%s delivery %s: its check asked by %s as %s: %s", h.Now().UTC().Format(time.RFC3339), - id, byOwner, asked, why))}) - h.keep(d) } } +// askOwnCheck asks the controller a proposed delivery's own check and keeps the ask. +func (h *Holder) askOwnCheck(id string, m Member, why string) { + asked, err := h.Controller.Check("", []Member{m}) + h.mu.Lock() + defer h.mu.Unlock() + if err != nil { + h.Logf("[mesh-delivery] %s's check cannot be asked yet: %v", id, err) + return + } + if asked == "" { + h.Logf("[mesh-delivery] %s's check was asked and the controller's answer held no id: taken as refused, asked again in a minute at most", id) + return + } + delete(h.askTried, id) + d := h.deliveries[id] + if d == nil || d.State != Proposed || d.Check != nil || askedCheck(d) { + return + } + d.CheckAsk = &CheckAsk{ID: asked, At: h.Now(), By: byOwner, Why: why} + h.Logf("[mesh-delivery] %s: its check asked of the controller as %s — %s", id, asked, why) + d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.Commit, Since: h.Now(), + Line: oneLine(fmt.Sprintf("%s delivery %s: its check asked by %s as %s: %s", h.Now().UTC().Format(time.RFC3339), + id, byOwner, asked, why))}) + h.keep(d) +} + // adopt makes a delivery of a walk no pull request is known for: running already — at the switch, or on the // controller's own path — it is delivering; waiting for a word, it waits for a person. func (h *Holder) adopt(w Walk) { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder_test.go b/modules/mesh-delivery/cmd/mesh-delivery/holder_test.go index 7fe4beef..3f1c841b 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder_test.go @@ -247,21 +247,21 @@ func TestATransitionIsKeptBeforeItIsSaid(t *testing.T) { } w.store.failing = false w.settleAll() - if len(w.said) != 1 || len(w.forge.notesOn(head)) != 1 { + if len(w.said) != 1 || len(w.forge.notesOn(head)) != 2 { t.Fatalf("once kept, said %d and noted %d", len(w.said), len(w.forge.notesOn(head))) } // A forge that is away is asked again; a note is appended once however often it is asked. w.forge.down = true w.h.Checked(verdict("novox/app", 7, head, "pass")) w.settleAll() - if len(w.forge.notesOn(head)) != 1 { + if len(w.forge.notesOn(head)) != 2 { t.Fatal("noted while the forge was away") } w.forge.down = false w.later(time.Hour) // each failure put its next try later: past every one of them w.settleAll() w.settleAll() - if notes := w.forge.notesOn(head); len(notes) != 3 { + if notes := w.forge.notesOn(head); len(notes) != 4 { t.Fatalf("after the forge came back the note has %d line(s): %q", len(notes), notes) } if d := w.delivery(IDOf("novox/app", head)); len(d.Owed) != 0 { @@ -364,18 +364,18 @@ func TestAGroupIsCheckedTogetherAndLetGoOnceEveryMemberIsMerged(t *testing.T) { } w.h.Checked(verdict("novox/mesh-controller", 1, ctlHead, "pass")) w.settleAll() - if len(w.ctl.checks) != 0 { + if len(composed(w.ctl.checks)) != 0 { t.Fatal("the group was checked composed before every member was ready") } w.h.Checked(verdict("novox/mesh-catalog", 2, catHead, "pass")) w.settleAll() - if len(w.ctl.checks) != 1 || w.ctl.checks[0] != "feat/x:"+cat+","+ctl { + if c := composed(w.ctl.checks); len(c) != 1 || c[0] != "feat/x:"+cat+","+ctl { t.Fatalf("the composed check asked %v, wanted the members by id", w.ctl.checks) } if state, _ := GroupState(w.h.groups["feat/x"], w.h.membersOf(w.h.groups["feat/x"])); state != "checking" { t.Fatalf("before its composed verdict the group is %s", state) } - w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-1", Verdict: "pass", Summary: "both compose"}) + w.h.Checked(CheckedEvent{Group: "feat/x", ID: w.h.groups["feat/x"].Check.Asked, Verdict: "pass", Summary: "both compose"}) w.settleAll() if state, _ := GroupState(w.h.groups["feat/x"], w.h.membersOf(w.h.groups["feat/x"])); state != "ready" { t.Fatalf("composed and ready, the group is %s", state) @@ -618,3 +618,14 @@ func TestGroupsNamesEachMembersPullRequestHeadBaseAndOpen(t *testing.T) { } } } + +// composed are the asks of a group's composed check among the checks asked: a member's own check has no group before its colon. +func composed(checks []string) []string { + var out []string + for _, c := range checks { + if !strings.HasPrefix(c, ":") { + out = append(out, c) + } + } + return out +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go b/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go index 413b58a1..05d44623 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go @@ -136,7 +136,6 @@ func TestAPullRequestWithItsApproveAskedHasItsCheckAskedOnTheReadyStream(t *test // One with no Approve asked is asked as before. w2 := newWorld(t) w2.h.PullUpdated(pr("novox/app", 8, pushed, "feat/y", "src/a.go")) - w2.later(checkGrace + 1) w2.settleAll() if len(w2.ctl.checkReady) == 0 || w2.ctl.checkReady[0] { t.Fatalf("the check of a pull request with no Approve asked was asked with ready %v", w2.ctl.checkReady) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index 1934f4ff..358d213c 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -32,18 +32,12 @@ import ( // The review record is read and put in the ask; it is never a gate (ADR 0421 §5). What this owner does is kept on the // delivery (Ready), said in its log, and the merge's transitions name the Approve as their warrant. -// mergeRetryAfter is how long after the merge tool refused a head's merge it is asked again, when nothing happened -// meanwhile that decides whether it may merge (a verdict, an Approve, a group's composed check are events, and ask it at -// once): a check it asked takes minutes, and a refusal costs the forge a few reads. A retry of a bounded step (askMerge). -const mergeRetryAfter = time.Minute +// readyRefusedFor is how long the merge tool may refuse an approved head before it is said as stalled: none (issue 1115) — +// a refusal is said at once, and the merge is asked again at each event that can change it, with no timer. +const readyRefusedFor time.Duration = 0 -// readyRefusedFor is how long the merge tool may refuse an approved head before it is said as stalled: longer than a -// check takes (the controller's checkTimeout is 45 minutes), so a check on its way is never said. -const readyRefusedFor = time.Hour - -// groupRefusedFor is how long the merge tool may refuse a closed group's member before it is said as stalled: the merge -// window of ADR 0418 §3, not the hour of a lone head, because the group's other members are already on the trunk. -const groupRefusedFor = 10 * time.Minute +// groupRefusedFor is how long the merge tool may refuse a closed group's member before it is said as stalled: none (issue 1115). +const groupRefusedFor time.Duration = 0 // groupClosed is whether a group is closed: a member reached the trunk. Under the lock. func (h *Holder) groupClosed(id string) bool { @@ -205,9 +199,8 @@ func (h *Holder) readyAsks(now time.Time) []func() { if waits != "" || r.merging { continue } - if !r.LastTry.IsZero() && now.Sub(r.LastTry) < mergeRetryAfter { - h.lookAt(r.LastTry.Add(mergeRetryAfter), "a merge the forge refused") - continue + if !r.LastTry.IsZero() { + continue // refused: asked again at the next event that decides whether it may merge (mergeAgainNow), with no timer } r.Tries++ r.LastTry, r.merging = now, true @@ -373,10 +366,9 @@ func (h *Holder) askMerge(id string, heads map[string]string) func() { r.RefusedSince = &now } if answer != r.Answer { - h.Logf("[mesh-delivery] %s is approved, and the merge tool did not merge it (asked again in %s): %s", id, - mergeRetryAfter, answer) + h.Logf("[mesh-delivery] %s is approved, and the merge tool did not merge it (asked again at the next verdict, Approve or composed check): %s", id, + answer) } - h.lookAt(now.Add(mergeRetryAfter), "a merge the forge refused") r.Answer = answer h.keep(d) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go index 49fd331c..c84a9286 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go @@ -67,7 +67,7 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { } } -func TestAMergeRefusedForAnHourIsSaid(t *testing.T) { +func TestAMergeRefusedIsSaidAtOnceAndAskedAgainOnlyOnAnEvent(t *testing.T) { w := newWorld(t) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) @@ -75,18 +75,21 @@ func TestAMergeRefusedForAnHourIsSaid(t *testing.T) { w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} w.h.SeeOpenPulls() w.settleAll() - w.later(30 * time.Second) + if lines := w.h.Stalled(); len(lines) != 1 || !strings.Contains(lines[0].Says, "refused it since") { + t.Fatalf("a merge refused is not said at once: %+v", lines) + } + w.later(time.Hour) w.settleAll() if len(w.forge.merges) != 1 { - t.Fatalf("asked again within the minute: %v", w.forge.merges) + t.Fatalf("asked again with no event: %v", w.forge.merges) } - w.later(readyRefusedFor) + again := w.delivery(IDOf("novox/app", readyHead)) + w.h.mu.Lock() + w.h.mergeAgainNow(again) // what a verdict, an Approve or a composed check does + w.h.mu.Unlock() w.settleAll() if len(w.forge.merges) != 2 { - t.Fatalf("not asked again after a minute: %v", w.forge.merges) - } - if lines := w.h.Stalled(); len(lines) != 1 || !strings.Contains(lines[0].Says, "refused it since") { - t.Fatalf("a mark refused for an hour is not said: %+v", lines) + t.Fatalf("not asked again on an event: %v", w.forge.merges) } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/table.go b/modules/mesh-delivery/cmd/mesh-delivery/table.go index d2e0b33e..b8ed6d08 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/table.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/table.go @@ -592,14 +592,9 @@ var Bounds = map[State]Bound{ Delivering: {For: 2 * time.Hour, H2: []Event{EvDone, EvSuperseded, EvFailed}, Says: "its walk runs"}, } -// checkGrace is how long a proposed delivery is left to the controller's own check of its announcement before -// this owner asks the check itself: longer than a check takes from announcement to verdict. A head adopted while -// it already waited — announced again from the bus's history — is asked once it has waited this long too. -const checkGrace = 15 * time.Minute - // boundSince is when a delivery's bound runs from: its state's start, except for a proposed delivery whose // check this owner asked — its bound is "asked and not answered", so it runs from the ask. One never asked -// runs from its start plus the grace before it is asked: an ask that cannot be made is still listed. +// runs from its start: an ask that cannot be made is still listed. func boundSince(d *Delivery) time.Time { if d.State == Proposed { if d.AskedAgain != nil && d.outOfDate() != "" { @@ -608,7 +603,7 @@ func boundSince(d *Delivery) time.Time { if d.CheckAsk != nil { return d.CheckAsk.At } - return d.Since.Add(checkGrace) + return d.Since } return d.Since } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go index 281994f2..648c88cd 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go @@ -27,27 +27,35 @@ func TestALookThatMeetsNothingWaitingOnTheClockWantsNoOtherLook(t *testing.T) { } } -func TestAProposedDeliveryWithNoVerdictHasItsCheckAskedWhenItsGraceEndsWithNoTick(t *testing.T) { +func TestAProposedDeliveryWithNoVerdictHasItsCheckAskedAtOnceWithNoGrace(t *testing.T) { w := newWorld(t) w.ctl.down = false w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) id := IDOf("novox/app", readyHead) w.h.React() - since := w.delivery(id).Since - if at, step := w.wantedAt(); !at.Equal(since.Add(checkGrace)) || !strings.Contains(step, "check") { - t.Fatalf("the look wanted at %s for %q; wanted when the grace ends, %s", at, step, since.Add(checkGrace)) - } - if w.delivery(id).CheckAsk != nil { - t.Fatal("the check was asked before the grace ended") - } - w.later(checkGrace + time.Second) - w.h.React() - if w.delivery(id).CheckAsk == nil { - t.Fatal("the look the grace's end wants did not ask the check") + if w.delivery(id).CheckAsk == nil || len(w.ctl.checks) != 1 { + t.Fatalf("the first look did not ask the check: %v", w.ctl.checks) } } -func TestAMergeTheForgeRefusedIsAskedAgainAfterAMinuteByTheTimerAlone(t *testing.T) { +func TestAProposedDeliveryWhoseHeadHoldsAPassingVerdictAtTheForgeAdoptsItAndAsksNoCheck(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + op := openPull("novox/app", 7, readyHead) + op.Checks = map[string]HeadCheck{"mesh/merge-gate": {State: "success"}, "mesh/repo-check": {State: "success"}} + w.forge.open = []OpenPull{op} + w.h.React() + w.settleAll() + if len(w.ctl.checks) != 0 { + t.Fatalf("a check was asked for a head whose passing verdict stands: %v", w.ctl.checks) + } + if d := w.delivery(id); d.Check == nil || !d.Check.Passes() || d.State != Ready { + t.Fatalf("the standing verdict was not adopted: %+v (%s)", d.Check, d.State) + } +} + +func TestAMergeTheForgeRefusedIsAskedAgainOnlyOnAnEventWithNoTimer(t *testing.T) { w := newWorld(t) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) @@ -58,12 +66,18 @@ func TestAMergeTheForgeRefusedIsAskedAgainAfterAMinuteByTheTimerAlone(t *testing if len(w.forge.merges) != 1 { t.Fatalf("merges %v", w.forge.merges) } - at, step := w.wantedAt() - if want := w.now.Add(mergeRetryAfter); !at.Equal(want) || !strings.Contains(step, "merge") { - t.Fatalf("the look wanted at %s for %q; wanted %s for the refused merge", at, step, want) + if at, step := w.wantedAt(); !at.IsZero() { + t.Fatalf("the refused merge wants a timer at %s for %q; it waits for the next event", at, step) } w.forge.refuse = nil - w.later(mergeRetryAfter + time.Second) + w.settleAll() + if len(w.forge.merges) != 1 { + t.Fatalf("asked again at a look with no event: %v", w.forge.merges) + } + again := w.delivery(IDOf("novox/app", readyHead)) + w.h.mu.Lock() + w.h.mergeAgainNow(again) + w.h.mu.Unlock() w.settleAll() if len(w.forge.merges) != 2 || w.delivery(IDOf("novox/app", readyHead)).Ready.Merged == nil { t.Fatalf("the refused merge was not asked again at the timer: %v", w.forge.merges) @@ -127,3 +141,71 @@ func TestAnAskTheControllerCouldNotAnswerIsAskedAgainAfterAWhileByTheTimerAlone( t.Fatalf("the look wanted at %s for %q; wanted %s for the plan that could not be asked", at, step, want) } } + +func TestAStandingGateWithNoRepoCheckIsNotAdoptedAndTheCheckIsAsked(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + op := openPull("novox/app", 7, readyHead) + op.Required = []string{"mesh/merge-gate", "mesh/repo-check"} + op.Checks = map[string]HeadCheck{"mesh/merge-gate": {State: "success"}} + w.forge.open = []OpenPull{op} + w.settleAll() + if d := w.delivery(id); d.Check != nil || d.State != Proposed { + t.Fatalf("a gate with no repo-check was adopted: %+v (%s)", d.Check, d.State) + } + if len(w.ctl.checks) != 1 { + t.Fatalf("its check was not asked: %v", w.ctl.checks) + } +} + +func TestAKeptAskWithNoIdIsNoAskAndAStandingVerdictIsAdopted(t *testing.T) { + w := newWorld(t) + w.op.auto = false + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + w.h.mu.Lock() + w.h.deliveries[id].CheckAsk = &CheckAsk{At: w.now, By: byOwner} + w.h.mu.Unlock() + op := openPull("novox/app", 7, readyHead) + op.Required = []string{"mesh/merge-gate", "mesh/repo-check"} + op.Checks = map[string]HeadCheck{"mesh/merge-gate": {State: "success"}, "mesh/repo-check": {State: "success"}} + w.forge.open = []OpenPull{op} + w.settleAll() + if d := w.delivery(id); d.State != Ready || d.Check == nil { + t.Fatalf("the standing verdict was not adopted over an ask with no id: %s", d.State) + } + if len(w.ctl.checks) != 0 || w.op.count() != 1 { + t.Fatalf("checks %v, Approves asked %d; wanted none and one", w.ctl.checks, w.op.count()) + } +} + +func TestAKeptAskWithNoIdAndNoStandingVerdictIsAskedAgainAtOnce(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + w.h.mu.Lock() + w.h.deliveries[id].CheckAsk = &CheckAsk{At: w.now, By: byOwner} + w.h.mu.Unlock() + w.settleAll() + if d := w.delivery(id); d.CheckAsk == nil || d.CheckAsk.ID == "" || len(w.ctl.checks) != 1 { + t.Fatalf("not asked again: %+v %v", d.CheckAsk, w.ctl.checks) + } +} + +func TestAnEmptyAnswerToACheckAskIsARefusalAskedAtMostOnceAMinute(t *testing.T) { + w := newWorld(t) + w.ctl.emptyCheck = true + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + id := IDOf("novox/app", readyHead) + w.settleAll() + w.settleAll() + if len(w.ctl.checks) != 1 || w.delivery(id).CheckAsk != nil || len(w.forge.notesOn(readyHead)) > 1 { + t.Fatalf("asked %v, ask kept %v; wanted one ask and none kept", w.ctl.checks, w.delivery(id).CheckAsk) + } + w.later(time.Minute) + w.settleAll() + if len(w.ctl.checks) != 2 { + t.Fatalf("not asked again after the minute: %v", w.ctl.checks) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go b/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go index da0f0cbf..b2999e7c 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go @@ -56,11 +56,11 @@ func TestANewlyAnnouncedHeadReadsTheForgeAndAKnownOneDoesNot(t *testing.T) { w := newWorld(t) before := w.forge.openCalls hear(t, w, "gitea.pull.updated", pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - if w.forge.openCalls != before+1 { - t.Fatalf("a new head read the forge %d times, wanted once", w.forge.openCalls-before) + if w.forge.openCalls != before+2 { // the unchecked heads, and the head's standing verdict before its check is asked (issue 1115) + t.Fatalf("a new head read the forge %d times, wanted twice", w.forge.openCalls-before) } hear(t, w, "gitea.pull.updated", pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - if w.forge.openCalls != before+1 { + if w.forge.openCalls != before+3 { // the look of a delivery awaiting its verdict reads its standing one t.Fatalf("the same head announced again read the forge again (%d reads)", w.forge.openCalls-before) } }