diff --git a/apps/web/src/app/api/submit/route.js b/apps/web/src/app/api/submit/route.js index e2c6b74..a130480 100644 --- a/apps/web/src/app/api/submit/route.js +++ b/apps/web/src/app/api/submit/route.js @@ -1,4 +1,4 @@ -import { submitCatalogue, hashIp } from '@rssamplifier/ingest'; +import { submitCatalogue, hashIp, EXPRESS_MAX } from '@rssamplifier/ingest'; import { parseOpml } from '@rssamplifier/feed'; import { q, newId } from '@rssamplifier/db'; @@ -41,12 +41,12 @@ const SNIFF_CHARS = 4096; /** * Entries above which a submission is handed over rather than imported here. * - * Importing means `importFeeds`: one read of every feed URL and slug in the - * directory, then a round trip per five hundred rows. That is seconds for a - * paste and minutes for a subscription export — a hundred and ten thousand - * entries is over two hundred sequential round trips inside a single request - * with a five-minute ceiling on it, which is a coin toss at best and loses the - * whole upload when it comes up wrong. + * Below this a submission is queued entry by entry: a slug is claimed for each + * one and the rows are written in chunks. That is milliseconds for a paste and + * minutes for a subscription export — a hundred and ten thousand entries is + * hundreds of sequential round trips inside a single request with a five-minute + * ceiling on it, which is a coin toss at best and loses the whole upload when + * it comes up wrong. * * Past this the entries are staged instead, exactly as the batched uploader * stages them: one bulk insert per couple of thousand, no lookups, no slugs, @@ -70,6 +70,25 @@ const STAGE_ABOVE = 5_000; */ const STAGE_CHUNK = 2_000; +/** + * How long a single-URL submission is resolved for before the submitter is sent + * to the status page instead. + * + * One URL is still resolved while its submitter waits, because landing on the + * blog you just added is the nicest thing this page does. What it must not do + * is wait without a bound: a site that publishes no feed costs up to eleven + * sequential candidate fetches at a fifteen-second timeout, and the submitter + * has no way to tell that from a page that has simply hung. + * + * Past this the request answers with the status page and the resolve carries on + * in the background — the same promise, so the feed is still inserted exactly + * once and there is no queued duplicate racing it. + */ +const INLINE_WAIT_MS = Number(process.env['SUBMIT_INLINE_WAIT_MS'] ?? 8_000) || 8_000; + +/** Returned by the race below when the inline resolve outlived its budget. */ +const TOO_SLOW = Symbol('too-slow'); + /** * Split a paste into candidate URLs. * @@ -268,7 +287,14 @@ export async function POST(req) { resolveQueued = resolve; }); - const opts = { submissionId, onQueued: (n) => resolveQueued(n) }; + const opts = { + submissionId, + // Small enough to have been typed rather than exported, so it goes in the + // express lane and is crawled within a tick or two instead of behind the + // backlog. See EXPRESS_MAX. + priority: catalogue.length <= EXPRESS_MAX ? 1 : 0, + onQueued: (n) => resolveQueued(n), + }; const work = submitCatalogue(client, catalogue, opts).then(async (result) => { await q.completeSubmission(client, submissionId, { accepted_count: result.accepted.length, @@ -285,9 +311,9 @@ export async function POST(req) { const statusUrl = `${siteUrl()}/submissions/${submissionId}`; if (browser) { - // An upload with a queue behind it is answered the moment that queue is - // durable: the status page streams the rest, so waiting for a hundred - // sequential fetches would buy the submitter nothing but a blank tab. + // Anything with a queue behind it is answered the moment that queue is + // durable, which since submitCatalogue stopped resolving lists inline is + // every submission of more than one URL. The status page streams the rest. const settled = work.catch(() => null); const queued = await Promise.race([queuedCount, settled.then(() => 0)]); @@ -295,13 +321,32 @@ export async function POST(req) { return new Response(null, { status: 303, headers: { location: `/submissions/${submissionId}` } }); } - // A handful of URLs resolves in seconds and has somewhere better to land: - // the blog itself. Nothing is gained by bouncing that through a status page. - const result = await settled; - const first = result?.accepted?.[0]; - const location = first ? `/${first.slug}` : '/submit?error=1'; + // One URL, and it has somewhere better to land than a status page: the blog + // itself. Bounded, because the resolve behind it is not — see INLINE_WAIT_MS. + let timer; + const deadline = new Promise((resolve) => { + timer = setTimeout(() => resolve(TOO_SLOW), INLINE_WAIT_MS); + }); + + try { + const result = await Promise.race([settled, deadline]); + + // Still resolving. It carries on in the background and completes the + // submission when it lands, so the status page is the honest answer now. + if (result === TOO_SLOW) { + return new Response(null, { + status: 303, + headers: { location: `/submissions/${submissionId}` }, + }); + } + + const first = result?.accepted?.[0]; + const location = first ? `/${first.slug}` : '/submit?error=1'; - return new Response(null, { status: 303, headers: { location } }); + return new Response(null, { status: 303, headers: { location } }); + } finally { + clearTimeout(timer); + } } const result = await work; diff --git a/apps/web/src/lib/mcp/tools.js b/apps/web/src/lib/mcp/tools.js index 02617c5..b076b17 100644 --- a/apps/web/src/lib/mcp/tools.js +++ b/apps/web/src/lib/mcp/tools.js @@ -1,6 +1,6 @@ import { q, newId, authors as people } from '@rssamplifier/db'; import { topicSlug } from '@rssamplifier/feed'; -import { submitCatalogue, hashIp } from '@rssamplifier/ingest'; +import { submitCatalogue, hashIp, EXPRESS_MAX } from '@rssamplifier/ingest'; import { db, siteUrl } from '../db.js'; import { readerView } from '../reader.js'; @@ -24,8 +24,17 @@ import { clip, plainText } from './text.js'; /** How much extracted article text one `read_post` call may return. */ const ARTICLE_LIMIT = 24_000; -/** Feeds one `submit_feed` call will resolve before queueing the rest. */ -const SUBMIT_INLINE_LIMIT = 20; +/** + * Feeds one `submit_feed` call will resolve before queueing the rest. + * + * One, matching the form, and for the same reason: a resolve is an outbound + * fetch — up to eleven of them sequentially at a fifteen-second timeout — so + * twenty of them was a tool call that could run for minutes and time out with + * nothing to show. A list is queued instead and the caller is handed the status + * URL it already returns. A single URL still resolves, so an agent submitting + * one blog gets its slug back in the same call. + */ +const SUBMIT_INLINE_LIMIT = 1; /** Submissions allowed per IP per hour, matching /api/submit. */ const SUBMIT_RATE_LIMIT = 20; @@ -561,7 +570,15 @@ export const TOOLS = [ const result = await submitCatalogue( client, urls.map((url) => ({ url })), - { submissionId, inlineLimit: SUBMIT_INLINE_LIMIT }, + { + submissionId, + inlineLimit: urls.length === 1 ? SUBMIT_INLINE_LIMIT : 0, + // A hand-written tool call is the same population as a hand-typed + // paste, so it gets the same express lane and the same bound. The + // tool accepts up to 200 URLs, which is past EXPRESS_MAX, so this + // is a real test rather than a formality. + priority: urls.length <= EXPRESS_MAX ? 1 : 0, + }, ); await q.completeSubmission(client, submissionId, { diff --git a/packages/db/migrations/20260819145808_feed_priority.sql b/packages/db/migrations/20260819145808_feed_priority.sql new file mode 100644 index 0000000..7bb1b2e --- /dev/null +++ b/packages/db/migrations/20260819145808_feed_priority.sql @@ -0,0 +1,25 @@ +-- An express lane for feeds a person submitted by hand. +-- +-- `dueFeeds` orders by `next_fetch_at asc`, which is right for a directory that +-- is keeping up and exactly wrong for one that is not. A new submission is +-- stamped with `now`, and there are ~307,000 feeds from the bulk uploads whose +-- next_fetch_at is already in the past -- so a blog somebody submits today +-- sorts behind every one of them and is not crawled for days. The submitter +-- watches a status page that says "pending" and concludes the site is broken. +-- +-- Priority is set only by the submit route, and only for a submission small +-- enough to have come from a person rather than an export. It is not a general +-- scheduling knob: nothing else writes it. +alter table feeds add column priority integer not null default 0; + +-- Partial on purpose, and both halves of the predicate matter. +-- +-- `priority > 0` keeps the index to the handful of hand-submitted feeds instead +-- of all 416,000. `last_fetched_at is null` is what makes the lane self- +-- clearing: the crawler sets that column on success *and* on failure, so a feed +-- leaves this index after exactly one attempt and cannot camp in the fast lane +-- for ever. Together they mean the express query reads an index that is +-- normally empty and never larger than one afternoon's submissions. +create index if not exists idx_feeds_express + on feeds (next_fetch_at) + where priority > 0 and last_fetched_at is null; diff --git a/packages/db/src/queries.js b/packages/db/src/queries.js index 99f88f0..7f230eb 100644 --- a/packages/db/src/queries.js +++ b/packages/db/src/queries.js @@ -2179,14 +2179,69 @@ export async function recentlyCrawled(db, limit = 20) { return rows; } +/** The columns a crawl needs off a feed row. Shared by both due queries. */ +const DUE_COLUMNS = `id, slug, title, feed_url, error_count, fetch_interval_minutes, source_kind, + item_count, last_published_at, + http_etag, http_last_modified, content_hash, change_log`; + +/** + * The share of a tick reserved for hand-submitted feeds. + * + * Half, so the express lane cannot starve the backlog no matter how many + * submissions arrive: a tick always spends at least half of itself on the + * ordinary queue. In practice the reservation is never taken up — real people + * submit a handful of blogs an hour against a batch of twenty-five — so this + * is a ceiling on the pathological case rather than a division of normal work. + */ +const EXPRESS_SHARE = 0.5; + /** - * Feeds whose next_fetch_at has passed. + * Feeds a person submitted by hand and that have never been crawled. + * + * The express lane. Read before the ordinary queue because ordering the two + * together cannot work: they are ordered by `next_fetch_at asc` and the backlog + * is *older*, so a submission stamped `now` sorts last behind ~307,000 feeds + * that were overdue before it arrived. Sorting by priority instead would put a + * 416,000-row sort in front of every tick. Two reads, the first against a + * partial index that is normally empty, is the cheap way to say "these first". + * + * `last_fetched_at is null` is in the predicate rather than being cleared after + * the fact, so this expedites the *first* crawl only — which is the whole of + * what a submitter is waiting for. Afterwards the feed is scheduled on its own + * publishing rhythm like everything else. + * + * @param {Client} db + * @param {number} [limit] + * @returns {Promise} + */ +export async function expressFeeds(db, limit = 25) { + if (limit <= 0) return []; + + const { rows } = await db.execute({ + sql: `select ${DUE_COLUMNS} + from feeds + where priority > 0 and last_fetched_at is null + and status <> 'dead' and next_fetch_at <= ? + order by next_fetch_at asc limit ?`, + args: [nowIso(), limit], + }); + return rows; +} + +/** + * Feeds whose next_fetch_at has passed, hand-submitted ones first. * * @param {Client} db * @param {number} [limit] * @returns {Promise} */ export async function dueFeeds(db, limit = 25) { + // Bounded rather than unbounded even though the express table is tiny: the + // point of a reserved share is that it is reserved in both directions. + const express = await expressFeeds(db, Math.floor(limit * EXPRESS_SHARE)); + const remaining = limit - express.length; + if (remaining <= 0) return express; + const { rows } = await db.execute({ // slug and title are along for the log: a crawler log line that names the // blog and links to its page is worth two columns the crawl itself is @@ -2206,15 +2261,19 @@ export async function dueFeeds(db, limit = 25) { // added in 0032: what the server's validators were, what the feed contained, // and when we last saw that change. They are what lets a feed stating no // dates be scheduled on evidence rather than on the doubling ladder. - sql: `select id, slug, title, feed_url, error_count, fetch_interval_minutes, source_kind, - item_count, last_published_at, - http_etag, http_last_modified, content_hash, change_log + // + // The express rows are excluded rather than deduplicated afterwards: they + // have just been read and are about to be crawled, and handing the same + // feed to two workers in one tick is two crawls of it. + sql: `select ${DUE_COLUMNS} from feeds where status <> 'dead' and next_fetch_at <= ? + and not (priority > 0 and last_fetched_at is null) order by next_fetch_at asc limit ?`, - args: [nowIso(), limit], + args: [nowIso(), remaining], }); - return rows; + + return express.concat(rows); } /** @@ -2336,13 +2395,14 @@ export async function insertFeedsBulk(db, feeds) { const now = nowIso(); const row = `(?, ?, ?, ?, ?, null, null, null, null, '[]', 'pending', null, null, null, 0, - 60, ?, 0, ?, ?, ?)`; + 60, ?, 0, ?, ?, ?, ?)`; const result = await db.execute({ sql: `insert into feeds (id, slug, feed_url, site_url, title, description, language, image_url, author, categories, status, last_fetched_at, last_success_at, last_error, error_count, - fetch_interval_minutes, next_fetch_at, item_count, submission_id, created_at, updated_at) + fetch_interval_minutes, next_fetch_at, item_count, submission_id, created_at, updated_at, + priority) values ${feeds.map(() => row).join(', ')} on conflict do nothing`, args: feeds.flatMap((f) => [ @@ -2355,6 +2415,10 @@ export async function insertFeedsBulk(db, feeds) { f.submission_id ?? null, now, now, + // Zero unless the caller is putting a hand-submitted feed in the express + // lane. Written here rather than defaulted by the column so that a row + // inserted by an import is explicit about not being expedited. + Number(f.priority) > 0 ? 1 : 0, ]), }); diff --git a/packages/db/test/express-lane.test.js b/packages/db/test/express-lane.test.js new file mode 100644 index 0000000..f5e6e96 --- /dev/null +++ b/packages/db/test/express-lane.test.js @@ -0,0 +1,143 @@ +import assert from 'node:assert/strict'; +import { test, before, after } from 'node:test'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { connect } from '../src/client.js'; +import { migrate } from '../src/migrate.js'; +import * as q from '../src/queries.js'; + +let dir; +let db; + +/** An hour ago, a day ago, and so on — the backlog is always older than `now`. */ +const ago = (ms) => new Date(Date.now() - ms).toISOString(); + +before(async () => { + dir = await mkdtemp(join(tmpdir(), 'rssamp-express-')); + db = connect({ url: `file:${join(dir, 'test.db')}` }); + await migrate(db); +}); + +after(async () => { + await rm(dir, { recursive: true, force: true }); +}); + +test('a hand-submitted feed is crawled before a backlog that is older than it', async () => { + // The shape production is actually in: a great many feeds from a bulk upload, + // every one of them overdue, and one blog somebody just submitted. Ordering + // by next_fetch_at alone puts the submission last, because it is the newest + // thing in the queue and the queue is sorted oldest-first. + await q.insertFeedsBulk( + db, + Array.from({ length: 30 }, (_, i) => ({ + slug: `backlog-${i}`, + feed_url: `https://backlog-${i}.example/feed.xml`, + title: `Backlog ${i}`, + next_fetch_at: ago(30 * 24 * 60 * 60_000), + })), + ); + + await q.insertFeedsBulk(db, [ + { + slug: 'hand-submitted', + feed_url: 'https://hand-submitted.example/feed.xml', + title: 'Hand Submitted', + next_fetch_at: ago(1_000), + priority: 1, + }, + ]); + + const due = await q.dueFeeds(db, 10); + + assert.equal(due[0].slug, 'hand-submitted', 'the submission is first, not 30 feeds later'); + assert.equal(due.length, 10, 'and the rest of the tick is still the backlog'); +}); + +test('the same feed is never handed out twice in one tick', async () => { + const due = await q.dueFeeds(db, 10); + const slugs = due.map((r) => String(r.slug)); + + assert.equal(new Set(slugs).size, slugs.length, 'no feed appears in both lanes'); +}); + +test('the express lane cannot take more than half a tick', async () => { + await q.insertFeedsBulk( + db, + Array.from({ length: 20 }, (_, i) => ({ + slug: `expedited-${i}`, + feed_url: `https://expedited-${i}.example/feed.xml`, + title: `Expedited ${i}`, + next_fetch_at: ago(1_000), + priority: 1, + })), + ); + + // Every feed currently in the lane, not just the twenty above: the feed the + // first test submitted is still waiting in it and is just as expedited. + const waiting = new Set((await q.expressFeeds(db, 100)).map((r) => String(r.slug))); + assert.ok(waiting.size > 5, 'more want the lane than the lane will give them'); + + const due = await q.dueFeeds(db, 10); + const expedited = due.filter((r) => waiting.has(String(r.slug))).length; + + // Five of ten, and the backlog keeps the other five. A flood of submissions + // must not be able to stop the directory draining. + assert.equal(expedited, 5); + assert.equal(due.length, 10); +}); + +test('a feed leaves the express lane after one crawl attempt, success or failure', async () => { + const before = await q.expressFeeds(db, 100); + assert.ok(before.length > 0, 'the lane has feeds in it to begin with'); + + // Not "priority is cleared" — nothing clears it. The lane is defined by + // last_fetched_at being null, and the crawler writes that column whether the + // fetch worked or not, so one attempt is all any feed ever gets. + await db.execute({ + sql: 'update feeds set last_fetched_at = ? where priority > 0', + args: [new Date().toISOString()], + }); + + const after = await q.expressFeeds(db, 100); + assert.equal(after.length, 0, 'crawled once, and back in the ordinary queue'); +}); + +test('a dead feed is not expedited, however it was submitted', async () => { + await q.insertFeedsBulk(db, [ + { + slug: 'dead-express', + feed_url: 'https://dead-express.example/feed.xml', + title: 'Dead Express', + next_fetch_at: ago(1_000), + priority: 1, + }, + ]); + + await db.execute("update feeds set status = 'dead' where slug = 'dead-express'"); + + const express = await q.expressFeeds(db, 100); + assert.equal( + express.find((r) => String(r.slug) === 'dead-express'), + undefined, + ); +}); + +test('an imported feed is queued at priority zero', async () => { + await q.insertFeedsBulk(db, [ + { + slug: 'from-an-import', + feed_url: 'https://from-an-import.example/feed.xml', + title: 'From An Import', + next_fetch_at: ago(1_000), + }, + ]); + + const { rows } = await db.execute({ + sql: 'select priority from feeds where slug = ?', + args: ['from-an-import'], + }); + + assert.equal(Number(rows[0].priority), 0, 'an upload does not buy its way up the queue'); +}); diff --git a/packages/ingest/index.js b/packages/ingest/index.js index f6be9cc..a0fb1cc 100644 --- a/packages/ingest/index.js +++ b/packages/ingest/index.js @@ -1,4 +1,4 @@ -export { submitOne, submitMany, submitOpml, submitCatalogue } from './src/submit.js'; +export { submitOne, submitMany, submitOpml, submitCatalogue, EXPRESS_MAX } from './src/submit.js'; export { crawlFeed, crawlDue, diff --git a/packages/ingest/src/queue.js b/packages/ingest/src/queue.js index e0a8406..5cfe670 100644 --- a/packages/ingest/src/queue.js +++ b/packages/ingest/src/queue.js @@ -61,6 +61,7 @@ const PREFETCH_DEPTH = 6; * submissionId?: string|null, * offsetMinutes?: number, * ratePerMinute?: number, + * priority?: number, * }} [opts] * @returns {Promise<{ queued: number, skipped: number, invalid: number, total: number }>} */ @@ -68,6 +69,9 @@ export async function queueFeeds(db, entries, opts = {}) { const submissionId = opts.submissionId ?? null; const rate = Math.max(1, opts.ratePerMinute ?? DEFAULT_RATE); const offsetMs = Math.max(0, opts.offsetMinutes ?? 0) * 60_000; + // The express lane, set by the submit route for a submission small enough to + // have been typed rather than exported. See `expressFeeds`. + const priority = Number(opts.priority) > 0 ? 1 : 0; // Deduplicated here as well as in the database, because two identical URLs in // the same batch would otherwise both be "not yet known" and the second would @@ -120,8 +124,17 @@ export async function queueFeeds(db, entries, opts = {}) { // Spread within the batch and offset by everything queued before it, so a // catalogue uploaded in four hundred pieces still schedules as one steady // line rather than four hundred overlapping four-hour bursts. - next_fetch_at: nowIso(offsetMs + (i / rate) * 60_000), + // + // Except in the express lane, where the spread would defeat the purpose: + // `expressFeeds` only returns what is already due, so a submission of fifty + // blogs scheduled at 240 a minute would have its last one wait twelve + // seconds to become eligible and then be crawled a tick later still. The + // burst this protects against is a catalogue of hundreds of thousands; a + // hand submission is capped at a hundred and is the thing somebody is + // sitting and watching. + next_fetch_at: nowIso(priority > 0 ? 0 : offsetMs + (i / rate) * 60_000), submission_id: submissionId, + priority, })); let queued = 0; diff --git a/packages/ingest/src/submit.js b/packages/ingest/src/submit.js index 6ea3b53..bcd1056 100644 --- a/packages/ingest/src/submit.js +++ b/packages/ingest/src/submit.js @@ -1,7 +1,7 @@ import { resolveFeed, scrapeFeed, normalizeUrl, parseOpml, uniqueSlug } from '@rssamplifier/feed'; import { q } from '@rssamplifier/db'; -import { importFeeds } from './import.js'; +import { queueFeeds } from './queue.js'; import { refreshFeedKeywords } from './crawl.js'; /** Cap on a single bulk submission, so one paste can't queue thousands of fetches. */ @@ -10,10 +10,40 @@ const MAX_BATCH = 200; /** * How many entries of a catalogue are resolved while the submitter waits. * - * Each one is an outbound fetch, so this is the request's time budget. The - * rest is queued for the poller rather than dropped. + * One, and only when the whole submission is that one entry — see the default + * in `submitCatalogue` for why the two conditions are not the same thing. + * + * This was a hundred, which read as a generous allowance and was in fact the + * reason the submit page felt broken. Every one of those hundred is an outbound + * resolve — up to eleven sequential candidate fetches at a fifteen-second + * timeout, then the feed insert, the items and the topics — and the route only + * answers early when something was *queued*, which below a hundred entries + * nothing ever was. Measured against production: eight URLs that were already + * in the directory took **65 seconds** and inserted nothing at all. + * + * So a list is queued now rather than crawled, and the queue is what the + * submitter is sent to watch. The single URL keeps its inline resolve because + * that is what redirects the submitter to the blog they just added, which is + * the nicest thing that happens on the page and costs one fetch. + */ +const INLINE_LIMIT = 1; + +/** + * Submissions at or below this many entries are crawled ahead of the backlog. + * + * The cut-in-line lane. Queueing a submission instantly is only half an answer + * if the queue never reaches it, and it did not: `dueFeeds` orders by + * `next_fetch_at asc`, a new feed is stamped `now`, and there are ~307,000 + * feeds from the bulk uploads whose next_fetch_at was already in the past. A + * blog somebody submitted today sorted behind every one of them. + * + * A hundred is the line between a person and an export. Nobody types more than + * that, and every catalogue is far larger — so this expedites submissions made + * by hand without letting an upload buy its way past the queue it belongs in. + * `expressFeeds` bounds the other end: the lane can never take more than half a + * tick, and a feed leaves it after one crawl attempt. */ -const INLINE_LIMIT = 100; +export const EXPRESS_MAX = 100; /** * Claim a free slug, consulting the database for collisions. @@ -52,6 +82,20 @@ export async function submitOne(db, input) { const url = normalizeUrl(input); if (!url) return { ok: false, url: String(input), error: 'invalid-url' }; + // Asked before anything is fetched, because most of what people submit is + // already here. The lookup below is the same question asked of the *resolved* + // feed URL, which is the only form that catches "myblog.com" for a blog + // stored as "myblog.com/feed.xml" — but it cannot be reached without paying + // for the resolve first, and a resolve is up to eleven sequential requests at + // a fifteen-second timeout. Eight already-indexed feeds cost 65 seconds in + // production for exactly this reason, and every one of those fetches was of + // a document the directory already had. + // + // Someone pasting a feed URL they got from this site is the common case, and + // it is settled here for one ~90ms indexed read. + const alreadyKnown = await q.feedByUrl(db, url); + if (alreadyKnown) return { ok: true, slug: String(alreadyKnown.slug), existing: true }; + // A site that publishes no feed used to end here, which quietly put most of // the web permanently outside the directory. Now the page itself is read: if // it turns out to be a list of posts, the directory builds the feed the site @@ -160,22 +204,36 @@ export async function submitMany(db, urls) { * * @param {import('@libsql/client').Client} db * @param {Array<{ url: string, title?: string, siteUrl?: string|null }>} entries - * @param {{ inlineLimit?: number, submissionId?: string|null, spreadMinutes?: number, onQueued?: (queued: number) => void }} [opts] + * @param {{ inlineLimit?: number, submissionId?: string|null, priority?: number, onQueued?: (queued: number) => void }} [opts] * @returns {Promise<{ accepted: object[], rejected: object[], queued: number, total: number }>} */ export async function submitCatalogue(db, entries, opts = {}) { - const inlineLimit = opts.inlineLimit ?? INLINE_LIMIT; + // A lone URL is resolved while the submitter waits, so that it can redirect + // to the blog it added. Anything longer is a list, and a list is queued. + // + // Deliberately not `min(entries.length, INLINE_LIMIT)`: that would resolve the + // first entry of a fifty-URL paste too, and the submitter is going to the + // status page either way, so the fetch would buy them nothing but a wait. + const inlineLimit = opts.inlineLimit ?? (entries.length === 1 ? INLINE_LIMIT : 0); const head = entries.slice(0, inlineLimit); const tail = entries.slice(inlineLimit); let queued = 0; if (tail.length > 0) { - const imported = await importFeeds(db, tail, { + // `queueFeeds`, not `importFeeds`. The difference is the opening read: + // `importFeeds` reads every feed_url and slug in the directory first, which + // is right for one process holding one whole file and catastrophic here. + // It pages the feeds table with `limit 5000 offset ?`, and offset paging is + // O(offset) — measured against production at 416,000 feeds it was **still + // running after 550 seconds**, inside a route capped at 300. Every paste of + // 101 to 2,000 URLs therefore timed out and queued nothing. `queueFeeds` + // asks only about the URLs in front of it. + const imported = await queueFeeds(db, tail, { submissionId: opts.submissionId ?? null, - spreadMinutes: opts.spreadMinutes, + priority: opts.priority, }); - queued = imported.inserted; + queued = imported.queued; } opts.onQueued?.(queued); diff --git a/packages/ingest/test/submit-queues.test.js b/packages/ingest/test/submit-queues.test.js new file mode 100644 index 0000000..9254681 --- /dev/null +++ b/packages/ingest/test/submit-queues.test.js @@ -0,0 +1,148 @@ +import assert from 'node:assert/strict'; +import { test, before, after } from 'node:test'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { connect, migrate, q } from '@rssamplifier/db'; + +import { submitCatalogue, submitOne } from '../src/submit.js'; + +let dir; +let db; + +/** + * The real `fetch`, put back after each test. + * + * These tests are about what submitting does *not* do, so the assertion is on + * the network rather than on the database: a stub that throws is the only way + * to state "and it never went to look". + */ +const realFetch = globalThis.fetch; + +before(async () => { + dir = await mkdtemp(join(tmpdir(), 'rssamp-submit-queues-')); + db = connect({ url: `file:${join(dir, 'test.db')}` }); + await migrate(db); +}); + +after(async () => { + globalThis.fetch = realFetch; + await rm(dir, { recursive: true, force: true }); +}); + +test('a pasted list is queued without a single feed being fetched', async () => { + globalThis.fetch = () => { + throw new Error('a list must not be resolved while the submitter waits'); + }; + + const entries = Array.from({ length: 8 }, (_, i) => ({ + url: `https://pasted-${i}.example/feed.xml`, + title: `Pasted ${i}`, + })); + + // No inlineLimit given, which is the whole point: the default has to be the + // safe one. Eight of these took 65 seconds in production when the default + // resolved the first hundred entries inline. + const res = await submitCatalogue(db, entries, { submissionId: 'sub-list' }); + + assert.equal(res.queued, 8); + assert.equal(res.accepted.length, 0); + assert.equal(res.total, 8); + + globalThis.fetch = realFetch; +}); + +test('a submission small enough to have been typed goes into the express lane', async () => { + globalThis.fetch = () => { + throw new Error('still no fetching'); + }; + + await submitCatalogue( + db, + Array.from({ length: 3 }, (_, i) => ({ url: `https://typed-${i}.example/feed.xml` })), + { submissionId: 'sub-typed', priority: 1 }, + ); + + const express = await q.expressFeeds(db, 100); + const slugs = new Set(express.map((r) => String(r.slug))); + + assert.equal(express.length, 3); + assert.ok([...slugs].every((s) => s.startsWith('typed-'))); + + globalThis.fetch = realFetch; +}); + +test('a catalogue is queued at priority zero unless the caller says otherwise', async () => { + globalThis.fetch = () => { + throw new Error('still no fetching'); + }; + + await submitCatalogue( + db, + Array.from({ length: 3 }, (_, i) => ({ url: `https://bulk-${i}.example/feed.xml` })), + { submissionId: 'sub-bulk' }, + ); + + const express = await q.expressFeeds(db, 100); + assert.ok( + express.every((r) => !String(r.slug).startsWith('bulk-')), + 'an upload is not expedited by default', + ); + + globalThis.fetch = realFetch; +}); + +test('a feed the directory already holds is answered without going to the network', async () => { + await q.insertFeed(db, { + slug: 'known-blog', + feed_url: 'https://known.example/feed.xml', + site_url: 'https://known.example/', + title: 'Known Blog', + }); + + globalThis.fetch = () => { + throw new Error('a feed we already have must not be re-resolved'); + }; + + const res = await submitOne(db, 'https://known.example/feed.xml'); + + assert.deepEqual(res, { ok: true, slug: 'known-blog', existing: true }); + + globalThis.fetch = realFetch; +}); + +test('the short-circuit normalises first, so a scruffy paste still matches', async () => { + globalThis.fetch = () => { + throw new Error('still a feed we already have'); + }; + + // What people actually paste: no scheme, a stray fragment, surrounding space. + const res = await submitOne(db, ' known.example/feed.xml#top '); + + assert.equal(res.ok, true); + assert.equal(res.slug, 'known-blog'); + + globalThis.fetch = realFetch; +}); + +test('a single URL is still resolved inline, because it has a blog to land on', async () => { + // The one case that keeps its fetch, and the assertion is the opposite of + // every test above: this one has to prove a resolve was *attempted*. + // + // It is proved by the attempt failing rather than by a stubbed response. + // `safeFetch` resolves the hostname itself before it fetches anything — the + // SSRF guard — and `.example` has no DNS, so the resolve stops at + // `blocked-host` without `fetch` ever being reached. A stub cannot observe + // this; a rejection can, and a rejection is only possible if the entry went + // to the head rather than the tail. + const res = await submitCatalogue(db, [{ url: 'https://solo.example/feed.xml' }], { + submissionId: 'sub-solo', + }); + + assert.equal(res.queued, 0, 'one URL is not queued, it is resolved'); + assert.equal(res.accepted.length, 0); + assert.deepEqual(res.rejected, [ + { url: 'https://solo.example/feed.xml', error: 'blocked-host' }, + ]); +});