From c07f604047be28350792ffed1a3ffb49237a3480 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Mon, 19 Oct 2026 23:23:23 +0000 Subject: [PATCH] Make the rollout tests wait for the plan look to return before their cleanups restore what it reads, which raced (issue 1338) --- cmd/mesh-controller/away_integration_test.go | 4 +++- cmd/mesh-controller/plan_looks.go | 20 ++++++++++++++----- cmd/mesh-controller/rollout_events_test.go | 12 ++++++++--- .../silent_mid_tier_integration_test.go | 4 +++- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/cmd/mesh-controller/away_integration_test.go b/cmd/mesh-controller/away_integration_test.go index 3cbd81ba..759b7ed9 100644 --- a/cmd/mesh-controller/away_integration_test.go +++ b/cmd/mesh-controller/away_integration_test.go @@ -174,7 +174,9 @@ func TestAPlanGoesOnWithoutAMachineAwayWhenItsTierIsSent(t *testing.T) { }} first, stopFirst := context.WithCancel(ctx) defer stopFirst() - startRolloutEvents(first, open, js) + waitFirst := startRolloutEvents(first, open, js) + // Ends with the test before the cleanups restore what the look reads, which run after it (issue 1338). + t.Cleanup(func() { stopFirst(); waitFirst() }) if err := inv.SavePlan(ctx, &p); err != nil { t.Fatal(err) } diff --git a/cmd/mesh-controller/plan_looks.go b/cmd/mesh-controller/plan_looks.go index f0a28d08..ded34bcb 100644 --- a/cmd/mesh-controller/plan_looks.go +++ b/cmd/mesh-controller/plan_looks.go @@ -138,7 +138,9 @@ func askToLook() { // startPlanLooks starts the look at the open plans, and asks for the first: the controller took the lease, and a // plan left by a replaced controller resumes from there (ADR 0406 §6). It replaces the rollout's 30 second ticker. -func startPlanLooks(ctx context.Context, open *stores) { +// It answers a channel that is closed once the look has returned after the context ended, for a test that restores what +// the look reads (issue 1338). +func startPlanLooks(ctx context.Context, open *stores) <-chan struct{} { l := newLooker(func(ctx context.Context) bool { return lookAtPlans(ctx, open) }) l.first = func(ctx context.Context) { rearmWalkEnds(ctx, open) } lookerMu.Lock() @@ -152,8 +154,13 @@ func startPlanLooks(ctx context.Context, open *stores) { } lookerMu.Unlock() }() - go l.run(ctx) + done := make(chan struct{}) + go func() { + defer close(done) + l.run(ctx) + }() l.ask() + return done } // applySilentAfter is how long a machine may say nothing on a send before the plan names it silent: the mesh's @@ -163,8 +170,10 @@ var applySilentAfter = inventory.ApplySilentAfter // startRolloutEvents starts what a rollout advances on, for as long as the context lives (novox/hq ADR 0406 §5, issue 974): // the wait for each machine's report as a key on the bus that expires (where the bus will not take it a timer of its own // for that wait looks instead), a provisioner client's words, and the look at the plans, asked for first so a plan left -// by a replaced controller resumes. The serving controller's start and the tests of a rollout call this one. -func startRolloutEvents(ctx context.Context, open *stores, js *broker.JetStream) { +// by a replaced controller resumes. The serving controller's start and the tests of a rollout call this one. It answers +// a function that waits until the look at the plans has returned, which only a test that ends the context needs (issue +// 1338): the serving controller ignores it. +func startRolloutEvents(ctx context.Context, open *stores, js *broker.JetStream) (wait func()) { if err := startRolloutSilent(ctx, js); err != nil { fmt.Printf("plans: the wait for the machines' reports could not be put on the bus (%v); each is looked at by a timer of its own\n", err) } @@ -174,7 +183,8 @@ func startRolloutEvents(ctx context.Context, open *stores, js *broker.JetStream) if err := startPlanMovedLooks(ctx, js); err != nil { fmt.Printf("plans: the plans moved by other processes could not be followed (%v)\n", err) } - startPlanLooks(ctx, open) + done := startPlanLooks(ctx, open) + return func() { <-done } } // servingPlanSaved is what the serving controller does for each plan it keeps: said as plan-moved, looked at (a build, a diff --git a/cmd/mesh-controller/rollout_events_test.go b/cmd/mesh-controller/rollout_events_test.go index a90d359c..1109247c 100644 --- a/cmd/mesh-controller/rollout_events_test.go +++ b/cmd/mesh-controller/rollout_events_test.go @@ -179,7 +179,7 @@ func TestARolloutAdvancesOnEventsAndAReplacedControllerResumesIt(t *testing.T) { // The first controller. Its plan has tier 0 built and tier 1 not asked: nothing but the save asks anything. first, stopFirst := context.WithCancel(ctx) defer stopFirst() - startRolloutEvents(first, open, js) + waitFirst := startRolloutEvents(first, open, js) built := time.Now().UTC() p := inventory.Plan{ID: "plan-974", Repository: "novox/mesh-catalog", Branch: "main", Commit: "c2", Created: built, State: inventory.PlanBuilding, Tiers: [][]string{{"a"}, {"b"}}, @@ -199,12 +199,15 @@ func TestARolloutAdvancesOnEventsAndAReplacedControllerResumesIt(t *testing.T) { // The controller is replaced: its looks and its watcher end. anchor reports meanwhile, to nobody. stopFirst() + waitFirst() time.Sleep(300 * time.Millisecond) reports("anchor") second, stopSecond := context.WithCancel(ctx) defer stopSecond() - startRolloutEvents(second, open, js) + waitSecond := startRolloutEvents(second, open, js) + // Ends with the test before the cleanups restore what the look reads, which run after it (issue 1338). + t.Cleanup(func() { stopSecond(); waitSecond() }) // Its first look finds anchor reported, so its key goes; laptop's stays, and is the new controller's to hear expire. eventually(t, "the new controller's first look deletes the key of the machine that reported", func() bool { h := heldKeys() @@ -321,7 +324,7 @@ func TestAWalkLetGoByTheDeliverVerbIsAskedWithNoOtherEvent(t *testing.T) { t.Cleanup(func() { inventory.PlanSaved = wasSaved }) started, stop := context.WithCancel(ctx) defer stop() - startRolloutEvents(started, open, js) + wait := startRolloutEvents(started, open, js) time.Sleep(500 * time.Millisecond) // the first look: the walk waits, and is asked nothing // The verb as its child process runs it: the delivery command, whose saves are not the serving controller's. @@ -333,6 +336,9 @@ func TestAWalkLetGoByTheDeliverVerbIsAskedWithNoOtherEvent(t *testing.T) { return verbAnswer{}, deliveryCommand(ctx, []string{"go", waiting.ID, "--why", "its turn"}) } t.Cleanup(func() { runTheVerb = wasRun }) + // Registered after every variable cleanup above, so it runs before them (last in, first out): the look at the plans + // has returned when checkEvents, inventory.PlanSaved and runTheVerb are restored (issue 1338). + t.Cleanup(func() { stop(); wait() }) handlers, _, err := seatToolHandlers() if err != nil { t.Fatal(err) diff --git a/cmd/mesh-controller/silent_mid_tier_integration_test.go b/cmd/mesh-controller/silent_mid_tier_integration_test.go index ac0a465e..a6ec577d 100644 --- a/cmd/mesh-controller/silent_mid_tier_integration_test.go +++ b/cmd/mesh-controller/silent_mid_tier_integration_test.go @@ -171,7 +171,9 @@ func TestAPlanPassesATierWhoseMachineNeverReportsAfterTheBoundAndSendsTheNextToT }} first, stopFirst := context.WithCancel(ctx) defer stopFirst() - startRolloutEvents(first, open, js) + waitFirst := startRolloutEvents(first, open, js) + // Ends with the test before the cleanups restore what the look reads, which run after it (issue 1338). + t.Cleanup(func() { stopFirst(); waitFirst() }) if err := inv.SavePlan(ctx, &p); err != nil { t.Fatal(err) }