Better handle group call ring race conditions

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<void> {
         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(

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,
+  processGroupCallRingCancellation,
-  processGroupCallRingCancelation,
+  cleanExpiredGroupCallRingCancellations,
-  cleanExpiredGroupCallRings,
+  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 {
+  initialize(reduxInterface: CallingReduxInterface, sfuUrl: string): void {
-    this.uxActions = uxActions;
+    this.reduxInterface = reduxInterface;
-    if (!uxActions) {
+    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<void> {
     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) {
+          if (this.reduxInterface) {
-            this.uxActions.setPresenting();
+            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);
+      if (await wasGroupCallRingPreviouslyCanceled(ringId)) {
-      switch (processResult) {
+        RingRTC.cancelGroupRing(groupIdBytes, ringId, null);
-        case ProcessGroupCallRingRequestResult.ShouldRing:
+      } else if (this.areAnyCallsActiveOrRinging()) {
-          shouldRing = true;
+        RingRTC.cancelGroupRing(groupIdBytes, ringId, RingCancelReason.Busy);
-          break;
+      } else if (window.Events.getIncomingCallNotification()) {
-        case ProcessGroupCallRingRequestResult.RingWasPreviouslyCanceled:
+        shouldRing = true;
-          RingRTC.cancelGroupRing(groupIdBytes, ringId, null);
+      } else {
-          break;
+        log.info(
-        case ProcessGroupCallRingRequestResult.ThereIsAnotherActiveRing:
+          'Incoming calls are disabled. Ignoring group call ring request'
-          RingRTC.cancelGroupRing(groupIdBytes, ringId, RingCancelReason.Busy);
+        );
-          break;
-        default:
-          throw missingCaseError(processResult);
       }
     } 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<CallSettings | null> {
     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;
+    const { reduxInterface } = this;
-    if (!uxActions) {
+    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<void> {
     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.

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<StoredJob>): Promise<void>;
   deleteJob(id: string): Promise<void>;
 
-  processGroupCallRingRequest(
+  wasGroupCallRingPreviouslyCanceled(ringId: bigint): Promise<boolean>;
-    ringId: bigint
+  processGroupCallRingCancellation(ringId: bigint): Promise<void>;
-  ): Promise<ProcessGroupCallRingRequestResult>;
+  cleanExpiredGroupCallRingCancellations(): Promise<void>;
-  processGroupCallRingCancelation(ringId: bigint): Promise<void>;
-  cleanExpiredGroupCallRings(): Promise<void>;
 
   getMaxMessageCounter(): Promise<number | undefined>;
 

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,
+  wasGroupCallRingPreviouslyCanceled,
-  processGroupCallRingCancelation,
+  processGroupCallRingCancellation,
-  cleanExpiredGroupCallRings,
+  cleanExpiredGroupCallRingCancellations,
 
   getMaxMessageCounter,
 
@@ -4819,7 +4818,7 @@ async function removeAll(): Promise<void> {
       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<void> {
   db.prepare<Query>('DELETE FROM jobs WHERE id = $id').run({ id });
 }
 
-async function processGroupCallRingRequest(
+async function wasGroupCallRingPreviouslyCanceled(
   ringId: bigint
-): Promise<ProcessGroupCallRingRequestResult> {
+): Promise<boolean> {
   const db = getInstance();
 
-  return db.transaction(() => {
+  return db
-    let result: ProcessGroupCallRingRequestResult;
+    .prepare<Query>(
-
+      `
-    const wasRingPreviouslyCanceled = Boolean(
+      SELECT EXISTS (
-      db
+        SELECT 1 FROM groupCallRingCancellations
-        .prepare<Query>(
+        WHERE ringId = $ringId
-          `
+        AND createdAt >= $ringsOlderThanThisAreIgnored
-          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<EmptyQuery>(
-            `
-            SELECT 1 FROM groupCallRings
-            WHERE isActive = 1
-            LIMIT 1;
-            `
-          )
-          .pluck(true)
-          .get()
       );
-      if (isThereAnotherActiveRing) {
+      `
-        result = ProcessGroupCallRingRequestResult.ThereIsAnotherActiveRing;
+    )
-      } else {
+    .pluck()
-        result = ProcessGroupCallRingRequestResult.ShouldRing;
+    .get({
-      }
+      ringId,
-
+      ringsOlderThanThisAreIgnored: Date.now() - MAX_GROUP_CALL_RING_AGE,
-      db.prepare<Query>(
+    });
-        `
-        INSERT OR IGNORE INTO groupCallRings (ringId, isActive, createdAt)
-        VALUES ($ringId, 1, $createdAt);
-        `
-      );
-    }
-
-    return result;
-  })();
 }
 
-async function processGroupCallRingCancelation(ringId: bigint): Promise<void> {
+async function processGroupCallRingCancellation(ringId: bigint): Promise<void> {
   const db = getInstance();
 
   db.prepare<Query>(
     `
-    INSERT INTO groupCallRings (ringId, isActive, createdAt)
+    INSERT INTO groupCallRingCancellations (ringId, createdAt)
-    VALUES ($ringId, 0, $createdAt)
+    VALUES ($ringId, $createdAt)
-    ON CONFLICT (ringId) DO
+    ON CONFLICT (ringId) DO NOTHING;
-    UPDATE SET isActive = 0;
     `
   ).run({ ringId, createdAt: Date.now() });
 }
@@ -5496,12 +5462,12 @@ async function processGroupCallRingCancelation(ringId: bigint): Promise<void> {
 //   that, it doesn't really matter what the value is.
 const MAX_GROUP_CALL_RING_AGE = 30 * durations.MINUTE;
 
-async function cleanExpiredGroupCallRings(): Promise<void> {
+async function cleanExpiredGroupCallRingCancellations(): Promise<void> {
   const db = getInstance();
 
   db.prepare<Query>(
     `
-    DELETE FROM groupCallRings
+    DELETE FROM groupCallRingCancellations
     WHERE createdAt < $expiredRingTime;
     `
   ).run({

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!');
+}

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 {

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<ActiveCallStateType, 'viewMode'> | undefined
 ): boolean => {

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);
+          `
+        );
+      });
+    });
+  });
 });

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,
-}