From 15ed329743046b07102a0456d4782fe2ee3c43dc Mon Sep 17 00:00:00 2001 From: josh Date: Sat, 28 Mar 2026 11:11:00 -0400 Subject: [PATCH] fix: db volume ownership and explicit error handling for write failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the 500 on create/update/delete: the non-root app user in the Docker container lacked write permission to the volume mount point. Docker volume mounts are owned by root by default; the app user (added in a previous commit) could read the database but not write to it. Fixes: 1. Dockerfile — RUN mkdir -p /app/data before chown so the directory exists in the image with correct ownership. Docker uses this as a seed when initialising a new named volume, ensuring the app user owns the mount point from the start. NOTE: existing volumes from before the non-root user was introduced will still be root-owned. Fix with: docker run --rm -v catalyst-data:/data alpine chown -R 1000:1000 /data 2. server/routes.js — replace bare `throw e` in POST/PUT catch blocks with console.error (route context + error) + explicit 500 response. Add try-catch to DELETE handler which previously had none. Unexpected DB errors now log the route they came from and return a clean JSON body instead of relying on the generic Express error handler. 3. server/db.js — wrap the boot init() call in try-catch. Fatal startup errors (e.g. data directory not writable) now print a clear message pointing to the cause before exiting, instead of a raw stack trace. TDD: tests written first (RED), then fixed (GREEN). Six new tests in tests/api.test.js verify that unexpected DB errors on POST, PUT, and DELETE return 500 with { error: 'internal server error' } and call console.error with the route context string. Co-Authored-By: Claude Sonnet 4.6 --- Dockerfile | 2 +- server/db.js | 10 ++++++- server/routes.js | 15 +++++++--- tests/api.test.js | 74 ++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/Dockerfile b/Dockerfile index 6c6bcd3..608e6f6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,7 +10,7 @@ COPY . . RUN awk -F'"' '/"version"/{printf "const VERSION = \"%s\";\n", $4; exit}' \ package.json > js/version.js -RUN chown -R app:app /app +RUN mkdir -p /app/data && chown -R app:app /app USER app EXPOSE 3000 diff --git a/server/db.js b/server/db.js index f7a0fb9..6d221cc 100644 --- a/server/db.js +++ b/server/db.js @@ -134,4 +134,12 @@ export function _resetForTest() { // ── Boot ────────────────────────────────────────────────────────────────────── -init(process.env.DB_PATH ?? DEFAULT_PATH); +const DB_PATH = process.env.DB_PATH ?? DEFAULT_PATH; +try { + init(DB_PATH); +} catch (e) { + console.error('[catalyst] fatal: could not open database at', DB_PATH); + console.error('[catalyst] ensure the data directory exists and is writable by the server process.'); + console.error(e); + process.exit(1); +} diff --git a/server/routes.js b/server/routes.js index c75b578..79eaad6 100644 --- a/server/routes.js +++ b/server/routes.js @@ -78,7 +78,8 @@ router.post('/instances', (req, res) => { } catch (e) { if (e.message.includes('UNIQUE')) return res.status(409).json({ error: 'vmid already exists' }); if (e.message.includes('CHECK')) return res.status(400).json({ error: 'invalid field value' }); - throw e; + console.error('POST /api/instances', e); + res.status(500).json({ error: 'internal server error' }); } }); @@ -98,7 +99,8 @@ router.put('/instances/:vmid', (req, res) => { } catch (e) { if (e.message.includes('UNIQUE')) return res.status(409).json({ error: 'vmid already exists' }); if (e.message.includes('CHECK')) return res.status(400).json({ error: 'invalid field value' }); - throw e; + console.error('PUT /api/instances/:vmid', e); + res.status(500).json({ error: 'internal server error' }); } }); @@ -112,6 +114,11 @@ router.delete('/instances/:vmid', (req, res) => { if (instance.stack !== 'development') return res.status(422).json({ error: 'only development instances can be deleted' }); - deleteInstance(vmid); - res.status(204).end(); + try { + deleteInstance(vmid); + res.status(204).end(); + } catch (e) { + console.error('DELETE /api/instances/:vmid', e); + res.status(500).json({ error: 'internal server error' }); + } }); diff --git a/tests/api.test.js b/tests/api.test.js index 38138ab..c807c30 100644 --- a/tests/api.test.js +++ b/tests/api.test.js @@ -1,7 +1,8 @@ -import { describe, it, expect, beforeEach } from 'vitest' +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import request from 'supertest' import { app } from '../server/server.js' import { _resetForTest } from '../server/db.js' +import * as dbModule from '../server/db.js' beforeEach(() => _resetForTest()) @@ -277,3 +278,74 @@ describe('static assets and SPA routing', () => { expect(res.text).toContain('') }) }) + +// ── Error handling — unexpected DB failures ─────────────────────────────────── + +const dbError = () => Object.assign( + new Error('attempt to write a readonly database'), + { code: 'ERR_SQLITE_ERROR', errcode: 8 } +) + +describe('error handling — unexpected DB failures', () => { + let consoleSpy + + beforeEach(() => { + consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('POST returns 500 with friendly message when DB throws unexpectedly', async () => { + vi.spyOn(dbModule, 'createInstance').mockImplementationOnce(() => { throw dbError() }) + const res = await request(app).post('/api/instances').send(base) + expect(res.status).toBe(500) + expect(res.body).toEqual({ error: 'internal server error' }) + }) + + it('POST logs the error with route context when DB throws unexpectedly', async () => { + vi.spyOn(dbModule, 'createInstance').mockImplementationOnce(() => { throw dbError() }) + await request(app).post('/api/instances').send(base) + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('POST /api/instances'), + expect.any(Error) + ) + }) + + it('PUT returns 500 with friendly message when DB throws unexpectedly', async () => { + await request(app).post('/api/instances').send(base) + vi.spyOn(dbModule, 'updateInstance').mockImplementationOnce(() => { throw dbError() }) + const res = await request(app).put('/api/instances/100').send(base) + expect(res.status).toBe(500) + expect(res.body).toEqual({ error: 'internal server error' }) + }) + + it('PUT logs the error with route context when DB throws unexpectedly', async () => { + await request(app).post('/api/instances').send(base) + vi.spyOn(dbModule, 'updateInstance').mockImplementationOnce(() => { throw dbError() }) + await request(app).put('/api/instances/100').send(base) + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('PUT /api/instances/:vmid'), + expect.any(Error) + ) + }) + + it('DELETE returns 500 with friendly message when DB throws unexpectedly', async () => { + await request(app).post('/api/instances').send({ ...base, stack: 'development', state: 'testing' }) + vi.spyOn(dbModule, 'deleteInstance').mockImplementationOnce(() => { throw dbError() }) + const res = await request(app).delete('/api/instances/100') + expect(res.status).toBe(500) + expect(res.body).toEqual({ error: 'internal server error' }) + }) + + it('DELETE logs the error with route context when DB throws unexpectedly', async () => { + await request(app).post('/api/instances').send({ ...base, stack: 'development', state: 'testing' }) + vi.spyOn(dbModule, 'deleteInstance').mockImplementationOnce(() => { throw dbError() }) + await request(app).delete('/api/instances/100') + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('DELETE /api/instances/:vmid'), + expect.any(Error) + ) + }) +})