Skip to content

fix(discord): claim dedup ID in missed-message backfill dispatch - #75067

Open
spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/discord-backfill-dedup-claim
Open

spfcraze wants to merge 1 commit into
NousResearch:mainfrom
spfcraze:fix/discord-backfill-dedup-claim

Conversation

@spfcraze

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes duplicate processing of Discord messages after a reconnect: the missed-message backfill never claims the dedup ID, so a message it recovers can be dispatched a second time by the live gateway.

Two Discord behaviors make this fire in practice: the gateway replays missed events on a successful resume, and on_ready — which kicks off the missed-message backfill — fires after resumes too. The backfill scans channel history over REST and dispatches what it finds; the replayed live event for the same message races or follows it.

The backfill's dispatch (_dispatch_recovered_message) admitted messages with claim=False, so:

  • the scan loop's contains() check and the admission check only ever looked — the recovered message never entered the dedup cache;
  • after a successful backfill dispatch, a replayed live event for the same message passed the live ingress (claim=True admission) and dispatched again — two agent runs, two replies for one user message;
  • the failure-path discard() calls ("Release a claimed message ID") were no-ops, proving the claim was always intended.

The fix admits recovered messages with claim=True. Whichever path — live ingress or backfill — claims the ID first wins; the other drops the duplicate. The existing discard() calls on the cancel/error paths now correctly release the claim so a later retry can proceed.

Related Issue

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

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/platforms/discord/adapter.py: _dispatch_recovered_message admits with claim=True (with a comment explaining why); the scan-loop comment updated to describe the actual claim ownership (cheap pre-filter at scan time, atomic claim at dispatch).
  • tests/gateway/test_discord_missed_message_backfill.py: three regression tests — a backfilled message enters the dedup cache and a subsequent live admission of the same message is dropped; a second backfill dispatch of the same message is a no-op; a dispatch that raises releases the claim (via the pre-existing discard() error path) so a retry dispatches and stays claimed.

Behavior notes:

  • _discord_message_admission claims before applying bot/self filters, so a backfill message rejected on policy grounds now consumes the ID. Both ingress paths use the identical admission function, so there is no divergence between live and backfill policy.
  • The dedup cache TTL is 300s (MessageDeduplicator default): a live replay arriving more than 5 minutes after a backfill dispatch would be admitted. Resume replays arrive within seconds of reconnect, so this boundary doesn't affect the reported symptom.

How to Test

Reproduction (against pre-fix code, using the existing test fixtures):

admitted = await adapter._dispatch_recovered_message(msg)   # True, _handle_message called once
adapter._dedup.contains(str(msg.id))                        # pre-fix: False (never claimed!)
admitted2, _ = adapter._discord_message_admission(msg, claim=True)
# pre-fix: admitted2 is True  -> live replay dispatches a second time (BUG)
# post-fix: admitted2 is False -> replay dropped

Focused validation completed:

  1. bash scripts/run_tests.sh tests/gateway/test_discord_missed_message_backfill.py — 18/18 pass.
  2. Sabotage check: reverting the adapter to main makes exactly the 3 new tests fail (15 pass); restoring it returns to 18/18.
  3. Related suites: test_discord_double_dispatch.py test_discord_connect.py test_discord_send.py test_discord_free_response.py test_message_deduplicator.py test_duplicate_reply_suppression.py test_dedupe_user_turns.py + the backfill file — 79/79 pass. (Note: the existing test_discord_double_dispatch.py covers the thread-starter pre-seed path, a different double-dispatch vector.)
  4. uvx --from ruff==0.15.10 ruff check plugins/platforms/discord/adapter.py tests/gateway/test_discord_missed_message_backfill.py — clean.
  5. git diff --check — clean.
  6. Adversarial cases executed live: backfill → live replay dropped; backfill → second backfill attempt dropped; _handle_message invoked exactly once across all three; a dispatch raising mid-loop releases the claim via the pre-existing discard() error path and a retry then dispatches and stays claimed (driven through the real _run_missed_message_backfill loop).

The full repo-wide suite was not run for this change; the Discord/dedup suites above cover the changed path, and GitHub CI owns full-suite validation.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — full suite not run locally (see How to Test; all Discord/dedup suites pass)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Ubuntu 24.04

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A (inline comments updated to match the new claim ownership)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — no platform surface (async dedup-cache admission)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Logs

Sabotage verification output:

# fix reverted to main:
=== Summary: 1 files, 15 tests passed, 3 failed ===   (all 3 new regression tests)
# fix restored:
=== Summary: 1 files, 18 tests passed, 0 failed ===

The missed-message backfill admitted recovered messages with
claim=False, so a successfully backfilled message never entered the
dedup cache. Discord replays missed events on a successful resume —
and on_ready, which starts the backfill, fires after resumes too — so
a live replay of the same message raced or followed the REST-scan
dispatch and was admitted again: two agent runs, two replies for one
user message. The adapter's own discard() calls on the failure paths
('Release a claimed message ID') were no-ops, proving the claim was
intended. Admit with claim=True; whichever path (live or backfill)
claims first now wins and the other drops the duplicate.
@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 Jul 30, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused regression fix. The premise is verified on current main: recovered dispatch calls _discord_message_admission(..., claim=False) at plugins/platforms/discord/adapter.py:2217, while live ingress claims at plugins/platforms/discord/adapter.py:1405. Since MessageDeduplicator.is_duplicate() records a new ID (gateway/platforms/helpers.py:48-71) and contains() does not (gateway/platforms/helpers.py:73-83), the recovered path currently leaves a successful message available for a later live replay.

The proposed claim=True makes the two ingress paths share atomic claim ownership, and the existing cancellation/error cleanup already discards the ID at plugins/platforms/discord/adapter.py:2151-2157. The diff is narrowly scoped and GitHub reports the PR as mergeable.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 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-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