Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ jobs:
- name: Format check
run: uv run ruff format --check src/
- name: Run tests
run: uv run pytest tests/ -q --tb=short
run: uv run pytest tests/ -q --tb=short --cov=chat_sdk --cov-fail-under=50
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -69,5 +69,6 @@ dev = [
"cryptography>=42.0",
"pytest>=8.0",
"pytest-asyncio>=0.23.0",
"pytest-cov>=5.0",
"ruff>=0.4.0",
]
56 changes: 41 additions & 15 deletions src/chat_sdk/adapters/slack/adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import os
import re
import time
from collections import OrderedDict
from collections.abc import AsyncIterable, Awaitable, Callable
from contextvars import ContextVar
from datetime import UTC, datetime
Expand Down Expand Up @@ -197,8 +198,9 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None:
# Channel external/shared cache
self._external_channels: set[str] = set()

# Cache of AsyncWebClient instances keyed by bot token
self._client_cache: dict[str, Any] = {}
# 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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The maximum cache size for Slack clients is hardcoded to 100. In large-scale multi-workspace deployments, this limit might be reached frequently, leading to cache churn and unnecessary session recreations. Consider making this value configurable via SlackAdapterConfig to allow users to tune it based on their needs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in PR #13 -- added client_cache_max: int | None to SlackAdapterConfig (defaults to 100). Users can now tune the LRU cache size for their deployment scale.


# Multi-workspace OAuth fields
self._client_id: str | None = config.client_id or (os.environ.get("SLACK_CLIENT_ID") if zero_config else None)
Expand Down Expand Up @@ -264,18 +266,35 @@ def _get_client(self, token: str | None = None) -> Any:
Clients are cached by token so we avoid creating a new instance on
every request. The import is deferred so that ``slack_sdk`` is only
required at call-time.

When *token* is explicitly passed (even as ``""``) it is used as-is;
only when *token* is ``None`` do we fall back to ``_get_token()``.
"""
resolved_token = token or self._get_token()
cached = self._client_cache.get(resolved_token)
if cached is not None:
return cached
resolved_token = self._get_token() if token is None else token

if resolved_token in self._client_cache:
self._client_cache.move_to_end(resolved_token)
return self._client_cache[resolved_token]

from slack_sdk.web.async_client import AsyncWebClient

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
Comment on lines +286 to +291
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Closing the session of an evicted client using create_task is risky in a concurrent environment. Since AsyncWebClient instances are cached and shared, an eviction triggered by one request (e.g., when a new workspace token is added) will terminate the underlying connection pool for any other concurrent requests that are still using the evicted client instance. This will lead to RuntimeError or connection failures in those requests.

A safer approach would be to share a single aiohttp.ClientSession across all AsyncWebClient instances (by passing it to the constructor), or to rely on garbage collection for the sessions of evicted clients to ensure they are only closed when no longer in use.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in PR #13 -- removed the create_task(evicted_client.session.close()) call. The evicted client's aiohttp session will now be cleaned up by the garbage collector once all references are released, avoiding the race condition with concurrent requests.

return client

def _invalidate_client(self, token: str) -> None:
"""Remove a cached client (e.g., on token revocation)."""
self._client_cache.pop(token, None)

# ------------------------------------------------------------------
# Initialization
# ------------------------------------------------------------------
Expand Down Expand Up @@ -2670,17 +2689,24 @@ def _handle_slack_error(self, error: Any) -> None:
Never returns (always raises).
"""
slack_error = error
code = getattr(slack_error, "response", {})
if isinstance(code, dict):
code.get("error")
else:
getattr(getattr(slack_error, "response", None), "get", lambda *a: None)("error")
resp = getattr(slack_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")

# Invalidate cached client on auth errors (token revocation / invalid_auth)
if error_code in ("invalid_auth", "token_revoked", "account_inactive"):
try:
token = self._get_token()
self._invalidate_client(token)
except AuthenticationError:
pass

# Check for rate limiting
if hasattr(slack_error, "response"):
resp = slack_error.response
if isinstance(resp, dict) and resp.get("error") == "ratelimited":
raise AdapterRateLimitError("slack") from error
if isinstance(resp, dict) and error_code == "ratelimited":
raise AdapterRateLimitError("slack") from error
Comment on lines +2708 to +2709
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The rate limit check is currently broken for standard Slack API errors. When using slack_sdk, the resp object is typically a SlackResponse instance, which is not a dict, so the isinstance(resp, dict) check will fail and the specialized AdapterRateLimitError will not be raised. Since error_code has already been safely extracted above (handling both dicts and SlackResponse objects), you should rely on error_code directly for this check.

Suggested change
if isinstance(resp, dict) and error_code == "ratelimited":
raise AdapterRateLimitError("slack") from error
# Check for rate limiting
if error_code == "ratelimited":
raise AdapterRateLimitError("slack") from error

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in PR #13 -- _handle_slack_error now correctly handles SlackResponse objects by checking resp.data (dict) first, falling back to isinstance(resp, dict). The rate limit check no longer requires the response to be a dict. Also now extracts the Retry-After header from resp.headers and passes it to AdapterRateLimitError.


raise error # type: ignore[misc]

Expand Down
Loading