carlos / platform Public

host: cycleWorkers dispatches sidecar wakes off the tick's blocking path

Paul Campbell pushed by paul@keymail.dev ab0f4e3c16f48eef5723f0df38cc9725cfaff48d parent c47079f
2 files changed, +148 −11
  • internal/host/restart.go +37 −5
  • internal/host/restart_test.go +111 −6
diff --git a/internal/host/restart.go b/internal/host/restart.go
index fd2e7febd..5f2646328 100644
--- a/internal/host/restart.go
+++ b/internal/host/restart.go
@@ -242,6 +242,24 @@ func bounceExecRoutes(routes []registry.Route, moved [][2]string, bounce func(ho
// activator.Service.Bounce's own doc comment, which states the same
// contract from the activator's side.
//
+// The BOUNCE half is synchronous, in-process and effectively instant — it
+// is counted in the return value, exactly like bounceExecRoutes' own bounce
+// call. The WAKE half is NOT: WakeWorker can block up to wakeWaitBound
+// (30s, activator.go) per sidecar waiting out the settle window, and this
+// function's only caller (restartRequestLeg) runs synchronously on the
+// box's single per-tick convergence goroutine (fastTick) — waiting here
+// would delay every other app's serial-driven convergence, and the
+// root-restart marker touch that relays unit-backed apps' own restarts,
+// sequentially, by up to that bound PER SIDECAR (fix round 2 review
+// finding). Wakes are therefore dispatched off this call's return path,
+// bounded by the same workerBootConcurrency Task 8's worker boot pass uses
+// (internal/host/agent.go) — one idiom for "wake several workers without
+// blocking a caller", not a second one. Safe because WakeWorker is
+// idempotent and `carlos restart`'s contract is "cycle the processes", not
+// "return once they are up" — a sidecar that comes up a moment after this
+// tick has still been restarted. A wake that errors is logged and does not
+// stop, delay, or fail the others.
+//
// wake is called ONLY for a host bounce actually stopped (true): a sidecar
// that was already asleep is left asleep, the same "never wake a sleeper"
// rule bounceExecRoutes' own HIBERNATE handling already gives an instance.
@@ -254,6 +272,7 @@ func cycleWorkers(routes []registry.Route, moved [][2]string, bounce func(host s
for _, k := range moved {
want[k] = true
}
+ sem := make(chan struct{}, workerBootConcurrency)
n := 0
for _, rt := range routes {
if !rt.IsSidecar() || !want[[2]string{release.RouteAccount(rt), rt.App}] {
@@ -264,9 +283,14 @@ func cycleWorkers(routes []registry.Route, moved [][2]string, bounce func(host s
}
n++
log.Printf("agent: restart requested: bounced sidecar %s", rt.Host)
- if err := wake(rt); err != nil {
- log.Printf("agent: restart requested: re-wake sidecar %s: %v", rt.Host, err)
- }
+ rt := rt
+ sem <- struct{}{}
+ go func() {
+ defer func() { <-sem }()
+ if err := wake(rt); err != nil {
+ log.Printf("agent: restart requested: re-wake sidecar %s: %v", rt.Host, err)
+ }
+ }()
}
return n
}
@@ -665,12 +689,20 @@ func restartPass(ctx context.Context, routes []registry.Route, store release.Sto
//
// The bounce goes first, and not only for tidiness: it is in-process and
// instant, while the marker write can block on a busy /data, and the children
-// bounced here are the ones cycled here or nowhere.
+// bounced here are the ones cycled here or nowhere. That "instant" holds for
+// EVERY bounce call this leg makes (bounceExecRoutes' and cycleWorkers'
+// alike) — cycleWorkers' own wake calls are the one thing on this path that
+// are not instant (WakeWorker can block up to 30s per sidecar), which is
+// exactly why cycleWorkers dispatches them off its own return path rather
+// than this leg waiting on them: this whole function still returns promptly
+// after the bounces, this leg's synchronous work (the marker touch) is
+// never delayed by a sidecar's wake, and neither is the next app's turn on
+// this box's single per-tick convergence goroutine (fastTick).
//
// wake is svc.WakeWorker in production (wakeFn), nil-safe like bounce: it is
// cycleWorkers' other half, called only for the sidecars this leg actually
// stops (see cycleWorkers' doc comment for why a sidecar needs both calls,
-// where an instance needs only bounce).
+// where an instance needs only bounce, and for why the wake is async).
func restartRequestLeg(ctx context.Context, routes []registry.Route, store release.Store, dd string,
bounce func(host string) bool, wake func(rt registry.Route) error, cache map[[2]string]string) bool {
moved, readErrs := restartGenPass(ctx, routes, store, cache)
diff --git a/internal/host/restart_test.go b/internal/host/restart_test.go
index 30582937e..dbf649ac6 100644
--- a/internal/host/restart_test.go
+++ b/internal/host/restart_test.go
@@ -770,16 +770,121 @@ func sidecarRoute(host, acct, app string) registry.Route {
func TestCycleWorkersBouncesAndWakesARunningSidecar(t *testing.T) {
routes := []registry.Route{sidecarRoute("bot.x.sidecar.internal", "bdm", "eleven")}
moved := [][2]string{{"bdm", "eleven"}}
+ var mu sync.Mutex
var bounced, woken []string
- bounce := func(h string) bool { bounced = append(bounced, h); return true }
- wake := func(rt registry.Route) error { woken = append(woken, rt.Host); return nil }
+ bounce := func(h string) bool { mu.Lock(); defer mu.Unlock(); bounced = append(bounced, h); return true }
+ wake := func(rt registry.Route) error {
+ mu.Lock()
+ defer mu.Unlock()
+ woken = append(woken, rt.Host)
+ return nil
+ }
n := cycleWorkers(routes, moved, bounce, wake)
- if n != 1 || !slices.Equal(bounced, []string{"bot.x.sidecar.internal"}) {
- t.Fatalf("bounced %v (n=%d), want exactly the sidecar", bounced, n)
+ mu.Lock()
+ bouncedSnap := slices.Clone(bounced)
+ mu.Unlock()
+ if n != 1 || !slices.Equal(bouncedSnap, []string{"bot.x.sidecar.internal"}) {
+ t.Fatalf("bounced %v (n=%d), want exactly the sidecar", bouncedSnap, n)
+ }
+ // The wake is dispatched OFF cycleWorkers' return path (fix round 2) — it
+ // may not have landed yet the instant the call above returns, so this
+ // waits for it rather than asserting synchronously.
+ waitForCond(t, "the bounced sidecar to be re-woken", func() bool {
+ mu.Lock()
+ defer mu.Unlock()
+ return slices.Equal(woken, []string{"bot.x.sidecar.internal"})
+ })
+}
+
+// TestCycleWorkersReturnsBeforeASlowWakeCompletes is fix round 2's own
+// finding, pinned directly: WakeWorker can block up to 30s per sidecar
+// (the settle window), and cycleWorkers' only caller (restartRequestLeg)
+// runs synchronously on the box's single per-tick convergence goroutine —
+// so cycleWorkers itself must never wait on a wake, or one slow sidecar
+// delays the root-restart marker touch and every other app's turn on this
+// box's tick. Asserted against a deliberately slow fake wake, the same
+// elapsed-time shape Task 8 used for the worker boot pass's own
+// bounded-concurrency claim.
+func TestCycleWorkersReturnsBeforeASlowWakeCompletes(t *testing.T) {
+ routes := []registry.Route{sidecarRoute("bot.x.sidecar.internal", "bdm", "eleven")}
+ moved := [][2]string{{"bdm", "eleven"}}
+ slow := make(chan struct{})
+ bounce := func(h string) bool { return true }
+ wake := func(rt registry.Route) error { <-slow; return nil }
+
+ start := time.Now()
+ n := cycleWorkers(routes, moved, bounce, wake)
+ elapsed := time.Since(start)
+ close(slow) // release the parked wake goroutine so the test can exit cleanly
+
+ if n != 1 {
+ t.Fatalf("cycleWorkers = %d, want 1 (the bounce itself is synchronous)", n)
+ }
+ if elapsed > time.Second {
+ t.Fatalf("cycleWorkers took %s to return with a wake blocked indefinitely — the wake must be dispatched asynchronously, not awaited", elapsed)
+ }
+}
+
+// TestCycleWorkersWakesEveryBouncedSidecarEventually: dispatching wakes
+// asynchronously must not mean dropping any of them — every sidecar
+// cycleWorkers bounces still gets woken, just not before this function
+// returns.
+func TestCycleWorkersWakesEveryBouncedSidecarEventually(t *testing.T) {
+ routes := []registry.Route{
+ sidecarRoute("bot.a.sidecar.internal", "bdm", "eleven"),
+ sidecarRoute("bot.b.sidecar.internal", "bdm", "eleven"),
+ sidecarRoute("bot.c.sidecar.internal", "bdm", "eleven"),
+ }
+ moved := [][2]string{{"bdm", "eleven"}}
+ var mu sync.Mutex
+ woken := map[string]bool{}
+ bounce := func(h string) bool { return true }
+ wake := func(rt registry.Route) error {
+ mu.Lock()
+ defer mu.Unlock()
+ woken[rt.Host] = true
+ return nil
+ }
+ if n := cycleWorkers(routes, moved, bounce, wake); n != 3 {
+ t.Fatalf("cycleWorkers = %d, want 3", n)
}
- if !slices.Equal(woken, []string{"bot.x.sidecar.internal"}) {
- t.Fatalf("woken %v, want the bounced sidecar re-woken", woken)
+ waitForCond(t, "all three sidecars to be re-woken", func() bool {
+ mu.Lock()
+ defer mu.Unlock()
+ return len(woken) == 3
+ })
+}
+
+// TestCycleWorkersFailingWakeDoesNotStopTheOthers: a wake error is logged
+// (not asserted here — bounceExecRoutes' own analogous case doesn't pin the
+// log line either) and must never prevent another sidecar's wake from
+// running, nor cycleWorkers' own return.
+func TestCycleWorkersFailingWakeDoesNotStopTheOthers(t *testing.T) {
+ routes := []registry.Route{
+ sidecarRoute("bot.bad.sidecar.internal", "bdm", "eleven"),
+ sidecarRoute("bot.good.sidecar.internal", "bdm", "eleven"),
}
+ moved := [][2]string{{"bdm", "eleven"}}
+ var mu sync.Mutex
+ woken := map[string]bool{}
+ bounce := func(h string) bool { return true }
+ wake := func(rt registry.Route) error {
+ if rt.Host == "bot.bad.sidecar.internal" {
+ return errors.New("boom")
+ }
+ mu.Lock()
+ defer mu.Unlock()
+ woken[rt.Host] = true
+ return nil
+ }
+ if n := cycleWorkers(routes, moved, bounce, wake); n != 2 {
+ t.Fatalf("cycleWorkers = %d, want 2 — both were bounced regardless of the wake outcome", n)
+ }
+ waitForCond(t, "the good sidecar to be re-woken despite the other's wake failing", func() bool {
+ mu.Lock()
+ defer mu.Unlock()
+ return woken["bot.good.sidecar.internal"]
+ })
}
// TestCycleWorkersLeavesASleepingSidecarAsleep: Bounce is a no-op on