amadan
Pin that a failed due-fired wake consumes nothing
The consume was already gated on a successful wake, but nothing tested it: the re-review mutated it to run unconditionally and the whole activator suite still passed. That is the direction worth guarding — consuming on failure turns harmless wake/park churn into a sidecar whose only way back up has been erased by a wake that never happened, silently. The test drives a failing spawn through the due sweep, asserts the row keeps its fired value and that the next sweep retries, and then asserts the wake which finally succeeds is the one that consumes. Two comments corrected alongside it. The sweep's exited arm claimed max_run bounds a permanent crash loop: it does not — the arm continues unconditionally while the proc reads as exited, so the ceiling is unreachable, and a successful respawn resets both spawnedAt and lastActive, so the idle clock cannot fire either. A crash loop is PACED by the doubling backoff and ended only by an operator wake or the record going away; a crash-count ceiling is filed as a follow-up. And restartWorkerLocked still described itself as the always-on path alone, which it stopped being when the hibernating-crash arm started calling it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014WvcQVWggXkE5BAkjRDgWF
2 files changed,
+66
−8
internal/activator/activator.go+22 −8internal/activator/activator_test.go+44 −0
diff --git a/internal/activator/activator.go b/internal/activator/activator.go| index 63c18d9af..d5c3a9809 100644 |
| --- a/internal/activator/activator.go |
| +++ b/internal/activator/activator.go |
| @@ -1320,9 +1320,12 @@ func workerEnv(socket, host, token string) map[string]string { |
| } |
| } |
| -// restartWorkerLocked replaces an ALWAYS-ON worker's dead child — the caller |
| -// holds a.mu and has already established that e is an awake worker with |
| -// `hibernate: false` and an exited proc. |
| +// restartWorkerLocked replaces a worker's dead child — the caller holds a.mu |
| +// and has already established that e is an awake worker whose proc has |
| +// exited, and that the exit is one that must be restarted: ANY exit for an |
| +// always-on worker (`hibernate: false` — the record exists ⇒ the process |
| +// runs), or a non-zero exit for a hibernating one (a crash; a clean exit is |
| +// that bot finishing its work, and parks instead). |
| // |
| // Restart=always without a supervisor, the co-spawn path's rule applied to a |
| // standalone member: doubling backoff from sidecarRestartBase to |
| @@ -1334,7 +1337,10 @@ func workerEnv(socket, host, token string) map[string]string { |
| // |
| // In place, not through a stop and re-wake: the lease and the replication are |
| // still ours (only the child died), and design §7.2 says an always-on worker |
| -// runs with no lease churn. |
| +// runs with no lease churn. A hibernating worker restarted here keeps its |
| +// lease for the same reason — with the consequence, recorded at the sweep's |
| +// exited arm, that a crash LOOP holds that lease until an operator or the |
| +// record intervenes. |
| func (a *Service) restartWorkerLocked(e *actEntry, now time.Time) { |
| if !e.spawnedAt.IsZero() && now.Sub(e.spawnedAt) >= sidecarHealthyRun { |
| e.workerBackoff = 0 |
| @@ -1825,10 +1831,18 @@ func (a *Service) sweepOnce() { |
| // 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. |
| + // PACES itself — it is not terminated. Nothing below this arm ever |
| + // runs for a crashing child: this branch continues unconditionally |
| + // while the proc reads as exited, and a successful respawn resets |
| + // both spawnedAt and lastActive, so neither max_run nor the idle |
| + // clock can fire between crashes. A hibernating sidecar that |
| + // crash-loops therefore keeps its entry and its lease at a 2s→1m |
| + // cadence until an operator parks it (a wake stands a broken |
| + // worker down and recycles it) or its record goes away. A |
| + // crash-COUNT ceiling that parks after N failures is the right |
| + // bound and is filed as a follow-up: it is a design decision about |
| + // when the platform gives up on a member's bot, not a detail of |
| + // this arm. |
| if e.worker && (!e.route.Hibernate || procCrashed(e.proc)) { |
| a.restartWorkerLocked(e, now) |
| continue |
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go| index 37d079dfd..9bd3e8c33 100644 |
| --- a/internal/activator/activator_test.go |
| +++ b/internal/activator/activator_test.go |
| @@ -2630,6 +2630,50 @@ func TestWorkerSleepingBeforeItsDueTimeStillWakesForIt(t *testing.T) { |
| waitFor(t, "the scheduled wake", func() bool { return b.workerStarts() == 2 }) |
| } |
| +// A FAILED due-fired wake must consume NOTHING. The index is the retry — the |
| +// due sweep's own contract for a wake that could not complete — and consuming |
| +// on failure would convert harmless churn into the far worse bug: a sidecar |
| +// whose only way back up has been erased, silently, by a wake that never |
| +// happened. |
| +func TestFailedDueFiredWakeConsumesNothing(t *testing.T) { |
| + a, b, clk, reg := testActivator(t, 10*time.Second) |
| + rt := testWorkerRoute("bot.x.internal") |
| + if err := reg.Upsert(rt); err != nil { |
| + t.Fatalf("upsert: %v", err) |
| + } |
| + fired := clk.now().Unix() - 1 |
| + if err := reg.SetNextDue(rt.Host, fired); err != nil { |
| + t.Fatalf("set next due: %v", err) |
| + } |
| + b.mu.Lock() |
| + b.workerErr = errors.New("no build to exec") // the wake fails at the spawn |
| + b.mu.Unlock() |
| + |
| + a.sweepOnce() |
| + waitFor(t, "the failed wake to clear its entry", func() bool { return a.entryCount() == 0 }) |
| + got, err := reg.Get(rt.Host) |
| + if err != nil { |
| + t.Fatalf("get: %v", err) |
| + } |
| + if got.NextDue != fired { |
| + t.Fatalf("next_due = %d after a FAILED due wake, want %d — the schedule is the retry, and a sidecar whose next_due is erased by a wake that never happened has no way back up at all", got.NextDue, fired) |
| + } |
| + |
| + // The next sweep retries it, exactly as the due sweep's own comment says. |
| + a.sweepOnce() |
| + waitFor(t, "the retry", func() bool { return b.workerStarts() == 2 }) |
| + |
| + // …and once a wake actually succeeds, THAT one consumes the schedule. |
| + b.mu.Lock() |
| + b.workerErr = nil |
| + b.mu.Unlock() |
| + a.sweepOnce() |
| + waitFor(t, "the successful wake", func() bool { return a.entryAwake(rt.Host) }) |
| + if got, err := reg.Get(rt.Host); err != nil || got.NextDue != 0 { |
| + t.Fatalf("next_due = %+v (%v) after the wake that SUCCEEDED, want 0", got, err) |
| + } |
| +} |
| + |
| // The consumption is conditional on the row still holding the value that |
| // fired: the child spawned by this very wake may post its NEXT due time over |
| // the agent socket during the settle window, and an unconditional clear would |