feat: auto retry with only TURN-TLS if UDP connects but fails DTLS#5080
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15584fb5d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 165657c223
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1997fc57fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -167,7 +169,7 @@ Media.createMediaConnection = ( | |||
| const iceServers = []; | |||
|
|
|||
| // we might not have any TURN server if TURN discovery failed or wasn't done or we land on a video mesh node | |||
| if (turnServerInfo?.urls.length > 0) { | |||
| if (turnServerInfo && turnServerInfo.urls.length > 0) { | |||
There was a problem hiding this comment.
q: why not turnServerInfo?.urls?.length > 0 ?
There was a problem hiding this comment.
if I remove turnServerInfo check here, then we have to add ?. to lines 175-177 too, so I prefer it this way
|
|
||
| const results: ReachabilityResults = JSON.parse(resultsJson); | ||
|
|
||
| return Object.values(results).some((result) => result[protocol]?.result === 'reachable'); |
There was a problem hiding this comment.
question: does it make a difference if connected via VMN ? This would trigger a Turn TLS connection attempt if the DTLS Handshake failed on a VMN, right?
There was a problem hiding this comment.
VMN doesn't support TURN-TLS, so if we land on a VMN node, we won't get any TURN-TLS url and the check inside establishMediaConnection() will set iceTransportPolicy back to undefined. I guess the only issue is that we will send a misleading metric with iceTransportPolicy still set to 'relay'. I'll see if maybe we can avoid that without complicating the code too much.
There was a problem hiding this comment.
nitpick: @jor-row asked this question, so it means it's not immediately obvious re: VMN not supporting turn TLS so maybe we'd be well off to add that comment here (or elsewhere near the 'relay')?
There was a problem hiding this comment.
I've added a comment to shouldRetryMediaWithOnlyTurnTLS() to explain the VMN case
There was a problem hiding this comment.
I've also fixed the misleading metric - I had to store iceTransportPolicy in Meeting.addMediaData, so that it can be modified in establishMediaConnection() and the metrics sent from addMediaInternal() use the latest value.
chrisadubois
left a comment
chrisadubois
left a comment
There was a problem hiding this comment.
PR looks good, please ignore my comments if you please as they are basically all about comments, which probably more comes from a place of wanting to make sure it's readable to somebody *who doesn't know what they're doing in here" which is not too important.
I did not get a chance to test this yet, I'd like to but tbh it's a failure case fallback and that is quite clear in the code so I'm happy to do some "deeper" testing on this once it merges. I'd love to get some info from you on doing this via pcap. I plan on writing a proxy that does exactly what you say, allow the first few packets of UDP in, and then drop after that (unless you already have a tool for this).
Approving.
| @@ -167,7 +169,7 @@ Media.createMediaConnection = ( | |||
| const iceServers = []; | |||
|
|
|||
| // we might not have any TURN server if TURN discovery failed or wasn't done or we land on a video mesh node | |||
| if (turnServerInfo?.urls.length > 0) { | |||
| if (turnServerInfo && turnServerInfo.urls.length > 0) { | |||
There was a problem hiding this comment.
q: why not turnServerInfo?.urls?.length > 0 ?
| @@ -187,6 +189,10 @@ Media.createMediaConnection = ( | |||
| config.bundlePolicy = bundlePolicy; | |||
| } | |||
|
|
|||
| if (iceTransportPolicy) { | |||
There was a problem hiding this comment.
nitpick: I'd love a comment on here for iceTransportPolicy, i.e., what it is, since this is a top level and we're adding a new config value.
There was a problem hiding this comment.
the type is clear that it's a browser level thing - RTCIceTransportPolicy, so then the natural thing to do is to google it and see the MDN page which will have a lot more and up-to-date info than what I can fit in a comment
| // drop the forced relay policy and use the default, because we don't have a TURN server | ||
| iceTransportPolicy = undefined; | ||
| } | ||
| const mc = await this.createMediaConnection(turnServerInfo, bundlePolicy, iceTransportPolicy); |
There was a problem hiding this comment.
nitpick: mediaConnection =
| false, | ||
| undefined, |
There was a problem hiding this comment.
nitpick: would love to comment the undefined parameters we pass, they are ALWAYS hard to read at first glance without tracing into the function call params and matching them up
|
|
||
| const results: ReachabilityResults = JSON.parse(resultsJson); | ||
|
|
||
| return Object.values(results).some((result) => result[protocol]?.result === 'reachable'); |
There was a problem hiding this comment.
nitpick: @jor-row asked this question, so it means it's not immediately obvious re: VMN not supporting turn TLS so maybe we'd be well off to add that comment here (or elsewhere near the 'relay')?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c177f22ee2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 599c07bf09
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ( | ||
| this.isMultistream && | ||
| prevError instanceof AddMediaFailed && | ||
| prevError.isDtlsHandshakeFailure && |
There was a problem hiding this comment.
When ICE reaches connected/completed before MediaConnectionAwaiter.waitForMediaConnectionConnected() installs its ICE listener (possible immediately after the remote answer is processed), the awaiter never samples that current state and only flips iceConnected on subsequent ICE state events. This new isDtlsHandshakeFailure gate then sees false on the timeout and skips the intended TURN-TLS relay retry for exactly the UDP-connected/DTLS-stalled case; initialize the flag from getIceConnectionState() when starting the await.
Useful? React with 👍 / 👎.
| * | ||
| * For clients landing on video mesh nodes (VMN), reachability might still have successful TLS (to public nodes) | ||
| * but TURN discovery will result in empty TURN urls array and iceTransportPolicy='relay' will still be overriden to undefined, | ||
| * see the code in establishMediaConnection() |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b19187cfdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| `${LOG_HEADER} cannot do iceTransportPolicy=relay, because there is no turn server info available` | ||
| ); | ||
| // drop the forced relay policy and use the default, because we don't have a TURN server | ||
| this.addMediaData.iceTransportPolicy = undefined; |
There was a problem hiding this comment.
When TURN discovery returns the VMN/empty-URL shape on a relay retry, this branch correctly falls back to the default ICE policy, but doTurnDiscovery() has already left this.turnServerUsed as true because there was no skip reason. If that fallback connection then fails, the CA error-code path and ADD_MEDIA_FAILURE still classify the attempt as having used TURN even though createMediaConnection() was given no TURN servers; please clear turnServerUsed here as well so the effective fallback is reported and handled as default/direct ICE.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
yes, it's an issue, but pre-existing and out of scope for this PR
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9be95d43b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| undefined, | ||
| false, | ||
| undefined /* turnServerInfo - it will be fetched via doTurnDiscovery() */, | ||
| undefined /* iceTransportPolicy - we're relying on the default value */, |
There was a problem hiding this comment.
When an app uses the public join() + addMedia() flow rather than joinWithMedia(), a UDP-connected/DTLS-timeout failure is now wrapped with the metadata needed for the TURN-TLS-only retry, but this path always passes undefined for iceTransportPolicy and never calls shouldRetryMediaWithOnlyTurnTLS (that helper is only invoked from joinWithMedia). In that scenario addMedia() still propagates the AddMediaFailed after the normal forced-TURN attempt instead of doing the new relay-only retry, so the new recovery path is unavailable for the standard public add-media flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
that's ok, we don't need this for join + addMedia flows
Thank you for your contribution! |
3 participants
COMPLETES #SPARK-829219
This pull request addresses
We see cases where UDP ICE connection succeeds, but DTLS handshake then fails. It may be that the firewall/proxy is letting only some initial packets through, so STUN goes through, but DTLS fails. Also historically there have been issues in Homer were Homer would sometimes send DTLS packets on a different transport to the one that browser is expecting. Normally we have UDP on ports 5004 and 9000, TCP and TLS as potential transports, so if all pass connectivity checks, there is room for Homer to do something wrong. By doing a retry on a single transport (TLS), we minimize the chances for that. I've decided to use TLS, because that one has the highest chance of getting through.
by making the following changes
Changed the logic in Meeting.joinWithMedia() so that if all the conditions are met:
then we do an automatic retry with iceTransportPolicy set to 'relay', so browser only uses relay candidates, so we will only connect over TURN-TLS.
Also had to improve types on some of the errors being thrown when we fail waiting for media connection to be established to be able to pass the info about connection type and ice connection status in the error.
Change Type
The following scenarios were tested
unit tests
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.