Joao/fix reconnection after verto invite only #1227

jpsantosbh · 2025-05-30T12:31:51Z

Description

This is a slip from #1216 to simplify the revision

This is handle case 2

Type of change

Internal refactoring
Bug fix (bugfix - non-breaking)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

In case of new feature or breaking changes, please include code snippets.

changeset-bot · 2025-05-30T12:31:54Z

🦋 Changeset detected

Latest commit: 9519fc2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@signalwire/webrtc	Minor
@signalwire/core	Minor
@signalwire/js	Minor
@sw-internal/e2e-js	Patch
@signalwire/realtime-api	Patch
@signalwire/web-api	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

iAmmar7 · 2025-06-03T15:37:41Z

Does this PR need to target the base branch or the main branch?

iAmmar7

I would look into the flow of not attaching any workers before receiving the verto.invite response. It might help us reducing all these complexities.

However, since this is urgent, I would suggest fixing the CI problems so that we can merge the PR.

Please look into my double retry logic comment for the verto invite.

iAmmar7 · 2025-06-04T09:46:56Z

packages/js/src/fabric/WSClient.ts

-      video = true
-      negotiateVideo = true
-    }
+      const queryParams = new URLSearchParams(query)


Please run the prettier

iAmmar7 · 2025-06-04T09:53:26Z

packages/webrtc/src/BaseConnection.ts

-        this.logger.error('Invite error', error)
-        reject(error)
-      }
+    return asyncRetry({


I don't think the retry logic should be touching the internal methods/or API calls. This will make the SDK inconsistent, and it also does not care about developer-passed params, which means the developer can not control the retry specifically for the verto.invite.

Also, I am confused. The global retry will also retry this API? Because this will in the end call the verto.invite, which will trigger the execute, and execute has a global retry logic. So, is this API nested under two retry methods?

The global retry doesn't retry verto.invites anymore, let it be handled here...
The verto.invite is a special case, and this is the main reason users are being disconnected from calls ATM.

In a nutshell, the SDK can't replay a verto.invite, it's to "execute" a new one using a new RTCPeer, or just after connecting, the call will disconnect for various reasons.

We may find other RPC with the same characteristics, but this PR only focuses on the verto.invite request.

iAmmar7 · 2025-06-04T09:58:47Z

packages/webrtc/src/BaseConnection.ts

+    return asyncRetry({
+      asyncCallable: async () => {
+        return new Promise(async (resolve, reject) => {
+          await this._waitUntilSessionAuthorized()


How do you think the user can call this API if the session is not authorized? This is already a part of the connect logic. If that does not happen, the user can't call this API. If that happens, then do we need to wait here?

It's not the user calling it, but he next scheduled retry, which is normally faster than the authentication.
So this Retry usage/implementation needs to handle that, or it's going to fail again.

iAmmar7 · 2025-06-04T09:59:50Z

packages/webrtc/src/BaseConnection.ts

@@ -794,7 +844,7 @@ export class BaseConnection<
  }

  /** @internal */
-  onLocalSDPReady(rtcPeer: RTCPeer<EventTypes>) {
+  async onLocalSDPReady(rtcPeer: RTCPeer<EventTypes>) {


This is not an async method

Suggested change

async onLocalSDPReady(rtcPeer: RTCPeer<EventTypes>) {

onLocalSDPReady(rtcPeer: RTCPeer<EventTypes>) {

async is not required here, but the method returns Promises, and since the return is being inferred,
Having the async act as a reminder for us, reading the code the the method is an async execution.

iAmmar7 · 2025-06-04T10:00:04Z

packages/webrtc/src/BaseConnection.ts

      this.logger.debug('Invite response', response)

      this.resuming = false
    } catch (error) {
-      this.setState('hangup')
+        this.setState('hangup')


run prettier please

iAmmar7 · 2025-06-04T10:01:47Z

packages/webrtc/src/workers/promoteDemoteWorker.ts


-  getLogger().debug('promoteDemoteWorker:', action.type, action.payload)
+    getLogger().debug('promoteDemoteWorker:', action.type, action.payload)


Run the prettier, please. I don't understand the changes here.

iAmmar7 · 2025-06-04T10:07:39Z

packages/webrtc/src/BaseConnection.ts

+      if (this.state === 'requesting') {
+        // The Server Created the call, and now is trying to connect us to the destination
+        this.setState('trying')
+      }


What's the point? Where are you using this trying state? The SDK is already setting the state to requesting when the invite begins, and it only sets the state to active when verto.answer is received. Please check the vertoEventWorker code.

https://github.com/signalwire/signalwire-js/blob/main/packages/webrtc/src/workers/vertoEventWorker.ts#L71

trying is a well-known state in a call setup.
https://github.com/signalwire/signalwire-js/blob/main/packages/core/src/utils/interfaces.ts#L276

The SDK should be in the requesting state only until the request is confirmed by the server.
After the successful request, the state should now be trying and later after the verto.answer active.

This makes more sense in 1:1 calls, supported by the CF SDK. I believe that's why it was missing in the Video SDK.

jpsantosbh · 2025-06-04T15:42:57Z

changing the base on this one too

jpsantosbh added 23 commits April 16, 2025 16:53

shouldAttach change

82e9b58

reenable tests

be09de2

Merge branch 'main' into joao/fix_network_handoff

e195e97

no verto.invite for simple re connections

d90b023

changeset

2fd2f12

Merge branch 'main' into joao/fix_network_handoff

783d4e8

Merge branch 'main' into joao/fix_network_handoff

571132f

review changes

8e7faa8

use dialAddress

26a9a10

manually tested

d6a9bed

fix ws re-connections

2cea4c1

revert expect auth)state

2982f35

outbound calls reconnects

f549670

fix import

02cb470

skip test

595fd47

restore auth retry

67b77c0

incoming calls reattach

5c60b10

Merge branch 'main' into joao/fix_reconnections

26cb6f1

cleanup

85c58cc

calee reattach fixed

406877b

cleanup

39f46b0

Merge branch 'main' into joao/fix_reconnections

1e44780

create a new peer to retry a verto.invite

9519fc2

jpsantosbh requested a review from iAmmar7 May 30, 2025 12:32

iAmmar7 reviewed Jun 4, 2025

View reviewed changes

jpsantosbh changed the base branch from main to joao/fix_reconnections June 4, 2025 15:43

jpsantosbh marked this pull request as draft June 4, 2025 15:43

Base automatically changed from joao/fix_reconnections to main June 11, 2025 16:40

	async onLocalSDPReady(rtcPeer: RTCPeer<EventTypes>) {
	onLocalSDPReady(rtcPeer: RTCPeer<EventTypes>) {


		getLogger().debug('promoteDemoteWorker:', action.type, action.payload)
		getLogger().debug('promoteDemoteWorker:', action.type, action.payload)

Joao/fix reconnection after verto invite only #1227

Are you sure you want to change the base?

Joao/fix reconnection after verto invite only #1227

Conversation

jpsantosbh commented May 30, 2025

Description

Type of change

Code snippets

Uh oh!

changeset-bot bot commented May 30, 2025

🦋 Changeset detected

Uh oh!

iAmmar7 commented Jun 3, 2025

Uh oh!

iAmmar7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpsantosbh commented Jun 4, 2025

Uh oh!

Uh oh!