Skip to content

fix(nats): bound the connection drain on scope close - #158

Open
adam-hotait-spiko wants to merge 1 commit into
feat/confirm-channelfrom
fix/bound-nats-connection-drain
Open

fix(nats): bound the connection drain on scope close#158
adam-hotait-spiko wants to merge 1 commit into
feat/confirm-channelfrom
fix/bound-nats-connection-drain

Conversation

@adam-hotait-spiko

Copy link
Copy Markdown

Stacked on #147 (base is feat/confirm-channel), because a bounded teardown and publisher confirms address the same failure from opposite ends: #147 makes a publish durable against an abrupt exit, this makes the exit not abrupt.

Summary

  • Give the NATS connection's scope finalizer a deadline instead of awaiting nc.drain() indefinitely, closing the connection outright once it expires.
  • Add drainTimeout as an optional second argument to layerNode / layerWebSocket, defaulting to 5 seconds. Existing callers are source-compatible.
  • Treat a rejected drain() as "not drained" so the connection is still closed, rather than leaving it open.
  • Move the teardown into internal/connectionTeardown.ts behind a narrow Closeable interface so it is unit-testable without a broker.

Why

drain() unsubscribes, waits for every buffered and in-flight message to be handled, then flushes and closes. A consumer that keeps receiving messages can hold that open indefinitely. Because Effect runs finalizers uninterruptibly, SIGTERM cannot cut it short, so the process never exits and the supervisor eventually SIGKILLs it.

Found while investigating a production alert on service-distributor, the only service in our app with a NATS connection: it is force-killed on roughly every Karpenter node consolidation because shutdown outlives the pod's termination grace period. Its webhook-retry consumer never idles by design (deliveryCount === 1 returns a nak, so scheduling ticks keep cycling), which is exactly the shape that keeps a drain open forever. A SIGKILL mid-handler is materially worse than a slow shutdown for us: the handler never writes its ack/nack, so the redelivery is later routed to the DLQ for manual replay.

Why the timeout is a promise-level race

Effect.timeout does not work here. Finalizers run in an uninterruptible region, where the loser of a race cannot be interrupted, so the timeout never fires. Verified before writing the fix:

Effect.addFinalizer(() => Effect.never.pipe(Effect.timeout("100 millis"), Effect.ignore))
// scope close still hangs; wrapping Scope.close in another Effect.timeout does not help either

So the deadline is raced against the already-started promise with Promise.race, and the timer is cleared on either outcome so a completed teardown never holds the event loop open.

Compatibility with #89a7459

This does not walk back fix(core): continue handling in-flight events when receiving SIGINT. Handlers are still never interrupted by shutdown; only the connection-level drain is bounded. The pre-existing Should let in-flight handler complete on interrupt tests in NATSSubscriber, JetStreamSubscriber and core all still pass.

Test plan

  • packages/nats/test/connectionTeardown.test.ts covers the three outcomes: drain never settles → connection closed within budget; drain rejects → connection closed; healthy drain → left alone, no forced close.
  • Mutation-checked the new suite by restoring the unbounded Effect.promise(() => nc.drain()): the never-settles test times out and the reject test fails, while the healthy-drain test still passes. It is not a vacuous suite.
  • packages/nats + packages/core: 42 tests pass against a clean nats:latest -js.
  • tsc -b tsconfig.json clean, tsc -b tsconfig.build.json + package builds clean, eslint clean over packages/*/{src,test}.

Note for reviewers: these tests use it.live rather than plain it. Plain it returns the Effect to vitest, which does not await it, so the body never runs and the test passes unconditionally.

The scope finalizer awaited nc.drain() with no deadline. drain() waits for
every buffered and in-flight message to be handled before it flushes and
closes, so a consumer that keeps receiving messages holds the finalizer open
indefinitely and strands process shutdown until the supervisor resorts to
SIGKILL.

Give the drain a budget, configurable via drainTimeout and defaulting to 5
seconds, and close the connection outright once it expires. A rejected drain
now closes too instead of leaving the connection open.

The budget is raced at the promise level rather than with Effect.timeout:
finalizers run in an uninterruptible region, where the loser of an
Effect-level race cannot be interrupted and the timeout never fires.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant