Merge pull request 'The indexer keeps an unreadable manifest's inputs, so an SDK upgrade judges it again at start (issue 1923)' (#29) from fix/1923-stale-none2 into main

Reviewed-on: #29

modules/mesh-delivery-indexer/cmd/mesh-delivery-indexer/integration_test.go

@@ -478,3 +478,74 @@ func TestAMergeOfMeshSDKChangingOnlyGoLeavesItsNamedTreeEqual(t *testing.T) {
 		t.Errorf("the new form moved: %q/%q, then %q/%q", first.FP2, first.None2, second.FP2, second.None2)
 	}
 }
+
+// TestIndexerJudgesAnUnreadableVerdictAgainAtStart: the modules bucket holds avahi as an older SDK wrote it, with the reason
+// 'the manifest cannot be read', the inputs the merge carried (the tree, the lock hash by file name) and the digest of its
+// manifest, which manifests.<digest> holds and the running SDK reads; the go toolchain is reported. Within 5 s of the start
+// and with no repository merge the value carries a fingerprint and no reason (issue 1923).
+func TestIndexerJudgesAnUnreadableVerdictAgainAtStart(t *testing.T) { // 1923/AC3
+	manifest, err := os.ReadFile("../../internal/index/testdata/catalogue-a67f0196-avahi.module.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	const goSum = "3b5028f0c1e9bde04dd766abd63eb5931bc516bae695cf46f00abcd8510b0381"
+	digest := delivery.ManifestDigest(manifest)
+	_, ctx, _, modules := startIndexer(t, func(context.Context, jetstream.KeyValue) {}, func(ctx context.Context, js jetstream.JetStream) {
+		mkey, err := delivery.ManifestsKey(digest)
+		if err != nil {
+			t.Fatal(err)
+		}
+		mdata, err := delivery.EncodeManifest(delivery.Manifest{Module: "avahi", JSON: string(manifest)})
+		if err != nil {
+			t.Fatal(err)
+		}
+		manifests, err := js.KeyValue(ctx, delivery.ManifestsBucket)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := manifests.Put(ctx, mkey, mdata); err != nil {
+			t.Fatal(err)
+		}
+		stale, err := delivery.EncodeModule(delivery.Module{Repository: "novox/mesh-catalog", Commit: "89abcdef0123456789abcdef0123456789abcdef", Path: "modules/avahi",
+			Manifest: digest, None2: "the manifest cannot be read: claims node-build-go, which is a seat in the mesh's own namespace that it does not define",
+			Inputs2: &delivery.KeptInputs2{Tree: "tree-avahi", Locks: map[string]string{"go.sum": goSum}}})
+		if err != nil {
+			t.Fatal(err)
+		}
+		held, err := js.KeyValue(ctx, delivery.ModulesBucket)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := held.Put(ctx, "avahi", stale); err != nil {
+			t.Fatal(err)
+		}
+		kv, err := js.CreateKeyValue(ctx, jetstream.KeyValueConfig{Bucket: delivery.ToolchainsBucket("mesh-build-go"), History: 1})
+		if err != nil {
+			t.Fatal(err)
+		}
+		key, err := delivery.ToolchainKey("n1", "go")
+		if err != nil {
+			t.Fatal(err)
+		}
+		value, err := delivery.EncodeToolchain(delivery.Toolchain{Language: "go", Version: "go1.27.1", Recipe: "1", At: time.Now()})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := kv.Put(ctx, key, value); err != nil {
+			t.Fatal(err)
+		}
+	})
+	var got delivery.Module
+	for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(50 * time.Millisecond) {
+		if e, err := modules.Get(ctx, "avahi"); err == nil {
+			if m, err := delivery.DecodeModule(e.Value()); err == nil {
+				if got = m; got.FP2 != "" {
+					break
+				}
+			}
+		}
+	}
+	if got.FP2 == "" || got.None2 != "" {
+		t.Errorf("avahi = fp2 %q none2 %q within 5 s of the start, want a fingerprint and no reason", got.FP2, got.None2)
+	}
+}

modules/mesh-delivery-indexer/internal/index/inputs.go

@@ -69,7 +69,7 @@ func inputsOf(module, raw, tree string, locks map[string]string, base baseFP) mo
 func inputsOfNamed(module, raw, tree string, locks, objects map[string]string, base baseFP) moduleInputs {
 	m, err := catalogue.ParseManifestForInputs([]byte(raw))
 	if err != nil {
-		return moduleInputs{None2: "the manifest cannot be read: " + err.Error()}
+		return moduleInputs{None2: None2Unreadable + err.Error()}
 	}
 	if m.Module != module {
 		return moduleInputs{None2: fmt.Sprintf("the manifest is of module %q, not %q", m.Module, module)}

modules/mesh-delivery-indexer/internal/index/modules.go

@@ -70,6 +70,11 @@ func (ix *Indexer) applyMerged(ctx context.Context, f delivery.Forge) error {
 		} else {
 			in := inputsOfNamed(name, raw, f.Trees[dir], f.Locks[dir], objects[dir], ix.baseFP(ctx))
 			next.Inputs2, next.None2, next.ArtifactPaths = in.Kept, in.None2, in.ArtifactPaths
+			if in.Kept == nil && strings.HasPrefix(in.None2, None2Unreadable) {
+				// The manifest cannot be read by this SDK; the inputs the merge carried are kept all the same, so a later SDK
+				// that reads it judges the module again at start with no merge (issue 1923).
+				next.Inputs2 = unreadInputs(f.Trees[dir], f.Locks[dir], f.Paths[dir])
+			}
 			if in.Kept == nil && namedErr[dir] != "" {
 				next.None2 = in.None2 + ": the forge could not say them: " + namedErr[dir]
 			}
@@ -253,3 +258,24 @@ func (ix *Indexer) Reconcile(ctx context.Context) error {
 	}
 	return nil
 }
+
+// None2Unreadable begins the reason of a module whose manifest the running SDK cannot read.
+const None2Unreadable = "the manifest cannot be read: "
+
+// unreadInputs are the inputs a merge carried for a module whose manifest cannot be read: the directory's tree, the lock
+// hashes by file name (the artifacts that own them are not known without the manifest) and the objects of the named
+// paths the message carried; nil when the message carried no tree. Judging the manifest again (rejudged) turns them into
+// the inputs of the module (issue 1923).
+func unreadInputs(tree string, locks, paths map[string]string) *delivery.KeptInputs2 {
+	if tree == "" {
+		return nil
+	}
+	in := &delivery.KeptInputs2{Tree: tree}
+	if len(locks) > 0 {
+		in.Locks = maps.Clone(locks)
+	}
+	if len(paths) > 0 {
+		in.Paths = maps.Clone(paths)
+	}
+	return in
+}

modules/mesh-delivery-indexer/internal/index/stale_none2_test.go

@@ -0,0 +1,77 @@
+package index
+
+import (
+	"context"
+	"strings"
+	"testing"
+)
+
+// unreadableAvahi is avahi's manifest with a field the running parser refuses: the shape of a manifest an older SDK could
+// not read.
+func unreadableAvahi(t *testing.T) string {
+	t.Helper()
+	raw := strings.Replace(catalogueManifest(t, "avahi"), `"version": "1",`, `"version": "1", "scope": "everything",`, 1)
+	if raw == catalogueManifest(t, "avahi") {
+		t.Fatal("the fixture has no version line")
+	}
+	return raw
+}
+
+func TestAMergeOfAnUnreadableManifestKeepsTheInputsBesideTheReason(t *testing.T) { // 1923/AC1
+	_, mods := newFormIndexer(t, mod{"avahi", unreadableAvahi(t)})
+	m, ok := moduleOf(t, mods, "avahi")
+	if !ok || !strings.HasPrefix(m.None2, "the manifest cannot be read: ") || m.FP2 != "" {
+		t.Fatalf("module = %+v, want the reason 'the manifest cannot be read' and no fingerprint", m)
+	}
+	if m.Inputs2 == nil || m.Inputs2.Tree != "tree-avahi" || m.Inputs2.Locks["go.sum"] != avahiGoSum {
+		t.Errorf("inputs = %+v, want the directory's tree and its lock hashes kept", m.Inputs2)
+	}
+}
+
+func TestAStaleUnreadableVerdictIsJudgedAgainAtStartAndAnotherReasonIsLeft(t *testing.T) { // 1923/AC2
+	ctx := context.Background()
+	// The fingerprint a merge of the readable manifest gives.
+	ixGood, good := newFormIndexer(t, mod{"avahi", catalogueManifest(t, "avahi")})
+	reportToolchain(t, ixGood, "mesh-build-go", "novox", "go", "go1.27.1")
+	reportToolchain(t, ixGood, "mesh-build-go", "ace", "go", "go1.27.1")
+	want, _ := moduleOf(t, good, "avahi")
+	if want.FP2 == "" {
+		t.Fatalf("the readable manifest gives no fingerprint: %+v", want)
+	}
+
+	// The same module as an older SDK wrote it: the reason, the inputs kept at the merge, the manifest stored by digest.
+	ix, mods := newFormIndexer(t, mod{"avahi", unreadableAvahi(t)})
+	reportToolchain(t, ix, "mesh-build-go", "novox", "go", "go1.27.1")
+	reportToolchain(t, ix, "mesh-build-go", "ace", "go", "go1.27.1")
+	stale, _ := moduleOf(t, mods, "avahi")
+	stale.Manifest = want.Manifest
+	ix.Manifests.(*memStore).put(want.Manifest, good2(t, ixGood, want.Manifest))
+	mods.put("avahi", mustEncode(t, stale))
+	if err := ix.Recompute(ctx); err != nil {
+		t.Fatal(err)
+	}
+	got, _ := moduleOf(t, mods, "avahi")
+	if got.FP2 != want.FP2 || got.None2 != "" {
+		t.Errorf("after the start: fp2 %q none2 %q, want %q and no reason", got.FP2, got.None2, want.FP2)
+	}
+
+	// A manifest still unreadable keeps its value as it was.
+	ix2, mods2 := newFormIndexer(t, mod{"avahi", unreadableAvahi(t)})
+	before := string(mods2.vals["avahi"])
+	if err := ix2.Recompute(ctx); err != nil {
+		t.Fatal(err)
+	}
+	if after := string(mods2.vals["avahi"]); after != before {
+		t.Errorf("a manifest still unreadable changed the value:\n%s\n%s", before, after)
+	}
+}
+
+// good2 is the stored manifest value of the digest in the indexer that merged the readable manifest.
+func good2(t *testing.T, ix *Indexer, digest string) []byte {
+	t.Helper()
+	v, _, found, err := ix.Manifests.Get(context.Background(), digest)
+	if err != nil || !found {
+		t.Fatalf("manifests.%s: %v %v", digest, found, err)
+	}
+	return v
+}

modules/mesh-delivery-indexer/internal/index/toolchains.go

@@ -156,6 +156,7 @@ func (ix *Indexer) recomputeModules(ctx context.Context, overlay map[string]deli
 	}
 	slices.Sort(keys)
 	held := map[string]delivery.Module{}
+	stale := map[string]bool{} // values an SDK that could not read their manifest wrote, judged again (issue 1923)
 	all := map[string]*known{}
 	for _, key := range keys {
 		cur, ok := overlay[key]
@@ -176,11 +177,15 @@ func (ix *Indexer) recomputeModules(ctx context.Context, overlay map[string]deli
 		if cur.Manifest == "" || cur.Inputs2 == nil {
 			continue
 		}
-		m, err := ix.manifestOf(ctx, cur.Manifest)
+		raw, m, err := ix.manifestRaw(ctx, cur.Manifest)
 		if err != nil {
 			ix.logf("modules.%s: its manifest %s cannot be read, left: %v", key, cur.Manifest, err)
 			continue
 		}
+		if again, ok := ix.rejudged(ctx, m, cur, raw); ok {
+			cur, stale[key] = again, true
+			held[key] = cur
+		}
 		all[key] = &known{key: key, cur: cur, languages: languagesOf(m), bases: basesOf(m), contexts: hasContext(m)}
 	}
 	tools := ix.agreed()
@@ -275,10 +280,10 @@ func (ix *Indexer) recomputeModules(ctx context.Context, overlay map[string]deli
 			finals[key] = final
 			continue
 		}
-		if k.cur.FP2 == r.fp && k.cur.None2 == r.none && reflect.DeepEqual(k.cur.Toolchains, r.tcs) {
+		if !stale[key] && k.cur.FP2 == r.fp && k.cur.None2 == r.none && reflect.DeepEqual(k.cur.Toolchains, r.tcs) {
 			continue
 		}
-		digest, inputs := k.cur.Manifest, k.cur.Inputs2
+		digest, inputs, paths := k.cur.Manifest, k.cur.Inputs2, k.cur.ArtifactPaths
 		err := write(ctx, ix.Modules, key, func(value []byte, found bool) ([]byte, bool, error) {
 			if !found {
 				return nil, false, nil
@@ -291,7 +296,7 @@ func (ix *Indexer) recomputeModules(ctx context.Context, overlay map[string]deli
 				return nil, false, nil
 			}
 			cur.FP2, cur.None2, cur.Toolchains = r.fp, r.none, r.tcs
-			cur.Inputs2 = inputs
+			cur.Inputs2, cur.ArtifactPaths = inputs, paths
 			data, err := delivery.EncodeModule(cur)
 			return data, false, err
 		})
@@ -326,24 +331,41 @@ func (ix *Indexer) writeOverlay(ctx context.Context, overlay, finals map[string]
 	return nil
 }
 
-// manifestOf is the parsed manifest manifests.<digest> holds.
-func (ix *Indexer) manifestOf(ctx context.Context, digest string) (catalogue.Manifest, error) {
+// manifestRaw is manifests.<digest>: the manifest as stored and parsed by the running SDK.
+func (ix *Indexer) manifestRaw(ctx context.Context, digest string) (string, catalogue.Manifest, error) {
 	key, err := delivery.ManifestsKey(digest)
 	if err != nil {
-		return catalogue.Manifest{}, err
+		return "", catalogue.Manifest{}, err
 	}
 	value, _, found, err := ix.Manifests.Get(ctx, key)
 	if err != nil {
-		return catalogue.Manifest{}, err
+		return "", catalogue.Manifest{}, err
 	}
 	if !found {
-		return catalogue.Manifest{}, fmt.Errorf("manifests.%s is not there", key)
+		return "", catalogue.Manifest{}, fmt.Errorf("manifests.%s is not there", key)
 	}
 	kept, err := delivery.DecodeManifest(value)
 	if err != nil {
-		return catalogue.Manifest{}, err
+		return "", catalogue.Manifest{}, err
 	}
-	return catalogue.ParseManifestForInputs([]byte(kept.JSON))
+	m, err := catalogue.ParseManifestForInputs([]byte(kept.JSON))
+	return kept.JSON, m, err
+}
+
+// rejudged is a value an SDK that could not read its manifest wrote, as the running SDK judges it: its kept inputs (the
+// tree, the lock hashes by file name, the named paths' objects) taken through the manifest as a merge takes them, and the
+// reason that gives. It answers false for any other value, and for one whose manifest the running SDK still cannot make
+// inputs of (issue 1923).
+func (ix *Indexer) rejudged(ctx context.Context, m catalogue.Manifest, cur delivery.Module, raw string) (delivery.Module, bool) {
+	if cur.FP2 != "" || cur.Inputs2 == nil || !strings.HasPrefix(cur.None2, None2Unreadable) {
+		return cur, false
+	}
+	in := inputsOfNamed(m.Module, raw, cur.Inputs2.Tree, cur.Inputs2.Locks, cur.Inputs2.Paths, ix.baseFP(ctx))
+	if in.Kept == nil {
+		return cur, false
+	}
+	cur.Inputs2, cur.None2, cur.ArtifactPaths = in.Kept, in.None2, in.ArtifactPaths
+	return cur, true
 }
 
 // hasContext is whether an artifact of the manifest reads another repository's tree.