fix(webhook): do not retry after a successful trigger delivery - #2674
fix(webhook): do not retry after a successful trigger delivery#2674WilliamBergamin wants to merge 3 commits into
Conversation
WebhookTrigger.send() read the success body with response.json(), which
throws on an empty or non-JSON 2xx body. That error was wrapped as a
retryable WebhookTriggerRequestError, so a trigger that already fired got
resent, causing duplicate Workflow Builder runs.
Read the 2xx body tolerantly via a new buildResult() that mirrors
IncomingWebhook: an empty or unparseable body resolves to { ok: true },
while a valid JSON body (including { ok: false, error }) is still surfaced.
A genuine mid-body read failure still propagates as retryable.
Fixes #2673
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
🦋 Changeset detectedLatest commit: d73a09d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2674 +/- ##
==========================================
- Coverage 89.11% 89.10% -0.01%
==========================================
Files 65 65
Lines 10339 10348 +9
Branches 471 474 +3
==========================================
+ Hits 9214 9221 +7
- Misses 1095 1097 +2
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin Thanks for making these technique more stable! 🧰 ✨
I'm leaving one comment on perhaps changes but don't think it should block so feel free to ignore and ship 🚢 💨
| } | ||
|
|
||
| return (await response.json()) as WebhookTriggerResult; | ||
| return await this.buildResult(response); |
| private async buildResult(response: FetchResponse): Promise<WebhookTriggerResult> { | ||
| const text = await response.text(); | ||
| try { | ||
| return text ? (JSON.parse(text) as WebhookTriggerResult) : { ok: true }; |
There was a problem hiding this comment.
🪬 quibble: I'd favor a separate if statement for blank responses but no blocker if this doesn't seem right!
| return text ? (JSON.parse(text) as WebhookTriggerResult) : { ok: true }; | |
| if (!text) { | |
| return { ok: true }; | |
| } | |
| return JSON.parse(text) as WebhookTriggerResult; |
Summary
Fixes #2673.
WebhookTrigger.send()retried after a trigger had already fired. This caused duplicate Workflow Builder runs.The cause is the success path. It read the body with
response.json(). That call throws on an empty or non-JSON 2xx body. The thrown error was wrapped as aWebhookTriggerRequestError, whichp-retrytreats as retryable. So a trigger that already succeeded got sent again.The fix reads the 2xx body tolerantly in a new
buildResult()method. It mirrorsIncomingWebhook.Requirements