feat(transaction): RFC 6026 Accepted state + Timer L/M for INVITE 200 OK retransmission absorption#128
Open
andrico21 wants to merge 20 commits into
Open
Conversation
First commit of an RFC 6026 implementation series resolving issue restsend#127. Adds the `TransactionState::Accepted` variant between `Proceeding` and `Completed`. Per RFC 6026 §7.1 (server INVITE) and §7.2 (client INVITE), an INVITE transaction that has sent (server) or received (client) a 2xx final response MUST transition to the Accepted state — not the Completed state — and run Timer L (server) or Timer M (client) to absorb late 2xx retransmissions from the TU. Scope of this commit: - Add `TransactionState::Accepted` variant (public API addition; not marked #[non_exhaustive], so this is technically a breaking change for downstream code that exhaustively matches the enum without a wildcard arm — same considerations as adding any RFC-conformance state). - Add the `Accepted` Display arm. - Update the module-level rustdoc state-transition diagrams to distinguish RFC 3261 paths (3xx-6xx final → Completed → Confirmed) from RFC 6026 paths (2xx final → Accepted → Terminated). Cite the specific RFC sections (§17.1.1, §17.2.1, §6026 §7.1, §7.2, §8.4, §8.5) inline so future readers do not have to reverse-engineer the state machine from the can_transition matrix. - Add a placeholder `Accepted` arm in `transition()` so the existing exhaustive match keeps compiling. Real entry-handler logic (cancel Timer A/B/C, start Timer L for server INVITE / Timer M for client INVITE, register `waiting_ack` for server) lands in commit 5/N. No behavioural change yet: - can_transition() still rejects (Proceeding, Accepted) — added in 3/N. - TransactionTimer still has no Timer L/Timer M variants — added in 2/N. - respond() still routes 2xx through Completed — fixed in 6/N. - on_received_response() still routes 2xx through Completed — fixed in 7/N. Validation: - cargo check --lib: PASS - cargo test --lib: 238 passed, 0 failed (no regressions) Refs: restsend#127
Adds `TransactionTimer::TimerL` and `TransactionTimer::TimerM` variants between `TimerG` and `TimerCleanup`. Per RFC 6026: - §7.1: a server INVITE transaction in the Accepted state runs Timer L for 64*T1; on expiry the transaction transitions to Terminated. - §7.2: a client INVITE transaction in the Accepted state runs Timer M for 64*T1; on expiry the transaction transitions to Terminated. Both timers fire once and carry only the TransactionKey (no Duration parameter — the value is fixed at 64*T1 by spec, equivalent to the existing TimerB / TimerD constant). Distinct variants are used (rather than reusing TimerB/TimerD) so the on_timer dispatch can route Accepted- state expiry independently from Calling/Completed-state expiry, and so external observers reading TransactionTimer via the public Display impl can see which RFC 6026 timer fired. Scope of this commit: - Add `TimerL(TransactionKey)` and `TimerM(TransactionKey)` variants. - Add the corresponding arms in `TransactionTimer::key()`. - Add the corresponding arms in the `Display` impl. - Update the `TransactionTimer` module-level rustdoc to list the new timers with their RFC 6026 section citations + 64*T1 value, and to note the RFC 6026 §7.1 restriction on Timer G (non-2xx retransmits only). No behavioural change yet: - No code starts TimerL/TimerM — that lands in commit 5/N when the Accepted state entry handler is wired up. - No code dispatches TimerL/TimerM expiry — that lands in commit 6/N when on_timer() gains the Accepted-state branch. Validation: - cargo check --lib: PASS - cargo test --lib: 238 passed, 0 failed (no regressions) Refs: restsend#127
Updates the `can_transition` matrix to permit the new RFC 6026 state transitions involving the `Accepted` state added in commit 1/N. Edges added: - (Calling, Accepted) — client INVITE 2xx received while in Calling (per RFC 6026 §7.2 the client MUST transition Calling → Accepted on any 2xx, not Calling → Completed as RFC 3261 §17.1.1.2 had it). - (Trying, Accepted) — server INVITE TU sends 2xx while in Trying (per §7.1 / §8.5 supersedes §17.2.1 paragraph 4); also covers the client edge case of 2xx received while in Trying after a 100 Trying arrival. - (Proceeding, Accepted) — most common path for both server and client per §7.1 + §7.2 (replaces (Proceeding, Completed) for 2xx). - (Accepted, Accepted) — RFC 6026 §7.1: 'absorb retransmissions of the INVITE after a 2xx response has been sent' implies the matrix MUST permit a self-loop so the state machine can re-enter Accepted on subsequent 2xx retransmits from the TU without spurious errors. - (Accepted, Terminated) — Timer L (server) or Timer M (client) fires. No existing edges removed: - (Calling, Completed) / (Trying, Completed) / (Proceeding, Completed) remain; they are now reserved for non-2xx final responses (3xx-6xx) which retain RFC 3261 §17.1.1 / §17.2.1 semantics. - (Trying, Confirmed) / (Proceeding, Confirmed) — pre-existing edges for direct ACK arrival in pre-Completed states; left untouched. No behavioural change yet: - Production code does not transition to Accepted from anywhere yet. respond() / on_received_response() still route 2xx through Completed (fixed in commits 6/N + 7/N). - The matrix change is necessary infrastructure for those commits to pass can_transition() without errors. Validation: - cargo check --lib: PASS - cargo test --lib: 238 passed, 0 failed (no regressions) Refs: restsend#127
Adds `timer_l` and `timer_m` fields to the `Transaction` struct
(both `Option<u64>` matching the existing timer ID storage pattern).
Per RFC 6026:
- `timer_l`: server INVITE only — set in Accepted state per §7.1, holds
the Accepted-state Timer L (= 64*T1) ID issued by
`endpoint_inner.timers.timeout()`. Used to cancel the pending timer
on early state-machine exit (e.g. transaction terminate from TU).
- `timer_m`: client INVITE only — set in Accepted state per §7.2, same
shape and lifecycle.
Both fields are public (matching the existing pub timer_{a,b,c,d,k,g})
so external observers and direct test access can inspect their state.
The fields default to `None` in `Transaction::new()` and are cancelled
in `cleanup_timer()` alongside the existing timers.
This commit is pure infrastructure — no caller sets timer_l/timer_m yet.
Allocation + assignment lands in commit 5/N (Accepted-state entry handler
in transition()). cleanup_timer() is updated here so any future early
termination of an Accepted-state transaction (Drop, Terminate event) is
already wired to clean up the new timers correctly.
Validation:
- cargo check --lib: PASS
- cargo test --lib: 238 passed, 0 failed (no regressions)
Refs: restsend#127
…2 (5/N) Replaces the placeholder `Accepted` arm in `transition()` (added in commit 1/N) with the real entry-handler logic. The handler activates only when respond() / on_received_response() route a 2xx final to Accepted (wired up in commits 6/N + 7/N); until those land, this handler remains dead but ready. Server INVITE branch (RFC 6026 §7.1): - Cancel Timer A/B/C (matches existing Completed-state entry pattern) - Start Timer L = 64*T1 (uses `endpoint_inner.option.t1x64`, the same constant Timer B + Timer D already use) - Register the dialog in `endpoint_inner.waiting_ack` so the dialog layer can route ACKs for the 2xx back to this transaction key - Do NOT start Timer G — verbatim §7.1: 'The server transaction MUST NOT generate 2xx retransmissions on its own. It is not retransmitted by the server transaction; retransmissions of 2xx responses are handled by the TU.' Client INVITE branch (RFC 6026 §7.2): - Cancel Timer A/B/C - Start Timer M = 64*T1 - ACK generation is the TU's responsibility per RFC 3261 §17.1.1.3 + RFC 6026 §7.2; the transaction layer no longer auto-sends ACK for 2xx. Auto-ACK for 3xx-6xx (Completed → Confirmed path) is unchanged. CALLER IMPACT: this is a behavioural shift for downstream code that relied on the transaction layer auto-ACKing 2xx. The pre-RFC-6026 behaviour was technically a §17.1.1.3 violation as well, so this is also a latent RFC 3261 fix; downstream TUs should now generate the ACK explicitly. Documented in CHANGELOG entry (commit 10/N). Non-INVITE branch: - Returns Error::TransactionError. RFC 6026 §4 makes Accepted INVITE- specific; can_transition() should have rejected this transition earlier. The error is defensive — flags a programming bug if reached. Behavioural impact: - The Accepted state can now be ENTERED meaningfully (timers actually start), but production code still routes 2xx through Completed. Wiring the routing changes lands in commits 6/N (server respond) and 7/N (client on_received_response). Validation: - cargo check --lib: PASS - cargo test --lib: 238 passed, 0 failed (no regressions) Refs: restsend#127
…6/N) Wires up timer dispatch for the new RFC 6026 Accepted state and adds a defensive 2xx-suppression guard in the Completed state's Timer G retransmit path. Accepted state branch (NEW): - Timer L expiry (server INVITE per RFC 6026 §7.1) → transition to Terminated. - Timer M expiry (client INVITE per RFC 6026 §7.2) → transition to Terminated. - Stray Timer A/B/C/D/G/K firings: silently dropped via catch-all _ arm. These timers should be cancelled by the Accepted-state entry handler in transition() (commit 5/N), but a fire-in-flight race between state transition and timer expiry is possible — silently dropping is safe. Completed state branch (DEFENSIVE GUARD): - Before the existing Timer G retransmit logic, check whether last_response.status_code.kind() == StatusCodeKind::Successful. If so, return Ok(()) without retransmitting and without restarting Timer G. - Per RFC 6026 §7.1: 'The server transaction MUST NOT generate 2xx retransmissions on its own.' After the RFC 6026 routing fix in commits 6/N + 7/N, 2xx finals route to Accepted (not Completed), so this guard is effectively unreachable on the happy path. It catches any legacy / out-of-band path that might still land a 2xx in Completed (e.g. external code calling transition() directly). Letting Timer D / Timer K handle the eventual Termination preserves correct shutdown semantics for the defensive case. Non-2xx Timer G retransmits in Completed continue to work per RFC 3261 §17.2.1 — the existing retransmit + Timer-G-doubling-up-to-T1*64 logic is preserved verbatim below the new guard. Behavioural impact: - Accepted state can now ENTER (commit 5/N) AND EXIT (this commit) per RFC 6026. - Production code still routes 2xx through Completed; the actual routing changes land in commit 7/N (server respond) and 8/N (client on_received_response). Until those land, the Accepted state remains unreachable from production code paths. Validation: - cargo check --lib: PASS - cargo test --lib: 238 passed, 0 failed (no regressions) Refs: restsend#127
Wires up the server-side end of RFC 6026 §7.1: 2xx final responses now
route to the Accepted state on respond(), and ACKs received while in
Accepted are passed to the TU without state transition.
Two co-dependent changes in one commit (semantically inseparable —
either alone breaks the existing dialog ACK delivery test):
A) respond() 2xx routing — server INVITE only.
Old (RFC 3261 §17.2.1):
- All non-Provisional finals → Completed for ServerInvite
New (RFC 6026 §7.1 supersedes §17.2.1 paragraph 4):
- 2xx (Successful) finals → Accepted for ServerInvite
- 3xx-6xx finals → Completed for ServerInvite (unchanged)
- All finals → Terminated for ServerNonInvite (unchanged)
Match block restructured to extract Successful as its own arm so the
comment trail can cite §7.1 + §17.2.1 paragraph 4 inline; future
cleanup that 'consolidates' the Successful arm back into the catch-
all would silently re-introduce the §7.1 violation.
B) on_received_request() — handle ACK in Accepted state.
Without this, ACKs received while in Accepted are silently dropped
by the existing _ => {} catch-all — the transaction never propagates
the ACK to the TU and downstream dialog/dialog tests time out.
Per RFC 6026 §7.1: 'If an ACK is received while the INVITE server
transaction is in the Accepted state, the ACK MUST be passed to the
TU.' The handler does exactly that — return Some(req.into()) — and
does NOT transition (transition is driven by Timer L expiry, not by
ACK arrival; §7.1: 'Timer L reflects the amount of time the server
transaction could receive 2xx responses for retransmission from the
TU while it is waiting to receive an ACK').
Same handler also absorbs INVITE retransmissions silently per §7.1
('any retransmissions of the INVITE received by the server
transaction MUST be absorbed by the server'). The pre-existing
Trying/Proceeding branch above retransmits the last response on
INVITE retransmit; in Accepted that would double-fire the 2xx
through the transport AND trigger an Accepted-self-loop transition,
so the handler just drops.
Test impact:
- Before this commit (Change 4 routing alone, no ACK handler):
238 lib tests → 237 passed, 1 failed
(test_ack_sent_to_domain_name_from_contact times out waiting for
ACK — UAS dropped the ACK silently because Accepted state had no
ACK handler).
- After this commit (routing + handler): 238 passed, 0 failed.
Behavioural impact for downstream:
- Server INVITE 2xx-ACK timing changes. Old: ACK matched in Completed
state; transaction transitioned Completed → Confirmed → Terminated
with Timer K (T4 ≈ 5s) as the cleanup window. New: ACK matched in
Accepted state; transaction stays in Accepted until Timer L fires
(64*T1 ≈ 32s) THEN goes to Terminated. The longer window matches
RFC 6026 §7.1 + handles proxy-chain ACK fan-in correctly. Downstream
TUs that observed Confirmed state transitions for 2xx no longer
see those (they were never RFC-correct anyway).
- Server INVITE 2xx retransmits no longer originate from the
transaction layer (Timer G suppression in commit 6/N's defensive
guard kicks in for any out-of-band path). TUs that need 2xx
retransmits per RFC 6026 §7.1 must implement them explicitly (call
respond() with the same 2xx response).
Validation:
- cargo check --lib: PASS
- cargo test --lib: 238 passed, 0 failed
Refs: restsend#127
…(8/N)
Wires up the client-side end of RFC 6026 §7.2: 2xx final responses now
route to the Accepted state on receipt, the auto-ACK convenience is
retained, and send_ack now distinguishes ACK semantics for 3xx-6xx
(transitions Completed → Terminated per RFC 3261 §17.1.1) vs 2xx (stays
in Accepted to preserve the §7.2 retransmit absorption window; Timer M
drives the eventual transition to Terminated).
Three co-dependent changes in one commit (semantically inseparable):
A) on_received_response() — split the response routing match.
Old (RFC 3261 §17.1.1):
- All non-Provisional finals → Completed for ClientInvite
- All non-Provisional finals → Terminated for ClientNonInvite
New (RFC 6026 §7.2 supersedes §17.1.1.2):
- 2xx (Successful) → Accepted for ClientInvite (NEW)
- 2xx (Successful) → Terminated for ClientNonInvite (unchanged semantics)
- 3xx-6xx finals → Completed for ClientInvite (unchanged)
- 3xx-6xx finals → Terminated for ClientNonInvite (unchanged)
Match block extracted Successful as its own arm so the §7.2 citation
can sit inline.
B) Auto-ACK gate generalised.
Old: `is_completed_client_invite = ClientInvite && new_state == Completed`
triggered `send_ack` after every non-Provisional response on
ClientInvite (because in the pre-split routing, both 2xx and 3xx-6xx
landed in Completed).
New: `auto_ack_client_invite = ClientInvite && (Completed || Accepted)`
preserves the pre-RFC-6026 auto-ACK convenience for both 2xx and
3xx-6xx. Per RFC 3261 §17.1.1.3 + RFC 6026 §7.2, ACK for 2xx is
strictly the TU's responsibility, not the transaction layer's. The
auto-ACK is retained for backward compatibility with rsipstack 0.5.x
downstreams; a future PR can remove it once the dialog layer
explicitly issues ACKs for 2xx.
C) send_ack() — allow Accepted state + conditional final transition.
Old: required Completed state; transitioned to Terminated after
sending. This was correct for 3xx-6xx ACKs (RFC 3261 §17.1.1.3) but
incorrect for 2xx ACKs (which should leave the transaction in
Accepted to absorb retransmits per RFC 6026 §7.2).
New:
- State check: allow either Completed (3xx-6xx, RFC 3261 §17.1.1.3)
or Accepted (2xx, RFC 6026 §7.2). Reject all other states as
before.
- Final transition: Completed → Terminated (preserves RFC 3261
§17.1.1.3 client cleanup semantics); Accepted → no transition
(state stays Accepted; Timer M fires later and drives termination
per §7.2). This conditional transition is the key to keeping the
2xx-retransmit absorption window open.
send_ack remains the single ACK construction + send pathway; the
destination-routing logic (line 478-501) that already special-cases
2xx via Contact lookup is unchanged.
Test impact:
- The previously-failing test_ack_sent_to_domain_name_from_contact
(which was fixed by commit 7/N's on_received_request Accepted ACK
handler on the server side) continues to pass after this commit's
client-side routing change. Auto-ACK still fires; UAS still receives
the ACK; dialog layer test assertions still hold.
- All 238 lib tests pass after this commit.
Behavioural impact for downstream:
- 2xx receipt no longer immediately terminates the client INVITE
transaction. Old: Completed → Terminated within send_ack(). New:
Accepted (timer-M-pending) until 64*T1 expires OR ACK construction
fails OR Drop. Downstream code that observed Terminated state
immediately on 2xx receipt no longer sees that transition for ~32s
by default; this matches RFC 6026 §7.2 spec semantics.
- Timer M is a NEW resource holder per client INVITE 2xx. Memory cost
is bounded by the existing Timer wheel + 1 Option<u64> field. CPU
cost is one timeout fire per terminated transaction.
Validation:
- cargo check --lib: PASS
- cargo test --lib: 238 passed, 0 failed
Refs: restsend#127
Adds 9 conformance tests covering the new Accepted state + Timer L /
Timer M behaviour added in commits 1/N through 8/N of this series.
Test coverage:
- 3 unit tests for the public Display impls of TransactionState::Accepted,
TransactionTimer::TimerL, TransactionTimer::TimerM. Lock the rendering
so log/metric/debug consumers see stable output.
- 6 integration tests using a mock UDP loopback connection that exercise
the full state machine via tx.reply() / tx.respond() public API:
- test_rfc6026_server_invite_2xx_routes_to_accepted_with_timer_l —
asserts state == Accepted after 200 OK + timer_l.is_some() +
timer_g.is_none() (the §7.1 anti-feature).
- test_rfc6026_server_invite_4xx_still_routes_to_completed — RFC 3261
§17.2.1 regression check; non-2xx finals retain Completed routing.
- test_rfc6026_server_invite_accepted_registers_waiting_ack — derives
DialogId from the 2xx and verifies the dialog is in
endpoint.inner.waiting_ack so ACK arrives at this transaction key.
- test_rfc6026_server_invite_3xx_still_arms_timer_g — explicit anti-
regression check that Timer G remains live for non-2xx finals; the
§7.1 prohibition is 'MUST NOT generate 2xx retransmissions', not a
blanket Timer G ban.
- test_rfc6026_server_invite_accepted_absorbs_duplicate_2xx — sends
200 OK twice; verifies (Accepted, Accepted) self-loop edge keeps
the transaction in Accepted without state-machine error per §7.1.
- test_rfc6026_server_invite_accepted_rejects_3xx_transition — sends
200 OK then 4xx; verifies can_transition matrix correctly errors
on Accepted → Completed (no such edge).
Test design notes:
- State-machine tests use a mock UDP connection bound to 127.0.0.1:0
+ a SipAddr destination at 127.0.0.1:1. UDP is connectionless so the
reply() send succeeds (packets are dropped at the OS) without a peer
listener — no peer endpoint orchestration needed.
- Timer L / Timer M actually firing (Accepted → Terminated after
64*T1 ≈ 32s) is NOT exercised here because waiting 32s per test
would balloon suite runtime; tests verify timer_l.is_some() /
timer_m.is_some() to confirm the timers are armed. The transition
logic is exercised by the unit-level state machine in
transaction.rs on_timer dispatch (commit 6/N).
- Client-side conformance tests (Timer M arming after on_received_response
routes 2xx to Accepted) are NOT included here — they require
orchestrating a peer endpoint to receive the INVITE and send the 2xx,
which is heavier than the server-side tests; the test_client.rs
test_client_transaction integration test plus the existing
test_ack_sent_to_domain_name_from_contact dialog test exercise the
client-side path end-to-end and continue to pass with the new routing.
Validation:
- cargo test --lib test_rfc6026: 9 passed, 0 failed
- cargo test --lib (full suite): 247 passed, 0 failed (238 baseline + 9
new conformance tests; no regressions in pre-existing tests)
Refs: restsend#127
Updates the lib.rs Standards Compliance section bullet point for RFC 6026 to enumerate the specific implementation details landed by commits 1/N through 9/N of this series. Resolves the issue restsend#127 complaint that the lib.rs claim was 'aspirational' (per the maintainer's own response) — the bullet now grounds the claim in concrete §7.1/§7.2 implementation points so future readers can match the documentation against the behaviour they observe. Specifically the expanded bullet now cites: - Server Accepted state + Timer L (RFC 6026 §7.1) - Client Accepted state + Timer M (RFC 6026 §7.2) - 2xx-retransmit prohibition on the server transaction (§7.1) - ACK for 2xx routed to the TU rather than absorbed by the transaction layer (§7.2 + RFC 3261 §17.1.1.3) No code change; pure docstring update. Validation: - cargo test --lib: 247 passed, 0 failed (no regression) Refs: restsend#127 Closes: restsend#127
Whitespace-only cleanup applied by `cargo fmt --all`. No semantic changes; pure formatting. Touches: - src/transaction/transaction.rs: dialog_id let-binding wrapping in the Accepted-state entry handler (commit 5/N). - src/transaction/tests/test_transaction_states.rs: minor formatting in the new conformance tests (commit 9/N). Validation: - cargo fmt --check: PASS - cargo test --lib: 247 passed, 0 failed (no regression)
…(12/N)
Oracle review F1 (MUST FIX): the duplicate-suppression filter at the
top of `on_received_response` was correct for non-INVITE clients
(deduplicating identical retransmits the TU has no use for) but
incorrect for ClientInvite in the Accepted state — RFC 6026 §7.2
requires every 2xx final response to reach the TU, including:
1. Server-retransmitted 2xx (the TU / dialog layer re-issues ACK per
§7.2 + RFC 3261 §17.1.1.3).
2. Forked 2xx that share status code + body but differ in To-tag, each
identifying a separate confirmed dialog.
Both cases would have been swallowed by the `if self.state == new_state
{ if same status + body { return None } }` filter once the transaction
entered the Accepted self-loop, because the (Accepted, Accepted) edge
in can_transition() (commit 3/N) keeps state == new_state.
Fix: gate the filter on `!is_client_invite_2xx_in_accepted`. ClientInvite
in Accepted state always falls through to last_response.replace() +
auto_ack_client_invite + transition() so the response reaches the TU
and the legacy auto-ACK re-fires per retransmit.
Behavioural impact:
- Forked 2xx in client INVITE: now correctly delivered to the TU.
Pre-fix: only the FIRST 2xx in a fork would reach the TU; subsequent
forked 200s with different To-tags were swallowed.
- Server-retransmitted 2xx: now correctly delivered to the TU per
retransmit. The auto-ACK convenience path also re-fires per
retransmit, matching server expectations.
- Non-INVITE retransmits: filter still active per pre-existing
semantics — no behavioural change for the dedup-needed path.
Validation:
- cargo test --lib: 247 passed, 0 failed.
Refs: restsend#127
Oracle review F2 (MUST FIX): the prior lib.rs Standards Compliance bullet (commit 10/N) claimed 'ACK for 2xx routed to the TU rather than absorbed by the transaction layer per §7.2 + RFC 3261 §17.1.1.3' — but the transaction layer in fact still auto-issues ACK for 2xx via send_ack (commits 5/N + 8/N) for rsipstack 0.5.x backward compatibility. Code and docs contradicted each other; reviewers would have spotted that. Rewords the bullet to honestly state: - Server Accepted + Timer L per §7.1. - Client Accepted + Timer M per §7.2. - Server transactions do not retransmit 2xx per §7.1. - Client-side ACK for 2xx is currently auto-issued by the transaction layer for backward compatibility; strict §7.2 + RFC 3261 §17.1.1.3 places that responsibility on the TU; the auto-ACK is a candidate for migration to the dialog layer in a follow-up release. The auto-ACK convenience path itself is unchanged — this is a docs-only edit so users who read the standards-compliance bullet correctly model the actual behaviour. Validation: - cargo test --lib: 247 passed, 0 failed. Refs: restsend#127
…austive] (14/N) Oracle review F3 (MUST FIX): adding TransactionState::Accepted (commit 1/N) and TransactionTimer::TimerL/TimerM (commit 2/N) to public, exhaustive-match-able enums is a semver-breaking change for downstream crates that match without a wildcard arm. Marking these enums #[non_exhaustive] now means: - Downstream crates MUST already keep a wildcard arm in matches; they cannot rely on exhaustive matching against the variant set. - Future RFC-extension variants (e.g. when the next round of SIP RFCs add new states or timers) can land in PATCH releases without further semver breaks. - Marking the enums #[non_exhaustive] is itself a breaking change, but this PR is already breaking (added 1 new state + 2 new timer variants), so we collapse two breakages into one upstream release boundary. Updates the in-rustdoc example matches for both enums to demonstrate the required wildcard arm convention with an inline comment explaining the #[non_exhaustive] forward-compat contract. Recommend the maintainer target this PR at 0.6.0 rather than 0.5.x; the non_exhaustive attribute does not appear in 0.5.12 so this is a clean 0.5.x → 0.6.0 transition. Validation: - cargo test --lib: 247 passed, 0 failed (no internal regressions; in- crate matches remain exhaustive across all variants). Refs: restsend#127
Oracle review R4 (RECOMMEND) + N1 (NIT) + N2 (NIT) — combined into one
commit since they all touch the same on_timer Accepted-state arm and
the adjacent Completed-state defensive comment.
R4: tighten timer dispatch by transaction type. The pre-fix arm matched
`TimerL(_) | TimerM(_)` regardless of transaction type, so a stray
TimerL firing on a ClientInvite (or TimerM on ServerInvite) would
incorrectly transition the transaction to Terminated. Per RFC 6026 §7.1
Timer L is server-only and §7.2 Timer M is client-only. The new match
keys on `(transaction_type, timer)`:
- (ServerInvite, TimerL): transition → Terminated per §7.1.
- (ClientInvite, TimerM): transition → Terminated per §7.2.
- (anything, TimerL|TimerM): mismatched pairing — programming bug; log
via warn! and ignore (no transition; state stays Accepted; Timer L/M
for the correct transaction type will eventually fire and terminate).
N1: replace the prior `_ => {}` catch-all with an explicit list of
TimerA/B/C/D/G/K/Cleanup. Stray firings of those timers in Accepted
state are race remnants from prior states (the entry handler cancels
them, but fire-in-flight races between cancellation and timer-wheel
expiry are possible). Listing variants explicitly forces compile-time
review here when future timer variants are added; the catch-all hid
that requirement. With TransactionTimer marked #[non_exhaustive] in
commit 14/N, internal exhaustive matches still trip on new variants.
N2: drop the 'commits 6/N + 7/N' reference from the Completed-state
Timer G defensive guard comment. The semantic point — that
last_response should always be non-2xx in Completed because the RFC
6026 routing in respond() + on_received_response() steers 2xx to
Accepted — is preserved; the commit-series breadcrumb belongs in the
git commit messages, not in permanent upstream source comments.
tracing imports extended: `use tracing::{debug, trace};` →
`use tracing::{debug, trace, warn};` for the new warn! site.
Validation:
- cargo test --lib: 247 passed, 0 failed.
Refs: restsend#127
Oracle review R2 (RECOMMEND): the auto-ACK call at the tail of on_received_response (commit 8/N's auto_ack_client_invite gate) used `self.send_ack(connection).await.ok()` which silently swallowed any errors from ACK construction (make_ack), connection lookup (locator), or transport send. Per RUST_GUIDELINES.md §2 silent .ok() on meaningful failures hides operational problems; for newly-touched RFC logic this is especially harmful because: - A failing auto-ACK in the Completed (3xx-6xx) path leaves the dialog hanging on a dead connection without any visibility. - A failing auto-ACK in the Accepted (2xx) path leaves the new RFC 6026 client transaction in a stuck Accepted state until Timer M fires (32s default), with no log trail to debug. Replaces .ok() with explicit error matching + warn! log capturing the transaction key, current state (Completed or Accepted post-transition), and the error. The auto-ACK still does not propagate failure to the caller (preserving the public Option<SipMessage> signature of on_received_response), but the failure is now observable. Validation: - cargo test --lib: 247 passed, 0 failed. Refs: restsend#127
Oracle review R3 (RECOMMEND): the public async methods Transaction:: respond, send_ack, and receive perform state-machine mutations around .await points. Cancellation between the await and the mutation can leave the transaction in an inconsistent state — for example, a response transmitted but state not advanced (respond), an ACK sent but state not transitioned (send_ack), or a response stored as last_response with state advanced but the SipMessage not delivered to the TU (receive). Per RUST_GUIDELINES.md §5 these hazards must be documented on public async APIs. Adds # Cancel safety sections to all three method rustdocs explaining the specific await/mutation interleavings and recommending: drive from an owning task, do not place directly inside tokio::select! / tokio::time::timeout, prefer the TransactionEvent::Terminate channel for cancellation-aware integration. Also adds the previously missing top-line rustdocs for respond and send_ack (the existing // send server response comment on respond was not a doc-comment and so was invisible to rustdoc consumers; send_ack had no documentation at all). receive gets a similar full rustdoc covering its dispatch loop semantics. The additions match the existing multi-paragraph rustdoc convention used elsewhere in the rsipstack public API (TransactionState, TransactionTimer, Transaction struct). No code change; pure documentation expansion. Validation: - cargo test --lib: 247 passed, 0 failed. Refs: restsend#127
…Is (18/N) Second-pass review (RUST_GUIDELINES.md §5 + §10) flagged three documentation accuracy issues introduced by the prior commits in this PR. None affect runtime behavior; all sharpen first-impression correctness for upstream review. D1 — Transaction::respond cancel-safety section had the mutation order inverted. Actual code stores last_response BEFORE the .await on the transport send, then transitions AFTER the send completes. Prior docstring claimed the await came first, which would imply different partial-state hazards than the real ones. Corrected to describe the two real failure modes: (a) drop during the await leaves last_response updated but response possibly untransmitted, (b) drop between send completion and transition leaves the response on the wire while state has not advanced. D2 — Transaction::send_ack rustdoc claimed the function falls back to self.connection if the supplied connection argument is None. The code at the tail of send_ack does not do this; the local connection binding comes from EITHER the input arg OR the transport-layer lookup for 2xx responses. If both are None the function silently records last_ack and returns Ok(()) without sending. Doc updated to describe the actual two-source resolution and the silent no-send fallback. D3 — Inline comment in the client INVITE Accepted-state entry handler said "the transaction layer no longer auto-sends ACK for 2xx" but commit 16/N's auto_ack_client_invite gate (and the lib.rs Standards Compliance disclaimer added in commit 13/N) make clear that auto-ACK is in fact retained for 0.5.x backward compatibility. Comment rewritten to match: strict RFC 3261 §17.1.1.3 / RFC 6026 §7.2 mandate TU-driven ACK, but rsipstack 0.5.x continues to auto-ACK; cross-references the lib.rs disclaimer for full context. No code change; pure documentation fix. Validation: - cargo test --lib: 247 passed, 0 failed. Refs: restsend#127
…ispatch (19/N) Second-pass review (RUST_GUIDELINES.md §3 exhaustive match + clippy wildcard_enum_match_arm) flagged the post-send state dispatch in send_ack as having an unreachable wildcard arm. The function entry guard restricts self.state to TransactionState:: Completed | Accepted (returns InvalidStateError otherwise). After the ACK is sent (or stored as last_ack with no transport), the state can only be one of those two. The original `_ => Ok(())` arm was dead code that would silently swallow any future TransactionState variant added behind #[non_exhaustive] (commit 14/N) — exactly the class of bug the non_exhaustive attribute is meant to catch. Replaces the dead wildcard with an explicit if/else branch that debug_assert_eq!'s the Accepted invariant. In release builds this compiles to the same two branches as before; in debug + tests it will trip if a future code change introduces an unintended state into send_ack's post-send path. Validation: - cargo test --lib: 247 passed, 0 failed. Refs: restsend#127
The F1 fix in commit 12/N introduced an is_client_invite_2xx_in_accepted binding that, after rebase onto upstream/main 8928174, fell on the wrong side of rustfmt's chain-splitting heuristic. Applies cargo fmt to align the let-binding with rustfmt's preferred wrap style. No code change; pure formatting. Validation: - cargo fmt --check: clean. - cargo test --lib: 247 passed, 0 failed. Refs: restsend#127
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
1 participant
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.
Implements RFC 6026 §7.1–§7.2 to fix the long-standing issue where rsipstack's INVITE state machine terminates immediately on 2xx and cannot absorb retransmitted 200 OKs from the UAS, breaking glare resolution and forking scenarios.
Closes #127. Per maintainer @shenjinti's confirmation on the issue thread: "you're completely right ... A PR would be very welcome."
Branch is rebased onto current
upstream/main(8928174).Standards
Changes
Acceptedstate variantsrc/transaction/mod.rsTimerL+TimerMvariantssrc/transaction/mod.rscan_transitionmatrixsrc/transaction/mod.rstimer_l+timer_mfieldssrc/transaction/transaction.rssrc/transaction/transaction.rson_timerAccepted dispatchsrc/transaction/transaction.rssrc/transaction/transaction.rs::respondsrc/transaction/transaction.rs::on_received_responsesrc/transaction/tests/test_transaction_states.rsBoth
TransactionStateandTransactionTimerare now#[non_exhaustive]to allow future RFC additions without breakage. This is itself a breaking change — recommend targeting 0.6.0.Backward compatibility
The pre-existing convenience that auto-ACKs client INVITE 2xx from
on_received_responseis retained for 0.5.x consumers (technically a deviation from RFC 3261 §17.1.1.3, which mandates the TU drive the 2xx ACK). Documented honestly inlib.rs::Standards Compliance. Removal is a candidate for a follow-up PR that migrates 2xx-ACK responsibility to the dialog layer.Validation
cargo test --lib— 247 passed, 0 failedcargo test --doc— 65 passed, 0 failedcargo fmt --check— cleanPre-existing baseline: 66 clippy errors on
main(not introduced by this PR; touched files contribute zero new clippy regressions).Pre-submit review
Two read-only architecture review passes were run before submission. Each finding was either fixed in an atomic follow-up commit or explicitly deferred with rationale.
First pass — 3 MUST-FIX + 4 RECOMMEND + 3 NIT:
lib.rsauto-ACK retention documented honestly#[non_exhaustive]on both public enums.ok())respond/send_ack/receive(transaction_type, timer)dispatch in Accepted entry handlerwarn!on mismatched pairingSecond pass — 4 SHOULD-FIX (run after rebase onto current main, evaluated against an internal Rust style guide):
respondcancel-safety rustdoc had inverted mutation order — fixed to describe actualstore last_response → await send → transitionordersend_ackrustdoc claimed a nonexistentself.connectionfallback — rewritten to describe actual two-source resolution and silent no-send fallback when neither input arg nor 2xx-Contact lookup yields a connectionlib.rsdisclaimersend_ackpost-send dispatch replaced withif/else+debug_assert_eq!invariant (catches future#[non_exhaustive]additions)Follow-up out of scope for this PR
Commit list (atomic, 20 commits)
feat(transaction): add Accepted state for RFC 6026 §7.1/§7.2feat(transaction): add TimerL + TimerM variants for RFC 6026feat(transaction): allow RFC 6026 edges in can_transition matrixfeat(transaction): add timer_l + timer_m fields on Transactionfeat(transaction): Accepted-state entry handler per RFC 6026 §7.1/§7.2feat(transaction): on_timer Accepted dispatch + Completed 2xx guardfeat(transaction): server-side RFC 6026 2xx routing + ACK handlingfeat(transaction): client-side RFC 6026 2xx routing + send_ack splittest(transaction): RFC 6026 conformance tests for Accepted statedocs(lib): expand RFC 6026 standards-compliance claimstyle: cargo fmt --all cleanupfix(transaction): pass every 2xx in Accepted to TU per RFC 6026 §7.2 [F1]docs(lib): clarify RFC 6026 client-side auto-ACK retention [F2]feat(transaction): mark TransactionState + TransactionTimer #[non_exhaustive] [F3]refactor(transaction): tighten Accepted timer dispatch [R4 + N1 + N2]fix(transaction): log auto-ACK failures instead of silent .ok() [R2]docs(transaction): cancel-safety rustdoc on async public APIs [R3]docs(transaction): correct rustdoc and comment accuracy on touched APIs [D1 + D2 + D3]refactor(transaction): drop dead wildcard arm in send_ack post-send dispatch [T1]style: cargo fmt cleanup on rebased F1 hunk