fix(mattermost): parse status code before matching generic '429' text

- Fix isRetryableError: check 'mattermost api 429' status code BEFORE
  generic '429' text match to avoid false positives
- Error messages containing '429' in detail (e.g., 'Invalid ID: 4294967295')
  are no longer incorrectly treated as rate limiting
- Add test for 400 error containing '429' text to verify no false retry
This commit is contained in:
Jonathan Jing
2026-03-10 12:58:47 -07:00
committed by Muhammed Mukhthar CM
parent ead65cf04c
commit 84fa7e2baa
2 changed files with 31 additions and 6 deletions

View File

@@ -98,6 +98,30 @@ describe("createMattermostDirectChannelWithRetry", () => {
expect(result.id).toBe("dm-channel-port");
});
it("does not retry on 400 even if error message contains '429' text", async () => {
// This tests that "429" in error detail doesn't trigger false rate-limit retry
// e.g., "Invalid user ID: 4294967295" should NOT be retried
mockFetch.mockResolvedValueOnce({
ok: false,
status: 400,
headers: new Headers({ "content-type": "application/json" }),
json: async () => ({ message: "Invalid user ID: 4294967295" }),
text: async () => "Invalid user ID: 4294967295",
} as Response);
const client = createMockClient();
await expect(
createMattermostDirectChannelWithRetry(client, ["user-1", "user-2"], {
maxRetries: 3,
initialDelayMs: 10,
}),
).rejects.toThrow();
// Should not retry - only called once (400 is a client error, even though message contains "429")
expect(mockFetch).toHaveBeenCalledTimes(1);
});
it("retries on 5xx server errors", async () => {
mockFetch
.mockResolvedValueOnce({

View File

@@ -257,11 +257,6 @@ export async function createMattermostDirectChannelWithRetry(
function isRetryableError(error: Error): boolean {
const message = error.message.toLowerCase();
// Retry on rate limiting (429)
if (message.includes("429") || message.includes("too many requests")) {
return true;
}
// Retry on 5xx server errors FIRST (before checking 4xx)
// Use "mattermost api" prefix to avoid matching port numbers (e.g., :443) or IP octets
// This prevents misclassification when a 5xx error detail contains a 4xx substring
@@ -270,13 +265,19 @@ function isRetryableError(error: Error): boolean {
return true;
}
// Check for explicit 429 rate limiting FIRST (before generic "429" text match)
// This avoids retrying when error detail contains "429" but it's not the status code
if (/mattermost api 429\b/.test(message) || message.includes("too many requests")) {
return true;
}
// Check for explicit 4xx status codes - these are client errors and should NOT be retried
// (except 429 which is handled above)
// Use "mattermost api" prefix to avoid matching port numbers like :443
const clientErrorMatch = message.match(/mattermost api (4\d{2})\b/);
if (clientErrorMatch) {
const statusCode = parseInt(clientErrorMatch[1], 10);
if (statusCode >= 400 && statusCode < 500 && statusCode !== 429) {
if (statusCode >= 400 && statusCode < 500) {
return false;
}
}