fix: give outbound media the same retry/backoff as text

[praisonai-triage-agent]

Fixes #3184

Summary

Outbound media/file uploads were best-effort: DeliveryRouter.send_media called the raw upload primitive (send_photo/files_upload_v2/Discord file send) exactly once, so a transient transport blip (HTTP 5xx, rate limit, connection reset) silently dropped the file — while the accompanying text was delivered durably.

Critical-review finding

I traced the real delivery path rather than the issue's idealised sketch. Text delivered via BotOutboundMessenger.send() does not itself ride the SQLite OutboundQueue/DLQ — that machinery lives on the adapter reply path. What text does get that media lacked is deliver_with_retry (bounded exponential backoff honouring server Retry-After): every adapter wraps its raw send_message in it via OutboundResilienceMixin, while deliver_media_to_adapter reached none of it.

So the confirmed, honest gap is media has no retry/backoff, and the minimal fix is to give it the same retry wrapper text already uses — at the same wrapper layer.

Change

• DeliveryRouter.send_media now wraps the upload in deliver_with_retry, reusing the adapter's already-configured _outbound_backoff policy when present (falling back to a sensible default otherwise).
• Non-retryable outcomes are preserved: an adapter exposing no upload primitive still returns False without retrying; a permanent error still surfaces as a failed send.
• No new params, modules, exports, YAML knobs, or DB schema — rejecting the larger scope the issue sketched (new idempotency-key module + routing every attachment through OutboundQueue.enqueue) as it duplicates surface with no live consumer and isn't how text achieves resilience here.

Tests

Added three focused tests in test_outbound_media.py:

• transient upload failure is retried and finally delivered;
• persistent transient failure gives up cleanly (False) after the attempt budget;
• an adapter with no upload primitive returns False without retrying.

All delivery/media/messenger/resilience tests pass (132 passed in the changed areas). Remaining failures in the wider bot suite are pre-existing environmental issues (missing pydantic), unrelated to this change.

Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Outbound media delivery now retries temporary upload failures automatically.
  • Media uploads use configured retry settings when available, with a safe default otherwise.
  • Persistent failures stop after the retry limit and report delivery failure.
  • Media delivery gracefully handles adapters that do not support uploads.

[MervinPraison]

@coderabbitai review

[MervinPraison]

/review

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 502fc554-9dce-4dba-9f12-6d2958013af6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

DeliveryRouter.send_media now routes media uploads through the shared retry helper, using a channel-specific or default backoff policy. Tests cover transient recovery, exhausted attempts, and adapters without media-upload support.

Changes

Media retry delivery

Layer / File(s)	Summary
Retry-wrapped media uploads src/praisonai-bot/praisonai_bot/bots/delivery.py	send_media uses deliver_with_retry with the target’s valid BackoffPolicy or a default policy before returning existing upload results.
Retry behavior validation src/praisonai-bot/tests/unit/bots/test_outbound_media.py	Tests verify success after transient failures, termination at the configured attempt limit, and no retries when the adapter lacks send_media.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• MervinPraison/PraisonAI#3144: Updates the same DeliveryRouter.send_media path with related delivery-router handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR adds retry/backoff for media uploads, but it does not implement the durable outbox, idempotency, crash recovery, or DLQ required by #3184.	Add the media delivery path to the durable outbox with stable idempotency, crash recovery, and DLQ handling, or narrow the issue scope accordingly.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: outbound media now uses the same retry/backoff path as text.
Out of Scope Changes check	✅ Passed	The changes stay focused on media delivery retries and the supporting tests, with no clear unrelated code introduced.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/issue-3184-20260719-0913

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR closes the retry/resilience gap between text and media sends in the outbound delivery path. Previously, DeliveryRouter.send_media called the upload primitive exactly once, so any transient failure (HTTP 5xx, rate-limit, connection reset) silently dropped the file; text already had bounded exponential backoff via deliver_with_retry.

• DeliveryRouter.send_media now wraps deliver_media_to_adapter in deliver_with_retry, reusing the adapter's _outbound_backoff when present and falling back to a conservative 3-attempt policy otherwise.
• The Discord branch in deliver_media_to_adapter is refactored to propagate transport errors (only a missing discord package import is caught locally), so the retry wrapper can see and back off on channel.send failures; the Telegram and Slack branches already propagated by design.
• Three focused unit tests cover the retry, exhaustion, and no-primitive code paths.

Confidence Score: 5/5

Safe to merge — the change is minimal and surgical, touching only the media upload call site and the Discord import guard.

The retry wrapper is already battle-tested on the text path. The fallback BackoffPolicy (3 attempts, 1s–10s exponential) is conservative. The Discord refactor correctly narrows the caught exception to import failure only, letting transport errors reach the retry wrapper as intended. The 'no upload primitive' False return bypasses retry entirely since no exception is raised. Exhausted-retry exceptions propagate to the outer handler and resolve to False, matching the documented contract. All three new test scenarios exercise real production code paths with sub-millisecond backoff so they run fast without sleeping.

No files require special attention.

Important Files Changed

Filename	Overview
src/praisonai-bot/praisonai_bot/bots/delivery.py	Wraps deliver_media_to_adapter in deliver_with_retry with the adapter's backoff policy or a sensible fallback; exceptions from exhausted retries are caught by the outer handler and correctly surface as False.
src/praisonai-bot/praisonai_bot/bots/_outbound_media.py	Discord branch split into a narrow import-only try/except plus unguarded channel.send; transport errors now propagate to the caller's retry wrapper as intended, while only the non-retryable 'discord package missing' condition is caught locally.
src/praisonai-bot/tests/unit/bots/test_outbound_media.py	Adds three router-level tests (retry success, exhaustion, no-primitive) and one deliver_media_to_adapter-level test for the Discord propagation refactor; BackoffPolicy is set to sub-millisecond delays so tests do not sleep.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant DeliveryRouter
    participant deliver_with_retry
    participant deliver_media_to_adapter
    participant PlatformAdapter

    Caller->>DeliveryRouter: send_media(target, path)
    DeliveryRouter->>DeliveryRouter: resolve(target) → platform, channel_id, thread_id
    DeliveryRouter->>DeliveryRouter: validate_media_delivery_path(path)
    DeliveryRouter->>DeliveryRouter: build BackoffPolicy (adapter._outbound_backoff or fallback)
    DeliveryRouter->>deliver_with_retry: deliver_with_retry(_upload, policy, platform)

    loop Retry loop (up to max_attempts)
        deliver_with_retry->>deliver_media_to_adapter: _upload() → deliver_media_to_adapter(...)
        deliver_media_to_adapter->>PlatformAdapter: send_photo / send_document / files_upload_v2 / channel.send
        alt Success
            PlatformAdapter-->>deliver_media_to_adapter: ok
            deliver_media_to_adapter-->>deliver_with_retry: True
            deliver_with_retry-->>DeliveryRouter: True
            DeliveryRouter-->>Caller: True
        else Transient error (5xx / rate-limit / reset)
            PlatformAdapter-->>deliver_media_to_adapter: raise
            deliver_media_to_adapter-->>deliver_with_retry: raise
            deliver_with_retry->>deliver_with_retry: is_recoverable? → sleep(backoff)
        else Max attempts exhausted
            deliver_with_retry-->>DeliveryRouter: raise (caught by outer except)
            DeliveryRouter-->>Caller: False
        else No upload primitive
            deliver_media_to_adapter-->>deliver_with_retry: False (no exception)
            deliver_with_retry-->>DeliveryRouter: False
            DeliveryRouter-->>Caller: False
        end
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant DeliveryRouter
    participant deliver_with_retry
    participant deliver_media_to_adapter
    participant PlatformAdapter

    Caller->>DeliveryRouter: send_media(target, path)
    DeliveryRouter->>DeliveryRouter: resolve(target) → platform, channel_id, thread_id
    DeliveryRouter->>DeliveryRouter: validate_media_delivery_path(path)
    DeliveryRouter->>DeliveryRouter: build BackoffPolicy (adapter._outbound_backoff or fallback)
    DeliveryRouter->>deliver_with_retry: deliver_with_retry(_upload, policy, platform)

    loop Retry loop (up to max_attempts)
        deliver_with_retry->>deliver_media_to_adapter: _upload() → deliver_media_to_adapter(...)
        deliver_media_to_adapter->>PlatformAdapter: send_photo / send_document / files_upload_v2 / channel.send
        alt Success
            PlatformAdapter-->>deliver_media_to_adapter: ok
            deliver_media_to_adapter-->>deliver_with_retry: True
            deliver_with_retry-->>DeliveryRouter: True
            DeliveryRouter-->>Caller: True
        else Transient error (5xx / rate-limit / reset)
            PlatformAdapter-->>deliver_media_to_adapter: raise
            deliver_media_to_adapter-->>deliver_with_retry: raise
            deliver_with_retry->>deliver_with_retry: is_recoverable? → sleep(backoff)
        else Max attempts exhausted
            deliver_with_retry-->>DeliveryRouter: raise (caught by outer except)
            DeliveryRouter-->>Caller: False
        else No upload primitive
            deliver_media_to_adapter-->>deliver_with_retry: False (no exception)
            deliver_with_retry-->>DeliveryRouter: False
            DeliveryRouter-->>Caller: False
        end
    end

Loading

_{Reviews (2): Last reviewed commit: "fix: propagate transient Discord media e..." | Re-trigger Greptile}

[MervinPraison]

@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.

Phase 1: Review per AGENTS.md

1. Protocol-driven: check heavy implementations vs core SDK
2. Backward compatible: ensure zero feature regressions
3. Performance: no hot-path regressions
4. SDK value: review in depth whether the change genuinely adds value to the SDK — never add features for the sake of adding them. It must strengthen the SDK (simpler, more user-friendly, robust, world-class, secure). If it does not clearly add value, request changes or recommend rejecting/closing rather than merging scope creep
5. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params.
6. Repo routing: agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; optional sandbox backends → PraisonAI-Plugins (praisonai.sandbox entry point) — request changes if wrongly added to praisonaiagents/

Phase 2: FIX Valid Issues
7. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
8. Also independently identify and fix any gaps or issues you find in the changed code — do not rely only on prior reviewer feedback
9. Push all code fixes directly to THIS branch (do NOT create a new PR)
10. Comment a summary of exact files modified and what you skipped

Phase 3: Final Verdict
11. If all issues are resolved, approve the PR / close the Issue
12. If blocking issues remain, request changes / leave clear action items

[praisonai-triage-agent]

Claude finished @MervinPraison's task —— View job

---

Final Architecture Review — PR #3186 ✅

• Read scope rules (Python bot package; no rust/ts touched)
• Review delivery.py send_media retry wrapping
• Verify Greptile's Discord native-path finding in _outbound_media.py
• Assess SDK value / scope creep
• Fix valid issue (Discord path)
• Add test + run suite
• Final verdict

---

Phase 1 — Review

Verdict: sound, minimal, correctly scoped. The PR wraps deliver_media_to_adapter in the existing deliver_with_retry helper (delivery.py:840-859), reusing the adapter's own _outbound_backoff BackoffPolicy when present and a conservative 1s/10s/3-attempt fallback otherwise. This gives media the same transient-failure resilience text already has, at the same wrapper layer — no new params, modules, exports, YAML knobs, or DB schema.

• Protocol-driven / no bloat: ✅ reuses _resilience primitives; nothing added to the Agent class or core SDK.
• Backward compatible: ✅ non-retryable False (no upload primitive) still returns False without retrying; permanent errors still surface as failed sends.
• Scope discipline: ✅ correctly rejected the issue's larger sketch (new idempotency-key module + routing every attachment through OutboundQueue.enqueue) as duplicate surface with no live consumer — matching how text actually achieves resilience here. I agree with this call; CodeRabbit's "durable outbox/DLQ" pre-merge warning is scope the text path itself doesn't have, so parity is the right bar.

Phase 2 — Valid issue found & fixed

Greptile's finding is correct and I confirmed it. The Discord native branch in _outbound_media.py caught all exceptions and returned False:

except Exception as e:
    logger.warning("Discord media upload failed for %s: %s", channel_id, e)
    return False

Because deliver_with_retry only retries on raised exceptions (_resilience.py:492-494), a transient Discord blip returned False → no retry → file silently dropped. That defeats this PR's own goal for Discord while Telegram/Slack (which let exceptions propagate) got it.

Fix pushed (_outbound_media.py:449-479): the Discord branch now lets transport errors propagate like the other transports, so the retry wrapper backs off and retries. Only the missing optional discord import is caught locally (permanent, non-retryable), and unresolved channels still fall through to the no-primitive False return. Behaviour for the "channel not found" and "no primitive" cases is unchanged.

Added test_discord_media_transient_error_propagates_for_retry (verified passing with discord.py installed; auto-skips otherwise via importorskip).

Files modified this review

• src/praisonai-bot/praisonai_bot/bots/_outbound_media.py — Discord branch propagates transient errors for retry.
• src/praisonai-bot/tests/unit/bots/test_outbound_media.py — added Discord retry-propagation test.

Skipped: the durable-outbox/idempotency/DLQ expansion from the original issue sketch (scope creep, no live consumer, not how text achieves resilience) — consistent with the PR author's own critical-review reasoning.

Tests

test_outbound_media.py 28 passed (Discord test verified with discord.py, skips without). Related suites — test_outbound_messenger, test_outbound_resilience, test_delivery_imports, test_background_job_delivery — 59 passed, no regressions. test_session_compaction.py fails only on pre-existing missing pydantic, unrelated to this change.

Phase 3 — Verdict

✅ Approve. The change is a genuine, lightweight robustness improvement with parity across all three native transports (Telegram, Slack, Discord) plus the send_media hook path. All reviewer-flagged gaps resolved.

(Note: I can't submit a formal GitHub approval for security reasons — this comment is the review.)
· branch claude/issue-3184-20260719-0913