From 204203e3b0f988c9188e98694e4d0a78af4fe39f Mon Sep 17 00:00:00 2001 From: Robert Helewka Date: Wed, 29 Jul 2026 06:04:01 -0400 Subject: [PATCH] fix(sip): correct five Sippy API mismatches that broke every real call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SippyEngine had never successfully placed a call or registered a trunk. Every failure was masked by broad exception handlers that logged and marked the leg terminated, so the gateway reported "ringing" and then ended the call rather than surfacing the fault. None of it was visible to the test suite, which runs exclusively on MockSIPEngine. Found by pointing the gateway at a local Asterisk instance (tests/lab) — each fix uncovered the next. 1. Trunk registration used kwargs the installed sippy (2.3.0) does not accept (auth_name/auth_password → user/passw), called register() instead of doregister(), and passed aor/contact as strings where SipRegistrationAgent calls .getCopy() and mutates .username/.port, so SipURL objects are required. It also posted registered=True at *send* time. Registration is asynchronous, so a rejected REGISTER would still have reported success — and /health treats a registered trunk as a condition for "healthy". Now wired to sippy's rok_cb/rfail_cb, so the rejection status line (typically a bad trunk password) reaches the operator. The Contact also fell back to loopback when the SIP bind is 0.0.0.0; a wildcard address is not somewhere a trunk can send an INVITE. 2. The INVITE passed SDP as a `body` kwarg. CCEventTry takes no such argument: UacStateIdle unpacks exactly six fields from the data tuple and expects the SDP as a MsgBody in position four. callingID/calledID are bare usernames — sippy builds the URIs itself from nh_address. 3. _sip_logger was absent from the global config. SipTransactionManager dereferences it on every message, so the first SIP packet in either direction raised KeyError inside the ED thread. 4. SippyCallController was not callable. Sippy invokes event_cb(event, ua) with CCEvent objects; the class only exposed on_* methods that nothing called. Added __call__ to dispatch CCEventRing/Connect/Disconnect/Fail to the existing handlers, guarding the body because an exception escaping into the ED dispatcher would hang the leg silently. 5. The UA was constructed without credentials, so sippy could not answer the 401/407 challenge that any authenticating trunk sends. Every outbound call died on the challenge. Verified end to end against Asterisk 22.10.1: 180 Ringing → Connected → 23s of audio → clean teardown, with the dialplan executing and audio playing in real time. Not fixed here, and still blocking media: MediaPipeline.create_tap is a stub that logs success and returns a tap nothing ever feeds, so the classifier receives no audio on a live call. Co-Authored-By: Claude Opus 5 (1M context) --- core/sippy_engine.py | 129 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 113 insertions(+), 16 deletions(-) diff --git a/core/sippy_engine.py b/core/sippy_engine.py index aeaf6b0..fdf4945 100644 --- a/core/sippy_engine.py +++ b/core/sippy_engine.py @@ -84,6 +84,48 @@ class SippyCallController: self.leg_id = leg_id self.engine = engine + def __call__(self, event, ua) -> None: + """Sippy's ``event_cb`` — invoked as ``event_cb(event, ua)``. + + Sippy delivers call progress as CCEvent objects through this one + entry point; it never calls the ``on_*`` methods directly. This + dispatches to them so each SIP fact still has a named handler. + """ + from sippy.CCEvents import ( + CCEventConnect, + CCEventDisconnect, + CCEventFail, + CCEventPreConnect, + CCEventRing, + ) + + try: + if isinstance(event, CCEventRing): + self.on_ringing() + elif isinstance(event, (CCEventConnect, CCEventPreConnect)): + # data is (code, reason, body) — the body carries the + # negotiated SDP that tells the media pipeline where to + # send RTP. + data = event.getData() + body = data[2] if isinstance(data, tuple) and len(data) > 2 else None + self.on_connected(str(body) if body is not None else None) + elif isinstance(event, CCEventDisconnect): + self.on_disconnected("remote hangup") + elif isinstance(event, CCEventFail): + data = event.getData() + reason = " ".join(str(d) for d in data[:2]) if data else "call failed" + self.on_disconnected(reason) + # DTMF is not handled here: SIP INFO arrives as a request and is + # picked up by _handle_incoming_info, and RFC 2833 DTMF rides in + # the RTP stream, which is the media pipeline's business. + except Exception as e: + # This runs on the Sippy ED thread: an escaping exception is + # swallowed by the dispatcher and the leg would hang silently. + logger.error( + f" {self.leg_id}: error handling {type(event).__name__}: {e}", + exc_info=True, + ) + def on_trying(self): """100 Trying received.""" logger.debug(f" {self.leg_id}: 100 Trying") @@ -319,10 +361,17 @@ class SippyEngine(SIPEngine): SipConf.my_port = self._sip_port SipConf.my_uaname = "Hold Slayer Gateway" + # SipTransactionManager dereferences _sip_logger unconditionally on + # every message, so it must exist before any SIP traffic. It + # defaults to the stderr backend (SIPLOG_BEND), not the + # /var/log/sip.log path in its signature — nothing to create. + from sippy.SipLogger import SipLogger + self._sippy_global_config = { "_sip_address": self._sip_address, "_sip_port": self._sip_port, "_sip_tm": None, # Transaction manager set after start + "_sip_logger": SipLogger("hold-slayer"), } # Start Sippy's SIP transaction manager in a background thread @@ -499,23 +548,53 @@ class SippyEngine(SIPEngine): def do_register(): try: from sippy.SipRegistrationAgent import SipRegistrationAgent + from sippy.SipURL import SipURL + def on_registered(_rtime, _contact, _cb_arg): + logger.info(" ✅ Trunk registration accepted") + self._post_from_ed("trunk_registered", {"registered": True}) + + def on_register_failed(status_line, _cb_arg): + # status_line is the response's status line (e.g. "403 + # Forbidden") — surface it; a bad trunk password is the + # most common cause and is otherwise invisible. + logger.error(f" ❌ Trunk registration rejected: {status_line}") + self._post_from_ed( + "trunk_registered", + {"registered": False, "reason": str(status_line)}, + ) + + # A wildcard bind is not a routable Contact — the trunk would + # have nowhere to send the inbound INVITE. Fall back to + # loopback, matching _generate_sdp's handling. + contact_host = ( + self._sip_address if self._sip_address != "0.0.0.0" else "127.0.0.1" + ) + + # aor/contact must be SipURL objects: the agent calls + # .getCopy() and mutates .username/.port on them. reg_agent = SipRegistrationAgent( self._sippy_global_config, - f"sip:{self._trunk_username}@{self._trunk_host}", - f"sip:{self._trunk_host}:{self._trunk_port}", - auth_name=self._trunk_username, - auth_password=self._trunk_password, + SipURL(f"sip:{self._trunk_username}@{self._trunk_host}"), + SipURL(f"sip:{self._trunk_username}@{contact_host}:{self._sip_port}"), + user=self._trunk_username, + passw=self._trunk_password, + rok_cb=on_registered, + rfail_cb=on_register_failed, ) - reg_agent.register() - logger.info(" ✅ Trunk registration sent") - self._post_from_ed("trunk_registered", {"registered": True}) + # Registration is asynchronous: success is reported by the + # callbacks above, not here. Reporting "registered" at send + # time would let /health go green on a rejected REGISTER. + reg_agent.doregister() + logger.info(" Trunk REGISTER sent, awaiting response") except ImportError: logger.warning(" Sippy registration agent not available") self._post_from_ed("trunk_registered", {"registered": False}) except Exception as e: - logger.error(f" ❌ Trunk registration failed: {e}") - self._post_from_ed("trunk_registered", {"registered": False}) + logger.error(f" ❌ Trunk registration failed: {e}", exc_info=True) + self._post_from_ed( + "trunk_registered", {"registered": False, "reason": str(e)} + ) self._run_on_sippy(do_register) @@ -570,7 +649,8 @@ class SippyEngine(SIPEngine): else: remote_uri = f"sip:{number}@{self._domain}" - from_uri = f"sip:{caller_id or self._did}@{self._domain}" + caller_number = caller_id or self._did + from_uri = f"sip:{caller_number}@{self._domain}" leg = SipCallLeg(leg_id, "outbound", remote_uri) self._legs[leg_id] = leg @@ -583,24 +663,38 @@ class SippyEngine(SIPEngine): def do_invite(): try: from sippy.CCEvents import CCEventTry + from sippy.MsgBody import MsgBody from sippy.SipCallId import SipCallId from sippy.UA import UA controller = SippyCallController(leg_id, self) - # Create Sippy UA for this call + # Create Sippy UA for this call. The credentials are required: + # a trunk answers the first INVITE with 401/407, and sippy + # only retries with a digest response when they are set — + # without them every outbound call dies on the challenge. ua = UA( self._sippy_global_config, event_cb=controller, + username=self._trunk_username or None, + password=self._trunk_password or None, nh_address=(self._trunk_host, self._trunk_port), ) self._ed_leg_to_ua[leg_id] = ua self._ed_ua_to_leg[ua] = leg_id - # Send INVITE + # SDP travels inside the event's data tuple as a MsgBody, not + # as a kwarg. needs_update=False marks it final: with it set, + # sippy would call ua.on_local_sdp_change (unset here) before + # sending, and the INVITE would never go out. + body = MsgBody(sdp_body, mtype="application/sdp") + body.needs_update = False + + # UacStateIdle unpacks exactly six fields and builds the SIP + # URIs itself from nh_address — callingID/calledID are bare + # usernames, not full URIs. event = CCEventTry( - (SipCallId(), from_uri, remote_uri), - body=sdp_body, + (SipCallId(), caller_number, number, body, None, None) ) ua.recvEvent(event) @@ -613,8 +707,11 @@ class SippyEngine(SIPEngine): self._post_from_ed("leg_state", {"leg_id": leg_id, "state": "ringing"}) except Exception as e: - logger.error(f" Failed to send INVITE for {leg_id}: {e}") - self._post_from_ed("leg_state", {"leg_id": leg_id, "state": "terminated"}) + logger.error(f" Failed to send INVITE for {leg_id}: {e}", exc_info=True) + self._post_from_ed( + "leg_state", + {"leg_id": leg_id, "state": "terminated", "error": str(e)}, + ) self._run_on_sippy(do_invite) return leg_id