fix(slack): bound all per-message tracking structures with oldest-first eviction - #69484
Merged
Conversation
_resolve_user_name() caches every resolved (team_id, user_id) → display name but never evicts entries. On a long-running bot in a large workspace every unique user the bot encounters adds a permanent entry. Sibling structures _bot_message_ts (BOT_TS_MAX=5000) and _assistant_threads (ASSISTANT_THREADS_MAX=5000) already have caps; _user_name_cache had none. Fix: add _USER_NAME_CACHE_MAX = 5000 and evict the oldest half on overflow after each write, matching the existing sibling pattern. Consolidates the two cache-write branches (success + API error) into a single write so eviction runs once per resolution. Reapplied from #23676 onto the current adapter (cache moved to plugins/platforms/slack/adapter.py and is now keyed by (team_id, user_id) tuples for multi-workspace safety).
_fetch_thread_context() caches per-thread Slack history under a 60-second TTL, but expired entries are never removed. On the current adapter this is worse than when first reported: each entry now also retains the raw conversations.replies payloads (messages) for watermark re-formatting, so a busy multi-channel workspace accumulates full message lists forever. _bot_message_ts, _mentioned_threads, and _assistant_threads all enforce a MAX constant with active eviction in the same __init__; _thread_context_cache had the TTL declared but no matching eviction path. Fix: add _THREAD_CACHE_MAX = 2500 and purge expired entries (fetched_at older than _THREAD_CACHE_TTL) whenever a new write pushes the cache past the limit. TTL-based eviction is used instead of LRU because fresh entries are still needed; evicting them would immediately re-trigger a rate-limited conversations.replies call. Adds two unit tests: one verifying stale entries are purged on overflow, one verifying fresh entries survive. Reapplied from #23375 onto the current adapter (moved to plugins/platforms/slack/adapter.py; cache entries now carry raw message payloads and parent_user_id).
…with oldest-first eviction Widening pass over the whole adapter following the cluster-C16 audit (#51019, #51097, #23676, #23375): every in-memory structure that accumulates per-message, per-user, or per-thread state is now bounded, and every eviction is oldest-first — never arbitrary set-iteration order, which is the #51019 failure mode (bot silently going quiet on the most ACTIVE thread because set.pop-order eviction removed it). Newly bounded: - _approval_resolved / _clarify_resolved (caps 1000): unclicked approval/clarify prompts leaked their double-click-guard entries forever; oldest-insertion eviction via _trim_oldest_dict_entries. - _reacting_message_ids (cap 5000): reaction lifecycle entries leaked when an exception fired between add and finalize; oldest-ts eviction. - _active_status_threads (cap 1000): statuses abandoned by error paths accumulated; oldest-thread-ts eviction so the newest live status is never cleared. - _channel_team (cap 10000): grew with every DM channel the bot ever saw (DM channel IDs are per-user). All four write sites now route through _remember_channel_team; eviction is safe because entries are re-learned from the next event and _get_client falls back to the primary client. - _slash_command_contexts (cap 1000): TTL cleanup only ran on lookup, so contexts whose ephemeral replies never happened accumulated; overflow purges expired entries first, then oldest-stash-first. Converted from arbitrary set-order eviction to oldest-first: - _titled_assistant_threads: keys are (team, channel, thread_ts) — now evicts oldest thread first via _discard_oldest_by_thread_ts. - _thread_rehydration_checked: keys are team:channel:thread_ts[:user] — arbitrary eviction here would re-run an ACTIVE thread's restart rehydration check and re-inject the missed-delta context; now evicts oldest thread first. - _reacting_message_ids uses #51097's _discard_oldest_slack_timestamps. Deliberately NOT bounded (naturally tiny, per-workspace): _team_clients, _team_bot_user_ids, _team_bot_names (one entry per installed workspace), _assistant_threads / _agent_view_contexts / _bot_message_ts / _mentioned_threads / _user_name_cache / _thread_context_cache (already bounded), _dedup (MessageDeduplicator has max_size + TTL internally). New helpers: _trim_oldest_dict_entries (dicts preserve insertion order, so oldest-first is exact) and _discard_oldest_by_thread_ts (chronological sort on the embedded Slack ts for keyed sets). Tests: caps hold under churn, eviction removes OLDEST not arbitrary entries, newest/active entries survive eviction pressure (regression shape for #51019), plus end-to-end paths through _resolve_user_name and _handle_slash_command. Part of the C16 cache-bounds consolidation with #51097 (markoub), #23676 and #23375 (EloquentBrush). Fixes #51019.
EloquentBrush's noreply email already maps to AhmetArif0 in the frozen legacy AUTHOR_MAP (same account, renamed) — no new mapping needed.
Contributor
૮ >ﻌ< ა ci reviewrunning on 29b692f CI timingsCI timings · View jobWall time 8m47s vs 9m59s (-12.0%). 9 job(s) slower, 8 faster, 3 unchanged.
|
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Every per-message/per-user tracking structure in the Slack adapter is now bounded with oldest-first eviction — fixing both the memory leaks on busy workspaces and the #51019-class bug where arbitrary set eviction made the bot silently stop responding to active threads.
Fixes #51019.
Changes
_bot_message_ts/_mentioned_threads: oldest-timestamp-first eviction instead of arbitrary set order (fix(slack): evict oldest tracked thread timestamps #51097, reconciled with fix(slack): thread-context lifecycle — cold-start, restart rehydration, mention refresh, reply-wake #69320's session-scoped thread registration)._user_name_cache: capped 5000, oldest-half eviction (fix(slack): cap _user_name_cache to prevent unbounded growth #23676, reapplied to the workspace-keyed cache)._thread_context_cache: capped 2500 with TTL-expired purge on overflow (fix(slack): bound _thread_context_cache with eviction on overflow #23375 — growth was worse post-fix(slack): thread-context lifecycle — cold-start, restart rehydration, mention refresh, reply-wake #69320 since entries now carry raw message payloads)._approval_resolved/_clarify_resolved(unclicked prompts leaked),_reacting_message_ids(leak on exception),_active_status_threads,_channel_team(grew per DM, 10k cap via new evict-safe_remember_channel_team),_slash_command_contexts(no-reply contexts never purged),_titled_assistant_threads+_thread_rehydration_checked(arbitrary → oldest-first). Full audit table in the report; naturally-tiny per-workspace registries deliberately left unbounded.Credits
Salvaged with authorship preserved: #51097 (@markoub), #23676 + #23375 (@AhmetArif0, commits authored under their renamed account EloquentBrush — same person per the frozen legacy map).
Validation
tests/gateway/ -q -k slackInfographic