Skip to content

Stage 1: mssql-tds reset correctness (ADO #47337) - #294

Draft
Saurabh Singh (saurabh500) wants to merge 1 commit into
saurabh500-odbc-connection-pooling-planfrom
saurabh500-stage-1-tds-reset-correctness
Draft

Stage 1: mssql-tds reset correctness (ADO #47337)#294
Saurabh Singh (saurabh500) wants to merge 1 commit into
saurabh500-odbc-connection-pooling-planfrom
saurabh500-stage-1-tds-reset-correctness

Conversation

@saurabh500

@saurabh500 Saurabh Singh (saurabh500) commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Stage 1 of the mssql-odbc connection pooling effort (ADO User Story #47317). This is the bottom implementation layer, scoped to mssql-tds only — it makes a physical connection reset actually clean the session-bound client state so a pool can safely hand a reused connection to the next borrower. mssql-odbc wiring comes in later stages.

Implements Workstream A (A1, A3) and discovery D3 from docs/odbc-connection-pooling-plan.md.

A1 — Centralize the ResetConnection ENVCHANGE acknowledgement

Previously the ResetConnection ENVCHANGE was handled in 5 token-draining spots that each only called session_state_table.reset(), leaving session-bound client caches stale after a real server reset.

  • New private TdsClient::on_reset_connection_ack() performs the full post-reset transition: resets the session-state table, clears prepared_handles / prepared_param_encryption / pending_capture / pending_prepared_param_encryption, and restores the negotiated DATABASE / LANGUAGE / COLLATION back to their login values (so a borrower's USE otherdb / SET LANGUAGE doesn't persist across a reset).
  • The cache-clearing is factored into a shared clear_session_bound_caches() helper reused by the reconnect path.
  • All 5 ResetConnection sites now call the single method; the following capture_change_property ordering is preserved (login-default restore moved out of execution_context into the single owner to avoid duplication).
  • Per D5 (verified), no post-login ANSI SET-option batch is re-emitted — ANSI defaults ride the fODBC LOGIN7 bit that sp_reset_connection restores. No BuildServerSideConnectOptions equivalent added.

D3 — Mark the transport dead on fatal server error tokens

known_dead was only set on socket write/read failure, EOF, or explicit close; a class ≥ 20 fatal error on a still-open socket left is_connection_dead() reporting alive.

  • Added TdsTransport::mark_known_dead() (default no-op; overridden by NetworkTransport).
  • The shared error-token path (record_error_token) now marks the transport known-dead when severity >= 20, so a later pool checkout discards it.

A3 — Tests

  • Unit test asserting the reset transition clears all four caches and restores DATABASE/LANGUAGE/COLLATION.
  • Unit tests for the D3 fatal-error path (severity ≥ 20 marks dead; severity < 20 does not).
  • Live test in tests/test_reset_connection.rs proving a prepared handle is invalidated across a reset (gated on a live SQL Server via .env, like the sibling reset tests).

Validation

  • cargo bfmt
  • cargo bclippy (warnings-as-errors) ✅
  • cargo nextest — new/related tests pass. Pre-existing certificate/TLS-validation test failures are unrelated to this change (they fail on the base branch too, expired cert fixtures).

Notes

Route every ResetConnection ENVCHANGE through a single
on_reset_connection_ack that resets session state, clears session-bound
prepared caches (shared with the reconnect path), and restores the
negotiated database/language/collation to their login values.

Add TdsTransport::mark_known_dead and flag the transport dead when a
server error token has severity/class >= 20, so a pool checkout discards
a fatally errored connection even on a still-open socket.

Add unit tests for the reset transition and the fatal-error path, plus a
live test proving a prepared handle is invalidated across a reset.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 63e24621-7665-4572-a5b8-e5dfa55060a0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant