From 8a41647426a70f808848ca97c287265723863ae7 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 19 Aug 2026 17:30:16 +0000 Subject: [PATCH 1/2] Carry a contact's provenance, which the crawler was failing without (#140) Every crawl of a feed that publishes a contact address but names nobody has been failing at the write since author enrichment shipped. In the hour this was found, 985 of 1,385 crawls errored; the queue stopped draining and /crawlstats went with it. `feedContacts` built each contact as `{ url, network }` and dropped the channel element it came from. Both `feed_links.source` and `author_links.source` are `not null`, so the statement bound `undefined` -- which the remote libSQL client will not serialize at all. It throws `Unsupported type of value` before any SQL runs, with no column named and no row to point at, and the crawl recorded it as `could not be crawled`: a publisher who looks down. The population is large and it is not a platform quirk. Any feed with a `` or `managingEditor` address whose name fails the person test takes this path -- WordPress and Substack alike, and Substack additionally names nobody else, so every newsletter on it qualified. Nothing caught it because the local SQLite driver the tests use binds `undefined` as null without complaint. The difference only exists on the wire, so `link-binds.test.js` asserts what the remote client accepts rather than what a local write happens to survive. Two fixes, because either alone leaves a hole: contacts now carry `source` (provenance worth keeping in its own right -- a mailbox from `itunes:owner` is a stronger claim than one from `webMaster`), and the four link bind sites default it, so no caller can put an unbindable value in a not-null column again. Also: a newsletter whose host fills in the iTunes block is no longer a podcast. Substack emits `` on every publication it serves and nothing else that looks like a show -- no `itunes:type`, no `podcast:` namespace, image enclosures rather than audio -- and that one tag filed the whole platform under /podcasts. This is the correction the video branch already makes: the tag has to be corroborated by what the feed actually ships. A declared show still stands on its own, so a podcast that has not released an episode yet keeps its category. Co-Authored-By: Claude Opus 5 (1M context) --- packages/db/src/authors.js | 33 ++++++++-- packages/db/test/link-binds.test.js | 98 +++++++++++++++++++++++++++++ packages/feed/src/identity.js | 10 ++- packages/feed/src/parse.js | 48 +++++++++++++- packages/feed/test/contacts.test.js | 43 ++++++++++++- packages/feed/test/parse.test.js | 56 +++++++++++++++++ 6 files changed, 276 insertions(+), 12 deletions(-) create mode 100644 packages/db/test/link-binds.test.js diff --git a/packages/db/src/authors.js b/packages/db/src/authors.js index 2413215..98c59f6 100644 --- a/packages/db/src/authors.js +++ b/packages/db/src/authors.js @@ -240,7 +240,7 @@ export async function addAuthorLinks(db, authorId, links) { link.network, link.url, link.handle || null, - link.source, + linkSource(link), link.verified ? 1 : 0, nowIso(), ], @@ -301,7 +301,7 @@ export async function addFeedLinks(db, feedId, links) { link.network, link.url, link.handle || null, - link.source, + linkSource(link), link.verified ? 1 : 0, nowIso(), ], @@ -685,6 +685,31 @@ export async function feedHasAuthors(db, feedId) { * @param {Array} [input.feedLinks] accounts to file under the feed * @returns {Array<{ sql: string, args: unknown[] }>} in dependency order */ +/** + * Where a link was found, as a string the database will accept. + * + * `author_links.source` and `feed_links.source` are both `not null`, and this is + * the last place before the wire that can say so. It matters more than a + * defensive default usually does, because of *how* the remote client fails: + * `undefined` is not a bindable libSQL value, so hrana throws `Unsupported type + * of value` while the statement is being serialized -- before any SQL runs, with + * no column named and no row to point at. That error surfaced as + * `could not be crawled`, which reads like a publisher who is down. + * + * It cost the directory a day. Every Substack newsletter emits `` + * and no other byline, so `feedContacts` harvested the mailbox, dropped the + * provenance, and every one of those feeds failed its crawl at the write -- + * 985 of 1,385 crawls in the hour this was found. Local SQLite binds `undefined` + * as null without complaint, so the tests and every local run passed. + * + * @param {{ source?: unknown }} link + * @returns {string} + */ +function linkSource(link) { + const source = link?.source; + return typeof source === 'string' && source !== '' ? source : 'feed-document'; +} + export function creditStatements({ feedId, identityKey, slug, person, authorLinks = [], feedLinks = [] }) { const now = nowIso(); const statements = []; @@ -765,7 +790,7 @@ export function creditStatements({ feedId, identityKey, slug, person, authorLink link.network, link.url, link.handle || null, - link.source, + linkSource(link), link.verified ? 1 : 0, now, identityKey, @@ -802,7 +827,7 @@ export function feedLinkStatements(feedId, links) { link.network, link.url, link.handle || null, - link.source, + linkSource(link), link.verified ? 1 : 0, nowIso(), ], diff --git a/packages/db/test/link-binds.test.js b/packages/db/test/link-binds.test.js new file mode 100644 index 0000000..86ca02d --- /dev/null +++ b/packages/db/test/link-binds.test.js @@ -0,0 +1,98 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; + +import { creditStatements, feedLinkStatements } from '../src/authors.js'; + +/** + * Everything the remote libSQL client will accept as a bound parameter. + * + * Mirrors `valueToProto` in @libsql/hrana-client: null, string, finite number, + * bigint, boolean, ArrayBuffer, Uint8Array, Date and any other object (which is + * stringified). What is left -- `undefined`, symbols and functions -- throws + * `TypeError: Unsupported type of value` during serialization. + * + * This is asserted here rather than left to an integration test because the + * local SQLite driver used by every test in this repo does *not* share the + * restriction: it binds `undefined` as null and passes. The only place the + * difference shows up is production. + * + * @param {unknown} value + * @returns {boolean} + */ +function bindable(value) { + if (value === null) return true; + const type = typeof value; + if (type === 'undefined' || type === 'symbol' || type === 'function') return false; + if (type === 'number') return Number.isFinite(value); + return true; +} + +/** + * @param {Array<{ sql: string, args: unknown[] }>} statements + */ +function assertBindable(statements) { + for (const statement of statements) { + for (const [index, arg] of statement.args.entries()) { + assert.ok( + bindable(arg), + `arg ${index} (${String(arg)}) cannot be bound: ${statement.sql.slice(0, 80)}`, + ); + } + } +} + +const person = { + name: 'Marta Nowak', + normName: 'marta nowak', + bio: '', + avatarUrl: '', + siteUrl: '', + email: 'marta@example.com', + confidence: 0.85, + role: 'owner', + evidence: 'itunes-owner', +}; + +test('a link that names no source is still storable', () => { + // The crawler-stopping bug, at the layer that has to be right whatever the + // callers do. `feed_links.source` and `author_links.source` are `not null`, + // and a link arriving without one used to bind `undefined` -- which the + // remote client refuses outright, failing the entire crawl transaction that + // carried the feed row and its posts. Every Substack newsletter in the + // directory produced exactly this shape. + const link = { network: 'email', url: 'mailto:marta@example.com' }; + + const statements = creditStatements({ + feedId: 'feed-1', + identityKey: 'marta@example.com', + slug: 'marta-nowak', + person, + authorLinks: [link], + feedLinks: [link], + }); + + assertBindable(statements); + assertBindable(feedLinkStatements('feed-1', [link])); +}); + +test('a link that does name its source keeps it', () => { + const link = { network: 'email', url: 'mailto:marta@example.com', source: 'rel-me' }; + const [statement] = feedLinkStatements('feed-1', [link]); + + assert.ok(statement.args.includes('rel-me')); + assertBindable([statement]); +}); + +test('a whole credit binds cleanly when the person is bare', () => { + // A credit carrying nothing but a name -- no bio, no avatar, no site, no + // email -- is the common case on the small web, and every one of those holes + // is a bound parameter. + const statements = creditStatements({ + feedId: 'feed-1', + identityKey: 'someone@example.com', + slug: 'someone', + person: { name: 'Someone', confidence: 0.4, role: 'author' }, + }); + + assertBindable(statements); +}); diff --git a/packages/feed/src/identity.js b/packages/feed/src/identity.js index 9b3f8c4..6f0db38 100644 --- a/packages/feed/src/identity.js +++ b/packages/feed/src/identity.js @@ -749,10 +749,16 @@ export function channelCreditInputs(channel, format, base = '') { * routed it there -- collecting it again here would file one address in two * places and count it twice. * + * Each contact carries the channel element it was read from, the same way a + * credit does. `feed_links.source` is `not null` and is what the page means by + * "where we found this" -- a mailbox harvested from `webMaster` is a weaker + * claim than one from `itunes:owner`, and dropping the provenance here lost + * that distinction *and* left the column with nothing to store. + * * @param {any} channel the raw parsed channel/feed object * @param {'rss'|'atom'|'rdf'|'json'} format * @param {string} [base] - * @returns {Array<{ url: string, network: string }>} + * @returns {Array<{ url: string, network: string, source: string }>} */ export function feedContacts(channel, format, base = '') { const out = []; @@ -776,7 +782,7 @@ export function feedContacts(channel, format, base = '') { const link = candidate ? classifyLink(candidate, base) : null; if (!link || seen.has(link.url)) continue; seen.add(link.url); - out.push({ url: link.url, network: link.network }); + out.push({ url: link.url, network: link.network, source: input.source }); } } diff --git a/packages/feed/src/parse.js b/packages/feed/src/parse.js index 6b0edd5..a7ee75f 100644 --- a/packages/feed/src/parse.js +++ b/packages/feed/src/parse.js @@ -118,6 +118,31 @@ function hasVideoEnclosure(item) { ); } +/** + * Does this element carry audio? + * + * @param {any} item + * @returns {boolean} + */ +function hasAudioEnclosure(item) { + if (arr(item?.enclosure).some((e) => AUDIO_TYPE.test(String(e?.['@type'] ?? '')))) return true; + return arr(item?.link).some( + (l) => l?.['@rel'] === 'enclosure' && AUDIO_TYPE.test(String(l?.['@type'] ?? '')), + ); +} + +/** + * Tags no platform sets unless it is publishing a show. + * + * The rest of `PODCAST_CHANNEL_TAGS` is weaker than it looks. Substack emits + * `` on every publication it hosts, podcast or not, so on its own + * that tag files a text newsletter under podcasts -- and Substack is thousands + * of feeds here. `itunes:type` and the `podcast:` namespace are different: they + * are written by podcast hosting, for podcast directories, and nothing else has + * a reason to emit them. + */ +const PODCAST_DECLARED_TAGS = ['itunes:type', 'podcast:guid', 'podcast:medium']; + /** * How much prose an item carries of its own, in characters of text. * @@ -406,8 +431,9 @@ function isNewsroom(channel, items) { * guessing. Then YouTube, because a channel feed says so in its own namespace * and nothing else needs weighing. Then video, which is an enclosure *and* * corroboration that the enclosure is the point. Then podcast, which is a - * publisher who filled in the podcast namespaces. Everything else is a blog, - * which is what the overwhelming majority of the directory is. + * publisher who filled in the podcast namespaces *and* ships audio, or who + * declared a show outright. Everything else is a blog, which is what the + * overwhelming majority of the directory is. * * One correction, arrived at twice from opposite ends of the directory: an * attachment is not a genre. A post with a file on it is still a post, and @@ -455,7 +481,23 @@ function kindOfChannel(channel, items) { const withVideo = sample.filter(hasVideoEnclosure); if (withVideo.length > 0 && (podcastTags || isShowShaped(sample, withVideo))) return KIND_VIDEO; - if (podcastTags) return KIND_PODCAST; + // A podcast publishes audio, and the same correction applies here as to video + // above: the tag has to be corroborated. A show attaches an episode to every + // entry, so one audio enclosure anywhere in the sample is enough -- and a feed + // with podcast tags and not a single audio file in five items is a newsletter + // whose host fills in the iTunes block. Substack does that for every + // publication it serves, which put thousands of text newsletters under + // /podcasts. + // + // A declaration still stands on its own: `itunes:type` and the `podcast:` + // namespace are the publisher stating what they made, and a show that has not + // released its first episode yet is still a podcast. So is a feed we have no + // items for -- with nothing sampled there is no evidence to corroborate, and + // the tags are the only thing said. + const declaredPodcast = PODCAST_DECLARED_TAGS.some((tag) => channel?.[tag] !== undefined); + if (podcastTags && (declaredPodcast || sample.length === 0 || sample.some(hasAudioEnclosure))) { + return KIND_PODCAST; + } // No audio branch, deliberately. Audio without a declared medium is a blog // that narrated itself, and `declaredMedium` above is the only way to music. diff --git a/packages/feed/test/contacts.test.js b/packages/feed/test/contacts.test.js index f4df67f..bcd7b9e 100644 --- a/packages/feed/test/contacts.test.js +++ b/packages/feed/test/contacts.test.js @@ -15,7 +15,7 @@ test('a feed whose only byline is a role keeps the mailbox that role published', assert.deepEqual(feedCredits(channel, [], 'rss'), []); assert.deepEqual(feedContacts(channel, 'rss'), [ - { url: 'mailto:marta@example.com', network: 'email' }, + { url: 'mailto:marta@example.com', network: 'email', source: 'itunes-owner' }, ]); }); @@ -54,7 +54,7 @@ test('a profile published beside a rejected name is kept as the feed’s', () => assert.deepEqual(feedCredits(channel, [], 'atom'), []); assert.deepEqual(feedContacts(channel, 'atom'), [ - { url: 'https://github.com/wirecutter', network: 'github' }, + { url: 'https://github.com/wirecutter', network: 'github', source: 'atom-feed-author' }, ]); }); @@ -77,8 +77,10 @@ test('the same address published twice is one contact', () => { 'itunes:owner': { 'itunes:name': 'Editorial Team', 'itunes:email': 'marta@example.com' }, }; + // The first element to publish it is the one credited with finding it, so a + // deduplicated address keeps the stronger provenance rather than the last. assert.deepEqual(feedContacts(channel, 'rss'), [ - { url: 'mailto:marta@example.com', network: 'email' }, + { url: 'mailto:marta@example.com', network: 'email', source: 'managing-editor' }, ]); }); @@ -86,3 +88,38 @@ test('a feed that credits nobody at all offers no contacts', () => { assert.deepEqual(feedContacts({ title: 'A Blog' }, 'rss'), []); assert.deepEqual(feedContacts(null, 'rss'), []); }); + +test('every contact says where it was found, because the column demands it', () => { + // The bug this pins, and it stopped the crawler for a day. + // + // `feed_links.source` and `author_links.source` are both `not null`, and a + // contact used to be built as `{ url, network }` with the provenance dropped. + // The remote libSQL client cannot bind `undefined` at all -- it throws + // `Unsupported type of value` while serializing the statement, before any SQL + // runs -- so the whole crawl failed at the write and the feed was recorded as + // uncrawlable. Local SQLite binds it as null without complaining, which is + // why every test and every local run passed. + // + // Substack is the population that found it: it emits `` on + // every publication it hosts and no other byline, so every Substack + // newsletter in the directory took this path. + // Copied from https://nemtodamulher.substack.com/feed, which is the shape + // every publication on that platform ships: a webMaster address, and an + // iTunes block naming the publication rather than a person. + const channel = { + title: 'Newsletter Nem Toda Mulher', + webMaster: 'nemtodamulher@substack.com', + 'itunes:author': 'Newsletter Nem Toda Mulher', + 'itunes:owner': { + 'itunes:name': 'Newsletter Nem Toda Mulher', + 'itunes:email': 'nemtodamulher@substack.com', + }, + }; + + const contacts = feedContacts(channel, 'rss'); + assert.equal(contacts.length, 1); + for (const contact of contacts) { + assert.equal(typeof contact.source, 'string'); + assert.notEqual(contact.source, ''); + } +}); diff --git a/packages/feed/test/parse.test.js b/packages/feed/test/parse.test.js index 3f54538..3648124 100644 --- a/packages/feed/test/parse.test.js +++ b/packages/feed/test/parse.test.js @@ -217,6 +217,62 @@ test('a feed carrying the podcast namespaces is a podcast', () => { assert.equal(feed.imageUrl, 'https://linuxmatters.sh/cover.png'); }); +const SUBSTACK_RSS = ` + + + Newsletter Nem Toda Mulher + https://nemtodamulher.substack.com + Por Vera Iaconelli e Carol Pires + Substack + nemtodamulher@substack.com + + Newsletter Nem Toda Mulher + nemtodamulher@substack.com + + + Nunca soube a escalação de um time de futebol + https://nemtodamulher.substack.com/p/nunca-soube + Uma conversa sobre futebol e meninos + + + +`; + +test('a newsletter whose host fills in the iTunes block is not a podcast', () => { + // Substack emits on every publication it hosts, podcast or + // not, and nothing else that looks like a show: no itunes:type, no podcast: + // namespace, and image enclosures rather than audio ones. On the strength of + // that one tag the whole platform -- thousands of feeds here -- was filed + // under /podcasts. + // + // The same correction the video branch already makes: an attachment is not a + // genre, and the tag has to be corroborated by what the feed actually ships. + assert.equal(parseFeed(SUBSTACK_RSS).kind, 'blog'); +}); + +test('a show still reads as a podcast on its tags and its audio', () => { + // The other side of the guard above: everything that genuinely is a podcast + // must stay one. Linux Matters declares itunes:type and podcast:guid *and* + // attaches an mp3, so it passes on either half of the test. + assert.equal(parseFeed(PODCAST_RSS).kind, 'podcast'); +}); + +test('a podcast that has not released an episode yet is still a podcast', () => { + // A declaration stands on its own. itunes:type and the podcast: namespace are + // written by podcast hosting for podcast directories, so there is nothing to + // corroborate -- and a show with no episodes has nothing to corroborate with. + const rss = ` + + A new show + https://new.example/ + Coming soon + episodic +`; + + assert.equal(parseFeed(rss).kind, 'podcast'); +}); + test('audio without the podcast namespaces is a blog, not music', () => { // Attaching an mp3 to a post says nothing about what the feed is: a narrated // article, a conference talk and a cross-posted episode all look like this, From 279e3a81a5c2fc50bcf8c0d0e8da7e9f2451b17f Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 19 Aug 2026 18:22:36 +0000 Subject: [PATCH 2/2] A rate limit is a schedule instruction, not a broken feed Crawling faster got us 429s from Substack, and the crawler recorded each one as the publisher's fault: `markCrawlFailure` sets status='error', increments error_count, walks the backoff ladder, and at ten consecutive failures marks the feed dead. Every feed on one backend is throttled in the same minute, so this retires a whole platform for our own crawl rate -- and Substack is a large share of the directory. A 429 now reschedules and touches nothing else. `status`, `error_count`, `last_error` and `last_success_at` are left exactly as they were, and so is `last_fetched_at`: it means "when we last read this publisher", and a throttle is precisely the case where we did not. Stamping it would make a feed we have been bounced from all day look freshly crawled on every staleness report. 503 with a Retry-After is treated the same way. It is the same statement from a server that is briefly unwilling rather than permanently unable. The server picks the interval, since it is the only party that knows when its limit resets. `Retry-After` is parsed in both forms RFC 9110 allows -- a delay in seconds and an HTTP date -- floored at a minute so `Retry-After: 0` cannot spin, capped at a day so a misread date cannot mothball the feed, and defaulted to 30 minutes when the server names nothing. That is deliberately far shorter than the error ladder it replaces: the feed is healthy and we want it back soon. It is the *rate* that has to come down, and lengthening one feed's interval is the wrong instrument for that. Co-Authored-By: Claude Opus 5 (1M context) --- packages/db/src/queries.js | 25 +++++++ packages/feed/src/fetch.js | 42 +++++++++++- packages/feed/test/retry-after.test.js | 34 ++++++++++ packages/ingest/src/crawl.js | 37 +++++++++++ packages/ingest/test/throttle.test.js | 90 ++++++++++++++++++++++++++ 5 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 packages/feed/test/retry-after.test.js create mode 100644 packages/ingest/test/throttle.test.js diff --git a/packages/db/src/queries.js b/packages/db/src/queries.js index 99f88f0..7f17661 100644 --- a/packages/db/src/queries.js +++ b/packages/db/src/queries.js @@ -2732,6 +2732,31 @@ export async function markCrawlFailure(db, id, error, errorCount, minutes) { }); } +/** + * Come back later, and hold every judgement about this feed. + * + * The counterpart to `markCrawlFailure` for a server that answered 429 -- or 503 + * with a Retry-After. Only the schedule moves: `status`, `error_count` and + * `last_error` are left exactly as they were, and `last_success_at` with them. + * + * `last_fetched_at` is *not* stamped either, and that is the subtle one. It + * means "when we last read this publisher", and a throttle is precisely the case + * where we did not read them. Stamping it would make a feed we have been bounced + * from for a day look freshly crawled on every page that reports staleness. + * + * @param {Client} db + * @param {string} id + * @param {number} minutes + * @returns {Promise} + */ +export async function markThrottled(db, id, minutes) { + const wait = Math.max(1, Math.round(Number(minutes) || 30)); + await db.execute({ + sql: 'update feeds set next_fetch_at = ?, updated_at = ? where id = ?', + args: [nowIso(wait * 60_000), nowIso(), id], + }); +} + /* ------------------------------------------------------------- feed cards */ /** diff --git a/packages/feed/src/fetch.js b/packages/feed/src/fetch.js index 29fc5fb..ed75a56 100644 --- a/packages/feed/src/fetch.js +++ b/packages/feed/src/fetch.js @@ -104,7 +104,7 @@ export async function isPublicHost(hostname) { * @param {string} url * @param {{ etag?: string|null, lastModified?: string|null, * headers?: Record }} [conditional] - * @returns {Promise<{ ok: boolean, status: number, contentType: string, body: string, url: string, notModified?: boolean, etag?: string|null, lastModified?: string|null, error?: string }>} + * @returns {Promise<{ ok: boolean, status: number, contentType: string, body: string, url: string, notModified?: boolean, etag?: string|null, lastModified?: string|null, retryAfter?: number|null, error?: string }>} */ export async function safeFetch(url, conditional = {}) { const normalized = normalizeUrl(url); @@ -196,6 +196,7 @@ export async function safeFetch(url, conditional = {}) { contentType: res.headers.get('content-type') ?? '', body, url: res.url, + retryAfter: retryAfterSeconds(res.headers.get('retry-after')), etag, lastModified, }; @@ -378,6 +379,31 @@ function concat(chunks, size) { return out; } +/** + * How long a server asked us to wait, in seconds. + * + * RFC 9110 allows either a delay in seconds or an HTTP date, and both are seen + * in the wild. Anything unparseable is null, which the caller reads as "throttled + * but unsaid" and answers with its own default rather than with zero. + * + * Clamped to a day. A server that asks for a month has almost certainly sent us + * a date we misread, and honouring it literally would retire the feed. + * + * @param {string|null} header + * @returns {number|null} + */ +export function retryAfterSeconds(header) { + if (!header) return null; + + const seconds = Number(String(header).trim()); + if (Number.isFinite(seconds) && seconds >= 0) return Math.min(Math.round(seconds), 86_400); + + const at = Date.parse(String(header)); + if (!Number.isNaN(at)) return Math.min(Math.max(0, Math.round((at - Date.now()) / 1000)), 86_400); + + return null; +} + /** * Resolve whatever a user submitted into a parsed feed. * @@ -409,7 +435,19 @@ export async function resolveFeed(input, conditional = {}) { if (first.notModified) { return { ok: false, notModified: true, etag: first.etag, lastModified: first.lastModified }; } - if (!first.ok) return { ok: false, error: first.error ?? `http-${first.status}` }; + // A throttle is not a broken feed, and the difference has to survive this + // return or the crawler cannot tell them apart. 429 is the explicit form; 503 + // with a Retry-After is the same statement from a server that is briefly + // unwilling rather than permanently unable. Both mean "come back later", which + // is a schedule instruction, not evidence about the publisher. + if (!first.ok) { + const throttled = first.status === 429 || (first.status === 503 && first.retryAfter != null); + return { + ok: false, + error: first.error ?? `http-${first.status}`, + ...(throttled ? { throttled: true, retryAfter: first.retryAfter ?? null } : {}), + }; + } if (looksLikeFeed(first.contentType, first.body, first.url)) { const feed = parseFeed(first.body, first.url); diff --git a/packages/feed/test/retry-after.test.js b/packages/feed/test/retry-after.test.js new file mode 100644 index 0000000..4ee47b8 --- /dev/null +++ b/packages/feed/test/retry-after.test.js @@ -0,0 +1,34 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; + +import { retryAfterSeconds } from '../src/fetch.js'; + +test('a delay in seconds is read as seconds', () => { + assert.equal(retryAfterSeconds('120'), 120); + assert.equal(retryAfterSeconds(' 45 '), 45); + assert.equal(retryAfterSeconds('0'), 0); +}); + +test('an HTTP date is read as the wait until then', () => { + // RFC 9110 permits either form and both are sent in the wild. + const at = new Date(Date.now() + 90_000).toUTCString(); + const seconds = retryAfterSeconds(at); + assert.ok(seconds >= 85 && seconds <= 95, `expected ~90, got ${seconds}`); +}); + +test('a date already past is no wait at all, never a negative one', () => { + const at = new Date(Date.now() - 60_000).toUTCString(); + assert.equal(retryAfterSeconds(at), 0); +}); + +test('nothing to read is null, so the caller can choose its own default', () => { + assert.equal(retryAfterSeconds(null), null); + assert.equal(retryAfterSeconds(''), null); + assert.equal(retryAfterSeconds('soon'), null); +}); + +test('an absurd wait is clamped to a day', () => { + // A server asking for a month has almost certainly sent a header we misread, + // and honouring it literally would retire the feed. + assert.equal(retryAfterSeconds('9999999'), 86_400); +}); diff --git a/packages/ingest/src/crawl.js b/packages/ingest/src/crawl.js index 7fb1b6d..3f52801 100644 --- a/packages/ingest/src/crawl.js +++ b/packages/ingest/src/crawl.js @@ -43,6 +43,29 @@ export function backoffMinutes(errorCount) { return Math.min(BACKOFF[Math.min(errorCount - 1, BACKOFF.length - 1)], MAX_INTERVAL); } +/** How long to wait after a throttle that named no interval of its own. */ +const THROTTLE_DEFAULT = 30; + +/** + * How long to wait after being throttled. + * + * The server's own `Retry-After` wins, because it is the only party that knows + * when its limit resets. Floored at a minute so a `Retry-After: 0` cannot spin, + * and capped at a day so a misread date cannot mothball the feed. + * + * Deliberately much shorter than the error ladder: this feed is healthy and we + * want it back soon. It is the *rate* that has to come down, and lengthening one + * feed's interval is the wrong instrument for that -- see POLL_CONCURRENCY. + * + * @param {number|null|undefined} retryAfter seconds the server asked for + * @returns {number} minutes + */ +export function throttleMinutes(retryAfter) { + const seconds = Number(retryAfter); + if (!Number.isFinite(seconds) || seconds <= 0) return THROTTLE_DEFAULT; + return Math.min(Math.max(1, Math.ceil(seconds / 60)), 1440); +} + /** * How long to wait before re-crawling a feed whose document carried no dates. * @@ -194,6 +217,20 @@ export async function crawlFeed(db, feed, opts = {}) { return { ok: true, newItems: 0, notModified: true }; } + // Throttled, which is a different fact from failed and must not be recorded as + // one. `markCrawlFailure` sets status='error', increments error_count and walks + // the backoff ladder -- and at ten consecutive failures it marks the feed dead. + // A publisher answering 429 is telling us we are asking too often; recording + // that against *their* health would retire a working feed for our own + // impatience, and it would do it to a whole platform at once, since one + // backend's rate limit is hit by every feed hosted on it in the same minute. + // + // So: come back when asked, leave every health column exactly as it was. + if (resolved.throttled) { + await q.markThrottled(db, id, throttleMinutes(resolved.retryAfter)); + return { ok: false, newItems: 0, throttled: true, error: resolved.error }; + } + if (!resolved.ok) { const errorCount = Number(feed.error_count ?? 0) + 1; await q.markCrawlFailure(db, id, resolved.error, errorCount, backoffMinutes(errorCount)); diff --git a/packages/ingest/test/throttle.test.js b/packages/ingest/test/throttle.test.js new file mode 100644 index 0000000..095668d --- /dev/null +++ b/packages/ingest/test/throttle.test.js @@ -0,0 +1,90 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; + +import { crawlFeed, throttleMinutes, backoffMinutes } from '../src/crawl.js'; + +/** + * A db stub that records what the crawl decided, without a database. + */ +function recorder() { + const calls = []; + return { + calls, + execute: async (statement) => { + calls.push(statement); + return { rows: [], rowsAffected: 0 }; + }, + batch: async (statements) => { + calls.push(...statements); + return statements.map(() => ({ rows: [], rowsAffected: 0 })); + }, + }; +} + +const feed = { + id: 'feed-1', + feed_url: 'https://example.substack.com/feed', + error_count: 3, + fetch_interval_minutes: 60, + item_count: 10, +}; + +test('a throttled feed is rescheduled, not marked broken', async () => { + // The damage this prevents. `markCrawlFailure` sets status='error', increments + // error_count and, at ten consecutive failures, marks the feed dead. A rate + // limit is hit by every feed on one backend at the same moment, so recording + // 429s as feed health would retire a whole platform for our own crawl rate. + const db = recorder(); + const resolve = async () => ({ ok: false, error: 'http-429', throttled: true, retryAfter: 120 }); + + const result = await crawlFeed(db, feed, { resolve }); + + assert.equal(result.ok, false); + assert.equal(result.throttled, true); + + const sql = db.calls.map((c) => c.sql).join('\n'); + assert.ok(!/error_count/.test(sql), 'must not touch error_count'); + assert.ok(!/status\s*=/.test(sql), 'must not touch status'); + assert.ok(!/last_error/.test(sql), 'must not touch last_error'); + assert.ok(!/last_fetched_at/.test(sql), 'must not claim we read the publisher'); + assert.ok(/next_fetch_at/.test(sql), 'must reschedule'); +}); + +test('an ordinary failure still counts against the feed', async () => { + // The other half: this guard must not swallow real breakage. + const db = recorder(); + const resolve = async () => ({ ok: false, error: 'http-404' }); + + const result = await crawlFeed(db, feed, { resolve }); + + assert.equal(result.ok, false); + assert.ok(!result.throttled); + + const sql = db.calls.map((c) => c.sql).join('\n'); + assert.ok(/error_count/.test(sql), 'a 404 is evidence about the feed'); +}); + +test('the server’s own Retry-After decides when we come back', () => { + assert.equal(throttleMinutes(120), 2); + assert.equal(throttleMinutes(90), 2); // rounded up, never down to zero + assert.equal(throttleMinutes(30), 1); // floored at a minute +}); + +test('a throttle that names no interval gets a sane default', () => { + assert.equal(throttleMinutes(null), 30); + assert.equal(throttleMinutes(undefined), 30); + assert.equal(throttleMinutes(0), 30); + assert.equal(throttleMinutes(-5), 30); + assert.equal(throttleMinutes('nonsense'), 30); +}); + +test('a throttle cannot mothball a feed', () => { + // A misparsed date must not turn into a month-long interval. + assert.equal(throttleMinutes(86_400 * 30), 1440); +}); + +test('a throttle is far shorter than the error ladder it replaces', () => { + // The feed is healthy; it is our rate that is wrong. Coming back in half an + // hour is right where a fourth consecutive *failure* would wait twelve. + assert.ok(throttleMinutes(null) < backoffMinutes(4)); +});