amadan

carlos / platform Public

zdd-readiness: Task 17 fix round 1, deterministic status tests, a host that joined mid-report reads transient

The two Status tests installed v2 and passed, so real swap goroutines raced them: they now install nothing. The lock probe only TryLocks (a violation fails instead of deadlocking), ignores the scheduler goroutine's own asks and snapshots the schedulers before Status. Status keeps the set of hosts it asked and reports one it did not ask about as transient. The Transient doc says what an older reader sees.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 61b83e279bf32acff74faba59fdc82a3857ae43a parent 98c1763
3 files changed, +123 −46
  • internal/rollout/status.go +18 −19
  • internal/rollout/status_test.go +102 −26
  • internal/status/snapshot.go +3 −1
diff --git a/internal/rollout/status.go b/internal/rollout/status.go
index 323c780f2..6fd9fbbcb 100644
--- a/internal/rollout/status.go
+++ b/internal/rollout/status.go
@@ -47,14 +47,17 @@ func (m *Manager) Status() status.RolloutReport {
}
}
m.mu.Unlock()
- transient := m.transientHosts(scheds)
+ asked := m.askActivator(scheds)
for _, s := range scheds {
app := status.AppRollout{}
s.mu.Lock()
for host, h := range s.q.hosts {
hr := status.HostRollout{Account: s.key.acct, App: s.key.app, Installed: h.wantBuild, Serving: h.serving,
Good: h.good, Awake: h.serving != ""}
- if transient[host] {
+ // A host the activator was not asked about joined between the name
+ // read and this loop: nothing says it is settled, so this report
+ // calls it transient and the next one asks.
+ if tr, ok := asked[host]; !ok || tr {
hr.Serving, hr.Awake, hr.Transient = "", false, true
}
if v, ok := cfg[host]; ok {
@@ -122,16 +125,16 @@ func (m *Manager) Status() status.RolloutReport {
return r
}
-// transientHosts is the set of hosts, among the schedulers', whose process is
-// starting or stopping: the activator owns the host and has no current
-// generation for it (a wake or an observation not yet current, a hibernation
-// under way). A host the activator does not own is asleep, and one it has a
-// current generation for is awake; neither is in the set. The names are read
-// under each scheduler.mu and the activator is asked after it is released
-// (scheduler.view's rule), so no Manager or scheduler lock is held across the
-// ask. A host that joins after the names are read is simply not transient in
-// this report; the next one has it.
-func (m *Manager) transientHosts(scheds []*scheduler) map[string]bool {
+// askActivator answers, for each host among the schedulers' at the moment
+// their names are read, whether its process is starting or stopping: the
+// activator owns the host and has no current generation for it (a wake or an
+// observation not yet current, a hibernation under way). A host the activator
+// does not own is asleep, and one it has a current generation for is awake;
+// both answer false. The names are read under each scheduler.mu and the
+// activator is asked after it is released (scheduler.view's rule), so no
+// Manager or scheduler lock is held across the ask. A host missing from the
+// answer joined after the names were read, and Status reports it transient.
+func (m *Manager) askActivator(scheds []*scheduler) map[string]bool {
var names []string
for _, s := range scheds {
s.mu.Lock()
@@ -140,17 +143,13 @@ func (m *Manager) transientHosts(scheds []*scheduler) map[string]bool {
}
s.mu.Unlock()
}
- var out map[string]bool
+ out := make(map[string]bool, len(names))
for _, host := range names {
if g, ok := m.d.Act.Current(host); ok && g.Build != "" {
+ out[host] = false
continue
}
- if m.d.Act.Owns(host) {
- if out == nil {
- out = map[string]bool{}
- }
- out[host] = true
- }
+ out[host] = m.d.Act.Owns(host)
}
return out
}
diff --git a/internal/rollout/status_test.go b/internal/rollout/status_test.go
index e1d1a3eb9..d42704637 100644
--- a/internal/rollout/status_test.go
+++ b/internal/rollout/status_test.go
@@ -2,8 +2,12 @@ package rollout
import (
"context"
+ "runtime"
+ "strings"
+ "sync"
"sync/atomic"
"testing"
+ "time"
"github.com/carlosframework/platform/internal/activator"
"github.com/carlosframework/platform/internal/release"
@@ -217,13 +221,13 @@ func TestPassForgetsHostsThatLeftTheBox(t *testing.T) {
// A host whose process is starting or stopping (the activator owns it and has
// no current generation) is transient: no serving build and not awake, though
// the last pass cached one. One that is awake or asleep is not (Astra R3
-// section 5 item 10).
+// section 5 item 10). Nothing is installed, so no attempt runs: the harness's
+// swap goroutines would otherwise write the activator's generations under the
+// test.
func TestStatusReportsAStartingOrStoppingHostAsTransient(t *testing.T) {
h := newHarness(t, "a", "b", "c")
- h.stage("v2")
- h.install("v2", "a", "b", "c")
h.pass()
- if got := h.m.Status().Hosts["a"]; got.Transient || got.Serving != "v1" || !got.Awake {
+ if got := h.m.Status().Hosts["a"]; got.Transient || got.Installed != "v1" || got.Serving != "v1" || !got.Awake {
t.Fatalf("a before = %+v, want awake on v1 and not transient", got)
}
h.act.mu.Lock()
@@ -235,7 +239,7 @@ func TestStatusReportsAStartingOrStoppingHostAsTransient(t *testing.T) {
// report must not repeat it for a host the activator says is mid-change.
r := h.m.Status()
a := r.Hosts["a"]
- if !a.Transient || a.Serving != "" || a.Awake || a.Installed != "v2" {
+ if !a.Transient || a.Serving != "" || a.Awake || a.Installed != "v1" {
t.Fatalf("a = %+v, want transient, not awake, no serving build", a)
}
if b := r.Hosts["b"]; b.Transient {
@@ -247,13 +251,22 @@ func TestStatusReportsAStartingOrStoppingHostAsTransient(t *testing.T) {
}
// Status asks the activator with no Manager or scheduler lock held (the lock
-// order on Manager).
+// order on Manager). The probe only TryLocks, so a violation fails the test
+// instead of deadlocking it. No attempt runs (nothing is installed), so no
+// other goroutine holds these locks.
func TestStatusAsksTheActivatorWithNoLockHeld(t *testing.T) {
- h := newHarness(t, "a")
- h.stage("v2")
- h.install("v2", "a")
+ h := newHarness(t, "a", "b")
h.pass()
- probe := &lockProbeAct{fakeAct: h.act, m: h.m, t: t}
+ h.m.mu.Lock()
+ scheds := make([]*scheduler, 0, len(h.m.apps))
+ for _, s := range h.m.apps {
+ scheds = append(scheds, s)
+ }
+ h.m.mu.Unlock()
+ if len(scheds) == 0 {
+ t.Fatal("the pass built no scheduler")
+ }
+ probe := &lockProbeAct{fakeAct: h.act, m: h.m, scheds: scheds, t: t}
h.m.d.Act = probe
h.m.Status()
if probe.asked.Load() == 0 {
@@ -265,29 +278,52 @@ func TestStatusAsksTheActivatorWithNoLockHeld(t *testing.T) {
// Manager's or a scheduler's mutex is held.
type lockProbeAct struct {
*fakeAct
- m *Manager
- t *testing.T
- asked atomic.Int64
+ m *Manager
+ scheds []*scheduler
+ t *testing.T
+ asked atomic.Int64
}
func (p *lockProbeAct) check() {
+ if !askedByStatus() {
+ return // the scheduler's own goroutine asks too; only Status is under test
+ }
p.asked.Add(1)
- if !p.m.mu.TryLock() {
- p.t.Error("activator asked with Manager.mu held")
- } else {
- p.m.mu.Unlock()
+ // A lock that Status holds stays held for the whole ask, while a
+ // scheduler goroutine's own hold is brief, so a violation is a lock that
+ // cannot be taken for a long while. TryLock, never Lock: a violation
+ // fails the test instead of deadlocking it.
+ free := func(mu *sync.Mutex) bool {
+ for end := time.Now().Add(300 * time.Millisecond); time.Now().Before(end); time.Sleep(time.Millisecond) {
+ if mu.TryLock() {
+ mu.Unlock()
+ return true
+ }
+ }
+ return false
}
- p.m.mu.Lock()
- scheds := make([]*scheduler, 0, len(p.m.apps))
- for _, s := range p.m.apps {
- scheds = append(scheds, s)
+ if !free(&p.m.mu) {
+ p.t.Error("activator asked with Manager.mu held")
}
- p.m.mu.Unlock()
- for _, s := range scheds {
- if !s.mu.TryLock() {
+ for _, s := range p.scheds {
+ if !free(&s.mu) {
p.t.Error("activator asked with a scheduler.mu held")
- } else {
- s.mu.Unlock()
+ }
+ }
+}
+
+// askedByStatus reports whether the calling goroutine is inside
+// Manager.Status.
+func askedByStatus() bool {
+ pcs := make([]uintptr, 32)
+ frames := runtime.CallersFrames(pcs[:runtime.Callers(2, pcs)])
+ for {
+ f, more := frames.Next()
+ if strings.HasSuffix(f.Function, "(*Manager).Status") {
+ return true
+ }
+ if !more {
+ return false
}
}
}
@@ -301,3 +337,43 @@ func (p *lockProbeAct) Current(host string) (activator.Gen, bool) {
p.check()
return p.fakeAct.Current(host)
}
+
+// A host that joins a scheduler after Status read the names was never asked of
+// the activator, so nothing says it is settled: that report calls it
+// transient, and the next one asks.
+func TestStatusCallsAHostThatJoinedAfterTheNamesWereReadTransient(t *testing.T) {
+ h := newHarness(t, "a")
+ h.pass()
+ h.m.mu.Lock()
+ var s *scheduler
+ for _, s = range h.m.apps {
+ }
+ h.m.mu.Unlock()
+ var once atomic.Bool
+ h.m.d.Act = &joinProbeAct{fakeAct: h.act, onAsk: func() {
+ if once.CompareAndSwap(false, true) { // the names are read; now a host joins
+ s.mu.Lock()
+ s.q.hosts["late"] = &hostSlot{wantBuild: "v1", good: "v1", serving: "v1"}
+ s.mu.Unlock()
+ }
+ }}
+ r := h.m.Status()
+ if late := r.Hosts["late"]; !late.Transient || late.Serving != "" || late.Awake {
+ t.Fatalf("late = %+v, want transient for the report that never asked", late)
+ }
+ if a := r.Hosts["a"]; a.Transient || a.Serving != "v1" {
+ t.Fatalf("a = %+v, want awake on v1", a)
+ }
+}
+
+type joinProbeAct struct {
+ *fakeAct
+ onAsk func()
+}
+
+func (p *joinProbeAct) Current(host string) (activator.Gen, bool) {
+ if askedByStatus() { // not the scheduler's own goroutine
+ p.onAsk()
+ }
+ return p.fakeAct.Current(host)
+}
diff --git a/internal/status/snapshot.go b/internal/status/snapshot.go
index fe1cbe0e2..c0dc42b44 100644
--- a/internal/status/snapshot.go
+++ b/internal/status/snapshot.go
@@ -301,7 +301,9 @@ type HostRollout struct {
// current generation for it: the process is starting or stopping, so the
// instance is neither asleep nor awake and no serving build is reported.
// The deploy watch keeps such a host pending (Astra R3 section 5 item 10).
- // Absent from an older box, which is false.
+ // Absent from an older box, which is false. A reader that ignores the
+ // field sees a transient host as Serving "" and Awake false, that is,
+ // asleep.
Transient bool `json:"transient,omitempty"`
}