From c24139a0fd9fc1d4aa3f263fc1e8145a1023439f Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 14:33:09 +0200 Subject: [PATCH] Take a build's outcome on the event loop without waiting for the plans, and leave it to the build records with a look asked, so a holder sending for minutes no longer stops the controller listening (issue 1708) --- .../outcome_off_the_loop_test.go | 106 ++++++++++++++++++ cmd/mesh-controller/release_plan.go | 79 ++++++++++++- internal/inventory/hold.go | 5 +- 3 files changed, 183 insertions(+), 7 deletions(-) create mode 100644 cmd/mesh-controller/outcome_off_the_loop_test.go diff --git a/cmd/mesh-controller/outcome_off_the_loop_test.go b/cmd/mesh-controller/outcome_off_the_loop_test.go new file mode 100644 index 00000000..f8bdad1b --- /dev/null +++ b/cmd/mesh-controller/outcome_off_the_loop_test.go @@ -0,0 +1,106 @@ +package main + +import ( + "context" + "testing" + "time" + + "github.com/novox/mesh-controller/internal/inventory" +) + +// heldElsewhere is the plans held by another act for longer than any test runs: waiting for them ends only with the +// context, and a look that does not wait is told at once that they are busy. +func heldElsewhere(ctx context.Context, wait bool) (func(), error) { + if !wait { + return nil, inventory.ErrPlansBusy + } + <-ctx.Done() + return nil, inventory.ErrPlansBusy +} + +// 1708/AC1: given the plans held by another act, when the event loop takes a build's outcome, then it returns at +// once, without waiting the plans' two minutes (on 2026-10-21 at 14:20:45 nodered's outcome held the loop until +// 14:22:45, and the operator was told the controller stopped listening). +func TestAnOutcomeDoesNotWaitForThePlansAnotherActHolds(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second) + defer cancel() + start := time.Now() + release, err := holdPlansForOutcome(ctx, heldElsewhere) + if took := time.Since(start); took > time.Second { + t.Fatalf("taking an outcome waited %s for plans another act holds; the event loop takes nothing meanwhile", took) + } + if err == nil || release != nil { + t.Fatalf("plans another act holds were said to be held: release %v, err %v", release != nil, err) + } +} + +// 1708/AC4: given the plans free, when the event loop takes a build's outcome, then it holds them and marks the plan +// from the outcome directly, as before. +func TestAnOutcomeHoldsThePlansWhenTheyAreFree(t *testing.T) { + let := false + release, err := holdPlansForOutcome(t.Context(), func(context.Context, bool) (func(), error) { + return func() { let = true }, nil + }) + if err != nil || release == nil { + t.Fatalf("free plans were not held: %v", err) + } + release() + if !let { + t.Fatal("the hold given was not the one taken") + } +} + +// withLooker installs a looker that is never run, and answers it, so a test can read whether a look was asked. +func withLooker(t *testing.T) *looker { + t.Helper() + l := newLooker(func(context.Context) bool { return false }) + lookerMu.Lock() + before := theLooker + theLooker = l + lookerMu.Unlock() + t.Cleanup(func() { + lookerMu.Lock() + theLooker = before + lookerMu.Unlock() + }) + return l +} + +// 1708/AC2: given an outcome left to the build records because the plans were held, when it is left, then the look +// 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) + select { + case <-l.signal: + default: + t.Fatal("an outcome left to the build records asked for no look: the plan waits for whatever event comes next") + } +} + +// 1708/AC3: given an outcome left to the records that said what the build did (ADR 0350 §4), when the look settles +// the plan from the build records by that build's id, then the module keeps those words, once. +func TestTheRecordsSettleAnOutcomeWithWhatTheBuildDid(t *testing.T) { + withLooker(t) + asked := time.Date(2026, 10, 21, 12, 15, 0, 0, time.UTC) + plan := func() inventory.Plan { + return inventory.Plan{ID: "plan-1708", Tiers: [][]string{{"nodered"}}, + Modules: map[string]*inventory.PlanModule{"nodered": {State: "asked", AskedAt: &asked, Build: "build-1708-ac3"}}} + } + 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) + + p := plan() + if !settleFromRecords(&p, p.Tiers[0], nil, byID) || p.Modules["nodered"].State != "built" { + t.Fatalf("the records did not settle the outcome: %+v", p.Modules["nodered"]) + } + if got := p.Modules["nodered"].Made; got != "reused from build-1708-check" { + t.Fatalf("settled from the records, the module says %q of what its build did, not the outcome's words", got) + } + again := plan() + settleFromRecords(&again, again.Tiers[0], nil, byID) + if got := again.Modules["nodered"].Made; got != "" { + t.Fatalf("an outcome's words were kept for a second settle: %q", got) + } +} diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 5dea2cea..0ddb310b 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -625,16 +625,81 @@ func planBuilt(ctx context.Context, open *stores, module, commit, failed string, planBuiltSaying(ctx, open, module, commit, failed, asked, id, "") } +// holdPlansForOutcome is how an outcome taken on the event loop holds the plans: **without waiting for them** +// (novox/hq issue 1708). The loop takes one message at a time, and the plans' holder may keep them for minutes while +// it sends machines their declarations; an outcome that waited its turn (up to inventory.HoldWaitFor, two minutes) +// held every report, build and merge behind it for as long, and the watchdog said the controller stopped listening +// (2026-10-21 14:20:45 to 14:22:45). Busy, the outcome is left to the build records, where it already is, and the +// look settles the plan from them (leaveToRecords). +func holdPlansForOutcome(ctx context.Context, hold func(context.Context, bool) (func(), error)) (func(), error) { + return hold(ctx, false) +} + +// leaveToRecords says an outcome was not taken into the plans, which settle it from the build records (novox/hq issue +// 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) { + 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()) + askToLook() +} + +// 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 ( + madeLeftMu sync.Mutex + madeLeft = map[string]madeKept{} +) + +type madeKept struct { + made string + at time.Time +} + +// madeLeftFor is how long words nobody settled are kept: a plan that ended or moved on never asks for them. +const madeLeftFor = time.Hour + +// keepMadeFor keeps what a build left to the records did for the settle that takes it, forgetting words older than +// madeLeftFor. +func keepMadeFor(id, made string, now time.Time) { + if id == "" || made == "" { + return + } + madeLeftMu.Lock() + defer madeLeftMu.Unlock() + for k, v := range madeLeft { + if now.Sub(v.at) > madeLeftFor { + delete(madeLeft, k) + } + } + madeLeft[id] = madeKept{made: made, at: now} +} + +// takeMadeOf is what the build of this id did, if its outcome was left to the records with words; taken once. +func takeMadeOf(id string) string { + madeLeftMu.Lock() + defer madeLeftMu.Unlock() + k, ok := madeLeft[id] + if !ok { + return "" + } + delete(madeLeft, id) + return k.made +} + // planBuiltSaying is planBuilt keeping what the build did — "reused from " or "built: " (novox/hq // ADR 0350 §4) — on the module of the walk it answers. func planBuiltSaying(ctx context.Context, open *stores, module, commit, failed string, asked time.Time, id, made string) { inv := open.inventory - // One controller works the plans at a time (novox/hq issue 213); an outcome waits its turn rather - // than write over what the holder is about to save. Not taken, it is still in the build records, - // which the holder settles the plan from (issue 214). - release, err := inv.HoldPlans(ctx, true) + // One controller works the plans at a time (novox/hq issue 213). Not taken, the outcome is still in the build + // records, which the holder settles the plan from (issue 214). + release, err := holdPlansForOutcome(ctx, inv.HoldPlans) if err != nil { - fmt.Printf("plans: %s's outcome is left to the build records: %v\n", module, err) + leaveToRecords(module, id, made, err) return } defer release() @@ -2531,6 +2596,10 @@ func settleFromRecords(p *inventory.Plan, tier []string, recorded map[string][]i s.State = "built" s.BuiltAt = &at s.Commit = outcome.Commit + // What the build did, when its outcome was left to the records with words (issue 1708). + if made := takeMadeOf(outcome.ID); made != "" { + s.Made = made + } } else { s.State = "failed" s.Why = outcome.Failed diff --git a/internal/inventory/hold.go b/internal/inventory/hold.go index 30f20ac3..272a40c7 100644 --- a/internal/inventory/hold.go +++ b/internal/inventory/hold.go @@ -100,8 +100,9 @@ var ErrPlansBusy = errors.New("another controller is working the plans") // returned function and by the session ending, so a controller that dies holding it holds nothing. // // wait false gives ErrPlansBusy at once when another holds them — the timer's way: the holder is -// moving the plans already. wait true looks again every HoldPoll for up to HoldWaitFor — an -// outcome's or a merge's way, which must be written. +// moving the plans already; and a build outcome's on the event loop, which the build records keep +// for the look to settle (novox/hq issue 1708). wait true looks again every HoldPoll for up to +// HoldWaitFor — a merge's way, which must be written. func (i *Inventory) HoldPlans(ctx context.Context, wait bool) (func(), error) { deadline := time.Now().Add(HoldWaitFor) for {