amadan
zdd-readiness: final review round 1, a host retry keeps its canary for the retried build
Finding 5 (Astra, Medium): RetryHost bound the queue's forced retry only when the slot wanted the retried build B at that moment, so a retry of B while the slot wanted A left the host exempt in the book but with no admission through B's halt. The entitlement (hostSlot.forceBuild) is now bound to B whenever the retry clears B's record, survives attempts onto other builds, restarts and config attempts, and is spent only by the forced adoption onto B; rearmRetry gives it back even when the desire has moved on.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 files changed,
+147
−31
internal/rollout/admit_test.go+21 −13internal/rollout/finalreview1_test.go+86 −0internal/rollout/manager.go+10 −2internal/rollout/queue.go+30 −16
diff --git a/internal/rollout/admit_test.go b/internal/rollout/admit_test.go| index afe72fd4f..b4754b22f 100644 |
| --- a/internal/rollout/admit_test.go |
| +++ b/internal/rollout/admit_test.go |
| @@ -178,7 +178,7 @@ func TestARetriedInstanceRunsAsACanaryThroughAHalt(t *testing.T) { |
| if ds := q.admit(currentView(q), t0); len(ds) != 0 { |
| t.Fatalf("admitted %+v from a halted cohort", ds) |
| } |
| - q.retryHost("b") |
| + q.retryHost("b", "v2") |
| ds := q.admit(currentView(q), t0) |
| if len(ds) != 1 || ds[0].ID.Host != "b" || !ds[0].Canary { |
| t.Fatalf("admitted %+v, want b alone, as a canary", ds) |
| @@ -252,7 +252,7 @@ func TestARetryThenAnUnhaltLeavesOneCanaryUntilItSettles(t *testing.T) { |
| q.concurrency = 2 |
| releaseAll(q, "v2", "a", "b", "c") |
| q.halt("v2", "health check failed") |
| - q.retryHost("b") |
| + q.retryHost("b", "v2") |
| forced := q.admit(currentView(q), t0) |
| if len(forced) != 1 || forced[0].ID.Host != "b" || !forced[0].Canary { |
| t.Fatalf("admitted %+v, want b alone, as a canary", forced) |
| @@ -278,7 +278,7 @@ func TestARetriedInstanceWaitsForTheCohortsUnsettledCanary(t *testing.T) { |
| if len(first) != 1 || first[0].ID.Host != "a" || !first[0].Canary { |
| t.Fatalf("admitted %+v, want a as the canary", first) |
| } |
| - q.retryHost("b") |
| + q.retryHost("b", "v2") |
| if more := q.admit(currentView(q), t0); len(more) != 0 { |
| t.Fatalf("admitted %+v beside a's unsettled canary", more) |
| } |
| @@ -301,7 +301,7 @@ func TestARetriedInstanceIsACanaryInACohortThatIsNotHalted(t *testing.T) { |
| t.Fatalf("admitted %+v, want a as the canary", first) |
| } |
| q.finish(first[0], activator.SwapDone) |
| - q.retryHost("b") |
| + q.retryHost("b", "v2") |
| ds := q.admit(currentView(q), t0) |
| if len(ds) != 1 || ds[0].ID.Host != "b" || !ds[0].Canary { |
| t.Fatalf("admitted %+v, want b as a canary", ds) |
| @@ -320,7 +320,7 @@ func TestARetriedInstanceWaitsForARunningCanaryInAHaltedCohort(t *testing.T) { |
| releaseAll(q, "v2", "a", "b") |
| first := q.admit(currentView(q), t0) |
| q.halt("v2", "another host failed") |
| - q.retryHost("b") |
| + q.retryHost("b", "v2") |
| if more := q.admit(currentView(q), t0); len(more) != 0 { |
| t.Fatalf("admitted %+v beside a running canary", more) |
| } |
| @@ -331,13 +331,16 @@ func TestARetriedInstanceWaitsForARunningCanaryInAHaltedCohort(t *testing.T) { |
| } |
| // Final review F8: a forced attempt that ran nothing gets its allowance |
| -// back, bound to the build it was for. A host whose desire moved on to |
| -// another build, or an attempt that was never forced, gets nothing. |
| +// back, bound to the build it was for. Since final review round 1, finding |
| +// 5, that holds when the host's desire has moved on to another build too: |
| +// the allowance waits for the adoption onto its build, and never lets an |
| +// adoption onto the other build through. An attempt that was never forced |
| +// gets nothing. |
| func TestARearmedRetryStaysBoundToItsBuild(t *testing.T) { |
| q := awakeQueue(noBlocks, "a", "b") |
| releaseAll(q, "v2", "a", "b") |
| q.halt("v2", "health check failed") |
| - q.retryHost("b") |
| + q.retryHost("b", "v2") |
| forced := q.admit(currentView(q), t0) |
| if len(forced) != 1 || forced[0].ID.Host != "b" || !forced[0].Forced { |
| t.Fatalf("admitted %+v, want b's forced canary", forced) |
| @@ -351,8 +354,12 @@ func TestARearmedRetryStaysBoundToItsBuild(t *testing.T) { |
| q.finish(again[0], activator.SwapAbandoned) |
| q.wantBuild("b", activator.CauseRelease, "v3", t0) // a newer build replaced the desire |
| q.rearmRetry(again[0]) |
| - if got := q.hosts["b"].forceBuild; got != "" { |
| - t.Fatalf("forceBuild = %q: an allowance for v2 came back to a host that now wants v3", got) |
| + if got := q.hosts["b"].forceBuild; got != "v2" { |
| + t.Fatalf("forceBuild = %q: want the allowance for v2 back, waiting for v2", got) |
| + } |
| + q.halt("v3", "health check failed") |
| + if ds := q.admit(currentView(q), t0); len(ds) != 0 { |
| + t.Fatalf("admitted %+v: an allowance for v2 let an adoption onto v3 through v3's halt", ds) |
| } |
| q.rearmRetry(dispatch{ID: Identity{Host: "a", Build: "v2"}}) // never forced |
| if got := q.hosts["a"].forceBuild; got != "" { |
| @@ -360,12 +367,13 @@ func TestARearmedRetryStaysBoundToItsBuild(t *testing.T) { |
| } |
| } |
| -// A retry on a host with nothing pending must not leave a flag that lets an |
| -// unrelated later attempt skip the settle, or a halt. |
| +// A retry of another build must not leave a flag that lets an unrelated |
| +// later attempt skip the settle, or a halt (the entitlement is its build's, |
| +// final review round 1, finding 5). |
| func TestAStaleRetryDoesNotBypassTheSettleOrAHalt(t *testing.T) { |
| q := awakeQueue(noBlocks, "a", "b", "c") |
| q.concurrency = 3 |
| - q.retryHost("c") // c has no desired build: nothing to retry |
| + q.retryHost("c", "v9") // a retry of a build c does not want: nothing for v2 |
| releaseAll(q, "v2", "a", "b", "c") |
| first := q.admit(currentView(q), t0) |
| if len(first) != 1 || first[0].ID.Host != "a" { |
diff --git a/internal/rollout/finalreview1_test.go b/internal/rollout/finalreview1_test.go| index ddb44941f..2e7b93ed4 100644 |
| --- a/internal/rollout/finalreview1_test.go |
| +++ b/internal/rollout/finalreview1_test.go |
| @@ -2,6 +2,7 @@ package rollout |
| import ( |
| "context" |
| + "slices" |
| "strings" |
| "testing" |
| "time" |
| @@ -285,3 +286,88 @@ func TestANoOpRetryNotesNoClear(t *testing.T) { |
| t.Fatalf("a failure after a no-op retry = %v, want recorded", res) |
| } |
| } |
| + |
| +// Final review round 1, finding 5 (Astra): a host retry for a build B the |
| +// slot does not want now (it wants A) still entitles the host to a canary |
| +// onto B. The entitlement survives an attempt onto another build in |
| +// between, and is spent only by the adoption onto B: when the slot comes |
| +// to want B while B's cohort is halted, that adoption is admitted as the |
| +// host's canary. |
| +func TestARetryForABuildNotWantedYetKeepsItsCanary(t *testing.T) { |
| + k := appKey{"bab", "blog"} |
| + h := newHarness(t, "h", "s") |
| + h.stage("v2", "v3") |
| + h.act.mu.Lock() |
| + h.act.outcome = func(a activator.Attempt) activator.SwapOutcome { |
| + if a.Host == "s" && a.Build == "v3" { |
| + return activator.SwapFailed |
| + } |
| + return activator.SwapDone |
| + } |
| + h.act.mu.Unlock() |
| + h.pass() |
| + |
| + // s fails v3: a record holds v3 for h too, and v3's cohort halts. |
| + h.install("v3", "s") |
| + h.pass() |
| + waitFor(t, "s's attempt onto v3 to fail", func() bool { calls, _, _ := h.act.snapshot(); return len(calls) == 1 }) |
| + s := h.m.scheduler(k) |
| + halted := func() bool { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + return s.q.cohortFor(cohortKey("v3", classAdopt)).halted |
| + } |
| + waitFor(t, "v3's cohort to halt", halted) |
| + |
| + // The console's retry of h names v3; h's slot wants v1, which it serves. |
| + if !h.retry("h", "v3") { |
| + t.Fatal("the retry for v3 cleared nothing") |
| + } |
| + // An adoption onto another build (A = v2) runs in between: it does not |
| + // spend the entitlement for v3. |
| + h.install("v2", "h") |
| + h.pass() |
| + waitFor(t, "h's attempt onto v2", func() bool { return h.servingAll("v2", "h") && !h.m.attemptRunning("h") }) |
| + |
| + // Now h's slot wants v3, its cohort still halted: the adoption onto v3 is |
| + // admitted, as h's canary. |
| + if !halted() { |
| + t.Fatal("v3's cohort is no longer halted: the case needs the halt") |
| + } |
| + gate := h.act.hold("h") |
| + defer close(gate) |
| + h.install("v3", "h") |
| + h.pass() |
| + waitFor(t, "h's adoption onto v3 through the halt", func() bool { |
| + _, started, _ := h.act.snapshot() |
| + return slices.Contains(started, "h:v3") |
| + }) |
| + s.mu.Lock() |
| + d := s.q.hosts["h"].running |
| + s.mu.Unlock() |
| + if d == nil || d.ID.Build != "v3" || !d.Canary || !d.Forced { |
| + t.Fatalf("h's running attempt %+v: want the forced canary onto v3", d) |
| + } |
| +} |
| + |
| +// The entitlement belongs to its build: it never lets an adoption onto |
| +// another build through that build's halt. |
| +func TestARetryEntitlementIsNeverSpentOnAnotherBuild(t *testing.T) { |
| + q := newAppQueue("bab", "blog", func(string, string, bool) bool { return false }) |
| + q.concurrency = 1 |
| + now := t0 |
| + q.observe("h", "v1", "v1", now) |
| + q.wantBuild("h", activator.CauseRelease, "v1", now) |
| + q.retryHost("h", "v3") |
| + q.cohortFor(cohortKey("v2", classAdopt)).halted = true |
| + q.wantBuild("h", activator.CauseRelease, "v2", now) |
| + if _, ok := q.gate(q.hosts["h"], now); ok { |
| + t.Fatal("an entitlement for v3 let an adoption onto v2 through v2's halt") |
| + } |
| + q.cohortFor(cohortKey("v3", classAdopt)).halted = true |
| + q.wantBuild("h", activator.CauseRelease, "v3", now) |
| + d, ok := q.gate(q.hosts["h"], now) |
| + if !ok || !d.Forced || !d.Canary { |
| + t.Fatalf("gate onto v3 = %+v, %v: want the forced canary", d, ok) |
| + } |
| +} |
diff --git a/internal/rollout/manager.go b/internal/rollout/manager.go| index f53062248..d88c51e17 100644 |
| --- a/internal/rollout/manager.go |
| +++ b/internal/rollout/manager.go |
| @@ -823,7 +823,10 @@ func (m *Manager) retryBuild(k appKey, build, host string) { |
| // added under the scheduler's lock: the book saves as it did before. |
| // |
| // A grant naming another build than the slot's (the marker's build B while |
| -// the slot wants A) clears B's restriction and leaves A's queue alone. |
| +// the slot wants A) clears B's restriction and leaves A's attempt alone; a |
| +// cleared record of B still binds the host's canary entitlement for B, |
| +// which the adoption onto B spends when the slot comes to want it, through |
| +// B's halt if one stands (final review round 1, finding 5). |
| func (m *Manager) RetryHost(host, build, book string, epoch uint64) bool { |
| m.mu.Lock() |
| k, ok := m.hostApp[host] |
| @@ -838,10 +841,15 @@ func (m *Manager) RetryHost(host, build, book string, epoch uint64) bool { |
| want = h.wantBuild |
| } |
| cleared, unmarked, cur, err := m.d.Book.RetryHostAt(k.acct, k.app, build, host, book, epoch) |
| + if cleared { |
| + // The canary entitlement is build's whatever the slot wants now |
| + // (final review round 1, finding 5): bound here, in the step that |
| + // cleared the book, and spent only by the adoption onto build. |
| + s.q.retryHost(host, build) |
| + } |
| requeue := cleared && want == build |
| if requeue { |
| s.q.wantBuild(host, activator.CauseRetry, build, m.d.Now()) |
| - s.q.retryHost(host) |
| } |
| s.mu.Unlock() |
| if err != nil { |
diff --git a/internal/rollout/queue.go b/internal/rollout/queue.go| index 63a6caae8..e8bf76bba 100644 |
| --- a/internal/rollout/queue.go |
| +++ b/internal/rollout/queue.go |
| @@ -82,9 +82,14 @@ type hostSlot struct { |
| configAfter time.Time // a failed config attempt's backoff (spec §8.1) |
| notBefore time.Time // a staging refusal's wait (spec §4.2) |
| - running *dispatch |
| - buildRev uint64 // newest(host): moved only by a change of desired build |
| - forceBuild string // `carlos instances retry`: lets the adoption onto this build through a halt |
| + running *dispatch |
| + buildRev uint64 // newest(host): moved only by a change of desired build |
| + // forceBuild is `carlos instances retry`'s canary entitlement: it lets |
| + // the adoption onto this build through a halt, as a canary. It is the |
| + // retried build's, whatever the slot wants when the retry lands, and is |
| + // spent only by that adoption, never by an attempt onto another build |
| + // or a restart or config attempt (final review round 1, finding 5). |
| + forceBuild string |
| } |
| // cohort is one app's attempts on this box onto one build, of one class |
| @@ -372,7 +377,12 @@ func (q *appQueue) start(h *hostSlot, d dispatch) { |
| c := q.cohortFor(d.Cohort) |
| c.canary, c.settled = h.host, false |
| } |
| - h.forceBuild = "" |
| + if d.Forced { |
| + // Only the adoption the entitlement let through spends it (final |
| + // review round 1, finding 5): a restart, a config attempt or an |
| + // adoption onto another build starting first leaves it for later. |
| + h.forceBuild = "" |
| + } |
| h.running = &d |
| q.running++ |
| } |
| @@ -450,13 +460,16 @@ func (q *appQueue) unhalt(build string) { |
| } |
| } |
| -// retryHost is `carlos instances retry <host>`: if the host is waiting on a |
| -// build, that adoption runs as a canary even through a halt. The flag is |
| -// bound to the build the host wants now, so a retry with nothing pending, or |
| -// a later change of desire, leaves nothing behind that gating could honour. |
| -func (q *appQueue) retryHost(host string) { |
| - if h := q.hosts[host]; h != nil { |
| - h.forceBuild = h.wantBuild |
| +// retryHost is `carlos instances retry <host>` naming build: the host's |
| +// adoption onto build runs as a canary even through a halt, now if the slot |
| +// wants build, or whenever it comes to want it (final review round 1, |
| +// finding 5, Astra: a retry of a build the slot did not want yet used to |
| +// leave the host exempt in the book but with no admission through the |
| +// halt, and no later retry could give it one). The entitlement is bound to |
| +// build: it never lets an adoption onto another build through. |
| +func (q *appQueue) retryHost(host, build string) { |
| + if h := q.hosts[host]; h != nil && build != "" { |
| + h.forceBuild = build |
| } |
| } |
| @@ -464,14 +477,15 @@ func (q *appQueue) retryHost(host string) { |
| // start (final review F8), for an attempt that ended with nothing of its |
| // build run: abandoned before its exec, or refused as a staging error. Spec |
| // §8.4 promises the retried host's next adoption runs as a canary, and that |
| -// adoption has not happened yet. The allowance stays bound to the build: |
| -// a host that now wants another build, or holds a newer allowance, gets |
| -// nothing back. An attempt whose build ran spent the allowance for good, |
| -// whatever its outcome. The caller holds the scheduler's lock, in the step |
| +// adoption has not happened yet. The allowance stays bound to the build, |
| +// and comes back even when the host now wants another build, as a retry |
| +// for a build not yet wanted keeps it (finding 5); a host that holds a |
| +// newer allowance gets nothing back. An attempt whose build ran spent the |
| +// allowance for good, whatever its outcome. The caller holds the scheduler's lock, in the step |
| // that frees the attempt's slot. |
| func (q *appQueue) rearmRetry(d dispatch) { |
| h := q.hosts[d.ID.Host] |
| - if !d.Forced || h == nil || h.forceBuild != "" || h.wantBuild != d.ID.Build { |
| + if !d.Forced || h == nil || h.forceBuild != "" { |
| return |
| } |
| h.forceBuild = d.ID.Build |