Two-node auto-failover trigger can assign draining to a primary already at wait_primary (no KeeperFSM edge)

[dimitri]

Summary

The two-node auto-failover trigger in ProceedGroupStateFromContext
(src/monitor/group_state_machine.c:631-635) can assign draining to a
primary that is already sitting at wait_primary, an assignment the
keeper's KeeperFSM[] has no edge for (fsm.c: only
PRIMARY/JOIN_PRIMARY/APPLY_SETTINGS -> DRAINING exist, there is no
WAIT_PRIMARY -> DRAINING).

Reported in detail here, with a full production timeline:
#1165 (comment)

Why this is a separate issue from #774, not part of the #1165 fix

#774 and PR #1165's Fix 1 address a different rule entirely: the "no
healthy standby in the quorum" rule in ProceedGroupStateForPrimaryNode
(now guarded by IsFailoverInProgress, around line 1260). That rule reassigns
a converged primary to wait_primary when it races against its own
failover candidate leaving SECONDARY.

This report is about a structurally different rule — the two-node
auto-failover trigger:

if (IsCurrentState(activeNode, REPLICATION_STATE_SECONDARY) &&
    IsInPrimaryState(primaryNode) &&
    NodeIsUnhealthy(primaryNode, ctx) && NodeIsHealthy(activeNode, ctx) &&
    activeNode->candidatePriority > 0 &&
    WalDifferenceWithin(activeNode, primaryNode, PromoteXlogThreshold))
{
    AssignGoalState(activeNode, REPLICATION_STATE_PREPARE_PROMOTION, message);
    AssignGoalState(primaryNode, REPLICATION_STATE_DRAINING, message);   // <-- bug
    return true;
}

IsInPrimaryState() goes through CanTakeWritesInState(), which includes
WAIT_PRIMARY — so this rule can fire against a primary that's already
a converged wait_primary (reached via some earlier, unrelated event), not
only against a live PRIMARY/JOIN_PRIMARY/APPLY_SETTINGS. Its sibling
rule for the multi-standby case (group_state_machine.c:382-384) explicitly
excludes this with !IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY)
— this rule doesn't, and that asymmetry looks like an oversight rather than
a deliberate design choice.

Two more differences from #774 confirmed while triaging:

1. Rule 1's own guard makes it a no-op for this scenario. Fix 1
   (IsFailoverInProgress) lives entirely on the "no healthy standby" rule,
   which already can't fire once the primary is at wait_primary
   (!IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY) is
   already part of its own condition). So PR Fix #774: monitor can assign a primary an unreachable goal state during failover #1165 as merged is a no-op
   against this trigger — verified by diff, not by assumption.
2. It resolves on its own, but slowly, and via an unrelated path. Traced
   the sequence forward: the unreachable draining assignment gets
   overwritten one tick later by the unconditional
   "prepare_promotion -> stop_replication" rule
   (group_state_machine.c:789, which doesn't check the primary's actual
   reported state), reassigning the primary's goal to DEMOTE_TIMEOUT
   (also unreachable from WAIT_PRIMARY, but harmless). The purely
   time-based NodeIsDrainTimeExpired then eventually reassigns DEMOTED,
   which is reachable from WAIT_PRIMARY
   (WAIT_PRIMARY_STATE, DEMOTED_STATE edge, added in Fix how many nodes need to report their LSN to perform a failover. #707). So this is
   bounded by roughly one health-check interval plus drainTimeoutMs, not
   a permanent hang — matching the reporter's own account of the incident
   eventually resolving, just slower than it should (their write-up puts
   the unnecessary resync/failover cost at several minutes, only a
   fraction of which is the extra wait this bug adds).

Suggested direction (not yet designed/implemented)

Mirror the multi-standby rule's guard by excluding
REPLICATION_STATE_WAIT_PRIMARY from the two-node trigger's
IsInPrimaryState(primaryNode) condition. The original reporter noted an
internal attempt at something similar had side effects on other legitimate
transitions (single -> wait_primary, apply_settings -> draining), so
this needs its own careful design and empirical (multi-node) verification
before landing — same rigor #1165 went through, and keeping the standing
invariant from that PR: the monitor must never assign an unreachable goal
state in the first place; the keeper's loud refusal (log_fatal + retry)
is a deliberate safety net, not a bug to soften.