From 0e0c775bfa5e20e8d1dce0c664c17db8305a26f6 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 15:03:23 +0200 Subject: [PATCH] Keep a refused registration left to the build records, in the store and the process, so the look fails the plan rather than settling it built; and forget a deleted module and say what was never sent when the records settle (issue 1708) --- .../outcome_off_the_loop_test.go | 60 +++++++- cmd/mesh-controller/release_plan.go | 129 +++++++++++++++++- .../0107-a-refused-registration-is-kept.sql | 10 ++ internal/inventory/refused.go | 33 +++++ internal/inventory/refused_test.go | 28 ++++ testdata/store-tests | 1 + 6 files changed, 257 insertions(+), 4 deletions(-) create mode 100644 internal/inventory/migrations/0107-a-refused-registration-is-kept.sql create mode 100644 internal/inventory/refused.go create mode 100644 internal/inventory/refused_test.go diff --git a/cmd/mesh-controller/outcome_off_the_loop_test.go b/cmd/mesh-controller/outcome_off_the_loop_test.go index 940745d9..6b52e126 100644 --- a/cmd/mesh-controller/outcome_off_the_loop_test.go +++ b/cmd/mesh-controller/outcome_off_the_loop_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "strings" "testing" "time" @@ -70,7 +71,7 @@ func withLooker(t *testing.T) *looker { // at the plans is asked for, so the records settle it once the holder lets go rather than at the next event. func TestAnOutcomeLeftToTheRecordsAsksForTheLook(t *testing.T) { l := withLooker(t) - leaveToRecords("nodered", "build-1708-ac2", "", inventory.ErrPlansBusy) + leaveToRecords("nodered", "build-1708-ac2", "", "", inventory.ErrPlansBusy) select { case <-l.signal: default: @@ -89,7 +90,7 @@ func TestTheRecordsSettleAnOutcomeWithWhatTheBuildDid(t *testing.T) { } byID := map[string]inventory.Build{"build-1708-ac3": {ID: "build-1708-ac3", Module: "nodered", Commit: "4efac366", At: asked.Add(5 * time.Minute)}} - leaveToRecords("nodered", "build-1708-ac3", "reused from build-1708-check", inventory.ErrPlansBusy) + leaveToRecords("nodered", "build-1708-ac3", "reused from build-1708-check", "", inventory.ErrPlansBusy) p := plan() if !settleFromRecords(&p, p.Tiers[0], nil, byID) || p.Modules["nodered"].State != "built" { @@ -140,3 +141,58 @@ func TestAnOutcomeMovesThePlansWhereNoLookRuns(t *testing.T) { t.Fatal("with no look running, an outcome left the plans unmoved") } } + +// 1708/AC6: given a registration the mesh refused (its build recorded as worked, the outcome failed), left to the +// records because the plans were held, when the look settles the plan from the records, then the module and the plan +// are failed, never built: settled as built, the tier would be sent the version from before (reviewer-1708@shanks). +func TestARefusedRegistrationLeftToTheRecordsFailsThePlan(t *testing.T) { + withLooker(t) + asked := time.Date(2026, 10, 21, 12, 15, 0, 0, time.UTC) + p := inventory.Plan{ID: "plan-1708-ac6", State: inventory.PlanBuilding, Tiers: [][]string{{"nodered"}}, + Modules: map[string]*inventory.PlanModule{"nodered": {State: "asked", AskedAt: &asked, Build: "build-1708-ac6"}}} + byID := map[string]inventory.Build{"build-1708-ac6": {ID: "build-1708-ac6", Module: "nodered", Commit: "4efac366", + At: asked.Add(5 * time.Minute)}} + leaveToRecords("nodered", "build-1708-ac6", "", "the mesh does not register it: a port is taken", inventory.ErrPlansBusy) + + overlayRefusals(nil, byID, refusalsKeptFor([]string{"build-1708-ac6"})) + settleFromRecords(&p, p.Tiers[0], nil, byID) + if s := p.Modules["nodered"]; s.State != "failed" || p.State != inventory.PlanFailed { + t.Fatalf("a refused registration left to the records settled as %q, the plan %q", s.State, p.State) + } + if !strings.Contains(p.Modules["nodered"].Why, "a port is taken") { + t.Fatalf("the refusal's words were lost: %q", p.Modules["nodered"].Why) + } +} + +// 1708/AC6: a refusal kept in the store (a controller replaced before the settle) fails the record as one kept in +// the process does. +func TestARefusalFromTheStoreFailsTheRecord(t *testing.T) { + recorded := map[string][]inventory.Build{"nodered": {{ID: "b-store", Module: "nodered"}, {ID: "b-other", Module: "nodered"}}} + byID := map[string]inventory.Build{"b-store": {ID: "b-store", Module: "nodered"}} + overlayRefusals(recorded, byID, map[string]string{"b-store": "not registered"}) + if recorded["nodered"][0].Worked() || byID["b-store"].Worked() { + t.Fatal("a build whose registration was refused still reads as worked") + } + if !recorded["nodered"][1].Worked() { + t.Fatal("a build nobody refused was failed") + } +} + +// 1708/AC7: given the records settle a module as deleted at its source, or fail the plan, when the look settles, then +// it does what taking the outcome directly does: the deleted module is named to be forgotten, and the plan's failure +// is said (reviewer-1708@shanks). +func TestSettlingFromTheRecordsDoesWhatTakingTheOutcomeDoes(t *testing.T) { + p := inventory.Plan{State: inventory.PlanFailed, Modules: map[string]*inventory.PlanModule{ + "gone": {State: planDeleted}, "x": {State: "failed"}, "y": {State: "built"}}} + before := map[string]string{"gone": "asked", "x": "asked", "y": "built"} + deleted, failed := settledEffects(before, &p, inventory.PlanBuilding) + if len(deleted) != 1 || deleted[0] != "gone" { + t.Fatalf("the module the records found deleted is not forgotten: %v", deleted) + } + if !failed { + t.Fatal("the plan the records failed is not said as failed") + } + if d, f := settledEffects(map[string]string{"gone": planDeleted}, &p, inventory.PlanFailed); len(d) != 0 || f { + t.Fatalf("what was deleted or failed before the settle is acted on again: %v %v", d, f) + } +} diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 7112e812..7331626c 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -639,15 +639,52 @@ func holdPlansForOutcome(ctx context.Context, hold func(context.Context, bool) ( // 214). When another act holds the plans it asks for the look, which waits for them off the event loop and settles // the plan from the records once they are let go (issue 1708); what the build did is kept for that settle by the // build's id, so the walk's module says it as it would have (ADR 0350 §4). -func leaveToRecords(module, id, made string, err error) { +func leaveToRecords(module, id, made, failed string, err error) { fmt.Printf("plans: %s's outcome is left to the build records: %v\n", module, err) if !errors.Is(err, inventory.ErrPlansBusy) { return } keepMadeFor(id, made, time.Now()) + // A refused outcome (failed, its build recorded as worked) is kept as refused, or the records settle it built. + keepRefusalFor(id, failed, time.Now()) askToLook() } +// refusalsKeptFor is the refusals kept in this process for these builds (keepRefusalFor), by id. +func refusalsKeptFor(ids []string) map[string]string { + out := map[string]string{} + refusedLeftMu.Lock() + defer refusedLeftMu.Unlock() + for _, id := range ids { + if k, ok := refusedLeft[id]; ok { + out[id] = k.made + } + } + return out +} + +// refusedLeft is why each outcome left to the records was refused, by the build's id: kept in the process beside the +// store's record (RecordRefused), so a store that did not take it still fails the plan while this controller serves. +var ( + refusedLeftMu sync.Mutex + refusedLeft = map[string]madeKept{} +) + +// keepRefusalFor keeps why an outcome left to the records was refused, forgetting refusals older than madeLeftFor. +func keepRefusalFor(id, why string, now time.Time) { + if id == "" || why == "" { + return + } + refusedLeftMu.Lock() + defer refusedLeftMu.Unlock() + for k, v := range refusedLeft { + if now.Sub(v.at) > madeLeftFor { + delete(refusedLeft, k) + } + } + refusedLeft[id] = madeKept{made: why, at: now} +} + // madeLeft is what each build left to the records did, by the build's id, until a settle takes it: kept in the // process, so a controller replaced before the settle loses only these words, never the outcome. var ( @@ -699,7 +736,13 @@ func planBuiltSaying(ctx context.Context, open *stores, module, commit, failed s // records, which the holder settles the plan from (issue 214). release, err := holdPlansForOutcome(ctx, inv.HoldPlans) if err != nil { - leaveToRecords(module, id, made, err) + if failed != "" && id != "" && errors.Is(err, inventory.ErrPlansBusy) { + // In the store too, so a controller replaced before the look settles still fails the plan (issue 1708). + if err := inv.RecordRefused(ctx, id, module, failed); err != nil { + fmt.Printf("plans: %s's refusal could not be kept in the store, only in this controller: %v\n", module, err) + } + } + leaveToRecords(module, id, made, failed, err) return } defer release() @@ -998,7 +1041,17 @@ func advanceOnce(ctx context.Context, open *stores, p *inventory.Plan, if err != nil { return false, err } + before, was := statesOf(p), p.State if settleFromRecords(p, tier, recorded, byID) { + // As taking the outcome directly does (planBuiltSaying): a module deleted at its source is forgotten, and a + // failed plan says what it built and never sent (issue 1708). + deleted, failed := settledEffects(before, p, was) + for _, m := range deleted { + forgetDeleted(ctx, inv, m, p) + } + if failed { + sayUnsent(p, rollsOut) + } return true, nil } // Asked: wait for every build. @@ -2556,9 +2609,81 @@ func recordsOfAsked(ctx context.Context, inv *inventory.Inventory, p *inventory. } } } + refused, err := refusalsOf(ctx, inv, recorded, byID) + if err != nil { + return nil, nil, err + } + overlayRefusals(recorded, byID, refused) return recorded, byID, nil } +// overlayRefusals marks failed every build record whose registration the mesh refused (novox/hq issue 1708): the record +// keeps the builder's words, which worked, and settled from it as it is the refused build would be built and sent. +func overlayRefusals(recorded map[string][]inventory.Build, byID map[string]inventory.Build, refused map[string]string) { + if len(refused) == 0 { + return + } + fail := func(b *inventory.Build) { + if why, ok := refused[b.ID]; ok && b.Worked() { + b.Failed = why + } + } + for m := range recorded { + for i := range recorded[m] { + fail(&recorded[m][i]) + } + } + for id, b := range byID { + fail(&b) + byID[id] = b + } +} + +// refusalsOf is every refusal kept for the builds these records name, in the store and in this process. +func refusalsOf(ctx context.Context, inv *inventory.Inventory, recorded map[string][]inventory.Build, + byID map[string]inventory.Build) (map[string]string, error) { + var ids []string + for _, bs := range recorded { + for _, b := range bs { + ids = append(ids, b.ID) + } + } + for id := range byID { + ids = append(ids, id) + } + refused, err := inv.RefusedOf(ctx, ids) + if err != nil { + return nil, fmt.Errorf("the refused registrations cannot be read: %w", err) + } + for id, why := range refusalsKeptFor(ids) { + refused[id] = why + } + return refused, nil +} + +// settledEffects is what settling from the records did that needs more than the plan: the modules it found deleted +// at their source, and whether it failed the plan. +func settledEffects(before map[string]string, p *inventory.Plan, was string) (deleted []string, failed bool) { + for m, s := range p.Modules { + if s != nil && s.State == planDeleted && before[m] != planDeleted { + deleted = append(deleted, m) + } + } + sort.Strings(deleted) + return deleted, p.State == inventory.PlanFailed && was != inventory.PlanFailed +} + +// statesOf is each module's state in a plan, for settledEffects. +func statesOf(p *inventory.Plan) map[string]string { + out := map[string]string{} + for m, s := range p.Modules { + if s != nil { + out[m] = s.State + } + } + return out +} + // settleFromRecords marks every module of the tier still `asked` built — or failed — from a build // recorded after it was asked, and says whether it changed anything (novox/hq 04-ISSUES/214). // Newest first, as Builds answers: the first record after the ask is the outcome of that ask. diff --git a/internal/inventory/migrations/0107-a-refused-registration-is-kept.sql b/internal/inventory/migrations/0107-a-refused-registration-is-kept.sql new file mode 100644 index 00000000..f0b606a3 --- /dev/null +++ b/internal/inventory/migrations/0107-a-refused-registration-is-kept.sql @@ -0,0 +1,10 @@ +-- A build's outcome whose registration the mesh refused, by the build's id (novox/hq issue 1708). The build record says +-- what the builder said, which worked; an outcome the event loop could not take into the plans (another act held them) +-- is settled from the records by the look, and without this it would read the refused build as built. Kept in the +-- store so a controller replaced before the settle still fails the plan. +create table build_refused ( + id text primary key, + module text not null, + why text not null, + at timestamptz not null default now() +); diff --git a/internal/inventory/refused.go b/internal/inventory/refused.go new file mode 100644 index 00000000..f07809fb --- /dev/null +++ b/internal/inventory/refused.go @@ -0,0 +1,33 @@ +package inventory + +import "context" + +// RecordRefused keeps that the mesh refused to register a build's outcome, and why (novox/hq issue 1708): the build +// record says the builder's words, which worked, and the look that settles a plan from the records reads this to fail +// it instead. The first refusal of an id stands. +func (i *Inventory) RecordRefused(ctx context.Context, id, module, why string) error { + _, err := i.store.Pool().Exec(ctx, + `insert into build_refused (id, module, why) values ($1, $2, $3) on conflict (id) do nothing`, id, module, why) + return err +} + +// RefusedOf is why each of these builds' registration was refused, by id; a build not refused is absent. +func (i *Inventory) RefusedOf(ctx context.Context, ids []string) (map[string]string, error) { + out := map[string]string{} + if len(ids) == 0 { + return out, nil + } + rows, err := i.store.Pool().Query(ctx, `select id, why from build_refused where id = any($1)`, ids) + if err != nil { + return nil, err + } + defer rows.Close() + for rows.Next() { + var id, why string + if err := rows.Scan(&id, &why); err != nil { + return nil, err + } + out[id] = why + } + return out, rows.Err() +} diff --git a/internal/inventory/refused_test.go b/internal/inventory/refused_test.go new file mode 100644 index 00000000..f38f7ea3 --- /dev/null +++ b/internal/inventory/refused_test.go @@ -0,0 +1,28 @@ +//go:build integration + +package inventory + +import "testing" + +// novox/hq issue 1708 (1708/AC6): a refused registration is kept by its build's id, the first refusal standing, and +// read back for the builds asked about only. +func TestARefusedRegistrationIsKeptByItsBuild(t *testing.T) { + inv := ForTest(t) + ctx := t.Context() + if err := inv.RecordRefused(ctx, "build-1708-a", "nodered", "a port is taken"); err != nil { + t.Fatal(err) + } + if err := inv.RecordRefused(ctx, "build-1708-a", "nodered", "said again"); err != nil { + t.Fatalf("a refusal said twice was refused: %v", err) + } + got, err := inv.RefusedOf(ctx, []string{"build-1708-a", "build-1708-b"}) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || got["build-1708-a"] != "a port is taken" { + t.Fatalf("the refusals read back are %v", got) + } + if none, err := inv.RefusedOf(ctx, nil); err != nil || len(none) != 0 { + t.Fatalf("no builds asked about answered %v %v", none, err) + } +} diff --git a/testdata/store-tests b/testdata/store-tests index a16b7f30..93092c0f 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -167,3 +167,4 @@ ./cmd/mesh-controller TestAMergeOnTheApproveIntoANewRepositoryAsksTheFirstBuildAndItsOutcomeRegistersTheModule ./cmd/mesh-controller TestTheApprovedFirstBuildPassesTheGuardOfAVerbAndIsQueued ./cmd/mesh-controller TestAPlanGoesOnWithoutAMachineAwayWhenItsTierIsSent +./internal/inventory TestARefusedRegistrationIsKeptByItsBuild