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]>
This commit is contained in:
ekklesia
2026-07-26 23:12:18 -05:00
co-authored by Claude Opus 5
parent 800ecfdb92
commit 490b85b07c
9 changed files with 590 additions and 22 deletions
+14
View File
@@ -152,6 +152,20 @@ 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.
Both use a short **test-only override**; the production defaults (10 s per module hook, 30 s HTTP
drain) 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: