Skip to content

Commit a7f5b53

Browse files
committed
fix(slack): evict web_client cache on invalidation + clarify caching test (review)
Two review follow-ups on the web_client port: - Gemini: `_invalidate_client` cleared only the async `_client_cache`, leaving stale/revoked synchronous `WebClient` instances in `_web_client_cache` on token revocation / auth-error eviction. Pop the token from both caches. New `test_invalidate_client_clears_web_client_cache` is load-bearing. - github-code-quality: `assert adapter.web_client is adapter.web_client` tripped "comparison of identical values". The test is a genuine caching check (the property is invoked twice), but binding each access to a name makes that intent explicit and silences the false-positive. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
1 parent d0692a4 commit a7f5b53

2 files changed

Lines changed: 23 additions & 1 deletion

File tree

src/chat_sdk/adapters/slack/adapter.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -808,6 +808,7 @@ def _get_client(self, token: str | None = None) -> Any:
808808
def _invalidate_client(self, token: str) -> None:
809809
"""Remove a cached client (e.g., on token revocation)."""
810810
self._client_cache.pop(token, None)
811+
self._web_client_cache.pop(token, None)
811812

812813
def _get_web_client_for_token(self, token: str) -> Any:
813814
"""Return a synchronous ``slack_sdk.WebClient`` for *token*, cached.

tests/test_slack_web_client.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,13 @@ def test_returns_same_instance_per_token(self):
109109
"""Repeated access returns the exact same cached object (identity)."""
110110
adapter = _single_workspace_adapter()
111111

112-
assert adapter.web_client is adapter.web_client
112+
# Bind each property access to a name so the identity check reads as a
113+
# caching assertion (two calls → same object), not a tautological
114+
# ``x is x`` self-comparison.
115+
first = adapter.web_client
116+
second = adapter.web_client
117+
118+
assert first is second
113119

114120
def test_caches_under_the_resolved_token(self):
115121
"""The cached client is keyed by the resolved token."""
@@ -119,6 +125,21 @@ def test_caches_under_the_resolved_token(self):
119125

120126
assert adapter._web_client_cache["xoxb-cache-key"] is client
121127

128+
def test_invalidate_client_clears_web_client_cache(self):
129+
"""``_invalidate_client`` evicts the sync WebClient too (token revocation).
130+
131+
Load-bearing: without the ``_web_client_cache.pop`` in
132+
``_invalidate_client`` a revoked token's stale ``WebClient`` would
133+
survive eviction and this assertion fails.
134+
"""
135+
adapter = _single_workspace_adapter("xoxb-revoke")
136+
client = adapter.web_client
137+
assert adapter._web_client_cache["xoxb-revoke"] is client
138+
139+
adapter._invalidate_client("xoxb-revoke")
140+
141+
assert "xoxb-revoke" not in adapter._web_client_cache
142+
122143

123144
# ---------------------------------------------------------------------------
124145
# Deprecated ``client`` alias

0 commit comments

Comments
 (0)