mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-19 21:46:49 +00:00
Maurice
3a7c8e1d26
An adversarial audit over the three merged migration PRs. Sixteen findings
survived triple refutation; these are the ones with a defect behind them.
The serious one is mine, from the gateway move. ws registers
`error: this.emit.bind(this, 'error')` on the http server it attaches to, and
the adapter now runs inside buildApp, before listen(). So from that moment the
http server HAS an error listener and Node stops throwing on one. The adapter's
handler was `() => {}`, which meant EADDRINUSE was forwarded there and dropped:
the process survived, the listen callback never ran, so the banner never
printed and the scheduler never started. A container that looks healthy and
serves nothing. Reproduced, then fixed at both ends: index.ts owns the fatal
decision on the http server, and the adapter logs instead of swallowing.
Four DTOs were tightening contracts rather than describing them, which is the
thing the pipe-before-handler ordering makes easy to do by accident:
- OauthClientCreateDto replaced 'Name is required' with a Zod field report, and
pre-empted the handler's 403 on an instance with the MCP addon off.
- BookingImportConfirmDto demanded per-item fields the service never reads, so
a caller assembling items itself got a 400 about `source`.
- TagCreateDto/TagUpdateDto typed name and color as strings; the legacy route
bound whatever arrived, so a numeric name used to create a tag.
- PluginUserSettingsUpdateDto typed config as a record, where the handler
answers 200 for anything it cannot use.
Two of my own tests were false-green, which the audit demonstrated by mutation
rather than by reading. WSGW-008 asserted on frames that never carry the user
object at all, so it passed with the password_version strip deleted; it reads
the retained identity now. GEO-009 made the same assertion before and after the
write; it distinguishes a cached negative from a miss now, through a hasCached
seam that says what getCached cannot. Both were re-run against the mutation
they exist to catch, and both fail on it.
booking-import geocodes every venue and endpoint in one request loop, up to
thirty sequential calls. Folding maps onto the shared Nominatim cursor put that
loop on the interactive lane, so an import starved every concurrent keystroke
for half a minute. It takes the background lane now, which is what the two-lane
split was built for.
Also: the README's in-container bridge list had outlived one entry and was
missing another, and the journey e2e still mocked a module this branch deleted.
80 lines
3.1 KiB
TypeScript
80 lines
3.1 KiB
TypeScript
import { describe, it, expect, vi } from 'vitest';
|
|
import http from 'node:http';
|
|
import { WebSocketServer } from 'ws';
|
|
|
|
/**
|
|
* A bind failure must stay loud after the ws server moved in front of listen().
|
|
*
|
|
* `ws` registers `error: this.emit.bind(this, 'error')` on the http server it
|
|
* attaches to (websocket-server.js). Since buildApp now creates the ws server
|
|
* BEFORE index.ts calls listen(), the http server already has an error listener
|
|
* by the time the bind is attempted — which is exactly the condition under which
|
|
* Node stops throwing on an unhandled one.
|
|
*
|
|
* The failure that hides behind that is not subtle: EADDRINUSE gets forwarded to
|
|
* the WebSocketServer, and if its handler is a no-op the process survives, the
|
|
* listen callback never runs, so the startup banner never prints and the
|
|
* scheduler never starts. A container that looks healthy and serves nothing.
|
|
*
|
|
* These cases pin the mechanism rather than index.ts itself, because the module
|
|
* calls process.exit and boots the whole app on import.
|
|
*/
|
|
async function occupyAPort(): Promise<{ port: number; release: () => void }> {
|
|
const blocker = http.createServer();
|
|
await new Promise<void>((resolve) => blocker.listen(0, '127.0.0.1', resolve));
|
|
const { port } = blocker.address() as { port: number };
|
|
return { port, release: () => blocker.close() };
|
|
}
|
|
|
|
describe('ws attached before listen()', () => {
|
|
it('WSBIND-001: ws silences the http server, so a no-op handler loses EADDRINUSE entirely', async () => {
|
|
const { port, release } = await occupyAPort();
|
|
try {
|
|
const server = http.createServer();
|
|
const wss = new WebSocketServer({ server, path: '/ws' });
|
|
const swallowed = vi.fn();
|
|
wss.on('error', swallowed); // the shape this used to have
|
|
|
|
let listenCallbackRan = false;
|
|
server.listen(port, '127.0.0.1', () => { listenCallbackRan = true; });
|
|
await new Promise((r) => setTimeout(r, 300));
|
|
|
|
// The bind failed, and the only thing that heard about it was the ws
|
|
// handler. Nothing threw, nothing exited.
|
|
expect(swallowed).toHaveBeenCalledTimes(1);
|
|
expect((swallowed.mock.calls[0][0] as NodeJS.ErrnoException).code).toBe('EADDRINUSE');
|
|
expect(listenCallbackRan).toBe(false);
|
|
|
|
wss.close();
|
|
server.close();
|
|
} finally {
|
|
release();
|
|
}
|
|
});
|
|
|
|
it('WSBIND-002: an explicit http-server error handler is what makes it fatal again', async () => {
|
|
const { port, release } = await occupyAPort();
|
|
try {
|
|
const server = http.createServer();
|
|
const wss = new WebSocketServer({ server, path: '/ws' });
|
|
wss.on('error', () => {});
|
|
|
|
// What index.ts registers. It has to sit on the HTTP server: by the time
|
|
// ws re-emits, the process has already declined to throw.
|
|
const fatal = vi.fn();
|
|
server.on('error', fatal);
|
|
|
|
server.listen(port, '127.0.0.1', () => {});
|
|
await new Promise((r) => setTimeout(r, 300));
|
|
|
|
expect(fatal).toHaveBeenCalledTimes(1);
|
|
expect((fatal.mock.calls[0][0] as NodeJS.ErrnoException).code).toBe('EADDRINUSE');
|
|
|
|
wss.close();
|
|
server.close();
|
|
} finally {
|
|
release();
|
|
}
|
|
});
|
|
});
|