Allow webhook retries after downstream delivery failure - #47293
Conversation
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the failed direct-delivery retry case. The underlying problem is present on current main, but the normal-delivery implementation needs rework before it covers the live failure path.
Problems
gateway/platforms/webhook.py:675attaches to the task awaitinghandle_message, buthandle_messageis fire-and-forget: it starts_process_message_backgroundand returns atgateway/platforms/base.py:4802. Actual processing errors are handled in that background task atgateway/platforms/base.py:5233-5235, so this callback will normally see no exception. The existingon_processing_completeoverride atgateway/platforms/webhook.py:803is the true completion seam. Commit64ed99a6fixed this exact lifecycle distinction.- Delaying the seen-ID write until after
_direct_deliver()allows concurrent same-ID requests to pass the lookup and both send. Preserve an in-flight claim, removing it only after a direct-delivery failure. hermes-webhook-delivery-retry-after-failure-pr.mdis local PR-draft metadata and should not be tracked.
Suggested changes
- Exercise the real adapter pipeline by stubbing
_message_handler, astests/gateway/test_webhook_session_close.py:114-135does, and clear failed IDs fromon_processing_complete. - Add a concurrent duplicate direct-delivery regression test.
Automated hermes-sweeper review.
| # Skip duplicate deliveries (webhook retries). | ||
| now = time.time() | ||
| if not self._record_delivery_id(delivery_id, now): | ||
| seen_at = self._seen_deliveries.get(delivery_id) |
There was a problem hiding this comment.
This removes the in-flight idempotency claim before _direct_deliver() awaits network I/O. A second concurrent POST with this ID can also pass this lookup and send a duplicate. Keep a pending claim before the await, then remove it only on direct-delivery failure.
| task = asyncio.create_task(self.handle_message(event)) | ||
| self._background_tasks.add(task) | ||
| task.add_done_callback(self._background_tasks.discard) | ||
| def _forget_failed_delivery(done_task: "asyncio.Task") -> None: |
There was a problem hiding this comment.
handle_message() is fire-and-forget: it starts _process_message_background and returns before the agent run. Processing failures are caught inside that later task, so this callback will usually see exc is None. Clear failed IDs from the existing on_processing_complete hook instead, and test by stubbing _message_handler rather than handle_message.
| @@ -0,0 +1,35 @@ | |||
| # PR Draft: Allow webhook retries after downstream delivery failure | |||
There was a problem hiding this comment.
Please remove this PR-draft artifact. It records local branch and review metadata rather than user-facing or developer documentation.
teknium1
added
sweeper:risk-session-state
4a830a0 to
afdca4f
Compare
GottZ
left a comment
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Three PRs address related but distinct webhook delivery defects: #47293 releases failed delivery claims without weakening successful/concurrent deduplication, #47436 preserves home-channel thread metadata for bare platform targets, and #49591 parses inline platform:chat_id targets across both agent and deliver_only paths.
Related pull requests
- #47293
related— (+122/-1) — merge after final re-review: The diff fixes retry poisoning by retaining the in-flight claim and removing it only on direct-delivery failure or ProcessingOutcome.FAILURE at the actual on_processing_complete seam; its concurrent-duplicate regression also preserves single-send behavior. Despite the keep_open review on #47293, the shown diff addresses the cited lifecycle and concurrency objections rather than attaching cleanup to handle_message. - #47436
related— (+154/-4) — merge: The diff carries home.thread_id through bare-target fallback, uses the gateway metadata builder when available, and preserves explicit deliver_extra thread precedence, with coverage for both cross-platform and deliver_only delivery. This directly addresses the keep_open review on #47436, which identifies the same current-main defect and rates the fix highly salvageable. - #49591
related— (+234/-10) — merge after final re-review: The diff adds a shared platform:chat_id parser used by both send() and _direct_deliver(), with inline-target precedence and regression coverage for agent and deliver_only modes. Despite the keep_open review on #49591, the updated diff shown here specifically addresses its two blockers: shared-path normalization and missing parity tests.
Suggested consolidation
Merge #47293, #47436, and #49591 as independent fixes after confirming the updated heads match the shown diffs and pass their webhook test suites. None should be closed as a duplicate: retry claim lifecycle (#47293), home-thread metadata fallback (#47436), and inline chat-target parsing (#49591) correct different causes and can coexist.
Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 30 kB of PR diffs, 5 kB of issue/PR text, 6 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
4 participants
Summary
This changes the webhook adapter so failed deliveries do not poison the idempotency cache.
deliver_onlyroutes now cache only after a successful downstream delivery, and normal webhook runs release the delivery ID if the background agent task fails.Why
deliver_onlyroutes are used for push-style notifications where the webhook POST itself is the delivery. Normal webhook routes also need this behavior: if the agent task fails after accepting the event, the provider should be able to retry the same delivery ID instead of getting a false duplicate response.Changes
deliver_onlyuntil after a successful direct delivery.Tests