fix(db-pg): make requireDbUrlForEnv guard outcome-based, not NODE_ENV-gated

The guard only threw when NODE_ENV==='production' AND resolution came
back empty. But NODE_ENV is exactly what isn't reliably set for the
maintenance scripts this guards (they're run by hand from an operator
shell with no service-definition env and no dotenv), so on the real
prod box the guard never fired: resolution silently returned '', the
script opened a brand-new empty SQLite file under <cwd>/data, and
printed a false "0 project(s) rebuilt" success.

Now the guard fires on the OUTCOME instead: whenever resolution
selects SQLite, refuse to run unless the target file already exists —
a migration/maintenance script must never CREATE a database. Applies
only to requireDbUrlForEnv (the script entry path); server.ts calls
resolveDbUrl directly and is untouched, so first boot still creates
its DB as before.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
main
Thomas Joise 1 day ago
parent 6f50799d3c
commit ee0367dfb5

@ -1,6 +1,7 @@
import { AsyncLocalStorage } from 'node:async_hooks' import { AsyncLocalStorage } from 'node:async_hooks'
import fs from 'node:fs'
import pg from 'pg' import pg from 'pg'
import type { DB } from './db' import { sqliteFilePath, type DB } from './db'
import { PG_MIGRATIONS } from './migrations-pg' import { PG_MIGRATIONS } from './migrations-pg'
/** /**
@ -124,20 +125,39 @@ export function resolveDbUrl(env: NodeJS.ProcessEnv = process.env, hardcoded = H
} }
/** /**
* Same resolution, but hard-fails instead of silently falling back to SQLite when running in * Same resolution, but hard-fails instead of silently falling back to a THROWAWAY SQLite
* production for one-off deploy scripts, where a throwaway SQLite write is never the right * database for one-off deploy/maintenance scripts, where creating a brand-new empty DB is
* outcome (unlike the server, which legitimately runs SQLite in dev). `hardcoded` is * never the right outcome (unlike the server, which legitimately creates one on first boot in
* injectable so tests can simulate the hardcoded URL being accidentally emptied out. * dev this function is never called from server.ts).
*
* The guard is OUTCOME-based, not NODE_ENV-based. The original fix gated the hard-fail on
* `NODE_ENV === 'production'`, but NODE_ENV is exactly what these scripts don't reliably have:
* the server gets it from its service definition, while the scripts are run by hand
* (`npx tsx apps/hq/scripts/…`) from an operator shell with no NODE_ENV and no dotenv. So on
* the real prod box the old guard never fired, resolution returned `''`, and the script
* silently opened/created a brand-new empty SQLite file in `<cwd>/data` "succeeding" against
* a throwaway DB while production was never touched.
*
* Fix: whenever resolution selects SQLite, refuse UNLESS the target file already exists a
* migration/maintenance script must never CREATE a database, regardless of what NODE_ENV is or
* isn't set to. `hardcoded` and `sqliteExists` are both injectable so tests never touch a real
* Postgres URL or the real filesystem.
*/ */
export function requireDbUrlForEnv(env: NodeJS.ProcessEnv = process.env, hardcoded = HARDCODED_DATABASE_URL): string { export function requireDbUrlForEnv(
env: NodeJS.ProcessEnv = process.env,
hardcoded = HARDCODED_DATABASE_URL,
sqliteExists: (file: string) => boolean = fs.existsSync,
): string {
const pgUrl = resolveDbUrl(env, hardcoded) const pgUrl = resolveDbUrl(env, hardcoded)
if (pgUrl === '' && env['NODE_ENV'] === 'production') { if (pgUrl === '') {
const file = sqliteFilePath(env['HQ_DATA_DIR'])
if (file !== ':memory:' && !sqliteExists(file)) {
throw new Error( throw new Error(
'[db] refusing to run: NODE_ENV=production but no Postgres target resolved ' + `[db] refusing to run: this would CREATE a new SQLite database at ${file}. ` +
'(DATABASE_URL is unset and the hardcoded production URL is empty) — this would ' + 'Set DATABASE_URL to target Postgres, or point HQ_DATA_DIR at the existing database.',
'otherwise silently create a throwaway SQLite database instead of touching production data.',
) )
} }
}
return pgUrl return pgUrl
} }

@ -4,6 +4,9 @@
// the scripts computed it as `DATABASE_URL ?? ''` (no production fallback), so on the // the scripts computed it as `DATABASE_URL ?? ''` (no production fallback), so on the
// production box — where DATABASE_URL is unset — each script silently opened a brand-new // production box — where DATABASE_URL is unset — each script silently opened a brand-new
// empty SQLite file instead of the real Postgres. // empty SQLite file instead of the real Postgres.
import fs from 'node:fs'
import os from 'node:os'
import path from 'node:path'
import { describe, it, expect } from 'vitest' import { describe, it, expect } from 'vitest'
import { pgTargetDescription, requireDbUrlForEnv, resolveDbUrl } from '../src/db-pg' import { pgTargetDescription, requireDbUrlForEnv, resolveDbUrl } from '../src/db-pg'
@ -26,16 +29,58 @@ describe('resolveDbUrl (D19 engine selection, the one shared resolver)', () => {
}) })
}) })
describe('requireDbUrlForEnv (deploy-script guard — hard-fail instead of a throwaway SQLite write)', () => { describe('requireDbUrlForEnv (deploy-script guard — OUTCOME-based, not NODE_ENV-based)', () => {
it('behaves exactly like resolveDbUrl when a Postgres target is available', () => { it('behaves exactly like resolveDbUrl when a Postgres target is available (no SQLite file check runs at all)', () => {
expect(requireDbUrlForEnv({ DATABASE_URL: 'postgres://x/y' })).toBe('postgres://x/y') expect(requireDbUrlForEnv({ DATABASE_URL: 'postgres://x/y' })).toBe('postgres://x/y')
expect(requireDbUrlForEnv({ NODE_ENV: 'production' }, HARDCODED)).toBe(HARDCODED) expect(requireDbUrlForEnv({ NODE_ENV: 'production' }, HARDCODED)).toBe(HARDCODED)
expect(requireDbUrlForEnv({})).toBe('') // dev with no env: SQLite is fine, no hard-fail
}) })
it('hard-fails instead of silently returning SQLite when NODE_ENV=production resolves nothing', () => { it('resolves to SQLite without hard-failing when the target file already exists (mocked fs check)', () => {
// Simulates the hardcoded URL having been accidentally emptied out. expect(requireDbUrlForEnv({}, undefined, () => true)).toBe('')
expect(() => requireDbUrlForEnv({ NODE_ENV: 'production' }, '')).toThrow(/refusing to run/i) // Even NODE_ENV=production is fine as long as the SQLite file it would land on already exists.
expect(requireDbUrlForEnv({ NODE_ENV: 'production' }, '', () => true)).toBe('')
})
it(':memory: never hard-fails — the guard never even calls the file-existence check', () => {
expect(
requireDbUrlForEnv({ HQ_DATA_DIR: ':memory:' }, undefined, () => {
throw new Error('must not be called for :memory:')
}),
).toBe('')
})
it('refuses when resolution selects SQLite and the target does not exist — regardless of NODE_ENV', () => {
// This is the original prod bug: an operator shell has no NODE_ENV set at all, so the old
// NODE_ENV==='production' gate never fired. The guard must fire on the OUTCOME instead.
expect(() => requireDbUrlForEnv({}, undefined, () => false)).toThrow(/refusing to run/i)
expect(() => requireDbUrlForEnv({}, undefined, () => false)).toThrow(/CREATE a new SQLite database/)
// Also still refuses in the NODE_ENV=production case (hardcoded emptied out + no existing file).
expect(() => requireDbUrlForEnv({ NODE_ENV: 'production' }, '', () => false)).toThrow(/refusing to run/i)
})
})
describe('requireDbUrlForEnv — real filesystem (the actual script entry path, no mocks)', () => {
it('refuses a non-existent SQLite path', () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'hq-db-resolve-'))
try {
expect(() => requireDbUrlForEnv({ HQ_DATA_DIR: dir })).toThrow(/refusing to run/i)
} finally {
fs.rmSync(dir, { recursive: true, force: true })
}
})
it('accepts an existing SQLite path', () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'hq-db-resolve-'))
try {
fs.writeFileSync(path.join(dir, 'hq.db'), '')
expect(requireDbUrlForEnv({ HQ_DATA_DIR: dir })).toBe('')
} finally {
fs.rmSync(dir, { recursive: true, force: true })
}
})
it(':memory: still works with no filesystem check', () => {
expect(requireDbUrlForEnv({ HQ_DATA_DIR: ':memory:' })).toBe('')
}) })
}) })

Loading…
Cancel
Save