Fix #1168: wait_primary -> draining/demote_timeout assignment had no KeeperFSM edge

src/monitor/group_state_machine.c

@@ -698,23 +698,47 @@ ProceedGroupStateFromContext(GroupStateContext *ctx)
 	{
 		char message[BUFSIZE];
 
-		LogAndNotifyMessage(
-			message, BUFSIZE,
-			"Setting goal state of " NODE_FORMAT
-			" to draining and " NODE_FORMAT
-			" to prepare_promotion "
-			"after " NODE_FORMAT
-			" became unhealthy.",
-			NODE_FORMAT_ARGS(primaryNode),
-			NODE_FORMAT_ARGS(activeNode),
-			NODE_FORMAT_ARGS(primaryNode));
+		/*
+		 * A primary already converged to wait_primary has no "draining" to
+		 * go through: it never had a synchronous standby to begin with, so
+		 * there's nothing live to gracefully drain, and KeeperFSM[] has no
+		 * wait_primary -> draining edge anyway (issue #1168). Leave its
+		 * goal untouched here; the prepare_promotion/stop_replication rules
+		 * below apply the same drainTimeoutMs safety margin via report
+		 * staleness instead of a goal-state timestamp, then commit the one
+		 * real, reachable wait_primary -> demoted transition once it has
+		 * genuinely expired.
+		 */
+		if (IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY))
+		{
+			LogAndNotifyMessage(
+				message, BUFSIZE,
+				"Setting goal state of " NODE_FORMAT
+				" to prepare_promotion after " NODE_FORMAT
+				" (at wait_primary) became unhealthy.",
+				NODE_FORMAT_ARGS(activeNode),
+				NODE_FORMAT_ARGS(primaryNode));
+		}
+		else
+		{
+			LogAndNotifyMessage(
+				message, BUFSIZE,
+				"Setting goal state of " NODE_FORMAT
+				" to draining and " NODE_FORMAT
+				" to prepare_promotion "
+				"after " NODE_FORMAT
+				" became unhealthy.",
+				NODE_FORMAT_ARGS(primaryNode),
+				NODE_FORMAT_ARGS(activeNode),
+				NODE_FORMAT_ARGS(primaryNode));
+
+			/* shut down the primary */
+			AssignGoalState(primaryNode, REPLICATION_STATE_DRAINING, message);
+		}
 
 		/* keep reading until no more records are available */
 		AssignGoalState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION, message);
 
-		/* shut down the primary */
-		AssignGoalState(primaryNode, REPLICATION_STATE_DRAINING, message);
-
 		return true;
 	}
 
@@ -854,22 +878,40 @@ ProceedGroupStateFromContext(GroupStateContext *ctx)
 	{
 		char message[BUFSIZE];
 
-		LogAndNotifyMessage(
-			message, BUFSIZE,
-			"Setting goal state of " NODE_FORMAT
-			" to demote_timeout and " NODE_FORMAT
-			" to stop_replication after " NODE_FORMAT
-			" converged to prepare_promotion.",
-			NODE_FORMAT_ARGS(primaryNode),
-			NODE_FORMAT_ARGS(activeNode),
-			NODE_FORMAT_ARGS(activeNode));
+		/*
+		 * wait_primary has no reachable demote_timeout edge either (issue
+		 * #1168); leave its goal alone here and let the completion rule
+		 * below apply the drainTimeoutMs safety margin via report
+		 * staleness instead.
+		 */
+		if (IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY))
+		{
+			LogAndNotifyMessage(
+				message, BUFSIZE,
+				"Setting goal state of " NODE_FORMAT
+				" to stop_replication after it converged to "
+				"prepare_promotion.",
+				NODE_FORMAT_ARGS(activeNode));
+		}
+		else
+		{
+			LogAndNotifyMessage(
+				message, BUFSIZE,
+				"Setting goal state of " NODE_FORMAT
+				" to demote_timeout and " NODE_FORMAT
+				" to stop_replication after " NODE_FORMAT
+				" converged to prepare_promotion.",
+				NODE_FORMAT_ARGS(primaryNode),
+				NODE_FORMAT_ARGS(activeNode),
+				NODE_FORMAT_ARGS(activeNode));
+
+			/* wait for possibly-alive primary to kill itself */
+			AssignGoalState(primaryNode, REPLICATION_STATE_DEMOTE_TIMEOUT, message);
+		}
 
 		/* perform promotion to stop replication */
 		AssignGoalState(activeNode, REPLICATION_STATE_STOP_REPLICATION, message);
 
