amadan

carlos / platform Public

activator, host: a bounce during a wake is held, not dropped; review fixes

Astra review of 76e0cb38 (reject, one P1):

- P1: with the converge leg off the tick, a release and a config change
  arriving together could cycle the instance before config sync wrote
  the new file. A request then woke the new build on the old config, and
  config sync's Bounce was refused because the entry was still waking.
  Nothing asks again: config delivery bounces only when the rendered file
  changes. The same race already existed for any wake that overlapped a
  config write. Bounce now holds a bounce that lands during an instance's
  wake and spends it as soon as the wake finishes, before any waiting
  request is let through, the way a fence already is. Workers keep the
  old rule: their restart pairs a stop with its own WakeWorker only when
  Bounce reports one.
- P2: a test now holds the fast tick inside its full pass and shows the
  converge leg acting meanwhile.
- P3: the outer 'serial pass set' timing line duplicated the per-stage
  one and is gone.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude b43d05d8e34547dcf31c67d7e1ea7db47c8f5a08 parent 76e0cb3
4 files changed, +120 −7
  • internal/activator/activator.go +28 −5
  • internal/activator/converge_adopted_test.go +43 −0
  • internal/host/agent.go +0 −2
  • internal/host/converge_adopted_test.go +49 −0

CI passed — run details

diff --git a/internal/activator/activator.go b/internal/activator/activator.go
index 0a719ed03..a1afa63f4 100644
--- a/internal/activator/activator.go
+++ b/internal/activator/activator.go
@@ -341,9 +341,14 @@ const (
)
type actEntry struct {
- host string
- route registry.Route
- state actState
+ // bounceAfterWake is a Bounce that arrived while this INSTANCE was still
+ // waking. The wake had already read its config and execed its build, so
+ // the bounce's reason has not reached the child; wake() spends it the
+ // moment the wake finishes, before any waiting request is admitted.
+ bounceAfterWake bool
+ host string
+ route registry.Route
+ state actState
ready chan struct{} // waking: closed when the wake resolves; then check err
err error // waking: the wake's outcome
@@ -986,8 +991,16 @@ func (a *Service) wake(e *actEntry) {
e.staleSince = time.Time{}
if a.fences[e.host] != 0 {
a.beginStopLocked(e)
- }
- if a.OnWake != nil && a.fences[e.host] == 0 {
+ } else if e.bounceAfterWake {
+ // A config delivery or `carlos restart` asked for while this wake
+ // was under way (Bounce). Same spot and same reason as the fence
+ // just above: the state flips to stopping before e.ready closes,
+ // so a request waiting on the wake is never proxied to this child
+ // — it waits out the stop and wakes a fresh one.
+ e.bounceAfterWake = false
+ a.bounceLocked(e)
+ }
+ if a.OnWake != nil && e.state == actAwake {
// Fire-and-forget, off this goroutine: e.route is read once here,
// before this goroutine can be reused for anything else — the
// hook must never share e.host's own goroutine's fate (a wedged
@@ -1402,6 +1415,16 @@ func (a *Service) Bounce(host string) bool {
a.mu.Lock()
defer a.mu.Unlock()
e := a.entries[host]
+ if e != nil && e.state == actWaking && !e.worker {
+ // Not dropped: held for the end of the wake (bounceAfterWake). Still
+ // reported as no stop, because none has happened — the callers that
+ // count stops, or fall through to systemctl on false, see what they
+ // always saw. Workers keep the old rule: their restart pairs a stop
+ // with its own WakeWorker only on true (host/restart.go cycleWorkers),
+ // and a stop held for later would park one with nothing to start it.
+ e.bounceAfterWake = true
+ return false
+ }
if e == nil || e.state != actAwake {
return false
}
diff --git a/internal/activator/converge_adopted_test.go b/internal/activator/converge_adopted_test.go
index 31f5781e9..be7ed7f55 100644
--- a/internal/activator/converge_adopted_test.go
+++ b/internal/activator/converge_adopted_test.go
@@ -201,3 +201,46 @@ func TestBounceAdoptedBouncesWhenTheBuildIsUnknown(t *testing.T) {
}
waitFor(t, "the bounce", func() bool { return a.entryCount() == 0 })
}
+
+// A bounce that lands while the instance is still waking is not dropped. The
+// wake already read its config and execed its build, so whatever the bounce
+// was for — new config, a `carlos restart` — has not reached the child, and
+// nothing will ask again: config delivery bounces only when the rendered
+// file changes. The bounce is held and spent the moment the wake finishes,
+// before any waiting request is let through, so every request lands on a
+// child started after the bounce was asked for.
+func TestBounceDuringAWakeIsAppliedWhenTheWakeFinishes(t *testing.T) {
+ a, b, _, _ := testActivator(t, time.Hour)
+ b.startGate = make(chan struct{})
+ host := "one.example.com"
+
+ served := make(chan error, 1)
+ go func() {
+ release, err := a.beginRequest(context.Background(), testRoute(host))
+ if err == nil {
+ release()
+ }
+ served <- err
+ }()
+ waitFor(t, "the wake to reach Start", func() bool { return b.startCount() == 1 })
+
+ if a.Bounce(host) {
+ t.Fatal("Bounce reported a stop on an instance that is still waking")
+ }
+ close(b.startGate)
+
+ if err := <-served; err != nil {
+ t.Fatalf("the request waiting on the wake: %v", err)
+ }
+ // It was served by a SECOND child: the first was stopped as soon as its
+ // wake finished, and the waiting request woke a fresh one.
+ if got := b.startCount(); got != 2 {
+ t.Fatalf("children started = %d, want 2: the bounce asked for during the wake was dropped", got)
+ }
+ if !b.procs[0].isStopped() {
+ t.Fatal("the child from the wake the bounce landed in is still running")
+ }
+ if a.Bounce(host) != true {
+ t.Fatal("an ordinary bounce of the awake replacement was refused")
+ }
+}
diff --git a/internal/host/agent.go b/internal/host/agent.go
index b9c9c2ee9..b38bf0ae4 100644
--- a/internal/host/agent.go
+++ b/internal/host/agent.go
@@ -1784,9 +1784,7 @@ func fastTick(ctx context.Context, d *fastDeps, now time.Time) {
default:
d.serialBroken = false
if doc.Serial != d.lastSerial && now.Sub(d.lastFull) >= fastFullClamp {
- start := time.Now()
d.fullPass(ctx)
- logSlow("serial pass set", start, "")
if d.restartLeg != nil {
d.restartLeg(ctx)
}
diff --git a/internal/host/converge_adopted_test.go b/internal/host/converge_adopted_test.go
index 36c98c91a..01630515a 100644
--- a/internal/host/converge_adopted_test.go
+++ b/internal/host/converge_adopted_test.go
@@ -447,3 +447,52 @@ func TestLogSlowSpeaksOnlyWhenSlow(t *testing.T) {
}
}
}
+
+// The point of the leg's own loop: it acts while the fast tick is stuck in a
+// long full pass. The pass is held open here until the leg has cycled the
+// drifted instance, which it could never do if it still rode the tick.
+func TestConvergeLoopActsWhileTheFullPassIsBusy(t *testing.T) {
+ env := newFastFixture(t)
+ d := env.deps(nil)
+ inPass, unblock := make(chan struct{}), make(chan struct{})
+ d.fullPass = func(context.Context) { close(inPass); <-unblock }
+
+ cycled := make(chan string, 1)
+ leg := func() {
+ convergeAdopted(
+ func() map[string]string { return map[string]string{"warm.test": "v2"} },
+ func(h string) (*registry.Route, error) { return &registry.Route{Host: h, Version: "v2"}, nil },
+ func(h, ver string) bool {
+ select {
+ case cycled <- h + "@" + ver:
+ default:
+ }
+ return true
+ },
+ adoptedCyclesPerTick)
+ }
+ stop := make(chan struct{})
+ defer close(stop)
+ go runConvergeLoop(leg, 5*time.Millisecond, stop)
+
+ ctx := context.Background()
+ now := time.Unix(1754640000, 0)
+ fastTick(ctx, d, now) // seeds the serial
+ if err := release.BumpSerial(ctx, env.store, now); err != nil {
+ t.Fatalf("BumpSerial: %v", err)
+ }
+ tickDone := make(chan struct{})
+ go func() { fastTick(ctx, d, now.Add(10*time.Second)); close(tickDone) }()
+ <-inPass // the tick is now stuck in its full pass
+
+ select {
+ case got := <-cycled:
+ if got != "warm.test@v2" {
+ t.Fatalf("cycled %q", got)
+ }
+ case <-time.After(10 * time.Second):
+ t.Fatal("the converge leg waited for the full pass")
+ }
+ close(unblock)
+ <-tickDone
+}