Skip to content

fix(discord): treat a rejected bot token as non-retryable - #68158

Closed
FixItFoundry wants to merge 1 commit into
NousResearch:mainfrom
FixItFoundry:fix/discord-loginfailure-nonretryable
Closed

fix(discord): treat a rejected bot token as non-retryable#68158
FixItFoundry wants to merge 1 commit into
NousResearch:mainfrom
FixItFoundry:fix/discord-loginfailure-nonretryable

Conversation

@FixItFoundry

Copy link
Copy Markdown
Contributor

The bug

DiscordAdapter.connect() has no handler for discord.LoginFailure, so a rejected token falls through to the generic except Exception at the end of the method. That branch logs and returns False but never calls _set_fatal_error, so _fatal_error_retryable keeps its constructor default of True (gateway/platforms/base.py:2449).

GatewayRunner reads that flag and queues the platform for background reconnection when it's true. The result: an auth rejection is retried on the reconnect backoff (30s, 60s, 120s, … capped at 5 min) forever. Retrying cannot fix it — the token is wrong — and every attempt is another failed identify, which Discord penalizes.

Rotating or revoking a bot token is enough to trigger this.

The fix

Catch LoginFailure ahead of the generic handler and record it with retryable=False, so the runner drops the platform and renders a clear disabled state instead of looping.

The handler is deliberately specific, not broad, so the generic except Exception below stays reachable — transient faults (DNS, proxy, network blips) must remain retryable. There's a regression test pinning exactly that.

Test-stub note

The shared gateway conftest gains a LoginFailure stub. It's a distinct Exception subclass rather than an alias of Exception, because aliasing the two would make the generic handler unreachable under test and quietly void the second test below. This mirrors how the same file already stubs PTB's error hierarchy.

Tests

  • test_connect_marks_login_failure_non_retryable — asserts fatal_error_code == "discord_auth_failed", fatal_error_retryable is False, and that the token lock is still released. Verified to fail when the new handler is removed.
  • test_connect_generic_failure_stays_retryable — a RuntimeError from the same path must stay retryable and must not be recorded as an auth failure. This is the guard against the new handler shadowing the existing one.

tests/gateway/test_discord_connect.py passes; the wider tests/gateway run shows no regressions against the upstream baseline.

connect() has no handler for discord.LoginFailure, so a rejected token
falls through to the generic `except Exception`, which never calls
_set_fatal_error. _fatal_error_retryable therefore keeps its default of
True and GatewayRunner re-attempts the connect on the reconnect backoff
forever -- each attempt another failed identify, which Discord penalizes.
Rotating or revoking a bot token is enough to trigger it, and the loop
never resolves because retrying cannot fix an auth rejection.

Catch LoginFailure ahead of the generic handler and record it with
retryable=False, so the runner drops the platform and surfaces a clear
"disabled" state instead of retrying indefinitely. The handler is
specific, not broad, so the generic branch below stays reachable and
transient faults (DNS, proxy, network blips) remain retryable.

The shared gateway conftest gains a LoginFailure stub as a distinct
Exception subclass rather than an alias of Exception, so the two handlers
stay distinguishable under test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter P3 Low — cosmetic, nice to have 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 labels Jul 20, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Discord failure classification. The premise holds on current main: plugins/platforms/discord/adapter.py:1290-1300 surfaces startup-task exceptions through _wait_for_ready_or_bot_exit(), while plugins/platforms/discord/adapter.py:1315-1322 currently handles them generically without recording a fatal classification. gateway/run.py:8520-8547 and gateway/run.py:9624-9644 use that classification to decide whether to enqueue or remove a failed platform.

The proposed LoginFailure branch preserves the existing cancellation and token-lock release path while marking only rejected credentials as non-retryable. The regression tests also preserve the generic retryable case. GitHub currently reports this branch as conflicted (mergeable_state: dirty), so it needs conflict-resolved salvage onto current main.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing with credit: you were the first submitter on the Discord LoginFailure → non-retryable classification (July 20), three weeks ahead of the broader fix that just landed. PR #85049 (#85049, salvaged from #83713) merged the same discord auth classification as part of a four-platform sweep (telegram/discord/photon/email) plus a needs_attention escalation for long-lived retry loops, so this PR's change is now on main via _classify_connect_exception (discord_auth_error, retryable=False).

Your test approach (real LoginFailure subclass in the conftest mock so the generic handler stays reachable) was solid — the merged suite covers the same branches. Thanks for spotting this early and sorry the credit lands via a close instead of a merge.

@teknium1 teknium1 closed this Aug 13, 2026
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-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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants