diff --git a/packages/appkit/src/plugin/interceptors/retry.ts b/packages/appkit/src/plugin/interceptors/retry.ts index 435e0fde..aeddf3d5 100644 --- a/packages/appkit/src/plugin/interceptors/retry.ts +++ b/packages/appkit/src/plugin/interceptors/retry.ts @@ -1,10 +1,38 @@ import type { RetryConfig } from "shared"; +import { AppKitError } from "../../errors/base"; import { createLogger } from "../../logging/logger"; import type { ExecutionInterceptor, InterceptorContext } from "./types"; const logger = createLogger("interceptors:retry"); -// interceptor to handle retry logic +/** + * Determines whether an error is safe to retry. + * + * Priority: + * 1. AppKitError — reads the `isRetryable` boolean property. + * 2. Databricks SDK ApiError (duck-typed) — calls `isRetryable()` method, + * or falls back to status-code heuristic (5xx / 429 → retryable). + * 3. Unknown errors — treated as retryable to preserve backward compatibility. + */ +function isRetryableError(error: unknown): boolean { + if (error instanceof AppKitError) { + return error.isRetryable; + } + + if (error instanceof Error && "statusCode" in error) { + const record = error as Record; + if (typeof record.statusCode !== "number") { + return true; + } + if (typeof record.isRetryable === "function") { + return (record.isRetryable as () => boolean)(); + } + return record.statusCode >= 500 || record.statusCode === 429; + } + + return true; +} + export class RetryInterceptor implements ExecutionInterceptor { private attempts: number; private initialDelay: number; @@ -36,7 +64,6 @@ export class RetryInterceptor implements ExecutionInterceptor { } catch (error) { lastError = error; - // last attempt, rethrow the error if (attempt === this.attempts) { logger.event()?.setExecution({ retry_attempts: attempt - 1, @@ -44,17 +71,19 @@ export class RetryInterceptor implements ExecutionInterceptor { throw error; } - // don't retry if was already aborted if (context.signal?.aborted) { throw error; } + if (!isRetryableError(error)) { + throw error; + } + const delay = this.calculateDelay(attempt); await this.sleep(delay); } } - // type guard throw lastError; } diff --git a/packages/appkit/src/plugin/tests/retry.test.ts b/packages/appkit/src/plugin/tests/retry.test.ts index b897c585..f6976f11 100644 --- a/packages/appkit/src/plugin/tests/retry.test.ts +++ b/packages/appkit/src/plugin/tests/retry.test.ts @@ -1,5 +1,9 @@ import type { RetryConfig } from "shared"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { AuthenticationError } from "../../errors/authentication"; +import { ConnectionError } from "../../errors/connection"; +import { ExecutionError } from "../../errors/execution"; +import { ValidationError } from "../../errors/validation"; import { RetryInterceptor } from "../interceptors/retry"; import type { InterceptorContext } from "../interceptors/types"; @@ -241,4 +245,191 @@ describe("RetryInterceptor", () => { vi.spyOn(Math, "random").mockRestore(); }); + + test("should not retry AuthenticationError (isRetryable=false)", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const fn = vi.fn().mockRejectedValue(AuthenticationError.missingToken()); + + await expect(interceptor.intercept(fn, context)).rejects.toThrow( + AuthenticationError, + ); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test("should not retry ValidationError (isRetryable=false)", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const fn = vi.fn().mockRejectedValue(ValidationError.missingField("name")); + + await expect(interceptor.intercept(fn, context)).rejects.toThrow( + ValidationError, + ); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test("should not retry ExecutionError (isRetryable=false)", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const fn = vi + .fn() + .mockRejectedValue(ExecutionError.statementFailed("syntax error")); + + await expect(interceptor.intercept(fn, context)).rejects.toThrow( + ExecutionError, + ); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test("should retry ConnectionError (isRetryable=true)", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const fn = vi + .fn() + .mockRejectedValueOnce(ConnectionError.queryFailed()) + .mockResolvedValue("recovered"); + + const promise = interceptor.intercept(fn, context); + await vi.runAllTimersAsync(); + + expect(await promise).toBe("recovered"); + expect(fn).toHaveBeenCalledTimes(2); + }); + + test("should not retry errors with 4xx statusCode", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const error = Object.assign(new Error("bad request"), { statusCode: 400 }); + const fn = vi.fn().mockRejectedValue(error); + + await expect(interceptor.intercept(fn, context)).rejects.toThrow( + "bad request", + ); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test("should retry errors with 5xx statusCode", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const fn = vi + .fn() + .mockRejectedValueOnce( + Object.assign(new Error("internal"), { statusCode: 500 }), + ) + .mockResolvedValue("recovered"); + + const promise = interceptor.intercept(fn, context); + await vi.runAllTimersAsync(); + + expect(await promise).toBe("recovered"); + expect(fn).toHaveBeenCalledTimes(2); + }); + + test("should retry errors with 429 statusCode (rate limit)", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const fn = vi + .fn() + .mockRejectedValueOnce( + Object.assign(new Error("rate limited"), { statusCode: 429 }), + ) + .mockResolvedValue("recovered"); + + const promise = interceptor.intercept(fn, context); + await vi.runAllTimersAsync(); + + expect(await promise).toBe("recovered"); + expect(fn).toHaveBeenCalledTimes(2); + }); + + test("should use isRetryable() method when available on error", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + + const nonRetryable = Object.assign(new Error("not found"), { + statusCode: 404, + isRetryable: () => false, + }); + const fn = vi.fn().mockRejectedValue(nonRetryable); + + await expect(interceptor.intercept(fn, context)).rejects.toThrow( + "not found", + ); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test("should respect isRetryable() returning true even for 4xx", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + + const retryableClientError = Object.assign(new Error("conflict"), { + statusCode: 409, + isRetryable: () => true, + }); + const fn = vi + .fn() + .mockRejectedValueOnce(retryableClientError) + .mockResolvedValue("ok"); + + const promise = interceptor.intercept(fn, context); + await vi.runAllTimersAsync(); + + expect(await promise).toBe("ok"); + expect(fn).toHaveBeenCalledTimes(2); + }); + + test("should still retry plain Error (backward compatibility)", async () => { + const config: RetryConfig = { + enabled: true, + attempts: 3, + initialDelay: 1000, + }; + const interceptor = new RetryInterceptor(config); + const fn = vi + .fn() + .mockRejectedValueOnce(new Error("transient")) + .mockResolvedValue("ok"); + + const promise = interceptor.intercept(fn, context); + await vi.runAllTimersAsync(); + + expect(await promise).toBe("ok"); + expect(fn).toHaveBeenCalledTimes(2); + }); });