feat(haproxy): self-heal HAProxy 3.4 runtime dynamic backends

[caffeinated92]

Problem

replication-manager's HAProxy runtime integration could reconcile server state only when the expected read/write backends and rows already existed in the running HAProxy process. If service_write, service_read, or one of their expected rows was missing at runtime, repman could only log the failure and wait for a manual HAProxy reload.

What this PR changes

This PR adds an optional HAProxy 3.4+ self-heal path for haproxy-mode=runtimeapi that can recreate missing runtime backends/servers without a reload.

Main changes

• add haproxy-runtime-dynamic-backends to opt into the new behavior
• add HAProxy runtime API helpers for:
  • add backend
  • publish backend
  • add server
  • enable server
  • enable health
  • del server
  • show servers state
• extend HaproxyProxy.Refresh() to detect and self-heal:
  • missing write/read backends
  • missing leader write row
  • missing read rows
  • unhealthy read rows left behind by partial runtime operations
  • stale read rows for removed cluster members
• verify runtime operations instead of trusting err == nil, since HAProxy often reports command rejection as plain CLI text
• reject unsafe hostname-backed and stale-address runtime repairs instead of falsely reporting success
• support IPv4 and IPv6 runtime server creation/address verification

Safety and compatibility

• feature is off by default
• self-heal runs only for:
  • HAProxy 3.4+
  • haproxy-mode=runtimeapi
• explicitly skipped for standby and staging paths
• existing generated proxy behavior is preserved by keeping the original defaults section unchanged and adding a separate trailing defaults dyn_defaults section used only by runtime-created backends via add backend ... from dyn_defaults
• OpenSVC's embedded HAProxy config was updated to match the same layout

Tests and validation

• added focused regression coverage in cluster/prx_haproxy_test.go for:
  • backend publish verification
  • partial add/enable failures
  • retry behavior across refresh passes
  • stale-address detection
  • duplicate synthetic row prevention
  • same-pass master/read visibility
  • stale row deletion verification
  • IPv6 runtime rows
  • standby/staging gating
• technical implementation doc added at doc/implementation/cluster/HAPROXY_DYNAMIC_BACKENDS.md
• validated with:
  • go test ./cluster/... -run Haproxy
  • go test ./cluster/...
  • go vet ./...
  • live Docker checks against haproxy:3.4-alpine

Scope notes

This PR keeps the existing fixed backend model (service_write / service_read) and does not attempt a broader shared-HAProxy or multi-cluster backend redesign.

Closes #1724

[claude]

Review

Reviewed cluster/prx_haproxy.go, router/haproxy/runtime_api.go, config/config.go, share/haproxy_config.template, and doc/implementation/cluster/HAPROXY_DYNAMIC_BACKENDS.md. Overall this is a well-documented, carefully-commented feature (the doc file and inline comments explaining why, e.g. the exact-match-vs-Contains guard, are genuinely good practice). One correctness gap and a couple of minor issues below.

Bugs / Correctness

Write-leader row has no stale-address check, unlike the read side (cluster/prx_haproxy.go:374 vs :454-458).
sawWriteLeader is set purely from writeLeaderRowHealthy(line[17]) — the row's status string. The read-backend path explicitly guards against this exact class of bug: it cross-checks the row's address against cluster.GetServerFromName(line[1]) because "a row can be healthy (UP/DRAIN/MAINT) at a stale address... which readServerRowHealthy can't catch on its own" (see the comment at line ~449). The write-leader path has no equivalent check. If the leader row's HAProxy health check reports "UP" while pointing at a stale/decommissioned host address, sawWriteLeader is true, selfHealDynamicBackends's if writeBackendReady && !sawWriteLeader branch is skipped, and writes stay silently routed to the wrong host indefinitely. This seems to undercut the core promise of the feature on the write side specifically — worth either applying the same GetServerFromName+literal-IP check used for read rows, or explaining why the write side doesn't need it (only one leader row exists vs. many read rows, so maybe the failure mode differs — but if so that reasoning belongs in a comment).

Performance

hasDynamicBackendSupport() issues a show version round trip on every Refresh() pass whenever haproxy-runtime-dynamic-backends is enabled (line ~570), but the HaproxyMode != "runtimeapi" and staging-proxy early-returns live inside selfHealDynamicBackends, which is only reached after that probe. For a cluster with the flag on but running standby/dataplaneapi mode, every monitoring tick pays for a needless runtime-API round trip. Minor, but easy to fix by hoisting the mode/staging checks ahead of the version probe.

