diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue888_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue888_test.go new file mode 100644 index 00000000..725029ff --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue888_test.go @@ -0,0 +1,134 @@ +package main + +import ( + "testing" + "time" + + sdk "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// Issue 888: the checker judges on the trunk source.main names (mesh-delivery modules/mesh-delivery-checker main.go), and the +// gate moved it only for a merge it handed to the forge bridge (finish). A merge made at the forge directly moved every other +// delivery's trunk (trunkMoved) and left source.main, so the checker kept judging on the trunk before it. + +const ( + i888Other = "novox/other" + i888OtherH = "0000000000000000000000000000000000000001" + i888App = "novox/app" + i888Head1 = "1111111111111111111111111111111111111111" + i888Head2 = "2222222222222222222222222222222222222222" + i888Merge1 = "3333333333333333333333333333333333333333" + i888Merge2 = "4444444444444444444444444444444444444444" +) + +func i888MergedAt(repo string, number int, head, merge string, at time.Time) PullEvent { + p := merged(repo, number, head, merge) + p.MergedAt = at.UTC().Format(time.RFC3339) + return p +} + +// 888/AC1: a merge into main the gate did not hand is heard from the forge: source.main holds it, and keeps the others. +func TestAMergeHeardOnlyFromTheForgeMovesSourceMain(t *testing.T) { + w := newWorld(t) + w.source.main = &sdk.Source{V: sdk.Version, Group: "earlier", Heads: map[string]string{i888Other: i888OtherH}} + w.h.PullUpdated(pr(i888App, 3, i888Head1, "fix/x", "internal/a.go")) + w.settleAll() + w.h.PullMerged(i888MergedAt(i888App, 3, i888Head1, i888Merge1, w.now.Add(-time.Minute))) + w.settleAll() + if w.source.main == nil || w.source.main.Heads[i888App] != i888Merge1 || w.source.main.Heads[i888Other] != i888OtherH { + t.Fatalf("source.main is %+v; wanted %s at %s and %s kept", w.source.main, i888App, i888Merge1, i888Other) + } + n := len(w.source.moves) + w.settleAll() + w.h.PullMerged(i888MergedAt(i888App, 3, i888Head1, i888Merge1, w.now.Add(-time.Minute))) // heard again + w.settleAll() + if len(w.source.moves) != n || n != 1 { + t.Fatalf("source.main moved %d then %d times; wanted once", n, len(w.source.moves)) + } +} + +// 888/AC2: a group the gate merged through the bridge moves source.main once, in finish, though the forge's word of each +// member's merge is heard too. +func TestAGroupTheGateMergedMovesSourceMainOnceThoughTheForgeSaysItsMerges(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + w.settleAll() + if len(w.forge.merges) != 2 || len(w.source.moves) != 1 { + t.Fatalf("merges %v, source.main moved %d times", w.forge.merges, len(w.source.moves)) + } + commit := w.source.moves[0].Heads[gCat] + w.h.PullMerged(i888MergedAt(gCtl, 1, gHc, w.source.moves[0].Heads[gCtl], w.now)) + w.h.PullMerged(i888MergedAt(gCat, 2, gHk, commit, w.now)) + w.settleAll() + if len(w.source.moves) != 1 { + t.Fatalf("source.main moved %d times; wanted once, by the group: %+v", len(w.source.moves), w.source.moves) + } +} + +// 888/AC3: an older merge heard after a newer one, or replayed as history, never moves source.main backwards. +func TestAnOlderMergeHeardLaterNeverMovesSourceMainBack(t *testing.T) { + w := newWorld(t) + w.h.PullMerged(i888MergedAt(i888App, 4, i888Head2, i888Merge2, w.now.Add(-time.Minute))) + w.settleAll() + w.h.PullMerged(i888MergedAt(i888App, 3, i888Head1, i888Merge1, w.now.Add(-2*time.Minute))) + w.settleAll() + if w.source.main == nil || w.source.main.Heads[i888App] != i888Merge2 || len(w.source.moves) != 1 { + t.Fatalf("source.main is %+v after %d moves; wanted the newer merge %s, moved once", w.source.main, len(w.source.moves), i888Merge2) + } + + h := newWorld(t) + h.source.main = &sdk.Source{V: sdk.Version, Group: "now", Heads: map[string]string{i888App: i888Merge2}} + h.h.PullMerged(i888MergedAt(i888App, 3, i888Head1, i888Merge1, h.now.Add(-4*24*time.Hour))) + h.settleAll() + if h.source.main.Heads[i888App] != i888Merge2 || len(h.source.moves) != 0 { + t.Fatalf("a merge replayed as history moved source.main to %+v", h.source.main) + } +} + +// 888/AC4: an approved group whose members were merged at the forge before the gate handed them moves source.main once. +func TestAnApprovedGroupMergedAtTheForgeMovesSourceMainOnce(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.op.answer(w.op.open()[0], OptionApprove) + mc, mk := "5555555555555555555555555555555555555555", "6666666666666666666666666666666666666666" + w.h.PullMerged(i888MergedAt(gCtl, 1, gHc, mc, w.now)) + w.h.PullMerged(i888MergedAt(gCat, 2, gHk, mk, w.now)) + w.settleAll() + w.settleAll() + if len(w.source.moves) != 1 || w.source.main.Heads[gCtl] != mc || w.source.main.Heads[gCat] != mk { + t.Fatalf("source.main moved %d times to %+v; wanted once, both members at their merge commits", len(w.source.moves), w.source.main) + } +} + +// 888/AC5 (review round 1): an older merge heard from the forge after the bridge merged a newer one, whose own pull.merged is +// not heard yet, never moves source.main back; nor does one older than the gate keeps records for, over a head it does not know. +func TestAnOlderForgeMergeNeverOverwritesANewerBridgeMergeOrAnUnknownHead(t *testing.T) { + w := approvedGroup(t) + allGreen(w) + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + w.settleAll() + if len(w.source.moves) != 1 { + t.Fatalf("the group did not move source.main once: %d", len(w.source.moves)) + } + bridged := w.source.main.Heads[gCat] + w.h.PullUpdated(pr(gCat, 7, "7777777777777777777777777777777777777777", "fix/old", "modules/app/module.json")) + w.h.PullMerged(i888MergedAt(gCat, 7, "7777777777777777777777777777777777777777", "8888888888888888888888888888888888888888", w.now.Add(-10*time.Minute))) + w.settleAll() + if w.source.main.Heads[gCat] != bridged || len(w.source.moves) != 1 { + t.Fatalf("an older forge merge moved source.main from the bridge's %s to %s", bridged, w.source.main.Heads[gCat]) + } + + o := newWorld(t) + o.source.main = &sdk.Source{V: sdk.Version, Group: "pruned", Heads: map[string]string{i888App: i888Merge2}} + at := o.now + o.h.PullUpdated(pr(i888App, 3, i888Head1, "fix/x", "internal/a.go")) + o.later(keptFinal + 24*time.Hour) + o.h.PullMerged(i888MergedAt(i888App, 3, i888Head1, i888Merge1, at.Add(time.Minute))) + o.settleAll() + if o.source.main.Heads[i888App] != i888Merge2 || len(o.source.moves) != 0 { + t.Fatalf("a merge older than the records kept overwrote a head the gate does not know: %+v", o.source.main) + } +} 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 4075efff..01692ce6 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go @@ -346,7 +346,8 @@ func (h *Holder) settleMerges() []func() { } } }) - case r.Merged != nil && !r.SourceMoved && !done[key]: + case (r.Merged != nil || d.MergedAs != "") && !r.SourceMoved && !done[key]: + // Merged through the bridge, or at the forge directly after the Approve (issue 888): the group is moved once. if !h.allMerged(r.Heads) { continue } @@ -420,3 +421,100 @@ func (h *Holder) finish(name, key string, approved map[string]string, last strin } h.Logf("[mesh-delivery] group %s is merged whole: source.main holds %s", name, strings.Join(members, ", ")) } + +// gateMerges says whether finish moves source.main for a delivery: one the operator approved through an ask, whose merge the +// gate hands to the forge bridge. +func gateMerges(d *Delivery) bool { + return d.Ready != nil && d.Ready.Approved != nil && d.Ready.AskID != "" && len(d.Ready.Heads) > 0 +} + +// settleForgeMerges moves source.main for each merge into the trunk heard only from the forge (issue 888): a pull request +// merged at the forge directly, not handed by the gate on an Approve. Every other delivery's trunk moved with it (trunkMoved), +// and the checker judges on the trunk source.main names, so source.main moves too. A merge replayed as history, or older than +// a merge of its repository already heard, is never written: source.main never moves backwards. Under the lock; the writing +// happens outside it. +func (h *Holder) settleForgeMerges() []func() { + var out []func() + for _, d := range h.deliveries { + if d.MergedAs == "" || d.InSource || gateMerges(d) { + continue + } + if d.Base != MergeBranch || d.History != "" || h.newerMergeOf(d) { + d.InSource = true // not the trunk, or not its head: nothing to write + h.keep(d) + continue + } + id, repository, base, commit, at, group := d.ID, d.Repository, d.Base, d.MergedAs, d.MergedAt, firstOf(d.Branch, d.Repository) + out = append(out, func() { + err := h.Source.Move(func(prev *sdk.Source) (sdk.Source, bool) { + next := sdk.Source{Group: group, Heads: map[string]string{repository: commit}} + if prev != nil { + held := prev.Heads[repository] + if sameCommit(held, commit) { + return next, false // there already: the bridge's word and the forge's are one merge + } + if held != "" && !h.olderHead(repository, base, held, at) { + return next, false // source.main holds a newer merge, or one the gate cannot date: never moved back + } + for r, head := range prev.Heads { + if r != repository { + next.Heads[r] = head + } + } + } + return next, true + }) + h.mu.Lock() + defer h.mu.Unlock() + if err != nil { + h.Logf("[mesh-delivery] %s merged at the forge, and its place in the source cannot be written yet: %v; asked again in %s", id, err, askAgainAfter) + h.again("source.main, moved for a merge heard from the forge") + return + } + if m := h.deliveries[id]; m != nil { + m.InSource = true + h.keep(m) + } + h.Logf("[mesh-delivery] %s merged at the forge: source.main holds %s at %s", id, repository, shortOf(commit, 12)) + }) + } + return out +} + +// mergeOf is a delivery's merge into its trunk as the gate knows it: the forge's word (MergedAs), or else the bridge's +// (Ready.MergeCommit, Ready.Merged), which comes first for a merge the gate handed. Empty when it is not merged. +func mergeOf(d *Delivery) (string, time.Time) { + if d.MergedAs != "" { + return d.MergedAs, d.MergedAt + } + if d.Ready != nil && d.Ready.Merged != nil && d.Ready.MergeCommit != "" { + return d.Ready.MergeCommit, *d.Ready.Merged + } + return "", time.Time{} +} + +// newerMergeOf says whether a merge into the same trunk of d's repository was heard that landed after d's. +func (h *Holder) newerMergeOf(d *Delivery) bool { + for _, o := range h.deliveries { + if commit, at := mergeOf(o); o != d && o.Repository == d.Repository && o.Base == d.Base && commit != "" && + !sameCommit(commit, d.MergedAs) && at.After(d.MergedAt) { + return true + } + } + return false +} + +// olderHead says whether the head source.main holds for a repository is a merge the gate knows landed before at, so a merge +// at that time may take its place. A head it does not know (written before its records, or pruned with its delivery) is +// older only when at is within the time final records are kept: a pruned merge ended before then (issue 888, review). +// Takes the lock. +func (h *Holder) olderHead(repository, base, head string, at time.Time) bool { + h.mu.Lock() + defer h.mu.Unlock() + for _, o := range h.deliveries { + if commit, when := mergeOf(o); o.Repository == repository && o.Base == base && commit != "" && sameCommit(commit, head) { + return !when.After(at) + } + } + return at.After(h.Now().Add(-keptFinal)) +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go index 5eb6cef3..32ccda2a 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go @@ -84,7 +84,10 @@ type Delivery struct { Others []PullEvent `json:"others,omitempty"` MergedAs string `json:"merged_as,omitempty"` MergedAt time.Time `json:"merged_at"` - HeldWhy string `json:"held_why,omitempty"` + // InSource says the merge was looked at for source.main: written there, or left as older than the trunk's head (issue 888). + // A merge the gate handed to the bridge on an Approve is moved there by finish instead (merges.go). + InSource bool `json:"in_source,omitempty"` + 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). 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 ca9c08b2..bda03ce7 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -267,6 +267,7 @@ func (h *Holder) readyAsks(now time.Time) []func() { } } asks = append(asks, h.settleMerges()...) + asks = append(asks, h.settleForgeMerges()...) asks = append(asks, h.withdrawnKeys()...) asks = append(h.cancels, asks...) h.cancels = nil