Skip to content

Commit

Permalink
feat(rest): Add auto-retry on 429
Browse files Browse the repository at this point in the history
  • Loading branch information
Sidnioulz committed Sep 28, 2024
1 parent 204671d commit 4d5aae7
Show file tree
Hide file tree
Showing 10 changed files with 294 additions and 31 deletions.
2 changes: 1 addition & 1 deletion packages/cache/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"test": "vitest run",
"test:coverage": "vitest --coverage",
"test:coverage": "vitest --coverage run",
"test:dev": "vitest dev",
"typecheck": "tsc --noEmit"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/cuttings/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"test": "vitest run",
"test:coverage": "vitest --coverage",
"test:coverage": "vitest --coverage run",
"test:dev": "vitest dev",
"typecheck": "tsc --noEmit"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin-figma/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"test": "vitest run",
"test:coverage": "vitest --coverage",
"test:coverage": "vitest --coverage run",
"test:dev": "vitest dev",
"typecheck": "tsc --noEmit"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/logger/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"test": "vitest run",
"test:coverage": "vitest --coverage",
"test:coverage": "vitest --coverage run",
"test:dev": "vitest dev",
"typecheck": "tsc --noEmit"
},
Expand Down
6 changes: 4 additions & 2 deletions packages/rest/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"test": "vitest run",
"test:coverage": "vitest --coverage",
"test:coverage": "vitest --coverage run",
"test:dev": "vitest dev",
"typecheck": "tsc --noEmit"
},
Expand Down Expand Up @@ -63,6 +63,7 @@
"cacheable": "^0.8.0",
"memfs": "^4.11.1",
"mocked-env": "^1.3.5",
"nock": "^13.5.5",
"swagger-typescript-api": "^13.0.22",
"tsc-watch": "^6.2.0",
"tsup": "^8.2.4",
Expand All @@ -73,6 +74,7 @@
"@figmarine/cache": "workspace:*",
"@figmarine/logger": "workspace:*",
"axios": "^1.7.7",
"axios-cache-interceptor": "^1.6.0"
"axios-cache-interceptor": "^1.6.0",
"axios-retry": "^4.5.0"
}
}
189 changes: 187 additions & 2 deletions packages/rest/src/__tests__/client.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,40 @@ import path from 'node:path';

import * as cacheModule from '@figmarine/cache';
import * as loggerModule from '@figmarine/logger';
import axios from 'axios';
import { test as base } from 'vitest';
import nock from 'nock';
import upstreamMockedEnv from 'mocked-env';
import { vol } from 'memfs';

import * as interceptorsModule from '../interceptors';
import { Client } from '../client';
import { Client, ClientInterface } from '../client';

/* Mock rate limit config. */
const { mockedConfig, mocked429Config } = vi.hoisted(() => {
return {
mockedConfig: vi.fn(),
mocked429Config: vi.fn(),
};
});

vi.mock(import('../rateLimit.config'), async (importOriginal) => {
const mod = await importOriginal();

return {
...mod,
getConfig: mockedConfig,
get429Config: mocked429Config,
};
});

/* FS mocks. */
vi.mock('node:fs');
vi.mock('node:fs/promises');

/* Mock the 1 minute initial delay for 429 retries. We use a shorter delay to avoid timeouts. */
const INITIAL_DELAY = 10;

/* Local context. */
interface ClientFixtures {
mockedEnv: (args?: typeof process.env) => void;
Expand All @@ -40,12 +63,36 @@ const it = base.extend<ClientFixtures>({

const tmpDir = os.tmpdir();

// TODO mock oas serv and write extra tests
/* Nock helpers. */
const callNockedEndpoint = async (c: ClientInterface) => {
try {
const response = await c.v1.getFile('testKey');
return {
status: response.status,
headers: response.headers as Record<string, string>,
data: response.data,
};
} catch (error) {
if (axios.isAxiosError(error) && error.response) {
return {
status: error.response.status,
headers: error.response.headers as Record<string, string>,
};
}
throw error;
}
};

describe('@figmarine/rest - client', () => {
beforeEach(() => {
vol.reset();
vol.fromJSON({ [tmpDir]: null });
mockedConfig.mockReturnValue({
reqLog: [],
WINDOW_BUDGET: 10,
WINDOW_LENGTH: 60,
});
mocked429Config.mockReturnValue({ INITIAL_DELAY: 500 });
});
afterEach(() => {
vi.restoreAllMocks();
Expand Down Expand Up @@ -283,4 +330,142 @@ describe('@figmarine/rest - client', () => {
expect(rlSpy).toHaveBeenCalled();
});
});

describe('Environment - FIGMA_API_BASE_URL', () => {
afterEach(() => {
nock.cleanAll();
});

it('allows customising the default URL for integration testing', async ({ mockedEnv }) => {
mockedEnv({ FIGMA_API_BASE_URL: 'https://localhost:4010' });
const c = await Client({ rateLimit: true });
const originalScole = nock('https://api.figma.com').get('/v1/files/testKey').reply(200, 'OK');
const modifiedUrlScope = nock('https://localhost:4010')
.get('/v1/files/testKey')
.reply(200, 'OK');

const response = await callNockedEndpoint(c);

expect(response.status).toBe(200);
expect(originalScole.isDone()).toBe(false);
expect(originalScole.activeMocks()).toHaveLength(1);
expect(modifiedUrlScope.isDone()).toBe(true);
expect(modifiedUrlScope.activeMocks()).toHaveLength(0);
});
});

describe('Auto Retry - 429', () => {
beforeEach(() => {
mocked429Config.mockReturnValue({ INITIAL_DELAY });
});

afterEach(() => {
nock.cleanAll();
});

it('retries 429 errors but not other errors', async ({ mockedEnv }) => {
mockedEnv();

const c = await Client({ rateLimit: true });
const scope = nock('https://api.figma.com').get('/v1/files/testKey').reply(403, 'Forbidden');

const response = await callNockedEndpoint(c);

expect(response.status).toBe(403);
expect(scope.isDone()).toBe(true);
expect(scope.activeMocks()).toHaveLength(0);
});

it.for([1, 2, 3, 4, 5])(
'retries %i minute(s) after equally many 429 errors without Retry-After',
async (numberOf429, { mockedEnv }) => {
mockedEnv();

const requestTimes: number[] = [];
let requestCount = 0;

const c = await Client({ rateLimit: true });
const scope = nock('https://api.figma.com');
for (let i = 0; i < numberOf429; i++) {
scope.get('/v1/files/testKey').reply(429, 'Too Many Requests');
}
scope.get('/v1/files/testKey').reply(200, 'OK');

scope.on('request', () => {
requestTimes.push(Date.now());
requestCount++;
});

const response = await callNockedEndpoint(c);

expect(response.status).toBe(200);
expect(scope.isDone()).toBe(true);
expect(scope.activeMocks()).toHaveLength(0);
expect(requestCount).toBe(numberOf429 + 1);

const totalDelay = requestTimes[requestTimes.length - 1] - requestTimes[0];
const expectedMinDelay = INITIAL_DELAY * (Math.pow(2, numberOf429 + 1) - 2);
// We know that axios-retry returns between 1 and 1.2 times the computed delay.
// We take that delay and add a 100ms buffer for JS operations.
const expectedMaxDelay = 1.2 * expectedMinDelay + 100;

expect(totalDelay).toBeGreaterThanOrEqual(expectedMinDelay);
expect(totalDelay).toBeLessThanOrEqual(expectedMaxDelay);
},
);

it('fails on the 7th consecutive retry', async ({ mockedEnv }) => {
mockedEnv();
let requestCount = 0;
const c = await Client({ rateLimit: true });

const scope = nock('https://api.figma.com')
.get('/v1/files/testKey')
.reply(429, 'Too Many Requests')
.persist();

scope.on('request', () => {
requestCount++;
});

const response = await callNockedEndpoint(c);

expect(response.status).toBe(429);
expect(scope.isDone()).toBe(true);
expect(scope.activeMocks()).toHaveLength(1);
expect(requestCount).toBe(7);
});

it('respects the Retry-After header delay if higher than our computed delay', async ({
mockedEnv,
}) => {
mockedEnv();
const headerDelay = 200;
const startTime = Date.now();
let endTime = 0;

const c = await Client({ rateLimit: true });

const scope = nock('https://api.figma.com')
.get('/v1/files/testKey')
.reply(429, 'Too Many Requests', { 'Retry-After': `${headerDelay / 1000}` })
.get('/v1/files/testKey')
.reply(200, 'OK');

scope.on('replied', (req) => {
if (req.response.statusCode === 200) {
endTime = Date.now();
}
});

const response = await callNockedEndpoint(c);

expect(response.status).toBe(200);
expect(scope.isDone()).toBe(true);
expect(scope.activeMocks()).toHaveLength(0);

const totalDelay = endTime - startTime;
expect(totalDelay).toBeGreaterThanOrEqual(headerDelay);
});
});
});
34 changes: 31 additions & 3 deletions packages/rest/src/client.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import axiosRetry, { exponentialDelay, isNetworkOrIdempotentRequestError } from 'axios-retry';
import { defaultKeyGenerator, setupCache } from 'axios-cache-interceptor';
import { makeCache, MakeCacheOptions } from '@figmarine/cache';
import { log } from '@figmarine/logger';

import { Api, type Api as ApiInterface } from './__generated__/figmaRestApi';
import { get429Config } from './rateLimit.config';
import { rateLimitRequestInterceptor } from './interceptors';
import { securityWorker } from './securityWorker';
import { storage } from './storage';
Expand Down Expand Up @@ -93,7 +95,7 @@ export async function Client(opts: ClientOptions = {}): Promise<ClientInterface>
* Ignore "don't cache" headers from Figma by default.
* Let the cache package handle TTL for us, and cache
* invalidation logic clear stale cache.
* @returns {number} A 999999999999 seconds TTL.
* @returns A 999999999999 seconds TTL.
*/
headerInterpreter: () => 999999999999,
/**
Expand All @@ -105,6 +107,12 @@ export async function Client(opts: ClientOptions = {}): Promise<ClientInterface>
});
}

/* Support changing the base URL so this package can be tested
* against a mock server. */
if (process.env.FIGMA_API_BASE_URL) {
api.instance.defaults.baseURL = process.env.FIGMA_API_BASE_URL;
}

// TODO: add endpoints to download arbitrary image URLs returned by
// other endpoints, which account for the need to deduce calls from
// the rate limit budget.
Expand All @@ -118,9 +126,29 @@ export async function Client(opts: ClientOptions = {}): Promise<ClientInterface>
api.instance.interceptors.request.use(
rateLimitRequestInterceptor(defaultKeyGenerator, diskCache),
);
}

// TODO: add response interceptor for 429 handling.
// Add response interceptor for 429 handling.
const rlConfig = get429Config();
axiosRetry(api.instance, {
onMaxRetryTimesExceeded: (error) => {
log(
`Client: 429 Too Many Requests on '${error.request.url}'. Aborting after too many retries.`,
);
},
onRetry: (retryCount, _, requestConfig) => {
log(
`Client: 429 Too Many Requests on '${requestConfig.url}'. Will retry automatically in ${2 ** retryCount / 2} seconds.`,
);
},
retries: 6,
retryCondition: (error) =>
isNetworkOrIdempotentRequestError(error) ||
error.status === 429 ||
error.response?.status === 429,
retryDelay: (retryCount, error) =>
exponentialDelay(retryCount, error, rlConfig.INITIAL_DELAY),
});
}

log(`Created Figma REST API client successfully.`);

Expand Down
24 changes: 13 additions & 11 deletions packages/rest/src/rateLimit.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,11 @@ type LogEntry = {
export type Log = LogEntry[];

/*
* Single event queue for number of requests in the last minute. Note that the Plugin API
* rate limit seems to use two sliding windows: one per minute and one per day.
* TODO: find a good storage place for these.
* Single event queue for number of requests in the last minute.
* TODO: ensure these logs are disk-cached and well restored across client runs.
*/
const reqLog: Log = [];

/**
* Log of previous Error 429 events. When we get a 429, we apply a multiplier to our budget
* calculations to slow future requests down for a while, in an attempt to limit the risk
* of further 429 errors.
*/
// TODO: implement
// const error429Log: Log = [];

/**
* Returns singleton objects and constants for the rate limit algorithm.
* Exposed this way for mockability in tests.
Expand All @@ -46,3 +36,15 @@ export function getConfig() {
RATE_DECREASE_LENGTH,
};
}

/**
* Returns singleton objects and constants for the exponential delay algorithm on 429 errors.
* Exposed this way for mockability in tests.
* @returns 429 retry algorithm config data
*/
export function get429Config() {
return {
// We want a one second initial delay, but axios-retry uses 2^count, so 2^1 * 500 = 1000ms for the initial retry.
INITIAL_DELAY: 500,
};
}
Loading

0 comments on commit 4d5aae7

Please sign in to comment.