From daf2bacae5b59d04a8d9e1501fb471a06035e798 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 19:54:24 -0300 Subject: [PATCH 01/33] feat(core): add rate-limit hint parsers Pure functions that turn a throttled response into a hint: Retry-After (delta seconds, HTTP-date, ISO), the reset headers (epoch s, epoch ms or delta, told apart by magnitude) and the IETF RateLimit field. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- .../modules/requester/rate-limit/parsers.js | 268 +++++++++++++ .../requester/rate-limit/parsers.test.js | 356 ++++++++++++++++++ 2 files changed, 624 insertions(+) create mode 100644 packages/core/modules/requester/rate-limit/parsers.js create mode 100644 packages/core/modules/requester/rate-limit/parsers.test.js diff --git a/packages/core/modules/requester/rate-limit/parsers.js b/packages/core/modules/requester/rate-limit/parsers.js new file mode 100644 index 000000000..7cc3d3bf8 --- /dev/null +++ b/packages/core/modules/requester/rate-limit/parsers.js @@ -0,0 +1,268 @@ +const MAX_HINT_WAIT_MS = 366 * 24 * 60 * 60 * 1000; + +const SECONDS_PATTERN = /^\d+(?:\.\d+)?$/; +const HAS_LETTER = /[A-Za-z]/; +const EPOCH_SECONDS_MIN = 1e9; +const EPOCH_MILLISECONDS_MIN = 1e12; + +const RESET_HEADERS = [ + 'x-ratelimit-reset', + 'ratelimit-reset', + 'x-rate-limit-reset', +]; +const REMAINING_HEADERS = [ + 'x-ratelimit-remaining', + 'ratelimit-remaining', + 'x-rate-limit-remaining', +]; + +const isMissing = (value) => value === undefined || value === null; + +function readEntries(headers, wanted) { + for (const entry of headers) { + if (Array.isArray(entry) && String(entry[0]).toLowerCase() === wanted) { + return entry[1]; + } + } + return undefined; +} + +function readKeys(headers, wanted) { + for (const key of Object.keys(headers)) { + if (key.toLowerCase() === wanted) return headers[key]; + } + return undefined; +} + +/** + * Reads one header from a Headers-like object, a Map, an entries array or a + * plain object. The name match is case-insensitive. + * @returns {string|undefined} + */ +function headerValue(headers, name) { + if (!headers || typeof headers !== 'object') return undefined; + const wanted = name.toLowerCase(); + const hasGet = typeof headers.get === 'function'; + + let value; + if (hasGet) { + value = headers.get(name); + if (isMissing(value)) value = headers.get(wanted); + } + if (isMissing(value) && typeof headers[Symbol.iterator] === 'function') { + value = readEntries(headers, wanted); + } + if (isMissing(value) && !hasGet && !Array.isArray(headers)) { + value = readKeys(headers, wanted); + } + if (Array.isArray(value)) value = value[0]; + if (isMissing(value)) return undefined; + + const text = String(value).trim(); + return text === '' ? undefined : text; +} + +function definedOnly(fields) { + const result = {}; + for (const [key, value] of Object.entries(fields)) { + if (value !== undefined) result[key] = value; + } + return result; +} + +/** + * Builds a hint from a wait in ms. Returns null for a wait that is not a + * finite number in [0, MAX_HINT_WAIT_MS]. + */ +function hintFromWait(waitMs, now, { source = 'header', ...extra } = {}) { + if (!Number.isFinite(waitMs) || waitMs < 0 || waitMs > MAX_HINT_WAIT_MS) { + return null; + } + return { + waitMs, + retryAt: new Date(now + waitMs), + ...definedOnly(extra), + source, + }; +} + +/** + * Builds a hint from an absolute time. A time in the past waits 0 ms. + */ +function hintFromRetryAt(retryAt, now, { source = 'header', ...extra } = {}) { + const at = retryAt instanceof Date ? retryAt.getTime() : NaN; + if (Number.isNaN(at)) return null; + const waitMs = Math.max(0, at - now); + if (waitMs > MAX_HINT_WAIT_MS) return null; + return { waitMs, retryAt: new Date(at), ...definedOnly(extra), source }; +} + +function parseDate(text) { + if (!HAS_LETTER.test(text)) return null; + const at = Date.parse(text); + return Number.isNaN(at) ? null : new Date(at); +} + +/** + * Retry-After: delta seconds, an HTTP-date or an ISO timestamp. + */ +function parseRetryAfter(value, { now = Date.now() } = {}) { + if (isMissing(value)) return null; + const text = String(value).trim(); + if (text === '') return null; + if (SECONDS_PATTERN.test(text)) + return hintFromWait(Number(text) * 1000, now); + const date = parseDate(text); + return date ? hintFromRetryAt(date, now) : null; +} + +function numberOrUndefined(text) { + if (text === undefined) return undefined; + const number = Number(text); + return Number.isFinite(number) ? number : undefined; +} + +function firstHeader(headers, names) { + for (const name of names) { + const value = headerValue(headers, name); + if (value !== undefined) return value; + } + return undefined; +} + +function parseResetValue(text, now, extra) { + if (SECONDS_PATTERN.test(text)) { + const number = Number(text); + if (number >= EPOCH_MILLISECONDS_MIN) { + return hintFromRetryAt(new Date(number), now, extra); + } + if (number >= EPOCH_SECONDS_MIN) { + return hintFromRetryAt(new Date(number * 1000), now, extra); + } + return hintFromWait(number * 1000, now, extra); + } + const date = parseDate(text); + return date ? hintFromRetryAt(date, now, extra) : null; +} + +/** + * X-RateLimit-Reset and the other reset headers. The magnitude tells the + * unit: epoch milliseconds, epoch seconds, or delta seconds. + */ +function parseResetHeaders(headers, { now = Date.now() } = {}) { + const remaining = numberOrUndefined( + firstHeader(headers, REMAINING_HEADERS) + ); + const extra = { remaining }; + + const after = headerValue(headers, 'x-ratelimit-reset-after'); + if (after !== undefined && SECONDS_PATTERN.test(after)) { + return hintFromWait(Number(after) * 1000, now, extra); + } + + for (const name of RESET_HEADERS) { + const value = headerValue(headers, name); + if (value === undefined) continue; + const hint = parseResetValue(value, now, extra); + if (hint) return hint; + } + return null; +} + +function splitList(text) { + const items = []; + let current = ''; + let quoted = false; + for (const char of text) { + if (char === '"') quoted = !quoted; + if (char === ',' && !quoted) { + items.push(current); + current = ''; + } else { + current += char; + } + } + items.push(current); + return items.map((item) => item.trim()).filter(Boolean); +} + +function parseStructuredItem(item) { + const [head, ...params] = item.split(';').map((part) => part.trim()); + const values = {}; + for (const param of params) { + const [key, raw] = param.split('=').map((part) => part.trim()); + if (key) values[key] = numberOrUndefined(raw); + } + const name = head.replace(/^"|"$/g, ''); + return { + name: name || undefined, + remaining: values.r, + waitSeconds: values.t, + }; +} + +function pickStructured(items) { + const usable = items.filter( + (item) => Number.isFinite(item.waitSeconds) && item.waitSeconds >= 0 + ); + usable.sort((a, b) => { + const byRemaining = + (a.remaining ?? Infinity) - (b.remaining ?? Infinity); + if (byRemaining !== 0 && !Number.isNaN(byRemaining)) return byRemaining; + return b.waitSeconds - a.waitSeconds; + }); + return usable[0]; +} + +function readKeyValue(text, key) { + const match = new RegExp( + `(?:^|[,;\\s])${key}\\s*=\\s*([^,;\\s]+)`, + 'i' + ).exec(text); + return match ? match[1] : undefined; +} + +/** + * The IETF RateLimit field: the key=value form (limit=, remaining=, reset=) + * and the structured-field form ("name";r=0;t=12). The list form picks the + * policy with the fewest remaining calls, then the longest wait. + */ +function parseIetfRateLimit(headers, { now = Date.now() } = {}) { + const raw = headerValue(headers, 'ratelimit'); + if (raw === undefined) return null; + const policyHeader = headerValue(headers, 'ratelimit-policy'); + + if (/;\s*t\s*=/.test(raw)) { + const chosen = pickStructured(splitList(raw).map(parseStructuredItem)); + if (!chosen) return null; + return hintFromWait(chosen.waitSeconds * 1000, now, { + remaining: chosen.remaining, + policy: chosen.name ?? policyHeader, + }); + } + + const reset = readKeyValue(raw, 'reset'); + if (reset === undefined || !SECONDS_PATTERN.test(reset)) return null; + return hintFromWait(Number(reset) * 1000, now, { + remaining: numberOrUndefined(readKeyValue(raw, 'remaining')), + policy: policyHeader, + }); +} + +const BUILT_IN_PARSERS = { + retryAfter: (headers, { now } = {}) => + parseRetryAfter(headerValue(headers, 'retry-after'), { now }), + resetHeaders: parseResetHeaders, + ietf: parseIetfRateLimit, +}; + +module.exports = { + BUILT_IN_PARSERS, + MAX_HINT_WAIT_MS, + headerValue, + hintFromRetryAt, + hintFromWait, + parseIetfRateLimit, + parseResetHeaders, + parseRetryAfter, +}; diff --git a/packages/core/modules/requester/rate-limit/parsers.test.js b/packages/core/modules/requester/rate-limit/parsers.test.js new file mode 100644 index 000000000..5192688ff --- /dev/null +++ b/packages/core/modules/requester/rate-limit/parsers.test.js @@ -0,0 +1,356 @@ +const { + BUILT_IN_PARSERS, + MAX_HINT_WAIT_MS, + headerValue, + parseIetfRateLimit, + parseResetHeaders, + parseRetryAfter, +} = require('./parsers'); + +const NOW = Date.parse('2026-09-28T12:00:00.000Z'); +const MAX_SECONDS = MAX_HINT_WAIT_MS / 1000; + +describe('rate-limit/parsers', () => { + describe('headerValue', () => { + const cases = { + 'a Headers-like object': () => ({ + get: (name) => + name.toLowerCase() === 'retry-after' ? '5' : null, + }), + 'a Map with a differently cased key': () => + new Map([['Retry-After', '5']]), + 'an entries array': () => [['Retry-After', '5']], + 'a plain object with a differently cased key': () => ({ + 'RETRY-AFTER': '5', + }), + 'a Headers-like object that is also iterable': () => ({ + get: () => null, + [Symbol.iterator]: function* () { + yield ['retry-after', '5']; + }, + }), + }; + + it.each(Object.keys(cases))('reads a header from %s', (shape) => { + expect(headerValue(cases[shape](), 'retry-after')).toBe('5'); + }); + + it.each([ + ['null headers', null], + ['undefined headers', undefined], + ['a string', 'Retry-After: 5'], + ['an empty object', {}], + ['a get() that finds nothing', { get: () => null }], + ])('returns undefined for %s', (_label, headers) => { + expect(headerValue(headers, 'retry-after')).toBeUndefined(); + }); + + it('trims the value and treats a blank value as missing', () => { + expect(headerValue({ 'retry-after': ' 7 ' }, 'retry-after')).toBe( + '7' + ); + expect( + headerValue({ 'retry-after': ' ' }, 'retry-after') + ).toBeUndefined(); + }); + + it('takes the first value of a multi-value header', () => { + expect( + headerValue({ 'retry-after': ['9', '10'] }, 'retry-after') + ).toBe('9'); + }); + }); + + describe('parseRetryAfter', () => { + it.each([ + ['120', 120_000], + ['0', 0], + ['1.5', 1_500], + [' 7 ', 7_000], + [String(MAX_SECONDS), MAX_HINT_WAIT_MS], + ])('reads delta seconds %j as %d ms', (value, waitMs) => { + const hint = parseRetryAfter(value, { now: NOW }); + expect(hint).toEqual({ + waitMs, + retryAt: new Date(NOW + waitMs), + source: 'header', + }); + }); + + it('reads an HTTP-date in the future', () => { + const value = new Date(NOW + 30_000).toUTCString(); + expect(parseRetryAfter(value, { now: NOW })).toEqual({ + waitMs: 30_000, + retryAt: new Date(NOW + 30_000), + source: 'header', + }); + }); + + it('reads an ISO timestamp in the future', () => { + const value = new Date(NOW + 45_000).toISOString(); + expect(parseRetryAfter(value, { now: NOW })).toEqual({ + waitMs: 45_000, + retryAt: new Date(NOW + 45_000), + source: 'header', + }); + }); + + it('keeps the date but waits 0 ms for a date in the past', () => { + const past = new Date(NOW - 60_000); + expect(parseRetryAfter(past.toUTCString(), { now: NOW })).toEqual({ + waitMs: 0, + retryAt: past, + source: 'header', + }); + }); + + it.each([ + ['a negative number', '-5'], + ['a word', 'soon'], + ['an empty string', ''], + ['undefined', undefined], + ['null', null], + ['a number that is not finite', '1e400'], + ['seconds above the cap', String(MAX_SECONDS + 1)], + ['a hex number', '0x10'], + ['a digit string with a suffix', '12abc'], + ['a bare ISO date without a time', '2026-09-28'], + ])('returns null for %s', (_label, value) => { + expect(parseRetryAfter(value, { now: NOW })).toBeNull(); + }); + + it('returns null for an HTTP-date above the cap', () => { + const far = new Date(NOW + MAX_HINT_WAIT_MS + 60_000).toUTCString(); + expect(parseRetryAfter(far, { now: NOW })).toBeNull(); + }); + + it('uses the current time when no clock is given', () => { + const before = Date.now(); + const hint = parseRetryAfter('10'); + expect(hint.retryAt.getTime()).toBeGreaterThanOrEqual( + before + 10_000 + ); + }); + }); + + describe('parseResetHeaders', () => { + const epochSeconds = (ms) => String(Math.floor((NOW + ms) / 1000)); + + it.each([ + [ + 'x-ratelimit-reset', + epochSeconds(90_000), + 90_000, + 'epoch seconds', + ], + [ + 'x-ratelimit-reset', + String(NOW + 90_000), + 90_000, + 'epoch milliseconds', + ], + ['x-ratelimit-reset', '60', 60_000, 'delta seconds'], + [ + 'x-ratelimit-reset', + new Date(NOW + 15_000).toISOString(), + 15_000, + 'an ISO timestamp', + ], + [ + 'x-rate-limit-reset', + epochSeconds(20_000), + 20_000, + 'epoch seconds (x-rate-limit-reset)', + ], + [ + 'ratelimit-reset', + '12', + 12_000, + 'delta seconds (ratelimit-reset)', + ], + [ + 'x-ratelimit-reset-after', + '2.5', + 2_500, + 'fractional delta seconds', + ], + ])('reads %s = %s as %d ms (%s)', (name, value, waitMs) => { + expect(parseResetHeaders({ [name]: value }, { now: NOW })).toEqual({ + waitMs, + retryAt: new Date(NOW + waitMs), + source: 'header', + }); + }); + + it('waits 0 ms when the reset time is already past', () => { + const hint = parseResetHeaders( + { 'x-ratelimit-reset': epochSeconds(-30_000) }, + { now: NOW } + ); + expect(hint.waitMs).toBe(0); + expect(hint.retryAt).toEqual(new Date(NOW - 30_000)); + }); + + it('prefers reset-after over a reset time', () => { + const hint = parseResetHeaders( + { + 'x-ratelimit-reset': epochSeconds(90_000), + 'x-ratelimit-reset-after': '3', + }, + { now: NOW } + ); + expect(hint.waitMs).toBe(3_000); + }); + + it('carries the remaining count', () => { + const hint = parseResetHeaders( + { 'x-ratelimit-reset': '10', 'x-ratelimit-remaining': '0' }, + { now: NOW } + ); + expect(hint.remaining).toBe(0); + }); + + it('ignores a remaining count that is not a number', () => { + const hint = parseResetHeaders( + { 'x-ratelimit-reset': '10', 'x-ratelimit-remaining': 'many' }, + { now: NOW } + ); + expect(hint.remaining).toBeUndefined(); + }); + + it.each([ + ['no reset header', { 'x-ratelimit-remaining': '0' }], + [ + 'a reset value that is not a time', + { 'x-ratelimit-reset': 'soon' }, + ], + ['a negative reset value', { 'x-ratelimit-reset': '-4' }], + ['a reset above the cap', { 'x-ratelimit-reset': '99999999' }], + ['no headers at all', undefined], + ])('returns null for %s', (_label, headers) => { + expect(parseResetHeaders(headers, { now: NOW })).toBeNull(); + }); + + it.each([ + [ + 'a Headers-like object', + { + get: (n) => + ({ 'x-ratelimit-reset': '30' }[n.toLowerCase()] ?? + null), + }, + ], + ['a Map', new Map([['X-RateLimit-Reset', '30']])], + ['an entries array', [['X-RateLimit-Reset', '30']]], + ['a plain object', { 'X-RateLimit-Reset': '30' }], + ])('finds the header on %s', (_label, headers) => { + expect(parseResetHeaders(headers, { now: NOW }).waitMs).toBe( + 30_000 + ); + }); + }); + + describe('parseIetfRateLimit', () => { + it('reads the key=value form', () => { + expect( + parseIetfRateLimit( + { ratelimit: 'limit=100, remaining=0, reset=12' }, + { now: NOW } + ) + ).toEqual({ + waitMs: 12_000, + retryAt: new Date(NOW + 12_000), + remaining: 0, + source: 'header', + }); + }); + + it('reads the structured-field form and names the policy', () => { + expect( + parseIetfRateLimit( + { ratelimit: '"default";r=0;t=12' }, + { now: NOW } + ) + ).toEqual({ + waitMs: 12_000, + retryAt: new Date(NOW + 12_000), + remaining: 0, + policy: 'default', + source: 'header', + }); + }); + + it('picks the exhausted policy of a list', () => { + const hint = parseIetfRateLimit( + { ratelimit: '"burst";r=5;t=1, "daily";r=0;t=3600' }, + { now: NOW } + ); + expect(hint).toMatchObject({ + waitMs: 3_600_000, + remaining: 0, + policy: 'daily', + }); + }); + + it('breaks a tie between exhausted policies with the longer wait', () => { + const hint = parseIetfRateLimit( + { ratelimit: '"a";r=0;t=5, "b";r=0;t=60' }, + { now: NOW } + ); + expect(hint).toMatchObject({ waitMs: 60_000, policy: 'b' }); + }); + + it('falls back to the RateLimit-Policy value when the limit names no policy', () => { + const hint = parseIetfRateLimit( + { + ratelimit: 'limit=100, remaining=0, reset=12', + 'ratelimit-policy': '100;w=60', + }, + { now: NOW } + ); + expect(hint.policy).toBe('100;w=60'); + }); + + it.each([ + ['no header', {}], + ['a header with no reset', { ratelimit: 'limit=100, remaining=0' }], + ['a structured item with no t', { ratelimit: '"default";r=0' }], + ['garbage', { ratelimit: 'not a rate limit' }], + [ + 'a reset that is not a number', + { ratelimit: 'limit=1, reset=soon' }, + ], + ['undefined headers', undefined], + ])('returns null for %s', (_label, headers) => { + expect(parseIetfRateLimit(headers, { now: NOW })).toBeNull(); + }); + }); + + describe('BUILT_IN_PARSERS', () => { + it('names the three parsers with one signature', () => { + expect(Object.keys(BUILT_IN_PARSERS)).toEqual([ + 'retryAfter', + 'resetHeaders', + 'ietf', + ]); + expect( + BUILT_IN_PARSERS.retryAfter( + { 'retry-after': '4' }, + { now: NOW } + ).waitMs + ).toBe(4_000); + expect( + BUILT_IN_PARSERS.resetHeaders( + { 'x-ratelimit-reset': '4' }, + { now: NOW } + ).waitMs + ).toBe(4_000); + expect( + BUILT_IN_PARSERS.ietf( + { ratelimit: '"d";r=0;t=4' }, + { now: NOW } + ).waitMs + ).toBe(4_000); + }); + }); +}); From 5f97d7cb12e33fb6e0e85357f5152e5443483a9a Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 19:57:12 -0300 Subject: [PATCH 02/33] feat(core): resolve rate-limit hints from policy, parsers and backoff readRateLimitPolicy reads and validates the static `rateLimit` of an API module class. classifyRateLimit runs the module's classify(), then the header parsers, then the static policy. A classify() that names only a reason takes its time from the parsers or the policy. resolveRateLimitHint adds the fixed backoff ladder as the last step. computeWaitMs and inProcessBudgetMs decide how long a request may sleep in process: the hint wins, the module floor and a 1 s minimum apply, jitter only adds, and the wait stays inside a cumulative cap and the time left in the invocation. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- .../modules/requester/rate-limit/index.js | 4 + .../modules/requester/rate-limit/policy.js | 323 ++++++++ .../requester/rate-limit/policy.test.js | 739 ++++++++++++++++++ 3 files changed, 1066 insertions(+) create mode 100644 packages/core/modules/requester/rate-limit/index.js create mode 100644 packages/core/modules/requester/rate-limit/policy.js create mode 100644 packages/core/modules/requester/rate-limit/policy.test.js diff --git a/packages/core/modules/requester/rate-limit/index.js b/packages/core/modules/requester/rate-limit/index.js new file mode 100644 index 000000000..316e2d957 --- /dev/null +++ b/packages/core/modules/requester/rate-limit/index.js @@ -0,0 +1,4 @@ +const parsers = require('./parsers'); +const policy = require('./policy'); + +module.exports = { ...parsers, ...policy }; diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js new file mode 100644 index 000000000..a3d764887 --- /dev/null +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -0,0 +1,323 @@ +const { + BUILT_IN_PARSERS, + hintFromRetryAt, + hintFromWait, +} = require('./parsers'); + +const DEFAULT_PARSERS = ['retryAfter', 'resetHeaders', 'ietf']; +const DEFAULT_MAX_IN_PROCESS_WAIT_MS = 300_000; +const MIN_HINTED_WAIT_MS = 1_000; +const JITTER_MAX_MS = 1_000; +const JITTER_RATIO = 0.1; + +const REASONS = new Set([ + 'burst', + 'daily', + 'monthly', + 'concurrency', + 'unknown', +]); +const CLASSIFY_SOURCES = new Set(['header', 'body', 'static']); +const SCOPES = new Set(['credential', 'entity', 'app']); + +const normalizedPolicies = new WeakSet(); +const policiesByClass = new WeakMap(); + +const isObject = (value) => value !== null && typeof value === 'object'; + +function isPlainObject(value) { + if (!isObject(value) || Array.isArray(value)) return false; + const prototype = Object.getPrototypeOf(value); + return prototype === Object.prototype || prototype === null; +} + +function definedOnly(fields) { + const result = {}; + for (const [key, value] of Object.entries(fields)) { + if (value !== undefined) result[key] = value; + } + return result; +} + +function readParsers(raw, owner) { + if (!Array.isArray(raw.parsers)) return [...DEFAULT_PARSERS]; + for (const name of raw.parsers) { + if (!Object.hasOwn(BUILT_IN_PARSERS, name)) { + throw new TypeError( + `${owner}.rateLimit.parsers: unknown parser "${String( + name + )}"; known: ${Object.keys(BUILT_IN_PARSERS).join(', ')}` + ); + } + } + return [...raw.parsers]; +} + +function normalizePolicy(raw, owner) { + if (!isPlainObject(raw)) return undefined; + + const policy = { + scope: + SCOPES.has(raw.scope) || typeof raw.scope === 'function' + ? raw.scope + : 'entity', + parsers: readParsers(raw, owner), + }; + if (typeof raw.classify === 'function') { + policy.classify = (signal) => raw.classify(signal); + } + for (const key of ['minRetryAfterMs', 'maxInProcessWaitMs']) { + if (Number.isFinite(raw[key]) && raw[key] >= 0) policy[key] = raw[key]; + } + if (Number.isInteger(raw.maxConcurrency) && raw.maxConcurrency > 0) { + policy.maxConcurrency = raw.maxConcurrency; + } + if (Array.isArray(raw.windows)) policy.windows = raw.windows; + if (isPlainObject(raw.userHints)) policy.userHints = raw.userHints; + + normalizedPolicies.add(policy); + return policy; +} + +/** + * Reads the static `rateLimit` policy of an API module class. Returns + * undefined when the class declares none. The result is memoized per class. + * Throws a TypeError for a parser name that is not built in. + */ +function readRateLimitPolicy(ctor) { + if (typeof ctor !== 'function') return undefined; + if (policiesByClass.has(ctor)) return policiesByClass.get(ctor); + const policy = normalizePolicy(ctor.rateLimit, ctor.name || 'Requester'); + policiesByClass.set(ctor, policy); + return policy; +} + +function asPolicy(value) { + if (!isObject(value)) return undefined; + if (normalizedPolicies.has(value)) return value; + return normalizePolicy(value, 'policy'); +} + +/** + * The key a limit counts against, for one requester. Undefined when the + * scope needs an id that the requester does not have. + */ +function computeScopeKey(policy, requester) { + const label = requester?._telemetryModuleLabel?.() ?? 'unknown'; + const scope = policy?.scope ?? 'entity'; + const delegate = requester?.delegate; + + if (scope === 'app') return `${label}:app`; + if (scope === 'entity') { + const id = delegate?.entity?.id; + return id === undefined || id === null + ? undefined + : `${label}:entity:${id}`; + } + if (scope === 'credential') { + const credential = delegate?.credential; + const id = + credential?.id ?? + (typeof credential === 'string' ? credential : undefined); + return id === undefined || id === null + ? undefined + : `${label}:credential:${id}`; + } + if (typeof scope === 'function') { + try { + const value = scope(requester); + return value === undefined || value === null + ? undefined + : `${label}:custom:${String(value)}`; + } catch { + return undefined; + } + } + return undefined; +} + +function normalizeClassified(raw, now) { + if (!isObject(raw)) return null; + + const extra = { + reason: REASONS.has(raw.reason) ? raw.reason : 'unknown', + policy: typeof raw.policy === 'string' ? raw.policy : undefined, + remaining: Number.isFinite(raw.remaining) ? raw.remaining : undefined, + source: CLASSIFY_SOURCES.has(raw.source) ? raw.source : 'body', + }; + + let hint = null; + if (raw.retryAt !== undefined && raw.retryAt !== null) { + const date = + raw.retryAt instanceof Date ? raw.retryAt : new Date(raw.retryAt); + hint = hintFromRetryAt(date, now, extra); + } + if (!hint && Number.isFinite(raw.waitMs)) { + hint = hintFromWait(raw.waitMs, now, extra); + } + return { + reason: extra.reason, + policy: extra.policy, + remaining: extra.remaining, + hint, + }; +} + +function runClassify(policy, signal, now, onClassifyError) { + if (typeof policy?.classify !== 'function') return null; + let raw; + try { + raw = policy.classify({ + status: signal.status, + headers: signal.headers, + body: signal.body, + }); + } catch (error) { + if (!onClassifyError) throw error; + onClassifyError(error); + return null; + } + return normalizeClassified(raw, now); +} + +function parsedHint(policy, headers, now) { + for (const name of policy?.parsers ?? DEFAULT_PARSERS) { + const hint = BUILT_IN_PARSERS[name](headers, { now }); + if (hint) return hint; + } + return null; +} + +function staticHint(policy, reason, now) { + const window = policy?.windows?.find( + (candidate) => + candidate && + candidate.name === reason && + Number.isFinite(candidate.perMs) && + candidate.perMs > 0 + ); + const waitMs = Math.max(window?.perMs ?? 0, policy?.minRetryAfterMs ?? 0); + return waitMs > 0 ? hintFromWait(waitMs, now, { source: 'static' }) : null; +} + +function withClassified(hint, classified) { + return { + ...hint, + reason: classified?.reason ?? 'unknown', + ...definedOnly({ + policy: classified?.policy ?? hint.policy, + remaining: classified?.remaining ?? hint.remaining, + }), + }; +} + +function resolveCandidates(policy, signal, { now, onClassifyError }) { + const classified = runClassify(policy, signal, now, onClassifyError); + if (signal.status !== 429 && !classified) return { hint: null, classified }; + if (classified?.hint) return { hint: classified.hint, classified }; + + const parsed = parsedHint(policy, signal.headers, now); + if (parsed) return { hint: withClassified(parsed, classified), classified }; + + const fromPolicy = staticHint(policy, classified?.reason, now); + if (fromPolicy) { + return { hint: withClassified(fromPolicy, classified), classified }; + } + return { hint: null, classified }; +} + +/** + * Finds the hint of a throttled response: the module's classify(), then the + * header parsers, then the static policy. Returns null when there is none. + * A status other than 429 counts only when classify recognised it. + * + * @param {object} [policy] The static `rateLimit` object of an API module. + * @param {{status?: number, headers?: object, body?: unknown}} signal + * @param {{now?: number, onClassifyError?: (error: Error) => void}} [options] + * Without `onClassifyError` an error thrown by classify propagates. + */ +function classifyRateLimit(policy, signal, options = {}) { + const { now = Date.now(), onClassifyError } = options; + return resolveCandidates(asPolicy(policy), signal, { now, onClassifyError }) + .hint; +} + +/** + * Like classifyRateLimit, but always answers: with no hint it returns the + * step of the fixed backoff ladder, with source "backoff". + */ +function resolveRateLimitHint(policy, signal, options = {}) { + const { + attempt = 0, + backOff = [], + now = Date.now(), + onClassifyError, + } = options; + const { hint, classified } = resolveCandidates(asPolicy(policy), signal, { + now, + onClassifyError, + }); + if (hint) return hint; + + const waitMs = + attempt < backOff.length ? Number(backOff[attempt]) * 1000 || 0 : 0; + return { + waitMs, + retryAt: new Date(now + waitMs), + reason: classified?.reason ?? 'unknown', + ...definedOnly({ + policy: classified?.policy, + remaining: classified?.remaining, + }), + source: 'backoff', + }; +} + +/** + * The wait before the next attempt, for a hint that came from the provider + * or the policy. Jitter only adds to the wait and never passes the budget. + * `fits` is false when the wait alone is over the budget. + */ +function computeWaitMs({ hint, policy, budgetMs, random = Math.random }) { + const base = Math.max( + hint.waitMs, + policy?.minRetryAfterMs ?? 0, + MIN_HINTED_WAIT_MS + ); + if (base > budgetMs) return { waitMs: base, fits: false }; + + const jitter = Math.floor( + random() * Math.min(JITTER_MAX_MS, base * JITTER_RATIO) + ); + return { waitMs: Math.min(base + jitter, budgetMs), fits: true }; +} + +/** + * How long this request may still sleep in process: the cap, less what it + * slept already, and the time left in the invocation, less one request. + */ +function inProcessBudgetMs({ + policy, + requestTimeoutMs = 0, + remainingMs = Infinity, + waitedMs = 0, +} = {}) { + const cap = policy?.maxInProcessWaitMs ?? DEFAULT_MAX_IN_PROCESS_WAIT_MS; + return Math.max( + 0, + Math.min(cap - waitedMs, remainingMs - (requestTimeoutMs || 0)) + ); +} + +module.exports = { + DEFAULT_MAX_IN_PROCESS_WAIT_MS, + DEFAULT_PARSERS, + JITTER_MAX_MS, + MIN_HINTED_WAIT_MS, + classifyRateLimit, + computeScopeKey, + computeWaitMs, + inProcessBudgetMs, + readRateLimitPolicy, + resolveRateLimitHint, +}; diff --git a/packages/core/modules/requester/rate-limit/policy.test.js b/packages/core/modules/requester/rate-limit/policy.test.js new file mode 100644 index 000000000..2a2b1e5b0 --- /dev/null +++ b/packages/core/modules/requester/rate-limit/policy.test.js @@ -0,0 +1,739 @@ +const { + DEFAULT_MAX_IN_PROCESS_WAIT_MS, + DEFAULT_PARSERS, + JITTER_MAX_MS, + MIN_HINTED_WAIT_MS, + classifyRateLimit, + computeScopeKey, + computeWaitMs, + inProcessBudgetMs, + readRateLimitPolicy, + resolveRateLimitHint, +} = require('./policy'); + +const NOW = Date.parse('2026-09-28T12:00:00.000Z'); +const options = { now: NOW }; + +describe('rate-limit/policy', () => { + it('exposes the defaults', () => { + expect(DEFAULT_PARSERS).toEqual(['retryAfter', 'resetHeaders', 'ietf']); + expect(DEFAULT_MAX_IN_PROCESS_WAIT_MS).toBe(300_000); + expect(MIN_HINTED_WAIT_MS).toBe(1_000); + expect(JITTER_MAX_MS).toBe(1_000); + }); + + describe('readRateLimitPolicy', () => { + it('returns undefined when the class declares nothing', () => { + class Api {} + expect(readRateLimitPolicy(Api)).toBeUndefined(); + }); + + it.each([ + ['a number', 5], + ['a string', 'fast'], + ['an array', []], + ['null', null], + ])('returns undefined when rateLimit is %s', (_label, value) => { + class Api { + static rateLimit = value; + } + expect(readRateLimitPolicy(Api)).toBeUndefined(); + }); + + it('returns undefined for something that is not a class', () => { + expect(readRateLimitPolicy(undefined)).toBeUndefined(); + expect(readRateLimitPolicy({ rateLimit: {} })).toBeUndefined(); + }); + + it('defaults the scope to entity and the parsers to the built-ins', () => { + class Api { + static rateLimit = {}; + } + expect(readRateLimitPolicy(Api)).toEqual({ + scope: 'entity', + parsers: DEFAULT_PARSERS, + }); + }); + + it('keeps the known keys and ignores the unknown ones', () => { + const windows = [{ name: 'burst', limit: 100, perMs: 10_000 }]; + const userHints = { daily: { links: [] } }; + class Api { + static rateLimit = { + scope: 'credential', + windows, + maxConcurrency: 5, + minRetryAfterMs: 60_000, + maxInProcessWaitMs: 30_000, + parsers: ['retryAfter'], + userHints, + somethingElse: true, + }; + } + const policy = readRateLimitPolicy(Api); + expect(policy).toMatchObject({ + scope: 'credential', + windows, + maxConcurrency: 5, + minRetryAfterMs: 60_000, + maxInProcessWaitMs: 30_000, + parsers: ['retryAfter'], + userHints, + }); + expect(policy).not.toHaveProperty('somethingElse'); + }); + + it('drops values of the wrong type', () => { + class Api { + static rateLimit = { + classify: 'nope', + minRetryAfterMs: -1, + maxInProcessWaitMs: Infinity, + maxConcurrency: 1.5, + windows: 'many', + userHints: 3, + scope: 'galaxy', + }; + } + expect(readRateLimitPolicy(Api)).toEqual({ + scope: 'entity', + parsers: DEFAULT_PARSERS, + }); + }); + + it('falls back to the default parsers when parsers is not an array', () => { + class Api { + static rateLimit = { parsers: 'retryAfter' }; + } + expect(readRateLimitPolicy(Api).parsers).toEqual(DEFAULT_PARSERS); + }); + + it('accepts an empty parser list', () => { + class Api { + static rateLimit = { parsers: [] }; + } + expect(readRateLimitPolicy(Api).parsers).toEqual([]); + }); + + it('accepts a function as the scope', () => { + const scope = () => 'x'; + class Api { + static rateLimit = { scope }; + } + expect(readRateLimitPolicy(Api).scope).toBe(scope); + }); + + it('throws a TypeError that names the class and the known parsers', () => { + class HubApi { + static rateLimit = { parsers: ['retryAfter', 'magic'] }; + } + expect(() => readRateLimitPolicy(HubApi)).toThrow(TypeError); + expect(() => readRateLimitPolicy(HubApi)).toThrow( + 'HubApi.rateLimit.parsers: unknown parser "magic"; known: retryAfter, resetHeaders, ietf' + ); + }); + + it('calls classify as a method of the declared object', () => { + class Api { + static rateLimit = { + reason: 'burst', + classify() { + return { reason: this.reason, waitMs: 1_000 }; + }, + }; + } + const policy = readRateLimitPolicy(Api); + expect(policy.classify({ status: 429 })).toEqual({ + reason: 'burst', + waitMs: 1_000, + }); + }); + + it('is memoized per class and resolved through subclasses', () => { + class Base { + static rateLimit = { minRetryAfterMs: 5_000 }; + } + class Child extends Base {} + const first = readRateLimitPolicy(Child); + expect(readRateLimitPolicy(Child)).toBe(first); + expect(first.minRetryAfterMs).toBe(5_000); + }); + }); + + describe('computeScopeKey', () => { + const requester = (delegate) => ({ + _telemetryModuleLabel: () => 'hubspot', + delegate, + }); + + it('keys an entity scope on the entity id, by default', () => { + const subject = requester({ entity: { id: 7 } }); + expect(computeScopeKey(undefined, subject)).toBe( + 'hubspot:entity:7' + ); + expect(computeScopeKey({ scope: 'entity' }, subject)).toBe( + 'hubspot:entity:7' + ); + }); + + it('keys a credential scope on the credential id or a string credential', () => { + expect( + computeScopeKey( + { scope: 'credential' }, + requester({ credential: { id: 3 } }) + ) + ).toBe('hubspot:credential:3'); + expect( + computeScopeKey( + { scope: 'credential' }, + requester({ credential: 'c-9' }) + ) + ).toBe('hubspot:credential:c-9'); + }); + + it('keys an app scope on the module alone', () => { + expect(computeScopeKey({ scope: 'app' }, requester({}))).toBe( + 'hubspot:app' + ); + }); + + it('keys a function scope on its result', () => { + const scope = (subject) => subject.delegate.entity.externalId; + expect( + computeScopeKey( + { scope }, + requester({ entity: { externalId: 'portal-1' } }) + ) + ).toBe('hubspot:custom:portal-1'); + }); + + it.each([ + ['no entity', { scope: 'entity' }, requester({})], + ['no credential', { scope: 'credential' }, requester({})], + ['no delegate', { scope: 'entity' }, requester(undefined)], + [ + 'a function that throws', + { + scope: () => { + throw new Error('boom'); + }, + }, + requester({}), + ], + [ + 'a function that returns nothing', + { scope: () => undefined }, + requester({}), + ], + ])('returns undefined for %s', (_label, policy, subject) => { + expect(computeScopeKey(policy, subject)).toBeUndefined(); + }); + }); + + describe('classifyRateLimit', () => { + it('returns null with no policy and no headers', () => { + expect( + classifyRateLimit( + undefined, + { status: 429, headers: {} }, + options + ) + ).toBeNull(); + }); + + it('reads Retry-After on a 429 with no policy', () => { + expect( + classifyRateLimit( + undefined, + { status: 429, headers: { 'retry-after': '5' } }, + options + ) + ).toEqual({ + waitMs: 5_000, + retryAt: new Date(NOW + 5_000), + reason: 'unknown', + source: 'header', + }); + }); + + it('does not read headers on a status other than 429 that nothing classified', () => { + expect( + classifyRateLimit( + undefined, + { status: 503, headers: { 'retry-after': '5' } }, + options + ) + ).toBeNull(); + expect( + classifyRateLimit( + { classify: () => null }, + { status: 403, headers: { 'retry-after': '5' } }, + options + ) + ).toBeNull(); + }); + + it('takes a classify hint that carries a time, and defaults its source to body', () => { + const policy = { + classify: () => ({ reason: 'daily', waitMs: 60_000 }), + }; + expect( + classifyRateLimit(policy, { status: 403, headers: {} }, options) + ).toEqual({ + waitMs: 60_000, + retryAt: new Date(NOW + 60_000), + reason: 'daily', + source: 'body', + }); + }); + + it('keeps a classify retryAt, as a Date, a timestamp or a number', () => { + const retryAt = new Date(NOW + 3_600_000); + for (const value of [ + retryAt, + retryAt.toISOString(), + retryAt.getTime(), + ]) { + const policy = { + classify: () => ({ reason: 'daily', retryAt: value }), + }; + const hint = classifyRateLimit( + policy, + { status: 429, headers: {} }, + options + ); + expect(hint.waitMs).toBe(3_600_000); + expect(hint.retryAt).toEqual(retryAt); + } + }); + + it('keeps the source classify gives, when it is a known one', () => { + const build = (source) => ({ + classify: () => ({ waitMs: 1_000, source }), + }); + const signal = { status: 429, headers: {} }; + expect( + classifyRateLimit(build('static'), signal, options).source + ).toBe('static'); + expect( + classifyRateLimit(build('backoff'), signal, options).source + ).toBe('body'); + }); + + it('lets a classify time win over Retry-After', () => { + const policy = { + classify: () => ({ reason: 'daily', waitMs: 60_000 }), + }; + const hint = classifyRateLimit( + policy, + { status: 429, headers: { 'retry-after': '2' } }, + options + ); + expect(hint.waitMs).toBe(60_000); + }); + + it('takes the time of a reason-only classify from the first parser', () => { + const policy = { + classify: () => ({ reason: 'burst', policy: 'SECONDLY' }), + }; + expect( + classifyRateLimit( + policy, + { status: 429, headers: { 'retry-after': '7' } }, + options + ) + ).toEqual({ + waitMs: 7_000, + retryAt: new Date(NOW + 7_000), + reason: 'burst', + policy: 'SECONDLY', + source: 'header', + }); + }); + + it('takes the time of a reason-only classify from the window when no header helps', () => { + const policy = { + windows: [{ name: 'burst', limit: 110, perMs: 10_000 }], + classify: () => ({ reason: 'burst' }), + }; + expect( + classifyRateLimit(policy, { status: 429, headers: {} }, options) + ).toEqual({ + waitMs: 10_000, + retryAt: new Date(NOW + 10_000), + reason: 'burst', + source: 'static', + }); + }); + + it('returns null for a reason-only classify with no time anywhere', () => { + const policy = { classify: () => ({ reason: 'daily' }) }; + expect( + classifyRateLimit(policy, { status: 403, headers: {} }, options) + ).toBeNull(); + }); + + it('treats a classify time that is not valid as no time', () => { + const policy = { + classify: () => ({ reason: 'daily', waitMs: -5 }), + }; + expect( + classifyRateLimit(policy, { status: 403, headers: {} }, options) + ).toBeNull(); + }); + + it('turns a static minRetryAfterMs into the hint of a bare 429', () => { + expect( + classifyRateLimit( + { minRetryAfterMs: 60_000 }, + { status: 429, headers: {} }, + options + ) + ).toEqual({ + waitMs: 60_000, + retryAt: new Date(NOW + 60_000), + reason: 'unknown', + source: 'static', + }); + }); + + it('reads the parsers in the order the policy lists them', () => { + const headers = { 'retry-after': '5', 'x-ratelimit-reset': '30' }; + const signal = { status: 429, headers }; + expect( + classifyRateLimit( + { parsers: ['resetHeaders', 'retryAfter'] }, + signal, + options + ).waitMs + ).toBe(30_000); + expect(classifyRateLimit(undefined, signal, options).waitMs).toBe( + 5_000 + ); + }); + + it('does not use a parser the policy leaves out', () => { + expect( + classifyRateLimit( + { parsers: ['retryAfter'] }, + { status: 429, headers: { 'x-ratelimit-reset': '30' } }, + options + ) + ).toBeNull(); + }); + + it('carries remaining and policy from a parser', () => { + const hint = classifyRateLimit( + undefined, + { + status: 429, + headers: { ratelimit: '"daily";r=0;t=3600' }, + }, + options + ); + expect(hint).toMatchObject({ + remaining: 0, + policy: 'daily', + waitMs: 3_600_000, + }); + }); + + it('turns a reason it does not know into unknown', () => { + const policy = { + classify: () => ({ reason: 'hourly', waitMs: 1_000 }), + }; + expect( + classifyRateLimit(policy, { status: 429, headers: {} }, options) + .reason + ).toBe('unknown'); + }); + + it('hands classify the status, the headers and the body', () => { + const classify = jest.fn(() => null); + const headers = { 'x-a': '1' }; + const body = { code: 'LIMIT' }; + classifyRateLimit( + { classify }, + { status: 403, headers, body }, + options + ); + expect(classify).toHaveBeenCalledWith({ + status: 403, + headers, + body, + }); + }); + + it('lets a throwing classify propagate when nobody listens', () => { + const policy = { + classify: () => { + throw new Error('bad classifier'); + }, + }; + expect(() => + classifyRateLimit(policy, { status: 429, headers: {} }, options) + ).toThrow('bad classifier'); + }); + + it('reports a throwing classify and goes on with the parsers', () => { + const error = new Error('bad classifier'); + const policy = { + classify: () => { + throw error; + }, + }; + const onClassifyError = jest.fn(); + const hint = classifyRateLimit( + policy, + { status: 429, headers: { 'retry-after': '4' } }, + { ...options, onClassifyError } + ); + expect(onClassifyError).toHaveBeenCalledWith(error); + expect(hint.waitMs).toBe(4_000); + }); + + it('accepts the policy object a module declares, without reading it first', () => { + const declared = { + parsers: ['retryAfter'], + classify: () => ({ reason: 'daily', waitMs: 1_000 }), + }; + expect( + classifyRateLimit( + declared, + { status: undefined, headers: {} }, + options + ).reason + ).toBe('daily'); + }); + }); + + describe('resolveRateLimitHint', () => { + const backOff = [1, 3, 10]; + + it('returns the hint when there is one', () => { + const hint = resolveRateLimitHint( + undefined, + { status: 429, headers: { 'retry-after': '9' } }, + { ...options, attempt: 0, backOff } + ); + expect(hint).toMatchObject({ waitMs: 9_000, source: 'header' }); + }); + + it('falls back to the ladder step with source backoff', () => { + const hint = resolveRateLimitHint( + undefined, + { status: 429, headers: {} }, + { ...options, attempt: 2, backOff } + ); + expect(hint).toEqual({ + waitMs: 10_000, + retryAt: new Date(NOW + 10_000), + reason: 'unknown', + source: 'backoff', + }); + }); + + it('waits 0 ms past the end of the ladder', () => { + const hint = resolveRateLimitHint( + undefined, + { status: 429, headers: {} }, + { ...options, attempt: 3, backOff } + ); + expect(hint).toMatchObject({ waitMs: 0, source: 'backoff' }); + }); + + it('keeps the reason and policy of a reason-only classify in the fallback', () => { + const policy = { + classify: () => ({ reason: 'daily', policy: 'DAILY' }), + }; + const hint = resolveRateLimitHint( + policy, + { status: 429, headers: {} }, + { ...options, attempt: 0, backOff } + ); + expect(hint).toMatchObject({ + reason: 'daily', + policy: 'DAILY', + source: 'backoff', + waitMs: 1_000, + }); + }); + + it('reports a throwing classify and still answers', () => { + const onClassifyError = jest.fn(); + const policy = { + classify: () => { + throw new Error('boom'); + }, + }; + const hint = resolveRateLimitHint( + policy, + { status: 429, headers: {} }, + { ...options, attempt: 0, backOff, onClassifyError } + ); + expect(onClassifyError).toHaveBeenCalledTimes(1); + expect(hint.source).toBe('backoff'); + }); + }); + + describe('computeWaitMs', () => { + const hintOf = (waitMs) => ({ + waitMs, + retryAt: new Date(NOW + waitMs), + source: 'header', + }); + const noJitter = () => 0; + const maxJitter = () => 0.999999; + + it('waits as long as the hint says', () => { + expect( + computeWaitMs({ + hint: hintOf(5_000), + budgetMs: 300_000, + random: noJitter, + }) + ).toEqual({ waitMs: 5_000, fits: true }); + }); + + it('applies minRetryAfterMs as a floor', () => { + expect( + computeWaitMs({ + hint: hintOf(5_000), + policy: { minRetryAfterMs: 60_000 }, + budgetMs: 300_000, + random: noJitter, + }).waitMs + ).toBe(60_000); + }); + + it('applies the minimum wait', () => { + expect( + computeWaitMs({ + hint: hintOf(0), + budgetMs: 300_000, + random: noJitter, + }).waitMs + ).toBe(MIN_HINTED_WAIT_MS); + }); + + it('adds jitter of at most 10 percent of the wait, and at most 1 s', () => { + expect( + computeWaitMs({ + hint: hintOf(5_000), + budgetMs: 300_000, + random: maxJitter, + }).waitMs + ).toBe(5_499); + expect( + computeWaitMs({ + hint: hintOf(60_000), + budgetMs: 300_000, + random: maxJitter, + }).waitMs + ).toBe(60_999); + }); + + it('never lets jitter push a wait over the budget', () => { + expect( + computeWaitMs({ + hint: hintOf(5_000), + budgetMs: 5_100, + random: maxJitter, + }) + ).toEqual({ waitMs: 5_100, fits: true }); + }); + + it('fits a wait that equals the budget', () => { + expect( + computeWaitMs({ + hint: hintOf(5_000), + budgetMs: 5_000, + random: maxJitter, + }) + ).toEqual({ waitMs: 5_000, fits: true }); + }); + + it('reports that a wait over the budget does not fit, with no jitter', () => { + expect( + computeWaitMs({ + hint: hintOf(5_000), + budgetMs: 4_000, + random: maxJitter, + }) + ).toEqual({ waitMs: 5_000, fits: false }); + }); + + it('uses Math.random when no source of randomness is given', () => { + const { waitMs } = computeWaitMs({ + hint: hintOf(5_000), + budgetMs: 300_000, + }); + expect(waitMs).toBeGreaterThanOrEqual(5_000); + expect(waitMs).toBeLessThan(5_500); + }); + }); + + describe('inProcessBudgetMs', () => { + it('defaults the cap to 5 minutes', () => { + expect( + inProcessBudgetMs({ + requestTimeoutMs: 0, + remainingMs: Infinity, + waitedMs: 0, + }) + ).toBe(300_000); + }); + + it('lets the policy override the cap', () => { + expect( + inProcessBudgetMs({ + policy: { maxInProcessWaitMs: 30_000 }, + remainingMs: Infinity, + }) + ).toBe(30_000); + expect( + inProcessBudgetMs({ + policy: { maxInProcessWaitMs: 0 }, + remainingMs: Infinity, + }) + ).toBe(0); + }); + + it('subtracts the wait already spent', () => { + expect( + inProcessBudgetMs({ remainingMs: Infinity, waitedMs: 100_000 }) + ).toBe(200_000); + }); + + it('uses the time left in the invocation, less one request', () => { + expect( + inProcessBudgetMs({ + requestTimeoutMs: 60_000, + remainingMs: 100_000, + }) + ).toBe(40_000); + }); + + it('is never negative', () => { + expect( + inProcessBudgetMs({ + requestTimeoutMs: 60_000, + remainingMs: 1_000, + }) + ).toBe(0); + expect( + inProcessBudgetMs({ remainingMs: Infinity, waitedMs: 400_000 }) + ).toBe(0); + }); + + it('treats a missing request timeout as 0', () => { + expect(inProcessBudgetMs({ remainingMs: 50_000 })).toBe(50_000); + expect( + inProcessBudgetMs({ + requestTimeoutMs: null, + remainingMs: 50_000, + }) + ).toBe(50_000); + }); + }); +}); From 4fa693158f0b03cc063e09418febb5baf7d4f6d8 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:10:58 -0300 Subject: [PATCH 03/33] feat(core): add RateLimitError A FetchError for a response that said a limit was hit and told us, or the module's policy told us, when to call again. It keeps statusCode and the redacted url and adds isRateLimited, retryAt, waitMs, reason, policy, source, module and scopeKey. A module whose client is not the Requester can build one from a hint with classifyRateLimit. FetchError.create now builds the class it is called on, and uses a responseBody it is given instead of reading the stream a second time. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- packages/core/errors/fetch-error.js | 18 +- packages/core/errors/fetch-error.test.js | 28 +++ packages/core/errors/index.js | 2 + packages/core/errors/rate-limit-error.js | 44 ++++ packages/core/errors/rate-limit-error.test.js | 205 ++++++++++++++++++ 5 files changed, 291 insertions(+), 6 deletions(-) create mode 100644 packages/core/errors/rate-limit-error.js create mode 100644 packages/core/errors/rate-limit-error.test.js diff --git a/packages/core/errors/fetch-error.js b/packages/core/errors/fetch-error.js index 545c8087a..ebba099ef 100644 --- a/packages/core/errors/fetch-error.js +++ b/packages/core/errors/fetch-error.js @@ -55,12 +55,18 @@ class FetchError extends BaseError { static async create(options = {}) { const { response } = options; - let responseBody = - response && !response.bodyUsed && typeof response.text === 'function' - ? await response.text() - : null; - if (!responseBody) responseBody = options.responseBody ?? options.body; - return new FetchError({ ...options, responseBody }); + const provided = options.responseBody ?? options.body; + let responseBody = provided; + if ( + !responseBody && + response && + !response.bodyUsed && + typeof response.text === 'function' + ) { + responseBody = (await response.text()) || provided; + } + const ErrorClass = typeof this === 'function' ? this : FetchError; + return new ErrorClass({ ...options, responseBody }); } } diff --git a/packages/core/errors/fetch-error.test.js b/packages/core/errors/fetch-error.test.js index bf7c8ba3c..604324cf1 100644 --- a/packages/core/errors/fetch-error.test.js +++ b/packages/core/errors/fetch-error.test.js @@ -192,6 +192,34 @@ describe('FetchError', () => { expect(error.body).toBe('fallback'); }); + it('create() uses a given responseBody and does not read the stream again', async () => { + const text = jest.fn(async () => 'second read'); + const error = await FetchError.create({ + response: { status: 429, bodyUsed: false, text }, + responseBody: 'already read', + }); + expect(text).not.toHaveBeenCalled(); + expect(error.body).toBe('already read'); + }); + + it('create() constructs the subclass it is called on', async () => { + class ProviderError extends FetchError {} + const error = await ProviderError.create({ + resource: 'https://h.example', + response: { status: 400, bodyUsed: true }, + }); + expect(error).toBeInstanceOf(ProviderError); + expect(error.name).toBe('ProviderError'); + }); + + it('create() still builds a FetchError when called without its class', async () => { + const { create } = FetchError; + const error = await create({ + response: { status: 400, bodyUsed: true }, + }); + expect(error).toBeInstanceOf(FetchError); + }); + it('create() propagates a rejected body read', async () => { const aborted = new Error('aborted mid-body'); aborted.name = 'AbortError'; diff --git a/packages/core/errors/index.js b/packages/core/errors/index.js index 836274679..b224b2cf1 100644 --- a/packages/core/errors/index.js +++ b/packages/core/errors/index.js @@ -1,6 +1,7 @@ const { BaseError } = require('./base-error'); const { FetchError } = require('./fetch-error'); const { HaltError } = require('./halt-error'); +const { RateLimitError } = require('./rate-limit-error'); const { RequiredPropertyError, ParameterTypeError, @@ -11,6 +12,7 @@ module.exports = { BaseError, FetchError, HaltError, + RateLimitError, RequiredPropertyError, ParameterTypeError, ClientSafeError, diff --git a/packages/core/errors/rate-limit-error.js b/packages/core/errors/rate-limit-error.js new file mode 100644 index 000000000..cacc05290 --- /dev/null +++ b/packages/core/errors/rate-limit-error.js @@ -0,0 +1,44 @@ +const { FetchError } = require('./fetch-error'); + +function resolveTiming({ hint, waitMs, now }) { + if (waitMs !== undefined) { + return { waitMs, retryAt: new Date(now + waitMs) }; + } + if (hint?.retryAt) { + const retryAt = new Date(hint.retryAt); + if (!Number.isNaN(retryAt.getTime())) { + return { waitMs: Math.max(0, retryAt.getTime() - now), retryAt }; + } + } + const fallbackWaitMs = hint?.waitMs ?? 0; + return { waitMs: fallbackWaitMs, retryAt: new Date(now + fallbackWaitMs) }; +} + +/** + * A FetchError for a response that said a limit was hit and told us, or the + * module's policy told us, when to call again. `retryAt` is that time. + */ +class RateLimitError extends FetchError { + constructor({ + hint, + waitMs, + module, + scopeKey, + now = Date.now(), + ...fetchErrorArgs + } = {}) { + super(fetchErrorArgs); + + const timing = resolveTiming({ hint, waitMs, now }); + this.isRateLimited = true; + this.waitMs = timing.waitMs; + this.retryAt = timing.retryAt; + this.reason = hint?.reason ?? 'unknown'; + this.policy = hint?.policy; + this.source = hint?.source ?? 'unknown'; + this.module = module; + this.scopeKey = scopeKey; + } +} + +module.exports = { RateLimitError }; diff --git a/packages/core/errors/rate-limit-error.test.js b/packages/core/errors/rate-limit-error.test.js new file mode 100644 index 000000000..2c203d172 --- /dev/null +++ b/packages/core/errors/rate-limit-error.test.js @@ -0,0 +1,205 @@ +const util = require('node:util'); +const { BaseError } = require('./base-error'); +const { FetchError } = require('./fetch-error'); +const errors = require('./index'); +const { RateLimitError } = require('./rate-limit-error'); +const { serializeError } = require('../logs'); +const { SECRETS } = require('../logs/__fixtures__/secrets'); +const { toContainNoSecretWindow } = require('../logs/__fixtures__/matchers'); + +expect.extend({ toContainNoSecretWindow }); + +const NOW = Date.parse('2026-09-28T12:00:00.000Z'); + +const secretUrl = `https://user:${SECRETS.password}@api.example.com/v1/items?api_key=${SECRETS.apiKeyQuery}&page=2`; +const sanitizedUrl = + 'https://api.example.com/v1/items?api_key=REDACTED&page=REDACTED'; + +function secretInit() { + return { + method: 'POST', + headers: { Authorization: `Bearer ${SECRETS.bearer}` }, + body: `client_secret=${SECRETS.clientSecret}`, + }; +} + +function throttledResponse(overrides = {}) { + return { + status: 429, + bodyUsed: false, + text: jest.fn(async () => `{"access_token":"${SECRETS.accessToken}"}`), + ...overrides, + }; +} + +const hint = { + waitMs: 5_000, + retryAt: new Date(NOW + 5_000), + reason: 'daily', + policy: 'DAILY', + source: 'header', +}; + +describe('RateLimitError', () => { + it('is a FetchError and a BaseError named RateLimitError', () => { + const error = new RateLimitError({ hint, now: NOW }); + expect(error).toBeInstanceOf(FetchError); + expect(error).toBeInstanceOf(BaseError); + expect(error.name).toBe('RateLimitError'); + }); + + it('is exported from the errors index', () => { + expect(errors.RateLimitError).toBe(RateLimitError); + }); + + it('can be built with no arguments', () => { + const error = new RateLimitError(); + expect(error.isRateLimited).toBe(true); + expect(error.waitMs).toBe(0); + expect(error.reason).toBe('unknown'); + expect(error.source).toBe('unknown'); + }); + + it('flags itself as rate limited', () => { + expect(new RateLimitError({ hint }).isRateLimited).toBe(true); + }); + + describe('retryAt and waitMs', () => { + it('counts a given waitMs from now', () => { + const error = new RateLimitError({ hint, waitMs: 8_000, now: NOW }); + expect(error.waitMs).toBe(8_000); + expect(error.retryAt).toEqual(new Date(NOW + 8_000)); + }); + + it('keeps the retryAt of the hint when no waitMs is given', () => { + const error = new RateLimitError({ hint, now: NOW + 1_000 }); + expect(error.retryAt).toEqual(hint.retryAt); + expect(error.waitMs).toBe(4_000); + }); + + it('waits 0 ms when the retryAt of the hint has passed', () => { + const error = new RateLimitError({ hint, now: NOW + 60_000 }); + expect(error.retryAt).toEqual(hint.retryAt); + expect(error.waitMs).toBe(0); + }); + + it('uses the waitMs of a hint that has no retryAt', () => { + const error = new RateLimitError({ + hint: { waitMs: 2_000 }, + now: NOW, + }); + expect(error.waitMs).toBe(2_000); + expect(error.retryAt).toEqual(new Date(NOW + 2_000)); + }); + + it('ignores a retryAt that is not a valid date', () => { + const error = new RateLimitError({ + hint: { retryAt: 'never', waitMs: 3_000 }, + now: NOW, + }); + expect(error.waitMs).toBe(3_000); + expect(error.retryAt).toEqual(new Date(NOW + 3_000)); + }); + + it('reads the clock when no time is given', () => { + const before = Date.now(); + const error = new RateLimitError({ waitMs: 1_000 }); + expect(error.retryAt.getTime()).toBeGreaterThanOrEqual( + before + 1_000 + ); + }); + }); + + describe('fields', () => { + it('carries the reason, policy, source, module and scope key', async () => { + const error = await RateLimitError.create({ + resource: secretUrl, + init: secretInit(), + response: throttledResponse(), + hint, + waitMs: 5_000, + module: 'hubspot', + scopeKey: 'hubspot:entity:7', + }); + expect(error).toMatchObject({ + isRateLimited: true, + statusCode: 429, + method: 'POST', + url: sanitizedUrl, + waitMs: 5_000, + reason: 'daily', + policy: 'DAILY', + source: 'header', + module: 'hubspot', + scopeKey: 'hubspot:entity:7', + }); + expect(error.retryAt).toBeInstanceOf(Date); + expect(error.message).toBe(`POST ${sanitizedUrl} 429`); + }); + + it('does not put the hint in the enumerable fields', () => { + const error = new RateLimitError({ hint }); + expect(Object.keys(error)).not.toContain('hint'); + expect(Object.keys(error)).not.toContain('hints'); + }); + }); + + describe('create()', () => { + it('reads the body once and keeps it out of the enumerable fields', async () => { + const response = throttledResponse(); + const error = await RateLimitError.create({ + resource: 'https://h.example', + response, + hint, + }); + expect(response.text).toHaveBeenCalledTimes(1); + expect(error).toBeInstanceOf(RateLimitError); + expect(error.body).toContain('access_token'); + expect(Object.keys(error)).not.toContain('body'); + }); + + it('does not read the stream when the body was read already', async () => { + const response = throttledResponse(); + const error = await RateLimitError.create({ + resource: 'https://h.example', + response, + responseBody: '{"policyName":"DAILY"}', + hint, + }); + expect(response.text).not.toHaveBeenCalled(); + expect(error.body).toBe('{"policyName":"DAILY"}'); + }); + }); + + describe('secrets', () => { + it('leaves no secret in the message, the stack, util.inspect or JSON', async () => { + const error = await RateLimitError.create({ + resource: secretUrl, + init: secretInit(), + response: throttledResponse(), + hint, + module: 'hubspot', + }); + expect(error.message).toContainNoSecretWindow(SECRETS); + expect(error.stack).toContainNoSecretWindow(SECRETS); + expect(util.inspect(error, { depth: 5 })).toContainNoSecretWindow( + SECRETS + ); + expect(JSON.stringify(error)).toContainNoSecretWindow(SECRETS); + }); + }); + + describe('logging', () => { + it('serializes with its own type and the status', async () => { + const error = await RateLimitError.create({ + resource: 'https://h.example', + response: throttledResponse(), + hint, + }); + expect(serializeError(error)).toMatchObject({ + type: 'RateLimitError', + status: 429, + }); + }); + }); +}); From a5769ac8e8c9a8924658b95a42cccc3e3dc030e2 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:10:58 -0300 Subject: [PATCH 04/33] feat(core): expose the invocation deadline runInvocationScope now records when the Lambda invocation ends, in its own AsyncLocalStorage. It is not the logs store: the telemetry bus forwards every non-log key of that store. remainingInvocationMs() reads it and is Infinity outside a Lambda (tests, frigg start, scripts). A nested scope can shorten the deadline and never extend it. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- packages/core/core/index.js | 6 ++ packages/core/core/invocation-deadline.js | 56 ++++++++++ .../core/core/invocation-deadline.test.js | 102 ++++++++++++++++++ packages/core/core/invocation-scope.js | 39 ++++--- packages/core/core/invocation-scope.test.js | 22 ++++ 5 files changed, 210 insertions(+), 15 deletions(-) create mode 100644 packages/core/core/invocation-deadline.js create mode 100644 packages/core/core/invocation-deadline.test.js diff --git a/packages/core/core/index.js b/packages/core/core/index.js index c69d9a2d4..f648c2e28 100644 --- a/packages/core/core/index.js +++ b/packages/core/core/index.js @@ -3,6 +3,10 @@ const { Worker } = require('./Worker'); const { loadInstalledModules } = require('./load-installed-modules'); const { createHandler } = require('./create-handler'); const { runInvocationScope } = require('./invocation-scope'); +const { + runWithInvocationDeadline, + remainingInvocationMs, +} = require('./invocation-deadline'); module.exports = { Delegate, @@ -10,4 +14,6 @@ module.exports = { loadInstalledModules, createHandler, runInvocationScope, + runWithInvocationDeadline, + remainingInvocationMs, }; diff --git a/packages/core/core/invocation-deadline.js b/packages/core/core/invocation-deadline.js new file mode 100644 index 000000000..41c94a932 --- /dev/null +++ b/packages/core/core/invocation-deadline.js @@ -0,0 +1,56 @@ +const { AsyncLocalStorage } = require('node:async_hooks'); + +const STORE_KEY = Symbol.for('@friggframework/core.invocation-deadline'); + +function storage() { + if (!globalThis[STORE_KEY]) { + globalThis[STORE_KEY] = new AsyncLocalStorage(); + } + return globalThis[STORE_KEY]; +} + +/** + * Runs fn with the time the invocation ends, as epoch milliseconds. A nested + * scope can shorten the deadline and never extend it. A deadline that is not + * a finite number runs fn with no limit. + */ +function runWithInvocationDeadline(deadlineAt, fn) { + if (typeof deadlineAt !== 'number' || !Number.isFinite(deadlineAt)) { + return fn(); + } + const outer = storage().getStore(); + const effective = outer + ? Math.min(outer.deadlineAt, deadlineAt) + : deadlineAt; + return storage().run(Object.freeze({ deadlineAt: effective }), fn); +} + +/** + * The end of a Lambda invocation, from its context. Undefined when the + * context cannot tell. + */ +function deadlineFromContext(context) { + try { + const remaining = context?.getRemainingTimeInMillis?.(); + return typeof remaining === 'number' && Number.isFinite(remaining) + ? Date.now() + remaining + : undefined; + } catch { + return undefined; + } +} + +/** + * Milliseconds left in the invocation. Infinity outside a deadline scope + * (tests, `frigg start`, scripts). + */ +function remainingInvocationMs(now = Date.now()) { + const store = storage().getStore(); + return store ? Math.max(0, store.deadlineAt - now) : Infinity; +} + +module.exports = { + deadlineFromContext, + remainingInvocationMs, + runWithInvocationDeadline, +}; diff --git a/packages/core/core/invocation-deadline.test.js b/packages/core/core/invocation-deadline.test.js new file mode 100644 index 000000000..6964aeb27 --- /dev/null +++ b/packages/core/core/invocation-deadline.test.js @@ -0,0 +1,102 @@ +const { + deadlineFromContext, + remainingInvocationMs, + runWithInvocationDeadline, +} = require('./invocation-deadline'); + +const tick = () => new Promise((resolve) => setImmediate(resolve)); + +describe('core/invocation-deadline', () => { + afterEach(() => jest.restoreAllMocks()); + + it('has no limit outside a deadline scope', () => { + expect(remainingInvocationMs()).toBe(Infinity); + }); + + it('reports the time left, deep inside async work', async () => { + const deadlineAt = 1_060_000; + await runWithInvocationDeadline(deadlineAt, async () => { + await tick(); + await Promise.resolve().then(() => { + expect(remainingInvocationMs(1_000_000)).toBe(60_000); + }); + }); + }); + + it('never reports a negative time', () => { + runWithInvocationDeadline(1_000, () => { + expect(remainingInvocationMs(5_000)).toBe(0); + }); + }); + + it('returns the value of fn and passes a rejection through', async () => { + await expect( + runWithInvocationDeadline(2e12, async () => 'ok') + ).resolves.toBe('ok'); + const boom = new Error('boom'); + await expect( + runWithInvocationDeadline(2e12, async () => { + throw boom; + }) + ).rejects.toBe(boom); + }); + + it.each([undefined, null, NaN, Infinity, 'soon'])( + 'runs fn with no limit for the deadline %p', + (deadlineAt) => { + const result = runWithInvocationDeadline(deadlineAt, () => + remainingInvocationMs() + ); + expect(result).toBe(Infinity); + } + ); + + it('lets a nested scope shorten the deadline, then restores the outer one', () => { + runWithInvocationDeadline(1_060_000, () => { + runWithInvocationDeadline(1_010_000, () => { + expect(remainingInvocationMs(1_000_000)).toBe(10_000); + }); + expect(remainingInvocationMs(1_000_000)).toBe(60_000); + }); + }); + + it('does not let a nested scope extend the deadline', () => { + runWithInvocationDeadline(1_010_000, () => { + runWithInvocationDeadline(1_060_000, () => { + expect(remainingInvocationMs(1_000_000)).toBe(10_000); + }); + }); + }); + + describe('deadlineFromContext', () => { + it('adds the time left to the current time', () => { + jest.spyOn(Date, 'now').mockReturnValue(1_000); + expect( + deadlineFromContext({ getRemainingTimeInMillis: () => 5_000 }) + ).toBe(6_000); + }); + + it.each([ + ['no context', undefined], + ['a context without the method', {}], + [ + 'a time that is not a number', + { getRemainingTimeInMillis: () => 'soon' }, + ], + [ + 'a time that is not finite', + { getRemainingTimeInMillis: () => Infinity }, + ], + [ + 'a method that throws', + { + getRemainingTimeInMillis: () => { + throw new Error('gone'); + }, + }, + ], + ])('returns undefined for %s', (_label, context) => { + expect(deadlineFromContext(context)).toBeUndefined(); + }); + }); +}); diff --git a/packages/core/core/invocation-scope.js b/packages/core/core/invocation-scope.js index 93e85f95e..1d0781fe6 100644 --- a/packages/core/core/invocation-scope.js +++ b/packages/core/core/invocation-scope.js @@ -1,6 +1,10 @@ const { runInContext, LOGGER_SCOPE_KEY } = require('../logs/context'); const { summarizeMessageBody } = require('../logs/summarize-event'); const { flushSinks, hasFlushableSinks } = require('../logs/logger-runtime'); +const { + deadlineFromContext, + runWithInvocationDeadline, +} = require('./invocation-deadline'); // Bounds the tail latency telemetry adds to every warm invocation. Kept low so // an unreachable OTLP endpoint (e.g. a VPC Lambda with no NAT/egress) costs at @@ -168,21 +172,26 @@ async function runInvocationScope(fields, fn, opts = {}) { flushTimeoutMs = DEFAULT_FLUSH_TIMEOUT_MS, context, } = opts; - return runInContext({ [LOGGER_SCOPE_KEY]: { ...(fields || {}) } }, async () => { - try { - return await fn(); - } finally { - // Every step is guarded, so a flush never changes the result. - await flushInvocation({ - telemetry, - usageRollup, - eventSummary, - shouldUseDatabase, - flushTimeoutMs, - context, - }); - } - }); + return runWithInvocationDeadline(deadlineFromContext(context), () => + runInContext( + { [LOGGER_SCOPE_KEY]: { ...(fields || {}) } }, + async () => { + try { + return await fn(); + } finally { + // Every step is guarded, so a flush never changes the result. + await flushInvocation({ + telemetry, + usageRollup, + eventSummary, + shouldUseDatabase, + flushTimeoutMs, + context, + }); + } + } + ) + ); } function toCount(value) { diff --git a/packages/core/core/invocation-scope.test.js b/packages/core/core/invocation-scope.test.js index 22edbcab1..0876b735f 100644 --- a/packages/core/core/invocation-scope.test.js +++ b/packages/core/core/invocation-scope.test.js @@ -10,6 +10,7 @@ const { const { getLogger, createMemorySink } = require('../logs'); const { setSinks } = require('../logs/logger-runtime'); const { getLoggerScope } = require('../logs/context'); +const { remainingInvocationMs } = require('./invocation-deadline'); const { mergeTelemetryContext } = require('../telemetry/telemetry-context'); const { NoOpTelemetry } = require('../telemetry/no-op-telemetry'); const { createUsageRollupSubscriber } = require('../telemetry/usage-rollup-subscriber'); @@ -59,6 +60,27 @@ describe('core/invocation-scope', () => { }); }); + it('makes the time left in the invocation visible deep inside fn', async () => { + const context = { getRemainingTimeInMillis: () => 60_000 }; + await runInvocationScope( + {}, + async () => { + await tick(); + await Promise.resolve().then(() => { + expect(remainingInvocationMs()).toBeLessThanOrEqual(60_000); + expect(remainingInvocationMs()).toBeGreaterThan(55_000); + }); + }, + { context } + ); + }); + + it('sets no deadline when there is no context', async () => { + await runInvocationScope({}, async () => { + expect(remainingInvocationMs()).toBe(Infinity); + }); + }); + it('drops undefined fields, so an absent context sets no requestId', async () => { await runInvocationScope({ requestId: undefined, handlerName: 'H' }, async () => { expect(getLoggerScope()).toEqual({ handlerName: 'H' }); From 1ee2708df7aded0f0bd4a3e0a18bfa4028e7989a Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:10:58 -0300 Subject: [PATCH 05/33] feat(core): honour Retry-After and static rateLimit in the Requester A 429 with a Retry-After header now waits as long as the provider says instead of the next step of the fixed ladder. A module can declare `static rateLimit` with a minimum wait, the parsers to read, and a classify() that recognises a limit signalled with another status, or with a body. A hinted wait that fits stays in process; a longer one throws RateLimitError with retryAt. A hinted wait is bounded by a cumulative cap (5 minutes, or the policy's maxInProcessWaitMs) and by the time left in the invocation. A 429 with no hint keeps today's ladder exactly: same calls, same delays, same FetchError, and it is not budgeted. Retry-After is read on 429 only; a 5xx keeps the 5xx ladder. Replaces the two skipped tests that called a live mock server. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- packages/core/index.js | 18 + .../requester/rate-limit/public-api.test.js | 26 + packages/core/modules/requester/requester.js | 180 +++++- .../core/modules/requester/requester.test.js | 574 +++++++++++++++++- 4 files changed, 768 insertions(+), 30 deletions(-) create mode 100644 packages/core/modules/requester/rate-limit/public-api.test.js diff --git a/packages/core/index.js b/packages/core/index.js index 30a91bbdd..7449ae984 100644 --- a/packages/core/index.js +++ b/packages/core/index.js @@ -12,6 +12,8 @@ const { loadInstalledModules, createHandler, runInvocationScope, + runWithInvocationDeadline, + remainingInvocationMs, } = require('./core/index'); const { prisma, @@ -55,6 +57,7 @@ const { BaseError, FetchError, HaltError, + RateLimitError, RequiredPropertyError, ParameterTypeError, } = require('./errors/index'); @@ -100,6 +103,12 @@ const { ModuleConstants, ModuleFactory, } = require('./modules/index'); +const { + classifyRateLimit, + parseRetryAfter, + parseResetHeaders, + parseIetfRateLimit, +} = require('./modules/requester/rate-limit'); const application = require('./application'); const utils = require('./utils'); @@ -120,6 +129,8 @@ module.exports = { loadInstalledModules, createHandler, runInvocationScope, + runWithInvocationDeadline, + remainingInvocationMs, // database prisma, @@ -142,9 +153,16 @@ module.exports = { BaseError, FetchError, HaltError, + RateLimitError, RequiredPropertyError, ParameterTypeError, + // rate limit + classifyRateLimit, + parseRetryAfter, + parseResetHeaders, + parseIetfRateLimit, + // integrations IntegrationBase, Options, diff --git a/packages/core/modules/requester/rate-limit/public-api.test.js b/packages/core/modules/requester/rate-limit/public-api.test.js new file mode 100644 index 000000000..749627afd --- /dev/null +++ b/packages/core/modules/requester/rate-limit/public-api.test.js @@ -0,0 +1,26 @@ +const core = require('../../../index'); +const { RateLimitError } = require('../../../errors'); +const rateLimit = require('./index'); +const deadline = require('../../../core/invocation-deadline'); + +describe('rate-limit public API', () => { + it('exports the error class from the package root', () => { + expect(core.RateLimitError).toBe(RateLimitError); + }); + + it.each([ + 'classifyRateLimit', + 'parseRetryAfter', + 'parseResetHeaders', + 'parseIetfRateLimit', + ])('exports %s from the package root', (name) => { + expect(core[name]).toBe(rateLimit[name]); + }); + + it.each(['runWithInvocationDeadline', 'remainingInvocationMs'])( + 'exports %s from the package root', + (name) => { + expect(core[name]).toBe(deadline[name]); + } + ); +}); diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index 514256c15..c8fa131c9 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -1,15 +1,26 @@ const fetch = require('node-fetch'); const { AsyncLocalStorage } = require('async_hooks'); const { Delegate } = require('../../core'); -const { FetchError } = require('../../errors'); +const { remainingInvocationMs } = require('../../core/invocation-deadline'); +const { FetchError, RateLimitError } = require('../../errors'); const { get } = require('../../assertions'); const { getTelemetry } = require('../../telemetry/telemetry-runtime'); const { getLogger } = require('../../logs'); const { redactUrl } = require('../../logs/redact'); const { toSanitizedSurrogate } = require('../../logs/serialize'); const { getLoggerScope } = require('../../logs/context'); +const { + classifyRateLimit, + computeScopeKey, + computeWaitMs, + headerValue, + inProcessBudgetMs, + readRateLimitPolicy, + resolveRateLimitHint, +} = require('./rate-limit'); const DEFAULT_REQUEST_TIMEOUT_MS = 60_000; +const JSON_CONTENT_TYPE = /^application\/(json|vnd\.api\+json|hal\+json)/; // A node-fetch error message holds the raw URL, and util.inspect prints the // cause chain, so the FetchError keeps only a sanitized copy. @@ -34,6 +45,7 @@ class Requester extends Delegate { constructor(params) { super(params); this.backOff = get(params, 'backOff', [1, 3, 10, 30, 60, 180]); + this._rateLimitPolicy = readRateLimitPolicy(this.constructor); this.isRefreshable = false; this.refreshCount = 0; this.authGraceRetryCount = 0; @@ -212,8 +224,11 @@ class Requester extends Delegate { * @param {number} attempt - 0-based count of retries already made for * this call. Indexes `this.backOff` for the next delay and is passed * back in on each recursive retry. + * @param {number} waitedMs - Milliseconds this call has slept already + * for a wait that a provider or a policy set. It counts against the + * in-process cap. */ - async _rawRequest(url, options, attempt = 0) { + async _rawRequest(url, options, attempt = 0, waitedMs = 0) { let encodedUrl = encodeURI(url); if (options.query) { let queryBuild = '?'; @@ -298,11 +313,79 @@ class Requester extends Delegate { const { status } = response; + const throttle = await this._detectThrottle( + response, + status, + attempt + ); + if (throttle?.throttled) { + const { hint } = throttle; + if (hint.source === 'backoff') { + if (attempt < this.backOff.length) { + clearRequestTimer(); + const delay = this.backOff[attempt] * 1000; + await new Promise((resolve) => + setTimeout(resolve, delay) + ); + return this._rawRequest( + url, + options, + attempt + 1, + waitedMs + ); + } + } else { + const budgetMs = inProcessBudgetMs({ + policy: this._rateLimitPolicy, + requestTimeoutMs: this.requestTimeoutMs, + remainingMs: remainingInvocationMs(), + waitedMs, + }); + const { waitMs, fits } = computeWaitMs({ + hint, + policy: this._rateLimitPolicy, + budgetMs, + }); + this._logRateLimited({ + status, + hint, + waitMs, + attempt, + waitedMs, + action: fits ? 'wait' : 'throw', + }); + if (fits) { + clearRequestTimer(); + await new Promise((resolve) => + setTimeout(resolve, waitMs) + ); + return this._rawRequest( + url, + options, + attempt + 1, + waitedMs + waitMs + ); + } + this._logRequestFailed(encodedUrl, options, status); + const rateLimitError = await RateLimitError.create({ + resource: encodedUrl, + init: options, + response, + responseBody: throttle.responseBody, + hint, + waitMs, + module: this._telemetryModuleLabel(), + scopeKey: computeScopeKey(this._rateLimitPolicy, this), + }); + throw this._maybeFlagTimeoutDuringBodyRead( + rateLimitError, + timeoutMs + ); + } + } + // If the status is retriable and there are back off requests left, retry the request - if ( - (status === 429 || status >= 500) && - attempt < this.backOff.length - ) { + if (status >= 500 && attempt < this.backOff.length) { clearRequestTimer(); const delay = this.backOff[attempt] * 1000; await new Promise((resolve) => setTimeout(resolve, delay)); @@ -392,6 +475,7 @@ class Requester extends Delegate { resource: encodedUrl, init: options, response, + responseBody: throttle?.responseBody, }); throw this._maybeFlagTimeoutDuringBodyRead( fetchError, @@ -421,6 +505,90 @@ class Requester extends Delegate { } } + /** + * Decides whether a response says a limit was hit. A 429 always does. A + * 4xx or 5xx other than 401 does only when the module's classify() names + * a limit. The body is read here, once, only for classify(); the text is + * handed on so the error need not read the stream again. + * + * @returns {Promise<{throttled: boolean, hint?: object, responseBody?: string}|null>} + */ + async _detectThrottle(response, status, attempt) { + const policy = this._rateLimitPolicy; + const canClassify = + typeof policy?.classify === 'function' && + status >= 400 && + status !== 401; + if (status !== 429 && !canClassify) return null; + + let responseBody; + let body; + if (canClassify) { + ({ text: responseBody, json: body } = + await this._readBodyForClassify(response)); + } + + const signal = { status, headers: response.headers, body }; + const options = { + attempt, + backOff: this.backOff, + now: Date.now(), + onClassifyError: (error) => this._logClassifyFailed(status, error), + }; + if (status === 429) { + return { + throttled: true, + hint: resolveRateLimitHint(policy, signal, options), + responseBody, + }; + } + const hint = classifyRateLimit(policy, signal, options); + return { throttled: Boolean(hint), hint, responseBody }; + } + + async _readBodyForClassify(response) { + if (response.bodyUsed || typeof response.text !== 'function') { + return {}; + } + const text = await response.text(); + if ( + !JSON_CONTENT_TYPE.test( + headerValue(response.headers, 'content-type') || '' + ) + ) { + return { text }; + } + try { + return { text, json: JSON.parse(text) }; + } catch { + return { text }; + } + } + + _logRateLimited({ status, hint, waitMs, attempt, waitedMs, action }) { + const logger = this.logger; + logger.info('Rate limited', { + eventName: `${logger.name}.rate_limited`, + statusCode: status, + waitMs, + retryAt: new Date(Date.now() + waitMs).toISOString(), + reason: hint.reason, + hintSource: hint.source, + attempt, + waitedMs, + action, + }); + } + + _logClassifyFailed(status, error) { + const logger = this.logger; + logger.warn('Rate limit classifier failed', { + eventName: `${logger.name}.rate_limit_classify_failed`, + statusCode: status, + error, + }); + } + _logRequestFailed(encodedUrl, options, status) { const logger = this.logger; if (!logger.isLevelEnabled('DEBUG')) return; diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index a2203fa1a..33fa3ee5e 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -1,4 +1,10 @@ +const { Headers } = require('node-fetch'); const { Requester } = require('./requester'); +const { FetchError, RateLimitError } = require('../../errors'); +const { createMemorySink } = require('../../logs'); +const { + runWithInvocationDeadline, +} = require('../../core/invocation-deadline'); /** * Requester is abstract: subclasses provide `addAuthHeaders`. For the @@ -39,30 +45,550 @@ function okFetch(body) { } describe('Requester', () => { - describe('429 and 5xx testing', () => { - let backOffArray = [1, 1, 1]; - let requester = new TestRequester({ backOff: backOffArray }); - let sum = backOffArray.reduce((a, b) => { - return a + b; - }, 0); - it.skip("should retry with 'exponential' back off due to 429", async () => { - let startTime = await Date.now(); - let res = await requester._get({ - url: 'https://70e18ff0-1967-4fb5-8f96-10477ab6bb9e.mock.pstmn.io//429', - }); - let endTime = await Date.now(); - let difference = endTime - startTime; - expect(difference).toBeGreaterThan(sum * 1000); - }); - - it.skip("should retry with 'exponential' back off due to 500", async () => { - let startTime = await Date.now(); - let res = await requester._get({ - url: 'https://70e18ff0-1967-4fb5-8f96-10477ab6bb9e.mock.pstmn.io//5xx', - }); - let endTime = await Date.now(); - let difference = endTime - startTime; - expect(difference).toBeGreaterThan(sum * 1000); + describe('rate-limit handling', () => { + const url = 'https://example.com/limited'; + const LADDER_MS = [1_000, 3_000, 10_000, 30_000, 60_000, 180_000]; + + function throttledFetch(responses) { + const queue = [...responses]; + const built = []; + const fetch = jest.fn(async () => { + const next = queue.shift(); + if (!next) throw new Error('unexpected fetch call'); + const contentType = next.contentType ?? 'application/json'; + const text = next.text ?? JSON.stringify(next.body ?? {}); + const response = { + status: next.status, + bodyUsed: false, + headers: + next.headersObject ?? + new Map( + Object.entries({ + 'Content-Type': contentType, + ...(next.headers ?? {}), + }) + ), + json: async () => next.body ?? {}, + text: jest.fn(async () => text), + }; + built.push(response); + return response; + }); + fetch.responses = built; + return fetch; + } + + const limited = (headers, extra = {}) => ({ + status: 429, + headers, + ...extra, + }); + const ok = { status: 200, body: { ok: true } }; + + let delays; + let sink; + + beforeEach(() => { + delays = []; + const realSetTimeout = global.setTimeout; + jest.spyOn(global, 'setTimeout').mockImplementation( + (fn, delay, ...args) => { + delays.push(delay); + return realSetTimeout(fn, 0, ...args); + } + ); + jest.spyOn(Math, 'random').mockReturnValue(0); + sink = createMemorySink(); + }); + + afterEach(() => jest.restoreAllMocks()); + + function makeRequester( + fetch, + params = {}, + RequesterClass = TestRequester + ) { + return new RequesterClass({ + fetch, + requestTimeoutMs: 0, + ...params, + }); + } + + const withPolicy = (rateLimit) => { + class PolicyRequester extends TestRequester { + static rateLimit = rateLimit; + } + return PolicyRequester; + }; + + describe('a module that declares nothing', () => { + it('retries a bare 429 on the fixed ladder, then throws a plain FetchError', async () => { + const fetch = throttledFetch(Array(7).fill(limited())); + const requester = makeRequester(fetch); + + const error = await requester._get({ url }).catch((e) => e); + + expect(error).toBeInstanceOf(FetchError); + expect(error).not.toBeInstanceOf(RateLimitError); + expect(error.isRateLimited).toBeUndefined(); + expect(error.statusCode).toBe(429); + expect(delays).toEqual(LADDER_MS); + expect(fetch).toHaveBeenCalledTimes(7); + }); + + it('keeps the ladder inside a short invocation, because only hinted waits are budgeted', async () => { + const fetch = throttledFetch(Array(7).fill(limited())); + const requester = makeRequester(fetch, { + requestTimeoutMs: 45_000, + }); + + const error = await runWithInvocationDeadline( + Date.now() + 29_000, + () => requester._get({ url }).catch((e) => e) + ); + + expect(error).toBeInstanceOf(FetchError); + expect(delays.filter((delay) => delay !== 45_000)).toEqual( + LADDER_MS + ); + expect(fetch).toHaveBeenCalledTimes(7); + }); + + it('leaves the 5xx ladder alone', async () => { + const fetch = throttledFetch([ + { status: 500 }, + { status: 500 }, + ok, + ]); + const result = await makeRequester(fetch)._get({ url }); + + expect(result).toEqual({ ok: true }); + expect(delays).toEqual([1_000, 3_000]); + }); + + it('keeps the ladder for a 503 that carries Retry-After', async () => { + const fetch = throttledFetch([ + { status: 503, headers: { 'Retry-After': '30' } }, + ok, + ]); + await makeRequester(fetch)._get({ url }); + + expect(delays).toEqual([1_000]); + }); + + it('writes no rate-limit record for the ladder', async () => { + const fetch = throttledFetch([limited(), ok]); + await makeRequester(fetch)._get({ url }); + + expect( + sink.records.filter((r) => + r.eventName?.endsWith('.rate_limited') + ) + ).toEqual([]); + }); + }); + + describe('Retry-After', () => { + it('waits as long as the header says, not one ladder step', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '5' }), + ok, + ]); + const result = await makeRequester(fetch)._get({ url }); + + expect(result).toEqual({ ok: true }); + expect(delays).toEqual([5_000]); + expect(fetch).toHaveBeenCalledTimes(2); + }); + + it('does not raise a short Retry-After to the ladder step of a later attempt', async () => { + const fetch = throttledFetch([ + limited(), + limited(), + limited({ 'Retry-After': '2' }), + ok, + ]); + await makeRequester(fetch)._get({ url }); + + expect(delays).toEqual([1_000, 3_000, 2_000]); + }); + + it('reads the header from a real Headers object', async () => { + const fetch = throttledFetch([ + limited(undefined, { + headersObject: new Headers({ 'retry-after': '3' }), + }), + ok, + ]); + await makeRequester(fetch)._get({ url }); + + expect(delays).toEqual([3_000]); + }); + + it('adds jitter that never passes the wait by more than a tenth', async () => { + Math.random.mockReturnValue(0.999999); + const fetch = throttledFetch([ + limited({ 'Retry-After': '60' }), + ok, + ]); + await makeRequester(fetch)._get({ url }); + + expect(delays).toEqual([60_999]); + }); + + it('sleeps on for hinted 429s beyond the length of the ladder', async () => { + const fetch = throttledFetch([ + ...Array(8).fill(limited({ 'Retry-After': '1' })), + ok, + ]); + const result = await makeRequester(fetch)._get({ url }); + + expect(result).toEqual({ ok: true }); + expect(delays).toEqual(Array(8).fill(1_000)); + expect(fetch).toHaveBeenCalledTimes(9); + }); + + it('throws a RateLimitError with retryAt when the wait is over the cap', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '400' }), + ok, + ]); + const before = Date.now(); + + const error = await makeRequester(fetch) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(error).toBeInstanceOf(FetchError); + expect(error).toMatchObject({ + isRateLimited: true, + statusCode: 429, + source: 'header', + waitMs: 400_000, + reason: 'unknown', + }); + expect(error.retryAt.getTime()).toBeGreaterThanOrEqual( + before + 400_000 + ); + expect(fetch).toHaveBeenCalledTimes(1); + expect(delays).toEqual([]); + }); + + it('stops sleeping once the waits add up to the cap', async () => { + const fetch = throttledFetch([ + ...Array(4).fill(limited({ 'Retry-After': '100' })), + ok, + ]); + + const error = await makeRequester(fetch) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(delays).toEqual([100_000, 100_000, 100_000]); + expect(fetch).toHaveBeenCalledTimes(4); + }); + + it('throws at once when the wait does not fit the time left in the invocation', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '4' }), + ok, + ]); + const requester = makeRequester(fetch, { + requestTimeoutMs: 5_000, + }); + + const error = await runWithInvocationDeadline( + Date.now() + 8_000, + () => requester._get({ url }).catch((e) => e) + ); + + expect(error).toBeInstanceOf(RateLimitError); + expect(fetch).toHaveBeenCalledTimes(1); + }); + + it('throws at once when the policy allows no in-process wait', async () => { + const Api = withPolicy({ maxInProcessWaitMs: 0 }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '2' }), + ok, + ]); + + const error = await makeRequester(fetch, {}, Api) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(fetch).toHaveBeenCalledTimes(1); + }); + + it('reads the body of the RateLimitError once', async () => { + const fetch = throttledFetch([ + limited( + { 'Retry-After': '400' }, + { body: { message: 'slow down' } } + ), + ]); + const error = await makeRequester(fetch) + ._get({ url }) + .catch((e) => e); + + expect(fetch.responses[0].text).toHaveBeenCalledTimes(1); + expect(error.body).toContain('slow down'); + }); + }); + + describe('static rateLimit', () => { + it('makes minRetryAfterMs the wait of a bare 429', async () => { + const Api = withPolicy({ minRetryAfterMs: 60_000 }); + const fetch = throttledFetch([limited(), ok]); + await makeRequester(fetch, {}, Api)._get({ url }); + + expect(delays).toEqual([60_000]); + }); + + it('makes minRetryAfterMs the floor of a shorter Retry-After', async () => { + const Api = withPolicy({ minRetryAfterMs: 60_000 }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '5' }), + ok, + ]); + await makeRequester(fetch, {}, Api)._get({ url }); + + expect(delays).toEqual([60_000]); + }); + + it('reads only the parsers it lists, in that order', async () => { + const Api = withPolicy({ + parsers: ['resetHeaders', 'retryAfter'], + }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '2', 'X-RateLimit-Reset': '7' }), + ok, + ]); + await makeRequester(fetch, {}, Api)._get({ url }); + + expect(delays).toEqual([7_000]); + }); + + it('carries the module name and the scope key on the error', async () => { + const Api = withPolicy({ scope: 'entity' }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '400' }), + ]); + const requester = makeRequester( + fetch, + { delegate: { name: 'hubspot', entity: { id: 7 } } }, + Api + ); + + const error = await requester._get({ url }).catch((e) => e); + + expect(error).toMatchObject({ + module: 'hubspot', + scopeKey: 'hubspot:entity:7', + }); + }); + + it('fails at construction for a parser that does not exist', () => { + const Api = withPolicy({ parsers: ['magic'] }); + expect(() => new Api({})).toThrow(TypeError); + }); + }); + + describe('classify', () => { + it('treats a 403 as throttled and retries when the wait fits', async () => { + const classify = jest.fn(({ status, body }) => + status === 403 && body?.code === 'LIMIT' + ? { reason: 'daily', waitMs: 2_000 } + : null + ); + const Api = withPolicy({ classify }); + const fetch = throttledFetch([ + { status: 403, body: { code: 'LIMIT' } }, + ok, + ]); + + const result = await makeRequester(fetch, {}, Api)._get({ + url, + }); + + expect(result).toEqual({ ok: true }); + expect(delays).toEqual([2_000]); + expect(classify).toHaveBeenCalledWith( + expect.objectContaining({ + status: 403, + body: { code: 'LIMIT' }, + }) + ); + }); + + it('throws a RateLimitError with the 403 status when the wait does not fit', async () => { + const Api = withPolicy({ + classify: () => ({ reason: 'daily', waitMs: 400_000 }), + }); + const fetch = throttledFetch([ + { status: 403, body: { code: 'LIMIT' } }, + ]); + + const error = await makeRequester(fetch, {}, Api) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(error).toMatchObject({ + statusCode: 403, + reason: 'daily', + source: 'body', + }); + expect(fetch.responses[0].text).toHaveBeenCalledTimes(1); + expect(error.body).toContain('LIMIT'); + }); + + it('leaves a 403 that classify does not recognise as a plain FetchError', async () => { + const Api = withPolicy({ classify: () => null }); + const fetch = throttledFetch([ + { status: 403, headers: { 'Retry-After': '2' } }, + ]); + + const error = await makeRequester(fetch, {}, Api) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(FetchError); + expect(error.isRateLimited).toBeUndefined(); + expect(fetch).toHaveBeenCalledTimes(1); + expect(delays).toEqual([]); + }); + + it('takes the time of a reason-only classify from Retry-After', async () => { + const Api = withPolicy({ + classify: () => ({ reason: 'burst' }), + }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '400' }), + ]); + + const error = await makeRequester(fetch, {}, Api) + ._get({ url }) + .catch((e) => e); + + expect(error).toMatchObject({ + reason: 'burst', + source: 'header', + }); + }); + + it('hands classify an undefined body when the response is not JSON', async () => { + const classify = jest.fn(() => ({ + reason: 'burst', + waitMs: 1_000, + })); + const Api = withPolicy({ classify }); + const fetch = throttledFetch([ + limited(undefined, { + contentType: 'text/plain', + text: 'slow down', + }), + ok, + ]); + + await makeRequester(fetch, {}, Api)._get({ url }); + + expect(classify.mock.calls[0][0].body).toBeUndefined(); + }); + + it('does not call classify for a success or for a 401', async () => { + const classify = jest.fn(() => null); + const Api = withPolicy({ classify }); + const fetch = throttledFetch([{ status: 401 }, ok]); + const requester = makeRequester(fetch, { backOff: [0] }, Api); + requester.notify = jest.fn(); + + await requester._get({ url }); + + expect(classify).not.toHaveBeenCalled(); + }); + + it('writes one WARN record and goes on with the parsers when classify throws', async () => { + const Api = withPolicy({ + classify: () => { + throw new Error('bad classifier'); + }, + }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '2' }), + ok, + ]); + + await makeRequester(fetch, {}, Api)._get({ url }); + + expect(delays).toEqual([2_000]); + expect( + sink.records.filter( + (r) => + r.eventName === + 'module.PolicyRequester.rate_limit_classify_failed' + ) + ).toEqual([ + expect.objectContaining({ + level: 'WARN', + statusCode: 429, + error: expect.objectContaining({ + message: 'bad classifier', + }), + }), + ]); + }); + }); + + describe('logging', () => { + it('writes one INFO record for a wait that a header set', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '5' }), + ok, + ]); + await makeRequester(fetch)._get({ url }); + + expect( + sink.records.filter( + (r) => + r.eventName === 'module.TestRequester.rate_limited' + ) + ).toEqual([ + expect.objectContaining({ + level: 'INFO', + logger: 'module.TestRequester', + statusCode: 429, + waitMs: 5_000, + reason: 'unknown', + hintSource: 'header', + attempt: 0, + waitedMs: 0, + action: 'wait', + retryAt: expect.any(String), + }), + ]); + }); + + it('says throw when the wait does not fit', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '400' }), + ]); + await makeRequester(fetch) + ._get({ url }) + .catch(() => {}); + + const [record] = sink.records.filter( + (r) => r.eventName === 'module.TestRequester.rate_limited' + ); + expect(record).toMatchObject({ + action: 'throw', + waitMs: 400_000, + }); + }); }); }); From 1cb07e64eb422c51ddd4535a1dc4a9380143eef4 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:10:58 -0300 Subject: [PATCH 06/33] fix(core): do not halt rate-limited 4xx in the queue worker The worker marks every 4xx except 408 and 429 as a halt and discards the message. A provider that signals a limit with 403 lost the message. An error the Requester flagged isRateLimited is now exempt and goes back to SQS. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- packages/core/handlers/backend-utils.js | 3 +- .../integration-defined-workers.test.js | 37 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/packages/core/handlers/backend-utils.js b/packages/core/handlers/backend-utils.js index 1ee6fee44..6cf32616c 100644 --- a/packages/core/handlers/backend-utils.js +++ b/packages/core/handlers/backend-utils.js @@ -285,7 +285,8 @@ const createQueueWorker = (integrationClass) => { status >= 400 && status < 500 && status !== 408 && - status !== 429 + status !== 429 && + !error.isRateLimited ) { error.isHaltError = true; console.warn( diff --git a/packages/core/handlers/workers/integration-defined-workers.test.js b/packages/core/handlers/workers/integration-defined-workers.test.js index 940fa2879..6a8eedd64 100644 --- a/packages/core/handlers/workers/integration-defined-workers.test.js +++ b/packages/core/handlers/workers/integration-defined-workers.test.js @@ -7,6 +7,7 @@ jest.mock('../../database/config', () => ({ const { createQueueWorker } = require('../backend-utils'); const { IntegrationBase } = require('../../integrations/integration-base'); const { IntegrationEventDispatcher } = require('../integration-event-dispatcher'); +const { RateLimitError } = require('../../errors'); class TestWebhookIntegration extends IntegrationBase { static Definition = { @@ -261,6 +262,42 @@ describe('Webhook Queue Worker', () => { expect(result.batchItemFailures).toEqual([{ itemIdentifier: 'msg-1' }]); }); + it('should NOT mark a 403 with isRateLimited as isHaltError (a provider limit outside 429)', async () => { + const error = new RateLimitError({ + resource: 'https://api.example.com/v1/items', + response: { status: 403, bodyUsed: true }, + hint: { waitMs: 60_000, reason: 'daily', source: 'body' }, + }); + + const FailingIntegration = class extends TestWebhookIntegration { + async onWebhook() { + throw error; + } + }; + + const QueueWorker = createQueueWorker(FailingIntegration); + const worker = new QueueWorker(); + + const sqsEvent = { + Records: [ + { + messageId: 'msg-1', + body: JSON.stringify({ + event: 'ON_WEBHOOK', + data: { body: {} }, + }), + }, + ], + }; + + const result = await worker.run(sqsEvent, {}); + expect(result.batchItemFailures).toEqual([ + { itemIdentifier: 'msg-1' }, + ]); + expect(error.statusCode).toBe(403); + expect(error.isHaltError).toBeUndefined(); + }); + it('should NOT mark 500 as isHaltError (server may recover)', async () => { const error = new Error('Internal Server Error'); error.statusCode = 500; From 14ae933e288c8a952a899a4a7f2c9a2ba1cf85a0 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:15:51 -0300 Subject: [PATCH 07/33] feat(core): type the rate-limit API Declares RateLimitError, RateLimitHint, RateLimitPolicy, RateLimitSignal and the static `rateLimit` and `requestTimeoutMs` of Requester, plus classifyRateLimit, the header parsers and the invocation deadline functions. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- packages/core/types/core/index.d.ts | 42 +++++++++++++++ packages/core/types/errors/index.d.ts | 56 ++++++++++++++++++++ packages/core/types/module-plugin/index.d.ts | 52 ++++++++++++++++++ 3 files changed, 150 insertions(+) diff --git a/packages/core/types/core/index.d.ts b/packages/core/types/core/index.d.ts index 2ac09c449..3627558b7 100644 --- a/packages/core/types/core/index.d.ts +++ b/packages/core/types/core/index.d.ts @@ -1,5 +1,10 @@ declare module "@friggframework/core" { import type { SendMessageCommandInput } from "@aws-sdk/client-sqs"; + import type { RateLimitHint } from "@friggframework/errors"; + import type { + RateLimitPolicy, + RateLimitSignal, + } from "@friggframework/module-plugin"; export class Delegate implements IFriggDelegate { delegate: any; @@ -53,6 +58,43 @@ declare module "@friggframework/core" { export function loadInstalledModules(): any[]; + /** + * Runs `fn` with the time the invocation ends, as epoch milliseconds. A + * nested scope can shorten the deadline and never extend it. + */ + export function runWithInvocationDeadline( + deadlineAt: number | undefined, + fn: () => T + ): T; + + /** Milliseconds left in the invocation. `Infinity` outside a Lambda. */ + export function remainingInvocationMs(now?: number): number; + + /** + * Finds the hint of a throttled response: the module's classify(), then the + * header parsers, then the static policy. Null when there is none. + */ + export function classifyRateLimit( + policy: RateLimitPolicy | undefined, + signal: RateLimitSignal, + options?: { now?: number; onClassifyError?: (error: Error) => void } + ): RateLimitHint | null; + + export function parseRetryAfter( + value: string | null | undefined, + options?: { now?: number } + ): RateLimitHint | null; + + export function parseResetHeaders( + headers: object | undefined, + options?: { now?: number } + ): RateLimitHint | null; + + export function parseIetfRateLimit( + headers: object | undefined, + options?: { now?: number } + ): RateLimitHint | null; + type GetQueueURLParams = { QueueName: string; QueueOwnerAWSAccountId?: string; diff --git a/packages/core/types/errors/index.d.ts b/packages/core/types/errors/index.d.ts index 5a321723d..f63ed6563 100644 --- a/packages/core/types/errors/index.d.ts +++ b/packages/core/types/errors/index.d.ts @@ -20,6 +20,62 @@ declare module "@friggframework/errors" { static create(options?: CreateFetchErrorParams): Promise; } + export type RateLimitReason = + | "burst" + | "daily" + | "monthly" + | "concurrency" + | "unknown"; + + export type RateLimitSource = "header" | "body" | "static" | "backoff"; + + /** What Frigg knows about when calls are accepted again. */ + export type RateLimitHint = { + /** When calls are accepted again. */ + retryAt: Date; + /** Milliseconds from the time the hint was read until `retryAt`. */ + waitMs: number; + reason: RateLimitReason; + /** A provider policy name, when the response names one. */ + policy?: string; + /** Calls left in the window, when the response says. */ + remaining?: number; + source: RateLimitSource; + }; + + /** + * A FetchError for a response that said a limit was hit, and for which the + * response or the module's policy says when to call again. + */ + export class RateLimitError extends FetchError { + constructor(options?: RateLimitErrorConstructor); + + isRateLimited: true; + /** When calls are accepted again. */ + retryAt: Date; + waitMs: number; + reason: RateLimitReason; + policy?: string; + source: RateLimitSource | "unknown"; + module?: string; + scopeKey?: string; + + static create( + options?: CreateRateLimitErrorParams + ): Promise; + } + + type RateLimitErrorConstructor = FetchErrorConstructor & { + hint?: Partial; + /** Wait from `now`. When absent, `hint.retryAt` sets the time. */ + waitMs?: number; + module?: string; + scopeKey?: string; + now?: number; + }; + + type CreateRateLimitErrorParams = RateLimitErrorConstructor; + type FetchErrorResponse = { headers?: object; status?: number; diff --git a/packages/core/types/module-plugin/index.d.ts b/packages/core/types/module-plugin/index.d.ts index 8eaf41f75..5224ac036 100644 --- a/packages/core/types/module-plugin/index.d.ts +++ b/packages/core/types/module-plugin/index.d.ts @@ -1,5 +1,6 @@ declare module "@friggframework/module-plugin" { import { Delegate, IFriggDelegate } from "@friggframework/core"; + import type { RateLimitHint } from "@friggframework/errors"; export interface Credential { id?: string; @@ -22,8 +23,59 @@ declare module "@friggframework/module-plugin" { export type MappedEntity = Entity & { id: string; type: any }; + export type RateLimitScope = + | "credential" + | "entity" + | "app" + | ((requester: Requester) => string | number | null | undefined); + + export type RateLimitSignal = { + status?: number; + headers?: { get(name: string): string | null } | object; + body?: unknown; + }; + + export type RateLimitWindow = { + name: string; + limit?: number; + perMs?: number; + rollingMs?: number; + resets?: { at: string; tz: string }; + }; + + export type RateLimitPolicy = { + /** The key a limit counts against. Default `entity`. */ + scope?: RateLimitScope; + windows?: RateLimitWindow[]; + maxConcurrency?: number; + /** A wait never shorter than this, for every throttled response. */ + minRetryAfterMs?: number; + /** The most a request sleeps in process, in total. Default 300000. */ + maxInProcessWaitMs?: number; + /** Header parsers, in order. Default all three. */ + parsers?: Array<"retryAfter" | "resetHeaders" | "ietf">; + /** + * Recognises a limit that is not a plain 429, or names its reason. Returns + * null when the response is not a limit. A hint with no time takes its + * time from the parsers or the policy. + */ + classify?( + signal: RateLimitSignal + ): Partial> & { + source?: "header" | "body" | "static"; + } | null | undefined; + userHints?: Record< + string, + { links?: Array<{ label: string; url: string }> } + >; + }; + export class Requester implements IFriggRequester { + static rateLimit?: RateLimitPolicy; + static requestTimeoutMs?: number; + DLGT_INVALID_AUTH: string; + requestTimeoutMs: number; backOff: number[]; fetch: any; isRefreshable: boolean; From 10f1676a00a793a52a2f0a6edefb4d08ec677a15 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:15:51 -0300 Subject: [PATCH 08/33] docs(core): document static rateLimit and RateLimitError Adds the rate-limit section to the API module reference (the keys, what classify returns, how the Requester waits, and the pattern for a client that is not the Requester), points the integration patterns guide at it, and notes the new record names in the logging guide. The core CLAUDE.md files now name RateLimitError and the halt exemption. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- docs/guides/INTEGRATION-PATTERNS.md | 10 +- docs/guides/LOGGING.md | 7 ++ .../api-module-definition-and-functions.md | 100 ++++++++++++++++++ packages/core/CLAUDE.md | 46 +++++++- packages/core/core/CLAUDE.md | 4 +- 5 files changed, 164 insertions(+), 3 deletions(-) diff --git a/docs/guides/INTEGRATION-PATTERNS.md b/docs/guides/INTEGRATION-PATTERNS.md index eddba15fa..aab7fe206 100644 --- a/docs/guides/INTEGRATION-PATTERNS.md +++ b/docs/guides/INTEGRATION-PATTERNS.md @@ -796,7 +796,15 @@ module.exports = { MyIntegration }; ### Rate Limiting -Always respect API rate limits: +Put what you know about a provider's limits in its API module: declare +`static rateLimit` (see the +[API module reference](../reference/api-module-definition-and-functions.md#rate-limits-static-ratelimit)). +The Requester then waits as the provider says. When the wait is too long to +sleep, it throws `RateLimitError` with `retryAt`. Let it reach the queue +handler's caller: the queue worker does not halt it and SQS redelivers the +message. Do not catch it to write a retry of your own. + +For pacing in your own loop, respect API rate limits: ```javascript async processBatchHandler({ processId, entityIds, entityType }) { diff --git a/docs/guides/LOGGING.md b/docs/guides/LOGGING.md index 53bfceedd..afaf24eb7 100644 --- a/docs/guides/LOGGING.md +++ b/docs/guides/LOGGING.md @@ -162,6 +162,13 @@ the app deploys defines the schema. layers throw with `cause` and do not log. `createHandler`, the express error middleware, `Worker.run` and the DLQ processor are the boundaries. +The `Requester` writes one `INFO` record, `module..rate_limited`, for +each wait that a provider or a module policy set, and for each rate-limit error +it throws. It carries `statusCode`, `waitMs`, `retryAt`, `reason`, `hintSource`, +`attempt`, `waitedMs` and `action` (`wait` or `throw`), because the serialized +error keeps no `retryAt`. The plain backoff ladder writes none. A `classify()` +that throws writes one `WARN`, `module..rate_limit_classify_failed`. + `FRIGG_LOG_LEVEL` sets the minimum level. The logger drops a record below it before it reads any field. diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index e209be457..0c049cffd 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -219,6 +219,106 @@ encryption: { credentialFields: ['signing_key', 'webhook_secret'] }, apiPropertiesToPersist: { credential: ['signing_key', 'webhook_secret'] } ``` +## Rate Limits (`static rateLimit`) + +An API module can tell the Requester how its provider limits calls, and how to +read a throttled response. Declare a static `rateLimit` on the API class. Every +key is optional. A module that declares nothing keeps the fixed backoff ladder +for a 429 (1, 3, 10, 30, 60 and 180 s), and reads `Retry-After` when the +response has one. + +| Key | Meaning | +|---|---| +| `scope` | The key a limit counts against: `'entity'` (default), `'credential'`, `'app'`, or a function `(requester) => key`. The Requester puts it on `RateLimitError.scopeKey`. | +| `minRetryAfterMs` | A wait never shorter than this. Use it when the docs say "wait 60 seconds" and the response says nothing. | +| `maxInProcessWaitMs` | The most one request sleeps in process, in total. Default `300000` (5 minutes). `0` means never sleep: every hinted wait throws `RateLimitError`. | +| `parsers` | The header parsers to read, in order: `retryAfter`, `resetHeaders`, `ietf`. Default: all three. | +| `classify` | Recognises a limit that is not a plain 429, or names its reason. See below. | +| `windows`, `maxConcurrency` | The provider's limits, as documentation. A window named like the `reason` (`{ name: 'burst', perMs: 10000 }`) sets the wait when no header does. Pacing uses them later. | +| `userHints` | Links per `reason` that a UI can show, for example `{ daily: { links: [{ label, url }] } }`. | + +### What `classify` returns + +`classify({ status, headers, body })` runs for a 429, and for any other 4xx or +5xx except 401. `body` is the parsed JSON body, or `undefined` when the +response is not JSON. It returns `null` when the response is not a limit, or a +hint: + +| Field | Meaning | +|---|---| +| `reason` | `'burst'`, `'daily'`, `'monthly'`, `'concurrency'` or `'unknown'` | +| `waitMs` or `retryAt` | When to call again. Leave both out to take the time from the parsers, then from the policy. | +| `policy` | The provider's own name for the limit, for logs and the UI | +| `remaining` | Calls left in the window, when the response says | +| `source` | `'header'`, `'body'` (default) or `'static'` | + +A status other than 429 is a limit only when `classify` returns a hint for it. +A `classify` that throws is logged (`rate_limit_classify_failed`) and skipped. + +```javascript +class Api extends OAuth2Requester { + static rateLimit = { + scope: 'entity', + windows: [{ name: 'burst', limit: 100, perMs: 10_000 }], + parsers: ['retryAfter', 'resetHeaders'], + classify({ status, body }) { + if (status !== 429) return null; + if (body?.policyName === 'DAILY') return { reason: 'daily', waitMs: 3_600_000 }; + return { reason: 'burst', policy: body?.policyName }; + }, + userHints: { + daily: { links: [{ label: 'API usage limits', url: 'https://example.com/limits' }] }, + }, + }; +} +``` + +```javascript +class Api extends ApiKeyRequester { + static rateLimit = { minRetryAfterMs: 60_000 }; +} +``` + +### How the Requester waits + +1. It looks for a hint in this order: `classify()`, the parsers, the policy + (`minRetryAfterMs`, or the `perMs` of the window named by the reason). +2. With no hint, a 429 keeps the fixed ladder: the same calls, the same delays, + then a plain `FetchError`. A 5xx keeps its ladder too. +3. With a hint, the wait is the largest of the hint, `minRetryAfterMs` and 1 s, + plus at most 10 % jitter. The Requester sleeps when the wait fits the time + this request may still sleep: `maxInProcessWaitMs` in total, and the time left + in the Lambda invocation less one request timeout. +4. A wait that does not fit throws `RateLimitError`. It extends `FetchError`, so + `statusCode` stays, and adds `isRateLimited`, `retryAt`, `waitMs`, `reason`, + `policy`, `source`, `module` and `scopeKey`. + +`Retry-After` is read on a 429 only. The queue worker does not halt a +`RateLimitError`, even when its status is 403: the message goes back to SQS. + +### A client that is not the Requester + +A module that drives another client (for example jsforce) calls +`classifyRateLimit` around its own calls and throws the error itself: + +```javascript +const { classifyRateLimit, RateLimitError } = require('@friggframework/core'); + +async withLimits(call) { + try { + return await call(); + } catch (err) { + const hint = classifyRateLimit(Api.rateLimit, { + status: err.statusCode, + headers: {}, + body: { errorCode: err.errorCode }, + }); + if (!hint) throw err; + throw new RateLimitError({ hint, module: this.name, cause: err }); + } +} +``` + ## Complete OAuth2 Example ```javascript diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 91b628746..440ffcb84 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -369,6 +369,48 @@ class MyIntegration extends IntegrationBase { - `ApiKeyRequester` - API key authentication - `BasicAuthRequester` - Basic authentication +**Rate limits** (ADR-049): an API module declares `static rateLimit` on its +`Requester` subclass. For a throttled response the Requester looks for a hint +in this order: + +| Step | Source | +|---|---| +| 1 | `classify({ status, headers, body })` of the module. It recognises a limit that is not a plain 429 (a 403 limit code, a `policyName` in the body) and may name only a `reason` | +| 2 | The header parsers in `parsers` order. Default: `retryAfter`, `resetHeaders`, `ietf` | +| 3 | The static policy: `minRetryAfterMs`, or the `perMs` of the window named by the `reason` | +| 4 | The fixed `backOff` ladder (1, 3, 10, 30, 60, 180 s) | + +- A 429 with no hint from steps 1 to 3 keeps today's ladder: the same calls, + the same delays, then a plain `FetchError`. Nothing budgets it. +- With a hint, the wait is `max(hint, minRetryAfterMs, 1 s)` plus at most 10 % + jitter. The total sleep of one request is capped at `maxInProcessWaitMs` + (default 5 minutes) and at the time left in the invocation less one request + timeout (`remainingInvocationMs()`). +- A wait that does not fit throws `RateLimitError` (`isRateLimited`, `retryAt`, + `waitMs`, `reason`, `policy`, `source`, `module`, `scopeKey`). The queue + worker does not halt it, whatever its status. +- `Retry-After` is read on a 429 only. Another status counts only when + `classify` recognises it. A 5xx keeps the 5xx ladder. +- `maxInProcessWaitMs: 0` turns off sleeping: every hinted wait throws. +- A module that does not use the Requester (for example one on jsforce) calls + `classifyRateLimit(policy, { status, headers, body })` around its own client + and throws `RateLimitError` itself. + +```javascript +class Api extends OAuth2Requester { + static rateLimit = { + scope: 'entity', + minRetryAfterMs: 1_000, + classify({ status, body }) { + if (status === 403 && body?.code === 'DAILY_LIMIT') { + return { reason: 'daily', waitMs: 60 * 60_000 }; + } + return null; + }, + }; +} +``` + **Module Factory**: - `ModuleFactory` - Creates and configures API module instances - Handles credential injection @@ -477,7 +519,8 @@ handlers (`{ req, res, next }`) and `this.on` events do not. `isLastAttempt` is `false` when either count is unknown: a local or non-SQS invocation, or a queue whose redrive policy the stack does not own (`ownership.queue: 'external'`). The value is information only: core still -rethrows retryable errors and discards halt errors (4xx except 408/429). +rethrows retryable errors and discards halt errors (4xx except 408, 429 and +errors with `isRateLimited`). Use it to end a run or count lost work on the final try: when a retryable error (429, 5xx, network) is about to be rethrown and `isLastAttempt` is @@ -507,6 +550,7 @@ it as `FRIGG_QUEUE_MAX_RECEIVE_COUNT` on the queue worker function. **Error Types**: - `BaseError` - Base error class - `FetchError` - HTTP request failures +- `RateLimitError` - A `FetchError` for a limit the provider or the module's policy says when to retry (`retryAt`) - `HaltError` - Stop processing without retry - `RequiredPropertyError` - Missing required parameters - `ParameterTypeError` - Invalid parameter type diff --git a/packages/core/core/CLAUDE.md b/packages/core/core/CLAUDE.md index 520a70d48..0c9d79a44 100644 --- a/packages/core/core/CLAUDE.md +++ b/packages/core/core/CLAUDE.md @@ -130,7 +130,7 @@ class MyIntegration extends Delegate { ### Lambda Handler Lifecycle 1. **Pre-Execution Setup**: ```javascript - runInvocationScope({ requestId, handlerName, method, route, invocation }, …); // Logger scope for the invocation + runInvocationScope({ requestId, handlerName, method, route, invocation }, …); // Logger scope + invocation deadline (remainingInvocationMs()) log.info('Handler invoked', { eventName: 'frigg.handler.invoked' }); await secretsToEnv(); // Secrets Manager injection await parametersToEnv(); // SSM Parameter Store fetch (only when SSM_PARAMETER_PREFIX + FRIGG_SSM_OFFLOADED_KEYS are set) @@ -226,6 +226,8 @@ const handler = createHandler({ - Logs error but returns success - Prevents infinite retries for known issues - Used for graceful degradation scenarios + - The queue worker sets it on a 4xx except 408, 429 and errors with + `isRateLimited` (a provider limit signalled with 403 clears with time) ### Logging Strategy ```javascript From 7302a64c61c7eb20e0b42a972c0406f75b1f347b Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 28 Sep 2026 20:53:53 -0300 Subject: [PATCH 09/33] fix(core): address the review of the rate-limit Requester - A response that classify() names as a limit but that has no time (a 403 with only a reason, no header, no policy time) is now throttled: it follows the same ladder as a 429 with no hint, and the last error is a FetchError flagged isRateLimited, so the queue worker does not halt and discard the message. Before, it became a plain 4xx. resolveRateLimitHint returns null for a status other than 429 that classify did not recognise. - classify() gets the parsed body of any application/json, text/json or +json response, whatever the case of the media type. The old prefix match missed application/problem+json and mixed case, and matched application/jsonp. - The cumulative in-process wait now travels through every retry, not only the hinted one. A 5xx or a connection reset between two hinted waits no longer resets the 5 minute cap. - FetchError.create reads the response only when no body was given, so an empty body the caller already read is not read a second time. - The throttle handling moves out of _rawRequest into _throttleRetry, which keeps that function from growing further. - The Requester takes an optional `random` parameter for the jitter, like `fetch` and `backOff`. The tests no longer replace Math.random: source-map picks its quicksort pivots with it, and a constant made jest overflow the stack while it formatted a stack trace. - Number.NaN and String.raw in the parsers. Refs ADR-049. Co-Authored-By: Claude Sonnet 5.5 --- .../api-module-definition-and-functions.md | 9 +- packages/core/CLAUDE.md | 5 +- packages/core/errors/fetch-error.js | 2 +- packages/core/errors/fetch-error.test.js | 10 + .../modules/requester/rate-limit/parsers.js | 4 +- .../modules/requester/rate-limit/policy.js | 7 +- .../requester/rate-limit/policy.test.js | 36 +++ packages/core/modules/requester/requester.js | 229 +++++++++++------- .../core/modules/requester/requester.test.js | 118 ++++++++- packages/core/types/errors/index.d.ts | 6 + 10 files changed, 322 insertions(+), 104 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 0c049cffd..959ef7167 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -240,14 +240,15 @@ response has one. ### What `classify` returns `classify({ status, headers, body })` runs for a 429, and for any other 4xx or -5xx except 401. `body` is the parsed JSON body, or `undefined` when the -response is not JSON. It returns `null` when the response is not a limit, or a -hint: +5xx except 401. `body` is the parsed body of an `application/json`, `text/json` +or `+json` response (`application/problem+json`, for example), whatever the +case of the media type, or `undefined` for any other response. It returns +`null` when the response is not a limit, or a hint: | Field | Meaning | |---|---| | `reason` | `'burst'`, `'daily'`, `'monthly'`, `'concurrency'` or `'unknown'` | -| `waitMs` or `retryAt` | When to call again. Leave both out to take the time from the parsers, then from the policy. | +| `waitMs` or `retryAt` | When to call again. Leave both out to take the time from the parsers, then from the policy. With no time from either, the response follows the same ladder as a 429 with no hint, and the last error is a `FetchError` with `isRateLimited: true` and the `reason`. The queue worker does not halt it. | | `policy` | The provider's own name for the limit, for logs and the UI | | `remaining` | Calls left in the window, when the response says | | `source` | `'header'`, `'body'` (default) or `'static'` | diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 440ffcb84..878010a49 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -381,7 +381,10 @@ in this order: | 4 | The fixed `backOff` ladder (1, 3, 10, 30, 60, 180 s) | - A 429 with no hint from steps 1 to 3 keeps today's ladder: the same calls, - the same delays, then a plain `FetchError`. Nothing budgets it. + the same delays, then a plain `FetchError`. Nothing budgets it. A response + that `classify` names as a limit but that has no time follows the same + ladder, and the last error is a `FetchError` flagged `isRateLimited` with + the `reason`, so the queue worker does not halt it. - With a hint, the wait is `max(hint, minRetryAfterMs, 1 s)` plus at most 10 % jitter. The total sleep of one request is capped at `maxInProcessWaitMs` (default 5 minutes) and at the time left in the invocation less one request diff --git a/packages/core/errors/fetch-error.js b/packages/core/errors/fetch-error.js index ebba099ef..ea74b3a30 100644 --- a/packages/core/errors/fetch-error.js +++ b/packages/core/errors/fetch-error.js @@ -58,7 +58,7 @@ class FetchError extends BaseError { const provided = options.responseBody ?? options.body; let responseBody = provided; if ( - !responseBody && + responseBody === undefined && response && !response.bodyUsed && typeof response.text === 'function' diff --git a/packages/core/errors/fetch-error.test.js b/packages/core/errors/fetch-error.test.js index 604324cf1..f24cff891 100644 --- a/packages/core/errors/fetch-error.test.js +++ b/packages/core/errors/fetch-error.test.js @@ -202,6 +202,16 @@ describe('FetchError', () => { expect(error.body).toBe('already read'); }); + it('create() keeps an empty responseBody it is given and does not read the stream', async () => { + const text = jest.fn(async () => 'second read'); + const error = await FetchError.create({ + response: { status: 500, bodyUsed: false, text }, + responseBody: '', + }); + expect(text).not.toHaveBeenCalled(); + expect(error.body).toBe(''); + }); + it('create() constructs the subclass it is called on', async () => { class ProviderError extends FetchError {} const error = await ProviderError.create({ diff --git a/packages/core/modules/requester/rate-limit/parsers.js b/packages/core/modules/requester/rate-limit/parsers.js index 7cc3d3bf8..69ee8cfcf 100644 --- a/packages/core/modules/requester/rate-limit/parsers.js +++ b/packages/core/modules/requester/rate-limit/parsers.js @@ -90,7 +90,7 @@ function hintFromWait(waitMs, now, { source = 'header', ...extra } = {}) { * Builds a hint from an absolute time. A time in the past waits 0 ms. */ function hintFromRetryAt(retryAt, now, { source = 'header', ...extra } = {}) { - const at = retryAt instanceof Date ? retryAt.getTime() : NaN; + const at = retryAt instanceof Date ? retryAt.getTime() : Number.NaN; if (Number.isNaN(at)) return null; const waitMs = Math.max(0, at - now); if (waitMs > MAX_HINT_WAIT_MS) return null; @@ -216,7 +216,7 @@ function pickStructured(items) { function readKeyValue(text, key) { const match = new RegExp( - `(?:^|[,;\\s])${key}\\s*=\\s*([^,;\\s]+)`, + String.raw`(?:^|[,;\s])${key}\s*=\s*([^,;\s]+)`, 'i' ).exec(text); return match ? match[1] : undefined; diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index a3d764887..aef1cde65 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -243,8 +243,10 @@ function classifyRateLimit(policy, signal, options = {}) { } /** - * Like classifyRateLimit, but always answers: with no hint it returns the - * step of the fixed backoff ladder, with source "backoff". + * Like classifyRateLimit, but a throttled response always gets a hint: with + * none found it gets the step of the fixed backoff ladder, with source + * "backoff". Null when the response is not throttled: a status other than 429 + * that classify did not recognise. */ function resolveRateLimitHint(policy, signal, options = {}) { const { @@ -258,6 +260,7 @@ function resolveRateLimitHint(policy, signal, options = {}) { onClassifyError, }); if (hint) return hint; + if (signal.status !== 429 && !classified) return null; const waitMs = attempt < backOff.length ? Number(backOff[attempt]) * 1000 || 0 : 0; diff --git a/packages/core/modules/requester/rate-limit/policy.test.js b/packages/core/modules/requester/rate-limit/policy.test.js index 2a2b1e5b0..bcd14c8b3 100644 --- a/packages/core/modules/requester/rate-limit/policy.test.js +++ b/packages/core/modules/requester/rate-limit/policy.test.js @@ -533,6 +533,42 @@ describe('rate-limit/policy', () => { }); }); + it('returns null for a status other than 429 that classify did not recognise', () => { + expect( + resolveRateLimitHint( + { classify: () => null }, + { status: 403, headers: { 'retry-after': '5' } }, + { ...options, attempt: 0, backOff } + ) + ).toBeNull(); + expect( + resolveRateLimitHint( + undefined, + { status: 503, headers: {} }, + { ...options, attempt: 0, backOff } + ) + ).toBeNull(); + }); + + it('falls back to the ladder for a status other than 429 that classify named by reason only', () => { + const policy = { + classify: () => ({ reason: 'daily', policy: 'DAILY' }), + }; + expect( + resolveRateLimitHint( + policy, + { status: 403, headers: {} }, + { ...options, attempt: 1, backOff } + ) + ).toEqual({ + waitMs: 3_000, + retryAt: new Date(NOW + 3_000), + reason: 'daily', + policy: 'DAILY', + source: 'backoff', + }); + }); + it('waits 0 ms past the end of the ladder', () => { const hint = resolveRateLimitHint( undefined, diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index c8fa131c9..e3a853909 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -10,7 +10,6 @@ const { redactUrl } = require('../../logs/redact'); const { toSanitizedSurrogate } = require('../../logs/serialize'); const { getLoggerScope } = require('../../logs/context'); const { - classifyRateLimit, computeScopeKey, computeWaitMs, headerValue, @@ -20,7 +19,18 @@ const { } = require('./rate-limit'); const DEFAULT_REQUEST_TIMEOUT_MS = 60_000; -const JSON_CONTENT_TYPE = /^application\/(json|vnd\.api\+json|hal\+json)/; + +function isJsonMediaType(contentType) { + const mediaType = String(contentType ?? '') + .split(';')[0] + .trim() + .toLowerCase(); + return ( + mediaType === 'application/json' || + mediaType === 'text/json' || + mediaType.endsWith('+json') + ); +} // A node-fetch error message holds the raw URL, and util.inspect prints the // cause chain, so the FetchError keeps only a sanitized copy. @@ -46,6 +56,7 @@ class Requester extends Delegate { super(params); this.backOff = get(params, 'backOff', [1, 3, 10, 30, 60, 180]); this._rateLimitPolicy = readRateLimitPolicy(this.constructor); + this._random = params?.random ?? Math.random; this.isRefreshable = false; this.refreshCount = 0; this.authGraceRetryCount = 0; @@ -294,7 +305,12 @@ class Requester extends Delegate { clearRequestTimer(); const delay = this.backOff[attempt] * 1000; await new Promise((resolve) => setTimeout(resolve, delay)); - return this._rawRequest(url, options, attempt + 1); + return this._rawRequest( + url, + options, + attempt + 1, + waitedMs + ); } const fetchError = await FetchError.create({ resource: encodedUrl, @@ -318,70 +334,27 @@ class Requester extends Delegate { status, attempt ); - if (throttle?.throttled) { - const { hint } = throttle; - if (hint.source === 'backoff') { - if (attempt < this.backOff.length) { - clearRequestTimer(); - const delay = this.backOff[attempt] * 1000; - await new Promise((resolve) => - setTimeout(resolve, delay) - ); - return this._rawRequest( - url, - options, - attempt + 1, - waitedMs - ); - } - } else { - const budgetMs = inProcessBudgetMs({ - policy: this._rateLimitPolicy, - requestTimeoutMs: this.requestTimeoutMs, - remainingMs: remainingInvocationMs(), - waitedMs, - }); - const { waitMs, fits } = computeWaitMs({ - hint, - policy: this._rateLimitPolicy, - budgetMs, - }); - this._logRateLimited({ - status, - hint, - waitMs, - attempt, - waitedMs, - action: fits ? 'wait' : 'throw', - }); - if (fits) { - clearRequestTimer(); - await new Promise((resolve) => - setTimeout(resolve, waitMs) - ); - return this._rawRequest( - url, - options, - attempt + 1, - waitedMs + waitMs - ); - } - this._logRequestFailed(encodedUrl, options, status); - const rateLimitError = await RateLimitError.create({ - resource: encodedUrl, - init: options, - response, - responseBody: throttle.responseBody, - hint, - waitMs, - module: this._telemetryModuleLabel(), - scopeKey: computeScopeKey(this._rateLimitPolicy, this), - }); - throw this._maybeFlagTimeoutDuringBodyRead( - rateLimitError, - timeoutMs - ); - } + const throttleRetry = await this._throttleRetry({ + throttle, + status, + attempt, + waitedMs, + encodedUrl, + options, + response, + timeoutMs, + }); + if (throttleRetry) { + clearRequestTimer(); + await new Promise((resolve) => + setTimeout(resolve, throttleRetry.delayMs) + ); + return this._rawRequest( + url, + options, + attempt + 1, + waitedMs + throttleRetry.hintedMs + ); } // If the status is retriable and there are back off requests left, retry the request @@ -389,7 +362,7 @@ class Requester extends Delegate { clearRequestTimer(); const delay = this.backOff[attempt] * 1000; await new Promise((resolve) => setTimeout(resolve, delay)); - return this._rawRequest(url, options, attempt + 1); + return this._rawRequest(url, options, attempt + 1, waitedMs); } if (status === 401) { @@ -410,7 +383,12 @@ class Requester extends Delegate { // This request did not try the current token. Retry with // it. Do not spend one more provider-side rotation. clearRequestTimer(); - return this._rawRequest(url, options, attempt + 1); + return this._rawRequest( + url, + options, + attempt + 1, + waitedMs + ); } if (!this.isRefreshable) { @@ -427,7 +405,12 @@ class Requester extends Delegate { await new Promise((resolve) => setTimeout(resolve, delay) ); - return this._rawRequest(url, options, attempt + 1); + return this._rawRequest( + url, + options, + attempt + 1, + waitedMs + ); } throw await this._invalidateAuth( @@ -460,7 +443,12 @@ class Requester extends Delegate { const refreshSucceeded = await this._refreshAuthOnce(); if (refreshSucceeded) { clearRequestTimer(); - return this._rawRequest(url, options, attempt + 1); + return this._rawRequest( + url, + options, + attempt + 1, + waitedMs + ); } throw await this._invalidateAuth(encodedUrl, options, response); @@ -477,6 +465,10 @@ class Requester extends Delegate { response, responseBody: throttle?.responseBody, }); + if (throttle?.throttled && status !== 429) { + fetchError.isRateLimited = true; + fetchError.reason = throttle.hint.reason; + } throw this._maybeFlagTimeoutDuringBodyRead( fetchError, timeoutMs @@ -505,6 +497,69 @@ class Requester extends Delegate { } } + /** + * What to do about a throttled response: the delay before the next + * attempt, or null when there is no attempt left. A wait that a hint set + * and that does not fit the budget throws RateLimitError. + * + * @returns {Promise<{delayMs: number, hintedMs: number}|null>} + * `hintedMs` is the part of the delay that a provider or a policy set, + * which counts against the in-process cap. + */ + async _throttleRetry({ + throttle, + status, + attempt, + waitedMs, + encodedUrl, + options, + response, + timeoutMs, + }) { + if (!throttle?.throttled) return null; + const { hint } = throttle; + if (hint.source === 'backoff') { + return attempt < this.backOff.length + ? { delayMs: this.backOff[attempt] * 1000, hintedMs: 0 } + : null; + } + + const budgetMs = inProcessBudgetMs({ + policy: this._rateLimitPolicy, + requestTimeoutMs: this.requestTimeoutMs, + remainingMs: remainingInvocationMs(), + waitedMs, + }); + const { waitMs, fits } = computeWaitMs({ + hint, + policy: this._rateLimitPolicy, + budgetMs, + random: this._random, + }); + this._logRateLimited({ + status, + hint, + waitMs, + attempt, + waitedMs, + action: fits ? 'wait' : 'throw', + }); + if (fits) return { delayMs: waitMs, hintedMs: waitMs }; + + this._logRequestFailed(encodedUrl, options, status); + const rateLimitError = await RateLimitError.create({ + resource: encodedUrl, + init: options, + response, + responseBody: throttle.responseBody, + hint, + waitMs, + module: this._telemetryModuleLabel(), + scopeKey: computeScopeKey(this._rateLimitPolicy, this), + }); + throw this._maybeFlagTimeoutDuringBodyRead(rateLimitError, timeoutMs); + } + /** * Decides whether a response says a limit was hit. A 429 always does. A * 4xx or 5xx other than 401 does only when the module's classify() names @@ -528,21 +583,17 @@ class Requester extends Delegate { await this._readBodyForClassify(response)); } - const signal = { status, headers: response.headers, body }; - const options = { - attempt, - backOff: this.backOff, - now: Date.now(), - onClassifyError: (error) => this._logClassifyFailed(status, error), - }; - if (status === 429) { - return { - throttled: true, - hint: resolveRateLimitHint(policy, signal, options), - responseBody, - }; - } - const hint = classifyRateLimit(policy, signal, options); + const hint = resolveRateLimitHint( + policy, + { status, headers: response.headers, body }, + { + attempt, + backOff: this.backOff, + now: Date.now(), + onClassifyError: (error) => + this._logClassifyFailed(status, error), + } + ); return { throttled: Boolean(hint), hint, responseBody }; } @@ -551,11 +602,7 @@ class Requester extends Delegate { return {}; } const text = await response.text(); - if ( - !JSON_CONTENT_TYPE.test( - headerValue(response.headers, 'content-type') || '' - ) - ) { + if (!isJsonMediaType(headerValue(response.headers, 'content-type'))) { return { text }; } try { diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index 33fa3ee5e..360fc6a62 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -55,6 +55,7 @@ describe('Requester', () => { const fetch = jest.fn(async () => { const next = queue.shift(); if (!next) throw new Error('unexpected fetch call'); + if (next.reject) throw next.reject; const contentType = next.contentType ?? 'application/json'; const text = next.text ?? JSON.stringify(next.body ?? {}); const response = { @@ -97,7 +98,6 @@ describe('Requester', () => { return realSetTimeout(fn, 0, ...args); } ); - jest.spyOn(Math, 'random').mockReturnValue(0); sink = createMemorySink(); }); @@ -111,6 +111,7 @@ describe('Requester', () => { return new RequesterClass({ fetch, requestTimeoutMs: 0, + random: () => 0, ...params, }); } @@ -227,12 +228,13 @@ describe('Requester', () => { }); it('adds jitter that never passes the wait by more than a tenth', async () => { - Math.random.mockReturnValue(0.999999); const fetch = throttledFetch([ limited({ 'Retry-After': '60' }), ok, ]); - await makeRequester(fetch)._get({ url }); + await makeRequester(fetch, { random: () => 0.999999 })._get({ + url, + }); expect(delays).toEqual([60_999]); }); @@ -291,6 +293,42 @@ describe('Requester', () => { expect(fetch).toHaveBeenCalledTimes(4); }); + it('counts a hinted wait against the cap across a 5xx retry', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '200' }), + { status: 500 }, + limited({ 'Retry-After': '200' }), + ok, + ]); + + const error = await makeRequester(fetch) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(delays).toEqual([200_000, 3_000]); + expect(fetch).toHaveBeenCalledTimes(3); + }); + + it('counts a hinted wait against the cap across a connection reset', async () => { + const reset = Object.assign(new Error('socket hang up'), { + code: 'ECONNRESET', + }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '200' }), + { reject: reset }, + limited({ 'Retry-After': '200' }), + ok, + ]); + + const error = await makeRequester(fetch) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(delays).toEqual([200_000, 3_000]); + }); + it('throws at once when the wait does not fit the time left in the invocation', async () => { const fetch = throttledFetch([ limited({ 'Retry-After': '4' }), @@ -463,6 +501,80 @@ describe('Requester', () => { expect(delays).toEqual([]); }); + it('treats a 403 that classify names by reason only like a bare 429: the ladder, then a flagged FetchError', async () => { + const Api = withPolicy({ + classify: () => ({ reason: 'daily' }), + }); + const fetch = throttledFetch( + Array(3).fill({ status: 403, body: { code: 'LIMIT' } }) + ); + + const error = await makeRequester( + fetch, + { backOff: [0, 0] }, + Api + ) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(FetchError); + expect(error).not.toBeInstanceOf(RateLimitError); + expect(error).toMatchObject({ + statusCode: 403, + isRateLimited: true, + reason: 'daily', + }); + expect(error.retryAt).toBeUndefined(); + expect(fetch).toHaveBeenCalledTimes(3); + expect(delays).toEqual([0, 0]); + }); + + it.each([ + 'application/problem+json', + 'Application/JSON; charset=utf-8', + 'application/vnd.api+json', + 'application/hal+json', + 'text/json', + ])( + 'hands classify the parsed body of a %s response', + async (contentType) => { + const classify = jest.fn(() => ({ + reason: 'burst', + waitMs: 1_000, + })); + const Api = withPolicy({ classify }); + const fetch = throttledFetch([ + { status: 403, contentType, body: { code: 'LIMIT' } }, + ok, + ]); + + await makeRequester(fetch, {}, Api)._get({ url }); + + expect(classify.mock.calls[0][0].body).toEqual({ + code: 'LIMIT', + }); + } + ); + + it.each(['text/plain', 'application/xml', 'application/jsonp'])( + 'hands classify no body for a %s response', + async (contentType) => { + const classify = jest.fn(() => ({ + reason: 'burst', + waitMs: 1_000, + })); + const Api = withPolicy({ classify }); + const fetch = throttledFetch([ + { status: 403, contentType, text: '{"code":"LIMIT"}' }, + ok, + ]); + + await makeRequester(fetch, {}, Api)._get({ url }); + + expect(classify.mock.calls[0][0].body).toBeUndefined(); + } + ); + it('takes the time of a reason-only classify from Retry-After', async () => { const Api = withPolicy({ classify: () => ({ reason: 'burst' }), diff --git a/packages/core/types/errors/index.d.ts b/packages/core/types/errors/index.d.ts index f63ed6563..04181f355 100644 --- a/packages/core/types/errors/index.d.ts +++ b/packages/core/types/errors/index.d.ts @@ -16,6 +16,12 @@ declare module "@friggframework/errors" { readonly body: any; isTimeout?: boolean; timeoutMs?: number; + /** + * True when `classify` named the response as a limit but gave no time. A + * `RateLimitError` always has it. + */ + isRateLimited?: boolean; + reason?: RateLimitReason; static create(options?: CreateFetchErrorParams): Promise; } From 0d9ce57850f74be170f19d278d4463e5ba3ecbf2 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 14:22:08 -0300 Subject: [PATCH 10/33] refactor(core): move the rate-limit notes from comments to the reference Delete the JSDoc and inline comments that the rate-limit change added to the parsers, the policy, the Requester, the invocation deadline, RateLimitError and the three type files. The API facts they held that the docs lacked now live in the API module reference: what each built-in parser reads, the TypeError for an unknown parser, an undefined scopeKey, the hint shape, classifyRateLimit and the RateLimitError constructor, and the invocation deadline. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 40 ++++++++++++++++++- packages/core/core/invocation-deadline.js | 13 ------ packages/core/errors/rate-limit-error.js | 4 -- .../modules/requester/rate-limit/parsers.js | 24 ----------- .../modules/requester/rate-limit/policy.js | 34 ---------------- packages/core/modules/requester/requester.js | 20 ---------- packages/core/types/core/index.d.ts | 9 ----- packages/core/types/errors/index.d.ts | 15 ------- packages/core/types/module-plugin/index.d.ts | 9 ----- 9 files changed, 38 insertions(+), 130 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 959ef7167..4acc204b8 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -229,14 +229,29 @@ response has one. | Key | Meaning | |---|---| -| `scope` | The key a limit counts against: `'entity'` (default), `'credential'`, `'app'`, or a function `(requester) => key`. The Requester puts it on `RateLimitError.scopeKey`. | +| `scope` | The key a limit counts against: `'entity'` (default), `'credential'`, `'app'`, or a function `(requester) => key`. The Requester puts it on `RateLimitError.scopeKey`, which is `undefined` when the requester has no id for the scope. | | `minRetryAfterMs` | A wait never shorter than this. Use it when the docs say "wait 60 seconds" and the response says nothing. | | `maxInProcessWaitMs` | The most one request sleeps in process, in total. Default `300000` (5 minutes). `0` means never sleep: every hinted wait throws `RateLimitError`. | -| `parsers` | The header parsers to read, in order: `retryAfter`, `resetHeaders`, `ietf`. Default: all three. | +| `parsers` | The header parsers to read, in order: `retryAfter`, `resetHeaders`, `ietf`. Default: all three. A name that is not built in throws a `TypeError` when the API class is constructed. | | `classify` | Recognises a limit that is not a plain 429, or names its reason. See below. | | `windows`, `maxConcurrency` | The provider's limits, as documentation. A window named like the `reason` (`{ name: 'burst', perMs: 10000 }`) sets the wait when no header does. Pacing uses them later. | | `userHints` | Links per `reason` that a UI can show, for example `{ daily: { links: [{ label, url }] } }`. | +### Built-in parsers + +| Parser | Reads | +|---|---| +| `retryAfter` | `Retry-After`: delta seconds, an HTTP-date or an ISO timestamp | +| `resetHeaders` | `X-RateLimit-Reset-After` (delta seconds), else `X-RateLimit-Reset`, `RateLimit-Reset` or `X-Rate-Limit-Reset`: epoch milliseconds, epoch seconds or delta seconds (told apart by size), or a date. `remaining` comes from `X-RateLimit-Remaining`, `RateLimit-Remaining` or `X-Rate-Limit-Remaining` | +| `ietf` | The IETF `RateLimit` field, as `limit=, remaining=, reset=` or as structured items (`"name";r=0;t=12`). In a list, the item with the fewest remaining calls wins, then the longest wait. `RateLimit-Policy` names the policy | + +A parser reads the headers from a `Headers` object, a `Map`, an entries array or +a plain object, and matches the names in any case. A time in the past waits +0 ms. A wait that is negative, not a number or longer than 366 days is ignored. +The package exports the parsers as `parseRetryAfter(value, { now })`, +`parseResetHeaders(headers, { now })` and `parseIetfRateLimit(headers, { now })`. +Each returns a hint or `null`. + ### What `classify` returns `classify({ status, headers, body })` runs for a 429, and for any other 4xx or @@ -320,6 +335,27 @@ async withLimits(call) { } ``` +`classifyRateLimit(policy, { status, headers, body }, { now, onClassifyError })` +resolves in the order the Requester uses and returns a hint or `null`. A hint is +`{ retryAt, waitMs, reason, source, policy, remaining }`: `retryAt` is a `Date`, +`waitMs` counts from the call, and `policy` and `remaining` are there only when +known. An error that `classify` throws propagates unless you pass +`onClassifyError`. + +`new RateLimitError({ hint, waitMs, module, scopeKey, ...fetchErrorArgs })` +takes the `FetchError` arguments (`resource`, `init`, `response`, `cause`, +`responseBody`). It sets `retryAt` to now plus `waitMs`, or to `hint.retryAt` +when `waitMs` is left out. + +### The invocation deadline + +`runInvocationScope` stores the end of every Lambda invocation, from +`context.getRemainingTimeInMillis()`. `remainingInvocationMs()` returns the +milliseconds left, or `Infinity` outside an invocation (tests, `frigg start`, +scripts). `runWithInvocationDeadline(deadlineAt, fn)` runs `fn` with a deadline +in epoch milliseconds. A nested call can only make the deadline earlier, and a +deadline that is not a finite number sets none. + ## Complete OAuth2 Example ```javascript diff --git a/packages/core/core/invocation-deadline.js b/packages/core/core/invocation-deadline.js index 41c94a932..a8492b807 100644 --- a/packages/core/core/invocation-deadline.js +++ b/packages/core/core/invocation-deadline.js @@ -9,11 +9,6 @@ function storage() { return globalThis[STORE_KEY]; } -/** - * Runs fn with the time the invocation ends, as epoch milliseconds. A nested - * scope can shorten the deadline and never extend it. A deadline that is not - * a finite number runs fn with no limit. - */ function runWithInvocationDeadline(deadlineAt, fn) { if (typeof deadlineAt !== 'number' || !Number.isFinite(deadlineAt)) { return fn(); @@ -25,10 +20,6 @@ function runWithInvocationDeadline(deadlineAt, fn) { return storage().run(Object.freeze({ deadlineAt: effective }), fn); } -/** - * The end of a Lambda invocation, from its context. Undefined when the - * context cannot tell. - */ function deadlineFromContext(context) { try { const remaining = context?.getRemainingTimeInMillis?.(); @@ -40,10 +31,6 @@ function deadlineFromContext(context) { } } -/** - * Milliseconds left in the invocation. Infinity outside a deadline scope - * (tests, `frigg start`, scripts). - */ function remainingInvocationMs(now = Date.now()) { const store = storage().getStore(); return store ? Math.max(0, store.deadlineAt - now) : Infinity; diff --git a/packages/core/errors/rate-limit-error.js b/packages/core/errors/rate-limit-error.js index cacc05290..549d02572 100644 --- a/packages/core/errors/rate-limit-error.js +++ b/packages/core/errors/rate-limit-error.js @@ -14,10 +14,6 @@ function resolveTiming({ hint, waitMs, now }) { return { waitMs: fallbackWaitMs, retryAt: new Date(now + fallbackWaitMs) }; } -/** - * A FetchError for a response that said a limit was hit and told us, or the - * module's policy told us, when to call again. `retryAt` is that time. - */ class RateLimitError extends FetchError { constructor({ hint, diff --git a/packages/core/modules/requester/rate-limit/parsers.js b/packages/core/modules/requester/rate-limit/parsers.js index 69ee8cfcf..6258dec2e 100644 --- a/packages/core/modules/requester/rate-limit/parsers.js +++ b/packages/core/modules/requester/rate-limit/parsers.js @@ -34,11 +34,6 @@ function readKeys(headers, wanted) { return undefined; } -/** - * Reads one header from a Headers-like object, a Map, an entries array or a - * plain object. The name match is case-insensitive. - * @returns {string|undefined} - */ function headerValue(headers, name) { if (!headers || typeof headers !== 'object') return undefined; const wanted = name.toLowerCase(); @@ -70,10 +65,6 @@ function definedOnly(fields) { return result; } -/** - * Builds a hint from a wait in ms. Returns null for a wait that is not a - * finite number in [0, MAX_HINT_WAIT_MS]. - */ function hintFromWait(waitMs, now, { source = 'header', ...extra } = {}) { if (!Number.isFinite(waitMs) || waitMs < 0 || waitMs > MAX_HINT_WAIT_MS) { return null; @@ -86,9 +77,6 @@ function hintFromWait(waitMs, now, { source = 'header', ...extra } = {}) { }; } -/** - * Builds a hint from an absolute time. A time in the past waits 0 ms. - */ function hintFromRetryAt(retryAt, now, { source = 'header', ...extra } = {}) { const at = retryAt instanceof Date ? retryAt.getTime() : Number.NaN; if (Number.isNaN(at)) return null; @@ -103,9 +91,6 @@ function parseDate(text) { return Number.isNaN(at) ? null : new Date(at); } -/** - * Retry-After: delta seconds, an HTTP-date or an ISO timestamp. - */ function parseRetryAfter(value, { now = Date.now() } = {}) { if (isMissing(value)) return null; const text = String(value).trim(); @@ -145,10 +130,6 @@ function parseResetValue(text, now, extra) { return date ? hintFromRetryAt(date, now, extra) : null; } -/** - * X-RateLimit-Reset and the other reset headers. The magnitude tells the - * unit: epoch milliseconds, epoch seconds, or delta seconds. - */ function parseResetHeaders(headers, { now = Date.now() } = {}) { const remaining = numberOrUndefined( firstHeader(headers, REMAINING_HEADERS) @@ -222,11 +203,6 @@ function readKeyValue(text, key) { return match ? match[1] : undefined; } -/** - * The IETF RateLimit field: the key=value form (limit=, remaining=, reset=) - * and the structured-field form ("name";r=0;t=12). The list form picks the - * policy with the fewest remaining calls, then the longest wait. - */ function parseIetfRateLimit(headers, { now = Date.now() } = {}) { const raw = headerValue(headers, 'ratelimit'); if (raw === undefined) return null; diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index aef1cde65..729a2e72c 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -79,11 +79,6 @@ function normalizePolicy(raw, owner) { return policy; } -/** - * Reads the static `rateLimit` policy of an API module class. Returns - * undefined when the class declares none. The result is memoized per class. - * Throws a TypeError for a parser name that is not built in. - */ function readRateLimitPolicy(ctor) { if (typeof ctor !== 'function') return undefined; if (policiesByClass.has(ctor)) return policiesByClass.get(ctor); @@ -98,10 +93,6 @@ function asPolicy(value) { return normalizePolicy(value, 'policy'); } -/** - * The key a limit counts against, for one requester. Undefined when the - * scope needs an id that the requester does not have. - */ function computeScopeKey(policy, requester) { const label = requester?._telemetryModuleLabel?.() ?? 'unknown'; const scope = policy?.scope ?? 'entity'; @@ -226,28 +217,12 @@ function resolveCandidates(policy, signal, { now, onClassifyError }) { return { hint: null, classified }; } -/** - * Finds the hint of a throttled response: the module's classify(), then the - * header parsers, then the static policy. Returns null when there is none. - * A status other than 429 counts only when classify recognised it. - * - * @param {object} [policy] The static `rateLimit` object of an API module. - * @param {{status?: number, headers?: object, body?: unknown}} signal - * @param {{now?: number, onClassifyError?: (error: Error) => void}} [options] - * Without `onClassifyError` an error thrown by classify propagates. - */ function classifyRateLimit(policy, signal, options = {}) { const { now = Date.now(), onClassifyError } = options; return resolveCandidates(asPolicy(policy), signal, { now, onClassifyError }) .hint; } -/** - * Like classifyRateLimit, but a throttled response always gets a hint: with - * none found it gets the step of the fixed backoff ladder, with source - * "backoff". Null when the response is not throttled: a status other than 429 - * that classify did not recognise. - */ function resolveRateLimitHint(policy, signal, options = {}) { const { attempt = 0, @@ -276,11 +251,6 @@ function resolveRateLimitHint(policy, signal, options = {}) { }; } -/** - * The wait before the next attempt, for a hint that came from the provider - * or the policy. Jitter only adds to the wait and never passes the budget. - * `fits` is false when the wait alone is over the budget. - */ function computeWaitMs({ hint, policy, budgetMs, random = Math.random }) { const base = Math.max( hint.waitMs, @@ -295,10 +265,6 @@ function computeWaitMs({ hint, policy, budgetMs, random = Math.random }) { return { waitMs: Math.min(base + jitter, budgetMs), fits: true }; } -/** - * How long this request may still sleep in process: the cap, less what it - * slept already, and the time left in the invocation, less one request. - */ function inProcessBudgetMs({ policy, requestTimeoutMs = 0, diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index e3a853909..09266f2fe 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -235,9 +235,6 @@ class Requester extends Delegate { * @param {number} attempt - 0-based count of retries already made for * this call. Indexes `this.backOff` for the next delay and is passed * back in on each recursive retry. - * @param {number} waitedMs - Milliseconds this call has slept already - * for a wait that a provider or a policy set. It counts against the - * in-process cap. */ async _rawRequest(url, options, attempt = 0, waitedMs = 0) { let encodedUrl = encodeURI(url); @@ -497,15 +494,6 @@ class Requester extends Delegate { } } - /** - * What to do about a throttled response: the delay before the next - * attempt, or null when there is no attempt left. A wait that a hint set - * and that does not fit the budget throws RateLimitError. - * - * @returns {Promise<{delayMs: number, hintedMs: number}|null>} - * `hintedMs` is the part of the delay that a provider or a policy set, - * which counts against the in-process cap. - */ async _throttleRetry({ throttle, status, @@ -560,14 +548,6 @@ class Requester extends Delegate { throw this._maybeFlagTimeoutDuringBodyRead(rateLimitError, timeoutMs); } - /** - * Decides whether a response says a limit was hit. A 429 always does. A - * 4xx or 5xx other than 401 does only when the module's classify() names - * a limit. The body is read here, once, only for classify(); the text is - * handed on so the error need not read the stream again. - * - * @returns {Promise<{throttled: boolean, hint?: object, responseBody?: string}|null>} - */ async _detectThrottle(response, status, attempt) { const policy = this._rateLimitPolicy; const canClassify = diff --git a/packages/core/types/core/index.d.ts b/packages/core/types/core/index.d.ts index 3627558b7..e0d39bc55 100644 --- a/packages/core/types/core/index.d.ts +++ b/packages/core/types/core/index.d.ts @@ -58,22 +58,13 @@ declare module "@friggframework/core" { export function loadInstalledModules(): any[]; - /** - * Runs `fn` with the time the invocation ends, as epoch milliseconds. A - * nested scope can shorten the deadline and never extend it. - */ export function runWithInvocationDeadline( deadlineAt: number | undefined, fn: () => T ): T; - /** Milliseconds left in the invocation. `Infinity` outside a Lambda. */ export function remainingInvocationMs(now?: number): number; - /** - * Finds the hint of a throttled response: the module's classify(), then the - * header parsers, then the static policy. Null when there is none. - */ export function classifyRateLimit( policy: RateLimitPolicy | undefined, signal: RateLimitSignal, diff --git a/packages/core/types/errors/index.d.ts b/packages/core/types/errors/index.d.ts index 04181f355..da2dac51f 100644 --- a/packages/core/types/errors/index.d.ts +++ b/packages/core/types/errors/index.d.ts @@ -16,10 +16,6 @@ declare module "@friggframework/errors" { readonly body: any; isTimeout?: boolean; timeoutMs?: number; - /** - * True when `classify` named the response as a limit but gave no time. A - * `RateLimitError` always has it. - */ isRateLimited?: boolean; reason?: RateLimitReason; @@ -35,29 +31,19 @@ declare module "@friggframework/errors" { export type RateLimitSource = "header" | "body" | "static" | "backoff"; - /** What Frigg knows about when calls are accepted again. */ export type RateLimitHint = { - /** When calls are accepted again. */ retryAt: Date; - /** Milliseconds from the time the hint was read until `retryAt`. */ waitMs: number; reason: RateLimitReason; - /** A provider policy name, when the response names one. */ policy?: string; - /** Calls left in the window, when the response says. */ remaining?: number; source: RateLimitSource; }; - /** - * A FetchError for a response that said a limit was hit, and for which the - * response or the module's policy says when to call again. - */ export class RateLimitError extends FetchError { constructor(options?: RateLimitErrorConstructor); isRateLimited: true; - /** When calls are accepted again. */ retryAt: Date; waitMs: number; reason: RateLimitReason; @@ -73,7 +59,6 @@ declare module "@friggframework/errors" { type RateLimitErrorConstructor = FetchErrorConstructor & { hint?: Partial; - /** Wait from `now`. When absent, `hint.retryAt` sets the time. */ waitMs?: number; module?: string; scopeKey?: string; diff --git a/packages/core/types/module-plugin/index.d.ts b/packages/core/types/module-plugin/index.d.ts index 5224ac036..f17ace051 100644 --- a/packages/core/types/module-plugin/index.d.ts +++ b/packages/core/types/module-plugin/index.d.ts @@ -44,21 +44,12 @@ declare module "@friggframework/module-plugin" { }; export type RateLimitPolicy = { - /** The key a limit counts against. Default `entity`. */ scope?: RateLimitScope; windows?: RateLimitWindow[]; maxConcurrency?: number; - /** A wait never shorter than this, for every throttled response. */ minRetryAfterMs?: number; - /** The most a request sleeps in process, in total. Default 300000. */ maxInProcessWaitMs?: number; - /** Header parsers, in order. Default all three. */ parsers?: Array<"retryAfter" | "resetHeaders" | "ietf">; - /** - * Recognises a limit that is not a plain 429, or names its reason. Returns - * null when the response is not a limit. A hint with no time takes its - * time from the parsers or the policy. - */ classify?( signal: RateLimitSignal ): Partial> & { From a6039526e4f2c24cd7d83ffdbfe92cdc95f70639 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 14:22:54 -0300 Subject: [PATCH 11/33] refactor(core): keep one definedOnly for the rate-limit helpers The policy had its own copy of the helper that the parsers already define. It now imports the parsers' one. Co-Authored-By: Claude Opus 5.5 --- packages/core/modules/requester/rate-limit/parsers.js | 1 + packages/core/modules/requester/rate-limit/policy.js | 9 +-------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/packages/core/modules/requester/rate-limit/parsers.js b/packages/core/modules/requester/rate-limit/parsers.js index 6258dec2e..dcf45190f 100644 --- a/packages/core/modules/requester/rate-limit/parsers.js +++ b/packages/core/modules/requester/rate-limit/parsers.js @@ -235,6 +235,7 @@ const BUILT_IN_PARSERS = { module.exports = { BUILT_IN_PARSERS, MAX_HINT_WAIT_MS, + definedOnly, headerValue, hintFromRetryAt, hintFromWait, diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index 729a2e72c..518792e00 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -1,5 +1,6 @@ const { BUILT_IN_PARSERS, + definedOnly, hintFromRetryAt, hintFromWait, } = require('./parsers'); @@ -31,14 +32,6 @@ function isPlainObject(value) { return prototype === Object.prototype || prototype === null; } -function definedOnly(fields) { - const result = {}; - for (const [key, value] of Object.entries(fields)) { - if (value !== undefined) result[key] = value; - } - return result; -} - function readParsers(raw, owner) { if (!Array.isArray(raw.parsers)) return [...DEFAULT_PARSERS]; for (const name of raw.parsers) { From 14783959886cf65249a66e22d33f8042333e6dfe Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 16:58:39 -0300 Subject: [PATCH 12/33] refactor(core): read the time left in a Lambda context in one place deadlineFromContext repeated the guarded read of getRemainingTimeInMillis() that the invocation scope uses for its flush deadline. The read now lives in invocation-deadline.js as remainingTimeMs, and both use it. Results are the same for every input: a finite time gives now + time; a missing method, a non-number, a non-finite value or a throw gives no deadline. Co-Authored-By: Claude Opus 5.5 --- packages/core/core/invocation-deadline.js | 16 ++++++++++------ packages/core/core/invocation-scope.js | 10 +--------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/packages/core/core/invocation-deadline.js b/packages/core/core/invocation-deadline.js index a8492b807..9c7f63466 100644 --- a/packages/core/core/invocation-deadline.js +++ b/packages/core/core/invocation-deadline.js @@ -20,17 +20,20 @@ function runWithInvocationDeadline(deadlineAt, fn) { return storage().run(Object.freeze({ deadlineAt: effective }), fn); } -function deadlineFromContext(context) { +function remainingTimeMs(context) { try { const remaining = context?.getRemainingTimeInMillis?.(); - return typeof remaining === 'number' && Number.isFinite(remaining) - ? Date.now() + remaining - : undefined; - } catch { - return undefined; + return typeof remaining === 'number' ? remaining : Infinity; + } catch (_) { + return Infinity; } } +function deadlineFromContext(context) { + const remaining = remainingTimeMs(context); + return Number.isFinite(remaining) ? Date.now() + remaining : undefined; +} + function remainingInvocationMs(now = Date.now()) { const store = storage().getStore(); return store ? Math.max(0, store.deadlineAt - now) : Infinity; @@ -39,5 +42,6 @@ function remainingInvocationMs(now = Date.now()) { module.exports = { deadlineFromContext, remainingInvocationMs, + remainingTimeMs, runWithInvocationDeadline, }; diff --git a/packages/core/core/invocation-scope.js b/packages/core/core/invocation-scope.js index 1d0781fe6..567850a09 100644 --- a/packages/core/core/invocation-scope.js +++ b/packages/core/core/invocation-scope.js @@ -3,6 +3,7 @@ const { summarizeMessageBody } = require('../logs/summarize-event'); const { flushSinks, hasFlushableSinks } = require('../logs/logger-runtime'); const { deadlineFromContext, + remainingTimeMs, runWithInvocationDeadline, } = require('./invocation-deadline'); @@ -115,15 +116,6 @@ function withDeadline(ms, fn) { }); } -function remainingTimeMs(context) { - try { - const remaining = context?.getRemainingTimeInMillis?.(); - return typeof remaining === 'number' ? remaining : Infinity; - } catch (_) { - return Infinity; - } -} - async function flushInvocation({ telemetry, usageRollup, From 1c1ba7998a677cff99b8298842abf1bccb420167 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:07:17 -0300 Subject: [PATCH 13/33] refactor(core): send an unhinted throttle down the base retry ladder _throttleRetry returned { delayMs, hintedMs } where hintedMs was 0 on the ladder and equal to delayMs for a hint, and its ladder half repeated the 5xx ladder block. The ladder condition is back to the base shape, with throttle?.throttled in place of status === 429, and the method, now _hintedRetryDelayMs, returns only the wait a hint sets, null, or throws RateLimitError. Same calls and delays for every path. Co-Authored-By: Claude Opus 5.5 --- packages/core/modules/requester/requester.js | 27 ++++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index 09266f2fe..e1f6fbc9a 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -331,7 +331,7 @@ class Requester extends Delegate { status, attempt ); - const throttleRetry = await this._throttleRetry({ + const hintedDelayMs = await this._hintedRetryDelayMs({ throttle, status, attempt, @@ -341,21 +341,24 @@ class Requester extends Delegate { response, timeoutMs, }); - if (throttleRetry) { + if (hintedDelayMs !== null) { clearRequestTimer(); await new Promise((resolve) => - setTimeout(resolve, throttleRetry.delayMs) + setTimeout(resolve, hintedDelayMs) ); return this._rawRequest( url, options, attempt + 1, - waitedMs + throttleRetry.hintedMs + waitedMs + hintedDelayMs ); } // If the status is retriable and there are back off requests left, retry the request - if (status >= 500 && attempt < this.backOff.length) { + if ( + (throttle?.throttled || status >= 500) && + attempt < this.backOff.length + ) { clearRequestTimer(); const delay = this.backOff[attempt] * 1000; await new Promise((resolve) => setTimeout(resolve, delay)); @@ -494,7 +497,7 @@ class Requester extends Delegate { } } - async _throttleRetry({ + async _hintedRetryDelayMs({ throttle, status, attempt, @@ -504,14 +507,10 @@ class Requester extends Delegate { response, timeoutMs, }) { - if (!throttle?.throttled) return null; - const { hint } = throttle; - if (hint.source === 'backoff') { - return attempt < this.backOff.length - ? { delayMs: this.backOff[attempt] * 1000, hintedMs: 0 } - : null; + if (!throttle?.throttled || throttle.hint.source === 'backoff') { + return null; } - + const { hint } = throttle; const budgetMs = inProcessBudgetMs({ policy: this._rateLimitPolicy, requestTimeoutMs: this.requestTimeoutMs, @@ -532,7 +531,7 @@ class Requester extends Delegate { waitedMs, action: fits ? 'wait' : 'throw', }); - if (fits) return { delayMs: waitMs, hintedMs: waitMs }; + if (fits) return waitMs; this._logRequestFailed(encodedUrl, options, status); const rateLimitError = await RateLimitError.create({ From bca32d746e14909f34b9c8f0da156c461baa3c97 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:13:00 -0300 Subject: [PATCH 14/33] fix(core): keep at most half the time left for the next request The in-process budget kept back one full request timeout from the time left in the invocation. The default request timeout is 60 s and an HTTP Lambda has 29 s, so the budget was 0 there and every hinted 429 threw at once instead of a short wait and a retry. The reserve is now min(requestTimeoutMs, remainingMs / 2): budget = max(0, min(cap - waitedMs, remainingMs - reserve)). With 29 s left and a 60 s timeout the budget is 14.5 s; with 900 s left it is the 5-minute cap, as before; with no deadline it is the cap alone. The deadline test now uses Retry-After: 5, over the 4 s budget of 8 s left and a 5 s timeout, so it still proves that the budget respects the deadline. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 4 ++- packages/core/CLAUDE.md | 2 +- .../modules/requester/rate-limit/policy.js | 6 ++--- .../requester/rate-limit/policy.test.js | 26 ++++++++++++++++--- .../core/modules/requester/requester.test.js | 24 ++++++++++++++++- 5 files changed, 51 insertions(+), 11 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 4acc204b8..ab1b6690a 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -304,7 +304,9 @@ class Api extends ApiKeyRequester { 3. With a hint, the wait is the largest of the hint, `minRetryAfterMs` and 1 s, plus at most 10 % jitter. The Requester sleeps when the wait fits the time this request may still sleep: `maxInProcessWaitMs` in total, and the time left - in the Lambda invocation less one request timeout. + in the Lambda invocation less one request timeout. That reserve is never more + than half the time left, so a 29 s HTTP invocation with the default 60 s + request timeout can still sleep 14.5 s. 4. A wait that does not fit throws `RateLimitError`. It extends `FetchError`, so `statusCode` stays, and adds `isRateLimited`, `retryAt`, `waitMs`, `reason`, `policy`, `source`, `module` and `scopeKey`. diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 878010a49..c22885a45 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -388,7 +388,7 @@ in this order: - With a hint, the wait is `max(hint, minRetryAfterMs, 1 s)` plus at most 10 % jitter. The total sleep of one request is capped at `maxInProcessWaitMs` (default 5 minutes) and at the time left in the invocation less one request - timeout (`remainingInvocationMs()`). + timeout, a reserve of at most half the time left (`remainingInvocationMs()`). - A wait that does not fit throws `RateLimitError` (`isRateLimited`, `retryAt`, `waitMs`, `reason`, `policy`, `source`, `module`, `scopeKey`). The queue worker does not halt it, whatever its status. diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index 518792e00..0ee4fb628 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -265,10 +265,8 @@ function inProcessBudgetMs({ waitedMs = 0, } = {}) { const cap = policy?.maxInProcessWaitMs ?? DEFAULT_MAX_IN_PROCESS_WAIT_MS; - return Math.max( - 0, - Math.min(cap - waitedMs, remainingMs - (requestTimeoutMs || 0)) - ); + const reserveMs = Math.min(requestTimeoutMs || 0, remainingMs / 2); + return Math.max(0, Math.min(cap - waitedMs, remainingMs - reserveMs)); } module.exports = { diff --git a/packages/core/modules/requester/rate-limit/policy.test.js b/packages/core/modules/requester/rate-limit/policy.test.js index bcd14c8b3..fb8767748 100644 --- a/packages/core/modules/requester/rate-limit/policy.test.js +++ b/packages/core/modules/requester/rate-limit/policy.test.js @@ -741,20 +741,38 @@ describe('rate-limit/policy', () => { ).toBe(200_000); }); - it('uses the time left in the invocation, less one request', () => { + it('keeps one request timeout of the time left for the next request', () => { expect( inProcessBudgetMs({ - requestTimeoutMs: 60_000, + requestTimeoutMs: 30_000, remainingMs: 100_000, }) - ).toBe(40_000); + ).toBe(70_000); + }); + + it('keeps at most half of the time left for the next request', () => { + expect( + inProcessBudgetMs({ + requestTimeoutMs: 60_000, + remainingMs: 29_000, + }) + ).toBe(14_500); + }); + + it('is the cap when the invocation has time to spare', () => { + expect( + inProcessBudgetMs({ + requestTimeoutMs: 60_000, + remainingMs: 900_000, + }) + ).toBe(300_000); }); it('is never negative', () => { expect( inProcessBudgetMs({ requestTimeoutMs: 60_000, - remainingMs: 1_000, + remainingMs: 0, }) ).toBe(0); expect( diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index 360fc6a62..33cc9ba80 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -329,9 +329,31 @@ describe('Requester', () => { expect(delays).toEqual([200_000, 3_000]); }); + it('sleeps a short wait inside an invocation shorter than the default request timeout', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '2' }), + ok, + ]); + const requester = makeRequester(fetch, { + requestTimeoutMs: undefined, + }); + + const result = await runWithInvocationDeadline( + Date.now() + 29_000, + () => requester._get({ url }) + ); + + expect(requester.requestTimeoutMs).toBe(60_000); + expect(result).toEqual({ ok: true }); + expect(delays.filter((delay) => delay !== 60_000)).toEqual([ + 2_000, + ]); + expect(fetch).toHaveBeenCalledTimes(2); + }); + it('throws at once when the wait does not fit the time left in the invocation', async () => { const fetch = throttledFetch([ - limited({ 'Retry-After': '4' }), + limited({ 'Retry-After': '5' }), ok, ]); const requester = makeRequester(fetch, { From 8911244c9787c3f71d338dfd413d6d1e913033f0 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:21:04 -0300 Subject: [PATCH 15/33] fix(core): write retryAt and reason of a rate-limited error to the log serializeError keeps only type, message, code, status, stack and cause, so a RateLimitError logged at its boundary did not say when the limit clears. The serializer now also writes retryAt (ISO) and reason when the error has isRateLimited and a valid Date retryAt. An error without them serializes as before. The logging guides list the two fields; the stability rule allows an added field. Co-Authored-By: Claude Opus 5.5 --- docs/guides/LOGGING-IN-INTEGRATIONS.md | 3 +- docs/guides/LOGGING.md | 2 +- packages/core/errors/rate-limit-error.test.js | 14 +++++++ packages/core/logs/serialize.js | 15 ++++++++ packages/core/logs/serialize.test.js | 38 +++++++++++++++++++ 5 files changed, 70 insertions(+), 2 deletions(-) diff --git a/docs/guides/LOGGING-IN-INTEGRATIONS.md b/docs/guides/LOGGING-IN-INTEGRATIONS.md index d7b3b2610..6b65771b5 100644 --- a/docs/guides/LOGGING-IN-INTEGRATIONS.md +++ b/docs/guides/LOGGING-IN-INTEGRATIONS.md @@ -189,7 +189,8 @@ try { error)` also works (the logger moves it to `error`), but `{ error }` is the standard form. - The logger writes the error as `{ type, message, code, status, stack, - cause }`, redacted. The `cause` chain is kept to 3 levels. Other own + cause }`, redacted, plus `retryAt` and `reason` for a `RateLimitError`. + The `cause` chain is kept to 3 levels. Other own properties of the error (for example axios `config`, `request`, `response`) are not written, because they often hold tokens. - A `FetchError` from an API module already has a safe message: diff --git a/docs/guides/LOGGING.md b/docs/guides/LOGGING.md index afaf24eb7..7ef2283e6 100644 --- a/docs/guides/LOGGING.md +++ b/docs/guides/LOGGING.md @@ -88,7 +88,7 @@ One record, shown on several lines (stdout has it on one line): | `entityId`, `credentialId` | string | `Module` bindings | in API module records | | `eventName` | string | call site: the `logger` name, then `.` | required for `frigg.*` at `WARN` and above, and for framework lifecycle `INFO` | | `trace_id`, `span_id`, `trace_flags` | string (hex) | the active OTel span | inside a span only | -| `error` | object | `{ type, message, code, status, stack, cause }`, sanitized | when a call site passes `error` | +| `error` | object | `{ type, message, code, status, stack, cause }`, sanitized; plus `retryAt` (ISO) and `reason` for an error with `isRateLimited` and a valid `retryAt` | when a call site passes `error` | | `droppedKeys` | string[] | logger | when the logger removed a field (see Precedence) | Call-site fields go next to these, at the top level. diff --git a/packages/core/errors/rate-limit-error.test.js b/packages/core/errors/rate-limit-error.test.js index 2c203d172..d308dbb74 100644 --- a/packages/core/errors/rate-limit-error.test.js +++ b/packages/core/errors/rate-limit-error.test.js @@ -201,5 +201,19 @@ describe('RateLimitError', () => { status: 429, }); }); + + it('serializes when the limit clears and why', () => { + const error = new RateLimitError({ + resource: 'https://h.example', + response: { status: 429 }, + hint, + waitMs: 5_000, + now: NOW, + }); + expect(serializeError(error)).toMatchObject({ + retryAt: '2026-09-28T12:00:05.000Z', + reason: 'daily', + }); + }); }); }); diff --git a/packages/core/logs/serialize.js b/packages/core/logs/serialize.js index b85efc3fc..5034cf5c6 100644 --- a/packages/core/logs/serialize.js +++ b/packages/core/logs/serialize.js @@ -235,6 +235,20 @@ function errorCode(err) { return undefined; } +function rateLimitFields(err) { + const retryAt = attempt( + () => (err.isRateLimited ? err.retryAt : undefined), + undefined + ); + if (!(retryAt instanceof Date) || Number.isNaN(retryAt.getTime())) { + return {}; + } + const reason = attempt(() => err.reason, undefined); + return typeof reason === 'string' + ? { retryAt: retryAt.toISOString(), reason: cleanString(reason) } + : { retryAt: retryAt.toISOString() }; +} + function errorStack(err, type, message) { const stack = attempt(() => err.stack, undefined); if (typeof stack !== 'string') return undefined; @@ -279,6 +293,7 @@ function serializeErrorAt(err, depth, seen) { if (code !== undefined) out.code = code; const status = errorStatus(err); if (status !== undefined) out.status = status; + Object.assign(out, rateLimitFields(err)); const stack = errorStack(err, type, message); if (stack !== undefined) out.stack = stack; diff --git a/packages/core/logs/serialize.test.js b/packages/core/logs/serialize.test.js index d2aad21e3..5fae77f2c 100644 --- a/packages/core/logs/serialize.test.js +++ b/packages/core/logs/serialize.test.js @@ -51,6 +51,44 @@ describe('serializeError', () => { expect(serializeError(bare).type).toBe('Error'); }); + it('adds retryAt as ISO and reason for a rate-limited error with a valid retryAt', () => { + const err = Object.assign(new Error('GET https://h.example 429'), { + statusCode: 429, + isRateLimited: true, + retryAt: new Date('2026-09-28T12:00:05.000Z'), + reason: 'daily', + }); + expect(serializeError(err)).toMatchObject({ + status: 429, + retryAt: '2026-09-28T12:00:05.000Z', + reason: 'daily', + }); + }); + + it.each([ + [ + 'is not rate limited', + { retryAt: new Date('2026-09-28T12:00:05.000Z'), reason: 'daily' }, + ], + ['has no retryAt', { isRateLimited: true, reason: 'daily' }], + [ + 'has an invalid retryAt', + { isRateLimited: true, retryAt: new Date(NaN), reason: 'daily' }, + ], + [ + 'has a retryAt that is not a Date', + { + isRateLimited: true, + retryAt: '2026-09-28T12:00:05.000Z', + reason: 'daily', + }, + ], + ])('adds neither retryAt nor reason when the error %s', (_label, fields) => { + const out = serializeError(Object.assign(new Error('x'), fields)); + expect(out).not.toHaveProperty('retryAt'); + expect(out).not.toHaveProperty('reason'); + }); + it('reads status from statusCode, then status, then response.status', () => { const a = Object.assign(new Error('a'), { statusCode: 401, status: 500 }); const b = Object.assign(new Error('b'), { status: 404 }); From 0889205c42a7bc55a3b9d901700908f925d29b54 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:22:46 -0300 Subject: [PATCH 16/33] fix(core): log a hinted wait at TRACE and leave the throw to the boundary The Requester wrote an INFO rate_limited record for a wait and for the RateLimitError it throws. Backoff is step detail (TRACE in the level table), and the logging standard says to log an error one time, at the boundary that decides its outcome, and not to log and rethrow. The wait record is now TRACE, the throw writes none, and the action field, left with one value, is gone. The DEBUG request_failed record before the throw stays, as on the other 4xx paths. The boundary record now carries retryAt and reason in its error field. Co-Authored-By: Claude Opus 5.5 --- docs/guides/LOGGING.md | 13 +++---- packages/core/modules/requester/requester.js | 18 ++++------ .../core/modules/requester/requester.test.js | 35 +++++++++---------- 3 files changed, 29 insertions(+), 37 deletions(-) diff --git a/docs/guides/LOGGING.md b/docs/guides/LOGGING.md index 7ef2283e6..abc7337a7 100644 --- a/docs/guides/LOGGING.md +++ b/docs/guides/LOGGING.md @@ -162,12 +162,13 @@ the app deploys defines the schema. layers throw with `cause` and do not log. `createHandler`, the express error middleware, `Worker.run` and the DLQ processor are the boundaries. -The `Requester` writes one `INFO` record, `module..rate_limited`, for -each wait that a provider or a module policy set, and for each rate-limit error -it throws. It carries `statusCode`, `waitMs`, `retryAt`, `reason`, `hintSource`, -`attempt`, `waitedMs` and `action` (`wait` or `throw`), because the serialized -error keeps no `retryAt`. The plain backoff ladder writes none. A `classify()` -that throws writes one `WARN`, `module..rate_limit_classify_failed`. +The `Requester` writes one `TRACE` record, `module..rate_limited`, for +each wait that a provider or a module policy set. It carries `statusCode`, +`waitMs`, `retryAt`, `reason`, `hintSource`, `attempt` and `waitedMs`. The +plain backoff ladder writes none. The `RateLimitError` it throws gets no record +of its own: the boundary logs it, and its `error` field carries `retryAt` and +`reason`. A `classify()` that throws writes one `WARN`, +`module..rate_limit_classify_failed`. `FRIGG_LOG_LEVEL` sets the minimum level. The logger drops a record below it before it reads any field. diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index e1f6fbc9a..b1a674f03 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -523,15 +523,10 @@ class Requester extends Delegate { budgetMs, random: this._random, }); - this._logRateLimited({ - status, - hint, - waitMs, - attempt, - waitedMs, - action: fits ? 'wait' : 'throw', - }); - if (fits) return waitMs; + if (fits) { + this._logRateLimited({ status, hint, waitMs, attempt, waitedMs }); + return waitMs; + } this._logRequestFailed(encodedUrl, options, status); const rateLimitError = await RateLimitError.create({ @@ -591,9 +586,9 @@ class Requester extends Delegate { } } - _logRateLimited({ status, hint, waitMs, attempt, waitedMs, action }) { + _logRateLimited({ status, hint, waitMs, attempt, waitedMs }) { const logger = this.logger; - logger.info('Rate limited', { + logger.trace('Rate limited', { eventName: `${logger.name}.rate_limited`, statusCode: status, waitMs, @@ -602,7 +597,6 @@ class Requester extends Delegate { hintSource: hint.source, attempt, waitedMs, - action, }); } diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index 33cc9ba80..c8cebe1ad 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -679,21 +679,19 @@ describe('Requester', () => { }); describe('logging', () => { - it('writes one INFO record for a wait that a header set', async () => { + it('writes one TRACE record for a wait that a header set', async () => { const fetch = throttledFetch([ limited({ 'Retry-After': '5' }), ok, ]); await makeRequester(fetch)._get({ url }); - expect( - sink.records.filter( - (r) => - r.eventName === 'module.TestRequester.rate_limited' - ) - ).toEqual([ + const records = sink.records.filter( + (r) => r.eventName === 'module.TestRequester.rate_limited' + ); + expect(records).toEqual([ expect.objectContaining({ - level: 'INFO', + level: 'TRACE', logger: 'module.TestRequester', statusCode: 429, waitMs: 5_000, @@ -701,27 +699,26 @@ describe('Requester', () => { hintSource: 'header', attempt: 0, waitedMs: 0, - action: 'wait', retryAt: expect.any(String), }), ]); + expect(records[0]).not.toHaveProperty('action'); }); - it('says throw when the wait does not fit', async () => { + it('writes no rate-limit record for the error it throws', async () => { const fetch = throttledFetch([ limited({ 'Retry-After': '400' }), ]); - await makeRequester(fetch) + const error = await makeRequester(fetch) ._get({ url }) - .catch(() => {}); + .catch((e) => e); - const [record] = sink.records.filter( - (r) => r.eventName === 'module.TestRequester.rate_limited' - ); - expect(record).toMatchObject({ - action: 'throw', - waitMs: 400_000, - }); + expect(error).toBeInstanceOf(RateLimitError); + expect( + sink.records.filter((r) => + r.eventName?.endsWith('.rate_limited') + ) + ).toEqual([]); }); }); }); From 7bbd1f28dce500947f5c4f2505fc189e4f3eb35b Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:28:22 -0300 Subject: [PATCH 17/33] fix(core): count a classify() result only when it names a limit normalizeClassified accepted any object. An async classify (a Promise), {} or { reason: undefined } made every 4xx and 5xx except 401 a rate limit: a 404 then ran the whole ladder (about 284 s) and ended flagged isRateLimited. A result now counts only when it has a known reason, a finite waitMs or a valid retryAt; anything else is null. A thenable result is a classify error: it goes to onClassifyError (the WARN rate_limit_classify_failed in the Requester) as a TypeError and counts as null, or propagates when nobody listens. Its rejection is handled, so a rejecting async classify cannot end the process with an unhandled rejection. Co-Authored-By: Claude Opus 5.5 --- docs/guides/LOGGING.md | 2 +- .../api-module-definition-and-functions.md | 5 +- .../modules/requester/rate-limit/policy.js | 24 +++-- .../requester/rate-limit/policy.test.js | 102 ++++++++++++++++++ .../core/modules/requester/requester.test.js | 33 ++++++ 5 files changed, 158 insertions(+), 8 deletions(-) diff --git a/docs/guides/LOGGING.md b/docs/guides/LOGGING.md index abc7337a7..92367ec55 100644 --- a/docs/guides/LOGGING.md +++ b/docs/guides/LOGGING.md @@ -167,7 +167,7 @@ each wait that a provider or a module policy set. It carries `statusCode`, `waitMs`, `retryAt`, `reason`, `hintSource`, `attempt` and `waitedMs`. The plain backoff ladder writes none. The `RateLimitError` it throws gets no record of its own: the boundary logs it, and its `error` field carries `retryAt` and -`reason`. A `classify()` that throws writes one `WARN`, +`reason`. A `classify()` that throws or returns a Promise writes one `WARN`, `module..rate_limit_classify_failed`. `FRIGG_LOG_LEVEL` sets the minimum level. The logger drops a record below it diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index ab1b6690a..421d05c19 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -269,7 +269,10 @@ case of the media type, or `undefined` for any other response. It returns | `source` | `'header'`, `'body'` (default) or `'static'` | A status other than 429 is a limit only when `classify` returns a hint for it. -A `classify` that throws is logged (`rate_limit_classify_failed`) and skipped. +A result counts only when it has a known `reason`, a finite `waitMs` or a valid +`retryAt`; anything else, `{}` for example, counts as `null`. `classify` must +be synchronous. A `classify` that throws or returns a Promise is logged +(`rate_limit_classify_failed`) and counts as `null`. ```javascript class Api extends OAuth2Requester { diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index 0ee4fb628..c5b5db788 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -25,6 +25,7 @@ const normalizedPolicies = new WeakSet(); const policiesByClass = new WeakMap(); const isObject = (value) => value !== null && typeof value === 'object'; +const isThenable = (value) => typeof value?.then === 'function'; function isPlainObject(value) { if (!isObject(value) || Array.isArray(value)) return false; @@ -120,8 +121,18 @@ function computeScopeKey(policy, requester) { return undefined; } +function toDate(value) { + if (value === undefined || value === null) return null; + const date = value instanceof Date ? value : new Date(value); + return Number.isNaN(date.getTime()) ? null : date; +} + function normalizeClassified(raw, now) { if (!isObject(raw)) return null; + const retryAt = toDate(raw.retryAt); + if (!REASONS.has(raw.reason) && !Number.isFinite(raw.waitMs) && !retryAt) { + return null; + } const extra = { reason: REASONS.has(raw.reason) ? raw.reason : 'unknown', @@ -130,12 +141,7 @@ function normalizeClassified(raw, now) { source: CLASSIFY_SOURCES.has(raw.source) ? raw.source : 'body', }; - let hint = null; - if (raw.retryAt !== undefined && raw.retryAt !== null) { - const date = - raw.retryAt instanceof Date ? raw.retryAt : new Date(raw.retryAt); - hint = hintFromRetryAt(date, now, extra); - } + let hint = retryAt ? hintFromRetryAt(retryAt, now, extra) : null; if (!hint && Number.isFinite(raw.waitMs)) { hint = hintFromWait(raw.waitMs, now, extra); } @@ -156,6 +162,12 @@ function runClassify(policy, signal, now, onClassifyError) { headers: signal.headers, body: signal.body, }); + if (isThenable(raw)) { + Promise.resolve(raw).catch(() => {}); + throw new TypeError( + 'classify() must return a hint or null, not a Promise' + ); + } } catch (error) { if (!onClassifyError) throw error; onClassifyError(error); diff --git a/packages/core/modules/requester/rate-limit/policy.test.js b/packages/core/modules/requester/rate-limit/policy.test.js index fb8767748..ccc448b2b 100644 --- a/packages/core/modules/requester/rate-limit/policy.test.js +++ b/packages/core/modules/requester/rate-limit/policy.test.js @@ -492,6 +492,108 @@ describe('rate-limit/policy', () => { expect(hint.waitMs).toBe(4_000); }); + it.each([ + ['an empty object', {}], + ['an undefined reason', { reason: undefined }], + ['a reason it does not know', { reason: 'hourly' }], + ['only a policy name', { policy: 'DAILY' }], + ['a retryAt that is not a date', { retryAt: 'soon' }], + ['a waitMs that is not a number', { waitMs: '5000' }], + ])('treats %s from classify as no limit', (_label, result) => { + const policy = { classify: () => result }; + expect( + classifyRateLimit( + policy, + { status: 404, headers: { 'retry-after': '5' } }, + options + ) + ).toBeNull(); + expect( + resolveRateLimitHint( + policy, + { status: 404, headers: {} }, + { ...options, attempt: 0, backOff: [1] } + ) + ).toBeNull(); + }); + + it.each([ + ['a finite waitMs', { waitMs: 2_000 }, 2_000], + ['a valid retryAt', { retryAt: new Date(NOW + 3_000) }, 3_000], + ])( + 'counts a classify result with %s and no reason as a limit', + (_label, result, waitMs) => { + expect( + classifyRateLimit( + { classify: () => result }, + { status: 404, headers: {} }, + options + ) + ).toMatchObject({ waitMs, reason: 'unknown', source: 'body' }); + } + ); + + it('reports a classify that returns a Promise and goes on with the parsers', () => { + const onClassifyError = jest.fn(); + const policy = { + classify: async () => ({ reason: 'daily', waitMs: 60_000 }), + }; + const hint = classifyRateLimit( + policy, + { status: 429, headers: { 'retry-after': '4' } }, + { ...options, onClassifyError } + ); + expect(onClassifyError).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'TypeError', + message: + 'classify() must return a hint or null, not a Promise', + }) + ); + expect(hint).toMatchObject({ + waitMs: 4_000, + reason: 'unknown', + source: 'header', + }); + }); + + it('treats a classify that returns a Promise as no limit for a status other than 429', () => { + const policy = { classify: async () => ({ reason: 'daily' }) }; + const onClassifyError = jest.fn(); + expect( + classifyRateLimit( + policy, + { status: 404, headers: {} }, + { ...options, onClassifyError } + ) + ).toBeNull(); + expect(onClassifyError).toHaveBeenCalledTimes(1); + }); + + it('lets the Promise error propagate when nobody listens', () => { + expect(() => + classifyRateLimit( + { classify: async () => null }, + { status: 429, headers: {} }, + options + ) + ).toThrow(TypeError); + }); + + it('handles the rejection of a Promise that classify returns', async () => { + const then = jest.fn(); + classifyRateLimit( + { classify: () => ({ then }) }, + { status: 429, headers: {} }, + { ...options, onClassifyError: () => {} } + ); + await Promise.resolve(); + expect(then).toHaveBeenCalledWith( + expect.any(Function), + expect.any(Function) + ); + }); + it('accepts the policy object a module declares, without reading it first', () => { const declared = { parsers: ['retryAfter'], diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index c8cebe1ad..c8fff8e1a 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -634,6 +634,39 @@ describe('Requester', () => { expect(classify.mock.calls[0][0].body).toBeUndefined(); }); + it('does not retry or flag a 404 when classify returns a Promise', async () => { + const Api = withPolicy({ + classify: async () => ({ reason: 'daily' }), + }); + const fetch = throttledFetch([{ status: 404 }, ok]); + + const error = await makeRequester(fetch, {}, Api) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(FetchError); + expect(error.statusCode).toBe(404); + expect(error.isRateLimited).toBeUndefined(); + expect(fetch).toHaveBeenCalledTimes(1); + expect(delays).toEqual([]); + expect( + sink.records.filter( + (r) => + r.eventName === + 'module.PolicyRequester.rate_limit_classify_failed' + ) + ).toEqual([ + expect.objectContaining({ + level: 'WARN', + statusCode: 404, + error: expect.objectContaining({ + message: + 'classify() must return a hint or null, not a Promise', + }), + }), + ]); + }); + it('does not call classify for a success or for a 401', async () => { const classify = jest.fn(() => null); const Api = withPolicy({ classify }); From 9ff5866635833fb1013e694733c621e36309a4d8 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:32:48 -0300 Subject: [PATCH 18/33] fix(core): parse the classify body as JSON whatever its media type _readBodyForClassify parsed only JSON media types, so JSON sent as text/plain or application/x-amz-json-1.1 reached classify as undefined, and a 403 limit in such a body was missed and halted. The Requester now always tries JSON.parse on the text it read: on failure the body is undefined and the text stays on the error. isJsonMediaType and the headerValue import are gone. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 7 +++-- packages/core/modules/requester/requester.js | 16 ----------- .../core/modules/requester/requester.test.js | 27 ++++++++++++++++--- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 421d05c19..72d61b0cf 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -255,10 +255,9 @@ Each returns a hint or `null`. ### What `classify` returns `classify({ status, headers, body })` runs for a 429, and for any other 4xx or -5xx except 401. `body` is the parsed body of an `application/json`, `text/json` -or `+json` response (`application/problem+json`, for example), whatever the -case of the media type, or `undefined` for any other response. It returns -`null` when the response is not a limit, or a hint: +5xx except 401. `body` is the response text parsed as JSON, whatever the media +type, or `undefined` when the text is not JSON (an HTML error page, for +example). It returns `null` when the response is not a limit, or a hint: | Field | Meaning | |---|---| diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index b1a674f03..572029676 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -12,7 +12,6 @@ const { getLoggerScope } = require('../../logs/context'); const { computeScopeKey, computeWaitMs, - headerValue, inProcessBudgetMs, readRateLimitPolicy, resolveRateLimitHint, @@ -20,18 +19,6 @@ const { const DEFAULT_REQUEST_TIMEOUT_MS = 60_000; -function isJsonMediaType(contentType) { - const mediaType = String(contentType ?? '') - .split(';')[0] - .trim() - .toLowerCase(); - return ( - mediaType === 'application/json' || - mediaType === 'text/json' || - mediaType.endsWith('+json') - ); -} - // A node-fetch error message holds the raw URL, and util.inspect prints the // cause chain, so the FetchError keeps only a sanitized copy. function sanitizedCause(err) { @@ -576,9 +563,6 @@ class Requester extends Delegate { return {}; } const text = await response.text(); - if (!isJsonMediaType(headerValue(response.headers, 'content-type'))) { - return { text }; - } try { return { text, json: JSON.parse(text) }; } catch { diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index c8fff8e1a..b8729658f 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -578,8 +578,8 @@ describe('Requester', () => { } ); - it.each(['text/plain', 'application/xml', 'application/jsonp'])( - 'hands classify no body for a %s response', + it.each(['text/plain', 'application/x-amz-json-1.1'])( + 'hands classify the parsed body of JSON sent as %s', async (contentType) => { const classify = jest.fn(() => ({ reason: 'burst', @@ -593,10 +593,31 @@ describe('Requester', () => { await makeRequester(fetch, {}, Api)._get({ url }); - expect(classify.mock.calls[0][0].body).toBeUndefined(); + expect(classify.mock.calls[0][0].body).toEqual({ + code: 'LIMIT', + }); } ); + it('hands classify an undefined body for an HTML error page', async () => { + const classify = jest.fn(() => null); + const Api = withPolicy({ classify }); + const fetch = throttledFetch([ + { + status: 403, + contentType: 'text/html', + text: 'Forbidden', + }, + ]); + + const error = await makeRequester(fetch, {}, Api) + ._get({ url }) + .catch((e) => e); + + expect(classify.mock.calls[0][0].body).toBeUndefined(); + expect(error.body).toBe('Forbidden'); + }); + it('takes the time of a reason-only classify from Retry-After', async () => { const Api = withPolicy({ classify: () => ({ reason: 'burst' }), From 4bb5df46f69041abf2e1904b2668ac1c7de2cc42 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:34:21 -0300 Subject: [PATCH 19/33] fix(core): never wait in process for a hint with an empty backOff A caller that sets backOff: [] wants no retries, but a hinted 429 still slept and retried, for up to 5 minutes. With an empty backOff the in-process budget is now 0: a real hint throws RateLimitError at once, so the queue worker can still defer it, and a 429 with no hint throws the plain FetchError as before. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 3 + packages/core/modules/requester/requester.js | 14 +++-- .../core/modules/requester/requester.test.js | 56 +++++++++++++++++++ 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 72d61b0cf..8e7876a60 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -313,6 +313,9 @@ class Api extends ApiKeyRequester { `statusCode` stays, and adds `isRateLimited`, `retryAt`, `waitMs`, `reason`, `policy`, `source`, `module` and `scopeKey`. +A Requester built with `backOff: []` never sleeps: a hinted wait throws +`RateLimitError` at once, and a 429 with no hint throws a plain `FetchError`. + `Retry-After` is read on a 429 only. The queue worker does not halt a `RateLimitError`, even when its status is 403: the message goes back to SQS. diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index 572029676..8b67f62df 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -498,12 +498,14 @@ class Requester extends Delegate { return null; } const { hint } = throttle; - const budgetMs = inProcessBudgetMs({ - policy: this._rateLimitPolicy, - requestTimeoutMs: this.requestTimeoutMs, - remainingMs: remainingInvocationMs(), - waitedMs, - }); + const budgetMs = this.backOff.length + ? inProcessBudgetMs({ + policy: this._rateLimitPolicy, + requestTimeoutMs: this.requestTimeoutMs, + remainingMs: remainingInvocationMs(), + waitedMs, + }) + : 0; const { waitMs, fits } = computeWaitMs({ hint, policy: this._rateLimitPolicy, diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index b8729658f..256841efc 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -400,6 +400,62 @@ describe('Requester', () => { }); }); + describe('with an empty backOff', () => { + it('throws a RateLimitError at once for a Retry-After', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '2' }), + ok, + ]); + + const error = await makeRequester(fetch, { backOff: [] }) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(error).toMatchObject({ + statusCode: 429, + source: 'header', + }); + expect(fetch).toHaveBeenCalledTimes(1); + expect(delays).toEqual([]); + }); + + it('throws a RateLimitError at once for a hint that classify gives', async () => { + const Api = withPolicy({ + classify: () => ({ reason: 'daily', waitMs: 2_000 }), + }); + const fetch = throttledFetch([ + { status: 403, body: { code: 'LIMIT' } }, + ok, + ]); + + const error = await makeRequester(fetch, { backOff: [] }, Api) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(error).toMatchObject({ + statusCode: 403, + reason: 'daily', + }); + expect(fetch).toHaveBeenCalledTimes(1); + expect(delays).toEqual([]); + }); + + it('still throws a plain FetchError for a 429 without a hint', async () => { + const fetch = throttledFetch([limited(), ok]); + + const error = await makeRequester(fetch, { backOff: [] }) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(FetchError); + expect(error).not.toBeInstanceOf(RateLimitError); + expect(error.statusCode).toBe(429); + expect(fetch).toHaveBeenCalledTimes(1); + }); + }); + describe('static rateLimit', () => { it('makes minRetryAfterMs the wait of a bare 429', async () => { const Api = withPolicy({ minRetryAfterMs: 60_000 }); From bf3847f8519b324ef20ba7d207f8d60801864bb5 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:35:26 -0300 Subject: [PATCH 20/33] fix(core): read the response stream before options.body in FetchError.create At base the stream won over options.body, an alias of the response body, and options.responseBody ?? options.body was only the fallback for an empty stream. The rate-limit change let options.body pre-empt the stream read. An explicit options.responseBody still skips the read (the Requester read the body for classify already); options.body is again used only when the stream gives nothing. No caller in the repository passes options.body. Co-Authored-By: Claude Opus 5.5 --- packages/core/errors/fetch-error.js | 16 +++++++--------- packages/core/errors/fetch-error.test.js | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/packages/core/errors/fetch-error.js b/packages/core/errors/fetch-error.js index ea74b3a30..b3b139109 100644 --- a/packages/core/errors/fetch-error.js +++ b/packages/core/errors/fetch-error.js @@ -55,15 +55,13 @@ class FetchError extends BaseError { static async create(options = {}) { const { response } = options; - const provided = options.responseBody ?? options.body; - let responseBody = provided; - if ( - responseBody === undefined && - response && - !response.bodyUsed && - typeof response.text === 'function' - ) { - responseBody = (await response.text()) || provided; + let { responseBody } = options; + if (responseBody === undefined || responseBody === null) { + const canRead = + response && + !response.bodyUsed && + typeof response.text === 'function'; + responseBody = (canRead && (await response.text())) || options.body; } const ErrorClass = typeof this === 'function' ? this : FetchError; return new ErrorClass({ ...options, responseBody }); diff --git a/packages/core/errors/fetch-error.test.js b/packages/core/errors/fetch-error.test.js index f24cff891..b428cd372 100644 --- a/packages/core/errors/fetch-error.test.js +++ b/packages/core/errors/fetch-error.test.js @@ -192,6 +192,24 @@ describe('FetchError', () => { expect(error.body).toBe('fallback'); }); + it('create() reads the stream before options.body', async () => { + const text = jest.fn(async () => 'from the stream'); + const error = await FetchError.create({ + response: { status: 400, bodyUsed: false, text }, + body: 'fallback', + }); + expect(text).toHaveBeenCalledTimes(1); + expect(error.body).toBe('from the stream'); + }); + + it('create() falls back to options.body when the stream gives nothing', async () => { + const error = await FetchError.create({ + response: { status: 400, bodyUsed: false, text: async () => '' }, + body: 'fallback', + }); + expect(error.body).toBe('fallback'); + }); + it('create() uses a given responseBody and does not read the stream again', async () => { const text = jest.fn(async () => 'second read'); const error = await FetchError.create({ From 9e61ffe6074211ea053962fb67c0fdcb032c44da Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:35:57 -0300 Subject: [PATCH 21/33] test(core): cover a reason-only classify with a short Retry-After The plan asks for a reason-only classify plus Retry-After: 7 to sleep 7000 ms with the reason from classify. The existing test used Retry-After: 400 and checked only the throw. The new test checks the delay and the reason on the rate_limited record. Co-Authored-By: Claude Opus 5.5 --- .../core/modules/requester/requester.test.js | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index 256841efc..2a63a3e0c 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -674,6 +674,36 @@ describe('Requester', () => { expect(error.body).toBe('Forbidden'); }); + it('sleeps the Retry-After of a reason-only classify and keeps its reason', async () => { + const Api = withPolicy({ + classify: () => ({ reason: 'burst' }), + }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '7' }), + ok, + ]); + + const result = await makeRequester(fetch, {}, Api)._get({ + url, + }); + + expect(result).toEqual({ ok: true }); + expect(delays).toEqual([7_000]); + expect( + sink.records.filter( + (r) => + r.eventName === + 'module.PolicyRequester.rate_limited' + ) + ).toEqual([ + expect.objectContaining({ + waitMs: 7_000, + reason: 'burst', + hintSource: 'header', + }), + ]); + }); + it('takes the time of a reason-only classify from Retry-After', async () => { const Api = withPolicy({ classify: () => ({ reason: 'burst' }), From b4e8789d50e27d9595a96b767bd361391698b4b1 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:36:57 -0300 Subject: [PATCH 22/33] fix(core): type the random option of the Requester constructor The Requester reads params.random as its source of jitter, so tests can make a wait exact without mocking Math.random. RequesterConstructor now declares it (random?: () => number); ApiKeyRequester and BasicAuthRequester take it through that type. Co-Authored-By: Claude Opus 5.5 --- packages/core/types/module-plugin/index.d.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/types/module-plugin/index.d.ts b/packages/core/types/module-plugin/index.d.ts index f17ace051..a9e6e6fd5 100644 --- a/packages/core/types/module-plugin/index.d.ts +++ b/packages/core/types/module-plugin/index.d.ts @@ -130,6 +130,7 @@ declare module "@friggframework/module-plugin" { type RequesterConstructor = { backOff?: number[]; fetch?: any; + random?: () => number; }; export class ApiKeyRequester From fcaf1fa69827d3390771876a1b271af1543d7955 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:37:51 -0300 Subject: [PATCH 23/33] docs(core): say which untimed throttles carry isRateLimited The reference said every classified response with no time ends as a FetchError flagged isRateLimited. Only one whose status is not 429 does; a 429 with no time ends as a plain FetchError, as before, and the queue worker never halts a 429. It also said Retry-After is read on a 429 only: the parsers read the headers of a 429 and of any response that classify recognises. And the static step takes the larger of minRetryAfterMs and the perMs of the window the reason names, not one or the other. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 8e7876a60..d3e9c1d3b 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -224,8 +224,8 @@ apiPropertiesToPersist: { credential: ['signing_key', 'webhook_secret'] } An API module can tell the Requester how its provider limits calls, and how to read a throttled response. Declare a static `rateLimit` on the API class. Every key is optional. A module that declares nothing keeps the fixed backoff ladder -for a 429 (1, 3, 10, 30, 60 and 180 s), and reads `Retry-After` when the -response has one. +for a 429 (1, 3, 10, 30, 60 and 180 s), and reads `Retry-After` when a 429 has +one. | Key | Meaning | |---|---| @@ -234,7 +234,7 @@ response has one. | `maxInProcessWaitMs` | The most one request sleeps in process, in total. Default `300000` (5 minutes). `0` means never sleep: every hinted wait throws `RateLimitError`. | | `parsers` | The header parsers to read, in order: `retryAfter`, `resetHeaders`, `ietf`. Default: all three. A name that is not built in throws a `TypeError` when the API class is constructed. | | `classify` | Recognises a limit that is not a plain 429, or names its reason. See below. | -| `windows`, `maxConcurrency` | The provider's limits, as documentation. A window named like the `reason` (`{ name: 'burst', perMs: 10000 }`) sets the wait when no header does. Pacing uses them later. | +| `windows`, `maxConcurrency` | The provider's limits, as documentation. A window named like the `reason` (`{ name: 'burst', perMs: 10000 }`) sets the wait when no header does, unless `minRetryAfterMs` is longer. Pacing uses them later. | | `userHints` | Links per `reason` that a UI can show, for example `{ daily: { links: [{ label, url }] } }`. | ### Built-in parsers @@ -262,7 +262,7 @@ example). It returns `null` when the response is not a limit, or a hint: | Field | Meaning | |---|---| | `reason` | `'burst'`, `'daily'`, `'monthly'`, `'concurrency'` or `'unknown'` | -| `waitMs` or `retryAt` | When to call again. Leave both out to take the time from the parsers, then from the policy. With no time from either, the response follows the same ladder as a 429 with no hint, and the last error is a `FetchError` with `isRateLimited: true` and the `reason`. The queue worker does not halt it. | +| `waitMs` or `retryAt` | When to call again. Leave both out to take the time from the parsers, then from the policy. Step 2 of [How the Requester waits](#how-the-requester-waits) says what happens when neither gives one. | | `policy` | The provider's own name for the limit, for logs and the UI | | `remaining` | Calls left in the window, when the response says | | `source` | `'header'`, `'body'` (default) or `'static'` | @@ -300,9 +300,15 @@ class Api extends ApiKeyRequester { ### How the Requester waits 1. It looks for a hint in this order: `classify()`, the parsers, the policy - (`minRetryAfterMs`, or the `perMs` of the window named by the reason). + (the larger of `minRetryAfterMs` and the `perMs` of the window named by the + reason). 2. With no hint, a 429 keeps the fixed ladder: the same calls, the same delays, - then a plain `FetchError`. A 5xx keeps its ladder too. + then a plain `FetchError`, as before. A response that `classify` recognises + but that gets no time follows the same ladder. When its status is not 429, + the last error is a `FetchError` flagged `isRateLimited`, with the `reason`, + so the queue worker does not halt it. A 429 needs no flag: the queue worker + never halts a 429. A 5xx that `classify` does not recognise keeps the 5xx + ladder. 3. With a hint, the wait is the largest of the hint, `minRetryAfterMs` and 1 s, plus at most 10 % jitter. The Requester sleeps when the wait fits the time this request may still sleep: `maxInProcessWaitMs` in total, and the time left @@ -316,8 +322,11 @@ class Api extends ApiKeyRequester { A Requester built with `backOff: []` never sleeps: a hinted wait throws `RateLimitError` at once, and a 429 with no hint throws a plain `FetchError`. -`Retry-After` is read on a 429 only. The queue worker does not halt a -`RateLimitError`, even when its status is 403: the message goes back to SQS. +The parsers, `Retry-After` among them, read the headers of a 429 and of a +response that `classify` recognises, and of no other response: a 503 with +`Retry-After` keeps the 5xx ladder unless `classify` recognises it. The queue +worker does not halt a `RateLimitError`, even when its status is 403: the +message goes back to SQS. ### A client that is not the Requester From 0d299d41f93a6a6f0e7d49057da89753fd88aedd Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:42:23 -0300 Subject: [PATCH 24/33] docs(core): show a module that throttles on a 2xx body The Requester runs classify only on a 4xx or 5xx, because a generic retry of a "throttled 200" could send a POST twice. The reference now shows the pattern for a provider that answers 200 with a limit error, such as a GraphQL THROTTLED error: the module wraps its own call, calls the exported classifyRateLimit(policy, { status, headers, body }) and throws new RateLimitError({ hint, module, cause, resource }). Both examples now name the module with this.delegate?.name: an API instance has no name of its own; its delegate, the Module, has the moduleName. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index d3e9c1d3b..c55ef5f0a 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -346,7 +346,7 @@ async withLimits(call) { body: { errorCode: err.errorCode }, }); if (!hint) throw err; - throw new RateLimitError({ hint, module: this.name, cause: err }); + throw new RateLimitError({ hint, module: this.delegate?.name, cause: err }); } } ``` @@ -363,6 +363,48 @@ takes the `FetchError` arguments (`resource`, `init`, `response`, `cause`, `responseBody`). It sets `retryAt` to now plus `waitMs`, or to `hint.retryAt` when `waitMs` is left out. +### A limit in a 2xx body + +The Requester calls `classify` only for a 4xx or 5xx: a generic retry of a +"throttled 200" could send a POST twice. A provider that answers 200 with a +limit error (a GraphQL `THROTTLED` error, for example) needs the module to +wrap its own call, classify the body and throw: + +```javascript +const { classifyRateLimit, RateLimitError } = require('@friggframework/core'); + +class Api extends OAuth2Requester { + static rateLimit = { + classify({ body }) { + const throttled = body?.errors?.some( + (error) => error.extensions?.code === 'THROTTLED' + ); + return throttled ? { reason: 'burst', waitMs: 2_000 } : null; + }, + }; + + async query(document, variables) { + const url = `${this.baseUrl}/graphql`; + const response = await this._post({ + url, + body: { query: document, variables }, + returnFullRes: true, + }); + const body = await response.json(); + if (!body.errors?.length) return body.data; + + const cause = new Error(body.errors[0].message); + const hint = classifyRateLimit(Api.rateLimit, { + status: response.status, + headers: response.headers, + body, + }); + if (!hint) throw cause; + throw new RateLimitError({ hint, module: this.delegate?.name, cause, resource: url }); + } +} +``` + ### The invocation deadline `runInvocationScope` stores the end of every Lambda invocation, from From ffedaa4b9526f8e2b7c747643768f96bea3ad8f1 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:42:37 -0300 Subject: [PATCH 25/33] docs(core): check isLastAttempt before rethrowing a RateLimitError The guide told a queue handler to let a RateLimitError reach its caller. The contract (ADR-049 section 3) is that the handler checks delivery.isLastAttempt first, ends the run on the last attempt, and then rethrows. Co-Authored-By: Claude Opus 5.5 --- docs/guides/INTEGRATION-PATTERNS.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/guides/INTEGRATION-PATTERNS.md b/docs/guides/INTEGRATION-PATTERNS.md index aab7fe206..d1a0e102f 100644 --- a/docs/guides/INTEGRATION-PATTERNS.md +++ b/docs/guides/INTEGRATION-PATTERNS.md @@ -800,9 +800,11 @@ Put what you know about a provider's limits in its API module: declare `static rateLimit` (see the [API module reference](../reference/api-module-definition-and-functions.md#rate-limits-static-ratelimit)). The Requester then waits as the provider says. When the wait is too long to -sleep, it throws `RateLimitError` with `retryAt`. Let it reach the queue -handler's caller: the queue worker does not halt it and SQS redelivers the -message. Do not catch it to write a retry of your own. +sleep, it throws `RateLimitError` with `retryAt`. A queue handler checks +`delivery.isLastAttempt` first (see [Queue Handler Delivery](#queue-handler-delivery)) +and ends the run on the last attempt, then rethrows: the queue worker does not +halt a `RateLimitError`, and SQS redelivers the message. Do not catch it to +write a retry of your own. For pacing in your own loop, respect API rate limits: From 5f25a0f9e51d9c4e3e0b4b82aeae0e92007e5a92 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:46:19 -0300 Subject: [PATCH 26/33] docs(core): cut the core CLAUDE.md rate-limit block to a summary The 45-line block repeated the API module reference, and had drifted from it. It is now a six-line summary that links to the Rate Limits section of docs/reference/api-module-definition-and-functions.md. Co-Authored-By: Claude Opus 5.5 --- packages/core/CLAUDE.md | 48 +++++------------------------------------ 1 file changed, 5 insertions(+), 43 deletions(-) diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index c22885a45..330bc01c7 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -370,49 +370,11 @@ class MyIntegration extends IntegrationBase { - `BasicAuthRequester` - Basic authentication **Rate limits** (ADR-049): an API module declares `static rateLimit` on its -`Requester` subclass. For a throttled response the Requester looks for a hint -in this order: - -| Step | Source | -|---|---| -| 1 | `classify({ status, headers, body })` of the module. It recognises a limit that is not a plain 429 (a 403 limit code, a `policyName` in the body) and may name only a `reason` | -| 2 | The header parsers in `parsers` order. Default: `retryAfter`, `resetHeaders`, `ietf` | -| 3 | The static policy: `minRetryAfterMs`, or the `perMs` of the window named by the `reason` | -| 4 | The fixed `backOff` ladder (1, 3, 10, 30, 60, 180 s) | - -- A 429 with no hint from steps 1 to 3 keeps today's ladder: the same calls, - the same delays, then a plain `FetchError`. Nothing budgets it. A response - that `classify` names as a limit but that has no time follows the same - ladder, and the last error is a `FetchError` flagged `isRateLimited` with - the `reason`, so the queue worker does not halt it. -- With a hint, the wait is `max(hint, minRetryAfterMs, 1 s)` plus at most 10 % - jitter. The total sleep of one request is capped at `maxInProcessWaitMs` - (default 5 minutes) and at the time left in the invocation less one request - timeout, a reserve of at most half the time left (`remainingInvocationMs()`). -- A wait that does not fit throws `RateLimitError` (`isRateLimited`, `retryAt`, - `waitMs`, `reason`, `policy`, `source`, `module`, `scopeKey`). The queue - worker does not halt it, whatever its status. -- `Retry-After` is read on a 429 only. Another status counts only when - `classify` recognises it. A 5xx keeps the 5xx ladder. -- `maxInProcessWaitMs: 0` turns off sleeping: every hinted wait throws. -- A module that does not use the Requester (for example one on jsforce) calls - `classifyRateLimit(policy, { status, headers, body })` around its own client - and throws `RateLimitError` itself. - -```javascript -class Api extends OAuth2Requester { - static rateLimit = { - scope: 'entity', - minRetryAfterMs: 1_000, - classify({ status, body }) { - if (status === 403 && body?.code === 'DAILY_LIMIT') { - return { reason: 'daily', waitMs: 60 * 60_000 }; - } - return null; - }, - }; -} -``` +`Requester` subclass. A hinted wait (from `classify()`, the header parsers or +the static policy) sleeps within a budget, or throws `RateLimitError` +(`isRateLimited`, `retryAt`) when it does not fit. A 429 with no hint keeps the +fixed `backOff` ladder. See +[Rate Limits](../../docs/reference/api-module-definition-and-functions.md#rate-limits-static-ratelimit). **Module Factory**: - `ModuleFactory` - Creates and configures API module instances From 4fb07b4e92324bbbf0d19153e2168642facc1182 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Tue, 29 Sep 2026 17:46:35 -0300 Subject: [PATCH 27/33] docs(core): note that the schema's rateLimit is not read at runtime The reference calls the JSON schema canonical, but api-module-definition.schema.json still describes an older config.rateLimit: { requests, period } that no runtime code reads. The Rate Limits section now says so; the schema is unchanged. Co-Authored-By: Claude Opus 5.5 --- docs/reference/api-module-definition-and-functions.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index c55ef5f0a..61c4aee91 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -225,7 +225,8 @@ An API module can tell the Requester how its provider limits calls, and how to read a throttled response. Declare a static `rateLimit` on the API class. Every key is optional. A module that declares nothing keeps the fixed backoff ladder for a 429 (1, 3, 10, 30, 60 and 180 s), and reads `Retry-After` when a 429 has -one. +one. The JSON schema still has an older `config.rateLimit: { requests, period }` +that the runtime does not read: only `static rateLimit` on the API class counts. | Key | Meaning | |---|---| From a9fb1cbd6579140b9a4de41af3378916399de925 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 12:10:36 -0300 Subject: [PATCH 28/33] fix(core): throw RateLimitError at once for a non-429 limit with no time A 403 that classify() names by reason only gets no wait time, so the Requester ran the full backoff ladder before it gave up. It now throws RateLimitError after one call, with retryAt and waitMs left unknown. Removes the post-ladder isRateLimited flag, which nothing reaches now. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 13 ++--- packages/core/CLAUDE.md | 3 +- packages/core/errors/rate-limit-error.js | 9 ++- packages/core/errors/rate-limit-error.test.js | 13 ++++- .../modules/requester/rate-limit/policy.js | 9 +++ .../requester/rate-limit/policy.test.js | 35 +++++++---- packages/core/modules/requester/requester.js | 34 +++++------ .../core/modules/requester/requester.test.js | 58 +++++++++---------- packages/core/types/errors/index.d.ts | 4 +- 9 files changed, 107 insertions(+), 71 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 61c4aee91..18d868e5c 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -303,13 +303,12 @@ class Api extends ApiKeyRequester { 1. It looks for a hint in this order: `classify()`, the parsers, the policy (the larger of `minRetryAfterMs` and the `perMs` of the window named by the reason). -2. With no hint, a 429 keeps the fixed ladder: the same calls, the same delays, - then a plain `FetchError`, as before. A response that `classify` recognises - but that gets no time follows the same ladder. When its status is not 429, - the last error is a `FetchError` flagged `isRateLimited`, with the `reason`, - so the queue worker does not halt it. A 429 needs no flag: the queue worker - never halts a 429. A 5xx that `classify` does not recognise keeps the 5xx - ladder. +2. With no time, a 429 keeps the fixed ladder: the same calls, the same delays, + then a plain `FetchError`, as before. A response with another status that + `classify` recognises but that gets no time throws `RateLimitError` at once, + after one call, with `retryAt` and `waitMs` `undefined`: the time is unknown, + and a ladder of seconds cannot outwait a daily limit. A 5xx that `classify` + does not recognise keeps the 5xx ladder. 3. With a hint, the wait is the largest of the hint, `minRetryAfterMs` and 1 s, plus at most 10 % jitter. The Requester sleeps when the wait fits the time this request may still sleep: `maxInProcessWaitMs` in total, and the time left diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 330bc01c7..739d284c0 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -373,7 +373,8 @@ class MyIntegration extends IntegrationBase { `Requester` subclass. A hinted wait (from `classify()`, the header parsers or the static policy) sleeps within a budget, or throws `RateLimitError` (`isRateLimited`, `retryAt`) when it does not fit. A 429 with no hint keeps the -fixed `backOff` ladder. See +fixed `backOff` ladder. A non-429 limit that `classify` names without a time +throws `RateLimitError` at once, with `retryAt` unknown. See [Rate Limits](../../docs/reference/api-module-definition-and-functions.md#rate-limits-static-ratelimit). **Module Factory**: diff --git a/packages/core/errors/rate-limit-error.js b/packages/core/errors/rate-limit-error.js index 549d02572..efead3765 100644 --- a/packages/core/errors/rate-limit-error.js +++ b/packages/core/errors/rate-limit-error.js @@ -10,8 +10,13 @@ function resolveTiming({ hint, waitMs, now }) { return { waitMs: Math.max(0, retryAt.getTime() - now), retryAt }; } } - const fallbackWaitMs = hint?.waitMs ?? 0; - return { waitMs: fallbackWaitMs, retryAt: new Date(now + fallbackWaitMs) }; + if (hint?.waitMs !== undefined) { + return { + waitMs: hint.waitMs, + retryAt: new Date(now + hint.waitMs), + }; + } + return { waitMs: undefined, retryAt: undefined }; } class RateLimitError extends FetchError { diff --git a/packages/core/errors/rate-limit-error.test.js b/packages/core/errors/rate-limit-error.test.js index d308dbb74..c163d7fc9 100644 --- a/packages/core/errors/rate-limit-error.test.js +++ b/packages/core/errors/rate-limit-error.test.js @@ -55,7 +55,8 @@ describe('RateLimitError', () => { it('can be built with no arguments', () => { const error = new RateLimitError(); expect(error.isRateLimited).toBe(true); - expect(error.waitMs).toBe(0); + expect(error.waitMs).toBeUndefined(); + expect(error.retryAt).toBeUndefined(); expect(error.reason).toBe('unknown'); expect(error.source).toBe('unknown'); }); @@ -101,6 +102,16 @@ describe('RateLimitError', () => { expect(error.retryAt).toEqual(new Date(NOW + 3_000)); }); + it('leaves retryAt and waitMs unknown for a hint that carries no time', () => { + const error = new RateLimitError({ + hint: { reason: 'daily' }, + now: NOW, + }); + expect(error.retryAt).toBeUndefined(); + expect(error.waitMs).toBeUndefined(); + expect(error.reason).toBe('daily'); + }); + it('reads the clock when no time is given', () => { const before = Date.now(); const error = new RateLimitError({ waitMs: 1_000 }); diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index c5b5db788..9a9b35622 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -241,6 +241,15 @@ function resolveRateLimitHint(policy, signal, options = {}) { }); if (hint) return hint; if (signal.status !== 429 && !classified) return null; + if (signal.status !== 429) { + return { + reason: classified.reason, + ...definedOnly({ + policy: classified.policy, + remaining: classified.remaining, + }), + }; + } const waitMs = attempt < backOff.length ? Number(backOff[attempt]) * 1000 || 0 : 0; diff --git a/packages/core/modules/requester/rate-limit/policy.test.js b/packages/core/modules/requester/rate-limit/policy.test.js index ccc448b2b..3b0b5b8f0 100644 --- a/packages/core/modules/requester/rate-limit/policy.test.js +++ b/packages/core/modules/requester/rate-limit/policy.test.js @@ -652,23 +652,34 @@ describe('rate-limit/policy', () => { ).toBeNull(); }); - it('falls back to the ladder for a status other than 429 that classify named by reason only', () => { + it('returns an untimed hint for a status other than 429 that classify named by reason only', () => { const policy = { - classify: () => ({ reason: 'daily', policy: 'DAILY' }), + classify: () => ({ + reason: 'daily', + policy: 'DAILY', + remaining: 0, + }), }; - expect( - resolveRateLimitHint( - policy, - { status: 403, headers: {} }, - { ...options, attempt: 1, backOff } - ) - ).toEqual({ - waitMs: 3_000, - retryAt: new Date(NOW + 3_000), + const hint = resolveRateLimitHint( + policy, + { status: 403, headers: {} }, + { ...options, attempt: 1, backOff } + ); + expect(hint).toEqual({ reason: 'daily', policy: 'DAILY', - source: 'backoff', + remaining: 0, }); + expect(hint).not.toHaveProperty('waitMs'); + expect(hint).not.toHaveProperty('retryAt'); + expect(hint).not.toHaveProperty('source'); + }); + + it('keeps classifyRateLimit null for a reason-only classify on a status other than 429', () => { + const policy = { classify: () => ({ reason: 'daily' }) }; + expect( + classifyRateLimit(policy, { status: 403, headers: {} }, options) + ).toBeNull(); }); it('waits 0 ms past the end of the ladder', () => { diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index 8b67f62df..c7c5df36c 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -452,10 +452,6 @@ class Requester extends Delegate { response, responseBody: throttle?.responseBody, }); - if (throttle?.throttled && status !== 429) { - fetchError.isRateLimited = true; - fetchError.reason = throttle.hint.reason; - } throw this._maybeFlagTimeoutDuringBodyRead( fetchError, timeoutMs @@ -498,20 +494,24 @@ class Requester extends Delegate { return null; } const { hint } = throttle; - const budgetMs = this.backOff.length - ? inProcessBudgetMs({ + const untimed = hint.waitMs === undefined; + const budgetMs = + this.backOff.length && !untimed + ? inProcessBudgetMs({ + policy: this._rateLimitPolicy, + requestTimeoutMs: this.requestTimeoutMs, + remainingMs: remainingInvocationMs(), + waitedMs, + }) + : 0; + const { waitMs, fits } = untimed + ? { waitMs: undefined, fits: false } + : computeWaitMs({ + hint, policy: this._rateLimitPolicy, - requestTimeoutMs: this.requestTimeoutMs, - remainingMs: remainingInvocationMs(), - waitedMs, - }) - : 0; - const { waitMs, fits } = computeWaitMs({ - hint, - policy: this._rateLimitPolicy, - budgetMs, - random: this._random, - }); + budgetMs, + random: this._random, + }); if (fits) { this._logRateLimited({ status, hint, waitMs, attempt, waitedMs }); return waitMs; diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index 2a63a3e0c..626672a34 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -2,9 +2,7 @@ const { Headers } = require('node-fetch'); const { Requester } = require('./requester'); const { FetchError, RateLimitError } = require('../../errors'); const { createMemorySink } = require('../../logs'); -const { - runWithInvocationDeadline, -} = require('../../core/invocation-deadline'); +const { runWithInvocationDeadline } = require('../../core/invocation-deadline'); /** * Requester is abstract: subclasses provide `addAuthHeaders`. For the @@ -579,32 +577,29 @@ describe('Requester', () => { expect(delays).toEqual([]); }); - it('treats a 403 that classify names by reason only like a bare 429: the ladder, then a flagged FetchError', async () => { + it('throws a RateLimitError at once for a 403 that classify names by reason only', async () => { const Api = withPolicy({ classify: () => ({ reason: 'daily' }), }); const fetch = throttledFetch( - Array(3).fill({ status: 403, body: { code: 'LIMIT' } }) + Array(7).fill({ status: 403, body: { code: 'LIMIT' } }) ); - const error = await makeRequester( - fetch, - { backOff: [0, 0] }, - Api - ) + const error = await makeRequester(fetch, {}, Api) ._get({ url }) .catch((e) => e); - expect(error).toBeInstanceOf(FetchError); - expect(error).not.toBeInstanceOf(RateLimitError); + expect(error).toBeInstanceOf(RateLimitError); expect(error).toMatchObject({ statusCode: 403, isRateLimited: true, reason: 'daily', }); expect(error.retryAt).toBeUndefined(); - expect(fetch).toHaveBeenCalledTimes(3); - expect(delays).toEqual([0, 0]); + expect(error.waitMs).toBeUndefined(); + expect(error.module).toEqual(expect.any(String)); + expect(fetch).toHaveBeenCalledTimes(1); + expect(delays).toEqual([]); }); it.each([ @@ -1035,16 +1030,11 @@ describe('Requester', () => { headers: { get: () => 'application/json' }, json: () => new Promise((_resolve, reject) => { - capturedSignal.addEventListener( - 'abort', - () => { - const err = new Error( - 'aborted mid-body' - ); - err.name = 'AbortError'; - reject(err); - } - ); + capturedSignal.addEventListener('abort', () => { + const err = new Error('aborted mid-body'); + err.name = 'AbortError'; + reject(err); + }); }), }; }); @@ -1238,11 +1228,15 @@ describe('Requester', () => { requester.refreshAuth = jest.fn().mockResolvedValue(true); requester.notify = jest.fn(); - const result = await requester._get({ url: 'https://example.com/protected' }); + const result = await requester._get({ + url: 'https://example.com/protected', + }); expect(result).toEqual({ ok: true }); expect(requester.refreshAuth).toHaveBeenCalledTimes(1); - expect(requester.notify).not.toHaveBeenCalledWith(requester.DLGT_INVALID_AUTH); + expect(requester.notify).not.toHaveBeenCalledWith( + requester.DLGT_INVALID_AUTH + ); expect(fetchMock).toHaveBeenCalledTimes(2); }); @@ -1321,13 +1315,19 @@ describe('Requester', () => { requester.refreshAuth = jest.fn().mockResolvedValue(true); requester.notify = jest.fn(); - const r1 = await requester._get({ url: 'https://example.com/first' }); - const r2 = await requester._get({ url: 'https://example.com/second' }); + const r1 = await requester._get({ + url: 'https://example.com/first', + }); + const r2 = await requester._get({ + url: 'https://example.com/second', + }); expect(r1).toEqual({ ok: 'first' }); expect(r2).toEqual({ ok: 'second' }); expect(requester.refreshAuth).toHaveBeenCalledTimes(2); - expect(requester.notify).not.toHaveBeenCalledWith(requester.DLGT_INVALID_AUTH); + expect(requester.notify).not.toHaveBeenCalledWith( + requester.DLGT_INVALID_AUTH + ); expect(fetchMock).toHaveBeenCalledTimes(4); }); }); diff --git a/packages/core/types/errors/index.d.ts b/packages/core/types/errors/index.d.ts index da2dac51f..f0cc21dad 100644 --- a/packages/core/types/errors/index.d.ts +++ b/packages/core/types/errors/index.d.ts @@ -44,8 +44,8 @@ declare module "@friggframework/errors" { constructor(options?: RateLimitErrorConstructor); isRateLimited: true; - retryAt: Date; - waitMs: number; + retryAt?: Date; + waitMs?: number; reason: RateLimitReason; policy?: string; source: RateLimitSource | "unknown"; From 468f83aeed9c24349f752de4229124207a7e66b6 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 12:12:03 -0300 Subject: [PATCH 29/33] fix(core): bound hinted retries and keep them off the ladder A provider that answers every call with a short Retry-After kept the Requester in a loop that only the time cap could end, and each hinted retry spent a ladder attempt, so a later 5xx got no retry. A request now makes at most 10 hinted retries, and they leave the ladder alone. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 2 + packages/core/modules/requester/requester.js | 81 ++++++++++--------- .../core/modules/requester/requester.test.js | 53 +++++++++++- 3 files changed, 94 insertions(+), 42 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 18d868e5c..e46963757 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -315,6 +315,8 @@ class Api extends ApiKeyRequester { in the Lambda invocation less one request timeout. That reserve is never more than half the time left, so a 29 s HTTP invocation with the default 60 s request timeout can still sleep 14.5 s. + A request makes at most 10 hinted retries. They do not use the retries of + the ladder, so a 5xx after a hinted wait still gets the whole 5xx ladder. 4. A wait that does not fit throws `RateLimitError`. It extends `FetchError`, so `statusCode` stays, and adds `isRateLimited`, `retryAt`, `waitMs`, `reason`, `policy`, `source`, `module` and `scopeKey`. diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index c7c5df36c..1bfb91bf1 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -30,6 +30,10 @@ function sanitizedCause(err) { } const MAX_AUTH_RETRIES = 3; +// Hinted waits are bounded by time, but a provider that always answers with a +// short Retry-After would still keep the request in a long loop. +const MAX_HINTED_RETRIES = 10; + // This context marks the async call chain that holds the refresh slot, and // names the requester that holds it. Token requests re-enter _rawRequest // through this._post. A 401 from inside that chain must fail fast: if it @@ -221,9 +225,16 @@ class Requester extends Delegate { * @param {Object} options - Fetch options, as built by `_request`. * @param {number} attempt - 0-based count of retries already made for * this call. Indexes `this.backOff` for the next delay and is passed - * back in on each recursive retry. + * back in on each recursive retry. A hinted retry leaves it unchanged. + * @param {{ waitedMs: number, retries: number }} hinted - The time slept + * and the retries made for rate-limit hints, for the whole call. */ - async _rawRequest(url, options, attempt = 0, waitedMs = 0) { + async _rawRequest( + url, + options, + attempt = 0, + hinted = { waitedMs: 0, retries: 0 } + ) { let encodedUrl = encodeURI(url); if (options.query) { let queryBuild = '?'; @@ -289,12 +300,7 @@ class Requester extends Delegate { clearRequestTimer(); const delay = this.backOff[attempt] * 1000; await new Promise((resolve) => setTimeout(resolve, delay)); - return this._rawRequest( - url, - options, - attempt + 1, - waitedMs - ); + return this._rawRequest(url, options, attempt + 1, hinted); } const fetchError = await FetchError.create({ resource: encodedUrl, @@ -322,7 +328,7 @@ class Requester extends Delegate { throttle, status, attempt, - waitedMs, + hinted, encodedUrl, options, response, @@ -333,12 +339,10 @@ class Requester extends Delegate { await new Promise((resolve) => setTimeout(resolve, hintedDelayMs) ); - return this._rawRequest( - url, - options, - attempt + 1, - waitedMs + hintedDelayMs - ); + return this._rawRequest(url, options, attempt, { + waitedMs: hinted.waitedMs + hintedDelayMs, + retries: hinted.retries + 1, + }); } // If the status is retriable and there are back off requests left, retry the request @@ -349,7 +353,7 @@ class Requester extends Delegate { clearRequestTimer(); const delay = this.backOff[attempt] * 1000; await new Promise((resolve) => setTimeout(resolve, delay)); - return this._rawRequest(url, options, attempt + 1, waitedMs); + return this._rawRequest(url, options, attempt + 1, hinted); } if (status === 401) { @@ -370,12 +374,7 @@ class Requester extends Delegate { // This request did not try the current token. Retry with // it. Do not spend one more provider-side rotation. clearRequestTimer(); - return this._rawRequest( - url, - options, - attempt + 1, - waitedMs - ); + return this._rawRequest(url, options, attempt + 1, hinted); } if (!this.isRefreshable) { @@ -396,7 +395,7 @@ class Requester extends Delegate { url, options, attempt + 1, - waitedMs + hinted ); } @@ -430,12 +429,7 @@ class Requester extends Delegate { const refreshSucceeded = await this._refreshAuthOnce(); if (refreshSucceeded) { clearRequestTimer(); - return this._rawRequest( - url, - options, - attempt + 1, - waitedMs - ); + return this._rawRequest(url, options, attempt + 1, hinted); } throw await this._invalidateAuth(encodedUrl, options, response); @@ -484,7 +478,7 @@ class Requester extends Delegate { throttle, status, attempt, - waitedMs, + hinted, encodedUrl, options, response, @@ -495,15 +489,16 @@ class Requester extends Delegate { } const { hint } = throttle; const untimed = hint.waitMs === undefined; - const budgetMs = - this.backOff.length && !untimed - ? inProcessBudgetMs({ - policy: this._rateLimitPolicy, - requestTimeoutMs: this.requestTimeoutMs, - remainingMs: remainingInvocationMs(), - waitedMs, - }) - : 0; + const mayWait = + this.backOff.length > 0 && hinted.retries < MAX_HINTED_RETRIES; + const budgetMs = mayWait + ? inProcessBudgetMs({ + policy: this._rateLimitPolicy, + requestTimeoutMs: this.requestTimeoutMs, + remainingMs: remainingInvocationMs(), + waitedMs: hinted.waitedMs, + }) + : 0; const { waitMs, fits } = untimed ? { waitMs: undefined, fits: false } : computeWaitMs({ @@ -513,7 +508,13 @@ class Requester extends Delegate { random: this._random, }); if (fits) { - this._logRateLimited({ status, hint, waitMs, attempt, waitedMs }); + this._logRateLimited({ + status, + hint, + waitMs, + attempt, + waitedMs: hinted.waitedMs, + }); return waitMs; } diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index 626672a34..fc8f4d431 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -249,6 +249,55 @@ describe('Requester', () => { expect(fetch).toHaveBeenCalledTimes(9); }); + it('stops after 10 hinted retries even when every wait fits the cap', async () => { + const fetch = throttledFetch( + Array(11).fill(limited({ 'Retry-After': '1' })) + ); + + const error = await makeRequester(fetch, { backOff: [1, 2] }) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(RateLimitError); + expect(error.statusCode).toBe(429); + expect(delays).toEqual(Array(10).fill(1_000)); + expect(fetch).toHaveBeenCalledTimes(11); + }); + + it('keeps the whole 5xx ladder after a hinted retry', async () => { + const fetch = throttledFetch([ + limited({ 'Retry-After': '1' }), + ...Array(7).fill({ status: 500 }), + ]); + + const error = await makeRequester(fetch) + ._get({ url }) + .catch((e) => e); + + expect(error).toBeInstanceOf(FetchError); + expect(error.statusCode).toBe(500); + expect(delays).toEqual([1_000, ...LADDER_MS]); + expect(fetch).toHaveBeenCalledTimes(8); + }); + + it('does not count a 5xx retry or a connection reset as a hinted retry', async () => { + const reset = Object.assign(new Error('socket hang up'), { + code: 'ECONNRESET', + }); + const fetch = throttledFetch([ + ...Array(9).fill(limited({ 'Retry-After': '1' })), + { status: 500 }, + { reject: reset }, + limited({ 'Retry-After': '1' }), + ok, + ]); + + const result = await makeRequester(fetch)._get({ url }); + + expect(result).toEqual({ ok: true }); + expect(fetch).toHaveBeenCalledTimes(13); + }); + it('throws a RateLimitError with retryAt when the wait is over the cap', async () => { const fetch = throttledFetch([ limited({ 'Retry-After': '400' }), @@ -304,7 +353,7 @@ describe('Requester', () => { .catch((e) => e); expect(error).toBeInstanceOf(RateLimitError); - expect(delays).toEqual([200_000, 3_000]); + expect(delays).toEqual([200_000, 1_000]); expect(fetch).toHaveBeenCalledTimes(3); }); @@ -324,7 +373,7 @@ describe('Requester', () => { .catch((e) => e); expect(error).toBeInstanceOf(RateLimitError); - expect(delays).toEqual([200_000, 3_000]); + expect(delays).toEqual([200_000, 1_000]); }); it('sleeps a short wait inside an invocation shorter than the default request timeout', async () => { From 6eb85ab403f6a3aa18fc5ad49c63c1799890ec55 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 12:14:40 -0300 Subject: [PATCH 30/33] refactor(core): let the caller own the in-process wait budget maxInProcessWaitMs was a key of the module's static rateLimit, but the caller knows how long it can wait: an HTTP handler 10 s, a queue worker 60 s, a script 300 s. The invocation store now carries it next to the deadline. runWithInvocationDeadline takes { deadlineAt, maxInProcessWaitMs }, createHandler takes maxInProcessWaitMs, and the module policy no longer reads it. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 39 ++++-- packages/core/CLAUDE.md | 4 +- packages/core/core/CLAUDE.md | 2 +- packages/core/core/create-handler.js | 3 + packages/core/core/create-handler.test.js | 24 ++++ packages/core/core/invocation-deadline.js | 58 +++++++- .../core/core/invocation-deadline.test.js | 124 ++++++++++++++++-- packages/core/core/invocation-scope.js | 7 +- packages/core/core/invocation-scope.test.js | 22 +++- ...integration-defined-workers-budget.test.js | 26 ++++ .../workers/integration-defined-workers.js | 2 + .../modules/requester/rate-limit/policy.js | 14 +- .../requester/rate-limit/policy.test.js | 45 ++++--- packages/core/modules/requester/requester.js | 7 +- .../core/modules/requester/requester.test.js | 54 ++++++-- packages/core/types/core/index.d.ts | 2 +- packages/core/types/module-plugin/index.d.ts | 1 - 17 files changed, 362 insertions(+), 72 deletions(-) create mode 100644 packages/core/handlers/workers/integration-defined-workers-budget.test.js diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index e46963757..ac7b26107 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -232,7 +232,6 @@ that the runtime does not read: only `static rateLimit` on the API class counts. |---|---| | `scope` | The key a limit counts against: `'entity'` (default), `'credential'`, `'app'`, or a function `(requester) => key`. The Requester puts it on `RateLimitError.scopeKey`, which is `undefined` when the requester has no id for the scope. | | `minRetryAfterMs` | A wait never shorter than this. Use it when the docs say "wait 60 seconds" and the response says nothing. | -| `maxInProcessWaitMs` | The most one request sleeps in process, in total. Default `300000` (5 minutes). `0` means never sleep: every hinted wait throws `RateLimitError`. | | `parsers` | The header parsers to read, in order: `retryAfter`, `resetHeaders`, `ietf`. Default: all three. A name that is not built in throws a `TypeError` when the API class is constructed. | | `classify` | Recognises a limit that is not a plain 429, or names its reason. See below. | | `windows`, `maxConcurrency` | The provider's limits, as documentation. A window named like the `reason` (`{ name: 'burst', perMs: 10000 }`) sets the wait when no header does, unless `minRetryAfterMs` is longer. Pacing uses them later. | @@ -311,12 +310,15 @@ class Api extends ApiKeyRequester { does not recognise keeps the 5xx ladder. 3. With a hint, the wait is the largest of the hint, `minRetryAfterMs` and 1 s, plus at most 10 % jitter. The Requester sleeps when the wait fits the time - this request may still sleep: `maxInProcessWaitMs` in total, and the time left - in the Lambda invocation less one request timeout. That reserve is never more - than half the time left, so a 29 s HTTP invocation with the default 60 s - request timeout can still sleep 14.5 s. - A request makes at most 10 hinted retries. They do not use the retries of - the ladder, so a 5xx after a hinted wait still gets the whole 5xx ladder. + this request may still sleep, in total, and the time left in the Lambda + invocation less one request timeout. That reserve is never more than half + the time left, so a 29 s HTTP invocation with the default 60 s request + timeout can still sleep 14.5 s. The caller owns the total, not the module: + 10 s in an HTTP handler, 60 s in an integration queue worker, and 300 s + outside an invocation (tests, scripts, `frigg start`). See + [The invocation deadline](#the-invocation-deadline). A request makes at most + 10 hinted retries. They do not use the retries of the ladder, so a 5xx after + a hinted wait still gets the whole 5xx ladder. 4. A wait that does not fit throws `RateLimitError`. It extends `FetchError`, so `statusCode` stays, and adds `isRateLimited`, `retryAt`, `waitMs`, `reason`, `policy`, `source`, `module` and `scopeKey`. @@ -409,12 +411,23 @@ class Api extends OAuth2Requester { ### The invocation deadline -`runInvocationScope` stores the end of every Lambda invocation, from -`context.getRemainingTimeInMillis()`. `remainingInvocationMs()` returns the -milliseconds left, or `Infinity` outside an invocation (tests, `frigg start`, -scripts). `runWithInvocationDeadline(deadlineAt, fn)` runs `fn` with a deadline -in epoch milliseconds. A nested call can only make the deadline earlier, and a -deadline that is not a finite number sets none. +`runInvocationScope` stores two limits for every Lambda invocation: the end of +the invocation, from `context.getRemainingTimeInMillis()`, and the most one +Requester call may sleep in process, in total. `createHandler` takes the second +as the option `maxInProcessWaitMs`. + +| Entry point | `maxInProcessWaitMs` | +|---|---| +| HTTP handlers (`createHandler` default) | `10000` | +| Integration queue worker | `60000` | +| Outside an invocation (tests, scripts, `frigg start`) | `300000` | + +`remainingInvocationMs()` returns the milliseconds left, or `Infinity` outside +an invocation. `runWithInvocationDeadline({ deadlineAt, maxInProcessWaitMs }, fn)` +runs `fn` with either limit or both. `deadlineAt` is in epoch milliseconds. A +nested call can only lower each limit, field by field. A field that is not a +finite number, or a negative `maxInProcessWaitMs`, sets nothing. `0` means never +sleep: every hinted wait throws `RateLimitError`. ## Complete OAuth2 Example diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 739d284c0..3628c8172 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -371,7 +371,9 @@ class MyIntegration extends IntegrationBase { **Rate limits** (ADR-049): an API module declares `static rateLimit` on its `Requester` subclass. A hinted wait (from `classify()`, the header parsers or -the static policy) sleeps within a budget, or throws `RateLimitError` +the static policy) sleeps within a budget that the caller owns (10 s in an HTTP +handler, 60 s in a queue worker, 300 s outside an invocation, at most 10 +hinted retries), or throws `RateLimitError` (`isRateLimited`, `retryAt`) when it does not fit. A 429 with no hint keeps the fixed `backOff` ladder. A non-429 limit that `classify` names without a time throws `RateLimitError` at once, with `retryAt` unknown. See diff --git a/packages/core/core/CLAUDE.md b/packages/core/core/CLAUDE.md index 0c9d79a44..905de2de5 100644 --- a/packages/core/core/CLAUDE.md +++ b/packages/core/core/CLAUDE.md @@ -130,7 +130,7 @@ class MyIntegration extends Delegate { ### Lambda Handler Lifecycle 1. **Pre-Execution Setup**: ```javascript - runInvocationScope({ requestId, handlerName, method, route, invocation }, …); // Logger scope + invocation deadline (remainingInvocationMs()) + runInvocationScope({ requestId, handlerName, method, route, invocation }, …); // Logger scope + invocation deadline (remainingInvocationMs()) + in-process wait budget (createHandler `maxInProcessWaitMs`: 10 s HTTP, 60 s queue worker) log.info('Handler invoked', { eventName: 'frigg.handler.invoked' }); await secretsToEnv(); // Secrets Manager injection await parametersToEnv(); // SSM Parameter Store fetch (only when SSM_PARAMETER_PREFIX + FRIGG_SSM_OFFLOADED_KEYS are set) diff --git a/packages/core/core/create-handler.js b/packages/core/core/create-handler.js index e5061d765..855b6e5dd 100644 --- a/packages/core/core/create-handler.js +++ b/packages/core/core/create-handler.js @@ -12,6 +12,7 @@ const { runInvocationScope, DEFAULT_FLUSH_TIMEOUT_MS, } = require('./invocation-scope'); +const { MAX_IN_PROCESS_WAIT_MS } = require('./invocation-deadline'); const { secretsToEnv } = require('./secrets-to-env'); const { parametersToEnv } = require('./parameters-to-env'); const { @@ -31,6 +32,7 @@ const createHandler = (optionByName = {}) => { telemetry, flushTimeoutMs = DEFAULT_FLUSH_TIMEOUT_MS, usageRollup, + maxInProcessWaitMs = MAX_IN_PROCESS_WAIT_MS.http, } = optionByName; if (!method) { @@ -144,6 +146,7 @@ const createHandler = (optionByName = {}) => { shouldUseDatabase, flushTimeoutMs, context, + maxInProcessWaitMs, } ); }; diff --git a/packages/core/core/create-handler.test.js b/packages/core/core/create-handler.test.js index a3cf9678f..ba1e027b0 100644 --- a/packages/core/core/create-handler.test.js +++ b/packages/core/core/create-handler.test.js @@ -7,6 +7,7 @@ jest.mock('./secrets-to-env', () => ({ const { connectPrisma } = require('../database/prisma'); const { createHandler } = require('./create-handler'); +const { invocationMaxInProcessWaitMs } = require('./invocation-deadline'); const { getLogger, createMemorySink } = require('../logs'); const { HaltError } = require('../errors/halt-error'); const { httpApiV2Event, sqsEvent } = require('../logs/__fixtures__/events'); @@ -476,3 +477,26 @@ describe('createHandler — logger scope and records (ADR-048)', () => { expect(inside.droppedKeys).toEqual(['requestId']); }); }); + +describe('createHandler: in-process wait budget', () => { + const ctx = { awsRequestId: 'r1' }; + + it('runs the method with a 10 s budget by default', async () => { + const handler = createHandler({ + eventName: 'X', + shouldUseDatabase: false, + method: async () => invocationMaxInProcessWaitMs(), + }); + await expect(handler({}, { ...ctx })).resolves.toBe(10_000); + }); + + it('runs the method with the budget the caller gives', async () => { + const handler = createHandler({ + eventName: 'X', + shouldUseDatabase: false, + maxInProcessWaitMs: 60_000, + method: async () => invocationMaxInProcessWaitMs(), + }); + await expect(handler({}, { ...ctx })).resolves.toBe(60_000); + }); +}); diff --git a/packages/core/core/invocation-deadline.js b/packages/core/core/invocation-deadline.js index 9c7f63466..5c8a6e13e 100644 --- a/packages/core/core/invocation-deadline.js +++ b/packages/core/core/invocation-deadline.js @@ -9,15 +9,50 @@ function storage() { return globalThis[STORE_KEY]; } -function runWithInvocationDeadline(deadlineAt, fn) { - if (typeof deadlineAt !== 'number' || !Number.isFinite(deadlineAt)) { +const MAX_IN_PROCESS_WAIT_MS = Object.freeze({ + outsideInvocation: 300_000, + http: 10_000, + queueWorker: 60_000, +}); + +const validDeadline = (value) => + typeof value === 'number' && Number.isFinite(value) ? value : undefined; + +const validWait = (value) => + typeof value === 'number' && Number.isFinite(value) && value >= 0 + ? value + : undefined; + +const lower = (outer, next) => + outer === undefined + ? next + : next === undefined + ? outer + : Math.min(outer, next); + +function runWithInvocationDeadline( + { deadlineAt, maxInProcessWaitMs } = {}, + fn +) { + const outer = storage().getStore(); + const effectiveDeadline = lower( + outer?.deadlineAt, + validDeadline(deadlineAt) + ); + const effectiveWait = lower( + outer?.maxInProcessWaitMs, + validWait(maxInProcessWaitMs) + ); + if (effectiveDeadline === undefined && effectiveWait === undefined) { return fn(); } - const outer = storage().getStore(); - const effective = outer - ? Math.min(outer.deadlineAt, deadlineAt) - : deadlineAt; - return storage().run(Object.freeze({ deadlineAt: effective }), fn); + return storage().run( + Object.freeze({ + deadlineAt: effectiveDeadline ?? Infinity, + maxInProcessWaitMs: effectiveWait, + }), + fn + ); } function remainingTimeMs(context) { @@ -39,8 +74,17 @@ function remainingInvocationMs(now = Date.now()) { return store ? Math.max(0, store.deadlineAt - now) : Infinity; } +function invocationMaxInProcessWaitMs() { + return ( + storage().getStore()?.maxInProcessWaitMs ?? + MAX_IN_PROCESS_WAIT_MS.outsideInvocation + ); +} + module.exports = { + MAX_IN_PROCESS_WAIT_MS, deadlineFromContext, + invocationMaxInProcessWaitMs, remainingInvocationMs, remainingTimeMs, runWithInvocationDeadline, diff --git a/packages/core/core/invocation-deadline.test.js b/packages/core/core/invocation-deadline.test.js index 6964aeb27..49496d6ad 100644 --- a/packages/core/core/invocation-deadline.test.js +++ b/packages/core/core/invocation-deadline.test.js @@ -1,5 +1,7 @@ const { + MAX_IN_PROCESS_WAIT_MS, deadlineFromContext, + invocationMaxInProcessWaitMs, remainingInvocationMs, runWithInvocationDeadline, } = require('./invocation-deadline'); @@ -15,7 +17,7 @@ describe('core/invocation-deadline', () => { it('reports the time left, deep inside async work', async () => { const deadlineAt = 1_060_000; - await runWithInvocationDeadline(deadlineAt, async () => { + await runWithInvocationDeadline({ deadlineAt }, async () => { await tick(); await Promise.resolve().then(() => { expect(remainingInvocationMs(1_000_000)).toBe(60_000); @@ -24,18 +26,18 @@ describe('core/invocation-deadline', () => { }); it('never reports a negative time', () => { - runWithInvocationDeadline(1_000, () => { + runWithInvocationDeadline({ deadlineAt: 1_000 }, () => { expect(remainingInvocationMs(5_000)).toBe(0); }); }); it('returns the value of fn and passes a rejection through', async () => { await expect( - runWithInvocationDeadline(2e12, async () => 'ok') + runWithInvocationDeadline({ deadlineAt: 2e12 }, async () => 'ok') ).resolves.toBe('ok'); const boom = new Error('boom'); await expect( - runWithInvocationDeadline(2e12, async () => { + runWithInvocationDeadline({ deadlineAt: 2e12 }, async () => { throw boom; }) ).rejects.toBe(boom); @@ -44,7 +46,7 @@ describe('core/invocation-deadline', () => { it.each([undefined, null, NaN, Infinity, 'soon'])( 'runs fn with no limit for the deadline %p', (deadlineAt) => { - const result = runWithInvocationDeadline(deadlineAt, () => + const result = runWithInvocationDeadline({ deadlineAt }, () => remainingInvocationMs() ); expect(result).toBe(Infinity); @@ -52,8 +54,8 @@ describe('core/invocation-deadline', () => { ); it('lets a nested scope shorten the deadline, then restores the outer one', () => { - runWithInvocationDeadline(1_060_000, () => { - runWithInvocationDeadline(1_010_000, () => { + runWithInvocationDeadline({ deadlineAt: 1_060_000 }, () => { + runWithInvocationDeadline({ deadlineAt: 1_010_000 }, () => { expect(remainingInvocationMs(1_000_000)).toBe(10_000); }); expect(remainingInvocationMs(1_000_000)).toBe(60_000); @@ -61,13 +63,117 @@ describe('core/invocation-deadline', () => { }); it('does not let a nested scope extend the deadline', () => { - runWithInvocationDeadline(1_010_000, () => { - runWithInvocationDeadline(1_060_000, () => { + runWithInvocationDeadline({ deadlineAt: 1_010_000 }, () => { + runWithInvocationDeadline({ deadlineAt: 1_060_000 }, () => { expect(remainingInvocationMs(1_000_000)).toBe(10_000); }); }); }); + describe('maxInProcessWaitMs', () => { + it('has one table of defaults, frozen', () => { + expect(MAX_IN_PROCESS_WAIT_MS).toEqual({ + outsideInvocation: 300_000, + http: 10_000, + queueWorker: 60_000, + }); + expect(Object.isFrozen(MAX_IN_PROCESS_WAIT_MS)).toBe(true); + }); + + it('reads the outside-invocation default when no scope is open', () => { + expect(invocationMaxInProcessWaitMs()).toBe(300_000); + }); + + it('reads the default when the scope sets only a deadline', () => { + runWithInvocationDeadline({ deadlineAt: 2e12 }, () => { + expect(invocationMaxInProcessWaitMs()).toBe(300_000); + }); + }); + + it('reads the value of the scope, deep inside async work', async () => { + await runWithInvocationDeadline( + { maxInProcessWaitMs: 10_000 }, + async () => { + await tick(); + expect(invocationMaxInProcessWaitMs()).toBe(10_000); + } + ); + }); + + it('sets no deadline when it is the only field', () => { + runWithInvocationDeadline({ maxInProcessWaitMs: 0 }, () => { + expect(invocationMaxInProcessWaitMs()).toBe(0); + expect(remainingInvocationMs()).toBe(Infinity); + }); + }); + + it.each([undefined, null, NaN, Infinity, -1, '5'])( + 'ignores the value %p and runs fn with no scope', + (maxInProcessWaitMs) => { + runWithInvocationDeadline({ maxInProcessWaitMs }, () => { + expect(invocationMaxInProcessWaitMs()).toBe(300_000); + expect(remainingInvocationMs()).toBe(Infinity); + }); + } + ); + + it('lets a nested scope lower the value, then restores the outer one', () => { + runWithInvocationDeadline({ maxInProcessWaitMs: 60_000 }, () => { + runWithInvocationDeadline({ maxInProcessWaitMs: 5_000 }, () => { + expect(invocationMaxInProcessWaitMs()).toBe(5_000); + }); + expect(invocationMaxInProcessWaitMs()).toBe(60_000); + }); + }); + + it('does not let a nested scope raise the value', () => { + runWithInvocationDeadline({ maxInProcessWaitMs: 5_000 }, () => { + runWithInvocationDeadline( + { maxInProcessWaitMs: 60_000 }, + () => { + expect(invocationMaxInProcessWaitMs()).toBe(5_000); + } + ); + }); + }); + + it('takes the lower of each field on its own', () => { + runWithInvocationDeadline( + { deadlineAt: 1_010_000, maxInProcessWaitMs: 60_000 }, + () => { + runWithInvocationDeadline( + { deadlineAt: 1_060_000, maxInProcessWaitMs: 5_000 }, + () => { + expect(remainingInvocationMs(1_000_000)).toBe( + 10_000 + ); + expect(invocationMaxInProcessWaitMs()).toBe(5_000); + } + ); + } + ); + }); + + it('keeps the outer value of a field the nested scope leaves out', () => { + runWithInvocationDeadline( + { deadlineAt: 1_010_000, maxInProcessWaitMs: 60_000 }, + () => { + runWithInvocationDeadline({ deadlineAt: 1_005_000 }, () => { + expect(invocationMaxInProcessWaitMs()).toBe(60_000); + }); + runWithInvocationDeadline( + { maxInProcessWaitMs: 5_000 }, + () => { + expect(remainingInvocationMs(1_000_000)).toBe( + 10_000 + ); + } + ); + } + ); + }); + }); + describe('deadlineFromContext', () => { it('adds the time left to the current time', () => { jest.spyOn(Date, 'now').mockReturnValue(1_000); diff --git a/packages/core/core/invocation-scope.js b/packages/core/core/invocation-scope.js index 567850a09..ccaa69646 100644 --- a/packages/core/core/invocation-scope.js +++ b/packages/core/core/invocation-scope.js @@ -163,8 +163,13 @@ async function runInvocationScope(fields, fn, opts = {}) { shouldUseDatabase = true, flushTimeoutMs = DEFAULT_FLUSH_TIMEOUT_MS, context, + maxInProcessWaitMs, } = opts; - return runWithInvocationDeadline(deadlineFromContext(context), () => + const deadline = { + deadlineAt: deadlineFromContext(context), + maxInProcessWaitMs, + }; + return runWithInvocationDeadline(deadline, () => runInContext( { [LOGGER_SCOPE_KEY]: { ...(fields || {}) } }, async () => { diff --git a/packages/core/core/invocation-scope.test.js b/packages/core/core/invocation-scope.test.js index 0876b735f..6766778ce 100644 --- a/packages/core/core/invocation-scope.test.js +++ b/packages/core/core/invocation-scope.test.js @@ -10,7 +10,10 @@ const { const { getLogger, createMemorySink } = require('../logs'); const { setSinks } = require('../logs/logger-runtime'); const { getLoggerScope } = require('../logs/context'); -const { remainingInvocationMs } = require('./invocation-deadline'); +const { + invocationMaxInProcessWaitMs, + remainingInvocationMs, +} = require('./invocation-deadline'); const { mergeTelemetryContext } = require('../telemetry/telemetry-context'); const { NoOpTelemetry } = require('../telemetry/no-op-telemetry'); const { createUsageRollupSubscriber } = require('../telemetry/usage-rollup-subscriber'); @@ -75,6 +78,23 @@ describe('core/invocation-scope', () => { ); }); + it('passes maxInProcessWaitMs on to the invocation scope', async () => { + await runInvocationScope( + {}, + async () => { + expect(invocationMaxInProcessWaitMs()).toBe(12_000); + expect(remainingInvocationMs()).toBe(Infinity); + }, + { maxInProcessWaitMs: 12_000 } + ); + }); + + it('sets the default wait budget when the caller gives none', async () => { + await runInvocationScope({}, async () => { + expect(invocationMaxInProcessWaitMs()).toBe(300_000); + }); + }); + it('sets no deadline when there is no context', async () => { await runInvocationScope({}, async () => { expect(remainingInvocationMs()).toBe(Infinity); diff --git a/packages/core/handlers/workers/integration-defined-workers-budget.test.js b/packages/core/handlers/workers/integration-defined-workers-budget.test.js new file mode 100644 index 000000000..4650016ae --- /dev/null +++ b/packages/core/handlers/workers/integration-defined-workers-budget.test.js @@ -0,0 +1,26 @@ +const mockCreateHandler = jest.fn((options) => options); + +jest.mock('@friggframework/core', () => ({ + createHandler: mockCreateHandler, +})); +jest.mock('../app-definition-loader', () => ({ + loadAppDefinition: () => ({ + integrations: [{ Definition: { name: 'hubspot' } }], + }), +})); +jest.mock('../backend-utils', () => ({ + createQueueWorker: jest.fn(() => class {}), +})); + +const { MAX_IN_PROCESS_WAIT_MS } = require('../../core/invocation-deadline'); + +describe('integration queue worker handler', () => { + it('runs with the queue worker wait budget', () => { + const { handlers } = require('./integration-defined-workers'); + + expect(handlers.hubspot.queueWorker.maxInProcessWaitMs).toBe( + MAX_IN_PROCESS_WAIT_MS.queueWorker + ); + expect(MAX_IN_PROCESS_WAIT_MS.queueWorker).toBe(60_000); + }); +}); diff --git a/packages/core/handlers/workers/integration-defined-workers.js b/packages/core/handlers/workers/integration-defined-workers.js index cb88ac0b6..4d4ddcb12 100644 --- a/packages/core/handlers/workers/integration-defined-workers.js +++ b/packages/core/handlers/workers/integration-defined-workers.js @@ -1,6 +1,7 @@ const { createHandler } = require('@friggframework/core'); const { loadAppDefinition } = require('../app-definition-loader'); const { createQueueWorker } = require('../backend-utils'); +const { MAX_IN_PROCESS_WAIT_MS } = require('../../core/invocation-deadline'); // TODO(Phase 2): mount extension-declared workers in addition to the per-integration // default queue worker. Today, getExtensionWorkers(IntegrationClass) returns the // declared workers but they are not yet bound to dedicated SQS sources. Extension- @@ -19,6 +20,7 @@ integrationClasses.forEach((IntegrationClass) => { queueWorker: createHandler({ eventName: `Queue Worker for ${IntegrationClass.Definition.name}`, isUserFacingResponse: false, + maxInProcessWaitMs: MAX_IN_PROCESS_WAIT_MS.queueWorker, method: async (event, context) => { const worker = new defaultQueueWorker(); return await worker.run(event, context); diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index 9a9b35622..535610d7e 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -6,7 +6,6 @@ const { } = require('./parsers'); const DEFAULT_PARSERS = ['retryAfter', 'resetHeaders', 'ietf']; -const DEFAULT_MAX_IN_PROCESS_WAIT_MS = 300_000; const MIN_HINTED_WAIT_MS = 1_000; const JITTER_MAX_MS = 1_000; const JITTER_RATIO = 0.1; @@ -60,8 +59,8 @@ function normalizePolicy(raw, owner) { if (typeof raw.classify === 'function') { policy.classify = (signal) => raw.classify(signal); } - for (const key of ['minRetryAfterMs', 'maxInProcessWaitMs']) { - if (Number.isFinite(raw[key]) && raw[key] >= 0) policy[key] = raw[key]; + if (Number.isFinite(raw.minRetryAfterMs) && raw.minRetryAfterMs >= 0) { + policy.minRetryAfterMs = raw.minRetryAfterMs; } if (Number.isInteger(raw.maxConcurrency) && raw.maxConcurrency > 0) { policy.maxConcurrency = raw.maxConcurrency; @@ -280,18 +279,19 @@ function computeWaitMs({ hint, policy, budgetMs, random = Math.random }) { } function inProcessBudgetMs({ - policy, + maxInProcessWaitMs, requestTimeoutMs = 0, remainingMs = Infinity, waitedMs = 0, } = {}) { - const cap = policy?.maxInProcessWaitMs ?? DEFAULT_MAX_IN_PROCESS_WAIT_MS; const reserveMs = Math.min(requestTimeoutMs || 0, remainingMs / 2); - return Math.max(0, Math.min(cap - waitedMs, remainingMs - reserveMs)); + return Math.max( + 0, + Math.min(maxInProcessWaitMs - waitedMs, remainingMs - reserveMs) + ); } module.exports = { - DEFAULT_MAX_IN_PROCESS_WAIT_MS, DEFAULT_PARSERS, JITTER_MAX_MS, MIN_HINTED_WAIT_MS, diff --git a/packages/core/modules/requester/rate-limit/policy.test.js b/packages/core/modules/requester/rate-limit/policy.test.js index 3b0b5b8f0..a8298b6c8 100644 --- a/packages/core/modules/requester/rate-limit/policy.test.js +++ b/packages/core/modules/requester/rate-limit/policy.test.js @@ -1,5 +1,4 @@ const { - DEFAULT_MAX_IN_PROCESS_WAIT_MS, DEFAULT_PARSERS, JITTER_MAX_MS, MIN_HINTED_WAIT_MS, @@ -17,7 +16,6 @@ const options = { now: NOW }; describe('rate-limit/policy', () => { it('exposes the defaults', () => { expect(DEFAULT_PARSERS).toEqual(['retryAfter', 'resetHeaders', 'ietf']); - expect(DEFAULT_MAX_IN_PROCESS_WAIT_MS).toBe(300_000); expect(MIN_HINTED_WAIT_MS).toBe(1_000); expect(JITTER_MAX_MS).toBe(1_000); }); @@ -76,11 +74,11 @@ describe('rate-limit/policy', () => { windows, maxConcurrency: 5, minRetryAfterMs: 60_000, - maxInProcessWaitMs: 30_000, parsers: ['retryAfter'], userHints, }); expect(policy).not.toHaveProperty('somethingElse'); + expect(policy).not.toHaveProperty('maxInProcessWaitMs'); }); it('drops values of the wrong type', () => { @@ -88,7 +86,6 @@ describe('rate-limit/policy', () => { static rateLimit = { classify: 'nope', minRetryAfterMs: -1, - maxInProcessWaitMs: Infinity, maxConcurrency: 1.5, windows: 'many', userHints: 3, @@ -823,26 +820,16 @@ describe('rate-limit/policy', () => { }); describe('inProcessBudgetMs', () => { - it('defaults the cap to 5 minutes', () => { + it('takes the cap from the caller', () => { expect( inProcessBudgetMs({ - requestTimeoutMs: 0, - remainingMs: Infinity, - waitedMs: 0, - }) - ).toBe(300_000); - }); - - it('lets the policy override the cap', () => { - expect( - inProcessBudgetMs({ - policy: { maxInProcessWaitMs: 30_000 }, + maxInProcessWaitMs: 30_000, remainingMs: Infinity, }) ).toBe(30_000); expect( inProcessBudgetMs({ - policy: { maxInProcessWaitMs: 0 }, + maxInProcessWaitMs: 0, remainingMs: Infinity, }) ).toBe(0); @@ -850,13 +837,18 @@ describe('rate-limit/policy', () => { it('subtracts the wait already spent', () => { expect( - inProcessBudgetMs({ remainingMs: Infinity, waitedMs: 100_000 }) + inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, + remainingMs: Infinity, + waitedMs: 100_000, + }) ).toBe(200_000); }); it('keeps one request timeout of the time left for the next request', () => { expect( inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, requestTimeoutMs: 30_000, remainingMs: 100_000, }) @@ -866,6 +858,7 @@ describe('rate-limit/policy', () => { it('keeps at most half of the time left for the next request', () => { expect( inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, requestTimeoutMs: 60_000, remainingMs: 29_000, }) @@ -875,6 +868,7 @@ describe('rate-limit/policy', () => { it('is the cap when the invocation has time to spare', () => { expect( inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, requestTimeoutMs: 60_000, remainingMs: 900_000, }) @@ -884,19 +878,30 @@ describe('rate-limit/policy', () => { it('is never negative', () => { expect( inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, requestTimeoutMs: 60_000, remainingMs: 0, }) ).toBe(0); expect( - inProcessBudgetMs({ remainingMs: Infinity, waitedMs: 400_000 }) + inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, + remainingMs: Infinity, + waitedMs: 400_000, + }) ).toBe(0); }); it('treats a missing request timeout as 0', () => { - expect(inProcessBudgetMs({ remainingMs: 50_000 })).toBe(50_000); expect( inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, + remainingMs: 50_000, + }) + ).toBe(50_000); + expect( + inProcessBudgetMs({ + maxInProcessWaitMs: 300_000, requestTimeoutMs: null, remainingMs: 50_000, }) diff --git a/packages/core/modules/requester/requester.js b/packages/core/modules/requester/requester.js index 1bfb91bf1..d81d39a17 100644 --- a/packages/core/modules/requester/requester.js +++ b/packages/core/modules/requester/requester.js @@ -1,7 +1,10 @@ const fetch = require('node-fetch'); const { AsyncLocalStorage } = require('async_hooks'); const { Delegate } = require('../../core'); -const { remainingInvocationMs } = require('../../core/invocation-deadline'); +const { + invocationMaxInProcessWaitMs, + remainingInvocationMs, +} = require('../../core/invocation-deadline'); const { FetchError, RateLimitError } = require('../../errors'); const { get } = require('../../assertions'); const { getTelemetry } = require('../../telemetry/telemetry-runtime'); @@ -493,7 +496,7 @@ class Requester extends Delegate { this.backOff.length > 0 && hinted.retries < MAX_HINTED_RETRIES; const budgetMs = mayWait ? inProcessBudgetMs({ - policy: this._rateLimitPolicy, + maxInProcessWaitMs: invocationMaxInProcessWaitMs(), requestTimeoutMs: this.requestTimeoutMs, remainingMs: remainingInvocationMs(), waitedMs: hinted.waitedMs, diff --git a/packages/core/modules/requester/requester.test.js b/packages/core/modules/requester/requester.test.js index fc8f4d431..bac68eb05 100644 --- a/packages/core/modules/requester/requester.test.js +++ b/packages/core/modules/requester/requester.test.js @@ -143,7 +143,7 @@ describe('Requester', () => { }); const error = await runWithInvocationDeadline( - Date.now() + 29_000, + { deadlineAt: Date.now() + 29_000 }, () => requester._get({ url }).catch((e) => e) ); @@ -386,7 +386,7 @@ describe('Requester', () => { }); const result = await runWithInvocationDeadline( - Date.now() + 29_000, + { deadlineAt: Date.now() + 29_000 }, () => requester._get({ url }) ); @@ -408,7 +408,7 @@ describe('Requester', () => { }); const error = await runWithInvocationDeadline( - Date.now() + 8_000, + { deadlineAt: Date.now() + 8_000 }, () => requester._get({ url }).catch((e) => e) ); @@ -416,19 +416,57 @@ describe('Requester', () => { expect(fetch).toHaveBeenCalledTimes(1); }); - it('throws at once when the policy allows no in-process wait', async () => { - const Api = withPolicy({ maxInProcessWaitMs: 0 }); + it('throws at once when the caller allows no in-process wait', async () => { const fetch = throttledFetch([ limited({ 'Retry-After': '2' }), ok, ]); - const error = await makeRequester(fetch, {}, Api) - ._get({ url }) - .catch((e) => e); + const error = await runWithInvocationDeadline( + { maxInProcessWaitMs: 0 }, + () => + makeRequester(fetch) + ._get({ url }) + .catch((e) => e) + ); expect(error).toBeInstanceOf(RateLimitError); expect(fetch).toHaveBeenCalledTimes(1); + expect(delays).toEqual([]); + }); + + it('sleeps only as long as the budget of the caller allows', async () => { + const fetch = throttledFetch([ + ...Array(3).fill(limited({ 'Retry-After': '10' })), + ok, + ]); + + const error = await runWithInvocationDeadline( + { maxInProcessWaitMs: 25_000 }, + () => + makeRequester(fetch) + ._get({ url }) + .catch((e) => e) + ); + + expect(error).toBeInstanceOf(RateLimitError); + expect(delays).toEqual([10_000, 10_000]); + expect(fetch).toHaveBeenCalledTimes(3); + }); + + it('ignores a maxInProcessWaitMs in the static rateLimit of the module', async () => { + const Api = withPolicy({ maxInProcessWaitMs: 0 }); + const fetch = throttledFetch([ + limited({ 'Retry-After': '2' }), + ok, + ]); + + const result = await makeRequester(fetch, {}, Api)._get({ + url, + }); + + expect(result).toEqual({ ok: true }); + expect(delays).toEqual([2_000]); }); it('reads the body of the RateLimitError once', async () => { diff --git a/packages/core/types/core/index.d.ts b/packages/core/types/core/index.d.ts index e0d39bc55..084ac9723 100644 --- a/packages/core/types/core/index.d.ts +++ b/packages/core/types/core/index.d.ts @@ -59,7 +59,7 @@ declare module "@friggframework/core" { export function loadInstalledModules(): any[]; export function runWithInvocationDeadline( - deadlineAt: number | undefined, + limits: { deadlineAt?: number; maxInProcessWaitMs?: number }, fn: () => T ): T; diff --git a/packages/core/types/module-plugin/index.d.ts b/packages/core/types/module-plugin/index.d.ts index a9e6e6fd5..0c468c552 100644 --- a/packages/core/types/module-plugin/index.d.ts +++ b/packages/core/types/module-plugin/index.d.ts @@ -48,7 +48,6 @@ declare module "@friggframework/module-plugin" { windows?: RateLimitWindow[]; maxConcurrency?: number; minRetryAfterMs?: number; - maxInProcessWaitMs?: number; parsers?: Array<"retryAfter" | "resetHeaders" | "ietf">; classify?( signal: RateLimitSignal From 3d2cd3e81ac5375c8ed669bad0148c63d1376546 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 18:39:20 -0300 Subject: [PATCH 31/33] fix(core): keep halting a rate-limited 4xx until deferral lands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 alone sent a rate-limited 403 back to SQS. With the generated queue config (3 receives about 30 min apart) a daily limit then reached the DLQ after about an hour, so the work was still lost. The halt exemption moves to the deferral change (#657), which puts the message back at its retry time. Phase 1 keeps the halt rule of next. The reference doc now also says that a 429 reads all default parsers, not only Retry-After. Refs ADR-049 §7. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 9 +++-- packages/core/CLAUDE.md | 3 +- packages/core/handlers/backend-utils.js | 3 +- .../integration-defined-workers.test.js | 37 ------------------- 4 files changed, 7 insertions(+), 45 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index ac7b26107..fa227b299 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -224,8 +224,9 @@ apiPropertiesToPersist: { credential: ['signing_key', 'webhook_secret'] } An API module can tell the Requester how its provider limits calls, and how to read a throttled response. Declare a static `rateLimit` on the API class. Every key is optional. A module that declares nothing keeps the fixed backoff ladder -for a 429 (1, 3, 10, 30, 60 and 180 s), and reads `Retry-After` when a 429 has -one. The JSON schema still has an older `config.rateLimit: { requests, period }` +for a 429 (1, 3, 10, 30, 60 and 180 s), and reads the time from a 429 with the +default `parsers`: `Retry-After`, the reset headers and the IETF `RateLimit` +headers. The JSON schema still has an older `config.rateLimit: { requests, period }` that the runtime does not read: only `static rateLimit` on the API class counts. | Key | Meaning | @@ -329,8 +330,8 @@ A Requester built with `backOff: []` never sleeps: a hinted wait throws The parsers, `Retry-After` among them, read the headers of a 429 and of a response that `classify` recognises, and of no other response: a 503 with `Retry-After` keeps the 5xx ladder unless `classify` recognises it. The queue -worker does not halt a `RateLimitError`, even when its status is 403: the -message goes back to SQS. +worker halts a `RateLimitError` with a 4xx status other than 408 and 429, like +any other 4xx: it discards the message. ### A client that is not the Requester diff --git a/packages/core/CLAUDE.md b/packages/core/CLAUDE.md index 3628c8172..5f827ca62 100644 --- a/packages/core/CLAUDE.md +++ b/packages/core/CLAUDE.md @@ -487,8 +487,7 @@ handlers (`{ req, res, next }`) and `this.on` events do not. `isLastAttempt` is `false` when either count is unknown: a local or non-SQS invocation, or a queue whose redrive policy the stack does not own (`ownership.queue: 'external'`). The value is information only: core still -rethrows retryable errors and discards halt errors (4xx except 408, 429 and -errors with `isRateLimited`). +rethrows retryable errors and discards halt errors (4xx except 408 and 429). Use it to end a run or count lost work on the final try: when a retryable error (429, 5xx, network) is about to be rethrown and `isLastAttempt` is diff --git a/packages/core/handlers/backend-utils.js b/packages/core/handlers/backend-utils.js index 6cf32616c..1ee6fee44 100644 --- a/packages/core/handlers/backend-utils.js +++ b/packages/core/handlers/backend-utils.js @@ -285,8 +285,7 @@ const createQueueWorker = (integrationClass) => { status >= 400 && status < 500 && status !== 408 && - status !== 429 && - !error.isRateLimited + status !== 429 ) { error.isHaltError = true; console.warn( diff --git a/packages/core/handlers/workers/integration-defined-workers.test.js b/packages/core/handlers/workers/integration-defined-workers.test.js index 6a8eedd64..940fa2879 100644 --- a/packages/core/handlers/workers/integration-defined-workers.test.js +++ b/packages/core/handlers/workers/integration-defined-workers.test.js @@ -7,7 +7,6 @@ jest.mock('../../database/config', () => ({ const { createQueueWorker } = require('../backend-utils'); const { IntegrationBase } = require('../../integrations/integration-base'); const { IntegrationEventDispatcher } = require('../integration-event-dispatcher'); -const { RateLimitError } = require('../../errors'); class TestWebhookIntegration extends IntegrationBase { static Definition = { @@ -262,42 +261,6 @@ describe('Webhook Queue Worker', () => { expect(result.batchItemFailures).toEqual([{ itemIdentifier: 'msg-1' }]); }); - it('should NOT mark a 403 with isRateLimited as isHaltError (a provider limit outside 429)', async () => { - const error = new RateLimitError({ - resource: 'https://api.example.com/v1/items', - response: { status: 403, bodyUsed: true }, - hint: { waitMs: 60_000, reason: 'daily', source: 'body' }, - }); - - const FailingIntegration = class extends TestWebhookIntegration { - async onWebhook() { - throw error; - } - }; - - const QueueWorker = createQueueWorker(FailingIntegration); - const worker = new QueueWorker(); - - const sqsEvent = { - Records: [ - { - messageId: 'msg-1', - body: JSON.stringify({ - event: 'ON_WEBHOOK', - data: { body: {} }, - }), - }, - ], - }; - - const result = await worker.run(sqsEvent, {}); - expect(result.batchItemFailures).toEqual([ - { itemIdentifier: 'msg-1' }, - ]); - expect(error.statusCode).toBe(403); - expect(error.isHaltError).toBeUndefined(); - }); - it('should NOT mark 500 as isHaltError (server may recover)', async () => { const error = new Error('Internal Server Error'); error.statusCode = 500; From f067d2afb4564475992b04033d4ac6d5f8196ba7 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 18:40:31 -0300 Subject: [PATCH 32/33] fix(core): drop a custom rate-limit scope that is not a string or number A scope function that was async, or that returned an object, put the same "[object Promise]" or "[object Object]" key on every RateLimitError. Unrelated entities then shared one scope key. Such a value now gives no scope key, like a function that returns nothing. Co-Authored-By: Claude Opus 5.5 --- docs/reference/api-module-definition-and-functions.md | 2 +- packages/core/modules/requester/rate-limit/policy.js | 6 +++--- .../core/modules/requester/rate-limit/policy.test.js | 10 ++++++++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index fa227b299..41c196a39 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -231,7 +231,7 @@ that the runtime does not read: only `static rateLimit` on the API class counts. | Key | Meaning | |---|---| -| `scope` | The key a limit counts against: `'entity'` (default), `'credential'`, `'app'`, or a function `(requester) => key`. The Requester puts it on `RateLimitError.scopeKey`, which is `undefined` when the requester has no id for the scope. | +| `scope` | The key a limit counts against: `'entity'` (default), `'credential'`, `'app'`, or a synchronous function `(requester) => key` that returns a string or a number. The Requester puts it on `RateLimitError.scopeKey`, which is `undefined` when the requester has no id for the scope or the function returns another type. | | `minRetryAfterMs` | A wait never shorter than this. Use it when the docs say "wait 60 seconds" and the response says nothing. | | `parsers` | The header parsers to read, in order: `retryAfter`, `resetHeaders`, `ietf`. Default: all three. A name that is not built in throws a `TypeError` when the API class is constructed. | | `classify` | Recognises a limit that is not a plain 429, or names its reason. See below. | diff --git a/packages/core/modules/requester/rate-limit/policy.js b/packages/core/modules/requester/rate-limit/policy.js index 535610d7e..8635feb5c 100644 --- a/packages/core/modules/requester/rate-limit/policy.js +++ b/packages/core/modules/requester/rate-limit/policy.js @@ -110,9 +110,9 @@ function computeScopeKey(policy, requester) { if (typeof scope === 'function') { try { const value = scope(requester); - return value === undefined || value === null - ? undefined - : `${label}:custom:${String(value)}`; + return typeof value === 'string' || Number.isFinite(value) + ? `${label}:custom:${value}` + : undefined; } catch { return undefined; } diff --git a/packages/core/modules/requester/rate-limit/policy.test.js b/packages/core/modules/requester/rate-limit/policy.test.js index a8298b6c8..2ce5ddf76 100644 --- a/packages/core/modules/requester/rate-limit/policy.test.js +++ b/packages/core/modules/requester/rate-limit/policy.test.js @@ -222,6 +222,16 @@ describe('rate-limit/policy', () => { { scope: () => undefined }, requester({}), ], + [ + 'an async function', + { scope: async () => 'portal-1' }, + requester({}), + ], + [ + 'a function that returns an object', + { scope: () => ({ id: 'portal-1' }) }, + requester({}), + ], ])('returns undefined for %s', (_label, policy, subject) => { expect(computeScopeKey(policy, subject)).toBeUndefined(); }); From 602fc501b2b433a6e220d651c369939ad0ec6245 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Mon, 9 Nov 2026 18:40:31 -0300 Subject: [PATCH 33/33] fix(core): keep the status of the cause on a RateLimitError A module that drives another client builds the error from that client's error and has no fetch response. statusCode was then undefined, though the cause had it. It now falls back to cause.statusCode. Co-Authored-By: Claude Opus 5.5 --- .../api-module-definition-and-functions.md | 3 ++- packages/core/errors/rate-limit-error.js | 1 + packages/core/errors/rate-limit-error.test.js | 18 ++++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/reference/api-module-definition-and-functions.md b/docs/reference/api-module-definition-and-functions.md index 41c196a39..23712f64d 100644 --- a/docs/reference/api-module-definition-and-functions.md +++ b/docs/reference/api-module-definition-and-functions.md @@ -366,7 +366,8 @@ known. An error that `classify` throws propagates unless you pass `new RateLimitError({ hint, waitMs, module, scopeKey, ...fetchErrorArgs })` takes the `FetchError` arguments (`resource`, `init`, `response`, `cause`, `responseBody`). It sets `retryAt` to now plus `waitMs`, or to `hint.retryAt` -when `waitMs` is left out. +when `waitMs` is left out. With no `response`, `statusCode` comes from +`cause.statusCode`. ### A limit in a 2xx body diff --git a/packages/core/errors/rate-limit-error.js b/packages/core/errors/rate-limit-error.js index efead3765..a4b3808c7 100644 --- a/packages/core/errors/rate-limit-error.js +++ b/packages/core/errors/rate-limit-error.js @@ -31,6 +31,7 @@ class RateLimitError extends FetchError { super(fetchErrorArgs); const timing = resolveTiming({ hint, waitMs, now }); + this.statusCode ??= fetchErrorArgs.cause?.statusCode; this.isRateLimited = true; this.waitMs = timing.waitMs; this.retryAt = timing.retryAt; diff --git a/packages/core/errors/rate-limit-error.test.js b/packages/core/errors/rate-limit-error.test.js index c163d7fc9..285304c3c 100644 --- a/packages/core/errors/rate-limit-error.test.js +++ b/packages/core/errors/rate-limit-error.test.js @@ -148,6 +148,24 @@ describe('RateLimitError', () => { expect(error.message).toBe(`POST ${sanitizedUrl} 429`); }); + it('takes the status of the cause when there is no response', () => { + const cause = Object.assign(new Error('REQUEST_LIMIT_EXCEEDED'), { + statusCode: 403, + }); + const error = new RateLimitError({ hint, cause }); + expect(error.statusCode).toBe(403); + }); + + it('keeps the status of the response over the status of the cause', () => { + const cause = Object.assign(new Error('boom'), { statusCode: 403 }); + const error = new RateLimitError({ + hint, + cause, + response: throttledResponse(), + }); + expect(error.statusCode).toBe(429); + }); + it('does not put the hint in the enumerable fields', () => { const error = new RateLimitError({ hint }); expect(Object.keys(error)).not.toContain('hint');