fix: judge transitional-state heartbeats by the persist floor (#752)

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.

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

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) {