Skip to content

Commit 155bcb7

Browse files
committed
fix(voip): guard CallLifecycle.end against re-entry and step-1 throws
Blocker 1: mediaCall.hangup() emits 'ended' synchronously (@rocket.chat/media-signaling Call.js line 703). The 'ended' listener in useCallStore re-calls callLifecycle.end('remote') BEFORE the outer end() finishes assigning _endPromise — bypassing the re-entry guard and triggering double teardown (callEnded twice, end command twice, infinite recursion in tests). Defer _runTeardown to a microtask so _endPromise is captured first; the synchronous re-entrant call now hits the guard and shares the in-flight promise. Blocker 3: wrap step 1 (mediaCall.reject/hangup) in try/catch with a logger.warn so a throw does not abort subsequent teardown steps (markActive, markAvailable, store reset, stopAudio, callEnded), which would leak listeners and native CallKit/Telecom state. Tests: regression for synchronous re-entry asserts callEnded fires exactly once (toHaveBeenCalledTimes/toHaveLength) and the end command issues exactly once. Step-1 throw isolation tests assert the remaining six steps still run when reject/hangup throws. Integration tests updated to flush the new teardown microtask via await act().
1 parent acc3e8d commit 155bcb7

3 files changed

Lines changed: 175 additions & 15 deletions

File tree

app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -451,8 +451,10 @@ describe('VoIP call lifecycle (integration)', () => {
451451
// Firing 'ended' triggers CallLifecycle teardown via the handleEnded listener.
452452
// Navigation.back() is now handled by CallNavRouter (not wired in this integration test).
453453
// We verify the teardown sequence runs: store cleared, native end issued.
454-
act(() => {
454+
// CallLifecycle.end() defers its body to a microtask (re-entry guard); flush it.
455+
await act(async () => {
455456
(call!.emitter as unknown as ReturnType<typeof mockCallEmitter>).emit('ended');
457+
await Promise.resolve();
456458
});
457459
expect((voipNative as InMemoryVoipNative).recorded).toContainEqual({ cmd: 'end', callUuid: 'call-user-1' });
458460
// Navigation.back() is now owned by CallNavRouter after callEnded emits.
@@ -639,8 +641,10 @@ describe('VoIP call lifecycle (integration)', () => {
639641
await act(() => Promise.resolve());
640642
expect(useCallStore.getState().call?.callId).toBe('call-user-1');
641643

642-
act(() => {
644+
// CallLifecycle.end() defers its body to a microtask (re-entry guard); flush it.
645+
await act(async () => {
643646
useCallStore.getState().endCall();
647+
await Promise.resolve();
644648
});
645649

646650
expect((voipNative as InMemoryVoipNative).recorded).toContainEqual({ cmd: 'end', callUuid: 'call-user-1' });
@@ -651,16 +655,18 @@ describe('VoIP call lifecycle (integration)', () => {
651655
expect(useCallStore.getState().callId).toBeNull();
652656
});
653657

654-
it('B2: MediaSessionInstance.endCall during active state → voipNative cleanup, store reset', () => {
658+
it('B2: MediaSessionInstance.endCall during active state → voipNative cleanup, store reset', async () => {
655659
// endCall now delegates to callLifecycle.end('local'). CallLifecycle reads the
656660
// active call from useCallStore, so the call must be set there first.
657661
const activeCall = makeCall({ callId: 'active-1', state: 'active' });
658662
act(() => {
659663
useCallStore.getState().setCall(activeCall);
660664
});
661665

662-
act(() => {
666+
// CallLifecycle.end() defers its body to a microtask (re-entry guard); flush it.
667+
await act(async () => {
663668
mediaSessionInstance.endCall('active-1');
669+
await Promise.resolve();
664670
});
665671

666672
// CallLifecycle.end() steps 2-4 run via InMemoryVoipNative (records commands instead of calling RNCallKeep).
@@ -670,16 +676,18 @@ describe('VoIP call lifecycle (integration)', () => {
670676
expect(useCallStore.getState().call).toBeNull();
671677
});
672678

673-
it('B3: MediaSessionInstance.endCall during ringing → reject (not hangup) + voipNative cleanup', () => {
679+
it('B3: MediaSessionInstance.endCall during ringing → reject (not hangup) + voipNative cleanup', async () => {
674680
// CallLifecycle reads the active call from useCallStore to decide reject vs hangup.
675681
// The ringing call must be in the store for reject() to be called.
676682
const ringingCall = makeCall({ callId: 'ringing-1', state: 'ringing' });
677683
act(() => {
678684
useCallStore.getState().setCall(ringingCall);
679685
});
680686

681-
act(() => {
687+
// CallLifecycle.end() defers its body to a microtask (re-entry guard); flush it.
688+
await act(async () => {
682689
mediaSessionInstance.endCall('ringing-1');
690+
await Promise.resolve();
683691
});
684692

685693
expect(ringingCall.reject).toHaveBeenCalled();
@@ -814,7 +822,7 @@ describe('VoIP call lifecycle (integration)', () => {
814822
expect(useCallStore.getState().isOnHold).toBe(true);
815823
});
816824

817-
it('D3: press end button → call.hangup, voipNative.call.end, store cleared', () => {
825+
it('D3: press end button → call.hangup, voipNative.call.end, store cleared', async () => {
818826
const call = makeCall({ callId: 'btn-end', role: 'caller', state: 'active' });
819827
act(() => {
820828
useCallStore.getState().setCall(call);
@@ -826,8 +834,10 @@ describe('VoIP call lifecycle (integration)', () => {
826834
</Wrapper>
827835
);
828836

829-
act(() => {
837+
// CallLifecycle.end() defers its body to a microtask (re-entry guard); flush it.
838+
await act(async () => {
830839
fireEvent.press(getByTestId('call-view-end'));
840+
await Promise.resolve();
831841
});
832842

833843
expect(call.hangup).toHaveBeenCalled();

app/lib/services/voip/CallLifecycle.test.ts

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,4 +356,133 @@ describe('CallLifecycle.end(reason)', () => {
356356
await expect((freshLifecycle as any)._runTeardown('local')).resolves.toBeUndefined();
357357
});
358358
});
359+
360+
// Blocker 1 regression: faithful spy whose hangup() synchronously emits 'ended'
361+
// (mirrors @rocket.chat/media-signaling/dist/lib/Call.js behavior at line 703).
362+
// The 'ended' listener at useCallStore.ts handleEnded re-enters callLifecycle.end('remote').
363+
// The re-entry guard MUST be set BEFORE _runTeardown body runs, otherwise re-entrant
364+
// teardown happens (callEnded fires twice, end command issues twice).
365+
describe('re-entry guard against synchronous ended emission from hangup()', () => {
366+
function makeCallWithSyncEndedOnHangup(callId: string): IClientMediaCall {
367+
const listeners: Record<string, Set<(...args: unknown[]) => void>> = {};
368+
const emitter = {
369+
on: (ev: string, fn: (...args: unknown[]) => void) => {
370+
if (!listeners[ev]) listeners[ev] = new Set();
371+
listeners[ev].add(fn);
372+
return () => listeners[ev].delete(fn);
373+
},
374+
off: (ev: string, fn: (...args: unknown[]) => void) => {
375+
listeners[ev]?.delete(fn);
376+
},
377+
emit: (ev: string, ...args: unknown[]) => {
378+
listeners[ev]?.forEach(fn => fn(...args));
379+
}
380+
};
381+
const hangup = jest.fn(() => {
382+
// Mirror Call.js line 703: changeState('hangup') → emitter.emit('ended')
383+
emitter.emit('ended');
384+
});
385+
return {
386+
callId,
387+
state: 'active',
388+
hidden: false,
389+
localParticipant: {
390+
local: true,
391+
role: 'caller',
392+
muted: false,
393+
held: false,
394+
contact: {},
395+
setMuted: jest.fn(),
396+
setHeld: jest.fn()
397+
},
398+
remoteParticipants: [
399+
{
400+
local: false,
401+
role: 'callee',
402+
muted: false,
403+
held: false,
404+
contact: { id: 'u', displayName: 'U', username: 'u', sipExtension: '' }
405+
}
406+
],
407+
hangup,
408+
reject: jest.fn(),
409+
sendDTMF: jest.fn(),
410+
emitter
411+
} as unknown as IClientMediaCall;
412+
}
413+
414+
it('end() called from inside hangup() synchronous ended emit hits the re-entry guard', async () => {
415+
const call = makeCallWithSyncEndedOnHangup('reentry-1');
416+
useCallStore.getState().setCall(call);
417+
native.reset();
418+
419+
const callEndedListener = jest.fn();
420+
const unsub = callLifecycle.emitter.on('callEnded', callEndedListener);
421+
422+
// Outer end('local') triggers hangup() → 'ended' → handleEnded → end('remote')
423+
// Re-entrant call MUST hit the guard and return the in-flight promise.
424+
await callLifecycle.end('local');
425+
426+
unsub();
427+
428+
// callEnded fires exactly once — guard worked.
429+
expect(callEndedListener).toHaveBeenCalledTimes(1);
430+
// End command issued exactly once.
431+
const endCmds = native.recorded.filter(c => c.cmd === 'end');
432+
expect(endCmds).toHaveLength(1);
433+
// hangup invoked exactly once.
434+
expect(call.hangup).toHaveBeenCalledTimes(1);
435+
});
436+
});
437+
438+
// Blocker 3 regression: step 1 (mediaCall.reject/hangup) is wrapped in try/catch
439+
// so a throw doesn't abort subsequent steps (markActive, markAvailable, store reset, stopAudio, callEnded).
440+
describe('step 1 throw isolation', () => {
441+
it('continues teardown when mediaCall.hangup() throws', async () => {
442+
const call = makeCall({ callId: 'throw-1', state: 'active' });
443+
(call.hangup as jest.Mock).mockImplementationOnce(() => {
444+
throw new Error('hangup boom');
445+
});
446+
useCallStore.getState().setCall(call);
447+
native.reset();
448+
449+
const callEndedListener = jest.fn();
450+
const unsub = callLifecycle.emitter.on('callEnded', callEndedListener);
451+
452+
await expect(callLifecycle.end('local')).resolves.toBeUndefined();
453+
454+
unsub();
455+
456+
// All subsequent steps still ran.
457+
expect(native.recorded).toContainEqual({ cmd: 'end', callUuid: 'throw-1' });
458+
expect(native.recorded).toContainEqual({ cmd: 'markActive', callUuid: '' });
459+
expect(native.recorded).toContainEqual({ cmd: 'markAvailable', callUuid: 'throw-1' });
460+
expect(native.recorded).toContainEqual({ cmd: 'stopAudio' });
461+
expect(useCallStore.getState().call).toBeNull();
462+
expect(callEndedListener).toHaveBeenCalledTimes(1);
463+
});
464+
465+
it('continues teardown when mediaCall.reject() throws (ringing path)', async () => {
466+
const call = makeCall({ callId: 'throw-rej-1', state: 'ringing' });
467+
(call.reject as jest.Mock).mockImplementationOnce(() => {
468+
throw new Error('reject boom');
469+
});
470+
useCallStore.getState().setCall(call);
471+
native.reset();
472+
473+
const callEndedListener = jest.fn();
474+
const unsub = callLifecycle.emitter.on('callEnded', callEndedListener);
475+
476+
await expect(callLifecycle.end('rejected')).resolves.toBeUndefined();
477+
478+
unsub();
479+
480+
expect(native.recorded).toContainEqual({ cmd: 'end', callUuid: 'throw-rej-1' });
481+
expect(native.recorded).toContainEqual({ cmd: 'markActive', callUuid: '' });
482+
expect(native.recorded).toContainEqual({ cmd: 'markAvailable', callUuid: 'throw-rej-1' });
483+
expect(native.recorded).toContainEqual({ cmd: 'stopAudio' });
484+
expect(useCallStore.getState().call).toBeNull();
485+
expect(callEndedListener).toHaveBeenCalledTimes(1);
486+
});
487+
});
359488
});

app/lib/services/voip/CallLifecycle.ts

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,13 @@
1515
* `callId` in the `callEnded` event uses `callId ?? nativeAcceptedCallId` (Pre-bind-safe).
1616
*/
1717

18+
import { MediaCallLogger } from './MediaCallLogger';
1819
import { voipNative, type VoipNativePort } from './VoipNative';
1920
import { useCallStore } from './useCallStore';
2021

22+
const logger = new MediaCallLogger();
23+
const TAG = '[CallLifecycle]';
24+
2125
// ── Event types ───────────────────────────────────────────────────────────────
2226

2327
export type CallEndReason = 'local' | 'remote' | 'rejected' | 'error' | 'cleanup'; // 'cleanup' reserved for slice 08 Pre-bind FSM cleanupAt elapse
@@ -108,9 +112,17 @@ class CallLifecycle {
108112
// Concurrent caller — share the in-flight teardown.
109113
return this._endPromise;
110114
}
111-
this._endPromise = this._runTeardown(reason).finally(() => {
112-
this._endPromise = null;
113-
});
115+
// Defer the teardown body to a microtask so `_endPromise` is assigned BEFORE
116+
// `_runTeardown` runs. This guarantees that any synchronous re-entry from
117+
// inside teardown (e.g. mediaCall.hangup() emits 'ended' synchronously and
118+
// useCallStore's handleEnded re-calls callLifecycle.end('remote')) hits the
119+
// guard above and shares the in-flight promise instead of starting a second
120+
// teardown. See @rocket.chat/media-signaling Call.js line 703.
121+
this._endPromise = Promise.resolve()
122+
.then(() => this._runTeardown(reason))
123+
.finally(() => {
124+
this._endPromise = null;
125+
});
114126
return this._endPromise;
115127
}
116128

@@ -125,12 +137,21 @@ class CallLifecycle {
125137

126138
// Step 1: Hang up the MediaCall (reject if ringing, hangup otherwise).
127139
// Read the active call from useCallStore — MediaSessionInstance owns it.
140+
// Wrapped in try/catch so a throw from reject/hangup does not abort
141+
// subsequent teardown steps (which would leak listeners / native state).
128142
const mediaCall = useCallStore.getState().call;
129143
if (mediaCall) {
130-
if ((mediaCall as any).state === 'ringing') {
131-
mediaCall.reject();
132-
} else {
133-
mediaCall.hangup();
144+
try {
145+
if ((mediaCall as any).state === 'ringing') {
146+
mediaCall.reject();
147+
} else {
148+
mediaCall.hangup();
149+
}
150+
} catch (error) {
151+
logger.warn(
152+
`${TAG} mediaCall.${(mediaCall as any).state === 'ringing' ? 'reject' : 'hangup'}() threw; continuing teardown`,
153+
error
154+
);
134155
}
135156
}
136157

0 commit comments

Comments
 (0)