feat(calling): keepalive flow refactor#4869
feat(calling): keepalive flow refactor#4869Kesari3008 wants to merge 2 commits intowebex:mobius-socketfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aadf329da6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!APIRequest.instance) { | ||
| APIRequest.instance = new APIRequest(config); | ||
| } | ||
|
|
||
| return APIRequest.instance; |
There was a problem hiding this comment.
APIRequest.getInstance() returns a process-wide singleton that preserves the first webex object forever, so later CallingClient/Registration/Call instances reuse the wrong SDK context. In multi-client scenarios (different users/orgs/devices in the same runtime), subsequent requests are sent with the first client's credentials/session, which can route traffic to the wrong account and break registration/call flows. This regression is introduced by the new static singleton in request.ts; the previous code always used each instance’s own this.webex.request(...) path.
Useful? React with 👍 / 👎.
| this.webWorker?.postMessage({ | ||
| type: WorkerMessageType.KEEPALIVE_RESULT, | ||
| statusCode: res.status, | ||
| }); |
There was a problem hiding this comment.
The keepalive handler posts KEEPALIVE_RESULT back through this.webWorker, which is mutable and may already point to a newly created worker after failover/re-registration. If an old in-flight keepalive resolves late, its failure can be delivered to the new worker and incorrectly increment retry counters, triggering spurious reconnect/unregister behavior. This race was introduced by moving keepalive I/O out of the worker while still routing results via the current shared worker reference.
Useful? React with 👍 / 👎.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| private async postKeepAlive( | ||
| accessToken: string, | ||
| deviceUrl: string, | ||
| url: string, | ||
| trackingId: string | ||
| ): Promise<{status: number}> { | ||
| const mobiusSocket = this.mobiusSocket; | ||
|
|
||
| if (this.isMobiusSocketEnabled && mobiusSocket?.isConnected?.()) { | ||
| await mobiusSocket.send({ | ||
| type: 'status', | ||
| deviceUrl, | ||
| trackingId, | ||
| }); | ||
|
|
||
| return {status: 200}; | ||
| } | ||
|
|
||
| const response = await fetch(`${url}/status`, { | ||
| method: HTTP_METHODS.POST, | ||
| headers: { | ||
| [CISCO_DEVICE_URL]: deviceUrl, | ||
| [SPARK_USER_AGENT]: CALLING_USER_AGENT, | ||
| Authorization: `${accessToken}`, | ||
| trackingId, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw response; | ||
| } | ||
|
|
||
| return response; | ||
| } |
There was a problem hiding this comment.
We can replace this whole thing with makeRequest --- it takes care of making http or socket request
| extension?: string, | ||
| voicemail?: string | ||
| voicemail?: string, | ||
| registrationOptions?: TransportLayer |
There was a problem hiding this comment.
We might not need to pass this information. MobiusSocket is singleton and we can use feature flag util
| isMobiusSocketEnabled?: boolean; | ||
| mobiusSocket?: { | ||
| isConnected?: () => boolean; | ||
| send: (payload: Record<string, unknown>) => Promise<unknown>; |
There was a problem hiding this comment.
We won't be calling this send directly. The type TransportLayer might not be required
| const line = this.isMobiusSocketEnabled | ||
| ? new Line( | ||
| this.webex.internal.device.userId, | ||
| this.webex.internal.device.url, | ||
| this.mutex, | ||
| this.primaryMobiusUris, | ||
| this.backupMobiusUris, | ||
| this.getLoggingLevel(), | ||
| this.sdkConfig?.serviceData, | ||
| this.sdkConfig?.jwe, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { | ||
| isMobiusSocketEnabled: this.isMobiusSocketEnabled, | ||
| mobiusSocket: this.mobiusSocket, | ||
| } | ||
| ) | ||
| : new Line( | ||
| this.webex.internal.device.userId, | ||
| this.webex.internal.device.url, | ||
| this.mutex, | ||
| this.primaryMobiusUris, | ||
| this.backupMobiusUris, | ||
| this.getLoggingLevel(), | ||
| this.sdkConfig?.serviceData, | ||
| this.sdkConfig?.jwe | ||
| ); |
There was a problem hiding this comment.
We don't have to create line object conditionally with mobiusSocket and feature flag info. We will be using the makeRequest method to make api call and it will take care of the transport type
There was a problem hiding this comment.
These are temp changes done only to test the flow, final changes will be adjusted based on other PRs being merged
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c569723e83
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } catch (err: any) { | ||
| this.webWorker?.postMessage({ | ||
| type: WorkerMessageType.KEEPALIVE_RESULT, | ||
| err, |
There was a problem hiding this comment.
When postKeepAlive() gets a non-2xx HTTP response it throws the raw Response object, and this catch block forwards that object directly via Worker.postMessage. Response is not reliably structured-cloneable across worker boundaries, so postMessage can throw DataCloneError on keepalive failures; in that case no KEEPALIVE_RESULT is delivered, the worker stays keepaliveInFlight = true, and keepalive/reconnect handling stops after the first failed tick.
Useful? React with 👍 / 👎.
2 participants
COMPLETES
This pull request addresses
Updated webworker logic of sending keepalives in a way so that we can use wbsocket to send messages along with HTTP
by making the following changes
Moved the actual send out of the webworker but timer is controlled in webworker
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.