Timeline-fork detection: fix #683 unrecoverable state after failed switchover#1161
Merged
Conversation
dimitri
force-pushed
the
timeline-fork-detection
branch
from
July 27, 2026 12:07
ba350e2 to
c9f3105
Compare
…ust numbers Fixes #683. standby_check_timeline_with_upstream() only ever compared timeline numbers (upstreamTimeline >= localTimeline): it cannot tell "genuine ancestor, just behind" apart from "diverged, but the timeline number still happens to be lower" -- a standby whose own WAL replay has gone past the point where its timeline was actually superseded looks identical to one that's simply still catching up, as long as its number is lower. The former can never resolve through ordinary streaming replication and needs pg_rewind; the latter just needs to wait. Add standby_verify_timeline_ancestry(), which walks the upstream's own TIMELINE_HISTORY data -- already fetched into replicationSource.system.timelines by the pgctl_identify_system() call standby_check_timeline_with_upstream() already makes, so this needs no new network round trip -- and confirms the local checkpoint LSN lies strictly before the point where the local timeline was superseded in the upstream's lineage. When it doesn't (genuine divergence), invoke the existing primary_rewind_to_standby() and retry the metadata fetch, rather than retrying the same doomed comparison forever.
Adds the shared infrastructure a report_lsn election needs to reason about timeline ancestry across nodes, since there is no elected primary at that point to check against -- that has to live on the monitor. - pgautofailover.node_timeline_history: append-only (nodeid, tli, parenttli, switchpoint_lsn) facts, unioned across every node's own history. pgautofailover.report_timeline_history(node_id, history jsonb) upserts a batch via ON CONFLICT DO NOTHING (immutable facts, safe to resend). Written as plpgsql rather than C specifically to lean on jsonb_array_elements() instead of hand-rolled JSONB C API code. - pgautofailover.accepted_timeline: the (empty by default) operator pin table part 3 will use; added now since it's part of the same migration. - timeline_history.c/.h (new): reads a node's own pg_wal/<tli>.history file directly (no replication connection needed) and encodes it as the JSON array report_timeline_history expects. - Keeper wiring, two call sites per the design: every tick in the main loop, unconditionally of FSM state (an uneventful secondary picks up a timeline switch through ordinary streaming replication with no fsm_* transition ever running, so this can't be hooked off transitions alone), gated by an in-memory watermark so it's a no-op once caught up; and once more, synchronously, immediately before fsm_report_lsn()'s standalone restart -- the riskiest moment, since a genuine divergence can hard FATAL there and the node never gets to report anything again. The election itself doesn't consult this table yet -- that's next.
The election never actually used timeline at all, despite a comment on WalDifferenceWithin() claiming it does: ListMostAdvancedStandbyNodes() and SelectFailoverCandidateNode() both sort/pick purely by raw reportedLSN. A node on a genuinely forked, diverged timeline compares as "more advanced" exactly like a node that's simply further along the same lineage -- there is no way to tell them apart from LSN alone. timeline_history.c/.h (new, monitor-side): walks pgautofailover.node_timeline_history to answer "is candidate tli a genuine ancestor of the group's reference tli", where the reference is either an operator's pin in pgautofailover.accepted_timeline (part 3) or, in the normal case, auto-detected as the branch containing the highest reported tli among the group's own nodes. ProceedGroupStateForMSFailover now filters the group's node list through FilterNodesByTimelineAncestry() before it reaches BuildCandidateList() and ListMostAdvancedStandbyNodes() -- the two places that assemble and rank the candidate pool. Nodes outside the reference lineage are excluded entirely, not deprioritized: a diverged node can never become comparable no matter how long the election waits for it, so counting it as "missing" would block the whole group forever (see #683, #1113); excluding it lets the remaining, genuinely comparable candidates proceed normally, and if exclusion happens to leave zero candidates, the existing "not enough candidates yet" handling already does the right, graceful thing with no new plumbing needed. The already-selected-candidate fast path (a failover already in progress) is untouched: filtering only applies to the "pick a new candidate" path.
candidate_priority doesn't work for this: it's consulted inside SelectFailoverCandidateNode, which runs strictly after BuildCandidateList has already assembled the comparable pool -- the ancestry filter lives at or below that assembly step, deciding whether nodes are comparable at all, before priority ever enters the picture. Zeroing a node's priority changes how it's ranked once it's in a comparable set; it does nothing to tell the ancestry filter "ignore this node's incomparable position". What's missing is a fact the operator asserts explicitly, stored somewhere the filter itself consults -- not inferred from a side effect of an unrelated field. - pgautofailover.accept_timeline(formation_id, group_id, tli, decided_by): validates the tli actually appears in node_timeline_history for the group (refuses to pin a timeline nobody has ever reported), then inserts the pin. GetAcceptedTimeline() (already wired into FilterNodesByTimelineAncestry in the previous commit) picks it up ahead of auto-detection. - pgautofailover.resolve_accepted_timeline(formation_id, group_id): marks the pin resolved. Wired into PromoteSelectedNode() so it fires automatically once a candidate is actually selected for promotion -- kept as a permanent audit record rather than deleted. - pg_autoctl accept timeline --formation --group --tli [--reason]: new CLI command calling accept_timeline() via monitor_accept_timeline().
Add node_timeline_status() SQL function (recursive ancestry CTE, per node current tli/lsn against the group's reference lineage) and wire it up to a new `pg_autoctl show timeline` command that prints the group's known timeline history plus each node's position against it, flagging any node that has diverged.
Covers report_timeline_history()/node_timeline_history idempotency,
node_timeline_status() ancestry computation (unpinned auto-detect,
operator pin, and post-resolve revert), accept_timeline()'s rejection
of unreported timelines, and an end-to-end election test: a 4-node
group with two diverging branches where the operator's pin makes the
election promote the pinned lineage and exclude the higher-numbered
sibling branch, auto-resolving the pin on promotion.
Also fixes two bugs the test caught:
- accept_timeline()'s target_tli parameter shadowed the tli column
it was compared against, so every call raised 'column reference
"tli" is ambiguous' instead of doing anything.
- timeline_history.c was missing from pgaftest's SHARED_SRCS list,
so the pgaftest binary failed to link as soon as fsm_transition.c
and service_keeper.c started calling into it (Part 2).
Without ON DELETE CASCADE, dropping any node that had ever published
timeline history left pg_autoctl node drop / node_active('dropped')
failing with 'update or delete on table node violates foreign key
constraint node_timeline_history_nodeid_fkey' -- caught by running
basic_operation.pgaf (test_016_drop_secondary) against a rebuilt
docker image after the timeline-fork-detection changes.
node_timeline_history rows are provenance (who reported this fact),
not the fact itself -- other nodes' rows for the same tli are
identical and unaffected, so cascading the delete loses nothing an
election or ancestry check depends on.
standby_check_timeline_with_upstream() only knew how to rewind a
standby that was BEHIND its upstream's timeline (Part 1's genuine-
divergence check). When local tli > upstream tli -- a standby that
generated WAL past a fork point, e.g. after being promoted outside of
pg_autoctl's control -- it just logged an error and returned false,
forever: Postgres itself refuses to stream ("requested timeline N is
not a child of this server's history" / "highest timeline N of the
primary is behind recovery timeline M"), and nothing ever called
pg_rewind to get out of it. That's the literal "unrecoverable state"
from #683. Fixed by rewinding symmetrically in that direction too.
Also broadens the ancestry check itself: fsm_follow_new_primary() is
used for both REPORT_LSN/JOIN_SECONDARY -> SECONDARY (already checked)
and SECONDARY -> CATCHINGUP (an ordinary health-check-driven resync,
previously skipped entirely since the check was gated on
assigned_role == SECONDARY_STATE). Manual reproduction showed this is
exactly the path a forked standby takes when the health checker
notices it and cycles it back through catchingup -- without the fix
here it hit the identical doomed-reconnect loop one gate over.
Finally, adds a defensive, synchronous timeline-history publish right
before both ancestry checks (refactored out of fsm_report_lsn's
existing one into keeper_defensive_publish_timeline_history()): the
periodic per-tick publish and FSM transition processing share a
cadence, so a fork that both appears and gets rewound within a single
tick could otherwise never make it into node_timeline_history at all,
losing the only record it happened.
Root-caused and verified via manual reproduction against a live
Docker cluster (network-partition the standby, promote it directly at
the Postgres level to fork it, reconnect, force it back through
catchingup): pre-fix, Postgres logs "waiting for WAL to become
available" in an infinite retry loop; post-fix, it rewinds (or falls
back to pg_basebackup when pg_rewind can't itself connect) and rejoins
cleanly. tests/tap/specs/timeline_fork_report_lsn_deadlock.pgaf turns
that reproduction into an automated, repeatable end-to-end spec.
…timeline coverage pg_autoctl show timeline and pg_autoctl accept timeline (Part 3/4) previously had monitor-SQL-level coverage only (timeline_fork_detection.sql) -- nothing exercised the actual CLI against a live cluster (cli_show.c/cli_accept.c, monitor_print_timeline()). New test_003_show_and_accept_timeline_cli step, inserted into the existing #683 fork scenario: unpinned, node2's fork reads clean (the known 2-node limitation from the header comment -- it IS the highest reported tli, and nobody else is arguing); accept_timeline rejects an unreported tli and pins the correct one; show timeline then correctly flags node2 as FORK. Orthogonal to the maintenance-cycle recovery step that follows it, since the standby-side #683 fix never consults the pin -- only a monitor-side election does.
Audited tests/tap/specs/*.pgaf against the .sch shards actually wired into ci.yml's pgaftest matrix (the flat tests/tap/schedule file is a manual/local reference, not read by CI). Found 5 specs that existed but never ran in CI: - guard_data_loss, fast_forward: real specs (issues #1113, #1060), present in the flat schedule file but dropped when the .sch shards were split out. Added to multi-misc.sch (3-node scenarios, same family as the existing multi_standbys/multi_maintenance). - demote_timeout_wait_primary_deadlock (#1025) and timeline_fork_report_lsn_deadlock (#683): added this session, never wired up. Added to node.sch alongside replication_stall_3dc, which had the same gap. Left alone, by design: debug_citus_worker_switchover.pgaf and debug_failover_pg19.pgaf are explicitly documented as interactive-only diagnostic templates; upgrade.pgaf has its own dedicated CI job (needs pgaf:current/pgaf:next images the standard matrix doesn't build). Also brought the flat tests/tap/schedule file up to date with specs it was missing entirely (launch_deferred_set_metadata, postgres_version_tracking, tablespaces) plus the two new ones, so it stays a genuine full-suite reference for local runs. Verified locally: multi-misc.sch runs 6/7 specs clean; the 7th (fast_forward test_003) fails identically on unmodified origin/main (confirmed via a throwaway worktree build) -- a pre-existing, branch-unrelated flake, not a regression from this branch's changes. node.sch's new entries were each verified individually before being added; a full sequential node.sch run is still in progress.
…via SQL Root-caused via a throwaway origin/main worktree: this deadlock is pre-existing, not caused by the timeline-fork-detection changes, and was previously masked simply by never running in CI (see the prior commit adding this spec to multi-misc.sch). exec pg_autoctl perform failover --allow-data-loss blocks waiting for a node to reach exact PRIMARY_STATE (both goalState and reportedState) within listen_notifications_timeout (60s default). In this scenario node2 can only reach wait_primary during step 3 -- number_sync_standbys=1 needs a second live quorum standby, and node3 doesn't come back until step 4 -- so that wait can never succeed and the exec always times out. guard_data_loss.pgaf's near-identical scenario (primary + one standby killed, survivor promoted with --allow-data-loss) already documented and worked around this exact deadlock by calling pgautofailover.perform_failover() directly over SQL instead of through the CLI. Apply the same fix here. Verified stable across two full runs (5/5).
Reverts fast_forward.pgaf back to its pristine origin/main content (undoing commit cf9ba18) and removes it from multi-misc.sch. While making the spec actually exercise the real fast_forward FSM state (per review feedback that the CLI-deadlock fix alone left it functionally duplicating guard_data_loss.pgaf), it surfaced a genuine, pre-existing, unrelated bug: standby_fetch_missing_wal() (primary_standby.c) blocks in an unbounded loop with no timeout or WAL source liveness check, so once a fast_forward candidate's keeper commits to fetching, the monitor's own issue #1060 stuck-detection guard can never reach it -- the keeper simply never calls node_active() again to receive the reset. That's a real fix (a timeout/liveness check on the fetch loop), not a test-file fix, and out of scope here. fast_forward.pgaf and its stuck-detection bug will be picked up on a separate branch/PR. guard_data_loss stays in multi-misc.sch; it does not depend on fast_forward and was independently verified passing.
My earlier local style-check invocation (docker run citus/stylechecker without an explicit citus_indent argument) wasn't actually running the check -- it silently did nothing, so this drift went unnoticed until CI's style_checker job (which runs citus_indent --check directly inside the container) caught it on push. The Makefile already has the right invocation (make docker-check / make docker-indent); using that going forward instead of hand-rolling the docker command.
…vior monitor_print_timeline() previously predicted that 'automatic election is refused' for diverged nodes. That's a server-side election policy statement made from client-side CLI code, with nothing tying the two together, and it's imprecise: the design excludes the diverged node from candidacy, it doesn't refuse the whole election. Reword to state only the observed fact and point at the resolution command.
…ult-tolerance, architecture, FAQ, operations) New manual pages for pg_autoctl accept timeline and pg_autoctl show timeline, modeled on the existing perform-failover and show-standby-names pages, with example output captured from a live 2-node cluster forked the same way tests/tap/specs/timeline_fork_report_lsn_deadlock.pgaf does it. Wires both into the existing indexes (pg_autoctl_show.rst, manual.rst, pg_autoctl.rst). Extends the existing prose docs rather than introducing new structure: - failover-state-machine.rst: ancestry-check paragraphs in the Catchingup and Report_LSN sections, alongside the existing guard_data_loss paragraph. - fault-tolerance.rst: new "Timeline Forks" subsection with a 6th tikz sequence diagram (seq-timeline-fork.tex/.svg), following the same prose+diagram pattern as the other four scenarios. - architecture.rst: extends the existing "Node recovery" section. - faq.rst: new entry alongside the existing report_lsn-stuck entry. - operations.rst: new "Resolving a detected timeline fork" subsection under Cluster Management and Operations, plus a Trouble-Shooting Guide pointer, using the same captured-transcript style as the other operations subsections. No new operations manual needed -- docs/operations.rst already exists and is the right place for this. Verified with a full sphinx html build (no warnings) and a full tikz svg rebuild.
CI's upgrade.pgaf job failed: an in-place ALTER EXTENSION UPDATE TO '2.3' left several functions this branch (and two other already-merged PRs) added to the fresh-install script completely absent from the incremental 2.2--2.3 upgrade script, so any node calling them post-upgrade got 'function does not exist'. Ports over, matching the fresh-install definitions in pgautofailover.sql exactly: - set_node_region() (#1156) - the four pg_version*/citus_version columns and report_postgres_version() (#1157) - node_timeline_history / accepted_timeline tables and report_timeline_history() / accept_timeline() / resolve_accepted_timeline() / node_timeline_status() (this branch) Verified with make -C tests/upgrade all (builds pgaf:current from the v2.2 tag and pgaf:next from this branch, then runs tests/tap/specs/upgrade.pgaf end to end against a live 3-node cluster) -- all 10 steps pass, including the convergence step that previously failed.
- pg_autoctl_accept_timeline.rst: drop the --tli 99 validation-error
example (that's a unit-test concern, not documentation) and replace it
with a pg_autoctl show timeline capture that makes the actual case for
needing to pin a lineage.
- fault-tolerance.rst: rephrase the Timeline Forks section to describe
only current behavior, not implementation history ("pre-fix, ...").
- pg_autoctl_show_timeline.rst: new worked example correcting an
assumption the docs didn't previously address -- a 3-node formation
does NOT by itself close the 2-node auto-detection blind spot.
FilterNodesByTimelineAncestry()'s heuristic only excludes a candidate
when a genuinely competing branch is reported; a sibling that simply
stays behind isn't competing with anything, so the fork still reads as
clean and pg_autoctl accept timeline is still required. Backed by a new
pgaf spec, tests/tap/specs/timeline_fork_3node_auto_detect.pgaf (added
to tests/tap/schedules/node.sch), verified locally end to end -- all 6
steps pass, including the corrected assertions after the initial design
wrongly assumed 3 nodes would auto-detect.
Both new/changed CLI examples use real output captured from live 3-node
(and, for the accept_timeline page, 2-node) clusters, same convention as
the rest of this branch's docs.
(The two new Unhealthy-Nodes sequence diagrams originally drafted
alongside this work -- Primary node monitored unhealthy, Monitor node
has failed -- document pre-existing, unrelated behavior and were split
out to PR #1162 instead.)
runner_exec_step()'s errBuf was declared char[512] at all 8 call sites, while r->lastSqlOutput (the actual command output an 'expect' step compares against) is char[4096]. Any expect mismatch against output bigger than ~450 bytes -- e.g. pg_autoctl show timeline against a 3+ node formation -- overflowed the format buffer, and sformat()'s own overflow guard silently swallowed the real diagnostic, printing only 'BUG: sformat needs N bytes...' instead of the actual 'expected X, got Y' message. Bumped all 8 err/tdErr declarations to 8192 bytes (2x lastSqlOutput's cap plus room for the template and cmd->expected). Verified by forcing a real expect mismatch against a multi-row show timeline table (tests/tap/specs/timeline_fork_3node_auto_detect.pgaf with one expect deliberately broken): previously printed only the BUG warning, now prints the full untruncated 'expected ..., got ...' message with the complete table.
The previous 3-node example forked node3 while node1/node2 sat completely idle -- unrealistic, since a fork developing on one standby elsewhere in the formation is no reason for the main system to pause. Extended timeline_fork_3node_auto_detect.pgaf so the application keeps writing to node1 (and replicating to node2) throughout every step of the scenario -- before the fork, during node3's isolation, after it reconnects, after the operator pins the lineage, and confirms all 200 rows survive node3's eventual recovery untouched. Recaptured the docs example from this updated spec: node1/node2's LSN is now visibly ahead of node3's diverged tip throughout, showing the ancestry-check blind spot alongside ordinary, uninterrupted application traffic rather than against a frozen cluster. Verified: all 6 pgaf steps pass end to end against a live 3-node cluster.
Adds a new guard in ProceedGroupStateFromContext(), alongside the existing SECONDARY-state checks: when a node currently in SECONDARY_STATE reports a timeline that FilterNodesByTimelineAncestry() excludes from the group's reference lineage, it's pushed to CATCHINGUP immediately -- on its very next node_active() call -- instead of waiting for an incidental health-check cycle or an explicit maintenance toggle to eventually drive it there. This only changes *when* a diverged secondary gets pushed through the existing, already-tested ancestry-check-and-rewind path (the #683 fix); it reuses the same FilterNodesByTimelineAncestry() the report_lsn election path already calls, so the two can't drift apart. Scoped to SECONDARY_STATE specifically, matching the exact guard the pre-existing "unhealthy secondary" transition uses for the identical goal-state assignment. Verified: monitor rebuilt, docker image rebuilt, both live-cluster specs (2-node and 3-node) still pass end to end. Directly confirmed via a manual repro and via a new dedicated step (test_005_automatic_catchingup_without_maintenance in timeline_fork_3node_auto_detect.pgaf) that the monitor now assigns catchingup within ~1s of an operator's accept_timeline pin, with no maintenance toggle involved -- log evidence: "Setting goal state of node3 to catchingup: its reported timeline 2 does not appear to be an ancestor of the group's reference timeline 1 -- forcing a resync...". Kept the original maintenance-cycle-based full recovery check as a separate, subsequent step: reaching CATCHINGUP is now immediate, but completing the actual pg_rewind still depends on node1's pg_hba.conf hostname resolution being current against node3's address, a pre-existing dependency this change doesn't add or remove. That dependency's own staleness-after-reconnect behavior is a separate, already-flagged issue, unrelated to this fix's correctness.
…hardcoded postgres Root-caused a permanent, 100%-reproducible pg_rewind failure hit while testing the new automatic timeline-fork detection: any formation whose --dbname isn't literally postgres (e.g. this branch's own timeline_fork_3node_auto_detect.pgaf, using demo) has pg_rewind's connection rejected by every peer's pg_hba.conf, every single time -- not a race, not a DNS caching issue (both directions of the hostname resolution were confirmed fine via the Postgres log's own DETAIL lines: forward lookup matches). pg_autoctl's own pghba_ensure_host_rules_exist() only ever writes HBA rules for the formation's configured database name plus the replication pseudo-database -- never a bare literal postgres unless that also happens to be the configured name. But pg_rewind()'s --source-server connection string hardcoded dbname=postgres regardless, so it could never match any rule pg_autoctl itself had written. ReplicationSource already carries the correct configured dbname (populated by standby_init_replication_source() from postgresSetup.dbname, the exact same value passed to pghba_ensure_host_rules_exist()) -- pg_rewind() just wasn't using it. Falls back to postgres only if that field is somehow empty, preserving prior behavior for that edge case. This masked itself in most existing coverage because fsm_rewind_or_init() (the pre-existing MAINTENANCE-to-CATCHINGUP path) already has a pg_basebackup fallback for any pg_rewind failure, so recovery would silently succeed via a full base backup instead of an incremental rewind -- slower and more wasteful, but not visibly broken. The #683 ancestry-check path (standby_check_timeline_with_upstream(), this branch's own new code) has no such fallback: a pg_rewind failure there just returns false and retries forever, with no escape valve, which is what actually exposed this. Verified directly: a manual replicator connection to the configured database now succeeds where it previously failed with 'no pg_hba.conf entry'; the full automatic-detection scenario (timeline_fork_3node_auto_detect.pgaf) now completes via a genuine incremental pg_rewind (pg_rewind: Done!, 85MB copied in under a second) instead of the pg_basebackup fallback it was hitting before. All 7 steps in that spec still pass end to end.
…orked secondary Once a diverged secondary's timeline mismatch is visible to the monitor (via an explicit accept_timeline pin, or a genuinely competing branch auto-detected with no pin at all), it is now pushed to catchingup and rewound with pg_rewind within about a second -- no maintenance cycle or incidental health-check-driven resync needed to trigger it, as the docs previously described. - failover-state-machine.rst: new paragraph under Secondary describing the monitor-side ancestry check on every report. - fault-tolerance.rst: rewritten Timeline Forks prose + updated seq-timeline-fork.tex/.svg diagram (operator pin -> immediate goal=catchingup, no maintenance step depicted). - architecture.rst: Node recovery section mentions the proactive check. - faq.rst: timeline-mismatch entry no longer frames recovery as waiting on the next health-check cycle. - ref/pg_autoctl_accept_timeline.rst, ref/pg_autoctl_show_timeline.rst, operations.rst: replace "cycle the node through maintenance to force it" language with the new automatic, immediate behavior, verified against a live docker cluster (accept timeline -> catchingup -> rewound secondary within ~1s, confirmed via monitor logs).
…-catchingup guard and the maintenance-cycle step
CI (logs_81878989213) showed test_004_maintenance_cycle_recovers failing
intermittently (3/6 PG versions) with: monitor ERROR cannot start
maintenance: ... current state for primary node 1 is "wait_primary" ->
"wait_primary".
Root cause: node2 is this formation's only standby, so once the new
monitor-side ancestry guard (added earlier this branch) excludes it
right after test_003's accept_timeline pin, node1 must also drop
synchronous replication -- it transitions primary -> wait_primary at the
same moment. The old test_004 called "enable maintenance" on node2
immediately after test_003 with nothing in between, racing node1's own
transition back to primary about half the time.
The 3-node companion spec never hit this: excluding one of two
standbys there doesn't force the primary to touch synchronous
replication at all, since a second real secondary is still available.
Fix: add test_004_automatic_catchingup_without_maintenance (mirroring
the 3-node spec's test_005) that both asserts the new automatic
detection directly and waits for node1/node2 to fully settle back to
primary/secondary before the next step touches maintenance --
renumbering the old test_004/test_005 to test_005/test_006. Updated the
header comment and test_003's own comment, both of which described the
pre-guard behavior ("goes unnoticed until forced") that's no longer
accurate.
Verified: 4/4 clean runs locally (previously ~50% failure), plus the
3-node spec re-run once more to confirm no regression there.
tests/tap/specs/multi_ifdown.pgaf's failover to a lagging standby caught
up via fast_forward (peer-to-peer WAL fetch, not from the primary) could
silently fail to promote while reporting success, leaving the candidate
stuck on its old timeline -- indistinguishable by timeline number from a
demoted former primary, and a genuine risk of electing a node that never
actually advanced.
Root causes, found by live reproduction against real container logs
rather than guesswork:
* fast_forward configured Postgres's own recovery_target_lsn to an
arbitrary snapshot of a peer standby's current replay position. That
position isn't guaranteed to land on a genuine WAL record boundary, so
Postgres could get stuck permanently 'waiting for WAL to become
available' at that exact LSN, or FATAL-crash if promoted while still in
that state ('recovery ended before configured recovery target was
reached'). fast_forward already had its own application-level polling
loop (pgsql_has_reached_target_lsn) to decide when enough WAL had been
fetched; asking Postgres to also independently enforce the same
threshold via recovery_target_lsn was both redundant and unsafe. The
new ReplicationSource.pauseAtRecoveryTarget flag makes that opt-in,
defaulting to off; fast_forward now just streams normally and decides
for itself when to promote, exactly like the non-fast_forward
promotion path already does. The one legitimate existing use
(crash-recovery's 'immediate' target, always a well-defined,
reachable point) opts back in explicitly.
* Independently, the FAST_FORWARD_STATE -> PREP_PROMOTION_STATE
transition (fsm_cleanup_as_primary) removed standby.signal from disk
well before the real pg_ctl promote (issued later, from
PREP_PROMOTION_STATE -> STOP_REPLICATION_STATE). Postgres's own
promotion-completion logic removes standby.signal itself as part of
processing the promote request, and FATAL-crashes if it's already
gone ('could not remove file "standby.signal": No such file or
directory'). fsm_cleanup_as_primary now leaves the on-disk standby
setup untouched; the already-correct post-promotion cleanup in
fsm_promote_standby (via standby_cleanup_as_primary) is the only place
that needs to run it.
* standby_cleanup_as_primary itself no longer restarts Postgres: restarting
after standby.signal has been removed, but before a genuine promotion
took place, makes Postgres boot as an ordinary (non-standby) server and
complete plain crash recovery on its current timeline -- reaching a
not-in-recovery state without ever going through the real promotion code
path, which is exactly the silent-corruption failure mode this commit
closes.
Defense in depth, in case a future change reintroduces a similar race:
* standby_promote() now calls the new standby_promotion_advanced_timeline
to verify, via the local pg_wal/<TLI>.history file, that a claimed
promotion genuinely advanced to a new timeline with a recorded
ancestor -- not just a bare, unverified 'not in recovery' state -- both
on its idempotent early-return path and after a real pg_ctl promote.
* fsm_init_primary and fsm_promote_standby now publish the node's fresh
timeline history to the monitor immediately after promotion
(keeper_defensive_publish_timeline_history), rather than waiting for
the next periodic tick, so other nodes and 'pg_autoctl show timeline'
observe it as soon as it's genuinely confirmed.
* standby_fetch_missing_wal and standby_promote now tell the separate
Postgres controller service (service_postgres_ctl.c) to stand down
(keeper_set_postgres_state_unknown/running) for the duration of their
own waits, so its independent 100ms liveness poll can't race a
transient unavailability during recovery/promotion into an unplanned
restart of its own.
Verified with repeated clean runs of multi_ifdown.pgaf (previously a
deterministic 300s timeout on test_008_failover), confirming node3's
genuine on-disk timeline advance (pg_walfile_name, pg_wal/*.history) and
correct 'pg_autoctl show timeline' reporting across all nodes, plus
timeline_fork_report_lsn_deadlock.pgaf and timeline_fork_3node_auto_detect.pgaf
regressions.
…_timeline_with_upstream standby_check_timeline_with_upstream's same-timeline branch previously treated 'upstream reports the same timeline number we're on' as sufficient proof of a shared history and returned success immediately. That mirrors pg_rewind's own well-documented limitation (and a matching, still-open Patroni issue for the identical scenario): a bare timeline-number match doesn't rule out genuine divergence when a node was promoted or written to outside of pg_autoctl's control while carrying the same timeline number as its nominal upstream. A genuine, unbroken standby can never get ahead of its own upstream in LSN terms while remaining on the exact same timeline -- ordinary streaming replication only ever replays WAL the upstream itself produced. So this adds one more check: when the timelines match, also compare the node's local LSN against the upstream's own reported current LSN. If local is ahead, that's the same genuine divergence #683 already handles for differing timeline numbers, just expressed here as same-number-but-diverged instead -- so it's resolved the same way, via primary_rewind_to_standby, followed by a metadata refresh and a re-check that the rewind actually landed us on the upstream's timeline.
…Citus workers fsm_citus_cleanup_and_resume_as_primary() is the Citus counterpart of fsm_cleanup_as_primary(), both firing at the FAST_FORWARD_STATE -> PREP_PROMOTION_STATE transition, i.e. before the real pg_ctl promote (issued later, from standby_promote() via fsm_promote_standby). The non-Citus path was already fixed in f375443/6462149 to leave the on-disk standby setup untouched at that point, but the Citus counterpart still called standby_cleanup_as_primary() (and, until just now, additionally restarted Postgres) at this same early call site. Live reproduction against a new debug spec (tests/tap/specs/ debug_citus_worker_fast_forward.pgaf, a 3-node Citus worker group mirroring multi_ifdown.pgaf's setup) showed Postgres FATAL-crashing during the actual promote: LOG: selected new timeline ID: 2 FATAL: could not remove file "standby.signal": No such file or directory LOG: shutting down due to startup process failure Postgres's own promotion-completion logic removes standby.signal itself while processing the promote request, and crashes if it's already gone. The crash-recovery restart that follows finds no standby.signal (already removed) and silently completes ordinary crash recovery of what looks like a primary, without ever genuinely promoting -- indistinguishable by timeline number from a demoted former primary. standby_promote()'s standby_promotion_advanced_timeline guard (from f375443) caught the resulting corruption and refused to proceed, but the underlying crash kept recurring on every retry. Fix: fsm_citus_cleanup_and_resume_as_primary() no longer touches the on-disk standby setup at all; it only prepares the coordinator-side master_update_node() call, exactly mirroring fsm_cleanup_as_primary()'s current no-op behavior. The already-correct post-promotion cleanup in fsm_promote_standby (shared with the non-Citus path) remains the only place that runs standby_cleanup_as_primary(). Verified with a clean run of debug_citus_worker_fast_forward.pgaf (previously a deterministic crash/300s timeout on this same repro), confirming worker1c's genuine promotion to timeline 2, plus citus_multi_standbys.pgaf (14/14) and citus_basic_operation.pgaf (14/14) regressions.
Pre-existing citus_indent violation in main's fsm_mermaid.c (added by #1164), surfaced by rebasing this branch onto origin/main: citus_indent wants a second blank line between FsmMermaidColorForState() and the following function comment, matching the two-blank-line convention used throughout the rest of the codebase. Caught by CI's 'Style check' job on the PR-merge ref (pull/1161/merge), which combines this branch with current main -- this file doesn't exist on this branch's pre-rebase history, so `make docker-check` only started failing locally once the rebase brought it in.
dimitri
force-pushed
the
timeline-fork-detection
branch
from
July 27, 2026 12:33
c9f3105 to
0f70556
Compare
Root-caused from CI: pytest / citus (PG16) and (PG17) both timed out at the 15-minute job-step ceiling in this branch's CI, despite every actual test assertion passing (76 passed). The failure was entirely in teardown_module()'s cluster.destroy(). Datanode.destroy() called self.stop_pg_autoctl() -- default SIGTERM -- which triggers pg_autoctl's graceful-shutdown maintenance handoff (keeper_graceful_shutdown() in service_keeper.c): for a primary with a viable standby, up to KEEPER_MAINTENANCE_SHUTDOWN_LOOP_MAX_SECS (30s) trying to reach maintenance, falling back to up to KEEPER_SHUTDOWN_LOOP_MAX_SECS (30s) reporting state to the monitor while Postgres stops -- 60s worst case, defined in defaults.h. That's exactly this module's own COMMAND_TIMEOUT (network.py), with zero margin for process/network/sudo overhead. test_citus_skip_pg_hba.py's coordinator (coord0a, primary, with standby coord0b) hit that ceiling, and stop_pg_autoctl() raised subprocess.TimeoutExpired with a margin of literal microseconds -- not random flakiness, two equal constants racing. Worse, that call ran *before* the try/except in Datanode.destroy() that respects ignore_failure, so the exception propagated straight out of Cluster.destroy()'s reversed-order loop and aborted it mid-iteration. Since coord0a is last in that order, every node destroyed cleanly except it -- and the abort meant self.monitor.destroy() and self.vlan.destroy() (the test network namespace teardown) never ran at all. The monitor's pg_autoctl process was left running as an orphan, holding the test container open until GitHub's 15-minute step timeout force-killed it, long after pytest itself had already printed its own summary and exited. There's no standby to protect when tearing down an entire test cluster, so there's no reason to pay for the graceful maintenance handoff here. SIGINT skips it entirely (keeper_graceful_shutdown() only runs on a plain SIGTERM -- service_keeper.c checks `asked_to_stop && !asked_to_stop_fast && !asked_to_quit`) while still stopping Postgres cleanly: for anything other than SIGTERM, supervisor_stop_subprocesses() forwards the signal to every service directly (not just the keeper), so the Postgres-controller sibling gets it too and calls ensure_postgres_status_stopped() on its own, same as it always does. The top-level supervisor process itself uses exitOnQuit=false (supervisor_init()), so SIGINT/SIGQUIT there still goes through the ordinary loop and properly reaps its children -- no new orphans introduced. SIGINT specifically (not SIGQUIT) because destroy() is a deliberate teardown, not a crash simulation -- that distinction is already documented on DataNode.fail()'s own default. Datanode.destroy() and MonitorNode.destroy() now both pass sig=SIGINT explicitly (stop_pg_autoctl()'s own default stays SIGTERM, still needed by tests wanting a plain graceful restart), and both wrap their stop call in the try/except that already existed for the rest of destroy() -- so a future timeout for an unrelated reason can no longer abort the rest of cluster teardown either.
dimitri
added a commit
that referenced
this pull request
Jul 27, 2026
… rebase origin/main merged PR #1161 (timeline_fork_detection), which now runs before this test in regress_schedule and consumes 6 more ids from the shared node-id sequence. This test's expected.out hardcoded the literal assigned_node_id values (24/25) from register_node's return row, per the schedule file's own documented caveat about every test needing the whole node-id sequence to itself in a fixed order. Regenerated against the actual node ids now assigned (31/32); no other part of the test changed. Verified: all 14 pg_regress + 6 isolation tests pass locally via 'make installcheck'.
dimitri
added a commit
that referenced
this pull request
Jul 27, 2026
…ng failover (#1165) * Fix #774: monitor can assign a primary an unreachable goal state during failover Two rules in group_state_machine.c could combine to assign the primary goal state demote_timeout while it is sitting at wait_primary. There is no KeeperFSM[] edge from wait_primary to demote_timeout, so the keeper fatals forever with "does not know how to reach state ... from ...". Rule 1 ("no healthy standby in the quorum"): 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. Guarded with !IsFailoverInProgress(). Rule 2 ("prepare_promotion -> stop_replication"): assigns the primary demote_timeout as soon as the candidate reports prepare_promotion, unconditionally. demote_timeout has a real KeeperFSM[] edge from primary/join_primary/apply_settings/draining -- but not from wait_primary. Guarded to only fire when the primary's reported state is actually one of those four. Adds a pg_regress test (failover_candidate_leaves_secondary) isolating each rule via direct state manufacture, following the same technique stale_primary_report.sql already uses for hard-to-reach states. Verified via authoritative `make installcheck` (pg_regress in Docker, all 13 monitor tests + isolation tests pass) and by empirically reproducing both bugs pre-fix and confirming both fixes independently load-bearing (temporarily reverting each and observing the failure reproduce). * Fix a second #774 gap: candidate stalls forever if the primary died before ever reaching PRIMARY Found via the actual CI failure (pgaftest multi-async, test_014_003_ restart_node1): when a primary is killed while still at wait_primary (never got a healthy secondary counted before being promoted the rest of the way), GetPrimaryOrDemotedNodeInGroupFromList still resolves it as primaryNode (its goal state, wait_primary, is writable). The previous fix's Rule 2 guard correctly refuses to assign it demote_timeout (no KeeperFSM[] edge from wait_primary), but the primaryNode == NULL fallback never fires either, since primaryNode is non-NULL -- the candidate is left stuck at prepare_promotion forever. Broadened the fallback rule to also fire whenever primaryNode exists but is not in a state with a real demote_timeout edge (mirroring the guard's own restricted set): there is no live primary to hand off to either way, so promote the candidate directly, exactly as when primaryNode is NULL. Updated failover_candidate_leaves_secondary.sql's Scenario B to assert the corrected behavior: the candidate now reaches wait_primary directly instead of deferring. Verified via: - `make installcheck` (pg_regress in Docker): all 13 monitor tests + isolation tests pass. - Reproduced the CI hang locally with own-tagged Docker images (pgaf774dbg:pg17 / pgaf774dbg:pgaftest) running the actual multi-async pgaftest schedule; confirmed test_014_003_restart_node1 now passes (previously hung for the full 180s timeout). * Guard both prepare_promotion rules on the primary being "settled" (converged or unhealthy), not just its reportedState The two rules that decide what to do with a candidate converged at prepare_promotion both act on primaryNode's reportedState to choose between the demote_timeout path and the direct-promotion fallback. Neither previously checked whether primaryNode was actually done changing: a primary that is healthy and legitimately still converging toward a real "primary" (goalState already PRIMARY, e.g. after a replication-quorum change, but reportedState still lagging one step behind at a real predecessor like apply_settings) could have its lagging reportedState misread as "no live primary", triggering the fallback and promoting a second candidate straight to wait_primary while the first primary was seconds away from completing its own promotion on its own -- two simultaneously-writable nodes, which is worse than the unreachable-goal-state bug (#774) these rules exist to prevent. Both rules now require primaryNode to be "settled" before acting on its reportedState at all: either primaryNode == NULL, or it has fully converged to its own currently assigned goal (reportedState == goalState), or it is confirmed unhealthy (NodeIsUnhealthy) and therefore cannot make further progress toward that goal regardless. If primaryNode is healthy but still mid-transition, neither rule fires this tick; the next node_active() round re-evaluates once it either converges or is confirmed unhealthy. Also fixed a maintenance-handling regression introduced while merging the two rules' guards: a primaryNode in maintenance must defer entirely (as before), not fall through to the direct-promotion fallback just because it doesn't match the demote_timeout-reachable state set. Added Scenario C to failover_candidate_leaves_secondary.sql, manufacturing exactly the healthy-but-unconverged primary case and asserting the candidate's goal is left untouched at prepare_promotion (deferred) rather than hastily promoted. Verified via `make installcheck` (pg_regress in Docker): all 13 monitor tests (including the new Scenario C) + isolation tests pass. Ran citus_indent (docker run citus/stylechecker:no-py) and reformatted accordingly; re-verified full suite passes after reformatting. * Revert to Fix 1 only: the "unreachable demote_timeout" restriction and everything built on it were solving a self-inflicted regression, not the real #774 bug Built and ran the exact CI reproduction against a fresh, completely unmodified origin/main worktree: all 27 multi-async tests pass, including the "primary dies at wait_primary" scenario this whole series of fixes was chasing (test_014_003_restart_node1 takes ~35s via the pre-existing drain-timeout escape hatch, then test_014_004 resolves cleanly with no split-brain). Root cause of the confusion: main already has a working, purely-time-based mechanism for this exact case (NodeIsDrainTimeExpired + the "demote timeout expired" rule) that reassigns a stuck-at-wait_primary primary to demoted -- a state genuinely reachable from wait_primary in KeeperFSM[] (added in #707, before #774 was even filed). The second commit in this branch ("Fix #774: monitor can assign a primary an unreachable goal state") restricted the demote_timeout-assigning rule to require the primary's reportedState be in a specific reachable set. That inadvertently prevented the primary's goal from ever becoming demote_timeout in the wait_primary case at all -- which is the precondition NodeIsDrainTimeExpired checks (node->goalState != DEMOTE_TIMEOUT_STATE => false). That broke the escape hatch's trigger, producing the "candidate hangs at prepare_promotion forever" bug the next two commits in this branch were built to fix -- via a new fallback path that promotes the candidate directly but never reconciles the old primary's own row, producing an actual split-brain (two simultaneously-writable nodes) that does not exist on main. Re-reading the original #774 issue thread confirms the actual reported bug is a *race*, not a permanent unreachable-state hang: a manual perform_failover can have the "no healthy standby in the quorum" rule (Rule 1) reassign the primary to wait_primary in the same window a separate rule assigns it demote_timeout, exactly matching the issue reporter's own diagnosis ("the monitor should not continue with the usual failover transition" when it just reassigned wait_primary). Fix 1 (IsFailoverInProgress guard on Rule 1, kept) directly prevents this race. The keeper's FATAL-and-retry behavior for a genuinely unreachable assignment is not itself a crash (log_fatal is a severity label only, per src/bin/lib/log/src/log.h -- the keeper loops and retries harmlessly) and should stay as-is: the monitor must never hand out an unreachable transition, full stop, and the keeper is right to refuse loudly if it ever does. Reverted group_state_machine.c's prepare_promotion handling back to origin/main's version, then reapplied only Fix 1 (the IsFailoverInProgress guard in ProceedGroupStateForPrimaryNode) -- diffed against origin/main to confirm this is now the only change. Trimmed failover_candidate_leaves_secondary.sql back to Scenario A (Fix 1's test) only; removed Scenarios B/C, which tested the now-reverted code. Verified via: - `make installcheck` (pg_regress in Docker): all 13 monitor tests + isolation tests pass. - citus_indent (docker run citus/stylechecker:no-py): clean. - Full multi-async pgaftest schedule against this exact code (own local image tags): all 27 tests pass, matching origin/main's baseline behavior exactly (including the ~35s drain-timeout path for test_014_003, and a clean test_014_004 with no split-brain). * tests: harden transient-connectivity retries for post-restart queries - has_needed_replication_slots(): bump retry_timeout 5s -> 15s, the existing 0.5s-backoff retry loop wasn't always covering a keeper- triggered Postgres restart under a loaded CI runner (test_multi_ifdown.py::test_003_add_standby saw a bare Connection refused). - test_citus_multi_standbys.py: use the existing run_sql_query_retry() helper for worker2c instead of a bare run_sql_query(), which had no retry at all (test_003_002_all_workers_have_data saw the same). * Regenerate expected.out for failover_candidate_leaves_secondary after rebase origin/main merged PR #1161 (timeline_fork_detection), which now runs before this test in regress_schedule and consumes 6 more ids from the shared node-id sequence. This test's expected.out hardcoded the literal assigned_node_id values (24/25) from register_node's return row, per the schedule file's own documented caveat about every test needing the whole node-id sequence to itself in a fixed order. Regenerated against the actual node ids now assigned (31/32); no other part of the test changed. Verified: all 14 pg_regress + 6 isolation tests pass locally via 'make installcheck'.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements timeline-fork detection for pg_auto_failover, fixing #683 ("Unrecoverable state after failed switchover") and adding the machinery to detect, diagnose, and operator-resolve genuine timeline forks — a standby whose local WAL has diverged onto a dead branch, which can never resolve via ordinary streaming replication.
Parts 1-4 (design)
standby_check_timeline_with_upstream/standby_verify_timeline_ancestryinprimary_standby.c): a standby reconnecting to a new upstream no longer trusts a bare TLI-number comparison. It walks the upstream's realTIMELINE_HISTORYto tell "still catching up" apart from "diverged onto a dead branch," andpg_rewinds in the latter case.pgautofailover.node_timeline_history(append-only per-node timeline lineage, published periodically and defensively before risky restarts) + the election'sFilterNodesByTimelineAncestry(), which excludes genuinely diverged candidates fromProceedGroupStateForMSFailoverrather than counting them as "missing" and blocking the whole election forever.pgautofailover.accepted_timeline+pg_autoctl accept timeline: an explicit operator pin overriding the auto-detected reference lineage when a genuine fork needs a human call, auto-resolved once a primary promotes on the accepted lineage.pg_autoctl show timeline: prints the group's known timeline history and each node's status against the reference lineage, flagging forks.The #683 fix
Root-caused via manual reproduction against a live Docker cluster (network-partition a standby, promote it directly at the Postgres level to fork it, reconnect, force it back through
catchingup):standby_check_timeline_with_upstream()only knew how to rewind a standby that was behind its upstream. When local tli > upstream tli (a standby that generated WAL after being promoted outside of pg_autoctl's control), it just logged an error and returned false — forever. That's the literal "unrecoverable state" from Unrecoverable state after failed switchover #683. Fixed by rewinding symmetrically in that direction too.assigned_role == SECONDARY_STATE, butfsm_follow_new_primary()is also used forSECONDARY_STATE → CATCHINGUP_STATE— an ordinary health-check-driven resync — which skipped the check entirely. Broadened the guard to cover both.node_timeline_historyat all.Verified pre-fix (infinite "waiting for WAL to become available" retry loop) vs. post-fix (rewind, or
pg_basebackupfallback whenpg_rewinditself can't connect — both already-existing recovery paths) viatests/tap/specs/timeline_fork_report_lsn_deadlock.pgaf.Also fixes a separate bug the tests caught:
node_timeline_history's FK tonode.nodeidlackedON DELETE CASCADE, breaking node removal for any node that had ever published timeline history.Testing
src/monitor/sql/timeline_fork_detection.sql,make installcheckon PG17+PG19): unit-level coverage ofreport_timeline_history(),node_timeline_status()ancestry computation (unpinned/pinned/resolved),accept_timeline()validation, plus an end-to-end 4-node election test where an operator pin flips the winner and excludes a sibling branch, auto-resolving on promotion.tests/tap/specs/timeline_fork_report_lsn_deadlock.pgaf, 5 steps): forks a standby out-of-band, confirms the fork is durably published, exercisesshow timeline/accept timelineagainst a live cluster, forces the real recovery path (validates the Unrecoverable state after failed switchover #683 fix end-to-end), confirms writes flow again post-recovery.basic_operation,multi_standbys,guard_data_loss,multi_maintenance,multi_async,multi_ifdown,drop_node_destroy,ensure,demote_timeout_wait_primary_deadlock,config_get_set, plus the full monitor installcheck — all green against the modified election/reconnect code paths (fsm_report_lsn,fsm_prepare_for_secondary,fsm_follow_new_primary).CI schedule audit
Cross-checked every
.pgafspec against what CI's.schshards actually run (not the flattests/tap/schedulereference file, which CI doesn't read) and found 4 real specs that existed but never ran in CI:guard_data_loss,replication_stall_3dc,demote_timeout_wait_primary_deadlock(#1025), and this branch'stimeline_fork_report_lsn_deadlock(#683). Added them tomulti-misc.sch/node.schand verified both schedules pass in full.Not included:
fast_forward.pgaf. Making it actually exercise the realfast_forwardFSM state (it previously didn't — see commit history) surfaced a genuine, pre-existing, unrelated bug:standby_fetch_missing_wal()blocks in an unbounded loop with no timeout or WAL-source liveness check, so the monitor's own issue #1060 stuck-detection guard can never reach a keeper that's already committed to fetching. That's a real fix, not a test-file fix, and is being picked up on a separate branch/PR.Manual verification
Docker image rebuilt and re-tested at each step; the #683 scenario was reproduced and verified fixed against a live 2-node cluster before being captured as an automated spec.