Merge pull request 'The forge bridge reads the forge's wrapped answer, and the indexer ends a merge its taken merge covers (issue 1966, recovery)' (#187) from fix/1966-forge-answer into main

modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go

@@ -184,6 +184,10 @@ func TestTheBridgeToolsReadTheGroupsAndAskTheEndAgain(t *testing.T) {
 				api: forgeAnswers(t, "/repos/novox/a/pulls/9", true, "fix/x", h2, c2, "dev"), fail: "not the trunk"},
 			{name: "a forge pull request with no head branch", raws: map[string][]byte{}, repository: "novox/a", merge: c2, number: 9,
 				api: forgeAnswers(t, "/repos/novox/a/pulls/9", true, "refs/pull/9/head", h2, c2, "main"), fail: "no head branch"},
+			{name: "a forge answer that is the bare body, no wrapper", raws: map[string][]byte{}, repository: "novox/a", merge: c2, number: 9,
+				api: func(string) (json.RawMessage, error) { return json.RawMessage(`{"number":9,"merged":true}`), nil }, fail: `{"result"`},
+			{name: "a forge answer with no pull request number", raws: map[string][]byte{}, repository: "novox/a", merge: c2, number: 9,
+				api: func(string) (json.RawMessage, error) { return json.RawMessage(`{"result":{"merged":true}}`), nil }, fail: "no number"},
 			{name: "a forge that does not answer", raws: map[string][]byte{}, repository: "novox/a", merge: c2,
 				api: func(string) (json.RawMessage, error) { return nil, errors.New("the forge is down") }, fail: "the forge is down"},
 		} {
@@ -221,6 +225,12 @@ func forgeAnswers(t *testing.T, path string, merged bool, branch, head, merge, b
 		if p != path {
 			return nil, errors.New("the forge has no " + p)
 		}
-		return body, nil
+		return wrapped(body), nil
 	}
 }
+
+// wrapped is gitea_api's answer for a body: {"result": <body>} (mesh-catalog modules/gitea/tools/index.ts).
+func wrapped(body []byte) json.RawMessage {
+	b, _ := json.Marshal(map[string]json.RawMessage{"result": body})
+	return b
+}

modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go

@@ -428,10 +428,10 @@ func TestBridgeTagsAnswersAPackagesTagsByPrefixPageByPage(t *testing.T) { // DLV
 			var tags []map[string]string
 			_ = json.Unmarshal(full, &tags)
 			tags = append(tags, map[string]string{"name": "go/v0.1.13"}, map[string]string{"name": "v0.0.1"})
-			b, _ := json.Marshal(tags)
+			b, _ := json.Marshal(map[string]any{"result": tags})
 			return b, nil
 		}
-		return json.RawMessage(`[{"name":"go/v0.1.2"}]`), nil
+		return json.RawMessage(`{"result":[{"name":"go/v0.1.2"}]}`), nil
 	}
 	a, err := Tags(api, "novox/mesh-sdk", "go/v")
 	if err != nil || !slicesEqual(a.Tags, []string{"go/v0.1.13", "go/v0.1.2"}) || a.More || len(asked) != 2 ||
@@ -441,6 +441,9 @@ func TestBridgeTagsAnswersAPackagesTagsByPrefixPageByPage(t *testing.T) { // DLV
 	if _, err := Tags(func(string) (json.RawMessage, error) { return nil, errors.New("down") }, "novox/mesh-sdk", ""); err == nil || !strings.Contains(err.Error(), "page 1") {
 		t.Errorf("a forge not answering = %v, want an error naming the page", err)
 	}
+	if _, err := Tags(func(string) (json.RawMessage, error) { return json.RawMessage(`[{"name":"go/v0.1.2"}]`), nil }, "novox/mesh-sdk", ""); err == nil || !strings.Contains(err.Error(), `{"result"`) {
+		t.Errorf("a bare list with no wrapper = %v, want the shape named", err)
+	}
 	if _, err := Tags(api, "mesh-sdk", ""); err == nil {
 		t.Error("a repository with no owner answered")
 	}

modules/mesh-delivery-forge-bridge/internal/bridge/forgemade.go

@@ -29,6 +29,10 @@ func ForgeMade(api API, owner, repo string, number int, merge string) (ForgePull
 	if err != nil {
 		return ForgePull{}, fmt.Errorf("the forge did not answer %s: %w", path, err)
 	}
+	body, err := forgeBody(raw)
+	if err != nil {
+		return ForgePull{}, fmt.Errorf("the forge's answer for %s: %w", path, err)
+	}
 	var p struct {
 		Number         int    `json:"number"`
 		Merged         bool   `json:"merged"`
@@ -41,10 +45,12 @@ func ForgeMade(api API, owner, repo string, number int, merge string) (ForgePull
 			Ref string `json:"ref"`
 		} `json:"base"`
 	}
-	if err := json.Unmarshal(raw, &p); err != nil {
+	if err := json.Unmarshal(body, &p); err != nil {
 		return ForgePull{}, fmt.Errorf("the forge's answer for %s is no pull request: %w", path, err)
 	}
 	switch {
+	case p.Number == 0:
+		return ForgePull{}, fmt.Errorf("the forge's answer for %s is no pull request (no number)", path)
 	case !p.Merged:
 		return ForgePull{}, fmt.Errorf("the forge says the pull request %s/%s#%d is not merged", owner, repo, p.Number)
 	case p.MergeCommitSHA != merge:

modules/mesh-delivery-forge-bridge/internal/bridge/tags.go

@@ -18,9 +18,23 @@ const (
 	TagsMaxPages = 40
 )
 
-// API asks the forge one GET of its API (the forge module's gitea_api), answering the body.
+// API asks the forge one GET of its API (the forge module's gitea_api), answering what the tool answers: the body
+// wrapped as {"result": <body>}. Every reader of an API answer takes the body through forgeBody, the one place that
+// knows that shape (issue 1966: two readers took the wrapper for the body).
 type API func(path string) (json.RawMessage, error)
 
+// forgeBody is the body of the forge's answer: the tool's "result". An answer without one is refused, naming the shape,
+// never read as an empty body.
+func forgeBody(raw json.RawMessage) (json.RawMessage, error) {
+	var w struct {
+		Result json.RawMessage `json:"result"`
+	}
+	if err := json.Unmarshal(raw, &w); err != nil || len(w.Result) == 0 || string(w.Result) == "null" {
+		return nil, fmt.Errorf(`the forge module's answer is not {"result": <body>}`)
+	}
+	return w.Result, nil
+}
+
 // TagsAnswer is what bridge_tags answers.
 type TagsAnswer struct {
 	Repository string   `json:"repository"`
@@ -48,10 +62,14 @@ func Tags(api API, repository, prefix string) (TagsAnswer, error) {
 		if err != nil {
 			return TagsAnswer{}, fmt.Errorf("the forge did not answer the tags of %s (page %d): %w", out.Repository, page, err)
 		}
+		body, err := forgeBody(raw)
+		if err != nil {
+			return TagsAnswer{}, fmt.Errorf("the forge's tags of %s (page %d): %w", out.Repository, page, err)
+		}
 		var tags []struct {
 			Name string `json:"name"`
 		}
-		if err := json.Unmarshal(raw, &tags); err != nil {
+		if err := json.Unmarshal(body, &tags); err != nil {
 			return TagsAnswer{}, fmt.Errorf("the forge's tags of %s (page %d) are no list of tags: %w", out.Repository, page, err)
 		}
 		for _, t := range tags {

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

@@ -99,8 +99,9 @@ type PendingEntry struct {
 	Forge    forgemsg.Forge
 	Light    bool
 	Holds    []string
-	// Covered says a newer merge of the same repository is in the same revision: its message is not read or applied, since
+	// Covered says its message is not read or applied: either a newer merge of the same repository is in the same revision
-	// applying the newest writes every module whose manifest differs from what is held (modules.go).
+	// (applying the newest writes every module whose manifest differs from what is held, modules.go), or its repository's
+	// taken merge is at or ahead of it (loadNewest, takenAlready), so applying it would write nothing.
 	Covered bool
 }
 
@@ -254,6 +255,14 @@ func (ix *Indexer) loadNewest(ctx context.Context, entries []PendingEntry) (out,
 			continue
 		}
 		newest[repo] = true
+		// A merge at or behind its repository's taken merge writes nothing: it is ended as covered without reading its
+		// message, which a bridge that cannot give it would otherwise hold the whole revision for (issue 1966). A merge whose
+		// ancestry cannot be asked is read as before.
+		if taken, err := ix.takenAlready(ctx, forgemsg.GroupMerge{Repository: out[i].Forge.Repository, Number: out[i].Forge.Number, Merge: out[i].Forge.Merge}); err == nil && taken {
+			ix.logf("take-in: %s (%s#%d) is covered by the taken merge of %s, its message is not read", out[i].Key, out[i].Forge.Repository, out[i].Forge.Number, out[i].Forge.Repository)
+			out[i].Covered = true
+			continue
+		}
 		full, err := ix.load(ctx, out[i])
 		if err != nil {
 			lost = append(lost, out[i])
@@ -605,7 +614,8 @@ func (ix *Indexer) headTaken(ctx context.Context, m meshdeliveryclient.Head) (bo
 	return in, err
 }
 
-// takenAlready says whether a group member's merge is at or behind its repository's taken merge.
+// takenAlready says whether a merge (a group member's, or the newest pending merge of a repository in loadNewest) is at or
+// behind its repository's taken merge.
 func (ix *Indexer) takenAlready(ctx context.Context, m forgemsg.GroupMerge) (bool, error) {
 	if ix.Taken == nil {
 		return false, nil
@@ -970,8 +980,11 @@ func (ix *Indexer) takeInWholeBy(ctx context.Context, entries []PendingEntry, ow
 	}
 	for _, e := range claimed {
 		if e.Covered {
-			by := newest[strings.ToLower(e.Forge.Repository)]
+			by := "the taken merge"
-			ix.logf("take-in: %s (%s#%d) is covered by %s, the newest merge of %s in this revision: its message is not read, and it is ended with it", e.Key, e.Forge.Repository, e.Forge.Number, by.Key, e.Forge.Repository)
+			if n, ok := newest[strings.ToLower(e.Forge.Repository)]; ok {
+				by = n.Key + ", the newest merge"
+			}
+			ix.logf("take-in: %s (%s#%d) is covered by %s of %s in this revision: its message is not read, and it is ended with it", e.Key, e.Forge.Repository, e.Forge.Number, by, e.Forge.Repository)
 			// Its pull request leaves its group's candidate as a merged message takes it out; nothing else of it is applied.
 			if err := ix.removeCandidate(ctx, e.Forge); err != nil {
 				ix.release(ctx, claimed)

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

@@ -710,6 +710,9 @@ func TestTakeIn(t *testing.T) { // 1585/T3-T13 (ADR 0633)
 		if len(b.said) != 1 || !slices.Equal(b.said[0].Lost, []string{takein.PendingKey(3)}) || len(waits) != 1 || !slices.Equal(waits[0].Lost, []string{takein.PendingKey(3)}) || b.commit(t, "avahi") != sha('b') {
 			t.Fatalf("lost: said %+v, waits %+v, avahi at %s: want m.3 lost on the event and in indexer_pending, nothing taken in", b.said, waits, b.commit(t, "avahi"))
 		}
+		if _, err := takein.EncodeTakeIn(b.said[0]); err != nil {
+			t.Errorf("the lost hold's event does not pass its check: %v", err)
+		}
 		if v, _, _, _ := b.pending.Get(ctx, takein.PendingKey(3)); !strings.Contains(string(v), "the bridge does not answer") {
 			t.Errorf("m.3 = %s: want the bridge's words", v)
 		}
@@ -753,6 +756,42 @@ func TestTakeIn(t *testing.T) { // 1585/T3-T13 (ADR 0633)
 			t.Errorf("an old value with the message whole: err %v, avahi at %s: want it taken in", err, b.commit(t, "avahi"))
 		}
 	})
+	t.Run("1966 a merge at or behind its repository's taken merge is ended without its message being read, one ahead of it is lost as before", func(t *testing.T) {
+		for _, c := range []struct {
+			name   string
+			behind bool
+		}{{"behind taken", true}, {"ahead of taken", false}} {
+			b := newTakeInBed(t)
+			f := b.merge(21, sha('c'), "tree-avahi-3", "tree-indexer-2")
+			f.Words, f.Branch = strings.Repeat("x", 300*1024), "feat/g"
+			b.arrive(t, 2, f) // held by reference, for an indexer build that is not this one
+			b.ix.Taken = newMem()
+			b.ix.Contains = func(context.Context, string, string, string) (bool, error) { return c.behind, nil }
+			data, _ := takein.EncodeTaken(takein.Taken{Repository: trunk.RepositoryName(catalogueRepo), Merge: sha('d')})
+			key, _ := trunk.RepositoryKey(catalogueRepo)
+			b.ix.Taken.(*memStore).put(key, data)
+			named := b.fp(t, Module)
+			asked := 0
+			next := &Indexer{Store: b.ix.Store, Modules: b.ix.Modules, Manifests: b.ix.Manifests, Graph: b.ix.Graph, Verdicts: b.ix.Verdicts,
+				Packages: b.ix.Packages, Builds: b.ix.Builds, Pending: b.pending, Taken: b.ix.Taken, Contains: b.ix.Contains,
+				Message: func(context.Context, string, string, int) ([]byte, error) {
+					asked++
+					return nil, errors.New("the bridge does not answer")
+				},
+				SayTakeIn: b.ix.SayTakeIn, Logf: b.ix.Logf, GoSum: b.ix.GoSum, OwnBuild: func(context.Context) (string, error) { return named, nil }}
+			next.toolchains = b.ix.toolchains
+			next.FixOwnBuild(ctx)
+			if _, _, err := next.TakeInAtStart(ctx); err != nil {
+				t.Fatal(err)
+			}
+			if c.behind && (asked != 0 || len(b.keys()) != 0 || !strings.Contains(b.logs.String(), "covered by the taken merge")) {
+				t.Errorf("%s: asked %d, pending %v: want no read and the entry ended", c.name, asked, b.keys())
+			}
+			if !c.behind && (asked == 0 || len(b.keys()) != 1) {
+				t.Errorf("%s: asked %d, pending %v: want the message asked and the entry held", c.name, asked, b.keys())
+			}
+		}
+	})
 	t.Run("the dry run names every field of the indexer", func(t *testing.T) {
 		// dry (pending.go) builds an indexer field by field: a field added to Indexer is named there, or this fails.
 		named := map[string]bool{}

modules/mesh-delivery-indexer/takein/takein.go

@@ -172,8 +172,8 @@ func (t TakeIn) check() error {
 		if !strings.HasPrefix(t.Key, "m.") {
 			return fmt.Errorf("delivery: a revision's take-in key is its oldest merge's m.<sequence>, not %q", t.Key)
 		}
-		if t.Kind == TakeInPending && t.WaitsFor == "" && t.Unknown == "" {
+		if t.Kind == TakeInPending && t.WaitsFor == "" && t.Unknown == "" && len(t.Lost) == 0 {
-			return fmt.Errorf("delivery: a pending revision %s names the indexer build it waits for, or why the indexer does not know its own", t.Key)
+			return fmt.Errorf("delivery: a pending revision %s names the indexer build it waits for, why the indexer does not know its own, or the merges it lost", t.Key)
 		}
 	case TakeInGroup:
 		if t.Group == "" {

modules/mesh-delivery-indexer/takein/takein_test.go

@@ -70,7 +70,7 @@ func TestTakeInEvents(t *testing.T) {
 	}{
 		{"group take-in without its group", "names its group", 1, func(t *TakeIn) { t.Group = "" }},
 		{"group take-in under another group's key", "g.other", 1, func(t *TakeIn) { t.Key = GroupTakeInKey("other") }},
-		{"pending revision waiting for no build", "waits for", 0, func(t *TakeIn) { t.WaitsFor = "" }},
+		{"pending revision waiting for no build", "lost", 0, func(t *TakeIn) { t.WaitsFor = "" }},
 		{"pending with a hand act", "only a taken event", 0, func(t *TakeIn) { t.By = "someone@node" }},
 		{"hand act without its by", "by", 2, func(t *TakeIn) { t.By = "" }},
 		{"take-in without merges", "no merge", 0, func(t *TakeIn) { t.Merges = nil }},
@@ -84,6 +84,13 @@ func TestTakeInEvents(t *testing.T) {
 			t.Errorf("%s: %v, want a refusal naming %q", c.name, err, c.want)
 		}
 	}
+	// A revision held for a merge the forge bridge could not give waits for no build and names what it lost (issue 1966: it was never said).
+	if held := sampleTakeIns(since)[0]; true {
+		held.WaitsFor, held.Lost = "", []string{PendingKey(12)}
+		if _, err := EncodeTakeIn(held); err != nil {
+			t.Errorf("a lost hold is refused: %v", err)
+		}
+	}
 	// indexer_pending's answer reads back; a wait of neither kind is refused.
 	pa := PendingAnswer{Waits: []PendingWait{{Key: PendingKey(12), What: TakeInRevision, Merges: sampleTakeIns(since)[0].Merges,
 		Keys: []string{PendingKey(12)}, WaitsFor: "src2:abc", Since: since, Holds: []string{"mesh-delivery-indexer"}, Said: "take-in.pending." + PendingKey(12), SaidAt: since}}}