amadan
zdd-readiness: final review round 2, retry entitlements per host and build
Astra R2 finding 5 (PARTIAL): the queue kept one forceBuild per host, so a retry of a second restricted build overwrote the first build's canary entitlement. The book already listed the host as retried for the first build, so its halted cohort refused the adoption and no second retry could restore it. hostSlot.forceBuilds is now a set of builds. Each entitlement is spent only by the adoption onto its own build, re-armed on proven-unexecuted abandonment (rearmRetry) without touching the others, and dropped by the pass (liftClearedHalts -> appQueue.dropRetries) once FailureBook.Retried says the book no longer lists the host as retried for that build (record cleared by a pointer, pruned, or the retry spent by a later failure), so the set cannot grow without bound. Tests: TestARetryForASecondBuildKeepsTheFirstBuildsCanary (RED before: timed out waiting for h's adoption onto v3 through the halt), TestARetryEntitlementIsDroppedWhenTheBookNoLongerListsTheHost (pointer clear and prune).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6 files changed,
+250
−24
internal/rollout/admit_test.go+4 −4internal/rollout/failures.go+11 −0internal/rollout/finalreview2_test.go+170 −0internal/rollout/manager.go+7 −1internal/rollout/queue.go+57 −18internal/rollout/retry_rearm_test.go+1 −1
diff --git a/internal/rollout/admit_test.go b/internal/rollout/admit_test.go| index b4754b22f..71b3600ed 100644 |
| --- a/internal/rollout/admit_test.go |
| +++ b/internal/rollout/admit_test.go |
| @@ -354,16 +354,16 @@ 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 != "v2" { |
| - t.Fatalf("forceBuild = %q: want the allowance for v2 back, waiting for v2", got) |
| + if got := q.hosts["b"].forceBuilds; len(got) != 1 || !got["v2"] { |
| + t.Fatalf("forceBuilds = %v: 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 != "" { |
| - t.Fatalf("forceBuild = %q: an attempt that was never forced armed a retry", got) |
| + if got := q.hosts["a"].forceBuilds; len(got) != 0 { |
| + t.Fatalf("forceBuilds = %v: an attempt that was never forced armed a retry", got) |
| } |
| } |
diff --git a/internal/rollout/failures.go b/internal/rollout/failures.go| index 55bfebc12..413fb3ce3 100644 |
| --- a/internal/rollout/failures.go |
| +++ b/internal/rollout/failures.go |
| @@ -843,6 +843,17 @@ func (b *FailureBook) Blocks(acct, app, build, host string, served bool) bool { |
| return i >= 0 && !slices.Contains(b.recs[i].Retried, host) |
| } |
| +// Retried reports whether build's record stands and lists host as retried: |
| +// a `carlos instances retry` of host cleared it and no failure of host has |
| +// spent it since. The scheduler keeps a retry's canary entitlement only |
| +// while this holds (appQueue.dropRetries, final review round 2, finding 5). |
| +func (b *FailureBook) Retried(acct, app, build, host string) bool { |
| + b.mu.Lock() |
| + defer b.mu.Unlock() |
| + i := b.find(acct, app, build) |
| + return i >= 0 && slices.Contains(b.recs[i].Retried, host) |
| +} |
| + |
| // HeldDown reports whether host's wakes are refused after a failed hold |
| // attempt onto build (spec §6.4). |
| func (b *FailureBook) HeldDown(acct, app, build, host string) bool { |
diff --git a/internal/rollout/finalreview2_test.go b/internal/rollout/finalreview2_test.go| new file mode 100644 |
| index 000000000..12d267d95 |
| --- /dev/null |
| +++ b/internal/rollout/finalreview2_test.go |
| @@ -0,0 +1,170 @@ |
| +package rollout |
| + |
| +import ( |
| + "maps" |
| + "slices" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/activator" |
| +) |
| + |
| +// retryHarness is a harness whose s fails v3 and whose t fails v4: each |
| +// failure writes a record that holds h too, and halts that build's cohort. |
| +// h serves and wants v1 throughout. |
| +func retryHarness(t *testing.T) (*harness, func(build string) bool) { |
| + t.Helper() |
| + k := appKey{"bab", "blog"} |
| + h := newHarness(t, "h", "s", "t") |
| + h.stage("v2", "v3", "v4") |
| + h.act.mu.Lock() |
| + h.act.outcome = func(a activator.Attempt) activator.SwapOutcome { |
| + if (a.Host == "s" && a.Build == "v3") || (a.Host == "t" && a.Build == "v4") { |
| + return activator.SwapFailed |
| + } |
| + return activator.SwapDone |
| + } |
| + h.act.mu.Unlock() |
| + h.pass() |
| + s := h.m.scheduler(k) |
| + halted := func(build string) bool { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + return s.q.cohortFor(cohortKey(build, classAdopt)).halted |
| + } |
| + h.install("v3", "s") |
| + h.pass() |
| + waitFor(t, "v3's cohort to halt", func() bool { return halted("v3") }) |
| + h.install("v4", "t") |
| + h.pass() |
| + waitFor(t, "v4's cohort to halt", func() bool { return halted("v4") }) |
| + return h, halted |
| +} |
| + |
| +// running is h's running attempt, nil when none. |
| +func (h *harness) running(host string) *dispatch { |
| + s := h.m.scheduler(appKey{"bab", "blog"}) |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + if d := s.q.hosts[host].running; d != nil { |
| + c := *d |
| + return &c |
| + } |
| + return nil |
| +} |
| + |
| +// Final review round 2, finding 5 (Astra): retry entitlements are kept per |
| +// host and build. B's (v3's) cohort is halted; h is retried for v3 while it |
| +// wants v1, then for another restricted build C (v4) before it adopts v3. |
| +// The second retry must not overwrite the first: when h comes to want v3, |
| +// the book already lists h as retried for v3 (so a second retry of v3 |
| +// clears nothing), and only the entitlement the first retry bound can take |
| +// h through v3's halt. |
| +func TestARetryForASecondBuildKeepsTheFirstBuildsCanary(t *testing.T) { |
| + h, halted := retryHarness(t) |
| + if !h.retry("h", "v3") { |
| + t.Fatal("the retry for v3 cleared nothing") |
| + } |
| + if !h.retry("h", "v4") { |
| + t.Fatal("the retry for v4 cleared nothing") |
| + } |
| + if h.retry("h", "v3") { |
| + t.Fatal("a second retry of v3 cleared something: the case needs the book to refuse it") |
| + } |
| + if !halted("v3") || !halted("v4") { |
| + t.Fatal("a cohort is no longer halted: the case needs both halts") |
| + } |
| + |
| + gate := h.act.hold("h") |
| + 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") |
| + }) |
| + if d := h.running("h"); d == nil || d.ID.Build != "v3" || !d.Canary || !d.Forced { |
| + t.Fatalf("h's running attempt %+v: want the forced canary onto v3", d) |
| + } |
| + close(gate) |
| + waitFor(t, "h's adoption onto v3", func() bool { return h.servingAll("v3", "h") && !h.m.attemptRunning("h") }) |
| + |
| + // v4's entitlement was neither overwritten nor spent by v3's adoption. |
| + gate = h.act.hold("h") |
| + defer close(gate) |
| + h.install("v4", "h") |
| + h.pass() |
| + waitFor(t, "h's adoption onto v4 through the halt", func() bool { |
| + _, started, _ := h.act.snapshot() |
| + return slices.Contains(started, "h:v4") |
| + }) |
| + if d := h.running("h"); d == nil || d.ID.Build != "v4" || !d.Canary || !d.Forced { |
| + t.Fatalf("h's running attempt %+v: want the forced canary onto v4", d) |
| + } |
| +} |
| + |
| +// An entitlement lasts only while the book lists the host as retried for its |
| +// build. Once the record is gone (cleared by a signed pointer, or pruned when |
| +// no route installs the build), the next pass drops it: the set cannot grow |
| +// without bound, and a record the build may get later starts with no |
| +// entitlement from a retry of the old one. |
| +func TestARetryEntitlementIsDroppedWhenTheBookNoLongerListsTheHost(t *testing.T) { |
| + for _, tc := range []struct { |
| + name string |
| + clear func(h *harness) |
| + }{ |
| + {"cleared by a pointer", func(h *harness) { |
| + // s still wants v3 and may adopt it before h: it succeeds now, |
| + // so no fresh record of v3 races h's adoption. |
| + h.act.mu.Lock() |
| + h.act.outcome = func(a activator.Attempt) activator.SwapOutcome { |
| + if a.Host == "t" && a.Build == "v4" { |
| + return activator.SwapFailed |
| + } |
| + return activator.SwapDone |
| + } |
| + h.act.mu.Unlock() |
| + if ok, _ := h.book.ClearOnPointer("bab", "blog", "v3", t0.Add(time.Hour)); !ok { |
| + h.t.Fatal("the pointer cleared nothing") |
| + } |
| + }}, |
| + {"pruned", func(h *harness) { |
| + h.install("v1", "s") // no route installs v3 any more |
| + }}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + h, halted := retryHarness(t) |
| + if !h.retry("h", "v3") { |
| + t.Fatal("the retry for v3 cleared nothing") |
| + } |
| + if !h.retry("h", "v4") { |
| + t.Fatal("the retry for v4 cleared nothing") |
| + } |
| + tc.clear(h) |
| + h.pass() |
| + if halted("v3") { |
| + t.Fatal("v3's halt outlived its record") |
| + } |
| + s := h.m.scheduler(appKey{"bab", "blog"}) |
| + s.mu.Lock() |
| + got := slices.Sorted(maps.Keys(s.q.hosts["h"].forceBuilds)) |
| + s.mu.Unlock() |
| + if !slices.Equal(got, []string{"v4"}) { |
| + t.Fatalf("h's entitlements %v, want only v4's: v3's record is gone", got) |
| + } |
| + |
| + gate := h.act.hold("h") |
| + defer close(gate) |
| + h.install("v3", "h") |
| + h.pass() |
| + waitFor(t, "h's adoption onto v3", func() bool { |
| + _, started, _ := h.act.snapshot() |
| + return slices.Contains(started, "h:v3") |
| + }) |
| + // Whether it is the cohort's canary depends on whether s went |
| + // first; either way it is not a retried host's forced canary. |
| + if d := h.running("h"); d == nil || d.ID.Build != "v3" || d.Forced { |
| + t.Fatalf("h's running attempt %+v: want an ordinary adoption onto v3, not a retried one", d) |
| + } |
| + }) |
| + } |
| +} |
diff --git a/internal/rollout/manager.go b/internal/rollout/manager.go| index a6be01e16..e6cbc0e87 100644 |
| --- a/internal/rollout/manager.go |
| +++ b/internal/rollout/manager.go |
| @@ -1121,7 +1121,10 @@ func (m *Manager) prune(routes []registry.Route) { |
| // hand lifts the hold). A halt lives in memory beside its record, so a record |
| // pruned after an install by hand would otherwise leave the build refused for |
| // good if it were installed again. failCohort writes the record and the halt |
| -// under the same scheduler lock this reads them under. |
| +// under the same scheduler lock this reads them under. In the same hold it |
| +// drops every retry entitlement the book no longer lists (final review round |
| +// 2, finding 5): RetryHost binds one in the step that lists the host, so an |
| +// entitlement and its listing go together. |
| func (m *Manager) liftClearedHalts() { |
| m.mu.Lock() |
| scheds := make([]*scheduler, 0, len(m.apps)) |
| @@ -1142,6 +1145,9 @@ func (m *Manager) liftClearedHalts() { |
| lifted = append(lifted, build) |
| } |
| } |
| + s.q.dropRetries(func(build, host string) bool { |
| + return m.d.Book.Retried(s.key.acct, s.key.app, build, host) |
| + }) |
| s.mu.Unlock() |
| if len(lifted) > 0 { |
| m.d.Logf("rollout: %s/%s: halt lifted for %v, their failure records are gone", s.key.acct, s.key.app, lifted) |
diff --git a/internal/rollout/queue.go b/internal/rollout/queue.go| index e8bf76bba..30c24e9bc 100644 |
| --- a/internal/rollout/queue.go |
| +++ b/internal/rollout/queue.go |
| @@ -43,7 +43,7 @@ type dispatch struct { |
| // (spec §8.1, §8.4). Zero: the failure's own time stands in. |
| FailedUnder time.Time |
| // Forced: the attempt is the adoption a `carlos instances retry` |
| - // allowance (hostSlot.forceBuild) let through, spent when it started. |
| + // allowance (hostSlot.forceBuilds) let through, spent when it started. |
| // An attempt that ends with nothing of its build run gets it back |
| // (rearmRetry; final review F8). |
| Forced bool |
| @@ -84,12 +84,18 @@ type hostSlot struct { |
| 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 |
| + // forceBuilds are `carlos instances retry`'s canary entitlements, one per |
| + // retried build: each lets the adoption onto its build through a halt, |
| + // as a canary. Each is its 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). They are kept per build because a retry of one build |
| + // must not take away another's (final review round 2, finding 5, |
| + // Astra): the book lists the host as retried for the first build, so a |
| + // second retry of it clears nothing and could not give the entitlement |
| + // back. dropRetries removes the ones the book no longer lists, so the |
| + // set holds at most one entry per record that lists the host. |
| + forceBuilds map[string]bool |
| } |
| // cohort is one app's attempts on this box onto one build, of one class |
| @@ -347,7 +353,7 @@ func (q *appQueue) gate(h *hostSlot, now time.Time) (dispatch, bool) { |
| // The flag is tied to the build it was set for, so a retry on a host with |
| // nothing pending cannot lift a later cohort's halt or turn an unrelated |
| // attempt into a canary. |
| - forced := d.Class == classAdopt && h.forceBuild != "" && h.forceBuild == d.ID.Build |
| + forced := d.Class == classAdopt && h.forceBuilds[d.ID.Build] |
| switch { |
| case forced: |
| // The next attempt after a retry is a canary (spec §8.4) whether or |
| @@ -380,8 +386,9 @@ func (q *appQueue) start(h *hostSlot, d dispatch) { |
| 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 = "" |
| + // adoption onto another build starting first leaves it for later, |
| + // and the other builds' entitlements stay (round 2, finding 5). |
| + delete(h.forceBuilds, d.ID.Build) |
| } |
| h.running = &d |
| q.running++ |
| @@ -466,10 +473,40 @@ func (q *appQueue) unhalt(build string) { |
| // 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. |
| +// build: it never lets an adoption onto another build through, and a |
| +// retry of another build leaves it standing (final review round 2, finding |
| +// 5). |
| func (q *appQueue) retryHost(host, build string) { |
| if h := q.hosts[host]; h != nil && build != "" { |
| - h.forceBuild = build |
| + h.arm(build) |
| + } |
| +} |
| + |
| +// arm binds a retry entitlement for build to h. |
| +func (h *hostSlot) arm(build string) { |
| + if h.forceBuilds == nil { |
| + h.forceBuilds = map[string]bool{} |
| + } |
| + h.forceBuilds[build] = true |
| +} |
| + |
| +// dropRetries drops every entitlement whose build the failure book no |
| +// longer lists the host as retried for (retried false): the record was |
| +// cleared by a signed pointer or pruned, or a later failure of the host |
| +// spent the retry. Such an entitlement could only make a later record of |
| +// the build, one no retry has touched, admit the host through its halt |
| +// (final review round 2, finding 5). The caller holds the scheduler's |
| +// lock; retried may take the book's, which comes after it. |
| +func (q *appQueue) dropRetries(retried func(build, host string) bool) { |
| + for _, h := range q.hosts { |
| + for build := range h.forceBuilds { |
| + if !retried(build, h.host) { |
| + delete(h.forceBuilds, build) |
| + } |
| + } |
| + if len(h.forceBuilds) == 0 { |
| + h.forceBuilds = nil |
| + } |
| } |
| } |
| @@ -479,16 +516,18 @@ func (q *appQueue) retryHost(host, build string) { |
| // §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, |
| // 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. |
| +// for a build not yet wanted keeps it (finding 5). Other builds' |
| +// allowances are untouched (round 2, finding 5). If the book has stopped |
| +// listing the host as retried for the build meanwhile, the next pass drops |
| +// it again (dropRetries). 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 != "" { |
| + if !d.Forced || h == nil { |
| return |
| } |
| - h.forceBuild = d.ID.Build |
| + h.arm(d.ID.Build) |
| } |
| // nextDue is the earliest moment a deferred attempt becomes admissible, so the |
diff --git a/internal/rollout/retry_rearm_test.go b/internal/rollout/retry_rearm_test.go| index e0c53d29e..1314db877 100644 |
| --- a/internal/rollout/retry_rearm_test.go |
| +++ b/internal/rollout/retry_rearm_test.go |
| @@ -11,7 +11,7 @@ import ( |
| // Final review F8: `carlos instances retry <host>` promises that host's next |
| // adoption runs as a canary through its cohort's halt. Admission spends the |
| -// allowance (forceBuild) when the attempt starts. An attempt that then ends |
| +// allowance (forceBuilds) when the attempt starts. An attempt that then ends |
| // before its build ran (abandoned with SwapResult.Executed false, or refused |
| // as a staging error) spent nothing of it: the allowance comes back, so the |
| // canary still runs once the host's deferral ends, with no second retry |