From f1fbd91f0df446018bb818f8c929974a0535d62a Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 14:15:49 +0200 Subject: [PATCH] =?UTF-8?q?Say=20a=20refusal=20the=20gate=20is=20already?= =?UTF-8?q?=20answering=20as=20stalled=20only=20once=20its=20fresh=20check?= =?UTF-8?q?=20did=20not=20cure=20it=20(ADR=200505=20=C2=A74)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../cmd/mesh-delivery-gate/adr505_test.go | 98 +++++++++++++++++++ .../cmd/mesh-delivery-gate/issue1275_test.go | 36 +++---- .../cmd/mesh-delivery-gate/merges.go | 9 +- .../cmd/mesh-delivery-gate/ready.go | 28 +++++- 4 files changed, 151 insertions(+), 20 deletions(-) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go index ef3888ef..e2638bfc 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go @@ -1,8 +1,10 @@ package main import ( + "encoding/json" "os" "path/filepath" + "strings" "testing" "time" ) @@ -65,3 +67,99 @@ func TestTheLastMeasuredCheckTimeIsTheNewestVerdictsOfItsRepository(t *testing.T t.Fatalf("no verdict: %s", got) } } + +// refusedForAMovedTrunk is a lone delivery approved and refused by the merge tool because its trunk moved and its check +// was asked again (gitea merge.ts:147, errAskedAgain). +func refusedForAMovedTrunk(t *testing.T) (*world, string) { + t.Helper() + w := announced(t) + id := IDOf("novox/app", readyHead) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.forge.refuse = errAskedAgain + approve(w) + if len(w.forge.merges) != 1 || w.delivery(id).Ready.RefusedSince == nil { + t.Fatalf("the merge was not asked and refused: %v %+v", w.forge.merges, w.delivery(id).Ready) + } + return w, id +} + +func linesFor(w *world, id string) []StalledLine { + var out []StalledLine + for _, l := range w.h.Stalled() { + if l.ID == id { + out = append(out, l) + } + } + return out +} + +// ADR 0505 §4: a refusal because the trunk moved, whose check the gate asked again, is in the gate's own list at once +// but is no stalled line while its check is unanswered; once that check passed and the merge is refused again, it is. +func TestATrunkMovedRefusalIsStalledOnlyOnceItsFreshCheckAnsweredAndTheMergeIsRefusedAgain(t *testing.T) { + w, id := refusedForAMovedTrunk(t) + if got := linesFor(w, id); len(got) != 0 { + t.Fatalf("said as stalled while its fresh check runs: %+v", got) + } + shown, err := w.h.Show(id) + if err != nil { + t.Fatal(err) + } + if raw, _ := json.Marshal(shown); !strings.Contains(string(raw), "is not merged: its verdict was judged against trunk") { + t.Fatalf("the gate's own list does not show the refusal at once: %s", raw) + } + w.later(5 * time.Minute) + if got := linesFor(w, id); len(got) != 0 { + t.Fatalf("said as stalled at five minutes, its check unanswered: %+v", got) + } + // As on the live mesh (events_test.go): the trunk moved, the head is announced again, its fresh verdict passes + // against the new trunk, and the merge is asked and refused again, the trunk having moved once more. + w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.later(time.Second) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) + w.settleAll() + if len(w.forge.merges) != 2 { + t.Fatalf("the fresh verdict did not ask the merge again: %v", w.forge.merges) + } + if got := linesFor(w, id); len(got) != 1 { + t.Fatalf("its check answered and the merge refused again, yet no stalled line: %+v", got) + } +} + +// ADR 0505 §4: with no answer, the refusal is said at twice the repository's last check time, at least ten minutes. +func TestATrunkMovedRefusalIsStalledAtTwiceTheLastCheckTimeAtLeastTheFloor(t *testing.T) { + w, id := refusedForAMovedTrunk(t) + w.later(9 * time.Minute) + if got := linesFor(w, id); len(got) != 0 { + t.Fatalf("said before the floor: %+v", got) + } + w.later(2 * time.Minute) + if got := linesFor(w, id); len(got) != 1 { + t.Fatalf("not said past the floor of ten minutes: %+v", got) + } + + w, id = refusedForAMovedTrunk(t) + w.h.mu.Lock() + w.h.deliveries[id].Check.Took = "7m0s" // its repository's last check ran seven minutes: the bound is fourteen + w.h.mu.Unlock() + w.later(11 * time.Minute) + if got := linesFor(w, id); len(got) != 0 { + t.Fatalf("said at eleven minutes, under twice the last check time: %+v", got) + } + w.later(4 * time.Minute) + if got := linesFor(w, id); len(got) != 1 { + t.Fatalf("not said past twice the last check time: %+v", got) + } +} + +// ADR 0505 §4: any other refusal is said at once, as issue 1115 has it. +func TestAnyOtherRefusalIsStalledAtOnce(t *testing.T) { + w := announced(t) + id := IDOf("novox/app", readyHead) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.forge.refuse = refusedWords("novox/app#7") + approve(w) + if got := linesFor(w, id); len(got) != 1 { + t.Fatalf("a missing status refusal is not said at once: %+v", got) + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_test.go index 4530e52c..807c5270 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1275_test.go @@ -249,8 +249,9 @@ func TestAMemberBothRefusedAndHeldForAStatusIsOneStalledLine(t *testing.T) { } } -// 1275/AC6: a member refused because its verdict is out of date (the trunk moved; its fresh check runs) is a warning at -// once, and urgent only when the group is not merged within ten minutes; the pass stops at the refusal all the same. +// 1275/AC6, as novox/hq ADR 0505 §4 changed it: a member refused because its verdict is out of date (the trunk moved; its +// fresh check runs) is no stalled line while the gate answers it, and urgent once ten minutes passed with the group not +// merged; the pass stops at the refusal all the same. func TestAnOutOfDateRefusalIsAWarningAtOnceAndUrgentAfterTenMinutes(t *testing.T) { w := approvedGroup(t) allGreen(w) @@ -267,12 +268,12 @@ func TestAnOutOfDateRefusalIsAWarningAtOnceAndUrgentAfterTenMinutes(t *testing.T } return out } - if got := sev(); len(got) != 1 || got[0] != "warning" { - t.Fatalf("an out of date refusal is %v at once; wanted one warning", got) + if got := sev(); len(got) != 0 { + t.Fatalf("an out of date refusal is %v at once; wanted no line while it is answered", got) } w.later(9 * time.Minute) - if got := sev(); len(got) != 1 || got[0] != "warning" { - t.Fatalf("at nine minutes it is %v; wanted still a warning", got) + if got := sev(); len(got) != 0 { + t.Fatalf("at nine minutes it is %v; wanted still no line", got) } w.later(2 * time.Minute) if got := sev(); len(got) != 1 || got[0] != "urgent" { @@ -300,8 +301,8 @@ func severities(w *world) []string { return out } -// 1275/AC6 (b): the same out of date refusal on a LATER member, after the earlier one merged, is a warning at once and -// urgent after ten minutes: the half merged group does not say it urgent at once. +// 1275/AC6 (b), as ADR 0505 §4 changed it: the same out of date refusal on a LATER member, after the earlier one merged, +// is no line at once and urgent after ten minutes: the half merged group does not say it at once. func TestAnOutOfDateRefusalOnALaterMemberIsAWarningThenUrgent(t *testing.T) { w := approvedGroup(t) allGreen(w) @@ -310,8 +311,8 @@ func TestAnOutOfDateRefusalOnALaterMemberIsAWarningThenUrgent(t *testing.T) { w.settleAll() w.h.PullMerged(merged(gCat, 2, gHk, gT1)) w.settleAll() - if got := severities(w); len(got) != 1 || got[0] != "warning" { - t.Fatalf("a later member's out of date refusal is %v at once; wanted one warning", got) + if got := severities(w); len(got) != 0 { + t.Fatalf("a later member's out of date refusal is %v at once; wanted no line while it is answered", got) } w.later(11 * time.Minute) if got := severities(w); len(got) != 1 || got[0] != "urgent" { @@ -371,8 +372,9 @@ var mergeToolRefusals = []struct { "Nothing was merged", false}, } -// 1275/AC6 (e): only the merge tool's two out of date refusals (:121, :147) are warnings; the one saying its check could -// not be asked (:144) and every other is urgent at once. +// 1275/AC6 (e), as ADR 0505 §4 changed it: only the merge tool's two out of date refusals (:121, :147) are answered by +// the gate and said as no line at once; the one saying its check could not be asked (:144) and every other is urgent at +// once. func TestOnlyTheMergeToolsTwoOutOfDateRefusalsAreWarnings(t *testing.T) { for _, c := range mergeToolRefusals { if got := outOfDateRefusal(c.words); got != c.outOfDated { @@ -383,12 +385,12 @@ func TestOnlyTheMergeToolsTwoOutOfDateRefusalsAreWarnings(t *testing.T) { w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: " + c.words) w.op.answer(w.op.open()[0], OptionApprove) w.settleAll() - want := "urgent" - if c.outOfDated { - want = "warning" + got := severities(w) + if c.outOfDated && len(got) != 0 { + t.Errorf("%s: said %v at once, wanted no line while it is answered", c.line, got) } - if got := severities(w); len(got) != 1 || got[0] != want { - t.Errorf("%s: severity %v at once, wanted %s", c.line, got, want) + if !c.outOfDated && (len(got) != 1 || got[0] != "urgent") { + t.Errorf("%s: severity %v at once, wanted urgent", c.line, got) } } } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go index 4070027b..4075efff 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go @@ -284,9 +284,14 @@ func (h *Holder) ForgeRefused(f sdk.Forge) { r, now := d.Ready, h.Now() r.merging, r.refusedNoted = false, false answer := oneLine(f.Words) - if r.RefusedSince == nil { - r.RefusedSince = &now + var on time.Time + if d.Check != nil { + on = d.Check.At } + if r.RefusedSince == nil { + r.RefusedSince, r.FirstRefusedOn = &now, on + } + r.LastRefusedOn = on r.noteRefusal(d, answer) if !outOfDateRefusal(answer) { r.Urgent = true // never a warning again for this head diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go index 77737660..6463e0b3 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -35,7 +35,9 @@ import ( // delivery (Ready), said in its log, and the merge's transitions name the Approve as their warrant. // 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. +// a refusal is said at once, and the merge is asked again at each event that can change it, with no timer. A refusal the +// gate is already answering (its trunk moved and its check was asked again) is said only once that check answered and +// the merge is still refused, or twice the repository's last check time passed (novox/hq ADR 0505 §4, `answering`). const readyRefusedFor time.Duration = 0 // groupRefusedFor is how long the merge tool may refuse a closed group's member before it is said as stalled: none (issue 1115). @@ -92,6 +94,11 @@ type ReadyRun struct { RepeatTrunk string `json:"repeat_trunk,omitempty"` // RefusedSince is when the merge tool began refusing it without a break. RefusedSince *time.Time `json:"refused_since,omitempty"` + // FirstRefusedOn and LastRefusedOn are when the verdict the merge was asked on was heard, at the first refusal of + // the run of refusals and at the last: a later one than the first is a fresh check answered and refused again + // (novox/hq ADR 0505 §4). + FirstRefusedOn time.Time `json:"first_refused_on,omitzero"` + LastRefusedOn time.Time `json:"last_refused_on,omitzero"` // Merged is when the merge tool merged it. Merged *time.Time `json:"merged,omitempty"` // Failed is why it is not merged and will not be on this head: its check failed. Said once on the pull request. @@ -482,6 +489,9 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { Bound: "0s", H2: "none: a new head, which asks the operator again — the operator's", Says: fmt.Sprintf("%s#%d at %s is not merged: the operator's answer %s", d.Repository, d.Number, shortOf(r.Head, 12), r.Refused)}) + case h.answering(d, now): + // The trunk moved and its check was asked again: the gate is answering it, so it is no stall yet (ADR 0505 + // §4). `show` lists the refusal meanwhile. case r.RefusedSince != nil && d.Group != "" && h.groupClosed(d.Group) && now.Sub(*r.RefusedSince) >= groupRefusedFor: // A closed group's member (novox/hq ADR 0418 §3, issue 1082): its sibling merged, and its own merge is refused // again after each composed check. Said after the merge window, with the group and the tool's last answer. @@ -542,6 +552,22 @@ func neededOf(d *Delivery) string { return ". Needed: the refusal's cause is met (the merge is asked again at each verdict, status or Approve), or the operator decides." } +// answering is whether an approved head the merge tool refused is one the gate is already answering, and so not said as +// stalled yet (novox/hq ADR 0505 §4): refused only because its trunk moved, its check asked again, and neither has that +// check answered with the merge refused again after it, nor has twice the repository's last check time (at least the +// floor) passed since the refusals began. Any other refusal of this head, and a refusal repeated past its limit, is said +// at once (r.Urgent). Under the lock. +func (h *Holder) answering(d *Delivery, now time.Time) bool { + r := d.Ready + if r == nil || r.RefusedSince == nil || r.Urgent || !outOfDateRefusal(r.Answer) { + return false + } + if r.LastRefusedOn.After(r.FirstRefusedOn) { + return false // its fresh check answered, and the merge was refused again on it + } + return now.Sub(*r.RefusedSince) < h.answeredRefusalBound(d.Repository) +} + // outOfDateFor is how long a refusal for an out of date verdict is a warning before it is urgent (issue 1275, AC6). const outOfDateFor = 10 * time.Minute