fix(node): deserialize void response now ignores payload length for all void commands

[oscarArismendi]

Which issue does this PR close?

Closes #3128

Rationale

The issue reported that deserializeVoidResponse had been changed from status === 0 && data.length === 0 to just status === 0, removing the protocol safety check for void commands. However, when I inspected the current code, the data.length === 0 check was already present. Tracing further up the call chain, I found the real root cause: handleResponse was using r.subarray(8) to slice the payload, ignoring the length field the server sends. This meant data could include trailing bytes from the next response in the buffer, making the data.length === 0 check unreliable regardless of whether it was there or not. SendMessages was a separate problem, it used deserializeVoidResponse even though the server returns a non-empty payload for that command. Instead of weakening the global void check to accommodate it, it now has its own deserializeStatusResponse that only checks status === 0

What changed?

handleResponse in client.utils.ts was using r.subarray(8) to slice the payload, ignoring the length field the server sends. This meant data could include trailing bytes from the next response in the buffer.

SendMessages was using deserializeVoidResponse which checks data.length === 0 even though the server returns a non-empty payload for that command. Instead of weakening the global void check, it now has its own deserializeStatusResponse that only checks status === 0.

Local Execution

Unit (npm run test:unit) — all pass. Two new test suites were added to client.utils.test.ts:

handleResponse: proves that with length=0 and trailing bytes, data is now empty
deserializeStatusResponse: proves it returns true with non-empty data (key difference from deserializeVoidResponse)
A deserialize block was also added to send-messages.command.test.ts proving SendMessages accepts non-empty server payloads.

Unit test screenshot

E2E (npm run test:e2e) — 57/58 pass. The one failure (getClusterMetadata) is pre-existing and unrelated, it expects a 2-node cluster which requires IGGY_CLUSTER_ENABLED=true and two running server instances, matching how CI runs it. The TLS test is skipped locally for the same infrastructure reason.

E2E test screenshot

BDD (npm run test:bdd) the main scenario (Create stream and send messages) initially failed because stream IDs were hardcoded as 0 in the step definitions instead of using the dynamically assigned IDs from the server. This was fixed by using this.stream.id and this.topic.id in the step implementations, and making the Given I have no streams in the system step delete any existing streams before asserting a clean state. The scenario now passes end-to-end, including SendMessages against a real server. Four cluster/leader redirection scenarios remain undefined, their step definitions don't exist yet in any SDK.

BDD test screenshot

AI Usage

If AI tools were used, please answer:

1. Which tools? Claude (claude.ai/code)
2. Scope of usage? Used for codebase exploration, understanding the binary protocol and TCP buffer framing, writing unit tests
3. How did you verify the generated code works correctly? Tests were run locally against a real iggy server built from source. Unit tests were written first to prove the bug, then the fix was applied to make them pass. E2E and BDD tests were run against a locally compiled server
4. Can you explain every line of the code if asked? Yes

Notes

During investigation I initially assumed SendMessages required a dedicated deserializer because the server might return a non-empty response payload, the CreateMessage type has a payload field, which led to some confusion and the issue description appear to be pointing in that direction as well. However, that payload is the outgoing message content sent in the request, not the server response.

Both the Rust server source (send_empty_ok_response() in send_messages_handler.rs) and live debug logs (ONDATA object true 8 false exactly 8 bytes = 4 status + 4 length, zero payload) confirm the response is always empty. With handleResponse correctly bounding data by length, SendMessages would work fine with deserializeVoidResponse.

deserializeStatusResponse was kept as a defensive measure in case server behaviour changes in the future, but it is not strictly necessary given the current implementation, just let me know if I should removed it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.75%. Comparing base (5556d30) to head (11a1f64).

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3182      +/-   ##
============================================
- Coverage     74.45%   73.75%   -0.70%     
  Complexity      943      943              
============================================
  Files          1186     1145      -41     
  Lines        106343   101476    -4867     
  Branches      83377    78654    -4723     
============================================
- Hits          79176    74843    -4333     
+ Misses        24426    23979     -447     
+ Partials       2741     2654      -87     

Components	Coverage Δ
Rust Core	74.74% <ø> (-1.00%)	⬇️
Java SDK	62.30% <ø> (+2.15%)	⬆️
C# SDK	69.42% <ø> (+0.03%)	⬆️
Python SDK	81.43% <ø> (ø)
Node SDK	91.53% <100.00%> (+0.12%)	⬆️
Go SDK	39.43% <ø> (-0.18%)	⬇️

Files with missing lines	Coverage Δ
foreign/node/src/client/client.utils.ts	90.19% <100.00%> (+0.29%)	⬆️
...ign/node/src/wire/message/send-messages.command.ts	100.00% <100.00%> (ø)

... and 98 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[hubcio]

@T1B0 mind taking a look?

[T1B0]

i get the limiting buffer size part it's probably safer this way, and the client test is definitly a nice addition ! i don't understand why this MR touch the bdd test, it seems unrelated to the fix part or i'm missing something ?

[oscarArismendi]

i get the limiting buffer size part it's probably safer this way, and the client test is definitly a nice addition ! i don't understand why this MR touch the bdd test, it seems unrelated to the fix part or i'm missing something ?

@T1B0 I modified the BDD scenarios since basic_messaging.feature helps confirm that the changes didn’t alter behavior. I’ve added context in the other threads, but I can revert them if it’s too much for one MR

[T1B0]

please keep this MR minimalist toward the issue goal (no bdd cleanup / no argument prefixing).

[T1B0]

re-reading this, i think this part "[BDD test] initially failed because stream IDs were hardcoded as 0 in the step definitions instead of using the dynamically assigned IDs from the server." is the base of the misunderstanding: stream/topics ID are not hardcoded, they are defined by bdd test scenario and passed as those arguments you prefixed. at the moment BDD test just expect a fresh server (you can restart your iggy server with --fresh opt to do so) so they'll fail if you run them twice without a fresh restart between each runs.
I think this concern should be another issue/MR subject and we should see with @hubcio about how it should be addressed - probably across all sdks.

Can we just revert the bdd/ part and LGTM ?

[oscarArismendi]

@T1B0 reverted BDD changes

[T1B0]

Nice catch, ty for your contribution! LGTM 🚀