Compare commits
3
Commits
4a34a8203c
...
2983789779
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
2983789779 | ||
|
|
490b85b07c | ||
|
|
800ecfdb92 |
@@ -163,6 +163,32 @@ database or cryptographic guarantee. Phase 5D is still not complete** — passes
|
||||
migration correctness) and 5 (deployment/supply chain plus a final independent re-audit) remain
|
||||
outstanding, and Phase 6 remains blocked.
|
||||
|
||||
Phase 5D pass 4 of 5 hardens the module runtime and the migration commands. **Two routes of one
|
||||
module that could both match the same request path are now rejected at registration**
|
||||
(`MODULE_ROUTE_OVERLAP`) instead of being resolved by declaration order — with different permissions
|
||||
on `/events/:id` and `/events/new`, registration order alone previously decided which permission
|
||||
guarded `/events/new`. Decoded route parameters are documented as untrusted, module-owned input that
|
||||
handlers must validate against their own domain grammar; no generic validator or route field was
|
||||
added. A module whose `start` hook fails is now rolled back through **its own** `stop` hook first,
|
||||
so partially acquired resources are released, and every stop/rollback hook runs under a fixed
|
||||
10-second deadline while HTTP shutdown drains for at most 30 seconds before force-closing
|
||||
connections through Node's supported public APIs — shutdown is finite at every stage, reports a
|
||||
forced drain or a hook failure as an unsuccessful shutdown, and always still reaches pool closure.
|
||||
`migrations:plan`/`migrations:apply` now connect with fixed 10-second connection, query, and
|
||||
`statement_timeout` defaults that a `DATABASE_URL` cannot weaken (the pinned driver lets a
|
||||
connection string override explicit options — verified), and each migration's SQL is sent to
|
||||
PostgreSQL **intact, as one parsed batch** inside the existing transaction rather than split on the
|
||||
literal `--> statement-breakpoint` bytes, which corrupted any migration containing those bytes in a
|
||||
string literal, dollar-quoted body, quoted identifier, or comment. See
|
||||
[Module development](docs/module-development/README.md#http-routes),
|
||||
[API](docs/development/api.md#graceful-shutdown), and
|
||||
[ADR 0020](docs/architecture/decisions/0020-module-runtime-and-migration-execution-hardening.md).
|
||||
**Deadlines bound how long the platform waits; they do not cancel or sandbox trusted in-process
|
||||
module code, and closing a socket does not prove a handler promise ended. No custom SQL parser
|
||||
exists. Modules remain trusted compiled backend code with no process, role, schema, or tenant
|
||||
isolation. Phase 5D is still not complete** — pass 5 (deployment/supply chain plus an independent
|
||||
whole-repository re-audit) remains outstanding, and Phase 6 remains blocked.
|
||||
|
||||
## Quick start
|
||||
|
||||
Prerequisites: Node.js 24 LTS, pnpm 11.12, and Docker with Compose.
|
||||
|
||||
+110
-6
@@ -130,6 +130,13 @@ export interface StartApiOptions {
|
||||
* test can prove real timeout behavior deterministically without waiting out the production
|
||||
* values. Never set by the executable entry point. */
|
||||
readonly nodeServerTimeoutOverride?: Partial<NodeServerTimeouts>;
|
||||
/** Test-only escape hatch: narrows the graceful HTTP drain deadline
|
||||
* (`HTTP_DRAIN_TIMEOUT_MS`) so a forced close can be proven in milliseconds. Never set by the
|
||||
* executable entry point. */
|
||||
readonly drainTimeoutMsOverride?: number;
|
||||
/** Test-only escape hatch: narrows the per-module stop/rollback hook deadline
|
||||
* (`MODULE_STOP_HOOK_TIMEOUT_MS`). Never set by the executable entry point. */
|
||||
readonly moduleHookTimeoutMsOverride?: number;
|
||||
}
|
||||
|
||||
async function safeCloseDatabase(handle: ApiDatabaseHandle): Promise<void> {
|
||||
@@ -152,6 +159,94 @@ async function safeStopModuleRuntime(moduleRuntime: StartedModuleRuntime): Promi
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The platform's fixed graceful-drain deadline. In-flight HTTP work has this long to finish after
|
||||
* the listener stops accepting new connections; after it, connections are closed so process
|
||||
* shutdown stays bounded. Not configurable in this slice.
|
||||
*/
|
||||
export const HTTP_DRAIN_TIMEOUT_MS = 30_000;
|
||||
|
||||
/**
|
||||
* A shutdown that did not complete cleanly. Carries only fixed booleans and the already-sanitized
|
||||
* module-runtime error — never a request, socket, URL, credential, SQL, or stack detail — so an
|
||||
* operator can see *which* stages failed without any of it becoming a disclosure channel.
|
||||
*/
|
||||
export class ApiShutdownError extends Error {
|
||||
readonly drainTimedOut: boolean;
|
||||
readonly moduleStopFailed: boolean;
|
||||
readonly moduleStopError: unknown;
|
||||
|
||||
constructor(drainTimedOut: boolean, moduleStopError: unknown) {
|
||||
super("The API did not shut down cleanly.");
|
||||
this.name = "ApiShutdownError";
|
||||
this.drainTimedOut = drainTimedOut;
|
||||
this.moduleStopFailed = moduleStopError !== undefined;
|
||||
this.moduleStopError = moduleStopError;
|
||||
this.stack = `${this.name}: ${this.message}`;
|
||||
Object.freeze(this);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Stops accepting new connections, then waits up to `timeoutMs` for existing HTTP work to finish.
|
||||
*
|
||||
* At the deadline it closes idle and then still-active connections using Node's own supported
|
||||
* public `Server` APIs (`closeIdleConnections`/`closeAllConnections`, both called only after
|
||||
* `server.close()`), and stops waiting for the close callback. No private socket or server field is
|
||||
* touched.
|
||||
*
|
||||
* Force-closing a connection ends the transport. It does **not** prove that arbitrary in-process
|
||||
* handler JavaScript was canceled — a handler promise may still be running and may still settle.
|
||||
* What is guaranteed is that shutdown no longer waits for it, and that the pool closes afterwards,
|
||||
* so its late database work fails.
|
||||
*/
|
||||
/** `ServerType` is a union that also covers HTTP/2 servers, which do not expose these two methods.
|
||||
* `serve()` here always creates a plain `http.Server`, which does; narrowing with a real capability
|
||||
* check keeps that honest instead of asserting a cast. */
|
||||
interface ConnectionClosingServer {
|
||||
closeAllConnections(): void;
|
||||
closeIdleConnections(): void;
|
||||
}
|
||||
|
||||
function supportsConnectionClosing(
|
||||
server: ServerType,
|
||||
): server is ServerType & ConnectionClosingServer {
|
||||
const candidate = server as Partial<ConnectionClosingServer>;
|
||||
return (
|
||||
typeof candidate.closeIdleConnections === "function" &&
|
||||
typeof candidate.closeAllConnections === "function"
|
||||
);
|
||||
}
|
||||
|
||||
async function drainHttpServer(
|
||||
server: ServerType,
|
||||
timeoutMs: number,
|
||||
): Promise<"drained" | "forced"> {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
const closed = new Promise<void>((resolve) => {
|
||||
server.close(() => resolve());
|
||||
});
|
||||
try {
|
||||
const outcome = await Promise.race([
|
||||
closed.then(() => "drained" as const),
|
||||
new Promise<"forced">((resolve) => {
|
||||
timer = setTimeout(() => resolve("forced"), timeoutMs);
|
||||
}),
|
||||
]);
|
||||
if (outcome === "drained") {
|
||||
return "drained";
|
||||
}
|
||||
if (supportsConnectionClosing(server)) {
|
||||
server.closeIdleConnections();
|
||||
server.closeAllConnections();
|
||||
}
|
||||
return "forced";
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
// The close callback may still fire later; nothing awaits it, and it carries no rejection.
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Binds the Node HTTP server. Two platform-owned transport concerns are applied at this exact
|
||||
* boundary, below Hono entirely: `wrapFetchWithClientAddress` resolves and sanitizes the caller's
|
||||
@@ -292,7 +387,11 @@ export async function startApi(
|
||||
// handle to stop here, only the pool this failure leaves open.
|
||||
let moduleRuntime: StartedModuleRuntime;
|
||||
try {
|
||||
moduleRuntime = await startDiscoveredModuleRuntime(inventory.modules, services);
|
||||
moduleRuntime = await startDiscoveredModuleRuntime(inventory.modules, services, {
|
||||
...(options.moduleHookTimeoutMsOverride === undefined
|
||||
? {}
|
||||
: { hookTimeoutMs: options.moduleHookTimeoutMsOverride }),
|
||||
});
|
||||
} catch (error) {
|
||||
await safeCloseDatabase(databaseHandle);
|
||||
throw error;
|
||||
@@ -345,13 +444,12 @@ export async function startApi(
|
||||
// code can run against it afterward. Unlike the startup-cleanup paths above, a module-stop
|
||||
// failure here is not swallowed — it is a genuine shutdown result a caller should observe — but
|
||||
// database cleanup still always runs first, so a stop failure never leaves the pool open.
|
||||
const drainTimeoutMs = options.drainTimeoutMsOverride ?? HTTP_DRAIN_TIMEOUT_MS;
|
||||
let shutdownPromise: Promise<void> | undefined;
|
||||
const shutdown = (): Promise<void> => {
|
||||
if (!shutdownPromise) {
|
||||
shutdownPromise = (async () => {
|
||||
await new Promise<void>((resolve) => {
|
||||
server.close(() => resolve());
|
||||
});
|
||||
const drainOutcome = await drainHttpServer(server, drainTimeoutMs);
|
||||
let moduleStopError: unknown;
|
||||
try {
|
||||
await moduleRuntime.stop();
|
||||
@@ -359,8 +457,14 @@ export async function startApi(
|
||||
moduleStopError = error;
|
||||
}
|
||||
await safeCloseDatabase(databaseHandle);
|
||||
if (moduleStopError) {
|
||||
throw moduleStopError;
|
||||
// Every stage above ran regardless of what the earlier ones did, so a drain timeout or a
|
||||
// module-stop failure can never leave the pool open. Both failures are reported together,
|
||||
// as fixed metadata: a forced drain is a genuine shutdown failure, not a success.
|
||||
if (drainOutcome === "forced" || moduleStopError) {
|
||||
throw new ApiShutdownError(
|
||||
drainOutcome === "forced",
|
||||
moduleStopError === undefined ? undefined : moduleStopError,
|
||||
);
|
||||
}
|
||||
})();
|
||||
}
|
||||
|
||||
@@ -336,6 +336,8 @@ function apiOrigin(running: RunningApi): string {
|
||||
|
||||
interface StartConfiguredApiOptions {
|
||||
readonly clientAddress?: ClientAddressConfigInput;
|
||||
/** Test-only narrowing of the platform's 30-second graceful HTTP drain deadline. */
|
||||
readonly drainTimeoutMsOverride?: number;
|
||||
readonly modulesDirectoryOverride?: string;
|
||||
readonly nodeServerTimeoutOverride?: Partial<NodeServerTimeouts>;
|
||||
/** Defaults to `true`: almost every existing scenario in this suite signs accounts up over HTTP
|
||||
@@ -391,6 +393,9 @@ async function startConfiguredApi(
|
||||
...(options.nodeServerTimeoutOverride === undefined
|
||||
? {}
|
||||
: { nodeServerTimeoutOverride: options.nodeServerTimeoutOverride }),
|
||||
...(options.drainTimeoutMsOverride === undefined
|
||||
? {}
|
||||
: { drainTimeoutMsOverride: options.drainTimeoutMsOverride }),
|
||||
});
|
||||
}
|
||||
|
||||
@@ -2725,3 +2730,114 @@ describe.sequential("live API — Node header/request timeouts and client errors
|
||||
expect(logs.join("\n")).not.toContain("zzz");
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Pass 4: shutdown is finite even when HTTP work does not settle.
|
||||
*
|
||||
* These drive a real bound listener and a real socket rather than a mock, because the behavior
|
||||
* under test is what Node's own `server.close()` does and does not do. Every socket, server, and
|
||||
* disposable database is closed in `finally`, and each scenario carries its own outer deadline so
|
||||
* a regression fails rather than hangs.
|
||||
*/
|
||||
describe.sequential("live API — bounded HTTP drain and shutdown", () => {
|
||||
async function withDeadline<T>(operation: Promise<T>, timeoutMs: number): Promise<T> {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
return await Promise.race([
|
||||
operation,
|
||||
new Promise<never>((_resolve, reject) => {
|
||||
timer = setTimeout(
|
||||
() => reject(harnessError("shutdown did not settle in time")),
|
||||
timeoutMs,
|
||||
);
|
||||
}),
|
||||
]);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
}
|
||||
|
||||
it("serves a real request and then drains cleanly, well inside the deadline", async () => {
|
||||
const database = await createDisposableDatabase("drain_ok");
|
||||
await applyCoreMigrations(database.url);
|
||||
const running = await startConfiguredApi(database.url, { drainTimeoutMsOverride: 5_000 });
|
||||
try {
|
||||
const origin = apiOrigin(running);
|
||||
const response = await fetch(`${origin}/api/v1/principal`);
|
||||
expect(response.status).toBe(200);
|
||||
expect(await response.json()).toEqual({ kind: "anonymous" });
|
||||
|
||||
// Nothing is stalled, so the graceful path completes and shutdown reports success — the
|
||||
// drain deadline is never reached and no connection is force-closed.
|
||||
await expect(withDeadline(running.shutdown(), 20_000)).resolves.toBeUndefined();
|
||||
|
||||
// The listener really is closed afterwards.
|
||||
await expect(fetch(`${origin}/api/v1/principal`)).rejects.toBeTruthy();
|
||||
} finally {
|
||||
await running.shutdown().catch(() => undefined);
|
||||
await dropDatabase(database.name);
|
||||
}
|
||||
});
|
||||
|
||||
it("force-closes a stalled connection at the drain deadline and still reaches pool closure", async () => {
|
||||
const database = await createDisposableDatabase("drain_forced");
|
||||
await applyCoreMigrations(database.url);
|
||||
// A short, test-only drain deadline; production keeps HTTP_DRAIN_TIMEOUT_MS unchanged.
|
||||
const running = await startConfiguredApi(database.url, { drainTimeoutMsOverride: 250 });
|
||||
const address = running.server.address();
|
||||
if (!address || typeof address !== "object") {
|
||||
throw harnessError("The API integration server has no bound address.");
|
||||
}
|
||||
const socket = net.connect({ host: "127.0.0.1", port: address.port });
|
||||
try {
|
||||
// A real, deliberately incomplete request: headers are never finished, so Node keeps the
|
||||
// connection open and `server.close()` alone would wait indefinitely.
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
socket.once("error", reject);
|
||||
socket.on("data", () => undefined);
|
||||
socket.write("GET /api/v1/principal HTTP/1.1\r\nHost: 127.0.0.1\r\n", () => resolve());
|
||||
});
|
||||
|
||||
const closed = new Promise<void>((resolve) => {
|
||||
socket.once("close", () => resolve());
|
||||
});
|
||||
|
||||
let captured: unknown;
|
||||
try {
|
||||
await withDeadline(running.shutdown(), 20_000);
|
||||
} catch (error) {
|
||||
captured = error;
|
||||
}
|
||||
|
||||
// A forced drain is a genuine shutdown failure, never reported as success.
|
||||
expect(captured).toMatchObject({ drainTimedOut: true, name: "ApiShutdownError" });
|
||||
// ...and it still carries nothing about the request, socket, or database.
|
||||
const publicText = `${String(captured)}\n${(captured as Error).stack ?? ""}`;
|
||||
expect(publicText).not.toContain("127.0.0.1");
|
||||
expect(publicText).not.toContain(database.name);
|
||||
// The connection was actually closed by the platform, not left dangling.
|
||||
await withDeadline(closed, 10_000);
|
||||
} finally {
|
||||
socket.destroy();
|
||||
await running.shutdown().catch(() => undefined);
|
||||
await dropDatabase(database.name);
|
||||
}
|
||||
});
|
||||
|
||||
it("shares one memoized shutdown across concurrent callers", async () => {
|
||||
const database = await createDisposableDatabase("drain_once");
|
||||
await applyCoreMigrations(database.url);
|
||||
const running = await startConfiguredApi(database.url, { drainTimeoutMsOverride: 5_000 });
|
||||
try {
|
||||
const [first, second, third] = await withDeadline(
|
||||
Promise.all([running.shutdown(), running.shutdown(), running.shutdown()]),
|
||||
20_000,
|
||||
);
|
||||
expect([first, second, third]).toEqual([undefined, undefined, undefined]);
|
||||
// A later caller observes the same completed result rather than restarting cleanup.
|
||||
await expect(running.shutdown()).resolves.toBeUndefined();
|
||||
} finally {
|
||||
await dropDatabase(database.name);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1067,11 +1067,14 @@ describe("Ekklesia API module runtime startup and shutdown", () => {
|
||||
issues: [expect.objectContaining({ code: "MODULE_START_FAILED" })],
|
||||
});
|
||||
|
||||
// The module whose start hook failed is rolled back first through its own stop hook, then the
|
||||
// module that had already started, in reverse order.
|
||||
expect(recorder.events).toEqual([
|
||||
"one:register",
|
||||
"two:register",
|
||||
"one:start",
|
||||
"two:start",
|
||||
"two:stop",
|
||||
"one:stop",
|
||||
]);
|
||||
expect(poolConstructor).toHaveBeenCalledTimes(1);
|
||||
@@ -1177,8 +1180,14 @@ describe("Ekklesia API module runtime startup and shutdown", () => {
|
||||
|
||||
const running = await startApi(validConfig(), { modulesDirectoryOverride: modulesDirectory });
|
||||
|
||||
// Shutdown now aggregates which stages failed; the module-runtime error is carried intact.
|
||||
await expect(running.shutdown()).rejects.toMatchObject({
|
||||
issues: [expect.objectContaining({ code: "MODULE_STOP_FAILED", moduleId: "stop-fails" })],
|
||||
drainTimedOut: false,
|
||||
moduleStopError: {
|
||||
issues: [expect.objectContaining({ code: "MODULE_STOP_FAILED", moduleId: "stop-fails" })],
|
||||
},
|
||||
moduleStopFailed: true,
|
||||
name: "ApiShutdownError",
|
||||
});
|
||||
expect(poolEnd).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
@@ -450,8 +450,15 @@ describe.sequential("live API — module database explicit migration ownership",
|
||||
await applyCoreMigrations(database.url);
|
||||
resetRecorder();
|
||||
|
||||
// The module whose `start` failed is now rolled back through its own `stop` hook first. That
|
||||
// hook also writes to the table its missing migration would have created, so it fails too —
|
||||
// which is exactly the documented reason a `stop` hook must tolerate partial initialization.
|
||||
// Both outcomes are reported, in order, and neither is swallowed.
|
||||
await expect(startMigratedApi(database.url, fixtureModulesDirectory)).rejects.toMatchObject({
|
||||
issues: [expect.objectContaining({ code: "MODULE_START_FAILED", moduleId: "db-facade" })],
|
||||
issues: [
|
||||
expect.objectContaining({ code: "MODULE_START_FAILED", moduleId: "db-facade" }),
|
||||
expect.objectContaining({ code: "MODULE_ROLLBACK_FAILED", moduleId: "db-facade" }),
|
||||
],
|
||||
});
|
||||
|
||||
// No schema inference, no automatic table creation as a side effect of the failed attempt.
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
type ModuleRouteHandler,
|
||||
type ModuleRouteHandlerContext,
|
||||
type ModuleRuntimeServices,
|
||||
moduleRoutesOverlap,
|
||||
parsePermissionId,
|
||||
type PermissionId,
|
||||
type RegisteredModuleHttpRoute,
|
||||
@@ -1454,3 +1455,153 @@ describe("module routes and the rest of the API", () => {
|
||||
expect(response.status).toBe(200);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Pass 4: the decoded-parameter boundary and the routing ambiguity that made it dangerous.
|
||||
*
|
||||
* These drive the pinned Hono matcher with real requests rather than asserting what a mock returns,
|
||||
* because the whole point is what the real router actually hands a module handler.
|
||||
*/
|
||||
describe("module route parameters are decoded, untrusted input", () => {
|
||||
function paramsApp() {
|
||||
const seen: Array<Readonly<Record<string, string>>> = [];
|
||||
const handler = vi.fn<ModuleRouteHandler>(async (_request, context) => {
|
||||
seen.push(context.params);
|
||||
return Response.json({ id: context.params.id });
|
||||
});
|
||||
const { app } = buildModuleApp({
|
||||
registry: permissionRegistry([{ id: readPermission, baseline: "public" }]),
|
||||
routes: [
|
||||
moduleRoute({
|
||||
handler,
|
||||
method: "GET",
|
||||
moduleId: "calendar",
|
||||
path: "/items/:id",
|
||||
permission: readPermission,
|
||||
}),
|
||||
],
|
||||
});
|
||||
return { app, handler, seen };
|
||||
}
|
||||
|
||||
it.each([
|
||||
// A single encoded segment can carry characters the declaration grammar forbids outright.
|
||||
["an encoded path separator and dot segments", "%2F..%2Fadmin", "/../admin"],
|
||||
["an encoded backslash", "a%5Cb", "a\\b"],
|
||||
["an embedded NUL", "a%00b", "a\u0000b"],
|
||||
["encoded CR/LF", "a%0D%0Ab", "a\r\nb"],
|
||||
["a percent sign", "100%25", "100%"],
|
||||
["non-ASCII text", "%C3%A9v%C3%A9nement", "événement"],
|
||||
["a statement separator", "1%3B%20drop", "1; drop"],
|
||||
])("delivers %s decoded, exactly as the router produced it", async (_label, encoded, decoded) => {
|
||||
const { app, seen } = paramsApp();
|
||||
|
||||
const response = await app.request(`/api/v1/modules/calendar/items/${encoded}`);
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(await response.json()).toEqual({ id: decoded });
|
||||
// The declared path grammar (`/items/:id`) permits none of these characters — validating the
|
||||
// *declaration* says nothing about what a request may send, which is exactly why a handler
|
||||
// must validate the decoded value against its own domain grammar before using it.
|
||||
expect(seen[0]?.id).toBe(decoded);
|
||||
expect(Object.isFrozen(seen[0])).toBe(true);
|
||||
});
|
||||
|
||||
it("keeps parameter delivery behind authorization", async () => {
|
||||
const handler = vi.fn<ModuleRouteHandler>(async () => new Response("never"));
|
||||
const { app, evaluator } = buildModuleApp({
|
||||
evaluator: { authorize: vi.fn().mockResolvedValue({ allowed: false }) },
|
||||
registry: permissionRegistry([{ id: readPermission, baseline: "restricted" }]),
|
||||
routes: [
|
||||
moduleRoute({
|
||||
handler,
|
||||
method: "GET",
|
||||
moduleId: "calendar",
|
||||
path: "/items/:id",
|
||||
permission: readPermission,
|
||||
}),
|
||||
],
|
||||
});
|
||||
|
||||
const response = await app.request("/api/v1/modules/calendar/items/%2F..%2Fadmin");
|
||||
|
||||
expect(response.status).toBe(403);
|
||||
expect(evaluator.authorize).toHaveBeenCalled();
|
||||
// An unauthorized caller can never observe how the handler would treat a parameter, because
|
||||
// the handler never runs.
|
||||
expect(handler).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("lets a handler reject a decoded value against its own domain grammar", async () => {
|
||||
// The platform supplies no generic parameter validator; this is the module's own check, which
|
||||
// is exactly the documented contract.
|
||||
const canonicalId = /^[0-9a-f]{8}$/u;
|
||||
const handler = vi.fn<ModuleRouteHandler>(async (_request, context) => {
|
||||
const id = context.params.id;
|
||||
if (id === undefined || !canonicalId.test(id)) {
|
||||
return Response.json({ code: "INVALID_REQUEST" }, { status: 400 });
|
||||
}
|
||||
return Response.json({ id });
|
||||
});
|
||||
const { app } = buildModuleApp({
|
||||
registry: permissionRegistry([{ id: readPermission, baseline: "public" }]),
|
||||
routes: [
|
||||
moduleRoute({
|
||||
handler,
|
||||
method: "GET",
|
||||
moduleId: "calendar",
|
||||
path: "/items/:id",
|
||||
permission: readPermission,
|
||||
}),
|
||||
],
|
||||
});
|
||||
|
||||
expect((await app.request("/api/v1/modules/calendar/items/deadbeef")).status).toBe(200);
|
||||
const rejected = await app.request("/api/v1/modules/calendar/items/%2F..%2Fadmin");
|
||||
expect(rejected.status).toBe(400);
|
||||
expect(await rejected.json()).toEqual({ code: "INVALID_REQUEST" });
|
||||
});
|
||||
});
|
||||
|
||||
describe("module route overlap can never reach mounting", () => {
|
||||
it("would otherwise let registration order choose the governing permission", async () => {
|
||||
// Both orders are mounted directly here — bypassing the runtime rejection — to show precisely
|
||||
// what `MODULE_ROUTE_OVERLAP` prevents: the same request reaching a different handler *and* a
|
||||
// different permission depending only on the order the routes were registered in.
|
||||
const createPermission = parsePermissionId("calendar.events.create");
|
||||
const byId = vi.fn<ModuleRouteHandler>(async () => Response.json({ handler: "byId" }));
|
||||
const create = vi.fn<ModuleRouteHandler>(async () => Response.json({ handler: "new" }));
|
||||
const registry = permissionRegistry([
|
||||
{ id: readPermission, baseline: "public" },
|
||||
{ id: createPermission, baseline: "public" },
|
||||
]);
|
||||
const paramRoute = moduleRoute({
|
||||
handler: byId,
|
||||
method: "GET",
|
||||
moduleId: "calendar",
|
||||
path: "/events/:id",
|
||||
permission: readPermission,
|
||||
});
|
||||
const staticRoute = moduleRoute({
|
||||
handler: create,
|
||||
method: "GET",
|
||||
moduleId: "calendar",
|
||||
path: "/events/new",
|
||||
permission: createPermission,
|
||||
});
|
||||
|
||||
const parameterFirst = buildModuleApp({ registry, routes: [paramRoute, staticRoute] });
|
||||
const staticFirst = buildModuleApp({ registry, routes: [staticRoute, paramRoute] });
|
||||
|
||||
expect(
|
||||
await (await parameterFirst.app.request("/api/v1/modules/calendar/events/new")).json(),
|
||||
).toEqual({ handler: "byId" });
|
||||
expect(
|
||||
await (await staticFirst.app.request("/api/v1/modules/calendar/events/new")).json(),
|
||||
).toEqual({ handler: "new" });
|
||||
|
||||
// Core rejects this pair during staged registration, so no such snapshot can ever be published
|
||||
// to `mountModuleHttpRoutes` — see `core/tests/module-runtime.test.ts`.
|
||||
expect(moduleRoutesOverlap(paramRoute, staticRoute)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -77,8 +77,72 @@ export class PostgresMigrationDatabaseError extends Error {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The platform's fixed migration connection deadline, applied when a caller does not narrow it.
|
||||
* Lives here, at the one construction boundary every migration command shares, so a future CLI
|
||||
* call site cannot silently omit it.
|
||||
*/
|
||||
export const MIGRATION_CONNECTION_TIMEOUT_MS = 10_000;
|
||||
|
||||
/**
|
||||
* The platform's fixed migration operation deadline: both `pg`'s client-side `query_timeout` and
|
||||
* PostgreSQL's own server-side `statement_timeout`. The client-side value bounds a query whose
|
||||
* response never arrives; the server-side value bounds work the server is actually doing. Applied
|
||||
* to every statement the session issues — history reads, advisory-lock queries, transaction
|
||||
* control, migration SQL, and history writes.
|
||||
*/
|
||||
export const MIGRATION_OPERATION_TIMEOUT_MS = 10_000;
|
||||
|
||||
/**
|
||||
* Connection-string parameters the pinned `[email protected]` would let silently override the timeouts
|
||||
* selected above. Its `ConnectionParameters` merges as
|
||||
* `Object.assign({}, config, parse(config.connectionString))`, so the **URL wins** over explicitly
|
||||
* passed options — verified directly against the pinned driver, where an explicit
|
||||
* `statement_timeout: 10000` alongside `?statement_timeout=0` produced a server-reported
|
||||
* `statement_timeout` of `0`. Rejecting these is therefore not stylistic: without it the policy
|
||||
* above is advisory.
|
||||
*
|
||||
* Deliberately narrow — only settings that actually conflict with the timeouts this boundary
|
||||
* selects. `options` is included solely because it can carry `-c statement_timeout=...`, and is
|
||||
* rejected only when it does.
|
||||
*/
|
||||
const CONFLICTING_TIMEOUT_PARAMETERS = Object.freeze([
|
||||
"connect_timeout",
|
||||
"query_timeout",
|
||||
"statement_timeout",
|
||||
]);
|
||||
|
||||
function assertNoConflictingTimeoutParameters(connectionString: string): void {
|
||||
let search: URLSearchParams;
|
||||
try {
|
||||
search = new URL(connectionString).searchParams;
|
||||
} catch {
|
||||
throw databaseError(
|
||||
"POSTGRES_MIGRATION_CONFIGURATION_INVALID",
|
||||
"The PostgreSQL database URL is invalid.",
|
||||
);
|
||||
}
|
||||
for (const [rawKey, rawValue] of search.entries()) {
|
||||
const key = rawKey.toLowerCase();
|
||||
if (CONFLICTING_TIMEOUT_PARAMETERS.includes(key)) {
|
||||
throw databaseError(
|
||||
"POSTGRES_MIGRATION_CONFIGURATION_INVALID",
|
||||
"The PostgreSQL database URL must not set a connection or statement timeout.",
|
||||
);
|
||||
}
|
||||
if (key === "options" && rawValue.toLowerCase().includes("statement_timeout")) {
|
||||
throw databaseError(
|
||||
"POSTGRES_MIGRATION_CONFIGURATION_INVALID",
|
||||
"The PostgreSQL database URL must not set a connection or statement timeout.",
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export interface PostgresMigrationDatabaseOptions {
|
||||
/** Omitted means `MIGRATION_CONNECTION_TIMEOUT_MS`. Tests may narrow it; nothing disables it. */
|
||||
readonly connectionTimeoutMs?: number;
|
||||
/** Omitted means `MIGRATION_OPERATION_TIMEOUT_MS`. Tests may narrow it; nothing disables it. */
|
||||
readonly operationTimeoutMs?: number;
|
||||
/** The validated transport policy (`core/src/database/transport.js`). Every production executable
|
||||
* that plans or applies migrations passes one; omitting it keeps the pre-existing plaintext
|
||||
@@ -177,14 +241,17 @@ function createClientConfig(
|
||||
throw invalidConfiguration("The PostgreSQL migration database options are invalid.");
|
||||
}
|
||||
|
||||
const validatedConnectionTimeoutMs = validateTimeout(
|
||||
connectionTimeoutMs,
|
||||
"The PostgreSQL connection timeout is invalid.",
|
||||
);
|
||||
const validatedOperationTimeoutMs = validateTimeout(
|
||||
operationTimeoutMs,
|
||||
"The PostgreSQL operation timeout is invalid.",
|
||||
);
|
||||
// Defaults are applied here, at the shared construction boundary, so every migration command
|
||||
// inherits a finite deadline without having to remember to ask for one.
|
||||
const validatedConnectionTimeoutMs =
|
||||
validateTimeout(connectionTimeoutMs, "The PostgreSQL connection timeout is invalid.") ??
|
||||
MIGRATION_CONNECTION_TIMEOUT_MS;
|
||||
const validatedOperationTimeoutMs =
|
||||
validateTimeout(operationTimeoutMs, "The PostgreSQL operation timeout is invalid.") ??
|
||||
MIGRATION_OPERATION_TIMEOUT_MS;
|
||||
|
||||
// The URL must not be able to weaken what was just selected.
|
||||
assertNoConflictingTimeoutParameters(connectionString);
|
||||
|
||||
// Also rejects a URL carrying its own TLS parameters, so `pg`'s connection-string precedence
|
||||
// can never silently replace this policy with a weaker one.
|
||||
@@ -195,16 +262,10 @@ function createClientConfig(
|
||||
|
||||
return {
|
||||
connectionString,
|
||||
connectionTimeoutMillis: validatedConnectionTimeoutMs,
|
||||
query_timeout: validatedOperationTimeoutMs,
|
||||
...(ssl === undefined ? {} : { ssl }),
|
||||
...(validatedConnectionTimeoutMs === undefined
|
||||
? {}
|
||||
: { connectionTimeoutMillis: validatedConnectionTimeoutMs }),
|
||||
...(validatedOperationTimeoutMs === undefined
|
||||
? {}
|
||||
: {
|
||||
query_timeout: validatedOperationTimeoutMs,
|
||||
statement_timeout: validatedOperationTimeoutMs,
|
||||
}),
|
||||
statement_timeout: validatedOperationTimeoutMs,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -402,7 +463,7 @@ class PostgresMigrationDatabaseSession implements MigrationDatabaseSession {
|
||||
|
||||
const transaction = Object.freeze<MigrationTransaction>({
|
||||
ensureHistoryTable: async () => this.#ensureHistoryTable(),
|
||||
executeStatement: async (sql) => this.#executeStatement(sql),
|
||||
executeMigrationSql: async (sql) => this.#executeMigrationSql(sql),
|
||||
insertAppliedMigration: async (migration) => this.#insertAppliedMigration(migration),
|
||||
});
|
||||
|
||||
@@ -479,15 +540,17 @@ class PostgresMigrationDatabaseSession implements MigrationDatabaseSession {
|
||||
}
|
||||
}
|
||||
|
||||
async #executeStatement(sql: string): Promise<void> {
|
||||
/**
|
||||
* Sends one migration's complete SQL to PostgreSQL in a single simple-query call — no values, no
|
||||
* prepared-statement name, no rewriting — so the server performs all SQL parsing and the packaged
|
||||
* bytes are what actually execute.
|
||||
*/
|
||||
async #executeMigrationSql(sql: string): Promise<void> {
|
||||
this.#assertTransactionActive("POSTGRES_MIGRATION_EXECUTION_FAILED");
|
||||
try {
|
||||
await this.#client.query(sql);
|
||||
} catch {
|
||||
throw databaseError(
|
||||
"POSTGRES_MIGRATION_EXECUTION_FAILED",
|
||||
"A module migration statement failed.",
|
||||
);
|
||||
throw databaseError("POSTGRES_MIGRATION_EXECUTION_FAILED", "A module migration failed.");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -4,7 +4,6 @@ import type {
|
||||
} from "../modules/migration-discovery.js";
|
||||
|
||||
const DEFAULT_MIGRATION_LOCK_TIMEOUT_MS = 10_000;
|
||||
const DRIZZLE_STATEMENT_BREAKPOINT = "--> statement-breakpoint";
|
||||
|
||||
export type MigrationRunnerErrorCode =
|
||||
| "MIGRATION_APPROVAL_LOOKUP_FAILED"
|
||||
@@ -40,7 +39,12 @@ export interface MigrationHistoryInsert {
|
||||
|
||||
export interface MigrationTransaction {
|
||||
ensureHistoryTable(): Promise<void>;
|
||||
executeStatement(sql: string): Promise<void>;
|
||||
/**
|
||||
* Executes one migration's **complete** SQL exactly as packaged, in a single call, inside the
|
||||
* caller's transaction. Never a fragment: the runner does not split migration SQL, so the
|
||||
* implementation must pass the string through to PostgreSQL unmodified.
|
||||
*/
|
||||
executeMigrationSql(sql: string): Promise<void>;
|
||||
insertAppliedMigration(migration: MigrationHistoryInsert): Promise<void>;
|
||||
}
|
||||
|
||||
@@ -484,9 +488,14 @@ export async function applyModuleMigrations(
|
||||
await transaction.ensureHistoryTable();
|
||||
for (const migration of pending) {
|
||||
executingIdentity = migration.identity;
|
||||
for (const statement of migration.sql.split(DRIZZLE_STATEMENT_BREAKPOINT)) {
|
||||
await transaction.executeStatement(statement);
|
||||
}
|
||||
// PostgreSQL parses the batch, not this runner. Drizzle's `--> statement-breakpoint`
|
||||
// lines are ordinary `--` comments to the server, so sending the migration whole keeps
|
||||
// the marker's bytes meaningful wherever they legitimately appear — inside a string
|
||||
// literal, a dollar-quoted function body, a quoted identifier, or a comment — while a
|
||||
// literal `split` on those bytes tore such a migration into fragments that were no
|
||||
// longer valid SQL. Deliberately not a homegrown SQL lexer and not a new parser
|
||||
// dependency: the database already has the only correct parser.
|
||||
await transaction.executeMigrationSql(migration.sql);
|
||||
await transaction.insertAppliedMigration(freezeHistoryInsert(migration));
|
||||
}
|
||||
});
|
||||
|
||||
+132
-12
@@ -12,6 +12,7 @@ import {
|
||||
type ModuleRuntimeDefinition,
|
||||
type ModuleRuntimeServices,
|
||||
moduleRouteShapeKey,
|
||||
moduleRoutesOverlap,
|
||||
parseModuleRuntimeDefinition,
|
||||
type RegisteredCapability,
|
||||
type RegisteredModuleHttpRoute,
|
||||
@@ -33,10 +34,12 @@ export type ModuleRuntimeIssueCode =
|
||||
| "MODULE_CAPABILITY_UNREGISTERED"
|
||||
| "MODULE_REGISTER_FAILED"
|
||||
| "MODULE_ROLLBACK_FAILED"
|
||||
| "MODULE_ROLLBACK_TIMEOUT"
|
||||
| "MODULE_ROUTE_DEFINITION_INVALID"
|
||||
| "MODULE_ROUTE_DUPLICATE"
|
||||
| "MODULE_ROUTE_MAX_BODY_BYTES_INVALID"
|
||||
| "MODULE_ROUTE_METHOD_INVALID"
|
||||
| "MODULE_ROUTE_OVERLAP"
|
||||
| "MODULE_ROUTE_PATH_INVALID"
|
||||
| "MODULE_ROUTE_PERMISSION_INVALID"
|
||||
| "MODULE_ROUTE_PERMISSION_UNDECLARED"
|
||||
@@ -45,7 +48,8 @@ export type ModuleRuntimeIssueCode =
|
||||
| "MODULE_RUNTIME_IMPORT_FAILED"
|
||||
| "MODULE_RUNTIME_VERSION_MISMATCH"
|
||||
| "MODULE_START_FAILED"
|
||||
| "MODULE_STOP_FAILED";
|
||||
| "MODULE_STOP_FAILED"
|
||||
| "MODULE_STOP_TIMEOUT";
|
||||
|
||||
export interface ModuleRuntimeIssue {
|
||||
readonly code: ModuleRuntimeIssueCode;
|
||||
@@ -72,7 +76,16 @@ export interface StartedModuleRuntime {
|
||||
stop(): Promise<void>;
|
||||
}
|
||||
|
||||
export interface StartModuleRuntimeOptions extends ModuleDiscoveryOptions {
|
||||
export interface ModuleRuntimeTimeoutOptions {
|
||||
/** Test-only narrowing of `MODULE_STOP_HOOK_TIMEOUT_MS`, applied to both normal stop and startup
|
||||
* rollback, so a deadline can be proven in milliseconds instead of ten seconds. Production
|
||||
* composition never sets this. */
|
||||
readonly hookTimeoutMs?: number;
|
||||
}
|
||||
|
||||
export interface StartModuleRuntimeOptions
|
||||
extends ModuleDiscoveryOptions,
|
||||
ModuleRuntimeTimeoutOptions {
|
||||
/** The same frozen services object every started module's `start`/`stop` hooks and (once
|
||||
* `apps/api` mounts routes) HTTP handlers receive for this runtime — a required production
|
||||
* dependency, never an optional test-only global. See `ModuleRuntimeServices`. */
|
||||
@@ -331,6 +344,22 @@ class StagedModuleHttpRoutes {
|
||||
),
|
||||
);
|
||||
}
|
||||
// Distinct from a duplicate: these are *different* routes that can both match at least one
|
||||
// request path. With an order-sensitive matcher, whichever registered first would decide
|
||||
// which permission governs that path — a permission-policy decision no routing order may
|
||||
// make. Core rejects the ambiguity here instead of ranking by specificity, so the module
|
||||
// publishes none of its staged routes and no request can reach an ambiguous match.
|
||||
const overlapping = local.find((existing) => moduleRoutesOverlap(existing, parsed.data));
|
||||
if (overlapping) {
|
||||
throw new ModuleRuntimeSignal(
|
||||
runtimeIssue(
|
||||
"MODULE_ROUTE_OVERLAP",
|
||||
"register",
|
||||
module.manifest.id,
|
||||
`Module ${module.manifest.id} registered HTTP routes that can both match the same request path.`,
|
||||
),
|
||||
);
|
||||
}
|
||||
shapes.add(shapeKey);
|
||||
local.push(
|
||||
Object.freeze({
|
||||
@@ -471,18 +500,93 @@ async function registerModules(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The platform's fixed wait for one module's `stop` hook, during either a normal stop or a startup
|
||||
* rollback. Not configurable in this slice — no environment variable, manifest field, or runtime
|
||||
* service — so every deployment bounds cleanup identically.
|
||||
*
|
||||
* This bounds *waiting*, not execution. An in-process promise cannot be canceled: a hook that
|
||||
* exceeds this deadline keeps running and may settle later, but it no longer delays the rest of
|
||||
* shutdown, and the pool closes after the bounded sequence, so its late database work fails. The
|
||||
* platform does not sandbox, terminate, or undo a trusted module's JavaScript.
|
||||
*/
|
||||
export const MODULE_STOP_HOOK_TIMEOUT_MS = 10_000;
|
||||
|
||||
interface StopModulesOptions {
|
||||
/** Test-only narrowing of `MODULE_STOP_HOOK_TIMEOUT_MS` so a deadline can be proven without
|
||||
* waiting out the production value. Production composition never sets this. */
|
||||
readonly hookTimeoutMs?: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Awaits one hook under a deadline. The losing branch is always handled: the timer is cleared when
|
||||
* the hook wins, and a hook that loses gets a permanent no-op rejection handler attached so a late
|
||||
* throw can never surface as an unhandled rejection. A timed-out hook is never awaited again.
|
||||
*/
|
||||
async function awaitHookWithinDeadline(
|
||||
hook: Promise<void>,
|
||||
timeoutMs: number,
|
||||
): Promise<"completed" | "timed-out"> {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
// Attached before the race so the rejection is considered handled even if the timeout wins.
|
||||
const observed = hook.then(
|
||||
() => "completed" as const,
|
||||
(error: unknown) => {
|
||||
throw error;
|
||||
},
|
||||
);
|
||||
observed.catch(() => undefined);
|
||||
try {
|
||||
return await Promise.race([
|
||||
observed,
|
||||
new Promise<"timed-out">((resolve) => {
|
||||
timer = setTimeout(() => resolve("timed-out"), timeoutMs);
|
||||
}),
|
||||
]);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Stops modules in reverse order, giving each hook a bounded slice of time and continuing to the
|
||||
* next module whatever the previous one did. Every applicable hook is invoked at most once.
|
||||
*
|
||||
* The whole sequence is finite because the module count is finite and fixed at discovery: its
|
||||
* worst case is `startedModuleCount * MODULE_STOP_HOOK_TIMEOUT_MS`, not one global ten-second
|
||||
* bound.
|
||||
*/
|
||||
async function stopModules(
|
||||
modules: readonly LoadedModule[],
|
||||
registry: StagedCapabilityRegistry,
|
||||
phase: "rollback" | "stop",
|
||||
services: ModuleRuntimeServices,
|
||||
options: StopModulesOptions = {},
|
||||
): Promise<readonly ModuleRuntimeIssue[]> {
|
||||
const timeoutMs = options.hookTimeoutMs ?? MODULE_STOP_HOOK_TIMEOUT_MS;
|
||||
const issues: ModuleRuntimeIssue[] = [];
|
||||
for (const module of [...modules].reverse()) {
|
||||
const moduleId = module.discovered.manifest.id;
|
||||
const stop = module.definition.stop;
|
||||
if (!stop) {
|
||||
// No synthetic callback and no timer for a module that declares no stop hook.
|
||||
continue;
|
||||
}
|
||||
try {
|
||||
await module.definition.stop?.(
|
||||
registry.createRuntimeContext(module.discovered, phase, services),
|
||||
const outcome = await awaitHookWithinDeadline(
|
||||
Promise.resolve(stop(registry.createRuntimeContext(module.discovered, phase, services))),
|
||||
timeoutMs,
|
||||
);
|
||||
if (outcome === "timed-out") {
|
||||
issues.push(
|
||||
runtimeIssue(
|
||||
phase === "rollback" ? "MODULE_ROLLBACK_TIMEOUT" : "MODULE_STOP_TIMEOUT",
|
||||
phase,
|
||||
moduleId,
|
||||
`Module ${moduleId} ${phase} did not complete within the platform deadline.`,
|
||||
),
|
||||
);
|
||||
}
|
||||
} catch (error) {
|
||||
if (error instanceof ModuleRuntimeSignal) {
|
||||
issues.push(error.issue);
|
||||
@@ -491,8 +595,8 @@ async function stopModules(
|
||||
runtimeIssue(
|
||||
phase === "rollback" ? "MODULE_ROLLBACK_FAILED" : "MODULE_STOP_FAILED",
|
||||
phase,
|
||||
module.discovered.manifest.id,
|
||||
`Module ${module.discovered.manifest.id} ${phase} failed.`,
|
||||
moduleId,
|
||||
`Module ${moduleId} ${phase} failed.`,
|
||||
),
|
||||
);
|
||||
}
|
||||
@@ -505,14 +609,21 @@ async function startModules(
|
||||
modules: readonly LoadedModule[],
|
||||
registry: StagedCapabilityRegistry,
|
||||
services: ModuleRuntimeServices,
|
||||
options: StopModulesOptions = {},
|
||||
): Promise<void> {
|
||||
const started: LoadedModule[] = [];
|
||||
// A module joins the rollback set the moment its `start` hook is *invoked*, not when it
|
||||
// completes. A hook that acquired a connection, registered a listener, or recorded partial
|
||||
// initialization before throwing still has cleanup to do, and previously only the modules that
|
||||
// had already finished were rolled back — leaving the failing module's own resources behind. Its
|
||||
// `stop` hook therefore runs first, ahead of the earlier modules in reverse order, which is why
|
||||
// module authors must make `stop` safe after a partially completed `start`.
|
||||
const rollbackCandidates: LoadedModule[] = [];
|
||||
for (const module of modules) {
|
||||
rollbackCandidates.push(module);
|
||||
try {
|
||||
await module.definition.start?.(
|
||||
registry.createRuntimeContext(module.discovered, "start", services),
|
||||
);
|
||||
started.push(module);
|
||||
} catch (error) {
|
||||
const startIssue =
|
||||
error instanceof ModuleRuntimeSignal
|
||||
@@ -523,7 +634,13 @@ async function startModules(
|
||||
module.discovered.manifest.id,
|
||||
`Module ${module.discovered.manifest.id} startup failed.`,
|
||||
);
|
||||
const rollbackIssues = await stopModules(started, registry, "rollback", services);
|
||||
const rollbackIssues = await stopModules(
|
||||
rollbackCandidates,
|
||||
registry,
|
||||
"rollback",
|
||||
services,
|
||||
options,
|
||||
);
|
||||
throw new ModuleRuntimeError([startIssue, ...rollbackIssues]);
|
||||
}
|
||||
}
|
||||
@@ -540,12 +657,13 @@ async function startModules(
|
||||
export async function startDiscoveredModuleRuntime(
|
||||
discoveredModules: readonly DiscoveredModule[],
|
||||
services: ModuleRuntimeServices,
|
||||
timeouts: ModuleRuntimeTimeoutOptions = {},
|
||||
): Promise<StartedModuleRuntime> {
|
||||
const modules = await loadRuntimeDefinitions(discoveredModules);
|
||||
const capabilityRegistry = new StagedCapabilityRegistry();
|
||||
const httpRoutes = new StagedModuleHttpRoutes();
|
||||
await registerModules(modules, capabilityRegistry, httpRoutes);
|
||||
await startModules(modules, capabilityRegistry, services);
|
||||
await startModules(modules, capabilityRegistry, services, timeouts);
|
||||
|
||||
const capabilities = capabilityRegistry.publish();
|
||||
const routes = httpRoutes.publish();
|
||||
@@ -565,7 +683,7 @@ export async function startDiscoveredModuleRuntime(
|
||||
routes,
|
||||
stop: (): Promise<void> => {
|
||||
stopPromise ??= (async () => {
|
||||
const issues = await stopModules(modules, capabilityRegistry, "stop", services);
|
||||
const issues = await stopModules(modules, capabilityRegistry, "stop", services, timeouts);
|
||||
if (issues.length > 0) {
|
||||
throw new ModuleRuntimeError(issues);
|
||||
}
|
||||
@@ -579,5 +697,7 @@ export async function startModuleRuntime(
|
||||
options: StartModuleRuntimeOptions,
|
||||
): Promise<StartedModuleRuntime> {
|
||||
const discovered = await discoverModules(options);
|
||||
return startDiscoveredModuleRuntime(discovered, options.services);
|
||||
return startDiscoveredModuleRuntime(discovered, options.services, {
|
||||
...(options.hookTimeoutMs === undefined ? {} : { hookTimeoutMs: options.hookTimeoutMs }),
|
||||
});
|
||||
}
|
||||
|
||||
@@ -200,7 +200,7 @@ class FakeMigrationDatabaseSession implements MigrationDatabaseSession {
|
||||
}
|
||||
stagedTableExists = true;
|
||||
},
|
||||
executeStatement: async (sql) => {
|
||||
executeMigrationSql: async (sql) => {
|
||||
this.#database.events.push("sql:execute");
|
||||
this.#database.executedStatements.push(sql);
|
||||
const statementIndex = this.#statementCount;
|
||||
@@ -655,7 +655,7 @@ describe("module migration apply", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("preserves exact Drizzle breakpoints without naively splitting semicolons", async () => {
|
||||
it("sends each migration's complete SQL to PostgreSQL in exactly one call, byte for byte", async () => {
|
||||
const sql =
|
||||
'CREATE TABLE "people_records" ("id" integer);\n--> statement-breakpoint\nALTER TABLE "people_records" ADD COLUMN "name" text; SELECT 2;';
|
||||
const initial = migration("people", 0, { sql });
|
||||
@@ -666,15 +666,54 @@ describe("module migration apply", () => {
|
||||
database,
|
||||
});
|
||||
|
||||
expect(database.executedStatements).toEqual([
|
||||
'CREATE TABLE "people_records" ("id" integer);\n',
|
||||
'\nALTER TABLE "people_records" ADD COLUMN "name" text; SELECT 2;',
|
||||
]);
|
||||
// One call, unmodified: the runner never splits, trims, normalizes, or rewrites migration SQL.
|
||||
// PostgreSQL parses the batch, and Drizzle's marker lines are ordinary `--` comments to it.
|
||||
expect(database.executedStatements).toEqual([sql]);
|
||||
expect(database.events.indexOf(`history:insert:${initial.identity}`)).toBeGreaterThan(
|
||||
database.events.lastIndexOf("sql:execute"),
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
[
|
||||
"a string literal containing the marker bytes",
|
||||
`INSERT INTO "notes" ("body") VALUES ('keep this: --> statement-breakpoint intact');`,
|
||||
],
|
||||
[
|
||||
"a dollar-quoted function body containing the marker and semicolons",
|
||||
`CREATE FUNCTION touch() RETURNS void AS $$\nBEGIN\n -- --> statement-breakpoint\n PERFORM 1;\n PERFORM 2;\nEND;\n$$ LANGUAGE plpgsql;`,
|
||||
],
|
||||
["a line comment containing the marker", `-- note: --> statement-breakpoint\nSELECT 1;`],
|
||||
["a block comment containing the marker", `/* --> statement-breakpoint */ SELECT 1;`],
|
||||
[
|
||||
"a quoted identifier containing the marker",
|
||||
`CREATE TABLE "odd--> statement-breakpoint name" ("id" integer);`,
|
||||
],
|
||||
])("never fragments %s", async (_label, sql) => {
|
||||
const initial = migration("people", 0, { sql });
|
||||
const database = new FakeMigrationDatabase();
|
||||
|
||||
await applyModuleMigrations({
|
||||
aggregate: aggregate({ id: "people", migrations: [initial] }),
|
||||
database,
|
||||
});
|
||||
|
||||
expect(database.executedStatements).toEqual([sql]);
|
||||
});
|
||||
|
||||
it("sends several generated migrations as one call each, in order", async () => {
|
||||
const first = migration("people", 0, { sql: `SELECT 1;\n--> statement-breakpoint\nSELECT 2;` });
|
||||
const second = migration("people", 1, { sql: `SELECT 3;` });
|
||||
const database = new FakeMigrationDatabase();
|
||||
|
||||
await applyModuleMigrations({
|
||||
aggregate: aggregate({ id: "people", migrations: [first, second] }),
|
||||
database,
|
||||
});
|
||||
|
||||
expect(database.executedStatements).toEqual([first.sql, second.sql]);
|
||||
});
|
||||
|
||||
it("rolls back the whole pending batch on a later SQL failure and preserves prior history", async () => {
|
||||
const prior = migration("people", 0);
|
||||
const pendingFirst = migration("people", 1);
|
||||
|
||||
@@ -369,10 +369,13 @@ describe("module runtime", () => {
|
||||
const error = await captureRuntimeError(modulesDirectory);
|
||||
|
||||
expect(runtimeIssueCodes(error)).toEqual(["MODULE_START_FAILED", "MODULE_ROLLBACK_FAILED"]);
|
||||
// The module whose `start` threw is rolled back *first*, then the previously started modules in
|
||||
// reverse order — its own hook may have partial initialization to release before it threw.
|
||||
expect(recorder.events).toEqual([
|
||||
"one:start",
|
||||
"two:start",
|
||||
"three:start",
|
||||
"three:stop",
|
||||
"two:stop",
|
||||
"one:stop",
|
||||
]);
|
||||
@@ -562,6 +565,141 @@ describe("module HTTP routes", () => {
|
||||
expect(runtimeIssueCodes(error)).toEqual(["MODULE_ROUTE_DUPLICATE"]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["parameter registered first", "/events/:id", "/events/new"],
|
||||
["static registered first", "/events/new", "/events/:id"],
|
||||
["multi-position, parameter first", "/events/:id/edit", "/events/new/:action"],
|
||||
["multi-position, static first", "/events/new/:action", "/events/:id/edit"],
|
||||
["parameters on both sides", "/a/:left/c", "/a/b/:right"],
|
||||
])("rejects routes that can both match one request path (%s) regardless of registration order", async (_label, firstPath, secondPath) => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
// Deliberately *different* permissions: the defect this rejects is that whichever route
|
||||
// registered first would decide which permission governs the ambiguous path.
|
||||
await createRuntimeModule(modulesDirectory, "overlap", {
|
||||
manifest: manifestFor("overlap", {
|
||||
permissions: [
|
||||
permissionDeclaration("overlap.events.read"),
|
||||
permissionDeclaration("overlap.events.create"),
|
||||
],
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "overlap", version: "0.1.0",
|
||||
register({ http }) {
|
||||
http.route({ method: "GET", path: "${firstPath}", permission: "overlap.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
http.route({ method: "GET", path: "${secondPath}", permission: "overlap.events.create", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
}
|
||||
};`,
|
||||
});
|
||||
|
||||
const error = await captureRuntimeError(modulesDirectory);
|
||||
expect(runtimeIssueCodes(error)).toEqual(["MODULE_ROUTE_OVERLAP"]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["two different static literals", "/events/new", "/events/archive"],
|
||||
["different segment counts", "/events/:id", "/events/:id/edit"],
|
||||
["a different static prefix", "/events/:id", "/people/:id"],
|
||||
["module root against a one-segment path", "/", "/events"],
|
||||
])("still allows disjoint routes (%s) and preserves their registration order", async (_label, firstPath, secondPath) => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
await createRuntimeModule(modulesDirectory, "disjoint", {
|
||||
manifest: manifestFor("disjoint", {
|
||||
permissions: [permissionDeclaration("disjoint.events.read")],
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "disjoint", version: "0.1.0",
|
||||
register({ http }) {
|
||||
http.route({ method: "GET", path: "${firstPath}", permission: "disjoint.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
http.route({ method: "GET", path: "${secondPath}", permission: "disjoint.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
}
|
||||
};`,
|
||||
});
|
||||
|
||||
const runtime = await startModuleRuntime({
|
||||
moduleSdkVersion: "0.1.0",
|
||||
modulesDirectory,
|
||||
platformVersion: "0.1.0",
|
||||
services: fakeModuleRuntimeServices(),
|
||||
});
|
||||
expect(runtime.routes.map((route) => route.path)).toEqual([firstPath, secondPath]);
|
||||
await runtime.stop();
|
||||
});
|
||||
|
||||
it("allows the same overlapping local path under different methods", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
await createRuntimeModule(modulesDirectory, "methods", {
|
||||
manifest: manifestFor("methods", {
|
||||
permissions: [permissionDeclaration("methods.events.read")],
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "methods", version: "0.1.0",
|
||||
register({ http }) {
|
||||
http.route({ method: "GET", path: "/events/:id", permission: "methods.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
http.route({ method: "POST", path: "/events/new", permission: "methods.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
}
|
||||
};`,
|
||||
});
|
||||
|
||||
const runtime = await startModuleRuntime({
|
||||
moduleSdkVersion: "0.1.0",
|
||||
modulesDirectory,
|
||||
platformVersion: "0.1.0",
|
||||
services: fakeModuleRuntimeServices(),
|
||||
});
|
||||
expect(runtime.routes).toHaveLength(2);
|
||||
await runtime.stop();
|
||||
});
|
||||
|
||||
it("allows two different modules to declare the same overlapping local paths", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
for (const moduleId of ["alpha", "beta"]) {
|
||||
await createRuntimeModule(modulesDirectory, moduleId, {
|
||||
manifest: manifestFor(moduleId, {
|
||||
permissions: [permissionDeclaration(`${moduleId}.events.read`)],
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "${moduleId}", version: "0.1.0",
|
||||
register({ http }) {
|
||||
http.route({ method: "GET", path: "/events/:id", permission: "${moduleId}.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
}
|
||||
};`,
|
||||
});
|
||||
}
|
||||
|
||||
// Each module has its own platform-owned mount prefix, so these can never collide.
|
||||
const runtime = await startModuleRuntime({
|
||||
moduleSdkVersion: "0.1.0",
|
||||
modulesDirectory,
|
||||
platformVersion: "0.1.0",
|
||||
services: fakeModuleRuntimeServices(),
|
||||
});
|
||||
expect(runtime.routes.map((route) => route.moduleId).sort()).toEqual(["alpha", "beta"]);
|
||||
await runtime.stop();
|
||||
});
|
||||
|
||||
it("publishes no route from a module whose registration was rejected for an overlap", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
await createRuntimeModule(modulesDirectory, "partial", {
|
||||
manifest: manifestFor("partial", {
|
||||
permissions: [permissionDeclaration("partial.events.read")],
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "partial", version: "0.1.0",
|
||||
register({ http }) {
|
||||
http.route({ method: "GET", path: "/healthy", permission: "partial.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
http.route({ method: "GET", path: "/events/:id", permission: "partial.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
http.route({ method: "GET", path: "/events/new", permission: "partial.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} });
|
||||
},
|
||||
start() { throw new Error("start must never run for a rejected registration"); }
|
||||
};`,
|
||||
});
|
||||
|
||||
const error = await captureRuntimeError(modulesDirectory);
|
||||
expect(runtimeIssueCodes(error)).toEqual(["MODULE_ROUTE_OVERLAP"]);
|
||||
// The earlier, perfectly valid `/healthy` route is not published either: registration is
|
||||
// all-or-nothing, and the start hook above would have thrown had it ever run.
|
||||
});
|
||||
|
||||
it("fails a malformed route with its specific structured issue code", async () => {
|
||||
const badMethodDirectory = await createModulesDirectory();
|
||||
await createRuntimeModule(badMethodDirectory, "bad-method", {
|
||||
@@ -1016,3 +1154,200 @@ describe("module runtime services", () => {
|
||||
expect(vi.mocked(services.database.transaction)).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Pass 4: a module whose `start` hook fails owns cleanup of whatever it managed to acquire, and
|
||||
* every stop/rollback hook runs under a finite platform deadline.
|
||||
*/
|
||||
describe("module startup rollback and bounded stop hooks", () => {
|
||||
it("releases a partially initialized resource through the failing module's own stop hook", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
const recorder = createEventRecorder();
|
||||
await createRuntimeModule(modulesDirectory, "partial", {
|
||||
serverSource: `let acquired = false;
|
||||
export default {
|
||||
id: "partial", version: "0.1.0",
|
||||
start() {
|
||||
acquired = true;
|
||||
${recorder.expression}.push("partial:acquired");
|
||||
throw new Error("secret-token at /private/path");
|
||||
},
|
||||
stop() {
|
||||
if (acquired) { acquired = false; ${recorder.expression}.push("partial:released"); }
|
||||
}
|
||||
};`,
|
||||
});
|
||||
|
||||
const error = await captureRuntimeError(modulesDirectory);
|
||||
|
||||
expect(runtimeIssueCodes(error)).toEqual(["MODULE_START_FAILED"]);
|
||||
expect(recorder.events).toEqual(["partial:acquired", "partial:released"]);
|
||||
expect(JSON.stringify(error.issues)).not.toContain("secret-token");
|
||||
expect(JSON.stringify(error.issues)).not.toContain("/private/path");
|
||||
});
|
||||
|
||||
it("reports the failing module's own rollback failure and still rolls back earlier modules", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
const recorder = createEventRecorder();
|
||||
await createRuntimeModule(modulesDirectory, "earlier", {
|
||||
serverSource: `export default {
|
||||
id: "earlier", version: "0.1.0",
|
||||
stop() { ${recorder.expression}.push("earlier:stop"); }
|
||||
};`,
|
||||
});
|
||||
await createRuntimeModule(modulesDirectory, "failing", {
|
||||
manifest: manifestFor("failing", {
|
||||
dependencies: { modules: { required: { earlier: "^0.1.0" } } },
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "failing", version: "0.1.0",
|
||||
start() { throw new Error("start blew up"); },
|
||||
stop() { ${recorder.expression}.push("failing:stop"); throw new Error("rollback blew up"); }
|
||||
};`,
|
||||
});
|
||||
|
||||
const error = await captureRuntimeError(modulesDirectory);
|
||||
|
||||
expect(runtimeIssueCodes(error)).toEqual(["MODULE_START_FAILED", "MODULE_ROLLBACK_FAILED"]);
|
||||
expect(recorder.events).toEqual(["failing:stop", "earlier:stop"]);
|
||||
});
|
||||
|
||||
it("gives a ModuleRuntimeSignal thrown during start the same rollback treatment", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
const recorder = createEventRecorder();
|
||||
// `capabilities.require` for a capability this module never declared throws a
|
||||
// `ModuleRuntimeSignal` from inside `start`, rather than an ordinary Error.
|
||||
await createRuntimeModule(modulesDirectory, "signal", {
|
||||
serverSource: `export default {
|
||||
id: "signal", version: "0.1.0",
|
||||
start({ capabilities }) { capabilities.require("undeclared.thing"); },
|
||||
stop() { ${recorder.expression}.push("signal:stop"); }
|
||||
};`,
|
||||
});
|
||||
|
||||
const error = await captureRuntimeError(modulesDirectory);
|
||||
|
||||
expect(error.issues[0]?.phase).toBe("start");
|
||||
expect(recorder.events).toEqual(["signal:stop"]);
|
||||
});
|
||||
|
||||
it("invokes each stop hook at most once across rollback", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
const recorder = createEventRecorder();
|
||||
await createRuntimeModule(modulesDirectory, "counted", {
|
||||
serverSource: `export default {
|
||||
id: "counted", version: "0.1.0",
|
||||
start() { throw new Error("nope"); },
|
||||
stop() { ${recorder.expression}.push("counted:stop"); }
|
||||
};`,
|
||||
});
|
||||
|
||||
await captureRuntimeError(modulesDirectory);
|
||||
|
||||
expect(recorder.events.filter((event) => event === "counted:stop")).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("bounds a never-settling rollback hook and continues with the earlier module", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
const recorder = createEventRecorder();
|
||||
await createRuntimeModule(modulesDirectory, "earlier", {
|
||||
serverSource: `export default {
|
||||
id: "earlier", version: "0.1.0",
|
||||
stop() { ${recorder.expression}.push("earlier:stop"); }
|
||||
};`,
|
||||
});
|
||||
await createRuntimeModule(modulesDirectory, "hanging", {
|
||||
manifest: manifestFor("hanging", {
|
||||
dependencies: { modules: { required: { earlier: "^0.1.0" } } },
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "hanging", version: "0.1.0",
|
||||
start() { throw new Error("nope"); },
|
||||
stop() { ${recorder.expression}.push("hanging:stop"); return new Promise(() => {}); }
|
||||
};`,
|
||||
});
|
||||
|
||||
// A short test-only deadline; production uses MODULE_STOP_HOOK_TIMEOUT_MS unchanged.
|
||||
let captured: ModuleRuntimeError | undefined;
|
||||
try {
|
||||
await startModuleRuntime({ ...runtimeOptions(modulesDirectory), hookTimeoutMs: 25 });
|
||||
} catch (error) {
|
||||
captured = error as ModuleRuntimeError;
|
||||
}
|
||||
|
||||
expect(captured).toBeInstanceOf(ModuleRuntimeError);
|
||||
expect(runtimeIssueCodes(captured as ModuleRuntimeError)).toEqual([
|
||||
"MODULE_START_FAILED",
|
||||
"MODULE_ROLLBACK_TIMEOUT",
|
||||
]);
|
||||
// The timed-out hook did not block the earlier module's rollback.
|
||||
expect(recorder.events).toEqual(["hanging:stop", "earlier:stop"]);
|
||||
});
|
||||
|
||||
it("bounds a never-settling normal stop hook and continues with the earlier module", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
const recorder = createEventRecorder();
|
||||
await createRuntimeModule(modulesDirectory, "earlier", {
|
||||
serverSource: `export default {
|
||||
id: "earlier", version: "0.1.0",
|
||||
stop() { ${recorder.expression}.push("earlier:stop"); }
|
||||
};`,
|
||||
});
|
||||
await createRuntimeModule(modulesDirectory, "hanging", {
|
||||
manifest: manifestFor("hanging", {
|
||||
dependencies: { modules: { required: { earlier: "^0.1.0" } } },
|
||||
}),
|
||||
serverSource: `export default {
|
||||
id: "hanging", version: "0.1.0",
|
||||
stop() { ${recorder.expression}.push("hanging:stop"); return new Promise(() => {}); }
|
||||
};`,
|
||||
});
|
||||
|
||||
const runtime = await startModuleRuntime({
|
||||
...runtimeOptions(modulesDirectory),
|
||||
hookTimeoutMs: 25,
|
||||
});
|
||||
let captured: ModuleRuntimeError | undefined;
|
||||
try {
|
||||
await runtime.stop();
|
||||
} catch (error) {
|
||||
captured = error as ModuleRuntimeError;
|
||||
}
|
||||
|
||||
expect(runtimeIssueCodes(captured as ModuleRuntimeError)).toEqual(["MODULE_STOP_TIMEOUT"]);
|
||||
expect(recorder.events).toEqual(["hanging:stop", "earlier:stop"]);
|
||||
// Repeated and concurrent callers share the memoized promise and never restart cleanup.
|
||||
const [first, second] = await Promise.allSettled([runtime.stop(), runtime.stop()]);
|
||||
expect(first.status).toBe("rejected");
|
||||
expect(second.status).toBe("rejected");
|
||||
expect(recorder.events.filter((event) => event === "hanging:stop")).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("never lets a hook that rejects after its deadline become an unhandled rejection", async () => {
|
||||
const modulesDirectory = await createModulesDirectory();
|
||||
await createRuntimeModule(modulesDirectory, "late", {
|
||||
serverSource: `export default {
|
||||
id: "late", version: "0.1.0",
|
||||
stop() { return new Promise((_resolve, reject) => { setTimeout(() => reject(new Error("late")), 40); }); }
|
||||
};`,
|
||||
});
|
||||
|
||||
const unhandled: unknown[] = [];
|
||||
const onUnhandled = (reason: unknown): void => {
|
||||
unhandled.push(reason);
|
||||
};
|
||||
process.on("unhandledRejection", onUnhandled);
|
||||
try {
|
||||
const runtime = await startModuleRuntime({
|
||||
...runtimeOptions(modulesDirectory),
|
||||
hookTimeoutMs: 10,
|
||||
});
|
||||
await expect(runtime.stop()).rejects.toBeInstanceOf(ModuleRuntimeError);
|
||||
// Give the late rejection time to fire, then confirm nothing escaped.
|
||||
await new Promise((resolve) => setTimeout(resolve, 80));
|
||||
expect(unhandled).toEqual([]);
|
||||
} finally {
|
||||
process.off("unhandledRejection", onUnhandled);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -6,6 +6,8 @@ vi.mock("pg", () => ({ Client: clientConstructor }));
|
||||
|
||||
import {
|
||||
createPostgresMigrationDatabase,
|
||||
MIGRATION_CONNECTION_TIMEOUT_MS,
|
||||
MIGRATION_OPERATION_TIMEOUT_MS,
|
||||
PostgresMigrationDatabaseError,
|
||||
} from "../src/migrations/postgres-database.js";
|
||||
|
||||
@@ -110,3 +112,110 @@ describe("PostgreSQL migration database composition", () => {
|
||||
expect(client.end).toHaveBeenCalledOnce();
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Pass 4: finite migration timeouts live at this one construction boundary, and the pinned driver's
|
||||
* connection-string precedence cannot weaken them.
|
||||
*/
|
||||
describe("PostgreSQL migration timeout policy", () => {
|
||||
function mockClient() {
|
||||
const client = {
|
||||
connect: vi.fn().mockResolvedValue(undefined),
|
||||
end: vi.fn().mockResolvedValue(undefined),
|
||||
on: vi.fn(),
|
||||
};
|
||||
clientConstructor.mockImplementation(
|
||||
class {
|
||||
readonly connect = client.connect;
|
||||
readonly end = client.end;
|
||||
readonly on = client.on;
|
||||
} as unknown as () => typeof client,
|
||||
);
|
||||
return client;
|
||||
}
|
||||
|
||||
it("applies the fixed 10-second defaults when a caller omits them", async () => {
|
||||
mockClient();
|
||||
|
||||
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia");
|
||||
const session = await database.openSession();
|
||||
|
||||
// This is exactly what `migrations:plan` and `migrations:apply` construct: no timeout options.
|
||||
expect(clientConstructor).toHaveBeenCalledWith({
|
||||
connectionString: "postgresql://example.test/ekklesia",
|
||||
connectionTimeoutMillis: MIGRATION_CONNECTION_TIMEOUT_MS,
|
||||
query_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
|
||||
statement_timeout: MIGRATION_OPERATION_TIMEOUT_MS,
|
||||
});
|
||||
expect(MIGRATION_CONNECTION_TIMEOUT_MS).toBe(10_000);
|
||||
expect(MIGRATION_OPERATION_TIMEOUT_MS).toBe(10_000);
|
||||
|
||||
await session.close();
|
||||
});
|
||||
|
||||
it("still honours an explicit narrower test override", async () => {
|
||||
mockClient();
|
||||
|
||||
const database = createPostgresMigrationDatabase("postgresql://example.test/ekklesia", {
|
||||
connectionTimeoutMs: 250,
|
||||
operationTimeoutMs: 500,
|
||||
});
|
||||
const session = await database.openSession();
|
||||
|
||||
expect(clientConstructor).toHaveBeenCalledWith({
|
||||
connectionString: "postgresql://example.test/ekklesia",
|
||||
connectionTimeoutMillis: 250,
|
||||
query_timeout: 500,
|
||||
statement_timeout: 500,
|
||||
});
|
||||
|
||||
await session.close();
|
||||
});
|
||||
|
||||
it.each([
|
||||
"postgresql://example.test/ekklesia?statement_timeout=0",
|
||||
"postgresql://example.test/ekklesia?STATEMENT_TIMEOUT=0",
|
||||
"postgresql://example.test/ekklesia?query_timeout=0",
|
||||
"postgresql://example.test/ekklesia?connect_timeout=0",
|
||||
"postgresql://example.test/ekklesia?options=-c%20statement_timeout%3D0",
|
||||
"postgresql://example.test/ekklesia?statement_timeout=900000",
|
||||
])("refuses a connection string that would override the policy (%s)", (databaseUrl) => {
|
||||
// The pinned [email protected] merges as `Object.assign({}, config, parse(connectionString))`, so the
|
||||
// URL wins over the explicit values above — verified against the real driver, where an explicit
|
||||
// `statement_timeout: 10000` alongside `?statement_timeout=0` produced a server-reported `0`.
|
||||
const constructionsBefore = clientConstructor.mock.calls.length;
|
||||
let captured: unknown;
|
||||
try {
|
||||
createPostgresMigrationDatabase(databaseUrl);
|
||||
} catch (error) {
|
||||
captured = error;
|
||||
}
|
||||
|
||||
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
|
||||
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONFIGURATION_INVALID" });
|
||||
// The rejection never echoes the URL, the offending parameter, or its value.
|
||||
const publicText = `${String(captured)}\n${
|
||||
captured instanceof Error ? (captured.stack ?? "") : ""
|
||||
}`;
|
||||
expect(publicText).not.toContain("example.test");
|
||||
expect(publicText).not.toContain("statement_timeout=");
|
||||
expect(publicText).not.toContain("900000");
|
||||
// Rejected before any client is constructed, so no connection is ever attempted.
|
||||
expect(clientConstructor.mock.calls.length).toBe(constructionsBefore);
|
||||
});
|
||||
|
||||
it("leaves unrelated connection-string parameters alone", async () => {
|
||||
mockClient();
|
||||
|
||||
const database = createPostgresMigrationDatabase(
|
||||
"postgresql://example.test/ekklesia?application_name=ekklesia&lock_timeout=5000",
|
||||
);
|
||||
const session = await database.openSession();
|
||||
|
||||
expect(clientConstructor).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ statement_timeout: MIGRATION_OPERATION_TIMEOUT_MS }),
|
||||
);
|
||||
|
||||
await session.close();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { createHash, randomBytes } from "node:crypto";
|
||||
import net from "node:net";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { Client, type QueryResult, type QueryResultRow } from "pg";
|
||||
@@ -13,6 +14,7 @@ import {
|
||||
MigrationRunnerError,
|
||||
type MigrationRunnerErrorCode,
|
||||
planModuleMigrations,
|
||||
PostgresMigrationDatabaseError,
|
||||
} from "../src/migrations/index.js";
|
||||
import type {
|
||||
DiscoveredModuleMigration,
|
||||
@@ -1457,3 +1459,179 @@ describe.sequential("live PostgreSQL Core identity migration execution", () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Pass 4: the migration commands' finite timeout defaults, and whole-batch SQL execution, proven
|
||||
* against real PostgreSQL and a real TCP endpoint rather than a fake.
|
||||
*
|
||||
* Every listener, client, and disposable database here is closed in `finally`/`afterAll`, including
|
||||
* after a failed assertion.
|
||||
*/
|
||||
describe.sequential("live PostgreSQL migration command hardening", () => {
|
||||
it("bounds a connection to an endpoint that accepts TCP but never completes PostgreSQL startup", async () => {
|
||||
// A real blackhole listener bound only to loopback: it accepts the socket and then says
|
||||
// nothing, which is exactly the case `connectionTimeoutMillis` exists for.
|
||||
const sockets: net.Socket[] = [];
|
||||
const listener = net.createServer((socket) => {
|
||||
sockets.push(socket);
|
||||
});
|
||||
try {
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
listener.once("error", reject);
|
||||
listener.listen(0, "127.0.0.1", () => resolve());
|
||||
});
|
||||
const address = listener.address();
|
||||
if (!address || typeof address !== "object") {
|
||||
throw harnessError("The blackhole listener has no bound address.");
|
||||
}
|
||||
|
||||
// A short, test-only override; production keeps MIGRATION_CONNECTION_TIMEOUT_MS unchanged.
|
||||
const blackholeDatabase = createPostgresMigrationDatabase(
|
||||
`postgresql://ekklesia:[email protected]:${address.port}/ekklesia`,
|
||||
{ connectionTimeoutMs: 300, operationTimeoutMs: 300 },
|
||||
);
|
||||
|
||||
const startedAt = Date.now();
|
||||
let captured: unknown;
|
||||
try {
|
||||
await blackholeDatabase.openSession();
|
||||
} catch (error) {
|
||||
captured = error;
|
||||
}
|
||||
|
||||
expect(captured).toBeInstanceOf(PostgresMigrationDatabaseError);
|
||||
expect(captured).toMatchObject({ code: "POSTGRES_MIGRATION_CONNECTION_FAILED" });
|
||||
// Finite, and nowhere near an unbounded wait.
|
||||
expect(Date.now() - startedAt).toBeLessThan(10_000);
|
||||
const publicText = `${String(captured)}\n${(captured as Error).stack ?? ""}`;
|
||||
expect(publicText).not.toContain("127.0.0.1");
|
||||
expect(publicText).not.toContain("ekklesia:ekklesia");
|
||||
expect(publicText).not.toContain(String(address.port));
|
||||
} finally {
|
||||
for (const socket of sockets) {
|
||||
socket.destroy();
|
||||
}
|
||||
await new Promise<void>((resolve) => {
|
||||
listener.close(() => resolve());
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
it("aborts a migration that exceeds the operation deadline and commits no history row", async () => {
|
||||
await withIsolatedDatabase("operation_timeout", async (database) => {
|
||||
const slow = migration("people", 0, {
|
||||
sql: "SELECT pg_sleep(5);",
|
||||
tag: "0000_slow",
|
||||
});
|
||||
// The operation deadline is the behavior under test, so a bounded server-side delay is the
|
||||
// only way to exercise it.
|
||||
const migrationDatabase = createPostgresMigrationDatabase(database.url, {
|
||||
connectionTimeoutMs: 5_000,
|
||||
operationTimeoutMs: 250,
|
||||
});
|
||||
|
||||
const error = await captureRunnerError(
|
||||
applyModuleMigrations({
|
||||
aggregate: aggregate({ id: "people", migrations: [slow] }),
|
||||
database: migrationDatabase,
|
||||
}),
|
||||
"MIGRATION_EXECUTION_FAILED",
|
||||
);
|
||||
|
||||
expect(error.migrationIdentity).toBe(slow.identity);
|
||||
const publicText = `${String(error)}\n${error.stack ?? ""}`;
|
||||
expect(publicText).not.toContain("pg_sleep");
|
||||
expect(publicText).not.toContain(database.name);
|
||||
|
||||
// Nothing was committed: the transaction rolled back with its history insert.
|
||||
const history = await safeQuery<{ identity: string }>(
|
||||
database.client,
|
||||
`select identity from public.ekklesia_core_migrations`,
|
||||
).catch(() => undefined);
|
||||
expect(history?.rows ?? []).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
it("executes marker bytes inside literals, dollar-quoted bodies, and comments intact", async () => {
|
||||
await withIsolatedDatabase("intact_sql", async (database) => {
|
||||
const marker = "--> statement-breakpoint";
|
||||
const sql = [
|
||||
`CREATE TABLE "people_notes" ("id" integer primary key, "body" text not null);`,
|
||||
marker,
|
||||
`INSERT INTO "people_notes" ("id", "body") VALUES (1, 'keep this: ${marker} intact');`,
|
||||
marker,
|
||||
`CREATE FUNCTION people_touch() RETURNS integer AS $$`,
|
||||
`BEGIN`,
|
||||
` -- ${marker}`,
|
||||
` PERFORM 1;`,
|
||||
` RETURN 42;`,
|
||||
`END;`,
|
||||
`$$ LANGUAGE plpgsql;`,
|
||||
marker,
|
||||
`/* block comment with ${marker} */`,
|
||||
`CREATE TABLE "odd${marker}name" ("id" integer);`,
|
||||
].join("\n");
|
||||
const intact = migration("people", 0, { sql, tag: "0000_intact" });
|
||||
|
||||
const result = await applyModuleMigrations({
|
||||
aggregate: aggregate({ id: "people", migrations: [intact] }),
|
||||
database: createPostgresMigrationDatabase(database.url),
|
||||
});
|
||||
expect(result.appliedCount).toBe(1);
|
||||
|
||||
// The literal survived byte-for-byte, so the marker was never treated as a split point.
|
||||
const stored = await safeQuery<{ body: string }>(
|
||||
database.client,
|
||||
`select body from "people_notes" where id = 1`,
|
||||
);
|
||||
expect(stored.rows[0]?.body).toBe(`keep this: ${marker} intact`);
|
||||
|
||||
// The dollar-quoted body executed as one function, not as fragments.
|
||||
const called = await safeQuery<{ result: number }>(
|
||||
database.client,
|
||||
`select people_touch() as result`,
|
||||
);
|
||||
expect(Number(called.rows[0]?.result)).toBe(42);
|
||||
|
||||
// The quoted identifier kept the marker in its name.
|
||||
const relations = await safeQuery<{ table_name: string }>(
|
||||
database.client,
|
||||
`select table_name from information_schema.tables
|
||||
where table_schema = 'public' and table_name like 'odd%'`,
|
||||
);
|
||||
expect(relations.rows.map((row) => row.table_name)).toEqual([`odd${marker}name`]);
|
||||
});
|
||||
});
|
||||
|
||||
it("rolls back the whole multi-statement migration when a later statement fails", async () => {
|
||||
await withIsolatedDatabase("intact_rollback", async (database) => {
|
||||
const failing = migration("people", 0, {
|
||||
sql: [
|
||||
`CREATE TABLE "people_partial" ("id" integer primary key);`,
|
||||
`--> statement-breakpoint`,
|
||||
`INSERT INTO "people_partial" ("id") VALUES (1);`,
|
||||
`--> statement-breakpoint`,
|
||||
`INSERT INTO "people_partial" ("id") VALUES (1);`,
|
||||
].join("\n"),
|
||||
tag: "0000_rollback",
|
||||
});
|
||||
|
||||
const error = await captureRunnerError(
|
||||
applyModuleMigrations({
|
||||
aggregate: aggregate({ id: "people", migrations: [failing] }),
|
||||
database: createPostgresMigrationDatabase(database.url),
|
||||
}),
|
||||
"MIGRATION_EXECUTION_FAILED",
|
||||
);
|
||||
expect(error.migrationIdentity).toBe(failing.identity);
|
||||
|
||||
// The earlier statements in the same batch rolled back with it.
|
||||
const relations = await safeQuery(
|
||||
database.client,
|
||||
`select table_name from information_schema.tables
|
||||
where table_schema = 'public' and table_name = 'people_partial'`,
|
||||
);
|
||||
expect(relations.rows).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -90,6 +90,16 @@ aggregate order, and its history row is inserted only after that migration succe
|
||||
failure rolls back every schema change and history insert from the current invocation. Migrations
|
||||
committed by earlier invocations remain applied.
|
||||
|
||||
> **Partial correction — see [ADR 0020](0020-module-runtime-and-migration-execution-hardening.md).**
|
||||
> Splitting on the literal marker, described in the paragraph immediately below, split a migration
|
||||
> whose SQL legitimately contained those bytes inside a string literal, dollar-quoted body, quoted
|
||||
> identifier, or comment. Core now sends each migration's complete SQL to PostgreSQL in one call
|
||||
> inside the same transaction, so the server does all SQL parsing and the marker lines are ordinary
|
||||
> `--` comments. Everything else recorded here — one transaction across the pending batch, history
|
||||
> rows inserted only after their migration succeeds, no semicolon splitting, no parsing,
|
||||
> classifying, reordering, or repairing SQL, and no non-transactional fallback — is unchanged and
|
||||
> still accepted. This section is left as originally accepted, as the historical record.
|
||||
|
||||
Core matches Drizzle's generated statement-breakpoint behavior: it splits only on the exact
|
||||
literal `--> statement-breakpoint` marker. It never splits SQL on semicolons or tries to parse,
|
||||
classify, reorder, or repair arbitrary SQL. Transaction-incompatible SQL fails the transaction;
|
||||
|
||||
+9
-1
@@ -127,7 +127,15 @@ decides this mapping, and a module has no way to choose or influence its own mou
|
||||
ordering is load-bearing, not cosmetic: Hono's router resolves two routes whose patterns overlap at
|
||||
the same depth (a static segment and a same-position `:parameter`) by registration order, not by
|
||||
static-path specificity, so preserving the runtime's exact publication order is what lets a module
|
||||
author's more-specific-first declaration continue to take precedence after mounting. Mounting uses
|
||||
author's more-specific-first declaration continue to take precedence after mounting.
|
||||
|
||||
> **Partial correction — see [ADR 0020](0020-module-runtime-and-migration-execution-hardening.md).**
|
||||
> Relying on registration order to resolve an overlap let the *permission* guarding an ambiguous
|
||||
> path depend on declaration sequence. Core now rejects any two same-module, same-method routes
|
||||
> that can both match one request path (`MODULE_ROUTE_OVERLAP`) during staged registration, so no
|
||||
> such pair ever reaches mounting and no ordering decides which permission applies. Preserving the
|
||||
> publication order of *disjoint* routes, and everything else in this section, is unchanged and
|
||||
> still accepted. Mounting uses
|
||||
one fixed-arity `app.on(method, path, ...middleware, handler)` call per route rather than spreading
|
||||
a variable-length handler array — Hono's overload resolution is arity-sensitive, and a spread call
|
||||
silently resolves to an unrelated, incompatible overload.
|
||||
|
||||
@@ -0,0 +1,216 @@
|
||||
# 0020: Module runtime and migration execution hardening
|
||||
|
||||
- Status: Accepted
|
||||
- Date: 2026-07-27
|
||||
- Amends: [0009](0009-transactional-module-migrations.md), [0015](0015-static-module-http-routing-and-runtime-lifecycle.md)
|
||||
- Extends: [0017](0017-public-request-perimeter-hardening.md), [0019](0019-durable-authorization-history-and-grant-management.md)
|
||||
|
||||
## Context
|
||||
|
||||
Phase 5D passes 1–3 hardened the public perimeter, the identity/database transport, and
|
||||
authorization integrity. Pass 4 addresses the module runtime and the migration commands underneath
|
||||
them. Seven defects were reproduced against the accepted pass-3 tree before anything changed.
|
||||
|
||||
**Registration order decided which permission governed a request.** A module may register both
|
||||
`GET /events/:id` and `GET /events/new`. Driven through the pinned router, `/events/new` reached the
|
||||
parameter handler with `demo.events.read` when the parameter route registered first, and the static
|
||||
handler with `demo.events.create` when it registered second. Nothing rejected the pair. A permission
|
||||
policy was being decided by array order.
|
||||
|
||||
**Decoded parameters looked safer than they are.** The declaration grammar admits only lowercase
|
||||
kebab-case literals and `:name` segments, but that constrains declarations, not requests. Verified
|
||||
through real requests: `%2F..%2Fadmin` arrives as `/../admin`, `a%5Cb` as `a\b`, `a%00b` with an
|
||||
embedded NUL, `a%0D%0Ab` with CR/LF, `1%3B%20drop` as `1; drop`, and `%C3%A9v%C3%A9nement` as
|
||||
`événement`.
|
||||
|
||||
**A module that failed to start was never cleaned up.** `startModules` appended to its rollback set
|
||||
only *after* `start()` resolved, so a hook that acquired a resource and then threw had its own
|
||||
`stop()` skipped while every previously completed module was rolled back.
|
||||
|
||||
**Nothing bounded shutdown.** `RunningApi.shutdown()` awaited `server.close()` with no deadline, so
|
||||
one open connection could hold it forever, and each module `stop()`/rollback hook was awaited
|
||||
without a deadline, so a hook that never settled prevented pool closure indefinitely.
|
||||
|
||||
**Migration commands had no timeouts.** `migrations:plan` and `migrations:apply` constructed their
|
||||
connection with neither a connection deadline nor an operation deadline; a real server confirmed
|
||||
`statement_timeout = 0` on the migration connection.
|
||||
|
||||
**Migration SQL was split on literal bytes.** The runner did
|
||||
`migration.sql.split("--> statement-breakpoint")`. Those bytes inside a string literal, a
|
||||
dollar-quoted body, a quoted identifier, or a comment split the migration into fragments that were
|
||||
no longer valid SQL — each verified to produce two fragments where one was required.
|
||||
|
||||
## Decision
|
||||
|
||||
### Overlapping same-module routes are rejected, not ranked
|
||||
|
||||
Two routes of one module overlap when they share an HTTP method and a segment count, and at every
|
||||
position either both segments are the same static literal or at least one is a parameter. Under
|
||||
this grammar — static literals or `:name` parameters, with no wildcard, regex, optional, or
|
||||
catch-all segment — that condition is exactly "some concrete request path matches both".
|
||||
|
||||
`moduleRoutesOverlap` (`packages/contracts/src/module-http.ts`) implements it, kept framework-neutral
|
||||
because path semantics already live in that package and Hono stays confined to `apps/api`. Core's
|
||||
staged registration checks it after the existing duplicate check, so identical shapes — including
|
||||
paths differing only in parameter name — keep returning `MODULE_ROUTE_DUPLICATE`, and a genuine
|
||||
non-duplicate intersection returns the new `MODULE_ROUTE_OVERLAP`. Detection happens during the
|
||||
registering module's staged registration, so the module publishes **none** of its staged routes or
|
||||
capabilities, no start hook runs, and the HTTP listener never binds.
|
||||
|
||||
Rejection rather than specificity ordering is the point. A specificity rule ("static beats
|
||||
parameter") would silently pick a winner, and the thing being picked is which permission guards the
|
||||
path — that is a policy decision, not a routing preference, and it must not be inferred. Disjoint
|
||||
routes keep their registration and publication order untouched; different methods, different
|
||||
modules (each has its own platform-owned mount prefix), and different segment counts remain legal.
|
||||
No priority, specificity, wildcard, regex, catch-all, optional-segment, mount-prefix, or manifest
|
||||
field was added.
|
||||
|
||||
### Decoded route parameters are untrusted, module-owned input
|
||||
|
||||
`context.params` continues to carry the router's decoded string values in a frozen context.
|
||||
Decoding is not validation, and validating a *declaration* says nothing about what a *request* may
|
||||
send. Every handler must validate each parameter it uses against its own exact domain grammar —
|
||||
UUID, slug, date, numeric id, or whatever that module owns — before any database, filesystem,
|
||||
command, identifier, or other side-effecting use, and reject missing, repeated, malformed, or
|
||||
semantically invalid values with its own sanitized response. Re-encoding or a decode/re-encode round
|
||||
trip is not a substitute.
|
||||
|
||||
No generic parameter grammar, validator service, path-normalization layer, or new route field was
|
||||
added: module parameters are module-owned domain types, and one platform-wide pattern would either
|
||||
reject legitimate values or approve meaningless ones. The platform keeps owning authentication,
|
||||
authorization, `Origin`, CORS, body limits, cache behavior, and uncaught-handler sanitization, and
|
||||
permission enforcement always completes before a handler — and therefore any parameter validation —
|
||||
can run.
|
||||
|
||||
### A module that fails to start rolls itself back first
|
||||
|
||||
A module joins the rollback set the moment its `start` hook is *invoked*. Its own `stop` hook runs
|
||||
first, then previously completed modules in reverse order. The original start issue stays first in
|
||||
`ModuleRuntimeError.issues`; a failure or timeout stopping the failing module is appended and never
|
||||
prevents earlier modules from being rolled back. Every applicable hook is invoked at most once, a
|
||||
runtime that fails to start is never returned, and every hook receives the same frozen
|
||||
`ModuleRuntimeServices` identity its start hook received. A module with no stop hook gets no
|
||||
synthetic callback.
|
||||
|
||||
The consequence for module authors is explicit: **`stop()` may run after a partially completed
|
||||
`start()`** and must be safe and idempotent about resources it may or may not have acquired. The
|
||||
platform cannot undo arbitrary side effects a trusted module performed outside its stop hook, and
|
||||
does not claim to.
|
||||
|
||||
### Fixed, finite deadlines for hooks and for HTTP drain
|
||||
|
||||
- Per-module `stop`/rollback hook: `MODULE_STOP_HOOK_TIMEOUT_MS` = **10 000 ms**.
|
||||
- Graceful HTTP drain: `HTTP_DRAIN_TIMEOUT_MS` = **30 000 ms**.
|
||||
|
||||
Neither is configurable in this pass — no environment variable, manifest field, runtime service, or
|
||||
flag. Narrow test-only overrides exist so tests need not wait out production durations.
|
||||
|
||||
A hook that reaches its deadline records `MODULE_STOP_TIMEOUT` or `MODULE_ROLLBACK_TIMEOUT` and the
|
||||
sequence continues with the next module in reverse order. The whole module-stop sequence is finite
|
||||
because the module count is fixed at discovery: its worst case is
|
||||
`startedModuleCount × 10 000 ms`, **not** one global ten-second bound. Timers that lose the race are
|
||||
cleared, the losing promise gets a permanent rejection handler so a late throw can never become an
|
||||
unhandled rejection, and a timed-out hook is never awaited again. `stop()` remains at-most-once and
|
||||
memoizes one shared promise for concurrent and later callers.
|
||||
|
||||
Shutdown stops accepting connections with `server.close()`, waits up to the drain deadline, and at
|
||||
the deadline closes idle and then active connections through Node's own public
|
||||
`closeIdleConnections`/`closeAllConnections` — after `server.close()`, never touching a private
|
||||
socket or server field — and stops waiting for the close callback. It then runs the bounded module
|
||||
stop sequence and closes the one shared pool. The accepted order
|
||||
`HTTP drain/forced close → bounded module stop → pool close` is unchanged, and a drain timeout or
|
||||
module-stop failure never skips later cleanup. Both outcomes are reported together as
|
||||
`ApiShutdownError`, carrying fixed booleans and the already-sanitized module error — never a
|
||||
request, socket, URL, credential, SQL, or stack detail. A forced drain is a shutdown *failure*, not
|
||||
a success; signal shutdown exits `0` only after a clean shutdown and `1` otherwise, after cleanup
|
||||
has been attempted.
|
||||
|
||||
**A deadline bounds waiting, not execution.** An in-process promise cannot be canceled: a timed-out
|
||||
hook keeps running and may settle later, and force-closing a connection ends the transport without
|
||||
proving that arbitrary handler JavaScript was canceled. What is guaranteed is that shutdown no
|
||||
longer waits, and that the pool closes afterwards, so late database work fails. Graceful ordering
|
||||
holds until the drain deadline; after it, the platform closes connections and proceeds so process
|
||||
shutdown stays bounded.
|
||||
|
||||
### Finite migration connection and operation defaults, enforced against driver precedence
|
||||
|
||||
`createPostgresMigrationDatabase` — the one construction boundary every migration command shares —
|
||||
now defaults to `MIGRATION_CONNECTION_TIMEOUT_MS` = **10 000 ms** (`connectionTimeoutMillis`) and
|
||||
`MIGRATION_OPERATION_TIMEOUT_MS` = **10 000 ms** for both `query_timeout` (client-side) and
|
||||
`statement_timeout` (server-side). Defaults live there so a future CLI call site cannot silently
|
||||
omit them; explicit test overrides may still narrow them. History reads, advisory-lock queries,
|
||||
transaction control, migration SQL, and history writes are all covered. The existing migration
|
||||
lock-wait deadline and error codes, and the Pass 2 TLS/hostname/certificate policy, are unchanged,
|
||||
as are Doctor's and the bootstrap CLI's own timeouts. No timeout environment variable or flag was
|
||||
added.
|
||||
|
||||
Explicit constructor fields do **not** win under the pinned driver. `[email protected]` builds parameters as
|
||||
`Object.assign({}, config, parse(config.connectionString))`, so the connection string overrides
|
||||
passed options — verified directly: an explicit `statement_timeout: 10000` alongside
|
||||
`?statement_timeout=0` produced a server-reported `statement_timeout` of `0`, while a clean URL
|
||||
produced `10s`. A `DATABASE_URL` carrying `connect_timeout`, `query_timeout`, `statement_timeout`,
|
||||
or an `options` value that sets `statement_timeout` is therefore refused with the existing fixed
|
||||
`POSTGRES_MIGRATION_CONFIGURATION_INVALID`, before any client is constructed and without echoing the
|
||||
URL, parameter, or value. The rejection is deliberately narrow — only settings that actually conflict
|
||||
with the selected policy; unrelated parameters are untouched.
|
||||
|
||||
### Each migration is one PostgreSQL-parsed batch
|
||||
|
||||
The runner no longer splits migration SQL. Each pending migration's exact string is passed once to
|
||||
the client — no values, no prepared-statement name, no trimming, normalizing, or rewriting — inside
|
||||
the existing transaction, and its history row is inserted only after that batch succeeds. One
|
||||
transaction still spans all pending migrations and their history records, and advisory locking,
|
||||
checksum validation, approval preflight, ordering, rollback, cleanup, and sanitized error identity
|
||||
are unchanged. The transaction port method was renamed `executeStatement` → `executeMigrationSql` so
|
||||
the contract states what it actually receives.
|
||||
|
||||
PostgreSQL was chosen as the parser because it is the only correct one. A homegrown splitter would
|
||||
need to track single quotes, dollar quoting with arbitrary tags, quoted identifiers, line comments,
|
||||
block-comment nesting, and escape syntax — reimplementing a SQL lexer to answer a question the
|
||||
server already answers — and a parser dependency would add supply-chain surface for the same
|
||||
reason. Drizzle's `--> statement-breakpoint` lines are ordinary `--` comments when the batch is sent
|
||||
whole, so the same bytes inside a literal, dollar-quoted body, quoted identifier, or comment retain
|
||||
their ordinary PostgreSQL meaning. **No custom SQL parser exists in this repository.**
|
||||
|
||||
## Alternatives
|
||||
|
||||
- **Ordering static routes ahead of parameter routes** was rejected: it silently resolves an
|
||||
ambiguity whose resolution is a permission decision, and it would make the governing permission a
|
||||
function of an implementation detail rather than of a declaration.
|
||||
- **A platform-wide route-parameter grammar** was rejected: parameters are module-owned domain
|
||||
types, so any single pattern is simultaneously too strict and too permissive.
|
||||
- **Normalizing or re-encoding decoded parameters** was rejected: it produces a value the module
|
||||
never declared and still does not answer whether the value is a valid domain identifier.
|
||||
- **A second, dedicated rollback hook** was rejected: it would double the lifecycle surface to
|
||||
express something `stop` already means, and every module would have to implement both.
|
||||
- **Making the deadlines configurable** was rejected for this pass: a configurable shutdown bound
|
||||
invites setting it to infinity, which is the defect being fixed.
|
||||
- **Claiming a timed-out hook is terminated** was rejected as untrue; the honest guarantee is that
|
||||
the platform stops waiting.
|
||||
- **Trusting explicit `pg` client fields over the connection string** was rejected once the pinned
|
||||
driver was shown to do the opposite.
|
||||
- **A timeout environment variable** was rejected as out of scope; a fixed default that cannot be
|
||||
weakened is the safer starting point.
|
||||
- **Writing a SQL statement splitter, or adding a SQL parser dependency** — see above.
|
||||
|
||||
## Consequences
|
||||
|
||||
An ambiguous same-method route pair fails module registration with a stable code instead of
|
||||
resolving by array order, and no request can reach a match whose permission depended on registration
|
||||
sequence. Decoded parameters are documented as untrusted input that handlers must validate, with no
|
||||
platform grammar implied. A module whose `start` fails is cleaned up through its own `stop` hook
|
||||
before earlier modules. Shutdown is finite in every stage, observable when it is not clean, and
|
||||
always reaches pool closure. Migration commands have finite connection, query, and statement
|
||||
deadlines that a connection string cannot weaken, and each migration reaches PostgreSQL byte-for-byte
|
||||
in a single parsed batch.
|
||||
|
||||
This pass adds no production schema, migration, manifest field, `manifestVersion` change,
|
||||
`ModuleRuntimeServices` field, `ModuleDatabase` operation, route-declaration field, handler-context
|
||||
field, second runtime, route registry, or runtime pool; the three accepted Core migrations and their
|
||||
checksums are byte-identical, one API runtime still owns one physical pool, migrations remain
|
||||
explicit operator work that never runs at application startup, and Hono remains confined to
|
||||
`apps/api`. Modules remain trusted compiled backend code: they are **not** isolated by process,
|
||||
PostgreSQL role, schema, or tenant, and these deadlines do not sandbox them.
|
||||
|
||||
**Phase 5D is not complete.** Pass 5 — deployment/supply-chain work and an independent
|
||||
whole-repository re-audit — remains outstanding, and Phase 6 remains blocked.
|
||||
@@ -28,3 +28,4 @@ ADRs capture durable choices that affect more than one package or phase.
|
||||
- [0017: Public request perimeter hardening — client address, signup gating, body limits, timeouts](0017-public-request-perimeter-hardening.md) — Accepted
|
||||
- [0018: Identity session resolution, canonical public origins, and PostgreSQL transport policy](0018-identity-session-resolution-and-database-transport.md) — Accepted
|
||||
- [0019: Durable authorization history and grant management](0019-durable-authorization-history-and-grant-management.md) — Accepted
|
||||
- [0020: Module runtime and migration execution hardening](0020-module-runtime-and-migration-execution-hardening.md) — Accepted
|
||||
|
||||
@@ -585,8 +585,9 @@ failure propagates: module-services/identity/authorization construction failing
|
||||
was started yet — the frozen `ModuleRuntimeServices` object is built in the same step, from the
|
||||
same pool); the server failing to bind its listen address; or an app-construction failure, both of
|
||||
which also stop the module runtime first, since it may have already started successfully. A module
|
||||
load/register/start failure uses Core's own existing rollback (already-started modules stop, in
|
||||
reverse order, before the failure propagates) and then closes the pool — there is no separate
|
||||
load/register/start failure uses Core's own rollback — the module whose `start` hook failed is
|
||||
stopped first, through its own `stop` hook, then already-started modules in reverse order, each
|
||||
under the same bounded per-hook deadline — before the failure propagates, and then closes the pool — there is no separate
|
||||
runtime handle to stop in that case, since `startDiscoveredModuleRuntime` never returned one.
|
||||
|
||||
## A real signup → sign-in → principal → sign-out transcript
|
||||
|
||||
@@ -68,7 +68,7 @@ under the reserved source identifier `"core"`, and
|
||||
aggregate, **Core first**. An installed module manifest that ever declares `id: "core"` is rejected
|
||||
at discovery time so Core and module migration identities can never collide.
|
||||
|
||||
The transactional runner, advisory lock, breakpoint splitter, and the fixed
|
||||
The transactional runner, advisory lock, whole-batch SQL execution, and the fixed
|
||||
`public.ekklesia_core_migrations` history table (ADR 0009) are unchanged: Core's migration honestly
|
||||
populates the existing `module_id` column with the literal value `"core"`, exactly like a module
|
||||
populates it with its own module ID. There is exactly one migration system and one executor.
|
||||
@@ -82,6 +82,21 @@ pnpm migrations:apply # locked, transactional application of the pending bat
|
||||
pnpm doctor # includes Core's migration in its discovery/plan checks
|
||||
```
|
||||
|
||||
Both `migrations:plan` and `migrations:apply` connect with fixed, finite defaults — a 10-second
|
||||
connection deadline, a 10-second client query timeout, and a 10-second PostgreSQL
|
||||
`statement_timeout` — applied at the one shared construction boundary so no command can omit them.
|
||||
A `DATABASE_URL` that tries to set `connect_timeout`, `query_timeout`, `statement_timeout`, or an
|
||||
`options` value carrying `statement_timeout` is refused with a sanitized configuration error,
|
||||
because the pinned driver lets the connection string override explicitly passed options. This bounds
|
||||
each individual operation; it does **not** promise that a migration completes within ten seconds
|
||||
across retries or cleanup. Migrations remain explicit operator work and never run at application
|
||||
startup.
|
||||
|
||||
Each pending migration's SQL is sent to PostgreSQL as **one complete batch**, inside the existing
|
||||
transaction, so the server does all SQL parsing — Drizzle's `--> statement-breakpoint` lines are
|
||||
ordinary `--` comments to it. No SQL parser exists in this repository, and the marker's bytes keep
|
||||
their ordinary meaning inside a string literal, dollar-quoted body, quoted identifier, or comment.
|
||||
|
||||
No new commands are required to include identity: it flows through the existing migration
|
||||
commands automatically. A fresh database applies the Core identity migration through
|
||||
`pnpm migrations:apply` like any other pending migration; a second application is a genuine no-op.
|
||||
|
||||
@@ -152,6 +152,29 @@ key material is ever committed to this repository** — and the disposable conta
|
||||
`afterAll` even when an assertion fails. See
|
||||
[ADR 0018](../architecture/decisions/0018-identity-session-resolution-and-database-transport.md).
|
||||
|
||||
### Which kind of timing proof lives where
|
||||
|
||||
Pass 4 introduced several deadlines, and their tests deliberately differ in kind:
|
||||
|
||||
- **Unit deadline tests** (`core/tests/module-runtime.test.ts`) drive module stop and rollback
|
||||
hooks through a short, test-only `hookTimeoutMs` override with manually controlled
|
||||
never-settling promises — no real waiting, no sleeps, and an explicit `unhandledRejection` check
|
||||
proving a hook that rejects after its deadline stays handled.
|
||||
- **Real raw-HTTP force-close** (`apps/api/tests/api-http.integration.ts`) binds the real listener,
|
||||
opens a real socket, writes a deliberately incomplete request, and proves shutdown force-closes
|
||||
it at a short `drainTimeoutMsOverride` and still reaches pool closure.
|
||||
- **Real TCP connection timeout** (`core/tests/postgres-migration.integration.ts`) binds a loopback
|
||||
blackhole listener that accepts the socket and never completes PostgreSQL startup, proving the
|
||||
configured connection deadline; the listener and every accepted socket close in `finally`.
|
||||
- **Real PostgreSQL operation timeout and SQL batching** (same file) use a bounded server-side
|
||||
delay only because the deadline itself is what is under test, and disposable databases to prove
|
||||
that marker bytes inside literals, dollar-quoted bodies, comments, and quoted identifiers execute
|
||||
intact, and that a failure rolls the whole batch back.
|
||||
|
||||
Every one of these uses a short **test-only override**; the production defaults (10 s per module
|
||||
hook, 30 s HTTP drain, 10 s migration connection/query/statement) are asserted separately as
|
||||
constants and are never waited out.
|
||||
|
||||
CI runs the same command in the dedicated `migration-smoke` job. After this change is merged, add
|
||||
the following pull-request status context to `main` branch protection:
|
||||
|
||||
|
||||
@@ -180,6 +180,21 @@ Entrypoint imports must have no top-level side effects. Modules are trusted back
|
||||
evaluation cannot be rolled back. Keep registration declarative; open resources in `start` and
|
||||
close them in `stop`.
|
||||
|
||||
### `stop()` may run after a partially completed `start()`
|
||||
|
||||
If your `start` hook throws, **your own `stop` hook runs first**, ahead of the previously started
|
||||
modules being rolled back in reverse order. That is deliberate: a hook that opened a connection,
|
||||
registered a listener, or recorded partial initialization before failing is the only code that knows
|
||||
how to release it. Write `stop` so it is safe and idempotent about resources it may or may not have
|
||||
acquired — guard on what you actually took, and never assume `start` completed.
|
||||
|
||||
Stop and rollback hooks have finite time: the platform waits at most **10 seconds** per module hook,
|
||||
then records a timeout and continues with the next module in reverse order. Settle promptly. A hook
|
||||
that exceeds its deadline is not killed — the platform simply stops waiting for it — but the shared
|
||||
PostgreSQL pool closes after the bounded stop sequence, so **a timed-out hook cannot rely on
|
||||
database availability afterwards**, and any late query will fail. The platform cannot undo side
|
||||
effects your module performed outside its stop hook.
|
||||
|
||||
## HTTP routes
|
||||
|
||||
A module registers plain Fetch (`Request` in, `Response` out) route handlers during `register`,
|
||||
@@ -219,6 +234,65 @@ registration outright rather than being ignored. `method` is one of `GET`, `POST
|
||||
`DELETE`, always uppercase; there is no `OPTIONS` (the platform owns CORS preflight) and no
|
||||
wildcard/all-method registration.
|
||||
|
||||
### Two routes may not both match one request path
|
||||
|
||||
Within one module and one HTTP method, two route patterns must never be able to match the same
|
||||
request path. `/events/:id` and `/events/new` can — `/events/new` matches both — and that pair is
|
||||
rejected during registration with `MODULE_ROUTE_OVERLAP`. So are `/events/:id/edit` against
|
||||
`/events/new/:action`, and `/a/:left/c` against `/a/b/:right`. Two identical shapes, including paths
|
||||
differing only in parameter name (`/events/:id` and `/events/:eventId`), keep the existing
|
||||
`MODULE_ROUTE_DUPLICATE` result.
|
||||
|
||||
This is rejected rather than resolved by a "static beats parameter" rule because the routes may
|
||||
carry *different permissions*, and letting declaration order decide which one guards `/events/new`
|
||||
would make a permission policy depend on the order lines appear in your `register` hook. A rejected
|
||||
registration publishes **none** of that module's routes or capabilities and runs no start hook, so
|
||||
the failure is loud at startup rather than subtle at request time.
|
||||
|
||||
Still perfectly legal: two different static literals (`/events/new` and `/events/archive`),
|
||||
different segment counts (`/events/:id` and `/events/:id/edit`), the same path under different
|
||||
methods, and the same local path in two different modules — each module has its own
|
||||
platform-owned mount prefix. Declaration order among disjoint routes is preserved exactly.
|
||||
|
||||
### Route parameters are decoded, untrusted input — validate them yourself
|
||||
|
||||
`context.params` holds the router's **decoded** path values. Decoding is not validation, and the
|
||||
fact that your *declared* path passed the platform's path grammar says nothing about what a caller
|
||||
may *send*. A single encoded segment routinely carries characters no declared path could:
|
||||
|
||||
| Request segment | `context.params` value |
|
||||
| --- | --- |
|
||||
| `%2F..%2Fadmin` | `/../admin` |
|
||||
| `a%5Cb` | `a\b` |
|
||||
| `a%00b` | `a`, NUL, `b` |
|
||||
| `a%0D%0Ab` | `a`, CR, LF, `b` |
|
||||
| `1%3B%20drop` | `1; drop` |
|
||||
| `%C3%A9v%C3%A9nement` | `événement` |
|
||||
|
||||
**Validate every parameter you use against your own exact domain grammar** — a UUID, slug, date,
|
||||
numeric identifier, or whatever your module owns — before any database, filesystem, command,
|
||||
identifier, or other side-effecting use, and reject a missing, repeated, malformed, or semantically
|
||||
invalid value with your own sanitized response:
|
||||
|
||||
```ts
|
||||
const eventId = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u;
|
||||
|
||||
async handler(request, context) {
|
||||
const id = context.params.eventId;
|
||||
if (id === undefined || !eventId.test(id)) {
|
||||
return Response.json({ code: "INVALID_REQUEST" }, { status: 400 });
|
||||
}
|
||||
// ...only now is `id` safe to use as a domain identifier.
|
||||
}
|
||||
```
|
||||
|
||||
Re-encoding the value, or a decode/re-encode round trip, is **not** a substitute for that check. The
|
||||
platform deliberately supplies no generic parameter validator: your parameters are your domain
|
||||
types, and one platform-wide pattern would either reject legitimate values or approve meaningless
|
||||
ones. The platform continues to own authentication, authorization, `Origin`, CORS, body limits,
|
||||
cache behavior, and uncaught-handler sanitization, and permission enforcement always completes
|
||||
before your handler runs — an unauthorized caller can never reach your validation at all.
|
||||
|
||||
`maxBodyBytes` is a conscious, required request-body contract, not an optional or defaulted field:
|
||||
`0` means the route accepts no body at all, and a non-empty declared (`Content-Length`) or streamed
|
||||
body is rejected before the handler ever runs; any other value is the exact maximum number of bytes
|
||||
|
||||
@@ -141,6 +141,49 @@ export function moduleRouteShapeKey(method: ModuleHttpMethod, path: ModuleRouteP
|
||||
return `${method} /${shapeSegments.join("/")}`;
|
||||
}
|
||||
|
||||
/** `"/"` is the module root and has no segments; every other canonical path is its `/`-separated
|
||||
* segments. Takes an already-validated `ModuleRoutePath`. */
|
||||
function moduleRoutePathSegments(path: ModuleRoutePath): readonly string[] {
|
||||
return path === "/" ? [] : path.slice(1).split("/");
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether two routes of the same module can both match at least one request path.
|
||||
*
|
||||
* Two routes overlap when they share an HTTP method and a segment count, and at every position
|
||||
* either both segments are the same static literal or at least one is a parameter. Under this
|
||||
* route grammar — static kebab-case literals or `:name` parameters, with no wildcard, regex,
|
||||
* optional, or catch-all segment — that condition is exactly "some concrete path matches both".
|
||||
*
|
||||
* This exists because an overlap is a *permission* ambiguity, not a routing preference:
|
||||
* `GET /events/:id` and `GET /events/new` can carry different permissions, and with an
|
||||
* order-sensitive matcher whichever registered first decides which permission governs
|
||||
* `/events/new`. Core rejects the overlap at registration instead of picking a winner, so no
|
||||
* router's matching order can ever select the governing permission. Identical shapes (including
|
||||
* paths differing only in parameter name) remain the pre-existing duplicate result; callers check
|
||||
* that first.
|
||||
*
|
||||
* Deliberately framework-neutral: path semantics already live in this package, and nothing about
|
||||
* this rule may depend on Hono, which stays confined to `apps/api`.
|
||||
*/
|
||||
export function moduleRoutesOverlap(
|
||||
first: { readonly method: ModuleHttpMethod; readonly path: ModuleRoutePath },
|
||||
second: { readonly method: ModuleHttpMethod; readonly path: ModuleRoutePath },
|
||||
): boolean {
|
||||
if (first.method !== second.method) {
|
||||
return false;
|
||||
}
|
||||
const firstSegments = moduleRoutePathSegments(first.path);
|
||||
const secondSegments = moduleRoutePathSegments(second.path);
|
||||
if (firstSegments.length !== secondSegments.length) {
|
||||
return false;
|
||||
}
|
||||
return firstSegments.every((segment, index) => {
|
||||
const other = secondSegments[index] as string;
|
||||
return segment.startsWith(":") || other.startsWith(":") || segment === other;
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* What a module's route handler receives about the caller, the match, and the platform services
|
||||
* available to it — never an authorization-store, grant-management, storage, jobs, configuration,
|
||||
@@ -149,6 +192,28 @@ export function moduleRouteShapeKey(method: ModuleHttpMethod, path: ModuleRouteP
|
||||
* before ever invoking its handler; this contracts-layer module never invokes a handler itself.
|
||||
* `services` is the same frozen `ModuleRuntimeServices` object the owning module's `start`/`stop`
|
||||
* hooks receive for this API runtime — never a fresh one built per request.
|
||||
*
|
||||
* ## `params` values are untrusted input
|
||||
*
|
||||
* `params` holds the HTTP router's **decoded** path values, frozen alongside this context. Decoding
|
||||
* is not validation, and the declaration grammar above constrains only what a module may *declare*
|
||||
* — it says nothing about what a request may *send*. A caller can percent-encode anything into a
|
||||
* single segment, so a decoded value routinely contains characters no declared path could:
|
||||
* verified against the pinned router, `%2F..%2Fadmin` arrives as `/../admin`, `a%5Cb` as `a\b`,
|
||||
* `a%00b` with an embedded NUL, `a%0D%0Ab` with CR/LF, and `%C3%A9v%C3%A9nement` as `événement`.
|
||||
*
|
||||
* Every handler must therefore validate each parameter it uses against its own exact domain
|
||||
* grammar — a UUID, slug, date, numeric id, or whatever that module owns — **before** any
|
||||
* database, filesystem, command, identifier, or other side-effecting use, and reject a missing,
|
||||
* repeated, malformed, or semantically invalid value with its own sanitized response. Re-encoding,
|
||||
* decode/re-encode round-tripping, or inspecting the raw URL is not a substitute for that check.
|
||||
* The platform deliberately supplies no generic parameter grammar: module parameters are
|
||||
* module-owned domain types, and one platform-wide pattern would either reject legitimate values
|
||||
* or approve meaningless ones.
|
||||
*
|
||||
* The platform continues to own authentication, authorization, `Origin`, CORS, body limits, cache
|
||||
* behavior, and uncaught-handler sanitization; permission enforcement always completes before a
|
||||
* handler — and therefore any parameter validation it performs — can run.
|
||||
*/
|
||||
export interface ModuleRouteHandlerContext {
|
||||
readonly moduleId: ModuleId;
|
||||
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
type ModuleRouteDefinitionInput,
|
||||
moduleRoutePathSchema,
|
||||
moduleRouteShapeKey,
|
||||
moduleRoutesOverlap,
|
||||
parseModuleRouteDefinition,
|
||||
parseModuleRoutePath,
|
||||
safeParseModuleRouteDefinition,
|
||||
@@ -294,3 +295,42 @@ describe("parseModuleRouteDefinition", () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("moduleRoutesOverlap", () => {
|
||||
const route = (method: "GET" | "POST", path: string) => ({
|
||||
method,
|
||||
path: parseModuleRoutePath(path),
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a parameter against a static literal", "/events/:id", "/events/new"],
|
||||
["a later position", "/events/:id/edit", "/events/new/:action"],
|
||||
["parameters on both sides at different positions", "/a/:left/c", "/a/b/:right"],
|
||||
["identical parameter shapes with different names", "/events/:id", "/events/:eventId"],
|
||||
["identical static paths", "/events/new", "/events/new"],
|
||||
["root against root", "/", "/"],
|
||||
])("reports an overlap for %s", (_label, first, second) => {
|
||||
expect(moduleRoutesOverlap(route("GET", first), route("GET", second))).toBe(true);
|
||||
// The relation is symmetric, so registration order cannot change the verdict.
|
||||
expect(moduleRoutesOverlap(route("GET", second), route("GET", first))).toBe(true);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["two different static literals", "/events/new", "/events/archive"],
|
||||
["different segment counts", "/events/:id", "/events/:id/edit"],
|
||||
["a static prefix that differs earlier", "/events/:id", "/people/:id"],
|
||||
["root against a one-segment path", "/", "/events"],
|
||||
])("reports no overlap for %s", (_label, first, second) => {
|
||||
expect(moduleRoutesOverlap(route("GET", first), route("GET", second))).toBe(false);
|
||||
expect(moduleRoutesOverlap(route("GET", second), route("GET", first))).toBe(false);
|
||||
});
|
||||
|
||||
it("never reports an overlap across different methods", () => {
|
||||
expect(moduleRoutesOverlap(route("GET", "/events/:id"), route("POST", "/events/new"))).toBe(
|
||||
false,
|
||||
);
|
||||
expect(moduleRoutesOverlap(route("GET", "/events/new"), route("POST", "/events/new"))).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user