Conversation
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #842 +/- ##
==========================================
+ Coverage 65.25% 65.69% +0.44%
==========================================
Files 51 46 -5
Lines 6588 8938 +2350
==========================================
+ Hits 4299 5872 +1573
- Misses 1915 2514 +599
- Partials 374 552 +178 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c5caffb to
fc373f0
Compare
|
Implemented the four requested review fixes in
Compatibility is covered by tests proving ordinary RTP/AVP and SDES-SRTP do not enter DTLS/ICE state. No transcoding was introduced. Server-side validation completed: focused DTLS/ICE/Opus regressions, existing re-INVITE and late-offer tests, race-sensitive DTLS teardown tests, legacy RTP/SDES isolation under the race detector, and the full Operational diagnosis: the affected call had bidirectional DTLS-SRTP media until inbound RTP stopped, followed by a media timeout and a five-minute teardown delay. The re-INVITE transport reuse and cancel-safe teardown changes directly cover those failure modes. |
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if ufrag, hasUfrag := mediaAttribute(m, "ice-ufrag"); hasUfrag { | ||
| pwd, hasPwd := mediaAttribute(m, "ice-pwd") |
There was a problem hiding this comment.
🟡 Session-level ICE credentials bypass ICE
Session-level ICE credentials make parseDTLSOffer treat the offer as non-ICE. DTLS targets the media address directly, so ICE-dependent calls lose media.
Learn more
ICE credentials can appear at session level and be overridden at media level. This parser only looks for media-level ice-ufrag and ice-pwd, unlike its fingerprint and setup handling. A conforming offer with session-level credentials therefore produces out.ice == nil. The transport then skips connectICE and starts DTLS against the SDP media address, which does not work when media requires ICE nomination.
Example: An offer contains session-level a=ice-ufrag:meta1 and a=ice-pwd:secret, plus a media-level candidate. The answer omits ICE attributes and the service sends DTLS directly; the expected behavior is an ICE-controlled connection followed by DTLS.
Recommended fix: Resolve each ICE credential from the media attributes first, then fall back to the session attributes. Reject offers where only one resolved credential exists, and parse media candidates whenever resolved credentials are present.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func (m *dtlsMux) Close() error { | ||
| m.once.Do(func() { close(m.closed); _ = m.dtls.Close(); _ = m.srtp.Close(); _ = m.srtcp.Close() }) | ||
| return nil |
There was a problem hiding this comment.
🟡 Rebuilt DTLS transports retain stale readers
A non-ICE transport rebuild leaves readLoop blocked on the reusable UDP socket. The stale reader competes with the replacement mux and discards its packets.
Learn more
A DTLS mux owns a goroutine blocked in conn.Read. Closing only its endpoint channels does not interrupt that read or wait for the goroutine. A changed fingerprint, setup role, or DTLS mode causes configure to close the old pipeline and immediately build another mux over the same udpConn. Both read loops then consume the same UDP socket, but the old loop routes packets to closed endpoints and drops them.
Example: A direct DTLS call receives a re-INVITE with a new fingerprint. The old mux remains blocked while the new pipeline starts. Subsequent DTLS or SRTP datagrams can be consumed and discarded by the old mux instead of reaching the new handshake.
Recommended fix: Give dtlsMux.Close an ownership-aware way to interrupt conn.Read, and wait for readLoop to exit before reusing the socket. For the durable udpConn, use its soft-close/deadline mechanism and reopen it only after the old mux has stopped; do not close the underlying OS socket during rebuilds.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
Adds opt-in Opus and inbound WebRTC-style DTLS-SRTP support, including ICE-lite interoperability required by Meta WhatsApp Business Calling.
Changes
Configuration
Both features default to disabled.
Validation
maingo test ./pkg/sip -count=1passes