From 4658e1cb1c6ca56abdfa2935713a7a98977ba53f Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 09:06:39 +0100 Subject: [PATCH] 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