You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add per-statement row counts, deferred batch errors, and a LOGIN7 name override - #617
Additions to TdsClient / ClientContext that a tool mirroring a server-side batch needs — per-statement row counts, reaching results past a failed statement, and a LOGIN7 server-name override — plus one generic Entra method for token factories. All opt-in: nothing changes for existing callers.
1. Statement errors are part of the batch walk
SELECT 1; RAISERROR('boom', 16, 1); SELECT 2 ends at the first error today: the error drains the rest of the response, so the second result set is unreachable.
A per-command option chooses what a statement error does to the rest of the batch:
#[derive(Default)]pubenumBatchErrorMode{#[default]Abort,// unchanged: the first error drains the stream and returns ErrContinue,// the statement's error returns Err; the batch stays readable}pubstructExecuteOptions<'a>{// ...pubon_error:BatchErrorMode,}
Under Continue, the error arrives at the statement that failed, on the same channel as every other failure:
letmut r = client.execute(sql,ExecuteOptions::new().on_error(BatchErrorMode::Continue)).await;loop{match r {Ok(StatementResult::Rows) => {/* next_row until None, then last_result_row_count() */}Ok(StatementResult::NoRows{ rows_affected }) => {/* per-statement count */}Ok(StatementResult::End) => break,Err(Error::SqlServerError{ .. })if client.has_open_batch() => {/* report, keep going */}Err(e) => returnErr(e),// fatal, transport, or the batch ended}
r = client.advance().await;}
execute, advance and next_row return Err(SqlServerError) for the failing statement without draining. has_open_batch() says whether the batch continues. This is ODBC's SQLMoreResults after SQL_ERROR.
A failed statement is one Err carrying every ERROR it sent (a statement can send several). Between results the ERROR and INFO tokens are read together, bounded like the other token loops, and the first token after them is kept for the next call. The DONE that completes the failed statement is not surfaced again as a result, even when it carries a count.
An error inside a row set comes from next_row once the DONE ending that row set is read; rows read before it are kept, and advance moves on.
A failure in the first statement comes back from execute with the batch explicitly kept open. Batches are otherwise opened by the first result boundary, which a leading error precedes, so without this the next command would read the leftovers.
A fatal error (severity >= 20) still closes the batch and retires the connection. So does any error met while consuming the response — transport, decode, or an unexpected/malformed token, in a row set, between results, or before the first result: after an Err, has_open_batch() is true only when the batch can still be walked. An error raised before anything is read (a UsageError rejecting a call) leaves the walk as it was. Abort is unchanged.
close_query drains a Continue batch past its errors rather than stopping at the first Err — which would clear the batch over unread tokens — and returns the errors it skipped.
The mode is per command: every request boundary resets it to Abort (begin_command for batches, RPCs and transaction requests; check_and_reconnect for the cursor RPCs, which do not call begin_command; the cancellation cleanup, and close_query). Only execute honours it. Every other entry point that takes ExecuteOptions (RPCs, prepared execution, unprepare, transaction requests) uses Abort and logs at debug level when given Continue; prepared batches keep reporting per-row errors through PreparedBatchResult.
Worth a close look during review: under Continue, the DONE tokens that complete a failed statement are read on the next call and carry the error flag, which is otherwise a protocol violation. The allowance is armed only when Continue returns an error. One error can be reflected by both the statement's DONEINPROC and the enclosing DONEPROC, so it survives a DONEINPROC and is spent by the DONEPROC or plain DONE that closes the chain. A later unpaired error-flagged DONE still fails. Live tests cover a failing procedure and a nested failing procedure.
2. Per-statement row counts
No-row statements already report their count on StatementResult::NoRows { rows_affected }. The one missing piece is a row set's DONE count, which is only known after its rows:
/// DONE count of the row set just completed, or None if the server sent none.pubfnlast_result_row_count(&self) -> Option<u64>
Read it once next_row returns None. It resets at every statement boundary, so a no-row statement or a failed statement never reports the previous row set's count. None (e.g. SET NOCOUNT ON) is deliberately distinct from Some(0). A row set's count is reported even when tagged SQLSELECT; that tag is only bogus on variable assignment, which the existing NoRows filtering already drops.
3. LOGIN7 server-name override
pub login_server_name:Option<String> // on ClientContext
Separates where the socket dials from what name is presented at login — what a tunnel or port-forward needs. Layered over main's existing get_login_server_name(): unset or empty, the DataSource derivation still applies. It is reused for a LOGIN7 retry after a routing redirect. A name over LOGIN7's 128-character limit is rejected by validate() with a UsageError.
It does not affect TLS: LOGIN7 is sent after the handshake, and the certificate is validated against the dialled host or host_name_in_cert. Through a tunnel a caller typically sets host_name_in_cert to the same name, or pins with server_certificate.
LOGIN7 stores the name as an offset/length pair separate from its payload, so both come from one accessor, which borrows the override rather than cloning it.
4. Entra token credential
One new variant, ActiveDirectoryTokenCredential: an Entra bearer token acquired by the application's registered EntraIdTokenFactory from whatever source it chooses (Azure CLI, Azure Developer CLI, Azure Pipelines, environment variables, client assertion, …). The source changes how the token is obtained, not what goes on the wire, so it shares the fedauth arm with ActiveDirectoryDefault and a new source needs no new variant. mssql-tds acquires no tokens itself.
ActiveDirectoryMSI had no fedauth arm and fell through to the unsupported-method error — it only worked because the ODBC binding rewrites the keyword first — so it now shares the managed-identity arm, and the catch-all is replaced by an explicit list.
Tests
unit
Continue returning the first statement's error with the batch open; several ERRORs of one statement as one Err, between results and inside a row set; a counted error DONE not surfaced twice; the error bounds on both paths (errors kept, the text they hold, and tokens read), and the most errors allowed still returned together; a transport failure after a statement error closing the batch, between results and inside a row set; an error-flagged DONEPROC with a count not surfaced twice; an error inside a row set; DONEINPROC/DONEPROC chains; the allowance not outliving its statement and not excusing an unpaired DONE; fatal errors closing the batch; any read failure (transport, decode, protocol; in rows, resumed rows, columns, PLP, or between results) ending a Continue walk, and a rejected call not ending it; close_query draining past errors and keeping at most the error bounds of them; the mode resetting at every request boundary, including the cursor path and cancellation; RPC paths staying Abort; prepared batches keeping errors on the row and counts off last_result_row_count; None vs Some(0) and resets at every boundary; fedauth mapping; LOGIN7 sizing, length limit and empty override
e2e (live SQL Server)
results after a failing statement; Abort unchanged; an error in the last statement; a statement with two ERRORs (primary key over duplicates) as one Err; rows kept before a mid-row-set error; a server-aborted batch; a failing and a nested failing stored procedure; errors unwinding nested procedures (three frames deep, batch aborts with and without a row set, XACT_ABORT); close_query past errors; per-statement counts; SET NOCOUNT ON; variable_assignment_counts_are_not_reported, the parity test for the SQLSELECT bogus-count filter (sqlctokn.cpp:2149)
mock server
ServerName read back off the wire, including longer/shorter overrides
Verification
cargo bfmt, cargo bclippy (workspace and mssql-py-core) pass.
mssql-tds unit tests: 2322/2322. Each new guard is pinned by a test that fails when the guard is removed.
Full workspace (cargo nextest run --workspace) against a live SQL Server: 4692 passed, 11 failed — all environmental (mock-server TLS certificates not generated, SSRP named instance, named-pipe access, self-signed certificate in the connectivity tests). The same tests fail on the head before this round.
Note
The iteration-guard hardening in advance_to_result_boundary has been reverted to main's original code, to be reviewed as its own change.
DONE token accounting varies by response navigation path
mssql-tds/src/connection/tds_client.rs:7674
take_done_row_counts() does not consistently return one entry per DONE token. Several response readers bypass record_done_row_count; for example, complete_current_result() consumes an RPC's terminal DONEPROC in settle_rpc_terminator() (line 5370) without recording it, while advancing through the same response via advance() reaches the generic DONE arm and does record it. The returned log therefore depends on which navigation API consumed an otherwise identical response. Route DONE accounting through every token-consumption path (or a shared observation hook).
Consume error allowance on every DONE to prevent stale acceptance
mssql-tds/src/connection/tds_client.rs:5013
consumed_pending_error() is only called when DONE_ERROR is set. If a collected ERROR is followed by a malformed DONE without that flag, the allowance remains armed and an unrelated later DONE_ERROR is accepted, so the promised one-DONE desynchronization check is weakened. Consume the allowance on every DONE and use the captured value only to decide whether an error-flagged DONE is valid.
This issue also appears on line 7600 of the same file.
Update PR description to reflect terminal error API semantics
mssql-tds/src/connection/tds_client.rs:7657
The PR description still promises that errors which end a batch “surface as Err,” but this public contract—and the implementation—now queues terminal errors and returns end-of-results. Please update the PR description to match the final API semantics so reviewers and downstream callers are not given the opposite behavior.
…e override
Three additions to TdsClient and ClientContext that a sqlcmd-style tool needs,
plus the Entra methods go-sqlcmd names. All are opt-in; nothing changes for
existing callers.
Per-statement DONE row counts. `count_map` only kept a running total per
command, so `UPDATE; DELETE; INSERT` reported one number instead of three.
`take_done_row_counts()` returns one entry per DONE in arrival order. `None`
marks a statement that reported no count at all, which is what `SET NOCOUNT ON`
produces, and is deliberately distinct from `Some(0)`.
Deferred batch errors. The first ERROR token ends the batch today, so
`SELECT 1; RAISERROR('boom', 16, 1); SELECT 2` loses the second result set.
Under `set_defer_batch_errors(true)` the errors are collected and iteration
follows the DONE tokens to the end; `take_pending_errors()` retrieves them.
The DONE that closes a collected error carries the error flag with no preceding
ERROR, which is normally a protocol violation, so `consumed_pending_error()`
excuses exactly one such DONE and the violation check stays intact otherwise.
LOGIN7 server-name override. `ClientContext::login_server_name` separates where
the socket dials from what name is presented at login, which is what a tunnel
or port-forward needs. It layers over the existing DataSource derivation rather
than replacing it: unset, `get_login_server_name()` still applies. The field is
stored in LOGIN7 as an offset/length pair separate from its payload, so both
the length and the bytes go through one accessor.
Entra methods. Five variants added for the credential sources go-sqlcmd
accepts. They all resolve to a bearer token out of band, so they share the
fedauth arm with ActiveDirectoryDefault. ActiveDirectoryMSI now shares the
managed-identity arm it always should have.
Tests: 9 unit and 2 e2e added, alongside 4 mock-server LOGIN7 tests that read
the ServerName back off the wire and 2 e2e deferred-error tests. Workspace run
is 4411 tests, 4388 passed, 23 failed — the same 23 that fail on main
unmodified in this environment (named pipe, shared memory and mock TLS).
- Route deferred errors through record_error_token so a fatal error still
retires the connection and the error stays attached to the prepared row
- Size the LOGIN7 record from the server name actually written rather than
the dialled address, so an override of a different length cannot corrupt
the record Length and the feature-extension offset
- Map the five new Entra variants in mssql-py-core, whose match over
TdsAuthenticationMethod is exhaustive and no longer compiled
- Correct the set_defer_batch_errors docs: while deferral is on every error
is collected, including the one that ends the batch
- Add regression tests for the length and fatal-retirement bugs; both were
confirmed to fail against the unfixed code
handle_row_read_token checked deferral before the prepared-batch arm, so a
prepared batch that errored mid-rows had the error queued on pending_errors
as well as filed on the row, while read_prepared_batch_result — which sees
the no-result-set case — only filed it on the row. The channel an error
arrived on therefore depended on the shape of the result, and prepared
batches double-reported.
Prepared batches own their error channel, so deferral now defers to it:
the prepared-batch arm runs first and the deferred queue is left for plain
batch iteration.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
DONE accounting is incomplete across response paths, the PR description contradicts terminal-error behavior, and the LOGIN7 tests contain a timing race.
unreported_error is only consumed on the right-hand side of done.has_error() && ..., so a deferred ERROR followed by a DONE without DONE_ERROR leaves the marker set. That sequence is already treated as valid elsewhere (for example a_deferred_fatal_error_still_retires_the_connection uses ERROR + done_no_more()), and within a continuing response the stale marker can incorrectly excuse a later error-flagged DONE that had no preceding ERROR. Consume the marker for every DONE, then use the saved value when validating done.has_error() in both DONE handlers.
Reset command buffers before every cursor request
mssql-tds/src/connection/tds_client.rs:7896
These per-command buffers are reset only by begin_command(), but the CursorClient request paths in connection/cursor_ops.rs deliberately call check_and_reconnect() directly and never invoke this prologue. A completed query's counts/errors therefore survive into a subsequent cursor request, and DONE/errors consumed by cursor_fetch are appended to stale data, violating the “current or most recent command” contract. Add a shared per-request reset that is also called by every cursor operation before sending its RPC.
take_done_row_counts claimed every DONE of the current request, but counts
are recorded only on the result-set iteration path. The other DONE readers
each report through their own API — bulk copy returns its total directly,
prepared batches carry per-row counts in PreparedBatchResult, and transaction
control has none — so recording there would double-count rather than complete
the log. Documented the scope instead of widening it.
The LOGIN7 tests waited a fixed 200ms for the mock to record a connection,
which nothing awaits: the store is written when the detached handler exits,
so a slow runner could fail a correct test. Poll under a bounded deadline.
SQL Server tags variable assignment (SET @x = 1, SELECT @x = col FROM t) as
SQLSELECT and still sets DONE_COUNT, so the log reported counts neither
msodbcsql nor SqlClient surfaces — a tool printing "(N rows affected)" per
statement emitted a line for each. A real row-returning SELECT carries the
same tag, so the tag alone cannot separate them; what differs is whether the
statement produced a result set, which is already implied by which reader
sees the DONE.
Row-returning statements keep their count, assignment statements record None.
Verified against sqlcmd: the reference prints two counts for a batch mixing
assignment, INSERT and SELECT, and so do we.
- The allowance for a failed statement's error-flagged DONEs now ends when the
next result set begins, as well as at the DONE closing its chain. Live
captures put the failing statement's flagged DONEINPROC before the next
COLMETADATA and never flag the enclosing DONEPROC, so an error-flagged DONE
after that point is unpaired and is reported.
- close_query collects only the skipped errors again: every error reaching it
carries no info messages, which stay in info_messages for take_info_messages.
- Test helpers: run_ddl moves up with the others, and blank lines separate the
nested helper functions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
A live test of the procedure shapes behind the latest review question: an
error before a count, after it, and a batch abort after one. Each count
arrives on its own DONEINPROC and is reported once; the procedure's DONEPROC
carries no count, with or without the error.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
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.
This came from an unattended run; the findings were not checked by a human before posting.
Summary
Three independent additions to mssql-tds: BatchErrorMode::Continue so a statement error no longer drains the rest of a SQL batch, last_result_row_count() to close the per-statement count gap for row sets, and a ClientContext::login_server_name override for LOGIN7, plus the ActiveDirectoryTokenCredential variant and an ActiveDirectoryMSI fedauth arm that was previously missing. The design reasoning is unusually well documented in the code itself, the Continue state machine routes every response-consuming failure through one rule (end_walk_on_read_error), and the DONE-error allowance is scoped tightly enough that an unpaired error-flagged DONE still fails. I found nothing blocking. Two nits below, one of them a quiet reversal of an earlier agreed fix.
The delta since the last clean automated pass (2d864705) is the two newest commits: ending the DONE-error allowance at the next result set, and pinning row counts around a failing statement inside a procedure. That is where both nits live.
Verification
Checked out at 276ea073 in an isolated worktree and diffed against the merge base 709e48cd. Read the full substantive diff plus the surrounding advance_to_result_boundary / handle_row_done / close_query / validate_done_error paths, drain_stream, begin_command, normalize_after_attention, retire_after_failed_drain, and every ExecuteOptions consumer. Read all prior review bodies, inline threads and PR comments before drafting, so answered items are not re-filed.
cargo fmt -- --check — passed.
cargo nextest run -p mssql-tds --lib on a 24-test filter covering the new guards — 24 passed.
Mutation checks, the thing worth building locally for: reverting calculate_login_record_length to the dialled name fails the_record_length_follows_the_server_name_actually_written; narrowing validate_done_error's chain close to done.has_error() fails an_unflagged_done_still_closes_the_error_completion_chain; deleting the new statement_error_completion_pending = false in the COLMETADATA arm fails the_allowance_does_not_span_the_next_row_set. None of the three guards is vacuously tested.
CI is green on this head, including the cross-repo mssql-python suite and the live-server ADO validation that the e2e tests in query_results.rs depend on, so I did not re-run those.
Compile reach of the new TdsAuthenticationMethod variant was left to CI; the check, Windows/Linux/macOS build and mssql-py-core jobs all pass on 276ea073.
Findings
Blocking
None.
Suggestion
None.
Nit
Two, both inline: close_query quietly reverting the from_sql_diagnostics fix agreed earlier in this PR (tds_client.rs:8493), and a redundant to_string() in the new LOGIN7 sizing test (login.rs:1665).
- The ServerName written into LOGIN7 is logged at debug rather than info, so
every login no longer puts a host name into default logs.
- The mock-server tests always stop the server and check its outcome: a server
error or panic now fails the test, and a connect or read failure is reported
only after the server has been shut down.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- The close_query comment now names what makes collecting only errors safe:
every SqlServerError reaching the drain is built by Error::from_sql_errors,
which attaches no info messages.
- The LOGIN7 length test calls len_bytes on the &str directly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of Vahid Beiranvand (@Vahid-b). 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
The login-server override and batch-error continuation changes look internally consistent; I did not find a correctness issue in the touched paths. The PR appears ready for human review but has not been approved.
Verification performed
Reviewed mssql-tds/src/connection/client_context.rs, mssql-tds/src/message/login.rs, mssql-tds/src/message/features/fedauth.rs, mssql-tds/src/connection/tds_client.rs, and mssql-py-core/src/python_entra_token_factory.rs at head 79a6062d0181898f65cb7c2de82a68169616dea6.
Ran the targeted client-context and batch-behavior tests earlier in the review run; they passed.
Rechecked the live required checks before posting.
Findings Blocking
None.
Suggestion
None.
Nit
None.
Required CI failures
mssql-rs Pull request validation (Build Stage Build Linux ARM) is failing on the current head.
The remaining required checks are still in progress.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the delta since the last marker (5 commits: 6a16268276ea073a6741779a6062fa66d46), diffed against 2d86470 (last reviewed head). Full discussion history (103 threads, prior Vahid-b/David-Engel/bot rounds) re-read for context.
New delta is correct.tds_client.rs: statement_error_completion_pending now clears on the COLMETADATA/new-row-set arm, backed by the_allowance_does_not_span_the_next_row_set — traced this doesn't regress the valid DONEINPROC→DONEPROC nested-completion path (different arm). close_query reverted to Vec<SqlErrorInfo>-only (from SqlServerDiagnostics) per the new doc comment's claim that info messages reach callers via take_info_messages(): verified by grep that every Error::from_sql_diagnostics construction (the only one carrying info messages) is confined to handler_factory.rs's login path, unreachable from close_query's drain — so this correctly resolves David-Engel's "reverts the agreed fix" Nit rather than reintroducing it. login.rs/tds_client.rsinfo!→debug! changes resolve the bot's repeated noisy-logging findings. query_results.rs test helper now panics on an unexpected first-column type instead of silently dropping it (resolves the bot's finding directly). test_login_server_name.rs restructure: shutdown now always runs and its outcome is checked even when connect/read fails (previously a failing assertion could skip shutdown), and the polling loop no longer allocates a Vec per iteration (connections.values().next() vs collect::<Vec<_>>().first()) — this fixes the bot's two remaining threads on this file in code; those GraphQL threads are still open only because nobody clicked resolve, not because the fix is missing.
Six checks:
msodbcsql parity — N/A, delta touches no ODBC/C++ surface.
Test sufficiency — cargo nextest run -p mssql-tds --lib fails to compile in this sandbox on crc32fast's AVX-512/pclmulqdq codegen (E9012/E9013, via the azure_identity dev-dependency chain), identical to the failure documented on #683's review today — confirmed again, not assumed. cargo check -p mssql-tds --lib --tests --offline passes clean. Reasoned the new-test mutations statically: reverting the COLMETADATA-arm clear breaks the_allowance_does_not_span_the_next_row_set's assertion that the allowance doesn't survive into the next row set. No untested branch identified in the delta.
Divergences — N/A, no new SqlClient/msodbcsql divergence in this delta.
PR description currency — "Tests" table still matches: the new unit test (row-set boundary) and new e2e test (nested failing procedure, live SQL Server) are both already represented in the existing table's wording. AB#48820 still linked. gh pr checks is currently all-pending (fresh push); no contradiction with the checklist.
Slop — clean. New doc comments (login_server_name_on_the_wire, read_login_server_name) state an invariant (shutdown-before-assert ordering) rather than restating code.
Evidence audit — re-derived the the_allowance_does_not_span_the_next_row_set claim from the diff itself (confirmed above). Could not reproduce "mssql-tds unit tests: 2322/2322" or "workspace 4692 passed, 11 failed" — both require a build/run this environment's crc32fast codegen bug blocks, same limitation as #683's review. Categorization of the two new tests as unit vs. e2e in the PR's own table matches each test's actual file/location.
On the 2 bot threads still genuinely open (not fixed by this delta):tds_client.rs:76's MAX_ERRORS_PER_FAILED_STATEMENT=10_000 bound and tests/connectivity.rs:81's todo!() for ActiveDirectoryTokenCredential are both real and already filed by the bot — not re-filing. The todo!() one matches the pre-existing pattern of 9 other unimplemented auth-method arms in that same test helper, so it's not a gap specific to this PR's new variant.
The reason will be displayed to describe this comment to others. Learn more.
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
Three opt-in additions to TdsClient/ClientContext for a sqlcmd-style batch walker — BatchErrorMode::Continue so a statement error no longer drains the rest of the batch, last_result_row_count() for a row set's DONE count, and a login_server_name LOGIN7 override — plus the ActiveDirectoryTokenCredential fedauth variant and an ActiveDirectoryMSI arm that was previously falling through to the unsupported-method error. The design holds up: the Continue state machine is confined behind batch_error_mode, every request boundary resets it through one reset_statement_walk_state(), and every path that consumes response bytes funnels its failures through end_walk_on_read_error, so has_open_batch() after an Err is a reliable "can this batch still be walked" answer. Nothing I found blocks merge; the two findings below are doc-scope observations on the new public login_server_name surface.
Verification
Read the full non-test diff against merge base 709e48c (tds_client.rs, client_context.rs, login.rs, fedauth.rs) plus the surrounding unchanged callers — next_response_token's park/replay contract, retire_after_failed_drain / take_settled_drain_interruption, settle_rpc_terminator, and handle_row_done's DONE accounting.
cargo nextest run -p mssql-tds --lib --no-fail-fast: 2324 run, 2317 passed. The 7 failures are the known fixture gap on a clean tree (certificate_validator::*, win_tls::validate::validate_pinned_cert_* needing generated tests/test_certificates/*.pem), unrelated to this diff.
Mutation-tested the subtlest new guard rather than trusting the test names: collapsing validate_done_error's chain allowance so a DONEINPROC spends it (instead of only a DONEPROC/DONE closing the chain) fails a_returned_error_allows_error_only_on_done_proc, a_returned_error_allows_error_on_done_in_proc_and_done_proc, and an_error_flagged_done_proc_with_a_count_is_not_surfaced_again. The DONEINPROC/DONEPROC logic is genuinely pinned. Mutation reverted.
API surface: the new ExecuteOptions::on_error field is not breaking — every out-of-workspace construction (mssql-py-coreasync_execute.rs, cursor.rs) uses ..Default::default(), and BatchErrorMode is reachable publicly via pub mod tds_client.
login_server_name's 128-UTF-16-unit cap is actually enforced on the connect path: tds_connection_provider.rs:111 calls context.validate()? before login.
E2E additions create only #-prefixed temp tables and temp procedures, so they are session-scoped and safe under parallel runs.
AB#48820 is linked and the description matches the diff. CI was queued/running at review time with nothing red.
Findings
Blocking
None.
Suggestion
mssql-tds/src/connection/client_context.rs:348 — the new field's doc rules out a TLS interaction but not the Kerberos one; worth one sentence pointing at server_spn. Details inline.
Nit
mssql-tds/src/connection/client_context.rs:235 — MAX_LOGIN7_NAME_UNITS's doc is written for "a LOGIN7 variable-length field" generally, while only one field is checked. Details inline.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
- Keep at most 1,000 errors and 8 MiB of error text per failed statement
under Continue, on both the look-ahead and the row-set paths.
- Serializer resolves the LOGIN7 ServerName once for length, header and
payload.
- Document that the integrated-auth SPN follows the dialled address
(set server_spn through a tunnel) and that only ServerName is length-
checked; move client_context imports to the top.
- Connectivity test helper reports an unsupported auth method as an error
instead of todo!().
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lling
- close_query keeps at most 1,000 skipped statement errors and 8 MiB of
their text when draining a Continue batch, as a prefix; it still drains
to the end and logs how many it dropped.
- The login server-name tests read the mock's store once: it records
LOGIN7 before sending LoginAck, so no polling is needed.
- Pass the cached LOGIN7 server name as &str explicitly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
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
Reviewed the full diff at 29cc05bd against origin/main (8 files, ~3,700 insertions): the BatchErrorMode::Continue statement-error walk in tds_client.rs, last_result_row_count, the login_server_name LOGIN7 override, the fedauth arm changes, and all new unit/e2e/mock tests. I found nothing I would ask you to change.
Specifically re-derived and could not break: the DONE-error allowance is armed only by a returned Continue error, survives a DONEINPROC, and is spent by the enclosing DONEPROC/DONE or cleared by the next COLMETADATA, so an unpaired error-flagged DONE still fails. completes_failed_statement suppresses both the duplicate result and the update count. Every response-consuming exit routes through fail_read/end_walk_on_read_error/end_walk_on_try_read_error, so a non-SQL failure closes the batch and retires the connection while a UsageError raised before any read leaves the walk intact; settle_interrupted_read runs normalize_after_attention, which resets the walk, so a cleanly settled timeout or cancellation is not retired twice. The mode is put back to Abort at begin_command, check_and_reconnect, normalize_after_attention and close_query, so it cannot outlive its command or survive a pool checkout. The look-ahead loops are bounded on errors kept, error text, and total tokens on both the between-results and in-row-set paths, and close_query keeps draining past statement errors with a bounded prefix rather than stopping over unread tokens.
On the smaller pieces: calculate_login_record_length, write_server_name and the ServerName payload now all read one resolved Cow, so the embedded Length, the offset/length pair and the bytes cannot diverge — the removal of impl SizedLoginItem for TransportContext is what makes that structural rather than conventional. The fedauth catch-all is replaced by an explicit list, so the next variant is a compile error instead of a runtime ProtocolError. I checked the four in-repo TdsAuthenticationMethod match sites outside the workspace build: mssql-py-core/src/python_entra_token_factory.rs is updated in this PR, and mssql-py-core/src/connection.rs:431, mssql-py-core/src/odbc_auth/odbc_authentication_transformer.rs:31 and mssql-odbc/src/auth/entra.rs:424 all have catch-alls, so the excluded crate still compiles. All e2e objects are #-prefixed temporaries, and the mock-server test shuts the server down and checks its join result before asserting.
Verification
cargo clippy --workspace --all-features --all-targets -- -D warnings — passed. (cargo bclippy could not be used as-is: Cargo.lock is not tracked, so --frozen fails in a fresh worktree. Not a PR defect.)
cargo nextest run -p mssql-tds --lib — 2330 run, 2323 passed, 7 failed. All 7 are the certificate_validator and win_tls::validate fixture tests that need generated test certificates; they fail identically on origin/main in this worktree and are unrelated to the diff.
Mutation probes against two of the new guards, to check they are pinned rather than merely present:
dropping && !completes_failed_statement from has_update_count fails a_counted_done_in_proc_completing_a_failed_statement_is_not_surfaced, a_failed_statement_is_not_surfaced_again_by_its_counted_done and an_error_flagged_done_proc_with_a_count_is_not_surfaced_again;
dropping the post-drain last_result_row_count = None in advance() fails advance_clears_the_previous_row_set_count.
Both reverted; the worktree is clean.
Read all 100 review threads (all resolved) plus the PR body and the ADO summary for AB#48820, and reconciled again immediately before submitting. Nothing I raise here duplicates a settled thread.
No live SQL Server was used, so the e2e tests in query_results.rs were reviewed by reading only.
Findings
None. No blocking issues, suggestions, or nits.
This looks ready for human review. To be explicit: this is not an approval, and a human reviewer still needs to sign off.
…he misuse exemption
- Serializer refuses a ServerName over 128 UTF-16 units, override or
dialled address, with a clear UsageError before writing anything.
SQL Server accepts 128 and drops the login at 129, which surfaced as an
opaque ProtocolError.
- Every synchronous transport read inside the try_* row reads maps a
UsageError to a ProtocolError, so the UsageError exemption from ending
a Continue walk can only ever cover calls rejected before reading.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
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
Reviewed the full diff at 4258fc2b against origin/main (8 files, ~3,900 insertions): the BatchErrorMode::Continue statement-error walk in tds_client.rs, last_result_row_count, the login_server_name LOGIN7 override and its new length guard, the fedauth arm changes, and the new unit/e2e/mock tests. I found nothing I would ask you to change.
The three commits since the previous clean pass (29cc05bd) are the LOGIN7 ServerName length guard, buffered_read_error, and the connectivity-test token change. I re-derived each: check_server_name_length runs first in serialize(), before any byte is written, and reads the same resolved Cow that calculate_login_record_length and write_server_name use, so the embedded Length, the offset/length pair and the payload still cannot diverge; the Cow::Borrowed/Cow::Owned split that picks the remedy text is exactly the override/derived split in ClientContext::login_server_name, since the fallback always builds an owned String. buffered_read_error only rewrites UsageError coming back from the synchronous try_* transport reads, which keeps the UsageError exemption in end_walk_on_try_read_error meaning "the call was rejected before reading"; I checked the network_transporttry_* paths and their decoders, and none of them produce UsageError today, so this is a defensive narrowing rather than a variant change existing Abort-mode callers would observe. The connectivity-test change removes todo!()/unwrap() from token acquisition and routes failures through the factory, so an unsupported method fails the connection with a reason instead of aborting the test process.
On the rest of the diff, re-derived and could not break: the DONE-error allowance is armed only by a returned Continue error, survives a DONEINPROC, is spent by the enclosing DONEPROC/DONE and cleared by the next COLMETADATA, so an unpaired error-flagged DONE still fails; completes_failed_statement suppresses both the duplicate result and its update count. Every response-consuming exit routes through fail_read/end_walk_on_read_error/end_walk_on_try_read_error, so a non-SQL failure closes the batch and retires the connection while a pre-read UsageError leaves the walk intact. The mode returns to Abort at begin_command, check_and_reconnect, normalize_after_attention and close_query, so it cannot outlive its command or survive a pool checkout; cursor_ops.rs takes no ExecuteOptions, which matches the claim that the cursor RPCs are covered by check_and_reconnect. The look-ahead loops are bounded on errors kept, error text and total tokens on both the between-results and in-row-set paths, and each bound is below the ERROR token's own protocol ceiling. The close_query drain keeps going past statement errors and makes progress on every iteration, since each kept error has consumed at least its own ERROR or DONE token. handle_row_done sets current_result_set_has_been_read_till_end before it returns the collected row-set errors, so the failed row set cannot be re-read.
Verification
cargo clippy --workspace --all-features --all-targets -- -D warnings — passed. (cargo bclippy is not usable as-is here: Cargo.lock is untracked, so --frozen fails in a fresh worktree. Not a PR defect.)
cargo nextest run -p mssql-tds --lib — 2332 run, 2325 passed, 7 failed. All 7 are the certificate_validator and win_tls::validate fixture tests that need generated test certificates; they are unrelated to this diff and fail the same way on origin/main in this worktree.
Read the PR body, the ADO summary for AB#48820, every prior review and all review threads, and reconciled the live PR state again immediately before submitting. Nothing here duplicates a settled thread.
Required CI on 4258fc2b has no failing check at the time of writing; several Azure Pipelines legs are still running.
No live SQL Server was used, so the e2e tests in query_results.rs were reviewed by reading only. All of their database objects are #-prefixed temporaries.
Findings
None. No blocking issues, suggestions, or nits.
This looks ready for human review. To be explicit: this is not an approval, and a human reviewer still needs to sign off.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
David Engel (David-Engel)
left a comment
Vahid Beiranvand (Vahid-b)
left a comment
ttk (Theekshna)
left a comment
What
Work item: AB#48820 — update mssql-tds to support the new mssql-tds-cli features
Additions to
TdsClient/ClientContextthat a tool mirroring a server-side batch needs — per-statement row counts, reaching results past a failed statement, and a LOGIN7 server-name override — plus one generic Entra method for token factories. All opt-in: nothing changes for existing callers.1. Statement errors are part of the batch walk
SELECT 1; RAISERROR('boom', 16, 1); SELECT 2ends at the first error today: the error drains the rest of the response, so the second result set is unreachable.A per-command option chooses what a statement error does to the rest of the batch:
Under
Continue, the error arrives at the statement that failed, on the same channel as every other failure:execute,advanceandnext_rowreturnErr(SqlServerError)for the failing statement without draining.has_open_batch()says whether the batch continues. This is ODBC'sSQLMoreResultsafterSQL_ERROR.Errcarrying every ERROR it sent (a statement can send several). Between results the ERROR and INFO tokens are read together, bounded like the other token loops, and the first token after them is kept for the next call. The DONE that completes the failed statement is not surfaced again as a result, even when it carries a count.next_rowonce the DONE ending that row set is read; rows read before it are kept, andadvancemoves on.executewith the batch explicitly kept open. Batches are otherwise opened by the first result boundary, which a leading error precedes, so without this the next command would read the leftovers.Err,has_open_batch()is true only when the batch can still be walked. An error raised before anything is read (aUsageErrorrejecting a call) leaves the walk as it was.Abortis unchanged.close_querydrains aContinuebatch past its errors rather than stopping at the firstErr— which would clear the batch over unread tokens — and returns the errors it skipped.Abort(begin_commandfor batches, RPCs and transaction requests;check_and_reconnectfor the cursor RPCs, which do not callbegin_command; the cancellation cleanup, andclose_query). Onlyexecutehonours it. Every other entry point that takesExecuteOptions(RPCs, prepared execution,unprepare, transaction requests) usesAbortand logs at debug level when givenContinue; prepared batches keep reporting per-row errors throughPreparedBatchResult.Worth a close look during review: under
Continue, the DONE tokens that complete a failed statement are read on the next call and carry the error flag, which is otherwise a protocol violation. The allowance is armed only whenContinuereturns an error. One error can be reflected by both the statement'sDONEINPROCand the enclosingDONEPROC, so it survives aDONEINPROCand is spent by theDONEPROCor plainDONEthat closes the chain. A later unpaired error-flagged DONE still fails. Live tests cover a failing procedure and a nested failing procedure.2. Per-statement row counts
No-row statements already report their count on
StatementResult::NoRows { rows_affected }. The one missing piece is a row set's DONE count, which is only known after its rows:Read it once
next_rowreturnsNone. It resets at every statement boundary, so a no-row statement or a failed statement never reports the previous row set's count.None(e.g.SET NOCOUNT ON) is deliberately distinct fromSome(0). A row set's count is reported even when taggedSQLSELECT; that tag is only bogus on variable assignment, which the existingNoRowsfiltering already drops.3. LOGIN7 server-name override
Separates where the socket dials from what name is presented at login — what a tunnel or port-forward needs. Layered over main's existing
get_login_server_name(): unset or empty, the DataSource derivation still applies. It is reused for a LOGIN7 retry after a routing redirect. A name over LOGIN7's 128-character limit is rejected byvalidate()with aUsageError.It does not affect TLS: LOGIN7 is sent after the handshake, and the certificate is validated against the dialled host or
host_name_in_cert. Through a tunnel a caller typically setshost_name_in_certto the same name, or pins withserver_certificate.LOGIN7 stores the name as an offset/length pair separate from its payload, so both come from one accessor, which borrows the override rather than cloning it.
4. Entra token credential
One new variant,
ActiveDirectoryTokenCredential: an Entra bearer token acquired by the application's registeredEntraIdTokenFactoryfrom whatever source it chooses (Azure CLI, Azure Developer CLI, Azure Pipelines, environment variables, client assertion, …). The source changes how the token is obtained, not what goes on the wire, so it shares the fedauth arm withActiveDirectoryDefaultand a new source needs no new variant.mssql-tdsacquires no tokens itself.ActiveDirectoryMSIhad no fedauth arm and fell through to the unsupported-method error — it only worked because the ODBC binding rewrites the keyword first — so it now shares the managed-identity arm, and the catch-all is replaced by an explicit list.Tests
Continuereturning the first statement's error with the batch open; several ERRORs of one statement as oneErr, between results and inside a row set; a counted error DONE not surfaced twice; the error bounds on both paths (errors kept, the text they hold, and tokens read), and the most errors allowed still returned together; a transport failure after a statement error closing the batch, between results and inside a row set; an error-flagged DONEPROC with a count not surfaced twice; an error inside a row set;DONEINPROC/DONEPROCchains; the allowance not outliving its statement and not excusing an unpaired DONE; fatal errors closing the batch; any read failure (transport, decode, protocol; in rows, resumed rows, columns, PLP, or between results) ending aContinuewalk, and a rejected call not ending it;close_querydraining past errors and keeping at most the error bounds of them; the mode resetting at every request boundary, including the cursor path and cancellation; RPC paths stayingAbort; prepared batches keeping errors on the row and counts offlast_result_row_count;NonevsSome(0)and resets at every boundary; fedauth mapping; LOGIN7 sizing, length limit and empty overrideAbortunchanged; an error in the last statement; a statement with two ERRORs (primary key over duplicates) as oneErr; rows kept before a mid-row-set error; a server-aborted batch; a failing and a nested failing stored procedure; errors unwinding nested procedures (three frames deep, batch aborts with and without a row set,XACT_ABORT);close_querypast errors; per-statement counts;SET NOCOUNT ON;variable_assignment_counts_are_not_reported, the parity test for theSQLSELECTbogus-count filter (sqlctokn.cpp:2149)Verification
cargo bfmt,cargo bclippy(workspace andmssql-py-core) pass.mssql-tdsunit tests: 2322/2322. Each new guard is pinned by a test that fails when the guard is removed.cargo nextest run --workspace) against a live SQL Server: 4692 passed, 11 failed — all environmental (mock-server TLS certificates not generated, SSRP named instance, named-pipe access, self-signed certificate in theconnectivitytests). The same tests fail on the head before this round.Note
The iteration-guard hardening in
advance_to_result_boundaryhas been reverted to main's original code, to be reviewed as its own change.