Choose ENV runtime teardown by process state: join while alive, leak while exiting

[David Engel (David-Engel)]

Description

Teardown of the per-ENV Tokio runtime has opposite requirements depending on whether the process is still alive. This makes the choice explicitly, closing two bugs that pull in opposite directions.

process state	policy	why
alive	Join — the normal Runtime drop, which waits for its threads	returning from SQLFreeHandle(ENV) lets the host unload the DLL (AB#47831)
exiting	Leak — touch nothing at all	joining panics (AB#47509); signalling hangs (AB#47510)

The shutdown hang (AB#47510)

A process that used this driver would hang forever on the way out, after its workload had already succeeded. mssql-python frees its pooled connection handles from its extension module's CRT onexit table, which the loader runs from DLL_PROCESS_DETACH inside LdrShutdownProcess — after Windows has terminated every thread but the one calling ExitProcess. SQLFreeHandle there released the last Arc<Runtime>, and the teardown waited on threads that no longer existed.

#459 addressed the panic with shutdown_background(), which skips the join but still signals the scheduler and takes locks a terminated worker may have died holding. Measured against main at 2e2afe2:

	worker thread	main thread
before #459	hang	panic, then exit
main at 2e2afe2	hang	hang
this PR	exits 0.5s	exits 1.4s

tests/test_025_logging_concurrency_deadlock.py went from 1 failure to 2 against that build; both pass here. The minimal repro is a single connect() / close() on a threading.Thread — no pooling, no logging, no concurrency, despite where AB#47510 was first observed.

The live-process unload race (AB#47831)

shutdown_background() is also wrong in the other direction. It returns without waiting, so after SQLFreeHandle(ENV) returns a host can unload mssqlodbc.dll while a runtime thread is still executing Tokio, mio, or driver code from that module — an intermittent STATUS_STACK_BUFFER_OVERRUN while mio's IOCP completion buffer was destroyed, at roughly one crash per 45 runs of a single e2e binary. DLL/thread lifetime is the leading hypothesis rather than an established mechanism; the captured stack shows where the fault lands, not why. Either way, waiting is what makes the unload safe.

Teardown paths beyond the ENV drop

Runtime::block_on is worse than either: it parks the calling thread and needs the scheduler's worker to drive the socket, so with that worker terminated the round-trip never completes. process_is_shutting_down() is pub(crate), and every block_on on a host's teardown sequence — SQLFreeHandle(STMT) → SQLDisconnect → SQLFreeHandle(ENV) — consults it:

site	reached from
free_handle.rs unprepare	SQLFreeHandle(SQL_HANDLE_STMT)
close_cursor.rs close_query	drain_and_release, from free / SQLCloseCursor
exec_common.rs cancel_streamed_write	unwind_dae, from SQLFreeHandle(SQL_HANDLE_STMT)
txn.rs rollback_transaction	rollback_before_disconnect, from SQLDisconnect

All four are best-effort cleanup the server redoes when the connection drops, so they are skipped while exiting; each still hands the client back so the DBC stays consistent. Three further block_on sites are on execute and fetch paths, unreachable during process shutdown, and are deliberately left alone.

Each site takes the flag as a parameter rather than reading it, so the skip arms — unreachable in a live process — are testable at all.

Validation

Mutation-verified, i.e. the guard was neutered and the test observed to fail:

behaviour	test	discriminator
ENV drop waits while alive	the_join_policy_waits_for_blocking_work_to_finish (+ EnvHandle and DBC-outlives-ENV twins)	blocking task finished on return
ENV drop touches nothing while exiting	the_leak_policy_touches_nothing_and_leaves_the_runtime_running	scheduler still polling
policy mapping	only_a_shutting_down_process_selects_the_leak_policy	both arms
cursor drain skipped	drain_and_release_skips_the_round_trip_while_the_process_is_exiting (+ flag-false twin)	batch left undrained
disconnect rollback skipped	rollback_before_disconnect_skips_the_round_trip_while_the_process_is_exiting	transaction left open

Not mutation-verified, and named accordingly: unwind_dae_leaves_the_connection_usable_while_the_process_is_exiting and unprepare_on_free_leaves_the_connection_usable_while_the_process_is_exiting. cancel_streamed_write returns (), and a failed unprepare neither errors observably nor marks the connection dead, so on a scripted client a skipped round-trip is indistinguishable from an attempted one. These pin the hand-off the skip arm still owes — client returned, active_stmt cleared — and the skip itself is covered by the mssql-python swap job.

dll_unload_stress_test drives the AB#47831 window directly: load, connect, query, free, unload, repeat. run_e2e.ps1 sets MSSQL_ODBC_DLL for the mssql-odbc leg only, so it runs there and skips on the reference leg (parity-neutral — parity_report.py treats a one-sided SKIP as not-compared). It only reproduces with a live connection: an allocate-and-free-only loop survived 1000 iterations against a known-bad shutdown_background build, because the I/O driver whose buffers fault is never exercised. The connect-and-query shape is load-bearing, and the file says so.

Limit worth stating plainly: AB#47831 did not reproduce on the machine this was developed on. Following the work item's own repro (execute_test.exe, 90 runs), a pre-fix shutdown_background build produced 0 crashes and this build produced 0 crashes, where roughly 2 were expected in the first column. That is a null result on this hardware, not confirmation — and it means a green dll_unload_stress_test is a regression guard going forward, not evidence the crash is gone. Confirming that needs a run on a machine where it reproduces. The change rests on the mechanism, on tests that provably catch a revert, and on no measurable regression.

End to end, a per-file run of all 42 mssql-python files on Windows shows no regression, with zero timeouts.

Related Issues

https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/47510

Checklist

• cargo bfmt passes
• cargo bclippy passes — including the check-private-items lint added by Enforce safety docs for private unsafe functions #485
• cargo btest passes — mssqlodbc's 1303 tests pass. The mssql-tds live-server integration and bench binaries fail locally for want of credentials (Login failed for user ''); they are unaffected by this change, which touches only mssql-odbc. Left to CI to confirm.
• New/changed functionality has tests — see Validation, including which guards are mutation-verified and which are not.
• Public API changes are documented — no public API change. tokio.md and docs/typed-columnar-fetch-plan.md are updated to describe the two-state teardown.

[Copilot]

🟡 Changes recommended

The Windows shutdown branch that fixes the regression lacks automated coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Prevents Windows host shutdown hangs by conditionally leaking the per-environment Tokio runtime during loader teardown.

Changes:

• Adds SharedRuntime with shutdown-aware disposal.
• Migrates ENV/DBC and fetch paths to the wrapper.
• Documents the failure mechanism and validation.

File summaries

File	Description
mssql-odbc/src/handles/runtime.rs	Implements shutdown-aware runtime ownership.
mssql-odbc/src/handles/mod.rs	Exports the runtime wrapper internally.
mssql-odbc/src/handles/env.rs	Stores SharedRuntime on ENV handles.
mssql-odbc/src/handles/dbc.rs	Shares the wrapper with DBC handles.
mssql-odbc/src/api/fetch_scroll.rs	Updates runtime parameter types.
mssql-odbc/tokio.md	Documents runtime teardown behavior.
mssql-odbc/docs/typed-columnar-fetch-plan.md	Records the resolved Windows hang.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

---

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

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 98%

🎯 Overall Coverage 93.4%

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

---

Diff Coverage

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

• mssql-odbc/src/api/close_cursor.rs (98.4%): Missing lines 322
• mssql-odbc/src/api/exec_common.rs (100%)
• mssql-odbc/src/api/free_handle.rs (100%)
• mssql-odbc/src/api/txn.rs (100%)
• mssql-odbc/src/handles/env.rs (97.2%): Missing lines 232-233

Summary

• Total: 253 lines
• Missing: 3 lines
• Coverage: 98%

mssql-odbc/src/api/close_cursor.rs

  318             ds.client = Some(client);
  319             if ds.active_stmt == Some(statement_handle) {
  320                 ds.active_stmt = None;
  321             }
! 322         }
  323         return DrainOutcome::Clean;
  324     }
  325 
  326     if let Err(e) = dbc.runtime.block_on(client.close_query()) {

---

mssql-odbc/src/handles/env.rs

  228         .worker_threads(1)
  229         .enable_all()
  230         .build()
  231         .inspect_err(|e| {
! 232             error!(%e, "failed to create Tokio runtime");
! 233         })
  234 }
  235 
  236 impl EnvHandle {
  237     pub(crate) fn new() -> io::Result<Self> {

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[ttk (Theekshna)]

⚠️ This review was produced by an unattended automated run.

This PR updates mssql-odbc's shared Tokio runtime teardown so ENV shutdown leaks the runtime during Windows process shutdown instead of signalling a dead scheduler, and it adds the corresponding regression tests and docs. I checked the merged diff, the prior Copilot thread and author follow-up, the coverage report, and the green CI; the code change itself looks sound, but one documentation update still presents the old and new Windows-validation states at the same time.

Severity	Count
Blocking	0
Suggestion	1
Nit	0

Blocking: none.

Suggestion:

• mssql-odbc/docs/typed-columnar-fetch-plan.md:289 — the new note says the fix now lets the 42-file Windows run complete with zero timeouts, but the paragraph immediately above still says there is no Windows-equivalent 42-file result yet and describes the old hang as current. Please rewrite or relocate that earlier paragraph so the plan presents one current state instead of both the pre-fix and post-fix story at once.

Nit: none.

[ttk (Theekshna)]

Automated unattended run — comment-only, not checked by a human before posting.

Summary

Re-review triggered solely by head move (my prior review at 8ea9b11c flagged a doc contradiction, findings=1). At the current head (c7564c86) that finding is fixed: the typed-columnar-fetch-plan.md section now presents one current validation state (the post-fix 42-file pass) and moves the pre-fix hang into explicit history, exactly as requested.

Re-reviewed the mechanism fresh rather than trusting that alone:

• FFI correctness: RtlDllShutdownInProgress is an undocumented but well-known ntdll export (BOOLEAN RtlDllShutdownInProgress(VOID), no params) — the declared signature (unsafe extern "system" fn() -> u8, no args) matches, and the SAFETY comment is accurate. CI's Windows and Windows ARM build legs confirm it links without WDK dependencies.
• Mutation verification claim is real, not asserted: reverted Leak → shutdown_background() locally and confirmed the_leak_policy_signals_nothing_and_leaves_the_runtime_running fails; reverted release_policy to always return Detach and confirmed only_a_shutting_down_process_selects_the_leak_policy fails. Both match the PR description.
• Existing regression tests still guard the non-shutdown path: dropping_env_handle_does_not_wait_for_blocking_work and the DBC-outlives-ENV variant still exercise Detach end-to-end via EnvHandle/DBC drop, unaffected by the refactor.

Check	Result
msodbcsql parity	N/A — msodbcsql has no async runtime/thread-pool analog to this teardown hazard. The closest related code (sqlncli's own DllMain, DLL_PROCESS_DETACH branch checking lpvReserved) solves a different problem: detecting whether its own DLL is unloading vs. the process exiting. This PR's hazard is a caller (mssql-python) freeing an ODBC handle from another module's onexit table during LdrShutdownProcess, where no lpvReserved-equivalent is available — RtlDllShutdownInProgress is the only applicable primitive, so this isn't a comparable/divergent code path.
Test sufficiency	The one genuinely untested branch (process_is_shutting_down() == true reached through the real Drop impl) is called out honestly in the PR description as unreachable from a Rust unit test, and is covered instead by the cross-repo mssql-python swap job (test_025_logging_concurrency_deadlock.py). No hidden gap.
Divergence documented	N/A per above — not a parity divergence.
PR description currency	Matches the diff; checklist claims match gh pr checks (all green); linked work items (AB#47509, AB#47510) present.
AI slop	None found — the added doc comments explain why (loader hazard, why the flag is declared by hand instead of via the windows crate, why new_runtime was extracted) rather than restating code.

Severity	Count
Blocking	0
Suggestion	0
Nit	0

No new findings.

[Shiwani Gupta (shiwanigupta0809)]

Read the diff, the resulting env.rs, and traced every block_on reachable from a teardown-time ODBC entry point. Both prior findings are resolved — the injectable-policy split genuinely closes the hole Copilot identified, and the doc section now presents one state. Not re-raising either.

Blocking: none. Suggestions: 1 (inline).

Verified

The policy split is the right shape and the mutation claims hold up. release_policy taking the flag as an argument rather than reading it is what makes the Leak arm reachable at all, and scheduler_still_runs_tasks is a real observable for "signalled nothing" rather than a restatement of the branch. The point in the reply — that the original test passed clean under the delete-the-condition mutation — is the part worth keeping in mind: it is the reason the refactor was necessary rather than cosmetic.

The #[cfg(not(windows))] arm is correct, not just convenient. Returning false unconditionally is right because POSIX atexit / destructor ordering does not terminate other threads before running library teardown, so shutdown_background() has live threads to signal. Worth stating that the arm is a real claim about the platform rather than a stub, since it is the half that never gets exercised by the repro.

Leaking the Runtime leaks only the runtime. The ManuallyDrop::take + mem::forget path drops nothing else on the way — EnvState's other fields still drop normally — so this does not silently suppress unrelated cleanup.

Suggestion

The fix guards SharedRuntime::drop, but that is not the only place the driver touches the scheduler during teardown. Left inline on the new primitive.

[Shiwani Gupta (shiwanigupta0809)]

Reviewed — posted as a review with one inline suggestion on process_is_shutting_down: #486 (review)

Short version: the policy split and the mutation evidence hold up, and the cfg(not(windows)) arm is a real platform claim rather than a stub. The suggestion is that the new flag has one caller — SharedRuntime::drop — while six block_on sites reachable from teardown-time ODBC calls touch the scheduler earlier in the same window. free_handle.rs:471 (unprepare on SQLFreeHandle(STMT)) sits on the exact DBC-free cascade the PR describes. Not firing today, but for state reasons rather than by design.

[Saurabh Singh (saurabh500)]

Unattended review sweep — automated, not checked by a human before posting. Treat the findings as a starting point, not a gate.

Verdict: no blocking issues. The core fix is sound, and I verified it independently beyond CI. I have one suggestion about an adjacent shutdown path this PR leaves unguarded.

What the PR does. It splits the runtime-release decision (release_policy(bool)) from the action (release) so both the shutdown Leak arm and the normal Detach arm are reachable from a live test process, and guards the three teardown-reachable block_on sites (unwind_dae, drain_and_release, the stmt-free unprepare) so SQLFreeHandle during DLL_PROCESS_DETACH skips its server round-trip instead of parking forever on a scheduler whose worker the OS already killed.

What this sweep checked independently (not just the green boxes):

• Mutation-tested the two new guard tests in a local worktree (cargo nextest run -p mssqlodbc --lib): reverting Leak => std::mem::forget back to shutdown_background (the #459 behaviour) fails the_leak_policy_signals_nothing_and_leaves_the_runtime_running; forcing release_policy to always return Detach fails only_a_shutting_down_process_selects_the_leak_policy. Both tests bite.
• Teardown-reachability of every block_on in the crate. The three left unguarded live on execute/fetch paths (finish_execute, release_busy_if_row_exhausted, flush_pending_unprepare) and are not reachable from SQLFreeHandle/SQLCloseCursor teardown, so leaving them alone is correct. The three guarded ones each still hand the client back (return_client_idle, or restoring dbc.client + clearing active_stmt), so the DBC is left consistent whether or not the round-trip runs.
• The RtlDllShutdownInProgress FFI (BOOLEAN -> u8, no args, resolved at load time) and the cfg(not(windows)) -> false arm both read correctly.

Blocking: none.

Suggestion:

• A shutdown-time SQLDisconnect still parks on an unguarded block_on — mssql-odbc/src/api/txn.rs:790-797. rollback_before_disconnect calls dbc.runtime.block_on(client.rollback_transaction(None, None)) with no process_is_shutting_down() check. It is reachable from SQLDisconnect (disconnect.rs:90), which a host can call from the same static-destructor / onexit path this PR fixes for SQLFreeHandle; with an open transaction at process exit it hangs on exactly the dead scheduler AB#47510 is about. This PR actually moves the disconnect path closer to that hang: close_all_cursors -> close_cursor_for_connection_op -> the now-guarded drain_and_release, so a shutdown-time SQLDisconnect clears the cursor sweep and proceeds straight into the unguarded rollback. txn.rs is not in this diff, so this is a pre-existing gap and a defensible deferral — but since the PR deliberately generalized process_is_shutting_down() to pub(crate) for exactly this shape, it is worth either guarding the sibling here:

  if client.has_active_transaction()
      && !process_is_shutting_down()
      && let Err(e) = dbc.runtime.block_on(client.rollback_transaction(None, None))
  {
      error!(%e, "{OP}: rollback failed; the server will roll back on disconnect");
  }

  (the fall-through to release_dbc_client still runs, and the server rolls the transaction back when the socket drops anyway), or filing a follow-up so the shutdown-hang fix is not silently partial. Is SQLDisconnect-at-shutdown intentionally out of scope for this PR?

Nit: none.

[David Engel (David-Engel)]

Saurabh Singh (@saurabh500) — thanks, this was worth catching.

Is SQLDisconnect-at-shutdown intentionally out of scope for this PR?

No, and it shouldn't be. Guarded in f08a29bc.

You are right that txn.rs was not in the diff, but "pre-existing" understates it: this PR moved that path closer to the hang. rollback_before_disconnect runs close_all_cursors first, which goes through the drain_and_release I guarded two commits ago. Before that guard, a shutdown-time SQLDisconnect with an open cursor stalled in the sweep. Now the sweep returns cleanly and control walks straight into the unguarded rollback. So the fix I pushed to close one hole widened the reachability of the next one along.

That is the shape I criticised #459 for — the visible symptom moves while the hazard stays — so deferring it after making that argument would have been hard to defend.

The guard is yours with one change: I log at debug on the skip, matching the other three sites, rather than folding !process_is_shutting_down() into the existing condition. It keeps "we deliberately skipped this" distinguishable from "there was no transaction" in a trace. release_dbc_client still runs either way, as you noted. I also extended the function's doc comment to say it is on the same DLL_PROCESS_DETACH path as SQLFreeHandle, since that is the non-obvious part.

On the reachability boundary. That is now every block_on on the teardown sequence SQLFreeHandle(STMT) → SQLDisconnect → SQLFreeHandle(ENV). The remaining ones are on execute/fetch paths, which agrees with your independent trace.

One thing worth flagging that is genuinely not mine. Re-running the 42-file suite after merging main showed test_023_execute_path_parity at 51 failed / 83 passed, against 21 / 113 before the merge. I did not want to hand-wave that, so I bisected it: identical 51/83 with my txn.rs change stashed, and identical 51/83 on pure origin/main at 71906b5f with none of this PR applied. It is deterministic across runs, not flake, and it is not from this branch — one of the commits that landed while this was open regressed it. Everything else moved the other way (test_003, test_004, test_013, test_015 all gained passes from those same commits). Someone may want to look at test_023 separately.

Also worth being explicit, since your sweep checked the tests carefully: none of these four guarded branches is reachable from a test process. process_is_shutting_down() is false in every unit test and CI run — which is also exactly what bounds their blast radius on the normal path. The evidence they are correct is the mechanism plus the unchanged e2e baseline, not coverage.

[ttk (Theekshna)]

Unattended run found no new findings. Covered: msodbcsql parity (N/A — independently confirmed against sqlncli_main.cpp's DLL_PROCESS_DETACH/lpvReserved check, which solves a different problem: DllMain unload-vs-exit, not an ODBC entry point called from another module's onexit after the OS has killed other threads), a fresh compile (cargo check -p mssqlodbc --all-targets), the newest commit's xn.rs guard (closes the SQLDisconnect rollback gap saurabh500 raised, consistent with the three sibling guards), test sufficiency (all four shutdown branches are honestly documented as unreachable from a Rust unit test and covered instead by the cross-repo mssql-python swap job), divergence docs (N/A, not a parity divergence), PR description currency (matches the diff, checklist matches green CI, AB#47509/AB#47510 linked), and AI slop (none — doc comments explain the loader hazard and are the load-bearing safety argument, not restatements).

[David Engel (David-Engel)]

Summary

The process-shutdown detection and guards address the dead-worker hang, but the live-process release policy still detaches runtime threads. AB#47831 reports an intermittent Windows teardown crash after this behavior landed, so normal release should synchronously join while only the already-shutting-down process path leaks.

[ttk (Theekshna)]

This is a well-reasoned fix — the two-state Join/Leak split is correct, the four block_on call-site guards are consistent, and the unit tests are mutation-verified (reverting Join→shutdown_background or dropping the shutdown check fails the tests that guard them, as claimed). msodbcsql parity is N/A — this is process-teardown/runtime-lifecycle machinery, not an ODBC SQL-type/attribute surface, so there's no divergence to register. Docs (tokio.md, typed-columnar-fetch-plan.md) match the code. However, the new Windows regression test for the bug this PR fixes does not compile.

Severity	Count
Blocking	1
Suggestion	0
Nit	0

[Blocking] mssql-odbc/tests/e2e/tests/dll_unload_stress_test.cpp:109 has malformed C++ — verified by actually compiling the line (not just reading it):

conn += "UID=" + uid + ";******ODBC_TEST_PWD", "") + ";";

g++ (-std=c++17) rejects it: error: expected ';' before ')' token. The shape (;******ODBC_TEST_PWD", "")) looks like PWD=" + GetEnvOr(" was replaced by ******, breaking the statement — the intended line is presumably:

conn += "UID=" + uid + ";PWD=" + GetEnvOr("ODBC_TEST_PWD", "") + ";";

matching the UID= branch two lines above and the other GetEnvOr(...) call sites in this same function. This file is registered in CMakeLists.txt via add_odbc_test(dll_unload_stress_test tests/dll_unload_stress_test.cpp), and enableOdbcE2E: true is set for the Windows PR build stage (validation-stages.yml:132, templates/build-template.yml:187), so this should fail that stage's compile — yet gh pr checks currently reports Build Stage Build Windows green. Worth a look either way: if CI is genuinely compiling this file and passing, something in the toolchain is silently accepting invalid syntax (very unlikely) or masking the failure; if it isn't being compiled (e.g. a stale/reused build), that's a validation gap independent of this bug. I did not have ADO access to inspect the actual build log for this run.

This also means the checklist's "New/changed functionality has tests" isn't fully true yet: the stress test that's supposed to directly guard AB#47831 (the DLL-unload crash this PR fixes) can't currently run on any platform where it isn't skipped.

Everything else checked clean: test mutation-resistance for the Rust-side unit tests, PR description/checklist currency against the diff and CI, and no AI slop in the new doc comments (they carry load-bearing safety reasoning, not restatement).

---

Unattended automated review. Findings above were not checked by a human before posting.

[David Engel (David-Engel)]

Following up on the "is CI actually compiling this file?" question from the review summary, since you noted you couldn't check it without ADO access. There is no validation gap — CI does compile it, and it compiled fine.

The chain:

1. templates/validation-stages.yml:132 sets enableOdbcE2E: true for the Windows PR build stage, as you found.
2. mssql-odbc/tests/e2e/run_e2e.ps1:682 runs cmake --build build --config Debug with no --target, so it builds every registered binary — including dll_unload_stress_test, which add_odbc_test registers.
3. Build Stage Build Windows was green on 7842d165, the exact commit you reviewed.

So the green check was correct and the compile error was not reproducible from the committed source. I verified the same thing two other ways: the raw character codes of that line are byte-identical locally and via the GitHub contents API at 7842d165, and MSVC compiled the file clean before I touched it.

The cause is a secret-redaction filter, not a syntax error. The keyword-equals-value pattern gets rewritten to asterisks by review and code-reading tooling, and the copy that was compiled was a redacted one. The tell is visible in your own review body: the "malformed" line you quote and the "intended" line you propose as the fix render identically, because both were redacted on the way out. My own file viewer has the same problem — I could only read the line by dumping character codes.

I've restructured it anyway in 02661b4c so this can't recur for the next reviewer, and re-verified: compiles under MSVC, passes 200 iterations against a live connection.

Two things you flagged that were fair and that I've acted on:

• The checklist claim. You were right that "New/changed functionality has tests" deserved scrutiny — not for the compile reason, but because I'd found separately that this stress test couldn't have caught AB#47831 as originally written. An allocate-and-free-only loop passes 1000 iterations against a deliberately-rebuilt shutdown_background driver, because the faulting IOCP buffers are never exercised without a live connection. It now connects and queries inside each cycle, with a comment warning against "simplifying" that away.
• A real bug the detour surfaced. While restructuring, a $P helper clobbered the $p path variable — PowerShell variable names are case-insensitive — so the rewrite landed in a stray file named P at the repo root while the real .cpp went unmodified, and the build I ran to "verify" was exercising the old file. Caught and corrected in 02661b4c; the stray file is deleted and the net diff is back to the intended 10 files.

Still worth a human's eye: I have not been able to reproduce AB#47831 on this machine in either direction (0 crashes in 90 runs against both the pre-fix and fixed builds, where ~2 were expected pre-fix), so the fix rests on mechanism and mutation-verified tests rather than on observing the crash disappear.

[David Engel (David-Engel)]

Test MacOS failed on 02661b4c with two failures, both in mssql-tds:

TRY 6 FAIL [ 2.377s] mssql-tds::timeout_and_cancel timeout_and_cancel_tests::query_cancel
TRY 6 FAIL [ 1.104s] mssql-tds::timeout_and_cancel timeout_and_cancel_tests::query_timeout_e2e
Summary [308.827s] 3728 tests run: 3726 passed, 2 failed, 40 skipped

Unrelated to this PR, and I checked rather than assumed:

• This PR touches no mssql-tds files. The diff is 10 files, all under mssql-odbc/. mssql-tds is a dependency, not a dependent, and timeout_and_cancel is its own test binary that never links mssqlodbc — there is no path from this change to that test's behaviour.
• The same code passed twice already. mssql-tds is byte-identical to what was on 7842d165 and afb5c9ee, both of which went 19/19 green. Only mssql-odbc files changed since.
• The tests are wall-clock assertions against a live server. query_timeout_e2e runs WAITFOR DELAY '00:00:05' with a 2s timeout and asserts the failure lands in a 1500–3000ms window. On a loaded macOS agent that window is easy to miss, and the observed durations (2.377s / 1.104s) are consistent with agent scheduling noise rather than a behavioural change.
• They already exhausted the retry budget — .config/nextest.toml sets retries = 5 for the ci profile, and both report TRY 6, so this is not something more retries inside the run will settle.

The file itself hasn't been modified since #143, so this looks like existing flakiness in that suite on the macOS leg rather than something new.

Re-running validation rather than touching an unrelated test:

/azp run

[ttk (Theekshna)]

Unattended re-review: the two new commits (082f4fae, 02661b4c) fix dll_unload_stress_test.cpp so the password-literal line is no longer mangled by secret-redaction filters, and the earlier #ifdef _WIN32-gated content compiles clean under the current Windows PR build stage (verified against check-runs for this exact head SHA, not a cached result). This also confirms my own prior Blocking finding on that line was a false positive from the same redaction artifact David-Engel documented in the fix commit — retracted. All 6 review threads are resolved. The merge from main and the empty re-trigger commit carry no new content of this PR's own. coverage-report's failure is a stale Azure DevOps build (172407, superseded by the passing 172423) per the re-trigger commit's own explanation, not a defect in this diff.

Covered: msodbcsql parity (N/A for this round's delta — no ODBC-table arms touched by the two new commits), test sufficiency (unchanged; stress test unaffected by the literal fix), divergence docs (N/A), PR description currency (matches current diff and checklist; AB#47510/47831/47509 linked), verbose slop (the new workaround comment at lines 109-113 records why, not slop), evidence audit (re-derived the 4-site process_is_shutting_down() gating table in free_handle.rs/close_cursor.rs/exec_common.rs/txn.rs — all four gate correctly; did not re-run the live-connection crash-rate or 42-file timing claims, no live SQL Server available in this context).

Severity	Count
Blocking	0
Suggestion	0
Nit	0

[ttk (Theekshna)]

Reviewed the change, the existing comment threads, and checked parity against msodbcsql in ODBC_2.

The policy decision is right, and I verified the parity claim rather than taking it on trust. sqlncli_main.cpp:139-141 skips cleanup outright when the process is shutting down (if (lpvReserved) break;), and sni.cpp:877 reinforces it ("AFTER THIS POINT, MEMORY APIS SHOULD NOT BE USED"). The Leak arm is the direct analogue of the shipping driver's own rule. The Join arm's argument stands on its own mechanism. Tests are good, not vacuous, and the mutation verification (revert Join -> 3 failures; delete the condition -> 1 failure) is the right bar.

Two blockers below, both the same shape: the guard does not guard anything yet. Neither is a correctness defect in the shipped logic.

One thing worth knowing, no change requested: Join makes SQLFreeHandle(ENV) unbounded, because Runtime::drop waits on spawn_blocking tasks, which are not cancellable, and auth/interactive.rs runs the OneAuth sign-in that way on this same runtime (reached via driver_connect.rs:408). Waiting is the fix and a bound would reintroduce AB#47831, so I am not asking for one -- flagging the wrinkle only. Reachability unverified.

Also: keep the "did not reproduce on the machine this was developed on" disclosure exactly as written. That is the correct call.

For the record, things I raised while reviewing and then withdrew after checking, so nobody re-litigates them: dropping the TdsClient in disconnect.rs as a residual hang (your own repro exits in 0.5s through that path, so the evidence contradicts it); Runtime::drop panicking in an async context (unreachable -- only EnvHandle/DbcHandle hold the Arc, both released from FFI entry points); and a lpvReserved-vs-RtlDllShutdownInProgress parity nit (moot -- this crate has no DllMain, so there is no detach callback to read lpvReserved from).

[ttk (Theekshna)]

Unattended hourly sweep: reviewed the delta since the last automated review (c1e12249→c1c45b13, the "Cover the four shutdown guards, and wire the stress test into CI" commit). It fully addresses the prior round's blocking finding — all four process_is_shutting_down() call sites now take the flag as a parameter via an _inner seam, matching the pattern already used for the ENV drop, so the skip arms are no longer dead code under test. Source-traced each new test: the two labeled mutation-verified (drain_and_release, rollback_before_disconnect) genuinely discriminate on the flag (asserting undrained batch / open transaction only when true, with a false twin proving the guard isn't unconditional); the two labeled not-mutation-verified (unwind_dae, unprepare_on_free) are honestly described — cancel_streamed_write returns () and a failed unprepare has no observable effect, so skip-vs-attempt is indistinguishable on a scripted client, and the tests correctly pin only the hand-off (client returned, connection released) rather than the skip itself. run_e2e.ps1 now threads MSSQL_ODBC_DLL through Invoke-CtestRun for the mssql-odbc leg only (correct — the runtime under test is ours), leaving the reference leg to skip, which parity_report.py treats as parity-neutral.

Local build here hit a pre-existing toolchain/environment limitation unrelated to this PR — the ms-prod-1.95 codegen backend can't lower crc32fast's AVX-512/VPCLMULQDQ intrinsics on this host regardless of -C target-feature/target-cpu overrides — so I could not independently re-run the mutation locally. gh pr checks shows all CI legs (Windows/Linux/Linux ARM/macOS build+test, Kerberos, coverage-report, cross-repo mssql-python suite) green on this head SHA, which is what actually exercises the new tests; I relied on that plus the source trace above rather than re-deriving the same verdict.

No findings. This is an automated unattended run; findings were not checked by a human first.

Severity	Count
Blocking	0
Suggestion	0
Nit	0

[David Engel (David-Engel)]

Adding one piece of evidence that was not available at review time: the stress test is now confirmed to actually run in CI, which is the thing the previous round's second finding was about.

From the Windows e2e leg of ADO build 172517 on this head (c1c45b13):

MSSQL_ODBC_DLL=C:\cargo_target_dir\debug\mssqlodbc.dll
      Start 39: dll_unload_stress_test
39/39 Test #39: dll_unload_stress_test ...........   Passed   23.48 sec     <- mssql-odbc leg
      Start 39: dll_unload_stress_test
39/39 Test #39: dll_unload_stress_test ...........   Passed    0.01 sec     <- msodbcsql leg
dll_unload_stress_test         PASS        PASS        parity

That is exactly the intended shape, and the timings are the proof rather than the pass/fail:

• 23.48s on the mssql-odbc leg — 200 real load/connect/query/free/unload cycles.
• 0.01s on the reference leg — skipped, because MSSQL_ODBC_DLL is scoped to the Rust leg.
• parity in the report, confirming the one-sided skip is not a divergence, as parity_report.py:46 predicted.

Before this commit that test skipped on both legs and cost 0.01s twice, which is what made it invisible.

On your local build problem — no action needed from your side. The crc32fast AVX-512 lowering failure is a known pre-existing toolchain issue on some hosts and is unrelated to this diff; relying on the CI legs plus the source trace was the right call, and the run above is the leg that matters.

One thing I do not want this green run to be read as: it is not confirmation that the AB#47831 crash is fixed. I have never reproduced that crash on my machine — 0 crashes in 90 runs against a deliberately-rebuilt shutdown_background driver, where roughly 2 were expected. So this test is a genuine regression guard from now on, but its first green run does not retire the bug. That still needs a run on a host where the crash reproduces, and it is called out in the PR description under Validation.

[David Engel (David-Engel)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[David Engel (David-Engel)]

Test MacOS failed on build 172551, but nothing in it was a test failure. The root cause is on the first line of the job:

Install and start Colima-based Docker  ->  ##[error]The task has timed out.

The Colima VM never finished coming up — its log stops at Waiting for the essential requirement 2 of 3: 'user session is ready for ssh' and the task hit its timeout there. Everything reported after that is downstream of the aborted setup:

Save code coverage ...  ->  error: no such command: `llvm-cov`
Publish Cobertura ...   ->  Path does not exist: .../target/cobertura.xml

llvm-cov is missing because the toolchain setup never ran, and the artifact is missing because the step that produces it never ran. Zero tests executed and zero tests failed.

This is agent infrastructure, not this diff. Same failure signature PR #491 hit twice on builds 172385 and 172387 earlier today, so the macOS leg looks broadly unstable at the moment rather than reacting to anything here — worth someone looking at the Colima step's timeout independently of this PR.

I have not touched the failing test or the macOS configuration. Instead I merged #479 from main, which was one commit ahead, and pushed f3950882 — that keeps the branch current and re-runs validation past the flake in one step.

Re-validated locally after the merge: 1355 unit tests pass, cargo bfmt and cargo bclippy clean, and all 8 shutdown-guard tests still pass. My own diff is unchanged at 11 files.

[David Engel (David-Engel)]

Test MacOS failed again on build 172563, same signature as 172551 — and this is now the third consecutive Colima timeout, so I dug into whether it is really "just flake."

No test failed. Every test task in 172563 reports succeeded or skipped; the only errors are downstream of a setup step that never completed:

Install and start Colima-based Docker  ->  ##[error]The task has timed out.
Save code coverage ...                 ->  error: no such command: `llvm-cov`
Publish Cobertura ...                  ->  Path does not exist: .../cobertura.xml

But the numbers suggest this is a budget problem rather than randomness. The step is capped at timeoutInMinutes: 13 (780s). Measuring the same step on recent successful builds:

build	Colima duration	result
172423	531s / 640s	succeeded
172517	539s / 560s	succeeded
172563	794s	timed out

So healthy runs land at 531–640s against a 780s cap — roughly 18% headroom on a good day. start-colima-macos.sh itself budgets COLIMA_BUDGET_SECONDS=480 for retries and COLIMA_START_TIMEOUT_SECONDS=540 for one boot, and its header already documents ~3% boot flakiness. When a boot is slow rather than wedged, the retry logic and the step cap can end up racing each other.

Two different stall points across the two failures, which fits "slow agent" better than "wedged VM":

• 172551 died at Waiting for the essential requirement 2 of 3: user session is ready for ssh
• 172563 died earlier still, mid Decompressing the image

I have not touched the timeout or the script. This PR changes no macOS, Docker, or pipeline-timeout behaviour, and raising a shared CI budget to get one PR green is exactly the kind of change that should not ride along in an unrelated diff. Flagging it instead: someone owning CI may want to look at whether 13 minutes is still the right cap, independently of this PR. PR #491 hit the same step twice today (172385, 172387), so it is not specific to this branch.

Re-running validation:

/azp run

[David Engel (David-Engel)]

Fourth consecutive Test MacOS failure, and it is now clear this is not specific to this PR and not something a re-run will clear. Reporting rather than retrying, because retrying burns shared macOS agents for no benefit.

It is repo-wide right now. Every PR that ran validation in the last hour is failing on the same step:

build	PR	failing task
172583	486 (this one)	Install and start Colima-based Docker
172581	482	Install and start Colima-based Docker
172579	491	Install and start Colima-based Docker

Identical downstream cascade in all three (llvm-cov missing, cobertura.xml missing) because the setup step aborts before the toolchain is installed. No test fails in any of them.

The measurements say the budget has been outgrown, not that the VM is wedged. Colima step duration across this PR's recent builds, against a timeoutInMinutes: 13 (780s) cap:

build	duration	result
172423	531s, 640s	succeeded
172517	560s, 539s	succeeded
172551	791s, 725s	failed, succeeded
172563	794s, 626s	failed, succeeded
172583	790s, 791s	failed, failed

Two things stand out. Every failure lands in a 790–794s band — right at the cap, not scattered, which is the signature of a step being cut off rather than hanging. And the successful runs have drifted from ~535s to 725s over the same window, so the headroom has been eroding and 172583 is simply the first build where both macOS jobs ran out of it at once.

start-colima-macos.sh budgets COLIMA_START_TIMEOUT_SECONDS=540 for one boot and COLIMA_BUDGET_SECONDS=480 for retries, both sized against historical timings ("slowest healthy boot seen over 113 runs (509s)"). Those numbers no longer match what the agents are doing, so the script's own retry path and the 780s step cap can now collide.

I am not changing the timeout in this PR. It is a shared CI setting, this diff touches no macOS/Docker/pipeline behaviour, and raising a global budget so one PR goes green is precisely the kind of change that should not ride along in an unrelated change. It needs its own PR from whoever owns CI, with the numbers above as the justification.

For this PR specifically: the approval is in, mergeable is MERGEABLE, and the only thing standing between it and merge is a macOS leg that is currently failing for every branch in the repo. Everything that actually exercises this change — Windows build + e2e (including dll_unload_stress_test), Linux, Linux ARM, Kerberos, and the cross-repo mssql-python suite — is green.

[David Engel (David-Engel)]

Update on the failures, including one I had not yet traced: all four share the single Colima root cause.

coverage-report looked like an independent failure, but it is a downstream effect. Its log ends:

Timeout: coverage artifact not found within 75 minutes

That workflow polls the ADO build for the merged Cobertura artifact. Build 172583's macOS job aborted at the Colima step, so the artifact was never published, and the workflow waited out its full 75-minute budget for something that could not arrive. Nothing to fix in the workflow — it did exactly what it should.

So the current red set is: Test MacOS → the Colima timeout; Build mssql-python macOS → the same step on the same agent pool; Pull request validation → the stage rollup; coverage-report → the missing artifact. One cause, four symptoms, zero test failures.

No build has run anywhere in the repo since 11:20 (the three that did — 486, 482, 491 — all failed identically), so there was no signal on whether the agents have recovered in the hour since. I have pushed an empty commit to get that signal. main has not moved and the branch is unchanged, so there was nothing real to piggyback on; the commit message records why.

If this run comes back green, the outage was transient and this PR is ready. If it fails at 780s again, that is a fifth data point and the timeout bump is worth doing on its own merits — the measurements are two comments up, and I am deliberately not making that change here since it is a shared CI setting and this diff touches no pipeline behaviour.