Skip to content

fix: notify dialog state only for transitions that are applied - #148

Open
tgeorge06 wants to merge 1 commit into
restsend:mainfrom
tgeorge06:fix/no-notification-after-terminated
Open

tgeorge06 wants to merge 1 commit into
restsend:mainfrom
tgeorge06:fix/no-notification-after-terminated

Conversation

@tgeorge06

Copy link
Copy Markdown

Problem

DialogInner::transition (src/dialog/dialog.rs) sends the new state on state_sender before it decides whether to apply it. When the transition is then ignored, subscribers have still been told about it:

  • Duplicate Terminated: two teardown paths can end the same dialog. For example, a local BYE completes while the peer's BYE is also handled. Each path calls transition(Terminated). The second call is ignored ("dialog already terminated") but has already been notified, so subscribers get Terminated twice. Anything that does teardown work on Terminated (call records, cleanup, releasing resources) runs twice.
  • Confirmed after Terminated: since 4feaf95, INFO/UPDATE/REFER/MESSAGE/NOTIFY handlers call return_to_confirmed once the request is answered. If a BYE lands while the application is still holding the INFO handle, the dialog terminates first. The late return_to_confirmed is then ignored but still notified, so subscribers see Terminated → Confirmed, which cannot happen.
  • WaitAck on a confirmed dialog: this transition is also ignored but still notified.

The dialog's stored state was always correct. Only the notification stream was wrong.

Reproduction

src/dialog/tests/test_state_after_terminated.rs reproduces it end to end through the public API: a UAS built the usual way (incoming_transactions → get_or_create_server_invite / match_dialog → handle) over UDP; the peer sends INFO, then BYE before the application answers the INFO; once the application answers, main notifies Confirmed after Terminated:

nothing may be notified after Terminated, got ["…(Confirmed)"]

The unit tests in src/dialog/tests/test_dialog_states.rs pin each case and fail like this on current main:

test_no_state_notification_after_terminated
  left: ["Confirmed", "Terminated", "Terminated", "Confirmed"]
 right: ["Confirmed", "Terminated"]

test_info_answered_after_bye_does_not_notify_confirmed   (drives ServerInviteDialog::handle end to end)
  left: ["Terminated", "Confirmed"]
 right: ["Terminated"]

test_ignored_waitack_after_confirmed_is_not_notified
  left: ["Confirmed", "WaitAck"]
 right: ["Confirmed"]

test_dialog_lifecycle_notifies_each_state_once checks that the normal lifecycle (Calling → Trying → Early → Confirmed → Terminated) still notifies each state exactly once and in order. It passes both before and after the change.

Fix

In transition, for state-changing variants:

  1. Take the state lock.
  2. Run the existing ignore checks. An ignored transition returns without notifying.
  3. Store the new state, then send it while still holding the lock.

Sending under the lock means the order of notifications matches the order of state changes when two tasks transition at the same time. The channel is an unbounded tokio mpsc, so send never blocks or runs user code. Holding the parking_lot mutex across it is safe.

The event-only variants (Updated, Notify, Info, Options) are unchanged. They are sent unconditionally and do not touch the stored state. They carry a TransactionHandle the application must answer, so they are passed through as before.

Diff: 8 lines changed in dialog.rs, plus tests.

Compatibility / risk

  • Subscribers no longer receive notifications for transitions that were ignored. A consumer that counted on a second Terminated, or on a WaitAck for an already-confirmed dialog, will no longer get it. Those events never described a real state change. No in-repo consumer (src/, examples/, bench_ua) relies on them.
  • A subscriber that reads dialog.state() right after receiving a notification now sees the notified state. Before, it could still see the old one.
  • No public API changes.

Checks

  • cargo fmt --all -- --check: clean
  • cargo build: ok
  • cargo test: 335 lib tests passed (5 new), 65 doc-tests passed, 0 failed
  • cargo clippy --all-targets: no new warnings. The existing never_loop error in lib tests is unchanged from main.

DialogInner::transition sent the new state to subscribers before
deciding whether to apply it, so ignored transitions were still
notified: a second Terminated when two teardown paths race (a local BYE
completing while the peer's BYE is handled), a Confirmed after
Terminated when a mid-dialog request (INFO/UPDATE/REFER/...) is
answered after a BYE and return_to_confirmed runs, and a WaitAck on an
already confirmed dialog.

Decide first, update the state, then send the notification while still
holding the state lock, so every notification describes a transition
that happened and notifications follow the order of state changes.
Event-only variants (Updated/Notify/Info/Options) are sent as before.

Adds an end-to-end test over UDP through DialogLayer: the peer sends an
INFO, then a BYE before the application answers it; once the INFO is
answered, no Confirmed may follow the Terminated notification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant