From af3b678a92f01e5f3e376101202d5309bd92e8e9 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 11:29:31 +0100 Subject: [PATCH] A lost take-in hold passes the take-in event's check, so it is said (issue 1966) holdLost builds a pending take-in naming neither WaitsFor nor Unknown, which TakeIn's check refused, so the hold stayed in the journal and the watchman never saw it. Quick fix: set Unknown on holdLost's event. Thorough fix (chosen): the check accepts a pending revision that names Lost, the field made for it; one naming nothing is still refused. Tests: a row accepting Lost, the refusal row kept, and the lost-hold scenario encodes its said event. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../mesh-delivery-forge-bridge-tools/main_test.go | 2 ++ .../internal/index/pending.go | 15 ++++++++++----- modules/mesh-delivery-indexer/takein/takein.go | 4 ++-- .../mesh-delivery-indexer/takein/takein_test.go | 9 ++++++++- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go index 0be6d7ad..946eece1 100644 --- a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go +++ b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go @@ -186,6 +186,8 @@ func TestTheBridgeToolsReadTheGroupsAndAskTheEndAgain(t *testing.T) { 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"}, } { diff --git a/modules/mesh-delivery-indexer/internal/index/pending.go b/modules/mesh-delivery-indexer/internal/index/pending.go index 067e4909..d92c6956 100644 --- a/modules/mesh-delivery-indexer/internal/index/pending.go +++ b/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 - // applying the newest writes every module whose manifest differs from what is held (modules.go). + // 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), or its repository's + // taken merge is at or ahead of it (loadNewest, takenAlready), so applying it would write nothing. Covered bool } @@ -613,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 @@ -978,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)] - 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) + by := "the taken merge" + 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) diff --git a/modules/mesh-delivery-indexer/takein/takein.go b/modules/mesh-delivery-indexer/takein/takein.go index b809ca5c..645535c2 100644 --- a/modules/mesh-delivery-indexer/takein/takein.go +++ b/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., not %q", t.Key) } - if t.Kind == TakeInPending && t.WaitsFor == "" && t.Unknown == "" { - 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) + 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, why the indexer does not know its own, or the merges it lost", t.Key) } case TakeInGroup: if t.Group == "" { diff --git a/modules/mesh-delivery-indexer/takein/takein_test.go b/modules/mesh-delivery-indexer/takein/takein_test.go index baaa3844..8aef1dbd 100644 --- a/modules/mesh-delivery-indexer/takein/takein_test.go +++ b/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}}}