diff --git a/server/src/index.ts b/server/src/index.ts index 9504ed2c0..017f8d52c 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -104,6 +104,20 @@ async function bootstrap(): Promise { // would serve the REST API fine and leave the gateway attached to a socket // nobody listens on. server = getHttpServer(); + + // A bind failure has to be fatal and loud, and it needs saying explicitly. + // listen() reports failure by event, not by rejecting, so the catch around + // bootstrap() never sees it. And since buildApp attaches the ws server to this + // http server, ws has already registered its own `error` listener on it, which + // is enough for Node to stop throwing on an unhandled one. Without this the + // process would survive EADDRINUSE, never run onListen, and serve nothing + // while looking healthy. + server.on('error', (err: NodeJS.ErrnoException) => { + const where = HOST ? `${HOST}:${PORT}` : `:${PORT}`; + console.error(`Fatal: cannot listen on ${where} — ${err.code ?? ''} ${err.message}`); + process.exit(1); + }); + if (HOST) server.listen(PORT, HOST, onListen); else server.listen(PORT, onListen); } diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 941603036..84897ac3c 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -243,8 +243,9 @@ through. `@trek/nest-mcp` hands the gate its declaring instance now, and `AddonsService`. `reservations.mcp.ts` (assignments) and `atlas.mcp.ts` / `journey.mcp.ts` (auth) went the same way. -Three in-container uses are left, each a real module cycle that injection does -not fix: +Four in-container uses are left, each for a reason that injection does not fix. +Keep this list current: an entry that outlives its cause reads as a standing +exception and invites the next one. - `places.mcp.ts` → `assignments.bridge`: a real cycle, `DaysModule → PlacesModule → AssignmentsModule → DaysModule`. @@ -252,11 +253,20 @@ not fix: `TripsModule` imports both, so importing it back closes the loop. - `auth/user-cleanup.service.ts` → `budget.bridge`: `BudgetModule` imports `AuthModule`, same shape. +- `backup/backup.impl.ts` → `permissions.bridge`: free functions, not a + provider, so there is nothing for an injected service to hang off. Its second + caller, `src/scheduler.ts`, is gone; the cache invalidation is not. -(A fourth — `files.controller.ts` / `journey.controller.ts` → `files.bridge` for -multer's module-scope interceptor options — was resolved 2026-08-10 exactly as +`files.bridge` used to be the fifth and is gone, resolved 2026-08-10 exactly as predicted: `MulterModule.registerAsync` over an `AllowedFileTypesService` leaf, -and `files.bridge.ts` is deleted.) +and `files.controller.ts` / `journey.controller.ts` build their multer options +from that. + +**Four bridges have no production consumer at all** — `days`, `packing`, +`photos`, `reservations`. Only their own delegation tests import them, and each +still builds a service outside the container at module load for nobody. They are +deletable, along with the comments in `days.service.ts`, `packing.service.ts` and +`reservations.service.ts` that still send readers to them. The nine fire-and-forget notification senders inject too. They were lazy `import('../notifications/notifications.bridge').then(({ send }) => …)` calls diff --git a/server/src/nest/booking-import/booking-import.service.ts b/server/src/nest/booking-import/booking-import.service.ts index 9e0c431a3..92f703654 100644 --- a/server/src/nest/booking-import/booking-import.service.ts +++ b/server/src/nest/booking-import/booking-import.service.ts @@ -153,7 +153,7 @@ export class BookingImportService { ].filter((q): q is string => !!q); for (const q of queries) { - const results = await this.maps.searchNominatim(q); + const results = await this.maps.searchNominatim(q, undefined, 'background'); const hit = results[0]; if (hit?.lat != null && hit?.lng != null) { lat = hit.lat; @@ -185,7 +185,7 @@ export class BookingImportService { for (const ep of reservationData.endpoints) { if ((ep.lat == null || ep.lng == null) && ep.name) { try { - const hit = (await this.maps.searchNominatim(ep.name))[0]; + const hit = (await this.maps.searchNominatim(ep.name, undefined, 'background'))[0]; if (hit?.lat != null && hit?.lng != null) { ep.lat = hit.lat; ep.lng = hit.lng; diff --git a/server/src/nest/geo/nominatim.client.ts b/server/src/nest/geo/nominatim.client.ts index ec1562f94..d3276964d 100644 --- a/server/src/nest/geo/nominatim.client.ts +++ b/server/src/nest/geo/nominatim.client.ts @@ -99,6 +99,18 @@ export function getCached(key: string): T | undefined { return cache.has(key) ? (cache.get(key) as T) : undefined; } +/** + * Whether the key is present at all, negative answers included. + * + * `getCached` cannot express the difference: a cached `undefined` and a miss + * both read as `undefined`. Callers that only want the value do not care, but a + * caller deciding whether to go back to the network does, and so does any test + * asserting that a negative answer is retained. + */ +export function hasCached(key: string): boolean { + return cache.has(key); +} + export function setCached(key: string, value: unknown): void { cache.set(key, value); } diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index cb04e558f..8a997aba9 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -13,7 +13,7 @@ import { decrypt_api_key } from '../common/crypto/apiKeyCrypto'; // ── Photo cache (disk-backed) ──────────────────────────────────────────────── import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; import { DatabaseService } from '../database/database.service'; -import { nominatimFetch } from '../geo/nominatim.client'; +import { nominatimFetch, type GeoLane } from '../geo/nominatim.client'; import { UA, toApiLang, @@ -540,7 +540,17 @@ export class MapsService { // ── Nominatim search ─────────────────────────────────────────────────────── - async searchNominatim(query: string, lang?: string) { + /** + * `lane` defaults to interactive because most callers are a keystroke. + * + * Bulk callers must pass 'background': booking-import geocodes every venue and + * every uncoordinated endpoint of an import in one request loop, which is up + * to thirty sequential calls. On the interactive lane those thirty take the + * next slot each time, so somebody typing in the place search waits behind the + * whole import. Yielding does not make the import faster, it stops it from + * being the only thing the process will do for half a minute. + */ + async searchNominatim(query: string, lang?: string, lane: GeoLane = 'interactive') { const params = new URLSearchParams({ q: query, format: 'json', @@ -553,9 +563,8 @@ export class MapsService { limit: '10', 'accept-language': toApiLang(lang), }); - // Through the shared client: one throttle for the whole process, and the - // interactive lane so a keystroke does not queue behind the atlas backfill. - const response = await nominatimFetch('search', params); + // Through the shared client: one throttle for the whole process. + const response = await nominatimFetch('search', params, { lane }); if (!response.ok) { const text = await response.text().catch(() => ''); throw new Error( diff --git a/server/src/nest/oauth/oauth.dto.ts b/server/src/nest/oauth/oauth.dto.ts index cd8ffb4b8..5200344a9 100644 --- a/server/src/nest/oauth/oauth.dto.ts +++ b/server/src/nest/oauth/oauth.dto.ts @@ -1,17 +1,31 @@ import { createZodDto } from 'nestjs-zod'; -import { oauthClientCreateRequestSchema, oauthConsentRequestSchema } from '@trek/shared'; +import { z } from 'zod'; +import { oauthConsentRequestSchema } from '@trek/shared'; /** - * Server-side createZodDto wrappers over the @trek/shared OAuth contracts. + * Server-side createZodDto wrappers for the OAuth bodies. * - * Both schemas already described these two bodies exactly; they were written - * with the contracts and then never wired to the handlers, which declared the - * same shape inline as an anonymous type. The inline types are gone now, so the - * wire shape has one definition instead of two that could drift. + * Consent takes the shared schema unchanged: every field it requires is one the + * handler dereferences immediately (redirect_uri is parsed as a URL, the PKCE + * pair goes straight into the stored code), so a body missing them had no + * meaningful old behaviour to preserve. * - * Neither handler owns a bespoke required-field message: consent redirects with - * `error=access_denied` on a rejection, and client creation surfaces whatever - * the service returns. So nothing user-visible moves in front of the pipe here. + * Client creation does NOT, and the difference is deliberate. The shared schema + * declares `name: z.string().min(1)` and `allowed_scopes` as required, but the + * handler answers 403 `{ error: 'MCP is not enabled' }` before it looks at the + * body at all, and the service owns `Name is required` and + * `At least one redirect URI is required`. The pipe runs first, so adopting the + * schema verbatim turned a 403 into a 400 on an instance with the addon off, and + * replaced two error strings a client shows the user. Describing the shape + * without demanding it keeps every one of those answers where it was. */ export class OauthConsentDto extends createZodDto(oauthConsentRequestSchema) {} -export class OauthClientCreateDto extends createZodDto(oauthClientCreateRequestSchema) {} + +export class OauthClientCreateDto extends createZodDto( + z.looseObject({ + name: z.string().optional(), + redirect_uris: z.array(z.string()).optional(), + allowed_scopes: z.array(z.string()).optional(), + allows_client_credentials: z.boolean().optional(), + }), +) {} diff --git a/server/src/nest/plugins/plugin-user-settings.controller.ts b/server/src/nest/plugins/plugin-user-settings.controller.ts index c83e8a8fa..7fced5d18 100644 --- a/server/src/nest/plugins/plugin-user-settings.controller.ts +++ b/server/src/nest/plugins/plugin-user-settings.controller.ts @@ -80,7 +80,7 @@ export class PluginUserSettingsController { ): { config: Record } { const userId = req.user?.id; if (!pluginsEnabled() || userId == null || !this.activeWithUserFields(id)) return { config: {} }; - const patch = body?.config && typeof body.config === 'object' ? body.config : {}; + const patch = body?.config && typeof body.config === 'object' ? (body.config as Record) : {}; return { config: this.plugins.updateUserConfig(id, userId, patch) }; } } diff --git a/server/src/nest/plugins/plugins.dto.ts b/server/src/nest/plugins/plugins.dto.ts index c3f123d35..877ee59d2 100644 --- a/server/src/nest/plugins/plugins.dto.ts +++ b/server/src/nest/plugins/plugins.dto.ts @@ -59,8 +59,13 @@ export class PluginEgressHostsDto extends createZodDto( */ export class PluginConfigDto extends createZodDto(z.looseObject({})) {} +/** + * `config` is unknown rather than a record: the handler answers 200 with the + * stored config for anything it cannot use, including a non-object, and a schema + * that rejected first would turn that into a 400. + */ export class PluginUserSettingsUpdateDto extends createZodDto( - z.looseObject({ config: z.record(z.string(), z.unknown()).optional() }), + z.looseObject({ config: z.unknown().optional() }), ) {} /** diff --git a/server/src/nest/realtime/trek-ws.adapter.ts b/server/src/nest/realtime/trek-ws.adapter.ts index 973212a3e..6359cd65a 100644 --- a/server/src/nest/realtime/trek-ws.adapter.ts +++ b/server/src/nest/realtime/trek-ws.adapter.ts @@ -6,6 +6,7 @@ import { WebSocketServer } from 'ws'; import type { Observable } from 'rxjs'; import { readEnv } from '../../app-config'; import { setServer, type TrekWebSocket } from './ws-state'; +import { logError } from '../audit/audit-log.logger'; // Per-connection message rate limiting. It lives in the adapter, not in the // gateway's handlers, because the original counted EVERY inbound frame before @@ -96,9 +97,16 @@ export class TrekWsAdapter extends WsAdapter { : {}), }); - // A server-level error must not reach the process. The base binds one on the - // instances it creates; this one is ours, so it needs its own. - server.on('error', () => {}); + // A server-level error is LOGGED, never swallowed. + // + // ws hangs its own `error: this.emit.bind(this, 'error')` on the http server + // (websocket-server.js), so from the moment this runs the http server has a + // listener and Node will not throw on one. Everything it forwards lands + // here. An empty handler therefore eats the http server's failures too, + // including EADDRINUSE at listen() — the process would stay up, serving + // nothing, with no output at all. index.ts owns the fatal decision for bind + // failures; this one keeps the transport's own errors visible. + server.on('error', (err) => logError(`ws server error: ${err instanceof Error ? err.message : String(err)}`)); // The fan-out primitives reach the live server through here, and so do the // out-of-container bridges. diff --git a/server/src/nest/reservation-import/reservation-import.controller.ts b/server/src/nest/reservation-import/reservation-import.controller.ts index 2f564c83f..af77fb9d7 100644 --- a/server/src/nest/reservation-import/reservation-import.controller.ts +++ b/server/src/nest/reservation-import/reservation-import.controller.ts @@ -185,6 +185,8 @@ export class ReservationImportController { throw new HttpException({ error: 'items must be a non-empty array' }, 400); } - return this.bookingImport.confirm(tripId, items, socketId); + // The DTO asserts an array of objects and stops there, so the per-item shape + // is still the service's to interpret — as it was before the contract landed. + return this.bookingImport.confirm(tripId, items as BookingImportPreviewItem[], socketId); } } diff --git a/server/src/nest/reservation-import/reservation-import.dto.ts b/server/src/nest/reservation-import/reservation-import.dto.ts index 3a7e2e1eb..5b44f02fa 100644 --- a/server/src/nest/reservation-import/reservation-import.dto.ts +++ b/server/src/nest/reservation-import/reservation-import.dto.ts @@ -1,17 +1,18 @@ import { createZodDto } from 'nestjs-zod'; import { z } from 'zod'; -import { bookingImportPreviewItemSchema } from '@trek/shared'; /** * Server-side createZodDto wrappers for the booking-import bodies. * - * Confirm validates each item against the shared contract but leaves `items` - * itself optional, which is narrower than `bookingImportConfirmRequestSchema` - * on purpose. That schema declares `.min(1)`, and the pipe reports a rejection - * as `field: message` — so adopting it verbatim would replace the endpoint's - * `{ error: 'items must be a non-empty array' }`. The handler keeps the check - * that owns the message, and the DTO adds what the handler never did: per-item - * shape validation. + * Confirm asserts that `items` is an array of objects, and nothing more. + * + * Two things pushed it there. `bookingImportConfirmRequestSchema` declares + * `.min(1)`, and the pipe reports a rejection as `field: message`, so adopting + * it verbatim would replace the endpoint's own + * `{ error: 'items must be a non-empty array' }`. And validating each item + * against `bookingImportPreviewItemSchema` demanded fields the service never + * reads, such as `source`: a caller assembling items itself rather than echoing + * a preview used to get a 201 and now got a 400 about a field nobody uses. * * Preview and its async twin carry a multipart body, so every field arrives as a * string and the real payload is the files. `mode` is described here for the @@ -21,7 +22,7 @@ import { bookingImportPreviewItemSchema } from '@trek/shared'; * adds and the handler ignores. */ export class BookingImportConfirmDto extends createZodDto( - z.object({ items: z.array(bookingImportPreviewItemSchema).optional() }), + z.object({ items: z.array(z.looseObject({})).optional() }), ) {} export class BookingImportPreviewDto extends createZodDto( diff --git a/server/src/nest/tags/tags.controller.ts b/server/src/nest/tags/tags.controller.ts index b3cd7d780..3dd8160e1 100644 --- a/server/src/nest/tags/tags.controller.ts +++ b/server/src/nest/tags/tags.controller.ts @@ -27,7 +27,10 @@ export class TagsController { @Post() create(@CurrentUser() user: User, @Body() body: TagCreateDto): { tag: Tag } { - const { name, color } = body; + // Cast rather than validate: the legacy route bound whatever arrived + // straight into SQLite, and a contract that rejected a numeric name here + // would be a new 400 for anything already sending one. + const { name, color } = body as { name?: string; color?: string }; if (!name) { throw new HttpException({ error: 'Tag name is required' }, 400); } @@ -40,7 +43,7 @@ export class TagsController { @Param('id') id: string, @Body() body: TagUpdateDto, ): { tag: Tag } { - const { name, color } = body; + const { name, color } = body as { name?: string; color?: string }; if (!this.tags.getByIdAndUser(id, user.id)) { throw new HttpException({ error: 'Tag not found' }, 404); } diff --git a/server/src/nest/tags/tags.dto.ts b/server/src/nest/tags/tags.dto.ts index a2b580b89..aefbb4d89 100644 --- a/server/src/nest/tags/tags.dto.ts +++ b/server/src/nest/tags/tags.dto.ts @@ -1,24 +1,25 @@ import { createZodDto } from 'nestjs-zod'; import { z } from 'zod'; -import { updateTagRequestSchema } from '@trek/shared'; /** * Server-side createZodDto wrappers over the @trek/shared tag contracts. * - * Create deliberately does NOT use `createTagRequestSchema` as written. That - * schema declares `name: z.string().min(1)`, and the pipe reports a rejection as + * Neither uses the shared schema as written. `createTagRequestSchema` declares + * `name: z.string().min(1)`, and the pipe reports a rejection as * `field: message` — so adopting it verbatim would replace the endpoint's - * `{ error: 'Tag name is required' }` with `name: ...`. Two tests pin that - * string, and more to the point it is what a client shows the user. - * - * So the DTO accepts the shape and leaves `name` optional, and the handler keeps - * the check that owns the message. The endpoint's actual contract is unchanged: - * a request without a name still gets 400, with the same body it always had. - * What the DTO does add is type and shape validation on everything else, which - * is what took this route off the allow-list. + * `{ error: 'Tag name is required' }`, which two tests pin and which is what a + * client shows the user. The handler keeps the check that owns the message. */ -export class TagCreateDto extends createZodDto( - z.object({ name: z.string().optional(), color: z.string().optional() }), -) {} +/** + * Both fields are `unknown`, not `string`. + * + * The handlers read them off `@Body('name')` / `@Body('color')` and passed + * whatever arrived to the service, which binds them into SQLite: a numeric name + * created a tag. Typing them as strings turns those requests into a 400 from the + * pipe, which is a contract change for anything already sending one. The service + * is where the value is interpreted, and it stays there. + */ +const tagBody = z.looseObject({ name: z.unknown().optional(), color: z.unknown().optional() }); -export class TagUpdateDto extends createZodDto(updateTagRequestSchema) {} +export class TagCreateDto extends createZodDto(tagBody) {} +export class TagUpdateDto extends createZodDto(tagBody) {} diff --git a/server/tests/e2e/journey.e2e.test.ts b/server/tests/e2e/journey.e2e.test.ts index b2bedf69f..d74569429 100644 --- a/server/tests/e2e/journey.e2e.test.ts +++ b/server/tests/e2e/journey.e2e.test.ts @@ -29,7 +29,6 @@ const { isAddonEnabled } = vi.hoisted(() => ({ isAddonEnabled: vi.fn(() => true) // The controller's pure helpers (isVideoExtension/isVideoMime/MAX_VIDEO_SIZE) // now come from the real files.constants; only the request-time app_settings // read is mocked, preserving the old '*'-allowlist semantics for the fixtures. -vi.mock('../../src/nest/files/files.bridge', () => ({ getAllowedExtensions: () => '*' })); // The memories providers are injected since the fold — stubbed on the prototype // so JourneyModule still resolves them through DI. vi.mock('../../src/nest/memories/immich.service', async (importOriginal) => { diff --git a/server/tests/unit/nest/geo.nominatim.test.ts b/server/tests/unit/nest/geo.nominatim.test.ts index c4dcb61f9..ad0a211be 100644 --- a/server/tests/unit/nest/geo.nominatim.test.ts +++ b/server/tests/unit/nest/geo.nominatim.test.ts @@ -15,6 +15,7 @@ import { cacheKeyFor, clearGeoCache, getCached, + hasCached, nominatimFetch, setCached, setGeoThrottleInterval, @@ -153,12 +154,17 @@ describe('geo cache', () => { expect(cacheKeyFor(52.53, 13.405, 'zoom18')).not.toBe(cacheKeyFor(52.52, 13.405, 'zoom18')); }); - it('GEO-009: distinguishes a cached undefined from a miss', () => { + it('GEO-009: a cached negative answer is a HIT, not a miss', () => { + // The earlier version asserted `undefined` before and after the write, which + // is the same assertion twice and held with the `has` guard removed. What + // the guard buys is that the second lookup does not go back to the network: + // the ocean does not become a country, so a null answer is worth keeping. const key = cacheKeyFor(0, 0, 'zoom3'); - expect(getCached(key)).toBeUndefined(); + expect(hasCached(key)).toBe(false); + setCached(key, undefined); - // A negative answer is worth caching: the ocean does not become a country. - // `has` rather than a truthy check is what keeps it from being re-fetched. + + expect(hasCached(key)).toBe(true); expect(getCached(key)).toBeUndefined(); }); diff --git a/server/tests/unit/nest/realtime.gateway.test.ts b/server/tests/unit/nest/realtime.gateway.test.ts index 1c0af527d..719c43a21 100644 --- a/server/tests/unit/nest/realtime.gateway.test.ts +++ b/server/tests/unit/nest/realtime.gateway.test.ts @@ -20,6 +20,7 @@ import { joinRoom, registerSocket, setServer, + userOf, type TrekWebSocket, } from '../../../src/nest/realtime/ws-state'; import { emitPluginEvent } from '../../../src/plugin-event-sink'; @@ -134,10 +135,16 @@ describe('RealtimeGateway handshake', () => { }); it('WSGW-008: never leaks password_version past the handshake', () => { - const { gw, ws } = connect('/ws?token=x'); - gw.handleJoin({ tripId: 7 }, ws); - broadcast(7, 'trip:updated' as never, {} as never); - expect(ws.sent.join('')).not.toContain('password_version'); + // Asserted on what the socket registry HOLDS, not on the frames it sends. + // The frames never carry the user object at all, so the earlier version of + // this case passed with the strip deleted: it proved nothing. What matters + // is that the retained identity is clean, because that object is what + // getOnlineUserIds and the onlyUserId filter read. + const { ws } = connect('/ws?token=x'); + const held = userOf(ws) as unknown as Record | undefined; + expect(held).toBeDefined(); + expect(held).not.toHaveProperty('password_version'); + expect(held).toMatchObject({ id: 3, email: 'm@x.test' }); }); }); diff --git a/server/tests/unit/nest/trek-ws-adapter.test.ts b/server/tests/unit/nest/trek-ws-adapter.test.ts index bc5828c22..d307f4ba8 100644 --- a/server/tests/unit/nest/trek-ws-adapter.test.ts +++ b/server/tests/unit/nest/trek-ws-adapter.test.ts @@ -12,7 +12,11 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; // The origin allowlist is env-driven and empty in the test environment, so the // branch that builds verifyClient would never run. Driving readEnv lets both // sides of it be asserted rather than assumed. -const { wsOrigins } = vi.hoisted(() => ({ wsOrigins: { value: null as string[] | null } })); +const { wsOrigins, logError } = vi.hoisted(() => ({ + wsOrigins: { value: null as string[] | null }, + logError: vi.fn(), +})); +vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ logError, logInfo: vi.fn(), logDebug: vi.fn(), logWarn: vi.fn() })); vi.mock('../../../src/app-config', async (importOriginal) => { const actual = (await importOriginal()) as Record; return { @@ -243,12 +247,21 @@ describe('TrekWsAdapter server creation', () => { } }); - it('WSAD-031c: a server-level error is absorbed, not rethrown at the process', () => { + it('WSAD-031c: a server-level error is LOGGED, never swallowed', () => { + // ws forwards the http server's errors here, so an empty handler eats the + // bind failure too and the process survives EADDRINUSE serving nothing. + // index.ts owns the fatal decision; this handler owes a log line. wsOrigins.value = null; const ad = new TrekWsAdapter(httpServer); - const server = ad.create(0, { path: '/ws' }) as { emit: (e: string, err: Error) => boolean }; + const server = ad.create(0, { path: '/ws' }) as { emit: (e: string, err: unknown) => boolean }; try { expect(() => server.emit('error', new Error('upstream reset'))).not.toThrow(); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('upstream reset')); + + // A non-Error rejection reads as itself rather than as [object Object]. + logError.mockClear(); + server.emit('error', 'ECONNRESET'); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('ECONNRESET')); } finally { void ad.close(server as never); } diff --git a/server/tests/unit/nest/ws-bind-failure.test.ts b/server/tests/unit/nest/ws-bind-failure.test.ts new file mode 100644 index 000000000..63ab06fc2 --- /dev/null +++ b/server/tests/unit/nest/ws-bind-failure.test.ts @@ -0,0 +1,79 @@ +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((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(); + } + }); +});