fix(voip): defer foreground service start until call is active on Android#7283
Conversation
…roid On Android 14, starting FOREGROUND_SERVICE_TYPE_PHONE_CALL before the Telecom connection is active throws ForegroundServiceStartNotAllowedException and kills the call. Move the startService call to after answerIncomingCall() succeeds. Also harden VoipCallService.onStartCommand to enter foreground state before branching on intent action, preventing ForegroundServiceDidNotStartInTimeException on sticky restarts and unexpected intent redelivery.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
WalkthroughThe changes modify the timing of VoIP service foreground initialization. VoipCallService now transitions to foreground immediately on command execution, while VoipNotification defers service startup until after the REST call succeeds and the Telecom connection is marked active. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 4/8 reviews remaining, refill in 26 minutes and 18 seconds.Comment |
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
274-303:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGuard the delayed service start against teardown races.
finish(true)always startsVoipCallService, butanswerIncomingCall(...)does not prove the call is still live. If the DDP hangup/other-device path tears the call down before this callback runs, the later success path can still resurrect the foreground service for an already-ended call.Please gate the service start on a real post-answer liveness result, or thread a success/failure signal back from
answerIncomingCall(...)so teardown can win cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt` around lines 274 - 303, The finish(...) path currently unconditionally calls VoipCallService.startService after answerIncomingCall, which can resurrect a service for a call already torn down; modify the flow so starting the foreground service is gated by a real post-answer liveness check: either make answerIncomingCall(payload.callId) return a boolean/Result indicating the call is actually connected (or throw) and only call VoipCallService.startService when that result is success, or call a new ddpRegistry/isCallActive(payload.callId) (or similar) immediately after answerIncomingCall and only start the service if that check returns true; ensure the failure branch still runs disconnectIncomingCall and storeAcceptFailureForJs when liveness is false so teardown wins cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 274-303: The finish(...) path currently unconditionally calls
VoipCallService.startService after answerIncomingCall, which can resurrect a
service for a call already torn down; modify the flow so starting the foreground
service is gated by a real post-answer liveness check: either make
answerIncomingCall(payload.callId) return a boolean/Result indicating the call
is actually connected (or throw) and only call VoipCallService.startService when
that result is success, or call a new ddpRegistry/isCallActive(payload.callId)
(or similar) immediately after answerIncomingCall and only start the service if
that check returns true; ensure the failure branch still runs
disconnectIncomingCall and storeAcceptFailureForJs when liveness is false so
teardown wins cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c5eb3f0-a1c0-4f53-83c7-432a935c6ceb
📒 Files selected for processing (2)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
📜 Review details
⏰ 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: ESLint and Test / run-eslint-and-test
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
📚 Learning: 2026-04-22T22:57:58.545Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt
🔇 Additional comments (2)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt (1)
66-95: Looks good.Moving
startForegroundWithNotification(...)ahead of the action branch addresses the Android 14 timing issue without changing the stop paths’ behavior.android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
262-267: Good deferral point.Starting the foreground service here would reintroduce the Android 14 ordering problem, so keeping it out of this pre-finish block is the right move.
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Failure
|
Android Build Available Rocket.Chat Experimental 4.72.0.108720 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTaxDmMOsZz3MPi3o7O8gBbojvORr88KSCRhltnLwFdC37GKsi4AEKuiUe5Zk7loI3wQSmCqt9LsXkMJwAv |
1 participant
Proposed changes
On Android 14,
FOREGROUND_SERVICE_TYPE_PHONE_CALLmay only be started after the Telecom connection has transitioned to the active state. The previous code startedVoipCallServiceimmediately when the user tapped Accept — before the server answer REST call completed and beforeconnection.onAnswer()was called — causingForegroundServiceStartNotAllowedExceptionon Android 14 and silently killing the call.A second issue:
VoipCallService.onStartCommandonly calledstartForegroundinside theACTION_STARTbranch. TheACTION_STOPand unknown-action branches calledstopSelfwithout entering foreground state, so the OS five-second rule firedForegroundServiceDidNotStartInTimeExceptionon sticky restarts and unexpected intent redelivery.Issue(s)
Part of PR #6918 ship-blocking fixes — blockers B2 and B3.
How to test or reproduce
Happy path (B2):
ForegroundServiceStartNotAllowedExceptionappears in Logcat.Failure path (B2):
Unknown-action / sticky restart (B3):
VoipCallServicewas running.ForegroundServiceDidNotStartInTimeExceptionin Logcat.Screenshots
N/A — notification behaviour change only.
Types of changes
Checklist
Further comments
B2 — accept flow ordering:
VoipCallService.startServiceis now called inside thefinish(true)branch ofhandleAcceptAction, immediately afteranswerIncomingCall()transitions the Telecom connection to active. If the REST answer fails or times out, the service is never started and the existing tear-down path runs unchanged.B3 — foreground-first command handler:
onStartCommandnow callsstartForegroundWithNotificationunconditionally at the top of the method before thewhenbranch. TheACTION_STOPand unknown-action branches still callstopSelf; the brief foreground promotion followed bystopSelfis the correct pattern Android documents for this scenario.Accept-tap-to-foreground-notification latency: ~200–800 ms on a healthy connection (dominated by the server round-trip for the
media-calls.answerREST call). This is within the target range specified in the acceptance criteria.Summary by CodeRabbit
Bug Fixes