diff --git a/packages/db/src/authors.js b/packages/db/src/authors.js index 98c59f6..049cc09 100644 --- a/packages/db/src/authors.js +++ b/packages/db/src/authors.js @@ -190,7 +190,10 @@ export async function upsertAuthor(db, author) { (id, slug, identity_key, name, norm_name, bio, avatar_url, site_url, email, confidence, created_at, updated_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) - on conflict (identity_key) do nothing`, + on conflict (identity_key) do nothing + -- Same race as creditStatements: the slug was chosen by an earlier + -- read, and another writer can take it in between. See there. + on conflict (slug) do nothing`, args: [ id, author.slug, @@ -738,7 +741,24 @@ export function creditStatements({ feedId, identityKey, slug, person, authorLink or (authors.avatar_url is null and excluded.avatar_url is not null) or (authors.site_url is null and excluded.site_url is not null) or (authors.email is null and excluded.email is not null) - or authors.confidence < excluded.confidence`, + or authors.confidence < excluded.confidence + -- A different person who happens to have claimed the same slug. + -- + -- claimAuthorSlug reads the slugs already taken and then this insert + -- runs later, so two crawls naming the same author can both pick + -- jane-doe before either has committed. The second violated the unique + -- constraint on authors.slug and, because this statement rides in the + -- crawl's own transaction, took the entire crawl down with it -- the + -- feed was recorded as uncrawlable and walked up the backoff ladder + -- toward dead, for a byline. Two Substack feeds were failing every + -- crawl this way for hours. + -- + -- Doing nothing is the right answer rather than merely the safe one. + -- The loser of the race is not lost: its slug is taken by the time the + -- feed is crawled again, so claimAuthorSlug picks the next free one + -- and the credit lands then. A missing byline for one cycle is a far + -- smaller thing than a feed marked dead. + on conflict (slug) do nothing`, args: [ newId(), slug, diff --git a/packages/db/test/author-slug-race.test.js b/packages/db/test/author-slug-race.test.js new file mode 100644 index 0000000..23e0a70 --- /dev/null +++ b/packages/db/test/author-slug-race.test.js @@ -0,0 +1,155 @@ +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 { creditStatements, upsertAuthor } from '../src/authors.js'; + +/** + * Two crawls claiming the same author slug must not fail a feed. + * + * `claimAuthorSlug` reads the slugs already taken and the insert runs later, so + * two feeds naming the same author can both pick `jane-doe` before either has + * committed. The loser used to violate `authors.slug` — and because the insert + * rides in the crawl's own write transaction, it took the whole crawl with it: + * the feed was recorded `could not be crawled` and walked up the backoff ladder + * toward `dead`, over a byline. + * + * Seen in production on 2026-08-19: two Substack feeds failed every crawl for + * hours on exactly this. + */ + +let dir; +let db; + +before(async () => { + dir = await mkdtemp(join(tmpdir(), 'rssamp-slug-')); + db = connect({ url: `file:${join(dir, 'test.db')}` }); + await migrate(db); + + // feed_authors carries a foreign key to feeds, so the credits have to have + // somewhere to land. + const now = '2026-01-01T00:00:00.000Z'; + for (const [id, slug] of [ + ['feed-1', 'one'], + ['feed-2', 'two'], + ]) { + await db.execute({ + sql: `insert into feeds (id, slug, title, feed_url, status, next_fetch_at, created_at, updated_at) + values (?, ?, ?, ?, 'active', ?, ?, ?)`, + args: [id, slug, slug, `https://${slug}.example/feed`, now, now, now], + }); + } +}); + +after(async () => { + await rm(dir, { recursive: true, force: true }); +}); + +const person = (name, email) => ({ + name, + normName: name.toLowerCase(), + bio: '', + avatarUrl: '', + siteUrl: '', + email, + confidence: 0.8, + role: 'author', + evidence: 'test', +}); + +test('a second person claiming a taken slug does not fail the write', async () => { + const first = creditStatements({ + feedId: 'feed-1', + identityKey: 'jane@one.example', + slug: 'jane-doe', + person: person('Jane Doe', 'jane@one.example'), + }); + await db.batch(first, 'write'); + + // A different identity — so the identity_key upsert does not catch it — that + // raced to the same slug. + const second = creditStatements({ + feedId: 'feed-2', + identityKey: 'jane@two.example', + slug: 'jane-doe', + person: person('Jane Doe', 'jane@two.example'), + }); + + await assert.doesNotReject( + () => db.batch(second, 'write'), + 'a slug collision must not take the crawl down', + ); +}); + +test('the first claimant keeps the slug, unaltered', async () => { + const { rows } = await db.execute({ + sql: 'select identity_key, email from authors where slug = ?', + args: ['jane-doe'], + }); + + assert.equal(rows.length, 1, 'exactly one author may hold a slug'); + assert.equal(String(rows[0].identity_key), 'jane@one.example'); + assert.equal(String(rows[0].email), 'jane@one.example', 'the loser must not overwrite the winner'); +}); + +test('the loser is not created, so a later crawl can claim a free slug', async () => { + // Not lost, deferred. By the next crawl the slug is taken, so + // `claimAuthorSlug` picks the next free one and the credit lands then. + const { rows } = await db.execute({ + sql: 'select count(*) as n from authors where identity_key = ?', + args: ['jane@two.example'], + }); + + assert.equal(Number(rows[0].n), 0); +}); + +test('the same person crawled twice still updates, rather than being skipped', async () => { + // The guard must not swallow the ordinary path: a matching identity_key is an + // update, and only a *different* identity colliding on slug is a no-op. + const better = creditStatements({ + feedId: 'feed-1', + identityKey: 'jane@one.example', + slug: 'jane-doe', + person: { ...person('Jane Doe', 'jane@one.example'), bio: 'Writes things.', confidence: 0.95 }, + }); + await db.batch(better, 'write'); + + const { rows } = await db.execute({ + sql: 'select bio, confidence from authors where identity_key = ?', + args: ['jane@one.example'], + }); + + assert.equal(String(rows[0].bio), 'Writes things.'); + assert.equal(Number(rows[0].confidence), 0.95); +}); + +test('upsertAuthor survives the same race', async () => { + // The enrichment pass takes the other path into this table. + await upsertAuthor(db, { + identityKey: 'sam@one.example', + slug: 'sam-smith', + name: 'Sam Smith', + normName: 'sam smith', + confidence: 0.5, + }); + + await assert.doesNotReject(() => + upsertAuthor(db, { + identityKey: 'sam@two.example', + slug: 'sam-smith', + name: 'Sam Smith', + normName: 'sam smith', + confidence: 0.5, + }), + ); + + const { rows } = await db.execute({ + sql: 'select count(*) as n from authors where slug = ?', + args: ['sam-smith'], + }); + assert.equal(Number(rows[0].n), 1); +});