Skip to content

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

Merged
kamilmysliwiec merged 2 commits into
nestjs:masterfrom
MingLin1995:fix/microservices-kafka-request-response-end-hook
Sep 17, 2026
Merged

kamilmysliwiec merged 2 commits into
nestjs:masterfrom
MingLin1995:fix/microservices-kafka-request-response-end-hook

Conversation

@MingLin1995

Copy link
Copy Markdown
Contributor

PR Checklist

PR Type

  • Bugfix

What is the current behavior?

Issue Number: N/A

Follow-up to #17794, which moved the processing 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 — a stream of three responses closes it three times
  • combineStreamsAndThrowIfRetriable rejects on a KafkaRetriableException,
    so this.send is never reached and the span stays open. kafkajs then
    redelivers the message, so every retry leaks another span
  • the NO_MESSAGE_HANDLER path publishes without ever running a start hook,
    so that end hook call is unpaired (the same unpaired call fix(microservices): close the request-response span exactly once #17794 removed
    from mqtt, nats and redis)

What is the new behavior?

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.
Error propagation is unchanged — the KafkaRetriableException still reaches
kafkajs so the message is redelivered.

Six specs were added for handleMessage on the request-response path. Three
of them fail against the current code and pin the bugs above (3 end hook calls
for a three-value stream, 0 for a retriable rejection, 1 unpaired call with no
handler); the other three are regression guards for paths that already worked.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

One behavioural note worth flagging: the span now closes once processing has
settled rather than after the last reply has actually been produced to Kafka.
That is what #17794 already does on the other transports, so this brings kafka
in line with them, but it does differ from the previous kafka-only timing.

MingLin1995 and others added 2 commits September 17, 2026 18:35
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
kamilmysliwiec merged commit 7f98a6a into nestjs:master Sep 17, 2026
4 checks passed
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.

2 participants