From 9519a81e08b1dc2ea2d00ad436bbef948198ac6d Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Mon, 6 Apr 2026 19:17:07 -0700 Subject: [PATCH] fix: Address unresolved PR review comments from #1, #7, #8 - Fix Slack rate limit detection: _handle_slack_error now correctly handles SlackResponse objects (not just dicts) by checking resp.data, and extracts the Retry-After header for rate limit errors - Fix Teams JWT verification blocking: wrap synchronous PyJWKClient.get_signing_key_from_jwt in asyncio.to_thread to avoid blocking the event loop during JWKS fetch - Make Slack client cache max configurable: add client_cache_max field to SlackAdapterConfig (defaults to 100) - Fix unsafe session cleanup on LRU eviction: remove create_task session close that could break concurrent requests using evicted clients; rely on garbage collection instead - Add tests for SlackResponse handling, rate limiting, and configurable cache max Co-Authored-By: Claude Opus 4.6 (1M context) --- src/chat_sdk/adapters/slack/adapter.py | 40 ++++++----- src/chat_sdk/adapters/slack/types.py | 3 + src/chat_sdk/adapters/teams/adapter.py | 3 +- tests/test_slack_client_cache.py | 99 +++++++++++++++++++++++++- 4 files changed, 124 insertions(+), 21 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 3c629c9..3d99353 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/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] diff --git a/src/chat_sdk/adapters/slack/types.py b/src/chat_sdk/adapters/slack/types.py index f55675c..a085b80 100644 --- a/src/chat_sdk/adapters/slack/types.py +++ b/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 diff --git a/src/chat_sdk/adapters/teams/adapter.py b/src/chat_sdk/adapters/teams/adapter.py index 7476a35..53d13e2 100644 --- a/src/chat_sdk/adapters/teams/adapter.py +++ b/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, diff --git a/tests/test_slack_client_cache.py b/tests/test_slack_client_cache.py index cd62d9e..4d98d5a 100644 --- a/tests/test_slack_client_cache.py +++ b/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