Fix Windows lob_max GetData regression and SChannel read overhead

[Saurabh Singh (saurabh500)]

Description

Fixes the Windows perf-lab regression on getdata/rowwise_1k_c3_lob_max/chunked_8192, and removes a Windows-only TLS read cost that made up part of the gap to Microsoft ODBC Driver 18.

1. Narrow PLP SQL_C_CHAR refill (regression from #631)

#631 made stream_active_plp_chunk keep reading until the output buffer is full. For varchar(max) under a code-page collation read as SQL_C_CHAR, each read was sized by narrow_max_read = room/3, so the refill was geometric: 2730, 1820, 1213 … 1 byte. That is about 20 wire reads and decode passes per 8 KB buffer. #628 kept the same pattern through the retry_bytes refill.

• Read the whole room. With no UTF-8 carry, read the full output room instead of room/3.
• Decode only what fits. narrow_source_fit counts the leading ASCII run, then applies the room/3 rule to the rest. Unconverted source goes back through restore_source_prefix, so a later target switch still finds raw bytes (the Fix PLP carry handling across SQLGetData target switches #628 contract).
• Top up read-ahead. A short, not-at-end read from prefetched_wire now tops up from the wire through read_plp_wire.
• Skip finished decoders. The partial-character check is skipped once the decoder is finished, because querying it then panics in encoding_rs.

2. Read whole TLS records before SChannel decrypt (Windows)

SchannelTlsStream::poll_read read the socket into an 8 KB stack buffer, copied it into enc_in, and called DecryptMessage after every read. A 16 KB TLS record therefore took two or three socket reads, and each partial record cost a failing DecryptMessage call (SEC_E_INCOMPLETE_MESSAGE).

• missing_record_bytes reads the 5-byte record header, and DecryptMessage is skipped until the record is whole. A header declaring more than the TLS maximum (a record_overflow, RFC 5246 §6.2.3) fails the read at once instead of waiting for the body.
• The socket is read straight into enc_in through poll_read_buf, with room reserved for one maximum-size record (5 + 16384 + 2048 bytes), so no copy is needed and one read can take a whole record.

Lab results (Windows, pipeline 2326)

Lab noise is about ±5% run to run, so each row is compared with the Microsoft driver measured in the same run.

Run	lob_max	baseline	MS ODBC 18	vs baseline	vs MS
178649 (main, regressed)	81.08	75.96	49.12	+6.7%	+65%
179025 (refill fix only)	67.96	68.35	47.37	-0.6%	+43%
179120 (refill fix + TLS change)	65.21	73.39	49.56	-11.2%	+32%

No other benchmark regressed. The Linux lab (run 179122) shows no change beyond noise; the TLS change is SChannel-only code.

Tests

• plp_cp1252_ascii_fills_char_buffer_in_one_wire_read: pins one wire read per buffer. Forcing reads back to room/3 makes it fail with 20 reads.
• plp_cp1252_mixed_text_streams_and_keeps_raw_bytes_for_binary: mixed ASCII, 0x80 (3-byte UTF-8) and 0xE9 across buffer sizes 2 to 1024 give byte-identical output. A switch to SQL_C_BINARY returns exactly the unconverted source.
• Updated plp_consecutive_high_surrogates_bound_carry_and_preserve_binary_input: the binary read now fills its 4-byte buffer because of the top-up.
• poll_read_waits_for_whole_record_without_decrypt_in_one_socket_read: an 18 KB partial record is read in one socket read and does not reach DecryptMessage. It fails if decrypt runs on partial records or if reads go back to 8 KB.
• poll_read_decrypts_once_record_is_whole: a complete record does reach DecryptMessage. It fails if the check never lets decrypt run.
• missing_record_bytes_tracks_header_and_body: header, body, exact and over-full cases.
• missing_record_bytes_rejects_oversized_length and poll_read_fails_on_oversized_header_without_waiting: a declared length above the TLS maximum fails from the header alone.

Related Issues

Fixes #674

Checklist

• cargo bfmt passes
• cargo bclippy passes
• cargo nextest -p mssqlodbc (1843 tests) and cargo nextest -p mssql-tds --lib pass, except 7 certificate-file tests (certificate_validator, validate_pinned_cert_*) that fail the same way on main
• New/changed functionality has tests
• Public API changes are documented (no public API changes)

[Copilot]

Copilot review overview

🟡 Changes recommended

The required GitHub issue or Azure DevOps work-item link is still a TODO.

Review effort: Balanced
Findings: 1

Open (1)

• Replace TODO placeholder with the required work-item link · New

What changed in this PR

Fixes excessive PLP refill reads when streaming narrow text as SQL_C_CHAR.

Changes:

• Reads full available capacity and decodes only the fitting prefix.
• Tops up short prefetched reads from the wire.
• Adds regressions for performance, conversion, and target switching.

File	Description
mssql-odbc/​src/​api/​get_data.rs	Optimizes narrow PLP streaming and expands regression coverage.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 84%

🎯 Overall Coverage 94.4%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core ℹ️ Note: diff coverage is reported, not enforced.

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql-odbc/src/api/get_data.rs (82.7%): Missing lines 2346-2347,2657-2658,2965-2966,2972-2976,2980-2984,2994-2996,3000-3004,3009-3010,3036,3040-3041,3049-3050,3054-3055,3068-3070,3072,3076
• mssql-tds/src/connection/transport/win_tls/stream.rs (86.6%): Missing lines 409-410,855,886-892,894-896,898-900,927,952

Summary

• Total: 354 lines
• Missing: 56 lines
• Coverage: 84%

mssql-odbc/src/api/get_data.rs

  2342             Ok(Ok(chunk)) => Ok(PlpChunk {
  2343                 read: buffered + chunk.read,
  2344                 ..chunk
  2345             }),
! 2346             Ok(Err(error)) => Err(error),
! 2347             Err(rc) => return rc,
  2348         }
  2349     };
  2350 
  2351     let PlpChunk {

---

  2653                 .narrow_decoder
  2654                 .as_ref()
  2655                 .map(|decoder| !finished && narrow_decoder_has_partial_character(decoder))
  2656             else {
! 2657                 error!("SQLGetData: narrow PLP stream has no encoding to convert through");
! 2658                 return SQL_ERROR;
  2659             };
  2660             let pending_before = stream.pending_bytes.len();
  2661             let fit = narrow_source_fit(
  2662                 &payload[..read],

---

  2961     let dbc = stmt.parent_dbc();
  2962     let mut dbc_state = match dbc.inner.lock() {
  2963         Ok(state) => state,
  2964         Err(_) => {
! 2965             error!("SQLGetData: dbc mutex poisoned while reading PLP stream");
! 2966             return Err(SQL_ERROR);
  2967         }
  2968     };
  2969     if let Some(busy_stmt) = dbc_state.active_stmt
  2970         && busy_stmt != statement_handle

---

  2968     };
  2969     if let Some(busy_stmt) = dbc_state.active_stmt
  2970         && busy_stmt != statement_handle
  2971     {
! 2972         drop(dbc_state);
! 2973         if let Ok(mut s) = stmt.inner.lock() {
! 2974             post_diag(&mut s, ERR_CONNECTION_BUSY);
! 2975         }
! 2976         return Err(SQL_ERROR);
  2977     }
  2978     let buffered_read = {
  2979         let Some(client) = dbc_state.client.as_mut() else {
! 2980             drop(dbc_state);
! 2981             if let Ok(mut s) = stmt.inner.lock() {
! 2982                 post_diag(&mut s, ERR_NO_ACTIVE_TDS_CLIENT);
! 2983             }
! 2984             return Err(SQL_ERROR);
  2985         };
  2986         client.try_read_active_plp_chunk(payload)
  2987     };
  2988     dbc_state.active_stmt = Some(statement_handle);

---

  2990         Ok(CursorPoll::Ready(chunk)) => {
  2991             drop(dbc_state);
  2992             Ok(chunk)
  2993         }
! 2994         Err(error) => {
! 2995             drop(dbc_state);
! 2996             Err(error)
  2997         }
  2998         Ok(CursorPoll::Pending) => {
  2999             let Some(mut client) = dbc_state.client.take() else {
! 3000                 drop(dbc_state);
! 3001                 if let Ok(mut s) = stmt.inner.lock() {
! 3002                     post_diag(&mut s, ERR_NO_ACTIVE_TDS_CLIENT);
! 3003                 }
! 3004                 return Err(SQL_ERROR);
  3005             };
  3006             drop(dbc_state);
  3007             let mut prefetch_scratch = {
  3008                 let Ok(mut stmt_state) = stmt.inner.lock() else {
! 3009                     error!("SQLGetData: stmt mutex poisoned while taking PLP prefetch buffer");
! 3010                     return Err(SQL_ERROR);
  3011                 };
  3012                 std::mem::take(&mut stmt_state.plp_prefetch_scratch)
  3013             };
  3014             let result = dbc.runtime.block_on(async {

---

  3032                     Ok(tail) => {
  3033                         let carry = (prefetch_scratch, tail, chunk.total_read);
  3034                         Ok((chunk, Some(carry), None, None))
  3035                     }
! 3036                     Err(error) => Ok((chunk, None, Some(prefetch_scratch), Some(error))),
  3037                 }
  3038             });
  3039             let Ok(mut dbc_state) = dbc.inner.lock() else {
! 3040                 error!("SQLGetData: dbc mutex poisoned after PLP read");
! 3041                 return Err(SQL_ERROR);
  3042             };
  3043             dbc_state.client = Some(client);
  3044             dbc_state.active_stmt = Some(statement_handle);
  3045             drop(dbc_state);

---

  3045             drop(dbc_state);
  3046             match result {
  3047                 Ok((chunk, carry, unused_scratch, prefetch_error)) => {
  3048                     let Ok(mut stmt_state) = stmt.inner.lock() else {
! 3049                         error!("SQLGetData: stmt mutex poisoned while saving PLP prefetch buffer");
! 3050                         return Err(SQL_ERROR);
  3051                     };
  3052                     if let Some((bytes, tail, total_read_before)) = carry {
  3053                         let Some(stream) = stmt_state.active_plp.as_mut() else {
! 3054                             error!("SQLGetData: PLP stream vanished while saving prefetched bytes");
! 3055                             return Err(SQL_ERROR);
  3056                         };
  3057                         stream.set_prefetched_wire(
  3058                             bytes,
  3059                             tail.read,

---

  3064                     } else if let Some(buffer) = unused_scratch {
  3065                         stmt_state.plp_prefetch_scratch = buffer;
  3066                     }
  3067                     if let Some(error) = prefetch_error {
! 3068                         let Some(stream) = stmt_state.active_plp.as_mut() else {
! 3069                             error!("SQLGetData: PLP stream vanished while saving prefetch error");
! 3070                             return Err(SQL_ERROR);
  3071                         };
! 3072                         stream.set_prefetch_error(error);
  3073                     }
  3074                     Ok(chunk)
  3075                 }
! 3076                 Err(error) => Err(error),
  3077             }
  3078         }
  3079     })
  3080 }

