Skip to content

retry.helper.ts is dead code, duplicated ad hoc by WebhookService, and isRetryable() has a bug #239

Description

@meshackyaro

Description

src/webhook/retry.helper.ts exports withRetry() and isRetryable(), but a repo-wide search shows neither is imported anywhere — WebhookService.sendWithRetry() (src/webhook/webhook.service.ts) independently re-implements the same retry-with-backoff pattern inline instead of using the shared helper. Separately, isRetryable()'s implementation has a real bug: error.message.includes('ECONNREFUSED') || error.message.includes('ETIMEDOUT') || error.message.includes('5') — the last check matches the literal character '5' appearing anywhere in the error message, not an HTTP 5xx status code. Any error message that happens to contain the digit 5 for unrelated reasons (e.g. a URL, a byte count, a port number) would be misclassified as retryable.

Component

Backend

Difficulty

🟢 Easy

Tasks

  • Either wire withRetry()/isRetryable() into WebhookService.sendWithRetry() (removing the duplicated inline logic), or remove retry.helper.ts if it's superseded and not meant to be reused
  • Fix isRetryable()'s status-code check to match actual HTTP 5xx status codes (e.g. a regex like /^5\d{2}$/ against a parsed status, not a raw substring match on the message)
  • Add src/webhook/retry.helper.spec.ts covering the fixed retryability logic with representative error messages

Acceptance Criteria

  • There is a single, correct implementation of retry/backoff and retryability logic — not one used and one dead
  • isRetryable() correctly distinguishes real 5xx-indicating errors from messages that merely contain the digit 5
  • A test covers the fix

Estimated Time

3-5 hours

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions