Skip to content

Graceful shutdown for RabbitMQ consumers (0.18.0) - #118

Merged
ramhr merged 25 commits into
masterfrom
feat/graceful-shutdown
Sep 9, 2026
Merged

ramhr merged 25 commits into
masterfrom
feat/graceful-shutdown

Conversation

@ramhr

@ramhr ramhr commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Additive graceful-shutdown API, inert unless called. Public surface is four things: arnavmq.close(), arnavmq.consumer.inFlight(), connection.isClosed, and ConnectionClosedError (exported from the package entry point, so instanceof works at runtime). Version bump to 0.18.0 (minor, purely additive).

Main focus: the consumer drains already in-flight messages before the connection closes

The point of this PR is that a message whose handler has already started running gets to finish and ack before we tear anything down, instead of being killed mid-flight and redelivered.

arnavmq.close() is two steps:

async close() {
  await this.consumer.stop();   // cancel every subscription, then drain
  await this.connection.close(); // flush channels, then close the socket
}
  1. Cancel every subscription by consumerTag, so the broker stops delivering new messages. Always by consumerTag, never by closing the shared channel — DEFAULT_CHANNEL is shared by every default-prefetch consumer on the connection, so closing it would take down unrelated consumers.
  2. Drain — wait for every in-flight handler to run to completion and ack. A message the broker had already pushed into the client buffer before cancel-ok landed has not started its handler, so it is rejected/requeued immediately rather than being run; it goes back to the queue for another instance, and nothing is dropped.
  3. Close the connection — round-trip Channel.Close/Close-Ok on every cached channel, then close the socket.

The connection stays open through cancel/drain on purpose: a draining handler may still publish() downstream, answer an RPC request it had already received, or complete a new RPC round-trip of its own. All of that keeps working, and anything a handler starts is waited on too. connection.close() is the first point at which publishing starts failing with ConnectionClosedError.

There is no drain timeout. A handler that never finishes means close() never resolves — deliberately left to the orchestrator's own kill grace period rather than this library abandoning in-flight work on a clock. That includes a handler awaiting an RPC response nobody is left to send (the peer is shutting down too, or is served by a consumer this process just cancelled): in that case the pod burns its grace period and gets SIGKILLed. That is an accepted, documented cost, chosen over a timeout that would abandon real work early — by the time the grace period runs out, the acks written by handlers that did finish were flushed to the broker long before, so nothing is at risk beyond a slow exit.

Also fixes a real, reproduced ~50% message-redelivery race on shutdown: channel.ack() is fire-and-forget in AMQP 0-9-1, so tearing down the raw connection right after a successful drain could lose the race against the broker's own disconnect-driven auto-requeue — a message that had been handled and acked would still be redelivered. Fixed by the Channel.Close/Close-Ok round-trip in step 3, which forces the broker to have processed the preceding acks. Channel.Close is bounded at 5s per channel so an unresponsive broker cannot hang shutdown at that step.

Pending RPC requests fail fast, from a channel-close listener

An RPC request still waiting for its response when the channel goes away is rejected with ConnectionClosedError, from a single 'close' listener the producer keeps on the channel.

This is not a shutdown step — earlier revisions of this PR had it as a producer.stop() call in close(), and that was wrong. The reply queue is exclusive to the connection, and every request already went out carrying that queue's name as its replyTo, so once the channel is gone no answer can arrive for those requests ever — not even after a reconnect. Making it reactive means the same code covers a deliberate close() and a connection dropped unexpectedly, with no shutdown-only flag and no hole.

Waiting for those responses instead was considered and rejected: by that point every locally-handled message has already finished, so the response resolves a promise inside a caller that is going away regardless — and with a long rpcTimeout (or rpcTimeout: 0, which arms no timer at all) waiting means blocking shutdown for that duration, or forever, on a peer that may never answer. Callers get a deterministic, catchable failure instead of a hang.

Review instructions

Read the final per-file diff, not the commit sequence. The commits are historical: several later ones reverse decisions made in earlier ones (notably the close() step ordering, and producer.stop(), which was added and then removed entirely). Reviewing commit-by-commit means reviewing designs that are no longer in the branch.

Suggested file order:

  1. src/modules/consumer.js — the core of the PR. The subscription registry, _onDelivery()'s in-flight accounting, _cancelSubscription(), _drain(), stop().
  2. src/modules/connection.js + src/modules/channels.js — the highest-risk part; please review closely. close()/isClosed and the ack-flush-race fix in Channels.closeAll().
  3. src/modules/producer.js — the channel-close listener and the RPC waiter registry.
  4. src/modules/arnavmq.js — the two-step close(). Small, but the ordering rationale lives in its JSDoc.
  5. types/, test/shutdown-spec.js, README.md — declarations, ~1400 lines of new coverage, docs.

What most needs a human eye:

  • The absence of a drain timeout. This is the one deliberate way close() can never resolve. The reasoning is above; disagreement with it is the most valuable review finding here.
  • In-flight accounting in _onDelivery(). A message counted as in-flight but never decremented hangs close() forever, since there is no timeout. Every path out of the handler — success, throw, reject/requeue, parse failure — must decrement exactly once.
  • The buffered-message rejection path. Confirm a message that arrived before cancel-ok really is requeued, not dropped or double-acked.
  • The channel flush in connection.close(). Whether Channel.Close/Close-Ok per cached channel is genuinely sufficient to order the preceding acks ahead of the socket teardown, and whether a channel already closed by the broker is handled.
  • No-regression on the reconnect path. _isLive() and the isClosed checks now gate resubscribe behavior. test/disconnect-spec.js is the gate and stays green, but the interaction between shutdown state and reconnect logic is worth a read.

What you can safely skip: the API is additive and inert unless close() is called, so existing-behavior regressions are concentrated in consumer.js's consume path and the reconnect logic, not in the new methods themselves.

Verifying locally: npm test needs a real broker — test/docker.js brings one up. Individual specs: npx mocha test/shutdown-spec.js --exit.

Also in this branch

  • eslint:recommended is now actually enabled. The flat config passed js.configs.recommended to FlatCompat's recommendedConfig option, which only tells the compat layer how to resolve "eslint:recommended" when some other config extends it — it never enables the rules. The config is now spread into the array directly. This is not cosmetic: it catches the no-unsafe-finally bug this branch shipped in withTimeout and fixed a few commits later (return clearTimeout(timeoutId) inside a finally, which swallowed the raced promise's result), which had been invisible to CI. Existing warnings were left as warnings so CI stays green.
  • Consumer cleanup. _consumeQueue() was one 108-line method wrapped around a nested async closure; the delivery path is now four named methods. A redundant _shuttingDown flag was folded into the already-present _stopPromise, and a dead try/catch around Channels.closeAll() (which cannot reject) was removed from Connection._close().

Process note

Built by AI agents against a written design plan, every task run against a real dockerized RabbitMQ broker. History was reorganized into one commit per logical concern.

The follow-up commits came out of a point-by-point human-led audit of an AI-generated PR review, which flagged four real bugs (of which three sub-claims and two proposed fixes were wrong and rejected): a close() step ordering that could deadlock, an RPC waiter registered before its publish could fail, the eslint gap above, and the producer.stop() design that has now been removed. The current design — including "no drain timeout, let the orchestrator kill us" — is a deliberate human decision, taken over the AI reviewer's recommendation.

Test plan

  • npm test — 105 passing, 0 failing; test/disconnect-spec.js (the resubscribe/reconnect regression gate) green throughout
  • npm run lint (eslint + prettier + tsc over types/) clean — exit 0
  • Human review of the ack-flush-race fix and of the no-drain-timeout decision

Comment thread src/modules/arnavmq.js Outdated
// in-flight: all bundled inside consumer.stop() already (see consumer.js), not three separate
// public calls to make here.
const { drained, abandoned } = await this.consumer.stop({ timeout });
await this.producer.stop();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will it affect the ack? we want an already running message to be acked once finished

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consumer:

  1. stops receiving new messages
  2. rejects any prefetched messages not yet started processing
  3. finishes processing and ack-ing in progress messages

The producer doesn't need to ack the messages, except when waiting for RPC responses. On that case, id don't block here, and you could get RPC responses that will never be read. This is not too bad as the message was already processed, so I keep it like this here.

Only after both consumer and producer are done, the channels are closed, and finally the connection is closed.

@ramhr
ramhr force-pushed the feat/graceful-shutdown branch 2 times, most recently from 4e8d193 to af83092 Compare August 18, 2026 12:16
consumer.js now tracks each subscribe() call as an explicit Subscription record (queue, options, callback, channel, consumerTag, onChannelClose, cancelled, inFlightMessages, abandonedMessages) instead of an implicit queue+options+callback closure. This lets shutdown mutate one object instead of racing untracked resubscribe state, and fixes a latent bug where every reconnect added one more 'close' listener to the shared DEFAULT_CHANNEL.

New public API: cancel(queue), cancelAll(), drain(timeoutMs), stop(options), inFlight(queue?). cancel/cancelAll cancel by consumerTag only - they never close the channel, since it is shared with every other consumer and with producer RPC replies. drain() polls inFlight() every 50ms rather than resolving off a deferred, since buffered deliveries keep arriving for a few ticks after cancel-ok. stop() = cancelAll() + drain(), and on timeout rejects+requeues every message still in flight, guarding the ack/reject/RPC-reply call sites against double-handling a message another pod already picked up.

_isLive() also treats a terminally closed connection as not-live, so connection.close() is safe to call directly even with active subscriptions, not just via arnavmq.close().
@ramhr
ramhr force-pushed the feat/graceful-shutdown branch 3 times, most recently from b246552 to bad4843 Compare August 18, 2026 13:05
ramhr added 5 commits August 18, 2026 16:08
…the socket

connection.js gains a terminal, idempotent close(): it flushes every cached channel (Channels.closeAll(), added in channels.js) before tearing the socket down, then clears the connection/channel caches. getConnection() now rejects with the new ConnectionClosedError once closed.

The channel flush matters because channel.ack()/reject() are fire-and-forget in AMQP 0-9-1 - there is no ack-ok frame - so closing the raw connection immediately after a drained handler's ack races the broker's own requeue-on-disconnect cleanup and can redeliver an already-acked message. Channel.Close does round-trip, so awaiting closeAll() guarantees those frames were processed first. Each channel closes independently and is capped at 5s (covering both the pending channel-open and the close() call itself) so one wedged or slow-to-open channel can't block process shutdown past a typical termination grace period.

Both listeners on a new channel are registered immediately after createChannel() resolves, before the prefetch await - amqplib rethrows an unhandled server-sent Channel.Close as an uncaught exception, so any await between channel creation and listener registration is a crash window.
Without this, a caller awaiting an RPC response hangs for the full rpcTimeout (15s default) after the connection is already gone. stop() walks amqpRPCQueues, clears each pending waiter's timeout, and rejects its responsePromise with ConnectionClosedError; it is idempotent and internal.

Also stop retrying the RPC-queue reconnect-on-close listener once the connection is terminally closed (ConnectionClosedError), instead of spinning forever, and fail fast instead of retrying indefinitely when publish()/produce() is called after connection.close().
Add ArnavMQ#close({timeout}), exposed on the top-level factory's returned object along with connection.close/isClosed and the additive consumer.cancel/stop/drain/inFlight, and export ConnectionClosedError from the package's main entry point. close() calls consumer.stop() (cancelAll -> drain -> reject-and-requeue-on-timeout) before producer.stop() and connection.close(), so the connection stays open for RPC replies and in-handler produce() calls until the very last step.

Add shutdownTimeout: 30000 as the config default for close()'s drain budget, and bump the package version to 0.18.0.
Types-only follow-up to the runtime graceful-shutdown work: public cancel/cancelAll/drain/stop/inFlight on Consumer with a Subscription type describing its internal record shape, close()/isClosed and ConnectionClosedError on Connection, and close() plus the consumer sub-object's new members on the Arnavmq type. Re-export ConnectionClosedError from index.d.ts.
Covers the subscription registry and listener-hygiene fix, in-flight tracking around the ack/reject/RPC-reply paths and all three abandoned-message guards, close()/isClosed idempotency (sequential and concurrent) and the in-flight-connect race, the channel-flush barrier (close ordering, one-bad-channel isolation, the wedged-channel cap), a full cancel(queueA) regression proving a concurrent queueB consumer and producer RPC call both keep working, and a pending RPC rejecting through the full arnavmq.close() orchestration.
@ramhr
ramhr force-pushed the feat/graceful-shutdown branch 3 times, most recently from 7add54a to da79f25 Compare August 18, 2026 14:16
@ramhr
ramhr force-pushed the feat/graceful-shutdown branch from da79f25 to e4565a5 Compare August 18, 2026 14:20
ramhr added 2 commits August 19, 2026 12:12
… on shutdown

drain()/stop() now wait indefinitely for in-flight handlers instead of
abandoning them past a shutdownTimeout - a stuck handler blocks close()
forever and the process orchestrator's own kill grace period takes over,
same as it would anyway once the old timeout elapsed. Simplifies
Subscription tracking from two Set<amqp.Message> (inFlightMessages,
abandonedMessages) to a single inFlightCount.

Also: a message the broker already buffered (prefetch > 1) before
cancel-ok landed is now rejected+requeued immediately instead of running
its handler, so it can't add to the drain wait for work that was never
going to be allowed to finish on this consumer.
@ramhr
ramhr marked this pull request as ready for review August 19, 2026 14:59
@ramhr
ramhr requested review from shamil and yosiat as code owners August 19, 2026 14:59
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add graceful RabbitMQ shutdown and acknowledgement flushing

✨ Enhancement 🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Adds idempotent graceful shutdown across consumers, RPC waiters, channels, and connections.
• Flushes channel acknowledgements before socket teardown to prevent shutdown redeliveries.
• Documents and comprehensively tests cancellation, draining, closure, and TypeScript APIs.
Diagram

sequenceDiagram
  actor App
  participant ArnavMQ
  participant Consumer
  participant Producer
  participant Connection
  participant Channels
  participant RabbitMQ
  App->>ArnavMQ: close()
  ArnavMQ->>Consumer: stop()
  Consumer->>RabbitMQ: basic.cancel
  Consumer-->>ArnavMQ: handlers drained
  ArnavMQ->>Producer: stop RPC waiters
  Producer-->>App: reject pending RPCs
  ArnavMQ->>Connection: close()
  Connection->>Channels: closeAll()
  Channels->>RabbitMQ: Channel.Close
  RabbitMQ-->>Channels: Close-Ok
  Connection->>RabbitMQ: close socket
  ArnavMQ-->>App: shutdown complete
Loading
High-Level Assessment

The staged cancel, drain, RPC rejection, channel flush, and socket-close sequence is appropriate. Cancelling by consumer tag preserves shared channels, while Channel.Close/Close-Ok provides the necessary AMQP ordering barrier that raw acknowledgement calls cannot provide. Closing shared channels earlier or imposing a library-level drain timeout would either disrupt unrelated consumers or abandon active work, so the current approach is preferable.

Files changed (14) +1810 / -76

Enhancement (9) +503 / -68
index.jsExport ConnectionClosedError at runtime +6/-0

Export ConnectionClosedError at runtime

• Exposes ConnectionClosedError from the package entry point so callers can use constructor and instanceof checks.

src/index.js

arnavmq.jsOrchestrate top-level graceful shutdown +33/-1

Orchestrate top-level graceful shutdown

• Adds close() to stop consumers, reject producer RPC waiters, and finally close the connection. Exposes consumer shutdown methods and restores the testable ArnavMQ class export.

src/modules/arnavmq.js

connection.jsAdd terminal, idempotent connection closure +91/-0

Add terminal, idempotent connection closure

• Introduces close(), isClosed, and ConnectionClosedError. Connection shutdown waits for cached channels, handles in-progress connection attempts, suppresses teardown failures, and prevents reconnection afterward.

src/modules/connection.js

consumer.jsTrack subscriptions and drain active handlers +242/-56

Track subscriptions and drain active handlers

• Replaces implicit subscription state with explicit records containing consumer tags, channel listeners, cancellation state, and in-flight counts. Adds cancellation, draining, stopping, resubscribe guards, and immediate requeueing of post-cancellation buffered deliveries.

src/modules/consumer.js

utils.jsAdd reusable promise timeout helper +22/-0

Add reusable promise timeout helper

• Adds withTimeout() for bounding channel-open and channel-close waits while reliably clearing its timer.

src/modules/utils.js

index.d.tsExport ConnectionClosedError type +11/-2

Export ConnectionClosedError type

• Re-exports ConnectionClosedError through the package-level TypeScript namespace alongside existing public types.

types/index.d.ts

arnavmq.d.tsDeclare top-level and consumer shutdown APIs +11/-0

Declare top-level and consumer shutdown APIs

• Adds declarations for close(), cancel(), stop(), drain(), and inFlight() on the public ArnavMQ interface.

types/modules/arnavmq.d.ts

connection.d.tsDeclare terminal connection lifecycle APIs +34/-1

Declare terminal connection lifecycle APIs

• Defines ConnectionClosedError and adds close() and isClosed to both the Connection class and singleton connection interface.

types/modules/connection.d.ts

consumer.d.tsDeclare subscription and draining state +53/-8

Declare subscription and draining state

• Adds the Subscription state type and consumer shutdown method declarations. Updates consume and subscribe results to boolean because cancellation can prevent successful registration.

types/modules/consumer.d.ts

Bug fix (2) +101 / -7
channels.jsFlush cached channels before connection teardown +57/-6

Flush cached channels before connection teardown

• Adds bounded, best-effort closure of every cached channel before socket shutdown. Channel.Close/Close-Ok acts as a barrier ensuring prior acknowledgement and rejection frames reached RabbitMQ.

src/modules/channels.js

producer.jsTerminate pending RPC work during shutdown +44/-1

Terminate pending RPC work during shutdown

• Adds idempotent producer stopping that rejects pending RPC waiters and clears their timers. Treats terminal connection closure as non-retryable and prevents RPC queue reinitialization loops.

src/modules/producer.js

Tests (1) +1188 / -0
shutdown-spec.jsCover graceful shutdown and AMQP teardown races +1188/-0

Cover graceful shutdown and AMQP teardown races

• Adds extensive unit and real-broker coverage for cancellation races, listener hygiene, draining, idempotency, RPC rejection, terminal connections, channel flush ordering, and redelivery prevention.

test/shutdown-spec.js

Documentation (1) +17 / -0
README.mdDocument graceful-shutdown APIs +17/-0

Document graceful-shutdown APIs

• Adds usage guidance for top-level close and lower-level consumer cancellation, draining, stopping, and in-flight inspection. Clarifies that draining has no timeout and shutdown is idempotent.

README.md

Other (1) +1 / -1
package.jsonRelease graceful shutdown as version 0.18.0 +1/-1

Release graceful shutdown as version 0.18.0

• Bumps the package version from 0.17.2 to 0.18.0 for the additive public API release.

package.json

@qodo-code-review

qodo-code-review Bot commented Aug 19, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Channels escape close barrier ✓ Resolved 🐞 Bug ≡ Correctness
Description
closeAll() snapshots and clears the channel cache before awaiting closure, while an
already-started getChannel() or getDefaultChannel() that passed getConnection() before
close() can resume afterward and insert a new channel into the cleared map. Because that late
channel is absent from the snapshot, _close() can close the raw connection without its pending
publish/ack or other fire-and-forget frames being protected by the Channel.Close-Ok barrier,
reintroducing the shutdown race this PR intends to eliminate.
Code

src/modules/channels.js[R114-118]

+  async closeAll() {
+    const entries = [...this._channels.entries()];
+    this._channels.clear();
+
+    await Promise.all(entries.map(([key, entry]) => closeChannel(key, entry)));
Evidence
getChannel() and getDefaultChannel() have an await boundary after their only closed-state check:
they await getConnection() and then access the mutable channel registry without rechecking
terminal shutdown. Meanwhile, shutdown awaits the connection, snapshots and clears the map, and then
closes the socket; because the map's _get() can insert a new entry after that snapshot, a resumed
channel acquisition can create an entry that closeAll() never closes.

src/modules/connection.js[79-125]
src/modules/connection.js[158-166]
src/modules/channels.js[62-94]
src/modules/channels.js[114-119]
src/modules/producer.js[145-152]
src/modules/connection.js[58-69]
src/modules/connection.js[158-165]
src/modules/channels.js[80-95]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description

Connection shutdown snapshots and clears cached channels, but a channel acquisition already in progress can resume afterward and add a channel that is never gracefully closed or flushed before the raw socket closes.

## Issue Context

`getChannel()` and `getDefaultChannel()` await `getConnection()` before accessing `this._channels`, and `close()` cannot revoke a caller that already passed the initial closed-state check. Closing must atomically prevent new channel creation, including operations that passed `getConnection()` before `close()` set `isClosed`; ensure channel acquisition rechecks terminal shutdown after awaits, or serialize channel creation with the shutdown barrier and close any late-created channel before socket teardown.

## Fix Focus Areas

- src/modules/connection.js[79-125]
- src/modules/connection.js[158-166]
- src/modules/channels.js[80-95]
- src/modules/channels.js[114-118]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Cancellation race leaks listeners 🐞 Bug ☼ Reliability
Description
If cancellation occurs while _initializeChannel() is awaiting getChannel(),
_cancelSubscription() returns before a channel/listener exists; initialization then attaches a new
close listener to the already-cancelled record and no later path removes it. Repeated races on the
shared default channel accumulate inert listeners and can trigger EventEmitter leak warnings.
Code

src/modules/consumer.js[R208-215]

+      const onChannelClose = () => {
+        if (!this._isLive(subscription)) {
+          return;
+        }
+        this._subscribe(subscription);
+      };
+      channel.addListener('close', onChannelClose);
+      subscription.channel = channel;
Evidence
Cancellation marks the record and returns when subscription.channel is still null. After the
pending acquisition resolves, _initializeChannel() attaches the listener without rechecking
liveness, and _subscribe() later returns false without removing it.

src/modules/consumer.js[189-216]
src/modules/consumer.js[401-410]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A cancellation racing pending channel acquisition can leave an inert close listener attached to the shared channel.

## Issue Context
After `getChannel()` resolves, recheck liveness before attaching the listener, or guarantee cleanup on every early return from `_subscribe()`.

## Fix Focus Areas
- src/modules/consumer.js[199-218]
- src/modules/consumer.js[401-410]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Cancelled subscriptions accumulate ✓ Resolved 🐞 Bug ➹ Performance
Description
Every subscribe() permanently appends a record containing its callback, options, channel, and
consumer tag, while cancel() only marks it and never removes or prunes it. Applications that
dynamically subscribe, cancel, and resubscribe will therefore grow _subscriptions and make
cancelAll()/inFlight() increasingly expensive for the lifetime of the process.
Code

src/modules/consumer.js[R138-148]

+    const subscription = {
+      queue,
+      options,
+      callback,
+      channel: null,
+      consumerTag: null,
+      onChannelClose: null,
+      cancelled: false,
+      inFlightCount: 0, // handlers currently running, not yet acked/rejected.
+    };
+    this._subscriptions.push(subscription);
Evidence
Subscription records are pushed on every registration and all counting/cancellation walks the full
array. Cancellation only flips a flag and cancels the broker tag; there is no removal path, and the
new test explicitly confirms cancelled records remain in the registry.

src/modules/consumer.js[138-148]
src/modules/consumer.js[385-421]
src/modules/consumer.js[463-464]
test/shutdown-spec.js[233-249]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Cancelled subscription records are retained forever, causing unbounded memory and linear shutdown/counting overhead under repeated subscribe/cancel cycles.

## Issue Context
A cancelled record may need to remain while its handlers are in flight, but it can be removed once cancellation is complete and its count reaches zero.

## Fix Focus Areas
- src/modules/consumer.js[138-148]
- src/modules/consumer.js[385-421]
- src/modules/consumer.js[463-464]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Timeout errors are suppressed ✓ Resolved 🐞 Bug ◔ Observability
Description
withTimeout() returns from its finally block, overriding successful results and rejections from
Promise.race() with undefined, so channel.close() failures and five-second timeouts never
reach closeChannel()'s warning path. Shutdown still waits for the race to settle and closeAll()
can continue, but operators receive no indication that the best-effort ack-flush barrier failed
before the socket closed.
Code

src/modules/utils.js[R38-42]

+  try {
+    return await Promise.race([promise, timeout]);
+  } finally {
+    return clearTimeout(timeoutId);
+  }
Evidence
The helper races the operation against a rejecting timeout, but returning clearTimeout(timeoutId)
from finally determines the async function's result and overrides the awaited race outcome.
Because closeChannel() relies on a rejection from this helper to invoke its warning logger, both a
rejected channel.close() and a timeout are converted into silent successful resolutions.

src/modules/utils.js[32-42]
src/modules/channels.js[18-33]
src/modules/channels.js[114-119]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description

`withTimeout()` returns `clearTimeout(timeoutId)` from `finally`, replacing the fulfilled value or rejection from `Promise.race()`. As a result, channel-close failures and timeout errors silently resolve instead of reaching `closeChannel()`'s warning handler.

## Issue Context

`closeChannel()` deliberately catches and logs failures while allowing `closeAll()` to continue, and shutdown still waits for the race to settle. The helper must preserve the race result or rejection while always clearing its timer; the `finally` block should only clear the timer and must not return a value.

## Fix Focus Areas

- src/modules/utils.js[32-42]
- src/modules/channels.js[18-33]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (2)
5. Queue filter is ignored ✓ Resolved 🐞 Bug ≡ Correctness
Description
The declared inFlight(queue?) API promises a queue-specific count when a queue is supplied, but
the runtime implementation ignores the argument and always sums every subscription. TypeScript
callers can therefore wait on or report the wrong queue's workload whenever another queue has an
active handler.
Code

src/modules/consumer.js[R463-464]

+  inFlight() {
+    return this._subscriptions.reduce((total, sub) => total + sub.inFlightCount, 0);
Evidence
The public declaration accepts an optional queue and explicitly assigns it queue-filter semantics,
while the runtime method accepts no parameter and unconditionally reduces the entire subscription
registry.

types/modules/consumer.d.ts[90-94]
src/modules/arnavmq.js[73-80]
src/modules/consumer.js[459-464]
src/modules/consumer.js[459-465]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description

The TypeScript API declares that `Consumer.inFlight(queue)` returns the count for only the supplied queue, but the JavaScript implementation accepts no parameter and sums all subscriptions.

## Issue Context

Keep the runtime implementation and public declaration aligned. Either update the implementation to preserve global counting when no queue is supplied and filter subscriptions when one is provided, or remove the unsupported parameter and its queue-specific documentation from the declaration.

## Fix Focus Areas

- src/modules/consumer.js[459-465]
- types/modules/consumer.d.ts[90-94]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Late RPC waiters leak ✓ Resolved 🐞 Bug ☼ Reliability
Description
Producer.stop() scans pending waiters only once and _shuttingDown does not gate RPC setup, so a
checkRpc() already awaiting queue initialization can register a correlation waiter after the scan.
The subsequent publish fails as connection.close() starts, but that failure path never removes the
newly registered waiter or timerless deferred, leaving it and its request data retained permanently.
Code

src/modules/producer.js[R186-190]

+  stop() {
+    if (this._shuttingDown) {
+      return;
+    }
+    this._shuttingDown = true;
Evidence
checkRpc() awaits asynchronous queue initialization before inserting the waiter, while stop()
only iterates entries present at that instant. It explicitly uses _shuttingDown only for
idempotency, and waiter cleanup/timeout setup occurs only after a successful publish, so a
shutdown-induced send failure leaves the late entry behind.

src/modules/producer.js[37-39]
src/modules/producer.js[97-141]
src/modules/producer.js[186-203]
src/modules/producer.js[214-237]
src/modules/arnavmq.js[56-60]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An RPC operation can insert its waiter after `stop()` has completed its one-time cleanup, then leak that waiter when the shutdown-induced publish failure occurs.

## Issue Context
Gate RPC setup after every relevant await and clean up the correlation entry whenever publishing fails.

## Fix Focus Areas
- src/modules/producer.js[186-203]
- src/modules/producer.js[214-237]
- src/modules/arnavmq.js[56-60]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 1 rule
Review mode: 🧠 Deep: This is a broad, behaviorally dense shutdown change spanning consumer concurrency, connection/channel teardown, producer RPC rejection, public APIs, and many independent race-sensitive paths where a redundant review could catch subtle defects.

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/modules/channels.js
…crash

Container was dying before the 10s health-check window elapsed, causing
flaky 'container is not running' failures. Detect an exited container
immediately (dumping its logs) instead of blindly retrying, and give
a healthy boot more time (40 x 1s).
getChannel()/getDefaultChannel() awaited getConnection() then touched
_channels with no recheck in between. close() can run entirely in
that gap, snapshotting/clearing the channel cache in closeAll() before
the resumed call inserts a new channel - never covered by the
Channel.Close-Ok barrier, and crashing with a null deref once _close()
finishes nulling _channels.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
nir-ben-menachem

This comment was marked as resolved.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds graceful RabbitMQ shutdown while preserving in-flight message handling and preventing acknowledged-message redelivery.

Changes:

  • Adds consumer cancellation/draining and producer RPC cleanup.
  • Adds terminal connection closure with channel flushing.
  • Adds public APIs, typings, documentation, and shutdown tests.

Reviewed changes

Copilot reviewed 11 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/modules/consumer.js Tracks subscriptions and in-flight handlers.
src/modules/connection.js Adds terminal connection closure.
src/modules/channels.js Flushes cached channels before disconnecting.
src/modules/producer.js Rejects pending RPC waiters.
src/modules/arnavmq.js Orchestrates graceful shutdown.
src/modules/utils.js Adds a timeout helper.
src/index.js Exports ConnectionClosedError.
types/modules/consumer.d.ts Declares consumer shutdown APIs.
types/modules/connection.d.ts Declares connection closure APIs.
types/modules/arnavmq.d.ts Declares top-level shutdown support.
types/index.d.ts Exports the new error type.
test/shutdown-spec.js Adds extensive shutdown coverage.
test/docker.js Extends broker readiness checks.
README.md Documents graceful shutdown.
package.json Bumps the version to 0.18.0.
Suppressed comments (1)

src/modules/consumer.js:465

  • The new declaration and JSDoc promise queue-scoped counts, but the implementation ignores its argument. With handlers active on queues A and B, inFlight('A') currently returns both counts, which makes the exposed API report incorrect state.
  inFlight() {
    return this._subscriptions.reduce((total, sub) => total + sub.inFlightCount, 0);
  }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/modules/utils.js Outdated
Comment thread src/modules/arnavmq.js
Comment on lines 73 to +79
const consumer = {
consume: instance.consume.bind(instance),
subscribe: instance.subscribe.bind(instance),
cancel: instance.consumer.cancel.bind(instance.consumer),
stop: instance.consumer.stop.bind(instance.consumer),
drain: instance.consumer.drain.bind(instance.consumer),
inFlight: instance.consumer.inFlight.bind(instance.consumer),
Comment thread src/modules/producer.js Outdated
Comment on lines +186 to +190
stop() {
if (this._shuttingDown) {
return;
}
this._shuttingDown = true;
Comment thread src/modules/consumer.js
cancelled: false,
inFlightCount: 0, // handlers currently running, not yet acked/rejected.
};
this._subscriptions.push(subscription);
ramhr and others added 6 commits August 31, 2026 15:01
close() awaited consumer.stop() (cancel + unbounded drain) before
producer.stop(), so a handler parked on an RPC response could never be
released: drain() waited on the handler, only producer.stop() could reject
the waiter the handler was blocked on, and producer.stop() sat behind the
drain. With the default rpcTimeout the RPC timer broke the cycle from
outside after 15s per parked handler; with rpcTimeout: 0 no timer is ever
armed, so close() never resolved at all and every later close() awaited the
same memoized promise until SIGKILL.

producer.stop() now runs first. It is synchronous, so it and the consumer's
shutting-down latch flip in the same turn of the loop, leaving no window
between them. Only new outgoing RPC requests fail fast from that point:
non-RPC publishes still go out while draining, and consumer.js's checkRpc
writes RPC replies straight to the channel rather than through the producer,
so a draining handler can still answer a request it had already received.

Moving producer.stop() to the front exposes an existing window in checkRpc,
which registers its waiter before awaiting the publish and only attaches a
handler to it afterward. A stop() landing in that window rejected a deferred
with no handler attached, and Node's default --unhandled-rejections=throw
turns that into process death during the very shutdown that rejected it. A
no-op catch at registration closes the window without changing what the
caller sees, and a catch around the publish deletes the registry entry so a
failed publish no longer orphans a waiter that nothing can ever settle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
js.configs.recommended was passed to FlatCompat as its recommendedConfig
option, which only tells FlatCompat how to resolve `extends:
'eslint:recommended'` inside a legacy config - it enables nothing on its
own. The exported array extended eslint-config-prettier alone, so the
recommended set was entirely off and `npm run lint` enforced only the
explicit rules block. A probe file with both a `return` inside a `finally`
and an unused variable linted clean.

That is how the utils.withTimeout bug reached master: `return
clearTimeout(timeoutId)` in a finally overrode the try's completion, so the
helper always resolved undefined and silently swallowed every rejection
routed through it, including the channel-close timeout it existed to
surface. no-unsafe-finally catches it, and now errors on it.

Enabling it surfaced two real violations, both fixed here:

- connection.js `_close()` assigned `connection = null` in the catch when
  the initializer already covers that path (the await it guards never
  completes), flagged by no-useless-assignment.
- docker.js `isRunning()` bound an error it deliberately never inspects;
  switched to an optional catch binding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ose()

close() is now just the two steps that shutdown actually needs:

  await this.consumer.stop();     // stop consuming, drain in-flight handlers
  await this.connection.close();

producer.stop() is gone. Failing pending RPC requests was never a shutdown
step - it is what has to happen whenever the channel goes away, because the
reply queue was exclusive to that connection and every request was published
carrying that queue's name as its replyTo, so no answer can arrive even after
a reconnect. Moving it onto a channel 'close' listener covers a deliberate
close() and a connection lost on its own with the same code, and drops both
producer._shuttingDown and the checkRpc guard that consulted it.

That listener replaces the one registered per RPC queue, which fire-and-forgot
a queue rebuild on every channel close. Remove-then-add with a bound reference
keeps it at exactly one listener on the channel however many RPC queues are
initialized, and re-arms it on the fresh channel after a reconnect. The rebuild
is now lazy - the next RPC publish calls createRpcQueue() anyway - so a whole
fire-and-forget promise chain goes away with it.

prepareTimeoutRpc() and checkRpc()'s cleanup now index amqpRPCQueues with `?.`:
the sweep drops whole per-queue entries, so a close landing while a request was
still being published leaves nothing to find, and the waiter it would have timed
out has already been rejected.

Consequences, both previously untested and now covered:

- A connection that dies on its own no longer leaves RPC callers hanging out
  rpcTimeout, or forever with rpcTimeout: 0. Removing the listener makes both
  new tests time out.
- A handler parked on an RPC response nobody is left to answer holds the drain
  open until the orchestrator's kill grace period. That is deliberate and now
  documented: the connection stays open for the whole drain precisely so a
  handler can finish its work, including a new RPC round-trip, and there is no
  drain timeout by design.

consumer._stopPromise stays. It is the same idempotency idiom as
Connection._closePromise, not a mode flag, and removing it needs
_cancelSubscription to be idempotent - which breaks _consumeQueue's deferred
re-cancel for a subscription cancelled mid-flight, whether that is spelled as an
early return on `cancelled` or by clearing consumerTag.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… flag

Three cleanups, no behavior change:

Removed the dead catch around `channels.closeAll()` in `Connection._close()`.
`closeAll()` cannot reject - it is a `Promise.all` over `closeChannel()`, which
wraps its whole body in try/catch and only logs - so the handler was
unreachable, and it was the last uncovered branch in connection.js. The local
snapshot of `this._channels` went with it: `this._channels?.closeAll()` is a
single expression, so nothing can interleave between the null check and the
call.

Dropped `Consumer._shuttingDown` in favour of the already-present
`_stopPromise`, which is the same "once set, never unset" signal.  `_isLive()`
was the flag's only reader, and `_stopPromise` is truthy by the time anything
can read it: `_cancelAll()` is only reachable through `stop()`, and its
synchronous prefix - which runs as the right-hand side of the
`_stopPromise = ...` assignment, before the memo is set - never consults
`_isLive()`; it only marks every existing subscription `cancelled`, which
already makes them non-live. That constraint is now noted on `stop()`, since an
`_isLive()` check added inside `_cancelAll()` later would read stale.

Broke `_consumeQueue()` up. It was 108 lines wrapped around a nested async
closure with two levels of try/catch; it is now the `channel.consume()` call
and its error log, with the delivery path in four named methods:
`_onDelivery()` (server-side cancel, the not-live reject, and the
`inFlightCount` bookkeeping - the only place that counter moves),
`_rejectAfterShutdown()` (synchronous now, it never needed to be async),
`_processMessage()`, and `_ackMessageAfterProcess()`, which pairs with the
existing `_rejectMessageAfterProcess()`. Subscribe's two call shapes now
resolve in a module-level `resolveSubscribeArgs()`, so `subscribe()` no longer
reassigns its parameters and the file-wide `no-param-reassign: "off"` escape
hatch is gone.

Also corrected three stale comments: the subscription registry claimed to be
"keyed by an incrementing id" when it is a plain array whose records have no
id; DRAIN_POLL_INTERVAL_MS referenced a `drain()` that is named `_drain()`; and
`_cancelSubscription()`'s guard comment described cancellation when the guard
actually exists because `removeListener()` throws on an undefined listener.

Two tests updated for the flag removal: the one that called `_cancelAll()`
directly and asserted `_shuttingDown` now goes through `stop()` (that path no
longer marks shutdown on its own), and a redundant flag assertion in stop()'s
clean-shutdown test is dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ramhr
ramhr force-pushed the feat/graceful-shutdown branch from 8520af8 to 2a81da7 Compare September 2, 2026 14:11
ramhr and others added 3 commits September 2, 2026 18:23
`_onChannelClose()` drops the whole per-queue entry from `amqpRPCQueues`, but
`checkRpc()` had two awaits between creating that entry and registering its
waiter in it, and read the entry back on both sides of them. A close landing in
that window left it indexing a key that was gone:

    TypeError: Cannot read properties of undefined (reading 'resQueuePromise')

This is the same defect the `?.` guards in `prepareTimeoutRpc()` fixed a couple
of commits ago; these two sites were missed.

The caller getting a TypeError is not just cosmetic. The right error for that
window is the publish's own failure against the dead channel
(`IllegalOperationError: Channel closed`), which unlike `ConnectionClosedError`
is retryable - so `_sendToQueue()` reconnects and republishes rather than losing
a real message. A TypeError happens to be retryable too, so it self-heals with
`producerMaxRetries: -1`, but with retries capped the publish fails for the
wrong reason.

`replyTo` now comes from `createRpcQueue()`'s return value instead of re-reading
the registry for the promise it already resolved, and the waiter registration
recreates the entry with `??=` if the sweep took it. One redundant await goes
away with it.

Covered by a new test that drives a close into exactly that window; it fails
with the TypeError above when the fix is reverted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the remaining actionable findings from the PR review.

**A queue that cannot be declared no longer spins.** `assertQueue`'s error was
logged and stepped over, so setup continued to `basic.consume` on a channel the
broker had already killed with its channel-level error; that failed too, was
also only logged, and `_subscribe` then returned `_isLive()` - `true`. The dead
channel's own 'close' event then resubscribed with no delay at all, opening a
fresh channel and re-declaring the queue as fast as the broker could answer.
Measured against a real broker with a queue pre-declared `durable: true` and
subscribed as `durable: false`: `subscribe()` resolved `true` in 9ms and the
process opened **1322 channels in the next 2 seconds**, indefinitely, while the
caller believed it was consuming. That is the common case for any queue-argument
change (adding `x-queue-type`, a TTL) on the next deploy.

A failed declaration is now reported rather than swallowed, and every
no-usable-channel path routes through one `_retrySubscribe()` that waits
`config.timeout` first. The same measurement now opens 19 channels in 2 seconds
at a 100ms backoff - exactly the configured rate.

The resubscribe-on-close listener is gated on `consumerTag` so that it and
`_retrySubscribe` cannot both drive one subscription: two loops, each opening a
channel whose death starts another, would double every cycle rather than merely
spin. A genuine reconnect has a tag and still resubscribes immediately, which is
what `disconnect-spec.js` covers.

**`subscribe()` no longer resolves `true` for a subscription that is not
consuming.** `true` now means the broker confirmed the consumer. A refused
`basic.consume` - an exclusive-consumer conflict, `ACCESS_REFUSED` - resolves
`false` rather than retrying, since that is about this consumer and not the
channel. A queue that cannot be declared leaves the promise pending while it
retries: a service awaiting it does not come up, instead of reporting ready and
consuming nothing. This PR had already widened the declared type from
`Promise<true>` to `Promise<boolean>` without saying what `false` meant; both
the JSDoc and the `.d.ts` now do.

**`subscribe()` after a shutdown rejects** with `ConnectionClosedError` instead
of quietly resolving `false`. Shutdown is terminal and the connection hands back
the same closed instance on a repeat configure, so such a subscribe can never
consume - and `publish()` already failed loudly on exactly that state. Resolving
`false` let a service boot "successfully", report healthy, and consume nothing
for the rest of its life.

**The shutdown requeue now reports itself** to `afterProcessMessageEvent` with a
new documented `requeued: true`. Rejecting a delivery the broker had buffered
before cancel-ok landed was invisible to the per-message hooks, so on every
rolling deploy up to `prefetch` messages per consumer disappeared from the
instrumentation and received/completed counters silently disagreed by that many.
No `beforeProcessMessageEvent` is emitted: that hook exists to wrap
`action.callback`, which never runs here, so instrumentation closing its span
from inside the wrapper would leak one span per requeued message.

Two comments were also load-bearing and wrong. `Channels.closeAll()` claimed
every frame previously written is guaranteed processed; that holds for acks and
rejects, which go out via amqplib's `sendImmediately`, but a synchronous method
parked in the channel's `pending` list is jumped by `Channel.Close` and then
discarded by `toClosed()` -> `_rejectPending()` - which is where a
"Failed to cancel consumer" log comes from in that case: local frame dropping,
not a broker refusal. And `_drain()` justified polling by saying a deferred
"would resolve on the first momentary zero", which is self-refuting since
polling has the same exit condition, merely sampled; the real reason it is safe
is that `stop()` cancels first, after which `inFlight()` only decreases.

Finally, the README claimed `connection.close()` was not exposed. It is, so it
now says what that call actually does - closes the socket without cancelling or
draining - and that `arnavmq.close()` is terminal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… after

Correction to the previous commit, which emitted only `afterProcessMessageEvent`
for a delivery requeued during shutdown, on the reasoning that instrumentation
wrapping `action.callback` in the before hook would leak a span when that
callback never runs.

Checked against the actual consumer: `@bringg/instrumentation-arnavmq` *starts*
the span in `beforeProcessMessage` and stores it on `message.properties`, and its
`afterProcessMessageHook` ends whatever it finds there, early-returning when
there is nothing. The `context.bind` on `action.callback` only propagates trace
context - it does not end the span. So the after event on its own does nothing
at all: verified by running the real compiled hooks against this working copy
with an in-memory span exporter, which recorded 0 spans for a requeued message.
Emitting both records 1 span, properly parented off the producer's context from
the message headers, and ended.

Wrapping is safe here for the same reason the leak was imagined: the span ends
from the after hook. The hook already documents a before event that skips
processing (a `beforeProcessMessage` returning `false`), so a callback that is
not invoked is a shape hook authors are expected to handle - now stated
explicitly in both the JSDoc and the `.d.ts`, along with `action.content` being
absent on this path.

`requeued?: boolean` is also added to the `AfterConsumeInfo` type, so a
TypeScript consumer can actually branch on the field the previous commit started
sending.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/modules/consumer.js
// ConnectionClosedError; resolving `false` here (as this used to) let a service boot
// "successfully", report healthy, and consume nothing for the rest of its life.
if (this._stopPromise || this._connection.isClosed) {
throw new ConnectionClosedError();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a "reason" field to the ConnectionClosedError so we know if it's closed because we are shutting down or another error.

@yosiat yosiat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The high level logic make sense to me and looks ok.
Having said that, I think the shape of the codebase is overly complex and not because of this change, because of technical debt that is accumulated in this project.

We need to invest time to do internal refactoring here.

Comment thread src/modules/connection.js Outdated
Comment thread src/modules/connection.js
Comment thread src/modules/consumer.js
@yosiat yosiat assigned ramhr and unassigned yosiat Sep 3, 2026
There is nothing left to close on that path, so skip the connection/channel
teardown below rather than falling through it.
…nect

Callers rejected with this error could not tell a deliberate close() apart
from the connection/channel dying on its own. The producer's channel-close
listener now derives it from Connection.isClosed at the moment it fires,
since that same listener runs on both paths; every other throw site is
already gated on a shutdown check, so it defaults to 'shutdown'.
Replaces the bare {object} annotations with a typed shape, since the
codebase isn't on TypeScript for these internals.
A silent connection drop was rejecting waiters with no trace in the logs.
Count them per _onChannelClose() sweep and log the count plus origin.
@ramhr
ramhr force-pushed the feat/graceful-shutdown branch from 0783cf5 to 820ffea Compare September 9, 2026 13:43
@ramhr
ramhr merged commit 6ba18bb into master Sep 9, 2026
5 of 8 checks passed
@ramhr
ramhr deleted the feat/graceful-shutdown branch September 9, 2026 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants