Skip to content

monitor: fix NodeIsHealthy and stale struct in NodeActive, add SQL regression test - #1141

Merged
dimitri merged 14 commits into
mainfrom
monitor/node-active-context
Jul 11, 2026
Merged

monitor: fix NodeIsHealthy and stale struct in NodeActive, add SQL regression test#1141
dimitri merged 14 commits into
mainfrom
monitor/node-active-context

Conversation

@dimitri

@dimitri dimitri commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Two bugs in the monitor's FSM that can cause spurious failovers or block
node recovery, plus a SQL regression test that exercises both on every CI
run via make installcheck.

Bug 1 — NODE_HEALTH_UNKNOWN blocks bootstrap (#1032 adjacent)

NodeIsHealthy() in node_metadata.c only handled GOOD and BAD
health values. Newly registered nodes have health = -1
(NODE_HEALTH_UNKNOWN) because the health-check worker has not run yet.
The function fell through to returning false, which permanently blocked
the catchingup → secondary transition during formation bootstrap (the FSM
guard requires NodeIsHealthy(joiningNode)).

Fix: add an UNKNOWN branch that trusts the keeper's own
pgIsRunning report, consistent with how BAD-with-fresh-report works.

Bug 2 — Spurious failover when primary has BAD health but is alive (Fixes: #1062)

When the health-check worker cannot reach the primary (marks it BAD)
but the primary is still calling node_active with pgIsRunning=true,
NodeIsHealthy returned false for health=BAD unconditionally. The
existing fresh-report override — healthchecktime < reportTime and
reportTime within nodeActiveCallsFrequencyMs — was supposed to save
it, but the UNKNOWN fall-through masked it. With the UNKNOWN fix in
place the BAD-with-fresh-report path now fires correctly, keeping the
primary in primary state instead of triggering a needless failover.

Bug 3 — Stale in-memory struct blocks recovery after failover (Fixes: #1032)

NodeActive() in node_active_protocol.c calls:

  1. GetAutoFailoverNodeById() → loads node from DB into struct
  2. ReportAutoFailoverNodeState() → writes pgIsRunning/reportTime to DB but not back to the struct
  3. ProceedGroupState(pgAutoFailoverNode) → BuildGroupStateContext stores the passed-in struct as ctx->activeNode; NodeIsHealthy reads ctx->activeNode->pgIsRunning

So when a recovering node transitions from pgIsRunning=false (demoted)
to pgIsRunning=true (catchingup), ProceedGroupState saw the stale
false value and NodeIsHealthy returned false, permanently blocking
catchingup → secondary.

Fix: sync pgIsRunning, reportedTLI, and reportTime from the
current report back into the struct after ReportAutoFailoverNodeState
and before ProceedGroupState.

Testing

Replaces two scenario-mode .pgaf specs (which required the full pgaftest
Docker stack) with a single SQL regression file run by pg_regress:

make installcheck          # PG16 default
make installcheck INSTALLCHECK_PGVERSION=17

Builds the test Docker image and runs all monitor SQL regression tests
inside a fresh container. The node_active_protocol test covers:

Fixes: #1062
Fixes: #1032

@dimitri dimitri self-assigned this Jul 11, 2026
@dimitri dimitri added the bug Something isn't working label Jul 11, 2026
dimitri added 6 commits July 11, 2026 02:41
@dimitri
monitor: extract GroupStateContext + pure health predicates
d5a159b 
Introduce GroupStateContext, a struct that bundles every input the
node_active protocol FSM needs:

  - activeNode, groupNodeList (loaded once from the DB)
  - formation (loaded once instead of twice)
  - now (single TimestampTz snapshot instead of per-call GetCurrentTimestamp())
  - GUC copies: unhealthyTimeoutMs, drainTimeoutMs, startupGracePeriodMs

BuildGroupStateContext() does a single DB read phase, replacing the
redundant AutoFailoverNodeGroup() call in ProceedGroupStateForMSFailover
and the second GetFormation() call in ProceedGroupStateForPrimaryNode.

ProceedGroupState() is now a thin wrapper that builds the context and
delegates to ProceedGroupStateFromContext().  The latter is exported so
test code can populate a context from fixtures and exercise the FSM
without a live DB connection.

Add NodeIsHealthy/NodeIsUnhealthy/NodeIsReporting/NodeIsDrainTimeExpired
alongside the existing IsHealthy/IsUnhealthy/IsReporting/IsDrainTimeExpired
shims.  The new variants take a const GroupStateContext * instead of
calling GetCurrentTimestamp() or reading GUC globals, making the FSM
decisions deterministic for a given context snapshot.  All call sites
inside group_state_machine.c now use the pure variants.
@dimitri
pgaftest: add node_active protocol test commands
0fd047b 
Add two new DSL commands for testing the monitor's node_active protocol:

  node_active { node2  reported: secondary  lsn: 0/5A0  tli: 1  pgrunning: true }
      expect { assigned: secondary }

  mark healthy: node2
  mark unhealthy: node3

node_active calls pgautofailover.node_active() on the monitor container
via psql, then asserts the returned assigned_group_state matches the
expected value in the expect block.  The block content is read as a raw
string and key-value pairs are parsed in C, keeping the grammar simple.

mark healthy/unhealthy directly updates the health column in
pgautofailover.node, mirroring what the health-check worker does, so
tests can place a node in a known health state before calling node_active.

Changes:
  test_spec.h          — CMD_NODE_ACTIVE, CMD_MARK_HEALTH, new TestCmd fields
  test_spec_scan.l     — keywords: node_active, mark, healthy, unhealthy, ':'
  test_spec_parse.y    — node_active_cmd and mark_health_cmd grammar rules
  test_runner.c        — execute + describe logic for both new commands
@dimitri
tests: add node_active_protocol.pgaf sample test
7cfd865 
Demonstrates the new node_active { ... } expect { ... } and
mark [un]healthy: syntax against a two-node formation.

The test drives a full primary-failure-and-recovery scenario through
direct FSM calls to the monitor, without waiting for the keeper
heartbeat cycle.  Each step asserts the monitor assigns the expected
next state given the reported node conditions.
@dimitri
monitor: fix stale in-memory struct and NodeIsHealthy for scenario mode
73e0853 
Three bugs fixed that blocked the pgaftest scenario-mode bootstrap:

1. node_active_protocol.c: sync pgIsRunning, reportedTLI, and reportTime into
   the in-memory AutoFailoverNode struct after ReportAutoFailoverNodeState.
   ProceedGroupState->NodeIsHealthy reads pgIsRunning and reportTime from the
   struct, not from the DB.  With the stale values from the previous call the
   health predicate saw pgIsRunning=false even when the keeper was reporting
   pgRunning=true, which permanently blocked catchingup->secondary.

2. node_metadata.c NodeIsHealthy: handle NODE_HEALTH_UNKNOWN (-1), the
   initial health value for newly registered nodes (set by the table DEFAULT).
   The health-check worker has not run yet for these nodes; trust the keeper's
   own pgIsRunning report rather than returning false.

3. pgaftest test_runner.c CMD_MARK_HEALTH: run the health UPDATE as the
   docker OS user (PostgreSQL superuser via peer auth) because autoctl_node
   lacks write permission on pgautofailover.node.  Also corrected the health
   value: NODE_HEALTH_BAD=0, not -1 (which is UNKNOWN).

Also updates the scenario-mode bootstrap in both specs to use pgrunning:true
throughout setup.  The catchingup->secondary rule requires NodeIsHealthy of
the joining node, which in turn needs pgIsRunning=true to have been written
to the DB by the previous call.  Virtual nodes in setup have no real Postgres
process; setting pgrunning:true is the correct semantic (the keeper would
report true once replication starts).

Tested: node_active_protocol.pgaf (3/3) and monitor_fsm_health.pgaf (4/4)
both pass clean.
@dimitri
monitor: replace scenario-mode pgaf specs with SQL regression test
baf3434 
The two .pgaf scenario files (node_active_protocol.pgaf and
monitor_fsm_health.pgaf) exercised the monitor's node_active() FSM through
pgaftest's scenario mode, which spins up a Docker monitor container and drives
it via a custom DSL runner.  The same test coverage is provided more simply by
a pg_regress SQL file that calls the SQL functions directly.

The comparison:

  scenario .pgaf              SQL regression
  ─────────────────────────── ───────────────────────────────────────────
  Docker + compose required   pg_regress (ships with PostgreSQL)
  ~20 s startup               < 1 s
  mark healthy/unhealthy via  UPDATE pgautofailover.node SET health = 0
    docker exec + psql          WHERE nodename = 'node1'
  goal-state string only      exact diff of all returned columns
  custom DSL runner (~10k C)  plain SQL

The new src/monitor/sql/node_active_protocol.sql:

- Bootstraps a two-node 'fsm_test' formation through all FSM states,
  ending at primary/secondary.  This exercises the NODE_HEALTH_UNKNOWN fix
  (NodeIsHealthy must trust pgIsRunning when health=-1, the table default
  for newly registered nodes).

- Verifies issue #1062: health=BAD + pgIsRunning=true keeps primary assigned
  primary (fresh-report override in NodeIsHealthy).

- Drives a complete two-node failover (primary → prepare_promotion →
  stop_replication → wait_primary; old primary → demote_timeout → demoted).

- Verifies issue #1032: old primary recovers with health still BAD; reports
  catchingup with pgIsRunning=true; the stale in-memory struct fix ensures
  ProceedGroupState sees the updated pgIsRunning, and the fresh-report
  override in NodeIsHealthy allows catchingup→secondary.

The C fixes from the previous commit are unchanged:
  src/monitor/node_metadata.c      NodeIsHealthy: handle NODE_HEALTH_UNKNOWN
  src/monitor/node_active_protocol.c  sync pgIsRunning/reportedTLI/reportTime
                                      into struct after ReportAutoFailoverNodeState

The pgaftest files reverted to their state at fc09d6c (before scenario-mode
DSL commands were added):
  src/bin/pgaftest/test_runner.c
  src/bin/pgaftest/test_spec.h
  src/bin/pgaftest/test_spec_parse.{c,h,y}
  src/bin/pgaftest/test_spec_scan.{c,l}
  src/bin/pgaftest/compose_gen.c
@dimitri
monitor: add make installcheck target and fix regression test
98f247f 
Add a top-level 'make installcheck' target that builds the PG16 test
Docker image and runs the monitor's SQL regression suite (pg_regress)
inside a fresh container.  Defaults to PGVERSION=16; override with
INSTALLCHECK_PGVERSION=17 etc.

Fix node_active_protocol.sql to work in the full regression sequence:
- Pass number_sync_standbys to create_formation() (5-arg form)
- Capture registered node IDs via \\gset after registration so the test
  is not sensitive to how many nodes prior tests (monitor, workers) created
- Move node_active_protocol before dummy_update in REGRESS order to avoid
  running against a downgraded 'dummy' extension version

Update expected/node_active_protocol.out from the actual pg_regress output.
@dimitri
dimitri force-pushed the monitor/node-active-context branch from 0416585 to 98f247f Compare July 11, 2026 00:42
dimitri added a commit that referenced this pull request Jul 11, 2026
@dimitri
monitor: guard TLI=0 in struct sync to fix event_reportedtli_check
381043d 
When syncing the in-memory pgAutoFailoverNode struct after
ReportAutoFailoverNodeState(), do not overwrite reportedTLI with 0.

A keeper in wait_standby state has not yet started streaming and
legitimately reports reportedTLI=0.  ReportAutoFailoverNodeState()
already handles this with a CASE WHEN 0 THEN reportedtli ELSE $4 END
guard in its UPDATE, so the DB value stays at the default of 1.  But
the previous sync code wrote currentNodeState->reportedTLI directly
into the struct, setting it to 0.

InsertEvent() in notifications.c passes node->reportedTLI directly to
the INSERT without that CASE guard.  The pgautofailover.event table
carries check (reportedtli > 0), so every state-change notification
for a freshly registered secondary failed with:

  ERROR: new row for relation "event" violates check constraint
  "event_reportedtli_check"

This blocked all failover-related CI tests.

Fix: only update the struct's reportedTLI when the reported value is
non-zero, matching the DB's CASE logic.

Fixes #1141
@dimitri
monitor: guard TLI=0 in struct sync to fix event_reportedtli_check
9600417 
When syncing the in-memory pgAutoFailoverNode struct after
ReportAutoFailoverNodeState(), do not overwrite reportedTLI with 0.

A keeper in wait_standby state has not yet started streaming and
legitimately reports reportedTLI=0.  ReportAutoFailoverNodeState()
already handles this with a CASE WHEN 0 THEN reportedtli ELSE $4 END
guard in its UPDATE, so the DB value stays at the default of 1.  But
the previous sync code wrote currentNodeState->reportedTLI directly
into the struct, setting it to 0.

InsertEvent() in notifications.c passes node->reportedTLI directly to
the INSERT without that CASE guard.  The pgautofailover.event table
carries check (reportedtli > 0), so every state-change notification
for a freshly registered secondary failed with:

  ERROR: new row for relation "event" violates check constraint
  "event_reportedtli_check"

This blocked all failover-related CI tests.

Fix: only update the struct's reportedTLI when the reported value is
non-zero, matching the DB's CASE logic.

Fixes #1141
@dimitri
dimitri force-pushed the monitor/node-active-context branch from 381043d to 9600417 Compare July 11, 2026 01:33
dimitri added 7 commits July 11, 2026 14:29
@dimitri
monitor: fix IsHealthy for NODE_HEALTH_UNKNOWN; add regression tests
8492f37 
Two bugs, two new test scenarios:

1. IsHealthy() / CountHealthyCandidates() inconsistency (RC1)

   NodeIsHealthy() (used by the FSM since this PR) treats
   NODE_HEALTH_UNKNOWN as 'trust pgIsRunning', allowing
   catchingup→secondary to fire before the health-check worker runs.
   But the old IsHealthy() (still called by CountHealthyCandidates and
   IsHealthySyncStandby) returned false for UNKNOWN, so a secondary that
   reached SECONDARY via the new fast path was invisible to
   start_maintenance, which then reported '0 candidate nodes available'.

   Fix: add the same NODE_HEALTH_UNKNOWN branch to IsHealthy() so that
   CountHealthyCandidates counts reachable-but-not-yet-health-checked
   secondaries as candidates.

   test_005 in node_active_protocol.sql: force node1 health=-1 after it
   reaches SECONDARY, then call start_maintenance on the primary.  Must
   return true (previously it errored).

2. Killed-primary failover path not tested (RC3)

   test_003 exercises the self-reported-down path (primary calls
   node_active with pgIsRunning=false).  The container-killed path
   (primary stops reporting; pgIsRunning stays TRUE in DB; health checker
   marks it BAD after unhealthyTimeoutMs) was not covered.

   test_006 in node_active_protocol.sql: fresh 'killed_test' formation,
   bootstrap to primary+secondary, then UPDATE the primary's row to set
   health=BAD and reporttime=60s ago (without touching pgIsRunning).
   Secondary's next node_active must receive prepare_promotion via the
   time-based NodeIsUnhealthy path.  startup_grace_period is temporarily
   set to 1ms so the condition fires immediately in the test environment.

Fixes #1141
@dimitri
monitor: make startup_grace_period superuser-settable (PGC_SUSET)
54d6217 
Changing from PGC_SIGHUP to PGC_SUSET lets superusers adjust the value
per-session with SET, without requiring a config file edit and reload.
This is useful in regression tests (SET pgautofailover.startup_grace_period = 1
instead of ALTER SYSTEM + pg_reload_conf) and for operators who need to
temporarily override the grace period during debugging.

Update test_006 in node_active_protocol.sql to use SET/RESET instead of
ALTER SYSTEM SET/RESET + pg_reload_conf().
@dimitri
tests: tolerate missing state file in DataNode.create(run=True)
9353325 
When run=True, pg_autoctl create --run starts in the background and
get_nodeid() is called immediately after.  If the background process
hasn't had time to write its state file yet, inspect fsm state returns
exit code 2.  The existing comment already noted the node id is grabbed
"if it's already available"; make the code match that intent by
catching CalledProcessError and leaving self.nodeid at its default.
@dimitri
tests: remove stale wait_primary check from enable_monitor_node2
cca96ca 
The IsHealthy() fix (trusting pgIsRunning for NODE_HEALTH_UNKNOWN nodes)
allows node1 to transition from wait_primary to primary much faster than
before.  By the time test_009b / enable_monitor_node2 runs, node1 has
already reached primary state.  When node2 then re-enables the monitor,
node1 receives apply_settings (not wait_primary) because it is already
primary.

The wait_until wait_primary assertion was testing an intermediate state
that is now transient and may be missed.  The follow-on wait_for_convergence
step already verifies the correct final state (primary / secondary).
@dimitri
tests: drop intermediate-state waits from enable_monitor_node2
701dc11 
node2's background keeper auto-registers with the new monitor the moment
the new monitor accepts connections during enable_monitor_node1, completing
the full wait_standby → catchingup → secondary cycle before test_009b /
enable_monitor_node2 even runs.  When the step then calls enable_monitor
on node2, the node is already in secondary and never re-enters catchingup.

The catchingup and wait_primary intermediate-state checks were testing a
sequence that no longer applies.  The wait_for_convergence / test_010 step
already verifies the correct final state (primary + secondary).
@dimitri
tests: retry wait_until_state on not-found exception; fix multi_async…
98df404 
… test_016

When DataNode.create(run=True) starts pg_autoctl in the background, the
node may not yet be registered on the monitor when wait_until_state() is
called immediately after. The base get_state() raises an Exception when
the node is not found, which bypasses the retry loop in wait_until_state()
and propagates immediately.

Fix: wrap get_state() calls in wait_until_state() and
wait_until_assigned_state() with a try/except so transient not-found
errors are treated like a state mismatch and retried until the timeout.

Also fix multi_async test_016_003/004: the IsHealthy() fix (treating
NODE_HEALTH_UNKNOWN as healthy when pgIsRunning=true) causes node2 to
transition from wait_primary to primary faster once node3 is assigned
secondary. The intermediate 'wait until node2 state is wait_primary'
check in test_016_003 races against that transition; remove it and let
test_016_004's final convergence check do the verification. Increase
test_016_004's timeout from 90s to 120s to match test_015_004.
@dimitri
monitor: acquire group lock before row lock in NodeActive to avoid de…
bd5a0f1 
…adlock

NodeActive acquired locks in the order: formation(Share) → row lock
(via ReportAutoFailoverNodeState UPDATE) → group(Exclusive).
set_node_candidate_priority acquires them in the order: formation(Share)
→ group(Exclusive) → row lock (via UPDATE).

When a node_active call and a set_node_candidate_priority call raced, the
opposite acquisition orders caused a deadlock: node_active held the node
row lock and waited for the group lock; set_node_candidate_priority held
the group lock and waited for the same row lock.

Fix: move LockNodeGroup to immediately after LockFormation in NodeActive,
before any UPDATE on the node table. Both code paths now acquire locks in
the same order (formation → group → row), eliminating the cycle.
@dimitri
dimitri merged commit 1ebe6bc into main Jul 11, 2026
54 checks passed
@dimitri
dimitri deleted the monitor/node-active-context branch July 11, 2026 15:24
This was referenced Jul 11, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@dimitri dimitri

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New primary stuck at unhealthy state after network issues between original primary and the monitor Both the data nodes left in read/write mode

1 participant

@dimitri