oscarArismendi opened a new pull request, #3182: URL: https://github.com/apache/iggy/pull/3182
## 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. <details><summary>Unit test screenshot</summary> <p> <img width="1813" height="839" alt="Screenshot 2026-04-26 at 7 47 26 PM" src="https://github.com/user-attachments/assets/26a5965d-3a89-4c78-970a-27bfd4c4272e"; /> </p> </details> 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. <details><summary>E2E test screenshot</summary> <p> <img width="1842" height="821" alt="Screenshot 2026-04-26 at 9 01 36 PM" src="https://github.com/user-attachments/assets/d38ec25a-3671-4842-9d4d-162ef1818c58"; /> </p> </details> 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. <details><summary>BDD test screenshot</summary> <p> <img width="1827" height="836" alt="Screenshot 2026-04-26 at 9 27 54 PM" src="https://github.com/user-attachments/assets/d324e56a-d097-4ec1-9438-a4b1d7f23b54"; /> </p> </details> ## 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](https://github.com/oscarArismendi/iggy/blob/fix%2FdeserializeVoidResponse-now-ignores-payload-length-for-all-void-commands/foreign/node/src/wire/message/message.utils.ts#L40-L47), 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](https://github.com/oscarArismendi/iggy/blob/fix%2FdeserializeVoidResponse-now-ignores-payload-length-for-all-void-commands/core/server/src/binary/handlers/messages/send_messages_handler.rs#L202-L203)) 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
