diff --git a/config/default.js b/config/default.js index c4fecc5..c707bf5 100644 --- a/config/default.js +++ b/config/default.js @@ -60,6 +60,13 @@ module.exports = { avoidDuplicates: true, retries: 3, concurrency: 10, + // wall-clock ceiling (ms) for a single item-download attempt (src/fetch/item.js). + // bhttp has no timeout of its own - a request that connects but then just stops + // responding (a stalled/misbehaving proxy tunnel is a common cause) would otherwise + // hang forever, and since the limiter's concurrency is capped, that one stuck request + // blocks every other download queued behind it indefinitely. This bounds each attempt + // so a stall fails (and retries, per `retries` above) instead of hanging forever. + timeout: 60000, watch: { schedule: '*/30 * * * *', }, @@ -102,6 +109,13 @@ module.exports = { proxy: { use: true, default: null, + // if a request fails (including timing out - see fetch.timeout above) and is about to + // be retried, retry it using whichever of proxied/direct is NOT what would normally + // apply for that host - e.g. the proxy itself is down or being blocked by the target + // site, or (the reverse) a host reachable directly is refusing unproxied traffic. + // Off by default: for a privacy-sensitive proxy (e.g. Tor) silently falling back to a + // direct connection on failure is very much not what you want. + retryFlipped: false, hosts: { // hostnames are matched by suffix, so e.g. 'reddit.com' also covers // 'www.reddit.com', but not 'v.redd.it' or 'redd.it' - those need their own diff --git a/src/fetch/item.js b/src/fetch/item.js index 808ea08..882af78 100644 --- a/src/fetch/item.js +++ b/src/fetch/item.js @@ -10,6 +10,21 @@ const { agentFor } = require('../proxy'); const logger = require('../logger')(__filename); const limiter = require('../limiter').items; +// bhttp has no timeout at all by default - if the server (or, more likely, a proxy tunnel +// that connects fine but then never relays anything) just stops responding mid-request without +// erroring, the returned promise never settles. With the limiter's concurrency capped, that one +// stuck request blocks every other download queued behind it, indefinitely. Race it against a +// hard wall-clock ceiling so a stall fails (and retries, below) instead of hanging forever. +function withTimeout(promise, ms, url) { + let timer; + + const timedOut = new Promise((resolve, reject) => { + timer = setTimeout(() => reject(new Error(`Timed out after ${ms}ms fetching '${url}'`)), ms); + }); + + return Promise.race([promise, timedOut]).finally(() => clearTimeout(timer)); +} + async function fetchItem(url, attempt, context) { async function retry(error) { logger.warn(`Failed to fetch '${url}', ${attempt < config.fetch.retries ? 'retrying' : 'giving up'}: ${error.message} (${context.post ? context.post.permalink : 'no post'})`); @@ -23,7 +38,15 @@ async function fetchItem(url, attempt, context) { try { // throw new Error('Failed it!'); - const res = await limiter.schedule(async () => bhttp.get(url, { headers: context.headers, agent: agentFor(url) })); + // config.proxy.retryFlipped: once this url has already failed once, retry it via + // whichever of proxied/direct is NOT what it'd normally get - see resolveProxyUrl() + const flip = config.proxy.retryFlipped && attempt > 0; + + const res = await limiter.schedule(async () => withTimeout( + bhttp.get(url, { headers: context.headers, agent: agentFor(url, { flip }) }), + config.fetch.timeout, + url, + )); if (res.statusCode !== 200) { throw new Error(`Response not OK for ${url} (${res.statusCode})`); diff --git a/src/proxy.js b/src/proxy.js index 15263fd..7624fd9 100644 --- a/src/proxy.js +++ b/src/proxy.js @@ -21,7 +21,7 @@ function hostMatches(hostname, pattern) { return hostname === normalized || hostname.endsWith(`.${normalized}`); } -function resolveProxyUrl(hostname) { +function resolveProxyUrl(hostname, { flip = false } = {}) { if (config.proxy?.use === false) { return null; } @@ -29,19 +29,32 @@ function resolveProxyUrl(hostname) { const hosts = config.proxy?.hosts || {}; const matchedPattern = Object.keys(hosts).find((pattern) => hostMatches(hostname, pattern)); - if (!matchedPattern) { - return config.proxy?.default || null; + const normal = (() => { + if (!matchedPattern) { + return config.proxy?.default || null; + } + + const matched = hosts[matchedPattern]; + + // `true` explicitly opts this host into `default`, `null`/`false`/'' explicitly opts it + // out (rather than falling through to `default`), anything else is a per-host override + if (matched === true) { + return config.proxy?.default || null; + } + + return matched || null; + })(); + + if (!flip) { + return normal; } - const matched = hosts[matchedPattern]; - - // `true` explicitly opts this host into `default`, `null`/`false`/'' explicitly opts it - // out (rather than falling through to `default`), anything else is a per-host override - if (matched === true) { - return config.proxy?.default || null; - } - - return matched || null; + // used for config.proxy.retryFlipped: retry via whichever of proxied/direct is NOT what + // this host would normally get. A request that would normally be proxied (whether via + // `default` or a per-host override) retries direct; a request that would normally go + // direct (no proxy configured for this host, or explicitly excluded from `default`) + // retries through `default`, if one is set. + return normal ? null : (config.proxy?.default || null); } // 'socks4a'/'socks5h' (as opposed to plain 'socks4'/'socks5') tell socks-proxy-agent to @@ -55,9 +68,9 @@ function isSocks(proxyUrl) { // undici (which node's built-in fetch() is implemented with) only tunnels through http(s) // proxies (CONNECT) via ProxyAgent - it has no built-in SOCKS support. agentFor() below can // still do SOCKS, since bhttp goes through node's plain http/https modules instead. -function dispatcherFor(targetUrl) { +function dispatcherFor(targetUrl, { flip = false } = {}) { const { hostname } = new URL(targetUrl); - const proxyUrl = resolveProxyUrl(hostname); + const proxyUrl = resolveProxyUrl(hostname, { flip }); if (!proxyUrl) { return undefined; @@ -81,9 +94,9 @@ function dispatcherFor(targetUrl) { // bhttp uses node's raw http/https modules under the hood, which take a plain Agent // instance - unlike fetch() above, this does support SOCKS -function agentFor(targetUrl) { +function agentFor(targetUrl, { flip = false } = {}) { const url = new URL(targetUrl); - const proxyUrl = resolveProxyUrl(url.hostname); + const proxyUrl = resolveProxyUrl(url.hostname, { flip }); if (!proxyUrl) { return undefined; @@ -105,11 +118,26 @@ function agentFor(targetUrl) { } // drop-in replacement for the global fetch() - importing this as `fetch` transparently -// proxies requests per config.proxy, if one is configured for the request's host +// proxies requests per config.proxy, if one is configured for the request's host. Also +// applies a hard wall-clock ceiling (config.fetch.timeout) to every request: unlike bhttp, +// fetch() doesn't hang *forever* on a stalled/black-holed connection (undici defaults to a +// 300s headers/body timeout), but that default is high enough - and, per live testing, a +// proxy CONNECT tunnel that never completes can still take well over 20s to surface any +// error - to look like a freeze, especially since reddit/client.js's requestJson() serializes +// all reddit API calls behind a single-concurrency limiter. Bounding it here covers every +// fetch()-based call site (reddit's API, and every hosting site's info-lookup) at once. +// Pass `proxyFlip: true` in options (stripped before reaching fetch()) to use whichever of +// proxied/direct is NOT what this host would normally get - see resolveProxyUrl(). async function proxiedFetch(url, options = {}) { - const dispatcher = dispatcherFor(String(url)); + const { proxyFlip, signal, ...fetchOptions } = options; + const dispatcher = dispatcherFor(String(url), { flip: proxyFlip }); + const timeoutSignal = AbortSignal.timeout(config.fetch.timeout); - return fetch(url, dispatcher ? { ...options, dispatcher } : options); + return fetch(url, { + ...fetchOptions, + ...(dispatcher ? { dispatcher } : {}), + signal: signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal, + }); } module.exports = { diff --git a/src/reddit/client.js b/src/reddit/client.js index b9e1f6b..2c612ab 100644 --- a/src/reddit/client.js +++ b/src/reddit/client.js @@ -111,43 +111,69 @@ async function wait(seconds) { async function requestJson(path, params = {}, attempt = 0) { const url = buildUrl(path, params); - return limiter.schedule(async () => { - const res = await fetch(url.toString(), { headers: headers() }); - const contentType = res.headers.get('content-type') || ''; + // Only the actual throttled network call belongs inside limiter.schedule() - this has + // maxConcurrent: 1, and every retry path below used to recurse back into requestJson() + // (and so back into limiter.schedule()) from *inside* that same callback. Bottleneck won't + // start a second job while the first is still running, and the first can't finish until + // its awaited recursive call starts - a guaranteed deadlock the instant a retry was ever + // needed (e.g. a single 429), freezing every reddit API call behind it for good. Retrying + // now happens here, after the scheduled call has actually returned and freed its slot. + let res; - if (res.status === 429 || res.status === 503) { - if (attempt >= MAX_RETRIES) { - throw new Error(`Reddit rate-limited request after ${attempt} retries (${res.status}) for '${path}'`); - } - - const retryAfter = Number(res.headers.get('retry-after')) || 2 ** attempt; - - logger.warn(`Rate limited by reddit (${res.status}) for '${path}', retrying in ${retryAfter}s`); - - await wait(retryAfter); - - return requestJson(path, params, attempt + 1); + try { + // config.proxy.retryFlipped: once this request has already failed once, retry it via + // whichever of proxied/direct is NOT what it'd normally get - see proxy.js + res = await limiter.schedule(async () => fetch(url.toString(), { + headers: headers(), + proxyFlip: config.proxy.retryFlipped && attempt > 0, + })); + } catch (error) { + // fetch() has no timeout of its own beyond undici's fairly generous defaults (see + // proxy.js) - this catches both that and genuine network errors (DNS, connection + // refused/reset, etc.), neither of which was retried before + if (attempt >= MAX_RETRIES) { + throw new Error(`Failed to reach reddit after ${attempt} retries for '${path}': ${error.message}`); } - if (!contentType.includes('json')) { - // reddit serves an HTML interstitial/block page instead of JSON when it decides - // the request looks like a bot - this is a distinct failure mode from a genuine - // 4xx and should be logged/handled separately rather than treated as "bad request" - const hint = cookieHeader() ? ' - session cookie may have expired, refresh config/cookies.js' : ' - likely blocked or interstitial page (consider setting config.reddit.cookies)'; + logger.warn(`Failed to reach reddit for '${path}', retrying: ${error.message}`); - throw new Error(`Reddit returned non-JSON response (${res.status}, content-type '${contentType || 'none'}') for '${path}'${hint}`); + return requestJson(path, params, attempt + 1); + } + + const contentType = res.headers.get('content-type') || ''; + + if (res.status === 429 || res.status === 503) { + if (attempt >= MAX_RETRIES) { + throw new Error(`Reddit rate-limited request after ${attempt} retries (${res.status}) for '${path}'`); } - const data = await res.json(); + const retryAfter = Number(res.headers.get('retry-after')) || 2 ** attempt; - if (!res.ok) { - const message = data?.message || data?.reason || JSON.stringify(data).slice(0, 200); + logger.warn(`Rate limited by reddit (${res.status}) for '${path}', retrying in ${retryAfter}s`); - throw new Error(`Reddit returned ${res.status} for '${path}': ${message}`); - } + await wait(retryAfter); - return data; - }); + return requestJson(path, params, attempt + 1); + } + + if (!contentType.includes('json')) { + // reddit serves an HTML interstitial/block page instead of JSON when it decides + // the request looks like a bot - this is a distinct failure mode from a genuine + // 4xx and should be logged/handled separately rather than treated as "bad request" + const hint = cookieHeader() ? ' - session cookie may have expired, refresh config/cookies.js' : ' - likely blocked or interstitial page (consider setting config.reddit.cookies)'; + + throw new Error(`Reddit returned non-JSON response (${res.status}, content-type '${contentType || 'none'}') for '${path}'${hint}`); + } + + const data = await res.json(); + + if (!res.ok) { + const message = data?.message || data?.reason || JSON.stringify(data).slice(0, 200); + + throw new Error(`Reddit returned ${res.status} for '${path}': ${message}`); + } + + return data; } async function getUser(username) {