Skip to content

gateway/slack: coalesce same-user top-level channel follow-ups (+ elapsed typing heartbeat) - #45702

Closed
MrAbsaroka wants to merge 1 commit into
NousResearch:mainfrom
MrAbsaroka:fix/slack-channel-turn-coalescing
Closed

gateway/slack: coalesce same-user top-level channel follow-ups (+ elapsed typing heartbeat)#45702
MrAbsaroka wants to merge 1 commit into
NousResearch:mainfrom
MrAbsaroka:fix/slack-channel-turn-coalescing

Conversation

@MrAbsaroka

@MrAbsaroka MrAbsaroka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Problem

A rapid same-user top-level channel follow-up — e.g. "you there?" sent while a long turn (a multi-minute delegation) is still running — starts a concurrent turn instead of queueing behind the active one. It then answers a question the in-flight turn has already made moot:

t+0:   user:  <question requiring a long turn>     -> turn A starts
t+1m:  user:  you there?                            -> turn B starts concurrently
t+3m:  bot:   <answer to the question>              -> turn A posts
t+3m:  bot:   Yes, I'm here! What do you need?      -> turn B posts (now stale)

Root cause

Serialization already exists: BasePlatformAdapter.handle_message tracks _active_sessions and routes busy follow-ups through _busy_session_handler / _pending_messages, keyed by session_key. But with reply_in_thread=true (the default) the Slack adapter mints a fresh per-message session for top-level channel messages (thread_ts = ts), so two top-level messages get different session_keys — the busy handler never engages across them and the follow-up spawns a concurrent turn. Threaded replies coalesce correctly; channel-root follow-ups slip past. (Under reply_in_thread=false, #15421 already collapses the channel to one session, so this path simply no-ops.)

Fix

gateway/platforms/base.py — add an _active_topfollow index mapping (chat_id, user_id) → session_key of that user's in-flight top-level group/channel turn, kept in sync at the three canonical lifecycle points (_start_session_processing, _release_session_guard, _heal_stale_session_lock). In handle_message, a same-user top-level follow-up whose own per-message session is idle is rebound to the active sibling session_key, so it flows through the existing busy handler instead of racing.

Because it goes through the normal busy path, it inherits the right semantics for free:

Scope is deliberately tight: same user only (per-user session isolation); thread replies (detected via thread_id != message_id), other users, and DMs are untouched; bypass commands (/approve, /stop, …) and the clarify text-intercept keep their existing behavior.

gateway/platforms/slack.py — make the assistant typing/status heartbeat elapsed-aware ("still working… (2m03s)") instead of a static "is thinking...", so a long turn visibly advances. (A turn that visibly progresses doesn't provoke the mid-turn "you there?" that triggers the race in the first place.)

Relationship to #15421 / #15464

Complementary, not overlapping. #15421 added the reply_in_thread flag and fixes the reply_in_thread=false case (whole-channel single session). This PR fixes the default reply_in_thread=true case — where each top-level message is intentionally its own session/thread — and does so per user, so concurrent conversations from different people in the same channel still run in parallel.

Tests

tests/gateway/test_channel_turn_coalescing.py:

  • _topfollow_key gating (DM / thread-reply / missing-user → None; top-level channel → composite).
  • Same-user top-level follow-up coalesces into the active turn's pending queue (no second concurrent session).
  • Other-user and thread-reply messages do not coalesce.
  • Index cleared on _release_session_guard and on stale-lock heal.

All pass under the repo's pytest config; no regression in the surrounding gateway session/bypass/merge suites.

…ed heartbeat

A rapid same-user top-level channel follow-up (e.g. "you there?" sent while a
long turn runs) starts a concurrent turn instead of queueing, then answers a
question the in-flight turn just made moot.

Serialization already exists (handle_message tracks _active_sessions and routes
busy follow-ups through _busy_session_handler / _pending_messages per
session_key), but with reply_in_thread=true (the default) each top-level channel
message gets its own session (thread_ts = ts), so two top-level messages get
different session_keys and the busy handler never engages across them. (Under
reply_in_thread=false the sessions already share a key and this path no-ops.)

Fix:
- base.py: add an _active_topfollow index ((chat_id,user_id) -> active
  top-level session_key), synced by _start_session_processing /
  _release_session_guard / _heal_stale_session_lock. handle_message rebinds a
  same-user top-level follow-up to the active session_key so it flows through
  the normal busy handler — which queues/cascades after the active turn (and the
  runner's NousResearch#30170 interrupt->queue demotion keeps a running delegation from
  being aborted), or interrupts only a cheap turn. Same-user only (per-user
  session isolation); thread replies, other users, DMs, bypass commands and
  clarify-intercept untouched.
- slack.py: make the assistant typing/status heartbeat elapsed-aware ("still
  working... (2m03s)") so a long turn visibly advances instead of a static
  "is thinking..." that reads as stuck (and provokes the "you there?" ping).

Tests: tests/gateway/test_channel_turn_coalescing.py.
@MrAbsaroka
MrAbsaroka force-pushed the fix/slack-channel-turn-coalescing branch from 17fb478 to 421b7af Compare June 13, 2026 15:46
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter labels Jun 13, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Thanks for this well-structured change. Verified the coalescing logic and it holds up:

  1. Scope guard correctness: _topfollow_key() correctly returns None for DMs, thread replies (where thread_id != message_id), and messages without user ids — so the coalescing only fires for the exact scenario described (per-message top-level sessions in Slack reply_in_thread=true mode). Thread replies are untouched.

  2. Stale session check: The _session_task_is_stale(_sibling) guard before rebinding prevents coalescing into a session whose task has already finished/cancelled but hasn't been cleaned up yet. Good defensive check.

  3. Cleanup symmetry: _drop_topfollow_for(session_key) is called in _release_session_guard, _heal_stale_session_lock, and the index entry is only set in _start_session_processing. No path leaks entries.

  4. Heartbeat UX: The elapsed-time status label uses time.monotonic() correctly (immune to wall-clock adjustments). The _status_started dict is cleaned in stop_typing alongside _active_status_threads.

The 217-line test file covers the key scenarios (same-user coalescing, different users, thread replies, stale sibling). Clean implementation.

@teknium1 teknium1 left a comment

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.

Thanks for isolating a real Slack concurrency gap. Current main still assigns thread_ts = ts to default top-level channel events (plugins/platforms/slack/adapter.py:2806-2809) and forwards it as SessionSource.thread_id (plugins/platforms/slack/adapter.py:3163-3170), so the premise remains valid.

Problems

  • gateway/platforms/base.py:3967 performs sibling rebinding before the active-session bypass check at :3999. A same-user top-level /stop, /new, /approve, or clarify reply can therefore act on the sibling session, despite the stated scope.
  • gateway/platforms/slack.py:1302 stores elapsed status timing per chat_id, although the change preserves concurrent turns for different users in the same channel. One thread can inherit another's elapsed value or clear its status.
  • Slack moved to plugins/platforms/slack/adapter.py in 5600105478ffde29d7566b45421b100eaa29c4ef; this is why the current PR is conflicting.

Suggested changes

  • Port the status change to the plugin and key timing/status by (chat_id, thread_ts).
  • Exempt command and clarify interception before coalescing, with regressions for both paths.
  • Exercise the real Slack event construction in the coalescing test.

Automated hermes-sweeper review.

Comment thread gateway/platforms/base.py
# in-flight turn is about to make moot. Same user only (per-user session
# isolation); thread replies and other users are untouched, and under
# reply_in_thread=false the keys already match so this never fires.
if session_key not in self._active_sessions:

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.

This rebind runs before the command bypass at line 3999 and the clarify interception below it. A top-level same-user /stop, /new, /approve, or clarify response is therefore treated as belonging to the sibling active session. Exempt those paths before rebinding and add regressions.

# progress for a long turn (e.g. a multi-minute delegation). A turn that
# visibly advances doesn't provoke a mid-turn "you there?" — which would
# otherwise race the in-flight answer in its own session.
started = self._status_started.get(chat_id)

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.

This timer is keyed only by chat_id, but different users' top-level channel turns are intentionally still concurrent. Their status refreshes can share elapsed time, and either stop_typing(chat_id) clears the shared state. Key status/timing by (chat_id, thread_ts) and test concurrent threads.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 2026
teknium1 pushed a commit that referenced this pull request Jul 23, 2026
…ong turns

Salvaged from PR #45702 (heartbeat half). A multi-minute turn showed a
static 'is thinking...' assistant status that reads as stuck and provokes
mid-turn 'you there?' pings. Derive the fallback status label from the
turn's elapsed time (>=30s → 'still working… (NmSSs)'), riding the
existing _keep_typing refresh — zero extra API calls.

Ported onto the current plugin adapter: the start time rides the tracked
_active_status_threads entry (workspace-scoped key), so it shares the
existing bounds/eviction and resets when stop_typing clears the status.
Explicit live-status phrases (set_status_text) and configured
typing_status_text always win; only the built-in default label changes.

The PR's other half (top-level channel follow-up coalescing) is NOT
included — dispatch semantics changed on main (busy-input active-turn
redirect, #30170 demotion) and need a fresh design pass.

Refs #45702. Co-authored-by: MrAbsaroka <mrabsaroka@gmail.com>
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #70197 (merged): the elapsed typing-heartbeat half landed with credit; the coalescing half overlaps the merged C10 gating and was dropped with rationale.

Thanks for the work — it's credited in #70197's summary.

@teknium1 teknium1 closed this Jul 23, 2026
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…ong turns

Salvaged from PR NousResearch#45702 (heartbeat half). A multi-minute turn showed a
static 'is thinking...' assistant status that reads as stuck and provokes
mid-turn 'you there?' pings. Derive the fallback status label from the
turn's elapsed time (>=30s → 'still working… (NmSSs)'), riding the
existing _keep_typing refresh — zero extra API calls.

Ported onto the current plugin adapter: the start time rides the tracked
_active_status_threads entry (workspace-scoped key), so it shares the
existing bounds/eviction and resets when stop_typing clears the status.
Explicit live-status phrases (set_status_text) and configured
typing_status_text always win; only the built-in default label changes.

The PR's other half (top-level channel follow-up coalescing) is NOT
included — dispatch semantics changed on main (busy-input active-turn
redirect, NousResearch#30170 demotion) and need a fresh design pass.

Refs NousResearch#45702. Co-authored-by: MrAbsaroka <mrabsaroka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/slack Slack app adapter sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants