`PacketReader` is test-only and diverges from the real read path in `NetworkTransport`

[saurabh500]

Found while profiling the ODBC row-fetch path (#215).

Summary

mssql-tds/src/io/packet_reader.rs defines PacketReader, but its constructor is gated:

#[cfg(test)]
pub(crate) fn new(network_reader_writer: &'a mut dyn NetworkReaderWriter) -> PacketReader<'a> {

So PacketReader cannot be constructed in a release build. Every call site of PacketReader::new is inside a #[cfg(test)] module (packet_reader.rs, col_info_parser.rs, tab_name_parser.rs, error_parser.rs, info_parser.rs, order_parser.rs).

The read path that actually runs is NetworkTransport::get_new_tds_packet (connection/transport/network_transport.rs), confirmed empirically: instrumenting NetworkTransport::receive — the method PacketReader calls — yields 0 calls during a row-fetch benchmark.

The concern isn't dead code. It's that a set of token-parser tests are validating parsing against a packet reader with different semantics from the one used in production, so those tests can pass while the real path is broken (or vice versa).

The concrete divergence

PacketReader::get_new_tds_packet (lines 146–197) returns bytes_read_from_transport, not packet_size_from_header:

let packet_size_from_header: usize = length_from_packet_header as usize;

// Keep reading until we have the complete packet in memory.
while bytes_read_from_transport < packet_size_from_header {
    bytes_read_from_transport += self.network_reader_writer.receive(...).await?;
}
...
Ok(bytes_read_from_transport)

The loop stops once it has at least a full packet, but a single receive() can return bytes past the packet boundary — i.e. the head of the next packet. The returned count then includes those bytes.

The caller (lines 122–135) treats the whole returned range as one packet and strips exactly one 8-byte header:

let new_packet_size = self.get_new_tds_packet().await?;
self.working_buffer.copy_within(
    self.buffer_length + 8..self.buffer_length + new_packet_size,
    self.buffer_length,
);
self.buffer_length += new_packet_size;
self.buffer_length -= 8;

If bytes_read_from_transport > packet_size_from_header, the next packet's 8-byte header is left inline and interpreted as payload, corrupting the token stream.

NetworkTransport::get_new_tds_packet handles this correctly by carrying the overshoot in pending_bytes. PacketReader has no equivalent.

This is latent today only because the mock readers in tests return at most one packet per receive(). It would surface immediately against a real socket.

Suggested resolution

Either is fine; the second is probably better:

1. Fix it — track the overshoot and return packet_size_from_header, mirroring pending_bytes.
2. Delete it and port the token-parser tests onto the NetworkTransport path (or a shared abstraction), so parser tests exercise the reader that actually ships.

Option 2 removes the divergence rather than duplicating a subtle invariant in two places.

Notes

• No impact on shipped behavior — the type is unreachable in a release build.
• Not urgent; filing so it isn't rediscovered.