From 588cfd54bb149388f24455d55d843317857250ff Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 25 Oct 2026 01:18:20 +0200 Subject: [PATCH] mesh-delivery-indexer: keep a merged member in its group's candidate until every member merged, so the heads the Approve covers hold and no new check starts for the members left (issue 1921, issue 1791, try speed S9) --- modules/mesh-delivery-indexer/go.mod | 2 +- modules/mesh-delivery-indexer/go.sum | 2 + .../internal/index/candidates.go | 23 ++++-- .../internal/index/index_test.go | 71 ++++++++++++++++++- 4 files changed, 91 insertions(+), 7 deletions(-) diff --git a/modules/mesh-delivery-indexer/go.mod b/modules/mesh-delivery-indexer/go.mod index 0aaff15..5f87256 100644 --- a/modules/mesh-delivery-indexer/go.mod +++ b/modules/mesh-delivery-indexer/go.mod @@ -3,7 +3,7 @@ module mesh-delivery-indexer go 1.26.0 require ( - git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261023150321-a2c813a6b468 + git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261024231716-571ac521f053 github.com/nats-io/nats-server/v2 v2.11.17 github.com/nats-io/nats.go v1.54.0 ) diff --git a/modules/mesh-delivery-indexer/go.sum b/modules/mesh-delivery-indexer/go.sum index 2a3e9f3..c1bfa2e 100644 --- a/modules/mesh-delivery-indexer/go.sum +++ b/modules/mesh-delivery-indexer/go.sum @@ -1,5 +1,7 @@ git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261023150321-a2c813a6b468 h1:UcMee+IV2ii5E4dRZdQUaSaeh9IedTFYumuQbBiB0k0= git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261023150321-a2c813a6b468/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY= +git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261024231716-571ac521f053 h1:8Kbp3aJG3jGndDOfmGdMSDgJVEcc1hyu921hVnnlnlQ= +git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261024231716-571ac521f053/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY= github.com/antithesishq/antithesis-sdk-go v0.7.0-default-no-op h1:Z/MZK75wC/NSrkgqeNIa7jexam9uWzhLmFTSCPI/kn0= github.com/antithesishq/antithesis-sdk-go v0.7.0-default-no-op/go.mod h1:FQyySiasQQM8735Ddel3MRojmy4dA1IqCeyJ5jmPMbI= github.com/google/go-tpm v0.9.8 h1:slArAR9Ft+1ybZu0lBwpSmpwhRXaa85hWtMinMyRAWo= diff --git a/modules/mesh-delivery-indexer/internal/index/candidates.go b/modules/mesh-delivery-indexer/internal/index/candidates.go index 7a14378..b9f9f21 100644 --- a/modules/mesh-delivery-indexer/internal/index/candidates.go +++ b/modules/mesh-delivery-indexer/internal/index/candidates.go @@ -71,7 +71,7 @@ func (ix *Indexer) logf(format string, args ...any) { } // Apply takes one message of the bridge's stream FORGE into the group of its branch. A pull message adds or moves the -// pull request, a closed one or a merged message takes it out; a group left with none is deleted. A change is written +// pull request, a closed one takes it out, a merged one marks it merged; a group left with none open is deleted. A change is written // by compare-and-set on the revision read, and read again on a conflict. A lone pull request is a group of one. A // merged message also writes the fingerprint of each module of its repository (modules.go). func (ix *Indexer) Apply(ctx context.Context, f delivery.Forge) error { @@ -154,12 +154,13 @@ func (ix *Indexer) applyOpen(ctx context.Context, f delivery.Forge) error { return nil, false, fmt.Errorf("candidates.%s holds what is not a candidate: %w", key, err) } kept := slices.DeleteFunc(slices.Clone(c.Pulls), func(p delivery.CandidatePull) bool { - return !named[delivery.OpenPull{Repository: p.Repository, Number: p.Number}] + // a member that merged is open no more, and stays while another member is (issue 1791) + return !p.Merged && !named[delivery.OpenPull{Repository: p.Repository, Number: p.Number}] }) switch { case len(kept) == len(c.Pulls): return nil, false, nil - case len(kept) == 0: + case !anyOpen(kept): ix.logf("%s: no pull request of the group is open, so no candidate", key) return nil, true, nil } @@ -232,7 +233,16 @@ func (ix *Indexer) next(cur *delivery.Candidate, f delivery.Forge, digests map[s if at < 0 { return cur } - pulls = slices.Delete(pulls, at, at+1) + if f.Kind == delivery.ForgeMerged { + // A member that merges stays, marked merged, with the head it merged at: the heads hash the group's Approve + // covers holds, so the checker starts no new check for the members still open (issue 1791; issue 1921 S9). + pulls[at].Merged = true + } else { + pulls = slices.Delete(pulls, at, at+1) + } + if !anyOpen(pulls) { + return nil // every member merged or closed: the group is done + } } else { entry := delivery.CandidatePull{Head: delivery.Head{Repository: f.Repository, Number: f.Number, Head: f.Head}, Modules: []string{}} switch { @@ -276,6 +286,11 @@ func (ix *Indexer) next(cur *delivery.Candidate, f delivery.Forge, digests map[s return &delivery.Candidate{V: delivery.Version, Group: f.Branch, Base: base, HeadsHash: hash, Pulls: pulls, FPSet: delivery.FingerprintSet(pulls)} } +// anyOpen says whether a group's pull requests hold one that has not merged. +func anyOpen(pulls []delivery.CandidatePull) bool { + return slices.ContainsFunc(pulls, func(p delivery.CandidatePull) bool { return !p.Merged }) +} + // NoneNoPreview is why a candidate's touched module has no fingerprint of the new form: the merge preview is not // computed in this release (novox/hq ADR 0465 clause 3; the checker comes with issue 1445). const NoneNoPreview = "no merge preview is computed" diff --git a/modules/mesh-delivery-indexer/internal/index/index_test.go b/modules/mesh-delivery-indexer/internal/index/index_test.go index 74bcc57..3ba9707 100644 --- a/modules/mesh-delivery-indexer/internal/index/index_test.go +++ b/modules/mesh-delivery-indexer/internal/index/index_test.go @@ -206,8 +206,8 @@ func TestDirsNotKnownKeepTheModulesOfTheSameHead(t *testing.T) { // 1238/AC4 } } -func TestClosedAndMergedTakeAPullOutAndTheLastDeletesTheKey(t *testing.T) { // 1238/AC5 - for _, how := range []string{"closed", "merged"} { +func TestClosedTakesAPullOutAndTheLastDeletesTheKey(t *testing.T) { // 1238/AC5 (a merge: TestAMergedMemberStays...) + for _, how := range []string{"closed"} { ix, st, _ := newIndexer(t) ctx := context.Background() _ = ix.Apply(ctx, pull("novox/mesh-delivery", 12, "feat/x", h1, "modules/a")) @@ -387,3 +387,70 @@ func TestAnOpenMessageDropsEveryCandidateItDoesNotName(t *testing.T) { // 1279/A t.Errorf("after an empty open message the candidates hold %v", st.vals) } } + +// 1791 (issue 1921, try speed S9): a member of a group that merges while another is open stays in the candidate, marked +// merged, with its head: the heads hash the Approve covers holds, so no new check starts for the members left. An open +// message does not drop it, a close of the last open member deletes the group, and the last merge does too. +func TestAMergedMemberStaysInItsGroupUntilEveryMemberMerged(t *testing.T) { + merged := func(repo string, n int, head string) delivery.Forge { + return delivery.Forge{V: delivery.Version, Kind: delivery.ForgeMerged, Repository: repo, Branch: "feat/x", Number: n, Head: head, Merge: h3, Root: "r", Trees: map[string]string{"a": "t"}} + } + ix, st, _ := newIndexer(t) + ctx := context.Background() + for _, f := range []delivery.Forge{pull("novox/mesh-delivery", 12, "feat/x", h1, "modules/a"), pull("novox/mesh-catalog", 300, "feat/x", h2, "modules/c"), + pull("novox/mesh-sdk", 7, "feat/x", h3)} { + if err := ix.Apply(ctx, f); err != nil { + t.Fatal(err) + } + } + before, _ := read(t, st, "feat/x") + if err := ix.Apply(ctx, merged("novox/mesh-sdk", 7, h3)); err != nil { + t.Fatal(err) + } + if err := ix.Apply(ctx, merged("novox/mesh-delivery", 12, h1)); err != nil { + t.Fatal(err) + } + c, ok := read(t, st, "feat/x") + if !ok || len(c.Pulls) != 3 || c.HeadsHash != before.HeadsHash { + t.Fatalf("after two merges the candidate = %+v (kept %v), want the three members and the same heads hash", c, ok) + } + for _, p := range c.Pulls { + if want := p.Repository != "novox/mesh-catalog"; p.Merged != want { + t.Errorf("%s#%d merged = %v, want %v", p.Repository, p.Number, p.Merged, want) + } + } + // The open message names only the member still open: the merged ones stay. + if err := ix.Apply(ctx, open(delivery.OpenPull{Repository: "novox/mesh-catalog", Number: 300})); err != nil { + t.Fatal(err) + } + if c, _ := read(t, st, "feat/x"); len(c.Pulls) != 3 || c.HeadsHash != before.HeadsHash { + t.Errorf("the open message dropped a merged member: %+v", c) + } + // The last open member merges: the group is done. + if err := ix.Apply(ctx, merged("novox/mesh-catalog", 300, h2)); err != nil { + t.Fatal(err) + } + if _, ok := read(t, st, "feat/x"); ok { + t.Error("every member merged and the group is still a candidate") + } + // A close of the last open member deletes the group too. + ix, st, _ = newIndexer(t) + _ = ix.Apply(ctx, pull("novox/mesh-delivery", 12, "feat/x", h1, "modules/a")) + _ = ix.Apply(ctx, pull("novox/mesh-catalog", 300, "feat/x", h2, "modules/c")) + _ = ix.Apply(ctx, merged("novox/mesh-delivery", 12, h1)) + closed := pull("novox/mesh-catalog", 300, "feat/x", h2) + closed.Action = delivery.PullClosed + if err := ix.Apply(ctx, closed); err != nil { + t.Fatal(err) + } + if _, ok := read(t, st, "feat/x"); ok { + t.Error("the last open member closed and the group, holding only a merged one, is still a candidate") + } + // A lone pull request that merges leaves no group. + ix, st, _ = newIndexer(t) + _ = ix.Apply(ctx, pull("novox/mesh-delivery", 12, "feat/x", h1, "modules/a")) + _ = ix.Apply(ctx, merged("novox/mesh-delivery", 12, h1)) + if _, ok := read(t, st, "feat/x"); ok { + t.Error("a lone merged pull request is still a candidate") + } +}