fix: address concurrency review findings (campaign-scoped counter, cleanup hardening, webrtc run validation)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

api/tests/telephony/cloudonix/test_routes.py

@@ -244,3 +244,34 @@ def test_parse_cloudonix_cdr_preserves_zero_billsec():
     )
 
     assert req["duration"] == "0"
+
+
+@pytest.mark.asyncio
+async def test_agent_stream_handshake_timeout_closes_socket(monkeypatch):
+    """An idle agent-stream socket holds an org concurrency slot, so the
+    handshake read must be bounded rather than waiting forever."""
+    import asyncio
+
+    from api.services.telephony.providers.cloudonix import provider as provider_module
+
+    monkeypatch.setattr(provider_module, "AGENT_STREAM_HANDSHAKE_TIMEOUT_S", 0.05)
+    provider = CloudonixProvider({})
+
+    async def never_returns():
+        await asyncio.Event().wait()
+
+    websocket = SimpleNamespace(receive_text=never_returns, close=AsyncMock())
+
+    await asyncio.wait_for(
+        provider.handle_external_websocket(
+            websocket,
+            organization_id=1,
+            workflow_id=2,
+            user_id=3,
+            workflow_run_id=4,
+            params={},
+        ),
+        timeout=5,
+    )
+
+    websocket.close.assert_awaited_once_with(code=4408, reason="Handshake timeout")

api/tests/telephony/test_ari_manager_transfer_teardown.py

@@ -1,5 +1,6 @@
 import json
 from types import SimpleNamespace
+from unittest.mock import AsyncMock
 
 import pytest
 
@@ -7,6 +8,13 @@ from api.services.telephony import ari_manager
 from api.services.telephony.ari_manager import ARIConnection
 
 
+@pytest.fixture
+def fake_call_concurrency(monkeypatch):
+    fake = SimpleNamespace(unregister_active_call=AsyncMock(return_value=True))
+    monkeypatch.setattr(ari_manager, "call_concurrency", fake)
+    return fake
+
+
 class _FakeDbClient:
     def __init__(self, gathered_context):
         self.workflow_run = SimpleNamespace(gathered_context=gathered_context)
@@ -77,6 +85,7 @@ def _completed_transfer_context():
 @pytest.mark.asyncio
 async def test_completed_transfer_tears_down_destination_when_caller_leaves(
     monkeypatch,
+    fake_call_concurrency,
 ):
     fake_db = _FakeDbClient(_completed_transfer_context())
     monkeypatch.setattr(ari_manager, "db_client", fake_db)
@@ -90,11 +99,13 @@ async def test_completed_transfer_tears_down_destination_when_caller_leaves(
     assert "dest-chan" in conn.deleted_channel_runs
     assert conn.deleted_transfer_channel_mappings == ["dest-chan"]
     assert fake_db.workflow_run.gathered_context["transfer_state"] == "terminated"
+    fake_call_concurrency.unregister_active_call.assert_awaited_with(123)
 
 
 @pytest.mark.asyncio
 async def test_completed_transfer_tears_down_caller_when_destination_leaves(
     monkeypatch,
+    fake_call_concurrency,
 ):
     fake_db = _FakeDbClient(_completed_transfer_context())
     monkeypatch.setattr(ari_manager, "db_client", fake_db)
@@ -108,6 +119,31 @@ async def test_completed_transfer_tears_down_caller_when_destination_leaves(
     assert "dest-chan" in conn.deleted_channel_runs
     assert conn.deleted_transfer_channel_mappings == ["dest-chan"]
     assert fake_db.workflow_run.gathered_context["transfer_state"] == "terminated"
+    fake_call_concurrency.unregister_active_call.assert_awaited_with(123)
+
+
+@pytest.mark.asyncio
+async def test_stasis_end_releases_concurrency_slot_on_normal_teardown(
+    monkeypatch,
+    fake_call_concurrency,
+):
+    """StasisEnd must release the org slot even when the pipeline never ran
+    (e.g. the caller hung up before external media connected)."""
+    fake_db = _FakeDbClient(
+        {
+            "call_id": "caller-chan",
+            "ext_channel_id": "ext-chan",
+            "bridge_id": "bridge-1",
+        }
+    )
+    monkeypatch.setattr(ari_manager, "db_client", fake_db)
+    conn = _RecordingARIConnection()
+
+    await conn._handle_stasis_end("caller-chan", "123")
+
+    fake_call_concurrency.unregister_active_call.assert_awaited_once_with(123)
+    assert conn.deleted_bridges == ["bridge-1"]
+    assert conn.deleted_channels == ["ext-chan"]
 
 
 @pytest.mark.asyncio

api/tests/test_call_concurrency.py

@@ -33,7 +33,7 @@ async def test_acquire_org_slot_logs_post_acquire_count_and_limit():
     assert slot.max_concurrent == 10
     assert slot.source == "test_source"
     mock_rate_limiter.try_acquire_concurrent_slot_details.assert_awaited_once_with(
-        199, 10
+        199, 10, scope_key=None, scope_max_concurrent=None
     )
     mock_logger.info.assert_called_once()
     log_message = mock_logger.info.call_args.args[0]
@@ -67,3 +67,222 @@ async def test_acquire_org_slot_logs_warning_when_limit_reached():
     assert "Concurrent call limit reached for org 199" in log_message
     assert "source=test_source" in log_message
     assert "active_calls=12/10" in log_message
+
+
+@pytest.mark.asyncio
+async def test_acquire_org_slot_passes_scope_to_rate_limiter():
+    service = CallConcurrencyService()
+
+    with (
+        patch("api.services.call_concurrency.db_client") as mock_db,
+        patch("api.services.call_concurrency.rate_limiter") as mock_rate_limiter,
+    ):
+        mock_db.get_configuration = AsyncMock(return_value=None)
+        mock_rate_limiter.try_acquire_concurrent_slot_details = AsyncMock(
+            return_value=ConcurrentSlotAcquisition(slot_id="slot-123", active_count=1)
+        )
+        mock_rate_limiter.store_workflow_slot_mapping_if_absent = AsyncMock(
+            return_value=True
+        )
+
+        slot = await service.acquire_org_slot(
+            199,
+            source="campaign:42",
+            scope_key="campaign:42",
+            scope_max_concurrent=3,
+        )
+        await service.bind_workflow_run(slot, 501)
+
+    assert slot.scope_key == "campaign:42"
+    mock_rate_limiter.try_acquire_concurrent_slot_details.assert_awaited_once_with(
+        199, 10, scope_key="campaign:42", scope_max_concurrent=3
+    )
+    mock_rate_limiter.store_workflow_slot_mapping_if_absent.assert_awaited_once_with(
+        501, 199, "slot-123", scope_key="campaign:42"
+    )
+
+
+@pytest.mark.asyncio
+async def test_release_workflow_run_slot_keeps_mapping_on_redis_error():
+    service = CallConcurrencyService()
+
+    with patch("api.services.call_concurrency.rate_limiter") as mock_rate_limiter:
+        mock_rate_limiter.get_workflow_slot_mapping = AsyncMock(
+            return_value=(11, "slot-1", None)
+        )
+        # None = Redis error during release (vs False = slot already gone)
+        mock_rate_limiter.release_concurrent_slot = AsyncMock(return_value=None)
+        mock_rate_limiter.delete_workflow_slot_mapping = AsyncMock()
+
+        released = await service.release_workflow_run_slot(501)
+
+    assert released is False
+    mock_rate_limiter.release_concurrent_slot.assert_awaited_once_with(
+        11, "slot-1", scope_key=None
+    )
+    mock_rate_limiter.delete_workflow_slot_mapping.assert_not_awaited()
+
+
+@pytest.mark.asyncio
+async def test_release_workflow_run_slot_deletes_mapping_when_slot_already_gone():
+    service = CallConcurrencyService()
+
+    with patch("api.services.call_concurrency.rate_limiter") as mock_rate_limiter:
+        mock_rate_limiter.get_workflow_slot_mapping = AsyncMock(
+            return_value=(11, "slot-1", "campaign:42")
+        )
+        mock_rate_limiter.release_concurrent_slot = AsyncMock(return_value=False)
+        mock_rate_limiter.delete_workflow_slot_mapping = AsyncMock(return_value=True)
+
+        released = await service.release_workflow_run_slot(501)
+
+    assert released is False
+    mock_rate_limiter.release_concurrent_slot.assert_awaited_once_with(
+        11, "slot-1", scope_key="campaign:42"
+    )
+    mock_rate_limiter.delete_workflow_slot_mapping.assert_awaited_once_with(501)
+
+
+@pytest.mark.asyncio
+async def test_unregister_active_call_never_raises():
+    service = CallConcurrencyService()
+
+    with patch("api.services.call_concurrency.rate_limiter") as mock_rate_limiter:
+        mock_rate_limiter.get_workflow_slot_mapping = AsyncMock(
+            side_effect=RuntimeError("redis down")
+        )
+
+        released = await service.unregister_active_call(501)
+
+    assert released is False
+
+
+# ---------------------------------------------------------------------------
+# Redis integration tests for scoped (campaign-level) slot acquisition
+# ---------------------------------------------------------------------------
+
+import os  # noqa: E402
+import uuid  # noqa: E402
+
+from api.services.campaign.rate_limiter import RateLimiter  # noqa: E402
+
+requires_redis = pytest.mark.skipif(
+    "REDIS_URL" not in os.environ,
+    reason="Requires Redis (set REDIS_URL via .env.test)",
+)
+
+
+def _unique_org_id() -> int:
+    return uuid.uuid4().int % 10_000_000
+
+
+@requires_redis
+@pytest.mark.asyncio
+async def test_scoped_acquisition_enforces_scope_limit_independently_of_org():
+    """A campaign scope caps its own calls without measuring — or being
+    starved by — other calls in the same org counter."""
+    rl = RateLimiter()
+    org_id = _unique_org_id()
+    scope = f"campaign:{org_id}"
+    org_key = f"concurrent_calls:{org_id}"
+    scope_key_full = f"concurrent_calls:{scope}"
+    redis_client = await rl._get_redis()
+
+    try:
+        # Unscoped (e.g. WebRTC) calls fill part of the org counter.
+        for _ in range(3):
+            assert await rl.try_acquire_concurrent_slot_details(org_id, 10)
+
+        # Scope limit 2: two scoped acquisitions succeed...
+        first = await rl.try_acquire_concurrent_slot_details(
+            org_id, 10, scope_key=scope, scope_max_concurrent=2
+        )
+        second = await rl.try_acquire_concurrent_slot_details(
+            org_id, 10, scope_key=scope, scope_max_concurrent=2
+        )
+        assert first and second
+
+        # ...the third is rejected by the scope even though the org has room.
+        third = await rl.try_acquire_concurrent_slot_details(
+            org_id, 10, scope_key=scope, scope_max_concurrent=2
+        )
+        assert third is None
+
+        # Unscoped calls are unaffected by the scope being full.
+        assert await rl.try_acquire_concurrent_slot_details(org_id, 10)
+
+        # Releasing with the scope key frees both counters.
+        released = await rl.release_concurrent_slot(
+            org_id, first.slot_id, scope_key=scope
+        )
+        assert released is True
+        assert await redis_client.zscore(org_key, first.slot_id) is None
+        assert await redis_client.zscore(scope_key_full, first.slot_id) is None
+
+        # And the scope accepts a new call again.
+        assert await rl.try_acquire_concurrent_slot_details(
+            org_id, 10, scope_key=scope, scope_max_concurrent=2
+        )
+    finally:
+        await redis_client.delete(org_key, scope_key_full)
+        await rl.close()
+
+
+@requires_redis
+@pytest.mark.asyncio
+async def test_org_limit_still_binds_scoped_acquisition():
+    rl = RateLimiter()
+    org_id = _unique_org_id()
+    scope = f"campaign:{org_id}"
+    org_key = f"concurrent_calls:{org_id}"
+    scope_key_full = f"concurrent_calls:{scope}"
+    redis_client = await rl._get_redis()
+
+    try:
+        assert await rl.try_acquire_concurrent_slot_details(org_id, 1)
+
+        # Org counter is full, so the scoped acquire fails and must not
+        # leave a phantom entry in the scope counter.
+        rejected = await rl.try_acquire_concurrent_slot_details(
+            org_id, 1, scope_key=scope, scope_max_concurrent=5
+        )
+        assert rejected is None
+        assert await redis_client.zcard(scope_key_full) == 0
+    finally:
+        await redis_client.delete(org_key, scope_key_full)
+        await rl.close()
+
+
+@requires_redis
+@pytest.mark.asyncio
+async def test_workflow_slot_mapping_round_trips_scope_key():
+    rl = RateLimiter()
+    run_id = _unique_org_id()
+    mapping_key = f"workflow_slot_mapping:{run_id}"
+    redis_client = await rl._get_redis()
+
+    try:
+        stored = await rl.store_workflow_slot_mapping_if_absent(
+            run_id, 11, "slot-1", scope_key="campaign:42"
+        )
+        assert stored is True
+        assert await rl.get_workflow_slot_mapping(run_id) == (
+            11,
+            "slot-1",
+            "campaign:42",
+        )
+
+        # Unscoped mappings surface scope_key=None.
+        run_id_2 = _unique_org_id()
+        try:
+            await rl.store_workflow_slot_mapping_if_absent(run_id_2, 11, "slot-2")
+            assert await rl.get_workflow_slot_mapping(run_id_2) == (
+                11,
+                "slot-2",
+                None,
+            )
+        finally:
+            await redis_client.delete(f"workflow_slot_mapping:{run_id_2}")
+    finally:
+        await redis_client.delete(mapping_key)
+        await rl.close()

api/tests/test_campaign_call_dispatcher.py

@@ -266,8 +266,9 @@ def mock_call_concurrency():
         return CallConcurrencySlot(
             organization_id=organization_id,
             slot_id=f"slot-{uuid.uuid4().hex[:8]}",
-            max_concurrent=kwargs.get("max_concurrent_override") or 20,
+            max_concurrent=20,
             source=source,
+            scope_key=kwargs.get("scope_key"),
         )
 
     with patch(
@@ -814,3 +815,49 @@ class TestProcessBatchEdgeCases:
                     {"campaign_id": campaign_test_data.campaign_id},
                 )
                 await session.commit()
+
+
+class TestAcquireConcurrentSlotScoping:
+    """Campaign max_concurrency must scope to the campaign, not the org counter."""
+
+    def _campaign(self, orchestrator_metadata):
+        campaign = MagicMock()
+        campaign.id = 42
+        campaign.orchestrator_metadata = orchestrator_metadata
+        return campaign
+
+    @pytest.mark.asyncio
+    async def test_campaign_max_concurrency_uses_campaign_scope(
+        self, mock_call_concurrency
+    ):
+        dispatcher = CampaignCallDispatcher()
+        campaign = self._campaign({"max_concurrency": 3})
+
+        await dispatcher.acquire_concurrent_slot(7, campaign, timeout=5)
+
+        mock_call_concurrency.acquire_org_slot.assert_awaited_once_with(
+            7,
+            source="campaign:42",
+            timeout=5,
+            scope_key="campaign:42",
+            scope_max_concurrent=3,
+            retry_interval=1,
+        )
+
+    @pytest.mark.asyncio
+    async def test_no_campaign_max_concurrency_skips_scope(
+        self, mock_call_concurrency
+    ):
+        dispatcher = CampaignCallDispatcher()
+        campaign = self._campaign({})
+
+        await dispatcher.acquire_concurrent_slot(7, campaign, timeout=5)
+
+        mock_call_concurrency.acquire_org_slot.assert_awaited_once_with(
+            7,
+            source="campaign:42",
+            timeout=5,
+            scope_key=None,
+            scope_max_concurrent=None,
+            retry_interval=1,
+        )

api/tests/test_public_agent_routes.py

@@ -154,7 +154,8 @@ def test_workflow_uuid_route_uses_scoped_lookup_and_shared_execution():
             new=AsyncMock(return_value=("https://api.example.com", "wss://ignored")),
         ),
     ):
