diff --git a/src/monitor/group_state_machine.c b/src/monitor/group_state_machine.c index 42a7125e5..85ed020a2 100644 --- a/src/monitor/group_state_machine.c +++ b/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,10 +968,16 @@ 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]; @@ -937,7 +985,7 @@ ProceedGroupStateFromContext(GroupStateContext *ctx) 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)); diff --git a/src/monitor/node_metadata.c b/src/monitor/node_metadata.c index c6d4c0eae..2b182ec70 100644 --- a/src/monitor/node_metadata.c +++ b/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); +} diff --git a/src/monitor/node_metadata.h b/src/monitor/node_metadata.h index ccc2eed57..0d336ca61 100644 --- a/src/monitor/node_metadata.h +++ b/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); diff --git a/tests/tap/schedule b/tests/tap/schedule index f7943bc96..5f2e58b51 100644 --- a/tests/tap/schedule +++ b/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 diff --git a/tests/tap/specs/wait_primary_draining_deadlock.pgaf b/tests/tap/specs/wait_primary_draining_deadlock.pgaf new file mode 100644 index 000000000..2246db0c9 --- /dev/null +++ b/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