Fix #774: monitor can assign a primary an unreachable goal state during failover

src/monitor/expected/failover_candidate_leaves_secondary.out

@@ -0,0 +1,184 @@
+-- Copyright (c) Microsoft Corporation. All rights reserved.
+-- Licensed under the PostgreSQL License.
+--
+-- Regression test for issue #774: a monitor rule can assign a node a goal
+-- state its own KeeperFSM[] (src/bin/pg_autoctl/fsm.c) has no transition
+-- path to reach from that node's current state.
+--
+-- Root cause: two independent rules in group_state_machine.c could race.
+-- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the
+-- quorum" block) reassigns the primary to wait_primary whenever
+-- secondaryQuorumNodesCount == 0. That count drops to zero the instant a
+-- failover candidate leaves SECONDARY (e.g. converges to prepare_promotion)
+-- -- even though the candidate *is* the failover in progress, not a lost
+-- standby. Without a guard, the primary could be reassigned wait_primary
+-- right as its own candidate is converging, landing it on wait_primary
+-- right when a later rule still expects to find it in draining and assigns
+-- demote_timeout -- an assignment with no KeeperFSM[] edge from
+-- wait_primary, which the keeper cannot execute.
+--
+-- Fixed by: Rule 1 now skips its reassignment when IsFailoverInProgress()
+-- is true (the candidate already being in prepare_promotion counts). This
+-- prevents the race from happening in the first place, matching the root
+-- cause described by the original issue reporter.
+--
+-- Note: if a live primary is nonetheless ever handed an unreachable goal
+-- state by some other path, the keeper is expected to loudly refuse (fatal)
+-- rather than silently proceed -- that is a deliberate invariant, not a bug
+-- to be softened. The monitor's own drain-timeout mechanism
+-- (NodeIsDrainTimeExpired, purely time-based) already reconciles a primary
+-- stuck unresponsive at wait_primary by reassigning it to demoted (a state
+-- genuinely reachable from wait_primary), independently of this fix.
+--
+-- This scenario isolates Rule 1's guard. It doesn't need the isolation
+-- tester (pg_isolation_regress): it's a pure sequencing issue inside
+-- ProceedGroupStateFromContext, reproducible deterministically with a
+-- straight-line sequence of node_active() calls and a direct UPDATE to
+-- manufacture the otherwise hard-to-reach precondition -- the same
+-- technique stale_primary_report.sql already uses for a similarly
+-- hard-to-reach scenario (a primary that never gets to report its own
+-- demotion).
+\x on
+-- ═══════════════════════════════════════════════════════════════════════
+-- Scenario A (Rule 1 / Fix 1): a candidate already mid-promotion must not
+-- be treated as "the standby is just gone" by the primary's own routine
+-- report.
+-- ═══════════════════════════════════════════════════════════════════════
+SELECT pgautofailover.create_formation('fclma_test', 'pgsql', 'postgres', true, 0);
+-[ RECORD 1 ]----+--------------------------------
+create_formation | (fclma_test,pgsql,postgres,t,0)
+
+SELECT *
+  FROM pgautofailover.register_node('fclma_test', 'fclma_p', 5432,
+                                    'postgres', 'fclma_p', 1);
+-[ RECORD 1 ]---------------+--------
+assigned_node_id            | 31
+assigned_group_id           | 0
+assigned_group_state        | single
+assigned_candidate_priority | 100
+assigned_replication_quorum | t
+assigned_node_name          | fclma_p
+
+SELECT nodeid AS np FROM pgautofailover.node
+ WHERE formationid = 'fclma_test' AND nodename = 'fclma_p' \gset
+SELECT *
+  FROM pgautofailover.register_node('fclma_test', 'fclma_s', 5432,
+                                    'postgres', 'fclma_s', 1);
+-[ RECORD 1 ]---------------+-------------
+assigned_node_id            | 32
+assigned_group_id           | 0
+assigned_group_state        | wait_standby
+assigned_candidate_priority | 100
+assigned_replication_quorum | t
+assigned_node_name          | fclma_s
+
+SELECT nodeid AS ns FROM pgautofailover.node
+ WHERE formationid = 'fclma_test' AND nodename = 'fclma_s' \gset
+-- bootstrap: p = primary, s = secondary
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'single');
+-[ RECORD 1 ]--------+-------
+assigned_group_state | single
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'wait_standby');
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_standby
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'single',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_primary
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'wait_primary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+-------------
+assigned_group_state | wait_primary
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'wait_standby');
+-[ RECORD 1 ]--------+-----------
+assigned_group_state | catchingup
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'catchingup',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+----------
+assigned_group_state | secondary
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'secondary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+----------
+assigned_group_state | secondary
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'wait_primary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+--------
+assigned_group_state | primary
+
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'primary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+--------
+assigned_group_state | primary
+
+SELECT nodename, reportedstate, goalstate
+  FROM pgautofailover.node
+ WHERE formationid = 'fclma_test'
+ ORDER BY nodename;
+-[ RECORD 1 ]-+----------
+nodename      | fclma_p
+reportedstate | primary
+goalstate     | primary
+-[ RECORD 2 ]-+----------
+nodename      | fclma_s
+reportedstate | secondary
+goalstate     | secondary
+
+-- The candidate is already fully converged at prepare_promotion (a
+-- promotion is genuinely in progress -- this is what IsFailoverInProgress()
+-- must recognize). This can be reached for real via perform_promotion() or
+-- a multi-standby perform_failover(); manufactured directly here to isolate
+-- Rule 1 from everything else.
+UPDATE pgautofailover.node
+   SET reportedstate = 'prepare_promotion',
+       goalstate = 'prepare_promotion'
+ WHERE formationid = 'fclma_test' AND nodename = 'fclma_s';
+-- The primary's own next routine report must NOT be reassigned
+-- wait_primary just because its one standby is no longer "secondary" --
+-- that standby is the active failover candidate, not a lost node.
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'primary',
+                                  current_lsn => '0/5000');
+-[ RECORD 1 ]--------+--------
+assigned_group_state | primary
+
+-- ASSERT: fclma_p's goalstate is still 'primary'.
+-- Pre-fix: Rule 1 reassigns it to 'wait_primary' here.
+SELECT nodename, reportedstate, goalstate
+  FROM pgautofailover.node
+ WHERE formationid = 'fclma_test'
+ ORDER BY nodename;
+-[ RECORD 1 ]-+------------------
+nodename      | fclma_p
+reportedstate | primary
+goalstate     | primary
+-[ RECORD 2 ]-+------------------
+nodename      | fclma_s
+reportedstate | prepare_promotion
+goalstate     | prepare_promotion
+

