amadan

carlos / platform Public

zdd-readiness: final review round 1, a declined box's restrictions hold on a row a move holds

Finding 2 (Astra, High): the claims move launch called the declined gate, but holdOf returned unrestricted whenever rollout.Managed(rt) was false, and a real moves probe or landing runs on a held row (landing, restoring). Restriction eligibility now reads a passing status as ready (rollout.Restrictable, which heldOnBox now shares) in holdOf, heldBuild and the declined box's retryHost, so a held row is refused at the claims move launch as its ready row is, and a signed grant that arrives while the row is moving is applied rather than read once and dropped. Move authorization stays the moves package's check.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 076bd9c0dbce4e5e8cc6d1d9f837fa8d29827953 parent e72e68d
4 files changed, +118 −5
  • internal/host/finalreview1_declined_test.go +94 −0
  • internal/host/rolloutdeclined.go +11 −3
  • internal/rollout/manager.go +1 −2
  • internal/rollout/policy.go +12 −0
diff --git a/internal/host/finalreview1_declined_test.go b/internal/host/finalreview1_declined_test.go
new file mode 100644
index 000000000..1b3e3a7f3
--- /dev/null
+++ b/internal/host/finalreview1_declined_test.go
@@ -0,0 +1,94 @@
+package host
+
+import (
+ "bytes"
+ "encoding/base64"
+ "os"
+ "testing"
+
+ "github.com/carlosframework/platform/internal/registry"
+ "github.com/carlosframework/platform/internal/release"
+ "github.com/carlosframework/platform/internal/rollout"
+)
+
+// Final review round 1, finding 2 (Astra): a real claims move launch (a
+// moves probe or a landing) runs on a row the move holds ("landing",
+// "restoring", movesprobeowners heldRoute), and Managed reads every held
+// status as not managed. The declined box's restriction must not depend on
+// that passing status: a held row the book restricts is refused at the
+// claims move launch exactly as its ready row is, for a hold-down and for a
+// hold-ran marker. Move authorization is the moves package's own check, and
+// stays separate.
+func TestDeclinedBoxGatesAClaimsMoveLaunchOfAHeldRow(t *testing.T) {
+ for _, status := range []string{"landing", "restoring"} {
+ for _, kind := range []string{"hold-down", "marker"} {
+ t.Run(status+", "+kind, func(t *testing.T) {
+ f := newAdminTokenFixture(t, "box-1")
+ if err := os.WriteFile(f.envPath(), []byte("CARLOS_ADMIN_TOKEN=tok-0123456789abcdef0123456789abcdef\n"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ t.Setenv("CARLOS_SECRETS_KEY", base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{7}, 32)))
+ rt := f.rt
+ rt.Kind, rt.Status, rt.HoldOp, rt.Channel, rt.Account, rt.App, rt.Hibernate, rt.Version = "instance", status, "op-1", "edge", "bab", "jam", true, "v1"
+ acct := release.RouteAccount(rt)
+ svc, reg, dd := rolloutFixture(t, false)
+ var want string
+ switch kind {
+ case "hold-down":
+ // v1 failed in hold mode on this instance: it stays down.
+ rt.GoodVersion, rt.GoodDigest = "v0", "sha256:v0"
+ writeFailureBook(t, dd, []rollout.FailureRecord{{Account: acct, App: "jam", Build: "v1", Host: rt.Host, Mode: "hold", HeldDown: []string{rt.Host}}}, nil)
+ want = rollout.HeldDownRefusal("v1")
+ case "marker":
+ // A hold build v2 ran here: the good build v1, which the
+ // launch would exec, is barred until a signed rollback.
+ rt.GoodVersion, rt.GoodDigest = "v1", "sha256:v1"
+ b, err := rollout.OpenFailureBook(dd)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := b.MarkHoldRan(rt.Host, rollout.HoldRan{Account: acct, App: "jam", Build: "v2"}); err != nil {
+ t.Fatal(err)
+ }
+ want = rollout.HeldDownRefusal("v2")
+ }
+ declineWithBook(t, svc, reg, dd)
+ if _, _, err := claimMoveLaunchSource(f.backend)(t.Context(), rt); err == nil || err.Error() != want {
+ t.Fatalf("move launch of a %s row = %v, want %q", status, err, want)
+ }
+ })
+ }
+ }
+}
+
+// Finding 2's second half (Task 10's deferred minor): a signed retry that
+// arrives while the instance's row is held by a move ("moving") is applied,
+// not consumed and dropped: the restart leg reads each request once.
+func TestADeclinedBoxAppliesARetryWhileTheRowIsMoving(t *testing.T) {
+ t.Setenv("CARLOS_OPT_DIR", t.TempDir())
+ svc, reg, dd := rolloutFixture(t, false)
+ const host = "h.example.com"
+ rt := heldRouteIn(t, reg, host, "v2", "v1")
+ acct := release.RouteAccount(rt)
+ writeFailureBook(t, dd, []rollout.FailureRecord{{
+ Account: acct, App: "blog", Build: "v2", Host: "sib.example.com", Mode: "hold", Reason: "health", HeldDown: []string{host},
+ }}, nil)
+ declineWithBook(t, svc, reg, dd)
+ hr, ok := declinedHostReport(t, host)
+ if !ok || hr.HeldBy != "v2" {
+ t.Fatalf("published %+v (%v), want v2 holding %s", hr, ok, host)
+ }
+ if err := reg.SetHold(host, "moving", "op-1"); err != nil {
+ t.Fatal(err)
+ }
+ moving, err := reg.Get(host)
+ if err != nil || moving == nil || !registry.IsHold(moving.Status) {
+ t.Fatalf("row %+v (%v): want it held by the move", moving, err)
+ }
+ if !heldHolds().retryHost(host, "v2", hr.HeldBook, hr.HeldEpoch) {
+ t.Fatal("a grant that arrived while the row was moving cleared nothing")
+ }
+ if heldHolds().book.HeldDown(acct, "blog", "v2", host) {
+ t.Fatal("the hold-down survived the grant")
+ }
+}
diff --git a/internal/host/rolloutdeclined.go b/internal/host/rolloutdeclined.go
index 8933705e5..a2c25040c 100644
--- a/internal/host/rolloutdeclined.go
+++ b/internal/host/rolloutdeclined.go
@@ -167,8 +167,13 @@ type hold struct {
// marker's build when the marker refuses the marked build or the good
// build, and otherwise the build a record holds, which a marker may refuse
// in its own words while only that record's retry lifts it.
+//
+// Eligibility is rollout.Restrictable, not Managed (final review round 1,
+// finding 2): a row a move holds (landing, restoring) is the row a moves
+// probe or landing launches, and the book's restriction of the instance
+// applies there as on its ready row.
func (h *failureHolds) holdOf(rt registry.Route, build string) hold {
- if !rollout.Managed(rt) || build == "" {
+ if !rollout.Restrictable(rt) || build == "" {
return hold{}
}
acct, app, host := release.RouteAccount(rt), rt.App, rt.Host
@@ -203,7 +208,7 @@ func (h *failureHolds) holdOf(rt registry.Route, build string) hold {
// root repoints it before it stamps the row; an unreadable link is left to
// the stamp.
func (h *failureHolds) heldBuild(rt registry.Route) (string, hold) {
- if !rollout.Managed(rt) {
+ if !rollout.Restrictable(rt) {
return "", hold{}
}
builds := []string{rt.Version}
@@ -271,7 +276,10 @@ func (h *failureHolds) report() map[string]status.HostRollout {
// canary: the next wake, or the converge leg, starts the build as today.
func (h *failureHolds) retryHost(host, build, book string, epoch uint64) bool {
rt, err := h.get(host)
- if err != nil || rt == nil || !rollout.Managed(*rt) || build == "" {
+ // Restrictable, not Managed (finding 2): a grant that arrives while a
+ // move holds the row (draining, moving, landing) is applied, not read
+ // once by the restart leg and dropped.
+ if err != nil || rt == nil || !rollout.Restrictable(*rt) || build == "" {
return false
}
acct := release.RouteAccount(*rt)
diff --git a/internal/rollout/manager.go b/internal/rollout/manager.go
index d88c51e17..a6be01e16 100644
--- a/internal/rollout/manager.go
+++ b/internal/rollout/manager.go
@@ -388,8 +388,7 @@ func (m *Manager) Pass(routes []registry.Route) {
// the queue, its pending restart or config, and its held ack; a row that is
// gone, or that stopped being an exec route this scheduler moves, does not.
func heldOnBox(rt registry.Route) bool {
- rt.Status = "ready"
- return Managed(rt)
+ return Restrictable(rt)
}
// forget drops what the manager holds for hosts no longer on the box: a
diff --git a/internal/rollout/policy.go b/internal/rollout/policy.go
index 924d9a798..54efe20ee 100644
--- a/internal/rollout/policy.go
+++ b/internal/rollout/policy.go
@@ -167,3 +167,15 @@ func Managed(rt registry.Route) bool {
}
return release.Adoptable(rt) && rt.Hibernate
}
+
+// Restrictable reports whether the failure book can restrict rt: Managed,
+// with whatever status the row passes through (draining, or a move's hold:
+// moving, landing, restoring, releasing) read as ready. A move holds the row
+// while its probe or landing launches the instance, and a restriction is a
+// fact about the instance and its database, not about the row's status, so
+// it must still apply there (final review round 1, finding 2, Astra).
+// Whether the move may launch at all is the moves package's own check.
+func Restrictable(rt registry.Route) bool {
+ rt.Status = "ready"
+ return Managed(rt)
+}