fix(voice): handle multiple transitions simultaneously#11100
fix(voice): handle multiple transitions simultaneously#11100kodiakhq[bot] merged 5 commits intodiscordjs:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11100 +/- ##
==========================================
+ Coverage 32.42% 32.46% +0.03%
==========================================
Files 369 369
Lines 13619 13601 -18
Branches 1069 1066 -3
==========================================
- Hits 4416 4415 -1
+ Misses 9068 9051 -17
Partials 135 135
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/voice/src/networking/DAVESession.ts (5)
211-224:prepareTransition()likely needs a guard against overwriting “better” state from commit/welcome (or vice‑versa).
Right now it unconditionallyset()s, butprocessCommit()/processWelcome()alsoset()the same key with a different value; whichever arrives last wins.Suggested direction: make
prepareTransition()the only place that sets the target protocol version, and have commit/welcome only “mark pending” without clobbering it (see comment onprocessCommit/processWelcomebelow).
278-285: Delete invalidated transition(s) frompendingTransitionsto avoid later false-success executes.
This prevents “stale pending” from being executed after you’ve already asked the voice server to invalidate it.public recoverFromInvalidTransition(transitionId: number) { if (this.reinitializing) return; this.emit('debug', `Invalidating transition ${transitionId}`); this.reinitializing = true; this.consecutiveFailures = 0; + this.pendingTransitions.delete(transitionId); this.emit('invalidateTransition', transitionId); this.reinit(); }
313-324:processCommit()should not overwrite the target protocol version recorded byprepareTransition().
this.pendingTransitions.set(transitionId, this.protocolVersion)will clobberprepareTransition(...protocol_version)ifprepareTransition()arrived first, causingexecuteTransition()to apply the wrong version.At minimum, only set if absent:
- this.pendingTransitions.set(transitionId, this.protocolVersion); + if (!this.pendingTransitions.has(transitionId)) { + this.pendingTransitions.set(transitionId, this.protocolVersion); + }If you actually need both “pending commit/welcome seen” and “target protocol version”, consider changing the map value to an object (e.g.
{ targetProtocolVersion?: number; commitSeen: boolean; }) instead of a single number.
340-351: Same overwrite bug asprocessCommit()forprocessWelcome().
This can clobberprepareTransition()’s target version for the sametransitionId.- this.pendingTransitions.set(transitionId, this.protocolVersion); + if (!this.pendingTransitions.has(transitionId)) { + this.pendingTransitions.set(transitionId, this.protocolVersion); + }
386-397: Verify decrypt-failure policy whenpendingTransitions.size > 0: you now suppress failure counting entirely.
If a transition gets “stuck” (pending forever), you may never recover becauseconsecutiveFailureswon’t advance.If intentional, consider pruning expired pending transitions (timestamped entries) and/or falling back to failure counting after some deadline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/voice/src/networking/DAVESession.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
| /** | ||
| * The pending transition. | ||
| * The pending transitions, mapped by their ID and the protocol version. | ||
| */ | ||
| private pendingTransition?: VoiceDavePrepareTransitionData | undefined; | ||
| private pendingTransitions = new Map<number, number>(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make pendingTransitions reference readonly to prevent accidental reassignment.
Map contents remain mutable, but the field itself shouldn’t be reassigned.
- private pendingTransitions = new Map<number, number>();
+ private readonly pendingTransitions = new Map<number, number>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * The pending transition. | |
| * The pending transitions, mapped by their ID and the protocol version. | |
| */ | |
| private pendingTransition?: VoiceDavePrepareTransitionData | undefined; | |
| private pendingTransitions = new Map<number, number>(); | |
| /** | |
| * The pending transitions, mapped by their ID and the protocol version. | |
| */ | |
| private readonly pendingTransitions = new Map<number, number>(); |
🤖 Prompt for AI Agents
In packages/voice/src/networking/DAVESession.ts around lines 107 to 110, the
class field pendingTransitions is declared mutable and should be marked readonly
to prevent accidental reassignment; change the field declaration to include the
readonly modifier (keeping the Map itself mutable) so only its contents can be
changed, not the reference.
| public executeTransition(transitionId: number) { | ||
| this.emit('debug', `Executing transition (${transitionId})`); | ||
| if (!this.pendingTransition) { | ||
| if (!this.pendingTransitions.has(transitionId)) { | ||
| this.emit('debug', `Received execute transition, but we don't have a pending transition for ${transitionId}`); | ||
| return; | ||
| return false; | ||
| } | ||
|
|
||
| let transitioned = false; | ||
| if (transitionId === this.pendingTransition.transition_id) { | ||
| const oldVersion = this.protocolVersion; | ||
| this.protocolVersion = this.pendingTransition.protocol_version; | ||
|
|
||
| // Handle upgrades & defer downgrades | ||
| if (oldVersion !== this.protocolVersion && this.protocolVersion === 0) { | ||
| this.downgraded = true; | ||
| this.emit('debug', 'Session downgraded'); | ||
| } else if (transitionId > 0 && this.downgraded) { | ||
| this.downgraded = false; | ||
| this.session?.setPassthroughMode(true, TRANSITION_EXPIRY); | ||
| this.emit('debug', 'Session upgraded'); | ||
| } | ||
|
|
||
| // In the future we'd want to signal to the DAVESession to transition also, but it only supports v1 at this time | ||
| transitioned = true; | ||
| this.reinitializing = false; | ||
| this.lastTransitionId = transitionId; | ||
| this.emit('debug', `Transition executed (v${oldVersion} -> v${this.protocolVersion}, id: ${transitionId})`); | ||
| } else { | ||
| this.emit( | ||
| 'debug', | ||
| `Received execute transition for an unexpected transition id (expected: ${this.pendingTransition.transition_id}, actual: ${transitionId})`, | ||
| ); | ||
| const oldVersion = this.protocolVersion; | ||
| this.protocolVersion = this.pendingTransitions.get(transitionId)!; | ||
|
|
||
| // Handle upgrades & defer downgrades | ||
| if (oldVersion !== this.protocolVersion && this.protocolVersion === 0) { | ||
| this.downgraded = true; | ||
| this.emit('debug', 'Session downgraded'); | ||
| } else if (transitionId > 0 && this.downgraded) { | ||
| this.downgraded = false; | ||
| this.session?.setPassthroughMode(true, TRANSITION_EXPIRY); | ||
| this.emit('debug', 'Session upgraded'); | ||
| } | ||
|
|
||
| this.pendingTransition = undefined; | ||
| return transitioned; | ||
| // In the future we'd want to signal to the DAVESession to transition also, but it only supports v1 at this time | ||
| this.reinitializing = false; | ||
| this.lastTransitionId = transitionId; | ||
| this.emit('debug', `Transition executed (v${oldVersion} -> v${this.protocolVersion}, id: ${transitionId})`); | ||
|
|
||
| this.pendingTransitions.delete(transitionId); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Don’t allow executeTransition() to “complete” a transition while reinitializing is true; also type the return explicitly.
As-is, a late executeTransition() can flip this.reinitializing = false and advance lastTransitionId even if you already invalidated that transition.
- public executeTransition(transitionId: number) {
+ public executeTransition(transitionId: number): boolean {
this.emit('debug', `Executing transition (${transitionId})`);
+ if (this.reinitializing) {
+ this.emit('debug', `Ignoring execute transition (${transitionId}) while reinitializing`);
+ return false;
+ }
if (!this.pendingTransitions.has(transitionId)) {
this.emit('debug', `Received execute transition, but we don't have a pending transition for ${transitionId}`);
return false;
}
const oldVersion = this.protocolVersion;
this.protocolVersion = this.pendingTransitions.get(transitionId)!;
// ...
this.pendingTransitions.delete(transitionId);
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public executeTransition(transitionId: number) { | |
| this.emit('debug', `Executing transition (${transitionId})`); | |
| if (!this.pendingTransition) { | |
| if (!this.pendingTransitions.has(transitionId)) { | |
| this.emit('debug', `Received execute transition, but we don't have a pending transition for ${transitionId}`); | |
| return; | |
| return false; | |
| } | |
| let transitioned = false; | |
| if (transitionId === this.pendingTransition.transition_id) { | |
| const oldVersion = this.protocolVersion; | |
| this.protocolVersion = this.pendingTransition.protocol_version; | |
| // Handle upgrades & defer downgrades | |
| if (oldVersion !== this.protocolVersion && this.protocolVersion === 0) { | |
| this.downgraded = true; | |
| this.emit('debug', 'Session downgraded'); | |
| } else if (transitionId > 0 && this.downgraded) { | |
| this.downgraded = false; | |
| this.session?.setPassthroughMode(true, TRANSITION_EXPIRY); | |
| this.emit('debug', 'Session upgraded'); | |
| } | |
| // In the future we'd want to signal to the DAVESession to transition also, but it only supports v1 at this time | |
| transitioned = true; | |
| this.reinitializing = false; | |
| this.lastTransitionId = transitionId; | |
| this.emit('debug', `Transition executed (v${oldVersion} -> v${this.protocolVersion}, id: ${transitionId})`); | |
| } else { | |
| this.emit( | |
| 'debug', | |
| `Received execute transition for an unexpected transition id (expected: ${this.pendingTransition.transition_id}, actual: ${transitionId})`, | |
| ); | |
| const oldVersion = this.protocolVersion; | |
| this.protocolVersion = this.pendingTransitions.get(transitionId)!; | |
| // Handle upgrades & defer downgrades | |
| if (oldVersion !== this.protocolVersion && this.protocolVersion === 0) { | |
| this.downgraded = true; | |
| this.emit('debug', 'Session downgraded'); | |
| } else if (transitionId > 0 && this.downgraded) { | |
| this.downgraded = false; | |
| this.session?.setPassthroughMode(true, TRANSITION_EXPIRY); | |
| this.emit('debug', 'Session upgraded'); | |
| } | |
| this.pendingTransition = undefined; | |
| return transitioned; | |
| // In the future we'd want to signal to the DAVESession to transition also, but it only supports v1 at this time | |
| this.reinitializing = false; | |
| this.lastTransitionId = transitionId; | |
| this.emit('debug', `Transition executed (v${oldVersion} -> v${this.protocolVersion}, id: ${transitionId})`); | |
| this.pendingTransitions.delete(transitionId); | |
| return true; | |
| } | |
| public executeTransition(transitionId: number): boolean { | |
| this.emit('debug', `Executing transition (${transitionId})`); | |
| if (this.reinitializing) { | |
| this.emit('debug', `Ignoring execute transition (${transitionId}) while reinitializing`); | |
| return false; | |
| } | |
| if (!this.pendingTransitions.has(transitionId)) { | |
| this.emit('debug', `Received execute transition, but we don't have a pending transition for ${transitionId}`); | |
| return false; | |
| } | |
| const oldVersion = this.protocolVersion; | |
| this.protocolVersion = this.pendingTransitions.get(transitionId)!; | |
| // Handle upgrades & defer downgrades | |
| if (oldVersion !== this.protocolVersion && this.protocolVersion === 0) { | |
| this.downgraded = true; | |
| this.emit('debug', 'Session downgraded'); | |
| } else if (transitionId > 0 && this.downgraded) { | |
| this.downgraded = false; | |
| this.session?.setPassthroughMode(true, TRANSITION_EXPIRY); | |
| this.emit('debug', 'Session upgraded'); | |
| } | |
| // In the future we'd want to signal to the DAVESession to transition also, but it only supports v1 at this time | |
| this.reinitializing = false; | |
| this.lastTransitionId = transitionId; | |
| this.emit('debug', `Transition executed (v${oldVersion} -> v${this.protocolVersion}, id: ${transitionId})`); | |
| this.pendingTransitions.delete(transitionId); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In packages/voice/src/networking/DAVESession.ts around lines 231 to 258,
executeTransition can still mark a transition complete even while
reinitializing; add an early guard that if this.reinitializing is true you emit
a debug message and return false without modifying lastTransitionId,
reinitializing, downgraded, session state, or deleting the pending transition;
also change the method signature to explicitly return boolean. Ensure the rest
of the method only runs when not reinitializing so late calls do not advance
state.
5 participants
Please describe the changes this PR makes and why it should be merged:
This makes it so multiple transitions can be handled properly, according to the DAVESessionManager sample code. This changes the private property
pendingTransitiontopendingTransitionsthat hold a map of transitions. I don't think that constitutes a breaking change since its a private property.Status and versioning classification: