Skip to content

DataMapperPool::Return performs no transaction cleanup -- a connection returned mid-transaction is silently inherited by the next caller #583

Description

@Yaraslaut

Summary

DataMapperPool::Return (and by extension ~PooledDataMapper) performs no transaction cleanup on a connection being returned to the pool. If a connection is returned while a SQL transaction (started by a raw BEGIN/BEGIN DEFERRED/BEGIN IMMEDIATE, or even via Lightweight::SqlTransaction's own autocommit-toggle if a caller's cleanup path is skipped) is still open, that open transaction is silently inherited by whichever unrelated caller acquires the same connection next. Combined with the driver's own busy_timeout (60000ms in this codebase's configuration), that caller's first write can stall for the full timeout before surfacing SQLITE_BUSY — an intermittent, environment-dependent, hard-to-attribute failure.

Evidence

Confirmed directly against the vendored source (tag v0.20260625.0, commit 8cd0795):

  • All three DataMapperPool::Return overloads (Pool.hpp) do only DropAsyncBackend(*dm) — which is just dm.Connection().DisableAsync() — plus pushing the mapper onto _idleDataMappers. No SQLEndTran, no ROLLBACK, no autocommit reset, no cursor close, no health check.
  • ~PooledDataMapper is noexcept and returns the connection unconditionally.
  • Acquire() does nothing to reset a recycled mapper's transaction state either.

Discovered while implementing a background report-job worker in an example application (ledger, rung 5) that pins a SQLite read snapshot via a raw BEGIN DEFERRED/COMMIT pair around an aggregation query, using a connection acquired via GlobalDataMapperPool().Acquire(). Two real paths could leave the connection returned to the pool mid-transaction:

  1. The raw BEGIN DEFERRED statement itself throwing (before any cleanup code runs).
  2. A recovery COMMIT on an exception-unwind path itself throwing (a real SQLITE_BUSY-on-commit possibility) — this replaces the in-flight exception and would propagate with the transaction still open.

Independently confirmed via review: the fix on the application side was an RAII guard (constructor issues BEGIN, destructor issues COMMIT inside its own catch (...), so the transaction is always closed exactly once per successful construction, on every exit path including exception unwinding) — but this is an application-level workaround for a gap that exists in the pool's own connection-lifecycle contract.

Suggested direction

DataMapperPool::Return (or ~PooledDataMapper) could defensively check whether the connection still has an open transaction (e.g. via the ODBC autocommit attribute, or tracking transaction state on SqlConnection itself) and roll it back before returning the connection to the idle pool — so a caller's bug in transaction cleanup degrades to "lost work in one transaction" rather than "silently corrupts pool state for an unrelated future caller." At minimum, this should be documented prominently on Return/Acquire's own doc comments as a caller responsibility, since it isn't currently called out.

Reference

Found while implementing LASTRADA-Software/morph's rung 5 (ledger) example, branch ladder-ledger-rung5 (not yet merged). See examples/ledger/src/models/ledger_model.cpp's WalSnapshotGuard class for the application-side mitigation and its own doc comment for the full reasoning.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions