Skip to content

fix(serve-instrument): keep the server running when a rebuild or request fails #1851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sanilas-ai wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(serve-instrument): keep the server running when a rebuild or request fails #1851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .agents/docs/architecture/testing-strategy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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` |
Expand Down
15 changes: 13 additions & 2 deletions packages/serve-instrument/AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 <dir>`.
96 changes: 78 additions & 18 deletions packages/serve-instrument/src/__tests__/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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<typeof vi.fn>[]
// 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<typeof vi.fn>;
target: string;
watcher: EventEmitter;
}[]
}));

vi.mock('node:fs', async (importOriginal) => {
const actual = await importOriginal<typeof import('node:fs')>();
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;
}
};
});
Expand Down Expand Up @@ -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();
});

Expand Down Expand Up @@ -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'))
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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();
}
});
Expand Down Expand Up @@ -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);
});
});
});
});
Loading
Loading