From bbe107fa2a183563d8dd7eb46bd6ba24c82ec812 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 08:52:16 +0100 Subject: [PATCH 1/3] A provision only the newest build requires is kept while that build's composition is unread, and a key needed again withdraws its retire ask (issue 1966) Not read is not unneeded on the newest scope too (issue 940's rule): a module whose newest composition is unpublished or refused is held at its newest build, said in provisioner_plans newest-held and bounded on the health. A retiring key needed again cancels its ask on operator-channel (accept.cancel), retried until the router takes it. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../cmd/mesh-provisioner/composed_test.go | 28 ++++++ .../cmd/mesh-provisioner/holder.go | 93 +++++++++++++++++-- .../cmd/mesh-provisioner/holder_test.go | 87 +++++++++++++++-- .../cmd/mesh-provisioner/holdertools.go | 12 ++- 4 files changed, 202 insertions(+), 18 deletions(-) diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go b/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go index 0af1ab3ca..fba00dafc 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go @@ -18,6 +18,7 @@ import ( catalogclient "git.novox.be/novox/mesh-delivery/contracts/go/clients/meshcatalogclient" catalog "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshcatalog" "os" + "slices" "strings" "testing" "time" @@ -191,6 +192,33 @@ func TestARefusedOrMissingCompositionHoldsItsModule(t *testing.T) { if e, _ := s.Get(PlansState, "n1.restic"); e == nil || string(e.Value) != string(plan.Value) { t.Errorf("the plan was not kept while unpublished: %s", valueOf(e)) } + // Its newest build's composition not published (issue 1966): held at its newest, and what only that build requires + // is kept, never judged. Then refused there: held with the engine's words, the want's plan kept. + s.write(t, WantsRead, "want.n1.restic", `{"v":1,"fp":"`+fpsOf["restic"]+`","self":true}`) + published(t, s, c) + if _, err := h.ComposeNode(context.Background(), c.Node); err != nil { + t.Fatal(err) + } + for _, row := range []struct { + name, composed, held string + }{ + {"not published", "", "is not published"}, + {"refused", `{"v":1,"node":"n1","module":"restic","fp":"` + fpsOf["restic"] + `","resources":[],"refused":"no postgres here"}`, "no postgres here"}, + } { + ahead := "src2:" + strings.Repeat("7", 64) + e, _ := s.Get(ModulesRead, "restic") + s.write(t, ModulesRead, "restic", strings.ReplaceAll(string(e.Value), fpsOf["restic"], ahead)) + if row.composed != "" { + s.write(t, ComposedRead, mustKey(composedKey("n1", "restic", ahead)), row.composed) + } + comp := FromCompositions(s)(context.Background(), c.Node, map[string]string{"restic": fpsOf["restic"]}) + if !strings.Contains(comp.Newest.Held["restic"], row.held) || slices.Contains(comp.UnpublishedNewest, "restic") != (row.composed == "") { + t.Errorf("%s: newest held %v, unpublished at the newest %v", row.name, comp.Newest.Held, comp.UnpublishedNewest) + } + if _, held := comp.AtWants.Held["restic"]; held { + t.Errorf("%s: its wanted build is composed, but the module is held there: %v", row.name, comp.AtWants.Held) + } + } } func readJSON(t *testing.T, path string, v any) { diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go index 357719f20..599b81e9c 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go @@ -110,7 +110,10 @@ type Composition struct { // Unpublished is each module whose composition at the build it is read at is not published yet: the node's picture // is not whole, so no retirement is judged on it (issue 940). Unpublished []string - Took []Phase + // UnpublishedNewest is each module whose composition at its newest build (which is not its wanted build) is not + // published yet: the newest build's needs are unread, so no key only that build requires is judged (issue 1966). + UnpublishedNewest []string + Took []Phase } // HeldPlans is one scope's plans by module, the modules held with why, and a failure that is no single module's. @@ -147,6 +150,9 @@ type Holder struct { mu sync.Mutex // faults is, by node, the retirement a pass held back as a fault of the mesh (issue 940), said by provisioner_health. faults map[string]string + // newestHeld is, by node and module, each module held at its newest build with why (unpublished or refused), + // rederived by every pass from the node-engines' compositions and said by provisioner_plans. + newestHeld map[string]map[string]string // unpublished is, by node and module, since when the module is held because its composition at this build is not // published, with the hold's words: one held longer than unpublishedFor is said by provisioner_health. unpublished map[string]map[string]unpublishedHold @@ -248,6 +254,7 @@ func (h *Holder) ComposeNodeWith(ctx context.Context, node string, retireMany bo if len(heldAtWants) > 0 { out.Held = heldAtWants } + h.setNewestHeld(node, heldNewest) for _, m := range sortedKeys(heldNewest) { if _, ok := heldAtWants[m]; !ok { out.Problems = append(out.Problems, fmt.Sprintf("%s at its newest build is not composed: %s", m, heldNewest[m])) @@ -258,7 +265,17 @@ func (h *Holder) ComposeNodeWith(ctx context.Context, node string, retireMany bo } // A module whose composition is not published at its build is held (heldAtWants): its own plan and keys are kept, // and nothing else on the node waits for it (review 1 of issue 940). A hold that lasts is said on the health. - h.countUnpublished(node, c.Unpublished, heldAtWants) + // One held at its newest build too is counted under its own name, so a lasting gap is said on the health (issue 1966). + unreadFor := map[string]string{} + for m, why := range heldAtWants { + unreadFor[m] = why + } + unread := append([]string(nil), c.Unpublished...) + for _, m := range c.UnpublishedNewest { + unreadFor[m+newestSuffix] = heldNewest[m] + unread = append(unread, m+newestSuffix) + } + h.countUnpublished(node, unread, unreadFor) if err := h.writePlans(node, atWants, heldAtWants, &out); err != nil { out.Problems = append(out.Problems, err.Error()) } @@ -331,6 +348,13 @@ func (h *Holder) ComposeNodeWith(ctx context.Context, node string, retireMany bo untouched[m] = "it has a want or a have on " + node + " and no plan of it was read" } } + // A module whose composition at its newest build is unread is left as it is too: not read is not unneeded, and a key + // only that build requires is not judged (ADR 0570 clause 5, issue 1966). + for m, why := range heldNewest { + if _, u := untouched[m]; !u { + untouched[m] = newestKept + why + } + } h.judgeElsewhere(elsewhere, &out) h.judgeNode(node, needed, ahead, untouched, retireMany, &out) lap("provisions judged") @@ -572,8 +596,11 @@ func (h *Holder) judgeNode(node string, needed map[string]meshprovisionerclient. for key, have := range held { consumers[have.Consumer.Module] = true if _, isNeeded := needed[key]; !isNeeded && have.Consumer.Node == node { - if _, left := untouched[have.Consumer.Module]; left { + if why, left := untouched[have.Consumer.Module]; left { delete(held, key) + if strings.HasPrefix(why, newestKept) && have.State != meshprovisionerclient.Retiring { + out.Changed = append(out.Changed, key+" kept: "+why) + } } } } @@ -628,6 +655,14 @@ func (h *Holder) judgeNode(node string, needed map[string]meshprovisionerclient. next.Reference = have.Reference } h.outcome(key, &next) + // A key needed again is not asked about any more: its open ask is withdrawn on the operator's channel, and + // kept on the key until the router took the cancel, so the next pass tries again (issue 1966). + if isHeld && have.Ask != "" && have.Warrant == nil { + if err := h.cancelAsk(have.Ask); err != nil { + out.Problems = append(out.Problems, fmt.Sprintf("the ask %s of %s could not be withdrawn: %v", have.Ask, key, err)) + next.Ask = have.Ask + } + } if module, isAhead := ahead[key]; isAhead && next.State == meshprovisionerclient.Asked { // The reason the provider's helper says too: no plan names it yet, and it binds it from this key's entry. why := "no plan names it yet: the newest build of " + module + " requires it and its wanted build does " + @@ -1093,6 +1128,14 @@ func (h *Holder) ask(p *meshprovisionerclient.Provision, key string) error { return nil } +// cancelAsk withdraws a retiring ask on the operator's channel, under mesh-provisioner's own name (accept.cancel). +func (h *Holder) cancelAsk(id string) error { + if h.Asks == nil { + return errors.New("the operator's channel is not reachable from here") + } + return h.Asks.Publish(oc.AcceptSubjectCancel(Module), oc.CancelAccept{ID: id}, "cancel."+id) +} + // openAsks is how many retiring keys hold an ask with no answer yet. func (h *Holder) openAsks() int { n := 0 @@ -1282,6 +1325,38 @@ func (h *Holder) countUnpublished(node string, modules []string, held map[string h.unpublished[node] = now } +// newestSuffix names a module's hold at its newest build beside its hold at its wanted build; newestKept is what an +// untouched module's why starts with when its newest build's composition is the one unread. +const ( + newestSuffix = " at its newest build" + newestKept = "its newest build's composition is unread: " +) + +// setNewestHeld keeps the modules held at their newest build on node, with why, for provisioner_plans. +func (h *Holder) setNewestHeld(node string, held map[string]string) { + if h.newestHeld == nil { + h.newestHeld = map[string]map[string]string{} + } + if len(held) == 0 { + delete(h.newestHeld, node) + return + } + h.newestHeld[node] = held +} + +// NewestHeld is, by ".", each module held at its newest build, with why. +func (h *Holder) NewestHeld() map[string]string { + h.mu.Lock() + defer h.mu.Unlock() + out := map[string]string{} + for n, mods := range h.newestHeld { + for m, why := range mods { + out[n+"."+m] = why + } + } + return out +} + // Faults is, by node, each retirement a pass held back as a fault of the mesh. func (h *Holder) Faults() map[string]string { h.mu.Lock() @@ -1466,7 +1541,7 @@ func orSaid(words ...string) string { // nothing, and takes each module's plan from the key of `composed-` at the build it is read at. At the wanted // builds a module is read at the fingerprint keptAt names, else at the indexer's newest (FP2); at the newest, each placed // module at FP2. A key whose engine refused the module holds it with the engine's words; a key not published yet holds -// it at its wanted build (its last plan kept, as for a refusal) and leaves it out of the newest, whose build the engine +// it at its wanted build (its last plan kept, as for a refusal) and at its newest too (issue 1966), whose build the engine // writes once it is done (ADR 0593 ยง6), and says it unpublished, so the pass judges no retirement on the node (issue // 940). A placed module no build of which is known is held too. A plan absent from a key is a module that hands the // provisioner nothing. @@ -1492,7 +1567,7 @@ func FromCompositions(s Store) Composer { for m, fp := range keptAt { atWants[m] = fp } - var unpublished []string + var unpublished, unpublishedNewest []string for _, m := range sortedKeys(atWants) { fp := atWants[m] if fp == "" { @@ -1519,13 +1594,19 @@ func FromCompositions(s Store) Composer { c, ok := published[composedAt(node, m, fp2[m])] switch { case !ok: + // Not read is not unneeded: the module is held at its newest build, so what only that build requires is kept + // (issue 1966). Held at its wanted build too, it is said there. + newest.Held[m] = fmt.Sprintf("the composition of %s at %s is not published by the node-engine of %s yet", m, fp2[m], node) + if fp2[m] != atWants[m] { + unpublishedNewest = append(unpublishedNewest, m) + } case c.Refused != "": newest.Held[m] = c.Refused case c.Plan != nil: newest.Plans[m] = *c.Plan } } - return Composition{AtWants: at, Newest: newest, Unpublished: unpublished, + return Composition{AtWants: at, Newest: newest, Unpublished: unpublished, UnpublishedNewest: unpublishedNewest, Took: []Phase{{Name: "compositions read", Took: time.Since(start)}}} } } diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go index 0e449009a..132a9968c 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go @@ -31,9 +31,24 @@ type router struct { asked map[string]oc.Ask records map[string][]byte drop bool + // cancelled is the ids of the asks withdrawn on accept.cancel, in order. + cancelled []string + // failCancel is a router that refuses a cancel. + failCancel bool } -func (r *router) Publish(_ string, body any, id string) error { +func (r *router) Publish(subject string, body any, id string) error { + if c, ok := body.(oc.CancelAccept); ok { + if subject != oc.AcceptSubjectCancel(Module) { + return fmt.Errorf("a cancel sent on %s", subject) + } + if r.failCancel { + return fmt.Errorf("the router is away") + } + r.cancelled = append(r.cancelled, c.ID) + delete(r.records, c.ID) + return nil + } r.asked[id] = body.(oc.Ask) if !r.drop { r.records[id], _ = json.Marshal(map[string]any{"state": "open", "ask": body}) @@ -71,20 +86,26 @@ type mesh struct { heldAt map[string]map[string]string // by node, the modules its composition at the wanted builds holds // unpublished is, by node, the modules whose composition is not published yet. unpublished map[string][]string - composed []string + // heldNewest is, by node, the modules its composition at the newest builds holds, and unpublishedNewest those of them + // not published yet. + heldNewest map[string]map[string]string + unpublishedNewest map[string][]string + composed []string } func newMesh() *mesh { m := &mesh{s: newStore(), r: &router{asked: map[string]oc.Ask{}, records: map[string][]byte{}}, atWant: map[string]map[string]catalogue.Plan{}, atNewest: map[string]map[string]catalogue.Plan{}, - heldAt: map[string]map[string]string{}, unpublished: map[string][]string{}} + heldAt: map[string]map[string]string{}, unpublished: map[string][]string{}, + heldNewest: map[string]map[string]string{}, unpublishedNewest: map[string][]string{}} now := time.Date(2026, 10, 29, 22, 0, 0, 0, time.UTC) m.h = &Holder{Store: m.s, Asks: m.r, Logf: func(string, ...any) {}, Now: func() time.Time { now = now.Add(time.Second); return now }, Compose: func(_ context.Context, node string, keptAt map[string]string) Composition { m.composed = append(m.composed, node) return Composition{AtWants: HeldPlans{Plans: m.atWant[node], Held: m.heldAt[node]}, - Newest: HeldPlans{Plans: m.atNewest[node]}, Unpublished: m.unpublished[node]} + Newest: HeldPlans{Plans: m.atNewest[node], Held: m.heldNewest[node]}, + Unpublished: m.unpublished[node], UnpublishedNewest: m.unpublishedNewest[node]} }} // What each provider's manifest declares: postgres, keycloak and the vault keep entries, the others report by their // have. @@ -295,19 +316,21 @@ func TestRetiringIsAskedAndActsOnlyOnTheWarrant(t *testing.T) { asked []string states map[string]string removed bool + // cancelled is how many asks have been withdrawn on the operator's channel once the step is done (issue 1966). + cancelled int }{ {"a key composed ahead of its want is asked, not retiring", func() {}, nil, - map[string]string{pg: meshprovisionerclient.Provided, kc: meshprovisionerclient.Asked}, false}, + map[string]string{pg: meshprovisionerclient.Provided, kc: meshprovisionerclient.Asked}, false, 0}, {"one requirement removed and another added: only the removed retires", func() { m.atWant["g14"] = map[string]catalogue.Plan{"gitea": planOf(oidc)} m.atNewest["g14"] = map[string]catalogue.Plan{"gitea": planOf(oidc)} - }, []string{pg}, map[string]string{pg: meshprovisionerclient.Retiring, kc: meshprovisionerclient.Asked}, false}, - {"composed again: no second ask", func() {}, nil, map[string]string{pg: meshprovisionerclient.Retiring}, false}, - {"needed again before the warrant: back to its outcome", func() { + }, []string{pg}, map[string]string{pg: meshprovisionerclient.Retiring, kc: meshprovisionerclient.Asked}, false, 0}, + {"composed again: no second ask", func() {}, nil, map[string]string{pg: meshprovisionerclient.Retiring}, false, 0}, + {"needed again before the warrant: back to its outcome, its ask withdrawn", func() { m.atWant["g14"] = map[string]catalogue.Plan{"gitea": planOf(oidc, pgDB)} - }, nil, map[string]string{pg: meshprovisionerclient.Provided}, false}, + }, nil, map[string]string{pg: meshprovisionerclient.Provided}, false, 1}, {"gone again: asked again", func() { m.atWant["g14"] = map[string]catalogue.Plan{"gitea": planOf(oidc)} }, - []string{pg}, map[string]string{pg: meshprovisionerclient.Retiring}, false}, + []string{pg}, map[string]string{pg: meshprovisionerclient.Retiring}, false, 1}, } for _, s := range steps { s.change() @@ -320,7 +343,27 @@ func TestRetiringIsAskedAndActsOnlyOnTheWarrant(t *testing.T) { t.Errorf("%s: %s is %s, want %s (%s)", s.name, k, p.State, st, p.Why) } } + if len(m.r.cancelled) != s.cancelled { + t.Errorf("%s: %d asks withdrawn, want %d", s.name, len(m.r.cancelled), s.cancelled) + } + if s.cancelled == 1 && (len(m.r.cancelled) == 0 || m.state(t, pg).Ask == m.r.cancelled[0]) { + t.Errorf("%s: the withdrawn ask %v stays on the key", s.name, m.r.cancelled) + } } + // A withdrawal the router does not take keeps the ask on the key and says so; the next pass withdraws it. + m.r.failCancel = true + was := m.state(t, pg).Ask + m.atWant["g14"] = map[string]catalogue.Plan{"gitea": planOf(oidc, pgDB)} + if c := m.compose(t, "g14"); !strings.Contains(strings.Join(c.Problems, " "), "could not be withdrawn") || m.state(t, pg).Ask != was { + t.Errorf("a refused cancel: %v, ask %q (was %q)", c.Problems, m.state(t, pg).Ask, was) + } + m.r.failCancel = false + m.compose(t, "g14") + if m.state(t, pg).Ask != "" || !slices.Equal(m.r.cancelled[len(m.r.cancelled)-1:], []string{was}) { + t.Errorf("the next pass did not withdraw %s: ask %q, withdrawn %v", was, m.state(t, pg).Ask, m.r.cancelled) + } + m.atWant["g14"] = map[string]catalogue.Plan{"gitea": planOf(oidc)} + m.compose(t, "g14") p := m.state(t, pg) a := m.r.asked[p.Ask] if !strings.Contains(a.Explanation, "postgres") || !strings.Contains(a.Explanation, "ace") || @@ -665,6 +708,30 @@ func TestARetirementIsJudgedOnlyFromAWholePicture(t *testing.T) { t.Errorf("the hold is said after it ended: %+v", a) } }}, + {name: "the newest composition of the consumer is unread: its key is kept, said, and a lasting gap is unhealthy", consumer: 2, + gone: 1, change: func(m *mesh) { + m.unpublishedNewest["novox"] = []string{"de-spiegel"} + m.heldNewest["novox"] = map[string]string{"de-spiegel": "the composition of de-spiegel at " + fp + " is not published yet"} + }, after: func(t *testing.T, m *mesh) { + c := m.compose(t, "novox") + if !strings.Contains(strings.Join(c.Changed, " "), "novox.de-spiegel.route.1 kept: its newest build's composition is unread") { + t.Errorf("the pass does not say the key is kept: %v", c.Changed) + } + if got := m.h.NewestHeld(); !strings.Contains(got["novox.de-spiegel"], "not published") { + t.Errorf("newest-held says %v", got) + } + now := m.h.Now() + m.h.Now = func() time.Time { return now.Add(unpublishedFor + time.Minute) } + m.compose(t, "novox") + if a := m.h.health(); a.Healthy || !strings.Contains(a.Why, "novox.de-spiegel at its newest build") { + t.Errorf("a lasting gap at the newest build: %+v", a) + } + m.heldNewest["novox"], m.unpublishedNewest["novox"] = nil, nil + m.compose(t, "novox") + if m.state(t, "novox.de-spiegel.route.1").State != meshprovisionerclient.Retiring { + t.Errorf("read again and not needed, the key does not retire") + } + }}, {name: "a consumer that still has a want: not retired", consumer: 2, gone: 1, change: func(m *mesh) { m.s.write(t, WantsRead, "want.novox.de-spiegel", `{"v":1,"fp":"`+fp+`","self":true}`) diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go index cbc36bd85..02853c7f2 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go @@ -42,7 +42,8 @@ func holderTools(h *Holder) []stdio.Tool { Description: "Every module's provisioning plan the seat keeps, ., as the node-engine's published " + "composition carries it (composed-), narrowed by node, with how many keys and bytes the state holds, " + "and each placed module held with its last plan kept and why (refused: the node-engine refused its " + - "composition, or has not published it at the wanted build yet); or one plan whole.", + "composition, or has not published it at the wanted build yet), and newest-held: each module whose composition at its newest " + + "build is unread (not published or refused), whose keys only that build requires are kept; or one plan whole.", Input: map[string]any{"type": "object", "properties": map[string]any{"key": str("one plan, ."), "node": str("a node")}}, Run: func(a map[string]any) (any, error) { @@ -55,6 +56,13 @@ func holderTools(h *Holder) []stdio.Tool { return nil, err } out.(map[string]any)[RefusedState] = refused.(map[string]any)[RefusedState] + newest := map[string]string{} + for k, why := range h.NewestHeld() { + if node := arg(a, "node"); node == "" || strings.HasPrefix(k, node+".") { + newest[k] = why + } + } + out.(map[string]any)["newest-held"] = newest return out, nil }}, {Name: ComposeTool, @@ -81,7 +89,7 @@ func holderTools(h *Holder) []stdio.Tool { Description: "The provisioner's runtime health, as the node-engine asks it (novox/hq ADR 0366): unhealthy while " + "a pass held back a retirement of most of a node's consumers at once as a fault of the mesh (issue 940), " + "naming the node, the consumers and the likely cause, or while a module's composition has stayed " + - "unpublished at one build for longer than 45 minutes (longer than a build takes), naming the module, " + + "unpublished at one build, wanted or newest, for longer than 45 minutes (longer than a build takes), naming the module, " + "the build and since when.", Run: func(map[string]any) (any, error) { return h.health(), nil }}, } -- 2.54.0 From 6e6b325cf1a075e40141201317f6f68b186f9601 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 09:02:40 +0100 Subject: [PATCH 2/3] Newest holds are kept on the bus in refused, and every key needed again withdraws its retire ask by one helper (issue 1966) Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../cmd/mesh-provisioner/composed_test.go | 22 ++++ .../cmd/mesh-provisioner/holder.go | 104 +++++++++--------- .../cmd/mesh-provisioner/holder_test.go | 19 +++- .../cmd/mesh-provisioner/holdertools.go | 11 +- 4 files changed, 99 insertions(+), 57 deletions(-) diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go b/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go index fba00dafc..b044b58d7 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go @@ -215,10 +215,32 @@ func TestARefusedOrMissingCompositionHoldsItsModule(t *testing.T) { if !strings.Contains(comp.Newest.Held["restic"], row.held) || slices.Contains(comp.UnpublishedNewest, "restic") != (row.composed == "") { t.Errorf("%s: newest held %v, unpublished at the newest %v", row.name, comp.Newest.Held, comp.UnpublishedNewest) } + // A pass writes the newest hold into `refused` beside the wanted build's, and deletes it once it is read. + h.Compose = func(context.Context, string, map[string]string) Composition { return comp } + if _, err := h.ComposeNode(context.Background(), c.Node); err != nil { + t.Fatal(err) + } + if e, _ := s.Get(RefusedState, "n1.restic@newest"); e == nil || !strings.Contains(string(e.Value), row.held) { + t.Errorf("%s: refused holds %s", row.name, valueOf(e)) + } + h.Compose = FromCompositions(s) if _, held := comp.AtWants.Held["restic"]; held { t.Errorf("%s: its wanted build is composed, but the module is held there: %v", row.name, comp.AtWants.Held) } } + s.write(t, ModulesRead, "restic", strings.ReplaceAll(valueOf(mustEntry(t, s)), "src2:"+strings.Repeat("7", 64), fpsOf["restic"])) + if _, err := h.ComposeNode(context.Background(), c.Node); err != nil { + t.Fatal(err) + } + if e, _ := s.Get(RefusedState, "n1.restic@newest"); e != nil { + t.Errorf("the newest hold stays once the composition is read: %s", valueOf(e)) + } +} + +func mustEntry(t *testing.T, s *memStore) *Entry { + t.Helper() + e, _ := s.Get(ModulesRead, "restic") + return e } func readJSON(t *testing.T, path string, v any) { diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go index 599b81e9c..c936414bb 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go @@ -64,6 +64,9 @@ type Refusal struct { Module string `json:"module"` Why string `json:"why"` Since time.Time `json:"since"` + // Scope is "newest" for a module held at its newest build (its composition there unread: key `.@newest`), + // "" for one held at its wanted build. + Scope string `json:"scope,omitempty"` } // The reads it makes the plans from. ComposedRead is every node's published compositions, `composed-` (ADR 0593 @@ -150,9 +153,6 @@ type Holder struct { mu sync.Mutex // faults is, by node, the retirement a pass held back as a fault of the mesh (issue 940), said by provisioner_health. faults map[string]string - // newestHeld is, by node and module, each module held at its newest build with why (unpublished or refused), - // rederived by every pass from the node-engines' compositions and said by provisioner_plans. - newestHeld map[string]map[string]string // unpublished is, by node and module, since when the module is held because its composition at this build is not // published, with the hold's words: one held longer than unpublishedFor is said by provisioner_health. unpublished map[string]map[string]unpublishedHold @@ -254,13 +254,12 @@ func (h *Holder) ComposeNodeWith(ctx context.Context, node string, retireMany bo if len(heldAtWants) > 0 { out.Held = heldAtWants } - h.setNewestHeld(node, heldNewest) for _, m := range sortedKeys(heldNewest) { if _, ok := heldAtWants[m]; !ok { out.Problems = append(out.Problems, fmt.Sprintf("%s at its newest build is not composed: %s", m, heldNewest[m])) } } - if err := h.writeRefused(node, heldAtWants); err != nil { + if err := h.writeRefused(node, heldAtWants, heldNewest); err != nil { out.Problems = append(out.Problems, err.Error()) } // A module whose composition is not published at its build is held (heldAtWants): its own plan and keys are kept, @@ -363,16 +362,35 @@ func (h *Holder) ComposeNodeWith(ctx context.Context, node string, retireMany bo // writeRefused keeps, for each module held on node, why, and deletes the refusal of one that composes again: a change // of the state is the event a watcher hears. -func (h *Holder) writeRefused(node string, held map[string]string) error { +func (h *Holder) writeRefused(node string, held, heldNewest map[string]string) error { var problems []error - for _, module := range sortedKeys(held) { - key, err := meshprovisionerclient.PlanKey(node, module) - if err != nil { - problems = append(problems, err) - continue + // Each hold has a key: `.` at the wanted build, `.@newest` at the newest. + type hold struct { + key string + r Refusal + } + var holds []hold + for _, scope := range []struct { + name string + by map[string]string + }{{"", held}, {"newest", heldNewest}} { + for _, module := range sortedKeys(scope.by) { + key, err := meshprovisionerclient.PlanKey(node, module) + if err != nil { + problems = append(problems, err) + continue + } + if scope.name != "" { + key += "@" + scope.name + } + holds = append(holds, hold{key, Refusal{Node: node, Module: module, Why: scope.by[module], Since: h.Now().UTC(), Scope: scope.name}}) } - r := Refusal{Node: node, Module: module, Why: held[module], Since: h.Now().UTC()} - if e, err := h.Store.Get(RefusedState, key); err == nil && e != nil { + } + still := map[string]bool{} + for _, d := range holds { + still[d.key] = true + r := d.r + if e, err := h.Store.Get(RefusedState, d.key); err == nil && e != nil { var was Refusal if json.Unmarshal(e.Value, &was) == nil && was.Why == r.Why { continue @@ -383,11 +401,11 @@ func (h *Holder) writeRefused(node string, held map[string]string) error { problems = append(problems, err) continue } - if err := h.Store.Put(RefusedState, key, json.RawMessage(raw)); err != nil { - problems = append(problems, fmt.Errorf("the refusal %s: %w", key, err)) + if err := h.Store.Put(RefusedState, d.key, json.RawMessage(raw)); err != nil { + problems = append(problems, fmt.Errorf("the refusal %s: %w", d.key, err)) continue } - h.Logf("[%s] %s on %s has no plan composed: %s", Module, module, node, r.Why) + h.Logf("[%s] %s on %s has no plan composed%s: %s", Module, r.Module, node, map[bool]string{true: " at its newest build"}[r.Scope != ""], r.Why) } keys, err := h.Store.Keys(RefusedState) if err != nil { @@ -395,10 +413,7 @@ func (h *Holder) writeRefused(node string, held map[string]string) error { } for _, k := range keys { n, module, ok := strings.Cut(k, ".") - if !ok || n != node { - continue - } - if _, still := held[module]; still { + if !ok || n != node || still[k] { continue } if err := h.Store.Delete(RefusedState, k); err != nil { @@ -476,12 +491,17 @@ func (h *Holder) judgeElsewhere(keys map[string]meshprovisionerclient.Provision, next := keys[key] var have meshprovisionerclient.Provision e, err := h.Store.Get(ProvisionsState, key) + decoded := false if err == nil && e != nil { if have, err = meshprovisionerclient.Decode(e.Value); err == nil { next.Reference = have.Reference + decoded = true } } h.outcome(key, &next) + if decoded { + h.withdrawAsk(key, have, &next, out) + } if e != nil && same(have, next) { continue } @@ -657,11 +677,8 @@ func (h *Holder) judgeNode(node string, needed map[string]meshprovisionerclient. h.outcome(key, &next) // A key needed again is not asked about any more: its open ask is withdrawn on the operator's channel, and // kept on the key until the router took the cancel, so the next pass tries again (issue 1966). - if isHeld && have.Ask != "" && have.Warrant == nil { - if err := h.cancelAsk(have.Ask); err != nil { - out.Problems = append(out.Problems, fmt.Sprintf("the ask %s of %s could not be withdrawn: %v", have.Ask, key, err)) - next.Ask = have.Ask - } + if isHeld { + h.withdrawAsk(key, have, &next, out) } if module, isAhead := ahead[key]; isAhead && next.State == meshprovisionerclient.Asked { // The reason the provider's helper says too: no plan names it yet, and it binds it from this key's entry. @@ -1128,6 +1145,18 @@ func (h *Holder) ask(p *meshprovisionerclient.Provision, key string) error { return nil } +// withdrawAsk withdraws the open retire ask of a key that is needed again (next), keeping the ask on it when the router +// does not take the cancel, so the next pass tries again. +func (h *Holder) withdrawAsk(key string, have meshprovisionerclient.Provision, next *meshprovisionerclient.Provision, out *Composed) { + if have.Ask == "" || have.Warrant != nil { + return + } + if err := h.cancelAsk(have.Ask); err != nil { + out.Problems = append(out.Problems, fmt.Sprintf("the ask %s of %s could not be withdrawn: %v", have.Ask, key, err)) + next.Ask = have.Ask + } +} + // cancelAsk withdraws a retiring ask on the operator's channel, under mesh-provisioner's own name (accept.cancel). func (h *Holder) cancelAsk(id string) error { if h.Asks == nil { @@ -1332,31 +1361,6 @@ const ( newestKept = "its newest build's composition is unread: " ) -// setNewestHeld keeps the modules held at their newest build on node, with why, for provisioner_plans. -func (h *Holder) setNewestHeld(node string, held map[string]string) { - if h.newestHeld == nil { - h.newestHeld = map[string]map[string]string{} - } - if len(held) == 0 { - delete(h.newestHeld, node) - return - } - h.newestHeld[node] = held -} - -// NewestHeld is, by ".", each module held at its newest build, with why. -func (h *Holder) NewestHeld() map[string]string { - h.mu.Lock() - defer h.mu.Unlock() - out := map[string]string{} - for n, mods := range h.newestHeld { - for m, why := range mods { - out[n+"."+m] = why - } - } - return out -} - // Faults is, by node, each retirement a pass held back as a fault of the mesh. func (h *Holder) Faults() map[string]string { h.mu.Lock() diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go index 132a9968c..d8d973002 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go @@ -364,6 +364,19 @@ func TestRetiringIsAskedAndActsOnlyOnTheWarrant(t *testing.T) { } m.atWant["g14"] = map[string]catalogue.Plan{"gitea": planOf(oidc)} m.compose(t, "g14") + // A key of another node (a provider's shared credential) needed again withdraws its ask too, by the same helper. + retiring := m.state(t, pg) + if retiring.Ask == "" { + t.Fatal("the key holds no ask") + } + needed := retiring + needed.State, needed.Ask, needed.Warrant = "", "", nil + before := len(m.r.cancelled) + m.h.judgeElsewhere(map[string]meshprovisionerclient.Provision{pg: needed}, &Composed{}) + if len(m.r.cancelled) != before+1 || m.r.cancelled[before] != retiring.Ask || m.state(t, pg).Ask != "" { + t.Errorf("a credential key needed again: withdrawn %v, ask %q", m.r.cancelled, m.state(t, pg).Ask) + } + m.compose(t, "g14") // gone again: asked again p := m.state(t, pg) a := m.r.asked[p.Ask] if !strings.Contains(a.Explanation, "postgres") || !strings.Contains(a.Explanation, "ace") || @@ -717,8 +730,10 @@ func TestARetirementIsJudgedOnlyFromAWholePicture(t *testing.T) { if !strings.Contains(strings.Join(c.Changed, " "), "novox.de-spiegel.route.1 kept: its newest build's composition is unread") { t.Errorf("the pass does not say the key is kept: %v", c.Changed) } - if got := m.h.NewestHeld(); !strings.Contains(got["novox.de-spiegel"], "not published") { - t.Errorf("newest-held says %v", got) + // The hold is on the bus, in `refused` beside the wanted build's, so a restart or another instance sees it. + if e, _ := m.s.Get(RefusedState, "novox.de-spiegel@newest"); e == nil || !strings.Contains(string(e.Value), `"scope":"newest"`) || + !strings.Contains(string(e.Value), "not published") { + t.Errorf("refused holds %s", valueOf(e)) } now := m.h.Now() m.h.Now = func() time.Time { return now.Add(unpublishedFor + time.Minute) } diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go index 02853c7f2..778e488f2 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holdertools.go @@ -56,10 +56,11 @@ func holderTools(h *Holder) []stdio.Tool { return nil, err } out.(map[string]any)[RefusedState] = refused.(map[string]any)[RefusedState] - newest := map[string]string{} - for k, why := range h.NewestHeld() { - if node := arg(a, "node"); node == "" || strings.HasPrefix(k, node+".") { - newest[k] = why + // The modules held at their newest build are the refused rows of that scope, kept on the bus beside the others. + newest := []map[string]any{} + for _, row := range refused.(map[string]any)[RefusedState].([]map[string]any) { + if row["scope"] == "newest" { + newest = append(newest, row) } } out.(map[string]any)["newest-held"] = newest @@ -156,7 +157,7 @@ func (h *Holder) read(state, key, node, want string) (any, error) { if err := json.Unmarshal(e.Value, &r); err != nil { row["unreadable"] = err.Error() } else { - row["module"], row["why"], row["since"] = r.Module, r.Why, r.Since + row["module"], row["why"], row["since"], row["scope"] = r.Module, r.Why, r.Since, r.Scope } } rows = append(rows, row) -- 2.54.0 From 4658e1cb1c6ca56abdfa2935713a7a98977ba53f Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 09:06:39 +0100 Subject: [PATCH 3/3] A newest hold's key is .=newest, which the bus holds, and the test store refuses what the bus refuses (issue 1966) Review round 2: '@' is outside the bus's key rule, so every newest hold failed to write on the live bus while the in-memory store took it. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../cmd/mesh-provisioner/composed_test.go | 4 ++-- modules/mesh-provisioner/cmd/mesh-provisioner/holder.go | 6 +++--- .../mesh-provisioner/cmd/mesh-provisioner/holder_test.go | 2 +- .../cmd/mesh-provisioner/provisioner_test.go | 9 +++++++++ 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go b/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go index b044b58d7..e1958d7a1 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/composed_test.go @@ -220,7 +220,7 @@ func TestARefusedOrMissingCompositionHoldsItsModule(t *testing.T) { if _, err := h.ComposeNode(context.Background(), c.Node); err != nil { t.Fatal(err) } - if e, _ := s.Get(RefusedState, "n1.restic@newest"); e == nil || !strings.Contains(string(e.Value), row.held) { + if e, _ := s.Get(RefusedState, "n1.restic=newest"); e == nil || !strings.Contains(string(e.Value), row.held) { t.Errorf("%s: refused holds %s", row.name, valueOf(e)) } h.Compose = FromCompositions(s) @@ -232,7 +232,7 @@ func TestARefusedOrMissingCompositionHoldsItsModule(t *testing.T) { if _, err := h.ComposeNode(context.Background(), c.Node); err != nil { t.Fatal(err) } - if e, _ := s.Get(RefusedState, "n1.restic@newest"); e != nil { + if e, _ := s.Get(RefusedState, "n1.restic=newest"); e != nil { t.Errorf("the newest hold stays once the composition is read: %s", valueOf(e)) } } diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go index c936414bb..bd7519586 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holder.go @@ -64,7 +64,7 @@ type Refusal struct { Module string `json:"module"` Why string `json:"why"` Since time.Time `json:"since"` - // Scope is "newest" for a module held at its newest build (its composition there unread: key `.@newest`), + // Scope is "newest" for a module held at its newest build (its composition there unread: key `.=newest`), // "" for one held at its wanted build. Scope string `json:"scope,omitempty"` } @@ -364,7 +364,7 @@ func (h *Holder) ComposeNodeWith(ctx context.Context, node string, retireMany bo // of the state is the event a watcher hears. func (h *Holder) writeRefused(node string, held, heldNewest map[string]string) error { var problems []error - // Each hold has a key: `.` at the wanted build, `.@newest` at the newest. + // Each hold has a key: `.` at the wanted build, `.=newest` at the newest. type hold struct { key string r Refusal @@ -381,7 +381,7 @@ func (h *Holder) writeRefused(node string, held, heldNewest map[string]string) e continue } if scope.name != "" { - key += "@" + scope.name + key += "=" + scope.name } holds = append(holds, hold{key, Refusal{Node: node, Module: module, Why: scope.by[module], Since: h.Now().UTC(), Scope: scope.name}}) } diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go b/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go index d8d973002..f1d67b485 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/holder_test.go @@ -731,7 +731,7 @@ func TestARetirementIsJudgedOnlyFromAWholePicture(t *testing.T) { t.Errorf("the pass does not say the key is kept: %v", c.Changed) } // The hold is on the bus, in `refused` beside the wanted build's, so a restart or another instance sees it. - if e, _ := m.s.Get(RefusedState, "novox.de-spiegel@newest"); e == nil || !strings.Contains(string(e.Value), `"scope":"newest"`) || + if e, _ := m.s.Get(RefusedState, "novox.de-spiegel=newest"); e == nil || !strings.Contains(string(e.Value), `"scope":"newest"`) || !strings.Contains(string(e.Value), "not published") { t.Errorf("refused holds %s", valueOf(e)) } diff --git a/modules/mesh-provisioner/cmd/mesh-provisioner/provisioner_test.go b/modules/mesh-provisioner/cmd/mesh-provisioner/provisioner_test.go index df72c7f27..0b1837325 100644 --- a/modules/mesh-provisioner/cmd/mesh-provisioner/provisioner_test.go +++ b/modules/mesh-provisioner/cmd/mesh-provisioner/provisioner_test.go @@ -2,7 +2,9 @@ package main import ( "encoding/json" + "fmt" "os" + "regexp" "slices" "sort" "strings" @@ -32,7 +34,14 @@ func (m *memStore) Get(state, key string) (*Entry, error) { return &Entry{Key: key, Value: append([]byte(nil), v...)}, nil } +// busKey is the bus's key-value key rule (nats.go's validKeyRe): the store refuses what the bus refuses, so a key +// the live bus cannot hold fails here too. +var busKey = regexp.MustCompile(`^[-/_=\.a-zA-Z0-9]+$`) + func (m *memStore) Put(state, key string, value any) error { + if !busKey.MatchString(key) { + return fmt.Errorf("nats: invalid key %q", key) + } raw, err := json.Marshal(value) if err != nil { return err -- 2.54.0