Skip to content

fix: SDP correctness, RTP goroutine race condition, and non-RFC SRC compatibility#35

Merged
loreste merged 1 commit into
loreste:mainfrom
Ma91Wa:fix/cognigy-vgw-compat-and-rtp-race
Jun 30, 2026
Merged

fix: SDP correctness, RTP goroutine race condition, and non-RFC SRC compatibility#35
loreste merged 1 commit into
loreste:mainfrom
Ma91Wa:fix/cognigy-vgw-compat-and-rtp-race

Conversation

@Ma91Wa

@Ma91Wa Ma91Wa commented Jun 30, 2026

Copy link
Copy Markdown

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.go

finalizeCall called forwarder.Stop() immediately followed by forwarder.Cleanup(). Cleanup() closes the WAV file, but the RTP goroutine's ticker loop (10 ms poll) may still be in processPacket writing the last audio sample, causing:

{"level":"error","message":"Failed to write PCM audio to recording","error":"write ….wav: file already closed"}

Fix:

  • Add doneChan chan struct{} + WaitDone(timeout) to RTPForwarder
  • Restructure defers in StartRTPForwarding so doneChan is closed after WAV Finalize but before Cleanup/Azure-upload:
    WAV Finalize → close(doneChan) → Cleanup → log
  • Replace bare Stop()+Cleanup() pairs in finalizeCall with a stopAndCleanup() helper that calls WaitDone(100ms) between them, ensuring the goroutine has exited its write path before the file is closed

Fix 2 – SDP answer: server mirrors SRC's own origin identity

File: pkg/sip/sdp.go

The SDP answer copied Username, SessionID, and SessionVersion from 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 recvonly offer

File: pkg/sip/sdp.go

When the offer contained a=recvonly the answer incorrectly echoed a=recvonly instead of the correct a=sendonly.

Fix: case "recvonly": directionAttr = sdp.Attribute{Key: "sendonly"}

Fix 4 – RS-metadata stream labels don't match SDP a=label values

Files: pkg/siprec/types.go, pkg/sip/custom_server.go

The <stream label="…"> values in the 200 OK rs-metadata body used internal counters (stream_0, stream_1) instead of the actual a=label:N values from the SDP offer. RFC 7866 §5.3 requires these to match.

Fix: Store actual SDP stream labels in RecordingSession.SDPStreamLabels during INVITE processing and use them when generating the rs-metadata response.

Fix 5 – Non-RFC-compliant SRC: missing <sessionrecordingassoc> rejected with 400

File: pkg/siprec/parser.go

Some 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.go

Some 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:

  • ConfigureForwarderForMediaDescription extracts the remote RTP/RTCP address from the SDP offer's c= and m= lines and pre-populates RemoteRTPAddr/RemoteRTCPAddr
  • StartRTPForwarding sends a minimal RTP probe (PT=13 Comfort Noise) immediately after binding the UDP socket

Test plan

  • go test ./... passes
  • Build succeeds: docker build --target production -t siprec:test .
  • SIPREC INVITE with RFC-compliant SRC: full call recorded, no errors in logs
  • SIPREC INVITE without <sessionrecordingassoc>: warning logged, session accepted
  • BYE received after call: no "file already closed" error in logs
  • SDP answer origin: o=siprec-srs (not mirroring SRC identity)
  • SDP stream labels in rs-metadata match a=label:N values from offer

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Improved SIPREC call handling with more consistent stream labeling in recording responses.
    • Added support for a plain SDP-only response mode for SIPREC INVITE requests when enabled.
  • Bug Fixes

    • Reduced RTP forwarding shutdown race conditions by waiting briefly for forwarding to fully stop before cleanup.
    • Better handles recvonly media direction by generating the expected opposite direction in returned SDP.
    • Uses more stable session origin values in generated SDP responses.
  • Chores

    • Added extra logging to help troubleshoot SDP and SIPREC message flows.

…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>
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds doneChan/WaitDone to RTPForwarder for goroutine exit signaling, inserts a symmetric-RTP probe (sendRTPProbe) triggered by remote address hints extracted from SDP, fixes SIPREC SDP answer direction (recvonlysendonly) and origin generation, propagates SDP a=label values into rs-metadata stream responses, downgrades missing SessionRecordingAssoc to a warning, and tightens forwarder shutdown ordering with WaitDone(100ms) calls throughout the SIP server.

Changes

SIPREC RTP Forwarder Lifecycle, Probe, SDP, and Response Fixes

Layer / File(s) Summary
RTPForwarder doneChan and WaitDone API
pkg/media/types.go
Adds doneChan chan struct{} and doneOnce sync.Once to RTPForwarder, initializes them in NewRTPForwarder, and exposes WaitDone(timeout time.Duration) bool.
doneChan close defer in StartRTPForwarding
pkg/media/rtp.go
Restructures defer ordering with LIFO comments and inserts doneOnce.Do(close(doneChan)) after WAV finalization so WaitDone unblocks at the right point.
Symmetric-RTP probe: remote address hints and sendRTPProbe
pkg/media/sdp_utils.go, pkg/media/rtp.go
setRemoteAddressHints extracts RTP/RTCP addresses from SDP and pre-populates RemoteRTPAddr/RemoteRTCPAddr; StartRTPForwarding calls new sendRTPProbe to transmit a minimal CN RTP packet for symmetric-RTP latching.
SDP answer direction and origin fixes
pkg/sip/sdp.go
Reverses recvonlysendonly in generated SDP media direction; replaces copied origin fields with a fixed username, freshly generated session ID, and constant version.
SDP stream labels in RecordingSession and rs-metadata
pkg/siprec/types.go, pkg/sip/custom_server.go
Adds SDPStreamLabels []string to RecordingSession, populates it from forwarder audio stream labels during INVITE, and uses it with MediaStreamTypes fallback when constructing rs-metadata response stream elements.
SIPREC validation: missing SessionRecordingAssoc to warning
pkg/siprec/parser.go
Downgrades zero-value SessionRecordingAssoc from validation error to warning, tolerating non-RFC-compliant sources.
PLAIN_SDP_RESPONSE env, logging, and shutdown hardening
pkg/sip/custom_server.go
Adds PLAIN_SDP_RESPONSE env override for plain SDP responses, SDP offer/answer and multipart body debug logs, raw metadata XML log on validation failure, and WaitDone(100ms) before Cleanup() in re-INVITE teardown, call finalization (stopAndCleanup helper), and server shutdown.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 A probe hops first to wake the latch,
Then doneChan shuts without a catch.
Stream labels flow from offer to reply,
recvonly flips — sendonly, oh my!
WaitDone waits before Cleanup runs free,
A tidier SIPREC, happy as me! 🎵

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main fixes: SDP handling, RTP shutdown race mitigation, and compatibility with non-RFC SRC behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Apply PLAIN_SDP_RESPONSE to re-INVITEs too.

The initial INVITE path can return application/sdp, but handleSiprecReInvite still sends multipart whenever RecordingSession exists. 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 win

Tighten the WaitDone contract wording.

doneChan is closed after RTP writing/WAV finalization, but before the goroutine-owned Cleanup()/exit defers in StartRTPForwarding. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 906d84a and f4054cc.

📒 Files selected for processing (7)
  • pkg/media/rtp.go
  • pkg/media/sdp_utils.go
  • pkg/media/types.go
  • pkg/sip/custom_server.go
  • pkg/sip/sdp.go
  • pkg/siprec/parser.go
  • pkg/siprec/types.go

Comment thread pkg/media/sdp_utils.go
Comment on lines +41 to +58
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()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Comment thread pkg/sip/custom_server.go
s.sendResponse(message, 488, "Not Acceptable Here", nil, nil)
return
}
logger.WithField("sdp_offer", string(sdpData)).Info("Received SDP offer from SRC")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.

Comment thread pkg/sip/custom_server.go
Comment on lines +1846 to +1854
// 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)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Comment thread pkg/sip/sdp.go
Comment on lines +418 to +429
// 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"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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/sip

Repository: 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"
fi

Repository: 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.go

Repository: 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.go

Repository: 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.

@loreste loreste merged commit 98f88f1 into loreste:main Jun 30, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants