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

[dimitri]

Problem

A monitor rule could 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 during a manual failover (pg_autoctl perform failover). 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 gets reassigned wait_primary right as its own candidate is converging — then a separate rule (assuming the primary is still in one of its normal pre-demote states) assigns it demote_timeout, which has no KeeperFSM[] edge from wait_primary.

This matches the original issue reporter's own diagnosis in the thread: "the monitor should not continue with the usual failover transition" once it has reassigned the primary to wait_primary.

Fix

Rule 1 now skips its reassignment when IsFailoverInProgress() is true (the candidate already being in prepare_promotion or later counts). This prevents the race from happening at all — the primary goes primary → draining → demote_timeout cleanly, with no detour through wait_primary.

What this PR deliberately does not change

An earlier version of this PR also restricted the rule that assigns demote_timeout, to require the primary's reported state be one KeeperFSM[] actually has an edge from. That turned out to be unnecessary and harmful: main already has a working, purely time-based escape hatch (NodeIsDrainTimeExpired) that reassigns a primary stuck unresponsive at wait_primary to demoted — a state genuinely reachable from wait_primary (added in #707, before this issue was filed). The restriction prevented the primary's goal from ever becoming demote_timeout in that case, which is the precondition that escape hatch checks, breaking it and requiring an increasingly complex set of follow-up rules to compensate. Verified this by running the full CI reproduction against an unmodified origin/main: it already resolves the "primary dies at wait_primary" scenario cleanly (~35s via the drain timeout), no changes needed there.

The keeper's log_fatal-and-retry behavior for a genuinely unreachable assignment is also left as-is: log_fatal here is a severity label only (not a crash — the keeper loops and retries harmlessly), and the monitor should simply never hand out an unreachable transition in the first place, rather than have the keeper silently tolerate one.

Testing

Added src/monitor/sql/failover_candidate_leaves_secondary.sql, a pg_regress test isolating Rule 1's guard via direct state manufacture (the same technique stale_primary_report.sql uses for otherwise hard-to-reach states).

Verified via:

• make installcheck (pg_regress in Docker) — all 13 monitor tests + isolation tests pass.
• Full multi-async pgaftest schedule run against this exact code and against an unmodified origin/main baseline, using own local Docker image tags — both pass all 27 tests with matching behavior.

Issues

Fixes #774.

Also looked at #883 and #369 as candidates for the same root cause:

• Concurrently triggering failovers with multiple standbys causes FSM to get stuck #369: unrelated, already fixed by Fix when to allow for a failover in multiple standbys. #370 in a different code path (node_metadata.c/node_active_protocol.c).
• Impossible / unresolveable state after crash - How to recover? #883: likely the same underlying race (the FATAL signature and triggering event match exactly, and the maintainer's own closing PR Improve documentation thanks to user feedback. #909 links directly to How to recover from a current/assigned state combination without a possible state transition path? #774), but it was closed via a documentation-only recovery procedure rather than a code fix. Commented on Impossible / unresolveable state after crash - How to recover? #883 linking here rather than closing it outright, since I can't fully confirm identical code path on that 1.6.3-era code.

[tboevil]

We hit #774 in production, and I think there is a second, independent way into it that this PR does not close. Our path reaches wait_primary without Rule 1 ever being involved, so Fix 1 cannot help, and the damage happens before Fix 2 and Fix 3 get a chance to.

Production timeline

Two data nodes (A, B) plus a witness/monitor. Times are offsets from the start of an operator-initiated failover. PGDATA is ~8.5 GB.

00:00   perform_failover(): A -> draining, B -> prepare_promotion
00:00   B -> stop_replication, A -> demote_timeout
01:14   demote timeout expires: B -> wait_primary, A -> demoted
01:15   A -> catchingup, starts pg_rewind (8.6 GB)

        ... B sits at wait_primary for the next 6 minutes ...
        wait_primary -> primary needs a healthy secondary in the quorum,
        and A is catchingup the whole time.

06:18   A finishes pg_rewind, Postgres starts
06:43   A converges to secondary
        ^ B should now be advanced wait_primary -> primary

06:47   B's keeper starts timing out to the monitor (node_active fails)
        ^ 4 seconds later. That rule lives in
          ProceedGroupStateForPrimaryNode(), which only runs when the
          primary itself calls node_active() -- so B never gets advanced
          and stays pinned at wait_primary.

07:04.34  monitor marks B unhealthy
07:04.38  SECOND FAILOVER, 42 ms later:
            A -> prepare_promotion
            B -> draining          <-- B is at wait_primary
07:06     B's keeper receives wait_primary -> draining
07:06     B's keeper receives wait_primary -> demote_timeout
07:14.69  B is healthy again (the outage lasted 10 seconds)
07:20-35  FATAL x5 over 15s:
            "does not know how to reach state demote_timeout from wait_primary"
07:35     demote_timeout expires monitor-side, B -> demoted
07:44     B -> catchingup, second pg_rewind (8.7 GB)
11:21     second pg_rewind completes
11:49     A -> primary, cluster stable

The real outage was 10 seconds. It cost a second full 8.7 GB resync.

This is a different entry into wait_primary

In the scenario this PR reproduces, the primary is put into wait_primary by Rule 1 mid-failover, and Fix 1 stops that from happening.

In ours, B was already at wait_primary — it was promoted there by the first (operator-initiated) failover at 01:14 and got stuck because the window to advance it to primary opened at 06:43 and its keeper lost the monitor at 06:47. secondaryQuorumNodesCount never entered into it.

Rule 1 in fact cannot fire here: its first condition is already !IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY), and B is at wait_primary. So Fix 1 is a no-op for this timeline.

