Sans-I/O TDS core (2/N): route NetworkTransport through PacketBuffer

[Saurabh Singh (saurabh500)]

Summary

Layer 2 of the sans-I/O restructure of mssql-tds. Stacks on #189 — base is dev/saurabh/sans-io-tds-core, not main.

Adopts the Layer-1 PacketBuffer as the single source of truth for packet buffering and scalar/byte decode in the production read path (NetworkTransport), and deletes the duplicated TdsReadBuffer (buffers.rs, 482 lines).

Net: +417 / −737 lines.

Behavior change — correctness fix (reviewer note)

A zero-byte socket read now returns ConnectionClosed on both socket-read loops. This is a production correctness fix, not merely a test-hang fix. Previously, if a peer closed the connection mid-packet, read() returning Ok(0) was treated as a successful (empty) read, so the refill loop spun a perpetually-ready future at 100% CPU forever — on real connections, not only under test. Both loops now detect bytes_read == 0 and return Error::ConnectionClosed. The symptom that surfaced it was an infinite cargo btest hang, but the underlying defect affected production peers that drop mid-packet. Reviewers should treat this as a distinct bug fix on its own merits.

Approach — Option A (extend PacketBuffer, delegate fully)

Rather than layering pending-bytes bookkeeping in the transport around a dumb buffer (Option B), I extended PacketBuffer with first-class multi-packet pending-bytes carryover so NetworkTransport delegates all buffering and decode:

• begin_refill() — compact readable bytes to front, relocate carried pending bytes with the same bounds guard production had.
• refill_window(base, already) — wide window to end of buffer so one socket read can pull multiple TDS packets (coalescing).
• validate_packet_length(base) — rejects < 8, > max_packet_size, or header-claims-overflow (preserves the production overflow guard).
• record_pending / strip_header / change_packet_size (resize to size*2 only on change — preserves the reset_reader resize fix) / reset_to_length (preserves pending).

NetworkTransport now has no hand-rolled scalar decode or position bookkeeping — every reader is if !has(N) { read_tds_packet().await? } take_X(). TdsReadBuffer is gone.

Test coverage note (deleted buffers.rs tests)

Deleting buffers.rs removed its 16 TdsReadBuffer unit tests; 8 new direct PacketBuffer tests were added (net −8 passing, failing set unchanged). This is a consolidation, not a silent loss:

• The subtle, high-value behavior is directly covered by the 8 new tests: multi-packet pending-bytes carryover (begin_refill_*), surplus tracking (record_pending_tracks_surplus), packet-length guards (validate_packet_length_guards), header strip (strip_header_exposes_payload), and reset-preserves-pending (reset_to_length_preserves_pending).
• The remaining deleted tests exercised trivial positional accessors (consume_bytes/get_slice/do_we_have_enough_data/get_remaining_byte_count) that are now PacketBuffer::{take_*, has, available, skip, copy_out}. These are exercised indirectly by network_transport (17/17) and packet_reader (23/23). Direct PacketBuffer unit tests for take_*/skip/copy_out roundtrips and the over-consume bounds guard are restored in Layer 3, which makes those methods the sync-core API (adding them here would force an L3 restack).

Notes

• PacketReader (test-only) keeps received-based semantics because TestPacketBuilder::build() writes payload_length (not total length) into the TDS header, so its header-length field can't be trusted as authoritative. Production NetworkTransport writes correct total lengths and uses the header field authoritatively via validate_packet_length + record_pending. Both share PacketBuffer but call different methods by design.
• PacketBuffer::with_packet_size is un-gated (was #[cfg(test)]) so production can construct it.

Validation

• cargo bfmt — clean
• cargo bclippy (-D warnings) — clean
• cargo btest (nextest + coverage) — no regressions. Verified by diffing the failing-test set against a clean baseline of the base branch: 359 failed on both, identical sets, zero regressions. The 359 are pre-existing/environmental (no live SQL Server, missing cert fixtures in tests/test_certificates/, odbc debug-assert tests) and fail the same way on dev/saurabh/sans-io-tds-core.
• Targeted: io tests 23/23, network_transport tests 17/17 (all pending-bytes, coalescing, reset_reader, bounds-guard, packet-length validation tests pass).
• cargo build --manifest-path mssql-py-core/Cargo.toml — builds clean (public surface unchanged).

Draft until the stack lands.

[Copilot]

Pull request overview

Routes production TDS reads through the shared sans-I/O PacketBuffer, consolidating buffering and fixing zero-byte read loops.

Changes:

• Adds pending-byte carryover, packet validation, resizing, and tests to PacketBuffer.
• Replaces TdsReadBuffer in NetworkTransport.
• Returns ConnectionClosed when packet reads reach EOF.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
mssql-tds/src/io/packet_reader.rs	Uses refill APIs and handles EOF.
mssql-tds/src/io/packet_buffer.rs	Adds production buffering and framing support.
mssql-tds/src/connection/transport/network_transport.rs	Delegates production reads to PacketBuffer.
mssql-tds/src/connection/transport/buffers.rs	Removes duplicated read buffer.
mssql-tds/src/connection/transport.rs	Removes the deleted buffer module.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.