From 92dae2e4499e374841eb7a598b3886366b90e5e1 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:18:11 +1000 Subject: [PATCH] fix: judge transitional-state heartbeats by the persist floor (#752) --- spinifex/handlers/rds/lifecycle_test.go | 15 +++++++++ spinifex/handlers/rds/reconciler.go | 42 ++++++------------------ spinifex/handlers/rds/reconciler_test.go | 24 +++++++++++++- 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/spinifex/handlers/rds/lifecycle_test.go b/spinifex/handlers/rds/lifecycle_test.go index 70ffa47a4..2b2f987c5 100644 --- a/spinifex/handlers/rds/lifecycle_test.go +++ b/spinifex/handlers/rds/lifecycle_test.go @@ -637,6 +637,21 @@ func TestReconciler_CompletesARestartOnAHeartbeatFromTheRestartedEngine(t *testi } } +// The beat that ends a restart usually reaches the leader through KV, where it +// is at most a persist floor behind the engine. Judging it by the raw stale +// window left a database that came back cleanly rebooting until it timed out. +func TestReconciler_CompletesARestartOnAPersistedHeartbeatInsideTheFloor(t *testing.T) { + h := newLifecycleHarness(t, false) + // Older than the stale window, younger than the window plus the floor, and + // still after the transition it proves finished. + persisted := time.Now().UTC().Add(-HeartbeatStaleAfter - time.Minute) + seedInstance(t, h.svc, restartingRecord(StatusRebooting, persisted.Add(-time.Second), persisted)) + + require.NoError(t, NewReconciler(h.svc, "node-a").reconcileOnce(t.Context())) + + assert.Equal(t, StatusAvailable, h.record(t).Status) +} + // The VM keeps its instance ID across a restart, so a beat sent before the // transition began would otherwise report the reboot as finished the instant it // started. diff --git a/spinifex/handlers/rds/reconciler.go b/spinifex/handlers/rds/reconciler.go index d78ea9c40..0187c230c 100644 --- a/spinifex/handlers/rds/reconciler.go +++ b/spinifex/handlers/rds/reconciler.go @@ -654,43 +654,21 @@ func transitionStarted(rec *DBInstanceRecord) time.Time { return rec.UpdatedAt } -// Both halves must hold: a healthy heartbeat from the record's *current* VM, -// and that VM actually running. A stale beat from a superseded VM would -// otherwise report a replaced instance as ready. Beats at or before since are -// ignored, which is how a restart is told from the engine it restarted. +// Both halves must hold: a fresh healthy heartbeat from the record's *current* +// VM, and that VM actually running. A stale beat from a superseded VM would +// otherwise report a replaced instance as ready. The observation is the same one +// the health classifier forms, so freshness is judged by one rule package-wide. func (r *Reconciler) engineReady(ctx context.Context, accountID string, rec *DBInstanceRecord, since time.Time) (bool, error) { - if rec.Agent.EngineHealth != EngineHealthHealthy || rec.InstanceID == "" || - rec.Agent.InstanceID != rec.InstanceID { + obs := r.observeAgent(accountID, rec) + if !obs.engineHealthy || !obs.heartbeatFresh { return false, nil } - if !r.heartbeatFresh(accountID, rec, since) { + // A beat at or before since came from the engine the restart replaced, so it + // says nothing about the one that took its place. + if !since.IsZero() && !obs.lastSeen.After(since) { return false, nil } - if r.svc.deps.InstanceState == nil { - return true, nil - } - state, err := r.svc.deps.InstanceState.InstanceState(ctx, rec.InstanceID, accountID) - if err != nil { - return false, fmt.Errorf("resolve VM state for %s: %w", rec.InstanceID, err) - } - return state == instanceStateRunning, nil -} - -// The in-memory beat is fresher than the persisted one but only this node sees -// it, so a leader that has seen no beat falls back to the record — which trails -// the truth by at most the persist floor. -func (r *Reconciler) heartbeatFresh(accountID string, rec *DBInstanceRecord, since time.Time) bool { - lastSeen, ok := r.svc.LastSeen(accountID, rec.DBInstanceIdentifier) - if !ok { - if rec.Agent.LastSeen == nil { - return false - } - lastSeen = *rec.Agent.LastSeen - } - if !since.IsZero() && !lastSeen.After(since) { - return false - } - return time.Since(lastSeen) <= HeartbeatStaleAfter + return r.vmRunning(ctx, accountID, rec) } // A CAS write, so a transition raced by an agent report or a lifecycle op is diff --git a/spinifex/handlers/rds/reconciler_test.go b/spinifex/handlers/rds/reconciler_test.go index 8ae1fe44c..c3522cf68 100644 --- a/spinifex/handlers/rds/reconciler_test.go +++ b/spinifex/handlers/rds/reconciler_test.go @@ -140,8 +140,10 @@ func TestReconciler_HoldsCreatingUntilTheEngineIsHealthy(t *testing.T) { // A beat from a VM other than the record's current one is a superseded VM // still running after a replace; it must not report the instance ready. {"BeatFromASupersededVM", func(rec *DBInstanceRecord) { rec.Agent.InstanceID = "i-oldvm" }}, + // Silent for longer than even the slack a persisted beat earns, so no + // source could have seen this agent recently. {"StaleHeartbeat", func(rec *DBInstanceRecord) { - stale := time.Now().UTC().Add(-2 * HeartbeatStaleAfter) + stale := time.Now().UTC().Add(-2 * (HeartbeatStaleAfter + HeartbeatPersistFloor)) rec.Agent.LastSeen = &stale }}, } @@ -161,6 +163,26 @@ func TestReconciler_HoldsCreatingUntilTheEngineIsHealthy(t *testing.T) { } } +// The transitional states judge freshness by the same rule as the health +// classifier. A leader handling none of an instance's beats sees them only +// through KV, which a healthy agent refreshes no more often than the persist +// floor; judging that beat by the raw stale window failed a live database at the +// transition timeout. +func TestReconciler_CompletesACreateOnAPersistedHeartbeatInsideTheFloor(t *testing.T) { + h := newReconcileHarness(t) + rec := healthyRecord() + // Older than the stale window, younger than the window plus the floor: what a + // perfectly healthy instance looks like to a leader that has seen no beat. + persisted := time.Now().UTC().Add(-HeartbeatStaleAfter - time.Minute) + rec.Agent.LastSeen = &persisted + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + status, _ := h.statusOf(t, testDBID) + assert.Equal(t, StatusAvailable, status) +} + // A create that never comes up has to end somewhere: the customer sees a broken // instance either way, and failed is the state they can act on. func TestReconciler_MarksFailedWhenTheBootstrapTimesOut(t *testing.T) {