Modbus TCP: handle multiple ADUs per tcp packet and reassemble fragments

[alde103]

Description

Modbux.Tcp.Client (active mode) crashed with a FunctionClauseError when a single tcp packet carried more than one Modbus TCP response, since handle_info({:tcp, ...}) assumed exactly one ADU per packet. TCP is a stream protocol, so responses can arrive concatenated (as reported with real devices) or fragmented.

This PR makes both sides of the TCP stack reassemble the stream properly:

• New Modbux.Tcp.split_adus/1 splits a tcp stream into complete ADUs, framed by the length field of the MBAP header, returning the remaining bytes.
• Modbux.Tcp.Client (active mode) processes every response in a packet and buffers incomplete bytes until the next packet arrives.
• Modbux.Tcp.Server.Handler does the same for requests (it previously crashed with a MatchError, dropping the connection).
• Buffers are per connection and cleared on socket close.

The passive mode (confirmation/1) was already correct because it reads exact lengths with :gen_tcp.recv/3.

Fixes #14

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Each fix was written test-first (the test reproduced the reported crash before the fix). All tests run against real :gen_tcp sockets, no mocks:

• test/tcp/modbus_tcp_client_test.exs: client receives two responses in a single tcp packet (issue Modbux.Tcp.Client terminates when TCP response contains 2 payloads #14 scenario).
• test/tcp/modbus_tcp_client_test.exs: client reassembles a response split across two tcp packets.
• test/tcp/modbus_tcp_server_test.exs: server answers two requests received in a single tcp packet.
• test/tcp/modbus_tcp_server_test.exs: server reassembles a request split across two tcp packets.
• Full suite: 43 tests, 0 failures (Elixir 1.18.3 / OTP 27).

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

🤖 Generated with Claude Code

https://claude.ai/code/session_01V9dLEMSgBybY4Xe9szthoQ

[palantir-valiot]

Overall Assessment

This PR correctly fixes the Modbus TCP stream reassembly issue (#14) by introducing Tcp.split_adus/1 to frame ADUs using the MBAP length field, and applying it to both the active-mode client and the server handler. The implementation is sound: incomplete bytes are buffered per-connection, complete messages are processed iteratively, and buffers are cleared on close. All four reported crash scenarios are covered by integration tests against real :gen_tcp sockets.

Findings

No actionable findings.

Notes

• The split_adus/1 function correctly uses the MBAP len field to frame ADUs and defers protocol-id validation to the existing unwrap/1 and unwrap/2 functions, which is the right separation of concerns.
• The client test that splits a response after 4 bytes and the server test that splits a request after 5 bytes both exercise reassembly when the break falls inside the MBAP header — good edge-case coverage.
• close_socket/1 explicitly resets buffer: <<>>, and the server handler discards its buffer on process termination; no stale-buffer risk was found.
• Considered flagging the lack of an upper-bound sanity check on len in split_adus, but decided against it: the guard byte_size(tail) >= len ensures memory is bounded by bytes actually received, and adding a hard cap (e.g., 253) could break non-standard peers. This is consistent with the library's existing relaxed protocol-identifier handling.

[Copilot]

Pull request overview

Fixes Modbus TCP active-mode stream handling by properly framing Modbus TCP ADUs from a TCP byte stream, so concatenated responses/requests in a single packet and fragmented frames across packets no longer crash the client/server.

Changes:

• Added Modbux.Tcp.split_adus/1 to split a TCP stream into complete ADUs (plus leftover bytes).
• Updated Modbux.Tcp.Client (active mode) and Modbux.Tcp.Server.Handler to process multiple ADUs per :tcp message and buffer incomplete fragments.
• Added regression tests for concatenation and fragmentation scenarios; updated changelog.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/tcp/modbus_tcp_server_test.exs	Adds server-side tests for concatenated requests and fragmented request reassembly.
test/tcp/modbus_tcp_client_test.exs	Adds client-side tests for concatenated responses and fragmented response reassembly.
lib/tcp/tcp.ex	Introduces split_adus/1 framing helper (and minor log formatting change).
lib/tcp/server/server_handler.ex	Buffers and iterates over multiple framed requests per TCP message.
lib/tcp/client.ex	Buffers and iterates over multiple framed responses per TCP message; clears buffer on close.
CHANGELOG.md	Documents the new TCP stream reassembly behavior in 0.3.14.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[palantir-valiot]

Overall Assessment

The PR correctly fixes the TCP stream reassembly issue (#14) by introducing Tcp.split_adus/1 to frame Modbus TCP ADUs using the MBAP header length field, and adds per-connection buffers to both the client and server handler. The implementation is sound: incomplete data is buffered until the next packet, multiple complete ADUs are processed in order, and buffers are cleared on connection close. All reported crash scenarios are covered by tests against real :gen_tcp sockets.

Findings

#	Severity	File	Issue
1	Low	.claude/settings.local.json.tmp.3761534.353b31b2950e	Temporary Claude Code settings file accidentally committed; should be removed

Notes

• Tcp.split_adus/1 correctly uses big-endian framing matching the Modbus TCP MBAP header spec, and safely buffers incomplete headers (< 6 bytes) and incomplete payloads.
• The client clears both buffer and pending_msg in close_socket/1, preventing stale state from leaking across reconnections.
• The server handler buffer is implicitly cleared when the GenServer stops on {:tcp_closed, _} or {:tcp_error, ...}, which is correct since the buffer is per-connection.
• Tests use nodelay: true to avoid Nagle coalescing when testing fragmentation, which is good test hygiene.
• The Enum.reduce in the client and Enum.each in the server preserve ADU processing order via Enum.reverse in split_adus.
• I considered flagging the unbounded buffer growth under a malicious peer that never sends a complete ADU, but since buffers are per-connection and cleared on disconnect, the blast radius is limited and acceptable for this library's threat model.
• I also considered whether attend_request/3 should receive the updated buffer state, but since it does not read the buffer field, the current pattern is functionally correct.

[palantir-valiot]

This file appears to be a temporary Claude Code settings file (note the .tmp. prefix and hash suffix) that was accidentally committed. It should be removed from the PR before merging. Consider also adding .claude/ to .gitignore to prevent future accidental commits.

[alde103]

Removed in 618345c and added .claude/ to .gitignore. The file only contained session permission allowlist entries, no secrets.

[palantir-valiot]

Overall Assessment

This PR correctly fixes the TCP stream reassembly issue (#14) by introducing Tcp.split_adus/1 and applying it to both the client (active mode) and server handler. The implementation is sound: it buffers incomplete bytes, processes complete ADUs framed by the MBAP length field, and clears buffers on connection close. All reported crash scenarios are covered by tests that exercise real :gen_tcp sockets.

Findings

No actionable findings.

Notes

• The split_adus/1 recursion correctly uses the MBAP len field as the payload boundary and returns trailing bytes in the second tuple element, which is exactly what TCP stream reassembly requires.
• Both client and server tests verify not only the "multiple ADUs per packet" case but also the "single ADU split across packets" case, including a split in the middle of the MBAP header (4-byte and 5-byte splits). This gives confidence the reassembly logic is robust.
• close_socket/1 now clears buffer and pending_msg, which prevents stale state from leaking across reconnections.
• The .gitignore change (.elixir_ls → .elixir_ls.claude/) appears to be an artifact of the generation tool; it is harmless but unrelated to the bug fix.
• I considered whether an unbounded buffer could be a concern if a peer sends a large len value and never completes the frame. This is a generic DoS vector inherent to any stream reassembly, not a regression introduced here, and the prior behavior was an immediate crash.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

[alde103]

Fixed in 83c81ab.

[palantir-valiot]

Overall Assessment

This PR correctly fixes the TCP stream reassembly issue (#14) by introducing Tcp.split_adus/1 to frame Modbus TCP ADUs using the MBAP length field. Both the client (active mode) and server handler now buffer incomplete data and process multiple complete messages per packet. The implementation is sound, well-tested, and follows existing codebase patterns.

Findings

No actionable findings.

Notes

• The split_adus/1 function correctly uses the MBAP len field to frame ADUs and safely falls back to buffering when data is incomplete (less than 6 bytes or insufficient payload).
• Client buffer is explicitly cleared in close_socket/1; server handler buffer is implicitly discarded when the process stops on tcp_closed.
• Tests cover the four key scenarios: multiple responses/requests per packet and fragmented responses/requests across packets, all using real :gen_tcp sockets.
• The process_response/2 helper in the client correctly accumulates state across multiple responses via Enum.reduce, which is the right approach for active mode.
• attend_request/3 in the server handler uses the original state (with the old buffer) for each request, but since it only reads model_pid and parent_pid — both immutable fields — this is safe.
• The nodelay: true option in fragmentation tests is a good practice to prevent Nagle's algorithm from coalescing test fragments.