diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 703c80a9dad6..4af177e3a4ce 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -421,7 +421,7 @@ export interface IContainer extends IEventProvider { * * By default, the container will close if attach fails. * However, closure can now be avoided in most cased by setting: - * Fluid.Container.RetryOnAttachFailure to true + * Fluid.Container.DisableCloseOnAttachFailure to true * via the config provider passed to the loader. * * If attach fails, check the closed property to discover if retry is possible. @@ -431,7 +431,9 @@ export interface IContainer extends IEventProvider { */ attach( request: IRequest, - attachProps?: { deltaConnection?: "none" | "delayed" }, + attachProps?: { + deltaConnection?: "none" | "delayed"; + }, ): Promise; /** diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index f6035607f005..fcda91121a38 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -1301,7 +1301,9 @@ export class Container }); // only enable the new behavior if the config is set - if (this.mc.config.getBoolean("Fluid.Container.RetryOnAttachFailure") !== true) { + if ( + this.mc.config.getBoolean("Fluid.Container.DisableCloseOnAttachFailure") !== true + ) { attachP = attachP.catch((error) => { throw normalizeErrorAndClose(error); }); @@ -1547,6 +1549,12 @@ export class Container service.on("metadataUpdate", this.metadataUpdateHandler); } } else { + // When DisableCloseOnAttachFailure is enabled, use no internal retries + // The consumer will own the retry policy + const disableCloseOnAttachFailure = + this.mc.config.getBoolean("Fluid.Container.DisableCloseOnAttachFailure") === true; + const maxRetries = disableCloseOnAttachFailure ? 0 : undefined; + service = await runWithRetry( async () => this.serviceFactory.createContainer( @@ -1560,6 +1568,7 @@ export class Container { cancel: this._deltaManager.closeAbortController.signal, }, // progress + maxRetries, ); } return service; diff --git a/packages/loader/container-loader/src/test/loader.spec.ts b/packages/loader/container-loader/src/test/loader.spec.ts index 3072a99416ba..cae59fc32d3b 100644 --- a/packages/loader/container-loader/src/test/loader.spec.ts +++ b/packages/loader/container-loader/src/test/loader.spec.ts @@ -98,7 +98,7 @@ describe("loader unit test", () => { urlResolver: failProxy(), configProvider: { getRawConfig: (name): ConfigTypes => - name === "Fluid.Container.RetryOnAttachFailure" ? true : undefined, + name === "Fluid.Container.DisableCloseOnAttachFailure" ? true : undefined, }, }); const detached = await loader.createDetachedContainer({ package: "none" }); @@ -136,7 +136,7 @@ describe("loader unit test", () => { }), configProvider: { getRawConfig: (name): ConfigTypes => - name === "Fluid.Container.RetryOnAttachFailure" ? true : undefined, + name === "Fluid.Container.DisableCloseOnAttachFailure" ? true : undefined, }, }); const detached = await loader.createDetachedContainer({ package: "none" }); diff --git a/packages/loader/driver-utils/src/runWithRetry.ts b/packages/loader/driver-utils/src/runWithRetry.ts index 5d5da9307dd1..1d62e9cae521 100644 --- a/packages/loader/driver-utils/src/runWithRetry.ts +++ b/packages/loader/driver-utils/src/runWithRetry.ts @@ -8,6 +8,7 @@ import { delay } from "@fluidframework/core-utils/internal"; import { DriverErrorTypes } from "@fluidframework/driver-definitions/internal"; import { isFluidError, + wrapError, type ITelemetryLoggerExt, } from "@fluidframework/telemetry-utils/internal"; @@ -57,6 +58,7 @@ export async function runWithRetry( fetchCallName: string, logger: ITelemetryLoggerExt, progress: IProgress, + maxRetries?: number, ): Promise { let result: T | undefined; let success = false; @@ -122,6 +124,35 @@ export async function runWithRetry( } numRetries++; + + // Check if max retries limit has been reached + if (maxRetries !== undefined && numRetries > maxRetries) { + logger.sendTelemetryEvent( + { + eventName: `${fetchCallName}_maxRetriesExceeded`, + retry: numRetries - 1, + maxRetries, + duration: performanceNow() - startTime, + fetchCallName, + }, + error, + ); + // Wrap the original error to preserve its details while marking it non-retriable + throw wrapError( + error, + (message) => + new NonRetryableError( + `runWithRetry failed after max retries: ${message}`, + DriverErrorTypes.genericError, + { + driverVersion: pkgVersion, + fetchCallName, + maxRetries, + }, + ), + ); + } + lastError = error; // Wait for the calculated time before retrying. retryAfterMs = calculateMaxWaitTime(retryAfterMs, error); diff --git a/packages/loader/driver-utils/src/test/runWithRetry.spec.ts b/packages/loader/driver-utils/src/test/runWithRetry.spec.ts index ce18a8334b01..255d8f5d28a1 100644 --- a/packages/loader/driver-utils/src/test/runWithRetry.spec.ts +++ b/packages/loader/driver-utils/src/test/runWithRetry.spec.ts @@ -221,4 +221,100 @@ describe("runWithRetry Tests", () => { assert.strictEqual((error as any).reason, "Sample abort reason"); } }); + + it("Should stop retrying after maxRetries is exceeded", async () => { + let retryTimes = 0; + const api = async (): Promise => { + retryTimes += 1; + const error = new Error("Throw error"); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access -- TODO: use a real type + (error as any).canRetry = true; + throw error; + }; + + try { + await runWithFastSetTimeout(async () => runWithRetry(api, "test", logger, {}, 3)); + assert.fail("Should not succeed"); + } catch (error) { + // Verify the wrapped error includes the original error message + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment -- TODO: use a real type + const errorMessage = (error as any).message; + assert.strictEqual(errorMessage, "runWithRetry failed after max retries: Throw error"); + // Verify the original error is preserved in the cause property + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment -- TODO: use a real type + const causeMessage = (error as any).cause?.message; + assert.strictEqual(causeMessage, "Throw error"); + } + // Initial call + maxRetries attempts + assert.strictEqual(retryTimes, 4, "Should retry exactly maxRetries times"); + }); + + it("Should succeed before maxRetries is exceeded", async () => { + let retryTimes = 0; + const api = async (): Promise => { + retryTimes += 1; + // Succeed on the 3rd attempt (after 2 failures) + if (retryTimes < 3) { + const error = new Error("Throw error"); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access -- TODO: use a real type + (error as any).canRetry = true; + throw error; + } + return true; + }; + + const success = await runWithFastSetTimeout(async () => + runWithRetry(api, "test", logger, {}, 5), + ); + assert.strictEqual(success, true, "Should succeed"); + assert.strictEqual(retryTimes, 3, "Should take 3 attempts to succeed"); + }); + + it("Should retry infinitely when maxRetries is undefined", async () => { + const totalRetries = 10; + let retryTimes = 0; + const api = async (): Promise => { + retryTimes += 1; + if (retryTimes <= totalRetries) { + const error = new Error("Throw error"); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access -- TODO: use a real type + (error as any).canRetry = true; + throw error; + } + return true; + }; + + const success = await runWithFastSetTimeout(async () => + runWithRetry(api, "test", logger, {}), + ); + assert.strictEqual(success, true, "Should succeed"); + assert.strictEqual(retryTimes, totalRetries + 1, "Should retry until success"); + }); + + it("Should fail immediately with maxRetries set to 0", async () => { + let retryTimes = 0; + const api = async (): Promise => { + retryTimes += 1; + const error = new Error("Throw error"); + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access -- TODO: use a real type + (error as any).canRetry = true; + throw error; + }; + + try { + await runWithFastSetTimeout(async () => runWithRetry(api, "test", logger, {}, 0)); + assert.fail("Should not succeed"); + } catch (error) { + // Verify the wrapped error includes the original error message + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment -- TODO: use a real type + const errorMessage = (error as any).message; + assert.strictEqual(errorMessage, "runWithRetry failed after max retries: Throw error"); + // Verify the original error is preserved in the cause property + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment -- TODO: use a real type + const causeMessage = (error as any).cause?.message; + assert.strictEqual(causeMessage, "Throw error"); + } + // Only the initial call, no retries + assert.strictEqual(retryTimes, 1, "Should not retry at all"); + }); }); diff --git a/packages/test/test-end-to-end-tests/src/test/blobs.spec.ts b/packages/test/test-end-to-end-tests/src/test/blobs.spec.ts index 724df0a58b23..fce8f07f43cd 100644 --- a/packages/test/test-end-to-end-tests/src/test/blobs.spec.ts +++ b/packages/test/test-end-to-end-tests/src/test/blobs.spec.ts @@ -583,7 +583,7 @@ function serializationTests({ loaderProps: { documentServiceFactory, configProvider: createTestConfigProvider({ - "Fluid.Container.RetryOnAttachFailure": true, + "Fluid.Container.DisableCloseOnAttachFailure": true, }), }, }); diff --git a/packages/test/test-end-to-end-tests/src/test/detachedContainerTests.spec.ts b/packages/test/test-end-to-end-tests/src/test/detachedContainerTests.spec.ts index c2625763be88..14a298fb1c30 100644 --- a/packages/test/test-end-to-end-tests/src/test/detachedContainerTests.spec.ts +++ b/packages/test/test-end-to-end-tests/src/test/detachedContainerTests.spec.ts @@ -956,7 +956,7 @@ describeCompat("Detached Container", "NoCompat", (getTestObjectProvider, apis) = { configProvider: { getRawConfig: (name) => - name === "Fluid.Container.RetryOnAttachFailure" ? true : undefined, + name === "Fluid.Container.DisableCloseOnAttachFailure" ? true : undefined, }, }, ); diff --git a/packages/test/test-end-to-end-tests/src/test/serializeAfterFailedAttach.spec.ts b/packages/test/test-end-to-end-tests/src/test/serializeAfterFailedAttach.spec.ts index 8c1610a5d267..9f000da9371f 100644 --- a/packages/test/test-end-to-end-tests/src/test/serializeAfterFailedAttach.spec.ts +++ b/packages/test/test-end-to-end-tests/src/test/serializeAfterFailedAttach.spec.ts @@ -87,7 +87,7 @@ describeCompat( logger: provider.logger, configProvider: { getRawConfig: (name) => - name === "Fluid.Container.RetryOnAttachFailure" ? true : undefined, + name === "Fluid.Container.DisableCloseOnAttachFailure" ? true : undefined, }, }); return testLoader;