diff --git a/middleware/src/channels/routeRegistry.ts b/middleware/src/channels/routeRegistry.ts index d1a9317c..e1f68f7e 100644 --- a/middleware/src/channels/routeRegistry.ts +++ b/middleware/src/channels/routeRegistry.ts @@ -11,25 +11,35 @@ import type { HttpMethod } from '@omadia/channel-sdk'; /** * Per-channel Express route mount. Express itself has no clean way to - * un-register middleware, so we use an indirection: every channel-owned - * route is mounted ONCE at registration time and reads a per-channel - * `active` flag from this registry. Deactivate flips the flag; the handler - * short-circuits with 503 afterwards. Re-activation flips it back. + * un-register middleware, so we use an indirection: every channel-owned route + * is mounted ONCE at first registration and reads its handler from a mutable + * per-route record plus a per-channel `active` flag. Deactivate flips the flag; + * the handler short-circuits with 503 afterwards. Re-activation flips it back, + * and a re-registration by the same channel (hot-reinstall) rebinds the handler + * in place rather than mounting a second, shadowed route. * - * Not the prettiest pattern, but it avoids restarting the HTTP server and - * keeps behaviour observable in logs. When Slice 2.5 adds dynamic load/ - * unload we can swap this out for a mutable `app.use` router-swap variant. + * When a future slice adds dynamic load/unload we can swap this out for a + * mutable `app.use` router-swap variant. */ -interface RegisteredRoute { +interface RouteMount { method: HttpMethod; path: string; channelId: string; handler: RequestHandler; } +interface RouterMount { + prefix: string; + channelId: string; + router: Router; +} + export class ExpressRouteRegistry { - private readonly routes: RegisteredRoute[] = []; + /** Keyed by `${method} ${path}` — the Express dispatch key. */ + private readonly routeMounts = new Map(); + /** Keyed by mount prefix. */ + private readonly routerMounts = new Map(); private readonly activeByChannel = new Map(); constructor(private readonly app: Express) {} @@ -40,22 +50,40 @@ export class ExpressRouteRegistry { path: string, handler: RequestHandler, ): void { + const key = `${method} ${path}`; + const existing = this.routeMounts.get(key); + if (existing) { + if (existing.channelId !== channelId) { + throw new Error( + `route '${key}' already owned by channel '${existing.channelId}'`, + ); + } + // Same channel re-registering (hot-reinstall): rebind in place. + // Every hot-swap path (config reactivate, version upload) deactivates + // then re-activates the channel, which re-runs the plugin's activate() + // and lands here. The mounted wrapper reads `mount.handler` on each + // dispatch, so overwriting it swaps the freshly-loaded module's handler + // in — no second, shadowing app.() mount (#395). + existing.handler = handler; + this.activeByChannel.set(channelId, true); + console.log(`[channels] route rebound ${key} (channel=${channelId})`); + return; + } + + const mount: RouteMount = { method, path, channelId, handler }; const wrapper: RequestHandler = ( req: Request, res: Response, next: NextFunction, ) => { - if (!this.activeByChannel.get(channelId)) { - res.status(503).json({ - code: 'channel.inactive', - message: `channel '${channelId}' is currently deactivated`, - }); + if (!this.activeByChannel.get(mount.channelId)) { + this.sendChannelInactive(res, mount.channelId); return; } - void Promise.resolve(handler(req, res, next)).catch((err: unknown) => { + void Promise.resolve(mount.handler(req, res, next)).catch((err: unknown) => { const message = err instanceof Error ? err.message : String(err); console.error( - `[channel:${channelId}] unhandled route error on ${method} ${path}:`, + `[channel:${mount.channelId}] unhandled route error on ${method} ${path}:`, message, ); if (!res.headersSent) { @@ -85,7 +113,7 @@ export class ExpressRouteRegistry { break; } - this.routes.push({ method, path, channelId, handler }); + this.routeMounts.set(key, mount); this.activeByChannel.set(channelId, true); console.log( `[channels] route registered ${method} ${path} (channel=${channelId})`, @@ -93,23 +121,36 @@ export class ExpressRouteRegistry { } /** - * Register a full Router under a prefix. Same active-flag gate — - * requests under the prefix return 503 while the channel is inactive. + * Register a full Router under a prefix. Same active-flag gate — requests + * under the prefix return 503 while the channel is inactive. Re-registration + * by the same channel rebinds the router in place, mirroring {@link register}. */ registerRouter(channelId: string, prefix: string, router: Router): void { + const existing = this.routerMounts.get(prefix); + if (existing) { + if (existing.channelId !== channelId) { + throw new Error( + `router prefix '${prefix}' already owned by channel '${existing.channelId}'`, + ); + } + existing.router = router; + this.activeByChannel.set(channelId, true); + console.log(`[channels] router rebound at ${prefix} (channel=${channelId})`); + return; + } + + const mount: RouterMount = { prefix, channelId, router }; this.app.use( prefix, (req: Request, res: Response, next: NextFunction) => { - if (!this.activeByChannel.get(channelId)) { - res.status(503).json({ - code: 'channel.inactive', - message: `channel '${channelId}' is currently deactivated`, - }); + if (!this.activeByChannel.get(mount.channelId)) { + this.sendChannelInactive(res, mount.channelId); return; } - router(req, res, next); + mount.router(req, res, next); }, ); + this.routerMounts.set(prefix, mount); this.activeByChannel.set(channelId, true); console.log( `[channels] router registered at ${prefix} (channel=${channelId})`, @@ -121,6 +162,13 @@ export class ExpressRouteRegistry { this.activeByChannel.set(channelId, active); } + private sendChannelInactive(res: Response, channelId: string): void { + res.status(503).json({ + code: 'channel.inactive', + message: `channel '${channelId}' is currently deactivated`, + }); + } + /** Mark every route owned by this channel as inactive (returns 503). */ deactivateChannel(channelId: string): void { this.activeByChannel.set(channelId, false); @@ -129,7 +177,7 @@ export class ExpressRouteRegistry { /** For introspection / dev-tools. */ describe(): Array<{ method: HttpMethod; path: string; channelId: string; active: boolean }> { - return this.routes.map((r) => ({ + return Array.from(this.routeMounts.values()).map((r) => ({ method: r.method, path: r.path, channelId: r.channelId, diff --git a/middleware/test/channelRouteRebind.test.ts b/middleware/test/channelRouteRebind.test.ts new file mode 100644 index 00000000..8e50a171 --- /dev/null +++ b/middleware/test/channelRouteRebind.test.ts @@ -0,0 +1,106 @@ +import { strict as assert } from 'node:assert'; +import type { AddressInfo } from 'node:net'; +import { after, before, describe, it } from 'node:test'; + +import express, { type Express } from 'express'; + +import { ExpressRouteRegistry } from '../src/channels/routeRegistry.js'; + +/** Regression coverage for #395: a plugin hot-reinstall must rebind the + * inbound handler in place, not serve the stale first-mounted route. */ +describe('ExpressRouteRegistry · hot-reinstall handler rebind (#395)', () => { + let app: Express; + let registry: ExpressRouteRegistry; + let baseUrl: string; + let server: ReturnType; + + const CHANNEL = '@test/channel-395'; + const PATH = '/api/test-395/messages'; + + before(async () => { + app = express(); + app.use(express.json()); + registry = new ExpressRouteRegistry(app); + server = app.listen(0); + await new Promise((resolve) => server.once('listening', resolve)); + const { port } = server.address() as AddressInfo; + baseUrl = `http://127.0.0.1:${port}`; + }); + + after(() => { + server.close(); + }); + + const post = async (): Promise<{ status: number; body: unknown }> => { + const res = await fetch(`${baseUrl}${PATH}`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: '{}', + }); + return { status: res.status, body: await res.json() }; + }; + + it('serves the freshly-registered handler after a re-registration', async () => { + registry.register(CHANNEL, 'POST', PATH, (_req, res) => { + res.json({ version: 'v1' }); + }); + registry.setActive(CHANNEL, true); + assert.deepEqual((await post()).body, { version: 'v1' }); + + // Hot reinstall → v2: deactivate, then re-register with the new handler. + registry.deactivateChannel(CHANNEL); + registry.register(CHANNEL, 'POST', PATH, (_req, res) => { + res.json({ version: 'v2' }); + }); + registry.setActive(CHANNEL, true); + + assert.deepEqual( + (await post()).body, + { version: 'v2' }, + 'inbound handler must serve the reinstalled module, not the stale one', + ); + }); + + it('does not stack a second Express route on re-registration', async () => { + const mountsFor = () => + registry.describe().filter((r) => r.path === PATH && r.method === 'POST'); + assert.equal(mountsFor().length, 1, 'exactly one mount before re-register'); + registry.register(CHANNEL, 'POST', PATH, (_req, res) => { + res.json({ version: 'v3' }); + }); + assert.equal(mountsFor().length, 1, 'still one mount after re-register'); + assert.deepEqual((await post()).body, { version: 'v3' }); + }); + + it('rejects a different channel claiming an already-owned path/prefix', () => { + assert.throws( + () => + registry.register('@test/other-channel', 'POST', PATH, (_req, res) => { + res.json({ version: 'intruder' }); + }), + /already owned by channel '@test\/channel-395'/, + ); + + const PREFIX = '/api/test-395-router'; + registry.registerRouter(CHANNEL, PREFIX, express.Router()); + assert.throws( + () => registry.registerRouter('@test/other-channel', PREFIX, express.Router()), + /already owned by channel '@test\/channel-395'/, + ); + // Same channel re-registering (hot-reinstall) must not throw. + assert.doesNotThrow(() => + registry.registerRouter(CHANNEL, PREFIX, express.Router()), + ); + }); + + it('gates a deactivated channel with 503 until re-activation', async () => { + registry.deactivateChannel(CHANNEL); + const down = await post(); + assert.equal(down.status, 503); + assert.equal((down.body as { code?: string }).code, 'channel.inactive'); + + registry.setActive(CHANNEL, true); + const up = await post(); + assert.equal(up.status, 200); + }); +});