fix(gateway): drain transports on SIGTERM, exit on unhandled faults - #30
Open
menor wants to merge 2 commits into
Open
fix(gateway): drain transports on SIGTERM, exit on unhandled faults#30menor wants to merge 2 commits into
menor wants to merge 2 commits into
Conversation
Upsun sends SIGTERM on deploy and on stop. Only `index.ts` handled it, and that handler flushed telemetry without touching the transports, so active SSE and Streamable HTTP sessions were never closed in production. SIGINT had the opposite problem: `index.ts` and `gateway.ts` both handled it, each with its own async work and its own `process.exit(0)`. Whichever finished first killed the other's cleanup. Consolidate into one path. `GatewayServer.shutdown()` drains both transports and returns; the signal handlers in `index.ts` own the ordering and the exit. Each step is guarded so a failure cannot skip the next step or the exit — `shutdownTelemetry()` rethrows, which previously escaped the handler and left the process hanging until SIGKILL. Verified on the built server: SIGTERM logs "Closing active transport sessions" -> "Transport sessions closed" -> "Shutdown complete", exit code 0.
Registering a listener for either event overrides Node's default crash. The
existing handlers only logged, so the server kept serving requests after a
fault it could not reason about — and since Node 15 that includes unhandled
rejections, which would otherwise terminate the process on their own.
Exit 1 instead. Upsun's supervisor restarts the process ("restarted if ever
terminated"), and .upsun/config.yaml runs `node build/index.js` directly under
it, so a restart is a shorter outage than an unbounded broken state. Neither
handler drains transports first: the state that made them fire is exactly the
state not to run more async work on.
Verified against the built server: an unhandled rejection and an uncaught
exception each exit 1. On main both leave the process running.
|
Coverage after merging fix/graceful-shutdown into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two process-lifecycle bugs in
gateway.ts. Both are in the same 18-line block, so they ship together as two commits — either can be dropped.1. SIGTERM never closed transport sessions
Upsun sends SIGTERM on deploy and on stop. Handler coverage before this PR:
index.tshandler (flush telemetry)gateway.tshandler (close transports)process.exit(0)So production never closed active SSE or Streamable HTTP sessions. Locally, the two SIGINT handlers raced: whichever finished its async work first exited the process and truncated the other.
GatewayServer.shutdown()now drains both transports and returns. The signal handlers inindex.tsown the ordering and the exit, so SIGTERM and SIGINT run identical steps. Each step is guarded on its own —shutdownTelemetry()rethrows on failure, which previously escaped the handler and left the process hanging until SIGKILL.Measured on the built server (
node build/index.js, thenkill -TERM):mainShutting down gracefully...Shutting down gracefully...Closing active transport sessions...Transport sessions closedShutdown complete2. Unhandled faults left the server running
Registering a listener for
uncaughtExceptionorunhandledRejectionoverrides Node's default crash. The existing handlers only logged. For rejections that is a behavior change Node made in v15 — unhandled rejections terminate the process unless something suppresses them, and this suppressed them.Both handlers now
process.exit(1). Upsun's supervisor restarts the process (docs: "restarted if ever terminated") and.upsun/config.yamlrunsnode build/index.jsdirectly under it, so a restart is a shorter outage than an unbounded broken state. Neither handler drains transports first: the state that made them fire is the state not to run more async work on.Measured by booting the built
GatewayServerand triggering each fault:mainunhandledRejectionuncaughtExceptionTests
test/core/gateway.test.tsgains 3 cases (487 -> 489, all green):shutdown()closes sessions on both transportslisten()registers no signal handlers of its own — this fails onmainit.each)The old SIGINT block had no test.
afterEachdetaches handlerslisten()added, so anexit(1)handler cannot leak into a later test and kill the Jest worker.Checks
npm run buildcleannpm test489 passed, 26 suitesnpm run lintcleannpx prettier --checkclean on all three changed filesNo tool output changes, so no benchmark run.