carlos / platform Public

Wait on the respawn's channel, never on its transient flag

Paul Campbell pushed by paul@keymail.dev b2db5e73e407f34fb207f6c4b8c912a3570dcba1 parent 5fb391e
2 files changed, +85 −2
  • internal/activator/activator.go +13 −2
  • internal/activator/activator_test.go +72 −0
diff --git a/internal/activator/activator.go b/internal/activator/activator.go
index 10cf5ad9a..8c3aa660d 100644
--- a/internal/activator/activator.go
+++ b/internal/activator/activator.go
@@ -1448,11 +1448,22 @@ func (a *Service) stop(e *actEntry) {
// this wait short (it aborts as soon as it sees this state flip); the
// bound is insurance against a wedged exec, and instances never wait at
// all (respawnDone is nil for them).
+ //
+ // Gated on the CHANNEL alone, deliberately, and never on
+ // e.workerRespawning: that flag is cleared in the respawn's final critical
+ // section, BEFORE it sheds a child it raced into existence, so a stop
+ // goroutine scheduled after that section would read false and skip the
+ // wait entirely — with up to sidecarStopTimeout of draining child left to
+ // go. beginStopLocked flips the state under a.mu and then launches this
+ // goroutine, so the two genuinely race for the lock with nothing ordering
+ // them. The channel does not have that gap: it is created before the
+ // respawn starts and closed only when the respawn is finished with every
+ // child it made. A respawn that ended long ago costs a receive on an
+ // already-closed channel.
a.mu.Lock()
waitRespawn := e.respawnDone
- respawning := e.workerRespawning
a.mu.Unlock()
- if respawning && waitRespawn != nil {
+ if waitRespawn != nil {
// The bound covers the respawn goroutine's OWN worst case end to end —
// retiring the old child, the exec, and shedding a raced one — so this
// wait can never give up on a respawn that is still making progress.
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go
index 0dc9ddc83..fc40a3ca9 100644
--- a/internal/activator/activator_test.go
+++ b/internal/activator/activator_test.go
@@ -2902,6 +2902,78 @@ func TestRespawnShedsARacedChildBeforeTheLeaseIsReleased(t *testing.T) {
}
}
+// The same invariant, but against the SCHEDULING order rather than the exec:
+// the stop goroutine arrives after the respawn has cleared its interlock and
+// while the shed is still draining. Gating the wait on that flag would skip it
+// here — the flag is cleared in the respawn's final critical section, before
+// the shed — and the lease would go while a child was still alive.
+//
+// beginStopLocked flips the state under a.mu and then launches the stop, so in
+// production the two race for the lock with nothing ordering them. This test
+// arranges the losing order deliberately, from inside the package: it holds
+// a.mu until the respawn's final critical section is queued behind it, condemns
+// the entry, and releases — Go hands a mutex directly to a waiter that has
+// waited past its starvation threshold, so the respawn runs first and the stop
+// goroutine reads the entry second. No invariant is bent to produce it; only
+// the schedule is pinned.
+func TestStopScheduledAfterTheInterlockClearStillWaitsForTheShed(t *testing.T) {
+ a, b, clk, ver := shippingActivator(t, time.Hour)
+ if err := a.WakeWorker(alwaysOnWorkerRoute("bot.x.internal")); err != nil {
+ t.Fatalf("WakeWorker: %v", err)
+ }
+ rep := b.lastRep()
+ gate := make(chan struct{})
+ b.mu.Lock()
+ b.workerStartDelay = 150 * time.Millisecond
+ b.workerStopGate = gate // held on the respawn's child: its drain blocks
+ b.mu.Unlock()
+
+ ver.Store("v2")
+ a.sweepOnce()
+ clk.advance(ConvergeGrace + time.Minute)
+ a.sweepOnce()
+ waitFor(t, "the respawn's exec to begin", func() bool { return b.workerEnters() == 2 })
+
+ // Take the lock while the exec is still running, and hold it: the respawn
+ // finishes and queues behind us for its final critical section. THEN
+ // condemn the entry — so its `live` check will fail and it must shed —
+ // and launch the stop, which queues behind the respawn. On release the
+ // respawn goes first (clearing the interlock, entering its gated shed) and
+ // the stop reads second: the order that used to skip the wait.
+ a.mu.Lock()
+ time.Sleep(300 * time.Millisecond) // exec completes; that goroutine queues on a.mu
+ e := a.entries["bot.x.internal"]
+ if e != nil {
+ a.beginStopLocked(e)
+ }
+ a.mu.Unlock()
+ if e == nil {
+ t.Fatal("the worker entry vanished before the stop could be arranged")
+ }
+ waitFor(t, "the raced child to be spawned", func() bool { return b.workerStarts() == 2 })
+
+ // The shed is now draining behind the gate. The lease must still be held:
+ // releasing it here is releasing it under a live process.
+ deadline := time.Now().Add(300 * time.Millisecond)
+ for time.Now().Before(deadline) {
+ if rep.isClosed() {
+ t.Fatal("the lease was released while the respawn's raced child was still draining — the stop skipped its wait because the interlock had already been cleared")
+ }
+ time.Sleep(10 * time.Millisecond)
+ }
+
+ close(gate)
+ waitFor(t, "the stop to complete", func() bool { return a.entryCount() == 0 })
+ if n := rep.closeCount(); n != 1 {
+ t.Fatalf("lease closes = %d, want exactly 1", n)
+ }
+ for i, p := range b.workers {
+ if !p.isStopped() {
+ t.Fatalf("worker child %d survived the stop", i)
+ }
+ }
+}
+
// Shutdown CANCELS an in-flight worker spawn rather than waiting it out. The
// wait is right everywhere else, but a shutdown answers to a deadline it does
// not set: systemd's TimeoutStopSec defaults to 90s and workerSpawnTimeout