Skip to content

Four High-severity data-loss / correctness bugs under SQLAlchemy 2.x (patch and regression suite available) #236

Description

@lenamonj

Hi, thanks for maintaining records - it is a small, useful library with a long history. While evaluating it under SQLAlchemy 2.0.x at HEAD ea42736 (Python 3.11), we found four High-severity defects that all trace back to the same root cause: SQLAlchemy 2.x removed the implicit autocommit / close-with-result semantics that records was written against, so writes and pooled connections are no longer finalized the way the code assumes. The existing pytest suite stays green (31 passed) through all four, because its fixtures are sqlite:///:memory: only, which shares a single connection and can never observe a commit boundary or a real pool. We have a complete patch (records.py, packaging, and a new file-backed regression suite) that fixes all four and is passing 37 tests; it is available now, and we're happy to attach it or link it here, whichever is easier on your end.

Below is a one-line summary and a minimal repro for each. All four were reproduced against unmodified upstream ea42736 before we touched anything.

1. Database.query() silently loses INSERTs (High)

Under SQLAlchemy 2.x there is no autocommit, so a write executed via query() is rolled back (silently) when the connection closes, with no exception raised.

import records
db = records.Database("sqlite:////tmp/h1.db")
db.query("CREATE TABLE t (a integer)")
db.query("INSERT INTO t VALUES (1)")
db.close()
db = records.Database("sqlite:////tmp/h1.db")
print(list(db.query("SELECT * FROM t")))  # expected [<Record {'a': 1}>], actual []

2. Database.bulk_query() silently loses inserts (High)

Same root cause as #1, same silent loss, via the bulk write path.

import records
db = records.Database("sqlite:////tmp/h2.db")
db.query("CREATE TABLE t (a integer)")
db.bulk_query("INSERT INTO t VALUES (:a)", [{"a": 1}, {"a": 2}])
db.close()
db = records.Database("sqlite:////tmp/h2.db")
print(list(db.query("SELECT * FROM t")))  # expected 2 rows, actual []

3. Database.transaction() swallows every exception (High)

The bare except: in the transaction context manager rolls back on any exception but never re-raises, so callers cannot tell a failed transaction from a successful one.

import records
db = records.Database("sqlite:///:memory:")
with db.transaction():
    raise RuntimeError("boom")
print("reached here - RuntimeError did not propagate")

We want to flag, with no criticism intended, that a fix for exactly this (except: -> except BaseException: ... raise) was already added upstream on 2026-02-08 in a1ebdde and reverted the same day in 5df61d3. We don't know the context behind that revert. Our regression suite exercises the re-raise-after-rollback behavior together with commit and explicit-rollback paths, and it is compatible with the rest of the suite, in case that's useful context if you revisit it.

4. Every query() leaks a pooled connection (High)

close_with_result is a no-op under SQLAlchemy 2.x, so single-result connections are never returned to the pool. A small pool exhausts after a handful of queries.

import records
from sqlalchemy.pool import QueuePool
db = records.Database("sqlite:///:memory:", poolclass=QueuePool,
                       pool_size=1, max_overflow=1, pool_timeout=2)
for _ in range(5):
    db.query("SELECT 1").all()  # TimeoutError by the 3rd call

What we have ready

A complete patch against ea42736 (touching records.py, tests/, and packaging metadata, roughly 180 lines) fixes all four issues above plus a related TypeError in the legacy varargs form of bulk_query(), and replaces the :memory:-only test fixture with a file-backed regression suite that actually exercises persistence, pooling, and exception propagation - the new tests fail 5 of 6 against the pre-fix code, confirming they test the right thing. Full suite is 37 passed with the patch applied. We're glad to attach the patch to this issue or link to it, whatever's more convenient for you to review.

If it would help, we're happy to open a PR with this patch and the regression suite - just let us know if that's welcome, and if there's a preferred approach (e.g. splitting into smaller PRs per issue) we're happy to follow it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions