Skip to content

fix(microservices): close the request-response span exactly once - #17794

Merged
kamilmysliwiec merged 1 commit into
masterfrom
fix/microservices-request-response-end-hook
Sep 17, 2026
Merged

kamilmysliwiec merged 1 commit into
masterfrom
fix/microservices-request-response-end-hook

Conversation

@kamilmysliwiec

Copy link
Copy Markdown
Member

PR Checklist

PR Type

  • Bugfix

What is the current behavior?

Issue Number: N/A

Follow-up to #17781, which fixed this leak on the event path. It raised the same
problem on the request-response path; this is that fix.

On the request-response path the end hook lives in the publisher —
ServerMqtt#getPublisher, ServerNats#getPublisher, ServerRedis#getPublisher,
the inline publisher in ServerTCP#handleMessage, and ServerRMQ#sendMessage.
Server#send invokes that publisher once per emitted packet, and only after
await handler(...) has resolved. Two things go wrong:

1. A multi-value response closes the span once per value. A handler
returning an observable of three values publishes three packets, so
onProcessingEndHook runs three times for one request. Measured on master
with ServerTCP:

multi-value: endHook calls = 3 | sendMessage calls = 3
single:      endHook calls = 1 | sendMessage calls = 1

2. A rejecting handler never closes the span at all. The await throws
before the publisher is ever reached, so the span stays open forever:

reject:      endHook calls = 0 | sendMessage calls = 0

Blast radius on the second one is narrow, for the reason #17781 documented:
handlers registered through the DI pipeline are wrapped by RpcProxy, which
turns a rejection into an error observable, so send publishes an { err }
packet and the span closes. What actually rejects is a custom exception filter
that throws and raw handlers registered via addHandler by custom transport
strategies. The first bug has no such escape hatch — any @MessagePattern
returning a multi-value observable hits it.

What is the new behavior?

Both are fixed the way #17781 fixed the event path. The hook moves out of the
publisher and onto the response stream's finalize teardown, guarded by the
shared Server#createProcessingEndHookRunner added in that PR, and the handler
call is wrapped in try/catch so a rejection closes the span before the error
is rethrown. After the change, all four paths close the span exactly once:

multi-value:  endHook = 1, sends = 3
single:       endHook = 1, sends = 1
reject:       endHook = 1, sends = 0
stream-error: endHook = 1, sends = 1

12 specs across the five transports; 10 of them fail on master. (The two that
pass there are the tcp single-value and stream-error cases, which already fired
once.)

This also lines the transports up with where the hook already fires elsewhere —
Server#handleEvent uses finalize, ServerKafka#sendMessage uses .finally(),
and grpc closes in a finally — so "the span closes when the handler's work
finishes" now holds everywhere.

One deliberate side effect: the NO_MESSAGE_HANDLER path in mqtt, nats and
redis no longer fires an end hook. It never ran a start hook, so that call was
unpaired; tcp and rmq already did not fire one there.

The ctx / context parameters of ServerNats#getPublisher,
ServerRedis#getPublisher and ServerRMQ#sendMessage are now unused. I left the
signatures alone since they are public.

Does this PR introduce a breaking change?

  • Yes
  • No

Timing changes for anyone consuming onProcessingEndHook: the hook now fires
when the response stream completes rather than immediately before each write.
That is the same shift #17779 and #17781 made on the other paths.

Other information

Two things I did not change, both worth a separate call:

  1. A rejecting handler still sends no reply. The span now closes, but the
    client waits out its own timeout with nothing on the wire. Publishing an
    error packet on that path would be the complete fix; it changes wire
    behaviour, so it should be its own PR.
  2. Should the NO_MESSAGE_HANDLER path be instrumented at all? Right now it
    sits outside onProcessingStartHook on every transport. Wrapping it would
    give exporters a span for "request arrived, no handler", which seems more
    useful than the current silence — but that is a design decision, not a bug
    fix.

🤖 Generated with Claude Code

follow-up to #17781, which fixed the same leak on the event path

on the request-response path the end hook lives in the publisher
(`ServerMqtt`/`ServerNats`/`ServerRedis#getPublisher`, the inline publisher in
`ServerTCP#handleMessage` and `ServerRMQ#sendMessage`), which `Server#send`
invokes once per emitted packet and only after `await handler(...)` resolves,
so the span is mismanaged in two ways:

- a handler that returns a multi-value observable closes the span once per
  emitted value, so a stream of three responses closes it three times
- a handler that rejects never reaches the publisher at all, so the span stays
  open forever and an APM exporter accumulates spans it never exports

both are fixed the way #17781 fixed the event path: the hook moves out of the
publisher and onto the response stream's `finalize` teardown, guarded by the
shared `Server#createProcessingEndHookRunner`, and the handler call is wrapped
in try/catch so a rejection closes the span before it is rethrown

as a side effect the `NO_MESSAGE_HANDLER` path in mqtt, nats and redis no
longer fires an end hook: it never ran a start hook, so that call was unpaired
(tcp and rmq already did not fire one there)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kamilmysliwiec
kamilmysliwiec merged commit c468b23 into master Sep 17, 2026
5 checks passed
MingLin1995 added a commit to MingLin1995/nest that referenced this pull request Sep 17, 2026
follow-up to nestjs#17794, which moved the end hook onto the response stream for
mqtt, nats, redis, tcp and rmq but left the kafka publisher untouched

`ServerKafka` still runs `onProcessingEndHook` from `sendMessage`, which
`Server#send` invokes once per emitted packet, so the span is mismanaged in
three ways:

- a handler returning a multi-value observable closes the span once per
  published reply
- `combineStreamsAndThrowIfRetriable` rejects on a `KafkaRetriableException`,
  so `this.send` is never reached and the span stays open; kafkajs then
  redelivers the message, and every retry leaks another span
- the `NO_MESSAGE_HANDLER` path publishes without ever running a start hook,
  so that end hook call is unpaired

all three are fixed the way nestjs#17781 and nestjs#17794 did it: the hook moves out of
`sendMessage` and onto the reply stream's `finalize` teardown, guarded by the
shared `Server#createProcessingEndHookRunner`, and the handler call is wrapped
in try/catch so a retriable rejection closes the span before it is rethrown

as on the other transports the span now closes once processing has settled
rather than after the last reply has been produced
kamilmysliwiec added a commit that referenced this pull request Sep 17, 2026
#17797)

* fix(microservices): close the kafka request-response span exactly once

follow-up to #17794, which moved the end hook onto the response stream for
mqtt, nats, redis, tcp and rmq but left the kafka publisher untouched

`ServerKafka` still runs `onProcessingEndHook` from `sendMessage`, which
`Server#send` invokes once per emitted packet, so the span is mismanaged in
three ways:

- a handler returning a multi-value observable closes the span once per
  published reply
- `combineStreamsAndThrowIfRetriable` rejects on a `KafkaRetriableException`,
  so `this.send` is never reached and the span stays open; kafkajs then
  redelivers the message, and every retry leaks another span
- the `NO_MESSAGE_HANDLER` path publishes without ever running a start hook,
  so that end hook call is unpaired

all three are fixed the way #17781 and #17794 did it: the hook moves out of
`sendMessage` and onto the reply stream's `finalize` teardown, guarded by the
shared `Server#createProcessingEndHookRunner`, and the handler call is wrapped
in try/catch so a retriable rejection closes the span before it is rethrown

as on the other transports the span now closes once processing has settled
rather than after the last reply has been produced

* fix(microservices): settle the kafka reply when the stream completes

---------

Co-authored-by: Kamil Myśliwiec <mail@kamilmysliwiec.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant