Add ODBC transaction support

[Saurabh Singh (saurabh500)]

Description

Implements ODBC transaction support (spec §4.9) in mssql-odbc: manual-commit
by default, SQLEndTran commit/rollback, isolation levels including SQL Server
SNAPSHOT, implicit rollback on close, and the four transaction SQLGetInfo
capabilities.

The design, the full goal/non-goal list and the divergences from msodbcsql are
in mssql-odbc/docs/transactions_plan.md,
added by this PR.

Goals implemented

#	Goal
G1	Manual-commit by default (SQL_AUTOCOMMIT_OFF), with the implicit transaction opened lazily on first execute
G2	SQLEndTran(SQL_HANDLE_DBC, …) commit and rollback, plus the SQL_HANDLE_ENV fan-out
G3	SQL_ATTR_TXN_ISOLATION for the four standard levels, and SQL_COPT_SS_TXN_ISOLATION for SQL_TXN_SS_SNAPSHOT
G4	SQLDisconnect rolls back an open transaction instead of stranding it
G5	SQL_TXN_CAPABLE, SQL_DEFAULT_TXN_ISOLATION, SQL_CURSOR_COMMIT_BEHAVIOR, SQL_MULTIPLE_ACTIVE_TXN

SQL_ATTR_AUTOCOMMIT is settable before connect and applied at connect time, and
SQLGetConnectAttr answers from cached state rather than issuing a live
SQLGetConnectAttr on every read. This was caught by the e2e
suite: the round trip is observable and msodbcsql does not make it.

Non-goals

Savepoints, explicit BEGIN TRANSACTION, DTC/XA enlistment, MARS-scoped
transactions, SQL_ATTR_ENLIST_IN_DTC, cursor-preserving commit behaviour and
the remaining msodbcsql-only capabilities are listed as N1–N12 in the plan, each
with a note on why it is out of scope for this PR. The skipped items are filed as
child work items under AB#46379 (47263–47269).

Related Issues

AB#46379

Testing

535 Rust unit tests pass (cargo nextest run -p mssql-odbc), covering
argument validation, state transitions and diagnostics without a server.

37 C++ e2e tests in tests/e2e/tests/transaction_test.cpp, run against a
live SQL Server. The full 18-binary suite was run twice locally — once against
the Rust driver and once against ODBC Driver 18 for SQL Server — and passes
18/18 on both, with only the 2 intentionally-guarded divergences skipped on
the reference leg.

Two behaviours turned out to be owned by the Driver Manager, not by either
driver, and the tests assert the observable result instead (§7.1 of the plan):

• The DM screens SQL_ATTR_TXN_ISOLATION and answers HY024 for anything
  outside the four standard levels — including SQL_TXN_SS_SNAPSHOT, and
  including on msodbcsql. The driver's own HYC00 is asserted by unit tests,
  which have no DM in the path.
• msodbcsql tells the DM SQL_CB_PRESERVE, so after a commit the DM rejects the
  next statement with 24000 until the cursor is closed explicitly. mssql-odbc
  answers truthfully, so the statement is immediately reusable. The shared part
  is asserted on both drivers; the divergence lives in its own
  SKIP_IF_COMPARING_MSODBCSQL() test.

Review round 1 (Copilot)

Fix
SQLEndTran no longer reports success when the connection is not connected
Isolation level is applied to the session even when set before connect
SQL_ATTR_AUTOCOMMIT set before connect is honoured at connect time
Rollback failure during SQLDisconnect no longer strands the connection
SQLGetInfo transaction capabilities moved out of the catch-all default arm
Stale local_tran_started after a server-side abort no longer wedges the connection
Isolation-level round trip removed from SQLGetConnectAttr
Duplicated isolation-mapping table folded into one helper
Test names and comments corrected

Review round 2 (10 comments)

Fix
SQLEndTran sweeps open cursors before committing, so SQL_CB_CLOSE is honoured
SQL_COPT_SS_TXN_ISOLATION accepts and reports SQL_TXN_SS_SNAPSHOT
Setting the isolation level already in effect is a no-op, matching SetCommitModeOption
SQLEndTran on SQL_HANDLE_ENV surfaces a failing connection's error instead of swallowing it
Isolation level is reset on pool check-in
SQLGetInfo string capabilities return the documented types
Autocommit switch-on commits open work and reports it with 01000
Plan documents the DM-owned behaviours and the two divergences
Test matrix extended to 31 rows
Comments corrected where they described the DM's behaviour as the driver's

36 e2e tests now (4 added), 533 unit tests, ctest 18/18 on both
drivers.

Statement diagnostics: divergence removed, now at parity

An earlier revision deliberately preserved statement diagnostics across
SQLEndTran, on the reading that ODBC clears diagnostics only on the handle the
function was called on. msodbcsql does not behave that way, and the divergence
has been removed.

CommitAbortTran sweeps the connection's statements through
SQLFreeStmt(SQL_CLOSE) (sqlctran.cpp:302-323), and that entry point calls
FreeErrors(lpstmt) before it inspects either the option or the cursor state
(sqlccmd.cpp:379-380). Every child statement therefore loses its records,
including one that failed and never opened a cursor.

close_all_cursors now clears statement diagnostics unconditionally, ahead of
its cursor-open check, so the e2e assertion runs against both drivers with no
SKIP_IF_COMPARING_MSODBCSQL(), and the unit test is renamed to
closing_cursors_clears_statement_diagnostics.