src/monitor/group_state_machine.c

@@ -1319,9 +1319,20 @@ ProceedGroupStateForPrimaryNode(GroupStateContext *ctx,
 		 * the two defective standby nodes is available again.
 		 */
 		if (!IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY) &&
-			secondaryQuorumNodesCount == 0)
+			secondaryQuorumNodesCount == 0 &&
+			!IsFailoverInProgress(ctx->groupNodeList))
 		{
 			/*
+			 * Do not second-guess an already-started failover: a candidate
+			 * that just converged to prepare_promotion (or later stages) has
+			 * left SECONDARY, dropping secondaryQuorumNodesCount to zero even
+			 * though it *is* the failover candidate. Without this guard the
+			 * primary can be reassigned wait_primary right as its own
+			 * candidate is converging, landing it on wait_primary while a
+			 * later rule still expects to find it in draining and assigns
+			 * demote_timeout -- an assignment with no FSM edge from
+			 * wait_primary (issue #774).
+			 *
 			 * Allow wait_primary when number_sync_standbys = 0, otherwise
 			 * block writes on the primary.
 			 */

src/monitor/regress_schedule

@@ -40,6 +40,7 @@ test: drop_node
 test: stale_primary_report
 test: lock_and_fetch_migration
 test: timeline_fork_detection
+test: failover_candidate_leaves_secondary
 test: dummy_update
 test: drop_extension
 test: upgrade

src/monitor/sql/failover_candidate_leaves_secondary.sql

@@ -0,0 +1,129 @@
+-- Copyright (c) Microsoft Corporation. All rights reserved.
+-- Licensed under the PostgreSQL License.
+--
+-- Regression test for issue #774: a monitor rule can assign a node a goal
+-- state its own KeeperFSM[] (src/bin/pg_autoctl/fsm.c) has no transition
+-- path to reach from that node's current state.
+--
+-- Root cause: two independent rules in group_state_machine.c could race.
+-- Rule 1 (ProceedGroupStateForPrimaryNode, "no healthy standby in the
+-- quorum" block) reassigns the primary to wait_primary whenever
+-- secondaryQuorumNodesCount == 0. That count drops to zero the instant a
+-- failover candidate leaves SECONDARY (e.g. converges to prepare_promotion)
+-- -- even though the candidate *is* the failover in progress, not a lost
+-- standby. Without a guard, the primary could be reassigned wait_primary
+-- right as its own candidate is converging, landing it on wait_primary
+-- right when a later rule still expects to find it in draining and assigns
+-- demote_timeout -- an assignment with no KeeperFSM[] edge from
+-- wait_primary, which the keeper cannot execute.
+--
+-- Fixed by: Rule 1 now skips its reassignment when IsFailoverInProgress()
+-- is true (the candidate already being in prepare_promotion counts). This
+-- prevents the race from happening in the first place, matching the root
+-- cause described by the original issue reporter.
+--
+-- Note: if a live primary is nonetheless ever handed an unreachable goal
+-- state by some other path, the keeper is expected to loudly refuse (fatal)
+-- rather than silently proceed -- that is a deliberate invariant, not a bug
+-- to be softened. The monitor's own drain-timeout mechanism
+-- (NodeIsDrainTimeExpired, purely time-based) already reconciles a primary
+-- stuck unresponsive at wait_primary by reassigning it to demoted (a state
+-- genuinely reachable from wait_primary), independently of this fix.
+--
+-- This scenario isolates Rule 1's guard. It doesn't need the isolation
+-- tester (pg_isolation_regress): it's a pure sequencing issue inside
+-- ProceedGroupStateFromContext, reproducible deterministically with a
+-- straight-line sequence of node_active() calls and a direct UPDATE to
+-- manufacture the otherwise hard-to-reach precondition -- the same
+-- technique stale_primary_report.sql already uses for a similarly
+-- hard-to-reach scenario (a primary that never gets to report its own
+-- demotion).
+
+\x on
+
+-- ═══════════════════════════════════════════════════════════════════════
+-- Scenario A (Rule 1 / Fix 1): a candidate already mid-promotion must not
+-- be treated as "the standby is just gone" by the primary's own routine
+-- report.
+-- ═══════════════════════════════════════════════════════════════════════
+
+SELECT pgautofailover.create_formation('fclma_test', 'pgsql', 'postgres', true, 0);
+
+SELECT *
+  FROM pgautofailover.register_node('fclma_test', 'fclma_p', 5432,
+                                    'postgres', 'fclma_p', 1);
+
+SELECT nodeid AS np FROM pgautofailover.node
+ WHERE formationid = 'fclma_test' AND nodename = 'fclma_p' \gset
+
+SELECT *
+  FROM pgautofailover.register_node('fclma_test', 'fclma_s', 5432,
+                                    'postgres', 'fclma_s', 1);
+
+SELECT nodeid AS ns FROM pgautofailover.node
+ WHERE formationid = 'fclma_test' AND nodename = 'fclma_s' \gset
+
+-- bootstrap: p = primary, s = secondary
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'single');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'wait_standby');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'single',
+                                  current_lsn => '0/5000');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'wait_primary',
+                                  current_lsn => '0/5000');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'wait_standby');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'catchingup',
+                                  current_lsn => '0/5000');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :ns, 0,
+                                  current_group_role => 'secondary',
+                                  current_lsn => '0/5000');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'wait_primary',
+                                  current_lsn => '0/5000');
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'primary',
+                                  current_lsn => '0/5000');
+
+SELECT nodename, reportedstate, goalstate
+  FROM pgautofailover.node
+ WHERE formationid = 'fclma_test'
+ ORDER BY nodename;
+
+-- The candidate is already fully converged at prepare_promotion (a
+-- promotion is genuinely in progress -- this is what IsFailoverInProgress()
+-- must recognize). This can be reached for real via perform_promotion() or
+-- a multi-standby perform_failover(); manufactured directly here to isolate
+-- Rule 1 from everything else.
+UPDATE pgautofailover.node
+   SET reportedstate = 'prepare_promotion',
+       goalstate = 'prepare_promotion'
+ WHERE formationid = 'fclma_test' AND nodename = 'fclma_s';
+
+-- The primary's own next routine report must NOT be reassigned
+-- wait_primary just because its one standby is no longer "secondary" --
+-- that standby is the active failover candidate, not a lost node.
+SELECT assigned_group_state
+  FROM pgautofailover.node_active('fclma_test', :np, 0,
+                                  current_group_role => 'primary',
+                                  current_lsn => '0/5000');
+
+-- ASSERT: fclma_p's goalstate is still 'primary'.
+-- Pre-fix: Rule 1 reassigns it to 'wait_primary' here.
+SELECT nodename, reportedstate, goalstate
+  FROM pgautofailover.node
+ WHERE formationid = 'fclma_test'
+ ORDER BY nodename;

tests/pgautofailover_utils.py

@@ -1848,7 +1848,7 @@ def list_replication_slot_names(self):
             self.print_debug_logs()
             raise e
 
-    def has_needed_replication_slots(self, retry_timeout=5):
+    def has_needed_replication_slots(self, retry_timeout=15):
         """
         Each node is expected to maintain a slot for each of the other nodes
         the primary through streaming replication, the secondary(s) manually

tests/test_citus_multi_standbys.py

@@ -192,7 +192,7 @@ def test_003_002_all_workers_have_data():
     results = worker2b.run_sql_query(q1)
     eq_(results, r2)
 
-    results = worker2c.run_sql_query(q1)
+    results = worker2c.run_sql_query_retry(q1)
     eq_(results, r2)