fix(sip): answer re-INVITE with the RFC 3264 direction, not the cached SDP

pkg/sip/inbound.go

@@ -349,6 +349,98 @@ func updateRemoteFromSDP(media *MediaPort, log logger.Logger, codecs *msdk.Codec
 	media.UpdateRemote(desc.Addr)
 }
 
+// ── RFC 3264 §6.1: ANSWER THE OFFER'S DIRECTION, DON'T ECHO OUR CACHED SDP ──────────
+//
+// Reproduced on v1.8.0 and v1.9.0. When a carrier puts a bridged call on hold it sends a
+// re-INVITE offering `a=sendonly`. Both the inbound and outbound re-INVITE paths answer by
+// replaying the CACHED local SDP verbatim, which carries `a=sendrecv`. RFC 3264 §6.1
+// requires a `sendonly` offer to be answered `recvonly` (and `inactive` -> `inactive`), so
+// the answer is invalid and the carrier tears the dialog down ~60ms after our 200 OK:
+//
+//	06:19:05.464  IN   INVITE  a=sendonly    <- carrier: hold
+//	06:19:05.465  OUT  200 OK  a=sendrecv    <- WRONG, must be a=recvonly
+//	06:19:05.521  IN   ACK
+//	06:19:05.527  IN   BYE                   <- carrier gives up
+//
+// The whole call then collapses: dropping the held leg drops the other party too, so a
+// supervisor pressing HOLD ends the customer's call. No application-layer workaround is
+// possible — the dialog is dead before the agent sees anything.
+//
+// answerDirectionFor maps the offer's direction to the RFC-correct answer, and
+// withSDPDirection rewrites the single a= line in our cached SDP. Deliberately a
+// text-level rewrite rather than a parse/re-serialize: the cached SDP is already a
+// negotiated, working body and reserializing it risks perturbing codecs/ptime/ICE lines
+// that the carrier has accepted. Only the direction attribute changes.
+//
+// If the offer states no direction, sendrecv is implied (RFC 4566 §6) and the cached SDP is
+// returned untouched — preserving today's behaviour for every non-hold re-INVITE (codec
+// renegotiation, port change, session-timer refresh).
+func answerDirectionFor(offer []byte) string {
+	// Scan media-level then session-level attributes; last one wins, matching how the
+	// direction applies to the (single) audio stream we negotiate.
+	dir := ""
+	for _, line := range strings.Split(string(offer), "\n") {
+		switch strings.TrimSpace(line) {
+		case "a=sendonly":
+			dir = "sendonly"
+		case "a=recvonly":
+			dir = "recvonly"
+		case "a=inactive":
+			dir = "inactive"
+		case "a=sendrecv":
+			dir = "sendrecv"
+		}
+	}
+	switch dir {
+	case "sendonly":
+		return "recvonly" // remote holds us: it sends, we only receive
+	case "recvonly":
+		return "sendonly"
+	case "inactive":
+		return "inactive"
+	default:
+		return "" // absent or sendrecv -> no rewrite needed
+	}
+}
+
+// withSDPDirection returns local with its direction attribute replaced by dir. If local has
+// no direction line, dir is appended to the audio media section (falling back to the end of
+// the body when no m= line is present).
+func withSDPDirection(local []byte, dir string) []byte {
+	if dir == "" || len(local) == 0 {
+		return local
+	}
+	// Preserve CRLF vs LF: SDP is CRLF per RFC 4566 and some SBCs are strict.
+	eol := "\n"
+	if strings.Contains(string(local), "\r\n") {
+		eol = "\r\n"
+	}
+	lines := strings.Split(strings.ReplaceAll(string(local), "\r\n", "\n"), "\n")
+	out := make([]string, 0, len(lines)+1)
+	replaced := false
+	for _, line := range lines {
+		switch strings.TrimSpace(line) {
+		case "a=sendrecv", "a=sendonly", "a=recvonly", "a=inactive":
+			if !replaced {
+				out = append(out, "a="+dir)
+				replaced = true
+			}
+			// drop any further duplicate direction lines
+		default:
+			out = append(out, line)
+		}
+	}
+	if !replaced {
+		// No direction line: insert after the audio m= section's attributes, i.e. at the
+		// end of the body. sendrecv was implied before, so stating it explicitly is safe.
+		for len(out) > 0 && strings.TrimSpace(out[len(out)-1]) == "" {
+			out = out[:len(out)-1]
+		}
+		out = append(out, "a="+dir, "")
+	}
+	return []byte(strings.Join(out, eol))
+}
+
 func (s *Server) onInvite(log *slog.Logger, req *sip.Request, tx sip.ServerTransaction) {
 	// Error processed in defer
 	_ = s.processInvite(req, tx)
@@ -417,9 +509,14 @@ func (s *Server) processInvite(req *sip.Request, tx sip.ServerTransaction) (retE
 	existing := s.byLocalTag[cc.ID()]
 	s.cmu.RUnlock()
 	if existing != nil && existing.cc.InviteCSeq() < cc.InviteCSeq() {
-		existing.log().Infow("reinvite", "content-length", req.ContentLength(), "cseq", cc.InviteCSeq())
-		existing.updateRemoteFromSDP(sdpBodyFromRequest(req))
-		cc.AcceptAsKeepAlive(existing.cc.OwnSDP())
+		offer := sdpBodyFromRequest(req)
+		// RFC 3264 §6.1: answer the OFFER's direction. Replaying our cached sendrecv SDP at a
+		// sendonly (hold) offer makes the carrier BYE the dialog — see answerDirectionFor.
+		answerDir := answerDirectionFor(offer)
+		existing.log().Infow("reinvite", "content-length", req.ContentLength(), "cseq", cc.InviteCSeq(),
+			"answerDirection", answerDir)
+		existing.updateRemoteFromSDP(offer)
+		cc.AcceptAsKeepAlive(withSDPDirection(existing.cc.OwnSDP(), answerDir))
 		return nil
 	}
 	if s.cli != nil { // Process reinvite for existing outbound calls
@@ -428,10 +525,15 @@ func (s *Server) processInvite(req *sip.Request, tx sip.ServerTransaction) (retE
 		if oc != nil && oc.cc != nil && oc.cc.InviteCSeq() < newCSeq {
 			localSDP := oc.cc.LocalSDP()
 			if len(localSDP) != 0 {
-				oc.log.Infow("accepting reinvite", "content-length", req.ContentLength(), "cseq", cc.InviteCSeq())
-				oc.updateRemoteFromSDP(sdpBodyFromRequest(req))
+				offer := sdpBodyFromRequest(req)
+				// RFC 3264 §6.1 — see answerDirectionFor. This is the path a supervisor HOLD on a
+				// bridged (transferred) call takes.
+				answerDir := answerDirectionFor(offer)
+				oc.log.Infow("accepting reinvite", "content-length", req.ContentLength(), "cseq", cc.InviteCSeq(),
+					"answerDirection", answerDir)
+				oc.updateRemoteFromSDP(offer)
 				oc.cc.RecordInvite(newCSeq)
-				cc.AcceptAsKeepAlive(localSDP)
+				cc.AcceptAsKeepAlive(withSDPDirection(localSDP, answerDir))
 				return nil
 			}
 		}

pkg/sip/sdp_hold_direction_test.go

@@ -0,0 +1,143 @@
+// Copyright 2026 LiveKit, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+
+package sip
+
+import (
+	"strings"
+	"testing"
+)
+
+// A carrier's HOLD re-INVITE, as captured on a live trunk (Plivo/genband SBC).
+const holdOffer = "v=0\r\n" +
+	"o=genband 1642851412 1894065156 IN IP4 203.0.113.20\r\n" +
+	"s=-\r\n" +
+	"c=IN IP4 203.0.113.20\r\n" +
+	"t=0 0\r\n" +
+	"m=audio 29076 RTP/AVP 0 8 101\r\n" +
+	"c=IN IP4 203.0.113.20\r\n" +
+	"a=rtpmap:0 PCMU/8000\r\n" +
+	"a=sendonly\r\n" +
+	"a=ptime:20\r\n"
+
+// Our cached, already-negotiated local SDP — note a=sendrecv.
+const cachedLocal = "v=0\r\n" +
+	"o=- 9157931411869268290 9157931411869268294 IN IP4 203.0.113.10\r\n" +
+	"s=LiveKit\r\n" +
+	"c=IN IP4 203.0.113.10\r\n" +
+	"t=0 0\r\n" +
+	"m=audio 17830 RTP/AVP 0 101\r\n" +
+	"a=rtpmap:0 PCMU/8000\r\n" +
+	"a=rtpmap:101 telephone-event/8000\r\n" +
+	"a=fmtp:101 0-16\r\n" +
+	"a=ptime:20\r\n" +
+	"a=sendrecv\r\n"
+
+// TestHoldOfferIsAnsweredRecvonly is THE regression test.
+//
+// Before this fix, both re-INVITE paths replayed cachedLocal verbatim, so a sendonly (hold)
+// offer was answered a=sendrecv. RFC 3264 §6.1 requires recvonly, and carriers BYE the dialog
+// ~60ms after the invalid answer — which on a bridged/transferred call ends BOTH legs, so a
+// supervisor pressing HOLD hung up the customer.
+func TestHoldOfferIsAnsweredRecvonly(t *testing.T) {
+	dir := answerDirectionFor([]byte(holdOffer))
+	if dir != "recvonly" {
+		t.Fatalf("a=sendonly offer must be answered recvonly (RFC 3264 §6.1), got %q", dir)
+	}
+
+	answer := string(withSDPDirection([]byte(cachedLocal), dir))
+	if !strings.Contains(answer, "a=recvonly") {
+		t.Fatalf("answer must carry a=recvonly:\n%s", answer)
+	}
+	if strings.Contains(answer, "a=sendrecv") {
+		t.Fatalf("answer must NOT still say sendrecv — this is the bug that makes carriers BYE:\n%s", answer)
+	}
+	// Everything else about the negotiated body must survive untouched.
+	for _, keep := range []string{
+		"m=audio 17830 RTP/AVP 0 101",
+		"a=rtpmap:0 PCMU/8000",
+		"a=rtpmap:101 telephone-event/8000",
+		"a=fmtp:101 0-16",
+		"a=ptime:20",
+		"c=IN IP4 203.0.113.10",
+	} {
+		if !strings.Contains(answer, keep) {
+			t.Errorf("rewrite must not disturb %q:\n%s", keep, answer)
+		}
+	}
+	if !strings.Contains(answer, "\r\n") {
+		t.Error("CRLF line endings must be preserved (RFC 4566; strict SBCs care)")
+	}
+}
+
+func TestDirectionMapping(t *testing.T) {
+	cases := map[string]string{
+		"a=sendonly": "recvonly",
+		"a=recvonly": "sendonly",
+		"a=inactive": "inactive",
+		"a=sendrecv": "", // no rewrite needed
+	}
+	for offer, want := range cases {
+		body := "v=0\r\nm=audio 1 RTP/AVP 0\r\n" + offer + "\r\n"
+		if got := answerDirectionFor([]byte(body)); got != want {
+			t.Errorf("offer %s -> got %q, want %q", offer, got, want)
+		}
+	}
+}
+
+// A re-INVITE with NO direction attribute (codec renegotiation, port change, session-timer
+// refresh) must be answered exactly as before — sendrecv is implied by RFC 4566 §6, and this
+// is the overwhelmingly common case. Regressing it would break every non-hold re-INVITE.
+func TestNoDirectionInOfferLeavesSDPUntouched(t *testing.T) {
+	offer := "v=0\r\nm=audio 29076 RTP/AVP 0\r\na=rtpmap:0 PCMU/8000\r\n"
+	if dir := answerDirectionFor([]byte(offer)); dir != "" {
+		t.Fatalf("absent direction must yield no rewrite, got %q", dir)
+	}
+	if got := string(withSDPDirection([]byte(cachedLocal), "")); got != cachedLocal {
+		t.Fatal("empty direction must return the cached SDP byte-identical")
+	}
+}
+
+// Unhold: the carrier re-offers sendrecv, and we must go back to sending audio.
+func TestUnholdRestoresSendrecv(t *testing.T) {
+	held := withSDPDirection([]byte(cachedLocal), "recvonly")
+	if !strings.Contains(string(held), "a=recvonly") {
+		t.Fatal("setup: expected held SDP to be recvonly")
+	}
+	// carrier unholds -> offer has a=sendrecv -> answerDirectionFor returns "" (no rewrite),
+	// so the caller passes the cached (sendrecv) SDP through unchanged.
+	if dir := answerDirectionFor([]byte(strings.Replace(holdOffer, "a=sendonly", "a=sendrecv", 1))); dir != "" {
+		t.Fatalf("sendrecv offer needs no rewrite, got %q", dir)
+	}
+}
+
+func TestDirectionLineAppendedWhenAbsentFromLocal(t *testing.T) {
+	local := "v=0\r\nm=audio 17830 RTP/AVP 0\r\na=rtpmap:0 PCMU/8000\r\n"
+	out := string(withSDPDirection([]byte(local), "recvonly"))
+	if !strings.Contains(out, "a=recvonly") {
+		t.Fatalf("direction must be appended when local has none:\n%s", out)
+	}
+	if strings.Count(out, "a=recvonly") != 1 {
+		t.Fatalf("exactly one direction line expected:\n%s", out)
+	}
+}
+
+func TestDuplicateDirectionLinesCollapseToOne(t *testing.T) {
+	local := "v=0\r\na=sendrecv\r\nm=audio 1 RTP/AVP 0\r\na=sendrecv\r\n"
+	out := string(withSDPDirection([]byte(local), "recvonly"))
+	if strings.Count(out, "a=recvonly") != 1 {
+		t.Fatalf("duplicate direction lines must collapse to one:\n%s", out)
+	}
+	if strings.Contains(out, "a=sendrecv") {
+		t.Fatalf("no stale sendrecv may remain:\n%s", out)
+	}
+}