Validated: 533/533 unit tests, e2e 36/36 on mssql-odbc and 34 passed + 2
skipped on msodbcsql.

Review round 3 (9 comments, David Engel (@David-Engel))

Fix
apply_post_connect_txn_settings frees stale errors when the claim fails, so a successful connect cannot leave a stray record
SQL_ATTR_TXN_ISOLATION short-circuits an unchanged value before the HY011 open-transaction rejection, matching SQL_ATTR_AUTOCOMMIT
set_txn_isolation uses the validated level instead of an unwrap_or fallback
…and instead of re-casting the raw value
SQLEndTran env fan-out routes a poisoned connection mutex to SQL_ERROR rather than skipping it as "not connected"
…and posts a summary HY000 on the ENV handle, so SQLGetDiagRec(SQL_HANDLE_ENV, …) is not SQL_NO_DATA after SQL_ERROR
ensure_transaction pass-through wrapper removed; call sites use begin_transaction_if_manual directly
Two doc comments corrected that described behaviour the code does not have

Review round 4 (4 comments, David Engel (@David-Engel))

The blocking comment was correct and caught a test that guarded nothing:
EndTranWithNoTransactionStartedStillClosesCursors switched to manual commit
first, but switch_to_manual_commit calls close_all_cursors as its own first
statement (txn.rs:432), so the cursor was already closed before SQLEndTran
ran. It passed with or without the sweep reordering.

The suggested replacement — the same test in plain autocommit mode — does not
work either, which took a live run to establish. It fails with
[HY000] Connection is busy with results for another command, raised by this
driver on the following statement, meaning the sweep never ran: while autocommit
is on the Driver Manager answers SQLEndTran itself and never calls the driver.

So the no-transaction-started-with-an-open-cursor combination is not reachable
through the public ODBC surface at all. In manual commit, all three callers of
begin_transaction_if_manual (SQLExecute, SQLExecDirect, SQLGetTypeInfo)
start a transaction before opening a cursor, so local_tran_started is never
false with a row stream open; in autocommit the DM swallows the call. That path
stays guarded by
txn::tests::end_tran_sweeps_cursors_even_with_no_transaction_started, which
calls end_transaction directly.

The e2e test is retargeted at the reachable half of the same contract — a
successful SQLEndTran leaves the connection free for other statements — as
EndTranClosesCursorsAndFreesTheConnection. It does not guard the reordering
(the pre-fix code swept on the started == true path too); it guards the sweep
contract, and it now runs on both drivers, removing one
SKIP_IF_COMPARING_MSODBCSQL().

Fix
e2e sweep test retargeted at a reachable assertion and un-skipped on msodbcsql
Connect-path diagnostics truncated to their pre-claim length instead of cleared, so a SQL_SUCCESS_WITH_INFO login keeps the server's INFO records
Two stale comment paragraphs removed that described the old, incorrect justification

Validated: 535/535 unit tests, e2e 37/37 on mssql-odbc and 35 passed + 2
skipped on msodbcsql.

Checklist

• cargo bfmt passes
• cargo bclippy passes
• cargo btest passes — mssql-odbc is green (535/535); the mssql-tds
  integration tests fail on this machine for environment reasons only (no
  .env, TLS SEC_E_WRONG_PRINCIPAL). This PR touches no file outside
  mssql-odbc, so CI is the real signal here.
• New/changed functionality has tests
• Public API changes are documented

[Copilot]

Pull request overview

Adds ODBC transaction support to mssql-odbc, including transaction control, connection attributes, capability reporting, and lifecycle integration.

Changes:

• Implements SQLEndTran, autocommit, and isolation-level handling.
• Integrates transactions with execution, connection, cursor, and disconnect paths.
• Adds unit/E2E coverage and design documentation.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
mssql-odbc/src/api/disconnect.rs	Adds transaction-safe disconnect behavior.
mssql-odbc/src/api/driver_connect.rs	Applies deferred transaction settings.
mssql-odbc/src/api/end_tran.rs	Implements SQLEndTran.
mssql-odbc/src/api/exec_common.rs	Adds shared transaction initialization.
mssql-odbc/src/api/exec_direct.rs	Starts transactions before direct execution.
mssql-odbc/src/api/execute.rs	Starts transactions before prepared execution.
mssql-odbc/src/api/exports.rs	Exports SQLEndTran.
mssql-odbc/src/api/get_connect_attr.rs	Returns transaction attributes.
mssql-odbc/src/api/get_functions.rs	Advertises transaction APIs.
mssql-odbc/src/api/get_info.rs	Reports transaction capabilities.
mssql-odbc/src/api/get_type_info.rs	Integrates catalog execution with transactions.
mssql-odbc/src/api/mod.rs	Registers transaction modules.
mssql-odbc/src/api/odbc_types.rs	Defines transaction constants.
mssql-odbc/src/api/set_connect_attr.rs	Handles transaction attributes.
mssql-odbc/src/api/sqlstate.rs	Adds transaction diagnostics.
mssql-odbc/src/api/txn.rs	Provides shared transaction state and operations.
mssql-odbc/src/handles/dbc.rs	Stores per-connection transaction state.
mssql-odbc/docs/transactions_plan.md	Documents design, scope, and tests.
mssql-odbc/tests/e2e/CMakeLists.txt	Registers transaction E2E tests.
mssql-odbc/tests/e2e/tests/transaction_test.cpp	Exercises transaction behavior against SQL Server.

Suppressed comments (2)

mssql-odbc/docs/transactions_plan.md:264

• The test matrix says invalid handle type produces HY092, but end_tran_bad_handle_type_returns_invalid_handle asserts SQL_INVALID_HANDLE. Keep this entry consistent with the implemented API contract.

| 10 | `SQLEndTran` invalid handle type → `HY092` | unit |

mssql-odbc/src/api/txn.rs:223

• If this lock is poisoned after the transaction begins, the function still returns success without setting local_tran_started. The statement then executes, but SQLEndTran treats it as having no user transaction and skips commit/rollback. Fail the execution when the state cannot be recorded.

    if let Ok(mut state) = dbc.inner.lock() {
        state.local_tran_started = true;
    }
    Ok(())

---

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

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 83%

🎯 Overall Coverage 91.2%

📦 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/close_cursor.rs (71.4%): Missing lines 148-149,160-161
• mssql-odbc/src/api/disconnect.rs (86.7%): Missing lines 49-50
• mssql-odbc/src/api/driver_connect.rs (80.0%): Missing lines 195
• mssql-odbc/src/api/end_tran.rs (95.0%): Missing lines 86-87,107-108,145-148,167
• mssql-odbc/src/api/exec_direct.rs (66.7%): Missing lines 131
• mssql-odbc/src/api/execute.rs (100%)
• mssql-odbc/src/api/exports.rs (100%)
• mssql-odbc/src/api/get_connect_attr.rs (100%)
• mssql-odbc/src/api/get_functions.rs (100%)
• mssql-odbc/src/api/get_info.rs (100%)
• mssql-odbc/src/api/get_type_info.rs (66.7%): Missing lines 188
• mssql-odbc/src/api/set_connect_attr.rs (100%)
• mssql-odbc/src/api/txn.rs (73.6%): Missing lines 53-56,58-59,65-66,78-79,82-84,87-89,138-139,152-153,168-169,194,220-221,227-228,235,241-242,247,277-280,302-305,328,334-335,368-369,376,379-380,388,392,404-405,408-410,432,436,439,447-448,451-453,467-468,507,511,523-525,533-535,540-541,544-546,565-566,572-573,577,580-582,584,608-609,625-628,638-642,651-654,662-664,666-667
• mssql-odbc/src/handles/dbc.rs (50.0%): Missing lines 111-113

Summary

• Total: 782 lines
• Missing: 127 lines
• Coverage: 83%

