Skip to content

Commit 48b1b0d

Browse files
committed
docs(consul-postgres-ha): retire ICE auth-race design brief; move bug to ROBUSTNESS "Already shipped"
Bug is fixed (commit 0490606), regression test is in place, design brief's purpose is done. Move the failure analysis + the closing note into ROBUSTNESS.md's "Already shipped" section so anyone debugging a future mesh-conn ICE issue lands on the writeup that includes the fix, not the now-obsolete brief. Also retire the design-doc row from design/README.md and remove the "add a loopback integration test for mesh-conn" line item from the Layer-2 recommended-fixes list — the auth-race regression test exists and covers the wire protocol too.
1 parent 0490606 commit 48b1b0d

3 files changed

Lines changed: 51 additions & 335 deletions

File tree

consul-postgres-ha/ROBUSTNESS.md

Lines changed: 51 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -97,80 +97,13 @@ whose direct path goes down can't fail over to TURN until it's back".
9797
| Resource exhaustion (many TCP streams) | QUIC per-session limits kick in (256 streams default); new TCP streams to that peer fail. UDP and existing TCP unaffected. | bump `AcceptBacklog` / `MaxIncomingStreams` if it ever hits us at scale. |
9898
| Head-of-line blocking | A big TCP write on one stream briefly delays a UDP datagram or another TCP stream. Imperceptible at Consul scale. | None needed today. If a future workload becomes jitter-sensitive, split into two ICE conns per pair (UDP-only + TCP-only). |
9999

100-
### The ICE auth-race convergence bug — the one real open issue
101-
102-
`dialICE` both publishes its local auth (ufrag+pwd) via the signalling
103-
broker and waits on the remote auth from the broker. On a peer
104-
restart or fresh bring-up, both sides republish in close succession.
105-
mesh-conn's protection against consuming a stale auth is to compare
106-
the most-recently-seen remote auth against the one it's currently
107-
dialing against; if a fresher auth arrives mid-dial, the in-flight
108-
attempt is cancelled and `runPeerLink` retries after 5 s.
109-
110-
What goes wrong is that **both peers do this symmetrically**:
111-
112-
```
113-
peer-A consumed=X publishes auth Y → dials with peer-B's X
114-
peer-B sees A's Y, supersedes its X-bound dial, publishes new auth Z
115-
peer-A sees B's Z, supersedes its Y-bound dial
116-
peer-B sees A's next auth, supersedes again
117-
118-
```
119-
120-
The 5 s back-off doesn't break the symmetry, so the loop never
121-
converges. Observed in live testing on 2026-05-13: multiple worker↔
122-
coordinator and worker↔worker pairs flapping for 7+ minutes without
123-
stabilizing, even with `MESH_CONN_RELAY_ONLY=1` set (so it's not a
124-
NAT-direct-path issue). Per-peer log signature:
125-
126-
```
127-
[<peer>] ice state: Checking
128-
[<peer>] fresh auth (ufrag=Y) supersedes consumed (ufrag=X) — aborting attempt
129-
[<peer>] link failed: ice: connecting canceled by caller — retrying in 5s
130-
[<peer>] ice state: Closed
131-
```
132-
133-
The previous commit message acknowledged this: `cc5a12d fix(consul-
134-
postgres-ha): partial — cancel in-flight ICE dial on fresher peer
135-
auth`. The cancellation logic is correct; the convergence isn't.
136-
137-
Practical impact: when this happens, the affected peer pair never
138-
forms a working link, which means the Consul agent on one side
139-
can't reach the server tier (Raft RPC on `:8300`), Envoy can't fetch
140-
its xDS config, and any cross-peer Connect call through that pair
141-
fails. Patroni leader election still works (Consul KV is on the
142-
working pairs), but replica `pg_basebackup` fails because the
143-
replica's local Envoy has no endpoint for `postgres-master`.
144-
145-
**Possible fix shapes**:
146-
147-
- **Asymmetric back-off keyed on peer ID lexicographic order.** The
148-
side with the *higher* peer-id waits 5 s before redialing; the
149-
*lower* side dials immediately. Breaks the symmetry; one side
150-
always wins the auth race within one cycle.
151-
- **Don't supersede if the new auth came from a poll happening
152-
during the in-flight dial.** Today's logic supersedes on any newer
153-
auth; a tighter rule would only supersede if the auth changed for
154-
a reason other than "the remote peer also just restarted." Harder
155-
to define; the asymmetric back-off is simpler.
156-
- **A "settling" window after publishing auth.** Don't act on a
157-
remote auth that arrives in the first ~1 s after we publish ours;
158-
the remote is probably reacting to our publish, not to a peer
159-
restart we should respect.
160-
161-
The first of these is the cheapest experiment. Needs a focused
162-
mesh-conn debugging session with `MESH_CONN_DEBUG_ICE=1` traces from
163-
a fresh repro, looking at which side initiates each supersession.
164-
165100
### Recommended fixes
166101

167-
1. **The ICE auth-race convergence bug above** — top priority. Blocks
168-
end-to-end live verification of every recent refactor.
169-
2. **Set a QUIC read deadline on the UDP-stream pumps**, so if a
102+
1. **Set a QUIC read deadline on the UDP-stream pumps**, so if a
170103
stream silently stalls (QUIC keep-alive happens at session
171104
level, not stream level), the pump returns and `runPeerLink`
172105
restarts.
173-
3. **Tune QUIC `MaxStreamWindowSize`** if we ever need higher
106+
2. **Tune QUIC `MaxStreamWindowSize`** if we ever need higher
174107
throughput; default is 256 KB which is fine for now.
175108

176109
### Already shipped
@@ -186,6 +119,39 @@ a fresh repro, looking at which side initiates each supersession.
186119
`:1006-1016`; the original bug described in earlier revisions of
187120
this doc is closed.
188121

122+
- **ICE auth-race convergence.** When two peers handshake in near-
123+
simultaneous restart (the normal startup case on a fresh cluster
124+
or after a redeploy), the supersession check in `pollLoop`
125+
"fresh peer auth aborts the in-flight dial against a now-stale
126+
consumed value" — used to fire symmetrically on both sides, and
127+
every aborted attempt's retry rebuilt the `ice.Agent` (mandatory
128+
because pion's `Restart()` doesn't support re-`Dial`; see
129+
`peerSession` docstring), which republished a fresh auth, which
130+
re-superseded the peer's next attempt, ad infinitum. Observed on
131+
2026-05-13 as worker↔coordinator and worker↔worker pairs flapping
132+
for 7+ minutes per pair, blocking Patroni replica `pg_basebackup`
133+
(replicas' local Envoys had no endpoint for `postgres-master`).
134+
135+
Fixed with two coupled changes in `runPeerLink` and `pollLoop`:
136+
asymmetric retry back-off (lex-smaller peer waits 2 s, larger
137+
waits 5 s — see `retryBackoff` in `mesh-conn/main.go`) and a
138+
3-second grace window on the supersession check (`supersedeGrace`)
139+
so an in-flight handshake gets a fair chance to converge before a
140+
peer-side credential roll can abort it. The pair composes: the
141+
back-off asymmetry guarantees one side's retry is the
142+
authoritative auth publisher per cycle; the grace window absorbs
143+
early-race noise that otherwise re-enters the supersede loop
144+
before either retry can settle.
145+
146+
Regression net: `mesh-conn/main_test.go::TestAuthRaceConvergence`
147+
is a deterministic in-process loopback harness that forges a
148+
fresh-auth from the peer while a real handshake is in flight.
149+
Before the fix, the test fails 32/33 of 33 trials (the survivor
150+
was a "link came up before forge arrived" race in the original
151+
single-shot variant). After the fix it passes 50/50 with the
152+
sustained-forge variant gated on the `onAttemptStarted` hook,
153+
which guarantees the forge lands inside the in-flight window.
154+
189155
## Layer 2 — mesh-conn forwarder
190156

191157
### What's there
@@ -228,17 +194,21 @@ The mitigations are mostly testing discipline:
228194

229195
### Recommended fixes
230196

231-
1. **Add a loopback integration test** that runs mesh-conn ↔
232-
mesh-conn locally with a real signalling broker on `127.0.0.1`.
233-
Catches protocol-level regressions (3-byte header, port plan)
234-
without burning CVMs. The unit tests in `validate_test.go` cover
235-
config-time bugs but not the wire format.
236-
2. **Periodic metrics** — counters for streams open/closed, bytes
197+
1. **Periodic metrics** — counters for streams open/closed, bytes
237198
in/out per port. A `/metrics` endpoint or even just stderr every
238199
30 s.
239200

240201
### Already shipped
241202

203+
- **Loopback integration test for mesh-conn**`mesh-conn/main_test.go`
204+
boots two `Mesh` instances against an inlined `/publish` + `/poll`
205+
broker on `httptest.NewServer`, drives a real pion/ice handshake on
206+
host loopback candidates, and asserts `link up` within a 30 s
207+
deadline. Three cases: green-path single handshake, the auth-race
208+
regression test (forges fresh peer auth mid-handshake), and a real
209+
Mesh-restart convergence test. Catches both the auth-race
210+
convergence regression and any future protocol-level break in the
211+
3-byte stream header or per-port plumbing.
242212
- **PEERS_JSON validation at startup**`validatePeers()` in
243213
`mesh-conn/main.go` runs before any goroutine starts and fails
244214
fast on duplicate IDs, missing self, VIP collisions, or
@@ -367,15 +337,12 @@ In order of worst-impact-per-fix-cost:
367337

368338
1. **Two coordinators** + signed signalling messages (Layer 0).
369339
Removes the new-join SPOF and closes the metadata-spoof gap.
370-
2. **Local loopback integration test for mesh-conn** (Layer 2).
371-
Unit tests cover the config layer; the wire protocol still has
372-
no regression net.
373-
3. **Periodic metrics on mesh-conn** (Layer 2). Cheap, dramatic
340+
2. **Periodic metrics on mesh-conn** (Layer 2). Cheap, dramatic
374341
improvement in operability.
375342

376343
Item 1 is what stands between "fun experiment that demos
377-
correctly" and "leave it running and forget about it"; items 2–3
378-
are the next plateau.
344+
correctly" and "leave it running and forget about it"; item 2 is
345+
the next plateau.
379346

380347
The deeper open question — **anyone with `terraform.tfstate` can
381348
read the cluster's gossip key and Patroni passwords** — is
@@ -385,6 +352,9 @@ in attestation rather than handed in by the deployer.
385352

386353
### Closed since the previous revision
387354

355+
- **ICE auth-race convergence flap** — fixed via asymmetric retry
356+
back-off + 3 s grace window on the supersession check. See Layer 1
357+
"Already shipped" for the failure analysis and the regression test.
388358
- **Auth-channel reconnect deadlock** — fixed via fresh
389359
`peerSession{}` per `dialICE` + drain-then-push on `authCh`.
390360
- **Three-server Consul** — coordinator deploys with

consul-postgres-ha/design/README.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ in their face.
1212

1313
| Doc | What |
1414
|---|---|
15-
| [`ice-auth-race-convergence.md`](ice-auth-race-convergence.md) | mesh-conn's "fresh auth supersedes consumed" cancellation is symmetric — both peers cancel each other indefinitely on near-simultaneous restart, blocking live verification of every recent refactor. Bundles a loopback regression test (the prerequisite for any confidence) with the convergence fix. |
1615
| [`attestation-admission.md`](attestation-admission.md) | Use dstack TEE attestation as the mesh-conn admission credential, replacing/augmenting the shared TURN HMAC. Phased plan: per-app-id first, Consul-KV-rooted policy later. |
1716

1817
Each doc includes:

0 commit comments

Comments
 (0)