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

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.