we dont close on GC connections that were detached (and weren't closed). should we? #13570

Describe the bug

A connection that has been detached from its pool with
Connection.detach() / PoolProxiedConnection.detach() and is then
garbage collected without being explicitly closed never has its DBAPI
connection closed. The connection is leaked until the DBAPI itself
reclaims it, if it ever does.

Detaching hands ownership of the DBAPI connection to the fairy, and the
explicit close path handles this correctly: _ConnectionFairy._checkin()
calls _finalize_fairy() with connection_record=None, which takes the
detach = connection_record is None branch and calls
pool._close_connection().

The garbage collection path never reaches that branch. The finalizer
established for the fairy at checkout closes over the record, and
_ConnectionFairy.detach() clears _ConnectionRecord.fairy_ref, so when
the fairy is later collected the cleanup is skipped entirely. The
connection is not closed, and for asyncio dialects the
"garbage collector is trying to clean up non-checked-in connection"
warning that a non-detached abandoned connection would produce is not
emitted either.

This is long standing behavior, reproduced unchanged on 1.4, 2.0 and
current main.

Optional link from https://docs.sqlalchemy.org which documents the behavior that is expected

https://docs.sqlalchemy.org/en/20/core/connections.html#sqlalchemy.engine.Connection.detach

SQLAlchemy Version in Use

2.1.0b4, also 2.0 and 1.4

DBAPI (i.e. the database driver)

sqlite3, aiosqlite

Database Vendor and Major Version

SQLite

Python Version

3.14

Operating system

Linux

To Reproduce

import gc
import sqlite3

from sqlalchemy import create_engine
from sqlalchemy import text

closed = []


class TrackingConnection:
    """wrap a DBAPI connection so we can see close() being called"""

    def __init__(self, real):
        self.real = real

    def close(self):
        closed.append(self)
        self.real.close()

    def __getattr__(self, name):
        return getattr(self.real, name)


def creator():
    return TrackingConnection(sqlite3.connect(":memory:"))


engine = create_engine("sqlite://", creator=creator)

# 1. detach, then close explicitly -> DBAPI connection is closed
conn = engine.connect()
conn.execute(text("select 1"))
conn.detach()
conn.close()
print("detach() + close()    -> DBAPI close() called:", len(closed) == 1)

# 2. detach, then just drop it -> DBAPI connection is leaked
closed.clear()
conn = engine.connect()
conn.execute(text("select 1"))
conn.detach()
del conn
gc.collect()
print("detach() + gc         -> DBAPI close() called:", len(closed) == 1)

The asyncio equivalent, showing that the GC warning is also skipped for a
detached connection:

import asyncio
import gc
import warnings

from sqlalchemy import text
from sqlalchemy.ext.asyncio import create_async_engine


async def main():
    e = create_async_engine("sqlite+aiosqlite://")
    for label, do_detach in (("not detached", False), ("detached", True)):
        conn = await e.connect()
        await conn.execute(text("select 1"))
        raw = await conn.get_raw_connection()
        if do_detach:
            raw.detach()
        del conn, raw
        with warnings.catch_warnings(record=True) as w:
            warnings.simplefilter("always")
            gc.collect()
        msgs = [
            str(x.message) for x in w if "garbage collector" in str(x.message)
        ]
        print(f"  {label:14s} -> gc warning emitted: {bool(msgs)}")


asyncio.run(main())

Error

detach() + close()    -> DBAPI close() called: True
detach() + gc         -> DBAPI close() called: False

  not detached   -> gc warning emitted: True
  detached       -> gc warning emitted: False

Additional context

Noticed while reviewing
https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6932, which
converts the fairy's garbage collection cleanup to weakref.finalize().
That change does not alter this behavior, it only makes it more visible:
where the previous code left a weakref callback in place that returned
early, _ConnectionFairy.detach() now cancels the finalizer outright.

A fix would need the detached fairy to keep a finalizer that closes over
connection_record=None, so that collection takes the same
detach/_close_connection() branch that the explicit close path takes.
This is not a two line change: _finalize_fairy() asserts a non-null
record in two places, and the second of those builds a throwaway
_ConnectionFairy to run the reset with, which currently requires a
record.