fix(sip): M7 follow-up batch — re-INVITE From identity + CP3 early-media teeth + CP1 ACK labels#162
Merged
Merged
Conversation
…dia teeth + CP1 ACK labels Three small items from the M7 independent reviews: 1. CMD-MINOR (CP2 From consistency): build_reinvite hardcoded sip:asterisk@<sig> in From, so a re-INVITE on a carrier dialog with from_user configured reverted to the internal identity mid-dialog (ConfBridge SFU-video path), contradicting the "From identity stays stable for the dialog's lifetime" invariant. build_reinvite and build_reinvite_ack now use from_uri() like the INVITE/BYE. Red-capable regression: reinvite_and_its_ack_from_match_configured_identity. 2. WIRE-MINOR-1 (CP3 test teeth): the NO-RTP-before-answer guard could only ever fail as an orphaned-200 — no code path emitted app media before answer. The harness now (a) runs an early-media scenario where the carrier sends 183 Session Progress WITH SDP before its delayed 200 (same To tag, one early dialog), so a pre-answer media path EXISTS, and (b) plays a real generated G.711u file via Playback() and REQUIRES post-answer RTP > 0, proving the app media pump delivers. Together: if the Originate wait were defeated so the app ran on the 183, the app's Playback RTP arrives pre-answer and the RTP phase=pre == 0 guard REDs on its own (captured: 52 pre-answer datagrams under a 183-treated-as-answer defeat). 3. WIRE-MINOR-2 (CP1 label clarity): carrier.py labeled ANY non-challenge ACK as ACK-2XX-AT-CORE by a coarse branch match, mislabeling a 403 retry's non-2xx ACK (which correctly returns to the core with the rejected INVITE's branch per RFC 3261 §17.1.1.3) as a route-set violation. ACKs are now labeled per-transaction: ACK-CHALLENGE / ACK-CHALLENGE-RETX / ACK-NON2XX-AT-CORE / ACK-2XX-AT-CORE. Existing run.sh assertions unchanged. TEST values only; isolated offline Docker harnesses; live stack untouched.
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.
Fast-track follow-up batch from the M7 independent reviews (dispatcher-approved fast-track: single PR, squash on green). master @ 4e1c017. TEST values only; isolated offline Docker harnesses; the live voice stack was not touched.
1. CMD-MINOR — re-INVITE From identity (CP2 consistency)
SipSession::build_reinvitehardcodedsip:asterisk@<sig>in the From header instead offrom_uri(), so a re-INVITE on a carrier dialog withfrom_user/from_domainconfigured (reachable via the ConfBridge SFU-video renegotiation path) reverted to the internal identity mid-dialog — contradicting the "From identity stays stable for the dialog's lifetime" invariant that INVITE/BYE already honor. Not a leak (it used the NAT-scoped signalling address), purely an identity-consistency defect.Fix:
build_reinviteANDbuild_reinvite_ack(the ACK for that same re-INVITE must carry the identical From) now usefrom_uri()+ the dialog local tag, exactly likebuild_bye.RED captured (test committed, fix reverted):
2. WIRE-MINOR-1 — CP3 NO-RTP guard gets independent teeth
The CP3 harness's
RTP phase=pre == 0check was correctly coded but vacuous: no code path emitted app media before answer, so a defeated Originate-wait could only ever surface as an orphaned-200. Two changes give it real, independent teeth:CP3_SCENARIO=early-media, both scenarios run by default): the carrier sends a 183 Session Progress WITH SDP before its delayed 200 (same To tag — one early dialog confirmed by the 200). A pre-answer media path now EXISTS: rustisk knows the carrier's RTP address from the 183.Playback()of a generated (TEST-only) 1s G.711u file, and the verdict REQUIRESRTP phase=post > 0— proving the app's media pump actually delivers to the carrier, sophase=pre == 0is a real no-media-before-answer guarantee, not a vacuous one.GREEN (fix intact, both scenarios): pre-answer silence PASS, 183-before-200 PASS,
RTP pre = 0,RTP post = 52— the Originate wait holds through early media.RED captured (temporary local defeat, never committed: 183 treated as answer —
(200..300).contains(&status_code) || status_code == 183— so the app runs on early media):The app's Playback RTP appears pre-answer and the RTP guard REDs on its own — the NO-RTP guarantee no longer depends on the orphaned-200 signal.
3. WIRE-MINOR-2 — CP1 carrier.py ACK labeling (cosmetic)
carrier.pylabeled ANY ACK that didn't match the challenged INVITE's branch asACK-2XX-AT-CORE(the route-set-violation RED label). A 403'd retry's non-2xx ACK — which correctly returns to the core reusing the rejected INVITE's branch (RFC 3261 §17.1.1.3) — would be mislabeled as a violation, and a retransmitted challenge-ACK likewise fell into the same coarseelse. ACKs are now labeled per-transaction (by branch):ACK-CHALLENGE/ACK-CHALLENGE-RETX/ACK-NON2XX-AT-CORE/ACK-2XX-AT-CORE. run.sh assertions are unchanged (the new labels cannot collide with the existinggreppatterns); CP1 harness re-run end-to-end: PASS (a)-(d).Verification
cargo test --workspace --exclude pjsip-shim: 4477 passed (includes the new red-capable regression)cargo clippy --workspace --exclude pjsip-shim -- -D warnings: clean