feat: implement RFC 4028 session timer compliance - #51
Open
thorsager wants to merge 5 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
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-ExpiresUnified 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