fix: exactly-once delivery reconciliation on Slack adapter#3187
fix: exactly-once delivery reconciliation on Slack adapter#3187praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…ixes #3185) The outbox's crash-reconciliation seam shipped unused: no channel adapter implemented was_delivered() or declared reconciles_unknown_send, so every built-in channel fell back to at-least-once (a duplicate on restart). Slack can read back recent channel history and match a client-side idempotency key stamped into the message metadata, so it now opts into effectively-once: - SlackBot declares reconciles_unknown_send=True and implements was_delivered() via conversations.history, matching the key in message metadata. - send_message stamps the idempotency key into Slack metadata when provided. - DurableDelivery carries the idempotency key in the outbox payload and forwards it to adapters whose send_message accepts it (backward-compatible). - _build_reconciler now passes the target channel to was_delivered when its signature accepts it (legacy single-arg form still supported). Channels whose platform cannot confirm delivery stay at-least-once by design. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDurableDelivery now preserves idempotency keys across outbox recovery. Slack embeds keys in message metadata, declares reconciliation support, and checks recent history before resending. Tests cover metadata, matching, recovery without resend, and resend when delivery is absent. ChangesSlack effectively-once delivery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DurableDelivery
participant SlackBot
participant SlackAPI
participant Outbox
DurableDelivery->>Outbox: Load recovered entry and idempotency_key
DurableDelivery->>SlackBot: Call was_delivered(target, idempotency_key)
SlackBot->>SlackAPI: Query recent channel history with metadata
SlackAPI-->>SlackBot: Return recent messages
alt Matching metadata exists
SlackBot-->>DurableDelivery: Return delivered
DurableDelivery->>Outbox: Mark entry sent
else No matching metadata
DurableDelivery->>SlackBot: Resend with idempotency_key
SlackBot->>SlackAPI: Post message with metadata
DurableDelivery->>Outbox: Mark entry sent
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-bot/praisonai_bot/bots/_delivery.py (1)
511-553: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winForward the outbox idempotency key on the initial send path
payload["idempotency_key"]is persisted, but thisdeliver_with_retry(...)call still sends only**send_kwargs. The first delivery never stamps the platform message with the key, so a crash after a successful send and beforemark_sent()can’t be reconciled and will be resent as a duplicate. Mirror thedrain_pending()sender logic here and pass the key whensend_messageaccepts it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-bot/praisonai_bot/bots/_delivery.py` around lines 511 - 553, The initial delivery in the outbox path must forward the generated idempotency key so sent messages can be reconciled after a crash. Update the deliver_with_retry call in the enqueue/send flow to pass idempotency_key using the same convention as drain_pending(), while preserving compatibility with adapters whose send_message does not accept that argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-bot/praisonai_bot/bots/slack.py`:
- Around line 828-863: The was_delivered method must inspect thread replies when
a thread parent timestamp is available, since conversations_history excludes
threaded replies. Extract or accept the relevant thread_ts for the target, call
conversations.replies for that parent, and match
metadata.event_payload.idempotency_key using the existing logic; retain
conversations_history for non-threaded messages and return False on unavailable
lookups.
---
Outside diff comments:
In `@src/praisonai-bot/praisonai_bot/bots/_delivery.py`:
- Around line 511-553: The initial delivery in the outbox path must forward the
generated idempotency key so sent messages can be reconciled after a crash.
Update the deliver_with_retry call in the enqueue/send flow to pass
idempotency_key using the same convention as drain_pending(), while preserving
compatibility with adapters whose send_message does not accept that argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dc3dabf-4b4e-4798-9b93-46791d517bc1
📒 Files selected for processing (3)
src/praisonai-bot/praisonai_bot/bots/_delivery.pysrc/praisonai-bot/praisonai_bot/bots/slack.pysrc/praisonai-bot/tests/unit/bots/test_slack_reconcile.py
|
|
||
| async def was_delivered(self, target: str, idempotency_key: str) -> bool: | ||
| """Confirm whether a prior send for ``idempotency_key`` already landed. | ||
|
|
||
| Enables effectively-once delivery: after a crash between the Slack API | ||
| call and the durable ack, the outbox asks this before re-sending. We | ||
| read back recent channel history and match the client-side idempotency | ||
| key carried in each message's ``metadata.event_payload`` — so a | ||
| confirmed send is marked ``sent`` instead of producing a duplicate. | ||
|
|
||
| Returns ``False`` (fall back to at-least-once re-send) when the lookup | ||
| is unavailable or the key is not found. | ||
| """ | ||
| if not self._client or not idempotency_key: | ||
| return False | ||
|
|
||
| # ``target`` is the outbox target ("slack:<channel>"); strip the prefix. | ||
| channel_id = target.split(":", 1)[1] if ":" in target else target | ||
| try: | ||
| response = await self._client.conversations_history( | ||
| channel=channel_id, | ||
| limit=100, | ||
| include_all_metadata=True, | ||
| ) | ||
| except Exception as e: # noqa: BLE001 — best-effort; never crash drain | ||
| logger.debug("was_delivered history lookup failed: %s", e) | ||
| return False | ||
|
|
||
| for message in response.get("messages", []) or []: | ||
| metadata = message.get("metadata") or {} | ||
| if metadata.get("event_type") != _OUTBOUND_EVENT_TYPE: | ||
| continue | ||
| payload = metadata.get("event_payload") or {} | ||
| if str(payload.get("idempotency_key", "")) == str(idempotency_key): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does Slack's conversations.history Web API endpoint include thread replies, or only top-level channel messages?
💡 Result:
No, Slack's conversations.history Web API endpoint does not include thread replies [1][2]. It only returns top-level messages within a channel [3][2]. To retrieve the replies associated with a thread, you must use the conversations.replies method, which requires the timestamp (ts) of the thread's parent message [4][3]. The conversations.history method will return the parent message if it exists within the requested time range, but it will not automatically include the subsequent threaded replies in its response [1][2].
Citations:
- 1: https://stackoverflow.com/questions/56744339/pulling-historical-channel-messages-python
- 2: https://stackoverflow.com/questions/72512171/slack-get-messages-threads-for-date-range-via-slack-webclient
- 3: https://docs.slack.dev/reference/methods/conversations.history.md
- 4: https://docs.slack.dev/reference/methods/conversations.replies.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai-bot/praisonai_bot/bots/slack.py"
echo "== relevant symbols =="
rg -n "def send_message|def was_delivered|thread_id|thread_ts|DurableDelivery|conversations_history|conversations_replies|_OUTBOUND_EVENT_TYPE" "$file"
echo
echo "== surrounding code around send_message and was_delivered =="
sed -n '760,890p' "$file"Repository: MervinPraison/PraisonAI
Length of output: 8799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== was_delivered call sites =="
rg -n "was_delivered\(" -S src/praisonai-bot
echo
echo "== DurableDelivery / deliver_outbound definition and call shape =="
rg -n "def deliver_outbound|class DurableDelivery|was_delivered|send_kwargs|send_message\(" src/praisonai-bot/praisonai_bot -SRepository: MervinPraison/PraisonAI
Length of output: 13687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding delivery integration without running project code.
file="src/praisonai-bot/praisonai_bot/bots/slack.py"
sed -n '1,220p' "$file"
echo
sed -n '220,360p' "$file"
echo
sed -n '360,520p' "$file"Repository: MervinPraison/PraisonAI
Length of output: 21680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where SlackBot.was_delivered is invoked and what context is available there.
rg -n "was_delivered\(" .Repository: MervinPraison/PraisonAI
Length of output: 1630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai-bot/praisonai_bot/bots/_delivery.py"
echo "== durable delivery reconcile path =="
sed -n '560,690p' "$file"
echo
echo "== relevant slack tests =="
sed -n '1,140p' "src/praisonai-bot/tests/unit/bots/test_slack_reconcile.py"Repository: MervinPraison/PraisonAI
Length of output: 10374
Threaded sends need a reply-history lookup conversations_history only returns top-level channel messages, so a crash-recovered send with thread_ts will never match the stamped idempotency key and will be re-sent. Use conversations.replies when a thread parent ts is available, or threaded replies will silently lose effectively-once delivery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-bot/praisonai_bot/bots/slack.py` around lines 828 - 863, The
was_delivered method must inspect thread replies when a thread parent timestamp
is available, since conversations_history excludes threaded replies. Extract or
accept the relevant thread_ts for the target, call conversations.replies for
that parent, and match metadata.event_payload.idempotency_key using the existing
logic; retain conversations_history for non-threaded messages and return False
on unavailable lookups.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@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
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3187 ✅
Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Fixes pushed (commit
|
…threads, first-send key) Addresses reviewer findings on PR #3187: - Graceful at-least-once fallback when metadata.message:write scope is absent (was a permanent message-loss regression on apps without the scope). - Reconcile threaded sends via conversations.replies (history excludes replies). - Forward idempotency_key on the initial send path, not just crash-recovered drain. - Gate key forwarding on an explicit idempotency_key param (drop **kwargs fallback). Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3185
Summary
The outbox has a crash-safe, effectively-once delivery seam (
recoveredstate +reconciler+PlatformCapabilities.reconciles_unknown_send), but no shipped adapter implemented it — so every built-in channel fell back to at-least-once and a gateway restart between platform-send and durable-ack re-delivered the message (a user-visible duplicate).This wires the seam end-to-end for Slack, the one built-in channel whose platform API can genuinely confirm a client-side-keyed send (read back recent history + match message
metadata). Channels whose platform cannot confirm delivery stay at-least-once by design (now an explicit per-channel fact, not a silent default).Changes
praisonai_bot/bots/slack.pySlackBot.default_capabilities()/platform_capabilitiesnow declarereconciles_unknown_send=True.send_message(..., idempotency_key=None)stamps the key into Slack messagemetadata(event_type="praisonai_outbound").was_delivered(target, idempotency_key)queriesconversations.historyand matches the key inmetadata.event_payload; best-effort (returnsFalse→ at-least-once fallback on any lookup failure).praisonai_bot/bots/_delivery.pyDurableDeliverycarriesidempotency_keyin the outbox payload and forwards it to adapters whosesend_messageaccepts it (signature-checked, backward-compatible — Telegram/Discord/etc. untouched)._build_reconcilerpasses thetargetchannel towas_deliveredwhen its signature accepts it; the legacy single-argwas_delivered(idempotency_key)form still works.Behaviour
Layer placement
Wrapper-only (
praisonai-botadapters). The corePlatformCapabilitiesflag, reconciler contract, and outbox state machine already existed — nothing in the core contract changed. No new params/CLI/YAML surface; runtime adapter behaviour only.Tests
New
tests/unit/bots/test_slack_reconcile.pycovers capability declaration, metadata stamping,was_deliveredmatching, and end-to-end drain reconcile (no re-send when already delivered; re-send when not). All 121 relevant delivery/outbox/slack tests pass; existingtest_outbox_reconcile.pyremains green.Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes