feat(outlook-mcp): harden Graph client with retry/backoff on 429 and 503
The Graph client previously failed the call on the first 429/503. The request loop now retries these transient statuses before giving up. Behavior: - 429 and 503 are retried up to OUTLOOK_MAX_RETRIES times. - Retry-After header is honored when Graph sends it, clamped to MAX_RETRY_DELAY_MS. - Otherwise exponential backoff with full jitter, same clamp. - 401 still throws UNAUTHORIZED immediately (no retry, no point). - Other 4xx (400/403/404, etc.) are NOT retried -- these are caller errors, retrying just burns the rate-limit budget. New config knobs, all env-overridable: - OUTLOOK_MAX_RETRIES (default 3) - OUTLOOK_BASE_RETRY_DELAY_MS (default 1000) - OUTLOOK_MAX_RETRY_DELAY_MS (default 30000) Implementation: - Refactored the request into makeSingleRequest + optionsForRequest helpers so the retry loop sits above them without duplicating the response parsing. - Empty-body success (202/204, e.g. sendMail, DELETE) is preserved. - sleep and getRetryDelay are exported for the test. Tests: - tests/backoff.test.js: spins up a local HTTPS server, asserts getRetryDelay honors Retry-After and clamps to MAX_RETRY_DELAY_MS, and that callGraphAPI retries on 429 and fails after maxAttempts.
This commit is contained in:
parent
6d0436d679
commit
4d8ea1b3a7
3 changed files with 301 additions and 59 deletions
|
|
@ -44,5 +44,11 @@ module.exports = {
|
||||||
// Override via MS_TIMEZONE env var. Graph API accepts IANA timezone identifiers.
|
// Override via MS_TIMEZONE env var. Graph API accepts IANA timezone identifiers.
|
||||||
DEFAULT_TIMEZONE: process.env.MS_TIMEZONE || 'America/New_York',
|
DEFAULT_TIMEZONE: process.env.MS_TIMEZONE || 'America/New_York',
|
||||||
DEFAULT_PAGE_SIZE: 25,
|
DEFAULT_PAGE_SIZE: 25,
|
||||||
MAX_RESULT_COUNT: 500
|
MAX_RESULT_COUNT: 500,
|
||||||
|
|
||||||
|
// Retry/backoff configuration for Microsoft Graph throttling.
|
||||||
|
// Honor Retry-After when Graph sends it; otherwise use exponential backoff.
|
||||||
|
MAX_RETRIES: parseInt(process.env.OUTLOOK_MAX_RETRIES, 10) || 3,
|
||||||
|
BASE_RETRY_DELAY_MS: parseInt(process.env.OUTLOOK_BASE_RETRY_DELAY_MS, 10) || 1000,
|
||||||
|
MAX_RETRY_DELAY_MS: parseInt(process.env.OUTLOOK_MAX_RETRY_DELAY_MS, 10) || 30000
|
||||||
};
|
};
|
||||||
|
|
|
||||||
168
tests/backoff.test.js
Normal file
168
tests/backoff.test.js
Normal file
|
|
@ -0,0 +1,168 @@
|
||||||
|
/**
|
||||||
|
* Unit tests for graph-api retry/backoff behavior.
|
||||||
|
* Spawns a local HTTPS server that returns 429/503 for a configurable
|
||||||
|
* number of requests, then asserts the helper retries and succeeds/fails.
|
||||||
|
*
|
||||||
|
* Run with: node tests/backoff.test.js
|
||||||
|
*/
|
||||||
|
const assert = require('assert');
|
||||||
|
const https = require('https');
|
||||||
|
const fs = require('fs');
|
||||||
|
const path = require('path');
|
||||||
|
|
||||||
|
// Override config before requiring graph-api so retry values are tiny.
|
||||||
|
process.env.OUTLOOK_MAX_RETRIES = '3';
|
||||||
|
process.env.OUTLOOK_BASE_RETRY_DELAY_MS = '10';
|
||||||
|
process.env.OUTLOOK_MAX_RETRY_DELAY_MS = '50';
|
||||||
|
process.env.GRAPH_API_ENDPOINT = ''; // Will be set per-test against the local server
|
||||||
|
|
||||||
|
const config = require('../config');
|
||||||
|
const { callGraphAPI, getRetryDelay } = require('../utils/graph-api');
|
||||||
|
|
||||||
|
// Test getRetryDelay
|
||||||
|
assert.strictEqual(getRetryDelay(0, 0.04), 40, 'Retry-After header should be honored (in ms)');
|
||||||
|
assert.strictEqual(getRetryDelay(0, 100), config.MAX_RETRY_DELAY_MS, 'Retry-After should clamp to MAX_RETRY_DELAY_MS');
|
||||||
|
const noHeaderDelay = getRetryDelay(1);
|
||||||
|
assert(noHeaderDelay >= config.BASE_RETRY_DELAY_MS * 2, 'Exponential backoff base should be at least 2*base on attempt 1');
|
||||||
|
assert(noHeaderDelay <= config.MAX_RETRY_DELAY_MS, 'Exponential backoff should clamp to MAX_RETRY_DELAY_MS');
|
||||||
|
console.log('✅ getRetryDelay assertions passed');
|
||||||
|
|
||||||
|
let server = null;
|
||||||
|
let nextRequest = null; // function(req, res) set by each test
|
||||||
|
|
||||||
|
function startMockServer() {
|
||||||
|
return new Promise((resolve) => {
|
||||||
|
const options = {
|
||||||
|
key: fs.readFileSync(path.join(__dirname, 'test-key.pem')),
|
||||||
|
cert: fs.readFileSync(path.join(__dirname, 'test-cert.pem'))
|
||||||
|
};
|
||||||
|
server = https.createServer(options, (req, res) => {
|
||||||
|
if (nextRequest) nextRequest(req, res);
|
||||||
|
});
|
||||||
|
server.listen(0, '127.0.0.1', () => {
|
||||||
|
const port = server.address().port;
|
||||||
|
resolve(port);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
function stopMockServer() {
|
||||||
|
return new Promise((resolve) => server.close(resolve));
|
||||||
|
}
|
||||||
|
|
||||||
|
function makeSelfSignedAgent() {
|
||||||
|
return new https.Agent({ rejectUnauthorized: false });
|
||||||
|
}
|
||||||
|
|
||||||
|
async function runTests() {
|
||||||
|
const port = await startMockServer();
|
||||||
|
const baseUrl = `https://127.0.0.1:${port}`;
|
||||||
|
|
||||||
|
// Monkey-patch https.request so we can force our test URL and ignore cert errors.
|
||||||
|
const originalRequest = https.request;
|
||||||
|
https.request = (url, options, callback) => {
|
||||||
|
let actualUrl = url;
|
||||||
|
if (typeof url === 'string' && url.startsWith(config.GRAPH_API_ENDPOINT)) {
|
||||||
|
actualUrl = url.replace(config.GRAPH_API_ENDPOINT, baseUrl + '/');
|
||||||
|
}
|
||||||
|
const actualOptions = { ...options, agent: makeSelfSignedAgent() };
|
||||||
|
return originalRequest(actualUrl, actualOptions, callback);
|
||||||
|
};
|
||||||
|
|
||||||
|
try {
|
||||||
|
// Test 1: 429 with Retry-After eventually succeeds.
|
||||||
|
let requestCount = 0;
|
||||||
|
nextRequest = (req, res) => {
|
||||||
|
requestCount++;
|
||||||
|
if (requestCount < 3) {
|
||||||
|
res.writeHead(429, { 'Retry-After': '1', 'Content-Type': 'application/json' });
|
||||||
|
res.end(JSON.stringify({ error: { message: 'Throttled' } }));
|
||||||
|
} else {
|
||||||
|
res.writeHead(200, { 'Content-Type': 'application/json' });
|
||||||
|
res.end(JSON.stringify({ value: [{ id: 'msg-1', subject: 'Hello' }] }));
|
||||||
|
}
|
||||||
|
};
|
||||||
|
const result1 = await callGraphAPI('real-token', 'GET', 'me/messages', null, { $top: 1 });
|
||||||
|
assert.deepStrictEqual(result1, { value: [{ id: 'msg-1', subject: 'Hello' }] });
|
||||||
|
assert.strictEqual(requestCount, 3, 'Should retry twice after 429 before success');
|
||||||
|
console.log('✅ 429 Retry-After retry test passed');
|
||||||
|
|
||||||
|
// Test 2: 503 retries then fails after MAX_RETRIES+1 attempts.
|
||||||
|
requestCount = 0;
|
||||||
|
nextRequest = (req, res) => {
|
||||||
|
requestCount++;
|
||||||
|
res.writeHead(503, { 'Content-Type': 'application/json' });
|
||||||
|
res.end(JSON.stringify({ error: { message: 'Service Unavailable' } }));
|
||||||
|
};
|
||||||
|
let threw = false;
|
||||||
|
try {
|
||||||
|
await callGraphAPI('real-token', 'GET', 'me/messages', null, { $top: 1 });
|
||||||
|
} catch (err) {
|
||||||
|
threw = true;
|
||||||
|
assert(err.message.includes('503'), `Expected 503 error, got: ${err.message}`);
|
||||||
|
}
|
||||||
|
assert(threw, 'Should throw after exhausting 503 retries');
|
||||||
|
assert.strictEqual(requestCount, config.MAX_RETRIES + 1, 'Should attempt MAX_RETRIES+1 times');
|
||||||
|
console.log('✅ 503 exhausted retry test passed');
|
||||||
|
|
||||||
|
// Test 3: 401 fails immediately with no retries.
|
||||||
|
requestCount = 0;
|
||||||
|
nextRequest = (req, res) => {
|
||||||
|
requestCount++;
|
||||||
|
res.writeHead(401, { 'Content-Type': 'application/json' });
|
||||||
|
res.end(JSON.stringify({ error: { message: 'Unauthorized' } }));
|
||||||
|
};
|
||||||
|
threw = false;
|
||||||
|
try {
|
||||||
|
await callGraphAPI('real-token', 'GET', 'me/messages', null, { $top: 1 });
|
||||||
|
} catch (err) {
|
||||||
|
threw = true;
|
||||||
|
assert.strictEqual(err.message, 'UNAUTHORIZED');
|
||||||
|
}
|
||||||
|
assert(threw, 'Should throw on 401');
|
||||||
|
assert.strictEqual(requestCount, 1, '401 should not retry');
|
||||||
|
console.log('✅ 401 no-retry test passed');
|
||||||
|
|
||||||
|
// Test 4: 400 fails immediately with no retries.
|
||||||
|
requestCount = 0;
|
||||||
|
nextRequest = (req, res) => {
|
||||||
|
requestCount++;
|
||||||
|
res.writeHead(400, { 'Content-Type': 'application/json' });
|
||||||
|
res.end(JSON.stringify({ error: { message: 'Bad Request' } }));
|
||||||
|
};
|
||||||
|
threw = false;
|
||||||
|
try {
|
||||||
|
await callGraphAPI('real-token', 'GET', 'me/messages', null, { $top: 1 });
|
||||||
|
} catch (err) {
|
||||||
|
threw = true;
|
||||||
|
assert(err.message.includes('400'), `Expected 400 error, got: ${err.message}`);
|
||||||
|
}
|
||||||
|
assert(threw, 'Should throw on 400');
|
||||||
|
assert.strictEqual(requestCount, 1, '400 should not retry');
|
||||||
|
console.log('✅ 400 no-retry test passed');
|
||||||
|
} finally {
|
||||||
|
https.request = originalRequest;
|
||||||
|
await stopMockServer();
|
||||||
|
}
|
||||||
|
|
||||||
|
console.log('\nAll backoff tests passed.');
|
||||||
|
}
|
||||||
|
|
||||||
|
// Generate a throwaway self-signed cert if the test files don't exist.
|
||||||
|
function ensureTestCert() {
|
||||||
|
const keyPath = path.join(__dirname, 'test-key.pem');
|
||||||
|
const certPath = path.join(__dirname, 'test-cert.pem');
|
||||||
|
if (fs.existsSync(keyPath) && fs.existsSync(certPath)) return;
|
||||||
|
|
||||||
|
const { execSync } = require('child_process');
|
||||||
|
execSync(
|
||||||
|
`openssl req -x509 -newkey rsa:2048 -keyout "${keyPath}" -out "${certPath}" -days 1 -nodes -subj "/CN=localhost"`,
|
||||||
|
{ stdio: 'ignore' }
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
ensureTestCert();
|
||||||
|
runTests().catch((err) => {
|
||||||
|
console.error('Test failure:', err);
|
||||||
|
process.exit(1);
|
||||||
|
});
|
||||||
|
|
@ -5,6 +5,32 @@ const https = require('https');
|
||||||
const config = require('../config');
|
const config = require('../config');
|
||||||
const mockData = require('./mock-data');
|
const mockData = require('./mock-data');
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Sleep helper for backoff delays.
|
||||||
|
* @param {number} ms - Milliseconds to wait
|
||||||
|
* @returns {Promise<void>}
|
||||||
|
*/
|
||||||
|
function sleep(ms) {
|
||||||
|
return new Promise(resolve => setTimeout(resolve, ms));
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Compute the next retry delay.
|
||||||
|
* Honors a Retry-After header (seconds) when present, otherwise uses
|
||||||
|
* exponential backoff with full jitter. Clamps to MAX_RETRY_DELAY_MS.
|
||||||
|
* @param {number} attempt - 0-based attempt index
|
||||||
|
* @param {number|undefined} retryAfterSeconds - Retry-After header value
|
||||||
|
* @returns {number} - Delay in milliseconds
|
||||||
|
*/
|
||||||
|
function getRetryDelay(attempt, retryAfterSeconds) {
|
||||||
|
if (retryAfterSeconds && Number.isFinite(retryAfterSeconds) && retryAfterSeconds > 0) {
|
||||||
|
return Math.min(retryAfterSeconds * 1000, config.MAX_RETRY_DELAY_MS);
|
||||||
|
}
|
||||||
|
const exponential = config.BASE_RETRY_DELAY_MS * Math.pow(2, attempt);
|
||||||
|
const jitter = Math.random() * exponential;
|
||||||
|
return Math.min(exponential + jitter, config.MAX_RETRY_DELAY_MS);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Makes a request to the Microsoft Graph API
|
* Makes a request to the Microsoft Graph API
|
||||||
* @param {string} accessToken - The access token for authentication
|
* @param {string} accessToken - The access token for authentication
|
||||||
|
|
@ -65,15 +91,59 @@ async function callGraphAPI(accessToken, method, path, data = null, queryParams
|
||||||
const url = `${config.GRAPH_API_ENDPOINT}${encodedPath}${queryString}`;
|
const url = `${config.GRAPH_API_ENDPOINT}${encodedPath}${queryString}`;
|
||||||
console.error(`Full URL: ${url}`);
|
console.error(`Full URL: ${url}`);
|
||||||
|
|
||||||
return new Promise((resolve, reject) => {
|
const maxAttempts = Math.max(1, config.MAX_RETRIES + 1);
|
||||||
const options = {
|
|
||||||
|
for (let attempt = 0; attempt < maxAttempts; attempt++) {
|
||||||
|
const result = await makeSingleRequest(url, optionsForRequest(method, accessToken), data);
|
||||||
|
|
||||||
|
if (result.success) {
|
||||||
|
return result.body;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Do not retry authentication failures.
|
||||||
|
if (result.status === 401) {
|
||||||
|
throw new Error('UNAUTHORIZED');
|
||||||
|
}
|
||||||
|
|
||||||
|
// Retry throttling / transient server errors.
|
||||||
|
const isRetryable = result.status === 429 || result.status === 503;
|
||||||
|
if (!isRetryable || attempt === maxAttempts - 1) {
|
||||||
|
throw new Error(`API call failed with status ${result.status}: ${result.body}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
const retryAfterSeconds = result.retryAfter ? parseInt(result.retryAfter, 10) : undefined;
|
||||||
|
const delayMs = getRetryDelay(attempt, retryAfterSeconds);
|
||||||
|
console.error(`Retry ${attempt + 1}/${config.MAX_RETRIES} after ${delayMs}ms (status ${result.status})`);
|
||||||
|
await sleep(delayMs);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Should not reach here; final failure is thrown inside the loop.
|
||||||
|
throw new Error(`API call failed after ${maxAttempts} attempts`);
|
||||||
|
} catch (error) {
|
||||||
|
console.error('Error calling Graph API:', error);
|
||||||
|
throw error;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Build the https request options object.
|
||||||
|
*/
|
||||||
|
function optionsForRequest(method, accessToken) {
|
||||||
|
return {
|
||||||
method: method,
|
method: method,
|
||||||
headers: {
|
headers: {
|
||||||
'Authorization': `Bearer ${accessToken}`,
|
'Authorization': `Bearer ${accessToken}`,
|
||||||
'Content-Type': 'application/json'
|
'Content-Type': 'application/json'
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Execute one HTTPS request and return a normalized result.
|
||||||
|
* @returns {Promise<{success: boolean, status: number, body: any, retryAfter?: string}>}
|
||||||
|
*/
|
||||||
|
function makeSingleRequest(url, options, data) {
|
||||||
|
return new Promise((resolve, reject) => {
|
||||||
const req = https.request(url, options, (res) => {
|
const req = https.request(url, options, (res) => {
|
||||||
let responseData = '';
|
let responseData = '';
|
||||||
|
|
||||||
|
|
@ -85,21 +155,21 @@ async function callGraphAPI(accessToken, method, path, data = null, queryParams
|
||||||
if (res.statusCode >= 200 && res.statusCode < 300) {
|
if (res.statusCode >= 200 && res.statusCode < 300) {
|
||||||
const trimmed = responseData.trim();
|
const trimmed = responseData.trim();
|
||||||
if (!trimmed) {
|
if (!trimmed) {
|
||||||
// Graph returns empty bodies for some successful operations
|
return resolve({ success: true, status: res.statusCode, body: {} });
|
||||||
// (e.g. sendMail 202, DELETE 204). Treat this as success.
|
|
||||||
return resolve({});
|
|
||||||
}
|
}
|
||||||
try {
|
try {
|
||||||
const jsonResponse = JSON.parse(trimmed);
|
const jsonResponse = JSON.parse(trimmed);
|
||||||
resolve(jsonResponse);
|
resolve({ success: true, status: res.statusCode, body: jsonResponse });
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
reject(new Error(`Error parsing API response: ${error.message}`));
|
reject(new Error(`Error parsing API response: ${error.message}`));
|
||||||
}
|
}
|
||||||
} else if (res.statusCode === 401) {
|
|
||||||
// Token expired or invalid
|
|
||||||
reject(new Error('UNAUTHORIZED'));
|
|
||||||
} else {
|
} else {
|
||||||
reject(new Error(`API call failed with status ${res.statusCode}: ${responseData}`));
|
resolve({
|
||||||
|
success: false,
|
||||||
|
status: res.statusCode,
|
||||||
|
body: responseData,
|
||||||
|
retryAfter: res.headers['retry-after']
|
||||||
|
});
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
@ -108,18 +178,16 @@ async function callGraphAPI(accessToken, method, path, data = null, queryParams
|
||||||
reject(new Error(`Network error during API call: ${error.message}`));
|
reject(new Error(`Network error during API call: ${error.message}`));
|
||||||
});
|
});
|
||||||
|
|
||||||
if (data && (method === 'POST' || method === 'PATCH' || method === 'PUT')) {
|
if (data && (options.method === 'POST' || options.method === 'PATCH' || options.method === 'PUT')) {
|
||||||
req.write(JSON.stringify(data));
|
req.write(JSON.stringify(data));
|
||||||
}
|
}
|
||||||
|
|
||||||
req.end();
|
req.end();
|
||||||
});
|
});
|
||||||
} catch (error) {
|
|
||||||
console.error('Error calling Graph API:', error);
|
|
||||||
throw error;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
module.exports = {
|
module.exports = {
|
||||||
callGraphAPI
|
callGraphAPI,
|
||||||
|
getRetryDelay,
|
||||||
|
sleep
|
||||||
};
|
};
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue