amadan

carlos / platform Public

zdd-readiness: final review round 1, a marker-only retry notes its place in the sequence

Finding 4 (Astra, Medium): RetryHostAt removed a matching hold-ran marker but noted no host clear when no failure record held the host, so a scheduled attempt or planned wake of the build that began before the retry could fail afterwards and write a record and halt the cohort. A retry that removes the marker alone now notes the clear (clearedLocked); a retry that removes nothing still notes nothing.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude c327f70364eef549ddc79b15391f4b67d564d7b8 parent c12c35c
2 files changed, +88 −0
  • internal/rollout/failures.go +10 −0
  • internal/rollout/finalreview1_test.go +78 −0
diff --git a/internal/rollout/failures.go b/internal/rollout/failures.go
index 15fc421ff..55bfebc12 100644
--- a/internal/rollout/failures.go
+++ b/internal/rollout/failures.go
@@ -947,6 +947,16 @@ func (b *FailureBook) RetryHostAt(acct, app, build, host, book string, epoch uin
if !rec && !mark {
return false, false, now, b.retryLocked()
}
+ if !rec {
+ // Only the marker was removed (no record held the host): still a
+ // retry, so its place in the sequence is noted as one (final review
+ // round 1, finding 4, Astra). A failure of an execution of build on
+ // host that began before it is then stale (Record, PreRetry): no
+ // record, no halt, though a late hold outcome still sets its marker.
+ // retryHostLocked has noted it when it cleared a record, and a retry
+ // that removed nothing notes nothing.
+ b.clearedLocked(acct, app, build, host)
+ }
return rec, mark, now, b.saveLocked()
}
diff --git a/internal/rollout/finalreview1_test.go b/internal/rollout/finalreview1_test.go
index 38319c089..ddb44941f 100644
--- a/internal/rollout/finalreview1_test.go
+++ b/internal/rollout/finalreview1_test.go
@@ -207,3 +207,81 @@ func TestAReopenAfterUnsavedEpochsNeverReissuesThem(t *testing.T) {
t.Fatal("the grant for the unsaved restriction cleared the new one")
}
}
+
+// Final review round 1, finding 4 (Astra): a retry that removes only a
+// hold-ran marker (no failure record holds the host) is still a retry. An
+// execution of the marked build that began before it, a scheduled attempt or
+// a planned wake, and reports failure after it, is stale: no record is
+// written and the cohort is not halted. A no-op retry changes nothing
+// (TestANoOpRetryNotesNoClear).
+func TestAMarkerOnlyRetryOrdersALateFailureBeforeIt(t *testing.T) {
+ for _, kind := range []string{"attempt", "wake"} {
+ t.Run(kind, func(t *testing.T) {
+ ctx := context.Background()
+ k := appKey{"bab", "blog"}
+ h := newHarness(t, "h")
+ h.swap = appconfig.SwapHold
+ h.stage("v2")
+ h.book.MarkHoldRan("h", HoldRan{Account: "bab", App: "blog", Build: "v2", Exec: "a-earlier", At: t0})
+ var plan activator.WakePlan
+ if kind == "attempt" {
+ gate := h.act.hold("h")
+ h.install("v2", "h")
+ h.pass()
+ waitFor(t, "h's attempt onto v2", func() bool {
+ _, started, _ := h.act.snapshot()
+ return len(started) == 1 && started[0] == "h:v2"
+ })
+ if !h.retry("h", "v2") {
+ close(gate)
+ t.Fatal("the marker-only retry cleared nothing")
+ }
+ h.act.mu.Lock()
+ h.act.outcome = func(activator.Attempt) activator.SwapOutcome { return activator.SwapFailed }
+ h.act.mu.Unlock()
+ next := h.act.hold("h") // a next attempt, if any, parks in Swap
+ defer close(next)
+ close(gate)
+ waitFor(t, "h's attempt to finish", func() bool { calls, _, _ := h.act.snapshot(); return len(calls) == 1 })
+ } else {
+ h.install("v2", "h")
+ h.sleepHost("h")
+ h.pass()
+ h.wakeHost("h")
+ plan = h.m.Plan(ctx, h.rows.get("h"))
+ if !plan.FirstWake || plan.Boot.Version != "v2" {
+ t.Fatalf("plan %+v: want a first wake of the marked v2", plan)
+ }
+ if !h.retry("h", "v2") {
+ t.Fatal("the marker-only retry cleared nothing")
+ }
+ h.m.OnGen(activator.GenEvent{Kind: activator.GenFailed, Route: h.rows.get("h"), Failed: plan.Boot, Mode: plan.Mode,
+ FirstWake: true, Reason: "health check failed", Token: plan.Token, ConfigRev: plan.ConfigRev})
+ }
+ if _, ok := h.book.Lookup("bab", "blog", "v2"); ok {
+ t.Fatal("a failure of an execution that began before the retry wrote a record")
+ }
+ s := h.m.scheduler(k)
+ s.mu.Lock()
+ halted := s.q.cohortFor(cohortKey("v2", classAdopt)).halted
+ s.mu.Unlock()
+ if halted {
+ t.Fatal("a failure of an execution that began before the retry halted the cohort")
+ }
+ })
+ }
+}
+
+// Finding 4's other half: a retry that clears nothing (no record, no marker
+// of the build) notes no clear, so a failure of an execution that began
+// before it is still recorded.
+func TestANoOpRetryNotesNoClear(t *testing.T) {
+ b := &FailureBook{}
+ snap := b.Snapshot()
+ if rec, mark, _, _ := b.RetryHostAt("bab", "blog", "v2", "h", b.ID(), 1); rec || mark {
+ t.Fatal("a retry with nothing to clear cleared something")
+ }
+ if res, _ := b.Record(FailureRecord{Account: "bab", App: "blog", Build: "v2", Host: "h", Seq: snap}); res != Recorded {
+ t.Fatalf("a failure after a no-op retry = %v, want recorded", res)
+ }
+}