Skip to content

fix(cron): surface media attachment delivery failures - #35100

Closed
wysie wants to merge 1 commit into
NousResearch:mainfrom
wysie:fix/cron-media-failure-status
Closed

fix(cron): surface media attachment delivery failures#35100
wysie wants to merge 1 commit into
NousResearch:mainfrom
wysie:fix/cron-media-failure-status

Conversation

@wysie

@wysie wysie commented May 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • return per-attachment errors from live cron MEDIA delivery
  • treat live-adapter media attachment failures as delivery errors instead of clean success
  • avoid standalone fallback after text may already have been sent, preventing duplicate text on partial media failure
  • add regression tests for failed MEDIA delivery and error propagation

Verification

  • python -m py_compile cron/scheduler.py
  • python -m pytest tests/cron/test_scheduler.py -q
  • python -m pytest tests/cron -q

Context

A cron job that returned a valid MEDIA:/path image could log a native media send failure, e.g. a transient WhatsApp bridge Connection Closed, but still report delivered ... via live adapter and leave the run looking green. This PR makes those attachment failures visible to cron delivery status.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels May 30, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks — the reported failure remains reproducible by inspection on current main: cron/scheduler.py:1319-1325 only logs failed media results, and :1834-1854 subsequently records a clean live-adapter delivery.

Problems

  • The branch now conflicts with current delivery code. Main added DeliveryRouter/topic metadata routing at cron/scheduler.py:1662-1712 and timeout handling at :1721-1762; salvage must preserve those paths rather than apply the original hunk unchanged.
  • The proposed if result and not getattr(result, "success", True) still treats None as success. Current main's _confirm_adapter_delivery() explicitly rejects None and success-less values at cron/scheduler.py:1328-1346; media delivery should use equivalent confirmation.

Suggested changes

  • Reapply the error aggregation around current cron/scheduler.py:1825-1854, returning partial-media failures while preventing standalone fallback after text has been sent.
  • Add a multi-attachment regression that confirms all files are attempted and a None media result is reported rather than logged as clean.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@dragos-cociu

dragos-cociu commented Jul 21, 2026

Copy link
Copy Markdown

I adapted the focused media-failure handling from this PR to current main (413ed6b9) and added coverage for the current delivery router.

The candidate change:

  • requires explicit SendResult.success confirmation for each native attachment;
  • reports None, failed results, scheduling failures, timeouts, and exceptions through the cron delivery result instead of logging them only;
  • continues attempting later attachments after one fails;
  • marks mixed outcomes as partial delivery and avoids whole-payload fallback when text or an earlier attachment may already have been delivered;
  • uses standalone fallback only for media-only batches where every live-adapter send is proven not delivered;
  • covers text+media partial failure, media-only partial failure, and media-only total explicit failure;
  • uses attachment filenames rather than injecting local media paths into generated error messages.

Validation on current main:

  • tests/cron: 736 passed
  • git diff --check: passed
  • py_compile for the modified source and test files: passed

Candidate commit: dragos-cociu@ee729d1

I have not opened a competing PR. This branch is available for cherry-pick or comparison if it helps update this PR.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks @wysie — you identified and fixed the live-adapter half of this bug first (May 30), returning per-attachment errors from _send_media_via_adapter so partial media failures surface in the run status instead of a clean success. That exact mechanism just landed on main via PR #88631 (merge commit 22f0f22), which extends the same idea to the standalone-sender lane (result['warnings']) and adds media path-policy parity for manual hermes cron run. Your duplicate-text concern is also covered: a failed media send no longer triggers a standalone fallback after text delivery, since adapter_ok stays true and only the error is recorded.

Closing as superseded — your PR was submitted first and is credited here and in the merged work. Much appreciated!

@teknium1 teknium1 closed this Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists 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