fix(datachannel): checkpoint lobby token refresh and llm resubscribe …#4818
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
9500d92 to
d04a422
Compare
d04a422 to
1348a53
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1348a535cd
ℹ️ 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".
| }); | ||
| this.locusInfo.on(LOCUSINFO.EVENTS.SELF_ADMITTED_GUEST, async (payload) => { | ||
| this.stopKeepAlive(); | ||
| await this.ensureDefaultDatachannelTokenAfterAdmit(); |
There was a problem hiding this comment.
Do not block admitted-guest flow on token prefetch
Awaiting ensureDefaultDatachannelTokenAfterAdmit() inside the SELF_ADMITTED_GUEST listener makes MEETING_SELF_GUEST_ADMITTED emission and subsequent updateLLMConnection() depend on a network token fetch. If /datachannel/token is slow or temporarily unavailable, the client can remain in a lobby-like state longer than necessary even though locus has already admitted the user. The admission event path should not be gated by this best-effort prefetch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We should not block admit process before get token. Maybe call it just before updateLLMConnection()
…nnect_reconnect_subscription
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50a55c2bae
ℹ️ 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".
| const refreshedPracticeSessionToken = await this.ensurePracticeSessionDatachannelToken(meeting); | ||
|
|
||
| if (refreshedPracticeSessionToken) { | ||
| practiceSessionDatachannelToken = refreshedPracticeSessionToken; | ||
| } | ||
|
|
||
| // @ts-ignore - Fix type | ||
| return this.webex.internal.llm |
There was a problem hiding this comment.
Re-check practice-session state after async token refresh
updatePSDataChannel() now awaits ensurePracticeSessionDatachannelToken() and then unconditionally calls registerAndConnect(...) using the earlier session state, but there is no second isJoinPracticeSessionDataChannel() guard after the await. If practice mode/panelist status flips while the refresh request is in flight (for example, host ends practice session), a stale invocation can reconnect the practice LLM channel after a newer invocation already cleaned it up, leaving captions/subscriptions attached to the wrong session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d70cb8651
ℹ️ 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".
| this.webex.internal.llm.setDatachannelToken( | ||
| fetchedDatachannelToken, | ||
| DataChannelTokenType.Default | ||
| ); |
There was a problem hiding this comment.
Revalidate meeting identity before caching fetched lobby token
ensureDefaultDatachannelTokenAfterAdmit() writes the fetched token into the shared LLM token cache after an awaited network call, but it never verifies that the meeting/locus is still the same one that initiated the fetch. If the user leaves (or switches meetings) while fetchDatachannelToken() is in flight, this stale token can overwrite the cache after teardown; the next admitted-guest flow then sees a token present and skips fetching the correct one, so updateLLMConnection() can reconnect with invalid credentials.
Useful? React with 👍 / 👎.
| private handleLLMOnline = (): void => { | ||
| // @ts-ignore | ||
| this.webex.internal.llm.off('online', this.handleLLMOnline); | ||
| this.restoreLLMSubscriptionsIfNeeded(); | ||
|
|
There was a problem hiding this comment.
Remove persistent LLM online listener on meeting cleanup
After this change, handleLLMOnline no longer unregisters itself, which makes the listener effectively persistent for the Meeting instance. Teardown still calls cleanupLLMConneciton() without removeOnlineListener, so ended/disposed meetings can keep online callbacks attached and receive future online events from later connections, causing duplicate MEETING_TRANSCRIPTION_CONNECTED triggers and stale-instance callbacks over time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5a1f77643
ℹ️ 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".
| this.ensureDefaultDatachannelTokenAfterAdmit().then(() => { | ||
| this.updateLLMConnection(); | ||
| }); | ||
|
|
||
| this.updateLLMConnection(); |
There was a problem hiding this comment.
Serialize post-admit LLM reconnect attempts
This handler now invokes updateLLMConnection() twice (immediately and again in the ensureDefaultDatachannelTokenAfterAdmit().then(...) path). When prefetch resolves quickly (for example, token already cached or token-auth feature disabled), both calls can run concurrently while isConnected() is still false, causing two registerAndConnect() register requests for the same session. Because LLMChannel.register() persists whichever binding/websocket metadata returns last, this race can leave cached routing metadata out of sync with the actual connected socket and break later routed sends; these reconnect attempts should be serialized or the second call should only run when a new token was fetched.
Useful? React with 👍 / 👎.
| this.webex.internal.llm.setDatachannelToken( | ||
| datachannelToken, | ||
| dataChannelTokenType || DataChannelTokenType.PracticeSession | ||
| ); |
There was a problem hiding this comment.
Revalidate state before caching refreshed practice token
ensurePracticeSessionDatachannelToken() stores the refreshed token in the shared LLM cache before updatePSDataChannel() performs its stale-invocation checks. If practice eligibility or meeting lifecycle changes while refreshDataChannelToken() is in flight (e.g., user leaves and cleanup clears tokens), this late write can repopulate cache with a stale token; subsequent updates then read a cached token and skip refresh, leading to practice-session register attempts with invalid credentials. Guard this cache write with a fresh state/sequence check tied to the current meeting context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16219deb81
ℹ️ 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".
| this._pendingOnlineListener = null; | ||
| } | ||
|
|
||
| const refreshedPracticeSessionToken = await this.ensurePracticeSessionDatachannelToken(meeting); |
There was a problem hiding this comment.
Revalidate default LLM connectivity after token refresh await
In updatePSDataChannel(), default-session connectivity is checked and _pendingOnlineListener is removed before await ensurePracticeSessionDatachannelToken(meeting), but there is no second default isConnected() check afterward. If the default LLM session drops while the token refresh is in flight, this stale invocation can still run registerAndConnect(..., LLM_PRACTICE_SESSION) even though the prerequisite connection is gone, and because the pending online listener was already cleared there is no automatic retry when default comes back online.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // @ts-ignore | ||
| this.webex.internal.voicea.updateSubchannelSubscriptions({subscribe: ['transcription']}); |
There was a problem hiding this comment.
Await transcription resubscribe call in online handler
restoreLLMSubscriptionsIfNeeded() wraps updateSubchannelSubscriptions() in a synchronous try/catch, but updateSubchannelSubscriptions is async and its promise is not awaited or .catched here. If that promise rejects (for example, during reconnect races), the rejection bypasses this catch and becomes unhandled, so the intended warning log and graceful recovery path do not run.
Useful? React with 👍 / 👎.
…nnect_reconnect_subscription
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d20822121
ℹ️ 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".
| latestPracticeSessionDatachannelUrl !== practiceSessionDatachannelUrl | ||
| ) { | ||
| return undefined; |
There was a problem hiding this comment.
Reconnect with latest practice URL after token refresh
The post-refresh stale check returns when latestPracticeSessionDatachannelUrl !== practiceSessionDatachannelUrl, but this branch does not schedule a follow-up reconnect. If the practice-session URL rotates while ensurePracticeSessionDatachannelToken() is in flight (and no role/practice-status transition happens), this invocation exits and the practice LLM channel can stay disconnected until an unrelated later trigger. Instead of returning here, continue with the latest URL or immediately re-invoke updatePSDataChannel().
Useful? React with 👍 / 👎.
Thank you for your contribution! |
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-763776
by making the following changes
Lobby admission
After being admitted from the lobby, the user must actively request and obtain the session token to connect to backend services.
Practice session promotion
Attendees in a practice session do not receive PS tokens initially; when promoted to panelist they must actively request the PS token to gain panel access.
Reconnect and resubscribe
On reconnect the client reestablishes the LLM connection and may need to resubscribe to previous channel
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.