logger: retry Slack webhook rate limits - #4957
Conversation
|
|
||
| export function isRetryableSlackPostError(error: unknown): boolean { | ||
| const status = getErrorStatus(error); | ||
| return status !== undefined && SLACK_RETRYABLE_STATUS_CODES.has(status); |
There was a problem hiding this comment.
isRetryableSlackPostError only returns true when the error carries an HTTP status in the retryable set. Requests that fail without a response — timeouts, ECONNRESET, DNS failures — have status === undefined, so they are not retried and surface as a TransportError on the first attempt. Since connection-level failures are among the most common transient Slack delivery issues (and the PR title targets transient failures broadly), consider retrying those too, or documenting that only status-coded failures are in scope.
There was a problem hiding this comment.
Agreed that the original title and behavior were broader than the safe retry boundary. The incident review showed that every inspected page was an HTTP 429, so I narrowed retries to 429 only and added coverage proving connection-level failures fail fast. Slack webhook posts are not idempotent, so a no-response failure is delivery-ambiguous and retrying it could duplicate the alert. I also updated the PR title and description to state that rate limits, rather than every transient failure, are in scope.
| export const SLACK_MAX_RETRY_DELAY_SECONDS = 5; | ||
|
|
||
| const SLACK_DEFAULT_RETRY_DELAY_SECONDS = 1; | ||
| const SLACK_RETRYABLE_STATUS_CODES = new Set([429, 500, 502, 503, 504]); |
There was a problem hiding this comment.
Retrying 5xx responses can lead to duplicate Slack messages: a 500/502/503/504 from an intermediary can occur after Slack already accepted the payload, and the retry re-posts the same chunk. Low impact for monitoring alerts, but worth noting since webhook posts aren't idempotent (429 is safe here since it's rejected, not delivered).
There was a problem hiding this comment.
Agreed. I removed retries for 500/502/503/504 and added a test asserting that a 503 is surfaced after one attempt. The helper now retries only Slack 429, where the webhook post was rejected rather than ambiguously accepted. I also raised the Retry-After safety cap from 5 to 60 seconds so the implementation can follow Slack's documented backoff signal during the rate-limit bursts seen in PagerDuty.
Signed-off-by: Chris Maree <[email protected]>
600600d to
0cc19d3
Compare
md0x
left a comment
There was a problem hiding this comment.
I reviewed and didn't find anything! LGTM
The "Runs with no errors" test read the real wall clock, so any run within SKIP_THRESHOLD_SECONDS (4h) of a two-day phase boundary took the skip-relay short circuit in run(), which logs a single message and makes spy.getCall(-2) return null. Pin Date to the middle of a phase with fake timers, matching the other tests in this file, so the assertion path is deterministic regardless of when the suite runs. Signed-off-by: droplet-rl <[email protected]>
What Changed
429responses.Retry-Afterheaders, including numeric and date forms, with a 60-second safety cap.TransportError.5xxresponses or connection-level failures because incoming webhook posts are not idempotent and delivery is ambiguous in those cases.Retry-Aftercapping.Why
429responses.429responses include the number of seconds to wait inRetry-After.429immediately became aTransportError. Waiting and retrying gives Slack a chance to accept the rejected post instead of surfacing a delivery failure.Impact
5xxor connection failures.High risk Sections to review with detail
postWithRetryinSlackTransport.ts: retry count and delay handling determine how long a logger write can wait.getSlackPostRetryDelaySeconds: parses Slack response headers and caps a single wait at 60 seconds.Validation
git diff --checksuccessfully.Docs