Skip to content

fix(gateway): notify on media delivery failure in more silent-swallow spots - #91335

Open
moon2sun wants to merge 1 commit into
NousResearch:mainfrom
moon2sun:fix/media-delivery-failure-notify
Open

fix(gateway): notify on media delivery failure in more silent-swallow spots#91335
moon2sun wants to merge 1 commit into
NousResearch:mainfrom
moon2sun:fix/media-delivery-failure-notify

Conversation

@moon2sun

Copy link
Copy Markdown

What does this PR do?

Several media-send exception handlers in the gateway only logged the error and otherwise dropped it silently — neither the user nor the model got any signal that a MEDIA: attachment never arrived. In practice this means a model that fabricates or mistypes a path keeps repeating the same broken tag indefinitely, with no feedback loop to correct it.

_notify_media_delivery_failure() already exists and is already called from several other spots in gateway/platforms/base.py. This extends the same, already-established pattern to the remaining silent handlers, rather than inventing a new mechanism.

Found through real, sustained production use of a Hermes gateway (heavy Telegram media traffic), not a code audit — happy to file a tracking issue first if that's preferred process here, just didn't want to hold up a small, self-contained fix on it.

Related Issue

No existing issue — small enough that I went straight to a PR per the "self-contained" guidance in CONTRIBUTING.md. Can open one if preferred.

Type of Change

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

Changes Made

  • gateway/platforms/base.py: notify on single-image send failure (both the !success and exception branches), both image-batch fallback paths, non-image media send, and local-file send.
  • gateway/platforms/weixin.py: notify on extracted-media delivery failure and bare local-file delivery failure.
  • gateway/run.py: notify on post-stream image-batch delivery failure, post-stream media delivery failure, and both background-task delivery paths (image + media).

Deliberately excludes two adjacent spots that overlap with in-flight work, to avoid adding merge-conflict noise against open PRs:

How to Test

  1. Point a platform adapter's send_image/send_multiple_images/send_voice/send_document at a path that will raise (e.g. a nonexistent file, or mock the adapter method to raise) in each of the newly-covered call sites.
  2. Before this change: the exception is logged at WARNING and nothing else happens — no user-facing signal.
  3. After this change: _notify_media_delivery_failure(...) is called with the chat id / path / metadata for that failure, matching the behavior already present at the other call sites in base.py that weren't touched by this PR.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate — found real overlap on 2 adjacent call sites (fix(gateway): handle silent audio and media producers #65745, fix(kanban): make notification delivery durable #83952) and deliberately excluded both from this PR; the sites included here don't overlap with any open PR I could find.
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — ran python -m py_compile on all 3 changed files (clean); didn't run the full suite in this environment.
  • I've added tests for my changes — _notify_media_delivery_failure() itself has zero existing test coverage anywhere in tests/ (confirmed via grep), so there's no existing pattern to extend for these call sites. Happy to add tests if a maintainer can point at the preferred mocking setup for BasePlatformAdapter in this repo's test suite.
  • I've tested on my platform: Debian 13 (CT/LXC), Python 3.11 — this exact diff (same code, ported by hand from an older diverged fork) has been running in a live, actively-used production Hermes gateway for real-world verification, not just a local dry run.

Documentation & Housekeeping

  • N/A — no config keys, docs, or tool schemas changed.

… spots

Several media-send exception handlers across the gateway only logged
the error and otherwise dropped it silently - neither the user nor the
model got any signal that a MEDIA: attachment never arrived, so a model
that fabricated or mistyped a path would keep repeating the same
broken tag indefinitely with no feedback loop to correct it.

_notify_media_delivery_failure() already exists and is already called
from several other spots in gateway/platforms/base.py - this extends
the same pattern to the remaining silent handlers:

- gateway/platforms/base.py: single image send (both the !success and
  exception branches), both image-batch fallback paths, non-image
  media send, and local-file send.
- gateway/platforms/weixin.py: extracted-media delivery and bare
  local-file delivery.
- gateway/run.py: post-stream image-batch delivery, post-stream media
  delivery, and both background-task delivery paths (image + media).

Deliberately excludes gateway/run.py's `_send_voice_reply` (the
`except Exception` after the TTS send) - NousResearch#65745 is already open
against that exact block for a related but distinct case
(`send_result.success is False` without an exception), and
gateway/kanban_watchers.py - NousResearch#83952 is already open and replaces those
two handlers entirely with a larger SendResult-based rework, so a
small notify-only patch there would just add merge-conflict noise
against an in-flight PR.

## Verification

- python -m py_compile on all 3 changed files
- No existing test coverage found for _notify_media_delivery_failure()
  itself (grepped tests/ - zero references), so there's no existing
  pattern to extend with new unit tests for these call sites; happy to
  add tests if a maintainer points at the preferred mocking setup for
  BasePlatformAdapter.
- These exact code paths (same diff) have been running in a live,
  actively-used production Hermes gateway deployment for real-world
  verification, not just a local dry run.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 21, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #56991 and #63837: this patch broadens existing failure notification coverage across generic and Weixin delivery paths rather than adding their retry/missing-file mechanisms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work 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.

2 participants