3 Commits
Author SHA1 Message Date
ekklesiaandClaude Opus 5 2983789779 fix: harden migration command execution
Two problems in the migration commands: no timeouts, and SQL split on literal
bytes.

`migrations:plan` and `migrations:apply` constructed their connection with
neither a connection deadline nor an operation deadline — a real server reported
`statement_timeout = 0` on the migration connection. Both now default to 10
seconds for the connection deadline, the client query timeout, and PostgreSQL's
own statement_timeout, applied at the one shared construction boundary so a
future call site cannot silently omit them. History reads, advisory-lock
queries, transaction control, migration SQL, and history writes are all covered.
The existing lock-wait deadline, the Pass 2 TLS policy, and Doctor's and the
bootstrap CLI's own timeouts are untouched, and no environment variable or flag
was added.

Explicit client fields do not win under the pinned driver. [email protected].0 builds
parameters as `Object.assign({}, config, parse(config.connectionString))`, so
the URL overrides passed options — verified directly, where an explicit
statement_timeout of 10000 alongside `?statement_timeout=0` produced a
server-reported 0, while a clean URL produced 10s. A DATABASE_URL carrying
connect_timeout, query_timeout, statement_timeout, or an `options` value setting
statement_timeout is therefore refused with the existing fixed configuration
error, before any client is constructed and without echoing the URL, parameter,
or value. Unrelated parameters are left alone.

The runner split each migration on the literal `--> statement-breakpoint` bytes.
Those bytes inside a string literal, a dollar-quoted function body, a quoted
identifier, or a comment tore the migration into fragments that were no longer
valid SQL — each case verified to produce two fragments where one was required.
Each migration's exact SQL is now passed to the client once, with no values and
no prepared-statement name, inside the existing transaction, and its history row
inserted only after that batch succeeds. One transaction still spans all pending
migrations; advisory locking, checksum validation, approval preflight, ordering,
rollback, cleanup, and sanitized error identity are unchanged. The port method
is renamed executeStatement to executeMigrationSql so the contract says what it
receives.

PostgreSQL parses the batch because it owns the only correct parser. A homegrown
splitter would have to track single quotes, dollar quoting with arbitrary tags,
quoted identifiers, line comments, nested block comments, and escape syntax to
answer a question the server already answers, and a parser dependency would add
supply-chain surface for the same reason. Drizzle's marker lines are ordinary
`--` comments when the batch is sent whole. No custom SQL parser exists here.

Co-Authored-By: Claude Opus 5 <[email protected]>
2026-07-26 23:13:52 -05:00
ekklesiaandClaude Opus 5 490b85b07c fix: bound module lifecycle shutdown
Nothing bounded shutdown, and 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 connection, registered a listener, or recorded partial
initialization and then threw had its own `stop()` skipped, while every
previously completed module was rolled back correctly. A module now joins the
rollback set the moment its start hook is invoked: its own stop hook runs first,
then earlier modules in reverse order. The original start issue stays first in
the reported issues, and a failure stopping the failing module is appended
without preventing the rest of the rollback. The consequence for module authors
is explicit and documented — `stop()` may run after a partially completed
`start()` and must be safe and idempotent about resources it may or may not
hold.

Neither stage of shutdown had a deadline. `shutdown()` awaited `server.close()`
with no bound, so one open connection could hold it forever, and each stop or
rollback hook was awaited without a bound, so a hook that never settled kept the
pool open indefinitely. Both now have fixed, non-configurable defaults: 10
seconds per module hook and 30 seconds for the graceful HTTP drain. A hook that
reaches its deadline records MODULE_STOP_TIMEOUT or MODULE_ROLLBACK_TIMEOUT and
the sequence continues with the next module, so the whole stop sequence is
finite in the started module count rather than one global ten-second bound.
Timers that lose the race are cleared, the losing promise gets a permanent
rejection handler so a late throw cannot become an unhandled rejection, and a
timed-out hook is never awaited again.

At the drain deadline the platform closes idle and then active connections
through Node's own public closeIdleConnections/closeAllConnections, after
server.close(), touching no private field, and stops waiting for the close
callback. It then runs the bounded module stop sequence and closes the pool, so
a drain timeout or a stop failure never skips later cleanup. Both are reported
together as an ApiShutdownError carrying only fixed booleans and the already
sanitized module error; a forced drain is a shutdown failure, not a success.

Stated honestly rather than overstated: a deadline bounds how long the platform
waits, not how long module code runs. An in-process promise cannot be canceled,
and closing a socket does not prove a handler promise ended. What is guaranteed
is that shutdown stops waiting and the pool closes afterwards, so late database
work fails.

Co-Authored-By: Claude Opus 5 <[email protected]>
2026-07-26 23:12:18 -05:00
ekklesiaandClaude Opus 5 800ecfdb92 fix: reject ambiguous module routes
A module can register both `GET /events/:id` and `GET /events/new`. Driven
through the pinned router, `/events/new` reached the parameter handler under
`demo.events.read` when the parameter route registered first, and the static
handler under `demo.events.create` when it registered second. Nothing rejected
the pair, so declaration order decided which permission guarded the path. That
is a permission policy, not a routing preference, and it must not be inferred
from array position.

Two routes of one module now overlap when they share a 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, no wildcard, regex, optional, or catch-all — that is exactly "some
concrete request path matches both". `moduleRoutesOverlap` implements it in the
contracts package, where path semantics already live and Hono cannot reach.

Core checks it during staged registration, after the existing duplicate check,
so identical shapes (including paths differing only in parameter name) keep
returning MODULE_ROUTE_DUPLICATE and a genuine intersection returns the new
MODULE_ROUTE_OVERLAP. The rejected module publishes none of its staged routes or
capabilities, no start hook runs, and no listener binds. Rejection rather than a
specificity rule: ranking static above parameter would still silently pick which
permission applies. Disjoint routes keep their order; different methods,
different segment counts, and the same local path in two different modules
(each has its own platform-owned mount prefix) all remain legal.

The same requests also pinned down what handlers actually receive. Route
declarations are validated; route *parameters* are not, and the two are
unrelated. A single encoded segment arrives decoded as `/../admin`, `a\b`, an
embedded NUL, CR/LF, `1; drop`, or non-ASCII text — none of which any declared
path could contain. The contract now says so explicitly: decoded values are
untrusted, module-owned input that a handler must validate against its own exact
domain grammar before any database, filesystem, command, or identifier use, and
re-encoding is not a substitute. No generic validator, path-normalization layer,
or route field was added, because module parameters are module-owned domain
types and one platform-wide pattern would either reject legitimate values or
approve meaningless ones.

Co-Authored-By: Claude Opus 5 <[email protected]>
2026-07-26 23:10:51 -05:00
23 changed files with 1777 additions and 58 deletions
+26
View File
@@ -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
View File
@@ -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,
);
}
})();
}
+116
View File
@@ -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);
}
});
});
+10 -1
View File
@@ -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.
+151
View File
@@ -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);
});
});
+86 -23
View File
@@ -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.");
}
}
+14 -5
View File
@@ -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
View File
@@ -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 }),
});
}
+45 -6
View File
@@ -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);
+335
View File
@@ -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;
@@ -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.
+1
View File
@@ -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
+3 -2
View File
@@ -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
+16 -1
View File
@@ -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.
+23
View File
@@ -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:
+74
View File
@@ -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
+65
View File
@@ -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,
);
});
});