From 67c566ac6886023c661204db7bd253eaec41f89c Mon Sep 17 00:00:00 2001 From: AAA <35992542+TNTcraftHIM@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:36:21 +0800 Subject: [PATCH] fix(route): keep direct ICE attempts viable (#368) --- .../0005-automatic-hybrid-media-routing.md | 11 ++++-- ...nection-local-nat-prediction-experiment.md | 13 ++++--- docs/operations/self-hosting.md | 17 +++++---- docs/product/routing-transport.md | 12 ++++-- docs/reference/configuration.md | 19 +++++----- docs/research/nat-traversal.md | 15 +++++--- src/client/webrtc/nat-prediction.ts | 25 ++++++------- src/server/room-route-controller.ts | 14 ++++++- tests/host-peer-tracks.test.ts | 3 +- tests/nat-prediction.test.ts | 30 +++++++++++++-- tests/room-route-controller.test.ts | 37 ++++++++++++++++++- 11 files changed, 138 insertions(+), 58 deletions(-) diff --git a/docs/adr/0005-automatic-hybrid-media-routing.md b/docs/adr/0005-automatic-hybrid-media-routing.md index 25502d7..9a908d0 100644 --- a/docs/adr/0005-automatic-hybrid-media-routing.md +++ b/docs/adr/0005-automatic-hybrid-media-routing.md @@ -140,10 +140,13 @@ Peer candidates converge behind working media, one at a time and round-robin across SFU Viewers. New join or repair work preempts this background convergence. Exhausting direct candidates simply keeps the working SFU route. -The five-second no-transport-progress window is candidate-relative across every -Peer operation. A new candidate receives its own window within the unchanged -total operation deadline; exact transport progress retains it until media proof -or that deadline. +The five-second no-transport-progress window advances a silent Peer only while +another bounded candidate or SFU fallback remains. It is a route scheduling +boundary, not evidence that Browser ICE has reached a terminal state. The final +or only Peer candidate remains until the unchanged total operation deadline; +exact transport progress also retains it until media proof or that deadline. +After SFU commits, each one-candidate direct continuation therefore uses its +full background operation while working media stays authoritative. If the Host is full and no publication exists, one bounded SFU-bootstrap intent owns the original waiting demand and a finite cursor of eligible Host-direct diff --git a/docs/adr/0009-connection-local-nat-prediction-experiment.md b/docs/adr/0009-connection-local-nat-prediction-experiment.md index 5485b4f..dfba56b 100644 --- a/docs/adr/0009-connection-local-nat-prediction-experiment.md +++ b/docs/adr/0009-connection-local-nat-prediction-experiment.md @@ -29,10 +29,10 @@ field, but no new message type, route-controller state, or media path. 3. For one ICE gathering generation, the adapter observes only UDP `srflx` candidates. If three or more distinct candidates for one media section and public address form an arithmetic port sequence, it appends at most eight - bounded candidates around the latest observation (four in each direction). - Every ordinary candidate is retained; no ordinary candidate is rejected or - replaced. A failed or inconclusive experiment uses the existing ICE and SFU - fallback unchanged. + bounded candidates around the sequence's high endpoint (four in each + direction). Every ordinary candidate keeps normal Trickle ICE timing; no + ordinary candidate is delayed, rejected, or replaced. A failed or + inconclusive experiment uses the existing ICE and SFU fallback unchanged. 4. Candidate observations remain in memory for that connection only. No NAT label, raw address, port, score, hard candidate skip, periodic probe, or route-controller branch is introduced. @@ -43,8 +43,9 @@ Positive: - operators can test the measured hole-punching mechanism without changing the default media path; -- normal ICE remains available, so an unsuccessful prediction cannot remove - the existing direct or SFU fallback; and +- normal ICE remains immediately available, so an unavailable auxiliary STUN + listener or unsuccessful prediction cannot delay the existing direct or SFU + fallback; and - the experiment is bounded to one connection generation and one small candidate set. diff --git a/docs/operations/self-hosting.md b/docs/operations/self-hosting.md index 3083632..29fb3a8 100644 --- a/docs/operations/self-hosting.md +++ b/docs/operations/self-hosting.md @@ -73,11 +73,12 @@ Coturn runs `stun-only`, `no-tcp`, and `no-tls`. It must answer STUN binding and reject TURN allocation. Avoid verbose STUN/ICE logging because infrastructure necessarily observes client IP addresses. -## Temporary NAT Survey +## Optional NAT Prediction Experiment -Enable the auxiliary listeners only for an approved aggregate-only survey. Add -the following to the service-owned coturn configuration, using the local -address already bound by the main listener: +Rooms can use the default-off NAT prediction experiment only while both +auxiliary STUN-only listeners are reachable. Add the following to the +service-owned coturn configuration, using the local address already bound by +the main listener: ```ini aux-server=:3479 @@ -88,9 +89,11 @@ Keep `stun-only`, `no-tcp`, and `no-tls` unchanged. Allow UDP 3479 and 3480 in both the cloud security group and the existing nftables input rule, validate the complete ruleset with `nft -c -f /etc/nftables.conf`, then restart only `coturn.service`. Verify `ss -lunp` shows all three UDP listeners and that an -ordinary STUN Binding succeeds on each; do not add these URLs to production -`STUN_URLS` or inject predicted ICE candidates. Remove the two `aux-server` -lines and firewall ports after the survey, validate again, and restart coturn. +ordinary STUN Binding succeeds on each. Do not add these URLs to production +`STUN_URLS`; enabled Browser connections derive them from the ordinary STUN +authority. Keep both listeners and firewall rules for as long as the room +experiment is offered. Disable the experiment before removing both +`aux-server` lines and both firewall rules, validate again, and restart coturn. These same-IP auxiliary ports measure destination-port mapping behavior. They are not a full RFC 5780 alternate-address deployment; that requires a second diff --git a/docs/product/routing-transport.md b/docs/product/routing-transport.md index 5160189..895f2f9 100644 --- a/docs/product/routing-transport.md +++ b/docs/product/routing-transport.md @@ -75,10 +75,14 @@ without discarding its subtree; departure and capacity changes move only affecte children. ADR-0005 owns filtering, ordering, cursor, revision, reservation, and rollback mechanics. -Every unconnected Peer candidate gets the same five-second no-progress window. -Transport-connected progress retains it through the operation deadline. This -applies to join, recovery, direct convergence, quality convergence and structural -convergence; candidate order never resets the total deadline. +Five seconds is a scheduling window only while another bounded candidate or SFU +fallback remains. It advances a silent Peer candidate instead of delaying the +next available route; it is not interpreted as terminal ICE failure. The final +or only Peer candidate remains until the unchanged total operation deadline. +Transport-connected progress also retains a candidate through that deadline. +Consequently a working SFU route can carry media while its one-candidate direct +convergence uses the full background operation, and peer-only acquisition does +not abandon its only possible route at the foreground boundary. When a newly committed Host-root Viewer exposes unused downstream capacity while another Host root has at least two direct children, the same background operation may diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 8e0bb0e..2cac141 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -50,7 +50,7 @@ Peer IDs, SDP, ICE candidates, tokens or media credentials. | ---: | --- | --- | | TCP 80/443 | public | HTTP redirect and HTTPS/WSS reverse proxy | | UDP 3478 | public | STUN-only coturn | -| UDP 3479/3480 | temporary public during approved NAT survey | auxiliary STUN-only coturn listeners | +| UDP 3479/3480 | public while the optional NAT experiment is offered | auxiliary STUN-only coturn listeners | | UDP 7882 | public when SFU enabled | LiveKit WebRTC media | | TCP 8787 | private | Screener application | | TCP 7880 | private when SFU enabled | LiveKit signaling/control | @@ -59,14 +59,15 @@ TCP 3478, TCP/TLS 5349, TURN relay ranges, LiveKit media TCP, and other media ports remain closed. HTTPS/WSS transport is independent of the UDP-only media contract. -The optional NAT survey uses `stun::3479` and `:3480` in a -diagnostic page only. It does not change `STUN_URLS`, ordinary Peer ICE, or any -media route, and it needs both cloud security-group rules and the host's -`/etc/nftables.conf` rule. Coturn must bind the auxiliary listeners to the -host's local address with `aux-server`; opening a cloud port without a listener -has no effect. No new DNS record is required. Same-IP ports measure destination -port allocation behavior; a full RFC 5780 alternate-address test requires a -second public IPv4. +The default-off NAT experiment derives `stun::3479` and `:3480` +from the ordinary STUN authority for enabled Browser P2P connections. It does +not change `STUN_URLS` or add a media route, and it needs both cloud +security-group rules and the host's `/etc/nftables.conf` rule. Coturn must bind +the auxiliary listeners to the host's local address with `aux-server`; opening +a cloud port without a listener has no effect. No new DNS record is required. +Keep both listeners available while the experiment is offered and remove them +only after disabling it. Same-IP ports expose destination-port allocation +behavior; a full RFC 5780 alternate-address test requires a second public IPv4. ## Bounds diff --git a/docs/research/nat-traversal.md b/docs/research/nat-traversal.md index 2eb41c7..7b15c7b 100644 --- a/docs/research/nat-traversal.md +++ b/docs/research/nat-traversal.md @@ -63,11 +63,13 @@ STUN-only listeners for an explicitly enabled room experiment. The accepted experiment is deliberately connection-local and additive. It derives the two auxiliary ports from the existing STUN authority, waits for a clear three-point arithmetic `srflx` shape in one ICE generation, and appends a -small two-sided candidate window. All ordinary candidates remain available; -there is no NAT label, hard candidate skip, route-controller input, or SFU -preference. The Host switch is default off, locks while sharing, and applies to -Host, Viewer upstream, and Viewer relay P2P connections. Its exact -scope is recorded in [ADR-0009](../adr/0009-connection-local-nat-prediction-experiment.md). +small two-sided candidate window from the observed port-sequence endpoint. +Ordinary candidates trickle immediately, so unavailable auxiliary listeners +cannot hold back stock ICE. There is no NAT label, hard candidate skip, +route-controller input, or SFU preference. The Host switch is default off, +locks while sharing, and applies to Host, Viewer upstream, and Viewer relay P2P +connections. Its exact scope is recorded in +[ADR-0009](../adr/0009-connection-local-nat-prediction-experiment.md). The Browser-only alternatives do not yet have an accepted implementation: @@ -98,7 +100,8 @@ The Browser API is also weaker than the raw-socket netcheck model: Consequently Browser observations can support a positive `varies` diagnostic, but cannot produce the proposed `eim | edm-sequential | edm-random` participant -fact. They cannot safely remove a Peer candidate or save its five-second window. +fact. They cannot safely remove a Peer candidate or alter route ordering and +deadlines. Port prediction remains an experiment rather than a default path. The controlled namespace matrix supports the bounded mechanism, but field diff --git a/src/client/webrtc/nat-prediction.ts b/src/client/webrtc/nat-prediction.ts index 6520604..34111aa 100644 --- a/src/client/webrtc/nat-prediction.ts +++ b/src/client/webrtc/nat-prediction.ts @@ -231,7 +231,9 @@ export function predictSrflxCandidates( if (step === null) { continue; } - const anchor = distinct.at(-1)!; + const anchor = distinct.reduce((latest, observation) => + observation.port > latest.port ? observation : latest, + ); return predictedFrom( anchor, step, @@ -242,12 +244,12 @@ export function predictSrflxCandidates( } /** - * Holds only srflx candidates until the mapping shape is known, then emits - * predictions before the held ordinary candidates. Non-srflx candidates keep - * their normal trickle timing. A new instance belongs to one ICE generation. + * Emits every ordinary candidate at normal trickle timing. Once one ICE + * generation proves a predictable srflx shape, the bounded predictions are + * appended without delaying or replacing stock ICE. */ export class NatPredictionCandidateBatch { - private readonly pendingSrflx: SignalCandidate[] = []; + private readonly observedSrflx: SignalCandidate[] = []; private predictionsSent = false; private completed = false; @@ -261,8 +263,9 @@ export class NatPredictionCandidateBatch { this.send(candidate); return; } - this.pendingSrflx.push(candidate); + this.observedSrflx.push(candidate); this.trySendPredictions(); + this.send(candidate); } complete(): void { @@ -270,23 +273,20 @@ export class NatPredictionCandidateBatch { return; } this.trySendPredictions(); - for (const candidate of this.pendingSrflx.splice(0)) { - this.send(candidate); - } this.send(null); this.completed = true; } discard(): void { this.completed = true; - this.pendingSrflx.length = 0; + this.observedSrflx.length = 0; } private trySendPredictions(): void { if (this.predictionsSent) { return; } - const predictions = predictSrflxCandidates(this.pendingSrflx); + const predictions = predictSrflxCandidates(this.observedSrflx); if (predictions.length === 0) { return; } @@ -294,9 +294,6 @@ export class NatPredictionCandidateBatch { this.send(candidate); } this.predictionsSent = true; - for (const candidate of this.pendingSrflx.splice(0)) { - this.send(candidate); - } } } diff --git a/src/server/room-route-controller.ts b/src/server/room-route-controller.ts index 99b7184..3ad0f95 100644 --- a/src/server/room-route-controller.ts +++ b/src/server/room-route-controller.ts @@ -2293,6 +2293,7 @@ export class RoomRouteController { const noProgress = current?.tuple.kind === "peer" && !current.transportConnected && + this.peerCandidateHasSuccessor(this.operation) && nowMs >= current.startedAtMs + this.directHeadStartMs() && nowMs < this.operation.deadlineAtMs; this.debug(noProgress ? "candidate-no-progress" : "operation-deadline", { @@ -2492,6 +2493,7 @@ export class RoomRouteController { const peerNoProgressExpired = attempt.tuple.kind === "peer" && !attempt.transportConnected && + this.peerCandidateHasSuccessor(operation) && nowMs >= attempt.startedAtMs + this.directHeadStartMs(); if ( !guardFailed && @@ -4168,7 +4170,8 @@ export class RoomRouteController { } if ( operation.current?.tuple.kind === "peer" && - !operation.current.transportConnected + !operation.current.transportConnected && + this.peerCandidateHasSuccessor(operation) ) { return Math.min( operation.deadlineAtMs, @@ -4178,6 +4181,15 @@ export class RoomRouteController { return operation.deadlineAtMs; } + private peerCandidateHasSuccessor( + operation: ChildOperation, + ): boolean { + return ( + operation.cursor + 1 < operation.candidates.length || + this.foregroundSfuIndex(operation) > operation.cursor + ); + } + private directHeadStartDeadlineAt( operation: ChildOperation, ): number { diff --git a/tests/host-peer-tracks.test.ts b/tests/host-peer-tracks.test.ts index 243c56c..ce79fa9 100644 --- a/tests/host-peer-tracks.test.ts +++ b/tests/host-peer-tracks.test.ts @@ -781,8 +781,9 @@ describe("HostPeer source replacement", () => { const ordinaryIndex = candidateSignals.findIndex((signal) => signal.candidate?.candidate.startsWith("candidate:base"), ); + expect(ordinaryIndex).toBe(0); expect(predictionIndex).toBeGreaterThanOrEqual(0); - expect(predictionIndex).toBeLessThan(ordinaryIndex); + expect(predictionIndex).toBeGreaterThan(ordinaryIndex); expect( candidateSignals.filter((signal) => signal.candidate?.candidate.startsWith("candidate:base"), diff --git a/tests/nat-prediction.test.ts b/tests/nat-prediction.test.ts index 514475b..05585be 100644 --- a/tests/nat-prediction.test.ts +++ b/tests/nat-prediction.test.ts @@ -84,6 +84,18 @@ describe("NAT prediction ICE adapter", () => { expect(predictions.every((value) => value?.sdpMid === "0")).toBe(true); }); + it("anchors on the port sequence extreme when responses arrive reversed", () => { + const predictions = predictSrflxCandidates([ + candidate(40_006), + candidate(40_003), + candidate(40_000), + ]); + + expect(predictions.map(portOf)).toEqual( + expect.arrayContaining([40_009, 39_997]), + ); + }); + it("does not predict an unstable or incomplete mapping shape", () => { expect(predictSrflxCandidates([candidate(40_000), candidate(40_001)])).toEqual( [], @@ -97,7 +109,7 @@ describe("NAT prediction ICE adapter", () => { ).toEqual([]); }); - it("sends predictions before held srflx candidates while retaining every one", () => { + it("appends predictions without delaying or replacing ordinary candidates", () => { const sent: Array = []; const batch = new NatPredictionCandidateBatch((value) => sent.push(value)); const first = candidate(40_000); @@ -107,17 +119,17 @@ describe("NAT prediction ICE adapter", () => { batch.add(hostCandidate()); batch.add(first); batch.add(second); + expect(sent).toEqual([hostCandidate(), first, second]); batch.add(third); batch.complete(); batch.complete(); expect(sent[0]).toEqual(hostCandidate()); - const firstOriginal = sent.indexOf(first); const firstPrediction = sent.findIndex( (value) => value?.candidate.includes("candidate:s") ?? false, ); - expect(firstPrediction).toBeGreaterThanOrEqual(1); - expect(firstPrediction).toBeLessThan(firstOriginal); + expect(firstPrediction).toBeGreaterThan(sent.indexOf(second)); + expect(firstPrediction).toBeLessThan(sent.indexOf(third)); expect(sent.filter((value) => value === first)).toHaveLength(1); expect(sent.filter((value) => value === second)).toHaveLength(1); expect(sent.filter((value) => value === third)).toHaveLength(1); @@ -136,4 +148,14 @@ describe("NAT prediction ICE adapter", () => { expect(sent).toEqual([first, second, null]); }); + + it("does not wait for auxiliary gathering before sending stock srflx", () => { + const sent: Array = []; + const batch = new NatPredictionCandidateBatch((value) => sent.push(value)); + const first = candidate(40_000); + + batch.add(first); + + expect(sent).toEqual([first]); + }); }); diff --git a/tests/room-route-controller.test.ts b/tests/room-route-controller.test.ts index 9a658a8..fc6bf27 100644 --- a/tests/room-route-controller.test.ts +++ b/tests/room-route-controller.test.ts @@ -1211,7 +1211,7 @@ describe("RoomRouteController", () => { expect(routes.reconcile(25).operation?.reason).toBe("quality-convergence"); }); - it("gives each unconnected Peer candidate its own bounded progress window", () => { + it("advances a silent Peer only when another bounded candidate remains", () => { const routes = controller(2, { operationTimeoutMs: 20_000 }); addViewer(routes, B, 2); addViewer(routes, C, 2); @@ -1233,10 +1233,40 @@ describe("RoomRouteController", () => { connectionId: "second_silent_candidate", reservation: { kind: "direct" }, }); - expect(routes.snapshot().operation?.wakeAtMs).toBe(10_200); + expect(routes.snapshot().operation?.wakeAtMs).toBe( + operation.deadlineAtMs, + ); expect(routes.snapshot().operation?.deadlineAtMs).toBe( operation.deadlineAtMs, ); + expect(routes.operationExpired(10_200).accepted).toBe(false); + expect(routes.snapshot().operation?.current?.connectionId).toBe( + "second_silent_candidate", + ); + }); + + it("retains a peer-only direct attempt through the total operation deadline", () => { + const routes = controller(1, { operationTimeoutMs: 20_000 }); + addViewer(routes, A, 0, 0); + + const operation = routes.reconcile(0).operation!; + beginCandidate(routes, { + nowMs: 100, + connectionId: "only_direct_candidate", + reservation: { kind: "direct" }, + }); + + expect(routes.snapshot().operation?.wakeAtMs).toBe( + operation.deadlineAtMs, + ); + expect(routes.operationExpired(5_100).accepted).toBe(false); + expect(routes.snapshot().operation?.current?.connectionId).toBe( + "only_direct_candidate", + ); + expect(routes.operationExpired(operation.deadlineAtMs)).toMatchObject({ + accepted: true, + failedPeerIds: [A], + }); }); it("keeps only fresh exact-edge quality shadow aggregates", () => { @@ -3294,6 +3324,9 @@ describe("RoomRouteController", () => { connectionId: "a_from_b", reservation: { kind: "direct" }, }).operation!; + expect(routes.snapshot().operation?.wakeAtMs).toBe( + convergence.deadlineAtMs, + ); expect(routes.snapshot().upstreamByViewer.get(A)).toMatchObject({ kind: "sfu", connectionId: "a_sfu_reuse",