fix: weak glare protection on webrtc renegotiations #37335
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: d46b081 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
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 |
|
Warning Rate limit exceeded@pierre-lehnen-rc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughIntroduces a Negotiation and NegotiationManager to centralize WebRTC renegotiation, refactors IWebRTCProcessor/Processor APIs and SDP/ICE flows, removes four client-state variants and schema values, updates Call and CallSignalProcessor to use the manager, and bumps package versions with a changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
actor Callee
participant NM as NegotiationManager
participant N as Negotiation
participant WP as WebRTCProcessor
Note over Caller,Callee: Two simultaneous renegotiations
Caller ->> NM: addNegotiation(id-A, remoteOffer?)
Callee ->> NM: addNegotiation(id-B, remoteOffer?)
NM ->> NM: enqueue & decide order (sequence, politeness)
rect rgb(230,245,235)
NM ->> N: process(selected negotiation)
N ->> WP: createOffer() / createAnswer()
WP -->> N: RTCSessionDescriptionInit
N ->> WP: setLocalDescription(sdp)
N ->> WP: waitForIceGathering()
N ->> NM: emit local-sdp (deliver)
end
Note over NM: Other negotiation deferred, retried, or started based on politeness/sequence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/voip-sync-audio-direction #37335 +/- ##
==================================================================
+ Coverage 67.62% 67.82% +0.19%
==================================================================
Files 3345 3345
Lines 113837 114470 +633
Branches 20666 20725 +59
==================================================================
+ Hits 76985 77637 +652
+ Misses 34163 34150 -13
+ Partials 2689 2683 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
force-pushed
the
fix/voip-weak-glare-protection
branch
from
73115f4 to
0adee6c
Compare
pierre-lehnen-rc
changed the title
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.changeset/six-adults-lie.md(1 hunks)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts(1 hunks)packages/media-signaling/src/definition/client.ts(0 hunks)packages/media-signaling/src/definition/services/IServiceProcessor.ts(1 hunks)packages/media-signaling/src/definition/services/index.ts(1 hunks)packages/media-signaling/src/definition/services/negotiation.ts(1 hunks)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts(2 hunks)packages/media-signaling/src/definition/signals/client/local-state.ts(1 hunks)packages/media-signaling/src/lib/Call.ts(13 hunks)packages/media-signaling/src/lib/NegotiationManager.ts(1 hunks)packages/media-signaling/src/lib/services/webrtc/Negotiation.ts(1 hunks)packages/media-signaling/src/lib/services/webrtc/Processor.ts(11 hunks)
💤 Files with no reviewable changes (1)
- packages/media-signaling/src/definition/client.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/media-signaling/src/definition/services/negotiation.ts (2)
packages/media-signaling/src/definition/logger.ts (1)
IMediaSignalLogger(1-6)packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
IClientMediaCall(67-101)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
WebRTCInternalStateMap(7-13)
packages/media-signaling/src/lib/Call.ts (1)
packages/media-signaling/src/lib/NegotiationManager.ts (1)
NegotiationManager(6-287)
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (2)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)packages/models/src/index.ts (1)
MediaCallNegotiations(186-186)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
packages/media-signaling/src/definition/services/IServiceProcessor.ts (1)
ServiceProcessorEvents(14-17)
packages/media-signaling/src/lib/NegotiationManager.ts (3)
packages/media-signaling/src/definition/services/negotiation.ts (3)
NegotiationManagerEvents(4-8)INegotiationCompatibleMediaCall(28-30)NegotiationManagerConfig(10-12)packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (1)
Negotiation(5-163)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
IWebRTCProcessor(21-45)
packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (3)
packages/media-signaling/src/definition/services/negotiation.ts (2)
NegotiationEvents(14-18)NegotiationData(20-26)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
IWebRTCProcessor(21-45)packages/media-signaling/src/definition/logger.ts (1)
IMediaSignalLogger(1-6)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/media-signaling/src/definition/services/IServiceProcessor.ts (1)
19-23: Add type constraint toServiceUniqueEventsgeneric parameter.The previous review comment suggested adding a type constraint to ensure
ServiceUniqueEventscan only be extended with compatible event types. Without the constraint, callers could pass invalid types that aren't compatible with theEmitterintersection.Apply this diff to add the missing constraint:
export interface IServiceProcessor< ServiceStateMap extends DefaultServiceStateMap = DefaultServiceStateMap, - ServiceUniqueEvents = Record<never, never>, + ServiceUniqueEvents extends Record<string, unknown> = Record<never, never>, > { emitter: Emitter<ServiceProcessorEvents<ServiceStateMap> & ServiceUniqueEvents>;
🧹 Nitpick comments (1)
packages/media-signaling/src/lib/NegotiationManager.ts (1)
34-49: Consider consistent initialization for negotiation ID tracking.Lines 45-46 initialize
highestNegotiationIdtonullandhighestKnownNegotiationIdto an empty string. This mixed approach (null vs empty string) could lead to confusion. Consider using a consistent initialization pattern (e.g., bothnull) unless the semantic difference is intentional and documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/media-signaling/src/definition/services/IServiceProcessor.ts(1 hunks)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts(2 hunks)packages/media-signaling/src/lib/NegotiationManager.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/media-signaling/src/lib/NegotiationManager.ts (3)
packages/media-signaling/src/definition/services/negotiation.ts (3)
NegotiationManagerEvents(4-8)INegotiationCompatibleMediaCall(28-30)NegotiationManagerConfig(10-12)packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (1)
Negotiation(5-163)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
IWebRTCProcessor(21-45)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
packages/media-signaling/src/definition/services/IServiceProcessor.ts (2)
ServiceProcessorEvents(14-17)IServiceProcessor(19-26)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
packages/media-signaling/src/lib/NegotiationManager.ts (3)
51-90: LGTM: Negotiation queuing logic implements glare protection.The method correctly handles duplicate negotiations, validates offer types, and implements the glare protection strategy by skipping negotiations with stale sequences. Line 65's assumption that negotiations arrive in order when
negotiationSequenceis not provided is reasonable for the use case.
120-144: LGTM: Processor setup and negotiation processing loop.The WebRTC processor setup correctly wires event listeners, and the
processNegotiationsmethod implements a clean state machine pattern with appropriate guards.
256-264: Fix typos in comments.Lines 257 and 262: "fullfil" should be "fulfill".
Apply this diff:
protected isFulfillingNegotiationQueued(): boolean { - // If we're a polite client, then any queued negotiation is enough to fullfil our negotiation needs + // If we're a polite client, then any queued negotiation is enough to fulfill our negotiation needs if (this.isPoliteClient()) { return this.highestSequence > this.highestProcessedSequence; } // If there's an impolite negotiation queued, that's good enough for any clientLikely an incorrect or invalid review comment.
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (2)
15-19: LGTM: WebRTC-specific event types properly extend service processor events.The new
WebRTCUniqueEventstype correctly defines thenegotiationNeededevent, and the intersection pattern withServiceProcessorEventsaligns with the generic extension mechanism introduced inIServiceProcessor.
21-37: LGTM: API simplification and negotiation-supporting methods.The interface changes correctly:
- Extend
IServiceProcessorwithWebRTCUniqueEventsto expose thenegotiationNeededevent- Simplify
createOfferandcreateAnswerreturn types by removing wrapper objects- Add essential methods (
setLocalDescription,setRemoteDescription,waitForIceGathering,getLocalDescription) that theNegotiationManagerrequires for coordinating SDP exchanges
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/media-signaling/src/lib/NegotiationManager.ts (2)
109-115: Consider adding descriptive message to error log.The error logging at line 112 now correctly uses structured logging (fixed from past review). For improved debugging, consider including a descriptive message and the negotiation ID.
Apply this diff:
try { return this.currentNegotiation.setRemoteAnswer(remoteDescription); } catch (e) { - this.config.logger?.error(e); + this.config.logger?.error('Failed to set remote answer', e, { negotiationId }); this.currentNegotiation = null; this.emitter.emit('error', { errorCode: 'failed-to-set-remote-answer', negotiationId }); }
223-229: Consider adding descriptive message to error log.The error logging at line 226 now correctly uses structured logging (fixed from past review). For improved debugging, consider including a descriptive message.
Apply this diff:
try { return negotiation.process(this.webrtcProcessor); } catch (e) { - this.config.logger?.error(e); + this.config.logger?.error('Failed to process negotiation', e, { negotiationId: negotiation.negotiationId }); this.currentNegotiation = null; this.emitter.emit('error', { errorCode: 'failed-to-process-negotiation', negotiationId: negotiation.negotiationId }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/media-signaling/src/lib/NegotiationManager.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/media-signaling/src/lib/NegotiationManager.ts
🧬 Code graph analysis (1)
packages/media-signaling/src/lib/NegotiationManager.ts (3)
packages/media-signaling/src/definition/services/negotiation.ts (3)
NegotiationManagerEvents(4-8)INegotiationCompatibleMediaCall(28-30)NegotiationManagerConfig(10-12)packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (1)
Negotiation(5-163)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
IWebRTCProcessor(21-45)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/media-signaling/src/lib/NegotiationManager.ts (1)
294-296: Good use of abstract class for type safety.The
WebRTCNegotiationManagerabstract class effectively narrows thewebrtcProcessortype from nullable to required, enabling type-safe assertions in methods likeprocessNegotiation(line 196) andisConfigured(line 232).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/media-signaling/src/lib/NegotiationManager.ts (4)
26-26: Remove unused fieldinputTrackAvailable.The
inputTrackAvailablefield is initialized in the constructor (line 44) but is never read or updated anywhere in the class. The actual input track availability is checked viathis.call.hasInputTrack()at line 244.Apply this diff to remove the unused field:
- protected inputTrackAvailable: boolean; - /** id of the newest negotiation that has reached the processing state */And remove from the constructor:
this.webrtcProcessor = null; - this.inputTrackAvailable = false; this.highestNegotiationId = null;
196-230: Consider adding cleanup for completed negotiations.The negotiation map grows indefinitely as new negotiations are added (line 155), but completed negotiations are never removed. This could lead to memory accumulation over long-lived calls with many renegotiations. Additionally, event listeners attached to each negotiation (lines 203, 213, 218) remain in memory even after the negotiation completes.
Consider adding cleanup logic to remove old negotiations from the map:
negotiation.emitter.on('ended', () => { if (this.currentNegotiation !== negotiation) { return; } this.config.logger?.debug('NegotiationManager.processNegotiation.ended'); this.currentNegotiation = null; // Clean up old negotiations that have been processed and ended for (const [id, neg] of this.negotiations.entries()) { if (neg.ended && neg.sequence < this.highestProcessedSequence) { this.negotiations.delete(id); } } void this.processNegotiations(); });Alternatively, implement a periodic cleanup or limit the map size based on a retention policy.
252-260: Consider using consistent spelling.The comment uses "fulfil" (British spelling), while line 178 uses "fulfilled" (American spelling). For consistency across the codebase, consider standardizing on one variant.
Apply this diff for consistency:
- // If we're a polite client, then any queued negotiation is enough to fulfil our negotiation needs + // If we're a polite client, then any queued negotiation is enough to fulfill our negotiation needs if (this.isPoliteClient()) {
294-296: Unconventional type narrowing pattern.The
WebRTCNegotiationManagerabstract class is used purely for type narrowing through theisConfigured()type guard (line 232) rather than actual inheritance. While this works correctly, it's an unconventional pattern. Consider adding a comment explaining this design choice to aid future maintainers.Add a clarifying comment:
+/** + * Type-narrowing helper: asserts that webrtcProcessor is non-null. + * Used by isConfigured() type guard to enable safe access to processor methods. + */ abstract class WebRTCNegotiationManager extends NegotiationManager { protected abstract webrtcProcessor: IWebRTCProcessor; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/media-signaling/src/lib/NegotiationManager.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/media-signaling/src/lib/NegotiationManager.ts
🧬 Code graph analysis (1)
packages/media-signaling/src/lib/NegotiationManager.ts (3)
packages/media-signaling/src/definition/services/negotiation.ts (3)
NegotiationManagerEvents(4-8)INegotiationCompatibleMediaCall(28-30)NegotiationManagerConfig(10-12)packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (1)
Negotiation(5-163)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
IWebRTCProcessor(21-45)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (12)
packages/media-signaling/src/lib/NegotiationManager.ts (12)
1-11: LGTM!The imports and the
currentNegotiationIdgetter are implemented correctly, with proper fallback logic for when no negotiation is actively being processed.
34-49: LGTM!Constructor properly initializes all state fields, with nullable fields set to
nulland numeric sequences set to0.
51-90: LGTM!The negotiation addition logic correctly implements the polite/impolite negotiation pattern for glare protection. The XOR logic at line 70 properly determines whether a negotiation originates from the polite peer (callee), and negotiations with outdated sequences are correctly skipped.
92-116: LGTM!The remote description handling correctly routes offers to
addNegotiationand validates that answers correspond to the current negotiation. Error handling uses structured logging appropriately.
118-124: Verify single processor initialization or add cleanup.If
setWebRTCProcessoris called multiple times with different processors, the old event listeners will remain attached to the previous processor, potentially causing memory leaks.If the processor can change during the lifecycle, consider removing old listeners before attaching new ones:
public setWebRTCProcessor(webrtcProcessor: IWebRTCProcessor) { this.config.logger?.debug('NegotiationManager.setWebRTCProcessor'); + // Clean up old listeners if processor is being replaced + if (this.webrtcProcessor) { + this.webrtcProcessor.emitter.off('internalError', this.onWebRTCInternalError); + this.webrtcProcessor.emitter.off('negotiationNeeded', this.onWebRTCNegotiationNeeded); + } + this.webrtcProcessor = webrtcProcessor; this.webrtcProcessor.emitter.on('internalError', (event) => this.onWebRTCInternalError(event)); this.webrtcProcessor.emitter.on('negotiationNeeded', () => this.onWebRTCNegotiationNeeded()); }Otherwise, verify and document that this method is only called once during initialization.
126-142: LGTM!The processing logic correctly gates on configuration state and ensures only one negotiation is processed at a time.
144-146: LGTM!The polite client determination correctly maps the callee role to polite behavior in the perfect negotiation pattern.
148-169: LGTM!The queue management correctly tracks sequence numbers and implements the glare protection logic where impolite negotiations preempt polite ones.
171-194: LGTM!The queue selection logic correctly prioritizes negotiations according to the perfect negotiation pattern, ensuring impolite negotiations preempt conflicting polite ones.
232-250: LGTM!The configuration check correctly gates negotiation processing on call state, processor availability, and input track readiness, with proper type narrowing.
262-280: LGTM!The negotiation-needed handler correctly prevents redundant renegotiation requests by checking if a fulfilling negotiation is already queued.
282-291: LGTM!The internal error handler correctly propagates WebRTC processor errors with proper context (negotiation ID when available).
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
VGA-56
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
New Features
Chores