From 6bc16d74db5ab1057bc8a90dbae138e6521cd6e9 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 16:38:02 +0100 Subject: [PATCH] Write and take in a merge that leaves no module.json, so a module-less repository's reach is not held for ever and a removed last module is dropped (issue 1126) The forge bridge wrote no merged message for such a merge, so taken. never moved and the view held 'the indexer has not yet taken in ' (hq #490-#492). The bridge now writes it with no trees; the indexer drops every module of the repository on it, after the same order check as any merged message. The view needed no change: with the taken key moving it reaches the merge, which its new row proves. Needs mesh-sdk fix/1126-no-module-reach (a merged message may carry no trees). --- .../cmd/mesh-delivery-forge-bridge/main.go | 4 --- .../internal/bridge/bridge.go | 16 +++++----- .../internal/bridge/bridge_test.go | 14 +++++++-- .../internal/index/modules.go | 6 ++-- .../internal/index/modules_test.go | 30 +++++++++++++++++-- .../internal/view/reach_test.go | 14 ++++++++- 6 files changed, 64 insertions(+), 20 deletions(-) diff --git a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge/main.go b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge/main.go index deb2a428..c8bb3b00 100644 --- a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge/main.go +++ b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge/main.go @@ -74,10 +74,6 @@ func handle(ctx context.Context, m delivered, pub publisher, trees bridge.Trees, logger.Printf("%v; heard again in %s", err, retryDelay) _ = m.NakWithDelay(retryDelay) return - case errors.Is(err, bridge.ErrNoModules): - logger.Printf("%v; no merged message", err) - _ = m.Ack() - return case errors.Is(err, bridge.ErrNotHandled): logger.Printf("%v; nothing written", err) _ = m.Ack() diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go index c8cb1a32..774bc6b1 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go @@ -29,9 +29,6 @@ var ErrNotHandled = errors.New("event not handled") // heard again later. var ErrRetry = errors.New("the forge could not say") -// ErrNoModules says a merge into the trunk left a repository with no module.json anywhere: no merged message. -var ErrNoModules = errors.New("no module.json in the merge") - // Trunk is the branch a merged message is written for. const Trunk = "main" @@ -100,7 +97,9 @@ func unreadable(subject, why string, args ...any) error { // Convert turns one event of the forge module, by its bus subject and body, into a message. An event it cannot read // is an ErrUnreadable error naming the subject and why; an event it writes nothing for is ErrNotHandled. A merge into // the trunk asks trees once and becomes a merged message; a merge into any other branch is a closed pull message, as the -// pull request left its group. A forge that cannot say is ErrRetry; a merge with no module.json is ErrNoModules. +// pull request left its group. A forge that cannot say is ErrRetry. A merge that leaves no module.json anywhere is a merged message with no +// trees all the same (issue 1126): the indexer takes it in and drops the repository's modules, and the view reads the +// repository as reached at it. func Convert(subject string, data []byte, trees Trees, files Files) (Message, error) { return ConvertKept(subject, data, trees, files, nil) } @@ -294,9 +293,6 @@ func merged(subject string, p pull, trees Trees, files Files, kept Kept) (Messag if err != nil { return Message{}, fmt.Errorf("%w: %s: the trees of %s at the merge commit %s: %v", ErrRetry, subject, repository, p.MergeSHA, err) } - if len(dirs) == 0 { - return Message{}, fmt.Errorf("%w: %s: %s at the merge commit %s", ErrNoModules, subject, repository, p.MergeSHA) - } names := make([]string, 0, len(dirs)) for dir := range dirs { names = append(names, dir) @@ -339,7 +335,11 @@ func merged(subject string, p pull, trees Trees, files Files, kept Kept) (Messag if err != nil { return Message{}, unreadable(subject, "%v", err) } - return Message{Event: event, Data: body}, nil + msg := Message{Event: event, Data: body} + if len(dirs) == 0 { + msg.Said = fmt.Sprintf("%s at the merge commit %s holds no module.json: a merged message with no module directories", repository, p.MergeSHA) + } + return msg, nil } // openPull is an entry of gitea_open_pulls's answer (mesh-catalog modules/gitea/client.ts:369-381 openPulls, with the diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go index 95367380..3bd1116f 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go @@ -269,11 +269,19 @@ func TestTreesRefusedWritesNothingAndSaysWhy(t *testing.T) { // 1236/AC3 (the co } } -func TestNoModuleJSONAnywhereWritesNoMergedMessage(t *testing.T) { // 1236 (notes: a repository with no module.json) +// 1126 (note 94): a merge that leaves no module.json anywhere (hq, or a repository whose last module was removed) is a merged +// message with no trees, so the indexer takes it in and the repository's reach is not held for ever. +func TestNoModuleJSONAnywhereWritesAMergedMessageWithNoTrees(t *testing.T) { var calls []call msg, err := Convert(mergedSubject, mergedBody(nil), treesOf(&calls, "r0", map[string]string{}, nil), anyFiles) - if !errors.Is(err, ErrNoModules) || msg.Data != nil { - t.Errorf("err = %v data = %s", err, msg.Data) + if err != nil { + t.Fatal(err) + } + f, err := delivery.DecodeForge(msg.Data) + want := delivery.Forge{V: delivery.Version, Kind: delivery.ForgeMerged, Repository: "novox/mesh-delivery", Branch: "feat/x", Number: 12, + Head: head, Merge: merge, Root: "r0"} + if err != nil || msg.Event != "forge.novox.mesh-delivery.merged" || !delivery.SameForge(f, want) || !strings.Contains(msg.Said, "no module.json") { + t.Errorf("event %q message %+v said %q, %v; want a merged message with no trees", msg.Event, f, msg.Said, err) } } diff --git a/modules/mesh-delivery-indexer/internal/index/modules.go b/modules/mesh-delivery-indexer/internal/index/modules.go index a8cc9842..77dc7568 100644 --- a/modules/mesh-delivery-indexer/internal/index/modules.go +++ b/modules/mesh-delivery-indexer/internal/index/modules.go @@ -117,8 +117,10 @@ func (ix *Indexer) applyMoving(ctx context.Context, f delivery.Forge, drop bool, overlay[key] = next } // A module of this repository the merge commit holds no module.json for is gone from the trunk, and so is its key - // (issue 1900). A message that carries no manifests (an older writer) or one that cannot be read whole says nothing. - if drop && len(f.Manifests) > 0 && every { + // (issue 1900). A message with no trees says the merge commit holds no module.json at all, so every module of the + // repository goes (issue 1126 note 94: a repository that removed its last module). A message with trees but no + // manifests (an older writer) or one that cannot be read whole says nothing. + if drop && (len(f.Manifests) > 0 || len(f.Trees) == 0) && every { if err := ix.dropRemoved(ctx, f.Repository, held, func(key string, _ delivery.Module) bool { return named[key] }); err != nil { return w, err } diff --git a/modules/mesh-delivery-indexer/internal/index/modules_test.go b/modules/mesh-delivery-indexer/internal/index/modules_test.go index b75b5a8f..767f8746 100644 --- a/modules/mesh-delivery-indexer/internal/index/modules_test.go +++ b/modules/mesh-delivery-indexer/internal/index/modules_test.go @@ -219,6 +219,32 @@ func TestAnUnchangedModuleWritesNothing(t *testing.T) { // 1239 (unchanged value } return ix.Apply(context.Background(), mergedMsg(repo, three)) }}, + // Issue 1126 note 94: a merge that leaves no module.json anywhere (hq, or a repository whose last module was removed) is a + // merged message with no trees; it drops the repository's modules and moves its taken key, and an older one, heard late, + // goes through the same order check and drops nothing. + {name: "a merge that leaves no module.json drops every module and is taken in", writes: 3, commit: map[string]string{"a": "", "b": "", "c": ""}, taken: next, + said: "modules written: none", + then: func(ix *Indexer) error { + m := mergedMsg(repo, nil) + m.Merge = next + return ix.Apply(context.Background(), m) + }}, + {name: "an earlier merge with no module.json heard after a later one drops nothing", writes: 0, commit: map[string]string{"a": merge, "b": merge, "c": merge}, taken: next, + said: "the merged message is older than taken.novox/mesh-delivery at cccccccccccc", + then: func(ix *Indexer) error { + const earlier = "dddddddddddddddddddddddddddddddddddddddd" + ix.Contains = func(_ context.Context, _, commit, in string) (bool, error) { + return commit == in || commit == earlier && in == next, nil + } + later := mergedMsg(repo, three) + later.Merge = next + if err := ix.Apply(context.Background(), later); err != nil { + return err + } + gone := mergedMsg(repo, nil) + gone.Merge = earlier + return ix.Apply(context.Background(), gone) + }}, // Review round 2: a 502 whose words carry "404" inside a sha is no not-found; the late message is left for redelivery. {name: "a late message whose order the forge fails with a 502 writes nothing and is left for redelivery", writes: 1, commit: map[string]string{"a": next, "b": merge, "c": merge}, taken: next, err: true, said: "502 Bad Gateway", @@ -305,8 +331,8 @@ func TestAnUnchangedModuleWritesNothing(t *testing.T) { // 1239 (unchanged value if got := mods.writes - writes; got != tc.writes { t.Errorf("%d values written, want %d", got, tc.writes) } - for module, want := range tc.commit { - if m, ok := moduleOf(t, mods, module); !ok || m.Commit != want { + for module, want := range tc.commit { // "" is a module the merge deleted + if m, ok := moduleOf(t, mods, module); ok != (want != "") || m.Commit != want { t.Errorf("%s = %+v (%v), want commit %.8s", module, m, ok, want) } } diff --git a/modules/mesh-delivery-view/internal/view/reach_test.go b/modules/mesh-delivery-view/internal/view/reach_test.go index 236a1697..276049ce 100644 --- a/modules/mesh-delivery-view/internal/view/reach_test.go +++ b/modules/mesh-delivery-view/internal/view/reach_test.go @@ -13,7 +13,7 @@ import ( func TestTheViewDerivesHowFarEachRepositoryReached(t *testing.T) { const web, sdk = "novox/web", "novox/sdk" commitA, commitB, commitC := strings.Repeat("a", 40), strings.Repeat("b", 40), strings.Repeat("c", 40) - commitS, commitX := strings.Repeat("5", 40), strings.Repeat("e", 40) + commitS, commitX, commitD := strings.Repeat("5", 40), strings.Repeat("e", 40), strings.Repeat("d", 40) const fpMail = "src2:3333333333333333333333333333333333333333333333333333333333333333" mod := func(module, fp, commit string, src uint64, history bool) Entry { v, err := delivery.EncodeModule(delivery.Module{FP2: fp, Manifest: strings.Repeat("a", 64), Repository: web, Commit: commit, Src: src, Path: module}) @@ -84,6 +84,18 @@ func TestTheViewDerivesHowFarEachRepositoryReached(t *testing.T) { then: []Entry{taken(web, commitB)}}, {name: "a change whose source revision is not known yet holds no head the source named", reached: commitC, then: []Entry{mod("web", "src2:"+strings.Repeat("9", 64), commitX, 0, true), mod("web", "src2:"+strings.Repeat("9", 64), commitX, 0, false)}}, + // 1126 note 94: a merge that removes the repository's last module.json moves nothing; once the indexer dropped its + // modules and took the merge in, the repository reaches it, held by nothing, as the library with no module does. + {name: "a merge that removed the last module is reached at the merge the indexer took in", reached: commitD, + then: func() []Entry { + head := src(13, commitD) + current := head + current.History = false + gone := func(module string, history bool) Entry { + return Entry{Bucket: delivery.ModulesBucket, Key: module, Deleted: true, At: t0, History: history} + } + return []Entry{head, current, gone("web", true), gone("mail", true), gone("web", false), gone("mail", false), taken(web, commitD)} + }()}, // 1126: the view started while the bus did not answer its writes; its reach and its event are made at the next change it // takes in, though the derivation is the same, so the state reach never keeps an older have than node_haves says. {name: "a write and an event refused at the start are made at the next change", reached: commitC, refused: true,