diff --git a/.changeset/webhook-trigger-retry-after-success.md b/.changeset/webhook-trigger-retry-after-success.md new file mode 100644 index 000000000..bdc2f797f --- /dev/null +++ b/.changeset/webhook-trigger-retry-after-success.md @@ -0,0 +1,7 @@ +--- +"@slack/webhook": patch +--- + +fix: stop `WebhookTrigger.send()` from retrying after a successful delivery + +A 2xx response with an empty or non-JSON body no longer throws. It now resolves to `{ ok: true }`. Before this fix that body threw a parse error, which was retried as a request error and caused duplicate Workflow Builder runs. A valid `{ ok: false, error }` body is still surfaced to the caller. diff --git a/packages/webhook/src/WebhookTrigger.test.ts b/packages/webhook/src/WebhookTrigger.test.ts index d48b42e1a..303df61eb 100644 --- a/packages/webhook/src/WebhookTrigger.test.ts +++ b/packages/webhook/src/WebhookTrigger.test.ts @@ -77,6 +77,33 @@ describe('WebhookTrigger', () => { assert.strictEqual(result.ok, true); scope.done(); }); + + it('should resolve to { ok: true } on an empty 2xx body', async () => { + const scope = nock('https://hooks.slack.com') + .post(/triggers/) + .reply(200); + const result = await trigger.send({ key: 'value' }); + assert.deepStrictEqual(result, { ok: true }); + scope.done(); + }); + + it('should resolve to { ok: true } on a non-JSON 2xx body', async () => { + const scope = nock('https://hooks.slack.com') + .post(/triggers/) + .reply(200, 'ok'); + const result = await trigger.send({ key: 'value' }); + assert.deepStrictEqual(result, { ok: true }); + scope.done(); + }); + + it('should surface a valid { ok: false } JSON body', async () => { + const scope = nock('https://hooks.slack.com') + .post(/triggers/) + .reply(200, { ok: false, error: 'trigger_error' }); + const result = await trigger.send({ key: 'value' }); + assert.deepStrictEqual(result, { ok: false, error: 'trigger_error' }); + scope.done(); + }); }); describe('on failure', () => { @@ -211,6 +238,16 @@ describe('WebhookTrigger', () => { scope.done(); }); + it('does not retry an empty 2xx body even when a retry policy is set', async () => { + const scope = nock('https://hooks.slack.com') + .post(/triggers/) + .reply(200); + const trigger = new WebhookTrigger(url, { retryConfig: rapidRetryPolicy }); + const result = await trigger.send({ key: 'value' }); + assert.deepStrictEqual(result, { ok: true }); + scope.done(); + }); + it('gives up with the HTTP error after exhausting retries', async () => { const scope = nock('https://hooks.slack.com') .post(/triggers/) diff --git a/packages/webhook/src/WebhookTrigger.ts b/packages/webhook/src/WebhookTrigger.ts index 13946845a..a42187d7b 100644 --- a/packages/webhook/src/WebhookTrigger.ts +++ b/packages/webhook/src/WebhookTrigger.ts @@ -1,7 +1,7 @@ import pRetry, { AbortError } from 'p-retry'; import { SlackWebhookError, WebhookTriggerHTTPError, WebhookTriggerRequestError } from './errors'; -import type { FetchFunction } from './IncomingWebhook'; +import type { FetchFunction, FetchResponse } from './IncomingWebhook'; import { getUserAgent } from './instrument'; import type { RetryOptions } from './retry-policies'; @@ -81,7 +81,7 @@ export class WebhookTrigger { throw response.status >= 500 ? httpError : new AbortError(httpError); } - return (await response.json()) as WebhookTriggerResult; + return await this.buildResult(response); } catch (error) { // Non-retryable signals (AbortError) and already-wrapped errors pass through untouched. if (error instanceof AbortError || error instanceof SlackWebhookError) { @@ -92,6 +92,15 @@ export class WebhookTrigger { } }, this.retryConfig); } + + private async buildResult(response: FetchResponse): Promise { + const text = await response.text(); + try { + return text ? (JSON.parse(text) as WebhookTriggerResult) : { ok: true }; + } catch { + return { ok: true }; + } + } } /*