Testing

Test coverage is extensive (2667 lines in prx_haproxy_test.go, covering missing backend/server/leader recovery, IPv6, retries, CLI-text rejections, etc.) — genuinely thorough for a fake-harness suite. However, per this repo's own T13 law and the doc's "Follow-up (not in this change)" section, the PR explicitly acknowledges it ships without the real HAProxy 3.4 Docker/regtest run that CLAUDE.md states is the required gate before a feature ships ("a real HAProxy 3.4 Docker/regtest run... should gate merge"). The doc does note manual protocol validation against a live haproxy:3.4-alpine container for command/response formats, which is good, but that's not the same as the automated regtest suite gate. Given the write-leader gap above, I'd want the real regtest run (or at least a manual failover/switchover exercise against live HAProxy) before merge, not as a follow-up.

Nits

• upsertHealedReadRow's Backend{} literal and the new HaproxyRuntimeDynamicBackends field in config/config.go have misaligned struct-tag/field spacing (an inserted comment broke gofmt's column alignment) — gofmt -l will likely flag these.

Verdict

Requesting changes — mainly on the write-leader stale-address gap, since it's the write path that self-heal is presumably most safety-critical for. Everything else here (docs, test breadth, read-side handling) is solid work.

[caffeinated92]

@claude review my PR changes

[claude]

Claude finished @caffeinated92's task in 1m 48s —— View job

---

Review

• Gather context (PR diff vs origin/develop, prior review comment)
• Check whether commit 663e832 addresses the write-leader stale-address gap from the earlier review
• Re-review cluster/prx_haproxy.go, router/haproxy/runtime_api.go, config/config.go, template changes
• Check hasDynamicBackendSupport() round-trip / gofmt nits from earlier review
• Post final review

I re-reviewed against the prior review comment on this PR (write-leader stale-address gap, hasDynamicBackendSupport() round-trip, gofmt nits, T13 regtest gate). Commit 663e832 (fix(haproxy): clarify leader stale-address recovery) addresses three of the four points:

1. Write-leader stale-address gap — resolved, via explanation + pinning test, not a new check.
cluster/prx_haproxy.go:374-388 now documents (and doc/implementation/cluster/HAPROXY_DYNAMIC_BACKENDS.md:208-222 restates) why sawWriteLeader doesn't need the read side's GetServerFromName+literal-IP cross-check: the pre-existing "Detecting wrong master" block (cluster/prx_haproxy.go:403-451) already looks up the row's current address via GetServerFromURL and calls SetMaster unconditionally on every Refresh() pass — independent of HaproxyRuntimeDynamicBackends/sawWriteLeader — and the !foundMasterInStat fallback at the end of Refresh() (:615-629) covers the case where the address matches no known server at all. I traced this and it checks out: SetMaster can update an existing row's address live (unlike AddServer on the read side, which can't touch an existing row's address). TestHaproxySelfHealWriteLeaderStaleAddressFixedByExistingMasterCheck pins the exact scenario down (stale "leader" row healthy at a slave's old address, expects set server service_write/leader addr 127.0.0.1 port 3306). This is a solid resolution of the concern — good root-cause explanation instead of bolting on a redundant check.

2. hasDynamicBackendSupport() round trip — fixed.
cluster/prx_haproxy.go:585-588 now hoists the HaproxyMode == "runtimeapi" and staging checks ahead of the show version probe, matching selfHealDynamicBackends's own early-return conditions (:685, :695) exactly (De Morgan-equivalent). New assertions in TestHaproxySelfHealSkipsStandbyMode and TestHaproxySelfHealSkipsStagingProxy confirm no "show version" command is issued in either mode. The doc acknowledges the duplication is intentional/cheap insurance rather than a DRY violation — reasonable given selfHealDynamicBackends has one caller today.

3. gofmt nits — fixed.
Both the Backend{} literal in upsertHealedReadRow and the HaproxyRuntimeDynamicBackends field in config/config.go are now correctly column-aligned per gofmt conventions (verified by inspection; I wasn't able to invoke the gofmt/go binaries directly in this environment to double-check mechanically — worth a quick gofmt -l locally before merge).

4. T13 real-HAProxy regtest gate — still open, as before.
doc/implementation/cluster/HAPROXY_DYNAMIC_BACKENDS.md:305-311's "Follow-up (not in this change)" section is unchanged and still lists the real HAProxy 3.4 Docker/regtest run as a follow-up rather than a merge gate. Per CLAUDE.md, this repo's stated policy is that this class of test is "the required gate before a feature ships," so I'd still flag this as something to resolve (run it, or get explicit maintainer sign-off to defer it) before merging, independent of the code quality here.

Net: the follow-up commit is a good-faith, well-reasoned response to the earlier review — the write-leader concern in particular is resolved with a real explanation and a test that would catch a regression, not just a comment. Nothing new stood out in the diff itself. The only remaining item from my read is the regtest gate question above, which is a process/sign-off matter rather than a code defect.

[caffeinated92]

@claude review my PR changes

[claude]

Claude finished @caffeinated92's task in 1m 24s —— View job

---

Review

• Gather context (PR diff vs origin/develop, prior review history)
• Check whether latest commit 19032a1 (regtest addition) changes the standing T13 gate concern
• Re-review cluster/prx_haproxy.go, router/haproxy/runtime_api.go, regtest/test_haproxy_dynamic_backend_self_heal.go
• Post final review

Since the last review, one new commit landed: 19032a1 ("test(haproxy): add dynamic backend self-heal regtest"), adding regtest/test_haproxy_dynamic_backend_self_heal.go (+272) plus the small wiring in regtest/regtest.go and server/regtest.go.

T13 regtest gate — now addressed

This is the item flagged as open in both prior reviews. TestHaproxyDynamicBackendSelfHeal is a real, non-mocked regtest: it talks to an actual HAProxy 3.4+ runtime API (haproxy.Runtime{Host: hprx.Host, Port: hprx.Port}, the same one Refresh()/self-heal use), deliberately breaks live runtime state, then polls hprx.Refresh() (the real monitor-loop method, not a hand-rolled call) until the cluster self-heals it. It covers:

• write-leader row deletion → recreation pointing at the real master, UP
• the exact write-leader "healthy status, stale address" scenario raised in the first review — repoints leader at the slave's address via SetMaster, confirms the break took effect, then waits for correction back to master
• read-server row deletion → recreation, UP

I checked the API usage against the actual signatures rather than trusting the doc comments: Runtime.SetMaintenance(name, pool), DelServer(name, pool), SetMaster(pool, host, port) in router/haproxy/runtime_api.go all match how the test calls them; cl.Conf.HaproxyAPIWriteBackend/HaproxyAPIReadBackend field names match config/config.go; Backend{Host, Port, Svname, PrxStatus} and server.Id as the read-row svname match how cluster/prx_haproxy.go itself populates and looks up rows (e.g. :783, :793). The rowAtRuntime helper correctly re-verifies each deliberate break actually took effect before waiting on it — consistent with this PR's broader "don't trust err == nil" theme.

It's intentionally excluded from the default tests / "ALL" list (same pattern as testStagingRecoverNoReadOnly) since it requires haproxy-runtime-dynamic-backends=true + haproxy-mode=runtimeapi, and must be run explicitly via --test=testHaproxyDynamicBackendSelfHeal. That's a reasonable tradeoff, not a gap — it does mean this won't run in a generic CI sweep, so whoever owns merge should confirm it's actually been run once against real haproxy:3.4-alpine before merging, not just that it exists.

One doc gap

doc/implementation/cluster/HAPROXY_DYNAMIC_BACKENDS.md:305-311's "Follow-up (not in this change)" section is now stale — it still reads as if the real-HAProxy regtest doesn't exist ("a real HAProxy 3.4 Docker/regtest run... should gate merge"), but 19032a1 added exactly that in this same PR. Worth updating that section to point at TestHaproxyDynamicBackendSelfHeal and note its scope limit (doesn't cover backend-level add/publish recovery for a deleted service_write/service_read, per the test file's own header comment) rather than leaving it listed as an open follow-up.

Net

This closes out the last remaining item from the prior two review rounds. Combined with the earlier fixes (write-leader stale-address explanation + pinning test, hasDynamicBackendSupport() round-trip hoist, gofmt alignment), I don't have further correctness concerns on this PR. Only the doc staleness above is worth a quick fix before merge.