|
| 1 | +# Design: fix the mesh-conn ICE auth-race convergence bug + add a loopback regression test |
| 2 | + |
| 3 | +**Status**: design accepted, not started. Single feature branch off |
| 4 | +`dstack-consul-ha-db`, PR back into it. |
| 5 | + |
| 6 | +**Why this bundles two punch-list items**: the bug is impossible to |
| 7 | +reproduce or fix-and-verify confidently against the live cluster |
| 8 | +($-spending each iteration, takes 5+ minutes per cycle, NAT-dependent). |
| 9 | +A deterministic loopback harness is the prerequisite. Once it exists, |
| 10 | +the fix becomes a small change against a measurable behavior, plus |
| 11 | +the harness stays as the regression net for every future mesh-conn |
| 12 | +change. |
| 13 | + |
| 14 | +## Why the bug matters |
| 15 | + |
| 16 | +Live-test on 2026-05-13 confirmed: when multiple peer pairs handshake |
| 17 | +in close succession (the normal startup case), some pairs flap |
| 18 | +indefinitely with this signature: |
| 19 | + |
| 20 | +``` |
| 21 | +[<peer>] ice state: Checking |
| 22 | +[<peer>] fresh auth (ufrag=Y) supersedes consumed (ufrag=X) — aborting attempt |
| 23 | +[<peer>] link failed: ice: connecting canceled by caller — retrying in 5s |
| 24 | +[<peer>] ice state: Closed |
| 25 | +``` |
| 26 | + |
| 27 | +Even with `MESH_CONN_RELAY_ONLY=1` set. The cluster forms partial |
| 28 | +quorum (some links work, some don't), and end-to-end verification of |
| 29 | +anything that needs cross-peer Connect routing (notably Patroni |
| 30 | +replica `pg_basebackup`) is blocked. |
| 31 | + |
| 32 | +The supersession logic itself is correct in intent — it prevents |
| 33 | +`agent.Dial`/`Accept` from completing against a remote ufrag/pwd the |
| 34 | +peer has already rolled. The bug is symmetry: both sides supersede on |
| 35 | +each other's fresh auth, and every retry publishes fresh auth (because |
| 36 | +each retry builds a new `ice.Agent`, which generates new credentials), |
| 37 | +so neither side ever lands on stable auth. |
| 38 | + |
| 39 | +See `mesh-conn/main.go:1231-1240` for the supersede trigger and |
| 40 | +`mesh-conn/main.go:407-422` for the fixed 5s retry loop. |
| 41 | + |
| 42 | +## Goal |
| 43 | + |
| 44 | +After this work: |
| 45 | + |
| 46 | +- **A loopback integration test** in `mesh-conn/` that boots two |
| 47 | + `mesh-conn` instances + a real `signaling` broker on `127.0.0.1`, |
| 48 | + drives an ICE handshake between them, and asserts the link comes up |
| 49 | + inside a bounded time window. Test must reproduce the auth-race |
| 50 | + bug deterministically (`go test -run AuthRace` shows the flap in |
| 51 | + the BEFORE state) and pass once the fix lands. |
| 52 | +- **The bug fixed.** Two mesh-conn instances handshaking with |
| 53 | + near-simultaneous restart converge to a working link within |
| 54 | + `<bound>` seconds (target: ≤ 15 s, comparable to a single normal |
| 55 | + handshake). |
| 56 | +- **The fix in plain bash & Go**, no new dependencies, no protocol |
| 57 | + changes. The wire format (auth + candidate messages on the |
| 58 | + signaling broker) stays identical. |
| 59 | + |
| 60 | +## Non-goals |
| 61 | + |
| 62 | +- Replacing the supersession logic. The protection against stale |
| 63 | + remote auth is correct; the fix is about making the retry cycle |
| 64 | + asymmetric enough to converge. |
| 65 | +- Re-architecting pion/ice usage or ICE Restart support. |
| 66 | +- Changing the signaling broker's wire format. |
| 67 | + |
| 68 | +## Investigative phase (before any code change) |
| 69 | + |
| 70 | +The fix shape isn't obvious from reading the code alone. The brief |
| 71 | +proposes three candidate fixes (below), but the experimenter should |
| 72 | +**build the harness first**, reproduce the bug, then test each |
| 73 | +candidate against measured behavior: |
| 74 | + |
| 75 | +1. **Asymmetric back-off keyed on peer-ID lex order.** Lower-ID side |
| 76 | + retries with shorter back-off (e.g. 2 s ± jitter); higher-ID side |
| 77 | + retries with longer (e.g. 5 s ± jitter). Lower side becomes the |
| 78 | + "stable" auth publisher; higher side reads it during its longer |
| 79 | + wait. Cheapest to implement (~5 LoC in `runPeerLink`'s retry |
| 80 | + sleep), but doesn't fully break the symmetry — if both sides are |
| 81 | + actively restarting, each retry still republishes auth, so the |
| 82 | + race can still manifest under sustained churn. |
| 83 | + |
| 84 | +2. **Time-gated supersession.** Track each attempt's start time on |
| 85 | + `peerSession`; in `pollLoop`, only supersede if the in-flight |
| 86 | + attempt has been running longer than some minimum window (e.g. |
| 87 | + 3 s). The reasoning: in the first few seconds after dial, both |
| 88 | + sides are still racing each other's restart noise; after the |
| 89 | + handshake has had time to make progress, only legitimate restarts |
| 90 | + should be able to abort it. ~15 LoC. |
| 91 | + |
| 92 | +3. **Don't republish auth on supersession-triggered retries.** Track |
| 93 | + the *reason* for the retry. If `dialAndPump` returns because of a |
| 94 | + supersession (not a real failure), reuse the already-published |
| 95 | + auth instead of building a fresh `ice.Agent` with new credentials. |
| 96 | + The peer's fresh auth is already in `authCh`; consume it and try |
| 97 | + again with the existing local agent. ~30 LoC + careful state |
| 98 | + management — pion's `ice.Agent` lifecycle may not support reuse |
| 99 | + cleanly. |
| 100 | + |
| 101 | +Approaches (1) and (2) compose. (1) reduces the rate at which both |
| 102 | +sides re-enter the supersede window simultaneously; (2) ensures that |
| 103 | +once a handshake has started making progress, it isn't aborted on |
| 104 | +spurious peer-side restarts. Implementing both is a real option if |
| 105 | +either alone proves insufficient. |
| 106 | + |
| 107 | +The experimenter's job is to: |
| 108 | + |
| 109 | +- Build the harness (see "Harness shape" below). |
| 110 | +- Reproduce the failing case deterministically. |
| 111 | +- Try (1) alone. Measure convergence time + success rate over N |
| 112 | + trials. Document. |
| 113 | +- If insufficient, layer (2) on top. Measure again. |
| 114 | +- (3) is the heavier change; only attempt if (1)+(2) don't converge. |
| 115 | + |
| 116 | +## Harness shape |
| 117 | + |
| 118 | +The test should look like this (sketch — exact API up to the |
| 119 | +implementer): |
| 120 | + |
| 121 | +```go |
| 122 | +func TestAuthRaceConvergence(t *testing.T) { |
| 123 | + // 1. Boot signaling broker on a random loopback port. |
| 124 | + broker := signaling.NewBroker(t, "127.0.0.1:0") |
| 125 | + defer broker.Close() |
| 126 | + |
| 127 | + // 2. Spawn two mesh-conn instances in goroutines, configured |
| 128 | + // to talk to each other through the broker. |
| 129 | + peerA := newPeer(t, "peer-A", broker.URL(), peerSpec{ |
| 130 | + peers: []Peer{{ID: "peer-B", VIP: 2}}, |
| 131 | + selfID: "peer-A", selfVIP: 1, |
| 132 | + }) |
| 133 | + peerB := newPeer(t, "peer-B", broker.URL(), peerSpec{ |
| 134 | + peers: []Peer{{ID: "peer-A", VIP: 1}}, |
| 135 | + selfID: "peer-B", selfVIP: 2, |
| 136 | + }) |
| 137 | + |
| 138 | + // 3. Start both within 100ms of each other — this is the |
| 139 | + // near-simultaneous-restart pattern that triggers the race. |
| 140 | + go peerA.Run() |
| 141 | + time.Sleep(50 * time.Millisecond) |
| 142 | + go peerB.Run() |
| 143 | + |
| 144 | + // 4. Assert both peers see "link up" within a deadline. |
| 145 | + // Before the fix: this times out. |
| 146 | + // After the fix: passes within ~10s. |
| 147 | + require.NoError(t, waitLinkUp(peerA, "peer-B", 30*time.Second)) |
| 148 | + require.NoError(t, waitLinkUp(peerB, "peer-A", 30*time.Second)) |
| 149 | +} |
| 150 | +``` |
| 151 | + |
| 152 | +Coverage targets: |
| 153 | + |
| 154 | +- `TestAuthRaceConvergence` — the primary regression test (above). |
| 155 | +- `TestSingleHandshake` — sanity check that a single peer-pair |
| 156 | + handshake without simultaneous restart still works. |
| 157 | +- `TestRestartConvergence` — kill one peer mid-link, restart it, |
| 158 | + assert the link re-forms within bounded time. |
| 159 | +- `TestPeersValidation` — already exists in `validate_test.go`; this |
| 160 | + refactor should not regress it. |
| 161 | + |
| 162 | +A few design notes for the harness: |
| 163 | + |
| 164 | +- **STUN/TURN**: skip. We're on loopback, ICE host candidates work |
| 165 | + fine without coturn. Set `TurnHost=""` in the test peer config and |
| 166 | + ensure `mesh-conn` handles that path (it should already, but worth |
| 167 | + verifying). The bug we're chasing isn't NAT-traversal-related; it's |
| 168 | + in the auth+retry state machine. |
| 169 | +- **No real TCP forwarding payload needed.** "Link up" = QUIC |
| 170 | + handshake completed + the `link up` log line emits / a signal can |
| 171 | + be observed. The harness doesn't need to push bytes through. |
| 172 | +- **Hook for observing "link up"**: easiest is a callback or channel |
| 173 | + injected into the test peer's config. Alternatively, capture log |
| 174 | + output and grep — uglier but works. Avoid adding a public API to |
| 175 | + mesh-conn just for the test if a test-only hook suffices. |
| 176 | +- **Determinism**: the bug needs a specific timing window to |
| 177 | + reproduce. The harness's 50 ms delay between starts is a starting |
| 178 | + guess; the experimenter should sweep that and document which |
| 179 | + delays trigger the race in the BEFORE state. |
| 180 | + |
| 181 | +## Implementation by file (assuming fix candidate (1) succeeds; pivot if not) |
| 182 | + |
| 183 | +- **`mesh-conn/main.go`**: |
| 184 | + - In `runPeerLink`, replace the fixed `time.Sleep(5*time.Second)` |
| 185 | + with a function that returns a duration based on `self.ID < |
| 186 | + peer.ID` plus a deterministic jitter. |
| 187 | + - Document the asymmetry rationale in a comment above the retry |
| 188 | + sleep. |
| 189 | +- **`mesh-conn/main_test.go`** (new): |
| 190 | + - The harness + the four test cases above. |
| 191 | + - `signaling` broker is the production one — import the binary's |
| 192 | + package or vendor a thin wrapper. If the broker's a separate |
| 193 | + Go module, write a minimal in-test broker that implements the |
| 194 | + same `/publish` + `/poll` shape (likely the cleanest, since |
| 195 | + `signaling/` is its own go.mod). |
| 196 | +- **`mesh-conn/go.mod`**: any new test-only deps (e.g. |
| 197 | + `require/assert` if not already there). |
| 198 | +- **`ROBUSTNESS.md`**: |
| 199 | + - Move the "ICE auth-race convergence bug" section from |
| 200 | + "Recommended fixes" to "Already shipped" once verified. |
| 201 | +- **Delete `design/ice-auth-race-convergence.md`** (this file) after |
| 202 | + the implementation lands. |
| 203 | + |
| 204 | +## Success criteria |
| 205 | + |
| 206 | +- [ ] `go test -run AuthRace -count 50 ./mesh-conn/...` passes 50/50 |
| 207 | + times (deterministic, no flake). |
| 208 | +- [ ] On the BEFORE branch (before the fix), the same test fails |
| 209 | + ≥ 30/50 times within the 30-second deadline. The agent should |
| 210 | + record this observation in the PR. |
| 211 | +- [ ] The "single handshake" test still passes — no regression on |
| 212 | + the non-racy path. |
| 213 | +- [ ] `mesh-conn/main.go` line count grows by ≤ 50 LoC for the fix |
| 214 | + itself (excluding the test). |
| 215 | +- [ ] All `mesh-conn` unit tests pass: `go test -count=1 ./...`. |
| 216 | +- [ ] Optional but recommended: live verification — rebuild images, |
| 217 | + deploy a 6-CVM cluster, observe that all peer pairs handshake |
| 218 | + within 30 s of CVM creation, run a `FAILOVER.md` recipe end |
| 219 | + to end (specifically the disk-loss `pg_basebackup` path that |
| 220 | + we observed failing on 2026-05-13). Spend ≤ $10. If skipped, |
| 221 | + surface that explicitly in the final report. |
| 222 | + |
| 223 | +## Risks + mitigations |
| 224 | + |
| 225 | +| Risk | Mitigation | |
| 226 | +|---|---| |
| 227 | +| Asymmetric back-off (fix #1) reduces the race rate but doesn't eliminate it | Layer fix #2 (time-gated supersession) on top. The brief assumes the experimenter will measure and pivot. | |
| 228 | +| Loopback harness doesn't exercise pion/ice the same way real ICE-over-coturn does | The auth-race bug is in mesh-conn's outer state machine (runPeerLink + pollLoop + sess.consumedAuth), not in pion. Loopback ICE host candidates are sufficient. If a follow-up reveals the bug only manifests with relay candidates, add a coturn fixture later. | |
| 229 | +| Test harness depends on production `signaling` package which has its own go.mod | Inline a minimal `/publish` + `/poll` broker in the test file. The wire format is JSON arrays of `{from, type, data}` messages; ~50 LoC of net/http handler. | |
| 230 | +| pion/ice agent reuse (fix #3) hits unsupported state in the library | Don't attempt #3 unless #1 + #2 together fail to converge. The brief flags it as the heavier option. | |
| 231 | +| Live verification skipped → fix appears to work locally but doesn't on dstack NAT | Mark this explicitly in the report. The deterministic loopback test is the *primary* deliverable; live verification is supplemental. If live verification reveals a separate issue, that's a follow-up. | |
| 232 | + |
| 233 | +## Hand-off |
| 234 | + |
| 235 | +Implementing agent: |
| 236 | + |
| 237 | +1. Read `mesh-conn/main.go` end-to-end. Focus on `runPeerLink` |
| 238 | + (407-423), `dialAndPump` (452-…), `dialICE` (990-…), `pollLoop` |
| 239 | + (~1180-…), and the `peerSession` struct (956-). |
| 240 | +2. Read this doc and the "ICE auth-race convergence bug" section of |
| 241 | + `ROBUSTNESS.md`. |
| 242 | +3. Build the harness first. **Do not write any fix code until you |
| 243 | + can reproduce the bug in a unit test.** The deterministic repro |
| 244 | + is the load-bearing deliverable; everything else depends on it. |
| 245 | +4. Once reproducing: try fix candidate (1), measure, document. |
| 246 | + Iterate to (2) or (1+2) if needed. Avoid (3) unless necessary. |
| 247 | +5. Land the fix + test in a single feature branch. |
| 248 | +6. Optionally live-verify per "Success criteria" #6. |
| 249 | +7. Update ROBUSTNESS.md. |
| 250 | +8. Delete this doc. |
| 251 | +9. Report back with: the BEFORE failure rate (n trials), the AFTER |
| 252 | + success rate (n trials), the chosen fix shape, and any deviations |
| 253 | + from this brief with file:line citations. |
0 commit comments