fix(ios): add NSLock to nativeAcceptHandledCallIds and 10s REST timeout to handleNativeAccept

[diegolmello]

Summary

H1 fix: Add NSLock (nativeAcceptLock) to synchronize all reads/writes of nativeAcceptHandledCallIds across:

• handleNativeAccept (CallKit answer path)
• clearNativeAcceptDedupe (called from DDP listener, timeout handler, call observer)
• CXCallObserver callbacks

H2 fix: Add a 10-second DispatchWorkItem timeout guard in handleNativeAccept. If the REST callback (api.fetch) hasn't fired within 10 seconds, the work item calls finishAccept(false). The work item cancels itself when the REST response arrives first.

Changes

• ios/Libraries/VoipService.swift:
  • Added nativeAcceptLock = NSLock() to serialize nativeAcceptHandledCallIds access
  • Wrapped the check+insert in handleNativeAccept with nativeAcceptLock.lock/unlock
  • Wrapped nativeAcceptHandledCallIds.remove(callId) in clearNativeAcceptDedupe with lock
  • Added 10s timeout DispatchWorkItem that checks if callId is still tracked before calling finishAccept(false)
  • finishAccept captures [weak timeoutWorkItem] to cancel the timeout on REST completion

Testing

• iOS build: xcodebuild compiles without errors (Watch App / JitsiWebRTC failures are pre-existing, unrelated to this change)
• Logic: timeout fires after 10s if REST hangs, cancels when REST completes; lock serializes concurrent access

Merge Order

This is PR 1 of 6. Merge in this order:

1. PR-2: fix(ios) DDP cleanup — independent ← already merged
2. PR-1: fix(ios) NSLock + timeout ← MERGE NEXT
3. PR-3: feat(android) VoipCallService — independent
4. PR-4: fix(android) service integration — MUST merge after PR-3
5. PR-5: fix(both) null guard — independent, merge before PR-6
6. PR-6: chore(ts) cleanup — merge last (shares file with PR-5)

Summary by CodeRabbit

• Bug Fixes
  • Improved concurrent call handling reliability by adding thread-safe serialization to prevent race conditions.
  • Added automatic 10-second timeout for calls to ensure proper cleanup if calls hang or don't complete.

[coderabbitai]

Walkthrough

Added thread-safety mechanisms to prevent concurrent access races in VoIP call deduplication by introducing an NSLock, and implemented a 10-second timeout that automatically fails incomplete call acceptance operations.

Changes

Cohort / File(s)	Summary
VoIP Service Thread Safety ios/Libraries/VoipService.swift	Added nativeAcceptLock to serialize access to nativeAcceptHandledCallIds. Wrapped dedupe mutations and reads with the lock. Introduced 10-second timeout with DispatchWorkItem that calls finishAccept(false) for incomplete operations and cancels via weak reference.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: adding NSLock for thread-safe access to nativeAcceptHandledCallIds and implementing a 10-second timeout mechanism in handleNativeAccept.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[diegolmello]

Code Review — PR #7198

Files Reviewed: ios/Libraries/VoipService.swift
Issues: 0 CRITICAL/HIGH, 1 LOW observation

---

[LOW] Capture of finishAccept in timeout workItem closure

The timeout workItem closure at line 461-470 captures finishAccept by reference:

let timeoutWorkItem = DispatchWorkItem { [weak payload] in
    ...
    if isStillTracked {
        finishAccept(false)  // ← captures `finishAccept` variable by reference
    }
}
let finishAccept: (Bool) -> Void = { [weak timeoutWorkItem] success in
    ...
}

Swift closures capture variables by reference (not by value), so when the workItem fires 10 seconds later, finishAccept will resolve to whichever closure was assigned to that variable. This is intentional and correct. However, a reviewer should be aware that the capture chain is: finishAccept variable → closure body → stopDDPClientInternal, storeInitialEvents, clearNativeAcceptDedupe, notification posts. All of these are safe to call from main thread (where the timeout fires via DispatchQueue.main.asyncAfter).

No action required — this is a correct design.

---

Verification Against Acceptance Criteria

Criterion	Status	Notes
nativeAcceptHandledCallIds reads/writes synchronized via NSLock	✅	Lock wraps: check+insert in handleNativeAccept (line 449-456), remove in clearNativeAcceptDedupe (line 284-287)
handleNativeAccept times out after 10s if REST callback hasn't fired	✅	DispatchWorkItem with 10s delay at line 471; fires finishAccept(false) if callId still tracked
Timeout cancels correctly when REST completes before 10s	✅	finishAccept captures [weak timeoutWorkItem] and calls timeoutWorkItem?.cancel() at line 474
No regression in existing call accept flow	✅	finishAccept logic unchanged; lock is additive; timeout path mirrors existing failure path

---

Positive Observations

1. Lock is correctly scoped. nativeAcceptLock covers all 3 access sites exactly, no more and no less.
2. Double-check pattern prevents race. Checking contains before insert inside the lock ensures only one path proceeds.
3. Timeout re-checks deduplication slot. The timeout workItem re-checks nativeAcceptHandledCallIds before calling finishAccept(false), preventing double-trigger (timeout fires and then REST fires).
4. Weak capture on payload. [weak payload] in the timeout workItem prevents retain cycle if the call is cleaned up before 10s.
5. Timeout fires on main thread. DispatchQueue.main.asyncAfter ensures all state mutations (finishAccept, notifications) happen on main, consistent with the rest of the code.
6. Matches Android pattern. VoipNotification.handleAcceptAction already has a 10s timeout (line 276: timeoutHandler.postDelayed(postedTimeout, 10_000L)), so this aligns the iOS behavior.

---

Verdict

