Skip to content

fix(transports): close DATABASE cursors before releasing the connection (BACKLOG #1104) - #294

Merged
wshallwshall merged 2 commits into
mainfrom
claude/1104-database-cursor-close
Aug 8, 2026
Merged

fix(transports): close DATABASE cursors before releasing the connection (BACKLOG #1104)#294
wshallwshall merged 2 commits into
mainfrom
claude/1104-database-cursor-close

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

Fixes BACKLOG #1104, found and fixed in the same pass. Reproduced against a real SQL Server 2022 container, not inferred.

The defect

messagefoundry/transports/database.py opened a cursor at five sites and closed it at nonecur.close() appeared nowhere in the file. aioodbc/pyodbc keep the ODBC statement handle open until the cursor is closed, so every one of those connections went back to the pool busy, and the next caller's first command failed with:

HY000 Connection is busy with results for another command

This is delivery semantics, not tidiness. An UPDATE leaves a row count pending, so the DATABASE source's mark is the usual victim — and _poll_once treats a failed mark as at-least-once: the row is left unmarked and re-emitted as a duplicate. Observed on main:

DATABASE source mark failed (row will re-emit, a duplicate): ('HY000', ...)
FAILED tests/test_database_source_integration.py::test_source_polls_and_marks_rows
assert [(1, 1)] == [(0, 2)]      # 1 row left unmarked -> it re-emits

Per CLAUDE.md §0, stated in the conditional: a deploying site running a DATABASE source against SQL Server would see duplicates, at a rate set by pool reuse.

Why it survived

The error lands on the innocent statement. The command that fails is not the one that left the handle open — it's whatever next draws that connection. Triaging the reported statement leads nowhere; the cause is one checkout earlier. That's also why the fix covers all five sites rather than only the one seen to fail.

And CI could not catch it on main. The sql server (store + connector) leg is gated on server-DB/docker path changes, so it is skipped on every recent main push — measured across the last five. It runs only on PRs touching those paths. A real defect sat on main while the leg that detects it stayed green-by-absence. That is the #1000 shape at workflow level, and this PR does not address it — the leg's main coverage is a separate question.

The evidence, and which half is weak

The integration numbers prove nothing on their own, and are not offered as proof. On the container: 1 failure in 10 runs unfixed, 0 in 10 fixed. At a ~10% base rate that difference is well inside chance. It is recorded as the reproduction that found the defect.

The proof is tests/test_database_cursor_close.py, which asserts close-before-release deterministically against a fake pool that records operation order — and which was verified to go RED on a mutant with the closes removed:

result
closes removed from _select and _mark 2 failed, 1 passed
restored 3 passed

The one that kept passing covers _close_cursor's own contract and correctly does not depend on the call sites. A guard with a 10% detection rate is not a guard.

_close_cursor never raises: a close failure must not mask the caller's real error, nor skip the release that follows — leaking a pooled connection to save a cursor is the worse trade.

Verification

  • ruff, ruff format, mypy strict (263 files) — clean
  • 236 tests pass against a live SQL Server 2022 container, including the previously-failing test_source_polls_and_marks_rows
  • backlog hygiene 19 passed; repo-wide link gate 5,359 links / 347 files, all resolve

On the container check. A native MSSQLSERVER service is also running on this machine, and my notes warn it can steal 1433 so the container looks healthy while pyodbc reaches a different server. Confirmed it did not: both listeners on 1433 were Docker processes, and SERVERPROPERTY('MachineName') returned the container's own hostname fa5c1fc5c648. Established by asking the server, not by reading the port table.

Provenance

Found while triaging PR #253's red SQL Server leg. #253 was exonerated by measurement — the same test fails identically on main — after first being exonerated by mechanism (that step runs an explicit path list, so testpaths cannot reach it). Its two reds were two different unrelated failures, which is why "it failed twice, so it's real" would have been the wrong read.

BACKLOG #1104. database.py opened a cursor at five sites and closed it at none
-- cur.close() appeared nowhere in the file. aioodbc/pyodbc keep the ODBC
statement handle open until the cursor is closed, so each of those connections
returned to the pool BUSY and the next caller's first command failed with

  HY000 Connection is busy with results for another command

This is delivery semantics, not tidiness. An UPDATE leaves a row count pending,
so the DATABASE source's mark is the usual victim, and _poll_once treats a failed
mark as at-least-once: the row is left unmarked and RE-EMITTED AS A DUPLICATE.

Observed on main against a real SQL Server 2022 container:

  DATABASE source mark failed (row will re-emit, a duplicate): ('HY000', ...)
  FAILED tests/test_database_source_integration.py::test_source_polls_and_marks_rows
  assert [(1, 1)] == [(0, 2)]

THE ERROR APPEARS ON THE INNOCENT STATEMENT. The command that fails is not the
one that left the handle open; it is whatever next draws that connection. That
misdirection is why this survived, and it is why the fix is at all five sites
rather than only the one that was seen to fail.

_close_cursor never raises: a close failure must not mask the caller's real error
and must not skip the release that follows it, because leaking a pooled
connection to save a cursor is the worse trade.

EVIDENCE, and the integration half of it is weak on its own. On the container the
unfixed tree failed 1 run in 10 and the fixed tree 0 in 10 -- at a ~10% base rate
that is well inside chance and proves nothing. It is recorded as the reproduction
that FOUND the defect, not as proof it is fixed. The proof is
tests/test_database_cursor_close.py, which asserts close-before-release
deterministically against a fake pool, and which was verified to go RED on a
mutant with the closes removed (2 of 3 failed; the third covers _close_cursor's
own contract and correctly did not).

Verified: ruff, ruff format, mypy strict (263 files) all clean; 236 tests pass
against a live SQL Server 2022 container, including the previously-failing
test_source_polls_and_marks_rows; backlog hygiene 19 passed; the repo-wide link
gate reports 5,359 links across 347 files all resolving.

Found while triaging PR #253's red SQL Server leg. #253 was exonerated by
measurement -- the same test fails identically on main -- after first being
exonerated by mechanism. Verified against the container only after confirming the
host reaches it and not the native MSSQLSERVER service also running on this box:
both listeners on 1433 were Docker processes and SERVERPROPERTY('MachineName')
returned the container's own hostname.
@wshallwshall
wshallwshall enabled auto-merge (squash) August 8, 2026 21:37
@wshallwshall
wshallwshall merged commit 89dcdaf into main Aug 8, 2026
35 checks passed
@wshallwshall
wshallwshall deleted the claude/1104-database-cursor-close branch August 8, 2026 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant