From 560558dd128bd5cb1dadbfb89ce7b7548ba4203d Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Wed, 20 May 2026 10:41:55 +1000 Subject: [PATCH] fix(elbv2): reset target health to initial on daemon startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Target HealthState is persisted in JetStream KV but the healthChecker's per-target counters live only in process memory. After a daemon restart (host reboot), the counters are gone but the stored 'healthy' claim remains — DescribeTargetHealth returns 'healthy' immediately even though zero post-restart health observations have been recorded and the LB VM's lb-agent hasn't shipped a fresh report yet. Tests that gate on WaitForTargetsHealthy advance prematurely, then ALB traffic lands on a backend that isn't actually up (reboot E2E run 26133224853 saw 20/0 burst instead of 10/10). Add ELBv2ServiceImpl.ResetTargetHealthOnStartup: scans all TGs, rewrites every non-draining target's HealthState back to 'initial' with the matching HealthDesc. Draining is preserved — that state is driven by DeregisterTargets and must not be clobbered. Wired from daemon startCluster() right after elbv2 service init, before NATS subscriptions go live, so first DescribeTargetHealth sees the reset state. 'initial' is the correct AWS-semantics representation of 'we don't know yet' — the next lb-agent heartbeat drives state forward through the existing evaluateHealth() path (single healthy probe transitions initial→healthy, matching fast-start behaviour). Multi-node caveat documented on the method: a sibling daemon's in-memory counters would be out-of-sync with the persisted 'initial' state until its next heartbeat re-converges. Acceptable — converges within one health-check interval, and round-robin transiently excluding a backend is safer than serving traffic to a dead one. Closes mulga-siv-119. Paired with the e2e-side guard on feat/e2e-go-harness that waits for both backends to actually serve before the post-reboot burst. --- spinifex/daemon/daemon.go | 10 +++ spinifex/handlers/elbv2/service_impl.go | 71 +++++++++++++++++++ .../elbv2/service_impl_health_test.go | 47 ++++++++++++ 3 files changed, 128 insertions(+) diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 71a4249df..e1b32e5f5 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1190,6 +1190,16 @@ func (d *Daemon) startCluster() error { return "sys.micro" }) + // Invalidate persisted target HealthState before subscriptions go live. + // Stale "healthy" entries from a pre-restart cluster otherwise satisfy + // DescribeTargetHealth waiters before any actual post-restart health + // observation has been recorded. Best-effort: a failure here just leaves + // the old behavior in place rather than blocking daemon startup. + if err := d.elbv2Service.ResetTargetHealthOnStartup(context.Background()); err != nil { + slog.Warn("ELBv2: target-health reset failed; continuing with stale state", + "err", err) + } + // Ensure default VPC exists for system and admin accounts // (matches AWS: every account has a default VPC with IGW + default SG) if d.vpcService != nil { diff --git a/spinifex/handlers/elbv2/service_impl.go b/spinifex/handlers/elbv2/service_impl.go index a08ed1bcb..4e955d0c0 100644 --- a/spinifex/handlers/elbv2/service_impl.go +++ b/spinifex/handlers/elbv2/service_impl.go @@ -160,6 +160,77 @@ func (s *ELBv2ServiceImpl) Close() { } } +// ResetTargetHealthOnStartup invalidates persisted target HealthState across +// all target groups by resetting every non-draining target to "initial". Run +// once during daemon startup before the service begins serving requests. +// +// Rationale: target HealthState is persisted in JetStream KV, but the +// healthChecker's per-target counters live only in process memory. After a +// daemon restart (e.g. host reboot), the counters are gone but the stored +// "healthy" claim remains — DescribeTargetHealth returns "healthy" +// immediately even though the daemon has zero active health observations +// and the LB system VM's lb-agent hasn't posted a fresh report yet. Tests +// that gate on WaitForTargetsHealthy advance prematurely, then ALB traffic +// lands on a backend that isn't actually up. +// +// Resetting to "initial" is the correct AWS-semantics representation of +// "we don't know yet, give us a moment" — the next lb-agent heartbeat +// drives the state machine forward through evaluateHealth. +// +// Multi-node caveat: in a multi-daemon cluster, each daemon that restarts +// resets the SHARED KV state. A sibling daemon's in-memory counters would +// then be out-of-sync with the persisted "initial" state until the next +// heartbeat re-converges. Acceptable trade-off — heartbeats arrive within +// one health-check interval (default 5-30s) and round-robin transiently +// excluding a backend is safer than serving traffic to a dead one. +// +// Refs mulga-siv-119. +func (s *ELBv2ServiceImpl) ResetTargetHealthOnStartup(ctx context.Context) error { + if s == nil || s.store == nil { + return nil + } + tgs, err := s.store.ListTargetGroups() + if err != nil { + return fmt.Errorf("list target groups: %w", err) + } + resetTGs := 0 + resetTargets := 0 + for _, tg := range tgs { + if ctx.Err() != nil { + return ctx.Err() + } + changed := false + for i := range tg.Targets { + t := &tg.Targets[i] + // Skip targets actively being drained — that state is driven by + // DeregisterTargets and must not be clobbered by a daemon restart. + if t.HealthState == TargetHealthDraining { + continue + } + if t.HealthState == TargetHealthInitial { + continue + } + t.HealthState = TargetHealthInitial + t.HealthDesc = "Target registration is in progress" + changed = true + resetTargets++ + } + if changed { + if err := s.store.PutTargetGroup(tg); err != nil { + slog.Error("ResetTargetHealthOnStartup: persist failed", + "tgId", tg.TargetGroupID, "err", err) + continue + } + resetTGs++ + } + } + if resetTargets > 0 { + slog.Info("ELBv2: reset target health on startup", + "targetsReset", resetTargets, "targetGroupsTouched", resetTGs) + } + return nil +} + // SetSystemInstanceTypeFunc sets a function that resolves the smallest available // instance type. Called at request time so it adapts to node capacity. func (s *ELBv2ServiceImpl) SetSystemInstanceTypeFunc(fn func() string) { diff --git a/spinifex/handlers/elbv2/service_impl_health_test.go b/spinifex/handlers/elbv2/service_impl_health_test.go index 31a4a2f23..1ea659b14 100644 --- a/spinifex/handlers/elbv2/service_impl_health_test.go +++ b/spinifex/handlers/elbv2/service_impl_health_test.go @@ -160,3 +160,50 @@ func TestRegisterTargets_MultipleInstances(t *testing.T) { assert.Equal(t, "10.0.1.50", tg.Targets[0].PrivateIP) assert.Equal(t, "10.0.1.51", tg.Targets[1].PrivateIP) } + +// TestResetTargetHealthOnStartup_TransitionsAllNonDrainingToInitial verifies +// that the startup reset rewrites every non-draining target's HealthState +// back to "initial" so a daemon restart can't surface stale "healthy" claims +// before the lb-agent has posted a fresh report. Refs mulga-siv-119. +func TestResetTargetHealthOnStartup_TransitionsAllNonDrainingToInitial(t *testing.T) { + store := setupTestNATS(t) + svc := &ELBv2ServiceImpl{store: store} + + tg := &TargetGroupRecord{ + TargetGroupArn: "arn:aws:elasticloadbalancing:us-east-1:000:targetgroup/test/tg-reset", + TargetGroupID: "tg-reset", + Port: 80, + HealthCheck: DefaultHealthCheck(), + Targets: []Target{ + {Id: "i-h", HealthState: TargetHealthHealthy, HealthDesc: "Target is healthy", PrivateIP: "10.0.0.1"}, + {Id: "i-u", HealthState: TargetHealthUnhealthy, HealthDesc: "Health check failed", PrivateIP: "10.0.0.2"}, + {Id: "i-d", HealthState: TargetHealthDraining, HealthDesc: "Draining", PrivateIP: "10.0.0.3"}, + {Id: "i-i", HealthState: TargetHealthInitial, PrivateIP: "10.0.0.4"}, + }, + } + require.NoError(t, store.PutTargetGroup(tg)) + + require.NoError(t, svc.ResetTargetHealthOnStartup(t.Context())) + + got, err := store.GetTargetGroup("tg-reset") + require.NoError(t, err) + require.NotNil(t, got) + require.Len(t, got.Targets, 4) + + byID := map[string]Target{} + for _, t := range got.Targets { + byID[t.Id] = t + } + assert.Equal(t, TargetHealthInitial, byID["i-h"].HealthState, "previously healthy must reset") + assert.Equal(t, TargetHealthInitial, byID["i-u"].HealthState, "previously unhealthy must reset") + assert.Equal(t, TargetHealthDraining, byID["i-d"].HealthState, "draining must be preserved") + assert.Equal(t, TargetHealthInitial, byID["i-i"].HealthState, "initial stays initial") +} + +func TestResetTargetHealthOnStartup_NilSafe(t *testing.T) { + var svc *ELBv2ServiceImpl + assert.NoError(t, svc.ResetTargetHealthOnStartup(t.Context())) + + svc2 := &ELBv2ServiceImpl{} + assert.NoError(t, svc2.ResetTargetHealthOnStartup(t.Context())) +}