From 2b801f5bd49a710203944af416a8bf641231f7f6 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 11:18:01 +0100 Subject: [PATCH] indexer_take_in reads a held module again only when its record is behind the source's head, and the two new tests are rows (issue 1126, review) A head older than the record, or one whose order the forge cannot say, would have written an older manifest back. The new tests become rows of existing ones: the repository is over its 500 unit tests. --- .../internal/bridge/files_test.go | 4 +- .../internal/index/registered.go | 16 ++++ .../internal/index/registered_test.go | 81 ++++++++++++------- 3 files changed, 71 insertions(+), 30 deletions(-) diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go b/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go index aaecb3d..3e88564 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go @@ -120,6 +120,8 @@ func TestMergedCarriesManifestsAndLocks(t *testing.T) { // 1448/AC1 if _, err := ConvertWith(mergedSubject, catalogue, treesOf(&calls, "r0", map[string]string{"modules/a": "t1"}, nil), forge.get, failing); !errors.Is(err, ErrRetry) { t.Errorf("a listing the forge cannot give: %v", err) } + // Issue 1126: a merge reads the forge only for the module directories whose tree changed. + t.Run("a merge reads only the directories whose tree changed", mergeReadsOnlyChangedDirs) } func TestPullWithKnownDirsCarriesTheSameAtTheHead(t *testing.T) { // 1448/AC2 @@ -378,7 +380,7 @@ func (k memKept) Put(tree string, in Inputs) { k[tree] = in } // Issue 1126 (mesh-catalog#535; issue 1458 met the kind first): a merge reads the forge only for the module directories // whose tree changed. Its message is the same as one read whole; a directory whose tree is kept costs no read. -func TestAMergeReadsOnlyTheDirectoriesWhoseTreeChanged(t *testing.T) { +func mergeReadsOnlyChangedDirs(t *testing.T) { manifest, sum := real(t, "module.json"), real(t, "go.sum") forge := &forgeFiles{have: map[string][]byte{"modules/a/module.json": manifest, "modules/a/go.sum": sum, "modules/b/module.json": []byte(`{"module":"b"}`)}} kept := memKept{} diff --git a/modules/mesh-delivery-indexer/internal/index/registered.go b/modules/mesh-delivery-indexer/internal/index/registered.go index 0a78a95..7fe4c28 100644 --- a/modules/mesh-delivery-indexer/internal/index/registered.go +++ b/modules/mesh-delivery-indexer/internal/index/registered.go @@ -313,6 +313,22 @@ func (ix *Indexer) takeInHeld(ctx context.Context, module string) (TakenIn, bool if head == "" || head == cur.Commit { return out, false } + // Only forward: a record at a merge the source does not name yet (a merge applied before the source moved) is never + // written back to the source's older head. Where the order cannot be known, nothing is read. + behind, why := false, "the forge cannot be asked" + if ix.Contains != nil { + var err error + if behind, err = ix.Contains(ctx, cur.Repository, cur.Commit, head); err != nil { + why = err.Error() + } else if !behind { + why = fmt.Sprintf("its record at %.12s is not in %.12s", cur.Commit, head) + } + } + if !behind { + out.Held, out.Repository, out.Commit, out.Path = true, cur.Repository, cur.Commit, cur.Path + out.Answer = fmt.Sprintf("held; the source names another head, %.12s, and not one its record is behind (%s), so it is not read again", head, why) + return out, true + } t, err := ix.indexAt(ctx, Registration{Module: module, Repository: cur.Repository, Path: cur.Path, Commit: head}, true, true) out.Held, out.Repository, out.Commit, out.Path = true, t.repository, t.commit, pathOf(t.dir) switch { diff --git a/modules/mesh-delivery-indexer/internal/index/registered_test.go b/modules/mesh-delivery-indexer/internal/index/registered_test.go index 6e3ac1e..879c7e5 100644 --- a/modules/mesh-delivery-indexer/internal/index/registered_test.go +++ b/modules/mesh-delivery-indexer/internal/index/registered_test.go @@ -111,6 +111,58 @@ func TestARegisteredModuleIsIndexedFromItsRepository(t *testing.T) { // the gran } }) } + + // Issue 1126: mesh-catalog#535 changed mesh-vault, but its merged message never reached the indexer, so the record + // stood at the merge before. A take-in of the held module reads it again at the source's head, only when the record is + // behind that head: a head older than the record (a merge applied before the source moved), or one whose order the forge + // cannot say, is never written back. + for _, tc := range []struct { + name string + contains func(context.Context, string, string, string) (bool, error) + moved bool + }{ + {name: "a held module behind the source's head is read again", moved: true, + contains: func(_ context.Context, _, commit, in string) (bool, error) { return commit == merge && in == h2, nil }}, + {name: "a source head older than the record is not read", contains: func(context.Context, string, string, string) (bool, error) { return false, nil }}, + {name: "a forge that cannot say the order reads nothing", contains: func(context.Context, string, string, string) (bool, error) { + return false, errors.New("timeout") + }}, + {name: "no forge to ask reads nothing"}, + } { + t.Run("take-in: "+tc.name, func(t *testing.T) { + ctx := context.Background() + ix, mods := modulesIndexer(t) + ix.Manifests = newMem() + if err := ix.Apply(ctx, mergedMsg("novox/mesh-catalog", map[string]string{"modules/a": "treeA"})); err != nil { + t.Fatal(err) + } + ix.source = sourceNow{rev: 2, heads: map[string]string{"novox/mesh-catalog": h2}} + ix.Contains = tc.contains + var asked []string + ix.Read = func(_ context.Context, _, sha, dir string) (RepoRead, error) { + asked = append(asked, sha+":"+dir) + return RepoRead{Root: "r1", Trees: map[string]string{"modules/a": "treeA2"}, Manifests: map[string]string{"modules/a": `{"module":"a","version":"2"}`}}, nil + } + got := ix.TakeIn(ctx, "a") + m, ok := moduleOf(t, mods, "a") + if !tc.moved { + if got.Stored || !got.Held || len(asked) != 0 || !strings.Contains(got.Answer, "another head") || !ok || m.Commit != merge { + t.Errorf("take-in answered %+v, asked %v, modules.a at %.8s", got, asked, m.Commit) + } + return + } + if !got.Stored || !got.Held || got.Commit != h2 || got.Path != "modules/a" || !strings.Contains(got.Answer, "written") { + t.Errorf("take-in answered %+v", got) + } + if !ok || m.Commit != h2 || m.Inputs2 == nil || m.Inputs2.Tree != "treeA2" { + t.Errorf("modules.a = %+v (%v), want it at %.8s with tree treeA2 (asked %v)", m, ok, h2, asked) + } + n := len(asked) // at the head now: a take-in answers held and reads nothing + if again := ix.TakeIn(ctx, "a"); again.Stored || !again.Held || len(asked) != n { + t.Errorf("a take-in of a module at the source's head answered %+v and asked %v", again, asked) + } + }) + } } func mustModule(t *testing.T, m delivery.Module) []byte { @@ -256,32 +308,3 @@ func TestARecordOfAPullRequestDoesNotReplaceTheRegisteredRef(t *testing.T) { // t.Fatalf("registrations = %+v", got) } } - -// Issue 1126: mesh-catalog#535 changed mesh-vault, but its merged message never reached the indexer, so the record stood at -// the merge before. A take-in of the held module reads it again at the head the gate's source names and writes it. -func TestATakeInOfAHeldModuleBehindTheSourceHeadReadsItAgain(t *testing.T) { - ctx := context.Background() - ix, mods := modulesIndexer(t) - ix.Manifests = newMem() - if err := ix.Apply(ctx, mergedMsg("novox/mesh-catalog", map[string]string{"modules/a": "treeA"})); err != nil { - t.Fatal(err) - } - ix.source = sourceNow{rev: 2, heads: map[string]string{"novox/mesh-catalog": h2}} - var asked []string - ix.Read = func(_ context.Context, _, sha, dir string) (RepoRead, error) { - asked = append(asked, sha+":"+dir) - return RepoRead{Root: "r1", Trees: map[string]string{"modules/a": "treeA2"}, Manifests: map[string]string{"modules/a": `{"module":"a","version":"2"}`}}, nil - } - got := ix.TakeIn(ctx, "a") - if !got.Stored || !got.Held || got.Commit != h2 || got.Path != "modules/a" || !strings.Contains(got.Answer, "written") { - t.Errorf("take-in answered %+v", got) - } - if m, ok := moduleOf(t, mods, "a"); !ok || m.Commit != h2 || m.Inputs2 == nil || m.Inputs2.Tree != "treeA2" { - t.Errorf("modules.a = %+v (%v), want it at %.8s with tree treeA2 (asked %v)", m, ok, h2, asked) - } - // At the head now: a take-in answers held and reads nothing. - n := len(asked) - if again := ix.TakeIn(ctx, "a"); again.Stored || !again.Held || len(asked) != n { - t.Errorf("a take-in of a module at the source's head answered %+v and asked %v", again, asked) - } -}