fix: SDP correctness, RTP goroutine race condition, and non-RFC SRC compatibility#35
Conversation
…ardown Fixes for production use with Cognigy Voice Gateway (jambonz-based SRC): PLAIN_SDP_RESPONSE mode (Fix 8): - Add PLAIN_SDP_RESPONSE env var to bypass multipart/mixed 200 OK - jambonz cannot parse rs-metadata XML in 200 OK body, silently skips media forking; plain application/sdp response unblocks RTP delivery - Add "os" import to custom_server.go Race condition fix (Fix 9): - BYE handler called Cleanup() (closes WAV file) while RTP goroutine was still in processPacket, causing "file already closed" write errors - Add doneChan/doneOnce/WaitDone() to RTPForwarder so BYE handler can wait for goroutine to exit its write path before calling Cleanup() - Restructure defers in StartRTPForwarding: WAV Finalize → close(doneChan) → Cleanup → log; doneChan fires after last write but before Azure upload, keeping WaitDone(100ms) well within budget - Replace bare Stop()+Cleanup() pairs in finalizeCall with stopAndCleanup() helper that inserts the WaitDone barrier Prior Cognigy compatibility fixes (already in working tree): - parser.go: downgrade missing sessionrecordingassoc to warning - sdp.go: fix recvonly→sendonly direction, use own SRS origin identity - types.go: add SDPStreamLabels field for correct rs-metadata stream labels - custom_server.go: populate SDPStreamLabels from actual a=label values, log SDP offer and full 200 OK body for diagnostics - sdp_utils.go: pre-populate RemoteRTPAddr from SDP offer for RTP probe - rtp.go: send initial RTP probe after bind for symmetric-RTP latching Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds ChangesSIPREC RTP Forwarder Lifecycle, Probe, SDP, and Response Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/sip/custom_server.go (1)
1950-1996: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply
PLAIN_SDP_RESPONSEto re-INVITEs too.The initial INVITE path can return
application/sdp, buthandleSiprecReInvitestill sends multipart wheneverRecordingSessionexists. SRCs that need plain SDP will fail again on the first re-INVITE.Sketch of the corresponding re-INVITE branch
+ plainSDPMode := os.Getenv("PLAIN_SDP_RESPONSE") == "true" // Generate SIPREC response if we have a recording session - if callState.RecordingSession != nil { + if callState.RecordingSession != nil && !plainSDPMode { contentType, multipartBody, err := s.generateSiprecResponse(responseSDP, callState.RecordingSession, logger) ... s.sendResponse(message, 200, "OK", responseHeaders, []byte(multipartBody)) } else { + responseHeaders["Content-Type"] = "application/sdp" callState.State = "awaiting_ack" callState.PendingAckCSeq = callState.RemoteCSeq callState.LastActivity = time.Now() s.sendResponse(message, 200, "OK", responseHeaders, responseSDP) }🤖 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 `@pkg/sip/custom_server.go` around lines 1950 - 1996, The plain-SDP bypass in the initial INVITE flow is not mirrored in re-INVITEs, so `handleSiprecReInvite` still generates multipart responses whenever a `RecordingSession` exists. Update the re-INVITE path to check `PLAIN_SDP_RESPONSE` the same way as the main invite handling, and if it is enabled, skip `generateSiprecResponse` and return `application/sdp` only; use the existing `handleSiprecReInvite`, `generateSiprecResponse`, and `RecordingSession` symbols to keep the behavior consistent across both paths.
🧹 Nitpick comments (1)
pkg/media/types.go (1)
107-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the
WaitDonecontract wording.
doneChanis closed after RTP writing/WAV finalization, but before the goroutine-ownedCleanup()/exit defers inStartRTPForwarding. Either move the close to the true final-exit point, or document this as “safe to cleanup recording resources” rather than “fully exited”.Suggested wording-only fix
- // doneChan is closed when the StartRTPForwarding goroutine has fully exited. - // Callers that need to ensure the goroutine is done before calling Cleanup() - // can select/wait on this channel (see WaitDone). + // doneChan is closed after RTP writing has stopped and WAV finalization has + // completed. Callers can wait on this before calling Cleanup() to avoid + // closing recording resources while the RTP loop is still writing. doneChan chan struct{}-// WaitDone blocks until the StartRTPForwarding goroutine has fully exited or -// the timeout elapses. Returns true if the goroutine exited, false on timeout. +// WaitDone blocks until RTP writing has stopped and recording finalization has +// completed, or until the timeout elapses. Returns true if finalization +// completed, false on timeout.Also applies to: 243-245
🤖 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 `@pkg/media/types.go` around lines 107 - 110, The current `WaitDone`/`doneChan` comment in `pkg/media/types.go` overstates what the signal means; it is not closed only after `StartRTPForwarding` has fully exited because goroutine-owned cleanup still runs afterward. Update the documentation near `doneChan`, `WaitDone`, and the `StartRTPForwarding` flow to say the channel indicates it is safe to clean up recording resources after RTP/WAV finalization, or alternatively move the close to the true final-exit point if that better matches the intended contract.
🤖 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 `@pkg/media/sdp_utils.go`:
- Around line 41-58: Populate RemoteRTCPAddr from the full a=rtcp hint in the
SDP parser helper: in the logic that reads md.Attributes, capture both the RTCP
port and any host/IP specified after the port (for example “a=rtcp:9001 IN IP4
203.0.113.10”), and use that address instead of always reusing the RTP IP; also
update the rtcp-mux path so RTCP is pointed at the RTP port as well. Keep the
change localized to the SDP parsing/forwarder address assignment flow in the
helper that sets forwarder.RemoteRTPAddr and forwarder.RemoteRTCPAddr.
In `@pkg/sip/custom_server.go`:
- Around line 1846-1854: Refresh the recordingSession SDP stream labels on every
SDP negotiation instead of only appending in generateSiprecResponse. Update the
logic around recordingSession.SDPStreamLabels so it is cleared and rebuilt from
the current audioStreams/forwarders state each time a response is generated,
ensuring generateSiprecResponse and any rs-metadata output always use the latest
labels after re-INVITE changes.
- Line 1663: Avoid logging the full SDP/SIPREC payload in the SRC receive path;
the current logger.WithField("sdp_offer", string(sdpData)).Info(...) call should
be changed to log only safe metadata such as payload size, a hash, or a
redacted/truncated preview. Apply the same treatment to the other matching
full-payload log sites in custom_server.go, and if full dumps are still needed,
gate them behind an explicit debug-only diagnostic flag rather than Info/Error
logging.
In `@pkg/sip/sdp.go`:
- Around line 418-429: generateSDPAdvanced is creating a fresh SDP Origin on
every re-INVITE answer, which breaks offer/answer continuity. Update the Origin
handling so the session-id stays stable for the same dialog and only
SessionVersion increases on subsequent answers, instead of resetting it to 1.
Use the generateSDPAdvanced flow and its SessionDescription/Origin construction
to locate the fix.
---
Outside diff comments:
In `@pkg/sip/custom_server.go`:
- Around line 1950-1996: The plain-SDP bypass in the initial INVITE flow is not
mirrored in re-INVITEs, so `handleSiprecReInvite` still generates multipart
responses whenever a `RecordingSession` exists. Update the re-INVITE path to
check `PLAIN_SDP_RESPONSE` the same way as the main invite handling, and if it
is enabled, skip `generateSiprecResponse` and return `application/sdp` only; use
the existing `handleSiprecReInvite`, `generateSiprecResponse`, and
`RecordingSession` symbols to keep the behavior consistent across both paths.
---
Nitpick comments:
In `@pkg/media/types.go`:
- Around line 107-110: The current `WaitDone`/`doneChan` comment in
`pkg/media/types.go` overstates what the signal means; it is not closed only
after `StartRTPForwarding` has fully exited because goroutine-owned cleanup
still runs afterward. Update the documentation near `doneChan`, `WaitDone`, and
the `StartRTPForwarding` flow to say the channel indicates it is safe to clean
up recording resources after RTP/WAV finalization, or alternatively move the
close to the true final-exit point if that better matches the intended contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a21d2708-68b0-4d4c-9209-9bc082134370
📒 Files selected for processing (7)
pkg/media/rtp.gopkg/media/sdp_utils.gopkg/media/types.gopkg/sip/custom_server.gopkg/sip/sdp.gopkg/siprec/parser.gopkg/siprec/types.go
| rtpPort := md.MediaName.Port.Value | ||
| rtcpPort := rtpPort + 1 | ||
| for _, attr := range md.Attributes { | ||
| if attr.Key == "rtcp" { | ||
| fields := strings.Fields(attr.Value) | ||
| if len(fields) > 0 { | ||
| if p, err := strconv.Atoi(fields[0]); err == nil { | ||
| rtcpPort = p | ||
| } | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| forwarder.remoteMutex.Lock() | ||
| forwarder.RemoteRTPAddr = &net.UDPAddr{IP: ip, Port: rtpPort} | ||
| forwarder.RemoteRTCPAddr = &net.UDPAddr{IP: ip, Port: rtcpPort} | ||
| forwarder.remoteMutex.Unlock() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Populate RTCP hints from the full a=rtcp attribute.
The helper currently keeps the RTP connection IP for RTCP and only overrides the port. Offers like a=rtcp:9001 IN IP4 203.0.113.10 will produce the wrong RemoteRTCPAddr; rtcp-mux should also point RTCP at the RTP port.
Proposed fix
rtpPort := md.MediaName.Port.Value
+ rtcpIP := ip
rtcpPort := rtpPort + 1
+ if forwarder.UseRTCPMux {
+ rtcpPort = rtpPort
+ }
for _, attr := range md.Attributes {
if attr.Key == "rtcp" {
fields := strings.Fields(attr.Value)
if len(fields) > 0 {
if p, err := strconv.Atoi(fields[0]); err == nil {
rtcpPort = p
}
}
+ if len(fields) >= 4 {
+ if parsed := net.ParseIP(fields[3]); parsed != nil {
+ rtcpIP = parsed
+ }
+ }
break
}
}
forwarder.remoteMutex.Lock()
forwarder.RemoteRTPAddr = &net.UDPAddr{IP: ip, Port: rtpPort}
- forwarder.RemoteRTCPAddr = &net.UDPAddr{IP: ip, Port: rtcpPort}
+ forwarder.RemoteRTCPAddr = &net.UDPAddr{IP: rtcpIP, Port: rtcpPort}
forwarder.remoteMutex.Unlock()🤖 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 `@pkg/media/sdp_utils.go` around lines 41 - 58, Populate RemoteRTCPAddr from
the full a=rtcp hint in the SDP parser helper: in the logic that reads
md.Attributes, capture both the RTCP port and any host/IP specified after the
port (for example “a=rtcp:9001 IN IP4 203.0.113.10”), and use that address
instead of always reusing the RTP IP; also update the rtcp-mux path so RTCP is
pointed at the RTP port as well. Keep the change localized to the SDP
parsing/forwarder address assignment flow in the helper that sets
forwarder.RemoteRTPAddr and forwarder.RemoteRTCPAddr.
| s.sendResponse(message, 488, "Not Acceptable Here", nil, nil) | ||
| return | ||
| } | ||
| logger.WithField("sdp_offer", string(sdpData)).Info("Received SDP offer from SRC") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging full SIPREC/SDP payloads at info/error level.
These fields can include participant identifiers, phone numbers/AORs, UUI/vendor IDs, and internal media IPs. Log sizes/hashes or redact/truncate; keep full payload dumps behind an explicit debug-only diagnostic flag.
Proposed safer logging pattern
- logger.WithField("sdp_offer", string(sdpData)).Info("Received SDP offer from SRC")
+ logger.WithField("sdp_offer_size", len(sdpData)).Debug("Received SDP offer from SRC")- logger.WithField("sdp_answer", string(responseSDP)).Info("SDP answer sent to SRC")
+ logger.WithField("sdp_answer_size", len(responseSDP)).Debug("SDP answer generated for SRC")- s.logger.WithField("raw_metadata", string(rsMetadata)).Error("SIPREC metadata validation failed – raw XML body")
+ s.logger.WithField("metadata_size", len(rsMetadata)).Error("SIPREC metadata validation failed")- logger.WithField("multipart_200ok_body", multipartBody).Info("Full 200 OK multipart body sent to SRC")
+ logger.WithField("multipart_200ok_size", len(multipartBody)).Debug("Generated 200 OK multipart body")Also applies to: 1892-1892, 4196-4196, 4867-4867
🤖 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 `@pkg/sip/custom_server.go` at line 1663, Avoid logging the full SDP/SIPREC
payload in the SRC receive path; the current logger.WithField("sdp_offer",
string(sdpData)).Info(...) call should be changed to log only safe metadata such
as payload size, a hash, or a redacted/truncated preview. Apply the same
treatment to the other matching full-payload log sites in custom_server.go, and
if full dumps are still needed, gate them behind an explicit debug-only
diagnostic flag rather than Info/Error logging.
| // Store the actual SDP stream labels so the RS-metadata response uses matching labels | ||
| if recordingSession != nil { | ||
| for idx := range forwarders { | ||
| label := audioStreams[idx].label | ||
| if label == "" { | ||
| label = fmt.Sprintf("%d", idx+1) | ||
| } | ||
| recordingSession.SDPStreamLabels = append(recordingSession.SDPStreamLabels, label) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Refresh SDP stream labels on every SDP negotiation.
generateSiprecResponse trusts session.SDPStreamLabels, but this slice is only appended during the initial INVITE. If a re-INVITE adds/removes/relabels streams, the rs-metadata response can return stale labels. Reset and repopulate it from the current SDP before each response.
Also applies to: 4819-4828
🤖 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 `@pkg/sip/custom_server.go` around lines 1846 - 1854, Refresh the
recordingSession SDP stream labels on every SDP negotiation instead of only
appending in generateSiprecResponse. Update the logic around
recordingSession.SDPStreamLabels so it is cleared and rebuilt from the current
audioStreams/forwarders state each time a response is generated, ensuring
generateSiprecResponse and any rs-metadata output always use the latest labels
after re-INVITE changes.
| // Use our own origin (not mirroring the SRC's origin) to avoid loop detection | ||
| // on Cognigy/jambonz media stacks that compare session IDs. | ||
| sessionDesc := &sdp.SessionDescription{ | ||
| Origin: sdp.Origin{ | ||
| Username: receivedSDP.Origin.Username, | ||
| SessionID: receivedSDP.Origin.SessionID, | ||
| SessionVersion: receivedSDP.Origin.SessionVersion, | ||
| Username: "siprec-srs", | ||
| SessionID: uint64(time.Now().UnixMicro()), | ||
| SessionVersion: 1, | ||
| NetworkType: "IN", | ||
| AddressType: "IP4", | ||
| UnicastAddress: connectionAddr, // Use NAT-aware address | ||
| UnicastAddress: connectionAddr, | ||
| }, | ||
| SessionName: receivedSDP.SessionName, | ||
| SessionName: sdp.SessionName("SRS Recording Session"), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm time import in sdp.go
rg -nP '"time"' pkg/sip/sdp.go
# Find callers of generateSDPAdvanced to see if it serves re-INVITEs
rg -nP '\bgenerateSDPAdvanced\s*\(' --type=go -C3
# Check for re-INVITE / UPDATE handling that regenerates SDP
rg -nP '(re-?INVITE|reinvite|handleReInvite|UPDATE)' --type=go -C2 pkg/sipRepository: loreste/siprec
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the generator definition and all call sites
rg -n -C 3 '\bgenerateSDPAdvanced\s*\(' pkg/sip
# Inspect SIP dialog / re-INVITE / UPDATE handling around the relevant code paths
rg -n -C 3 '(re-?INVITE|reinvite|UPDATE|SessionVersion|SessionID|Origin: sdp\.Origin)' pkg/sip
# Show the surrounding implementation of generateSDPAdvanced
FILE=$(rg -l '\bgenerateSDPAdvanced\s*\(' pkg/sip | head -n 1)
if [ -n "${FILE:-}" ]; then
echo "=== ${FILE} ==="
sed -n '1,260p' "$FILE"
fiRepository: loreste/siprec
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the SDP generator and nearby wrappers with line numbers
sed -n '100,230p' pkg/sip/sdp.go
# Read the SIPREC re-INVITE response path that calls the generator
sed -n '2430,2490p' pkg/sip/custom_server.go
# Read the UPDATE response path if it generates SDP too
sed -n '2969,3060p' pkg/sip/custom_server.goRepository: loreste/siprec
Length of output: 10251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the full origin/session handling inside the SDP generator
sed -n '230,480p' pkg/sip/sdp.go
# Inspect any helpers that might preserve or reuse SDP origin/version
rg -n -C 3 'SessionVersion|SessionID|Origin: sdp\.Origin|copy.*Origin|increment.*Version|marshal.*Origin' pkg/sip/sdp.go pkg/sip/custom_server.goRepository: loreste/siprec
Length of output: 34632
Preserve the SDP origin across re-INVITE answers
generateSDPAdvanced is used for re-INVITE responses, but it mints a new o= session-id and resets session-version to 1 on every call. That breaks RFC 3264 offer/answer continuity for in-dialog SDP updates. Keep a stable session-id per dialog and increment the version on subsequent answers.
🤖 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 `@pkg/sip/sdp.go` around lines 418 - 429, generateSDPAdvanced is creating a
fresh SDP Origin on every re-INVITE answer, which breaks offer/answer
continuity. Update the Origin handling so the session-id stays stable for the
same dialog and only SessionVersion increases on subsequent answers, instead of
resetting it to 1. Use the generateSDPAdvanced flow and its
SessionDescription/Origin construction to locate the fix.
Summary
This PR bundles several independent bug fixes discovered during production testing with Cognigy Voice Gateway (jambonz-based SIPREC SRC) but affecting any SIPREC deployment.
Fix 1 – Race condition: "file already closed" on WAV write during BYE teardown
Files:
pkg/media/types.go,pkg/media/rtp.go,pkg/sip/custom_server.gofinalizeCallcalledforwarder.Stop()immediately followed byforwarder.Cleanup().Cleanup()closes the WAV file, but the RTP goroutine's ticker loop (10 ms poll) may still be inprocessPacketwriting the last audio sample, causing:Fix:
doneChan chan struct{}+WaitDone(timeout)toRTPForwarderStartRTPForwardingsodoneChanis closed after WAV Finalize but before Cleanup/Azure-upload:WAV Finalize → close(doneChan) → Cleanup → logStop()+Cleanup()pairs infinalizeCallwith astopAndCleanup()helper that callsWaitDone(100ms)between them, ensuring the goroutine has exited its write path before the file is closedFix 2 – SDP answer: server mirrors SRC's own origin identity
File:
pkg/sip/sdp.goThe SDP answer copied
Username,SessionID, andSessionVersionfrom the incoming offer, causing some SIP stacks to detect a loop (seeing their own session identity in the response).Fix: Use a distinct SRS identity:
o=siprec-srs <unix-microsecond-timestamp> 1 …Fix 3 – SDP answer: wrong direction attribute for
recvonlyofferFile:
pkg/sip/sdp.goWhen the offer contained
a=recvonlythe answer incorrectly echoeda=recvonlyinstead of the correcta=sendonly.Fix:
case "recvonly": directionAttr = sdp.Attribute{Key: "sendonly"}Fix 4 – RS-metadata stream labels don't match SDP
a=labelvaluesFiles:
pkg/siprec/types.go,pkg/sip/custom_server.goThe
<stream label="…">values in the 200 OK rs-metadata body used internal counters (stream_0,stream_1) instead of the actuala=label:Nvalues from the SDP offer. RFC 7866 §5.3 requires these to match.Fix: Store actual SDP stream labels in
RecordingSession.SDPStreamLabelsduring INVITE processing and use them when generating the rs-metadata response.Fix 5 – Non-RFC-compliant SRC: missing
<sessionrecordingassoc>rejected with 400File:
pkg/siprec/parser.goSome SRC implementations (notably jambonz/drachtio) omit the
<sessionrecordingassoc>element. The server returned 400, refusing all recordings from these SRCs.Fix: Downgrade to a warning and continue; the recording session is still usable.
Fix 6 – Symmetric RTP / RTP-latching: pre-populate remote address and send initial probe
Files:
pkg/media/sdp_utils.go,pkg/media/rtp.goSome SRC implementations only start sending RTP after receiving the first UDP datagram from the SRS (RTP latching / symmetric RTP). The server was purely passive, causing a deadlock.
Fix:
ConfigureForwarderForMediaDescriptionextracts the remote RTP/RTCP address from the SDP offer'sc=andm=lines and pre-populatesRemoteRTPAddr/RemoteRTCPAddrStartRTPForwardingsends a minimal RTP probe (PT=13 Comfort Noise) immediately after binding the UDP socketTest plan
go test ./...passesdocker build --target production -t siprec:test .<sessionrecordingassoc>: warning logged, session accepted"file already closed"error in logso=siprec-srs(not mirroring SRC identity)a=label:Nvalues from offer🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
recvonlymedia direction by generating the expected opposite direction in returned SDP.Chores