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

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) {

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 `<node>.<module>@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-<node>` (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: `<node>.<module>` at the wanted build, `<node>.<module>@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 "<node>.<module>", 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()

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) }

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)