Skip to content

fix(discord): honor retry_after in auto-thread creation instead of dropping messages on 429 - #76060

Open
mytrashcan wants to merge 2 commits into
NousResearch:mainfrom
mytrashcan:fix/discord-auto-thread-rate-limit
Open

fix(discord): honor retry_after in auto-thread creation instead of dropping messages on 429#76060
mytrashcan wants to merge 2 commits into
NousResearch:mainfrom
mytrashcan:fix/discord-auto-thread-rate-limit

Conversation

@mytrashcan

@mytrashcan mytrashcan commented Aug 1, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes auto-thread creation silently dropping user messages when Discord rate-limits thread creation (HTTP 429).

Root cause: When Discord returns 429 on message.create_thread(), discord.py raises RateLimited only when the server-requested retry_after exceeds its internal max_ratelimit_timeout. The old retry loop in _auto_create_thread slept a fixed 0.75s and retried — guaranteed to fail against a long Retry-After (observed in production logs: 260s) — then dropped the message with a generic "⚠️ could not create a Discord thread … please retry" notice.

Fix (in plugins/platforms/discord/adapter.py):

  • Short rate limit (retry_after ≤ 30s, new _AUTO_THREAD_MAX_RATE_LIMIT_WAIT_SECONDS): wait out the bucket, then retry the direct create_thread path. The retry now actually succeeds instead of re-429ing.
  • Long rate limit (> 30s): give up immediately — never block the message handler for minutes — skip the seed-message fallback (it would hit the same bucket and spam a stray 🧵 Thread created by Hermes message into the channel), and return a _AutoThreadRateLimited sentinel carrying the delay.
  • The failure notice in _handle_message is now rate-limit-aware: "⚠️ Discord is rate-limiting requests (retry in ~260s)" instead of the generic message, so the user knows when to retry.
  • Reuses the adapter's existing _is_discord_rate_limit / _extract_discord_retry_after helpers (already used for slash-command sync).

Related Issue

No issue filed; discovered live in production logs (auto-thread creation 429'd with Retry in 260.56 seconds while the retry loop only slept 0.75s). Related work that motivated the direction:

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • plugins/platforms/discord/adapter.py
    • Add _AUTO_THREAD_MAX_RATE_LIMIT_WAIT_SECONDS = 30.0 module constant
    • Add _AutoThreadRateLimited sentinel class (carries retry_after per call)
    • Rework _auto_create_thread to honor Discord's retry_after (short: wait + retry; long: give up fast + return sentinel; fallback skipped on 429 to avoid channel spam)
    • _handle_message consumes the sentinel locally for a rate-limit-aware user notice (no shared adapter state — race-free across concurrent messages)
  • tests/gateway/test_discord_auto_thread_rate_limit.py (new, 7 tests)
    • Short rate limit waited out then retried
    • Non-rate-limit connect error still uses the seed-message fallback ([Bug]: Discord auto-thread failure silently falls back to inline channel reply #20243 behavior preserved)
    • Two short rate limits → give up, fallback NOT attempted (no channel spam)
    • Long rate limit → immediate give-up, sentinel carries retry_after
    • Integration: rate-limit failure surfaces "retry in ~Ns" notice
    • Integration: non-rate-limit failure keeps the original generic notice
    • Interleaving regression: two concurrent rate-limited messages each get their own notice (45s/90s, no cross-talk)

How to Test

  1. scripts/run_tests.sh tests/gateway/test_discord_auto_thread_rate_limit.py7 passed
  2. Regression: scripts/run_tests.sh tests/gateway/test_discord_channel_controls.py tests/gateway/test_discord_double_dispatch.py tests/gateway/test_discord_sync_limit.py12 passed
  3. Full suite: scripts/run_tests.sh -j 4 (CI parity) — see note below
  4. ruff check plugins/platforms/discord/adapter.py tests/gateway/test_discord_auto_thread_rate_limit.py → clean

Manual reproduction (needs a Discord server with discord.auto_thread: true): send a burst of @mention messages to exhaust the thread-creation bucket; the bot should either create the thread after the short wait or show "Discord is rate-limiting requests (retry in ~Ns)" instead of silently dropping the request.

Checklist

  • Branch named per CONTRIBUTING (fix/description)
  • Tests run via scripts/run_tests.sh (CI parity)
  • Manual test of the changed code path (Discord auto-thread creation)
  • No cross-platform impact (network-only change; no file I/O, process, or terminal handling)
  • PR is one logical change (429 handling in auto-thread creation)
  • Conventional Commits (fix(discord): ...)
  • Tested on: macOS (Apple Silicon, Python 3.11)

Platforms Tested

  • macOS (Apple Silicon, Python 3.11.15) — full test suite + ruff

Note on full-suite failures (pre-existing, unrelated)

scripts/run_tests.sh -j 4 reports 46 failures across 18 files — none touch the Discord plugin or this change. Verified pre-existing: the same failures reproduce on clean origin/main (stash of this branch) — e.g. test_daytona_environment (needs Daytona), test_voice_mode / test_wake_word / test_transcription_tools (needs audio/OS APIs), test_systemd_notify / test_gateway_service / test_service_manager (Linux/systemd-only, run on macOS), test_web_tools_config (network), and test_update_eol_churn (line-ending churn). All Discord gateway tests pass.

When Discord rate-limits thread creation (HTTP 429), discord.py raises
RateLimited only when the server-requested retry_after exceeds its
internal max_ratelimit_timeout. The old _auto_create_thread loop slept a
fixed 0.75s and retried — guaranteed to fail against a long Retry-After
(e.g. 260s) — so the user's message was dropped with a generic
'please retry' notice.

Honor Discord's retry_after:
- Within _AUTO_THREAD_MAX_RATE_LIMIT_WAIT_SECONDS (30s): wait out the
  bucket, then retry the direct create_thread path.
- Beyond the bound: give up immediately (do not block the handler for
  minutes), skip the seed-message fallback (it would hit the same bucket),
  and record the delay on _last_auto_thread_rate_limit so the caller can
  surface a 'Discord is rate-limiting (retry in ~Ns)' notice.

Adds tests for short/long rate limits, fallback behavior, and the
rate-limit-aware user notice.
@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 sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #52422 and #52423 address the orphaned fallback seed, while #44488 retries the standalone send path. This PR instead handles rate limits during auto-thread creation.

@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 targeting a live failure path: current main still uses the fixed 0.75-second retry after direct and seed-message failures (plugins/platforms/discord/adapter.py:6574-6603), so the 429-specific direction is warranted.

Problems

  • The new self._last_auto_thread_rate_limit is shared adapter state (plugins/platforms/discord/adapter.py:6590, :6605) but is consumed later by _handle_message (:7592-7595). _auto_create_thread yields during retry waits (:6619, :6660), so another message can reset or overwrite this value before the first failure chooses its user notice. That can mislabel a non-429 failure or lose a 429 retry hint.
  • The new tests cover serial outcomes but not this interleaving (tests/gateway/test_discord_auto_thread_rate_limit.py:106-294).

Suggested changes

  • Carry retry-after metadata in the individual _auto_create_thread result and consume that local result in _handle_message, rather than storing it on self.
  • Add a two-message interleaving regression test that proves each failure receives its own notice.

This is an automated hermes-sweeper review.

Comment thread plugins/platforms/discord/adapter.py Outdated
@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Aug 1, 2026
…dapter state

Addresses hermes-sweeper review: self._last_auto_thread_rate_limit was
shared adapter state consumed later by _handle_message. Because
_auto_create_thread awaits during retry sleeps, a second message could
interleave and overwrite the attribute before the first message chose
its user notice — mislabeling a non-429 failure or losing the 429 hint.

- Replace the shared attribute with _AutoThreadRateLimited, a sentinel
  returned by _auto_create_thread that carries retry_after per call.
- _handle_message checks isinstance(...) before the generic success test
  and consumes the per-call value locally; no adapter state is read.
- Add interleaving regression test: two concurrent rate-limited messages
  each surface their own retry hint (45s / 90s) with no cross-talk.
@mytrashcan

Copy link
Copy Markdown
Author

Addressed in 0b380e7f2 — thanks for the sharp catch.

Changes

1. Shared adapter state removed → per-call sentinel

  • self._last_auto_thread_rate_limit is gone. _auto_create_thread now returns a _AutoThreadRateLimited sentinel (carrying retry_after) directly from the long-rate-limit path, instead of mutating adapter state that a concurrent message could overwrite mid-flight.
  • _handle_message checks isinstance(thread, _AutoThreadRateLimited) before the generic if thread: success test and consumes the per-call value locally — no adapter attribute is read or written.

2. Interleaving regression test added

  • test_interleaved_rate_limit_failures_get_own_notices: two messages race through _handle_message concurrently (via asyncio.gather), each hitting a different retry_after (45s / 90s). Asserts each channel receives its own notice with its own retry_after and no cross-talk — this test fails against the old shared-state implementation.

Why this fixes the race

_auto_create_thread awaits during retry sleeps (asyncio.sleep(retry_after)), so a second message can interleave. With the old attribute, message A's failure could be mislabeled by message B's overwrite (or lose its hint entirely). The sentinel travels with the call, so every failure chooses its own notice.

Verification

  • tests/gateway/test_discord_auto_thread_rate_limit.py7 passed (including the new interleaving test)
  • Regression: test_discord_channel_controls.py, test_discord_double_dispatch.py, test_discord_sync_limit.py12 passed
  • ruff check → clean

@BrinkhaT BrinkhaT left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified against a current production reproduction and current main (a90d5369f).

The observed failure was a Discord auto-thread 429 with a server retry delay of about 243 seconds. Current main immediately entered the seed-message fallback, emitted misleading Thread created by Hermes notices, retried after the fixed 0.75-second delay, and then failed—matching this PR’s diagnosis.

I rebased both commits locally onto current main without conflicts and ran the full Discord-focused suite:

49 files, 337 tests passed, 0 failed

The per-call sentinel also avoids cross-message state races, and the bounded wait is a reasonable upstream safety policy for long Discord rate limits. This PR is still relevant on current main and cleanly addresses the production bug class.

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 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.

4 participants