From 00232c71977be18fe2534822f95de6f4f9be1c84 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 8 Nov 2026 11:50:52 +0100 Subject: [PATCH] The indexer writes no module whose bytes it already holds (issue 1966, recovery) Every recompute wrote every key of its modules state again with identical bytes: recomputeLocked compared the package versions with reflect.DeepEqual across two map types (the contract's map[string]string and sdkstep.PackageVersions), which never match. Each write was a views event, and a reader of the views took each as a gap and asked the whole first view again. The comparison is by maps.Equal now, and write itself skips an answer whose bytes are the bytes held, for every caller and every state it writes; the merge's written list counts only real writes. Claude-Session: https://claude.ai/code/session_01NcbgfFfQ4Z9UE5jK32PmE5 --- .../internal/index/candidates.go | 12 ++++- .../internal/index/toolchains.go | 16 +++++-- .../internal/index/toolchains_test.go | 44 +++++++++++++++---- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/modules/mesh-delivery-indexer/internal/index/candidates.go b/modules/mesh-delivery-indexer/internal/index/candidates.go index bb003573..6eaa3cb7 100644 --- a/modules/mesh-delivery-indexer/internal/index/candidates.go +++ b/modules/mesh-delivery-indexer/internal/index/candidates.go @@ -1,6 +1,7 @@ package index import ( + "bytes" "cmp" "context" "errors" @@ -387,6 +388,10 @@ func (ix *Indexer) applyOpen(ctx context.Context, f forgemsg.Forge) error { // write changes one key of a store by compare-and-set: compute is given the value read (found says there is one) and // answers the new value, or del to remove the key, or neither for no change. A conflict reads again, up to writeTries. // read, when given, is set to the revision of the value compute was last given. +// +// **A value whose bytes are the bytes held is not written** (issue 1966, 2026-11-08: a compare of two map types that never +// matched rewrote every module on every recompute, each write a views event, and a reader of the views took each as a gap +// and asked its whole first view again): whatever compute decides, a write that changes nothing but the revision is none. func write(ctx context.Context, st Store, key string, compute func(value []byte, found bool) (next []byte, del bool, err error), read ...*uint64) error { for try := 0; try < writeTries; try++ { value, revision, found, err := st.Get(ctx, key) @@ -401,7 +406,7 @@ func write(ctx context.Context, st Store, key string, compute func(value []byte, *r = revision } switch { - case next == nil && !del: + case next == nil && !del, unchanged(next, del, value, found): return nil case del: err = st.Delete(ctx, key, revision) @@ -421,6 +426,11 @@ func write(ctx context.Context, st Store, key string, compute func(value []byte, return fmt.Errorf("%s changed under every one of %d tries", key, writeTries) } +// unchanged says that compute's answer next is the value held: write writes nothing then (write). +func unchanged(next []byte, del bool, held []byte, found bool) bool { + return found && !del && next != nil && bytes.Equal(next, held) +} + // next is the group after the message; nil when the group is left with no pull request. // // A pull message that carries manifests (issue 1448) gives each module it touches, in the pull request's Fps, the digest of diff --git a/modules/mesh-delivery-indexer/internal/index/toolchains.go b/modules/mesh-delivery-indexer/internal/index/toolchains.go index 0ab8bc43..738427a5 100644 --- a/modules/mesh-delivery-indexer/internal/index/toolchains.go +++ b/modules/mesh-delivery-indexer/internal/index/toolchains.go @@ -324,7 +324,7 @@ func (ix *Indexer) recomputeLocked(ctx context.Context, overlay map[string]meshc finals[key] = final continue } - if !stale[key] && k.cur.Fp2 == r.fp && k.cur.None2 == r.none && reflect.DeepEqual(k.cur.Toolchains, r.tcs) && reflect.DeepEqual(k.cur.Packages, r.pkgs) { + if !stale[key] && k.cur.Fp2 == r.fp && k.cur.None2 == r.none && sameUsed(k.cur, r.tcs, r.pkgs) { continue } digest, inputs, paths := k.cur.Manifest, k.cur.Inputs2, k.cur.Paths @@ -337,7 +337,7 @@ func (ix *Indexer) recomputeLocked(ctx context.Context, overlay map[string]meshc if err != nil || cur.Manifest != digest || cur.Inputs2 == nil { return nil, false, nil // a merge changed it meanwhile; its own recompute follows } - if cur.Fp2 == r.fp && cur.None2 == r.none && reflect.DeepEqual(cur.Toolchains, r.tcs) && reflect.DeepEqual(cur.Packages, r.pkgs) { + if cur.Fp2 == r.fp && cur.None2 == r.none && sameUsed(cur, r.tcs, r.pkgs) { return nil, false, nil } if cur.Fp2 != r.fp { @@ -350,7 +350,7 @@ func (ix *Indexer) recomputeLocked(ctx context.Context, overlay map[string]meshc cur.Fp2, cur.None2, cur.Toolchains, cur.Packages = r.fp, r.none, r.tcs, r.pkgs cur.Inputs2, cur.Paths = inputs, paths data, err := meshcatalogclient.EncodeModule(cur) - wrote = err == nil + wrote = err == nil && !unchanged(data, false, value, found) return data, false, err }) if err != nil { @@ -408,7 +408,7 @@ func (ix *Indexer) writeOverlay(ctx context.Context, overlay, finals map[string] w.taken(next.Manifest) // a new module, or a changed manifest: fresh, judged in full (ADR 0579 ยง1) } data, err := meshcatalogclient.EncodeModule(next) - wrote = err == nil + wrote = err == nil && !unchanged(data, false, value, found) return data, false, err }) if err != nil { @@ -469,6 +469,14 @@ func (ix *Indexer) rejudged(ctx context.Context, m catalogue.Manifest, cur meshc return cur, true } +// sameUsed says whether the toolchains and the package versions a recompute gave are the ones the value holds. The value's +// package versions are the contract's map and the recompute's are sdkstep.PackageVersions: compared as maps, never by +// reflect.DeepEqual, which holds two map types unequal whatever they hold (issue 1966: every module was written again, with +// the same bytes, at every recompute). A nil map and an empty one are the same, as the value's encoding has it. +func sameUsed(cur meshcatalog.ModulesByModuleValue, tcs map[string]meshcatalog.ToolchainUsed, pkgs sdkstep.PackageVersions) bool { + return maps.Equal(cur.Toolchains, tcs) && maps.Equal(cur.Packages, map[string]string(pkgs)) +} + // hasContext is whether an artifact of the manifest reads another repository's tree. func hasContext(m catalogue.Manifest) bool { if m.Build == nil { diff --git a/modules/mesh-delivery-indexer/internal/index/toolchains_test.go b/modules/mesh-delivery-indexer/internal/index/toolchains_test.go index 1e783939..762649d9 100644 --- a/modules/mesh-delivery-indexer/internal/index/toolchains_test.go +++ b/modules/mesh-delivery-indexer/internal/index/toolchains_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "maps" "reflect" "slices" "strings" @@ -378,14 +379,41 @@ func TestABuildHeardBeforeAnyToolchainIsReadDropsNoFingerprint(t *testing.T) { / } } -func TestAToolchainEntryChangedToTheSameValuesWritesNothing(t *testing.T) { // 1444/AC9 - ix, mods := newFormIndexer(t, builderModules(t)...) - reportToolchain(t, ix, "mesh-build-go", "novox", "go", "go1.27.1") - before := mods.writes - time.Sleep(2 * time.Millisecond) - reportToolchain(t, ix, "mesh-build-go", "novox", "go", "go1.27.1") // the same version and recipe, a later time - if mods.writes != before { - t.Errorf("modules written %d times after the same values, want none", mods.writes-before) +func TestAToolchainEntryChangedToTheSameValuesWritesNothing(t *testing.T) { // 1444/AC9; 1966: nothing moved writes nothing + ctx := context.Background() + for _, c := range []struct { + name string + do func(t *testing.T, ix *Indexer, mods *memStore) + }{ + {"the same version and recipe, a later time", func(t *testing.T, ix *Indexer, _ *memStore) { + time.Sleep(2 * time.Millisecond) + reportToolchain(t, ix, "mesh-build-go", "novox", "go", "go1.27.1") + }}, + // Issue 1966 (2026-11-08): the package versions compared by reflect.DeepEqual across two map types never matched, so + // every recompute wrote every module again with the bytes it held, each write a views event. + {"a recompute with nothing moved", func(t *testing.T, ix *Indexer, _ *memStore) { + if err := ix.Recompute(ctx); err != nil { + t.Fatal(err) + } + }}, + // write itself: an answer whose bytes are the bytes held is no write, whoever computed it. + {"a compute answering the bytes held", func(t *testing.T, ix *Indexer, mods *memStore) { + for _, key := range slices.Sorted(maps.Keys(mods.vals)) { + if err := write(ctx, ix.Modules, key, func(value []byte, _ bool) ([]byte, bool, error) { return slices.Clone(value), false, nil }); err != nil { + t.Fatal(err) + } + } + }}, + } { + t.Run(c.name, func(t *testing.T) { + ix, mods := newFormIndexer(t, builderModules(t)...) + reportToolchain(t, ix, "mesh-build-go", "novox", "go", "go1.27.1") + before := mods.writes + c.do(t, ix, mods) + if mods.writes != before { + t.Errorf("modules written %d times, want none", mods.writes-before) + } + }) } } -- 2.54.0