diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/configheld.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/configheld.go new file mode 100644 index 00000000..3d40e5bc --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/configheld.go @@ -0,0 +1,146 @@ +package main + +import ( + "fmt" + "regexp" + "strconv" + "strings" +) + +// A config value held or dropped for a pull request asks its check again (novox/hq ADR 0502 clause 5, issue 1826). +// +// The controller's composed check lays the approved values held for a pull request (mesh-config's `held`) when it +// composes, so a check run before a value was held, or after it was dropped, judged another mesh. mesh-config +// raises `config-changed` with the action hold or drop and the pull request; this owner then asks that pull +// request's check, or its group's composed check, again, the way it asks a moved head: its own path, with the +// reason in the delivery's transition, and not the `recheck` verb, which stays an exception (ADR 0421 §12). + +// ConfigChangedEvent is the event this owner follows from mesh-config. +const ConfigChangedEvent = "mesh-config.config-changed" + +// ConfigChange is mesh-config's `config-changed` as far as this owner reads it. +type ConfigChange struct { + Action string `json:"action"` + Pull string `json:"pull,omitempty"` + Head string `json:"head,omitempty"` + Key string `json:"key,omitempty"` + Level struct { + Node string `json:"node,omitempty"` + Module string `json:"module,omitempty"` + } `json:"level"` +} + +var pullNamed = regexp.MustCompile(`^([A-Za-z0-9_.-]+)/([A-Za-z0-9_.-]+)#([0-9]+)$`) + +// ConfigChanged takes a hold or a drop of a value for a pull request: every live delivery of that pull request has its +// check asked again. Anything else mesh-config says (a value set, a resolution, the merge's set) asks nothing. +func (h *Holder) ConfigChanged(c ConfigChange) { + if c.Action != "hold" && c.Action != "drop" { + return + } + m := pullNamed.FindStringSubmatch(strings.TrimSpace(c.Pull)) + if m == nil { + return + } + number, _ := strconv.Atoi(m[3]) + repository := strings.ToLower(m[1] + "/" + m[2]) + verb := "held for" + if c.Action == "drop" { + verb = "dropped from" + } + why := fmt.Sprintf("a config value was %s this pull request: %s of %s on %s (mesh-config)", verb, c.Key, + orWord(c.Level.Module, "a module"), orWord(c.Level.Node, "a node")) + h.mu.Lock() + var ids []string + for id, d := range h.deliveries { + if d.Number == number && strings.EqualFold(d.Repository, repository) && d.MergedAs == "" && !d.State.Final() { + ids = append(ids, id) + } + } + h.mu.Unlock() + for _, id := range ids { + h.askAgainForConfig(id, why) + } +} + +func orWord(s, word string) string { + if s == "" { + return word + } + return s +} + +// askAgainForConfig asks one delivery's check again: a rejected or ready one is proposed again, as any judged again +// head is; a proposed one has its pending ask replaced. Its group's composed check goes out of date, and is asked +// once a member is judged again. +func (h *Holder) askAgainForConfig(id, why string) { + h.mu.Lock() + d := h.deliveries[id] + if d == nil { + h.mu.Unlock() + return + } + probe := *d + probe.Transitions = append([]Transition(nil), d.Transitions...) + _, refused := Apply(&probe, EvRecheck, true, Facts{Now: h.Now(), Why: why, By: byOwner}) + proposed := d.State == Proposed + h.mu.Unlock() + switch { + case refused == nil: + if _, err := h.Recheck(id, why, byOwner); err != nil { + h.Logf("[mesh-delivery] %s: its check could not be asked again for a config value: %v", id, err) + } + case proposed: + h.askProposedAgain(id, why) + } +} + +// askProposedAgain asks a proposed delivery's check again, its merge check put back to pending first. +func (h *Holder) askProposedAgain(id, why string) { + h.mu.Lock() + d := h.deliveries[id] + if d == nil || d.State != Proposed { + h.mu.Unlock() + return + } + owner, repo, commit, page, number := d.Owner(), d.Repo(), d.Commit, d.HTMLURL, d.Number + h.mu.Unlock() + if err := h.checkingAgain(owner, repo, number, commit, page, why); err != nil { + h.Logf("[mesh-delivery] %s: its check is not asked again for a config value: %v", id, err) + return + } + h.mu.Lock() + d = h.deliveries[id] + if d == nil || d.State != Proposed { + h.mu.Unlock() + return + } + d.Check = nil + if g := h.groups[d.Group]; g != nil && g.Check != nil { + now := h.Now() + g.Check.OutOfDate = fmt.Sprintf("composed check out of date: %s was rechecked (%s)", d.ID, why) + g.Check.OutOfDateAt = now + g.Updated = now + h.keepGroup(g) + } + m := MemberOf(d) + h.mu.Unlock() + 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 again yet (%v); it is asked at the next look", id, err) + h.again("a check asked again for a config value that could not be asked") + return + } + d = h.deliveries[id] + if d == nil || d.State != Proposed { + return + } + d.CheckAsk = &CheckAsk{ID: asked, At: h.Now(), By: byOwner, Why: why} + h.Logf("[mesh-delivery] %s: its check asked again 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 again by %s as %s: %s", h.Now().UTC().Format("2006-01-02T15:04:05Z07:00"), + id, byOwner, asked, why))}) + h.keep(d) +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1826_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1826_test.go new file mode 100644 index 00000000..04cf8bf6 --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1826_test.go @@ -0,0 +1,145 @@ +package main + +import ( + "encoding/json" + "strings" + "testing" + + stdio "git.novox.be/novox/mesh-sdk/go" +) + +// Issue 1826 (novox/hq ADR 0502 clause 5): when mesh-config holds or drops a value for a pull request that is a +// delivery, its check, or its group's composed check, is asked again, and the transition says why. + +func configChanged(action, pull string) ConfigChange { + c := ConfigChange{Action: action, Pull: pull, Head: head, Key: "media-gid"} + c.Level.Node, c.Level.Module = "ace", "n8n" + return c +} + +// 1826/K4: a lone delivery that was ready is proposed again with its merge check pending, its check asked, and the +// transition says that a config value was held. +func TestAHeldValueAsksTheReadyDeliverysCheckAgain(t *testing.T) { + w := newWorld(t) + id := heldGreen(t, w, "novox/app", 7) + asked := len(w.ctl.checks) + w.h.ConfigChanged(configChanged("hold", "novox/app#7")) + if got := w.ctl.checks[asked:]; len(got) != 1 || got[0] != ":"+id { + t.Fatalf("asked %v; wanted its own check once", got) + } + d := w.delivery(id) + last := d.Transitions[len(d.Transitions)-1] + if d.State != Proposed || last.Event != EvRecheck || !strings.Contains(last.Why, "media-gid") || + !strings.Contains(last.Why, "held") || last.By != byOwner { + t.Errorf("state %s, last transition %+v", d.State, last) + } + if s := w.forge.statuses["novox/app@"+head+" "+ctxGate]; !strings.HasPrefix(s, "pending checking again") { + t.Errorf("the merge check says %q: the old verdict would still let it merge", s) + } +} + +// 1826/K4: a rejected delivery (the very case: the composed check failed on the missing value) is asked again on a +// hold; and a drop asks it again too. +func TestAHeldValueAsksARejectedDeliverysCheckAgainAndADropToo(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 8, head, "feat/y", "src/a.go")) + w.h.Checked(verdict("novox/app", 8, head, "fail")) + w.settleAll() + id := IDOf("novox/app", head) + if w.state(id) != Rejected { + t.Fatalf("it is %s", w.state(id)) + } + asked := len(w.ctl.checks) + w.h.ConfigChanged(configChanged("hold", "novox/app#8")) + if w.state(id) != Proposed || len(w.ctl.checks) != asked+1 { + t.Fatalf("a hold left it %s with %v asked", w.state(id), w.ctl.checks[asked:]) + } + w.h.Checked(verdict("novox/app", 8, head, "pass")) + w.settleAll() + asked = len(w.ctl.checks) + w.h.ConfigChanged(configChanged("drop", "novox/app#8")) + d := w.delivery(id) + if len(w.ctl.checks) != asked+1 || !strings.Contains(d.Transitions[len(d.Transitions)-1].Why, "dropped") { + t.Errorf("a drop asked %v; last transition %+v", w.ctl.checks[asked:], d.Transitions[len(d.Transitions)-1]) + } +} + +// 1826/K4: a delivery whose check is on its way is asked again, its old ask replaced, and says why in its note. +func TestAHeldValueAsksTheCheckOfAProposedDeliveryAgain(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 9, head, "feat/z", "src/a.go")) + w.settleAll() + id := IDOf("novox/app", head) + before := w.delivery(id).CheckAsk + asked := len(w.ctl.checks) + w.h.ConfigChanged(configChanged("hold", "novox/app#9")) + d := w.delivery(id) + if len(w.ctl.checks) != asked+1 || d.CheckAsk == nil || (before != nil && d.CheckAsk.ID == before.ID) || + !strings.Contains(d.CheckAsk.Why, "held") { + t.Errorf("asked %v; ask %+v (was %+v)", w.ctl.checks[asked:], d.CheckAsk, before) + } + if d.State != Proposed { + t.Errorf("it is %s", d.State) + } +} + +// 1826/K4: a hold for a member of a group asks the member's check again and then the group's composed check, which +// the group waits for. +func TestAHeldValueForAGroupMemberAsksTheComposedCheckAgain(t *testing.T) { + w := approvedGroup(t) + g := w.h.groups["feat/g"] + firstComposed := g.Check.Asked + asked := len(w.ctl.checks) + w.h.ConfigChanged(configChanged("hold", gCat+"#2")) + if got := w.ctl.checks[asked:]; len(got) != 1 || got[0] != ":"+IDOf(gCat, gHk) { + t.Fatalf("a hold asked %v; wanted the member's own check", got) + } + if g = w.h.groups["feat/g"]; g.Check.OutOfDate == "" || !strings.Contains(g.Check.OutOfDate, "rechecked") { + t.Fatalf("the composed check stands: %+v", g.Check) + } + w.later(60e9) + w.h.Checked(judged(gCat, 2, gHk, "pass", gTrunk0)) + w.settleAll() + last := w.ctl.checks[len(w.ctl.checks)-1] + if !strings.HasPrefix(last, "feat/g:") || w.h.groups["feat/g"].Check.Asked == firstComposed { + t.Errorf("the composed check was not asked again: %v", w.ctl.checks[asked:]) + } +} + +// 1826/K4: what is not a hold or a drop for a delivery of this owner asks nothing: the merge's set, another action, +// another pull request, a pull request with no delivery, and a delivery already merged. +func TestOnlyAHoldOrADropForADeliveryAsksACheckAgain(t *testing.T) { + w := newWorld(t) + heldGreen(t, w, "novox/app", 7) + asked := len(w.ctl.checks) + for _, c := range []ConfigChange{configChanged("set", "novox/app#7"), configChanged("resolve", ""), + configChanged("hold", "novox/app#99"), configChanged("hold", "novox/other#7"), configChanged("hold", ""), + configChanged("hold", "garbage")} { + w.h.ConfigChanged(c) + } + if len(w.ctl.checks) != asked || w.state(IDOf("novox/app", head)) != Ready { + t.Errorf("asked %v; it is %s", w.ctl.checks[asked:], w.state(IDOf("novox/app", head))) + } +} + +// 1826/K4: the event is heard as mesh-config's config-changed, and it is no recheck verb. +func TestTheConfigChangedEventIsFollowed(t *testing.T) { + w := newWorld(t) + heldGreen(t, w, "novox/app", 7) + found := false + for _, f := range followed { + found = found || f == "mesh-config.config-changed" + } + if !found { + t.Fatalf("not followed: %v", followed) + } + raw, _ := json.Marshal(map[string]any{"action": "hold", "pull": "novox/app#7", "head": head, "key": "media-gid", + "level": map[string]any{"level": "module", "node": "ace", "module": "n8n"}}) + asked := len(w.ctl.checks) + if err := handle(w.h, stdio.Envelope{Key: "mesh-config.config-changed", Body: raw}); err != nil { + t.Fatal(err) + } + if len(w.ctl.checks) != asked+1 { + t.Errorf("the event asked %v", w.ctl.checks[asked:]) + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go index 1a5ae466..03855f7c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go @@ -34,7 +34,7 @@ func (l *listening) get() string { l.mu.Lock(); defer l.mu.Unlock(); return l.n // The forge's status events say that a check answered (events.go). var followed = []string{"gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed", "gitea.forge.status", "gitea.forge.pull-request.edited", "gitea.forge.pull-request.closed", ControllerSeat + ".checked", ControllerSeat + ".plan-moved", - forgePattern} + forgePattern, ConfigChangedEvent} // forgePattern is the forge bridge's messages on FORGE (novox/hq ADR 0437 §8): the gate follows a merge and the refusal of one. const forgePattern = "mesh-delivery-forge-bridge.forge.**" @@ -187,6 +187,13 @@ func take(h *Holder, e stdio.Envelope) error { } h.Checked(c) h.KeepVerdict(c) + case ConfigChangedEvent: + var c ConfigChange + if err := json.Unmarshal(e.Body, &c); err != nil { + h.Refuse(fmt.Errorf("a config change is not readable: %v", err)) + return nil + } + h.ConfigChanged(c) case ControllerSeat + ".plan-moved": var w Walk if err := json.Unmarshal(e.Body, &w); err != nil { diff --git a/modules/mesh-delivery-gate/module.json b/modules/mesh-delivery-gate/module.json index ec838521..a80fad26 100644 --- a/modules/mesh-delivery-gate/module.json +++ b/modules/mesh-delivery-gate/module.json @@ -32,7 +32,8 @@ "gitea.forge.pull-request.closed", "mesh-controller.checked", "mesh-controller.plan-moved", - "mesh-delivery-forge-bridge.forge.**" + "mesh-delivery-forge-bridge.forge.**", + "mesh-config.config-changed" ], "emits": [ "transition",