-        mock_concurrency.acquire_org_slot = AsyncMock(return_value=object())
+        slot = object()
+        mock_concurrency.acquire_org_slot = AsyncMock(return_value=slot)
         mock_concurrency.bind_workflow_run = AsyncMock()
         mock_concurrency.release_workflow_run_slot = AsyncMock()
         mock_concurrency.release_slot = AsyncMock()
@@ -180,6 +181,12 @@ def test_workflow_uuid_route_uses_scoped_lookup_and_shared_execution():
         11,
     )
     assert not mock_db.get_agent_trigger_by_path.called
+    mock_concurrency.acquire_org_slot.assert_awaited_once_with(
+        workflow.organization_id,
+        source="public_agent",
+        timeout=0,
+    )
+    mock_concurrency.bind_workflow_run.assert_awaited_once_with(slot, 601)
 
     create_kwargs = mock_db.create_workflow_run.await_args.kwargs
     assert create_kwargs["user_id"] == workflow.user_id

api/tests/test_webrtc_signaling_concurrency.py

@@ -115,3 +115,53 @@ async def test_public_embed_renegotiation_does_not_acquire_another_slot():
     mock_concurrency.acquire_org_slot.assert_not_called()
     pc.renegotiate.assert_awaited_once()
     assert ws.send_json.await_args.args[0]["type"] == "answer"
+
+
+@pytest.mark.asyncio
+async def test_signaling_websocket_rejects_run_not_owned_by_workflow():
+    """The URL workflow_id drives org/quota/concurrency accounting, so a
+    workflow_id that doesn't own the run must be rejected before signaling."""
+    from fastapi import HTTPException
+
+    from api.routes.webrtc_signaling import signaling_websocket
+
+    ws = _FakeWebSocket()
+    user = SimpleNamespace(id=7)
+
+    with (
+        patch("api.routes.webrtc_signaling.db_client") as mock_db,
+        patch("api.routes.webrtc_signaling.signaling_manager") as mock_manager,
+    ):
+        mock_db.get_workflow_run = AsyncMock(
+            return_value=SimpleNamespace(id=501, workflow_id=99)
+        )
+        mock_manager.handle_websocket = AsyncMock()
+
+        with pytest.raises(HTTPException) as exc_info:
+            await signaling_websocket(
+                ws, workflow_id=33, workflow_run_id=501, user=user
+            )
+
+    assert exc_info.value.status_code == 400
+    mock_manager.handle_websocket.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_signaling_websocket_accepts_matching_workflow_and_run():
+    from api.routes.webrtc_signaling import signaling_websocket
+
+    ws = _FakeWebSocket()
+    user = SimpleNamespace(id=7)
+
+    with (
+        patch("api.routes.webrtc_signaling.db_client") as mock_db,
+        patch("api.routes.webrtc_signaling.signaling_manager") as mock_manager,
+    ):
+        mock_db.get_workflow_run = AsyncMock(
+            return_value=SimpleNamespace(id=501, workflow_id=33)
+        )
+        mock_manager.handle_websocket = AsyncMock()
+
+        await signaling_websocket(ws, workflow_id=33, workflow_run_id=501, user=user)
+
+    mock_manager.handle_websocket.assert_awaited_once()