refactor(errors): one canonical error handler + process resilience (Pillar B)

- formatError() now emits `code` (the field the client reads) and hides
  internals on any 5xx (missing status treated as 500). Single body shape
  everywhere: { error, message, code, field? }.
- The terminal error handler now delegates to formatError, so a thrown
  ApiError produces its real status + message + code (previously every
  error collapsed to a generic "Internal server error"). Only 5xx are
  logged/recorded; 4xx client errors no longer spam the error tracker.
  This unblocks the Express-5 `throw ApiError` route pattern.
- Resilience: added `uncaughtException` (record + exit for clean restart),
  enriched `unhandledRejection` (record, don't crash), and a SIGTERM/SIGINT
  graceful shutdown that drains connections, checkpoints the WAL, and
  closes the DB — verified: kill -TERM → "database closed cleanly" + exit.

check:server + typecheck:server clean; 232/232 tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
null 2026-07-06 14:33:38 -05:00
parent 88674fa2f5
commit 5ad3c4e18e
2 changed files with 82 additions and 17 deletions

View File

@ -281,31 +281,87 @@ app.use((err, req, res, next) => {
// If a response was already (partially) sent, we can't write a new body/status — // If a response was already (partially) sent, we can't write a new body/status —
// delegate to Express's default handler, which closes the connection cleanly. // delegate to Express's default handler, which closes the connection cleanly.
if (res.headersSent) return next(err); if (res.headersSent) return next(err);
recordError('Express', err);
console.error('[error]', err.message || String(err)); const status = err.status || 500;
// Only server errors (5xx) are logged/recorded; 4xx are expected client errors
// (validation, auth, not-found) and must not spam the error tracker.
if (status >= 500) {
recordError('Express', err);
console.error('[error]', err.stack || err.message || String(err));
}
if (/^\/api\/imports?\//.test(req.path || '')) { if (/^\/api\/imports?\//.test(req.path || '')) {
const isBodyError = err.type === 'entity.too.large' || err instanceof SyntaxError; const isBodyError = err.type === 'entity.too.large' || err instanceof SyntaxError;
const code = isBodyError ? 'IMPORT_REQUEST_ERROR' : 'IMPORT_SERVER_ERROR';
return res.status(err.status || (isBodyError ? 400 : 500)).json({ return res.status(err.status || (isBodyError ? 400 : 500)).json({
error: 'Import request failed', error: code,
message: isBodyError message: isBodyError
? 'The import request could not be read. Please retry with a smaller or valid file.' ? 'The import request could not be read. Please retry with a smaller or valid file.'
: 'Unexpected import server error. Please try again.', : 'Unexpected import server error. Please try again.',
code: isBodyError ? 'IMPORT_REQUEST_ERROR' : 'IMPORT_SERVER_ERROR', code,
}); });
} }
res.status(err.status || 500).json({ // Single canonical error body: { error, message, code, field? } (utils/apiError).
error: 'Internal server error', const { formatError } = require('./utils/apiError.cts');
code: 'INTERNAL_ERROR', res.status(status).json(formatError(err));
});
}); });
// ── Safety net: log unhandled promise rejections instead of crashing ───────── // ── Safety net: unhandled promise rejections — log + record, don't crash ─────
// (Most rejections here are non-fatal side effects; a crash would be worse than
// surfacing them. Truly fatal states are handled by uncaughtException below.)
process.on('unhandledRejection', (reason) => { process.on('unhandledRejection', (reason) => {
console.error('[server] Unhandled promise rejection:', reason?.message || reason); console.error(
'[server] Unhandled promise rejection:',
reason?.stack || reason?.message || reason,
);
try {
recordError('unhandledRejection', reason instanceof Error ? reason : new Error(String(reason)));
} catch {}
}); });
// An uncaught exception leaves the process in an undefined state — record it and
// exit non-zero so the supervisor (Docker `restart: unless-stopped`) restarts clean.
process.on('uncaughtException', (err) => {
console.error('[server] Uncaught exception:', err?.stack || err);
try {
recordError('uncaughtException', err);
} catch {}
process.exit(1);
});
// ── Graceful shutdown ────────────────────────────────────────────────────────
// Docker `stop` sends SIGTERM. Stop accepting connections, let in-flight requests
// finish, then checkpoint the WAL and close the DB so no write is lost. Hard-exit
// if it stalls.
let shuttingDown = false;
function gracefulShutdown(signal, server) {
if (shuttingDown) return;
shuttingDown = true;
console.log(`[server] ${signal} received — shutting down gracefully`);
const finish = (code) => {
try {
const { getDb, closeDb } = require('./db/database.cts');
try {
getDb().pragma('wal_checkpoint(TRUNCATE)');
} catch {}
closeDb();
console.log('[server] database closed cleanly');
} catch (e) {
console.error('[server] error closing database:', e?.message || e);
}
process.exit(code);
};
server.close(() => finish(0));
// Don't let a hung connection block shutdown forever.
setTimeout(() => {
console.error('[server] shutdown timed out after 10s — forcing exit');
finish(1);
}, 10000).unref();
}
// ── Bootstrap ───────────────────────────────────────────────────────────────── // ── Bootstrap ─────────────────────────────────────────────────────────────────
async function main() { async function main() {
const db = getDb(); const db = getDb();
@ -381,7 +437,7 @@ async function main() {
createRegularUser(); createRegularUser();
} }
app.listen(PORT, HOST, () => { const server = app.listen(PORT, HOST, () => {
console.log(`Bill Tracker running on ${HOST}:${PORT}`); console.log(`Bill Tracker running on ${HOST}:${PORT}`);
if (userCount > 0) console.log(`Users found: ${userCount}`); if (userCount > 0) console.log(`Users found: ${userCount}`);
@ -437,6 +493,10 @@ async function main() {
console.error('[dailyWorker] Failed to start daily worker:', err.message); console.error('[dailyWorker] Failed to start daily worker:', err.message);
} }
}); });
// Wire graceful shutdown once the server handle exists.
process.on('SIGTERM', () => gracefulShutdown('SIGTERM', server));
process.on('SIGINT', () => gracefulShutdown('SIGINT', server));
} }
main().catch((err) => { main().catch((err) => {

View File

@ -78,23 +78,28 @@ function RateLimitError(message = 'Too many requests', code = 'RATE_LIMITED'): A
* Format an error for JSON response * Format an error for JSON response
* This ensures consistent error format across all routes * This ensures consistent error format across all routes
*/ */
function formatError(err: any): { error: string; message: string; field?: string } { function formatError(err: any): { error: string; message: string; code: string; field?: string } {
if (err instanceof ApiError) { if (err instanceof ApiError) {
const output: { error: string; message: string; field?: string } = { // `error` and `code` are both the machine code (the client reads `code`);
// `error` is kept for backward compatibility.
const output: { error: string; message: string; code: string; field?: string } = {
error: err.code, error: err.code,
message: err.message, message: err.message,
code: err.code,
}; };
if (err.field) output.field = err.field; if (err.field) output.field = err.field;
return output; return output;
} }
// Fallback for non-standard errors (log internally, show safe message) // Non-ApiError: hide internals on any 5xx (missing status is treated as 500).
const safeMessage = const status = err.status || 500;
err.status === 500 ? 'Internal server error' : err.message || 'An error occurred'; const code = err.code || 'INTERNAL_ERROR';
const safeMessage = status >= 500 ? 'Internal server error' : err.message || 'An error occurred';
return { return {
error: err.code || 'ERROR', error: code,
message: safeMessage, message: safeMessage,
code,
}; };
} }