fix(slack): track assistant typing per-thread to unstick concurrent chats#24139
fix(slack): track assistant typing per-thread to unstick concurrent chats#24139briandevans wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a Slack adapter lifecycle bug where concurrent Assistant threads in the same channel/DM could overwrite each other’s tracked typing/status thread, leaving older threads stuck showing “is thinking…”. The change updates tracking to be per-(chat, thread) and ensures status cleanup happens for the correct thread, including on send failures.
Changes:
- Change
_active_status_threadsfrom a per-chat singlethread_tsto a per-chatset[thread_ts]to support concurrent Assistant threads. - Route
stop_typing()to clear a specific thread whenmetadata["thread_id"/"thread_ts"]is provided; otherwise clear all tracked threads for the chat (legacy behavior). - Update
send()to clear typing for its own resolvedthread_ts, and attempt cleanup on exceptions; add regression tests covering concurrency, targeted clearing, failure cleanup, and per-chat bounding.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
gateway/platforms/slack.py |
Implements per-thread status tracking/clearing and adds best-effort cleanup on send failure. |
tests/gateway/test_slack.py |
Updates existing tests for the new tracking shape and adds focused regression coverage for concurrent-thread scenarios and bounds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tracked.discard(ts) | ||
| try: | ||
| await client.assistant_threads_setStatus( | ||
| channel_id=chat_id, | ||
| thread_ts=ts, | ||
| status="", | ||
| ) |
Address Copilot review on NousResearch#24139: stop_typing previously discarded the thread from _active_status_threads before calling assistant_threads_setStatus, so a transient API failure removed the tracking entry while Slack's UI still showed "is thinking…", with no way for a later stop_typing call to retry the clear. Reorder so the discard only happens after a successful clear; on failure the thread stays tracked and a subsequent stop_typing (e.g. the next finalize, or a bulk clear without metadata) gets another chance to clear it. The exception is still swallowed, so a permanent failure (missing assistant:write scope) doesn't surface to callers. Updated test_stop_typing_handles_api_error_gracefully to assert the new contract (thread stays tracked on failure). Added test_stop_typing_partial_failure_keeps_failed_thread_tracked covering the mixed success/failure case where one thread clears and another fails — succeeding threads are discarded, failing ones stay. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot Addressed in c604e8c — Updated tests:
|
|
CI audit — all 3 failing checks on this PR are pre-existing baselines on clean
Happy to address the Copilot finding (already fixed in |
…hats
A single Slack channel / DM can host multiple Assistant threads at the
same time (overlapping user requests, parallel sessions, etc.). The
typing-indicator tracker was keyed by chat_id alone:
self._active_status_threads: Dict[str, str] = {}
so the second concurrent `send_typing` for the same chat overwrote the
first thread's `thread_ts`. When the earlier thread's send finished,
`stop_typing` popped the wrong (newer) `thread_ts` — and the older
thread stayed stuck in "is thinking…" forever, as reported in NousResearch#24117.
The existing fix landed via NousResearch#18553 (April 22) covered the single-thread
lifecycle (set status on send_typing, clear on send / edit-finalize).
This PR extends that lifecycle to concurrent threads:
- Track per-thread instead of per-chat: `Dict[str, set[str]]`.
- `send_typing` adds the thread_ts to the chat's set (bounded at 128
active threads per chat as a defensive guard against missed clears).
- `stop_typing(chat_id, metadata={"thread_ts": ts})` clears that
specific thread. Without metadata, every tracked thread in the chat
is cleared (preserves the legacy base.py wiring that has no thread
context).
- `send()` finalize now passes its own `thread_ts` to `stop_typing` so
it clears only its own thread among concurrent ones.
- `send()` exception path now also clears the thread's status, so a
failed Slack post doesn't leave the user staring at "is thinking…".
- `edit_message(finalize=True)` keeps its existing behavior (clears
whatever's tracked for the chat — it has no thread context to
target a specific thread).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Copilot review on NousResearch#24139: stop_typing previously discarded the thread from _active_status_threads before calling assistant_threads_setStatus, so a transient API failure removed the tracking entry while Slack's UI still showed "is thinking…", with no way for a later stop_typing call to retry the clear. Reorder so the discard only happens after a successful clear; on failure the thread stays tracked and a subsequent stop_typing (e.g. the next finalize, or a bulk clear without metadata) gets another chance to clear it. The exception is still swallowed, so a permanent failure (missing assistant:write scope) doesn't surface to callers. Updated test_stop_typing_handles_api_error_gracefully to assert the new contract (thread stays tracked on failure). Added test_stop_typing_partial_failure_keeps_failed_thread_tracked covering the mixed success/failure case where one thread clears and another fails — succeeding threads are discarded, failing ones stay. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c604e8c to
283f4a3
Compare
Summary
chat_id, so a second concurrentsend_typingoverwrote the first thread'sthread_tsand the earlier thread was left stuck in "is thinking…" forever.stop_typingbymetadata["thread_ts"]when present, and also clears the indicator whensend()fails.The bug
gateway/platforms/slack.pytracked the active Assistant status thread per chat:send_typing(chat_id, metadata)wroteself._active_status_threads[chat_id] = thread_ts, so two overlapping Assistant requests in the same Slack channel/DM collided.stop_typing(chat_id, ...)then popped the dict and cleared whicheverthread_tswas last written — usually the wrong (newer) one. The result: the older Slack Assistant thread stayed inis thinking…indefinitely even though Hermes had already replied to it. Reported in #24117.A related failure path: the
send()exception branch did not clear the typing indicator at all, so a Slack post error left the user staring atis thinking…until the gateway restarted.The fix
_active_status_threads: Dict[str, set[str]]— track each active thread.send_typingaddsthread_tsto the chat's set (bounded at 128 active threads per chat as a defensive guard against missed clears).stop_typing(chat_id, metadata={"thread_ts": ts})clears exactly that thread. Called without metadata, it clears every tracked thread for the chat (preserves the legacybase.pywiring that callsstop_typing(chat_id)without thread context).send()finalize now passes its ownthread_tstostop_typingso it only clears its own thread among concurrent ones.send()exception path now also clears the thread's status, so a failed Slack post doesn't leave the user stuck onis thinking….edit_message(finalize=True)is unchanged — it has no thread context to target, and clearing every tracked thread for the chat matches the existing behavior.Test plan
tests/gateway/test_slack.py::TestSendTyping— 16/16 passing including five new tests covering concurrent threads, per-threadstop_typingrouting, the legacy clear-all path, send-failure clearing, and the per-chat bound.tests/gateway/test_slack.py(186 passed), plustest_slack_approval_buttons.py/test_slack_channel_skills.py/test_slack_mention.py(89 passed).ruff check gateway/platforms/slack.py tests/gateway/test_slack.pyclean.Regression guard — the two new tests fail against current
main:test_send_typing_tracks_multiple_concurrent_threads— asserts boththread_aandthread_bare tracked after twosend_typingcalls in the same chat. With the oldDict[str, str]shape, the second overwrites the first.test_send_clears_only_targeted_thread_among_concurrent— asserts thatsend(metadata={"thread_id": "thread_a"})clears onlythread_aand leavesthread_btracked. The old code popped the dict and cleared whichever single thread happened to be stored.Related
edit_messagepath is intentionally left clearing-all-by-chat because it has no per-message thread context. Happy to widen if a maintainer prefers carrying thread_ts throughedit_message's call sites.