microsoft / microsoft/mssql-rs

mssql-odbc: instructions say "never recover a poisoned Mutex via into_inner()" but PR #415 has a justified exception

Open Beginner friendly
#430 0 comments 0 reactions 0 assignees View on GitHub
documentation mssql-odbc
Dominant language
Rust
Stars
53
Forks
14
Avg merge
1d 15h
Merged PRs (30d)
137

Description

### Context

`.github/instructions/mssql-odbc.instructions.md` states, without qualification:

> For `Mutex::lock()`, return `SQL_ERROR` on poison — use `let Ok(state) = handle.inner.lock() else { return SQL_ERROR; }`. Do **not** recover via `e.into_inner()`.

PR #415 (https://github.com/microsoft/mssql-rs/pull/415) added `mssql-odbc/src/handles/mod.rs`'s `live_handles()` helper, which does exactly what this rule forbids:

```rust
fn live_handles() -> MutexGuard<'static, HashMap> {
LIVE_HANDLES
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner())
}
```

This was a deliberate, reviewed exception (raised by @shiwanigupta0809, confirmed correct by a follow-up automated review from @David-Engel/Copilot), not an oversight: `LIVE_HANDLES` is a process-global registry whose critical sections are a single `HashMap` operation each (`insert`/`remove`/`get`), with no user code, I/O, or user-supplied `Hash` impl that could panic mid-update and leave the map in a genuinely inconsistent state. Treating poison as fatal here (per the general rule) would mean one poisoning — which the crate's analysis suggests may not even be reachable in practice — permanently blinds `free_stmt`/`free_desc`'s liveness check for the rest of the process, turning every subsequent handle-free into a silent leak. The same pattern (a process-global `Mutex` over a plain collection with no invariants to protect, recovered via `into_inner()`) already exists elsewhere in the crate: `auth/msqa.rs:311` and `auth/entra.rs:272`.

### The gap

The instructions file states the rule absolutely, with no carve-out for this narrower, already-established pattern. As the reviewing agent put it: "That mismatch is what makes the next reviewer or agent re-litigate a settled decision."

### Suggested follow-up

Amend `.github/instructions/mssql-odbc.instructions.md`'s poison-handling guidance to add a narrow, explicit exception: recovery via `into_inner()` is acceptable **only** for a process-global lock guarding a plain collection with no invariant that a panic could leave broken (no user code/I/O/user trait impls in the critical section) — `SQL_ERROR` on poison remains the rule everywhere else, in particular for any lock guarding a handle's own mutable state.

### Severity / urgency

Low — this is a documentation consistency issue, not a functional bug. Nothing is broken today; the risk is purely that a future contributor or reviewing agent re-flags the same already-settled `into_inner()` usage as a violation, or a different future `into_inner()` usage gets added without the same justification this one had.

Contributor guide

Open the contributing guide

Research direction

Start with .github/instructions/mssql-odbc.instructions.md and compare its poison-handling guidance with the live_handles() example in mssql-odbc/src/handles/mod.rs. Review the related patterns in auth/msqa.rs:311 and auth/entra.rs:272. Done means the instructions document the narrow justified exception while retaining SQL_ERROR as the general rule.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
documentation
Issue type
Documentation
Difficulty
1/5
Estimated time
Under an hour
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.