feat: implement RFC 4028 session timer compliance - #51
Open
thorsager wants to merge 8 commits into
Open
Conversation
Implement SIP session timers per RFC 4028 for both internal B2BUA calls and trunk calls, with configurable Session-Expires and Min-SE values exposed as CLI flags (-session-timer, -min-se). Session timer negotiation: - Add Supported: timer, Min-SE, and Session-Expires headers to outbound INVITEs on both internal and trunk legs - Negotiate Session-Expires and refresher (uac/uas) in 200 OK responses to Alice when the inbound INVITE supports timer - Reject inbound INVITEs with Session-Expires below Min-SE using 422 Session Interval Too Small (new status code constant) - Handle 422 responses from Bob/trunk peers by raising Min-SE and retrying the INVITE with an updated Session-Expires (trunk path retries in place; internal path forwards the 422 to Alice) Timer lifecycle (new internal/b2bua/sessiontimer.go): - SessionTimer struct tracks interval, Min-SE, refresher role, and expiry per call leg (Alice and Bob independently) - Refresher side sends a re-INVITE at half the interval; on refresh failure or rejection (408/481) the call is torn down - Non-refresher side tears down the call if no refresh arrives before a safety margin of min(32s, interval/3) prior to expiry - Helpers for parsing and formatting Session-Expires/Min-SE headers and detecting timer support in the Supported header In-dialog re-INVITE handling: - Detect re-INVITEs by To tag and Call-ID in HandleInvite and route them to a new dialog handler - Forward re-INVITEs (with SDP if present) to the opposite leg and relay provisional, 200, and error responses back - Reset the session timer for both legs on re-INVITE and its 200 OK Teardown: - Replace the ad-hoc trunkSessionTimer goroutine with unified sendBye/sendByeBothLegs helpers that send BYE with a Reason header, stop timers and media, remove the call from the store, and release trunk channels Add unit tests for Session-Expires/Min-SE parsing, timer support detection, and header formatting helpers.
There was a problem hiding this comment.
Pull request overview
This PR adds RFC 4028 session timer support to the B2BUA by introducing per-leg session timer state, negotiating Session-Expires/Min-SE with peers (including 422 handling), and adding in-dialog re-INVITE forwarding and timer-driven refresh/teardown behavior.
Changes:
- Added
SessionTimerinfrastructure (parsing/formatting helpers + refresher/non-refresher timer loops + refresh re-INVITE sending). - Added in-dialog re-INVITE detection/forwarding and unified BYE teardown helpers.
- Added configuration flags for default session timer interval and minimum acceptable session interval.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/sip_constants.go | Adds RFC 4028 status code constant (422). |
| internal/b2bua/sessiontimer.go | New session timer implementation (parse/format helpers, timer loops, refresh sending). |
| internal/b2bua/sessiontimer_test.go | Unit tests for session timer parsing/formatting helpers. |
| internal/b2bua/handler.go | Integrates session timers into INVITE flows, adds re-INVITE forwarding, 422 handling, and unified BYE teardown. |
| internal/b2bua/call.go | Stores per-leg session timer state on the Call object. |
| cmd/trecsd/main.go | Adds CLI flags for session timer defaults (--session-timer, --min-se). |
Suppressed comments (1)
internal/b2bua/handler.go:1678
- In b2buaResponseLoop, the 422 handler logs that it is "retrying" but then explicitly forwards the 422 to Alice because there is no retry mechanism. This contradicts the intended RFC 4028 behavior and the surrounding log message, and it also mutates
h.minSEglobally based on one peer response.
// Handle 422 Session Interval Too Small (RFC 4028 §5).
if sc == proto.SIPStatusSessionIntervalTooSmall {
h.cancelPRACK(cc.callID)
peerMinSE := ParseMinSE(resp.Headers.GetFirst("Min-SE"))
if peerMinSE > h.minSE {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The B2BUA session timer logic had several bugs around RFC 4028 negotiation. Min-SE was mutated on the handler-wide default instead of per Call-ID, the refresher role for non-timer peers was miscalculated, and 422 retries / re-INVITEs / session refreshes used unregistered Via branches, so responses could not be routed back to their UAC transactions. This change scopes Min-SE negotiation to each call, negotiates the Bob leg timer from the peer 200 OK correctly, propagates session-timer headers across legs, and creates UAC transactions before building requests so their branches are registered in the UACManager. CANCEL now targets the live retry transaction, and BYE uses the dialog's incremented local CSeq. Also fixes flag validation (Min-SE >= 90s per RFC 4028 §5), a UAS refresher check that would never fire, and adds tests covering re-INVITE branch matching, session header forwarding, and session timer negotiation.
Add a deadline to waitForRefreshResponse so the server tears down the call if the peer does not accept the re-INVITE within the session interval. The trunk test ghost peer now stops answering re-INVITEs after the initial INVITE so the teardown path is exercised. Update the wait time accordingly.
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in session-timer parsing/negotiation and timer lifecycle control (including an inability to disable timers via the documented flag) that can break RFC 4028 behavior in production.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 6
- Review effort level: Lite
- honor --session-timer=0 as a full opt-out of timer negotiation (422 checks, offers, and 200 OK handling are skipped when disabled) - ParseSessionExpires reports absent/unparseable headers as 0 instead of silently forcing DefaultSessionExpires, so configured defaults and UAS refresher are preserved across both legs - parse refresher param case-insensitively and accept generic Min-SE parameters (e.g. "90;foo=bar") - keep session timers attached to their parent context on reset, stop both legs' timers on BYE, guard sendByeBothLegs against already removed calls, and cancel refresh transactions on context teardown
There was a problem hiding this comment.
🟡 Changes recommended
The re-INVITE relay path currently alters/strips RFC 4028 negotiation headers (and advertises Supported: timer unconditionally) and the PR description overstates 422 retry coverage, which can break session-timer compliance in real dialog refresh flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/b2bua/handler.go:1772
- PR description says 422 responses trigger an automatic retry with updated Min-SE / Session-Expires for outbound INVITEs, but the non-trunk B2BUA path explicitly forwards 422 to Alice and returns (no retry). Either implement the retry here (similar to trunkResponseLoop) or update the PR description to scope the retry behavior to trunks only.
internal/b2bua/handler.go:905
- reInviteResponseLoop forwards error responses using proto.NewResponse(origReq, ...) but doesn’t copy required headers from the upstream response. In particular, a 422 response must carry Min-SE (RFC 4028 §5); dropping it prevents the originating leg from retrying with an acceptable interval.
if sc >= 300 {
log.Info("B2BUA: re-INVITE error response, forwarding",
"statusCode", sc, "fromAlice", isFromAlice)
errReason := resp.Status()
if idx := strings.Index(errReason, " "); idx != -1 {
errReason = errReason[idx+1:]
}
origTx.Respond(proto.NewResponse(origReq, sc, errReason))
return
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
- advertise Supported: timer on forwarded re-INVITEs only when session timers are actually in play for the dialog (request engages them or a leg-level timer was negotiated) - relay Session-Expires/Min-SE and timer Require/Supported option tags from the peer response back to the re-INVITE originator, so a 422 keeps its mandatory Min-SE (RFC 4028 §5) - replace time.After with a stoppable timer in waitForRefreshResponse to avoid arming a full-interval timer on every early return
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one confirmed resource-leak bug in the 422 handling path (and an in-dialog refresh concurrency risk) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/b2bua/handler.go:1787
- On a 422 (Session Interval Too Small) from Bob, this path forwards the 422 to Alice and returns without closing the allocated RTP conns. Since there is explicitly no retry implemented here, the call setup is failing and these RTP ports will leak until process exit.
Either implement the RFC 4028 retry here (like trunkResponseLoop does) or perform the same cleanup as other failure paths (close both RTP conns) before responding.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
The refresher loop launched waitForRefreshResponse in a goroutine and immediately re-armed its timer, so a silent peer caused a second in-dialog re-INVITE at the next half-interval while the first was still outstanding (RFC 3261 §12.2.1 glare, 491 risk). The loop now awaits each refresh's outcome inline: accepted (timer reset, new generation owns the schedule) and terminated (teardown or context cancel) stop the loop, while non-fatal rejections such as 491 retry at the next half-interval point. sendSessionRefresh returns the UAC transaction, and tests cover the no-overlap guarantee, resumption after acceptance, and stop on context cancel.
This was referenced Sep 8, 2026
The overlap test used a 1s session interval, leaving only ~400ms between feeding the refresh's 200 OK and the first transaction's in-call interval deadline; a stall on loaded CI could tear the call down mid-test. Double the interval, express the in-flight and post-cancel checks as interval-relative sleeps, and extend the poll deadline, keeping every window seconds of slack. Behavior asserted (one branch per refresh, resume after acceptance, stop on cancel) is unchanged.
There was a problem hiding this comment.
🔵 Needs a closer look
The current 422 retry logic can misbehave when Min-SE is missing/invalid, and session-timer reset semantics for forwarded re-INVITEs can extend sessions even when refresh ultimately fails.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
internal/b2bua/handler.go:828
- handleReInvite resets the originating leg’s session timer immediately after sending the forwarded re-INVITE. If the forwarded re-INVITE is ultimately rejected (>=300) and that error is relayed back, the session wasn’t actually refreshed, but the timer was still extended on the originating leg.
internal/b2bua/handler.go:1341 - The 422 (Session Interval Too Small) retry path assumes the peer always includes a valid Min-SE. If the peer omits Min-SE or sends an unparsable value, peerMinSE becomes 0 and the code will still retry (potentially repeatedly) without ever raising the interval, which can loop until the response context is canceled.
internal/b2bua/handler.go:1605 - negotiateAliceSessionTimer only engages timers when the request has Supported: timer. Elsewhere (HandleInvite) Min-SE enforcement is triggered purely by the presence of Session-Expires, so an inbound INVITE with Session-Expires but without Supported: timer can be rejected for being too small, yet (if acceptable) will later be treated as having no timer support and get no Session-Expires/Require in the 200 OK. Consider treating Session-Expires / Min-SE / Require: timer as engaging timers too.
internal/b2bua/handler.go:1777 - This 422 handling block explicitly notes that a retry is "not yet implemented" and just forwards the 422 to Alice. That contradicts the PR description’s claim that 422 responses trigger an automatic retry with updated Min-SE/Session-Expires (at least for the internal/B2BUA (non-trunk) call path handled by this response loop).
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Follow-up to the Copilot review of the session-timer PR: - Reset both legs' session timers only when a forwarded re-INVITE is confirmed with a 200 OK (RFC 4028 §7.2 refresh semantics). A re-INVITE that is ultimately rejected no longer extends the session on the originating leg. - Guard the trunk 422 renegotiation loop: a 422 whose Min-SE is missing, unparsable, or already satisfied by the current offer is not retryable and now fails the call with 488 instead of resending the identical INVITE; consecutive renegotiations are capped (max422Retries) so a peer that keeps raising Min-SE cannot spin. - Release the RTP connections on the non-trunk 422 relay path (no retry mechanism there, so the setup is failing; previously the ports leaked until process exit). - Centralize timer engagement in requestEngagesTimers (Supported: timer, Require: timer, Session-Expires, or Min-SE) and use it in both the inbound 422 gate's counterpart negotiation and re-INVITE forwarding, so a peer offering Session-Expires without Supported: timer is treated consistently (previously it could be 422-enforced on one path and silently ignored on another). Tests: re-INVITE timer reset fires only on confirmed 200; trunk 422 rig refactor with unretryable-Min-SE and retry-cap cases; RTP conns asserted closed on the non-trunk 422 path; engagement cases in negotiateAliceSessionTimer.
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements full RFC 4028 session timer compliance, replacing the previous fire-and-forget timer with a proper session timer system that negotiates parameters with peers, sends re-INVITEs to refresh sessions, and handles 422 responses.
Changes
Session Timer Infrastructure (
internal/b2bua/sessiontimer.go)SessionTimerstruct tracking interval, refresher role, and expiry per dialog legParseSessionExpires,ParseMinSE,HasTimerSupportStartSessionTimer/StopSessionTimer/ResetSessionTimerfor timer lifecycleIn-Dialog re-INVITE Handling (
internal/b2bua/handler.go)HandleInvitedetects re-INVITEs by checking To tag + Call-ID in storehandleReInviteforwards re-INVITEs across legs with response relay viareInviteResponseLoopSession Timer Header Negotiation
Supported: timer,Min-SE, andSession-ExpiresSession-Expires,Supported: timer, andRequire: timerSession-ExpiresbelowMin-SEare rejected with 422Min-SEand higherSession-Expires, on a fresh transaction; renegotiation is bounded (requires progress from the peer'sMin-SE, capped retries) so a misbehaving peer cannot cause a retry loopMin-SE; retry there is not yet implemented and remains open on Implement Full RFC 4028 Session Timer Compliance #39Unified BYE Handling
sendByeandsendByeBothLegsreplace ad-hoc BYE logic with shared helpersConfiguration
--session-timerflag (default 1800s) — global default Session-Expires;0fully disables session-timer negotiation (no offers, no 422 handling, noRequire: timer)--min-seflag (default 90s) — minimum acceptable Session-ExpiresSessionExpiresSeccontinues to work as overrideReferences
Testing