Skip to content

fix(discord): bound stop_typing reap so a stuck typing loop can't block delivery - #85435

Open
handnewb wants to merge 1 commit into
NousResearch:mainfrom
handnewb:fix/discord-typing-stop-nonblocking
Open

handnewb wants to merge 1 commit into
NousResearch:mainfrom
handnewb:fix/discord-typing-stop-nonblocking

Conversation

@handnewb

Copy link
Copy Markdown
Contributor

Summary

Bounds DiscordAdapter.stop_typing() so a stuck typing loop can never block message delivery. This closes member #3 of the Discord typing race family tracked in #85427 (design issue).

Root cause

stop_typing() cancelled the typing loop and then awaited it with no bound:

task = self._typing_tasks.pop(chat_id, None)
if task:
    task.cancel()
    try:
        await task          # ← unbounded
    except (asyncio.CancelledError, Exception):
        pass

Cancellation is only delivered at the loop's next await point. If that await is a /channels/{id}/typing POST that ignores cancellation until the request completes (the failure mode described in #64874 — a stuck request that only clears at the ~30s transport timeout), stop_typing blocks for the whole timeout, holding up response delivery after the turn has already finished.

Fix

Reap the cancelled task with a bounded timeout and detach if it does not finish promptly, mirroring the existing base._stop_typing_refresh pattern (wait_for(shield(task), timeout=0.5)):

task = self._typing_tasks.pop(chat_id, None)
if task:
    task.cancel()
    try:
        await asyncio.wait_for(
            asyncio.shield(task), timeout=_TYPING_STOP_REAP_TIMEOUT
        )
    except (asyncio.CancelledError, asyncio.TimeoutError, Exception):
        pass

_TYPING_STOP_REAP_TIMEOUT = 0.5 (module constant, matching base._stop_typing_refresh).

Test

tests/gateway/test_discord_typing_stop_nonblocking.py emulates a cancellation-immune transport (a request coroutine that swallows CancelledError until released) and asserts stop_typing detaches within the reap bound.

The test uses asyncio.wait (not wait_for) deliberately: wrapping stop_typing in wait_for would itself deadlock on the buggy code, because stop_typing's unbounded await task is stuck on the cancellation-immune loop and cannot be cancelled cleanly either.

  • buggy code: fails fast (~2.8s) with AssertionError: stop_typing blocked on the cancellation-immune loop…
  • fixed code: passes (2/2).

Relationship to the rest of the race family (#85427)

Member Mechanism Covered by
Orphaned re-registered loop loop finally popped the registry entry unconditionally #85425
HTTP hang stalls cancellation await request(route) has no timeout #64910
stop_typing blocks delivery unbounded await task this PR
Redundant double refresh _keep_typing no-op calls for Discord design issue #85427 (long-term)

This PR is independent and non-conflicting with #85425 (different lines: stop_typing vs the loop's finally) and complements #64910 (which bounds the request; this bounds the reap). Together they close the whole class short of the single-ownership refactor proposed in #85427.

…ck delivery

stop_typing() cancelled the typing loop and then awaited it with no bound.
Cancellation is only delivered at the loop's next await; if that await is a
/channels/{id}/typing POST that ignores cancellation until the request
completes (the NousResearch#64874 failure mode), stop_typing blocks for the transport
timeout and holds up message delivery.

Reap the cancelled task with a bounded timeout and detach if it does not
finish promptly, mirroring base._stop_typing_refresh. Adds a regression
test that emulates a cancellation-immune transport and asserts stop_typing
detaches within the reap bound.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 13, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

PR: fix(discord): bound stop_typing reap so a stuck typing loop can't block delivery

Clean fix with a good regression test. Observations:

  • plugins/platforms/discord/adapter.py stop_typing: except (asyncio.CancelledError, asyncio.TimeoutError, Exception)TimeoutError (incl. asyncio.TimeoutError) is already a subclass of Exception, so the explicit mention is redundant; except (asyncio.CancelledError, Exception) suffices. Harmless, but the triple-tuple reads as if TimeoutError needs special handling.
  • After stop_typing detaches (shield + timeout), the cancelled loop keeps running in the background until its next await. If _typing_loop's unwind path re-registers itself (e.g., a finally that re-adds to _typing_tasks or sends another typing request), the indicator could resurrect after "stop". Worth confirming _typing_loop has no such re-registration path, since the whole point of the fix is that stop means stop.
  • The 0.5s reap bound is a magic constant conceptually mirrored in base._stop_typing_refresh; consider exporting one constant that both sites import so the "mirror" can't drift. Cosmetic.

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 P2 Medium — degraded but workaround exists platform/discord Discord bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants