Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 48 additions & 11 deletions packages/drivers/routerlicious-driver/src/restWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { performanceNow } from "@fluid-internal/client-utils";
import type { ITelemetryBaseProperties } from "@fluidframework/core-interfaces";
import { assert } from "@fluidframework/core-utils/internal";
import { assert, delay } from "@fluidframework/core-utils/internal";
import type { RateLimiter } from "@fluidframework/driver-utils/internal";
import { GenericNetworkError, NonRetryableError } from "@fluidframework/driver-utils/internal";
import {
Expand Down Expand Up @@ -48,6 +48,9 @@ const buildRequestInitConfig = (requestConfig: RequestConfig): RequestInit => {
return requestInit;
};

// Transport-level failures that are safe to retry on a fresh socket.
const transientNetworkErrorPattern = /socket hang up|ECONNRESET|EPIPE/i;

export interface IR11sResponse<T> {
content: T;
headers: Map<string, string>;
Expand Down Expand Up @@ -117,6 +120,14 @@ class RouterliciousRestWrapper extends RestWrapper {
*/
private readonly retryCounter = new Map<string, number>();

/**
* Maximum number of attempts (1 initial + retries) for a single fetch that fails with a transient network error.
*/
private static readonly maxNetworkErrorAttempts = 3;

/** Delay between transient network-error retries. */
private static readonly networkErrorRetryDelayMs = 250;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In networking, I generally expect https://en.wikipedia.org/wiki/Exponential_backoff unless there is some known category of issue this timeout is selected to be relevant to. Might be good to document why this specific duration makes sense.

Might also be good to document the consequences of setting this or the retry count higher (I assume it can result in something taking longer before failing, or maybe hiding flakey service issues?).

Anyway, this isn't an area I work in, so I'm no expert here, those are just my thoughts.


constructor(
logger: TelemetryLoggerExt,
private readonly rateLimiter: RateLimiter,
Expand Down Expand Up @@ -169,9 +180,17 @@ class RouterliciousRestWrapper extends RestWrapper {
const fetchRequestConfig = buildRequestInitConfig(translatedConfig);

const res = await this.rateLimiter.schedule(async () => {
const perfStart = performanceNow();
const result = await fetch(completeRequestUrl, fetchRequestConfig).catch(
async (error) => {
// Retry transient transport failures (e.g. a pooled socket closed by the peer while
// this process's execution context was frozen) on a fresh socket before surfacing the error.
for (let attempt = 1; ; attempt++) {
const perfStart = performanceNow();
try {
const result = await fetch(completeRequestUrl, fetchRequestConfig);
return {
response: result,
duration: performanceNow() - perfStart,
};
} catch (error: any) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for improved type safety, type errors as unknown not any

// on failure, add the request entry into the retryCounter map to count the subsequent retries, if any
this.retryCounter.set(requestKey, requestRetryCount ? requestRetryCount + 1 : 1);

Expand All @@ -187,12 +206,34 @@ class RouterliciousRestWrapper extends RestWrapper {
const errorMessage = isNetworkError
? `NetworkError: ${error.message}`
: safeStringify(error);

// A self-signed-cert failure is permanent, never transient — exclude it from retry.
const isSelfSignedCertError = errorMessage.includes(
"failed, reason: self signed certificate",
);
const errorCode: unknown = error?.code ?? error?.cause?.code;
const isTransientNetworkError =
!isSelfSignedCertError &&
(isNetworkError ||
(typeof errorCode === "string" &&
transientNetworkErrorPattern.test(errorCode)) ||
transientNetworkErrorPattern.test(error?.message ?? "") ||
transientNetworkErrorPattern.test(error?.cause?.message ?? ""));
if (
isTransientNetworkError &&
canRetry &&
attempt < RouterliciousRestWrapper.maxNetworkErrorAttempts
) {
await delay(RouterliciousRestWrapper.networkErrorRetryDelayMs);
continue;
}

// If a service is temporarily down or a browser resource limit is reached, RestWrapper will throw
// a network error with no status code (e.g. err:ERR_CONN_REFUSED or err:ERR_FAILED) and
// the error message will start with NetworkError as defined in restWrapper.ts
// If there exists a self-signed SSL certificates error, throw a NonRetryableError
// TODO: instead of relying on string matching, filter error based on the error code like we do for websocket connections
const err = errorMessage.includes("failed, reason: self signed certificate")
const err = isSelfSignedCertError
? new NonRetryableError(
errorMessage,
RouterliciousErrorTypes.sslCertError,
Expand All @@ -204,12 +245,8 @@ class RouterliciousRestWrapper extends RestWrapper {
telemetryProps,
);
throw err;
},
);
return {
response: result,
duration: performanceNow() - perfStart,
};
}
}
});

const response = res.response;
Expand Down
153 changes: 146 additions & 7 deletions packages/drivers/routerlicious-driver/src/test/restWrapper.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ interface MockFetchEntry {
/** Response body. Objects are JSON-serialized with content-type: application/json. */
body?: string | object;
/** Simulate a network error by rejecting fetch with a TypeError */
networkError?: { code: string };
networkError?: { code?: string; message?: string };
/** Dynamically compute the response as [status, body] */
replyFn?: () => [number, string | object];
}
Expand Down Expand Up @@ -138,8 +138,10 @@ function createMockFetch(entries: MockFetchEntry[]): MockFetch {

// Simulate network error
if (entry.networkError) {
const err = new TypeError(`request to ${inputUrl} failed`);
(err as any).code = entry.networkError.code;
const err = new TypeError(entry.networkError.message ?? `request to ${inputUrl} failed`);
if (entry.networkError.code !== undefined) {
(err as any).code = entry.networkError.code;
}
throw err;
}

Expand Down Expand Up @@ -330,14 +332,28 @@ describe("RouterliciousDriverRestWrapper", () => {
errorType: RouterliciousErrorTypes.fileNotFoundOrAccessDeniedError,
});
});
it("throws retriable error on Network Error", async () => {
it("throws retriable error on Network Error after exhausting retries", async () => {
// request() now retries transient network errors, so mock a failure for each of the
// 3 attempts (1 initial + 2 retries) before it gives up.
mockFetch = createMockFetch([
{
method: "GET",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "GET",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "GET",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
]);
await assert.rejects(restWrapper.get(testUrl), {
canRetry: true,
Expand Down Expand Up @@ -542,14 +558,28 @@ describe("RouterliciousDriverRestWrapper", () => {
errorType: RouterliciousErrorTypes.fileNotFoundOrAccessDeniedError,
});
});
it("throws retriable error on Network Error", async () => {
it("throws retriable error on Network Error after exhausting retries", async () => {
// request() now retries transient network errors, so mock a failure for each of the
// 3 attempts (1 initial + 2 retries) before it gives up.
mockFetch = createMockFetch([
{
method: "POST",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "POST",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "POST",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
]);
await assert.rejects(restWrapper.post(testUrl, { test: "payload" }), {
canRetry: true,
Expand Down Expand Up @@ -661,14 +691,28 @@ describe("RouterliciousDriverRestWrapper", () => {
errorType: RouterliciousErrorTypes.fileNotFoundOrAccessDeniedError,
});
});
it("throws retriable error on Network Error", async () => {
it("throws retriable error on Network Error after exhausting retries", async () => {
// request() now retries transient network errors, so mock a failure for each of the
// 3 attempts (1 initial + 2 retries) before it gives up.
mockFetch = createMockFetch([
{
method: "PATCH",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "PATCH",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "PATCH",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
]);
await assert.rejects(restWrapper.patch(testUrl, { test: "payload" }), {
canRetry: true,
Expand Down Expand Up @@ -780,19 +824,114 @@ describe("RouterliciousDriverRestWrapper", () => {
errorType: RouterliciousErrorTypes.fileNotFoundOrAccessDeniedError,
});
});
it("throws retriable error on Network Error", async () => {
it("throws retriable error on Network Error after exhausting retries", async () => {
// request() now retries transient network errors, so mock a failure for each of the
// 3 attempts (1 initial + 2 retries) before it gives up.
mockFetch = createMockFetch([
{
method: "DELETE",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "DELETE",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
{
method: "DELETE",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET" },
},
]);
await assert.rejects(restWrapper.delete(testUrl), {
canRetry: true,
errorType: RouterliciousErrorTypes.genericNetworkError,
});
});
});

// Transport-level retry: the routerlicious driver runs in AWS Lambda, whose execution context
// is frozen between invocations. A keep-alive socket pooled by the underlying fetch transport
// can be closed by the peer (e.g. an ALB) during the freeze; the next request reuses the dead
// socket and rejects with "socket hang up" / ECONNRESET / EPIPE. request() retries such
// transient network errors (a fresh socket is opened on retry) up to a small bound before
// surfacing the error.
describe("transient network error retry", () => {
it("retries a transient network error and succeeds when a later attempt connects", async () => {
mockFetch = createMockFetch([
{
method: "GET",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET", message: "socket hang up" },
},
{ method: "GET", origin: testHost, path: testPath, status: 200 },
]);
await assert.doesNotReject(restWrapper.get(testUrl));
});

it("retries POST requests too", async () => {
mockFetch = createMockFetch([
{
method: "POST",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET", message: "socket hang up" },
},
{ method: "POST", origin: testHost, path: testPath, status: 200 },
]);
await assert.doesNotReject(restWrapper.post(testUrl, { test: "payload" }));
});

it("gives up after 3 attempts and throws a retriable network error", async () => {
// Fail all 3 attempts (1 initial + 2 retries). afterEach's mockFetch.done() asserts
// exactly 3 fetch calls were made, proving the loop doesn't retry beyond the bound.
mockFetch = createMockFetch([
{
method: "GET",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET", message: "socket hang up" },
},
{
method: "GET",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET", message: "socket hang up" },
},
{
method: "GET",
origin: testHost,
path: testPath,
networkError: { code: "ECONNRESET", message: "socket hang up" },
},
]);
await assert.rejects(restWrapper.get(testUrl), {
canRetry: true,
errorType: RouterliciousErrorTypes.genericNetworkError,
});
});

it("does not retry a self-signed certificate error (non-retriable)", async () => {
mockFetch = createMockFetch([
{
method: "GET",
origin: testHost,
path: testPath,
networkError: {
message:
"request to https://localhost/ failed, reason: self signed certificate in certificate chain",
},
},
]);
await assert.rejects(restWrapper.get(testUrl), {
canRetry: false,
errorType: RouterliciousErrorTypes.sslCertError,
});
});
});
});