From 4fb1796331deba07b691148b2967effc4d5c90ef Mon Sep 17 00:00:00 2001 From: markoub <2418548+markoub@users.noreply.github.com> Date: Tue, 23 Jun 2026 01:25:34 +0200 Subject: [PATCH] fix(slack): evict oldest tracked thread timestamps --- plugins/platforms/slack/adapter.py | 58 ++++++++++++++------ tests/gateway/test_slack_approval_buttons.py | 51 ++++++++++------- 2 files changed, 74 insertions(+), 35 deletions(-) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 61240bc9eb16..46e5addec872 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -742,11 +742,11 @@ class SlackAdapter(BasePlatformAdapter): self._clarify_resolved: Dict[str, bool] = {} # Track timestamps of messages sent by the bot so we can respond # to thread replies even without an explicit @mention. - self._bot_message_ts: set = set() + self._bot_message_ts: set[str] = set() self._BOT_TS_MAX = 5000 # cap to avoid unbounded growth # Track threads where the bot has been @mentioned — once mentioned, # respond to ALL subsequent messages in that thread automatically. - self._mentioned_threads: set = set() + self._mentioned_threads: set[str] = set() self._MENTIONED_THREADS_MAX = 5000 # Assistant thread metadata keyed by (team_id, channel_id, thread_ts). # Slack's AI Assistant lifecycle events can arrive before/alongside @@ -827,6 +827,43 @@ class SlackAdapter(BasePlatformAdapter): await result break + @staticmethod + def _slack_timestamp_sort_key(ts: str) -> Tuple[int, int, str]: + """Return a chronological, deterministic sort key for Slack timestamps.""" + seconds, _, fraction = str(ts).partition(".") + try: + seconds_int = int(seconds) + except ValueError: + seconds_int = 0 + try: + fraction_int = int((fraction + "000000")[:6] or "0") + except ValueError: + fraction_int = 0 + return seconds_int, fraction_int, str(ts) + + @classmethod + def _discard_oldest_slack_timestamps( + cls, timestamps: set[str], count: int + ) -> None: + """Discard the oldest Slack timestamps from a bounded tracking set.""" + if count <= 0: + return + for old_ts in sorted(timestamps, key=cls._slack_timestamp_sort_key)[:count]: + timestamps.discard(old_ts) + + def _trim_bot_message_timestamps(self) -> None: + if len(self._bot_message_ts) <= self._BOT_TS_MAX: + return + excess = len(self._bot_message_ts) - self._BOT_TS_MAX // 2 + self._discard_oldest_slack_timestamps(self._bot_message_ts, excess) + + def _trim_mentioned_threads(self) -> None: + if len(self._mentioned_threads) <= self._MENTIONED_THREADS_MAX: + return + self._discard_oldest_slack_timestamps( + self._mentioned_threads, self._MENTIONED_THREADS_MAX // 2 + ) + def _start_socket_mode_handler(self) -> None: """Start the Slack Socket Mode background task.""" if not self._app or not self._app_token: @@ -2002,10 +2039,7 @@ class SlackAdapter(BasePlatformAdapter): # Also register the thread root so replies-to-my-replies work if thread_ts: self._bot_message_ts.add(thread_ts) - if len(self._bot_message_ts) > self._BOT_TS_MAX: - excess = len(self._bot_message_ts) - self._BOT_TS_MAX // 2 - for old_ts in list(self._bot_message_ts)[:excess]: - self._bot_message_ts.discard(old_ts) + self._trim_bot_message_timestamps() return SendResult( success=True, @@ -2545,10 +2579,7 @@ class SlackAdapter(BasePlatformAdapter): if not thread_ts: return self._bot_message_ts.add(thread_ts) - if len(self._bot_message_ts) > self._BOT_TS_MAX: - excess = len(self._bot_message_ts) - self._BOT_TS_MAX // 2 - for old_ts in list(self._bot_message_ts)[:excess]: - self._bot_message_ts.discard(old_ts) + self._trim_bot_message_timestamps() def _is_retryable_upload_error(self, exc: Exception) -> bool: """Best-effort detection for transient Slack upload failures.""" @@ -3772,12 +3803,7 @@ class SlackAdapter(BasePlatformAdapter): if not thread_ts: return self._mentioned_threads.add(thread_ts) - if len(self._mentioned_threads) > self._MENTIONED_THREADS_MAX: - to_remove = list(self._mentioned_threads)[ - : self._MENTIONED_THREADS_MAX // 2 - ] - for t in to_remove: - self._mentioned_threads.discard(t) + self._trim_mentioned_threads() async def _bot_authored_thread_root( self, channel_id: str, thread_ts: str, team_id: str = "" diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index e848abb6e2f1..f1427d8b8c57 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -1109,18 +1109,21 @@ class TestThreadEngagement: # Thread root should also be tracked assert "8000.0" in adapter._bot_message_ts - @pytest.mark.asyncio - async def test_bot_message_ts_cap(self): - """Verify memory is bounded when many messages are sent.""" + def test_bot_message_ts_cap_evicts_oldest_timestamps(self): + """Bot thread tracking evicts the oldest Slack timestamps first.""" adapter = _make_adapter() - adapter._BOT_TS_MAX = 10 # low cap for testing - mock_client = adapter._team_clients["T1"] + adapter._BOT_TS_MAX = 4 - for i in range(20): - mock_client.chat_postMessage = AsyncMock(return_value={"ts": f"{i}.0"}) - await adapter.send(chat_id="C1", content=f"msg {i}") + for ts in [ + "1000.000002", + "999.999999", + "1000.000004", + "1000.000001", + "1000.000003", + ]: + adapter._record_uploaded_file_thread("C1", ts) - assert len(adapter._bot_message_ts) <= 10 + assert adapter._bot_message_ts == {"1000.000003", "1000.000004"} def test_mentioned_threads_populated_on_mention(self): """When bot is @mentioned in a thread, that thread is tracked.""" @@ -1129,14 +1132,24 @@ class TestThreadEngagement: adapter._mentioned_threads.add("1000.0") assert "1000.0" in adapter._mentioned_threads - def test_mentioned_threads_cap(self): - """Verify _mentioned_threads is bounded.""" + def test_mentioned_threads_cap_evicts_oldest_timestamps(self): + """Mentioned-thread tracking evicts the oldest Slack timestamps first.""" adapter = _make_adapter() - adapter._MENTIONED_THREADS_MAX = 10 - for i in range(15): - adapter._mentioned_threads.add(f"{i}.0") - if len(adapter._mentioned_threads) > adapter._MENTIONED_THREADS_MAX: - to_remove = list(adapter._mentioned_threads)[:adapter._MENTIONED_THREADS_MAX // 2] - for t in to_remove: - adapter._mentioned_threads.discard(t) - assert len(adapter._mentioned_threads) <= 10 + adapter._MENTIONED_THREADS_MAX = 4 + adapter._mentioned_threads.update( + { + "1000.000002", + "999.999999", + "1000.000004", + "1000.000001", + "1000.000003", + } + ) + + adapter._trim_mentioned_threads() + + assert adapter._mentioned_threads == { + "1000.000002", + "1000.000003", + "1000.000004", + }