---

mssql-tds/src/connection/transport/win_tls/stream.rs

  405             };
  406             if missing == 0 {
  407                 match record.decrypt(enc_in, plain_out) {
  408                     Ok(Decrypted::Ok) => continue, // back to step 1 to drain
! 409                     Ok(Decrypted::PeerClosed) => return Poll::Ready(Ok(())),
! 410                     Ok(Decrypted::NeedMoreInput) => {}
  411                     Err(e) => return Poll::Ready(Err(e)),
  412                 }
  413             }

---

  851         let mut backing = [0u8; 16];
  852         let mut rb = ReadBuf::new(&mut backing);
  853         match Pin::new(&mut s).poll_read(&mut cx, &mut rb) {
  854             Poll::Ready(Err(e)) => assert_eq!(e.kind(), io::ErrorKind::InvalidData),
! 855             other => panic!("oversized header must fail, got {other:?}"),
  856         }
  857     }
  858 
  859     /// Socket that serves `data` as fast as the caller's buffer allows, then

---

  882         }
  883     }
  884 
  885     impl AsyncWrite for FeedSocket {
! 886         fn poll_write(
! 887             self: Pin<&mut Self>,
! 888             _: &mut Context<'_>,
! 889             b: &[u8],
! 890         ) -> Poll<io::Result<usize>> {
! 891             Poll::Ready(Ok(b.len()))
! 892         }
  893 
! 894         fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<io::Result<()>> {
! 895             Poll::Ready(Ok(()))
! 896         }
  897 
! 898         fn poll_shutdown(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<io::Result<()>> {
! 899             Poll::Ready(Ok(()))
! 900         }
  901     }
  902 
  903     fn record_prefix(body_len: u16, have: usize) -> Vec<u8> {
  904         let [hi, lo] = body_len.to_be_bytes();

---

  923         let mut backing = [0u8; 16];
  924         let mut rb = ReadBuf::new(&mut backing);
  925         match Pin::new(&mut s).poll_read(&mut cx, &mut rb) {
  926             Poll::Pending => {}
! 927             other => panic!("partial record must wait, got {other:?}"),
  928         }
  929         assert_eq!(
  930             s.socket.data_reads, 1,
  931             "a record larger than 8 KB takes one read"

---

  948         let mut backing = [0u8; 16];
  949         let mut rb = ReadBuf::new(&mut backing);
  950         match Pin::new(&mut s).poll_read(&mut cx, &mut rb) {
  951             Poll::Ready(Err(_)) => {}
! 952             other => panic!("whole record must reach DecryptMessage, got {other:?}"),
  953         }
  954     }
  955 }

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[Copilot]

Copilot review overview

🔵 Needs a closer look

An oversized TLS record-length issue remains, and the streaming and TLS paths warrant final human review.

Review effort: Balanced
Findings: 1

Open (1)

• Reject oversized TLS record lengths after header parsing · New

Resolved since last review (1)

• Replace TODO placeholder with the required work-item link

[ttk (Theekshna)]

Unattended hourly review of PR #668 at e203c4e7 (merge-base f0cbff05 against main). This PR was never reviewed by me before.

Read all prior discussion first. Two Copilot bot review passes: one low-severity finding (PR description TODO placeholder for the work-item link), resolved by the author replacing it with Fixes #674 (thread confirmed resolved via the GraphQL API). One medium-severity finding remains open and unanswered: an oversized/invalid TLS record length is not rejected before poll_read waits for it (win_tls/stream.rs:358). I independently traced this rather than trusting it, and I'm extending it with new information below instead of re-filing it.

Read code directly rather than trusting the description for: narrow_max_read/narrow_source_fit/restore_source_prefix in get_data.rs, and missing_record_bytes/MAX_TLS_RECORD/poll_read in win_tls/stream.rs. Traced restore_source_prefix's offset/total_read bookkeeping by hand (stmt.rs:181) to confirm restored bytes are re-served in original order without double-counting total_read on the next read_prefetched_wire call — correct.

Six required checks

1. msodbcsql parity. The narrow-PLP refill buffer-sizing heuristic (room/3 vs whole-room) has no msodbcsql counterpart to compare against — it's an internal read-ahead strategy, not an observable protocol contract, and the PR's own tests already pin byte-for-byte decode equivalence with the non-streaming path (AB#47566). N/A for that half.

   For the TLS record gating, I read msodbcsql's SChannel read path (Sql\Common\DK\sni\src\SNI_SslProvider.cpp, the DecryptMessage call site and its SEC_E_INCOMPLETE_MESSAGE handling). msodbcsql calls DecryptMessage on every accumulated read and lets SChannel's own return code decide completeness — it does not pre-parse the record's declared length against a maximum before deciding whether to call decrypt. This Rust path instead gates the call on its own missing_record_bytes, computed purely from the wire-declared length, with no cap against MAX_TLS_RECORD. I can't cite what SChannel itself does with an out-of-range declared length (that's inside DecryptMessage, not sourced anywhere I can read), so I'm not asserting msodbcsql would reject this case either — only that this new gate has no backstop of its own, where msodbcsql's design incidentally gets one from calling decrypt unconditionally. See inline.

2. Test sufficiency. Could not execute cargo nextest -p mssqlodbc --lib or -p mssql-tds --lib in this sandbox: both pull in crc32fast transitively (reqwest → async-compression → flate2, a direct dependency of mssqlodbc and a dev-dependency of mssql-tds via azure_identity), and its build-time AVX-512/pclmulqdq codegen hits E9012/E9013 on this toolchain's backend — the same documented, diff-unrelated limitation recorded on PR #617's and #650's reviews. cargo build -p mssql-tds --lib (no test target, so no dev-deps) does succeed clean on e203c4e7.

   Verified by hand-trace instead: plp_cp1252_ascii_fills_char_buffer_in_one_wire_read is load-bearing — it asserts PLP_WIRE_READS == 1 via a #[cfg(test)] counter incremented inside the real read_plp_wire production function (get_data.rs:2960), not a mock proxy. Reverting max_read_for's transcode_narrow_to_utf8 arm from "read the whole room when utf8_carry_len == 0" back to unconditional narrow_max_read (the pre-PR shape) would make the first wire request only room/3 of 2048, forcing at least a second read_plp_wire call to fill the buffer — the counter would read ≥ 2, failing the assertion. missing_record_bytes_tracks_header_and_body and poll_read_waits_for_whole_record_without_decrypt_in_one_socket_read/poll_read_decrypts_once_record_is_whole are similarly load-bearing by trace: shrinking MAX_TLS_RECORD back toward 8 KB, or reverting the header-length arithmetic, changes data_reads/the Poll::Pending vs Poll::Ready(Err(_)) outcome these tests assert.

   Coverage gap confirmed by reading the tests, not asserted blind: none of the new TLS tests use a declared record length exceeding MAX_TLS_RECORD - 5 (the largest tested body is 18,000, under the 18,432 ceiling). The oversized-length path in check 1 is genuinely untested, not just unhandled.

3. Divergences documented. N/A — both changes are internal performance rework with no observable output difference (the PR's own decode-equivalence tests establish this), so no parity-deviations.md entry applies. Checked the registry for existing TLS/PLP-buffer-sizing entries; none exist and none is needed here.

4. PR description currency. Matches the diff. Fixes #674 is linked and I read the issue: it names the same two problems (stream_active_plp_chunk geometric room/3 refill; SChannel DecryptMessage-per-partial-read) with the same pipeline/run numbers this PR's table uses. Checklist claims cargo bfmt/cargo bclippy/cargo nextest pass with 7 known-on-main certificate-fixture failures — consistent with gh pr checks on e203c4e7: every Build/Test/CodeQL/Kerberos/cross-repo stage is green; only "Merge Coverage" and the umbrella "Pull request validation" summary are still pending as of this post, which I could not wait out within this run's budget.

5. Slop. None found. Every new doc comment (narrow_max_read, narrow_source_fit, restore_source_prefix, the MAX_TLS_RECORD/missing_record_bytes pair) states a why — the room/3 ratio's derivation, the overshoot-is-harmless argument, the record-header-parsing rationale — not a restatement of the following line.

6. Evidence audit. Re-derived the motivating claim against the linked issue rather than accepting the PR's own numbers: #674's report of "~20 wire reads per 8 KB buffer" for the room/3 geometric series (2730, 1820, 1213, …) is arithmetically consistent with repeatedly applying x/3 from a 2048–8192-ish starting room down to 1 — checks out. I could not independently re-run the Windows perf-lab pipeline (no ADO access in this environment; failing open per this run's instructions) to re-derive the 81.08/67.96/65.21 ms row values themselves.

   Completeness gap found by reading the code, not the lab: the lab table (runs 178649/179025/179120) measures only the lob_max scenario under what the issue describes as a single-byte code-page collation (CP1252, matching the PR's own plp_cp1252_* tests). narrow_source_fit/narrow_max_read are generic — they also gate DBCS collations (e.g. GBK, already exercised elsewhere in this file's test suite via open_mock_gbk_plp) through the same "read whole room, decode what fits, restore the rest" path, but GBK's occasional 4-byte output-per-1-input-byte expansion (noted in this PR's own narrow_max_read doc comment) means the restore/re-decode churn per wire read differs from CP1252's 3-byte worst case. No DBCS-collation row appears in the lab table, so the perf claim is unmeasured for that neighboring, code-shared input class.

Severity	Count
Blocking	0
Suggestion	2
Nit	0

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Unattended review of e203c4e7 against merge-base f0cbff05. Both halves of the change are well-motivated and the narrow-PLP rework is carefully built: narrow_source_fit keeps the worst-case expansion budget intact while taking the leading ASCII run whole, and restore_source_prefix preserves the #628 "a target switch still finds raw bytes" contract, including prefetched_total_read_before accounting and ordering against an existing prefetch tail. The TLS record gate is also sound on the paths I traced: enc_in is always record-aligned at index 0 (compact_suffix on Decrypted::Ok, encrypted_in.clear() plus prepend_leftover on the SEC_I_RENEGOTIATE path), missing_record_bytes cannot underflow because the [_, _, _, hi, lo, ..] arm covers every len >= 5, and tokio_util::io::poll_read_buf only returns Ready(Ok(0)) on real EOF for a Vec<u8> sink, so read_eof_outcome still means what it did. Vec::reserve's amortized growth also keeps enc_in capacity bounded at roughly 2 * MAX_TLS_RECORD rather than growing per read.

One blocking finding, posted inline: the new "top up a short read-ahead from the wire" branch consumes prefetched bytes before the wire read and discards them on the error exits, which can silently truncate a value on the retryable ERR_CONNECTION_BUSY path.

I deliberately did not re-file two things already on this PR: the open bot thread about oversized TLS record lengths at win_tls/stream.rs:358, and ttk (@Theekshna)'s note that the lab table covers only a CP1252-shaped lob_max and leaves the DBCS side of the shared narrow_source_fit path unmeasured. I independently reached the same conclusion on the second point — an all-non-ASCII CP1252 stream settles into one wire read per call, same as before, but pays two extra multi-KB copies per call for the restore/re-serve round trip — so consider that an independent confirmation rather than a new finding.

Checked and found fine, for the record: decode_oem maps 0x00..0x7F to themselves for CP437/CP850, so ascii_valid_up_to is a safe 1:1 proxy for OEM collations too; from a neutral decoder state no SQL Server DBCS collation has a lead byte below 0x80, so the ASCII run cannot swallow a trail byte; and the new !finished guard on narrow_decoder_has_partial_character is genuinely needed (encoding_rs panics on a finished decoder), while the two unguarded call sites later in the function stay safe because their enclosing if !reached_end && read > 0 block is exactly the condition under which the decoder was not finalized.

Verification

Built and ran cargo nextest run -p mssqlodbc --lib -E 'test(plp_) or test(narrow_)' against the assigned target dir: 83 passed, 0 failed. gh pr checks 668 shows no failing required check (Build/Test matrix, CodeQL, Kerberos and cross-repo mssql-python all green; Test MacOS, coverage-report and the umbrella summary still pending). The Windows-only win_tls half was reviewed by reading, not executed. The finding below is from tracing the code, not from a reproduction.

Findings

Blocking

1. mssql-odbc/src/api/get_data.rs — prefetched PLP bytes are consumed and then dropped when the wire top-up fails, silently truncating the value on a retryable ERR_CONNECTION_BUSY. Details inline.

Suggestion

None beyond what is already open on the PR.

Nit

None.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The interacting streaming paths warrant human review, and the TLS buffer reservation still needs correction.

Review effort: Balanced
Findings: 1

Open (1)

• Avoid over-reserving TLS input buffer after short reads · New

Resolved since last review (1)

• Reject oversized TLS record lengths after header parsing

[ttk (Theekshna)]

Unattended sweep review: no findings at any severity.

Covered: the one new commit (e2c31895, oversized-TLS-record rejection) against msodbcsql parity, test sufficiency via mutation trace, divergence documentation, PR description/CI currency, AI-slop, and an independent re-derivation of the reserve-growth evidence in the already-resolved reserve-overhead thread.

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Fresh review of e2c31895 against merge-base f0cbff05. This head adds the oversized-record rejection on top of the two changes reviewed at e203c4e7; I re-read the whole diff rather than just the delta. No new findings.

The new missing_record_bytes gate is correct on the paths I traced. The [_, _, _, hi, lo, ..] arm covers every len >= 5, so the 5 - enc_in.len() fallback cannot underflow, and record_len is bounded by 5 + u16::MAX before the comparison, so no overflow either. MAX_TLS_RECORD = 5 + 16384 + 2048 matches RFC 5246 §6.2.3's TLSCiphertext.length ceiling, and being looser than RFC 8446's 2^14 + 256 is the right call for a stream that may negotiate TLS 1.2. Rejecting from the header alone means a peer can no longer park poll_read on an advertised body no valid record could contain. Dropping missing.max(MAX_TLS_RECORD) down to enc_in.reserve(MAX_TLS_RECORD) is now equivalent, because the overflow check caps missing at MAX_TLS_RECORD.

On the ODBC half I re-derived the bookkeeping around the new narrow_source_fit / restore_source_prefix step. ascii_valid_up_to(source).min(room) keeps room - ascii from underflowing; the worst case is ascii + (room - ascii)/3 decoded bytes expanding to at most room + 2, so the documented overshoot stays in pending_bytes rather than past the caller's buffer. fit.max(1) cannot fire with room == 0, because max_read_for already returns 0 there and min(source.len()) clamps it to an empty read. The total_read rewind pairs correctly with restore_source_prefix's prefetched_total_read_before = total_read - bytes.len(), including the case where a tail prefetch is already queued and the restored bytes are spliced in front of it. progress.wire_read needs no separate rewind here — unlike the widening path at line 2815 — because read is reassigned to fit before line 2775 accumulates it.

Also checked and found fine: transcode_narrow_to_utf8 forces wire_shaped_output false, so direct_wire_output is never true on this path and the enlarged payload never aliases the application's buffer; the retry/refill loop still terminates, since max_read_for(0, 0, 0) returns 0 for the narrow arm; and pending_before moving from the destructured pending_utf8.len() to stream.pending_bytes.len() is the same field under its original name.

I deliberately did not re-file the two things already discussed on this PR and answered by the author: the prefetch-consumed-then-dropped ERR_CONNECTION_BUSY exit, and the enc_in.reserve over-reservation. Both replies are reasoned and I have nothing to add that would change them.

Verification

Built against the assigned CARGO_TARGET_DIR on Windows and ran the affected tests. cargo nextest run -p mssql-tds --lib -E 'test(win_tls::stream)': 13 passed, 0 failed — including all five new record-gate tests, executed against real SChannel rather than read only. cargo nextest run -p mssqlodbc --lib -E 'test(plp_) or test(narrow_)': 83 passed, 0 failed. The wider test(win_tls) selector also fails the three validate_pinned_cert_* certificate-fixture tests, which are the known missing-DER-fixture failures the PR checklist already calls out and are unrelated to this diff. gh pr checks 668 is fully green, including the ADO validation run.

Findings

Blocking

None.

Suggestion

None.

Nit

None.

This looks ready for human review. That is an observation, not an approval, and it does not satisfy the human review requirement.

[Vahid Beiranvand (Vahid-b)]

Unattended automated review on behalf of Vahid Beiranvand (@Vahid-b). This is a COMMENT, not an approval or a substitute for human review.

Summary

Reviewed the complete PR at e2c31895de7149abd7ad0893312ad6b05332fcf1 against merge-base f0cbff05b4084c479a26972252279aac7332c1b0. No new blocking findings, suggestions, or nits after both review rounds. The new regression tests detected all eight temporary mutations I applied.

What changes

The ODBC retrieval layer previously filled narrow SQL_C_CHAR buffers through repeated room/3 reads. It now reads the available room, decodes the fitting prefix, and preserves unconverted bytes for later reads or target switches; short prefetched reads are topped up. In the Windows transport layer, socket reads now go directly into the ciphertext buffer and decryption waits for a complete record, with oversized declarations rejected from the header. This preserves the intended data-delivery contract while reducing read/decode work; it does not change TDS token encoding.

Verification

• Read .github/copilot-instructions.md, .github/instructions/pr-workflow.instructions.md, .github/skills/code-review/SKILL.md and posting.md, .github/instructions/mssql-odbc.instructions.md, and .github/prompts/verify-odbc-changes.prompt.md from refreshed origin/main (685f8bd0c20580c4ecd6c3938fbf1767d21bb6ef). Also read the component README, parity registry, validation/coverage configuration, issue #674, all three prior-discussion endpoints, and review-thread dispositions.
• Used a dedicated detached worktree and isolated CARGO_TARGET_DIR. Re-fetched main after reviewing; the merge base and affected-file set were unchanged. Synchronized read-only reference revisions: native contract bacdc8e78f2704b1fd95e28be3cfb39d06ab2e93; informational reference b54230719eb35604ec7d90b2e50e6923fde9cec6 (refreshed only, not used for a verdict).
• cargo nextest run -p mssqlodbc -p mssql-tds --lib -E 'test(plp_) | test(narrow_) | test(win_tls::stream)' --no-fail-fast: 171 passed on Windows before mutation.
• After restoring everything, cargo nextest run -p mssqlodbc -p mssql-tds --lib -E 'package(mssqlodbc) | test(plp_) | test(narrow_) | test(win_tls::stream)' --no-fail-fast: 1,931 passed, comprising all 1,843 ODBC lib tests and 88 focused TDS tests. Other tests were excluded by the selector, not counted as passing. git diff --exit-code and the PR's git diff --check were clean.

Mutation evidence

Each row was run with cargo nextest run -p mssqlodbc -p mssql-tds --lib -E 'test(plp_cp1252_) | test(win_tls::stream)' --no-fail-fast; row 3 additionally selected test(plp_consecutive_high_surrogates_bound_carry_and_preserve_binary_input).

Temporary edits	Result
No-carry condition changed to false, restoring room/3 reads; decrypt gate changed to missing <= MAX_TLS_RECORD	13 passed, 2 failed: ASCII read counter became 20 instead of 1; partial-record test reached decrypt.
fit < read restoration disabled; oversized-record guard disabled	12 passed, 3 failed: binary suffix indicator became 2700 instead of 2742; both oversized-record tests failed.
Prefetch acceptance changed from read == payload.len() to read <= payload.len(); decrypt gate changed to missing > MAX_TLS_RECORD	13 passed, 3 failed: mixed-text/binary and surrogate top-up tests failed; complete-record test stayed pending.
Partial-character query moved before !finished; socket read restored to the old 8192-byte temporary buffer	13 passed, 2 failed: finished-decoder test failed; socket-read counter became 3 instead of 1.

Restored afterwards; the tracked review worktree is clean.

CI and compatibility

gh pr checks 668 --repo microsoft/mssql-rs --required passes. Validation build 179246 succeeded on merge commit 3e3d8bc87785b4dd635c401cb2024d8e851f9217, whose parents are the refreshed main and this exact PR head.

Parity result: no new divergence found in the reviewed paths. I read the reference GetData carry/indicator branches (sqlcdata.h:573-596,1230-1234) and TLS decrypt path (SNI_SslProvider.cpp:284-320,480-559). Build 179246's Linux comparison log 218 installs reference build 18.6.2.1-1; logs 218 and 240 both report get_data_test PASS PASS and 49 test-binary parity results, zero divergences/shared failures. Existing per-case encoding skips still apply, so these summaries are not proof that every individual case ran on both drivers. Miri logs 261 and 276 each report 25 passed on Windows x64 and Linux x64 respectively.

Not independently rerun: the performance-lab timings in the description and the broader non-Windows runtime suites. Timing improvements remain the author's reported measurements; platform validation is supported by the CI evidence above.

Coverage ledger

Area	Status
Primary logic	Examined: complete PLP and SChannel diff.
Siblings	Examined: CHAR/WCHAR/binary/typed delivery, carry and refill variants.
Callers and implementers	Examined: GetData entry paths, stream state, TDS PLP reader, TLS engine and record layer.
Diagnostics	Examined: top-up errors, cursor closure, deferred errors, indicators and busy ownership.
Tests	Examined: fixtures, real entry paths, Windows tests and eight mutation checks.
Build, packaging, pipelines	Examined: existing tokio-util features and validation routing; no packaging/dependency changes.
Docs and comments	Examined: buffer arithmetic, record ceiling, decoder-finalization claims.
Description and work items	Examined: both changes map to #674 and the current description.
CI evidence	Examined: exact-head checks, tested merge parents, comparison logs and Miri summaries.

R1 handoff: examined input lengths, buffer ownership, FFI exposure, decoder state, and secret/dependency changes; no new security finding. R2 followed the state, diagnostic, sibling, and test paths above; no additional finding. The previously answered prefetch/busy and reserve-growth discussions were considered, not re-filed.

Findings

Blocking: none. Suggestions: none. Nits: none.

Severity	R1	R2	Total
Blocking	0	0	0
Suggestion	0	0	0
Nit	0	0	0

Deferred / to file

No new item from this review. No threads resolved or labels changed.

[David Engel (David-Engel)]

Automated review — unattended run, not checked by a human first. This is a COMMENT, not an approval, and it does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Reviewed the full diff at e2c31895 against merge-base f0cbff05 (mssql-odbc/src/api/get_data.rs, mssql-tds/src/connection/transport/win_tls/stream.rs). No blocking findings, suggestions, or nits. Both halves hold up under tracing and execution.

Narrow PLP refill. narrow_source_fit is sound: ascii_valid_up_to(source).min(room) keeps room - ascii from underflowing, and the worst case ascii + (room - ascii)/3 decoded bytes expands to at most room + 3 — the same bounded overshoot narrow_max_read's doc already accepts, absorbed by pending_bytes rather than the caller's buffer. Every encoding reachable here is ASCII-transparent, which is what makes the leading-run shortcut safe: transcode_narrow_to_utf8 excludes UTF-8 collations, lcid_to_encoding reaches only windows-125x/874, SHIFT_JIS, GBK, BIG5 and EUC-KR (no ISO-2022-JP, the one stateful encoding that would break it), and decode_oem maps 0x00..0x7F to themselves for CP437/CP850. pending_before moving from the destructured pending_utf8.len() to stream.pending_bytes.len() is the same field under its original name (pending_bytes: pending_utf8 in the destructure). The total_read rewind pairs correctly with restore_source_prefix's prefetched_total_read_before = total_read - bytes.len(), and forcing reached_end = false after a restore is what keeps the decoder from being finalized on a chunk whose tail is going back on the stream.

Top-up path. The read == payload.len() gate implies the prefetch buffer is exhausted whenever the top-up runs (read_prefetched_wire returns min(remaining, out.len())), so set_prefetched_wire replacing the buffer in read_plp_wire cannot strand unconsumed read-ahead. transcode_narrow_to_utf8 forces wire_shaped_output false, so the enlarged payload never aliases the application's buffer on the transcoding path.

SChannel record gate. enc_in is record-aligned at index 0 on every entry: it starts as the handshake's SECBUFFER_EXTRA tail, and decrypt either compact_suffixes the extra to the front or clears the buffer, including on the SEC_I_RENEGOTIATE branch. The [_, _, _, hi, lo, ..] arm covers every len >= 5, so the 5 - enc_in.len() fallback cannot underflow, and record_len is bounded by 5 + u16::MAX before the comparison. MAX_TLS_RECORD = 5 + 16384 + 2048 is RFC 5246 §6.2.3's ceiling; being looser than RFC 8446's 2^14 + 256 is right for a stream that may negotiate TLS 1.2. Decrypted::NeedMoreInput with missing == 0 still falls through to a socket read, so the gate cannot livelock.

Verification

Windows, isolated CARGO_TARGET_DIR, dedicated detached worktree.

• cargo nextest run -p mssqlodbc --lib: 1843 passed, 0 failed.
• cargo nextest run -p mssqlodbc --lib -E 'test(plp_) or test(narrow_)': 83 passed, 0 failed.
• gh pr checks 668 --required: no failing required check; ADO validation 179246 green.

Mutation checks I ran (each reverted afterwards; the worktree is clean):

Temporary edit	Result
if fit < read restore disabled	plp_cp1252_mixed_text_streams_and_keeps_raw_bytes_for_binary fails — the guard is load-bearing.
held_partial guard dropped from narrow_source_fit (ASCII run always taken)	All 1843 tests still pass. Traced rather than filed: with a gb18030 lead already carried, the first fed byte can emit 4 UTF-8 bytes, so the overshoot rises from room + 2 to room + 3 and still lands in pending_bytes. max_utf8_buffer_length accounts for held source bytes, so no buffer is undersized. The branch is over-conservative defensive code, not a latent bug, so this is an observation about coverage rather than a finding.

Throwaway probe, the one axis the suite did not cover. ttk (@Theekshna) noted the lab table measures only a CP1252-shaped lob_max, leaving DBCS collations — which share narrow_source_fit and are the only way to reach the held_partial state at all — unmeasured. I wrote a temporary test modelled on plp_cp1252_mixed_text_streams_and_keeps_raw_bytes_for_binary that streams a ~3 KB mixed ASCII/CJK value through SQL_C_CHAR at buffer sizes 2, 3, 4, 5, 6, 7, 8, 9, 16, 33, 64, 101 and 1024, for GBK (CP936) and Shift_JIS (CP932), and compares the concatenated delivery against Encoding::decode_without_bom_handling of the same wire bytes. Byte-identical at every size for both encodings, including the sizes small enough to split a DBCS pair across calls and the sizes large enough to drive fit < read into restore_source_prefix. The probe was removed afterwards; it is evidence, not a request to add a test.

Not re-run: the Windows perf-lab timings in the description, and the non-Windows suites.

Not re-filed

Already raised and answered on this PR, and I have nothing that would change either reply: the prefetch-consumed-then-dropped ERR_CONNECTION_BUSY exit, the enc_in.reserve over-reservation, the oversized TLS record length (fixed in e2c31895), and the work-item link. Vahid Beiranvand (@Vahid-b)'s and David Engel (@David-Engel)'s clean passes at this same head were read before writing this, not duplicated.

Findings

Blocking: none. Suggestions: none. Nits: none.

This looks ready for human review. That is an observation, not an approval, and it does not satisfy the human review requirement.

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Unattended run; these findings were not checked by a human before posting.

Summary

Reviewed the full diff at e2c31895 against merge-base f0cbff05 (mssql-odbc/src/api/get_data.rs, mssql-tds/src/connection/transport/win_tls/stream.rs). No blocking findings, no suggestions, no nits. Both halves hold up under tracing and under mutation.

Narrow PLP refill. The whole-room read is safe because delivery is bounded separately: narrow_source_fit takes the leading ASCII run whole (1:1 for every encoding reachable here) and applies the existing room/3 worst case to the remainder, so decoded output overshoots room by at most a couple of bytes and those land in pending_bytes, not in the caller's buffer. ascii_valid_up_to(source).min(room) keeps room - ascii from underflowing, and fit.max(1) cannot fire with room == 0 because max_read_for already returns 0 there and min(source.len()) clamps an empty read. The ASCII shortcut is sound for the whole reachable encoding set — lcid_to_encoding yields only windows-125x/874, SHIFT_JIS, GBK, BIG5 and EUC-KR (all ASCII-transparent, no ISO-2022-JP), decode_oem maps 0x00..0x7F to themselves for CP437/CP850, and ResolvedDecoder::has_pending_narrow_character already debug_assert!s is_ascii_compatible(). From a neutral decoder state no SQL Server DBCS collation has a lead byte below 0x80, so the run cannot swallow a trail byte; the held_partial guard covers the non-neutral state, and skipping the query when the decoder is finished avoids the encoding_rs panic.

Bookkeeping around the restore. restore_source_prefix splices the unconverted prefix ahead of any queued tail in original order, and prefetched_total_read_before = total_read - bytes.len() pairs with the caller's total_read -= read - fit so the next read_prefetched_wire reports the same cumulative. progress.wire_read needs no separate rewind because read is reassigned to fit before it is accumulated. Forcing reached_end = false is what keeps the decoder from being finalized on a chunk whose tail is going back on the stream.

Top-up path. The reached_end || read == payload.len() gate means the prefetch buffer is always exhausted when the top-up runs (read_prefetched_wire returns min(remaining, out.len())), so set_prefetched_wire inside read_plp_wire cannot strand unconsumed read-ahead, and chunk.total_read already counts the prefetched bytes so read: buffered + chunk.read, ..chunk is consistent.

SChannel record gate. enc_in is record-aligned at index 0 on every entry: it starts as the handshake's SECBUFFER_EXTRA tail, and decrypt either compact_suffixes the extra to the front, clears the buffer, or prepend_leftovers a whole post-handshake record on the SEC_I_RENEGOTIATE branch. The [_, _, _, hi, lo, ..] arm covers every len >= 5, so the 5 - enc_in.len() fallback cannot underflow, and record_len is bounded by 5 + u16::MAX before the comparison. MAX_TLS_RECORD = 5 + 16384 + 2048 is RFC 5246 §6.2.3's ceiling; being looser than RFC 8446's 2^14 + 256 is the right call for a stream that may negotiate TLS 1.2. Decrypted::NeedMoreInput with missing == 0 still falls through to a socket read, so the gate cannot livelock, and tokio_util::io::poll_read_buf over a Vec<u8> sink only returns Ready(Ok(0)) on real EOF, so read_eof_outcome still means what it did.

Verification

Windows, dedicated review worktree, isolated CARGO_TARGET_DIR.

• cargo nextest run -p mssql-tds --lib win_tls::stream: 13 passed, 0 failed, including all five new record-gate tests against real SChannel.
• cargo nextest run -p mssqlodbc --lib plp_: 62 tests, 59 passed on the mutated tree / all passing on the clean tree; the two new plp_cp1252_* tests pass clean.
• The wider win_tls selector also fails the three validate_pinned_cert_* fixture tests. Those are the known missing-DER-fixture failures the checklist already calls out and are unrelated to this diff.
• gh pr checks 668: every check green, including ADO validation build 179246.

Mutation checks (each reverted; git status clean afterwards):

Temporary edit	Result
Decrypt gate forced on for partial records (missing == 0 || true)	poll_read_waits_for_whole_record_without_decrypt_in_one_socket_read fails — reaches DecryptMessage and returns SEC_E_INVALID_HANDLE.
max_read_for narrow arm reverted to unconditional narrow_max_read	plp_cp1252_ascii_fills_char_buffer_in_one_wire_read fails with 20 wire reads instead of 1 — exactly the regression the PR describes.
narrow_source_fit returns source.len() (decode everything, never restore)	three tests fail: plp_cp1252_mixed_text_streams_and_keeps_raw_bytes_for_binary (binary suffix 2700 vs 2742), plp_gbk_completes_source_character_before_binary_switch, plp_null_target_survives_narrow_completion_after_output. The restore contract is guarded on the DBCS side too, not only CP1252.

Not re-run: the Windows perf-lab timings in the description, and the non-Windows suites.

Not re-filed

Already raised and answered on this PR, and I have nothing that would change either reply: the prefetch-consumed-then-dropped ERR_CONNECTION_BUSY exit, the enc_in.reserve over-reservation, the oversized TLS record length (fixed in e2c31895), and the work-item link. The clean passes from Vahid Beiranvand (@Vahid-b) and from earlier automated runs at this same head were read before writing this rather than duplicated; the mutation table above is independent evidence, not a restatement.

Findings

Blocking: none. Suggestion: none. Nit: none.

This looks ready for human review. That is an observation, not an approval, and it does not satisfy the human review requirement.