Skip to content

perf(discord): build reply references from ids instead of fetch_message - #76357

Closed
spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/discord-reply-fetch
Closed

perf(discord): build reply references from ids instead of fetch_message#76357
spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/discord-reply-fetch

Conversation

@spfcraze

@spfcraze spfcraze commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Every Discord reply paid one extra API round trip: the text send path, the voice send path, and the edit path each called fetch_message() just to obtain a reference or an editable handle. Discord resolves message_reference payloads from ids alone and PartialMessage.edit() needs no fetch, so build MessageReference directly (fail_if_not_exists=False preserves the deleted-target behavior the send-side 10008 retry already covered) and use channel.get_partial_message() for edits. Overflow continuations keep threading via an ids-built reference fallback for PartialMessage.

Related Issue

No GitHub issue — discovered via code review and reproduced live (see below). Happy to file one first if preferred.

Changes Made

  • fix/discord-reply-fetch — 4 file(s) changed vs base:
    • plugins/platforms/discord/adapter.py
    • tests/gateway/test_discord_edit_message_overflow.py
    • tests/gateway/test_discord_reply_mode.py
    • tests/gateway/test_discord_send.py

plugins/platforms/discord/adapter.py: 3 sites (text reply, voice reply, edit path) + an ids-built reference fallback in _edit_overflow_split so PartialMessage continuations stay threaded. Tests: new first-mode no-fetch pin in test_discord_reply_mode.py; deleted-target retry test now pins fetch await_count==0 (retry happens purely send-side); overflow/edit mocks retargeted from fetch_message to get_partial_message so any fetch regression breaks all five; reply_mode harness cleaned of the dead fetch setup and its stale comments. Note: 4 discord-suite failures are pre-existing ordering flakes, identical with the change stashed on clean main. Adjacent: #75945 and #75633 touch the line after the edit-path fetch — trivial rebase either way, no semantic overlap.

How to Test

Measured by call count (deterministic — API RTTs vary): reply sends and edits made 1 fetch_message call each (plus 1 per streaming edit tick); after the change they make zero. At a typical 30-150 ms Discord RTT that is one avoided blocking round trip per reply and per edit tick.

Validation completed (recorded by prp):

  1. Sabotage check: pre-fix code fails the regression tests (2 failed), with the fix all pass (13 passed, 0 failed) — target tests/gateway/test_discord_reply_mode.py.
  2. Suite tests/gateway/: branch 4512 passed / 6 failed vs baseline 4509 passed / 6 failed — zero branch-only failures.
  3. Targeted: 44/44 across the 4 affected discord test files (incl. 2 gap-coverage tests: voice-path reference construction, PartialMessage continuation threading). Sabotage revert-verified: the no-fetch pins fail pre-fix (base 2 fail), 13/13 pass with the fix. Fullcheck on the full tests/gateway/ suite: 4512 passed vs 4509 baseline (+3 new tests), identical pre-existing failures on both legs — zero branch-only failures. Full discord suite: 264 passed + 4 pre-existing ordering flakes (identical on clean main with change stashed). Claims audit CLEAR, seam sweep clean. Full repo-wide suite NOT run locally for this PR (skipped by decision; CI owns full-suite validation).
  4. The full repo-wide suite was not run for this change; GitHub CI owns full-suite validation.

Logs

Sabotage verification output:

# base leg (pre-fix code + branch tests):
#   tests: 11 passed, 2 failed
# head leg (with fix):
#   tests: 13 passed, 0 failed

Every reply paid one extra Discord API round trip: the text send path,
the voice send path, and the edit path each called fetch_message() just
to obtain a reference or an editable handle. Discord resolves
message_reference payloads from ids alone, and PartialMessage.edit()
works without a fetch — so build MessageReference directly (with
fail_if_not_exists=False, preserving the deleted-target behavior the
existing send-side 10008 retry already covered) and use
channel.get_partial_message() for edits. Overflow continuations keep
threading via an ids-built reference fallback for PartialMessage.

Measured by call count (deterministic): reply sends and edits now make
ZERO fetch_message calls where they made 1 per reply and 1 per edit
(including every streaming edit tick).

Tests: pin that first-mode replies construct the reference without any
fetch, deleted-target retry test updated to assert fetch await_count==0
(retry now happens purely send-side), overflow/edit mocks retargeted
from fetch_message to get_partial_message (any fetch regression breaks
all five). Note: 4 discord-suite failures are pre-existing ordering
flakes — identical with the change stashed on clean main.
@alt-glitch alt-glitch added type/perf Performance improvement or optimization 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
@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the focused Discord performance improvement. Current main still performs the three redundant fetches at plugins/platforms/discord/adapter.py:3017, :3264, and :3702. The proposed replacements align with the pinned discord.py==2.7.1 APIs: MessageReference is constructible from IDs and get_partial_message() is explicitly the no-request edit handle. The overflow continuation fallback preserves the existing threaded-continuation contract in adapter.py:3393-3405 when the initial edit target is partial.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Aug 1, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks @spfcraze — verified end-to-end (byte-identical wire payloads against discord.py 2.7.1 source, all 7 send/edit paths audited, mutation-checked) and salvaged into #76875 with your authorship preserved via cherry-pick, plus a small refactor follow-up (shared reference-builder helper + a comment correction). Closing in favor of the salvage.

kshitijk4poor added a commit that referenced this pull request Aug 2, 2026
…lMessage comment

Review follow-ups on the #76357 salvage:
- _message_reference_from_ids + _reply_reference_for_send collapse the
  3x duplicated MessageReference construction (naming mirrors telegram's
  _reply_to_message_id_for_send).
- The overflow elif's comment claimed PartialMessage has no to_reference;
  discord.py 2.7.1's PartialMessage does (message.py L1901) — the branch
  is belt-and-suspenders for duck-typed priors, now labeled as such.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…lMessage comment

Review follow-ups on the NousResearch#76357 salvage:
- _message_reference_from_ids + _reply_reference_for_send collapse the
  3x duplicated MessageReference construction (naming mirrors telegram's
  _reply_to_message_id_for_send).
- The overflow elif's comment claimed PartialMessage has no to_reference;
  discord.py 2.7.1's PartialMessage does (message.py L1901) — the branch
  is belt-and-suspenders for duck-typed priors, now labeled as such.
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/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants