diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..7379cfb --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,57 @@ +# The first CI this repo has had. +# +# Until now nothing ran the suite automatically: the only checks on a pull +# request were Socket's dependency scans, which say nothing about whether the +# code works. Eleven workspaces of tests existed and were only ever run by +# whoever remembered to. That is also how three migrations came to quietly claim +# an already-taken number — the guard against it is a test, and a test nobody +# runs is a comment. +name: Test + +on: + pull_request: + push: + branches: [main] + +# A second push to the same branch makes the first run irrelevant, so stop it +# rather than paying for both. Cancelling is limited to pull requests: a push to +# main is a merge, and cancelling that would leave the branch with no verdict. +concurrency: + group: test-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + test: + runs-on: ubuntu-latest + # The suite runs in about a minute. Ten is generous for a slow runner and + # short enough that a hung test gets reported rather than left burning. + timeout-minutes: 10 + + steps: + - uses: actions/checkout@v4 + + # Before setup-node, which is not the obvious order: `cache: pnpm` needs + # the pnpm binary to exist so it can ask where the store lives. The + # version comes from `packageManager` in package.json, so it stays in step + # with local development without being written down twice. + - uses: pnpm/action-setup@v4 + + - uses: actions/setup-node@v4 + with: + # Matching apps/*/Dockerfile, which is node:22-alpine. `engines` says + # >=22, but CI should run what production runs rather than the newest + # thing that satisfies the range. + node-version: 22 + cache: pnpm + + - run: pnpm install --frozen-lockfile + + # Every workspace. The suite is hermetic — it builds SQLite databases in + # tmpdir and stubs the network — so it needs no Turso credentials and no + # secrets, which is what makes it safe to run on a pull request. + - run: pnpm -r test + + # The web build, because a broken page is not something the tests catch: + # they cover packages and route handlers, and a JSX or import error in a + # page only surfaces when Next compiles it. + - run: pnpm build diff --git a/packages/db/migrations/README.md b/packages/db/migrations/README.md new file mode 100644 index 0000000..a114012 --- /dev/null +++ b/packages/db/migrations/README.md @@ -0,0 +1,69 @@ +# Migrations + +Name a new migration `YYYYMMDDHHMMSS_what_it_does.sql`, in UTC: + +```sh +printf '%s_add_thing.sql\n' "$(date -u +%Y%m%d%H%M%S)" +``` + +## Why timestamps rather than the next number + +Because the next number is not knowable from a branch. On 2026-08-19 two +branches both took `0032` — `0032_feed_refetch_signals.sql` (#127) and +`0032_queue_hourly.sql` (#128) — and **git raised no conflict**, because they +are different filenames. Nothing failed, nothing warned, and the collision was +only noticed because somebody happened to list the table afterwards. + +Writing the test that now guards this found it had happened twice before: + + 0006 feed_items_created_idx, feeds_sitemap_index + 0018 crawl_log, queue, scraped_feeds <- three files + 0032 feed_refetch_signals, queue_hourly + +None of it ever broke anything, because `migrate()` keys `_migrations` on the +filename rather than the number, so each file still ran exactly once. That is +luck rather than design: it holds only while the colliding files commute, and +nothing was checking that they did. + +That is the whole argument. A sequential scheme is only correct if whoever +merges second renumbers, and they are never prompted to, because the tool that +would prompt them sees two unrelated new files. + +Timestamps make the collision essentially impossible: two people would have to +create a migration in the same second. + +## What timestamps do not fix, so you know + +They order by **when a migration was written, not when it was merged.** Write +one on Monday, merge it on Friday, and a colleague's Wednesday migration will +have run first in production while a freshly created database replays yours +first. + +This is not a regression — sequential numbering had the same divergence *and* +the silent collisions — but it is worth knowing: + +- **Keep migrations independent of each other.** Additive `alter table add + column` and `create index if not exists` commute, so order does not matter. + Two migrations that must run in a fixed order belong in one file. +- The order production actually used is recorded in `_migrations.applied_at`, + which is the authority when the two disagree. + +## Never rename a migration that has been applied + +`migrate()` keys `_migrations` on the **filename**. Rename an applied file and +the runner sees a migration it has never run and runs it again. For a purely +additive file that is survivable — `ALREADY_APPLIED` in `migrate.js` swallows +"duplicate column name" and "already exists" — but `0001_init.sql`, +`0013_feed_category.sql` and `0017_crawl_hourly.sql` also carry `insert` and +`update` statements, and those are not idempotent. + +So the `0001`–`0032` files stay exactly as they are, every duplicate included. +They sort before any timestamp anyway, since `'0'` precedes `'2'`, so the +history keeps its order for good and needs no tidying to stay correct. + +## Statements + +Files are split on `;` outside `BEGIN…END`, so trigger bodies are safe. Each +statement runs on its own; a file that fails partway is retried from the +statement that failed the next time the poller boots, and only recorded once +the whole file lands. diff --git a/packages/db/test/migration-names.test.js b/packages/db/test/migration-names.test.js new file mode 100644 index 0000000..6d49714 --- /dev/null +++ b/packages/db/test/migration-names.test.js @@ -0,0 +1,139 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { readdir } from 'node:fs/promises'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * That two people cannot silently claim the same migration slot. + * + * On 2026-08-19 two branches both took `0032` — `0032_feed_refetch_signals.sql` + * (#127) and `0032_queue_hourly.sql` (#128). Git raised no conflict, because + * they are different filenames; nothing failed and nothing warned. Both applied + * cleanly, since `migrate()` keys `_migrations` on the filename rather than the + * number, so this cost nothing that day — but it was luck, and the next pair + * might not commute. + * + * A convention in a README does not stop this happening again. Whoever merges + * second is never prompted to renumber, because the tool that would prompt them + * sees two unrelated new files. This is the prompt. + */ + +const MIGRATIONS = join(dirname(fileURLToPath(import.meta.url)), '..', 'migrations'); + +/** + * The last sequential number ever issued. + * + * Everything up to here is frozen history and keeps its name; see the README + * for why an applied migration must never be renamed. Anything new must be a + * timestamp, and this is what makes that a rule rather than a suggestion. + */ +const LAST_SEQUENTIAL = 32; + +/** + * The duplicates that already shipped, which cannot be renamed away. + * + * Writing this test surfaced that `0032` was not the first time — it was the + * third, and `0018` was claimed by *three* separate files. None of it ever + * failed and none of it was ever noticed, which is precisely the argument: a + * silent fault that costs nothing for a year is still a fault, and it only has + * to meet one pair of order-dependent migrations to stop being free. + * + * Recorded rather than tidied. Every one of these is applied in production and + * keyed by filename, so renaming any of them would make the runner treat it as + * new and run it a second time — survivable for a purely additive file, not for + * the three that carry `insert` and `update` statements. + */ +const GRANDFATHERED = new Set([ + '0006_feed_items_created_idx.sql', + '0006_feeds_sitemap_index.sql', + '0018_crawl_log.sql', + '0018_queue.sql', + '0018_scraped_feeds.sql', + '0032_feed_refetch_signals.sql', + '0032_queue_hourly.sql', +]); + +const files = (await readdir(MIGRATIONS)).filter((f) => f.endsWith('.sql')).sort(); + +test('there are migrations to check', () => { + assert.ok(files.length > 30, `only found ${files.length}`); +}); + +test('every migration is named for when it was written, or is frozen history', () => { + for (const file of files) { + const match = /^(\d{4}|\d{14})_[a-z0-9_]+\.sql$/.exec(file); + assert.ok(match, `${file}: expected YYYYMMDDHHMMSS_lower_snake_case.sql`); + + const prefix = match[1]; + if (prefix.length === 14) continue; + + assert.ok( + Number(prefix) <= LAST_SEQUENTIAL, + `${file}: sequential numbering stopped at ${LAST_SEQUENTIAL}. Name a new migration ` + + `for the moment it was written — \`date -u +%Y%m%d%H%M%S\` — because the next number ` + + `is not knowable from a branch, and 0006, 0018 and 0032 were each claimed twice ` + + `or more before anybody noticed.`, + ); + } +}); + +test('no two migrations claim the same slot', () => { + const seen = new Map(); + + for (const file of files) { + if (GRANDFATHERED.has(file)) continue; + const prefix = file.slice(0, file.indexOf('_')); + const first = seen.get(prefix); + assert.equal( + first, + undefined, + `${file} and ${first} share the prefix ${prefix}. Git will not have flagged this — ` + + `they are different filenames — so rename the one that has NOT been applied to ` + + `production yet. Never rename one that has.`, + ); + seen.set(prefix, file); + } +}); + +test('a timestamped migration names a real moment that has passed', () => { + for (const file of files) { + const prefix = file.slice(0, 14); + if (!/^\d{14}$/.test(prefix)) continue; + + const [y, mo, d, h, mi, s] = [ + prefix.slice(0, 4), + prefix.slice(4, 6), + prefix.slice(6, 8), + prefix.slice(8, 10), + prefix.slice(10, 12), + prefix.slice(12, 14), + ].map(Number); + + const at = Date.UTC(y, mo - 1, d, h, mi, s); + assert.ok(Number.isFinite(at), `${file}: unparseable timestamp`); + assert.equal(mo >= 1 && mo <= 12, true, `${file}: month ${mo}`); + assert.equal(d >= 1 && d <= 31, true, `${file}: day ${d}`); + assert.equal(h <= 23 && mi <= 59 && s <= 59, true, `${file}: time ${h}:${mi}:${s}`); + // A future timestamp sorts after everything and would jump the queue for + // however long it stays in the future — usually a typo'd year or a machine + // with a bad clock. + assert.ok(at <= Date.now(), `${file}: dated in the future`); + } +}); + +test('history still runs before anything new, whatever is added later', () => { + // The reason the old files can simply be left alone: `migrate()` sorts + // lexically and '0' precedes '2', so every 0001-0032 file sorts before every + // 20xx timestamp. Asserted rather than assumed, because the whole no-rename + // decision rests on it. + const sequential = files.filter((f) => /^\d{4}_/.test(f)); + const timestamped = files.filter((f) => /^\d{14}_/.test(f)); + + if (sequential.length === 0 || timestamped.length === 0) return; + + const lastOld = sequential[sequential.length - 1]; + const firstNew = timestamped[0]; + assert.ok(lastOld < firstNew, `${lastOld} must sort before ${firstNew}`); + assert.equal(files.indexOf(lastOld) < files.indexOf(firstNew), true); +}); diff --git a/packages/db/test/request-timeout.test.js b/packages/db/test/request-timeout.test.js index 901bbfe..8c6037b 100644 --- a/packages/db/test/request-timeout.test.js +++ b/packages/db/test/request-timeout.test.js @@ -10,6 +10,54 @@ import { withTimeout } from '../src/client.js'; * out entirely. */ +/** + * A `fetch` that never answers, and rejects when the request is abandoned. + * + * Two things a naive stub gets wrong, both of which cost a CI run to find. + * + * **It has to keep the process alive.** `AbortSignal.timeout()` schedules an + * *unref'd* timer — by design, so a pending deadline never holds a program + * open — and a stub that merely returns a promise gives the event loop nothing + * else to do. The loop drains, and the test runner reports "Promise resolution + * is still pending but the event loop has already resolved" and cancels the + * whole file. In production this cannot happen, because a real in-flight fetch + * holds a socket open; only a stub that does literally nothing is exposed to + * it. Node 24's runner happens to keep the loop alive and Node 22's does not, + * which is why this passed locally and failed in CI on the same commit. + * + * **It has to honour a signal that has already fired.** A real fetch handed an + * aborted signal rejects at once; an `abort` listener added afterwards hears + * nothing, because the event has been and gone. + * + * @param {(reason: unknown) => Error|unknown} [reasonFor] what to reject with + * @returns {(input: unknown, init?: { signal?: AbortSignal }) => Promise} + */ +function neverAnswers(reasonFor = (reason) => reason) { + return (_input, init = {}) => + new Promise((_resolve, reject) => { + const { signal } = init; + // Deliberately ref'd, and cleared on every exit below so it cannot outlive + // the request it is standing in for. + const inFlight = setTimeout(() => {}, 30_000); + const abandon = (reason) => { + clearTimeout(inFlight); + reject(reasonFor(reason)); + }; + + // `withTimeout` always supplies one; a stub left pending with nothing to + // wake it would wedge the file for thirty seconds rather than fail. + if (!signal) { + clearTimeout(inFlight); + return; + } + if (signal.aborted) { + abandon(signal.reason); + return; + } + signal.addEventListener('abort', () => abandon(signal.reason)); + }); +} + test('a request that outlives its deadline is abandoned', async () => { // The reason this exists. undici's default is five minutes, and five minutes // is not a timeout -- it is a promise that one wedged request will hold a @@ -19,10 +67,7 @@ test('a request that outlives its deadline is abandoned', async () => { const fetching = withTimeout(30); const original = globalThis.fetch; - globalThis.fetch = (_input, init) => - new Promise((_resolve, reject) => { - init.signal.addEventListener('abort', () => reject(init.signal.reason)); - }); + globalThis.fetch = neverAnswers(); try { await assert.rejects( @@ -54,10 +99,7 @@ test("a caller's own signal still wins if it fires first", async () => { const original = globalThis.fetch; const controller = new AbortController(); - globalThis.fetch = (_input, init) => - new Promise((_resolve, reject) => { - init.signal.addEventListener('abort', () => reject(new Error('aborted by caller'))); - }); + globalThis.fetch = neverAnswers(() => new Error('aborted by caller')); try { const pending = fetching('https://example.invalid/', { signal: controller.signal });