Skip to content

Conversation

@pierre-lehnen-rc
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc commented Dec 12, 2025

Proposed changes (including videos or screenshots)

Issue(s)

VGA-106

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes
    • Fixed error handling for SIP integration misconfigurations: the system now detects and logs signaling errors, performs proper cleanup, and fails gracefully. This improves call stability and reliability when SIP settings are incorrect, reducing failed or hanging calls and improving user experience.

✏️ Tip: You can customize this high-level summary in your review settings.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Dec 12, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2025

🦋 Changeset detected

Latest commit: 9f42482

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/media-calls Patch
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

Walkthrough

Adds a changeset entry and adds explicit error handling in createDialog so Drachtio request errors are logged, trigger a server-side hangup with signaling-error, and return early to avoid crashes when SIP transport is misconfigured.

Changes

Cohort / File(s) Summary
Changeset Documentation
.changeset/wet-papayas-buy.md
New patch-release changeset documenting the fix for @rocket.chat/media-calls when SIP integration is misconfigured.
SIP Dialog Error Handling
ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts
In createDialog's cbRequest callback: on error, logs the error, triggers a server-side hangup with a signaling-error, and returns early; request-start logging is moved into the success path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Focused change localized to one callback and a new changeset.
  • Review attention:
    • Confirm hangup call cleans up resources and emits expected signaling state.
    • Verify logging does not leak sensitive SIP payloads.

Possibly related PRs

Suggested reviewers

  • ggazzo
  • sampaiodiego

Poem

🐰 I hopped where SIP waves roam,
Found a bug that broke the home.
I logged the sigh, hung up with care,
No more crashes — peace in the air.
🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main fix: handling unhandled exceptions when Drachtio fails to initiate a request, which aligns with the primary change in OutgoingSipCall.ts.
Linked Issues check ✅ Passed The PR addresses VGA-106 by adding error handling in createDialog to catch Drachtio request failures and hang up the call gracefully instead of crashing VGA-106.
Out of Scope Changes check ✅ Passed All changes are scope-aligned: error handling in OutgoingSipCall.ts and a changeset entry document the fix for Drachtio request failures.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch voip/fix/drachtio-request-error

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between b598e49 and 9f42482.

📒 Files selected for processing (1)
  • ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts
⏰ 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

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pierre-lehnen-rc pierre-lehnen-rc marked this pull request as ready for review December 12, 2025 19:45
@pierre-lehnen-rc pierre-lehnen-rc added this to the 7.14.0 milestone Dec 12, 2025
@pierre-lehnen-rc pierre-lehnen-rc added the stat: QA assured Means it has been tested and approved by a company insider label Dec 12, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 12, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (1)

154-180: Add callId to cbRequest error log and make req parameter optional to match defensive coding pattern.

The req parameter is checked for truthiness at line 164 despite being typed as non-optional SrfRequest. Given the RocketChat patch to drachtio-srf adds error handling to cbRequest with an error parameter, defensive typing (req?: SrfRequest) is appropriate. Additionally, the error log at line 156 should include callId: call._id for consistency with the catch-block error logging at line 185.

-					cbRequest: (err: unknown, req: SrfRequest) => {
+					cbRequest: (err: unknown, req?: SrfRequest) => {
 						if (err) {
 							logger.error({
 								msg: 'OutgoingSipCall.createDialog - request failed',
 								err,
+								callId: call._id,
 							});
 							void mediaCallDirector.hangupByServer(call, 'signaling-error');
 							return;
 						}
🧹 Nitpick comments (1)
.changeset/wet-papayas-buy.md (1)

1-5: Changeset looks consistent with the fix; consider adding a period (and optionally the issue key).
Not required, but it can help release notes scanning.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d3538e7 and b598e49.

📒 Files selected for processing (2)
  • .changeset/wet-papayas-buy.md (1 hunks)
  • ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
🧬 Code graph analysis (1)
ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (2)
ee/packages/media-calls/src/logger.ts (1)
  • logger (3-3)
ee/packages/media-calls/src/server/CallDirector.ts (1)
  • mediaCallDirector (422-422)
⏰ 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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 12, 2025

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.2GiB 1.2GiB +12MiB
rocketchat 358MiB 347MiB +12MiB
omnichannel-transcript-service 132MiB 132MiB +355B
queue-worker-service 132MiB 132MiB -231B
ddp-streamer-service 126MiB 126MiB -1.4KiB
account-service 113MiB 113MiB -635B
stream-hub-service 111MiB 111MiB +100B
presence-service 111MiB 111MiB -188B
authorization-service 111MiB 111MiB -675B

📊 Historical Trend

---
config:
  theme: "dark"
  xyChart:
    width: 900
    height: 400
---
xychart
  title "Image Size Evolution by Service (Last 30 Days + This PR)"
  x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/12 23:51 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
  line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
  line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Loading

Statistics (last 19 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.2GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 1.2GiB
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-37793
  • Baseline: develop
  • Timestamp: 2025-12-12 23:51:43 UTC
  • Historical data points: 19

Updated: Fri, 12 Dec 2025 23:51:43 GMT

@codecov
Copy link

codecov bot commented Dec 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.72%. Comparing base (d3538e7) to head (9f42482).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37793      +/-   ##
===========================================
- Coverage    67.80%   67.72%   -0.09%     
===========================================
  Files         3458     3458              
  Lines       113955   113953       -2     
  Branches     20927    20926       -1     
===========================================
- Hits         77271    77175      -96     
- Misses       34553    34651      +98     
+ Partials      2131     2127       -4     
Flag Coverage Δ
e2e 57.34% <ø> (-0.03%) ⬇️
e2e-api 42.32% <ø> (-0.96%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kodiakhq kodiakhq bot merged commit 8bf0bab into develop Dec 13, 2025
89 of 91 checks passed
@kodiakhq kodiakhq bot deleted the voip/fix/drachtio-request-error branch December 13, 2025 01:16
@dougfabris dougfabris modified the milestones: 7.14.0, 8.0.0 Jan 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants