Ekklesia/docs/architecture/decisions/0020-module-runtime-and-migration-execution-hardening.md

0020: Module runtime and migration execution hardening

• Status: Accepted
• Date: 2026-07-27
• Amends: 0009, 0015
• Extends: 0017, 0019

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.

A connection string can reach those values, because [email protected] builds parameters as Object.assign({}, config, parse(config.connectionString)) and pg-connection-string retains arbitrary query keys. What each key actually does was measured against the installed driver, a real blackhole listener, and a real PostgreSQL rather than inferred from that merge, and only one of them genuinely weakens the policy:

Source	Measured effect on the selected policy
?statement_timeout=0 / =900000	Wins. Server reported 0 (disabled) and 15min against an explicit statement_timeout: 10000. The one real downgrade.
?query_timeout=0	Overrides the explicit field, but breaks the client rather than loosening it: the parsed value is the string "0", which is truthy, so every query gets a 0 ms read timer and fails nondeterministically with Query read timeout.
?connectionTimeoutMillis=0 / =900000	Pollutes ConnectionParameters.connect_timeout (10 → 0/900 s), but that field feeds only getLibpqConnectionString(), which the pure-JS client never calls. The effective connect timer is unchanged — an explicit 500 ms still expired at ~500 ms with either value.
?connect_timeout=0	No effect: ConnectionParameters recomputes it from connectionTimeoutMillis.
?options=-c statement_timeout=… (incl. a backslash-escaped spelling)	No effect: the named startup parameter wins. Server still reported 10s.
inherited PGOPTIONS=-c statement_timeout=0	No effect. Server still reported 10s.
inherited PGCONNECT_TIMEOUT=0	No effect: consulted only when connectionTimeoutMillis is undefined, which this boundary never leaves unset.
unrelated parameters (application_name, …)	No effect; deliberately untouched.

A DATABASE_URL carrying statement_timeout, query_timeout, connectionTimeoutMillis, connect_timeout, or an options value naming statement_timeout is refused with the existing fixed POSTGRES_MIGRATION_CONFIGURATION_INVALID, before any client is constructed and without echoing the URL, parameter, or value. Only the first is a precedence fix; the rest are rejected as unsupported configuration and defense in depth — no legitimate DATABASE_URL carries a driver option, connectionTimeoutMillis would take effect on the native binding, and query_timeout produces intermittent failures an operator would struggle to diagnose. Stating that honestly matters: an earlier revision of this ADR claimed connect_timeout overrode the explicit field, which measurement disproved.

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.