From 24daca2cbbd9cd1d5ba7bdcb9c496e62b2ffb5c5 Mon Sep 17 00:00:00 2001 From: Copilot Date: Tue, 14 Apr 2026 09:19:57 +0000 Subject: [PATCH] =?UTF-8?q?[copilot-finds]=20Bug:=20Fix=20off-by-one=20in?= =?UTF-8?q?=20createServiceConfig=20maxRetries=E2=86=92maxAttempts=20conve?= =?UTF-8?q?rsion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The createServiceConfig() function used the maxRetries option value directly as the gRPC maxAttempts field. However, gRPC's maxAttempts includes the initial attempt, while maxRetries means retries after the initial call. This caused users to get one fewer retry than configured. Setting maxRetries: 0 also produced an invalid maxAttempts: 0 config. Fix: Add +1 when converting maxRetries to maxAttempts. Also add comprehensive unit tests for the entire retry-policy module which previously had zero test coverage. Fixes #221 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/retry-policy.ts | 4 +- .../test/unit/retry-policy.spec.ts | 185 ++++++++++++++++++ 2 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 packages/durabletask-js-azuremanaged/test/unit/retry-policy.spec.ts diff --git a/packages/durabletask-js-azuremanaged/src/retry-policy.ts b/packages/durabletask-js-azuremanaged/src/retry-policy.ts index 9f889ac..f3b331a 100644 --- a/packages/durabletask-js-azuremanaged/src/retry-policy.ts +++ b/packages/durabletask-js-azuremanaged/src/retry-policy.ts @@ -88,7 +88,9 @@ function statusCodeToName(code: GrpcStatus): string { * @returns A JSON string representing the service config. */ export function createServiceConfig(options?: ClientRetryOptions): string { - const maxAttempts = options?.maxRetries ?? DEFAULT_MAX_ATTEMPTS; + // gRPC maxAttempts includes the original attempt, while maxRetries means + // retries *after* the initial call. Convert by adding 1. + const maxAttempts = options?.maxRetries != null ? options.maxRetries + 1 : DEFAULT_MAX_ATTEMPTS; const initialBackoffMs = options?.initialBackoffMs ?? DEFAULT_INITIAL_BACKOFF_MS; const maxBackoffMs = options?.maxBackoffMs ?? DEFAULT_MAX_BACKOFF_MS; const backoffMultiplier = options?.backoffMultiplier ?? DEFAULT_BACKOFF_MULTIPLIER; diff --git a/packages/durabletask-js-azuremanaged/test/unit/retry-policy.spec.ts b/packages/durabletask-js-azuremanaged/test/unit/retry-policy.spec.ts new file mode 100644 index 0000000..caf3dcf --- /dev/null +++ b/packages/durabletask-js-azuremanaged/test/unit/retry-policy.spec.ts @@ -0,0 +1,185 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { status as GrpcStatus } from "@grpc/grpc-js"; +import { + createServiceConfig, + DEFAULT_SERVICE_CONFIG, + DEFAULT_MAX_ATTEMPTS, + DEFAULT_INITIAL_BACKOFF_MS, + DEFAULT_MAX_BACKOFF_MS, + DEFAULT_BACKOFF_MULTIPLIER, + ClientRetryOptions, +} from "../../src/retry-policy"; + +describe("retry-policy", () => { + describe("DEFAULT_SERVICE_CONFIG", () => { + it("should be valid JSON", () => { + expect(() => JSON.parse(DEFAULT_SERVICE_CONFIG)).not.toThrow(); + }); + + it("should contain a retry policy", () => { + const config = JSON.parse(DEFAULT_SERVICE_CONFIG); + const retryPolicy = config.methodConfig[0].retryPolicy; + expect(retryPolicy).toBeDefined(); + }); + + it("should use default values", () => { + const config = JSON.parse(DEFAULT_SERVICE_CONFIG); + const retryPolicy = config.methodConfig[0].retryPolicy; + + expect(retryPolicy.maxAttempts).toBe(DEFAULT_MAX_ATTEMPTS); + expect(retryPolicy.initialBackoff).toBe(`${DEFAULT_INITIAL_BACKOFF_MS / 1000}s`); + expect(retryPolicy.maxBackoff).toBe(`${DEFAULT_MAX_BACKOFF_MS / 1000}s`); + expect(retryPolicy.backoffMultiplier).toBe(DEFAULT_BACKOFF_MULTIPLIER); + }); + + it("should include UNAVAILABLE status code by default", () => { + const config = JSON.parse(DEFAULT_SERVICE_CONFIG); + const retryPolicy = config.methodConfig[0].retryPolicy; + expect(retryPolicy.retryableStatusCodes).toContain("UNAVAILABLE"); + }); + + it("should apply to all gRPC methods via empty service name", () => { + const config = JSON.parse(DEFAULT_SERVICE_CONFIG); + expect(config.methodConfig[0].name).toEqual([{ service: "" }]); + }); + }); + + describe("createServiceConfig", () => { + it("should return the same config as DEFAULT_SERVICE_CONFIG when called without options", () => { + const config = createServiceConfig(); + expect(config).toBe(DEFAULT_SERVICE_CONFIG); + }); + + it("should return the same config as DEFAULT_SERVICE_CONFIG when called with undefined", () => { + const config = createServiceConfig(undefined); + expect(JSON.parse(config)).toEqual(JSON.parse(DEFAULT_SERVICE_CONFIG)); + }); + + describe("maxRetries to maxAttempts conversion", () => { + it("should convert maxRetries to maxAttempts by adding 1 (retries + initial attempt)", () => { + const config = JSON.parse(createServiceConfig({ maxRetries: 3 })); + // maxRetries: 3 means 3 retries + 1 initial = 4 total attempts + expect(config.methodConfig[0].retryPolicy.maxAttempts).toBe(4); + }); + + it("should produce maxAttempts: 1 when maxRetries is 0 (no retries, just the initial call)", () => { + const config = JSON.parse(createServiceConfig({ maxRetries: 0 })); + // maxRetries: 0 means 0 retries + 1 initial = 1 total attempt + expect(config.methodConfig[0].retryPolicy.maxAttempts).toBe(1); + }); + + it("should produce maxAttempts: 2 when maxRetries is 1", () => { + const config = JSON.parse(createServiceConfig({ maxRetries: 1 })); + expect(config.methodConfig[0].retryPolicy.maxAttempts).toBe(2); + }); + + it("should use DEFAULT_MAX_ATTEMPTS when maxRetries is not specified", () => { + const config = JSON.parse(createServiceConfig({})); + expect(config.methodConfig[0].retryPolicy.maxAttempts).toBe(DEFAULT_MAX_ATTEMPTS); + }); + }); + + describe("backoff configuration", () => { + it("should convert initialBackoffMs to seconds string", () => { + const config = JSON.parse(createServiceConfig({ initialBackoffMs: 100 })); + expect(config.methodConfig[0].retryPolicy.initialBackoff).toBe("0.1s"); + }); + + it("should convert maxBackoffMs to seconds string", () => { + const config = JSON.parse(createServiceConfig({ maxBackoffMs: 5000 })); + expect(config.methodConfig[0].retryPolicy.maxBackoff).toBe("5s"); + }); + + it("should apply the specified backoff multiplier", () => { + const config = JSON.parse(createServiceConfig({ backoffMultiplier: 3 })); + expect(config.methodConfig[0].retryPolicy.backoffMultiplier).toBe(3); + }); + }); + + describe("retryable status codes", () => { + it("should always include UNAVAILABLE even when custom codes are specified", () => { + const config = JSON.parse( + createServiceConfig({ + retryableStatusCodes: [GrpcStatus.INTERNAL], + }), + ); + const statusCodes = config.methodConfig[0].retryPolicy.retryableStatusCodes; + expect(statusCodes).toContain("UNAVAILABLE"); + expect(statusCodes).toContain("INTERNAL"); + }); + + it("should not duplicate UNAVAILABLE when already in custom codes", () => { + const config = JSON.parse( + createServiceConfig({ + retryableStatusCodes: [GrpcStatus.UNAVAILABLE, GrpcStatus.INTERNAL], + }), + ); + const statusCodes: string[] = config.methodConfig[0].retryPolicy.retryableStatusCodes; + const unavailableCount = statusCodes.filter((c) => c === "UNAVAILABLE").length; + expect(unavailableCount).toBe(1); + }); + + it("should map all standard gRPC status codes correctly", () => { + const allCodes = [ + { code: GrpcStatus.OK, name: "OK" }, + { code: GrpcStatus.CANCELLED, name: "CANCELLED" }, + { code: GrpcStatus.UNKNOWN, name: "UNKNOWN" }, + { code: GrpcStatus.INVALID_ARGUMENT, name: "INVALID_ARGUMENT" }, + { code: GrpcStatus.DEADLINE_EXCEEDED, name: "DEADLINE_EXCEEDED" }, + { code: GrpcStatus.NOT_FOUND, name: "NOT_FOUND" }, + { code: GrpcStatus.ALREADY_EXISTS, name: "ALREADY_EXISTS" }, + { code: GrpcStatus.PERMISSION_DENIED, name: "PERMISSION_DENIED" }, + { code: GrpcStatus.RESOURCE_EXHAUSTED, name: "RESOURCE_EXHAUSTED" }, + { code: GrpcStatus.FAILED_PRECONDITION, name: "FAILED_PRECONDITION" }, + { code: GrpcStatus.ABORTED, name: "ABORTED" }, + { code: GrpcStatus.OUT_OF_RANGE, name: "OUT_OF_RANGE" }, + { code: GrpcStatus.UNIMPLEMENTED, name: "UNIMPLEMENTED" }, + { code: GrpcStatus.INTERNAL, name: "INTERNAL" }, + { code: GrpcStatus.UNAVAILABLE, name: "UNAVAILABLE" }, + { code: GrpcStatus.DATA_LOSS, name: "DATA_LOSS" }, + { code: GrpcStatus.UNAUTHENTICATED, name: "UNAUTHENTICATED" }, + ]; + + for (const { code, name } of allCodes) { + const config = JSON.parse( + createServiceConfig({ retryableStatusCodes: [code] }), + ); + const statusCodes: string[] = config.methodConfig[0].retryPolicy.retryableStatusCodes; + expect(statusCodes).toContain(name); + } + }); + + it("should fall back to 'UNKNOWN' for unrecognized status codes", () => { + const config = JSON.parse( + createServiceConfig({ retryableStatusCodes: [999 as GrpcStatus] }), + ); + const statusCodes: string[] = config.methodConfig[0].retryPolicy.retryableStatusCodes; + expect(statusCodes).toContain("UNKNOWN"); + }); + }); + + describe("full custom configuration", () => { + it("should apply all custom options together", () => { + const options: ClientRetryOptions = { + maxRetries: 5, + initialBackoffMs: 200, + maxBackoffMs: 2000, + backoffMultiplier: 1.5, + retryableStatusCodes: [GrpcStatus.UNAVAILABLE, GrpcStatus.DEADLINE_EXCEEDED], + }; + + const config = JSON.parse(createServiceConfig(options)); + const retryPolicy = config.methodConfig[0].retryPolicy; + + expect(retryPolicy.maxAttempts).toBe(6); // 5 retries + 1 initial + expect(retryPolicy.initialBackoff).toBe("0.2s"); + expect(retryPolicy.maxBackoff).toBe("2s"); + expect(retryPolicy.backoffMultiplier).toBe(1.5); + expect(retryPolicy.retryableStatusCodes).toContain("UNAVAILABLE"); + expect(retryPolicy.retryableStatusCodes).toContain("DEADLINE_EXCEEDED"); + }); + }); + }); +});