amadan
Tell a hibernating sidecar's crash from its completion by exit status
The sweep restarted only always-on workers, so a hibernating one that crashed before it could set a next-due slept forever — nothing dials a sidecar, and its schedule was the way back up it never got to write. The log even called it "exited", indistinguishable from a bot that had finished its work. Spec §7.2 says a crash restarts, unqualified. Exit status is what separates them. For a hibernating worker a clean exit still means "my work is done" and parks as before; a non-zero exit is a crash and restarts with the existing doubling backoff, exactly as an always-on worker's does. Proc grows an optional Crashed() that only the exec backend implements (it is the only one that spawns a worker), read from ProcessState rather than from Wait's error so a clean exit whose pipes outlive it is not mistaken for a failure; a child that cannot say counts as not crashed, which is the recoverable direction. max_run now applies to always-on workers too — the wedged reconnect loop it exists to catch is exactly the shape a long-lived worker gets into — but it is spent there as a recycle of the child on the same entry, keeping the lease and replication, because stopping an always-on worker leaves nothing to start it again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014WvcQVWggXkE5BAkjRDgWF
3 files changed,
+173
−24
internal/activator/activator.go+75 −15internal/activator/activator_test.go+83 −9internal/activator/backend_exec.go+15 −0
diff --git a/internal/activator/activator.go b/internal/activator/activator.go| index e33edf39d..18b67170c 100644 |
| --- a/internal/activator/activator.go |
| +++ b/internal/activator/activator.go |
| @@ -249,6 +249,33 @@ type Proc interface { |
| Exited() bool |
| } |
| +// procCrasher is the OPTIONAL half of Proc: a child that can say how it |
| +// exited. A hibernating worker's exit is ambiguous without it — design §7.2 |
| +// says a crash restarts, unqualified, while a clean exit from a bot that has |
| +// finished its work is exactly what parking is for — and exit status is the |
| +// one fact that tells them apart. |
| +// |
| +// Optional rather than a widening of Proc because it is only ever consulted |
| +// for a worker, and ExecBackend (the only backend that spawns one; |
| +// UnitBackend refuses StartWorker outright) is the only implementation that |
| +// can answer. Kept additive so no other Proc — production or test — has to |
| +// grow a method it cannot answer honestly. |
| +type procCrasher interface { |
| + // Crashed reports a DEFINITELY non-zero exit. Only ever true for a child |
| + // that has actually exited. |
| + Crashed() bool |
| +} |
| + |
| +// procCrashed asks a Proc whether it crashed, defaulting to NO for a child |
| +// that cannot say. That default is the safe direction: a crash misread as a |
| +// finish parks a worker its schedule or an operator can still wake, while a |
| +// finish misread as a crash restarts a bot that had nothing left to do — |
| +// forever. |
| +func procCrashed(p Proc) bool { |
| + c, ok := p.(procCrasher) |
| + return ok && c.Crashed() |
| +} |
| + |
| // Replication classes — the registry's routes.replication vocabulary |
| // ("" | stream | batched | daily), mapped to litestream sync intervals by |
| // replicationSyncInterval. |
| @@ -1770,18 +1797,36 @@ func (a *Service) sweepOnce() { |
| continue |
| } |
| if e.proc.Exited() { |
| - // An ALWAYS-ON worker's exit is a crash: `hibernate: false` means |
| - // the record exists ⇒ the process runs, so it is restarted in |
| - // place — same supervision as the co-spawn path, and no lease |
| - // churn, because only the child died (the lease and replication |
| - // are still ours). A HIBERNATING worker's exit means "I finished |
| - // my work", and the stand-down below is exactly right for it — |
| - // as it is for an instance. |
| - if e.worker && !e.route.Hibernate { |
| + // A worker's exit is read by STATUS, which is the only thing that |
| + // can tell "my work is done" from "I fell over": |
| + // |
| + // - always-on (`hibernate: false`): the record exists ⇒ the |
| + // process runs, so ANY exit is restarted in place — same |
| + // supervision as the co-spawn path, and no lease churn, |
| + // because only the child died (the lease and replication are |
| + // still ours). |
| + // - hibernating, clean exit (status 0): the bot finished. Park |
| + // it — the stand-down below, exactly as for an instance. |
| + // - hibernating, NON-ZERO exit: a crash, and design §7.2 says a |
| + // crash restarts, unqualified. Without this arm a hibernating |
| + // worker that fell over before setting a next-due slept |
| + // forever, and the log called it "exited" as though it had |
| + // finished. |
| + // |
| + // The restart carries the same doubling backoff, so a crash loop |
| + // paces itself. It is checked BEFORE the idle arm on purpose: a |
| + // crashing child is failing, not idle, and what bounds a |
| + // permanent crash loop is max_run (below) — the ceiling declared |
| + // for exactly that shape of misbehaviour. |
| + if e.worker && (!e.route.Hibernate || procCrashed(e.proc)) { |
| a.restartWorkerLocked(e, now) |
| continue |
| } |
| - log.Printf("activator: instance %s exited unexpectedly — recycling", e.host) |
| + if e.worker { |
| + log.Printf("activator: sidecar %s exited cleanly — its work is done; parking it", e.host) |
| + } else { |
| + log.Printf("activator: instance %s exited unexpectedly — recycling", e.host) |
| + } |
| a.beginStopLocked(e) |
| continue |
| } |
| @@ -1796,11 +1841,13 @@ func (a *Service) sweepOnce() { |
| // both. |
| // |
| // An ALWAYS-ON worker has no idle clock at all (design §7.2: |
| - // "hibernate: false … no idle clock, no lease churn"), and no run |
| - // ceiling either. Both would strand it: a stopped always-on worker has |
| - // no wake source — no request can reach it, and it has no schedule — |
| - // so it would stay down until the agent restarted. An instance is |
| - // never in this arm: unflagged instances don't enter the activator. |
| + // "hibernate: false … no idle clock, no lease churn"): idleness is |
| + // not a reason to stop something whose record says it must be |
| + // running. It does still have a run CEILING, but that ceiling |
| + // recycles its child rather than stopping the entry (see max_run |
| + // below) — a stopped always-on worker has no wake source of its own. |
| + // An instance is never in this arm: unflagged instances don't enter |
| + // the activator. |
| hibernates := !e.worker || e.route.Hibernate |
| idleWindow := a.cfg.Idle |
| if e.workerIdle > 0 { |
| @@ -1815,7 +1862,20 @@ func (a *Service) sweepOnce() { |
| // reconnect loop must not hold a lease and a box forever. Zero means |
| // no ceiling — a sidecar that declared none is a legitimate shape, so |
| // this must never invent one nobody asked for. |
| - if e.worker && hibernates && e.workerMaxRun > 0 && now.Sub(e.spawnedAt) >= e.workerMaxRun { |
| + // |
| + // It applies to an ALWAYS-ON worker too — the wedged reconnect loop |
| + // this guard exists for is exactly the shape a long-lived worker gets |
| + // into — but it is spent differently there: the child is RECYCLED on |
| + // the same entry, keeping the lease and replication, because stopping |
| + // an always-on worker leaves nothing to start it again. That is the |
| + // same move build convergence makes for one (convergeWorkerLocked), |
| + // and for the same reason. |
| + if e.worker && e.workerMaxRun > 0 && now.Sub(e.spawnedAt) >= e.workerMaxRun { |
| + if !e.route.Hibernate { |
| + log.Printf("activator: always-on sidecar %s hit its max_run of %s — recycling its child", e.host, e.workerMaxRun) |
| + a.respawnWorkerLocked(e, e.proc, "hit its max_run") |
| + continue |
| + } |
| log.Printf("activator: %s hit its max_run of %s — stopping", e.host, e.workerMaxRun) |
| a.beginStopLocked(e) |
| continue |
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go| index 7999ec627..fc01cf0e4 100644 |
| --- a/internal/activator/activator_test.go |
| +++ b/internal/activator/activator_test.go |
| @@ -80,6 +80,9 @@ type fakeProc struct { |
| // stopGate, when non-nil, blocks Stop() until closed — holds an entry in |
| // the stopping state so a test can poke at it. |
| stopGate chan struct{} |
| + // crashed marks a NON-ZERO exit — the fact that tells a hibernating |
| + // worker's crash from its completion. |
| + crashed bool |
| // label + rec feed the backend's order log ("instance" / "sidecar"), so |
| // the co-spawn/co-stop tests can assert interleavings. |
| label string |
| @@ -118,6 +121,23 @@ func (p *fakeProc) Exited() bool { |
| defer p.mu.Unlock() |
| return p.dead |
| } |
| + |
| +// Crashed models the exec child's own answer (backend_exec.go's childProc): |
| +// only a definitely non-zero exit is a crash. kill() therefore models a CLEAN |
| +// exit — a bot that finished its work — and crash() models a child that fell |
| +// over. |
| +func (p *fakeProc) Crashed() bool { |
| + p.mu.Lock() |
| + defer p.mu.Unlock() |
| + return p.dead && p.crashed |
| +} |
| + |
| +// crash models the child dying with a NON-ZERO status. |
| +func (p *fakeProc) crash() { |
| + p.mu.Lock() |
| + p.dead, p.crashed = true, true |
| + p.mu.Unlock() |
| +} |
| func (p *fakeProc) isStopped() bool { |
| p.mu.Lock() |
| defer p.mu.Unlock() |
| @@ -3009,7 +3029,7 @@ func TestHibernatingWorkerExitSleepsRatherThanRestarting(t *testing.T) { |
| if err := a.WakeWorker(testWorkerRoute("bot.x.internal")); err != nil { // Hibernate: true |
| t.Fatalf("WakeWorker: %v", err) |
| } |
| - b.lastWorker().kill() |
| + b.lastWorker().kill() // a CLEAN exit: "my work is done" |
| a.sweepOnce() |
| waitFor(t, "the finished worker to sleep", func() bool { return a.entryCount() == 0 }) |
| @@ -3021,25 +3041,79 @@ func TestHibernatingWorkerExitSleepsRatherThanRestarting(t *testing.T) { |
| } |
| } |
| -// An always-on worker has no idle clock and no run ceiling: a stopped one has |
| -// no wake source at all (no request can reach it, and it has no schedule), so |
| -// either would strand it until the agent restarted. |
| -func TestAlwaysOnWorkerIgnoresIdleClockAndMaxRun(t *testing.T) { |
| - a, _, clk, _ := testActivator(t, 10*time.Second) |
| +// An always-on worker has no idle clock: idleness is no reason to stop |
| +// something whose record says it must be running, and a stopped one has no |
| +// wake source at all (no request can reach it, and it has no schedule). |
| +func TestAlwaysOnWorkerIgnoresTheIdleClock(t *testing.T) { |
| + a, b, clk, _ := testActivator(t, 10*time.Second) |
| rt := alwaysOnWorkerRoute("bot.x.internal") |
| rt.IdleSeconds = 5 |
| - rt.MaxRunSeconds = 30 |
| if err := a.WakeWorker(rt); err != nil { |
| t.Fatalf("WakeWorker: %v", err) |
| } |
| - clk.advance(time.Hour) // far past both the idle window and the ceiling |
| + clk.advance(time.Hour) // far past the idle window |
| a.sweepOnce() |
| // entryAwake, not entryCount: beginStopLocked flips the state under the |
| // lock and stops on its own goroutine, so a count taken right after the |
| // sweep would still see the entry it just condemned. |
| if !a.entryAwake("bot.x.internal") { |
| - t.Fatal("an always-on worker was stopped by an idle window or a run ceiling — nothing would ever wake it again") |
| + t.Fatal("an always-on worker was stopped by an idle window — nothing would ever wake it again") |
| + } |
| + if n := b.workerStarts(); n != 1 { |
| + t.Fatalf("workerStarts = %d, want 1 — nothing should have recycled a worker with no declared ceiling", n) |
| + } |
| +} |
| + |
| +// A HIBERNATING worker that exits NON-ZERO has crashed, and design §7.2 says |
| +// a crash restarts, unqualified. Read as a finish, it would sleep forever: |
| +// it fell over before it could set a next-due, and nothing can dial one. |
| +func TestHibernatingWorkerCrashRestartsInsteadOfSleeping(t *testing.T) { |
| + a, b, _, _ := testActivator(t, time.Hour) |
| + if err := a.WakeWorker(testWorkerRoute("bot.x.internal")); err != nil { // Hibernate: true |
| + t.Fatalf("WakeWorker: %v", err) |
| + } |
| + b.lastWorker().crash() // fell over, non-zero |
| + a.sweepOnce() |
| + |
| + waitForRestart(t, a, b, "bot.x.internal", 2) |
| + if !a.entryAwake("bot.x.internal") { |
| + t.Fatal("a crashed hibernating worker was parked — it never set a next-due, so nothing would wake it again") |
| + } |
| + if b.lastRep().isClosed() { |
| + t.Fatal("the lease was released on a crash restart — only the child died") |
| + } |
| +} |
| + |
| +// max_run is the wedged-reconnect-loop guard, and an always-on worker is |
| +// exactly the shape that gets wedged. It is spent as a RECYCLE there, not a |
| +// stop: stopping one leaves nothing to start it again. |
| +func TestAlwaysOnWorkerMaxRunRecyclesItsChild(t *testing.T) { |
| + a, b, clk, _ := testActivator(t, time.Hour) |
| + rt := alwaysOnWorkerRoute("bot.x.internal") |
| + rt.MaxRunSeconds = 30 |
| + if err := a.WakeWorker(rt); err != nil { |
| + t.Fatalf("WakeWorker: %v", err) |
| + } |
| + first := b.lastWorker() |
| + |
| + clk.advance(31 * time.Second) |
| + a.sweepOnce() |
| + waitForRestart(t, a, b, "bot.x.internal", 2) |
| + if !a.entryAwake("bot.x.internal") { |
| + t.Fatal("an always-on worker was STOPPED by its ceiling — nothing would ever start it again") |
| + } |
| + if !first.isStopped() { |
| + t.Fatal("the wedged child was left running beside its replacement") |
| + } |
| + if b.lastRep().isClosed() { |
| + t.Fatal("the lease was released by a max_run recycle — the entry keeps its lease and replication") |
| + } |
| + |
| + // And the ceiling re-arms on the new child rather than firing every sweep. |
| + a.sweepOnce() |
| + if n := b.workerStarts(); n != 2 { |
| + t.Fatalf("workerStarts = %d, want 2 — the fresh child's own max_run clock starts at its spawn", n) |
| } |
| } |
diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go| index 4338af469..b8e9878d5 100644 |
| --- a/internal/activator/backend_exec.go |
| +++ b/internal/activator/backend_exec.go |
| @@ -935,6 +935,21 @@ func (p *childProc) Exited() bool { |
| } |
| } |
| +// Crashed answers the activator's procCrasher seam: a DEFINITELY non-zero |
| +// exit status. Read from ProcessState rather than from p.err because a clean |
| +// exit whose pipes outlive it comes back as exec.ErrWaitDelay (cmd.WaitDelay |
| +// is set on every child here) — an error that says nothing about the exit |
| +// status, and reading it as a crash would restart a worker that finished. |
| +// Anything this cannot establish reads as NOT a crash, which is procCrashed's |
| +// own default and for its reasons. |
| +func (p *childProc) Crashed() bool { |
| + if !p.Exited() { |
| + return false |
| + } |
| + st := p.cmd.ProcessState // safe: written before done closed, read after |
| + return st != nil && !st.Success() |
| +} |
| + |
| func (p *childProc) Stop(ctx context.Context) error { |
| if p.purge != nil { |
| p.purge.Close() |