mssql-odbc/src/api/close_cursor.rs

  144 /// with no FFI entry and no drain.
  145 pub(super) fn close_cursor_for_connection_op(stmt: &StmtHandle, handle: SqlHandle) -> SqlReturn {
  146     {
  147         let Ok(mut stmt_state) = stmt.inner.lock() else {
! 148             error!("close_cursor_for_connection_op: stmt mutex poisoned");
! 149             return SQL_ERROR;
  150         };
  151         free_errors(&mut stmt_state);
  152         if !stmt_state.has_state(STMT_STATE_CURSOR_OPEN) {
  153             return SQL_SUCCESS;

---

  156     }
  157 
  158     match drain_and_release(stmt, handle) {
  159         DrainOutcome::Failed => {
! 160             error!("close_cursor_for_connection_op: failed to drain TDS stream");
! 161             SQL_ERROR
  162         }
  163         DrainOutcome::InfoPosted | DrainOutcome::Clean => SQL_SUCCESS,
  164     }
  165 }

---

mssql-odbc/src/api/disconnect.rs

  45     // Validate under a short lock; the rollback that may follow needs network
  46     // I/O and must not run while the mutex is held.
  47     {
  48         let Ok(mut state) = dbc.inner.lock() else {
! 49             error!("SQLDisconnect: dbc mutex poisoned");
! 50             return SQL_ERROR;
  51         };
  52         free_errors(&mut state);
  53 
  54         if state.connection_state != ConnectionState::Connected {

---

mssql-odbc/src/api/driver_connect.rs

  191     // usable, but the application must be able to see that its requested
  192     // settings did not take effect.
  193     drop(state);
  194     if apply_post_connect_txn_settings(dbc) == SQL_SUCCESS_WITH_INFO {
! 195         return SQL_SUCCESS_WITH_INFO;
  196     }
  197 
  198     result
  199 }

---

mssql-odbc/src/api/end_tran.rs

  82 fn sql_end_tran_dbc_safe(dbc: &DbcHandle, completion_type: SqlSmallInt) -> SqlReturn {
  83     if let Ok(mut state) = dbc.inner.lock() {
  84         free_errors(&mut state);
  85     } else {
! 86         error!("SQLEndTran: dbc mutex poisoned");
! 87         return SQL_ERROR;
  88     }
  89 
  90     let Some(commit) = commit_flag(completion_type) else {
  91         error!(completion_type, "SQLEndTran: invalid completion type");

---

  103 /// # Safety
  104 /// Every pointer in `EnvState::connections` must be a live `DbcHandle`.
  105 unsafe fn sql_end_tran_env_safe(env: &EnvHandle, completion_type: SqlSmallInt) -> SqlReturn {
  106     let Ok(mut env_state) = env.inner.lock() else {
! 107         error!("SQLEndTran: env mutex poisoned");
! 108         return SQL_ERROR;
  109     };
  110     free_errors(&mut env_state);
  111 
  112     if commit_flag(completion_type).is_none() {

---

  141         // fan-out.
  142         let connected = match dbc.inner.lock() {
  143             Ok(state) => state.connection_state == ConnectionState::Connected,
  144             Err(_) => {
! 145                 error!(?dbc_ptr, "SQLEndTran: dbc mutex poisoned");
! 146                 worst = SQL_ERROR;
! 147                 failed += 1;
! 148                 continue;
  149             }
  150         };
  151         if !connected {
  152             debug!(

---

  163         if ret == SQL_ERROR {
  164             worst = SQL_ERROR;
  165             failed += 1;
  166         } else if ret != SQL_SUCCESS && worst == SQL_SUCCESS {
! 167             worst = ret;
  168         }
  169     }
  170 
  171     // The detail for each failure is on the connection that produced it, but an

---

mssql-odbc/src/api/exec_direct.rs

  127     // Release any handle orphaned by the reset above before running the batch.
  128     flush_pending_unprepare(dbc, stmt, &mut client, "SQLExecDirectW");
  129 
  130     if let Err(e) = begin_transaction_if_manual(dbc, &mut client, "SQLExecDirectW") {
! 131         return fail_with_tds(dbc, stmt, statement_handle, client, &e);
  132     }
  133 
  134     // Parameterized text runs via sp_executesql (direct execution, no cached
  135     // handle); unparameterized text runs as a plain SQL batch. Neither DBC nor

---

mssql-odbc/src/api/get_type_info.rs

  184     // Release any handle orphaned by the reset above before running the RPC.
  185     flush_pending_unprepare(dbc, stmt, &mut client, "SQLGetTypeInfoW");
  186 
  187     if let Err(e) = begin_transaction_if_manual(dbc, &mut client, "SQLGetTypeInfoW") {
! 188         return fail_with_tds(dbc, stmt, statement_handle, client, &e);
  189     }
  190 
  191     let exec_result = dbc.runtime.block_on(client.execute_stored_procedure(
  192         DATATYPE_INFO_PROC.to_string(),

---

mssql-odbc/src/api/txn.rs

  49 /// Reports a cursor that could not be closed ahead of a connection-scoped
  50 /// transaction operation. The per-statement diagnostic already went to its own
  51 /// STMT handle, which the application is not looking at here, so restate it on
  52 /// the DBC.
! 53 fn fail_cursor_close(dbc: &DbcHandle, op: &str) -> SqlReturn {
! 54     error!("{op}: could not close open cursors");
! 55     let Ok(mut state) = dbc.inner.lock() else {
! 56         return SQL_ERROR;
  57     };
! 58     post_sql_error(
! 59         &mut state,
  60         SQLSTATE_HY000,
  61         0,
  62         "An open cursor on this connection could not be closed, so the \
  63          transaction request could not be sent.",

---

  61         0,
  62         "An open cursor on this connection could not be closed, so the \
  63          transaction request could not be sent.",
  64     );
! 65     SQL_ERROR
! 66 }
  67 
  68 /// Claims the connection's TDS client for a connection-scoped operation that no
  69 /// statement owns (commit, rollback, isolation change, autocommit change).
  70 ///

---

  74 /// is genuinely idle. Returns `Err` with the diagnostic already posted to the
  75 /// DBC.
  76 pub(super) fn claim_dbc_client(dbc: &DbcHandle, op: &str) -> Result<TdsClient, SqlReturn> {
  77     let Ok(mut state) = dbc.inner.lock() else {
! 78         error!("{op}: dbc mutex poisoned");
! 79         return Err(SQL_ERROR);
  80     };
  81     if state.connection_state != ConnectionState::Connected {
! 82         error!("{op}: DBC is not connected");
! 83         post_diag(&mut state, ERR_CONNECTION_DOES_NOT_EXIST);
! 84         return Err(SQL_ERROR);
  85     }
  86     if state.active_stmt.is_some() {
! 87         error!("{op}: connection is busy with results for another statement");
! 88         post_diag(&mut state, ERR_CONNECTION_BUSY);
! 89         return Err(SQL_ERROR);
  90     }
  91     let Some(client) = state.client.take() else {
  92         error!("{op}: no active TDS client");
  93         post_diag(&mut state, ERR_NO_ACTIVE_TDS_CLIENT);

---

  134 pub(super) fn close_all_cursors(dbc: &DbcHandle) -> SqlReturn {
  135     let statements = match dbc.inner.lock() {
  136         Ok(state) => state.statements.clone(),
  137         Err(_) => {
! 138             error!("close_all_cursors: dbc mutex poisoned");
! 139             return SQL_ERROR;
  140         }
  141     };
  142     let mut worst = SQL_SUCCESS;
  143     for stmt_ptr in statements {

---

  148         // `SQLDisconnect` documents (see the TODO in `disconnect.rs`), which
  149         // refcounted handles will close for the whole driver at once.
  150         let stmt = unsafe { handle_from_raw::<StmtHandle>(stmt_ptr) };
  151         if close_cursor_for_connection_op(stmt, stmt_ptr) == SQL_ERROR {
! 152             error!(?stmt_ptr, "close_all_cursors: could not close cursor");
! 153             worst = SQL_ERROR;
  154         }
  155     }
  156     worst
  157 }

---

  164 /// a **silent success**, never a warning or error.
  165 pub(super) fn end_transaction(dbc: &DbcHandle, commit: bool, op: &str) -> SqlReturn {
  166     let started = {
  167         let Ok(mut state) = dbc.inner.lock() else {
! 168             error!("{op}: dbc mutex poisoned");
! 169             return SQL_ERROR;
  170         };
  171         if state.connection_state != ConnectionState::Connected {
  172             error!("{op}: DBC is not connected");
  173             post_diag(&mut state, ERR_CONNECTION_DOES_NOT_EXIST);

---

  190     // `SQL_CB_PRESERVE`, so the DM never closes cursors on its behalf. Both
  191     // drivers are internally consistent; they just report different truths about
  192     // themselves.
  193     if close_all_cursors(dbc) == SQL_ERROR {
! 194         return fail_cursor_close(dbc, op);
  195     }
  196 
  197     // msodbcsql `sqlctran.cpp:293`: nothing started, so no TM request.
  198     if !started {

---

  216                 client.rollback_transaction(None, None).await
  217             }
  218         })
  219     } else {
! 220         debug!("{op}: no server-side transaction active — clearing stale flag");
! 221         Ok(())
  222     };
  223 
  224     release_dbc_client(dbc, client);

---

  223 
  224     release_dbc_client(dbc, client);
  225 
  226     let Ok(mut state) = dbc.inner.lock() else {
! 227         error!("{op}: dbc mutex poisoned");
! 228         return SQL_ERROR;
  229     };
  230     // Cleared unconditionally, matching msodbcsql's `Return:` label, so a failed
  231     // commit cannot strand the connection permanently un-disconnectable.
  232     state.local_tran_started = false;

---

  231     // commit cannot strand the connection permanently un-disconnectable.
  232     state.local_tran_started = false;
  233 
  234     if let Err(e) = result {
! 235         error!(%e, "{op}: transaction-manager request failed");
  236         // 08007 is ODBC's specific state for "connection failure during
  237         // transaction": the commit or rollback did not reach the server, so its
  238         // outcome is unknown to the application. Neither 08007 nor HY000 puts
  239         // the connection into the suspended state, so this is a strictly more

---

  237         // transaction": the commit or rollback did not reach the server, so its
  238         // outcome is unknown to the application. Neither 08007 nor HY000 puts
  239         // the connection into the suspended state, so this is a strictly more
  240         // precise diagnostic, not a behavioural change.
! 241         post_tds_error(&mut state, &e, SQLSTATE_08007);
! 242         return SQL_ERROR;
  243     }
  244 
  245     debug!(
  246         "{op}: transaction {} complete",
! 247         if commit { "commit" } else { "rollback" }
  248     );
  249     SQL_SUCCESS
  250 }

---

  273 ) -> Result<(), mssql_tds::error::Error> {
  274     let (autocommit, already_recorded) = match dbc.inner.lock() {
  275         Ok(state) => (state.autocommit, state.local_tran_started),
  276         Err(_) => {
! 277             error!("{op}: dbc mutex poisoned reading autocommit");
! 278             return Err(mssql_tds::error::Error::ImplementationError(
! 279                 "connection state is poisoned".to_string(),
! 280             ));
  281         }
  282     };
  283     if autocommit {
  284         return Ok(());

---

  298 
  299     // The transaction is open on the server at this point; failing to record it
  300     // would leak it past commit/rollback and past disconnect.
  301     let Ok(mut state) = dbc.inner.lock() else {
! 302         error!("{op}: dbc mutex poisoned recording transaction state");
! 303         return Err(mssql_tds::error::Error::ImplementationError(
! 304             "connection state is poisoned".to_string(),
! 305         ));
  306     };
  307     state.local_tran_started = true;
  308     Ok(())
  309 }

---

  324         if let Ok(mut state) = dbc.inner.lock() {
  325             free_errors(&mut state);
  326             error!(value, "{OP}: invalid value");
  327             post_diag(&mut state, ERR_INVALID_ATTRIBUTE_VALUE);
! 328         }
  329         return SQL_ERROR;
  330     };
  331 
  332     let connected = {

---

  330     };
  331 
  332     let connected = {
  333         let Ok(mut state) = dbc.inner.lock() else {
! 334             error!("{OP}: dbc mutex poisoned");
! 335             return SQL_ERROR;
  336         };
  337         free_errors(&mut state);
  338         // msodbcsql `sqlcmisc.cpp:1720`: re-setting the current mode is free.
  339         if state.autocommit == enable {

---

  364 fn switch_to_autocommit(dbc: &DbcHandle, op: &str) -> SqlReturn {
  365     let had_user_txn = match dbc.inner.lock() {
  366         Ok(state) => state.local_tran_started,
  367         Err(_) => {
! 368             error!("{op}: dbc mutex poisoned");
! 369             return SQL_ERROR;
  370         }
  371     };
  372 
  373     if had_user_txn {

---

  372 
  373     if had_user_txn {
  374         let ret = end_transaction(dbc, true, op);
  375         if ret != SQL_SUCCESS {
! 376             return ret;
  377         }
  378         let Ok(mut state) = dbc.inner.lock() else {
! 379             error!("{op}: dbc mutex poisoned");
! 380             return SQL_ERROR;
  381         };
  382         state.autocommit = true;
  383         post_diag(&mut state, WARN_TRANSACTION_COMMITTED);
  384         return SQL_SUCCESS_WITH_INFO;

---

  384         return SQL_SUCCESS_WITH_INFO;
  385     }
  386 
  387     if close_all_cursors(dbc) == SQL_ERROR {
! 388         return fail_cursor_close(dbc, op);
  389     }
  390     let mut client = match claim_dbc_client(dbc, op) {
  391         Ok(c) => c,
! 392         Err(ret) => return ret,
  393     };
  394     let result = if client.has_active_transaction() {
  395         debug!("{op}: rolling back driver-begun transaction");
  396         dbc.runtime

---

  400     };
  401     release_dbc_client(dbc, client);
  402 
  403     let Ok(mut state) = dbc.inner.lock() else {
! 404         error!("{op}: dbc mutex poisoned");
! 405         return SQL_ERROR;
  406     };
  407     if let Err(e) = result {
! 408         error!(%e, "{op}: rollback of driver-begun transaction failed");
! 409         post_tds_error(&mut state, &e, SQLSTATE_HY000);
! 410         return SQL_ERROR;
  411     }
  412     state.autocommit = true;
  413     state.local_tran_started = false;
  414     SQL_SUCCESS

---

  428 /// in place, since msodbcsql issues no transaction-manager request when nothing
  429 /// was started.
  430 fn switch_to_manual_commit(dbc: &DbcHandle, op: &str) -> SqlReturn {
  431     if close_all_cursors(dbc) == SQL_ERROR {
! 432         return fail_cursor_close(dbc, op);
  433     }
  434     let mut client = match claim_dbc_client(dbc, op) {
  435         Ok(c) => c,
! 436         Err(ret) => return ret,
  437     };
  438     let result = if client.has_active_transaction() {
! 439         Ok(())
  440     } else {
  441         dbc.runtime
  442             .block_on(client.begin_transaction(TransactionIsolationLevel::NoChange, None))
  443     };

---

  443     };
  444     release_dbc_client(dbc, client);
  445 
  446     let Ok(mut state) = dbc.inner.lock() else {
! 447         error!("{op}: dbc mutex poisoned");
! 448         return SQL_ERROR;
  449     };
  450     if let Err(e) = result {
! 451         error!(%e, "{op}: could not begin transaction");
! 452         post_tds_error(&mut state, &e, SQLSTATE_HY000);
! 453         return SQL_ERROR;
  454     }
  455     state.autocommit = false;
  456     state.local_tran_started = false;
  457     SQL_SUCCESS

---

  463     const OP: &str = "SQLSetConnectAttrW(SQL_ATTR_TXN_ISOLATION)";
  464 
  465     let (level, tsql) = {
  466         let Ok(mut state) = dbc.inner.lock() else {
! 467             error!("{OP}: dbc mutex poisoned");
! 468             return SQL_ERROR;
  469         };
  470         free_errors(&mut state);
  471 
  472         let Some((level, tsql)) = u32::try_from(value)

---

  503         (level, tsql)
  504     };
  505 
  506     if close_all_cursors(dbc) == SQL_ERROR {
! 507         return fail_cursor_close(dbc, OP);
  508     }
  509     let mut client = match claim_dbc_client(dbc, OP) {
  510         Ok(c) => c,
! 511         Err(ret) => return ret,
  512     };
  513 
  514     // `local_tran_started` was false above, so any transaction open here was
  515     // begun by the driver at the autocommit switch and carries no user work.

---

  519     // the autocommit switch establishes. Rolling back loses nothing because
  520     // there is nothing in it.
  521     let reopen = client.has_active_transaction();
  522     let mut result = if reopen {
! 523         debug!("{OP}: rolling back empty driver-begun transaction to apply isolation");
! 524         dbc.runtime
! 525             .block_on(client.rollback_transaction(None, None))
  526     } else {
  527         Ok(())
  528     };
  529     if result.is_ok() {

---

  529     if result.is_ok() {
  530         result = exec_batch(dbc, &mut client, tsql);
  531     }
  532     if result.is_ok() && reopen {
! 533         result = dbc
! 534             .runtime
! 535             .block_on(client.begin_transaction(TransactionIsolationLevel::NoChange, None));
  536     }
  537     release_dbc_client(dbc, client);
  538 
  539     let Ok(mut state) = dbc.inner.lock() else {
! 540         error!("{OP}: dbc mutex poisoned");
! 541         return SQL_ERROR;
  542     };
  543     if let Err(e) = result {
! 544         error!(%e, "{OP}: could not apply isolation level");
! 545         post_tds_error(&mut state, &e, SQLSTATE_HY000);
! 546         return SQL_ERROR;
  547     }
  548     // `value` was validated into `level` above.
  549     state.txn_isolation = level;
  550     debug!(tsql, "{OP}: isolation level applied");

---

  561     // A cursor that will not close leaves the connection mid-batch, so the
  562     // rollback below cannot be sent. Disconnecting anyway is still correct:
  563     // the server rolls the transaction back when the socket closes.
  564     if close_all_cursors(dbc) == SQL_ERROR {
! 565         error!("{OP}: could not close all cursors; the server will roll back on disconnect");
! 566         return;
  567     }
  568 
  569     let client = match dbc.inner.lock() {
  570         Ok(mut state) => state.client.take(),

---

  568 
  569     let client = match dbc.inner.lock() {
  570         Ok(mut state) => state.client.take(),
  571         Err(_) => {
! 572             error!("{OP}: dbc mutex poisoned");
! 573             return;
  574         }
  575     };
  576     let Some(mut client) = client else {
! 577         return;
  578     };
  579     if client.has_active_transaction()
! 580         && let Err(e) = dbc
! 581             .runtime
! 582             .block_on(client.rollback_transaction(None, None))
  583     {
! 584         error!(%e, "{OP}: rollback failed; the server will roll back on disconnect");
  585     }
  586     release_dbc_client(dbc, client);
  587 }

---

  604             state.txn_isolation,
  605             state.diag_records().len(),
  606         ),
  607         Err(_) => {
! 608             error!("{OP}: dbc mutex poisoned");
! 609             return SQL_SUCCESS;
  610         }
  611     };
  612     if autocommit && isolation == SQL_TXN_READ_COMMITTED {
  613         return SQL_SUCCESS;

---

  621             // succeeded, so returning SQL_SUCCESS with that record still on the
  622             // handle would show an application a connect failure that never
  623             // happened. Truncating rather than clearing keeps the server's
  624             // login INFO messages, which a SQL_SUCCESS_WITH_INFO result needs.
! 625             if let Ok(mut state) = dbc.inner.lock() {
! 626                 state.diag_records_mut().truncate(diag_len);
! 627             }
! 628             return SQL_SUCCESS;
  629         }
  630     };
  631 
  632     let mut failure: Option<String> = None;

---

  634     if isolation != SQL_TXN_READ_COMMITTED
  635         && let Some(tsql) = txn_isolation_to_tsql(isolation)
  636         && let Err(e) = exec_batch(dbc, &mut client, tsql)
  637     {
! 638         error!(%e, "{OP}: could not apply pre-connect isolation level");
! 639         failure = Some(format!(
! 640             "The transaction isolation level set before connecting could not be \
! 641              applied to the session: {e}"
! 642         ));
  643     }
  644 
  645     if !autocommit
  646         && !client.has_active_transaction()

---

  647         && let Err(e) = dbc
  648             .runtime
  649             .block_on(client.begin_transaction(TransactionIsolationLevel::NoChange, None))
  650     {
! 651         error!(%e, "{OP}: could not begin transaction for manual-commit mode");
! 652         failure.get_or_insert_with(|| {
! 653             format!("The manual-commit transaction could not be started: {e}")
! 654         });
  655     }
  656 
  657     release_dbc_client(dbc, client);

---

  658 
  659     let Some(message) = failure else {
  660         return SQL_SUCCESS;
  661     };
! 662     let Ok(mut state) = dbc.inner.lock() else {
! 663         error!("{OP}: dbc mutex poisoned");
! 664         return SQL_SUCCESS;
  665     };
! 666     post_sql_error(&mut state, SQLSTATE_01000, 0, &message);
! 667     SQL_SUCCESS_WITH_INFO
  668 }
  669 
  670 #[cfg(test)]
  671 mod tests {

---

mssql-odbc/src/handles/dbc.rs

  107                 "access_token",
  108                 &self.access_token.as_ref().map(|_| "<REDACTED>"),
  109             )
  110             .field("login_timeout", &self.login_timeout)
! 111             .field("autocommit", &self.autocommit)
! 112             .field("txn_isolation", &self.txn_isolation)
! 113             .field("local_tran_started", &self.local_tran_started)
  114             .finish()
  115     }
  116 }

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[Saurabh Singh (saurabh500)]

Self-review pass focused on ODBC spec conformance and hot-path cost. Overall the design is sound — the lock discipline is genuinely careful (client is take()n out of the mutex rather than held across I/O, locks consistently released before block_on), and the local_tran_started vs TdsClient::has_active_transaction() split is the right call: it's what makes recovery from a server-side rollback work.

Two blocking issues, both verified against the spec and the code:

1. SQLEndTran(SQL_HANDLE_ENV) fails when any DBC on the environment is not connected. The spec says inactive connections must not affect the transaction. Currently one spare or already-disconnected DBC fails an environment-wide commit — and a unit test asserts the wrong behavior.
2. Connection-scoped transaction ops wipe the diagnostics of every statement on the connection. close_all_cursors routes through SQLFreeStmt(SQL_CLOSE), which calls free_errors before its no-cursor early return. SQLEndTran(SQL_ROLLBACK) after a failed statement erases the error the app was about to report.

Plus a return-code ranking bug that's latent but one line from being live, a capability we advertise but no app can reach (SQL_TXN_SS_SNAPSHOT), and several hot-path costs — the biggest being that close_all_cursors is O(N) full FFI entries per commit when the common case is zero open cursors.

Things I checked that are correct and easy to get wrong, so nobody re-litigates them:

• No lock held across network I/O anywhere in txn.rs; no self-deadlock in the cursor sweep (statements is cloned in a let, so the guard drops before re-locking).
• Poison handling is consistent — let Ok(..) = .. else throughout, no into_inner() recovery, no unwrap() outside tests.
• Auto-begin recovery is real: has_active_transaction() is driven by ENVCHANGE Begin/Commit/Rollback/Defect tokens, so XACT_ABORT and raw T-SQL ROLLBACK genuinely clear it.
• Every SQL-executing entry point is covered by auto-begin (SQLExecDirect, SQLExecute, SQLGetTypeInfo); SQLPrepare only stages, and free_handle's best-effort sp_unprepare correctly must not open one.
• Suspended-state interaction is right — local_tran_started is cleared unconditionally so the mandatory follow-up SQLDisconnect isn't blocked by its own 25000, and 1205/1211 map to 40001, which the DM treats as confirmed non-completion.
• SQL_CB_CLOSE semantics hold (prepared handle survives via capture_prepared_handle), HY012 / SQL_INVALID_HANDLE validation matches the spec table, no-active-transaction returns SQL_SUCCESS, and all the new SQLGetInfo constants and return widths are correct.
• HYC00 for 0x10 is right — that's SQL_TXN_VERSIONING, a real ODBC value we don't implement, so HYC00 beats HY024.

Also worth flagging in the PR body before this leaves draft: SQLDisconnect now returns 25000 with an open transaction, and since auto-begin runs before every statement in manual-commit mode (including read-only SELECTs and SQLGetTypeInfo), a manual-commit app that only ever read data must now call SQLEndTran before disconnecting. Matches msodbcsql, but it's user-visible.

[David Engel (David-Engel)]

Summary

Solid, well-researched implementation of ODBC transactions in mssql-odbc — SQLEndTran, SQL_ATTR_AUTOCOMMIT, SQL_ATTR_TXN_ISOLATION / SQL_COPT_SS_TXN_ISOLATION, the SQLGetInfo transaction types, and the SQLGetFunctions entries. The msodbcsql source citations throughout, the plan doc, and the 36-test C++ e2e suite that runs against both drivers are exactly the right way to build this. I verified locally: cargo clippy -p mssql-odbc --all-targets is clean and 514 lib unit tests pass.

One correctness issue I'd want resolved before merge (cursor sweep skipped in autocommit mode), plus a few smaller items. Details inline.

Two repo-level items that aren't attached to a diff line

Diff coverage. The repo targets 85% diff coverage in CI (.github/copilot-instructions.md), and the C++ e2e suite doesn't feed cargo-llvm-cov. Measured on this branch:

mssql-odbc/src/api/end_tran.rs      150 lines,   5 missed,  96.67%
mssql-odbc/src/api/txn.rs           380 lines, 254 missed,  33.16%

end_tran.rs is in great shape. txn.rs is the risk: everything that needs a TdsClient (end_transaction's network path, switch_to_autocommit / switch_to_manual_commit, the connected branch of set_txn_isolation, apply_post_connect_txn_settings) is only exercised by e2e. mssql-odbc doesn't currently depend on mssql-mock-tds, so the cheapest path is probably to lift the pure decision logic out — e.g. a function that maps (autocommit, local_tran_started, has_active_transaction, requested_mode) to an action enum — and unit-test that, leaving the thin I/O shells uncovered. Worth checking what the CI gate actually reports before merge.

CHANGELOG. No CHANGELOG.md entry. This PR adds a new exported entry point (SQLEndTran), three connection attributes, five SQLGetInfo types, and two SQLGetFunctions advertisements — the ## [Unreleased] / ### Added section already carries mssql-odbc: entries at this level of granularity (the INFO-message work, for instance).

[David Engel (David-Engel)]

Re-review of 651c36e

All nine points from the previous round are addressed, and the fixes are the right ones. Verified locally on this head: cargo clippy -p mssql-odbc --all-targets clean, 516 lib unit tests pass (up from 514).

Previous finding	Status
Blocking — cursor sweep skipped when local_tran_started is false	Fixed. close_all_cursors moved ahead of the early return, SQL_SUCCESS semantics preserved, new unit test end_tran_sweeps_cursors_even_with_no_transaction_started reproduces it correctly
SQLEndTran(ENV) returned SQL_ERROR with no ENV diagnostic	Fixed. HY000 summary with the failure count, detail left on each DBC, plus a negative test asserting no record on the all-success path
Poisoned DBC mutex silently skipped in the ENV fan-out	Fixed — now contributes SQL_ERROR and increments failed
apply_post_connect_txn_settings returned SQL_SUCCESS over a stale error record	Fixed
ensure_transaction pass-through wrapper	Removed, three call sites now use begin_transaction_if_manual directly
value as u32 truncating cast / dead unwrap_or	Fixed — (level, tsql) carried out of the locked block
HY011 ordered before the same-value no-op	Fixed, with a good comment on why the order matters
switch_to_manual_commit doc drift	Fixed

Two items from the previous round are still open, and there's one new issue with the regression guard for the blocking fix. Details inline.

Still open

CHANGELOG. No CHANGELOG.md entry yet (the diff is 20 files, none of them the changelog). New exported entry point, three connection attributes, five SQLGetInfo types.

Diff coverage. txn.rs is at 36.07% line coverage (257/402 missed), up from 33.16%; end_tran.rs is at 95.00%. The two new unit tests helped, but the CI 85% diff-coverage gate is still the thing to watch — everything in txn.rs that needs a live TdsClient remains e2e-only.