From b9ad7a294853207950a04f749000c130e149aaa9 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 4 Oct 2026 03:27:32 +0200 Subject: [PATCH] Review before merge: refuse a silent disagreement, and bound the sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things found reading this back, each of which would have been quiet. A consumer that keeps several holders of one provision (ADR 0094) gets a login per holder, and a provider derives from the login — so it would make a resource per holder while the consumer is told one value for the requirement. That is issue 124's own failure one case to the side: authenticate, then be refused on every object. Refused now, naming both ends. The sweep runs inside somebody's build and was unbounded. At most two hundred artifacts and sixty seconds, stopping at the first refusal because a store that refuses one refuses all; the rest is offered again next build. The citation and migration renumbers are in the commit before this one. --- cmd/mesh-controller/collect.go | 54 +++++++++++++------ internal/catalogue/consumer_into_serves.go | 23 +++++++- internal/catalogue/declaration.go | 2 +- .../catalogue/derived_for_consumer_test.go | 46 ++++++++++++++++ 4 files changed, 108 insertions(+), 17 deletions(-) diff --git a/cmd/mesh-controller/collect.go b/cmd/mesh-controller/collect.go index c90e98a..5464800 100644 --- a/cmd/mesh-controller/collect.go +++ b/cmd/mesh-controller/collect.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "time" "github.com/novox/mesh-controller/internal/artifacts" "github.com/novox/mesh-controller/internal/inventory" @@ -49,28 +50,43 @@ func collect(ctx context.Context, inv *inventory.Inventory) { return } + // **Bounded, because this runs inside somebody's build.** The first sweep of a mesh that has + // never collected has the whole history to get through, and a person waiting on `build` should + // not pay for it. Two bounds, and what is left over is simply offered again next time — + // builds are frequent, and the point is that the store stops growing, not that it empties + // tonight. + within, stop := context.WithTimeout(ctx, sweepBudget) + defer stop() store := artifacts.Store{Address: address} + var done []string - var refused int - for _, reference := range references { - switch err := store.LetGo(ctx, reference); { - case err == nil, errors.Is(err, artifacts.Gone): + var left int + for i, reference := range references { + if i >= mostPerSweep || within.Err() != nil { + left = len(references) - i + break + } + err := store.LetGo(within, reference) + if err == nil || errors.Is(err, artifacts.Gone) { // Gone is the outcome wanted, already true. Recorded so the next sweep does not ask // again for ever. done = append(done, reference) - default: - refused++ - if refused == 1 { - // Once per sweep. A store that refuses one refuses all of them, and a hundred - // identical lines would bury the reason. - fmt.Fprintf(os.Stderr, "the artifact store kept %s: %v\n", reference, err) - } + continue } + // **Stopped at the first refusal, not pushed through.** A store that refuses one refuses + // all of them — deletion disabled, the store down, the network gone — so going on would + // be a hundred identical failures and a hundred identical log lines in front of whoever + // was building something. + fmt.Fprintf(os.Stderr, "the artifact store kept %s, so nothing more was asked of it: %v\n", + reference, err) + left = len(references) - i + break } + if len(done) > 0 { + // Recorded outside `within`: the deletions happened, and losing the record of them because + // the sweep ran out of budget would mean asking about them again for ever. if err := inv.MarkCollected(ctx, done); err != nil { - // Said, and that is all: the artifacts are gone either way, and the only cost of an - // unrecorded collection is that the next sweep asks about them again. fmt.Fprintf(os.Stderr, "the store let go of %d artifact(s) and the record of it did not keep: %v\n", len(done), err) return @@ -78,7 +94,15 @@ func collect(ctx context.Context, inv *inventory.Inventory) { fmt.Fprintf(os.Stderr, "the artifact store let go of %d artifact(s) the mesh no longer keeps\n", len(done)) } - if refused > 0 { - fmt.Fprintf(os.Stderr, "%d artifact(s) were not collected; the next build asks again\n", refused) + if left > 0 { + fmt.Fprintf(os.Stderr, "%d more to collect; the next build asks again\n", left) } } + +// mostPerSweep is how many artifacts one sweep will ask about. Enough that a mesh building +// several times a day converges within days of this landing; small enough that no single build +// waits on the whole backlog. +const mostPerSweep = 200 + +// sweepBudget is the longest a sweep will keep a build waiting. +const sweepBudget = 60 * time.Second diff --git a/internal/catalogue/consumer_into_serves.go b/internal/catalogue/consumer_into_serves.go index 1a40bfb..897411d 100644 --- a/internal/catalogue/consumer_into_serves.go +++ b/internal/catalogue/consumer_into_serves.go @@ -177,7 +177,7 @@ func sortedAnyKeys(values map[string]any) []string { // choice servedOnThisMachine makes for the consumer's half. Nothing serving it on this machine is // not an error: a contribution can reach a machine whose provider is a record or an adapter, and // then there is nothing derived to tell. -func (r Resolution) derivedFor(provision, as string, settings SettingsBy) (map[string]any, error) { +func (r Resolution) derivedFor(provision, as, consumer, local string, settings SettingsBy) (map[string]any, error) { for _, m := range r.Modules { serves, said := m.Serves[provision] if !said { @@ -195,6 +195,27 @@ func (r Resolution) derivedFor(provision, as string, settings SettingsBy) (map[s if names == nil { return nil, nil } + // **A consumer that keeps several holders of this provision is refused** — this is issue + // 124's own failure one case to the side, and it would be just as quiet. + // + // Each holder gets its own login, `…_` (ADR 0094), and a provider derives from the + // login, so it would make one resource per holder. The consumer's side has no such + // dimension: one binding file per provision, one `${bound::}`, both + // derived from the un-suffixed identity. So the provider would create the holder's + // resource and the consumer would be configured against a name nothing made — it would + // authenticate successfully and be refused on every object, which reads like a credential + // fault and is not one. + // + // Lifting this means giving the consumer's side a local dimension. That is a decision, + // not an omission, and until it is taken the mesh says so rather than guessing. + if local != "" { + return nil, fmt.Errorf( + "%s keeps several holders of %s (this one is %q), and %s derives %s for each "+ + "consumer from the login the mesh minted. Each holder has its own login, and a "+ + "consumer is told one value per requirement — so the two ends would name "+ + "different things and nothing would compare them (novox/hq ADR 0201)", + consumer, local, provision, m.Module, orNothing(sortedAnyKeys(names))) + } settled, err := Settle(names, settings[m.Module]) if err != nil { return nil, fmt.Errorf("%s serving %s: %w", m.Module, provision, err) diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 1e5c78e..b2f651b 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -1307,7 +1307,7 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, continue } as := holderAs(ConsumerIdentity(g.Consumer, IdentitySource(g.Slug, g.From)), g.Local) - derived, err := r.derivedFor(g.Provision, as, settings) + derived, err := r.derivedFor(g.Provision, as, g.From, g.Local, settings) if err != nil { return nil, err } diff --git a/internal/catalogue/derived_for_consumer_test.go b/internal/catalogue/derived_for_consumer_test.go index 20a8da4..5d85206 100644 --- a/internal/catalogue/derived_for_consumer_test.go +++ b/internal/catalogue/derived_for_consumer_test.go @@ -295,3 +295,49 @@ func storeGrants(t *testing.T, out []map[string]any) []Contribution { t.Fatalf("the provider was given no contributions file: %v", out) return nil } + +// A consumer that keeps SEVERAL holders of one provision is refused, rather than told one thing +// while its provider is told another. +// +// **This is issue 124's own failure, one case to the side.** The mesh gives each holder its own +// login — `mesh_node_mod_` (ADR 0094) — and the provider derives from the login, so it +// would make one resource per holder. The consumer's side has no such dimension: there is one +// binding file per provision and one `${bound::}`, both derived from the +// un-suffixed identity. So the provider would create `…-mod-cold` and the consumer would be +// configured against `…-mod`: it would authenticate successfully and be refused on every object, +// which is exactly the fault this whole record exists to end. +// +// Refused, loudly, at the one place that can see both halves. Lifting it means giving the +// consumer's side a local dimension, which is a decision and not an omission. +func TestAConsumerWithSeveralHoldersOfADerivingProviderIsRefused(t *testing.T) { + m := files() + // Two holders of the one provision, the shape ADR 0094 gives a module that keeps several. + m.Secrets = nil + m.SecretsMany = map[string]map[string]string{"s3-bucket": { + "hot": "/var/lib/files/hot.secret", + "cold": "/var/lib/files/cold.secret", + }} + m.Resources = []map[string]any{{ + "id": "env", "type": "file", "path": "/var/lib/files/env", "mode": "0600", + "content": "BUCKET=${bound:s3-bucket:bucket}\n", + }} + r, err := Resolve(shelf(store(), m), []string{"store", "files"}, reachable(), World{}) + if err != nil { + t.Fatal(err) + } + _, err = r.Declaration(Rendering{Grants: []Grant{ + {Provision: "s3-bucket", Consumer: "workstation", From: "files", Slug: "files", + Local: "hot", Values: map[string]any{}, Sealed: "c2VhbGVk"}, + {Provision: "s3-bucket", Consumer: "workstation", From: "files", Slug: "files", + Local: "cold", Values: map[string]any{}, Sealed: "c2VhbGVk"}, + }}) + if err == nil { + t.Fatal("a consumer with several holders of a deriving provider was accepted; " + + "its two ends would have disagreed in silence") + } + for _, want := range []string{"files", "s3-bucket", "bucket"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal does not name %q: %v", want, err) + } + } +}