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)

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

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.

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()
+);

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

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

testdata/store-tests

@@ -167,3 +167,4 @@
 ./cmd/mesh-controller TestAMergeOnTheApproveIntoANewRepositoryAsksTheFirstBuildAndItsOutcomeRegistersTheModule
 ./cmd/mesh-controller TestTheApprovedFirstBuildPassesTheGuardOfAVerbAndIsQueued
 ./cmd/mesh-controller TestAPlanGoesOnWithoutAMachineAwayWhenItsTierIsSent
+./internal/inventory TestARefusedRegistrationIsKeptByItsBuild