LGTM — All 4 acceptance criteria satisfied. No blocking issues. The LOW observation is informational only.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

ios/Libraries/VoipService.swift (1)

460-546: ⚠️ Potential issue | 🔴 Critical

Make native accept completion single-shot with atomic deduplication check.

The timeout closure (line 461) can call finishAccept(false), then a late REST callback (line 541) can still call finishAccept(true), posting both VoipAcceptFailed and VoipAcceptSucceeded notifications for the same call. The timeout closure also references finishAccept before its declaration (line 468 refs line 473). Move the deduplication check into finishAccept as an atomic remove-and-proceed guard: only the first completion (timeout or REST) should proceed; others should no-op.

🐛 Proposed fix

-        // 10-second timeout guard: if REST hasn't completed by then, call finishAccept(false).
-        let timeoutWorkItem = DispatchWorkItem { [weak payload] in
-            guard let payload else { return }
-            // Check the callId is still tracked (not already cleaned up).
-            nativeAcceptLock.lock()
-            let isStillTracked = nativeAcceptHandledCallIds.contains(payload.callId)
-            nativeAcceptLock.unlock()
-            if isStillTracked {
-                finishAccept(false)
-            }
-        }
-        DispatchQueue.main.asyncAfter(deadline: .now() + 10.0, execute: timeoutWorkItem)
-
-        let finishAccept: (Bool) -> Void = { [weak timeoutWorkItem] success in
-            timeoutWorkItem?.cancel()
+        var timeoutWorkItem: DispatchWorkItem?
+
+        let finishAccept: (Bool) -> Void = { success in
+            timeoutWorkItem?.cancel()
+            timeoutWorkItem = nil
+
+            nativeAcceptLock.lock()
+            let shouldFinish = nativeAcceptHandledCallIds.remove(payload.callId) != nil
+            nativeAcceptLock.unlock()
+            guard shouldFinish else {
+                return
+            }
+
             stopDDPClientInternal(callId: payload.callId)
             if success {
                 storeInitialEvents(payload)
-                clearNativeAcceptDedupe(for: payload.callId)
                 NotificationCenter.default.post(
                     name: NSNotification.Name("VoipAcceptSucceeded"),
                     object: nil,
                     userInfo: payload.toDictionary()
                 )
             } else {
-                clearNativeAcceptDedupe(for: payload.callId)
                 RNCallKeep.endCall(withUUID: payload.callId, reason: 6)
                 let failedPayload = VoipPayload(
                     callId: payload.callId,
@@
                 )
             }
         }
+
+        // 10-second timeout guard: if REST hasn't completed by then, call finishAccept(false).
+        timeoutWorkItem = DispatchWorkItem {
+            finishAccept(false)
+        }
+        if let timeoutWorkItem {
+            DispatchQueue.main.asyncAfter(deadline: .now() + 10.0, execute: timeoutWorkItem)
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/VoipService.swift` around lines 460 - 546, The timeoutClosure
(timeoutWorkItem) can race with the REST callback causing finishAccept to run
twice and post both VoipAcceptFailed and VoipAcceptSucceeded; also
timeoutWorkItem captures finishAccept before it's declared. Fix by moving the
dedupe atomic check and remove into finishAccept: have
finishAccept(callSucceeded:) first acquire nativeAcceptLock, check and remove
payload.callId from nativeAcceptHandledCallIds (no-op and return if not
present), then proceed to cancel timeoutWorkItem, call stopDDPClientInternal,
clearNativeAcceptDedupe(for:), and post the appropriate notification; keep
timeoutWorkItem only calling finishAccept(false) and the REST callback calling
finishAccept(true) so only the first caller proceeds. Ensure you reference and
update nativeAcceptHandledCallIds, nativeAcceptLock, timeoutWorkItem,
finishAccept, stopDDPClientInternal, and clearNativeAcceptDedupe in the change.

🤖 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 `@ios/Libraries/VoipService.swift`:
- Around line 460-546: The timeoutClosure (timeoutWorkItem) can race with the
REST callback causing finishAccept to run twice and post both VoipAcceptFailed
and VoipAcceptSucceeded; also timeoutWorkItem captures finishAccept before it's
declared. Fix by moving the dedupe atomic check and remove into finishAccept:
have finishAccept(callSucceeded:) first acquire nativeAcceptLock, check and
remove payload.callId from nativeAcceptHandledCallIds (no-op and return if not
present), then proceed to cancel timeoutWorkItem, call stopDDPClientInternal,
clearNativeAcceptDedupe(for:), and post the appropriate notification; keep
timeoutWorkItem only calling finishAccept(false) and the REST callback calling
finishAccept(true) so only the first caller proceeds. Ensure you reference and
update nativeAcceptHandledCallIds, nativeAcceptLock, timeoutWorkItem,
finishAccept, stopDDPClientInternal, and clearNativeAcceptDedupe in the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f6218167-3325-42ee-9e0c-f80914bb3f2e

📥 Commits

Reviewing files that changed from the base of the PR and between d30c291 and 797b53d.

📒 Files selected for processing (1)

• ios/Libraries/VoipService.swift

📜 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 (1)

📚 Learning: 2026-04-07T17:49:17.538Z

Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate

Applied to files:

• ios/Libraries/VoipService.swift

🔇 Additional comments (3)

ios/Libraries/VoipService.swift (3)

39-41: LGTM — scoped lock for the native accept dedupe set.

This is a focused synchronization primitive for the new cross-context nativeAcceptHandledCallIds access.

---

285-287: LGTM — removal is now synchronized.

This closes the obvious race between cleanup paths and handleNativeAccept.

---

449-456: LGTM — check-and-insert is atomic now.

Serializing this block prevents duplicate accepts from concurrent CXCallObserver callbacks.