From d7332ea792476f45a827834bfb1c79ce2ed77259 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 19 Aug 2026 07:48:02 +0000 Subject: [PATCH 1/4] Name migrations for when they were written, and prove nobody can collide again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Yesterday 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, so nothing failed and nothing warned. Writing the test that now guards this found it had happened twice before and nobody had ever noticed: 0006 feed_items_created_idx, feeds_sitemap_index 0018 crawl_log, queue, scraped_feeds <- three files 0032 feed_refetch_signals, queue_hourly It has cost nothing so far because `migrate()` keys `_migrations` on the filename rather than the number, so each file still ran exactly once. That is luck, not design: it holds only while the colliding files commute, and nothing was checking that they did. The first order-dependent pair would apply in whichever order lexical sort happened to pick. So new migrations are named `YYYYMMDDHHMMSS_what_it_does.sql`, as Supabase does it. Two people would have to write one in the same second to collide. The honest trade, written down in the README rather than discovered later: timestamps order by when a migration was *written*, not when it was *merged*, so a fresh database can replay in a different order from the one production used. That is not a regression — sequential numbering had the same divergence and the silent collisions on top — but it means migrations must stay independent of each other, and `_migrations.applied_at` is the authority when the two disagree. Nothing is renamed. `migrate()` keys on the filename, so renaming an applied migration makes the runner treat it as new and run it again — survivable for a purely additive file, since ALREADY_APPLIED swallows "duplicate column name", but 0001, 0013 and 0017 also carry inserts and updates. The old files sort before every timestamp anyway ('0' precedes '2'), so history keeps its order untouched, and the test asserts that rather than assuming it. The test is the point. A convention in a README would not have stopped any of the three collisions above, because whoever merges second is never prompted to renumber — the tool that would prompt them sees two unrelated new files. This is the prompt. Verified both ways: it rejects a new `0033_*` and it rejects two files sharing a timestamp. Co-Authored-By: Claude Opus 5 (1M context) --- packages/db/migrations/README.md | 69 +++++++++++ packages/db/test/migration-names.test.js | 139 +++++++++++++++++++++++ 2 files changed, 208 insertions(+) create mode 100644 packages/db/migrations/README.md create mode 100644 packages/db/test/migration-names.test.js 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); +}); From fe047e1424b349deb63daebf2dabc255eafdc60c Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 19 Aug 2026 07:51:17 +0000 Subject: [PATCH 2/4] Run the tests on every pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration guard in this branch is a test, and nothing in this repository ran tests. The only checks on a pull request were Socket's dependency scans, which say nothing about whether the code works — so eleven workspaces of tests existed and were run by whoever remembered to. That is not a side issue here, it is the same issue. A guard nobody runs is a comment, and the three migrations that quietly claimed a taken number went unnoticed for exactly that reason. Node 22 to match apps/*/Dockerfile rather than the newest version `engines` would allow, and pnpm from `packageManager` so the version is not written down in two places that can drift. The build runs too. The tests cover packages and route handlers; a JSX or import error in a page only surfaces when Next compiles it, which is how the about page rewrite could have shipped broken. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 57 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 .github/workflows/test.yml 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 From 7dff2b36e955bc435c350434133a0ee219d13b8e Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 19 Aug 2026 07:56:11 +0000 Subject: [PATCH 3/4] Stop the request-timeout stub hanging when the deadline wins the race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first run of the new workflow cancelled all four tests in request-timeout.test.js with "Promise resolution is still pending but the event loop has already resolved". They pass every time on a quiet laptop, and the suite had never run anywhere else, so this had been latent since #111. The stub is at fault, not `withTimeout`. It answered only by listening: init.signal.addEventListener('abort', () => reject(init.signal.reason)) A real fetch handed a signal that has *already* fired rejects immediately. An abort listener added afterwards hears nothing — the event has been and gone — so the promise never settles, the file is torn down, and the three tests behind it are cancelled by their parent. On a quiet machine the 30ms deadline never beats the stub to its first line. On a loaded two-core runner it does. Forced both ways to be sure, with a deadline already expired on arrival: old (listener only) -> HUNG new (checks aborted) -> rejected TimeoutError after 40ms So the stub now checks `signal.aborted` first, which is what it was always claiming to imitate. Shared between the two tests that need it rather than fixed twice. Co-Authored-By: Claude Opus 5 (1M context) --- packages/db/test/request-timeout.test.js | 38 +++++++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/packages/db/test/request-timeout.test.js b/packages/db/test/request-timeout.test.js index 901bbfe..11af80d 100644 --- a/packages/db/test/request-timeout.test.js +++ b/packages/db/test/request-timeout.test.js @@ -10,6 +10,34 @@ import { withTimeout } from '../src/client.js'; * out entirely. */ +/** + * A `fetch` that never answers, and rejects when the request is abandoned. + * + * The `aborted` check is the whole point of this helper existing. A real fetch + * handed a signal that has *already* fired rejects immediately, but an `abort` + * listener added afterwards never hears anything — the event has been and gone. + * So a stub that only listens hangs for ever whenever the deadline wins the + * race to the first line of the stub, which is exactly what a loaded CI runner + * arranges: these four tests were cancelled on the first run of the new + * workflow with "Promise resolution is still pending but the event loop has + * already resolved", while passing every time on a quiet laptop. + * + * @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; + if (!signal) return; + if (signal.aborted) { + reject(reasonFor(signal.reason)); + return; + } + signal.addEventListener('abort', () => reject(reasonFor(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 +47,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 +79,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 }); From 15922df4471981e878d770c956b8d736c30284f1 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 19 Aug 2026 07:59:22 +0000 Subject: [PATCH 4/4] Keep the process alive while the request-timeout stub waits to be abandoned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit fixed a real defect in this stub and not the one that was breaking CI. The tests were still cancelled, identically, and the reason it looked like a loaded-runner race was that it reproduces on Node 22 *in isolation on an idle machine* — CI runs Node 22 to match apps/*/Dockerfile, while local development is on Node 24. `AbortSignal.timeout()` schedules an **unref'd** timer. That is deliberate: a pending deadline must not hold a program open. Verified identical on both versions — a script whose only pending work is such a deadline exits in 1ms under 22 and 24 alike. In production nothing notices, because the real in-flight fetch holds a socket open and the loop stays alive until the deadline fires. A stub that returns a promise and does nothing else gives the loop no reason to stay awake at all, so it drains and the runner reports "Promise resolution is still pending but the event loop has already resolved" and cancels the file. Node 24's test runner happens to keep the loop alive and Node 22's does not, which is the whole of the difference between green locally and red in CI. So the stub now holds a ref'd timer for the life of the request it is standing in for, cleared on every exit. Verified on both: 4 passed, 0 cancelled under Node 22 and Node 24, and the full workspace suite is clean under Node 22. The `aborted` check from the previous commit stays. It was a genuine fault — a listener added after the event has fired never hears it — and would have bitten as soon as a deadline beat the stub to its first line. Co-Authored-By: Claude Opus 5 (1M context) --- packages/db/test/request-timeout.test.js | 42 +++++++++++++++++------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/packages/db/test/request-timeout.test.js b/packages/db/test/request-timeout.test.js index 11af80d..8c6037b 100644 --- a/packages/db/test/request-timeout.test.js +++ b/packages/db/test/request-timeout.test.js @@ -13,14 +13,21 @@ import { withTimeout } from '../src/client.js'; /** * A `fetch` that never answers, and rejects when the request is abandoned. * - * The `aborted` check is the whole point of this helper existing. A real fetch - * handed a signal that has *already* fired rejects immediately, but an `abort` - * listener added afterwards never hears anything — the event has been and gone. - * So a stub that only listens hangs for ever whenever the deadline wins the - * race to the first line of the stub, which is exactly what a loaded CI runner - * arranges: these four tests were cancelled on the first run of the new - * workflow with "Promise resolution is still pending but the event loop has - * already resolved", while passing every time on a quiet laptop. + * 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} @@ -29,12 +36,25 @@ function neverAnswers(reasonFor = (reason) => reason) { return (_input, init = {}) => new Promise((_resolve, reject) => { const { signal } = init; - if (!signal) return; + // 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) { - reject(reasonFor(signal.reason)); + abandon(signal.reason); return; } - signal.addEventListener('abort', () => reject(reasonFor(signal.reason))); + signal.addEventListener('abort', () => abandon(signal.reason)); }); }