From deecce6e7799c97dee99da212a665caf63baac4a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Apr 2026 00:35:59 +0000 Subject: [PATCH 1/2] fix: additional WebRTC call hardening - CallActivity.onDestroy: use standalone CoroutineScope instead of lifecycleScope which is cancelled during super.onDestroy(), ensuring hangup/reject signaling events are reliably published - disposePeerSession: remove videoSenders entry for the departing peer to prevent stale RtpSender references leaking after PeerConnection disposal - initiateGroupCall: detect when all PeerConnection creations fail and hang up immediately instead of leaving the call in Offering state until the 60-second timeout https://claude.ai/code/session_017HrFJNxD6zrGwiZ3s69xTh --- .../amethyst/service/call/CallController.kt | 8 ++++++++ .../amethyst/ui/call/CallActivity.kt | 15 +++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt index d0d6b08b7a..0fea96b867 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt @@ -220,6 +220,7 @@ class CallController( callManager.beginOffering(callId, peerPubKeys, callType) + var successCount = 0 for (peerPubKey in peerPubKeys) { try { val webRtcSession = withContext(Dispatchers.IO) { createWebRtcSession(peerPubKey) } @@ -230,10 +231,16 @@ class CallController( callManager.publishOfferToPeer(peerPubKey, peerPubKeys, callType, callId, sdp.description) } } + successCount++ } catch (e: Exception) { Log.e(TAG, "Failed to create PeerConnection for ${peerPubKey.take(8)}", e) } } + if (successCount == 0) { + Log.e(TAG, "All PeerConnection creations failed, hanging up") + _errorMessage.value = "Failed to start call: could not create any connections" + callManager.hangup() + } } } @@ -576,6 +583,7 @@ class CallController( // ---- Per-peer cleanup ---- fun disposePeerSession(peerPubKey: HexKey) { + videoSenders.remove(peerPubKey) val entry = peerSessionMgr.removeSession(peerPubKey) if (entry != null) { try { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt index 152aa6c8ea..a0836fa878 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt @@ -46,7 +46,10 @@ import com.vitorpamplona.amethyst.ui.StringResSetup import com.vitorpamplona.amethyst.ui.screen.ManageRelayServices import com.vitorpamplona.amethyst.ui.screen.ManageWebOkHttp import com.vitorpamplona.amethyst.ui.theme.AmethystTheme +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -213,13 +216,13 @@ class CallActivity : AppCompatActivity() { // Safety net: if the Activity is destroyed while a call is still // ringing/offering, ensure the call is hung up so audio stops. - // Use NonCancellable so the signaling event is published even - // though the lifecycle scope is being cancelled. + // Use a standalone CoroutineScope because lifecycleScope is cancelled + // during super.onDestroy() and may drop suspend work. val manager = CallSessionBridge.callManager when (manager?.state?.value) { is CallState.IncomingCall -> { - lifecycleScope.launch { - withContext(NonCancellable) { manager.rejectCall() } + CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch { + manager.rejectCall() } } @@ -227,8 +230,8 @@ class CallActivity : AppCompatActivity() { is CallState.Connecting, is CallState.Connected, -> { - lifecycleScope.launch { - withContext(NonCancellable) { manager.hangup() } + CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch { + manager.hangup() } } From c82ba766f8f0161f4ab58875c9e43c4c83e556ef Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Apr 2026 00:48:27 +0000 Subject: [PATCH 2/2] fix: WebRTC race conditions, thread safety, and PiP lifecycle - CallActivity.onStop: move finishAndRemoveTask inside coroutine so hangup signaling completes before Activity destruction; add hangupInitiated flag to prevent double-hangup in onDestroy - CallController: add per-peer renegotiation debouncing via pendingRenegotiation map to prevent queuing multiple createOffer calls when video is toggled rapidly - CallController: guard ensureForegroundService in onPeerConnected callback with state check to prevent restarting the service after cleanup - RemoteVideoMonitor: synchronize onRemoteVideoTrack, onPeerRemoved, and dispose with trackLock to prevent non-atomic map mutations and leaked video sinks from concurrent WebRTC callback threads - CallMediaManager: add @Synchronized to createVideoResources to prevent check-then-act race between IO and main threads https://claude.ai/code/session_017HrFJNxD6zrGwiZ3s69xTh --- .../amethyst/service/call/CallController.kt | 20 +++++-- .../amethyst/service/call/CallMediaManager.kt | 1 + .../service/call/RemoteVideoMonitor.kt | 54 +++++++++++-------- .../amethyst/ui/call/CallActivity.kt | 48 +++++++++-------- 4 files changed, 76 insertions(+), 47 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt index 0fea96b867..17c2db79bb 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt @@ -103,6 +103,8 @@ class CallController( private val cleanedUp = AtomicBoolean(false) private val videoSenders = ConcurrentHashMap() + private val pendingRenegotiation = ConcurrentHashMap() + private val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager private var networkCallbackRegistered = false private val networkCallback = @@ -409,10 +411,19 @@ class CallController( } private fun performRenegotiation(peerPubKey: HexKey) { - val webRtcSession = webRtcSession(peerPubKey) ?: return + if (pendingRenegotiation.putIfAbsent(peerPubKey, true) != null) return + val webRtcSession = + webRtcSession(peerPubKey) ?: run { + pendingRenegotiation.remove(peerPubKey) + return + } val state = callManager.state.value - if (state !is CallState.Connected && state !is CallState.Connecting) return + if (state !is CallState.Connected && state !is CallState.Connecting) { + pendingRenegotiation.remove(peerPubKey) + return + } webRtcSession.createOffer { sdp -> + pendingRenegotiation.remove(peerPubKey) scope.launch { callManager.sendRenegotiation(sdp.description, peerPubKey) } } } @@ -540,7 +551,9 @@ class CallController( Log.d(TAG) { "Peer ${peerPubKey.take(8)} connected!" } scope.launch { callManager.onPeerConnected() - ensureForegroundService() + if (callManager.state.value is CallState.Connected) { + ensureForegroundService() + } } }, onRemoteVideoTrack = { track -> videoMonitor.onRemoteVideoTrack(peerPubKey, track) }, @@ -627,6 +640,7 @@ class CallController( _isAudioMuted.value = false videoPausedByProximity = false videoSenders.clear() + pendingRenegotiation.clear() cleanedUp.set(false) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallMediaManager.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallMediaManager.kt index 35a279271f..3b29f79bd8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallMediaManager.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallMediaManager.kt @@ -117,6 +117,7 @@ class CallMediaManager( } } + @Synchronized fun createVideoResources() { if (localVideoSource != null) return val factory = peerConnectionFactory ?: return diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/RemoteVideoMonitor.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/RemoteVideoMonitor.kt index 0a5993b030..d2f44122f4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/RemoteVideoMonitor.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/RemoteVideoMonitor.kt @@ -85,46 +85,56 @@ class RemoteVideoMonitor( private val perPeerLastFrameTimeMs = ConcurrentHashMap() private var groupVideoMonitorJob: Job? = null + /** Protects compound read-modify-write on [_remoteVideoTracks] and + * [_remoteVideoTrack] which can be called from WebRTC callback threads. */ + private val trackLock = Any() + fun onRemoteVideoTrack( peerPubKey: HexKey, track: VideoTrack, ) { Log.d(TAG) { "Remote video track from ${peerPubKey.take(8)}" } - _remoteVideoTracks.value = _remoteVideoTracks.value + (peerPubKey to track) - if (_remoteVideoTrack.value == null) { - _remoteVideoTrack.value = track - startPrimaryMonitor(track) + synchronized(trackLock) { + _remoteVideoTracks.value = _remoteVideoTracks.value + (peerPubKey to track) + if (_remoteVideoTrack.value == null) { + _remoteVideoTrack.value = track + startPrimaryMonitor(track) + } } startPeerMonitor(peerPubKey, track) } fun onPeerRemoved(peerPubKey: HexKey) { stopPeerMonitor(peerPubKey) - val currentTracks = _remoteVideoTracks.value - if (peerPubKey in currentTracks) { - _remoteVideoTracks.value = currentTracks - peerPubKey - if (_remoteVideoTrack.value == currentTracks[peerPubKey]) { - stopPrimaryMonitor() - val nextTrack = _remoteVideoTracks.value.values.firstOrNull() - _remoteVideoTrack.value = nextTrack - if (nextTrack != null) { - startPrimaryMonitor(nextTrack) + synchronized(trackLock) { + val currentTracks = _remoteVideoTracks.value + if (peerPubKey in currentTracks) { + _remoteVideoTracks.value = currentTracks - peerPubKey + if (_remoteVideoTrack.value == currentTracks[peerPubKey]) { + stopPrimaryMonitor() + val nextTrack = _remoteVideoTracks.value.values.firstOrNull() + _remoteVideoTrack.value = nextTrack + if (nextTrack != null) { + startPrimaryMonitor(nextTrack) + } } } } } fun dispose() { - stopPrimaryMonitor() - stopGroupMonitor() - for (peerPubKey in perPeerFrameSinks.keys.toList()) { - stopPeerMonitor(peerPubKey) + synchronized(trackLock) { + stopPrimaryMonitor() + stopGroupMonitor() + for (peerPubKey in perPeerFrameSinks.keys.toList()) { + stopPeerMonitor(peerPubKey) + } + _remoteVideoTrack.value = null + _remoteVideoTracks.value = emptyMap() + _isRemoteVideoActive.value = false + _remoteVideoAspectRatio.value = null + _activePeerVideos.value = emptySet() } - _remoteVideoTrack.value = null - _remoteVideoTracks.value = emptyMap() - _isRemoteVideoActive.value = false - _remoteVideoAspectRatio.value = null - _activePeerVideos.value = emptySet() } private fun startPrimaryMonitor(track: VideoTrack) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt index a0836fa878..bade080219 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/call/CallActivity.kt @@ -48,10 +48,8 @@ import com.vitorpamplona.amethyst.ui.screen.ManageWebOkHttp import com.vitorpamplona.amethyst.ui.theme.AmethystTheme import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext class CallActivity : AppCompatActivity() { val isInPipMode = mutableStateOf(false) @@ -69,6 +67,7 @@ class CallActivity : AppCompatActivity() { } private var pendingAcceptIsVideo = false + private var hangupInitiated = false private val pipActionReceiver = object : BroadcastReceiver() { @@ -201,13 +200,17 @@ class CallActivity : AppCompatActivity() { // We must NOT hang up when the user simply presses Home from the full-screen // call UI (that enters PiP via onUserLeaveHint instead). if (wasInPipMode && !isInPictureInPictureMode) { - val state = CallSessionBridge.callManager?.state?.value + hangupInitiated = true + val manager = CallSessionBridge.callManager + val state = manager?.state?.value if (state is CallState.Connected || state is CallState.Connecting || state is CallState.Offering) { - lifecycleScope.launch { - withContext(NonCancellable) { CallSessionBridge.callManager?.hangup() } + CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch { + manager.hangup() + finishAndRemoveTask() } + } else { + finishAndRemoveTask() } - finishAndRemoveTask() } } @@ -216,26 +219,27 @@ class CallActivity : AppCompatActivity() { // Safety net: if the Activity is destroyed while a call is still // ringing/offering, ensure the call is hung up so audio stops. - // Use a standalone CoroutineScope because lifecycleScope is cancelled - // during super.onDestroy() and may drop suspend work. - val manager = CallSessionBridge.callManager - when (manager?.state?.value) { - is CallState.IncomingCall -> { - CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch { - manager.rejectCall() + // Skip if onStop already initiated the hangup to avoid double signaling. + if (!hangupInitiated) { + val manager = CallSessionBridge.callManager + when (manager?.state?.value) { + is CallState.IncomingCall -> { + CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch { + manager.rejectCall() + } } - } - is CallState.Offering, - is CallState.Connecting, - is CallState.Connected, - -> { - CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch { - manager.hangup() + is CallState.Offering, + is CallState.Connecting, + is CallState.Connected, + -> { + CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch { + manager.hangup() + } } - } - else -> {} + else -> {} + } } super.onDestroy()