diff --git a/pkg/sip/inbound.go b/pkg/sip/inbound.go index 8b7f4f77..7244cecb 100644 --- a/pkg/sip/inbound.go +++ b/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 } } diff --git a/pkg/sip/sdp_hold_direction_test.go b/pkg/sip/sdp_hold_direction_test.go new file mode 100644 index 00000000..16a01b02 --- /dev/null +++ b/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) + } +}