diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index e20fed60150..4bce11d590c 100644 --- a/packages/controller-utils/CHANGELOG.md +++ b/packages/controller-utils/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Allow overriding `isServiceFailure` in `createServicePolicy` ([#9123](https://github.com/MetaMask/core/pull/9123)) + ### Changed - Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074)) diff --git a/packages/controller-utils/src/create-service-policy.test.ts b/packages/controller-utils/src/create-service-policy.test.ts index 2d38112fe7a..27c5fcb7d19 100644 --- a/packages/controller-utils/src/create-service-policy.test.ts +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -3574,6 +3574,118 @@ describe('createServicePolicy', () => { await expect(policy.execute(mockService)).rejects.toThrow('failure'); }); }); + + describe('using a custom isServiceFailure predicate', () => { + it('opens the circuit when the predicate treats the error as a service failure', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreakListener = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + isServiceFailure: () => true, + }); + policy.onBreak(onBreakListener); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + jest.runAllTimersAsync(); + await ignoreRejection(promise); + + expect(onBreakListener).toHaveBeenCalledTimes(1); + expect(onBreakListener).toHaveBeenCalledWith({ error }); + }); + + it('never opens the circuit when the predicate does not treat the error as a service failure', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreakListener = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + isServiceFailure: () => false, + }); + policy.onBreak(onBreakListener); + + // Execute more times than the max consecutive failures so that the + // circuit would open if these errors were counted as failures. + for (let i = 0; i < maxConsecutiveFailures + 1; i++) { + const promise = policy.execute(mockService); + // eslint-disable-next-line @typescript-eslint/no-floating-promises + jest.runAllTimersAsync(); + await ignoreRejection(promise); + } + + expect(onBreakListener).not.toHaveBeenCalled(); + }); + + it('calls the predicate with the error thrown by the service', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const isServiceFailure = jest.fn(() => true); + const policy = createServicePolicy({ + maxConsecutiveFailures: DEFAULT_MAX_RETRIES + 1, + isServiceFailure, + }); + + const promise = policy.execute(mockService); + // eslint-disable-next-line @typescript-eslint/no-floating-promises + jest.runAllTimersAsync(); + await ignoreRejection(promise); + + expect(isServiceFailure).toHaveBeenCalledWith(error); + }); + }); + + describe('using the default isServiceFailure predicate', () => { + it('opens the circuit for an error with an HTTP status >= 500', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = Object.assign(new Error('failure'), { httpStatus: 500 }); + const mockService = jest.fn(() => { + throw error; + }); + const onBreakListener = jest.fn(); + const policy = createServicePolicy({ maxConsecutiveFailures }); + policy.onBreak(onBreakListener); + + const promise = policy.execute(mockService); + // eslint-disable-next-line @typescript-eslint/no-floating-promises + jest.runAllTimersAsync(); + await ignoreRejection(promise); + + expect(onBreakListener).toHaveBeenCalledTimes(1); + }); + + it('never opens the circuit for an error with an HTTP status < 500', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = Object.assign(new Error('failure'), { httpStatus: 400 }); + const mockService = jest.fn(() => { + throw error; + }); + const onBreakListener = jest.fn(); + const policy = createServicePolicy({ maxConsecutiveFailures }); + policy.onBreak(onBreakListener); + + // Execute more times than the max consecutive failures so that the + // circuit would open if these errors were counted as failures. + for (let i = 0; i < maxConsecutiveFailures + 1; i++) { + const promise = policy.execute(mockService); + // eslint-disable-next-line @typescript-eslint/no-floating-promises + jest.runAllTimersAsync(); + await ignoreRejection(promise); + } + + expect(onBreakListener).not.toHaveBeenCalled(); + }); + }); }); /** diff --git a/packages/controller-utils/src/create-service-policy.ts b/packages/controller-utils/src/create-service-policy.ts index 0c27fdf6155..b7e5a91a142 100644 --- a/packages/controller-utils/src/create-service-policy.ts +++ b/packages/controller-utils/src/create-service-policy.ts @@ -53,6 +53,10 @@ export type CreateServicePolicyOptions = { * regarded as degraded (affecting when `onDegraded` is called). */ degradedThreshold?: number; + /** + * Predicate function for when an error should be considered a service failure. + */ + isServiceFailure?: (error: unknown) => boolean; /** * The maximum number of times that the service is allowed to fail before * pausing further retries. @@ -189,7 +193,7 @@ export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; */ export const DEFAULT_DEGRADED_THRESHOLD = 5_000; -const isServiceFailure = (error: unknown): boolean => { +const defaultIsServiceFailure = (error: unknown): boolean => { if ( typeof error === 'object' && error !== null && @@ -199,8 +203,9 @@ const isServiceFailure = (error: unknown): boolean => { return error.httpStatus >= 500; } - // If the error is not an object, or doesn't have a numeric code property, - // consider it a service failure (e.g., network errors, timeouts, etc.) + // If the error is not an object, or doesn't have a numeric httpStatus + // property, consider it a service failure (e.g., network errors, timeouts, + // etc.) return true; }; @@ -283,6 +288,7 @@ export function createServicePolicy( circuitBreakDuration = DEFAULT_CIRCUIT_BREAK_DURATION, degradedThreshold = DEFAULT_DEGRADED_THRESHOLD, backoff = new ExponentialBackoff(), + isServiceFailure = defaultIsServiceFailure, } = options; let availabilityStatus: AvailabilityStatus = AVAILABILITY_STATUSES.Unknown; diff --git a/packages/network-controller/CHANGELOG.md b/packages/network-controller/CHANGELOG.md index fcff6929fd4..efee9f25923 100644 --- a/packages/network-controller/CHANGELOG.md +++ b/packages/network-controller/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The constructor argument `isRpcFailoverEnabled` is no longer available. - `RemoteFeatureFlagController:stateChange` and `RemoteFeatureFlagController:getState` are now required. - Drop `async-mutex` dependency, which was no longer used in source ([#9064](https://github.com/MetaMask/core/pull/9064)) +- Consider all Infura HTTP errors as service failures except `400` and `429` ([#9123](https://github.com/MetaMask/core/pull/9123)) ### Removed diff --git a/packages/network-controller/src/rpc-service/rpc-service.test.ts b/packages/network-controller/src/rpc-service/rpc-service.test.ts index 9c4da444b26..309296cab77 100644 --- a/packages/network-controller/src/rpc-service/rpc-service.test.ts +++ b/packages/network-controller/src/rpc-service/rpc-service.test.ts @@ -344,6 +344,89 @@ describe('RpcService', () => { }); }); + describe('treating errors as service failures', () => { + const jsonRpcRequest = { + id: 1, + jsonrpc: '2.0' as const, + method: 'eth_chainId', + params: [], + }; + + describe('when the endpoint is an Infura URL', () => { + const endpointUrl = 'https://mainnet.infura.io'; + + it.each([400, 429])( + 'does not break the circuit when the endpoint responds with %d', + async (httpStatus) => { + nock(endpointUrl) + .post('/', jsonRpcRequest) + .times(3) + .reply(httpStatus); + const service = new RpcService({ + fetch, + btoa, + endpointUrl, + isOffline: (): boolean => false, + policyOptions: { maxConsecutiveFailures: 2 }, + }); + + // Make more requests than the max consecutive failures so that the + // circuit would open if these errors were treated as failures. + await ignoreRejection(service.request(jsonRpcRequest)); + await ignoreRejection(service.request(jsonRpcRequest)); + await ignoreRejection(service.request(jsonRpcRequest)); + + expect(service.getCircuitState()).toBe(CircuitState.Closed); + }, + ); + + it.each([401, 500])( + 'breaks the circuit when the endpoint responds with %d', + async (httpStatus) => { + nock(endpointUrl) + .post('/', jsonRpcRequest) + .times(2) + .reply(httpStatus); + const service = new RpcService({ + fetch, + btoa, + endpointUrl, + isOffline: (): boolean => false, + policyOptions: { maxConsecutiveFailures: 2 }, + }); + + await ignoreRejection(service.request(jsonRpcRequest)); + await ignoreRejection(service.request(jsonRpcRequest)); + + expect(service.getCircuitState()).toBe(CircuitState.Open); + }, + ); + }); + + describe('when the endpoint is not an Infura URL', () => { + const endpointUrl = 'https://rpc.example.chain'; + + it('does not break the circuit for a 4xx response that is not a server error', async () => { + nock(endpointUrl).post('/', jsonRpcRequest).times(3).reply(401); + const service = new RpcService({ + fetch, + btoa, + endpointUrl, + isOffline: (): boolean => false, + policyOptions: { maxConsecutiveFailures: 2 }, + }); + + // Make more requests than the max consecutive failures so that the + // circuit would open if these errors were treated as failures. + await ignoreRejection(service.request(jsonRpcRequest)); + await ignoreRejection(service.request(jsonRpcRequest)); + await ignoreRejection(service.request(jsonRpcRequest)); + + expect(service.getCircuitState()).toBe(CircuitState.Closed); + }); + }); + }); + describe('request', () => { // NOTE: Keep this list synced with CONNECTION_ERRORS describe.each([ diff --git a/packages/network-controller/src/rpc-service/rpc-service.ts b/packages/network-controller/src/rpc-service/rpc-service.ts index c9904f43e61..6e6cba3509e 100644 --- a/packages/network-controller/src/rpc-service/rpc-service.ts +++ b/packages/network-controller/src/rpc-service/rpc-service.ts @@ -67,10 +67,13 @@ export type RpcServiceOptions = { */ logger?: Pick; /** - * Options to pass to `createServicePolicy`. Note that `retryFilterPolicy` is - * not accepted, as it is overwritten. See {@link createServicePolicy}. + * Options to pass to `createServicePolicy`. Note that `retryFilterPolicy` and `isServiceFailure` + * are not accepted, as they are overwritten. See {@link createServicePolicy}. */ - policyOptions?: Omit; + policyOptions?: Omit< + CreateServicePolicyOptions, + 'retryFilterPolicy' | 'isServiceFailure' + >; /** * A function that checks if the user is currently offline. If it returns true, * connection errors will not be retried, preventing degraded and break @@ -284,6 +287,31 @@ function stripCredentialsFromUrl(url: URL): URL { return strippedUrl; } +const INFURA_NON_FAILURE_HTTP_STATUS_CODES = [400, 429]; + +/** + * Predicate function that determines if an error from Infura is treated as a service failure. + * + * @param error - The error. + * @returns True if the error should be treated as a service policy failure. Most errors are treated like failures, + * with the exception of certain HTTP status codes. + */ +function isServiceFailureInfura(error: unknown): boolean { + if ( + typeof error === 'object' && + error !== null && + hasProperty(error, 'httpStatus') && + typeof error.httpStatus === 'number' + ) { + return !INFURA_NON_FAILURE_HTTP_STATUS_CODES.includes(error.httpStatus); + } + + // If the error is not an object, or doesn't have a numeric httpStatus + // property, consider it a service failure (e.g., network errors, timeouts, + // etc.) + return true; +} + /** * This class is responsible for making a request to an endpoint that implements * the JSON-RPC protocol. It is designed to gracefully handle network and server @@ -368,10 +396,13 @@ export class RpcService { this.endpointUrl = stripCredentialsFromUrl(normalizedUrl); this.#logger = logger; + const isInfura = normalizedUrl.hostname.endsWith('.infura.io'); + this.#policy = createServicePolicy({ maxRetries: DEFAULT_MAX_RETRIES, maxConsecutiveFailures: DEFAULT_MAX_CONSECUTIVE_FAILURES, ...policyOptions, + isServiceFailure: isInfura ? isServiceFailureInfura : undefined, retryFilterPolicy: handleWhen((error) => { // If user is offline, don't retry any errors // This prevents degraded/break callbacks from being triggered