fix(5k webinar):prevent LLM connection from being permanently killed#4878
fix(5k webinar):prevent LLM connection from being permanently killed#4878mickelr wants to merge 2 commits intowebex:nextfrom
Conversation
mickelr
changed the title
![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: 8ff06e9c65
ℹ️ 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 prevents permanently killing the LLM when: | ||
| // 1. An unjoined meeting (e.g. incoming CALL) triggers updateLLMConnection | ||
| // 2. datachannelUrl becomes null/undefined due to a locus.difference event | ||
| if (!isJoined || !url || !dataChannelUrl) { |
There was a problem hiding this comment.
The new !isJoined early-return prevents cleanupLLMConneciton() from running even when this meeting already owns an active LLM connection. updateLLMConnection() is called from the SELF_UNADMITTED_GUEST flow, so when a user transitions from JOINED to unadmitted/lobby, this branch now leaves the socket plus relay/locus listeners active instead of tearing them down. In that state, the meeting can continue receiving/processing LLM events for an unjoined participant until some later explicit cleanup path runs.
Useful? React with 👍 / 👎.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c78bed7541
ℹ️ 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".
| // Guard 1: Don't disconnect if this meeting doesn't own the LLM connection. | ||
| // This prevents an unjoined meeting (e.g. incoming CALL) from killing another | ||
| // meeting's active LLM connection. | ||
| if (!isJoined && llmLocusUrl && llmLocusUrl !== url) { |
There was a problem hiding this comment.
This guard treats llmLocusUrl !== url as proof that another meeting owns the LLM connection, but the same meeting can legitimately change locus URL during locus replacement flows (locus.replaces handling updates the meeting’s locus URL). In that case, if updateLLMConnection() runs while isJoined is false (for example from the SELF_UNADMITTED_GUEST path), this branch returns early and skips cleanupLLMConneciton(), leaving stale LLM socket/listener state active even though the participant is no longer joined.
Useful? React with 👍 / 👎.
mickelr
force-pushed
the
fix/llm-lost
branch
2 times, most recently
from
ef464c6 to
34307ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34307ab105
ℹ️ 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 (!isJoined && llmLocusUrl && llmLocusUrl !== url) { | ||
| LoggerProxy.logger.info( | ||
| 'Meeting:index#updateLLMConnection --> Not disconnecting LLM: owned by different meeting' | ||
| ); | ||
|
|
||
| return undefined; |
There was a problem hiding this comment.
The new !isJoined && llmLocusUrl !== url guard can misclassify ownership after locus replacement: setUpLocusUrlListener updates the meeting’s locus URL, but the active LLM connection may still carry the previous locus until a successful reconnect, so when SELF_UNADMITTED_GUEST calls updateLLMConnection() this branch can return early for the same meeting and skip cleanupLLMConneciton(), leaving LLM socket/listener state active while the user is no longer joined. Fresh evidence in this revision is the added unit test that enforces this early-return behavior for unjoined meetings without covering the same-meeting locus-replacement case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e834899784
ℹ️ 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".
| handleDataChannelUrlChange(datachannelUrl) { | ||
| // @ts-ignore - config coming from registerPlugin | ||
| if (datachannelUrl && this.config.enableAutomaticLLM) { | ||
| if (datachannelUrl && this.config.enableAutomaticLLM && this.isJoined()) { |
There was a problem hiding this comment.
Placing this.isJoined() in the outer handleDataChannelUrlChange guard now blocks this.webinar.updatePSDataChannel() whenever the meeting transitions to a non-joined state (for example lobby/unadmitted). That is a regression because updatePSDataChannel() is also the cleanup path: it explicitly calls cleanupPSDataChannel() when meeting.isJoined() is false, so skipping this call can leave the practice-session LLM channel/listeners active for an unjoined participant until a later full meeting cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think unjoined participant has chance to connect PS data channel
1 participant
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
updateLLMConnection() disconnects the shared LLM singleton (code 3050, permanent) before validating whether reconnection is feasible. This causes permanent LLM loss in two scenarios:
Unjoined meeting triggers disconnect - An incoming CALL creates a new meeting object that calls updateLLMConnection(). Since isJoined() is false on the new meeting, it disconnects the webinar LLM but never reconnects.
datachannelUrl cleared by locus.difference - A locus delta clears datachannelUrl. The method disconnects the LLM, then registerAndConnect() silently bails out because datachannelUrl is null.
by making the following changes
Add a guard before cleanupLLMConneciton() that validates reconnection is feasible (isJoined, url, and dataChannelUrl must all be truthy). If reconnection is not possible, the existing LLM connection is preserved.
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.