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

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

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

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)

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