amadan
fix wave 1: two rollout test flakes (a scheduler swap's v1 ack; the retry attempt's own pointer read)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+35
−1
internal/rollout/manager_test.go+25 −0internal/rollout/wakeplan_test.go+10 −1
diff --git a/internal/rollout/manager_test.go b/internal/rollout/manager_test.go| index 8a569fdde..8506c1cdc 100644 |
| --- a/internal/rollout/manager_test.go |
| +++ b/internal/rollout/manager_test.go |
| @@ -4,6 +4,7 @@ import ( |
| "context" |
| "errors" |
| "fmt" |
| + "runtime" |
| "strings" |
| "sync" |
| "sync/atomic" |
| @@ -1141,6 +1142,11 @@ func TestCloseJoinsAnAttemptCancelledByShutdown(t *testing.T) { |
| // cleared build at once, before it reads the app's next channel, as the |
| // armed pass did before the pointer work was shared (onCleared). Both |
| // channels re-sign v2, so the second read always follows a clear. |
| +// |
| +// Only the pass's reads are counted (fix wave 1, item 6). The requeued |
| +// retry's attempt runs on the scheduler's goroutine and reads the pointer |
| +// too, for its FailedUnder (scheduler.execute, failedUnder); counted, it |
| +// made a third read about once in a hundred runs. |
| func TestAPointerClearRequeuesBeforeTheNextChannelIsRead(t *testing.T) { |
| h := newHarness(t, "a", "b") |
| h.stage("v2") |
| @@ -1156,6 +1162,9 @@ func TestAPointerClearRequeuesBeforeTheNextChannelIsRead(t *testing.T) { |
| var requeuedLate atomic.Bool |
| h.mu.Lock() |
| h.pointerHook = func() { |
| + if !calledFrom("(*Manager).applyPointers") { |
| + return |
| + } |
| if reads.Add(1) != 2 { |
| return |
| } |
| @@ -1483,3 +1492,19 @@ func TestAPlannedWakeIsCountedUntilItsIntentEnds(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// calledFrom reports whether a function whose name ends in fn is on the |
| +// calling goroutine's stack. |
| +func calledFrom(fn string) bool { |
| + pc := make([]uintptr, 64) |
| + frames := runtime.CallersFrames(pc[:runtime.Callers(2, pc)]) |
| + for { |
| + f, more := frames.Next() |
| + if strings.HasSuffix(f.Function, fn) { |
| + return true |
| + } |
| + if !more { |
| + return false |
| + } |
| + } |
| +} |
diff --git a/internal/rollout/wakeplan_test.go b/internal/rollout/wakeplan_test.go| index dd44c694b..e0ced1eba 100644 |
| --- a/internal/rollout/wakeplan_test.go |
| +++ b/internal/rollout/wakeplan_test.go |
| @@ -241,6 +241,14 @@ func TestAnUntriedFirstWakeIsAnOrdinaryWakeOfTheGoodBuild(t *testing.T) { |
| // Spec §6.4: an intentional stop during the 10 s observation records |
| // nothing. GenCancelled only closes the observation. |
| +// |
| +// What it must not publish is the observed build, v2 (fix wave 1, item 6). |
| +// The row's serving build is not required to stay empty: the scheduler last |
| +// saw host a wanting v1 (registerThenInstall), and with the fake activator |
| +// now serving v2 its loop may admit a swap back onto v1 at any moment, whose |
| +// success acks v1 off this goroutine. Asserting "" made the test fail |
| +// whenever that swap won the race (seen 3 in 300 runs beside the pointer |
| +// test). The cancel's own writes are flushed before the row is read. |
| func TestACancelledObservationRecordsNothing(t *testing.T) { |
| h := newHarness(t, "a") |
| h.stage("v2") |
| @@ -248,10 +256,11 @@ func TestACancelledObservationRecordsNothing(t *testing.T) { |
| h.act.serve("a", "v2") |
| g, _ := h.act.Current("a") |
| h.m.OnGen(activator.GenEvent{Kind: activator.GenCancelled, Route: h.rows.get("a"), Gen: g, FirstWake: true}) |
| + h.flushAcks() |
| if _, ok := h.book.Lookup("bab", "blog", "v2"); ok { |
| t.Fatal("a cancelled observation recorded a failure") |
| } |
| - if r := h.rows.get("a"); r.GoodVersion != "v1" || r.ServingVersion != "" { |
| + if r := h.rows.get("a"); r.GoodVersion != "v1" || r.ServingVersion == "v2" { |
| t.Fatalf("row = %+v: a cancelled observation published a build", r) |
| } |
| } |