fix(test): stop the suite reaching a real database, and make it exit
All checks were successful
PR Checks / client-build (pull_request) Successful in 25s
PR Checks / server-tests (pull_request) Successful in 1m34s
PR Checks / bot-install (pull_request) Successful in 8m45s

`npm test` never terminated. Twenty-two test files omitted the two lines that
point the pool at a dead port, so utils/db.js -- which builds its mariadb pool at
require time and calls dotenv.config() itself -- picked up server/.env and opened
five live connections to the developer's MariaDB. The tests still passed, because
they stub their models and never issue a query; the only symptoms were a process
that never exited and five connections held for as long as it lived. Thirty
stranded workers is 150 connections, which is the whole server's limit, and that
is the "too many connections" this workspace has hit before.

The convention was right and only ever as good as the next test file's memory of
it, so it moves into the harness: test/_setup.js is loaded with --require by the
npm script, ahead of the test file it hosts, which is the only moment early
enough to matter. It pins the dead port -- dotenv does not overwrite an existing
variable, so an explicit DB_PORT= still wins for anyone who wants a live database
-- and closes the pool after the file's tests, so the process exits at once
instead of waiting out the driver's connect retries. The per-file preambles stay:
they keep `node --test test/one.test.js` safe on its own.

Two supporting fixes:

- db.close() is idempotent. pool.end() throws "pool is already closed" on a
  second call, and closing twice is now normal rather than exceptional -- the
  harness closes the pool for every file on top of the suites that close it
  themselves, and a SIGINT followed by a SIGTERM already reached the shutdown
  handler twice.
- test/_helper.js's close() destroys open connections. server.close() only stops
  accepting and waits for existing connections to end, and node's global fetch
  keeps its sockets alive, so the listener outlived the test that created it --
  invisible until now, because the pool was holding the process open anyway.

announceJobs.test.js alone: 120s+ hang -> 0.35s. The whole suite now finishes in
~75s where it previously did not finish at all: 901 tests, 901 pass, verified
three times on CI's exact platform (node:20 on Linux, via Docker).

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
2026-08-10 21:27:47 -05:00
parent 21196466ed
commit 32ed8e4411
4 changed files with 59 additions and 2 deletions

View File

@@ -14,7 +14,14 @@ async function startApp(configure) {
const { port } = server.address()
return {
url: `http://127.0.0.1:${port}`,
close: () => new Promise((resolve) => server.close(resolve)),
// `server.close()` stops accepting and waits for open connections to end on
// their own — and node's global fetch keeps its sockets alive, so nothing
// ever ends them. The listener then outlives the test that made it, which
// used to be invisible because the pool held the process open anyway.
close: () => new Promise((resolve) => {
server.closeAllConnections()
server.close(resolve)
}),
}
}

42
server/test/_setup.js Normal file
View File

@@ -0,0 +1,42 @@
// ── Test-process setup, loaded into every test file ────────────────────────
//
// `npm test` passes this with `--require`, so it runs before the test file it
// is hosting — which is the only moment early enough to matter, because
// `utils/db.js` builds its mariadb pool at REQUIRE time.
//
// It fixes two things that were per-file conventions, and therefore held only
// as well as the next test file remembered them:
//
// 1. **Nothing in the suite may reach a real database.** Without this, a file
// that forgot the two `process.env` lines picked up `server/.env` through
// db.js's own `dotenv.config()` and pooled five live connections to the
// developer's MariaDB. The tests still passed — they stub their models —
// so the only symptom was the process never exiting, plus five connections
// held for as long as the worker lived. Thirty stranded workers is 150
// connections, which is the whole server's limit.
// 2. **The pool is closed when the file's tests are done**, so the process can
// exit at once instead of waiting out the driver's connect retries.
//
// `dotenv` does not overwrite variables that already exist, so pinning the dead
// port here beats `.env` while still letting an explicit `DB_PORT=… npm test`
// through for anyone who deliberately wants a live database.
const { after } = require('node:test')
process.env.DB_HOST = process.env.DB_HOST || '127.0.0.1'
process.env.DB_PORT = process.env.DB_PORT || '59999'
// A root-level hook, registered before the test file is even read, so it runs
// once after everything in that file.
//
// Only in the per-file child processes. `--require` is inherited by the runner
// process too, and registering a root hook there gives node:test a second root
// context to report — an empty "tests 0 / pass 0" summary printed after the real
// one, which reads like a suite that silently ran nothing.
if (process.env.NODE_TEST_CONTEXT) {
after(async () => {
// Resolved rather than required: a file that never touched the database must
// not have a pool built for it here just so this can close one.
const cached = require.cache[require.resolve('../src/utils/db')]
if (cached) await cached.exports.close()
})
}