amadan
zdd-5 final review: give back a retry allowance an attempt never used (F8, scheduler half)
A forced adoption (dispatch.Forced: the one a `carlos instances retry` allowance let through) spends forceBuild at its start. When it ends abandoned with SwapResult.Executed false (a refusal before its exec, a staging refusal, an unreadable row), done() re-arms the allowance for the same build in the step that frees the slot, so the promised canary runs after the host's deferral and RetryHost is not needed again. An attempt whose build ran spends it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 files changed,
+159
−3
internal/rollout/admit_test.go+30 −0internal/rollout/fake_test.go+10 −2internal/rollout/queue.go+23 −1internal/rollout/retry_rearm_test.go+89 −0internal/rollout/scheduler.go+7 −0
diff --git a/internal/rollout/admit_test.go b/internal/rollout/admit_test.go| index d7c6c3215..92e446cb0 100644 |
| --- a/internal/rollout/admit_test.go |
| +++ b/internal/rollout/admit_test.go |
| @@ -323,6 +323,36 @@ 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. |
| +func TestARearmedRetryStaysBoundToItsBuild(t *testing.T) { |
| + q := awakeQueue(noBlocks, "a", "b") |
| + releaseAll(q, "v2", "a", "b") |
| + q.halt("v2", "health check failed") |
| + q.retryHost("b") |
| + 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) |
| + } |
| + q.finish(forced[0], activator.SwapAbandoned) |
| + q.rearmRetry(forced[0]) |
| + again := q.admit(currentView(q), t0) |
| + if len(again) != 1 || again[0].ID.Host != "b" || !again[0].Canary || !again[0].Forced { |
| + t.Fatalf("after the re-arm admitted %+v, want b's forced canary again", again) |
| + } |
| + 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) |
| + } |
| + 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) |
| + } |
| +} |
| + |
| // 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. |
| func TestAStaleRetryDoesNotBypassTheSettleOrAHalt(t *testing.T) { |
diff --git a/internal/rollout/fake_test.go b/internal/rollout/fake_test.go| index e0f9cf5df..adc673fb1 100644 |
| --- a/internal/rollout/fake_test.go |
| +++ b/internal/rollout/fake_test.go |
| @@ -50,6 +50,10 @@ type fakeAct struct { |
| // onHeldPublish, when set, runs on every PublishHeldServing: a test's |
| // view of the moment the scheduler releases a held worker publication. |
| onHeldPublish func(host string) |
| + // executed, when set, is Swap's execution evidence (SwapResult.Executed) |
| + // for an attempt that ended out. Unset, only an abandoned attempt ran |
| + // nothing. |
| + executed func(a activator.Attempt, out activator.SwapOutcome) bool |
| } |
| // PublishHeldServing runs onHeldPublish, when set. |
| @@ -116,17 +120,21 @@ func (f *fakeAct) Swap(_ context.Context, a activator.Attempt, newest func(strin |
| defer f.mu.Unlock() |
| f.inFlight-- |
| f.calls = append(f.calls, swapCall{a: a, outcome: out}) |
| + executed := out != activator.SwapAbandoned |
| + if f.executed != nil { |
| + executed = f.executed(a, out) |
| + } |
| if out != activator.SwapDone { |
| reason := "health check failed" |
| if f.reason != "" { |
| reason = f.reason |
| } |
| - return activator.SwapResult{Outcome: out, Reason: reason, Mode: f.mode} |
| + return activator.SwapResult{Outcome: out, Reason: reason, Mode: f.mode, Executed: executed} |
| } |
| f.n++ |
| g := activator.Gen{N: f.n, Build: a.Build, StagedPath: a.StagedPath, Digest: a.Digest, Demand: a.Demand} |
| f.gens[a.Host] = g |
| - return activator.SwapResult{Outcome: out, Gen: g} |
| + return activator.SwapResult{Outcome: out, Gen: g, Executed: executed} |
| } |
| // ReplaceWorker models plan 5's same-entry socketless attempt (Task 11): |
diff --git a/internal/rollout/queue.go b/internal/rollout/queue.go| index 83a0f9f3e..c59763273 100644 |
| --- a/internal/rollout/queue.go |
| +++ b/internal/rollout/queue.go |
| @@ -42,6 +42,11 @@ type dispatch struct { |
| // runs, so a failure is published with no blocking read in between |
| // (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. |
| + // An attempt that ends with nothing of its build run gets it back |
| + // (rearmRetry; final review F8). |
| + Forced bool |
| } |
| // hostSlot is one instance's desired state. The attempt to run is derived |
| @@ -330,7 +335,7 @@ func (q *appQueue) gate(h *hostSlot, now time.Time) (dispatch, bool) { |
| if c.canary != "" && c.canary != h.host && !c.settled { |
| return dispatch{}, false |
| } |
| - d.Canary = true |
| + d.Canary, d.Forced = true, true |
| case c.halted && d.Class == classAdopt: |
| return dispatch{}, false |
| case c.canary == "": |
| @@ -438,6 +443,23 @@ func (q *appQueue) retryHost(host string) { |
| } |
| } |
| +// rearmRetry gives back the retry allowance a forced attempt spent at its |
| +// 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 |
| +// 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 { |
| + return |
| + } |
| + h.forceBuild = d.ID.Build |
| +} |
| + |
| // nextDue is the earliest moment a deferred attempt becomes admissible, so the |
| // scheduler's goroutine knows when to look again; zero when nothing waits. |
| func (q *appQueue) nextDue(now time.Time) time.Time { |
diff --git a/internal/rollout/retry_rearm_test.go b/internal/rollout/retry_rearm_test.go| new file mode 100644 |
| index 000000000..182329a21 |
| --- /dev/null |
| +++ b/internal/rollout/retry_rearm_test.go |
| @@ -0,0 +1,89 @@ |
| +package rollout |
| + |
| +import ( |
| + "sync/atomic" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/activator" |
| +) |
| + |
| +// 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 |
| +// 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 |
| +// (which the book would refuse anyway: the host is already in Retried). An |
| +// attempt whose build ran spends it, abandoned or not. |
| +func TestARetryAllowanceComesBackOnlyWhenNothingRan(t *testing.T) { |
| + for _, tc := range []struct { |
| + name string |
| + staging bool // b's first retry attempt is refused as a staging error |
| + executed bool // b's first retry attempt is abandoned after its build ran |
| + deferral time.Duration |
| + }{ |
| + {"abandoned before its build ran", false, false, abandonDeferral}, |
| + {"refused as a staging error", true, false, stagingDeferral}, |
| + {"abandoned after its build ran", false, true, abandonDeferral}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + h := newHarness(t, "a", "b") |
| + h.stage("v2") |
| + var bTries atomic.Int32 |
| + h.act.outcome = func(a activator.Attempt) activator.SwapOutcome { |
| + switch { |
| + case a.Build != "v2": |
| + return activator.SwapDone |
| + case a.Host == "a": |
| + return activator.SwapFailed // the canary that halts the cohort |
| + case bTries.Add(1) == 1 && !tc.staging: |
| + return activator.SwapAbandoned |
| + } |
| + return activator.SwapDone |
| + } |
| + h.act.executed = func(_ activator.Attempt, out activator.SwapOutcome) bool { |
| + return out != activator.SwapAbandoned || tc.executed |
| + } |
| + h.install("v2", "a", "b") |
| + h.pass() |
| + waitFor(t, "the halt", func() bool { _, ok := h.book.Lookup("bab", "blog", "v2"); return ok }) |
| + if tc.staging { |
| + h.mu.Lock() |
| + h.mismatch["v2"] = true |
| + h.mu.Unlock() |
| + } |
| + if !h.m.RetryHost("b") { |
| + t.Fatal("RetryHost did not clear b") |
| + } |
| + if tc.staging { |
| + waitFor(t, "the staging refusal", func() bool { return h.touched.Load() > 0 }) |
| + h.mu.Lock() |
| + h.mismatch["v2"] = false |
| + h.mu.Unlock() |
| + } else { |
| + waitFor(t, "b's first retry attempt", func() bool { return bTries.Load() >= 1 }) |
| + } |
| + // Its slot is free and its deferral set (done) before the next pass. |
| + waitFor(t, "b's slot to free", func() bool { return !h.m.attemptRunning("b") }) |
| + before, _, _ := h.act.snapshot() |
| + h.advance(tc.deferral + time.Second) |
| + h.pass() |
| + if tc.executed { |
| + time.Sleep(100 * time.Millisecond) |
| + if calls, _, _ := h.act.snapshot(); len(calls) != len(before) { |
| + t.Fatalf("calls %+v: an attempt whose build ran spent the retry, yet b ran again through the halt", calls[len(before):]) |
| + } |
| + return |
| + } |
| + waitFor(t, "b's retry canary", func() bool { return h.servingAll("v2", "b") }) |
| + calls, _, _ := h.act.snapshot() |
| + if c := calls[len(calls)-1].a; c.Host != "b" || !c.Canary || c.Cause != activator.CauseRetry { |
| + t.Fatalf("attempt after the re-armed retry = %+v, want b's retry canary", c) |
| + } |
| + if h.servingAll("v2", "a") { |
| + t.Fatal("the re-armed retry lifted the halt for another host") |
| + } |
| + }) |
| + } |
| +} |
diff --git a/internal/rollout/scheduler.go b/internal/rollout/scheduler.go| index 8914a46f0..0eeda9aec 100644 |
| --- a/internal/rollout/scheduler.go |
| +++ b/internal/rollout/scheduler.go |
| @@ -205,6 +205,13 @@ func (s *scheduler) done(d dispatch, res activator.SwapResult, mode activator.Sw |
| // record (in memory at once, on disk as it can) and the halt. |
| added, superseded, recErr = failCohort(m.d.Book, s.q, *rec) |
| } |
| + if res.Outcome == activator.SwapAbandoned && !res.Executed { |
| + // Nothing of the build ran (a refusal before its exec, a staging |
| + // refusal, an unreadable row): a retried host's allowance is not |
| + // spent, and its promised canary still runs after the deferral |
| + // (final review F8). |
| + s.q.rearmRetry(d) |
| + } |
| h := s.q.hosts[d.ID.Host] |
| if cfgCounted && h != nil && h.wantConfig == d.ID.ConfigRev { |
| s.q.configFailed(d.ID.Host, now.Add(cfgWait)) |