Repository navigation
Follow TURN 300 Try Alternate redirect for Realm connections - #29
extremeheat merged 3 commits into
Conversation
extremeheat
left a comment
There was a problem hiding this comment.
Thanks @Pix3lPirat3 — the comparison against native WebRTC and the successful Realm join/reconnect make the diagnosis convincing. CI is green on all three platforms, and the four new URL/cache tests also pass locally. A temporary workaround through ICE configuration is reasonable, but I would fix these issues before merging this PoC:
-
Validate the probe response before accepting a redirect (
src/turnRedirect.js:47–55). The request transaction ID is discarded, and the first received datagram ends the probe without checking sender, message type, magic cookie, transaction ID, or declared message/attribute bounds. I reproduced acceptance of a fabricated 300 response sent by a different local UDP socket, with a zero cookie, zero transaction ID and invalid response type; it returned the supplied alternate address. Preserve the request ID, match the queried endpoint and Allocate response, check framing, and ignore unrelated/malformed datagrams until a valid response or timeout. This is ordinary STUN response matching, not optional extra validation. Add local UDP tests that actually exercise the probe; the current tests seed the cache and never run it. -
Cancel or discard stale pre-resolution (
src/client.js:228). The new await occurs before a peer exists, soclose()cannot dispose of anything yet. After it resolves,createOffer()still creates a peer without checking whether the client closed, negotiation timed out, or a newer attempt replaced it. I reproduced a peer-constructor call afterclient.close()using a silent local TURN endpoint and valid dummy TURN credentials. Tie the probe to the attempt's lifetime, recheck that attempt after awaiting, and keep rewritten ICE servers local to it instead of assigning an old asynchronous result back intothis.credentials. Cover close and timeout/retry during probing. -
Respect the selected backend and TURN transport (
src/client.js:225–228,src/turnRedirect.js:67–73).turn:also permits?transport=tcp; this implementation probes it over UDP and can rewrite it using a UDP-only alternate. I reproduced a TCP URL being rewritten from the host:port cache. Skip TCP/TLS/unsupported address forms in this UDP-specific workaround, and apply it only when the selected backend is Werift (includingautoresolving to Werift). Native currently also waits for and consumes the pre-resolution, so it is not actually unaffected. The existingREQUESTED-TRANSPORT=UDPattribute describes the relay allocation; it does not make the client-to-TURN transport UDP. -
Avoid permanent caching of failures and stale alternates (
src/turnRedirect.js:70–79). A single timeout is cached asnullfor the entire process, so future Realm attempts never retry the lookup even after the network recovers. Positive results also never expire. At minimum do not cache probe failures; give successful redirects a bounded lifetime (and keep any cache bounded). Add a test where an initial failed probe is followed by a successful redirect.
Please retain the live Realm join/reconnect check after these changes. I also support a proper Werift upstream fix so this workaround can eventually disappear; we do not need to implement a general TURN client here.
Reviewed at 892aad0. These local checks establish the edge cases above; I have not independently repeated the authenticated Realm test.
— OpenAI Astra
The findings from this review are addressed by 70b69bb, which I implemented at the maintainer’s request. All 54 tests and Linux/macOS/Windows CI pass. Clearing my stale change request; this is not an independent approval of my follow-up changes.
|
@Pix3lPirat3 I've pushed the review fixes directly to your PR branch as
Lint, TypeScript and 54 tests pass, including local UDP tests exercising the actual probe and the cancellation races. Linux, macOS and Windows CI are green: https://github.com/PrismarineJS/node-nethernet/actions/runs/35710481348. I cleared my previous change request because those findings are addressed; I authored the follow-up, so this is not a separate independent approval. When convenient, could you reconfirm Realm join/reconnect with this exact commit? We haven't repeated the authenticated live test ourselves. This PR is still open, and Bedrock's pin has not changed yet. — OpenAI Astra |
|
Reconfirmed on
Both reach the world and spawn, so the validated probe + attempt-lifetime cancellation did not regress the Realm path. One note in case it matters for the pin bump + integration CI: my Realm is on 1.26.51, so at bedrock-protocol's default I did not exercise the native |
|
/makerelease patch |
Realm joins time out on the default Werift backend because Microsoft's TURN gateway replies to Allocate with
300 Try Alternate, which Werift 0.24.4 does not follow. Native WebRTC already handles this redirect.For Werift clients only, pre-resolve IPv4 UDP TURN redirects before creating the peer and pass the resulting ICE configuration to unmodified Werift. The probe matches the remote endpoint and STUN transaction, validates response framing, ignores unrelated/malformed packets, and stops when the connection attempt closes or times out. TCP/TLS/IPv6 URLs and native WebRTC bypass it. Failed probes preserve the original URL.
Redirects are not cached: each connection gets a fresh result, avoiding permanent failures and stale relay assignments. Probes run concurrently with a three-second limit per endpoint, bounded by the connection attempt. Results remain local to their attempt rather than overwriting shared credentials.
Pixel verified the original proof of concept against a live Realm: join and reconnect both spawned on Werift, whereas the unpatched backend timed out. The subsequent review fixes still need the same live confirmation.
Validation at
70b69bb: lint, strict TypeScript, and 54 local runtime tests pass, including all four Werift/native backend combinations and local UDP regression tests for response matching, malformed packets, transport selection, repeat lookups, cancellation, and timeout/retry races. Remote CI runs on Linux, macOS, and Windows.After this merges, bedrock-protocol's pinned node-nethernet dependency needs updating and integration testing. A proper Werift upstream fix remains the preferred long-term solution.
Original diagnosis and live proof of concept by @Pix3lPirat3; review fixes implemented by OpenAI Astra at the maintainers' request.