Fix memory leak when taking service responses addressed to other clients

[pavlo-yashchuk]

Description

rmw_take_response_request drains the reply reader in a loop, deserializing each sample into the same ros_data and skipping replies that belong to other clients of the same service (GUID mismatch). The message content of the skipped iteration was never finalized, so the next deserialization overwrote sequence/string pointers, leaking the payload.

In our deployment, three services are polling the same ros image service; each service leaked ~13-30 mb/min. With these changes, ram usage is stable even after 5h

Is this user-facing behavior change?

Did you use Generative AI?

yes, was diagnosed and patch drafted with Claude code (Fable 5)

Additional Information

The leak needs two or more processes holding clients for the same service - single-client tests never hit it, since the caller always frees the last deserialized sample

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[pavlo-yashchuk]

@jmachowinski
I reopened it with the right branch. Sorry for missing

[fujitatomoya]

@pavlo-yashchuk it does not look like comments on #589 are addressed in this PR? can you confirm?

[jmachowinski]

Please update the description of the PR to use the default form to avoid future confusion. I also need a way to reproduce the memory leak.

I still believe that this is not the correct fix, and that we should rather check the deserialization or the runtime functions.

[pavlo-yashchuk]

hey @jmachowinski yep, you are right, and it sits in desirialization. Ive asked claude to track changes and find the fix:

The leak is in the old C-introspection deserializer, TypeSupport_impl.hpp, in the primitive-sequence branch of deserialize_field<T>():

auto & data = *reinterpret_cast<typename GenericCSequence<T>::type *>(field);                                                                                                                                                                                                                            
deser >> dsize;                                                                                                                                                                                                                                                                                          
if (!GenericCSequence<T>::init(&data, dsize)) { ... }   // rosidl_runtime_c__<T>__Sequence__init — no __fini first                                                                                                                                                                                       

It calls rosidl_runtime_c__<T>__Sequence__init directly, with no preceding __fini. When a message is deserialized into more than once (the reply take-loop reusing ros_data across the GUID skip), __init allocates a new buffer over the old pointer and the previous payload leaks — the
documented "init on an already-initialized sequence leaks memory." The string branch goes through resize_field (fini + init), which is why only primitive sequences leak, i.e. exactly the image data. Matches the trace: deserializeROSmessage → …__octet__Sequence__init.

This is already handled on the newer branches by #575, which routes deserialization through cdr_reader->deserialize (Serialization.cpp) → resize() (fini + init), freeing the old buffer before reallocation regardless of reuse. Confirmed empirically:

build	3 clients, shared reply topic	result
jazzy 2.2.3	+8.6 GB	leaks
jazzy branch HEAD (with the #553/#562 backports)	+14.6 GB	still leaks
rolling/lyrical 4.1.4 (post-#575)	+12 MB flat	fixed

So rolling/lyrical need nothing. Jazzy is the problem: it only got the #553/#562 perf backports, which kept the old deserialize_field, so it still leaks. Since #575 replaces the whole deserializer (TypeSupport_impl.hpp deleted, ~1.9k lines reworked), backporting it to an LTS branch seems too
invasive — so this PR takes the minimal route: fini + re-init the message between take iterations, restoring the same "finalize before re-deserialize" invariant #575 gets structurally (33 lines, one file, no serialization/ABI changes).

Does that seem like a reasonable jazzy-only fix, or would you prefer backporting the deserializer change itself?