Repository navigation
fix: end the session when a UAS 2xx to an INVITE is never ACKed - #11
Merged
Merged
Conversation
A server INVITE transaction in Completed armed Timer K (T4, 5 s by default), which terminated it long before 64*T1: the 2xx was retransmitted for about 5 s instead of 32 s, and when no ACK came the dialog stayed in WaitAck (or Confirmed, for a re-INVITE) forever, with no event and no BYE. RFC 3261 §13.3.1.4: the UAS retransmits the 2xx starting at T1 and doubling up to T2 until the ACK arrives; if none arrives within 64*T1, the session SHOULD be ended with a BYE. §17.2.1 likewise keeps a non-2xx in Completed for Timer H (64*T1); T4 (Timer I) applies only once the ACK has arrived. - Do not arm Timer K when a server INVITE enters Completed; Timer D (64*T1) already ends it, and Confirmed still uses T4. - Cap Timer G at T2 instead of 64*T1, with a new EndpointOption::t2 (default 4 s, the RFC value). - When the INVITE or re-INVITE transaction ends without an ACK after a 2xx, terminate the dialog with TerminatedReason::Timeout and send a BYE. Applied to InviteDialog and the deprecated ServerInviteDialog.
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.
Assessment
Verdict: genuine library defect, confidence HIGH
Spec: RFC 3261 §13.3.1.4. The UAS retransmits its 2xx starting at T1 and doubling up to T2 until the ACK arrives. If no ACK arrives within 64·T1, the session SHOULD be ended with a BYE. §14.2 applies the same rule to re-INVITEs. §17.2.1 says a non-2xx waits for its ACK for Timer H (64·T1); T4 (Timer I) applies only after the ACK arrives.
Codex tiebreak (library defect vs. application usage): LIBRARY-DEFECT, high. The documented UAS flow (
incoming_transactions→get_or_create_server_invite→handle, thenaccept()) stops retransmitting at T4 and leaves the dialog inWaitAckforever.Codex design calls:
Terminatedthe publicbye()does nothing.TerminatedReason::Timeout, which was defined but never used.EndpointOption::t2. Earlier fields were added the same way, and every in-repo literal already uses..Default::default().Red on
main(5958064):cargo test --lib test_uas_ack_timeoutgives 2 failures:the 2xx must be retransmitted until 64*T1, got 4 transmissionsthe re-INVITE 2xx must be retransmitted until 64*T1, got 4 transmissionsThe ACKed control passes. The T2-cap test needs the new
t2field, so it can't compile on main.Green on this branch: 4 passed, 5 repeat runs with no flakes.
cargo testpasses 334 lib + 65 doc tests.cargo fmt --all -- --checkis clean and the clippy warning set is identical to main.Upstream: upstream-ready, queued. It overlaps open feat(transaction): RFC 6026 Accepted state + Timer L/M for INVITE 200 OK retransmission absorption restsend/rsipstack#128 (RFC 6026
Accepted+ Timer L), which would supersede the 2xx half of the transaction change. Rebase whichever lands second.Problem
When a server INVITE transaction enters
Completedit arms Timer K = T4 (5 s by default). Timer K terminates the transaction well before 64·T1 (32 s), which causes two problems:WaitAck(initial INVITE) orConfirmed(re-INVITE) forever, with no state event and no BYE. The peer may consider the call up while no media flows, and the application cannot see that anything is wrong.In addition, Timer G doubled up to 64·T1 rather than T2, so later retransmissions were 8 s and 16 s apart.
Reproduction
New
src/dialog/tests/test_uas_ack_timeout.rsuses real UDP, a raw UAC and the usual UAS loop. Timers are short but keep the RFC's T4/T1 ratio: T1 = 20 ms, T4 = 200 ms, 64·T1 = 1.28 s.Terminated(Timeout), and a BYE must arrive no earlier than 64·T1, with the right Call-ID, From tag (the 2xx's To tag) and To tag.t2= 4·T1, every retransmission interval stays at or below T2 until 64·T1.Terminated(Timeout)and BYE.Confirmed.Fix
transaction.rs: a server INVITE enteringCompletedno longer arms Timer K. Timer D (64·T1) already ends the transaction; for a non-2xx it acts as Timer H, and for a 2xx it is the retransmission limit.Confirmedstill arms T4 (Timer I).transaction.rs: Timer G doubles up to the newEndpointOption::t2, which defaults to 4 s (the RFC value).dialog.rs: newDialogInner::end_session_without_ack. When the INVITE or re-INVITE transaction reachesTerminatedafter we sent a 2xx and no ACK came, it emitsTerminated(TerminatedReason::Timeout)and then sends a BYE (best effort; a failure is logged). It is called fromhandle_inviteandhandle_reinvitein bothInviteDialogand the deprecatedServerInviteDialog.Transaction::cleanuptakeslast_responseon termination.Nothing changes when the ACK arrives, when the INVITE is rejected or CANCELled, or when the endpoint shuts down (in that case the transaction is not
Terminated).Review
Codex adversarial review, round 1:
t2and a test.Terminated(Timeout)is published before the BYE is sent. Sending the BYE first would delayTerminatedby up to 64·T1 when the peer is gone, and the peer has already failed to ACK. A provisional to that BYE could still notifyEarlyafterTerminated; that is the separatetransition()ordering fix. Codex agreed this is an acceptable tradeoff.Confirmation pass: all three fixes confirmed. It found one new Minor: the re-INVITE test did not wait for
Confirmedbefore sending the re-INVITE, which could race. Fixed.Out of scope, left unchanged:
🤖 Generated with Claude Code
https://claude.ai/code/session_01L1Gu5CifqgBjASbxYmJ6mr