carlos / platform Public

zdd-5 final review w2: the outcome fence fails closed (N1)

A wake still waiting on an earlier planned wake's undelivered outcome
when outcomeFenceWait (or its own context) ends is now refused before
the Planner and the lease (errOutcomeUndelivered): nothing is planned or
launched, and the fence stays until the outcome's OnGen returns. The
member sees the maintenance fence's existing line, as for errWakeUnproven.
TestTheOutcomeFenceIsBounded, which expected planning after the bound,
is replaced by TestTheOutcomeFenceFailsClosed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 2 weeks ago 6d0a154044993ef47d6666de6be9e0aed009fa4b parent fe65e9b

5 files changed, +92 −28

internal/activator/activator.go +6 −2
internal/activator/claimwake.go +5 −2
internal/activator/logsink.go +5 −2
internal/activator/outcome_fence_test.go +51 −13
internal/activator/wakeplan.go +25 −9

diff --git a/internal/activator/activator.go b/internal/activator/activator.go

index 1eb57b6f4..9dc67c9d6 100644

--- a/internal/activator/activator.go

+++ b/internal/activator/activator.go

@@ -1700,8 +1700,12 @@ func (a *Service) doWake(ctx context.Context, e *actEntry) (*wokenInstance, erro

if a.Planner != nil && e.pin == nil {

// An earlier planned wake's outcome still on its way to the

// scheduler is waited for, bounded (final review F3): planning

- // before its failure record exists would pick the failed build.

- a.awaitOutcomes(ctx, host)

+ // before its failure record exists would pick the failed build. A

+ // wait that ends first refuses the wake before the planner and the

+ // lease (final review wave 2, N1): nothing is planned or launched.

+ if err := a.awaitOutcomes(ctx, host); err != nil {

+ return w, fmt.Errorf("%s: refusing to wake: %w", host, err)

+ }

plan = a.Planner(ctx, rt)

if plan.Refuse != "" {

return w, fmt.Errorf("%s: refusing to wake: %s", host, plan.Refuse)

diff --git a/internal/activator/claimwake.go b/internal/activator/claimwake.go

index a6fe26055..f221ad47d 100644

--- a/internal/activator/claimwake.go

+++ b/internal/activator/claimwake.go

@@ -191,8 +191,11 @@ func (a *Service) doClaimWake(ctx context.Context, e *actEntry) (*wokenInstance,

// before its exec hook (RoleLaunch.Start), so none of it runs a build.

if a.Planner != nil && e.pin == nil {

// As doWake: an earlier planned wake's outcome is delivered before

- // this one plans (final review F3), bounded.

- a.awaitOutcomes(ctx, rt.Host)

+ // this one plans (final review F3), and a wait that ends first

+ // refuses the wake (final review wave 2, N1).

+ if err := a.awaitOutcomes(ctx, rt.Host); err != nil {

+ return nil, fmt.Errorf("%s: refusing to wake: %w", rt.Host, err)

+ }

plan := a.Planner(ctx, rt)

if plan.Refuse != "" {

return nil, fmt.Errorf("%s: refusing to wake: %s", rt.Host, plan.Refuse)

diff --git a/internal/activator/logsink.go b/internal/activator/logsink.go

index 69aeafce4..9caf80813 100644

--- a/internal/activator/logsink.go

+++ b/internal/activator/logsink.go

@@ -57,7 +57,7 @@ func memberSafeWakeReason(err error) string {

// operator's (persistGenerations logs it).

err = ErrFenced

}

- if errors.Is(err, errWakeUnproven) || genStartUnproven(err) {

+ if errors.Is(err, errWakeUnproven) || genStartUnproven(err) || errors.Is(err, errOutcomeUndelivered) {

// A wake refused while the host's last failed wake is not proven

// stopped (zero-downtime spec §7.4), or that failed wake itself: a

// start whose generation kill was not proven (errGenUnproven,

@@ -65,7 +65,10 @@ func memberSafeWakeReason(err error) string {

// generations on or off. Members get the maintenance fence's

// existing text, the state that is like it: the platform holds the

// host closed until its own work is done, and it reopens by itself.

- // The generation detail stays in the operator log (wake).

+ // The generation detail stays in the operator log (wake). The same

+ // for a wake refused while an earlier planned wake's outcome is

+ // still being delivered (errOutcomeUndelivered, final review wave 2

+ // N1).

err = ErrFenced

}

s := err.Error()

diff --git a/internal/activator/outcome_fence_test.go b/internal/activator/outcome_fence_test.go

index fb92e5651..13105bc32 100644

--- a/internal/activator/outcome_fence_test.go

+++ b/internal/activator/outcome_fence_test.go

@@ -119,37 +119,75 @@ func TestANewWakeWaitsForAFailedFirstWakesOutcome(t *testing.T) {

}

-// The fence is bounded (outcomeFenceWait): a wake behind an OnGen that never

-// returns proceeds as it did before the fence, so a wedged scheduler cannot

-// keep a host down. The planner's exec-time check still reads whatever the

-// record says by then.

-func TestTheOutcomeFenceIsBounded(t *testing.T) {

+// Final review wave 2 (N1): the fence fails closed. The wait is bounded

+// (outcomeFenceWait), the restriction is not. A wake still behind an

+// undelivered outcome when the bound passes is refused before it plans:

+// no Planner call, nothing launched, and the member reads the maintenance

+// fence's existing line. The fence stays, so the next request is refused

+// the same way. Once the outcome is delivered a wake proceeds to plan

+// (here the planner, with the record in place, refuses the held build).

+// Before the fix the wake planned after the bound and could boot B again

+// without its record.

+func TestTheOutcomeFenceFailsClosed(t *testing.T) {

old := outcomeFenceWait

outcomeFenceWait = 200 * time.Millisecond

t.Cleanup(func() { outcomeFenceWait = old })

dir := t.TempDir()

next := stagedID(t, dir, "v2")

- a, _, _ := planActivator(t, WakePlan{})

+ a, b, _ := planActivator(t, WakePlan{})

const host = "one.example.com"

a.genHealth = func(context.Context, Gen) error { return errors.New("GET / answered 500") }

o := newBlockedOnGen(GenFailed)

- t.Cleanup(func() { close(o.release) })

+ var releaseOnce sync.Once

+ release := func() { releaseOnce.Do(func() { close(o.release) }) }

+ t.Cleanup(release)

a.OnGen = o.on

+ sink := &lockedSink{}

+ a.Logs = sink

saw := make(chan bool, 4)

a.Planner = secondPlanner(o, WakePlan{Boot: next, Mode: ModeHold, FirstWake: true}, saw)

if _, err := a.beginRequest(context.Background(), testRoute(host)); err == nil {

t.Fatal("the failed hold first wake served")

}

o.waitEntered(t)

- start := time.Now()

- go func() { _, _ = a.beginRequest(context.Background(), testRoute(host)) }()

- select {

- case <-saw:

+ for i := 1; i <= 2; i++ {

+ start := time.Now()

+ _, err := a.beginRequest(context.Background(), testRoute(host))

+ if !errors.Is(err, errOutcomeUndelivered) {

+ t.Fatalf("wake %d behind the undelivered outcome = %v, want refused (errOutcomeUndelivered)", i, err)

+ }

if took := time.Since(start); took < 150*time.Millisecond {

- t.Fatalf("planned after %v: it did not wait for the fence", took)

+ t.Fatalf("wake %d refused after %v: it did not wait for the fence", i, took)

+ }

+ select {

+ case rec := <-saw:

+ t.Fatalf("wake %d planned behind the undelivered outcome (record in place: %v)", i, rec)

+ default:

+ }

+ if gens := b.started(); len(gens) != 1 {

+ t.Fatalf("spawned %+v: a wake behind the undelivered outcome launched something", gens)

+ }

+ want := memberSafeWakeReason(ErrFenced)

+ got := sink.errors()

+ if len(got) != 3 || got[1] != want || got[2] != want {

+ t.Fatalf("member error events %q: want the failed wake's, then the maintenance fence's line (%q) for each refusal", got, want)

+ }

+ release()

+ _, err := a.beginRequest(context.Background(), testRoute(host))

+ select {

+ case rec := <-saw:

+ if !rec {

+ t.Fatal("the wake after the delivery planned before the failure was recorded")

}

case <-time.After(5 * time.Second):

- t.Fatal("the second wake never planned: the fence is not bounded")

+ t.Fatal("the wake after the delivery never planned: the fence outlived the outcome")

+ }

+ if err == nil || !strings.Contains(err.Error(), "held down") {

+ t.Fatalf("wake after the delivery = %v, want refused by the recorded hold-down", err)

+ }

+ if gens := b.started(); len(gens) != 1 {

+ t.Fatalf("spawned %+v: B ran again", gens)

}

diff --git a/internal/activator/wakeplan.go b/internal/activator/wakeplan.go

index 56e9198c1..227cb5480 100644

--- a/internal/activator/wakeplan.go

+++ b/internal/activator/wakeplan.go

@@ -220,9 +220,9 @@ var (

// work is in memory plus one local failure-book write, so it normally

// returns in milliseconds; 5 s covers a slow disk with room to spare and

// still leaves the waiting requests most of their wakeWaitBound (30 s).

- // Past it the wake proceeds as it did before the fence, so a wedged

- // scheduler cannot keep a host down, and the plan's exec-time check

- // still reads the record if it has landed by then.

+ // It bounds the waiting request, never the restriction (final review

+ // wave 2, N1): past it the wake is refused before it plans, and the

+ // fence stays until the outcome is delivered. A later request retries.

outcomeFenceWait = 5 * time.Second

)

@@ -556,28 +556,44 @@ func (a *Service) holdOutcomeLocked(host string) func() {

}

+// errOutcomeUndelivered refuses a wake, before it plans, while an earlier

+// planned wake's outcome is still being delivered past outcomeFenceWait

+// (final review wave 2, N1). The fence fails closed: planning without the

+// outcome could boot a build whose failure (a hold's included) is not yet

+// recorded. Members get the maintenance fence's existing text

+// (memberSafeWakeReason): the platform holds the host closed until its own

+// work is done, and it reopens by itself.

+var errOutcomeUndelivered = errors.New("an earlier planned wake's outcome is still being delivered")

// awaitOutcomes is a new wake's wait, before it plans, for host's

// planned-wake outcomes still being delivered (final review F3; spec §8.1,

// O7): a failed first wake's entry goes before afterWake's OnGen records

// the failure, and a first wake's observer reaches its verdict before

// OnGen hears it. Planning in that gap would pick the failed build again,

-// with no record yet to hold it. Bounded by outcomeFenceWait (and the

-// wake's own context): past it the wake plans as it did before the fence.

-// Called off a.mu.

-func (a *Service) awaitOutcomes(ctx context.Context, host string) {

+// with no record yet to hold it. The wait is bounded by outcomeFenceWait

+// and the wake's own context; the fence is not (final review wave 2, N1).

+// A wait that ends before the delivery refuses the wake: an error means

+// the caller must not plan or launch anything. The fence stays until the

+// outcome's OnGen returns, so the next request waits again. Called off

+// a.mu.

+func (a *Service) awaitOutcomes(ctx context.Context, host string) error {

a.mu.Lock()

f := a.outcomeFences[host]

a.mu.Unlock()

if f == nil {

- return

+ return nil

}

bound := time.NewTimer(outcomeFenceWait)

defer bound.Stop()

select {

case <-f.done:

+ return nil

case <-ctx.Done():

+ // Refused the same way: the outcome is still undelivered.

+ return fmt.Errorf("%w: %w", errOutcomeUndelivered, ctx.Err())

case <-bound.C:

- log.Printf("activator: %s: an earlier planned wake's outcome is still being delivered after %v; planning without waiting for it", host, outcomeFenceWait)

+ log.Printf("activator: %s: an earlier planned wake's outcome is still being delivered after %v; refusing this wake until it is", host, outcomeFenceWait)

+ return errOutcomeUndelivered

}