d0ca479d4f
throttle() read lastRequestAt, awaited delay(), then wrote it back — a
TOCTOU race with no mutex or queue, under concurrency: 10. Ten jobs read the
same timestamp, slept the same duration and fired in the same tick, giving
up to ~10 req/s against MusicBrainz's 1 req/s policy and risking an IP
block.
Replaced the lastRequestAt map with a per-host { lastRequestAt, tail }
limiter; each call links onto that host's promise chain, so the
read-sleep-write critical section is serialized and N concurrent callers
space out by minIntervalMs. Chain rejections are swallowed so one failure
cannot poison the queue. Per-host rather than global, so other integrations
are not starved by MusicBrainz.
Measured: 5 concurrent same-host calls at 200ms -> 802ms (previously all in
one tick); 3 distinct hosts at 1000ms -> 0ms, confirming no cross-host
starvation.
musicbrainz.client caught HttpError and returned null at all 7 catch sites,
making a rate-limited MusicBrainz indistinguishable from "no data for your
library" while every job reported success. A shared logMbFailure() now logs
429 (and 503 whose body mentions a rate limit) at error, stating results are
INCOMPLETE. The error model is otherwise unchanged.
REVIEW-2026-07-30.md finding 8.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
312 lines
11 KiB
TypeScript
312 lines
11 KiB
TypeScript
// HTTP base shared by every external-integration client.
|
|
//
|
|
// Provides:
|
|
// - configurable User-Agent (REQUIRED by MusicBrainz, format:
|
|
// `muzick/0.1 ( <contact> )`)
|
|
// - SOCKS5 proxy support via SOCKS_PROXY_URL env var
|
|
// (e.g. `socks5://127.0.0.1:10808`; no proxy when unset/empty)
|
|
// - exponential backoff with jitter on 429 / 5xx / network errors,
|
|
// honouring a `Retry-After` header when present
|
|
// - a simple per-host rate limit (min interval between requests to the same
|
|
// host) so e.g. MusicBrainz's <=1 req/sec rule is respected
|
|
// - typed JSON parsing via generics and a typed error on persistent failure.
|
|
//
|
|
// Why not global fetch? Node's built-in fetch doesn't support proxy agents.
|
|
// When SOCKS_PROXY_URL is set we build a proxy-aware fetcher via
|
|
// socks-proxy-agent + native https.request. Otherwise we use the global fetch
|
|
// (faster path, no overhead).
|
|
|
|
import * as http from 'http';
|
|
import * as https from 'https';
|
|
import { SocksProxyAgent } from 'socks-proxy-agent';
|
|
|
|
/** Error thrown when a request ultimately fails (non-2xx after all retries). */
|
|
export class HttpError extends Error {
|
|
constructor(
|
|
message: string,
|
|
readonly status: number,
|
|
readonly url: string,
|
|
readonly body?: string
|
|
) {
|
|
super(message);
|
|
this.name = 'HttpError';
|
|
}
|
|
}
|
|
|
|
// ── Proxy-aware fetch factory ────────────────────────────────────────────────
|
|
|
|
const SOCKS_PROXY_URL = process.env.SOCKS_PROXY_URL || '';
|
|
|
|
/**
|
|
* Return a `fetch`-compatible function that routes through the configured
|
|
* SOCKS5 proxy (if any), or the bare global fetch when no proxy is needed.
|
|
*/
|
|
function createFetch(): typeof fetch {
|
|
if (!SOCKS_PROXY_URL) return globalThis.fetch.bind(globalThis);
|
|
|
|
const agent = new SocksProxyAgent(SOCKS_PROXY_URL);
|
|
|
|
// Internal helper: issue a single request (no redirect following).
|
|
function requestOnce(
|
|
url: string,
|
|
method: string,
|
|
headers: Record<string, string> | undefined,
|
|
body: BodyInit | undefined,
|
|
signal: AbortSignal | undefined,
|
|
): Promise<Response> {
|
|
const urlObj = new URL(url);
|
|
const mod = urlObj.protocol === 'https:' ? https : http;
|
|
|
|
return new Promise<Response>((resolve, reject) => {
|
|
const req = mod.request(url, { agent, method, headers, signal }, (res) => {
|
|
const chunks: Buffer[] = [];
|
|
res.on('data', (chunk: Buffer) => chunks.push(chunk));
|
|
res.on('end', () => {
|
|
resolve(
|
|
new Response(Buffer.concat(chunks), {
|
|
status: res.statusCode,
|
|
statusText: res.statusMessage || '',
|
|
headers: new Headers(
|
|
Object.entries(res.headers).reduce(
|
|
(acc, [k, v]) => {
|
|
acc[k] = Array.isArray(v) ? v.join(', ') : String(v ?? '');
|
|
return acc;
|
|
},
|
|
{} as Record<string, string>,
|
|
)
|
|
),
|
|
})
|
|
);
|
|
});
|
|
});
|
|
req.on('error', reject);
|
|
if (body) req.write(body);
|
|
req.end();
|
|
});
|
|
}
|
|
|
|
const MAX_REDIRECTS = 5;
|
|
|
|
return async (input, init): Promise<Response> => {
|
|
let url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url;
|
|
const method = init?.method || 'GET';
|
|
const headers = init?.headers as Record<string, string> | undefined;
|
|
const body = init?.body ?? undefined;
|
|
const signal = init?.signal ?? undefined;
|
|
|
|
for (let i = 0; i <= MAX_REDIRECTS; i++) {
|
|
const res = await requestOnce(url, method, headers, body, signal);
|
|
const code = res.status;
|
|
|
|
// Follow redirect (301, 302, 307, 308) — max 5 hops.
|
|
if (code === 301 || code === 302 || code === 307 || code === 308) {
|
|
const location = res.headers.get('location');
|
|
if (!location) return res;
|
|
url = new URL(location, url).href;
|
|
continue;
|
|
}
|
|
|
|
return res;
|
|
}
|
|
|
|
// Too many redirects.
|
|
return new Response(null, { status: 508, statusText: 'Loop Detected' });
|
|
};
|
|
}
|
|
|
|
// Singleton fetcher — created once at module load.
|
|
const fetchFn = createFetch();
|
|
|
|
export interface RequestJsonOptions {
|
|
/** Full User-Agent header value. Required for MusicBrainz; recommended always. */
|
|
userAgent: string;
|
|
/** Minimum interval (ms) between requests to the same host. Default 1000. */
|
|
minIntervalMs?: number;
|
|
/** Max retry attempts on transient failures. Default 5. */
|
|
maxRetries?: number;
|
|
/** Base backoff delay (ms). Default 500. */
|
|
baseDelayMs?: number;
|
|
/** Cap on a single backoff delay (ms). Default 20000. */
|
|
maxDelayMs?: number;
|
|
/** Extra request headers (e.g. Accept). */
|
|
headers?: Record<string, string>;
|
|
/** Per-request timeout (ms). Default 15000. */
|
|
timeoutMs?: number;
|
|
}
|
|
|
|
// Per-host rate-limit state. Module-scoped so all clients sharing a host
|
|
// coordinate automatically.
|
|
//
|
|
// `tail` is the promise chain for a host: every throttle() call links onto the
|
|
// host's current tail, so slot acquisition is strictly serialized. Without this
|
|
// chain the old implementation was a TOCTOU race — under the worker's
|
|
// `concurrency: 10`, ten jobs read the same `lastRequestAt`, slept the same
|
|
// duration and fired in the same tick, i.e. ~10 req/s against MusicBrainz's
|
|
// 1 req/s policy. Serialization is deliberately PER HOST (not global) so a slow
|
|
// MusicBrainz queue cannot starve Last.fm, Discogs, cover art, etc.
|
|
interface HostLimiter {
|
|
/** Timestamp of the last granted request slot. */
|
|
lastRequestAt: number;
|
|
/** Tail of the serialization chain; resolves when the previous waiter is done. */
|
|
tail: Promise<void>;
|
|
}
|
|
|
|
const hostLimiters = new Map<string, HostLimiter>();
|
|
|
|
const delay = (ms: number): Promise<void> =>
|
|
new Promise((resolve) => setTimeout(resolve, ms));
|
|
|
|
function hostOf(url: string): string {
|
|
try {
|
|
return new URL(url).host;
|
|
} catch {
|
|
return url;
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Acquire this host's next request slot, blocking until at least
|
|
* `minIntervalMs` has elapsed since the previously granted slot.
|
|
*
|
|
* Concurrent callers for the same host queue strictly in arrival order: each
|
|
* one appends to `limiter.tail` and only computes its wait once every earlier
|
|
* waiter has already claimed its timestamp, so N concurrent callers are spaced
|
|
* `minIntervalMs` apart rather than all firing at once.
|
|
*/
|
|
function throttle(host: string, minIntervalMs: number): Promise<void> {
|
|
let limiter = hostLimiters.get(host);
|
|
if (!limiter) {
|
|
limiter = { lastRequestAt: 0, tail: Promise.resolve() };
|
|
hostLimiters.set(host, limiter);
|
|
}
|
|
const lim = limiter;
|
|
|
|
// Link onto the tail. The critical section (read lastRequestAt → sleep →
|
|
// write lastRequestAt) runs only after the previous waiter finished it.
|
|
const slot = lim.tail.then(async () => {
|
|
const wait = minIntervalMs - (Date.now() - lim.lastRequestAt);
|
|
if (wait > 0) await delay(wait);
|
|
lim.lastRequestAt = Date.now();
|
|
});
|
|
|
|
// The next caller waits for this slot. Swallow rejections on the chain itself
|
|
// so one failure can never poison the queue for subsequent requests.
|
|
lim.tail = slot.catch(() => {});
|
|
return slot;
|
|
}
|
|
|
|
/**
|
|
* Test-only handle on the per-host throttle, so its serialization can be
|
|
* asserted without issuing real network requests. Not used in production code.
|
|
*/
|
|
export const __throttleForTest = throttle;
|
|
|
|
/** Parse a Retry-After header (delta-seconds or HTTP date) into ms, or null. */
|
|
function parseRetryAfter(value: string | null): number | null {
|
|
if (!value) return null;
|
|
const secs = Number(value);
|
|
if (Number.isFinite(secs)) return Math.max(0, secs * 1000);
|
|
const when = Date.parse(value);
|
|
if (!Number.isNaN(when)) return Math.max(0, when - Date.now());
|
|
return null;
|
|
}
|
|
|
|
/** Exponential backoff with full jitter, capped at maxDelayMs. */
|
|
function backoffDelay(attempt: number, baseDelayMs: number, maxDelayMs: number): number {
|
|
const exp = Math.min(maxDelayMs, baseDelayMs * 2 ** attempt);
|
|
return Math.floor(Math.random() * exp);
|
|
}
|
|
|
|
/**
|
|
* Fetch a URL and parse the JSON body as `T`. Retries transient failures with
|
|
* backoff + jitter; enforces the per-host rate limit. Throws `HttpError` on a
|
|
* persistent non-2xx response and rethrows the last network error if all
|
|
* attempts fail.
|
|
*/
|
|
export async function requestJson<T>(
|
|
url: string,
|
|
opts: RequestJsonOptions
|
|
): Promise<T> {
|
|
const {
|
|
userAgent,
|
|
minIntervalMs = 1000,
|
|
maxRetries = 5,
|
|
baseDelayMs = 500,
|
|
maxDelayMs = 20000,
|
|
headers = {},
|
|
timeoutMs = 15000,
|
|
} = opts;
|
|
|
|
const host = hostOf(url);
|
|
let lastError: unknown;
|
|
|
|
for (let attempt = 0; attempt <= maxRetries; attempt++) {
|
|
await throttle(host, minIntervalMs);
|
|
|
|
const controller = new AbortController();
|
|
const timer = setTimeout(() => controller.abort(), timeoutMs);
|
|
try {
|
|
const res = await fetchFn(url, {
|
|
headers: {
|
|
'User-Agent': userAgent,
|
|
Accept: 'application/json',
|
|
...headers,
|
|
},
|
|
signal: controller.signal,
|
|
});
|
|
|
|
// Retry on 429 (rate limited) and 5xx (transient server errors).
|
|
if (res.status === 429 || res.status >= 500) {
|
|
if (attempt < maxRetries) {
|
|
const retryAfter = parseRetryAfter(res.headers.get('retry-after'));
|
|
await delay(retryAfter ?? backoffDelay(attempt, baseDelayMs, maxDelayMs));
|
|
continue;
|
|
}
|
|
const body = await res.text().catch(() => undefined);
|
|
throw new HttpError(
|
|
`Request failed after ${maxRetries} retries: ${res.status} ${res.statusText}`,
|
|
res.status,
|
|
url,
|
|
body
|
|
);
|
|
}
|
|
|
|
if (!res.ok) {
|
|
const body = await res.text().catch(() => undefined);
|
|
throw new HttpError(
|
|
`Request failed: ${res.status} ${res.statusText}`,
|
|
res.status,
|
|
url,
|
|
body
|
|
);
|
|
}
|
|
|
|
return (await res.json()) as T;
|
|
} catch (err) {
|
|
// Non-retryable application errors propagate immediately.
|
|
if (err instanceof HttpError) throw err;
|
|
|
|
// Network / abort errors: retry with backoff, otherwise rethrow.
|
|
lastError = err;
|
|
if (attempt < maxRetries) {
|
|
await delay(backoffDelay(attempt, baseDelayMs, maxDelayMs));
|
|
continue;
|
|
}
|
|
throw err;
|
|
} finally {
|
|
clearTimeout(timer);
|
|
}
|
|
}
|
|
|
|
// Unreachable in practice; satisfies the type checker.
|
|
throw lastError instanceof Error
|
|
? lastError
|
|
: new Error('requestJson: exhausted retries');
|
|
}
|
|
|
|
/** Build the MusicBrainz-compliant User-Agent string. */
|
|
export function buildUserAgent(contact: string): string {
|
|
const c = contact && contact.trim() !== '' ? contact.trim() : 'no-contact-configured';
|
|
return `muzick/0.1 ( ${c} )`;
|
|
}
|