amadan
zdd-4 second fix wave: a recycle refused for a changed copy stays refused (N1, R17 revert of R12)
Reverts 74a43e4b's behaviour. When the backend refuses a recycle onto the captured build with release.ErrStagedCopyChanged, the failed-spawn arm reports it with the existing log line and event and keeps the captured build. The worker stays down, and the next crash restart asks for the same captured identity again, refused again while the bytes differ (spec §4.2). No fallback to the live link on changed bytes. The R12 test now asserts the refusal stands: the captured build is kept, the worker is not restarted, and the next attempt asks for the captured v1 (never nil). The "other error keeps the captured build" test is kept.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+31
−28
internal/activator/activator.go+9 −15internal/activator/worker_identity_test.go+22 −13
diff --git a/internal/activator/activator.go b/internal/activator/activator.go| index de9eea175..f2b571e3f 100644 |
| --- a/internal/activator/activator.go |
| +++ b/internal/activator/activator.go |
| @@ -1852,8 +1852,9 @@ const workerSpawnTimeout = 2 * time.Minute |
| // (recycleWorkerLocked), or nil for a converge, which execs what live/<host> |
| // names now. A recycle onto a staged copy whose bytes have changed is refused |
| // by the backend (release.ErrStagedCopyChanged) and lands in the failed-spawn |
| -// arm below, which drops the captured build so the crash restart's next |
| -// attempt, with its backoff, execs what live/<host> names. |
| +// arm below, which keeps the captured build: the worker stays down, and each |
| +// crash restart asks for that build again and is refused again while its |
| +// bytes differ (spec §4.2, ruling R17). |
| // |
| // Everything expensive happens OFF a.mu, on its own goroutine. That is not a |
| // stylistic preference: ExecBackend.StartWorker can materialize a build from |
| @@ -1963,22 +1964,15 @@ func (a *Service) respawnWorkerLocked(e *actEntry, stopOld Proc, why string, ont |
| a.mu.Unlock() |
| log.Printf("activator: worker %s NOT respawned (%s): %v — standing down and releasing its lease", host, why, refused) |
| case err != nil: |
| - // A recycle refused because the captured copy changed or vanished |
| - // (ruling R12) drops the captured build: retrying those bytes can |
| - // never succeed, so the crash restart's next attempt takes |
| - // recycleWorkerLocked's no-captured-build path and execs what |
| - // live/<host> names, as a failed wake's next wake does. Any other |
| - // failure may be transient and keeps the captured build. |
| - dropped := onto != nil && errors.Is(err, release.ErrStagedCopyChanged) |
| - if dropped { |
| - e.workerServing = release.StagedIdentity{} |
| - } |
| + // The captured build is kept whatever the failure, a refused |
| + // recycle (release.ErrStagedCopyChanged) included: spec §4.2 |
| + // refuses a recovery onto changed bytes, and that refusal must |
| + // outlast the retry (ruling R17), so the next crash restart asks |
| + // for the same captured build again. Only a converge, admitted |
| + // on its own terms, moves the worker onto another build. |
| a.mu.Unlock() |
| log.Printf("activator: WORKER %s failed to respawn (%s): %v — the sweep's crash restart takes it from here", host, why, err) |
| emitEvent(a.Logs, rt, "error", "sidecar could not be restarted — retrying") |
| - if dropped { |
| - log.Printf("activator: worker %s dropped its captured build %s because the staged copy no longer matches the digest it was exec'd with; its next restart execs what live/%s names", host, onto.Version, host) |
| - } |
| default: |
| e.proc = proc |
| e.workerToken = tok |
diff --git a/internal/activator/worker_identity_test.go b/internal/activator/worker_identity_test.go| index 0eceabf61..d1a7f6296 100644 |
| --- a/internal/activator/worker_identity_test.go |
| +++ b/internal/activator/worker_identity_test.go |
| @@ -226,40 +226,49 @@ func waitForSpawnCalls(t *testing.T, a *Service, b *stagedFake, host string, n i |
| }) |
| } |
| -// Ruling R12: a recycle the backend refuses because the captured copy |
| -// changed drops the captured build, so the crash restart's next attempt |
| -// execs what live/<host> names, as a failed wake's next wake would. |
| -func TestWorkerRecycleRefusedForAChangedCopyDropsTheCapturedBuild(t *testing.T) { |
| +// Ruling R17 (spec §4.2): a recycle the backend refuses because the captured |
| +// copy changed stays refused. The worker stays down, it keeps its captured |
| +// build, and the next crash restart asks for that same build again, never |
| +// for what live/<host> names. Here live/<host> still names v1, so there is |
| +// no drift that could admit a converge either. |
| +func TestWorkerRecycleRefusedForAChangedCopyStaysRefused(t *testing.T) { |
| a, b, clk, _, _ := stagedActivator(t) |
| host := "bot.x.internal" |
| if err := a.WakeWorker(alwaysOnWorkerRoute(host)); err != nil { |
| t.Fatalf("WakeWorker: %v", err) |
| } |
| - b.failNextStagedSpawn(fmt.Errorf("worker %s: %w", host, release.ErrStagedCopyChanged)) |
| + changed := fmt.Errorf("worker %s: %w", host, release.ErrStagedCopyChanged) |
| + b.failNextStagedSpawn(changed) |
| b.lastWorker().kill() |
| a.sweepOnce() |
| waitForSpawnCalls(t, a, b, host, 2) |
| if calls := b.calls(); calls[1] == nil || *calls[1] != fakeIdentity("v1") { |
| t.Fatalf("crash recycle asked for %v; want the captured v1 build", calls) |
| } |
| - if got := a.entryServing(host); got != (release.StagedIdentity{}) { |
| - t.Fatalf("serving after a refused recycle = %+v; want the captured build dropped", got) |
| + if got := a.entryServing(host); got != fakeIdentity("v1") { |
| + t.Fatalf("serving after a refused recycle = %+v; want the captured v1 kept", got) |
| + } |
| + if b.workerStarts() != 1 || !b.lastWorker().Exited() { |
| + t.Fatalf("workerStarts = %d; a refused recycle must leave the worker down", b.workerStarts()) |
| } |
| + // The bytes still differ: the next attempt asks for the captured build |
| + // again and is refused again. |
| + b.failNextStagedSpawn(changed) |
| clk.advance(sidecarRestartMax + time.Second) |
| a.sweepOnce() |
| waitForSpawnCalls(t, a, b, host, 3) |
| - if calls := b.calls(); calls[2] != nil { |
| - t.Fatalf("restart after the refusal asked for %v; want the live link (nil)", calls[2]) |
| + if calls := b.calls(); calls[2] == nil || *calls[2] != fakeIdentity("v1") { |
| + t.Fatalf("restart after the refusal asked for %v; want the captured v1 again, never the live link", calls[2]) |
| } |
| - if b.workerStarts() != 2 || b.lastWorker().Exited() { |
| - t.Fatalf("workerStarts = %d; want the worker running again from the live link", b.workerStarts()) |
| + if b.workerStarts() != 1 || !b.lastWorker().Exited() { |
| + t.Fatalf("workerStarts = %d; the refusal must stand while the bytes differ", b.workerStarts()) |
| } |
| if got := a.entryServing(host); got != fakeIdentity("v1") { |
| - t.Fatalf("serving after the respawn = %+v, want what live/<host> names", got) |
| + t.Fatalf("serving after a second refusal = %+v; want the captured v1 kept", got) |
| } |
| } |
| -// Ruling R12: any other spawn failure keeps the captured build. |
| +// Any other spawn failure keeps the captured build too. |
| func TestWorkerRecycleFailingForAnotherReasonKeepsTheCapturedBuild(t *testing.T) { |
| a, b, clk, _, _ := stagedActivator(t) |
| host := "bot.x.internal" |