olcRTC: connect end-to-end over a public meet carrier (signaling + peer-connection fixes) - #119
Merged
Merged
Conversation
The sidecar's protected resolver + http.DefaultTransport dialer (used for the Jitsi XMPP websocket/BOSH signaling) dialed whatever the resolver returned. For a carrier host that also publishes an AAAA (e.g. framatalk.org -> 2a01:4f8:231:1213::2) the protected socket, marked for the physical interface, dialed the v6 address; on a v4-only physical path that SYN is blackholed and the dial hangs silently until the readiness deadline (observed on device: the sidecar logs 'joining MUC' then nothing, then 'wait for peer: context deadline exceeded' after 60s). A carrier with no usable AAAA (meet.ffmuc.net) joined fine, and a normal host joins framatalk fine via Happy Eyeballs v4 fallback — which the custom protected dialer does not do. Force the protected resolver lookups and the signaling dials to tcp4/udp4 so a published AAAA can never cause a silent hang on a v4-only routed path. Media/ICE (pion) already falls back to v4 candidates. Verified root cause on-device (framatalk AAAA + no global IPv6 on the test WiFi) and on the server (signaling joined over v4; only ICE tried v6 and fell back).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe protected resolver and HTTP transport now force IPv4 dialing where appropriate, apply TCP tuning on protected sockets, add HTTP timing settings, and enable debug-only request tracing with TCP MSS logging. ChangesProtected dial and trace setup
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
buildScript/lib/olcrtc-src/main.go (1)
104-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify that IPv4 selection comes from
forceTCP4, not the resolver
Resolver.Dialonly controls how DNS is reached; it doesn’t make lookups A-only. Reword this to attribute the IPv6 avoidance to the transport-levelforceTCP4on the target dial.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@buildScript/lib/olcrtc-src/main.go` around lines 104 - 109, Clarify the IPv4-only comment by attributing IPv6 avoidance to the transport-level forceTCP4 behavior rather than Resolver.Dial. Update the explanatory comment near the IPv4-only signaling dial logic in main.go so it states that forceTCP4 on the target dial keeps the connection on tcp4 and prevents hanging on unreachable AAAA addresses, while Resolver.Dial only affects how DNS is reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@buildScript/lib/olcrtc-src/main.go`:
- Around line 104-109: Clarify the IPv4-only comment by attributing IPv6
avoidance to the transport-level forceTCP4 behavior rather than Resolver.Dial.
Update the explanatory comment near the IPv4-only signaling dial logic in
main.go so it states that forceTCP4 on the target dial keeps the connection on
tcp4 and prevents hanging on unreachable AAAA addresses, while Resolver.Dial
only affects how DNS is reached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d094a90-4495-43d3-ad08-db162a0f148b
📒 Files selected for processing (1)
buildScript/lib/olcrtc-src/main.go
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… working - forceTCP4 -> forceIPv4Network: the helper also rewrites udp/udp6, so the old name understated its scope. - Skip the udp4/tcp4 rewrite for resolver queries when -dns is an IPv6 literal (zone-scoped literals handled via netip.ParseAddr); forcing a v4 transport toward a v6 server would fail every lookup. - Clarify the doc comment: A-only lookups come from the transport dialing tcp4, not from the resolver rewrite.
…meouts The IPv4 forcing did not resolve the observed device stall where the sidecar logs 'joining MUC' then reads nothing for the full readiness window. The signature (host-specific, network-specific, IPv4-indifferent, zero output until deadline) is a reduced-MTU path dropping an oversized inbound TLS certificate flight mid-handshake, so the read blocks. Clamp TCP_MAXSEG to 1200 on the protected signaling sockets in the existing socket Control hook so those inbound segments fit on a routed path. Also restore explicit transport timeouts (add ResponseHeaderTimeout; TLSHandshakeTimeout was already set) so a future stall surfaces as a typed error in seconds instead of blocking silently, and add a debug-only httptrace RoundTripper that logs each request's connection stages (dns / connect / tls / headers / first byte) to attribute a stall to a specific stage. Trace is gated behind -debug (it logs request URLs).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@buildScript/lib/olcrtc-src/main.go`:
- Line 192: The shared default HTTP transport is forcing HTTP/2, which can
interfere with websocket Upgrade handshakes used through the j library’s
`http.DefaultClient`. Update the transport configuration in `main.go` to avoid
enabling HTTP/2 on the shared default transport, either by setting
`ForceAttemptHTTP2` off or by moving websocket traffic to a separate transport.
Use the existing transport setup near `ForceAttemptHTTP2` to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 471ff651-88ea-4574-9482-4c15a96a0d71
📒 Files selected for processing (1)
buildScript/lib/olcrtc-src/main.go
…stall) Device stage-tracing pinpointed the stall: framatalk.org negotiates HTTP/2 via ALPN on the first signaling connection, and the XMPP client's WebSocket upgrade cannot ride an h2 connection, so the upgrade request hangs with no response until the header timeout; only a later HTTP/1.1 attempt returns the 101 and the XMPP stream opens (trace showed tls-done proto=h2 -> wrote-headers -> [15s silence] -> retry with proto="" -> first-byte -> full XMPP flow). A carrier that serves the signaling over h1 (meet.ffmuc.net) never hit this. Disable HTTP/2 on the signaling transport (ForceAttemptHTTP2=false + non-nil empty TLSNextProto) so the WebSocket upgrade always uses h1 and connects on the first attempt, removing the wasted round that was eating into the readiness window. The MSS clamp, restored timeouts, and debug stage-tracing from the previous commit stay.
A device run with the MSS=1200 clamp still stalled ~15s on the first large inbound payload (GET /config.js body) and got no reply to the Jicofo conference IQ (and no stream-management ack either) — the classic signature of a reduced-MTU inbound blackhole with TCP head-of-line blocking: small inbound stanzas (bind/session/enabled) arrive, large ones (config body, Jicofo's conference reply) are dropped and stall the whole stream. Small inbound payloads working while large ones stall, in the same direction, size-correlated, points at the advertised MSS not being small enough for the path (or not applied). Lower TCP_MAXSEG to 1000, surface the setsockopt error under -debug, and log the live connection's TCP_INFO (advmss/rcv_mss/snd_mss) in the debug trace so the next run proves whether the clamp reached the SYN (advmss ~= 1000 vs the ~1460 default).
The advmss/rcv_mss diagnostic asserted syscall.Conn directly on the connection handed to the httptrace GotConn hook. That connection is a *tls.Conn, which exposes NetConn() rather than SyscallConn(), so the assertion always failed and the line was silently omitted. Unwrap the TLS layer via NetConn() first so the trace actually reports the values; log the concrete type when the unwrap still does not yield a syscall.Conn.
Two diagnostic-driven changes on the protected signaling sockets, both debug-gated via the existing trace path: - Set SO_RCVBUF pre-connect (32 KiB). The TCP window scale is fixed from the receive buffer at SYN time, so a small buffer shrinks the advertised window and forces the peer to deliver its response in several small flights rather than one large burst. This targets a middlebox that mishandles window scaling or polices large bursts on the routed path, whose signature (small inbound fine, large inbound stalls even with the segment-size clamp proven on the wire) matches what the trace shows. - Poll TCP_INFO on the connection every 500ms for ~16s after it is established, logging bytes_received / segs_in / rcv_ooopack / rcv_wnd. During a large-inbound stall this distinguishes 'nothing arrives' (return-path drop, no client fix) from 'segments arrive out of order with a hole at the front' (size-selective drop). The protected socket bypasses the tun, so this is the only on-device observation available to an unprivileged process.
Point the olcRTC sidecar build at the hawkff fork at the commit that adds
ICE-server URL sanitization. Deployments whose XEP-0215 disco advertises
TURN/STUN without a port previously made pion reject the peer connection
config ("invalid port") before the data channel could start; the fork drops
those malformed entries so the media path constructs. Repo + commit are set
in both the build script default and the CI workflow env.
hawkff
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the olcRTC sidecar connect end-to-end in VPN/tun mode against a public
anonymous meet carrier, verified on Android. The connect previously stalled at
signaling on one carrier and then failed at peer-connection setup on another; this
PR lands the signaling robustness work plus the peer-connection fix.
Changes
buildScript/lib/olcrtc-src/main.go): theprotected signaling dials are pinned to HTTP/1.1 (the WebSocket upgrade cannot ride
an h2 connection), forced to IPv4 on the physical path, MSS-clamped, and given
explicit handshake/response timeouts. A debug-only stage trace (httptrace + a
TCP_INFOpoll, unwrapping the TLS conn to read the advertised MSS) makes any stallattributable to a specific stage instead of surfacing as silence. All of this is
gated behind
-debug.commit that sanitizes the ICE server list before building the peer connection. Some
deployments advertise TURN/STUN over service discovery without a port; the resulting
URL has an empty port and the WebRTC stack rejected the whole config before any
candidate was added. Dropping the malformed entries lets the connection construct;
routable host candidates plus the bridge channel establish the data channel without
those relays. Pin updated in both
buildScript/lib/olcrtc.shand.github/workflows/ci.yml.Verification
server: the peer connection establishes, the data channel opens, the session pairs,
and real traffic egresses through the tunnel (server-side traffic counters confirm
bytes in/out for live destinations). No local data was wiped; the release-signed CI
APK installs over the prior build.