Skip to content

Commit cc5a12d

Browse files
h4x3rotabclaude
andcommitted
fix(consul-postgres-ha): partial — cancel in-flight ICE dial on fresher peer auth
Approach 2 from the hot-patch flakiness investigation: extend peerSession with the consumed (ufrag, pwd) and the dialCtx cancel fn under a mutex; in pollLoop's auth handler, if dialICE has already consumed earlier auth (cancelDial != nil) and the new auth differs from what was consumed, cancel the in-flight dial so runPeerLink's 5s retry re-enters dialICE with a fresh agent. Verified live this catches the race: during a hot-patch experiment on a 6-CVM cluster, two cancellations fired immediately after worker-3 came back up: [worker-5] fresh auth (ufrag=dTTgJQtAWZDKXQoq) supersedes consumed (ufrag=GqdAyDeiuscgMTgN) — cancelling stale dial [coordinator-1] fresh auth (ufrag=tlqpstQTsxAneeab) supersedes consumed (ufrag=hJcjbvQIbtmKzGBs) — cancelling stale dial So the race is real and the fix detects it correctly. But this is NOT a complete fix for the hot-patch flakiness — after the cancels, the cluster still didn't re-converge: ICE handshakes run to their 30s connectivity-check timeout despite both sides having mutually-current auth. Verified across 30+ minutes on the live cluster: only 2 cancel events the entire time, and the subsequent dial cycles all fail without further fresh-auth events to cancel. So there's a second failure mode (possibly a coturn permission staleness, dstack-edge NAT mapping rotation, or pion ICE state-machine quirk in relay-only mode) that approach 2 doesn't reach. Pion debug logging is the next step to pin it down. Keeping this commit because the fix is a real improvement against a real bug — just not the full hot-patch story. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f8ae0ab commit cc5a12d

1 file changed

Lines changed: 61 additions & 7 deletions

File tree

  • consul-postgres-ha/mesh-conn

consul-postgres-ha/mesh-conn/main.go

Lines changed: 61 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -844,9 +844,28 @@ func serverTLS() *tls.Config {
844844
// to handshake) and pollLoop (delivering signalling messages). It is
845845
// replaced wholesale on every reconnect so stale state from a previous
846846
// failed attempt can't poison the next one.
847+
//
848+
// The "stale-auth races" mitigation: dialICE reads the remote's auth
849+
// exactly once from authCh, then calls agent.Dial/Accept which uses
850+
// those credentials in every connectivity-check it sends for the
851+
// duration of the attempt. If the peer republishes a fresher auth
852+
// while we're mid-dial (which happens during hot-patches and other
853+
// asynchronous restarts), pushing it to authCh would just sit in the
854+
// buffer unread — Dial/Accept never re-reads. ICE then runs to its
855+
// 30s timeout against a peer whose pion agent has rolled to new
856+
// credentials and is rejecting our message-integrity-mismatched
857+
// BINDING_REQUESTs. To break the deadlock, dialICE registers its
858+
// dialCtx-cancel + the consumed auth string on the session under mu;
859+
// pollLoop, on receiving a fresher auth, cancels the in-flight dial
860+
// so runPeerLink's 5s retry loop re-enters dialICE with a fresh agent
861+
// that publishes new auth and converges with the peer.
847862
type peerSession struct {
848863
agent *ice.Agent
849864
authCh chan [2]string
865+
866+
mu sync.Mutex
867+
consumedAuth [2]string // ufrag,pwd dialICE pulled off authCh; zero before
868+
cancelDial context.CancelFunc // non-nil once dialICE has consumed and is dialing
850869
}
851870

852871
var (
@@ -965,6 +984,15 @@ func dialICE(cfg *Config, remoteID string) (*ice.Conn, error) {
965984
return nil, fmt.Errorf("timeout waiting for remote auth from %s", remoteID)
966985
}
967986

987+
// Record what we're about to dial against, so pollLoop can detect
988+
// fresher auth from the peer and cancel us instead of letting Dial
989+
// run to its 30s connectivity-check timeout against stale creds.
990+
// See the comment on peerSession for why this is necessary.
991+
sess.mu.Lock()
992+
sess.consumedAuth = remote
993+
sess.cancelDial = cancelDial
994+
sess.mu.Unlock()
995+
968996
// 60s is comfortably longer than pion's default 30s connectivity-check
969997
// window. If Dial/Accept hasn't succeeded by then, ICE has already
970998
// transitioned to Failed and the state callback above cancelled the ctx.
@@ -1056,17 +1084,43 @@ func pollLoop(cfg *Config) {
10561084
log.Printf("[%s] bad auth %q", m.From, m.Data)
10571085
continue
10581086
}
1059-
// Always keep the LATEST auth. select-default would drop
1060-
// the new one — and if the buffered one was stale (from
1061-
// before the peer's last bounce), dialICE would consume
1062-
// that stale auth, Dial against the wrong ufrag, ICE
1063-
// would Fail, and we'd repeat forever. Drain-then-push
1064-
// ensures the channel always holds the most-recent auth.
1087+
newAuth := [2]string{parts[0], parts[1]}
1088+
1089+
// If dialICE has already consumed an earlier auth and
1090+
// is mid-Dial, we can't deliver this one through authCh
1091+
// — agent.Dial doesn't re-read. Cancel the stale dial
1092+
// so runPeerLink's 5s retry re-enters dialICE with a
1093+
// fresh agent that publishes new auth and converges
1094+
// with the peer. Skip the cancel if the new auth
1095+
// matches what we already consumed (defensive against
1096+
// a hypothetical broker re-delivery; pion never reuses
1097+
// ufrag/pwd across restarts so in practice this means
1098+
// "the peer didn't actually change").
1099+
sess.mu.Lock()
1100+
if sess.cancelDial != nil {
1101+
if newAuth != sess.consumedAuth {
1102+
log.Printf("[%s] fresh auth (ufrag=%s) supersedes consumed (ufrag=%s) — cancelling stale dial",
1103+
m.From, newAuth[0], sess.consumedAuth[0])
1104+
sess.cancelDial()
1105+
}
1106+
sess.mu.Unlock()
1107+
continue
1108+
}
1109+
sess.mu.Unlock()
1110+
1111+
// First auth for this attempt — drain-then-push so
1112+
// dialICE always reads the most recent value if multiple
1113+
// arrive before its select fires. (mailbox.push on the
1114+
// broker also drops a sender's prior queued messages on
1115+
// auth, but the broker's drop only takes effect on
1116+
// subsequent /publish calls; the message that's already
1117+
// queued for our pending /poll is what this drain
1118+
// handles.)
10651119
select {
10661120
case <-sess.authCh:
10671121
default:
10681122
}
1069-
sess.authCh <- [2]string{parts[0], parts[1]}
1123+
sess.authCh <- newAuth
10701124
case "candidate":
10711125
if sess.agent == nil {
10721126
continue

0 commit comments

Comments
 (0)