From a88243f26dab54bd7a476ce4f760158f3ece7c8a Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Wed, 16 Nov 2022 20:52:04 -0600 Subject: [PATCH] Better handle group call ring race conditions --- ts/background.ts | 7 +- ts/services/calling.ts | 104 ++++++++++-------- ts/sql/Interface.ts | 9 +- ts/sql/Server.ts | 86 +++++---------- .../69-group-call-ring-cancellations.ts | 33 ++++++ ts/sql/migrations/index.ts | 2 + ts/state/selectors/calling.ts | 6 + ts/test-node/sql_migrations_test.ts | 32 ++++++ ts/types/Calling.ts | 6 - 9 files changed, 169 insertions(+), 116 deletions(-) create mode 100644 ts/sql/migrations/69-group-call-ring-cancellations.ts diff --git a/ts/background.ts b/ts/background.ts index 6b125a41f111..50d7941e4c25 100644 --- a/ts/background.ts +++ b/ts/background.ts @@ -103,6 +103,7 @@ import { isDirectConversation, isGroupV2 } from './util/whatTypeOfConversation'; import { BackOff, FIBONACCI_TIMEOUTS } from './util/BackOff'; import { AppViewType } from './state/ducks/app'; import type { BadgesStateType } from './state/ducks/badges'; +import { areAnyCallsActiveOrRinging } from './state/selectors/calling'; import { badgeImageFileDownloader } from './badges/badgeImageFileDownloader'; import { actionCreators } from './state/actions'; import { Deletes } from './messageModifiers/Deletes'; @@ -1049,7 +1050,11 @@ export async function startApp(): Promise { window.reduxActions.updates ); window.Signal.Services.calling.initialize( - window.reduxActions.calling, + { + ...window.reduxActions.calling, + areAnyCallsActiveOrRinging: () => + areAnyCallsActiveOrRinging(window.reduxStore.getState()), + }, window.getSfuUrl() ); window.reduxActions.expiration.hydrateExpirationStatus( diff --git a/ts/services/calling.ts b/ts/services/calling.ts index 1b662ceddb1c..8faf2510a253 100644 --- a/ts/services/calling.ts +++ b/ts/services/calling.ts @@ -38,7 +38,7 @@ import { import { uniqBy, noop } from 'lodash'; import type { - ActionsType as UxActionsType, + ActionsType as CallingReduxActionsType, GroupCallParticipantInfoType, GroupCallPeekInfoType, } from '../state/ducks/calling'; @@ -55,7 +55,6 @@ import { CallMode, GroupCallConnectionState, GroupCallJoinState, - ProcessGroupCallRingRequestResult, } from '../types/Calling'; import { AudioDeviceModule, @@ -102,9 +101,9 @@ import * as log from '../logging/log'; import { assertDev } from '../util/assert'; const { - processGroupCallRingRequest, - processGroupCallRingCancelation, - cleanExpiredGroupCallRings, + processGroupCallRingCancellation, + cleanExpiredGroupCallRingCancellations, + wasGroupCallRingPreviouslyCanceled, } = dataInterface; const RINGRTC_HTTP_METHOD_TO_OUR_HTTP_METHOD: Map< @@ -129,6 +128,24 @@ enum GroupCallUpdateMessageState { SentLeft, } +type CallingReduxInterface = Pick< + CallingReduxActionsType, + | 'callStateChange' + | 'cancelIncomingGroupCallRing' + | 'groupCallAudioLevelsChange' + | 'groupCallStateChange' + | 'outgoingCall' + | 'receiveIncomingDirectCall' + | 'receiveIncomingGroupCall' + | 'refreshIODevices' + | 'remoteSharingScreenChange' + | 'remoteVideoChange' + | 'setPresenting' + | 'startCallingLobby' +> & { + areAnyCallsActiveOrRinging(): boolean; +}; + function isScreenSource(source: PresentedSource): boolean { return source.id.startsWith('screen'); } @@ -254,7 +271,7 @@ export class CallingClass { readonly videoRenderer: CanvasVideoRenderer; - private uxActions?: UxActionsType; + private reduxInterface?: CallingReduxInterface; private sfuUrl?: string; @@ -281,9 +298,9 @@ export class CallingClass { this.callsByConversation = {}; } - initialize(uxActions: UxActionsType, sfuUrl: string): void { - this.uxActions = uxActions; - if (!uxActions) { + initialize(reduxInterface: CallingReduxInterface, sfuUrl: string): void { + this.reduxInterface = reduxInterface; + if (!reduxInterface) { throw new Error('CallingClass.initialize: Invalid uxActions.'); } @@ -321,7 +338,7 @@ export class CallingClass { }); ipcRenderer.on('stop-screen-share', () => { - uxActions.setPresenting(); + reduxInterface.setPresenting(); }); ipcRenderer.on('quit', () => { @@ -390,7 +407,7 @@ export class CallingClass { throw missingCaseError(callMode); } - if (!this.uxActions) { + if (!this.reduxInterface) { log.error('Missing uxActions, new call not allowed.'); return; } @@ -492,7 +509,7 @@ export class CallingClass { ): Promise { log.info('CallingClass.startOutgoingDirectCall()'); - if (!this.uxActions) { + if (!this.reduxInterface) { throw new Error('Redux actions not available'); } @@ -544,7 +561,7 @@ export class CallingClass { RingRTC.setVideoRenderer(call.callId, this.videoRenderer); this.attachToCall(conversation, call); - this.uxActions.outgoingCall({ + this.reduxInterface.outgoingCall({ conversationId: conversation.id, hasLocalAudio, hasLocalVideo, @@ -715,7 +732,7 @@ export class CallingClass { } const localAudioLevel = groupCall.getLocalDeviceState().audioLevel; - this.uxActions?.groupCallAudioLevelsChange({ + this.reduxInterface?.groupCallAudioLevelsChange({ conversationId, localAudioLevel, remoteDeviceStates, @@ -985,7 +1002,7 @@ export class CallingClass { conversationId: string, groupCall: GroupCall ): void { - this.uxActions?.groupCallStateChange({ + this.reduxInterface?.groupCallStateChange({ conversationId, ...this.formatGroupCallForRedux(groupCall), }); @@ -1247,8 +1264,8 @@ export class CallingClass { icon: 'images/icons/v2/video-solid-24.svg', message: window.i18n('calling__presenting--notification-body'), onNotificationClick: () => { - if (this.uxActions) { - this.uxActions.setPresenting(); + if (this.reduxInterface) { + this.reduxInterface.setPresenting(); } }, silent: true, @@ -1365,7 +1382,7 @@ export class CallingClass { await this.selectPreferredDevices(newSettings); this.lastMediaDeviceSettings = newSettings; - this.uxActions?.refreshIODevices(newSettings); + this.reduxInterface?.refreshIODevices(newSettings); } } @@ -1703,34 +1720,31 @@ export class CallingClass { let shouldRing = false; if (update === RingUpdate.Requested) { - const processResult = await processGroupCallRingRequest(ringId); - switch (processResult) { - case ProcessGroupCallRingRequestResult.ShouldRing: - shouldRing = true; - break; - case ProcessGroupCallRingRequestResult.RingWasPreviouslyCanceled: - RingRTC.cancelGroupRing(groupIdBytes, ringId, null); - break; - case ProcessGroupCallRingRequestResult.ThereIsAnotherActiveRing: - RingRTC.cancelGroupRing(groupIdBytes, ringId, RingCancelReason.Busy); - break; - default: - throw missingCaseError(processResult); + if (await wasGroupCallRingPreviouslyCanceled(ringId)) { + RingRTC.cancelGroupRing(groupIdBytes, ringId, null); + } else if (this.areAnyCallsActiveOrRinging()) { + RingRTC.cancelGroupRing(groupIdBytes, ringId, RingCancelReason.Busy); + } else if (window.Events.getIncomingCallNotification()) { + shouldRing = true; + } else { + log.info( + 'Incoming calls are disabled. Ignoring group call ring request' + ); } } else { - await processGroupCallRingCancelation(ringId); + await processGroupCallRingCancellation(ringId); } if (shouldRing) { log.info('handleGroupCallRingUpdate: ringing'); - this.uxActions?.receiveIncomingGroupCall({ + this.reduxInterface?.receiveIncomingGroupCall({ conversationId, ringId, ringerUuid, }); } else { log.info('handleGroupCallRingUpdate: canceling any existing ring'); - this.uxActions?.cancelIncomingGroupCallRing({ + this.reduxInterface?.cancelIncomingGroupCallRing({ conversationId, ringId, }); @@ -1782,7 +1796,7 @@ export class CallingClass { private async handleIncomingCall(call: Call): Promise { log.info('CallingClass.handleIncomingCall()'); - if (!this.uxActions || !this.localDeviceId) { + if (!this.reduxInterface || !this.localDeviceId) { log.error('Missing required objects, ignoring incoming call.'); return null; } @@ -1815,7 +1829,7 @@ export class CallingClass { this.attachToCall(conversation, call); - this.uxActions.receiveIncomingDirectCall({ + this.reduxInterface.receiveIncomingDirectCall({ conversationId: conversation.id, isVideoCall: call.isVideoCall, }); @@ -1866,8 +1880,8 @@ export class CallingClass { private attachToCall(conversation: ConversationModel, call: Call): void { this.callsByConversation[conversation.id] = call; - const { uxActions } = this; - if (!uxActions) { + const { reduxInterface } = this; + if (!reduxInterface) { return; } @@ -1883,7 +1897,7 @@ export class CallingClass { this.lastMediaDeviceSettings = undefined; delete this.callsByConversation[conversation.id]; } - uxActions.callStateChange({ + reduxInterface.callStateChange({ conversationId: conversation.id, acceptedTime, callState: call.state, @@ -1896,7 +1910,7 @@ export class CallingClass { // eslint-disable-next-line no-param-reassign call.handleRemoteVideoEnabled = () => { - uxActions.remoteVideoChange({ + reduxInterface.remoteVideoChange({ conversationId: conversation.id, hasVideo: call.remoteVideoEnabled, }); @@ -1904,7 +1918,7 @@ export class CallingClass { // eslint-disable-next-line no-param-reassign call.handleRemoteSharingScreen = () => { - uxActions.remoteSharingScreenChange({ + reduxInterface.remoteSharingScreenChange({ conversationId: conversation.id, isSharingScreen: Boolean(call.remoteSharingScreen), }); @@ -2189,7 +2203,7 @@ export class CallingClass { icon: 'images/icons/v2/video-solid-24.svg', message: notificationMessage, onNotificationClick: () => { - this.uxActions?.startCallingLobby({ + this.reduxInterface?.startCallingLobby({ conversationId: conversation.id, isVideoCall: true, }); @@ -2199,9 +2213,13 @@ export class CallingClass { }); } + private areAnyCallsActiveOrRinging(): boolean { + return this.reduxInterface?.areAnyCallsActiveOrRinging() ?? false; + } + private async cleanExpiredGroupCallRingsAndLoop(): Promise { try { - await cleanExpiredGroupCallRings(); + await cleanExpiredGroupCallRingCancellations(); } catch (err: unknown) { // These errors are ignored here. They should be logged elsewhere and it's okay if // we don't do a cleanup this time. diff --git a/ts/sql/Interface.ts b/ts/sql/Interface.ts index d2e4b29e1e65..0e0a67e22f80 100644 --- a/ts/sql/Interface.ts +++ b/ts/sql/Interface.ts @@ -10,7 +10,6 @@ import type { import type { StoredJob } from '../jobs/types'; import type { ReactionType } from '../types/Reactions'; import type { ConversationColorType, CustomColorType } from '../types/Colors'; -import type { ProcessGroupCallRingRequestResult } from '../types/Calling'; import type { StorageAccessType } from '../types/Storage.d'; import type { AttachmentType } from '../types/Attachment'; import type { BodyRangesType, BytesToStrings } from '../types/Util'; @@ -689,11 +688,9 @@ export type DataInterface = { insertJob(job: Readonly): Promise; deleteJob(id: string): Promise; - processGroupCallRingRequest( - ringId: bigint - ): Promise; - processGroupCallRingCancelation(ringId: bigint): Promise; - cleanExpiredGroupCallRings(): Promise; + wasGroupCallRingPreviouslyCanceled(ringId: bigint): Promise; + processGroupCallRingCancellation(ringId: bigint): Promise; + cleanExpiredGroupCallRingCancellations(): Promise; getMaxMessageCounter(): Promise; diff --git a/ts/sql/Server.ts b/ts/sql/Server.ts index 5b928d66302f..2b37c28554a2 100644 --- a/ts/sql/Server.ts +++ b/ts/sql/Server.ts @@ -46,7 +46,6 @@ import { parseIntOrThrow } from '../util/parseIntOrThrow'; import * as durations from '../util/durations'; import { formatCountForLogging } from '../logging/formatCountForLogging'; import type { ConversationColorType, CustomColorType } from '../types/Colors'; -import { ProcessGroupCallRingRequestResult } from '../types/Calling'; import { RemoveAllConfiguration } from '../types/RemoveAllConfiguration'; import type { BadgeType, BadgeImageType } from '../badges/types'; import { parseBadgeCategory } from '../badges/BadgeCategory'; @@ -333,9 +332,9 @@ const dataInterface: ServerInterface = { insertJob, deleteJob, - processGroupCallRingRequest, - processGroupCallRingCancelation, - cleanExpiredGroupCallRings, + wasGroupCallRingPreviouslyCanceled, + processGroupCallRingCancellation, + cleanExpiredGroupCallRingCancellations, getMaxMessageCounter, @@ -4819,7 +4818,7 @@ async function removeAll(): Promise { DELETE FROM badges; DELETE FROM conversations; DELETE FROM emojis; - DELETE FROM groupCallRings; + DELETE FROM groupCallRingCancellations; DELETE FROM identityKeys; DELETE FROM items; DELETE FROM jobs; @@ -5425,69 +5424,36 @@ async function deleteJob(id: string): Promise { db.prepare('DELETE FROM jobs WHERE id = $id').run({ id }); } -async function processGroupCallRingRequest( +async function wasGroupCallRingPreviouslyCanceled( ringId: bigint -): Promise { +): Promise { const db = getInstance(); - return db.transaction(() => { - let result: ProcessGroupCallRingRequestResult; - - const wasRingPreviouslyCanceled = Boolean( - db - .prepare( - ` - SELECT 1 FROM groupCallRings - WHERE ringId = $ringId AND isActive = 0 - LIMIT 1; - ` - ) - .pluck(true) - .get({ ringId }) - ); - - if (wasRingPreviouslyCanceled) { - result = ProcessGroupCallRingRequestResult.RingWasPreviouslyCanceled; - } else { - const isThereAnotherActiveRing = Boolean( - db - .prepare( - ` - SELECT 1 FROM groupCallRings - WHERE isActive = 1 - LIMIT 1; - ` - ) - .pluck(true) - .get() + return db + .prepare( + ` + SELECT EXISTS ( + SELECT 1 FROM groupCallRingCancellations + WHERE ringId = $ringId + AND createdAt >= $ringsOlderThanThisAreIgnored ); - if (isThereAnotherActiveRing) { - result = ProcessGroupCallRingRequestResult.ThereIsAnotherActiveRing; - } else { - result = ProcessGroupCallRingRequestResult.ShouldRing; - } - - db.prepare( - ` - INSERT OR IGNORE INTO groupCallRings (ringId, isActive, createdAt) - VALUES ($ringId, 1, $createdAt); - ` - ); - } - - return result; - })(); + ` + ) + .pluck() + .get({ + ringId, + ringsOlderThanThisAreIgnored: Date.now() - MAX_GROUP_CALL_RING_AGE, + }); } -async function processGroupCallRingCancelation(ringId: bigint): Promise { +async function processGroupCallRingCancellation(ringId: bigint): Promise { const db = getInstance(); db.prepare( ` - INSERT INTO groupCallRings (ringId, isActive, createdAt) - VALUES ($ringId, 0, $createdAt) - ON CONFLICT (ringId) DO - UPDATE SET isActive = 0; + INSERT INTO groupCallRingCancellations (ringId, createdAt) + VALUES ($ringId, $createdAt) + ON CONFLICT (ringId) DO NOTHING; ` ).run({ ringId, createdAt: Date.now() }); } @@ -5496,12 +5462,12 @@ async function processGroupCallRingCancelation(ringId: bigint): Promise { // that, it doesn't really matter what the value is. const MAX_GROUP_CALL_RING_AGE = 30 * durations.MINUTE; -async function cleanExpiredGroupCallRings(): Promise { +async function cleanExpiredGroupCallRingCancellations(): Promise { const db = getInstance(); db.prepare( ` - DELETE FROM groupCallRings + DELETE FROM groupCallRingCancellations WHERE createdAt < $expiredRingTime; ` ).run({ diff --git a/ts/sql/migrations/69-group-call-ring-cancellations.ts b/ts/sql/migrations/69-group-call-ring-cancellations.ts new file mode 100644 index 000000000000..1a283002c833 --- /dev/null +++ b/ts/sql/migrations/69-group-call-ring-cancellations.ts @@ -0,0 +1,33 @@ +// Copyright 2022 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import type { Database } from 'better-sqlite3'; + +import type { LoggerType } from '../../types/Logging'; + +export default function updateToSchemaVersion69( + currentVersion: number, + db: Database, + logger: LoggerType +): void { + if (currentVersion >= 69) { + return; + } + + db.transaction(() => { + db.exec( + ` + DROP TABLE IF EXISTS groupCallRings; + + CREATE TABLE groupCallRingCancellations( + ringId INTEGER PRIMARY KEY, + createdAt INTEGER NOT NULL + ); + ` + ); + + db.pragma('user_version = 69'); + })(); + + logger.info('updateToSchemaVersion69: success!'); +} diff --git a/ts/sql/migrations/index.ts b/ts/sql/migrations/index.ts index de2ba1ecbc02..4f8fa8318f3c 100644 --- a/ts/sql/migrations/index.ts +++ b/ts/sql/migrations/index.ts @@ -44,6 +44,7 @@ import updateToSchemaVersion65 from './65-add-storage-id-to-stickers'; import updateToSchemaVersion66 from './66-add-pni-signature-to-sent-protos'; import updateToSchemaVersion67 from './67-add-story-to-unprocessed'; import updateToSchemaVersion68 from './68-drop-deprecated-columns'; +import updateToSchemaVersion69 from './69-group-call-ring-cancellations'; function updateToSchemaVersion1( currentVersion: number, @@ -1950,6 +1951,7 @@ export const SCHEMA_VERSIONS = [ updateToSchemaVersion66, updateToSchemaVersion67, updateToSchemaVersion68, + updateToSchemaVersion69, ]; export function updateSchema(db: Database, logger: LoggerType): void { diff --git a/ts/state/selectors/calling.ts b/ts/state/selectors/calling.ts index f9b28b88d0d7..73bbaa01b226 100644 --- a/ts/state/selectors/calling.ts +++ b/ts/state/selectors/calling.ts @@ -80,6 +80,12 @@ export const getIncomingCall = createSelector( } ); +export const areAnyCallsActiveOrRinging = createSelector( + getActiveCall, + getIncomingCall, + (activeCall, incomingCall): boolean => Boolean(activeCall || incomingCall) +); + export const isInSpeakerView = ( call: Pick | undefined ): boolean => { diff --git a/ts/test-node/sql_migrations_test.ts b/ts/test-node/sql_migrations_test.ts index 737f062a82d8..e2dcfed9d12d 100644 --- a/ts/test-node/sql_migrations_test.ts +++ b/ts/test-node/sql_migrations_test.ts @@ -2390,4 +2390,36 @@ describe('SQL migrations test', () => { ); }); }); + + describe('updateToSchemaVersion69', () => { + beforeEach(() => { + updateToVersion(69); + }); + + it('removes the legacy groupCallRings table', () => { + const tableCount = db + .prepare( + ` + SELECT COUNT(*) FROM sqlite_schema + WHERE type = "table" + AND name = "groupCallRings" + ` + ) + .pluck(); + + assert.strictEqual(tableCount.get(), 0); + }); + + it('adds the groupCallRingCancellations table', () => { + assert.doesNotThrow(() => { + db.exec( + ` + INSERT INTO groupCallRingCancellations + (ringId, createdAt) + VALUES (1, 2); + ` + ); + }); + }); + }); }); diff --git a/ts/types/Calling.ts b/ts/types/Calling.ts index 7a7580449f78..1b3d778400ae 100644 --- a/ts/types/Calling.ts +++ b/ts/types/Calling.ts @@ -196,9 +196,3 @@ export type ChangeIODevicePayloadType = | { type: CallingDeviceType.CAMERA; selectedDevice: string } | { type: CallingDeviceType.MICROPHONE; selectedDevice: AudioDevice } | { type: CallingDeviceType.SPEAKER; selectedDevice: AudioDevice }; - -export enum ProcessGroupCallRingRequestResult { - ShouldRing, - RingWasPreviouslyCanceled, - ThereIsAnotherActiveRing, -}