Skip to content

fix(discord): stop the typing indicator sticking on the stale-result path - #71471

Open
Kyzcreig wants to merge 1 commit into
NousResearch:mainfrom
ANG-Ventures:up/discord-typing-loop
Open

fix(discord): stop the typing indicator sticking on the stale-result path#71471
Kyzcreig wants to merge 1 commit into
NousResearch:mainfrom
ANG-Ventures:up/discord-typing-loop

Conversation

@Kyzcreig

Copy link
Copy Markdown
Contributor

fix(discord): a finished typing loop must not deregister a newer turn's loop

Symptom on current main

A Discord "is typing…" bubble stays lit indefinitely after a turn has been
fully delivered. Nothing clears it — not the turn's own completion, not
/stop, not the gateway's error path — because the loop still POSTing it is no
longer reachable from the adapter. It only disappears if an unrelated later turn
happens to arm and stop a fresh loop on the same channel.

No rate limiting is involved; this is a plain check-then-act race across
teardown, and it needs only two turns overlapping on one channel.

Deterministic reproduction against the real adapter (the first two tests in the
new file):

turn A arms  → _typing_tasks["chan"] = A
turn A ends  → A.cancel(); slot released     (A has NOT yet run its finally)
turn B arms  → _typing_tasks["chan"] = B
A's finally runs → _typing_tasks.pop("chan")  ← pops **B's** entry
B still running, registry empty:
    await adapter.stop_typing("chan")   → finds nothing, cancels nothing
    assert task_b.done()                → AssertionError: still running
    typing POSTs continue forever

Exact line where it manifests

plugins/platforms/discord/adapter.py:4784, in send_typing()'s _typing_loop:

            except asyncio.CancelledError:
                pass
            finally:
                self._typing_tasks.pop(chat_id, None)   # ← unconditional

        self._typing_tasks[chat_id] = asyncio.create_task(_typing_loop())

The finally clears _typing_tasks[chat_id] whatever it currently holds
including a task belonging to a different, still-running turn.

Root cause

Cancellation in asyncio is not instantaneous. task.cancel() only schedules the
CancelledError; the coroutine reaches its finally when the event loop next
runs it. In the window between those two moments the next turn can legitimately
arm a fresh loop for the same channel — and it will, because the duplicate-guard
at :4750 (if chat_id in self._typing_tasks: return) sees an empty slot.

When the old loop finally unwinds it pops the entry the new loop installed.
That leaves an orphan with two properties that make it permanent:

  1. Its owner cannot stop it. stop_typing() (:4790) is
    self._typing_tasks.pop(chat_id, None) — an empty slot means it cancels
    nothing and returns successfully.
  2. Nothing else blocks it. The duplicate-guard is also empty, so the orphan
    keeps POSTing /channels/{id}/typing every 12s indefinitely.

The generic backstop in gateway/platforms/base.py doesn't help: it cancels the
_keep_typing refresh task and calls stop_typing(), and stop_typing() is
exactly the call that has been disarmed.

The fix

Clear the registry entry only when we are still the task it holds:

            finally:
                if self._typing_tasks.get(chat_id) is asyncio.current_task():
                    self._typing_tasks.pop(chat_id, None)

An earlier turn's cleanup can no longer release a later turn's slot, so the
newer loop stays reachable and its owner's stop_typing() works.

Deliberately minimal: one condition, no new state, no new field, no signature
change. send_typing and stop_typing keep their exact contracts; the
duplicate-guard, the 429 backoff and the 12s cadence are untouched.

Over-reach guard. The obvious failure mode of an ownership check is leaking
the entry — a loop that is the owner when it exits must still clear its slot,
or the duplicate-guard would suppress every later indicator on that channel.
asyncio.current_task() is exactly the identity being compared, so the normal
path is unaffected; a dedicated test pins it, plus a control for the ordinary
single-turn arm→stop sequence.

Whole bug class — sibling call paths

Discord is the only adapter with this pattern. It is the sole place in the
tree that stores a per-channel task in _typing_tasks and clears it from
inside the task itself:

$ grep -rn '_typing_tasks\.pop\|_typing_tasks\[' --include=*.py . | grep -v ^./tests
plugins/platforms/discord/adapter.py:4791   ← the finally (this fix)
plugins/platforms/discord/adapter.py:4793   ← the registration
plugins/platforms/discord/adapter.py:4797   ← stop_typing
gateway/platforms/signal.py:1518            ← stop only

Signal's _typing_tasks is only ever read/popped from outside: its
send_typing (gateway/platforms/signal.py:1125) is a one-shot RPC with no
loop and never assigns into the dict, so there is no equivalent path. Every
other adapter uses one-shot typing or the generic _keep_typing refresh, both
of which own their task in the caller's frame.

Test evidence

Four new tests in tests/gateway/test_discord_typing_loop_ownership.py, driving
the real DiscordAdapter.send_typing / stop_typing (constructed via
object.__new__, the pattern already used by
tests/gateway/test_discord_race_polish.py):

test asserts
test_later_turns_loop_survives_an_earlier_turns_teardown after the overlap, the registry still points at the newer loop
test_stop_typing_actually_stops_the_bubble_after_an_overlap the user-visible contract: stop_typing terminates the loop and no further typing POSTs are issued
test_a_loop_that_ends_on_its_own_still_releases_the_channel over-reach guard: the owner still clears its slot, and a later turn can arm again
test_stop_typing_clears_the_registry_for_the_normal_single_turn_path control: the ordinary non-overlapping case is unchanged

Per "Behavior contracts over snapshots", the headline test asserts the
observable outcome — the indicator stops POSTing — rather than mock call
counts or internal dict contents. The race is driven deterministically by
asyncio.sleep(0) scheduling, not by wall-clock timing, so the tests are not
flaky.

# fix applied
$ pytest tests/gateway/test_discord_typing_loop_ownership.py -q
4 passed

# RED proof — ownership check removed, tests kept
$ pytest tests/gateway/test_discord_typing_loop_ownership.py -q
2 failed, 2 passed
  FAILED ...::test_later_turns_loop_survives_an_earlier_turns_teardown
  FAILED ...::test_stop_typing_actually_stops_the_bubble_after_an_overlap
      AssertionError: the indicator loop must have terminated
        where False = <Task pending ... _typing_loop() running at adapter.py:4780>.done()

# no regression
$ pytest tests/gateway/ -q -k "typing"
138 passed, 1 skipped, 11042 deselected

Footprint

  • plugins/platforms/discord/adapter.py: +7 / −1 (one guarded clear, six lines
    of comment). No new state, no signature change, no behavioural surface beyond
    "the bubble now stops".
  • No new core tool, no new HERMES_* env var, no config key, no new dependency.

…rn's loop

## Symptom on current `main`

A Discord **"is typing…" bubble stays lit indefinitely** after a turn has been
fully delivered. Nothing clears it — not the turn's own completion, not
`/stop`, not the gateway's error path — because the loop still POSTing it is no
longer reachable from the adapter. It only disappears if an unrelated later turn
happens to arm *and* stop a fresh loop on the same channel.

No rate limiting is involved; this is a plain check-then-act race across
teardown, and it needs only two turns overlapping on one channel.

Deterministic reproduction against the real adapter (the first two tests in the
new file):

```
turn A arms  → _typing_tasks["chan"] = A
turn A ends  → A.cancel(); slot released     (A has NOT yet run its finally)
turn B arms  → _typing_tasks["chan"] = B
A's finally runs → _typing_tasks.pop("chan")  ← pops **B's** entry
B still running, registry empty:
    await adapter.stop_typing("chan")   → finds nothing, cancels nothing
    assert task_b.done()                → AssertionError: still running
    typing POSTs continue forever
```

## Exact line where it manifests

`plugins/platforms/discord/adapter.py:4784`, in `send_typing()`'s `_typing_loop`:

```python
            except asyncio.CancelledError:
                pass
            finally:
                self._typing_tasks.pop(chat_id, None)   # ← unconditional

        self._typing_tasks[chat_id] = asyncio.create_task(_typing_loop())
```

The `finally` clears `_typing_tasks[chat_id]` **whatever it currently holds** —
including a task belonging to a different, still-running turn.

## Root cause

Cancellation in asyncio is not instantaneous. `task.cancel()` only schedules the
`CancelledError`; the coroutine reaches its `finally` when the event loop next
runs it. In the window between those two moments the next turn can legitimately
arm a fresh loop for the same channel — and it will, because the duplicate-guard
at `:4750` (`if chat_id in self._typing_tasks: return`) sees an empty slot.

When the old loop finally unwinds it pops the entry the *new* loop installed.
That leaves an orphan with two properties that make it permanent:

1. **Its owner cannot stop it.** `stop_typing()` (`:4790`) is
   `self._typing_tasks.pop(chat_id, None)` — an empty slot means it cancels
   nothing and returns successfully.
2. **Nothing else blocks it.** The duplicate-guard is also empty, so the orphan
   keeps POSTing `/channels/{id}/typing` every 12s indefinitely.

The generic backstop in `gateway/platforms/base.py` doesn't help: it cancels the
`_keep_typing` **refresh task** and calls `stop_typing()`, and `stop_typing()` is
exactly the call that has been disarmed.

## The fix

Clear the registry entry only when we are still the task it holds:

```python
            finally:
                if self._typing_tasks.get(chat_id) is asyncio.current_task():
                    self._typing_tasks.pop(chat_id, None)
```

An earlier turn's cleanup can no longer release a later turn's slot, so the
newer loop stays reachable and its owner's `stop_typing()` works.

Deliberately minimal: one condition, no new state, no new field, no signature
change. `send_typing` and `stop_typing` keep their exact contracts; the
duplicate-guard, the 429 backoff and the 12s cadence are untouched.

**Over-reach guard.** The obvious failure mode of an ownership check is leaking
the entry — a loop that *is* the owner when it exits must still clear its slot,
or the duplicate-guard would suppress every later indicator on that channel.
`asyncio.current_task()` is exactly the identity being compared, so the normal
path is unaffected; a dedicated test pins it, plus a control for the ordinary
single-turn arm→stop sequence.

## Whole bug class — sibling call paths

Discord is the **only** adapter with this pattern. It is the sole place in the
tree that stores a per-channel task in `_typing_tasks` *and* clears it from
inside the task itself:

```
$ grep -rn '_typing_tasks\.pop\|_typing_tasks\[' --include=*.py . | grep -v ^./tests
plugins/platforms/discord/adapter.py:4791   ← the finally (this fix)
plugins/platforms/discord/adapter.py:4793   ← the registration
plugins/platforms/discord/adapter.py:4797   ← stop_typing
gateway/platforms/signal.py:1518            ← stop only
```

Signal's `_typing_tasks` is only ever read/popped from outside: its
`send_typing` (`gateway/platforms/signal.py:1125`) is a one-shot RPC with no
loop and never assigns into the dict, so there is no equivalent path. Every
other adapter uses one-shot typing or the generic `_keep_typing` refresh, both
of which own their task in the caller's frame.

## Test evidence

Four new tests in `tests/gateway/test_discord_typing_loop_ownership.py`, driving
the **real** `DiscordAdapter.send_typing` / `stop_typing` (constructed via
`object.__new__`, the pattern already used by
`tests/gateway/test_discord_race_polish.py`):

| test | asserts |
|---|---|
| `test_later_turns_loop_survives_an_earlier_turns_teardown` | after the overlap, the registry still points at the newer loop |
| `test_stop_typing_actually_stops_the_bubble_after_an_overlap` | **the user-visible contract**: `stop_typing` terminates the loop and no further typing POSTs are issued |
| `test_a_loop_that_ends_on_its_own_still_releases_the_channel` | over-reach guard: the owner still clears its slot, and a later turn can arm again |
| `test_stop_typing_clears_the_registry_for_the_normal_single_turn_path` | control: the ordinary non-overlapping case is unchanged |

Per *"Behavior contracts over snapshots"*, the headline test asserts the
**observable outcome** — the indicator stops POSTing — rather than mock call
counts or internal dict contents. The race is driven deterministically by
`asyncio.sleep(0)` scheduling, not by wall-clock timing, so the tests are not
flaky.

```
# fix applied
$ pytest tests/gateway/test_discord_typing_loop_ownership.py -q
4 passed

# RED proof — ownership check removed, tests kept
$ pytest tests/gateway/test_discord_typing_loop_ownership.py -q
2 failed, 2 passed
  FAILED ...::test_later_turns_loop_survives_an_earlier_turns_teardown
  FAILED ...::test_stop_typing_actually_stops_the_bubble_after_an_overlap
      AssertionError: the indicator loop must have terminated
        where False = <Task pending ... _typing_loop() running at adapter.py:4780>.done()

# no regression
$ pytest tests/gateway/ -q -k "typing"
138 passed, 1 skipped, 11042 deselected
```

## Footprint

- `plugins/platforms/discord/adapter.py`: +7 / −1 (one guarded clear, six lines
  of comment). No new state, no signature change, no behavioural surface beyond
  "the bubble now stops".
- No new core tool, no new `HERMES_*` env var, no config key, no new dependency.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter labels Jul 25, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks — this is a focused fix for a race that remains on current main.

Current plugins/platforms/discord/adapter.py:5071 unconditionally pops the channel's task entry from the loop's finally, while stop_typing() removes and cancels the prior task at plugins/platforms/discord/adapter.py:5077-5081. A replacement can therefore be registered after the old entry is removed and before the cancelled loop executes its cleanup. The proposed task-identity check prevents that stale cleanup from removing the replacement.

The ownership pattern is consistent with existing guarded task cleanup in gateway/platforms/helpers.py:170 and plugins/platforms/feishu/adapter.py:3779-3780. The PR's new tests cover the overlap, post-overlap stop behavior, owner cleanup, and the normal stop path. GitHub reports the PR as MERGEABLE/CLEAN with required checks successful.

Automated hermes-sweeper review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/discord Discord bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants