fix(store): a cancelled call returned a pooled SQL Server connection mid-transaction (BACKLOG #348, ADR 0159) - #154
Open
wshallwshall wants to merge 5 commits into
Open
fix(store): a cancelled call returned a pooled SQL Server connection mid-transaction (BACKLOG #348, ADR 0159)#154wshallwshall wants to merge 5 commits into
wshallwshall wants to merge 5 commits into
Conversation
…mid-transaction (BACKLOG #348, ADR 0159) 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).
… 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 6644309 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.
…af0254 # Conflicts: # docs/BACKLOG.md
wshallwshall
enabled auto-merge (squash)
August 2, 2026 18:29
…af0254 # Conflicts: # docs/BACKLOG.md # docs/adr/README.md
…af0254 # Conflicts: # docs/adr/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CancelledErroris aBaseException, so theexcept Exception: rollback()idiom never fires. The pooled connection goes back to the free deque still holding its locks — and holds them for as long as it sits there unclaimed.Measured on a live SQL Server, locks still held on
queueafter a cancelled call:release_claimedreschedule_claimedmark_doneenqueue_ingress(the pre-ACK ingress commit)claim_fifo_heads(control)A subsequent real
claim_fifo_headsthen returned EMPTY-all and a raw writer took native 1222.In-source gap independently confirmed by the BACKLOG #344 session: 90 rollback sites guarded by
except Exception, zero usingBaseExceptionorfinally.How this relates to #344 instance 2 (PR #153), stated precisely because it would be easy to overclaim: this is a new source of that same 1222 chain. #344's analysis assumes momentary producer contention; a poisoned connection makes the contention persistent — the 1222 can repeat across successive sweep ticks instead of clearing on the next. Production still recovers. Neither session escalated severity on the other's finding, and this PR does not either.
Fix is a
_release_dirtyplus atryaround_acquire's yield — the acquire chokepoint, so it covers every call site rather than 90 of them.Authored by the
upbeat-mendelsession; pushed and opened by the coordinator at the owner's instruction, so pushes are serialised through one place. Merge remains the owner's call.🤖 Generated with Claude Code