From 2fb1dc3214f464307929e792d9ae6306f38bde19 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 2 Nov 2026 19:37:22 +0100 Subject: [PATCH] mesh-delivery-checker: a placement that cannot be read leaves the composition not made, said Review finding 4: an unreadable placement gave no nodes, so nobody was asked and the check passed. The placement read now answers its error, and the verdict says the composition was not made and why. Claude-Session: https://claude.ai/code/session_01NcbgfJ9Gum47zkFKWRaovL --- .../cmd/mesh-delivery-checker/engines.go | 20 ++++++++++--------- .../cmd/mesh-delivery-checker/main.go | 5 ++--- .../internal/check/engines.go | 9 ++++++--- .../internal/check/engines_test.go | 17 ++++++++++++++-- .../internal/check/judge.go | 2 ++ 5 files changed, 36 insertions(+), 17 deletions(-) diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go index bc193f5b..22c6de0e 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "sort" "sync" @@ -51,24 +52,25 @@ func (e engineAsks) ComposeCandidate(ctx context.Context, node string, ask deliv } // placedNodes names every node the seat mesh-placement places now, read from its state when a check composes (no -// watch: a check reads it once). A state that cannot be read places no node, said in the journal. +// watch: a check reads it once). A state that cannot be read is an error, so the check says its composition was not +// made rather than passing with no node asked (review finding 4). type placedNodes struct { - kv func(ctx context.Context) (jetstream.KeyValue, error) - logf func(string, ...any) + kv func(ctx context.Context) (jetstream.KeyValue, error) } -func (p placedNodes) Placed() []string { +func (p placedNodes) Placed() ([]string, error) { ctx, cancel := context.WithTimeout(context.Background(), delivery.ComposeAnswerBound) defer cancel() kv, err := p.kv(ctx) if err != nil { - p.logf("the state %s cannot be read, so no node is asked to compose: %v", delivery.PlacementBucket, err) - return nil + return nil, fmt.Errorf("the state %s cannot be read: %w", delivery.PlacementBucket, err) } lister, err := kv.ListKeys(ctx) + if errors.Is(err, jetstream.ErrNoKeysFound) { + return nil, nil // a placement that places nothing + } if err != nil { - p.logf("the state %s cannot be listed, so no node is asked to compose: %v", delivery.PlacementBucket, err) - return nil + return nil, fmt.Errorf("the state %s cannot be listed: %w", delivery.PlacementBucket, err) } seen := map[string]bool{} for key := range lister.Keys() { @@ -81,7 +83,7 @@ func (p placedNodes) Placed() []string { out = append(out, n) } sort.Strings(out) - return out + return out, nil } // seatSandbox runs a compose job on the Go build seat's check stream, as the checker's own ask, and answers the diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go index 1f03586b..a5fbef99 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go @@ -176,9 +176,8 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro } verdicts := &manifestVerdicts{} compositions := check.EngineCompositions{ - Asker: engineAsks{call: caller.CallOn}, - Placed: placedNodes{kv: func(at context.Context) (jetstream.KeyValue, error) { return js.KeyValue(at, delivery.PlacementBucket) }, - logf: logger.Printf}.Placed, + Asker: engineAsks{call: caller.CallOn}, + Placed: placedNodes{kv: func(at context.Context) (jetstream.KeyValue, error) { return js.KeyValue(at, delivery.PlacementBucket) }}.Placed, Bound: answerBound, Sandbox: seatSandbox{asker: buildrun.Asker{Publisher: asks, Waiter: waiter, Bound: stepBound}}, Verdicts: verdicts.all, diff --git a/modules/mesh-delivery-checker/internal/check/engines.go b/modules/mesh-delivery-checker/internal/check/engines.go index aef0ef10..64c8579b 100644 --- a/modules/mesh-delivery-checker/internal/check/engines.go +++ b/modules/mesh-delivery-checker/internal/check/engines.go @@ -39,8 +39,9 @@ type Sandbox interface { // EngineCompositions is the checker's composition of a check (Examiner.Compositions). type EngineCompositions struct { Asker EngineAsker - // Placed names every node in placement now (the seat mesh-placement's state). - Placed func() []string + // Placed names every node in placement now (the seat mesh-placement's state), or why it cannot be read: then no node + // can be asked, and the composition is not made (an error), never passed with nobody asked. + Placed func() ([]string, error) // Bound is the answer bound (the setting compose-answer-bound); zero is delivery.ComposeAnswerBound. Bound time.Duration // Composer is the commit of the node-engine the mesh runs, named as what composed (ADR 0548 ยง6's running commit). @@ -71,7 +72,9 @@ func (e EngineCompositions) Compose(ctx context.Context, c *Case) (delivery.Comp } var nodes []string if e.Placed != nil { - nodes = e.Placed() + if nodes, err = e.Placed(); err != nil { + return delivery.CompositionVerdict{}, fmt.Errorf("the placement cannot be read, so no node can be asked: %w", err) + } } bound := e.Bound if bound <= 0 { diff --git a/modules/mesh-delivery-checker/internal/check/engines_test.go b/modules/mesh-delivery-checker/internal/check/engines_test.go index 29cee46a..52e98c68 100644 --- a/modules/mesh-delivery-checker/internal/check/engines_test.go +++ b/modules/mesh-delivery-checker/internal/check/engines_test.go @@ -81,7 +81,7 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { key, _ := delivery.CompositionKey(cs.Group, cs.HeadsHash) compose := func(e *fakeEngines, nodes ...string) delivery.CompositionVerdict { t.Helper() - ec := EngineCompositions{Asker: e, Placed: func() []string { return nodes }, Bound: 50 * time.Millisecond, + ec := EngineCompositions{Asker: e, Placed: func() ([]string, error) { return nodes, nil }, Bound: 50 * time.Millisecond, Composer: func() string { return strings.Repeat("7", 40) }} v, err := ec.Compose(context.Background(), cs) if err != nil { @@ -149,7 +149,7 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { return composer.Verdict{V: composer.Version, Cases: []composer.CaseVerdict{{Name: "change", Machines: map[string]composer.Machine{node: {Composes: false, Reason: "the new composer refuses it"}}}}} }} - ec := EngineCompositions{Asker: e, Placed: func() []string { return []string{"anchor"} }, Bound: time.Second, + ec := EngineCompositions{Asker: e, Placed: func() ([]string, error) { return []string{"anchor"}, nil }, Bound: time.Second, Sandbox: sb, Manifests: digests{"web": "sha256:" + strings.Repeat("c", 64)}} v, err := ec.Compose(context.Background(), host) if err != nil { @@ -178,4 +178,17 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { } }) + // Review finding 4: a placement that cannot be read is no mesh with no node: the composition is not made, said. + t.Run("an unreadable placement makes no composition", func(t *testing.T) { + ec := EngineCompositions{Asker: &fakeEngines{}, Placed: func() ([]string, error) { return nil, errors.New("no such bucket") }} + if _, err := ec.Compose(context.Background(), cs); err == nil || !strings.Contains(err.Error(), "no such bucket") { + t.Errorf("an unreadable placement: %v", err) + } + b := newBench(t) + b.ex.Compositions = ec + v := b.judge(t, cs).Verdict + if v.Composition == nil || v.Composition.State != delivery.CompositionNotRun || !strings.Contains(v.Composition.Words, "no such bucket") { + t.Errorf("the verdict's composition = %+v", v.Composition) + } + }) } diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index 53285764..b561a1fb 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -279,7 +279,9 @@ func (st *state) composed(ctx context.Context, v *delivery.Verdict) { } comp, err := st.j.Compositions.Compose(ctx, st.c) if err != nil { + // Said in the verdict, never left as a composition not yet run (review finding 4). st.j.logf("%s: the composition could not be made, so it is said as not run: %v", st.c.Key, err) + v.Composition = &delivery.Composition{State: delivery.CompositionNotRun, Words: "not made: " + err.Error()} return } withComposition(v, comp)