diff --git a/.agents/docs/architecture/testing-strategy.md b/.agents/docs/architecture/testing-strategy.md index 338fbe34c..f0acf24be 100644 --- a/.agents/docs/architecture/testing-strategy.md +++ b/.agents/docs/architecture/testing-strategy.md @@ -50,7 +50,7 @@ package's config, which is **not** always the directory name. | `runtime-internal` | `packages/runtime-internal` | node environment; `bootstrap.js` and `worker.js` are tested against stubbed browser globals — see `packages/runtime-internal/AGENTS.md` | | `runtime-meta` | `packages/runtime-meta` | | | `schemas` | `packages/schemas` | | -| `serve-instrument` | `packages/serve-instrument` | node environment; `test/` mocks `fs.watch` (never closed by `Server.stop()`, see `packages/serve-instrument/AGENTS.md`) and stubs the built `client.js`/`__TAILWIND_STYLES__` that only exist post-build | +| `serve-instrument` | `packages/serve-instrument` | node environment; `src/__tests__/server.test.ts` mocks `fs.watch` (see `packages/serve-instrument/AGENTS.md`) and stubs the built `client.js`/`__TAILWIND_STYLES__` that only exist post-build | | `subject-utils` | `packages/subject-utils` | | | `vite-plugin-runtime` | `packages/vite-plugin-runtime` | | | `runtime-v1` | `runtime/v1` | directory is `v1`, project is `runtime-v1` | diff --git a/packages/serve-instrument/AGENTS.md b/packages/serve-instrument/AGENTS.md index cddeda9ff..f1d9bc3ca 100644 --- a/packages/serve-instrument/AGENTS.md +++ b/packages/serve-instrument/AGENTS.md @@ -34,6 +34,16 @@ instrument directory and exposes `close()`; `InstrumentLoaderMap` and both handl handler) that is not wired into that chain leaks a live watcher per `Server` instance, which keeps the process alive and, in tests, keeps firing against deleted temp directories. +**A request or a rebuild must never reject unhandled.** Both handlers' `handle` and +`InstrumentLoader.updateEncodedBundle` run detached from an http request callback or an `fs.watch` +callback, where Node turns an unhandled rejection into a process exit, killing every instrument in +`--all` mode. `Server`'s request callback catches a rejected `handle`, logs it and answers 500; that +works only because every `await` in `handle` runs before `writeHead`, so render the page first and +write the head after. `updateEncodedBundle` keeps the directory listing and file reads inside its +`try` and clears `encodedBundle` on failure, so a deleted instrument answers 404 (503 in single +mode) instead of its last bundle. Each watcher also has an `'error'` listener (Windows reports a +deleted directory as `EPERM` there). + **`cli.ts` validation belongs in commander's parsers, not the `.action()` body.** `parseTarget` is passed to `.argument()` and `parsePort` to `.option()`, so an `InvalidArgumentError` thrown by either takes commander's print-message-and-`process.exit(1)` path. `parseTarget` needs the `--all` flag, @@ -57,8 +67,9 @@ stubs the `__ROOT_PROPS__` global before importing the entry. `src/__tests__/ser real `Server` against real temp-directory fixtures and drives it with real `fetch` calls; it stubs the `client.js` / `__TAILWIND_STYLES__` globals that only exist post-build, wraps instrument-bundler's `bundle` in `vi.fn(actual.bundle)` so a test can force a rejection, and replaces `fs.watch` at the -module level (it is a named ESM export, which `vi.spyOn` cannot touch) so a test can fire the rebuild -callback on demand and assert every watcher's `close()` ran after `stop()`. +module level (it is a named ESM export, which `vi.spyOn` cannot touch) so a test can fire one directory's rebuild +callback on demand, emit `'error'` on its watcher, and assert every watcher's `close()` ran after +`stop()`. There is no `dev` script either: `pnpm --filter @opendatacapture/serve-instrument build`, then `node dist/cli.js `. diff --git a/packages/serve-instrument/src/__tests__/server.test.ts b/packages/serve-instrument/src/__tests__/server.test.ts index c3e7120d6..570b0d3b1 100644 --- a/packages/serve-instrument/src/__tests__/server.test.ts +++ b/packages/serve-instrument/src/__tests__/server.test.ts @@ -1,3 +1,4 @@ +import type { EventEmitter } from 'node:events'; import * as fs from 'node:fs'; import * as net from 'node:net'; import * as os from 'node:os'; @@ -7,23 +8,29 @@ import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } // A real watcher would keep firing against a deleted temp directory after each test, so `fs.watch` // is stubbed. It is a named ESM export, which vitest cannot `spyOn` directly ("Module namespace is -// not configurable"), so it is replaced at the module level instead. `watchCallbacks` captures what -// each call registered so a test can trigger the rebuild deliberately, and `watchCloses` records -// every `close()` so a test can assert `Server.stop()` disposes the handles rather than leaking them. -const { watchCallbacks, watchCloses } = vi.hoisted(() => ({ - watchCallbacks: [] as (() => void)[], - watchCloses: [] as ReturnType[] +// not configurable"), so it is replaced at the module level instead. Each entry in `watchers` keeps the +// watched directory and the callback it registered, so a test can trigger one instrument's rebuild +// deliberately; its `close()` mock, so a test can assert `Server.stop()` disposes the handles rather +// than leaking them; and the returned emitter, so a test can raise the `'error'` a real watcher emits. +const { watchers } = vi.hoisted(() => ({ + watchers: [] as { + callback: () => void; + close: ReturnType; + target: string; + watcher: EventEmitter; + }[] })); vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal(); + const { EventEmitter } = await import('node:events'); return { ...actual, - watch: (_target: unknown, _options: unknown, callback: () => void) => { - watchCallbacks.push(callback); + watch: (target: string, _options: unknown, callback: () => void) => { const close = vi.fn(); - watchCloses.push(close); - return { close }; + const watcher = Object.assign(new EventEmitter(), { close }); + watchers.push({ callback, close, target, watcher }); + return watcher; } }; }); @@ -127,8 +134,7 @@ describe('Server', () => { tmpDirs.push(tmpDir); port = await getFreePort(); stubClientBundle(); - watchCallbacks.length = 0; - watchCloses.length = 0; + watchers.length = 0; vi.mocked(bundle).mockClear(); }); @@ -160,8 +166,8 @@ describe('Server', () => { try { // Populate the initial bundle first, then simulate the file watcher firing. await fetch(`http://localhost:${port}/`); - expect(watchCallbacks).toHaveLength(1); - watchCallbacks[0]!(); + expect(watchers).toHaveLength(1); + watchers[0]!.callback(); await vi.waitFor(() => { expect( logSpy.mock.calls.some(([message]) => typeof message === 'string' && message.includes('rebuilding')) @@ -177,9 +183,9 @@ describe('Server', () => { fs.writeFileSync(path.join(tmpDir, 'index.ts'), FORM_SOURCE); const server = await Server.create({ mode: 'single', port, target: tmpDir, verbose: false }); await server.start(); - expect(watchCloses).toHaveLength(1); + expect(watchers).toHaveLength(1); await server.stop(); - expect(watchCloses[0]).toHaveBeenCalledOnce(); + expect(watchers[0]!.close).toHaveBeenCalledOnce(); }); it('should reject a non-GET request with 405', async () => { @@ -280,6 +286,32 @@ describe('Server', () => { }); }); + it('should answer 500 and keep serving when handling a request fails, rather than crashing', async () => { + fs.writeFileSync(path.join(tmpDir, 'index.ts'), FORM_SOURCE); + const errors = spyOnLogs('error'); + await withServer('single', async () => { + expect((await fetch(`http://localhost:${port}/`)).status).toBe(200); + vi.mocked(fs.promises.readFile).mockRejectedValueOnce(new Error('client bundle unreadable')); + expect((await fetch(`http://localhost:${port}/`)).status).toBe(500); + expect((await fetch(`http://localhost:${port}/`)).status).toBe(200); + }); + expect(errors.some(/client bundle unreadable/)).toBe(true); + expect(errors.some(/Failed to handle GET \//)).toBe(true); + }); + + it('should log a watcher error and rebuild on the next request, rather than crashing', async () => { + fs.writeFileSync(path.join(tmpDir, 'index.ts'), FORM_SOURCE); + const errors = spyOnLogs('error'); + await withServer('single', async () => { + await fetch(`http://localhost:${port}/`); + expect(bundle).toHaveBeenCalledOnce(); + watchers[0]!.watcher.emit('error', new Error('EPERM: operation not permitted, watch')); + expect(errors.some(/Stopped watching instrument for changes/)).toBe(true); + expect((await fetch(`http://localhost:${port}/`)).status).toBe(200); + expect(bundle).toHaveBeenCalledTimes(2); + }); + }); + it('should reject a second stop, since the http server is no longer running', async () => { fs.writeFileSync(path.join(tmpDir, 'index.ts'), FORM_SOURCE); const server = await Server.create({ mode: 'single', port, target: tmpDir, verbose: false }); @@ -317,9 +349,9 @@ describe('Server', () => { } const server = await Server.create({ mode: 'all', port, target: tmpDir, verbose: false }); await server.start(); - expect(watchCloses).toHaveLength(2); + expect(watchers).toHaveLength(2); await server.stop(); - for (const close of watchCloses) { + for (const { close } of watchers) { expect(close).toHaveBeenCalledOnce(); } }); @@ -416,5 +448,33 @@ describe('Server', () => { expect(await res.json()).toMatchObject({ sources: expect.any(Array) }); }); }); + + it('should keep serving other instruments after an instrument directory is deleted, so a rebuild failure does not kill the server', async () => { + for (const name of ['FOO', 'BAR']) { + fs.mkdirSync(path.join(tmpDir, 'forms', name), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, 'forms', name, 'index.ts'), FORM_SOURCE); + } + const errors = spyOnLogs('error'); + await withServer('all', async () => { + expect((await fetch(`http://localhost:${port}/forms/FOO`)).status).toBe(200); + expect((await fetch(`http://localhost:${port}/forms/BAR`)).status).toBe(200); + const barDir = path.join(tmpDir, 'forms', 'BAR'); + fs.rmSync(barDir, { force: true, recursive: true }); + watchers.find(({ target }) => target === barDir)!.callback(); + await vi.waitFor(() => { + expect(errors.some(/Failed to compile forms\/BAR/)).toBe(true); + }); + expect((await fetch(`http://localhost:${port}/forms/FOO`)).status).toBe(200); + expect((await fetch(`http://localhost:${port}/forms/BAR`)).status).toBe(404); + }); + }); + + it('should answer 400 to a malformed percent-escape in an instrument URL rather than crashing', async () => { + fs.mkdirSync(path.join(tmpDir, 'forms'), { recursive: true }); + await withServer('all', async () => { + expect((await fetch(`http://localhost:${port}/forms/%E0%A4%A`)).status).toBe(400); + expect((await fetch(`http://localhost:${port}/`)).status).toBe(200); + }); + }); }); }); diff --git a/packages/serve-instrument/src/server.tsx b/packages/serve-instrument/src/server.tsx index 418520d82..480c97231 100644 --- a/packages/serve-instrument/src/server.tsx +++ b/packages/serve-instrument/src/server.tsx @@ -32,6 +32,12 @@ function logError(message: string): void { console.error(`${timestamp()} ${message}`); } +function logFailure(err: unknown, summary: string): void { + const formattedError = err instanceof Error ? formatError(err) : err; + logError(`${chalk.dim(String(formattedError))}\n`); + logError(chalk.red('✘') + chalk.bold.red(` Error: ${summary}`)); +} + class InstrumentLoader { private encodedBundle: null | string; private readonly watcher: fs.FSWatcher; @@ -46,6 +52,14 @@ class InstrumentLoader { log(chalk.yellow('↺') + chalk.dim(` [${this.label}] File changed, rebuilding...`)); void this.updateEncodedBundle(); }); + this.watcher.on('error', (err) => { + this.encodedBundle = null; + logFailure(err, `Stopped watching ${this.displayName} for changes, restart to resume`); + }); + } + + private get displayName(): string { + return this.label || 'instrument'; } close(): void { @@ -60,32 +74,33 @@ class InstrumentLoader { } private async updateEncodedBundle(): Promise { - const inputs: BundlerInput[] = []; - const filepaths = await fs.promises - .readdir(this.target, 'utf-8') - .then((filenames) => filenames.filter((filename) => BUNDLER_FILE_EXT_REGEX.test(filename))) - .then((filenames) => filenames.map((filename) => path.resolve(this.target, filename))); - - for (const filepath of filepaths) { - const loader = inferLoader(filepath); - const content: string | Uint8Array = await fs.promises.readFile(filepath, loader === 'dataurl' ? null : 'utf-8'); - inputs.push({ - content: content instanceof Uint8Array ? new Uint8Array(content) : content, - name: path.basename(filepath) - }); - } - const start = Date.now(); try { + const inputs: BundlerInput[] = []; + const filepaths = await fs.promises + .readdir(this.target, 'utf-8') + .then((filenames) => filenames.filter((filename) => BUNDLER_FILE_EXT_REGEX.test(filename))) + .then((filenames) => filenames.map((filename) => path.resolve(this.target, filename))); + + for (const filepath of filepaths) { + const loader = inferLoader(filepath); + const content: string | Uint8Array = await fs.promises.readFile( + filepath, + loader === 'dataurl' ? null : 'utf-8' + ); + inputs.push({ + content: content instanceof Uint8Array ? new Uint8Array(content) : content, + name: path.basename(filepath) + }); + } + const start = Date.now(); this.encodedBundle = encodeUnicodeToBase64(await bundle({ inputs })); const elapsed = Date.now() - start; const timing = this.verbose ? chalk.dim(` (${elapsed}ms)`) : ''; const labelPart = this.label ? chalk.dim(` [${this.label}]`) : ''; log(chalk.green('✓') + labelPart + chalk.dim(' Bundle ready') + timing); } catch (err) { - const formattedError = err instanceof Error ? formatError(err) : err; - logError(`${chalk.dim(String(formattedError))}\n`); - const labelPart = this.label ? ` ${this.label}` : ' instrument'; - logError(chalk.red('✘') + chalk.bold.red(` Error: Failed to compile${labelPart}`)); + this.encodedBundle = null; + logFailure(err, `Failed to compile ${this.displayName}`); } } } @@ -160,8 +175,9 @@ class SingleModeHandler { } else if (req.url === '/') { const encodedBundle = await this.instrumentLoader.getEncodedBundle(); if (encodedBundle) { + const html = await this.renderPage({ encodedBundle, page: 'single' }); res.writeHead(200, { 'Content-Type': 'text/html' }); - res.end(`${await this.renderPage({ encodedBundle, page: 'single' })}`); + res.end(`${html}`); } else { res.writeHead(503, { 'Content-Type': 'text/plain' }); res.end('Bundle Loading or Error (See Console)'); @@ -234,30 +250,37 @@ class AllModeHandler { } if (req.url === '/') { - const props: RootProps = { instruments: this.loaderMap.getInstrumentList(), page: 'index' }; + const html = await this.renderPage({ instruments: this.loaderMap.getInstrumentList(), page: 'index' }); res.writeHead(200, { 'Content-Type': 'text/html' }); - res.end(`${await this.renderPage(props)}`); + res.end(`${html}`); return; } const instrumentMatch = req.url?.match(/^\/(forms|interactive)\/([^/]+)\/?$/); if (instrumentMatch) { const [, type, name] = instrumentMatch; - const decodedName = decodeURIComponent(name!); + let decodedName: string; + try { + decodedName = decodeURIComponent(name!); + } catch { + res.writeHead(400, { 'Content-Type': 'text/plain' }); + res.end('Bad Request'); + return; + } const encodedBundle = await this.loaderMap.getEncodedBundle(type!, decodedName); if (!encodedBundle) { res.writeHead(404, { 'Content-Type': 'text/plain' }); res.end('Instrument Not Found'); return; } - const props: RootProps = { + const html = await this.renderPage({ encodedBundle, instrumentName: decodedName, instrumentType: type!, page: 'instrument' - }; + }); res.writeHead(200, { 'Content-Type': 'text/html' }); - res.end(`${await this.renderPage(props)}`); + res.end(`${html}`); return; } @@ -309,7 +332,13 @@ export class Server { private constructor(params: { handler: AllModeHandler | SingleModeHandler; port: number }) { this.handler = params.handler; this.port = params.port; - this.server = http.createServer((...args) => void this.handler.handle(...args)); + this.server = http.createServer((req, res) => { + this.handler.handle(req, res).catch((err: unknown) => { + logFailure(err, `Failed to handle ${req.method} ${req.url}`); + res.writeHead(500, { 'Content-Type': 'text/plain' }); + res.end('Internal Server Error'); + }); + }); } static async create({