-		/* wait for possibly-alive primary to kill itself */
-		AssignGoalState(primaryNode, REPLICATION_STATE_DEMOTE_TIMEOUT, message);
-
 		return true;
 	}
 
@@ -926,18 +968,24 @@ ProceedGroupStateFromContext(GroupStateContext *ctx)
 	/*
 	 * when drain time expires or primary reports it's drained:
 	 *  draining -> demoted
+	 *
+	 * NodeIsWaitPrimaryPresumedDead covers the wait_primary equivalent
+	 * (issue #1168): the same drainTimeoutMs safety margin, applied via
+	 * report staleness instead of a demote_timeout goal-state timestamp
+	 * since that state is never reachable from wait_primary.
 	 */
 	if (IsCurrentState(activeNode, REPLICATION_STATE_STOP_REPLICATION) &&
 		(IsCurrentState(primaryNode, REPLICATION_STATE_DEMOTE_TIMEOUT) ||
-		 NodeIsDrainTimeExpired(primaryNode, ctx)))
+		 NodeIsDrainTimeExpired(primaryNode, ctx) ||
+		 NodeIsWaitPrimaryPresumedDead(primaryNode, activeNode, ctx)))
 	{
 		char message[BUFSIZE];
 
 		LogAndNotifyMessage(
 			message, BUFSIZE,
 			"Setting goal state of " NODE_FORMAT
 			" to wait_primary and " NODE_FORMAT
-			" to demoted after the demote timeout expired.",
+			" to demoted after the primary was presumed dead.",
 			NODE_FORMAT_ARGS(activeNode),
 			NODE_FORMAT_ARGS(primaryNode));
 

src/monitor/node_metadata.c

@@ -2520,3 +2520,46 @@ NodeIsDrainTimeExpired(const AutoFailoverNode *node,
 	return TimestampDifferenceExceeds(node->stateChangeTime, ctx->now,
 									  ctx->drainTimeoutMs);
 }
+
+
+/*
+ * NodeIsWaitPrimaryPresumedDead is the wait_primary equivalent of
+ * NodeIsDrainTimeExpired (issue #1168): a primary already converged to
+ * wait_primary never had a synchronous standby to begin with, so there is
+ * nothing "live" to gracefully drain, and KeeperFSM[] has no
+ * wait_primary -> draining or wait_primary -> demote_timeout edge either.
+ *
+ * We still apply the exact same safety margin (drainTimeoutMs, not the
+ * shorter unhealthyTimeoutMs that triggers a failover attempt in the first
+ * place) before presuming it dead. NodeIsDrainTimeExpired anchors on
+ * primaryNode's own stateChangeTime, which only moves when the monitor
+ * (re)assigns its goal -- a timestamp the primary itself can't refresh
+ * just by resuming contact. We can't reuse that same trick on primaryNode
+ * here, because this path deliberately never reassigns its goal (there is
+ * nothing reachable to reassign it to); if we anchored on primaryNode's
+ * own reportTime instead, a primary that reconnects and resumes reporting
+ * wait_primary (without ever converging on the failover) would keep
+ * resetting the clock forever, even though it never actually completes
+ * the promotion -- a permanent stall, not a bounded wait.
+ *
+ * So we anchor on activeNode's stateChangeTime instead: it is only
+ * touched by the monitor (re)assigning activeNode's own goal, which stops
+ * once activeNode converges to stop_replication (no further rule targets
+ * it while it stays there), giving us the same "fixed once committed,
+ * immune to the other node's unrelated activity" property that
+ * NodeIsDrainTimeExpired gets from primaryNode's stateChangeTime.
+ */
+bool
+NodeIsWaitPrimaryPresumedDead(const AutoFailoverNode *primaryNode,
+							  const AutoFailoverNode *activeNode,
+							  const struct GroupStateContext *ctx)
+{
+	if (primaryNode == NULL || activeNode == NULL ||
+		primaryNode->goalState != REPLICATION_STATE_WAIT_PRIMARY)
+	{
+		return false;
+	}
+
+	return TimestampDifferenceExceeds(activeNode->stateChangeTime, ctx->now,
+									  ctx->drainTimeoutMs);
+}

src/monitor/node_metadata.h

@@ -286,3 +286,6 @@ extern bool NodeIsReporting(const AutoFailoverNode *node,
 							const struct GroupStateContext *ctx);
 extern bool NodeIsDrainTimeExpired(const AutoFailoverNode *node,
 								   const struct GroupStateContext *ctx);
+extern bool NodeIsWaitPrimaryPresumedDead(const AutoFailoverNode *primaryNode,
+										  const AutoFailoverNode *activeNode,
+										  const struct GroupStateContext *ctx);

tests/tap/schedule

@@ -28,6 +28,7 @@ guard_data_loss
 replication_stall_3dc
 fast_forward
 demote_timeout_wait_primary_deadlock
+wait_primary_draining_deadlock
 timeline_fork_report_lsn_deadlock
 extension_update
 tablespaces

tests/tap/specs/wait_primary_draining_deadlock.pgaf

@@ -0,0 +1,90 @@
+# Regression test for https://github.com/hapostgres/pg_auto_failover/issues/1168.
+#
+# node1 (primary) loses its only quorum-counted standby (node2) and gets
+# reassigned wait_primary, even though node3 -- an async standby
+# (candidate-priority 50, replication-quorum false) -- stays healthy
+# throughout: async standbys never count toward secondaryQuorumNodesCount,
+# so losing node2 alone is enough to zero it out.
+#
+# node1 then itself becomes unhealthy while sitting at (converged)
+# wait_primary. The auto-failover trigger in ProceedGroupStateFromContext
+# (`IsCurrentState(activeNode, SECONDARY) && IsInPrimaryState(primaryNode)
+# && NodeIsUnhealthy(primaryNode) && ...`) used to unconditionally assign
+# node1 "draining" -- a goal state with no KeeperFSM[] edge from
+# wait_primary (only PRIMARY/JOIN_PRIMARY/APPLY_SETTINGS -> DRAINING
+# exist). node1's keeper would fatal and retry forever on that specific
+# assignment; the cluster still recovered, but only via the unrelated,
+# purely time-based drain-timeout self-heal, ~drain_timeout_ms late.
+#
+# Fixed by never reassigning primaryNode's goal away from wait_primary
+# along this path (there is nothing reachable to reassign it to) and
+# instead applying the same drain_timeout_ms safety margin via
+# NodeIsWaitPrimaryPresumedDead, anchored on the candidate's own
+# stateChangeTime rather than the primary's reportTime -- the primary
+# reconnecting and resuming reports (without ever converging) must not
+# reset that clock, or the promotion would stall forever instead of
+# completing on the same bounded timeline as any other primary failure.
+# This spec asserts the fatal never happens and the cluster still
+# converges within the same safety margin.
+
+cluster {
+    monitor
+    ssl off
+    formation {
+        node1
+        node2
+        node3  async
+    }
+}
+
+setup {
+    wait until node1 state is primary
+        and node2 state is secondary
+        and node3 state is secondary
+        timeout 120s
+}
+
+teardown {
+    compose down
+}
+
+step test_001_losing_the_only_sync_standby_reassigns_wait_primary {
+    network disconnect node2
+    wait until node1 assigned-state is wait_primary  timeout 60s
+    assert node3 state is secondary
+}
+
+step test_002_primary_failure_at_wait_primary_promotes_cleanly {
+    network disconnect node1
+
+    # node3 is the only remaining healthy candidate (async, candidate
+    # priority 50 > 0); the auto-failover trigger targets it once node1 is
+    # marked unhealthy by the monitor's own health checks. node1's own
+    # goal stays at wait_primary throughout (issue #1168 fix) instead of
+    # bouncing through the unreachable draining/demote_timeout states.
+    wait until node3 assigned-state is prepare_promotion  timeout 90s
+
+    network connect node1
+
+    # Recovers within the same drain_timeout_ms safety margin as any other
+    # primary failure -- not sped up, not stalled by node1 reconnecting and
+    # resuming (fruitless) reports along the way.
+    wait until node1 state is demoted  timeout 90s
+
+    # The fix's whole point: no unreachable assignment was ever made, so
+    # node1's keeper never fatals.
+    logs node1 not contains "does not know how to reach state"
+}
+
+step test_003_cluster_converges_back_after_recovery {
+    network connect node2
+    wait until node3 state is primary
+        and node1 state is secondary
+        and node2 state is secondary
+        timeout 120s
+}
+
+sequence
+    test_001_losing_the_only_sync_standby_reassigns_wait_primary
+    test_002_primary_failure_at_wait_primary_promotes_cleanly
+    test_003_cluster_converges_back_after_recovery