fix(nats): bound the connection drain on scope close - #158
Open
adam-hotait-spiko wants to merge 1 commit into
Open
fix(nats): bound the connection drain on scope close#158adam-hotait-spiko wants to merge 1 commit into
adam-hotait-spiko wants to merge 1 commit into
Conversation
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.
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
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.
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
nc.drain()indefinitely, closing the connection outright once it expires.drainTimeoutas an optional second argument tolayerNode/layerWebSocket, defaulting to 5 seconds. Existing callers are source-compatible.drain()as "not drained" so the connection is still closed, rather than leaving it open.internal/connectionTeardown.tsbehind a narrowCloseableinterface 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 === 1returns 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.timeoutdoes 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: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-existingShould let in-flight handler complete on interrupttests inNATSSubscriber,JetStreamSubscriberand core all still pass.Test plan
packages/nats/test/connectionTeardown.test.tscovers the three outcomes: drain never settles → connection closed within budget; drain rejects → connection closed; healthy drain → left alone, no forced close.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 cleannats:latest -js.tsc -b tsconfig.jsonclean,tsc -b tsconfig.build.json+ package builds clean,eslintclean overpackages/*/{src,test}.Note for reviewers: these tests use
it.liverather than plainit. Plainitreturns the Effect to vitest, which does not await it, so the body never runs and the test passes unconditionally.