Merge pull request #13 from Chinchill-AI/fix/pr-review-comments

fix: Address unresolved PR review comments from #1, #7, #8

Author: patrick-chinchill

src/chat_sdk/adapters/slack/adapter.py

@@ -200,7 +200,7 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None:
 
         # Cache of AsyncWebClient instances keyed by bot token (LRU-bounded)
         self._client_cache: OrderedDict[str, Any] = OrderedDict()
-        self._client_cache_max = 100  # max cached clients
+        self._client_cache_max = config.client_cache_max or 100
 
         # Multi-workspace OAuth fields
         self._client_id: str | None = config.client_id or (os.environ.get("SLACK_CLIENT_ID") if zero_config else None)
@@ -281,14 +281,12 @@ def _get_client(self, token: str | None = None) -> Any:
         client = AsyncWebClient(token=resolved_token)
         self._client_cache[resolved_token] = client
         if len(self._client_cache) > self._client_cache_max:
-            # Evict oldest (LRU)
-            evicted_token, evicted_client = self._client_cache.popitem(last=False)
-            # Close the evicted client's session if possible
-            try:
-                if hasattr(evicted_client, "session") and evicted_client.session:
-                    asyncio.get_running_loop().create_task(evicted_client.session.close())
-            except RuntimeError:
-                pass
+            # Evict oldest (LRU).  We intentionally do NOT close the evicted
+            # client's session here because other concurrent requests may still
+            # hold a reference to the evicted AsyncWebClient instance.  The
+            # underlying aiohttp.ClientSession will be closed by the garbage
+            # collector (via __del__) once all references are released.
+            self._client_cache.popitem(last=False)
         return client
 
     def _invalidate_client(self, token: str) -> None:
@@ -2688,13 +2686,16 @@ def _handle_slack_error(self, error: Any) -> None:
 
         Never returns (always raises).
         """
-        slack_error = error
-        resp = getattr(slack_error, "response", None)
+        # slack_sdk's SlackApiError has a .response attribute (SlackResponse)
+        # SlackResponse has a .data dict and an .get() method
+        resp = getattr(error, "response", None)
         error_code: str | None = None
-        if isinstance(resp, dict):
-            error_code = resp.get("error")
-        elif resp is not None:
-            error_code = getattr(resp, "get", lambda *a: None)("error")
+        if resp is not None:
+            # SlackResponse has .data dict or direct attribute access
+            if hasattr(resp, "data") and isinstance(resp.data, dict):
+                error_code = resp.data.get("error")
+            elif isinstance(resp, dict):
+                error_code = resp.get("error")
 
         # Invalidate cached client on auth errors (token revocation / invalid_auth)
         if error_code in ("invalid_auth", "token_revoked", "account_inactive"):
@@ -2705,8 +2706,13 @@ def _handle_slack_error(self, error: Any) -> None:
                 pass
 
         # Check for rate limiting
-        if isinstance(resp, dict) and error_code == "ratelimited":
-            raise AdapterRateLimitError("slack") from error
+        if error_code == "ratelimited":
+            retry_after = None
+            if hasattr(resp, "headers"):
+                retry_after = resp.headers.get("Retry-After")
+            elif isinstance(resp, dict):
+                retry_after = resp.get("headers", {}).get("Retry-After")
+            raise AdapterRateLimitError("slack", int(retry_after) if retry_after else None) from error
 
         raise error  # type: ignore[misc]
 

src/chat_sdk/adapters/slack/types.py

@@ -34,6 +34,9 @@ class SlackAdapterConfig:
     logger: Logger | None = None
     # Signing secret for webhook verification. Defaults to SLACK_SIGNING_SECRET env var.
     signing_secret: str | None = None
+    # Maximum number of cached AsyncWebClient instances (LRU-bounded).
+    # Defaults to 100. Increase for large multi-workspace deployments.
+    client_cache_max: int | None = None
     # Override bot username (optional)
     user_name: str | None = None
 

src/chat_sdk/adapters/teams/adapter.py

@@ -8,6 +8,7 @@
 
 from __future__ import annotations
 
+import asyncio
 import base64
 import json
 import os
@@ -1707,7 +1708,7 @@ async def _verify_bot_framework_token(self, request: Any) -> Any | None:
                     return self._make_response("Unauthorized", 401)
                 self._jwks_client = PyJWKClient(jwks_uri)
 
-            signing_key = self._jwks_client.get_signing_key_from_jwt(token)
+            signing_key = await asyncio.to_thread(self._jwks_client.get_signing_key_from_jwt, token)
             payload = pyjwt.decode(
                 token,
                 signing_key.key,

tests/test_slack_client_cache.py

@@ -151,10 +151,25 @@ def test_invalidate_client_removes_entry(self):
 # ---------------------------------------------------------------------------
 
 
-def _make_slack_api_error(error_code: str) -> Exception:
-    """Build a mock SlackApiError whose response contains *error_code*."""
+def _make_slack_api_error(error_code: str, *, use_slack_response: bool = False, retry_after: str | None = None) -> Exception:
+    """Build a mock SlackApiError whose response contains *error_code*.
+
+    When *use_slack_response* is True, the response mimics a ``SlackResponse``
+    object (with ``.data`` dict and ``.headers``) instead of a plain dict.
+    """
     err = Exception(f"Slack error: {error_code}")
-    err.response = {"error": error_code}  # type: ignore[attr-defined]
+    if use_slack_response:
+
+        class _FakeSlackResponse:
+            def __init__(self):
+                self.data = {"error": error_code}
+                self.headers = {}
+                if retry_after is not None:
+                    self.headers["Retry-After"] = retry_after
+
+        err.response = _FakeSlackResponse()  # type: ignore[attr-defined]
+    else:
+        err.response = {"error": error_code}  # type: ignore[attr-defined]
     return err
 
 
@@ -193,3 +208,81 @@ def test_handle_slack_error_non_auth_error_keeps_cache(self):
             adapter._handle_slack_error(_make_slack_api_error("channel_not_found"))
 
         assert "xoxb-tok" in adapter._client_cache, "Non-auth error should not evict the client"
+
+    def test_handle_slack_error_slack_response_object(self):
+        """Auth eviction must work when resp is a SlackResponse (not a dict)."""
+        adapter = _make_adapter(bot_token="xoxb-tok")
+        adapter._get_client("xoxb-tok")
+        assert "xoxb-tok" in adapter._client_cache
+
+        with pytest.raises(Exception, match="invalid_auth"):
+            adapter._handle_slack_error(
+                _make_slack_api_error("invalid_auth", use_slack_response=True)
+            )
+
+        assert "xoxb-tok" not in adapter._client_cache
+
+
+# ---------------------------------------------------------------------------
+# _handle_slack_error — rate limiting
+# ---------------------------------------------------------------------------
+
+
+class TestHandleSlackErrorRateLimit:
+    """_handle_slack_error should raise AdapterRateLimitError on ratelimited."""
+
+    def test_rate_limit_from_dict_response(self):
+        """Rate limit detection with a plain dict response."""
+        from chat_sdk.shared.errors import AdapterRateLimitError
+
+        adapter = _make_adapter(bot_token="xoxb-tok")
+        with pytest.raises(AdapterRateLimitError):
+            adapter._handle_slack_error(_make_slack_api_error("ratelimited"))
+
+    def test_rate_limit_from_slack_response(self):
+        """Rate limit detection with a SlackResponse-like object."""
+        from chat_sdk.shared.errors import AdapterRateLimitError
+
+        adapter = _make_adapter(bot_token="xoxb-tok")
+        with pytest.raises(AdapterRateLimitError) as exc_info:
+            adapter._handle_slack_error(
+                _make_slack_api_error("ratelimited", use_slack_response=True, retry_after="30")
+            )
+        assert exc_info.value.retry_after == 30
+
+    def test_rate_limit_without_retry_after(self):
+        """Rate limit with no Retry-After header should still raise."""
+        from chat_sdk.shared.errors import AdapterRateLimitError
+
+        adapter = _make_adapter(bot_token="xoxb-tok")
+        with pytest.raises(AdapterRateLimitError) as exc_info:
+            adapter._handle_slack_error(
+                _make_slack_api_error("ratelimited", use_slack_response=True)
+            )
+        assert exc_info.value.retry_after is None
+
+
+# ---------------------------------------------------------------------------
+# Configurable client_cache_max
+# ---------------------------------------------------------------------------
+
+
+class TestConfigurableCacheMax:
+    """client_cache_max should be configurable via SlackAdapterConfig."""
+
+    def test_default_cache_max(self):
+        """Default cache max should be 100."""
+        adapter = _make_adapter()
+        assert adapter._client_cache_max == 100
+
+    def test_custom_cache_max(self):
+        """Custom cache max should override the default."""
+        adapter = _make_adapter(client_cache_max=50)
+        assert adapter._client_cache_max == 50
+
+        # Fill to capacity + 1
+        for i in range(51):
+            adapter._get_client(f"tok-{i}")
+
+        assert len(adapter._client_cache) == 50
+        assert "tok-0" not in adapter._client_cache