From ea71fc1031948273c216b633a01cdf1976630048 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 11:45:59 -0500 Subject: [PATCH 1/2] fix(store): a cancelled call returned a pooled SQL Server connection mid-transaction (BACKLOG #348, ADR 0159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SqlServerStore's write idiom is `except Exception: await conn.rollback(); raise`, used at 90 of the 91 self._acquire() sites. CancelledError derives from BaseException, so on a cancellation no rollback runs, and aioodbc does not compensate: Pool.release() appends a non-closed connection straight back onto the free deque with no rollback, reset or transaction check (0.5.0 pool.py:196-205), and _ContextManager.__aexit__ uses the SAME release on the exception path (utils.py:60-62). The next borrower inherited an open transaction still holding X locks. Measured on a live SQL Server 2022 before the fix: cancelling release_claimed left 7 X locks on queue, reschedule_claimed 7, mark_done 9, enqueue_ingress 11 (the pre-ACK ingress commit) -- against 0 for the claim_fifo_heads control. The connection went back on the free list, a raw writer got error 1222, and a real second claim_fifo_heads returned EMPTY-all. Under ADR 0066 §9 that 1222 is a sanctioned yield, so the symptom was silence, not an error. Fix at the _acquire chokepoint, on a non-Exception BaseException only: synchronously drop the driver handle (conn._conn = None -- aioodbc derives `closed` from it and re-adds only `if not conn.closed`) with NO await in front of it, then close the raw handle off-loop under a 5s bound. The ordering IS the guarantee: a cleanup that awaits first is defeated by the second cancellation that shutdown's cancel-then-gather delivers. A plain `await conn.rollback()` was built and rejected on measurement -- it runs in the default executor bounded only by command_timeout (30s) with no upstream timeout; a cancel measured 1.005s against a 1.0s rollback, stalling exactly the demotion path (_stop_graph cancels but does NOT close the store, so the poisoned connection is re-borrowed there). Corrects the lead that found this on two points: it is not a two-method asymmetry (90 of 91 sites share the idiom; mark_done and enqueue_ingress leak identically), and claim_fifo_heads does NOT shield against it -- its guard is a SET LOCK_TIMEOUT reset guard, and ADR 0114 §2 plus a frozen test record that no rollback runs on its cancellation path. It ends clean because the guard COMMITS. Ordinary errors keep today's rollback-and-recycle behaviour, pinned by controls that pass before and after. SQL Server only: Postgres is safe twice over (asyncpg rolls back on any BaseException and its pool resets under shield); SQLite has no pool. Gate: 6/12 failing pre-fix (both cancellation properties x all three methods, controls already green), 12/12 after. Live repro 7 locks -> 0. Live SQL Server suite 182 passed (15 sqlserver arms verified executed, not skipped). --- docs/BACKLOG.md | 24 ++ ...d-txn-discard-at-the-acquire-chokepoint.md | 197 +++++++++++++ docs/adr/README.md | 1 + messagefoundry/store/sqlserver.py | 63 ++++- tests/test_backlog348_cancel_dirty_release.py | 264 ++++++++++++++++++ 5 files changed, 548 insertions(+), 1 deletion(-) create mode 100644 docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md create mode 100644 tests/test_backlog348_cancel_dirty_release.py diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 3e04e138..7be085f8 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8423,3 +8423,27 @@ Two worked instances the same day. **#74** went green on 2026-07-30 and sat unme **Related:** #339, #342 (sibling fd-1 issue), CLAUDE.md §9 (PHI logging), ADR 0087. **Source:** adversarial review of the ADR 0087 sandbox codec, 2026-08-01. + +--- + +## 348. SQL Server: a cancelled store call returns a pooled connection mid-transaction holding X locks + +> ✅ **Status CLOSED (filed + fixed 2026-08-02, [ADR 0159](adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md)).** `SqlServerStore`'s write idiom is `except Exception: await conn.rollback(); raise` — used at **90 of the 91** `self._acquire()` sites. `asyncio.CancelledError` derives from `BaseException`, so on a cancellation **no rollback runs**, and aioodbc's `Pool.release()` appends the connection straight back onto the free deque with no rollback, reset or transaction check (0.5.0 `pool.py:196-205`; `_ContextManager.__aexit__` uses the *same* `release` on the exception path). The next borrower inherited an open transaction still holding X locks on `queue` rows. Fixed by quarantining the connection at the `_acquire` chokepoint. + +**Cluster:** Store & Reliability. **Priority:** P2. **Verdict:** built. **Severity:** medium (pool integrity + a silent stall), low (likelihood: needs a cancellation to land inside a pooled write). + +**Reproduced on a live SQL Server 2022 before the fix**, cancelling each call mid-body and inspecting the server: `release_claimed` **7** X locks on `queue`, `reschedule_claimed` **7**, `mark_done` **9**, `enqueue_ingress` **11** — and `claim_fifo_heads` **0** (the control). The connection was back on the free list (`size=1 freesize=1`), a raw writer got **error 1222**, and a real second `claim_fifo_heads` returned **EMPTY-all**. After the fix: **0** locks, no open-transaction session, writer unblocked, connection dropped from the pool rather than re-lent. + +**Why it was invisible.** Under [ADR 0066](adr/0066-pooled-stage-claimers.md) §9 the claim runs `SET LOCK_TIMEOUT 0`, so a blocked claimer raises 1222 and the claim path translates that to a **sanctioned** EMPTY-all yield. The symptom is therefore silence — a lane that quietly claims nothing — not an error anyone would see. `enqueue_ingress` is the pre-ACK ingress commit, the engine's hottest path. + +**The path that bites is demotion, not shutdown.** `engine.stop()` closes the store shortly after cancelling, so the poisoned connection dies at teardown. Loss of leadership ([`engine.py:1242-1252`](../messagefoundry/pipeline/engine.py), `_stop_graph`) runs the identical cancel chain but **does not close the store** — the pool stays live and shared with the coordinator/convergence loops, so the connection sits in `_free` and is re-borrowed by unrelated callers. + +**Two corrections to the lead that opened this.** (1) It is **not** a two-method asymmetry: an AST census found 90 of 91 `_acquire` bodies share the idiom, and `mark_done`/`enqueue_ingress` were measured leaking identically — a two-method patch would have fixed an arbitrary slice. (2) `claim_fifo_heads` does **not** shield against this; its guard is a `SET LOCK_TIMEOUT` *reset* guard, [ADR 0114](adr/0114-phase-4-claim-path-call-complexity-reduction-driver-interface-redesign-ingress-routed-reset-fold.md) §2 states there is **no rollback** on its cancellation path, and `test_adr0114_claim_fold.py::test_ac3_cancellation_at_body_await_no_rollback_guard_runs` freezes that. It ends clean because the guard **commits**. + +**Not a data-integrity bug.** At-least-once was never at risk — a cancelled `release_claimed` leaves rows `INFLIGHT` and `reset_stale_inflight` re-pends them, which [`stage_dispatcher.py:491-492`](../messagefoundry/pipeline/stage_dispatcher.py) already declares the intended outcome. What was broken is pool integrity. + +**Backend scope: SQL Server only.** Postgres is safe twice over (asyncpg's `Transaction.__aexit__` rolls back on any `BaseException`; its pool also resets under `asyncio.shield`). SQLite shares the code shape but has one writer connection under an `asyncio.Lock` and no pool, so there is no next borrower. + +**Related:** ADR 0159, [ADR 0066](adr/0066-pooled-stage-claimers.md) §9, ADR 0114 §2, §1 of this file (the original H-6/H-7/H-8/M-6 concurrency-safety work — M-6 was scoped to `_fetchall`/bootstrap rollback hygiene and never covered the cancellation path). + +**Source:** secondary lead from a PR #138 CI diagnosis, 2026-08-02; confirmed by live reproduction rather than by the reasoning in the lead, two of whose premises proved false. diff --git a/docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md b/docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md new file mode 100644 index 00000000..9ecd5a29 --- /dev/null +++ b/docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md @@ -0,0 +1,197 @@ + + + +# ADR 0159 — Cancellation-safe pooled-connection release: quarantine at the `_acquire` chokepoint + +- **Status:** Accepted (2026-08-02) +- **Date:** 2026-08-02 +- **Related:** [BACKLOG #348](../BACKLOG.md) · [ADR 0066](0066-pooled-stage-claimers.md) §9 (the `SET LOCK_TIMEOUT 0` never-block claim, whose 1222→EMPTY translation is what made this silent) · [ADR 0114](0114-phase-4-claim-path-call-complexity-reduction-driver-interface-redesign-ingress-routed-reset-fold.md) §2 (the shielded finally-guard — **explicitly not** a rollback guard; see §3 below) · [ADR 0001](0001-staged-pipeline-architecture.md) (the staged queue whose at-least-once contract bounds the damage) + +--- + +## Context + +`SqlServerStore`'s house idiom for a write is: + +```python +async with self._acquire() as conn, self._cursor(conn) as cur: + try: + await cur.execute(...) + await self._commit(conn) + except Exception: + await conn.rollback() + raise +``` + +An AST census over `messagefoundry/store/sqlserver.py` finds **91** `self._acquire()` call sites, and in +**90** of them the `async with` body is a single top-level `try` whose only handler is `Exception`. This is +the dominant idiom of the file, not a slip at one or two sites. + +`asyncio.CancelledError` derives from `BaseException`, not `Exception` (Python 3.8+). So on a cancellation +**none of those rollbacks run**. The body unwinds with its transaction still open and its row locks still +held, and nothing downstream compensates: + +- `_cursor` (`sqlserver.py:2957`) closes only the cursor. Its docstring records that it *deliberately* + bypasses aioodbc's own cursor context manager **because** that manager would commit/rollback and would + "override each caller's own explicit `commit`/`rollback`". +- `_acquire` (`sqlserver.py:2891`) had no `try` at all — it applied the STORE-3 statement timeout and + yielded. +- aioodbc's pool does not reset. `Pool.release()` (0.5.0 `pool.py:196-205`) is `self._used.remove(conn)` + then, `if not conn.closed`, `self._free.append(conn)` — no commit, no rollback, no transaction-status + check. `_ContextManager.__aexit__` (`utils.py:90-103`) calls `_release_on_exception`, which + `Pool.acquire()` never supplies, so it defaults to the same `release` (`utils.py:60-62`): **the + cancellation path and the success path release identically.** `create_pool` is called with no + `pool_recycle` and no `after_created`, so the recycle branch is dead. + +The pool is `autocommit=False` (`sqlserver.py:2244-2249`), so the transaction is real. The next borrower +inherits it: its own commit durably commits the stranger's statements, its rollback discards them. + +### What was measured + +Against a live SQL Server 2022 container, cancelling a call mid-body and then inspecting the server: + +| Method | X/U row locks left on `queue` | +| --- | --- | +| `release_claimed` | **7** | +| `reschedule_claimed` | **7** | +| `mark_done` | **9** | +| `enqueue_ingress` | **11** | +| `claim_fifo_heads` (control) | **0** | + +The connection was back on the pool's free list (`size=1 freesize=1`), a raw writer against the locked row +got **error 1222**, and a real second `claim_fifo_heads` returned **EMPTY-all**. Under ADR 0066 §9 that 1222 +is translated to EMPTY-all by design — a *sanctioned* outcome — which is exactly why this never surfaced as +an error: **the failure mode is silence, not a stack trace.** + +**`@@TRANCOUNT` is not a usable discriminator here** and was nearly mistaken for one. Under ODBC +manual-commit a connection sits at `@@TRANCOUNT=1` with **zero** locks as its normal resting state (a fresh +empty transaction opens after each commit). The clean control reports `@@TRANCOUNT=1` too. Only **held X/U +row locks** distinguish poisoned from clean; a guard keyed on `@@TRANCOUNT` would report a leak on every +healthy connection and prove nothing. + +### Reachability + +`StageDispatcher.stop()` cancels the lane tasks (`stage_dispatcher.py:509-511`); `_run_lane` is the body +that awaits `reschedule_claimed` (:739) and `release_claimed` (:751), and both call sites are themselves +guarded `except Exception`, so the `CancelledError` propagates. A third site is +`wiring_runner.py:4266`. + +Two driving paths, and they differ in consequence: + +- **Full shutdown** — `engine.stop()` closes the store shortly after, so the poisoned connection is closed + at teardown. Bounded. +- **Loss of leadership** (`engine.py:1242-1252`, `_stop_graph`) — runs the identical cancel chain but + **does not close the store**. The pool stays live and shared with the coordinator and convergence loops, + so the poisoned connection sits in `_free` and is re-borrowed by unrelated callers. **This is the path + that bites.** + +## Decision + +Contain the poison at the **`_acquire` chokepoint**, where all 91 sites funnel, rather than at individual +methods. + +```python +try: + yield conn +except BaseException as exc: + if not isinstance(exc, Exception): + await self._release_dirty(conn) + raise +``` + +`_release_dirty` does two things, **in an order that is itself the guarantee**: + +1. **Synchronously** drop the driver handle — `conn._conn = None` — with **no await in front of it**. + aioodbc derives `Connection.closed` from `_conn` (`connection.py:89-93`) and `Pool.release()` re-adds a + connection only `if not conn.closed`, so this one attribute write makes it unlendable. Because it cannot + suspend, no cancellation can skip it. +2. **Then**, best-effort and time-boxed, close the raw handle off the event loop + (`asyncio.to_thread(raw.close)` under `wait_for(shield(...), _DIRTY_CLOSE_TIMEOUT)`). pyodbc's `close()` + rolls back uncommitted work per DBAPI, which is what actually frees the locks. + +`isinstance(exc, Exception)` is the discriminator: an ordinary error has **already** been rolled back by the +caller's own handler, so that path is left byte-identical and the connection is recycled as before. Only the +cancellation path — the one no handler saw — quarantines. + +### Why not a rollback in the same place + +The obvious fix, `await conn.rollback()` on the cancellation path, was **built and rejected on measurement**. +`Connection.rollback()` is `run_in_executor(self._executor, ...)` with `_executor is None`, i.e. the loop's +default thread pool, whose threads may still be occupied by the abandoned statement — bounded only by +`command_timeout` (default 30s). Nothing upstream bounds the wait: `stage_dispatcher.py:514` gathers with no +timeout and `_stop_graph` awaits `runner.stop()` with no timeout. Measured: a cancel returned in **1.005s** +against a 1.0s rollback, serialized across lanes. That trades a bounded, contract-legal row-level bleed for a +multi-second-to-minutes stall **on the demotion path**, which is the one case that matters most. + +A second defect killed the rollback draft outright: writing `await rb` on the `except CancelledError` arm +installs the rollback task as the outer task's `_fut_waiter`, so a **further** cancel cancels the rollback +itself — releasing the connection mid-transaction *and* with a rollback abandoned mid-flight, strictly worse +than today, while a single-cancel regression test stays green. The ordering rule in step 1 above exists +precisely to make that class of mistake unrepresentable, and the test suite pins it with an explicit +re-cancel arm. + +### 3. `claim_fifo_heads` is not the precedent it appears to be + +The lead that opened this investigation reasoned that `claim_fifo_heads` "already shields against precisely +this hazard". **It does not, and the record says so.** Its shielded finally is a `SET LOCK_TIMEOUT` *reset* +guard (the setting is session-scoped and would otherwise leak onto the next borrower). ADR 0114 §2 states +that on a cancellation at a body await "there is **no rollback**… This is **shipped** behavior", its +exit-path table row reads "**no rollback ran** on this path", and +`test_adr0114_claim_fold.py::test_ac3_cancellation_at_body_await_no_rollback_guard_runs` **freezes** it with +`assert "rollback" not in kinds`. + +`claim_fifo_heads` ends on a clean boundary because the guard **commits**, not because it rolls back — which +is why it measures 0 locks in the table above while its siblings measure 7-11. Copying "what +`claim_fifo_heads` does" would therefore have copied a guard that does not roll back. This section exists so +the next reader does not re-derive the wrong precedent from the same comment. + +## Consequences + +- **All 91 `_acquire` sites** are covered, including `enqueue_ingress` — the pre-ACK ingress commit, the + engine's hottest path — which the original two-method framing would have left leaking. +- **One reconnect per cancelled call.** The pool's `size` is derived (`freesize + len(_used) + _acquiring`), + so a dropped connection simply shrinks it and `_fill_free_pool` reopens on demand. Paid only on a path + that was previously corrupting the pool. +- **Shutdown/demotion stays bounded** by `_DIRTY_CLOSE_TIMEOUT` (5s), and on expiry the close completes + detached — the connection is already out of the pool, so expiry costs a slower reclaim and nothing else. +- **No behaviour change on the success or ordinary-error paths**, pinned by two control tests that pass + both before and after the change. +- **Not a data-integrity fix.** At-least-once was never at risk: a cancelled `release_claimed` leaves rows + `INFLIGHT` and `reset_stale_inflight` re-pends them, which `stage_dispatcher.py:491-492` already declares + the intended outcome. What is fixed is pool integrity and the silent EMPTY-all yield. +- **Backend scope: SQL Server only.** Postgres is structurally safe twice over — `async with + conn.transaction()` rolls back on any `BaseException` (asyncpg's `__aexit__` tests `extype is not None`, + with no `Exception` filter), and asyncpg's pool additionally resets under `asyncio.shield`. SQLite shares + the `except Exception` shape but has a single writer connection under an `asyncio.Lock` and no pool, so + there is no next-borrower to inherit anything. +- **Private-attribute coupling.** `conn._conn` is aioodbc-internal. This is pre-existing — `_acquire` + already reaches through it to apply the STORE-3 timeout — and aioodbc is hash-locked at 0.5.0, but a + version bump must re-check `Pool.release`'s `if not conn.closed` rule. + +## Acceptance Criteria + +- **AC-1** A cancellation delivered at any body await inside a pooled write leaves the connection + **unlendable** — verified as "not on the pool's free list", against a fake pool that mirrors aioodbc's + real `if not conn.closed` rule rather than an implementation detail. +- **AC-2** AC-1 holds under a **second** cancellation delivered during cleanup. +- **AC-3** An ordinary `Exception` still rolls back and **recycles** the connection (control: must pass + before and after, so AC-1 cannot be satisfied by blanket-discarding). +- **AC-4** The success path still commits and recycles, untouched. +- **AC-5** AC-1..AC-4 hold for `release_claimed`, `reschedule_claimed` **and** `mark_done` — a method the + original lead did not name — so the gate measures the chokepoint, not two patched call sites. +- **AC-6** ADR 0114's frozen no-rollback-on-cancellation test still passes unchanged. + +Verified: the gate failed 6/12 against unpatched code (both cancellation properties × all three methods) +with the four controls already green, and passes 12/12 after. On the live server the same cancellation now +leaves **0** locks, no open-transaction session, an unblocked independent writer, and a pool that dropped +the connection rather than re-lending it. + +## Options considered + +| Option | Verdict | +| --- | --- | +| **Quarantine at `_acquire` (chosen)** | Covers all 91 sites; sync containment is cancellation-proof; bounded cleanup | +| Patch `release_claimed` + `reschedule_claimed` only | **Rejected** — arbitrary slice; `mark_done` and `enqueue_ingress` were measured leaking identically | +| `await conn.rollback()` on the cancellation path | **Rejected** — unbounded await on the demotion path (measured 1.005s, capped only by `command_timeout`); and the `await rb` arm is defeated by a second cancellation | +| Widen the 90 bodies to `except BaseException` | **Rejected** — 90-site edit, each needing its own rollback semantics, with the same unbounded-await problem | +| Document only, fix nothing | **Rejected** — at-least-once holds, but pool poisoning on the demotion path is real and its symptom is silent | diff --git a/docs/adr/README.md b/docs/adr/README.md index 3ae66028..2c593ca5 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -183,3 +183,4 @@ what is withheld and what you can request. | [0154](0154-synchronous-captured-downstream-reply-and-intake-authentication-for-the-inbound-http-listener-adr-0023-deferred-tail.md) | **Synchronous captured-downstream-reply and intake authentication for the inbound HTTP listener** — the ADR 0023 deferred tail: `reply_from` blocks the HTTP turn on a **committed** ADR 0013 `response` row (never an in-flight `DeliveryResponse`), and `intake_auth` (API key / bearer / mTLS subject) adds a peer control behind a posture-keyed gate. Also closes a live hole — `check_http_tls_exposure` returns early on truthy `tls`, so an off-loopback `Http(tls=True)` listener authenticates nobody today | **Accepted (2026-07-31)** — owner-ratified at rev 5; authorises **increment A only** (intake-auth + peer-control gate), which is **built and merged** (2026-08-01, `f2ef0ea9`); sync-reply (increment B) deferred pending a customer | | [0155](0155-dast-dynamic-security-testing-of-the-running-engine.md) | **DAST — dynamic security testing of the running engine** (BACKLOG #318) — no DAST had ever run against this project: every security test was static or in-process, leaving the [Secure_Development_Standards](../Secure_Development_Standards.md) §6.1 *Dynamic* tier row empty. Increment 1 builds a **self-run, authenticated authorization sweep** with **no new dependency**: one `uvicorn` listener on loopback in front of a real Engine + real AuthService, both identities minted over the wire through `POST /auth/login`, and the authorization expectation **derived from the live route table** by a single shared `require*()`-closure walk ([`scripts/security/route_gates.py`](../../scripts/security/route_gates.py), hoisted out of the security doc-drift guard so exactly one derivation of *is this route gated* exists in the tree) rather than a hand-kept list that goes stale the day a route lands. Measured shape: 105 route rows, 100 gated, 87 permission-gated, exactly 5 anonymous. Three passes — **negative** (every gated HTTP row sent with no credential and with an invalid bearer; anything but 401 is a finding), **authorized reach** (how many gated `GET` rows a *privileged* token got past authentication and authorization on — the positive number that stops a wall of 401s reading as *all endpoints protected*), and **viewer BFLA** (anything but a refusal, **including 404**, is a finding, because a 404 on a matched path template means the caller got past authorization into resource lookup). The receipt names what it examined and **fails closed** — below any floor it exits **2 (could not measure)**, never 0 — and records method, path template, status codes, counts and the *relaxed* posture it scanned, never a body, header or token. Two canaries are built from **supported configuration, not source patches** (authentication disabled at the target; the low-privilege identity over-granted while the expectation set stays the viewer's), avoiding the patch-rot failure where a canary silently stops applying; CI runs both **before** the real scan and requires each to exit exactly **1** (findings) *with* its receipt on disk — a 0 (blind), a 2 (could not measure, which is what a neutered canary actually produces) or a crash fails the job and the real scan never runs. The inversion is the design: the sweep and both canaries run as ordinary pytest in the **existing required** test legs, so a change that blinds the detector reds a PR, while the nightly workflow is advisory — **not** a required context (it has no `pull_request` trigger, so it cannot report on a PR) and deliberately **not** `continue-on-error`. **Scope boundary: see the ADR's *Scope boundary* section** — it is stated once there, verbatim, and this row deliberately carries a pointer and no wording of its own. Rejected/deferred: schemathesis (the shipped OpenAPI declares no `securitySchemes` and no per-operation `security`, so `ignored_auth` would pass on every operation having probed none of them; adoption also needs a fifth DEP-1 lock over a ~30-distribution closure that could pull a shipped runtime floor down), ZAP (a SHA-pinned action still pulls a mutable `:stable` image, and the published images do not install the web console), nuclei (template matcher; `pip install nuclei` is an abandoned unrelated package), Dredd/RESTler/CATS, in-process ASGI transport (bypasses the HTTP parser, leaves `request.client` unset), the unauthenticated MLLP/TCP/X12/DICOM ingress plane, the `/ui` console plane, a TLS black-box target, and non-`GET` reach/BFLA | Accepted (2026-07-31) — increment 1 built; advisory, not a required context | | [0156](0156-asvs-scorecard-as-data-a-derived-count-verified-evidence-anchors-and-a-fail-closed-drift-gate.md) | **ASVS scorecard as data — a derived count, verified evidence anchors, and a fail-closed drift gate** — the ASVS score is maintained as prose, and nothing checks it. One re-anchoring session (2026-08-01) re-derived the headline count **6 times**, found **12 residuals of record factually false at HEAD** (five of them *absence* claims that had silently stopped being true), and found **10 cells missing from an enumeration described as "arithmetic-checked and complete"** — which survived because the arithmetic closed to 345 and closure was read as proof. **Closure only proves the four buckets sum, not that every cell landed in one.** Decision: hold one `[[cell]]` record per requirement (all 345) in `asvs-scorecard.toml`; **compute** the count so no document can state one; assert **every corpus id appears exactly once** (the check whose absence cost ten cells); machine-verify each cell's `evidence` anchor by asserting an expected **token** still resolves, so code movement reds a test instead of rotting a sentence; require an absence claim to record the **search that proved it plus a positive control that must still hit**, because a grep naming the wrong token returns zero and reads exactly like proof; make **`unverified` a first-class verdict** so inherited-versus-verified Pass is countable (~219 Passes have never been read against the requirement text); and **fail closed rather than skip**. Tool + schema + fixture tests live in this repo and run in public CI; the real scorecard lives in the vault with a vault-CI job — which closes **ASVS 15.1.3**, currently open precisely because six `*_doc_drift` modules assert against documents that `git ls-files docs/security/` shows are **not present** in the tree where CI runs. Rejected: *keep prose and review harder* (every false residual **read as true**; the project's own standard says the mitigation must be structural, not diligence), *one document to rule them all* (that is the current lineage, and it produces five documents asserting three counts), and *publish the vault documents so the guards see them* (attacker roadmap, `SECURITY-DOCS-POLICY.md`). Explicitly **does not** make the score correct — only consistent, derived and drift-detecting; adversarial verification remains the only cure for a wrong verdict | **Accepted (2026-08-01)** — built and merged the same day. **§7 was amended at ratification**: it proposed a vault CI job, and the actions API showed every vault workflow `disabled_manually` (last run 2026-07-27; two vault PRs merged that day with zero checks), so a CI-only design would have shipped dead. Built instead as a vault **pre-commit hook** plus one **narrow new workflow**. That §7 was a confident, unchecked claim about system state is the ADR own thesis applied to itself | +| [0159](0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md) | **Cancellation-safe pooled-connection release — quarantine at the `_acquire` chokepoint** (BACKLOG #348) — `SqlServerStore`'s write idiom is `except Exception: await conn.rollback(); raise`, used at **90 of the 91** `self._acquire()` sites. `CancelledError` derives from `BaseException`, so on a cancellation **no rollback runs**, and aioodbc does not compensate: `Pool.release()` appends a non-closed connection straight back onto the free deque with no rollback, reset or transaction check (0.5.0 `pool.py:196-205`), and `_ContextManager.__aexit__` uses the *same* `release` on the exception path (`utils.py:60-62`). The next borrower inherits an open transaction still holding X locks. **Measured on a live SQL Server**: cancelling `release_claimed` left **7** X locks on `queue`, `reschedule_claimed` 7, `mark_done` 9, `enqueue_ingress` 11 (the pre-ACK ingress commit), against **0** for the `claim_fifo_heads` control; the connection returned to the free list, a raw writer got **1222**, and a real second claim yielded **EMPTY-all** — which ADR 0066 §9 sanctions, so the symptom is *silence*, not an error. Fix: at `_acquire`, on a non-`Exception` `BaseException` only, **synchronously** drop the driver handle (`conn._conn = None` — aioodbc derives `closed` from it and re-adds only `if not conn.closed`) **with no await in front of it**, then close the raw handle off-loop under a 5s bound. The ordering *is* the guarantee: a cleanup that awaits first is defeated by the **second** cancellation that shutdown's cancel-then-gather delivers. A plain `await conn.rollback()` was built and **rejected on measurement** — it runs in the default executor bounded only by `command_timeout` (30s) with no upstream timeout, and a cancel measured **1.005s** against a 1.0s rollback, stalling exactly the **demotion** path (`_stop_graph` cancels but does **not** close the store, so the poisoned connection is re-borrowed there). Corrects the lead that found it on two points: it is **not** a two-method asymmetry, and `claim_fifo_heads` does **not** shield against it — its guard is a `SET LOCK_TIMEOUT` *reset* guard and ADR 0114 §2 plus a frozen test record that **no rollback** runs on its cancellation path; it ends clean because the guard **commits**. Ordinary errors keep today's rollback-and-recycle behaviour, pinned by controls that pass before and after. SQL Server only — Postgres is safe twice over (asyncpg rolls back on any `BaseException` and its pool resets under `shield`), SQLite has no pool | Accepted (2026-08-02) — built and verified the same day; gate failed 6/12 pre-fix, passes 12/12 post-fix, live repro 7→0 locks | diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 924d8b79..8efa7637 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -136,6 +136,14 @@ # transaction, so the all-or-nothing recovery pass is unchanged. _RESET_LANE_CHUNK = 500 +# BACKLOG #348 / ADR 0159: how long the quarantine of a cancellation-poisoned pooled connection waits +# for its off-loop close before giving up and leaving it to finish detached. A BOUND, not a deadline: +# the connection is already out of the pool before this wait starts, so expiring it costs nothing but +# a slower reclaim. It exists because the close runs on a worker thread that may still be occupied by +# the abandoned statement, which is bounded only by command_timeout (default 30s) — without a cap here +# an engine.stop()/demotion would block for that long, per lane. +_DIRTY_CLOSE_TIMEOUT = 5.0 + # SQL Server native error 1222 = "Lock request time out period exceeded" — raised by SET LOCK_TIMEOUT 0 # in the pooled claim (ADR 0066 §9) when a probe cannot IMMEDIATELY acquire a contended head lock. It is # the normal "head is contended, yield" signal, not an error, so it maps to the EMPTY-all fail-closed @@ -2909,7 +2917,60 @@ async def _acquire(self) -> AsyncIterator[Any]: raw = getattr(conn, "_conn", None) if raw is not None: raw.timeout = self._settings.command_timeout # seconds; 0 = no limit - yield conn + try: + yield conn + except BaseException as exc: + # BACKLOG #348 / ADR 0159. Every caller's own handler is `except Exception` (90 of + # the 91 _acquire sites), so an ordinary error has ALREADY rolled back by the time it + # reaches here — leave that path exactly as it was, connection recycled. A + # CancelledError derives from BaseException, so NONE of those handlers ran: the body + # is unwinding with its transaction still open and its X locks still held, and + # aioodbc's pool does not compensate (`Pool.release()` appends a non-closed + # connection straight back onto the free deque — 0.5.0 pool.py:196-205 — and + # `_ContextManager.__aexit__` releases identically on the exception path). Quarantine + # it so the next borrower can never inherit it. + if not isinstance(exc, Exception): + await self._release_dirty(conn) + raise + + async def _release_dirty(self, conn: Any) -> None: + """Quarantine a pooled connection whose transaction was abandoned by a cancellation, so it + can never be lent to another caller (BACKLOG #348, ADR 0159). + + **The load-bearing line is the synchronous one.** aioodbc derives ``Connection.closed`` from + ``self._conn`` (0.5.0 connection.py:89-93) and ``Pool.release()`` re-adds a connection to the + free deque only ``if not conn.closed`` (pool.py:200-204) — so dropping the handle is a plain + attribute write that makes the connection unlendable with **no await in front of it**. That + matters: shutdown cancels the lane task and the gather can cancel it AGAIN, so any cleanup + that awaits *before* containing the poison is defeated by the second cancellation and + silently restores the bug. Ordering here is the guarantee; the close below is only hygiene. + + Closing the raw handle is then best-effort, off the event loop, and time-boxed. pyodbc's + ``close()`` rolls back uncommitted work (DBAPI), which is what actually frees the X locks — + but it runs on a worker thread that may still hold the abandoned statement, so it is never + awaited unbounded on a shutdown/demotion path. On expiry the close finishes detached; the + pool has already lost the connection and reopens on demand (``size`` is derived, so the pool + simply shrinks). This costs one reconnect per cancelled call — paid only on a path that was + previously corrupting the pool. + """ + raw = getattr(conn, "_conn", None) + if raw is None: # already closed/quarantined — nothing lendable to contain + return + conn._conn = None # ← MUST stay first, and MUST stay await-free + closer = asyncio.ensure_future(asyncio.to_thread(raw.close)) + try: + await asyncio.wait_for(asyncio.shield(closer), _DIRTY_CLOSE_TIMEOUT) + except (TimeoutError, asyncio.CancelledError): + # Expired, or a further cancellation landed while we waited. Either way the connection is + # already out of the pool; let the close land on its own. Swallowed deliberately — the + # caller re-raises the ORIGINAL cancellation, which is the outcome that must propagate. + log.debug( + "sqlserver: quarantined connection close did not complete within %.1fs; it will" + " finish detached (the connection is already out of the pool)", + _DIRTY_CLOSE_TIMEOUT, + ) + except Exception: # noqa: BLE001 - a close failure must not mask the cancellation + log.debug("sqlserver: quarantined connection close failed", exc_info=True) def pool_status(self) -> PoolStatus | None: """The aioodbc pool snapshot (B11): size/idle occupancy + the PRIMARY acquire-wait percentiles. diff --git a/tests/test_backlog348_cancel_dirty_release.py b/tests/test_backlog348_cancel_dirty_release.py new file mode 100644 index 00000000..ccdc2797 --- /dev/null +++ b/tests/test_backlog348_cancel_dirty_release.py @@ -0,0 +1,264 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""BACKLOG #348 / ADR 0159 — a cancelled store call must never hand the next borrower a pooled SQL +Server connection that is still MID-TRANSACTION. + +``asyncio.CancelledError`` derives from ``BaseException``, so the store's house idiom +(``except Exception: await conn.rollback(); raise`` — 90 of the 91 ``_acquire()`` sites) never runs +its rollback on a cancellation. Nothing downstream compensates: aioodbc's ``Pool.release()`` appends +the connection straight back onto the free deque with no rollback, no reset and no transaction check +(aioodbc 0.5.0 ``pool.py:196-205``), and ``_ContextManager.__aexit__`` releases identically on the +exception path (``utils.py:60-62``, ``:90-103``). The next borrower then inherits an open transaction +still holding X locks on ``queue`` rows; a claimer that blocks on them under ``SET LOCK_TIMEOUT 0`` +(ADR 0066 §9) raises error 1222, which the claim path translates into a *silent* EMPTY-all yield. + +Measured against a live SQL Server BEFORE the fix: cancelling ``release_claimed`` left 7 KEY X locks +on ``queue``, the connection went back onto the pool's free list, an independent ``claim_fifo_heads`` +returned EMPTY-all, and a raw writer got 1222. ``mark_done`` (9 locks) and ``enqueue_ingress`` (11) +leaked identically — this is the file's dominant idiom, not a two-method slip. + +NOT a claim_fifo_heads analogue. That method's shielded finally is a ``SET LOCK_TIMEOUT`` *reset* +guard, not a rollback guard: ADR 0114 §2 states there is **no rollback** on its cancellation path and +``test_adr0114_claim_fold.py::test_ac3_cancellation_at_body_await_no_rollback_guard_runs`` freezes +that. It ends on a clean boundary only because the guard COMMITS. + +The fake pool below mirrors aioodbc's REAL release rule (``if not conn.closed: free.append(conn)``), +so these tests assert the contract that actually matters — *can the next borrower be handed this +connection?* — rather than an implementation detail of how it was made safe. +""" + +from __future__ import annotations + +import asyncio +import types +from contextlib import asynccontextmanager +from typing import Any + +import pytest + +from messagefoundry.store.pool_metrics import AcquireWaitHistogram +from messagefoundry.store.sqlserver import SqlServerStore + +# The three methods are deliberately NOT the two the original lead named: mark_done is included to +# pin that the guarantee is a property of the _acquire chokepoint, not of two patched call sites. +_METHODS: list[tuple[str, dict[str, Any]]] = [ + ("release_claimed", {"ids": ["row-1"], "now": 100.0}), + ("reschedule_claimed", {"ids": ["row-1"], "next_attempt_at": 900.0, "now": 100.0}), + ("mark_done", {"outbox_id": "row-1", "now": 100.0}), +] + + +class _RawConn: + """Stands in for the underlying ``pyodbc.Connection`` that aioodbc wraps as ``conn._conn``. + + ``close()`` is synchronous and blocking, exactly like pyodbc's — the store must therefore not + perform it inline on the event loop. + """ + + def __init__(self, ops: list[str]) -> None: + self.ops = ops + self.timeout = 0 + self.closed = False + self.release_close = None # type: Any + + def close(self) -> None: + if self.release_close is not None: + self.release_close.wait() # a threading.Event — hold the close open mid-flight + self.ops.append("raw.close") + self.closed = True + + +class _FakeConn: + """An aioodbc-shaped connection. ``closed`` is derived from ``_conn`` exactly as aioodbc's is.""" + + def __init__(self, ops: list[str], *, gate: asyncio.Event | None = None) -> None: + self.ops = ops + self._conn: _RawConn | None = _RawConn(ops) + self._gate = gate + self.cursor_obj = _GatedCursor(ops, gate) + + @property + def closed(self) -> bool: + return self._conn is None + + async def cursor(self) -> _GatedCursor: + return self.cursor_obj + + async def commit(self) -> None: + self.ops.append("commit") + + async def rollback(self) -> None: + self.ops.append("rollback") + + +class _GatedCursor: + def __init__(self, ops: list[str], gate: asyncio.Event | None) -> None: + self.ops = ops + self._gate = gate + self.description = None + + async def execute(self, sql: str, params: object = None) -> None: + self.ops.append("execute") + if self._gate is not None: + await self._gate.wait() # suspend inside the method body; the test cancels here + + async def fetchall(self) -> list[object]: + return [] + + async def fetchone(self) -> object | None: + # mark_done SELECTs first; None makes it commit-and-return without the event/finalize path. + return None + + async def close(self) -> None: + self.ops.append("cursor.close") + + +class _FakePool: + """aioodbc's pool semantics, verbatim on the point that matters: a released connection rejoins + the FREE list — and so becomes lendable to the next borrower — only when it is not ``closed`` + (aioodbc 0.5.0 ``pool.py:200-204``).""" + + def __init__(self, conn: _FakeConn, ops: list[str]) -> None: + self._conn = conn + self.ops = ops + self.free: list[_FakeConn] = [] + + def acquire(self) -> Any: + conn = self._conn + ops = self.ops + free = self.free + + @asynccontextmanager + async def _cm() -> Any: + try: + yield conn + finally: + ops.append("release") + if not conn.closed: + free.append(conn) # lendable again + + return _cm() + + +def _make_store(conn: _FakeConn, ops: list[str]) -> SqlServerStore: + store = SqlServerStore.__new__(SqlServerStore) + store._pool = _FakePool(conn, ops) # type: ignore[assignment] + store._settings = types.SimpleNamespace(command_timeout=0) # type: ignore[assignment] + store._acquire_wait = AcquireWaitHistogram() + store.committed_txns = 0 + store.body_copies = 0 + return store + + +async def _cancel_inside( + store: SqlServerStore, method: str, kwargs: dict[str, Any] +) -> asyncio.Task: + """Drive ``method`` until it suspends inside its body, then cancel it there.""" + task: asyncio.Task = asyncio.create_task(getattr(store, method)(**kwargs)) + for _ in range(200): # let the task reach the gated execute + await asyncio.sleep(0) + if "execute" in store._pool.ops: # type: ignore[attr-defined] + break + assert "execute" in store._pool.ops, "never reached the gated execute" # type: ignore[attr-defined] + task.cancel() + return task + + +@pytest.mark.parametrize(("method", "kwargs"), _METHODS, ids=[m for m, _ in _METHODS]) +async def test_cancelled_call_never_returns_a_dirty_connection_to_the_pool( + method: str, kwargs: dict[str, Any] +) -> None: + """THE regression gate. Unpatched this fails on every arm: the body's ``except Exception`` does + not see CancelledError, so no rollback runs and the connection rejoins the free list holding its + X locks.""" + ops: list[str] = [] + conn = _FakeConn(ops, gate=asyncio.Event()) + store = _make_store(conn, ops) + pool = store._pool # type: ignore[attr-defined] + + task = await _cancel_inside(store, method, kwargs) + with pytest.raises(asyncio.CancelledError): + await task + + assert pool.free == [], ( + f"{method}: a cancelled call returned the connection to the pool's free list while its" + f" transaction was still open — the next borrower inherits its X locks (ops={ops})" + ) + assert conn.closed, f"{method}: the poisoned connection was not quarantined (ops={ops})" + + +@pytest.mark.parametrize(("method", "kwargs"), _METHODS, ids=[m for m, _ in _METHODS]) +async def test_second_cancellation_cannot_defeat_the_quarantine( + method: str, kwargs: dict[str, Any] +) -> None: + """A cleanup that *awaits* before making the connection unlendable is defeated by a second + cancellation — shutdown cancels, then the gather cancels again — and would silently restore the + original bug while the single-cancel gate above stayed green. So the quarantine must be + established with NO await in front of it.""" + import threading + + ops: list[str] = [] + conn = _FakeConn(ops, gate=asyncio.Event()) + assert conn._conn is not None + hold = threading.Event() + conn._conn.release_close = hold # block the raw close mid-flight + store = _make_store(conn, ops) + pool = store._pool # type: ignore[attr-defined] + + task = await _cancel_inside(store, method, kwargs) + for _ in range(50): # let the cleanup start while the raw close is held open + await asyncio.sleep(0) + task.cancel() # the SECOND cancellation, landing during cleanup + hold.set() + with pytest.raises(asyncio.CancelledError): + await task + + assert pool.free == [], ( + f"{method}: a second cancellation let the connection back into the pool mid-transaction" + f" (ops={ops})" + ) + + +@pytest.mark.parametrize(("method", "kwargs"), _METHODS, ids=[m for m, _ in _METHODS]) +async def test_ordinary_exception_still_rolls_back_and_recycles( + method: str, kwargs: dict[str, Any] +) -> None: + """The non-vacuous control. An ordinary error must keep today's behaviour — the body rolls back + and the connection is RECYCLED, not discarded — so the gate above cannot be satisfied by + blanket-discarding on every exit.""" + ops: list[str] = [] + conn = _FakeConn(ops, gate=None) + store = _make_store(conn, ops) + pool = store._pool # type: ignore[attr-defined] + + async def boom(sql: str, params: object = None) -> None: + ops.append("execute") + raise RuntimeError("execute boom") + + conn.cursor_obj.execute = boom # type: ignore[assignment] + + with pytest.raises(RuntimeError, match="execute boom"): + await getattr(store, method)(**kwargs) + + assert "rollback" in ops, f"{method}: the ordinary-error rollback regressed (ops={ops})" + assert pool.free == [conn], ( + f"{method}: an ordinary error must not discard the connection — only a cancellation, which" + f" leaves it mid-transaction, does (ops={ops})" + ) + assert not conn.closed + + +@pytest.mark.parametrize(("method", "kwargs"), _METHODS, ids=[m for m, _ in _METHODS]) +async def test_success_path_unchanged(method: str, kwargs: dict[str, Any]) -> None: + """The happy path must still commit and recycle the connection untouched.""" + ops: list[str] = [] + conn = _FakeConn(ops, gate=None) + store = _make_store(conn, ops) + pool = store._pool # type: ignore[attr-defined] + + await getattr(store, method)(**kwargs) + + assert "commit" in ops + assert "rollback" not in ops + assert pool.free == [conn] + assert not conn.closed From d8ea1a0f1acd383a583eec61b71e8b44f1ec4cf0 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 11:57:47 -0500 Subject: [PATCH 2/2] docs(#348): record where this meets #344 instance 2, and what it does NOT claim Both items independently reached the same 1222. #344 instance 2 traces the far end -- contended head -> native 1222 -> the store swallows it as a normal EMPTY -> the dispatcher goes to phase IDLE with no timer armed -- and correctly concludes that is a TEST-RIG GAP, not an engine defect, because production's periodic sweep re-readies such a lane and the ADR 0070 tests disable that sweep on purpose. Nothing in #348/ADR 0159 contradicts that, and this commit deliberately does NOT escalate #348's severity on the strength of it. What it adds is a DURATION PROFILE: #344 assumes momentary producer contention, whereas a connection poisoned by #348 holds its queue X locks for as long as it sits unclaimed in the pool's free deque, so the 1222 it manufactures can repeat across successive sweep ticks rather than clearing on the next one. Production still recovers; the mechanism just supplies a PERSISTENT contention source where a momentary one was assumed. Cited by ledger number, not SHA: that work is unpushed and may be rebased. (A peer message named 66443098 for it; that SHA is a one-line #344 banner edit touching no code. The work is on claude/keen-mclaren-31648a.) Docs only -- no code, no test, no behaviour change. --- docs/BACKLOG.md | 2 ++ ...e-mid-txn-discard-at-the-acquire-chokepoint.md | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 7be085f8..2a701b68 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8446,4 +8446,6 @@ Two worked instances the same day. **#74** went green on 2026-07-30 and sat unme **Related:** ADR 0159, [ADR 0066](adr/0066-pooled-stage-claimers.md) §9, ADR 0114 §2, §1 of this file (the original H-6/H-7/H-8/M-6 concurrency-safety work — M-6 was scoped to `_fetchall`/bootstrap rollback hygiene and never covered the cancellation path). +**Meets #344 instance 2 at the 1222.** That item (found independently and concurrently) traces the far end of this same chain: a contended head raises 1222, the store swallows it as a normal EMPTY, and the dispatcher goes to phase IDLE with no timer armed — **a test-rig gap, not an engine defect**, since production's periodic sweep re-readies such a lane and the ADR 0070 tests disable that sweep deliberately. Nothing here contradicts that and this item's severity is **not** escalated on it. What this adds is a **duration profile**: #344 assumes momentary producer contention, whereas a connection poisoned by *this* defect holds its `queue` X locks for as long as it sits unclaimed in the pool's free deque, so the 1222 can repeat across successive sweep ticks instead of clearing on the next. Cited by ledger number, not SHA — that branch is unpushed and may be rebased. + **Source:** secondary lead from a PR #138 CI diagnosis, 2026-08-02; confirmed by live reproduction rather than by the reasoning in the lead, two of whose premises proved false. diff --git a/docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md b/docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md index 9ecd5a29..f2857cad 100644 --- a/docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md +++ b/docs/adr/0159-cancellation-safe-pooled-connection-release-mid-txn-discard-at-the-acquire-chokepoint.md @@ -164,6 +164,21 @@ the next reader does not re-derive the wrong precedent from the same comment. with no `Exception` filter), and asyncpg's pool additionally resets under `asyncio.shield`. SQLite shares the `except Exception` shape but has a single writer connection under an `asyncio.Lock` and no pool, so there is no next-borrower to inherit anything. +- **A new *source* for a 1222 that was assumed to come only from producer contention** (BACKLOG #344 + instance 2, found independently and concurrently). That work traced the other end of this same chain: + a contended head raises 1222, the store swallows it as a normal EMPTY (the `_is_lock_timeout` branch), + and the dispatcher's EMPTY branch goes to phase IDLE with **no timer armed**. It correctly concludes + that this is a **test-rig gap, not an engine defect**, because production's periodic sweep re-readies + exactly such a lane — the ADR 0070 tests disable that sweep on purpose, which is what makes IDLE + terminal *there*. **Nothing in this ADR contradicts that**, and the severity above is deliberately not + escalated on the strength of it. + + The connection worth recording is the **duration profile**. That analysis assumes the contention is + momentary — a producer holding a head lock in flight. A connection poisoned by this defect holds its + `queue` X locks for as long as it sits unclaimed in the pool's free deque, so the 1222 it manufactures + can repeat across successive sweep ticks rather than clearing on the next one. Production still + recovers, but the mechanism supplies a *persistent* contention source where a momentary one was + assumed. Referenced by ledger number, not by SHA — that branch is unpushed and may be rebased. - **Private-attribute coupling.** `conn._conn` is aioodbc-internal. This is pre-existing — `_acquire` already reaches through it to apply the STORE-3 timeout — and aioodbc is hash-locked at 0.5.0, but a version bump must re-check `Pool.release`'s `if not conn.closed` rule.