What fires at 07:04.38 is the two-node auto-failover trigger (group_state_machine.c:631-635):

	if (IsCurrentState(activeNode, REPLICATION_STATE_SECONDARY) &&
		IsInPrimaryState(primaryNode) &&
		NodeIsUnhealthy(primaryNode, ctx) && NodeIsHealthy(activeNode, ctx) &&
		activeNode->candidatePriority > 0 &&
		WalDifferenceWithin(activeNode, primaryNode, PromoteXlogThreshold))

IsInPrimaryState() goes through CanTakeWritesInState(), which includes WAIT_PRIMARY. So this rule assigns draining directly to a node sitting at wait_primary — and KeeperFSM[] has no wait_primary -> draining edge. The assignment is unreachable at the moment it is made, before any of the rules this PR touches are reached.

Worth noting the multi-standby path already guards against exactly this (group_state_machine.c:382-383):

		if (IsInPrimaryState(primaryNode) &&
			!IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY) &&
			candidatesCount >= 1)

Both rules assign the primary draining. The multi-standby one excludes wait_primary; the two-node one does not.

What #1165 changes for this timeline — and what it doesn't

Replaying the above with this PR applied, as best I can tell from the diff:

• 07:04.38 — the trigger still fires. B still gets draining, still unreachable, still fatals. Not covered.
• 07:06 — Fix 2's whitelist correctly blocks the follow-on demote_timeout (B's reportedState is wait_primary). Covered.
• Fix 3's complement rule promotes A from prepare_promotion directly, so the cluster no longer waits out demote_timeout. It would recover around 07:06 instead of 07:35. Real improvement.

But the failover still happens. A becomes the new primary, so B must demote and rejoin — and the 8.7 GB pg_rewind at 07:44 happens anyway. This PR makes an unnecessary failover complete cleanly; it doesn't stop it from being started. For this incident the entire cost was in that resync, so the saving would be about 30 seconds out of roughly 4.5 minutes.

I have not tested this against your branch — the above is from reading the diff, and the trigger at :631 being untouched is the part I'm confident about. Happy to actually run it if that's useful.

Suggested addition

One line, mirroring the multi-standby guard:

	if (IsCurrentState(activeNode, REPLICATION_STATE_SECONDARY) &&
		IsInPrimaryState(primaryNode) &&
		!IsCurrentState(primaryNode, REPLICATION_STATE_WAIT_PRIMARY) &&
		NodeIsUnhealthy(primaryNode, ctx) && NodeIsHealthy(activeNode, ctx) &&
		...

This is also the semantically right call on its own terms: CanInitiateFailover() already refuses manual failover from wait_primary, with the comment "we're not sure if the secondary has done catching-up yet" — which applies just as much to an automatic one. Our current internal fix is ​​this, but after further consideration, we found that it blocks some normal paths, such as single -> wait_primary or apply_settings -> draining.

[dimitri]

Thanks for the detailed write-up and the production timeline — that's a really useful trace, and you're right that it's a real gap.

I confirmed your read against the current state of this branch: Rule 1's own guard (!IsCurrentState(primaryNode, WAIT_PRIMARY)) means it can't fire once the primary is already at wait_primary, so this PR's fix is a no-op here — it's a genuinely separate path into the same class of bug (monitor assigns a goal state with no KeeperFSM[] edge from the node's current state), not something this PR happens to cover.

Given your own note that a similar internal fix broke single -> wait_primary and apply_settings -> draining, this needs its own careful design rather than a quick patch on top of this PR, so I've opened it as a separate issue: #1168.

To pin down the exact behavior (and give the fix something concrete to converge against), I also put together a minimal end-to-end repro as a pgaftest spec — #1169. It's a 3-node cluster (primary + sync secondary + async secondary), where the sync secondary fails first (async standbys never count toward quorum, so this alone reassigns the primary to wait_primary even with a healthy async standby still around), and then the primary itself fails while sitting at that wait_primary. On the current code that confirms exactly what you described: the primary gets handed an unreachable goal (draining, then demote_timeout), the keeper fatals and retries on it, and it only recovers via the unrelated, purely time-based drain-timeout mechanism — not a permanent hang, but a real unnecessary delay plus an assignment that should never have happened.

# Reproduces 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) && ...`) fires against node3 and assigns
# node1 "draining" -- a goal state with no KeeperFSM[] edge from
# wait_primary (only PRIMARY/JOIN_PRIMARY/APPLY_SETTINGS -> DRAINING
# exist). node1's keeper fatals and retries forever on that specific
# assignment. The cluster still recovers, but only via the unrelated,
# purely time-based drain-timeout self-heal (goal jumps straight to
# demoted once drain_timeout_ms elapses) -- not because the assignment was
# ever reachable.

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_hits_1168 {
    network disconnect node1

    # node3 is the only remaining healthy candidate (async, candidate
    # priority 50 > 0); the buggy trigger targets it once node1 is marked
    # unhealthy by the monitor's own health checks. node1's goal moves
    # draining -> demote_timeout within the same polling cycle (neither
    # reachable from wait_primary), so we don't probe that intermediate
    # value directly -- the FATAL log below is the stable signature.
    wait until node3 assigned-state is prepare_promotion  timeout 90s

    network connect node1

    # Recovers, but only via the unrelated drain-timeout self-heal (goal
    # re-targeted straight to demoted once drain_timeout_ms elapses), not
    # because "draining"/"demote_timeout" ever became reachable from
    # wait_primary.
    wait until node1 state is demoted  timeout 90s

    # node1 reported back in still at wait_primary along the way, was
    # handed one of those unreachable goals, and fataled/retried -- the
    # bug this spec documents.
    logs node1 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_hits_1168
    test_003_cluster_converges_back_after_recovery

The fix itself will happen on top of #1169 once it's merged, tracked under #1168.

Thanks again for catching this — it's a good find.