P1: Graceful shutdown — drain, flush traces, uncaught handlers (#170)

[dkijania]

What & why

Part of the production-readiness epic (#163). Closes #170.

Shutdown previously called server.close() then process.exit(0) from the close event. It didn't bound how long draining could take, never flushed OpenTelemetry spans (losing the tail of traces on every deploy), and had no handlers for uncaughtException / unhandledRejection.

Changes

• New src/server/graceful-shutdown.ts — a small, unit-tested createGracefulShutdown orchestrator.
• Entry point wires it up:
  • Drain in-flight requests via server.close(), then run teardown steps (flush the tracer provider, close the Postgres pool) in order.
  • A hard SHUTDOWN_TIMEOUT_MS deadline (default 10s) forces exit if draining/teardown hangs; the process exits at most once.
  • The handler is idempotent — a second signal is ignored.
  • SIGINT/SIGTERM/SIGQUIT plus uncaughtException/unhandledRejection all route through it.
• buildPlugins now returns the tracer provider so the entry point can flush it.

Env var	Default	Meaning
SHUTDOWN_TIMEOUT_MS	10000	Max ms to drain before forcing exit

Testing

• npm run build — clean
• npm run test:unit — all pass; new tests cover ordering, idempotency, a failing teardown step (logged, doesn't abort the rest), and the timeout-forces-exit path (injected exit hook, asserts exit code 1 fires exactly once)
• npm run lint — clean
• npx prettier --debug-check . — exit 0

🤖 Generated with Claude Code

[SanabriaRusso]

Really nice to see this landed — bounding the drain, flushing spans, and centralizing exit into one idempotent, unit-tested orchestrator is exactly what #170 needed, and dropping the old server.on('close') exit path cleans up the double-exit nicely.

One thing worth fixing before merge: the uncaughtException / unhandledRejection handlers route through shutdown(), whose happy path always exitOnce(0). So a real crash that drains cleanly exits 0. Today (no handler) Node exits 1 on an uncaught error, so this actually reduces crash signal — a container exiting 0 reads as "Completed" to Kubernetes/systemd, which suppresses OnFailure restarts and non-zero-exit alerting. A crash-initiated shutdown should exit non-zero. Small thread-through:

// graceful-shutdown.ts
return async function shutdown(reason: string, exitCode = 0): Promise<void> {
  // …unchanged…
  exitOnce(exitCode);        // success path uses the caller's code
  // catch { exitOnce(1) }   // unchanged
};

// index.ts
process.on('uncaughtException', (error) => {
  console.error('Uncaught exception:', error);
  void shutdown('uncaughtException', 1);
});
process.on('unhandledRejection', (reason) => {
  console.error('Unhandled rejection:', reason);
  void shutdown('unhandledRejection', 1);
});

Signals keep the default 0.

Secondary (optional): the closers run only after await closeServer() resolves, and server.close() waits on idle keep-alive connections — browsers like the mina-explorer client hold those open up to keepAliveTimeout (~5s). So drains will routinely be slow, and if SHUTDOWN_TIMEOUT_MS is ever ≤ that, you'd hit the force path and skip the trace flush + pool close entirely. Adding server.closeIdleConnections() (Node 18.2+) right after server.close() inside closeServer makes drains prompt and keeps the flush reliable.

Heads-up on merges: this overlaps #193 and #190 on both index.ts and plugins.ts (and all three touch the docs/getting-started.md env table) — no behavioral clash, but whoever goes second will need a manual reconcile.

[dkijania]

Thanks @SanabriaRusso — good catch on the exit code, that one was a real regression in crash signal and I've fixed it in 8e72253.

Crash exits are now non-zero. shutdown() takes an exitCode used on the success path; signals keep 0 and the uncaughtException / unhandledRejection handlers pass 1, exactly as you sketched. The drain still runs in full on a crash — only the code differs — and there's a unit test pinning that, since the whole failure mode was "drains cleanly, exits 0, looks like a clean stop":

await shutdown('uncaughtException', 1);
assert.deepStrictEqual(calls, ['server', 'traces']);  // drain still happens
assert.deepStrictEqual(exits, [1]);                   // but supervisors see a failure

Took the closeIdleConnections() suggestion too. It's in closeServer right after server.close(). Your reasoning was the convincing part: the failure isn't just a slow drain, it's that a drain pushed past SHUTDOWN_TIMEOUT_MS hits the force path and skips the trace flush and pool close entirely — so the keep-alive wait could silently cost us the very teardown this PR exists to add.

On the merge overlap with #193/#190 — agreed, and that's the plan: this sits in wave 2 of the merge order, so I'll reconcile index.ts / plugins.ts and the docs env table by hand as they land.

[SanabriaRusso]

Verdict: MERGEABLE ✅

Both of @SanabriaRusso's points are genuinely fixed at 8e72253, and nothing here touches the three downstream hard constraints. Notes below are all non-blocking, but #2 and #3 are worth acting on before this rides a real deploy.

What I checked

• Maintainer's point 1 (exit code) — fixed. src/server/graceful-shutdown.ts:44 takes exitCode = 0, success path is exitOnce(exitCode) (:69), and src/index.ts:47-55 passes 1 from both crash handlers while signals keep 0. Pinned by tests/unit/graceful-shutdown.test.ts ("a crash-initiated shutdown drains cleanly but still exits non-zero").
• Maintainer's point 2 (closeIdleConnections) — fixed. src/index.ts:27-32, inside closeServer, right after server.close(). Node 20 base image (Dockerfile:2), so the 18.2+ API is available.
• uncaughtException / unhandledRejection are implemented the correct way. src/index.ts:47-55 logs, routes through the bounded drain, and always terminates — it never resumes serving from a corrupted process, which is the dangerous variant. And exiting on unhandledRejection is not new restart-churn here: Node 20's default is already --unhandled-rejections=throw, i.e. an unhandled rejection already killed the process abruptly. This PR converts that abrupt kill into a bounded drain with the same non-zero exit. Strictly better.
• No downstream constraint touched. No change to error text, maskedErrors, CORS (src/server/server.ts:24-27 CORS_ORIGIN ?? '*' untouched), or any resolver/schema surface — Check Schema and GraphQL Inspector are green. mina-explorer's inBestChain string match (src/services/api/bestChainFilter.ts) and mina-explorer-api's SCHEMA_ERROR_MARKERS (app/upstream/graphql.py) are unaffected.
• Registration + reachability. Handlers are registered once inside main() in the actual entry point (package.json:24 → build/src/index.js), not per-request. Idempotency guard is real: started at graceful-shutdown.ts:36/45-46 plus a separate exited guard at :48-53, so a second SIGTERM/SIGINT is a silent no-op rather than a re-entry or a throw. Covered by the "is idempotent" test.

Non-blocking nits

1. Neither closer is individually bounded, and they're serialized — the first hang eats the whole budget.

src/index.ts:33-40 runs provider.shutdown() then db_client.close() sequentially, with only the outer 10s force timer as protection. Both can outlast it:

• provider.shutdown() → BatchSpanProcessor export timeout defaults to OTEL_BSP_EXPORT_TIMEOUT = 30000ms (node_modules/@opentelemetry/core/build/src/utils/environment.js:80, consumed at @opentelemetry/sdk-trace-base/build/src/export/BatchSpanProcessorBase.js:44-47). A Jaeger collector that went unreachable after boot (initJaegerProvider only probes at startup, src/tracing/jaeger-tracing.ts:30) blocks for 30s > the 10s deadline.
• db_client.close() → sql.end() with no args, and porsager's default is { timeout = null } (node_modules/postgres/src/index.js:364) = wait forever for open connections.

So today an unreachable Jaeger means the trace flush and the pg pool close are both skipped via force-exit — the exact teardown this PR exists to add. Bound each step:

// src/index.ts
const withTimeout = (label: string, ms: number, run: () => Promise<unknown>) =>
  Promise.race([
    run(),
    new Promise<void>((_, reject) =>
      setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms).unref()
    ),
  ]).then(() => undefined);

closers: [
  // Flush buffered spans. 3s: well under SHUTDOWN_TIMEOUT_MS so an unreachable
  // Jaeger (OTEL_BSP_EXPORT_TIMEOUT defaults to 30s) can't eat the whole budget
  // and skip the pool close below.
  () => withTimeout('trace flush', 3_000, async () => { if (provider) await provider.shutdown(); }),
  // Close the Postgres pool. Bounded because porsager's sql.end() defaults to
  // timeout: null (waits indefinitely for open connections).
  () => withTimeout('pg pool close', 5_000, () => context.db_client.close()),
],

A failing closer is already logged-and-continued (graceful-shutdown.ts:62-66), so a timeout rejection degrades correctly instead of aborting the rest.

Ordering is already right: both run after closeServer() resolves, so the pool isn't closed out from under in-flight queries.

2. SHUTDOWN_TIMEOUT_MS = 10000 is a small regression for the heaviest real query.

main's drain is unbounded, so an in-flight request that takes 15s completes and then the process exits (Node's default keepAliveTimeout is 5s, so idle sockets clear well inside the 30s grace period — main rarely actually gets SIGKILLed). With this PR the same request is force-killed at 10s.

The concrete victim is mina-explorer's analytics query: ANALYTICS_BLOCK_LIMIT = 2000 (/Users/sanabriarusso/github/mina-explorer/src/services/api/analytics.ts:14), a single browser POST pulling 2000 blocks with userCommands, budgeted at 20s client-side (src/services/api/http.ts:9). The explorer does not retry — GraphQLClient.query surfaces the TypeError as a network error straight to the UI (src/services/api/client.ts:38-46). mina-explorer-api absorbs it (2 retries with backoff on transport errors, app/upstream/graphql.py:229-238), so the asymmetry is that only the browser UI shows the failure.

10s is correctly below the k8s default terminationGracePeriodSeconds: 30, which is the important property. But there's headroom: 20000 would still force-exit 10s before SIGKILL and would cover the analytics query. Related footgun worth naming while #182 is open: a 30s statement_timeout with a 30s drain inside a 30s grace period is three deadlines racing at the same value — keeping the drain strictly shorter (10s or 20s) is the right call, just make the gap deliberate rather than incidental.

3. The force timer is .unref()'d, which can defeat it (graceful-shutdown.ts:59).

clearTimeout(forceTimer) already runs in finally (:73), so the timer can never hold the process open past a completed shutdown — unref() buys nothing. What it costs: if the drain stalls on a promise that never settles and no active handles remain (plausible on the crash path, after closeServer() has torn the sockets down), the event loop empties and Node exits 0 on its own instead of the intended exitOnce(1) — reintroducing the exact "crash looks like a clean stop" failure mode this PR just fixed. Dropping the unref line closes it.

4. Readiness pre-drain is missing — pre-existing, but closeIdleConnections() sharpens it.

SIGTERM goes straight to server.close() + closeIdleConnections(). There's no window where readiness fails first so the LB stops routing. k8s endpoint propagation takes seconds, so requests routed in that window hit a closed listener: ECONNREFUSED for new connections, and a reset for pooled clients that write onto a socket as it's destroyed. mina-explorer shows a hard network error; mina-explorer-api retries twice and mostly absorbs it.

main has the same gap, so this is not a regression and I'm not blocking on it — but closeIdleConnections() (correctly added) widens the pooled-socket reset race, and #187 is landing /readiness (src/server/readiness.ts, READINESS_PATH = '/readiness') as an isolated Yoga plugin with no shutdown integration. Neither PR owns the wiring, so it'll fall through the crack unless someone claims it. Suggested follow-up once both are in — a mutable flag the readiness plugin reads, flipped before the drain:

// src/index.ts, after #187 merges
const PRE_DRAIN_DELAY_MS = Number(process.env.PRE_DRAIN_DELAY_MS) || 5_000;

createGracefulShutdown({
  timeoutMs: SHUTDOWN_TIMEOUT_MS,
  closeServer: async () => {
    setNotReady();                 // /readiness -> 503; LB drops this pod
    await new Promise((r) => setTimeout(r, PRE_DRAIN_DELAY_MS)); // let endpoints propagate
    await new Promise<void>((resolve, reject) => {
      server.close((error) => (error ? reject(error) : resolve()));
      server.closeIdleConnections();
    });
  },
  // ...
});

Budget check: PRE_DRAIN_DELAY_MS (5s) + SHUTDOWN_TIMEOUT_MS must stay under terminationGracePeriodSeconds (30s default) — note the pre-drain sleep is inside closeServer, so it's already counted against SHUTDOWN_TIMEOUT_MS rather than added to it. With 20s that's 5s propagation + 15s of real drain, force-exit at 20s, 10s of slack before SIGKILL.

Regression test for the ordering, mirroring the existing suite's style:

test('marks unready and waits before closing the listener', async () => {
  const calls: string[] = [];
  const shutdown = createGracefulShutdown({
    closeServer: async () => {
      calls.push('unready');           // stand-in for setNotReady()
      calls.push('close');
    },
    closers: [async () => calls.push('db')],
    timeoutMs: 1000,
    onExit: () => {},
    log: () => {},
  });
  await shutdown('SIGTERM');
  assert.deepStrictEqual(calls, ['unready', 'close', 'db']);
});

Branch is BEHIND main — needs a rebase, and per your wave-2 plan the index.ts / plugins.ts / docs-env-table reconcile with #193 and #190 still applies.

Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.

[SanabriaRusso]

Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.

Two things this approval does not mean:

• It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
• It does not by itself mean the branch is ready to merge. main requires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.

Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.

[SanabriaRusso]

Approving. Both fix commits do what they claim, and I chased the cross-PR interaction I expected to be broken and found it already handled.

26bdb57a — non-zero exit on crash: correct. src/index.ts:73-80 logs the error/reason and then calls shutdown(reason, 1); the code is threaded through to exitOnce(exitCode) at graceful-shutdown.ts:68, so a crash drains in full and exits 1. The regression test asserts both halves ("a crash-initiated shutdown drains cleanly but still exits non-zero" → calls == ['server','traces'] and exits == [1]), which will catch a regression in either direction — an exit-0 regression or one that skips the drain. Nothing is swallowed: the console.error precedes the shutdown call.

848af58c — bounded teardown: correct, and the timeout cannot itself hang. I traced all three bounds:

• the outer forceTimer (graceful-shutdown.ts:54-57, default 20 s → exitOnce(1)) — a bare setTimeout with nothing to await, so it structurally cannot hang;
• withTimeout('trace flush', 3000, …) (index.ts:57) for an unreachable Jaeger collector;
• withTimeout('pg pool close', 5000, …) (index.ts:63) for postgres.js, which otherwise waits forever.

closeServer has no individual timeout but the outer timer covers it. clearTimeout is in a finally (:73), exitOnce (:48-52) prevents a double-exit when the timer races a successful drain, a throwing closer is caught per-iteration (:64) without aborting the rest, and the started guard (:36/:44) makes a second signal — or an exception recurring mid-drain — a no-op rather than a loop. server.closeIdleConnections() alongside server.close() is the right pairing; without it browser keep-alives would hold the drain open for keepAliveTimeout.

The #187 integration gap I was looking for doesn't exist. #187 doesn't add a separate teardown path — it extends the existing close() to await Promise.all([this.client.end(), this.pingClient.end()]). So context.db_client.close() here transitively closes the new dedicated max:1 ping client, inside this PR's 5 s bound. Noting it explicitly so nobody "helpfully" adds a second close later.

Non-blocking notes

1. unhandledRejection fatality is not a regression — worth being explicit, since registering the listener suppresses Node's default --unhandled-rejections=throw. On main a stray rejection already kills the process (default throw → uncaught → exit 1). After this it still does, but gracefully and with the same exit code. Net strictly better. If yoga/whatwg-node produced spurious rejections on client aborts, main would already be crash-looping — it isn't.

2. The crash path drains for up to the full 20 s in a possibly-corrupt state. Node's guidance after uncaughtException is to exit fast. The listener is already closed so no new work is accepted, and letting in-flight requests finish is usually what you want — but a shorter budget for the crash path specifically (2–3 s) would be more conservative. Your call.

3. SHUTDOWN_TIMEOUT_MS=0 silently becomes 20000 (Number(x) || 20000, index.ts:9). Cosmetic.

Cross-cutting gap this PR should NOT be asked to fix. Round 1 flagged the rolling-deploy drop as an unowned gap, and it still is. Once this lands, SIGTERM closes the listener immediately — closeServer (index.ts:46-52) calls server.close() then closeIdleConnections(), with no readiness pre-drain and no delay. #187 adds /readiness with no shutdown integration; #196 does now add a preStop: sleep 15, which covers most of it. What is still missing is the in-process half: a readiness flag that flips to failing on SIGTERM before closeServer() runs, so /readiness returns 503 while the listener is still open and the endpoint removal has somewhere to land.

This should not block this PR — #188 is a strict improvement over main, which does a bare server.close() with no drain bound, no timeout, and process.exit(0) on every path including crashes. The remaining half belongs with #187, since it owns /readiness and would export the flag this handler sets. Recommend filing it as a standalone issue rather than expanding scope here; the shutdown machinery in this PR is already the correct foundation for it.

Downstream: positive, no risk. For both consumers this is a strict improvement — main tears the listener down with no drain bound at all, so in-flight requests are lost outright on every restart; this gives them up to 20 s to complete, comfortably inside both consumers' 20 s client timeout. No schema, resolver, error-text, or HTTP-status change.

Merge ordering: conflicts textually with #189 in .env.example.compose / .env.example.lightnet (this PR inserts SHUTDOWN_TIMEOUT_MS right after PORT; #189 deletes APP_COMMAND right before it — overlapping hunks, both changes should survive), with #193 in the docs/getting-started.md config table, and with both #193 and #194 in src/server/plugins.ts. All semantically compatible, just needs care.

[SanabriaRusso]

Re-approving after the rebase (the previous approval was dismissed by the force-push).

The rebase here was not mechanical, so I re-checked the delta rather than assuming. Compared against the commit I approved (32b8afe), the only content change is the one the rebase required, and it is the correct resolution of the buildPlugins() signature hazard:

tests/unit/metrics.test.ts
-    plugins: await buildPlugins(),
+  const built = await buildPlugins();
+    plugins: built.plugins,

#191 landed on main after my review and its test called the old form. Every caller is now consistent — src/index.ts:35 destructures { plugins, provider }, tests/unit/metrics.test.ts:26 uses built.plugins, and there is no remaining return plugins call site. All checks green.

Carrying forward the round-2 verification unchanged:

• Non-zero exit on crash-initiated shutdown verified, with a test asserting both that it drains and that it exits 1.
• Bounded teardown verified: three independent bounds, and the outer forceTimer is a bare setTimeout so it cannot itself hang. exitOnce guards the timer racing a successful drain.
• The #187 integration gap does not exist: #187 extended the existing close() rather than adding a second path, so db_client.close() here transitively closes the readiness ping client inside the 5s bound.

Merge-train note: this signature change is the one that six other open PRs can silently undo. A conflict resolution that keeps return plugins drops trace flushing on shutdown with no error — provider is simply undefined. Merge this before the remaining plugins.ts editors, or re-check the destructure after each.

Still unowned (not this PR's scope): the in-process half of the readiness pre-drain flip — a flag that flips /readiness to 503 on SIGTERM before closeServer() runs, so endpoint removal has somewhere to land. That belongs with #187, which owns /readiness. Tracked in #210.