Commit f2d8f66
committed
tests: fix test_005_002_fail_primary_again quorum stall
Reproduces 5/6 pytest/monitor CI failures (PG14, PG15, PG17, PG18, PG19),
identical on every run: "node3 failed to reach primary after 300 seconds".
Root cause: NOT pgautofailover.guard_data_loss. That GUC (added in #1142)
only changes behavior when explicitly set to false (--allow-data-loss);
its default (true) reproduces byte-for-byte the pre-#1142 blocking
behavior in ProceedGroupStateForMSFailover -- confirmed by diffing ca7834b:
every "return false;" guarding on missingNodesCount/quorumCandidateCount
already existed unconditionally before that commit, the commit only wraps
it in "if (GuardDataLoss)" and adds an unblocking else-branch that only
runs when the GUC is off. Default-on behavior is unchanged.
The real regression is commit a7974f8 ("fsm: wait for local Postgres to
accept connections before reporting SECONDARY"), landed earlier in this
branch. While fixing an unrelated connection-refused race, it also
"fixed" this test's assertion:
assert node1.wait_until_assigned_state(target_state="draining")
assert node3.wait_until_assigned_state(target_state="report_lsn")
-->
assert node3.wait_until_state(target_state="primary", timeout=300)
reasoning that 'report_lsn' is too transient to reliably observe. That's
true, but the replacement assertion is unreachable as written: at this
point in the test node2 is still down (killed in test_005_001 and not
restarted until test_005_003), so with number_sync_standbys=1 the monitor
needs 2 LSN reports before it will elect a new primary and node3 cannot
reach "primary" here -- only node2's restart in test_005_003 can unblock
that, and origin/main's version of this test correctly waits for
"report_lsn" here and defers the "primary" assertion to test_005_003.
a7974f8 turned a transient-state observability problem into a guaranteed
300s timeout by moving the wrong assertion to the wrong test function.
The .pgaf port of this same test (multi_alternate.pgaf) hit the identical
issue and was already fixed correctly in 3c9ca49, right after a7974f8:
it both moves "compose start node2" earlier (into test_005_002) *and*
adds a "passing through report_lsn" wait primitive to robustly observe
the transient state via LISTEN. This commit applies the equivalent fix to
the plain pytest side, which 3c9ca49 didn't touch: start node2 before
killing node1, so quorum is satisfiable and "wait for primary" is
reachable within test_005_002 itself. test_005_003_bring_up_first_failed_primary
no longer needs to start node2 itself, and its wait for the transient
'demoted' state (which may have already come and gone by the time the
test runs, since node2 restarts earlier now) is replaced with a wait for
the stable end state, matching the same fix pattern already used
elsewhere in this file.
test_003_002_stop_primary (the 1/6 PG16-specific failure) was not touched:
its assertions already tolerate node3 staying stuck at report_lsn (it
explicitly checks node3 does NOT reach wait_primary), so it isn't exposed
to this bug the same way -- its single failure is more likely independent
CI timing flakiness and should be monitored separately.
Reproduced locally before and after: fails identically to CI before this
change (333s runtime, node3 timeout after 300s); after the fix, all 15
tests in the file pass in 173s.1 parent 4064a38 commit f2d8f66
1 file changed
Lines changed: 16 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
196 | 196 | | |
197 | 197 | | |
198 | 198 | | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
199 | 210 | | |
200 | 211 | | |
201 | 212 | | |
| |||
208 | 219 | | |
209 | 220 | | |
210 | 221 | | |
211 | | - | |
212 | | - | |
213 | | - | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
214 | 227 | | |
215 | | - | |
216 | | - | |
217 | | - | |
218 | 228 | | |
219 | 229 | | |
220 | 230 | | |
| |||
0 commit comments