[laughingman7743]

WHAT

AthenaDialect.do_ping() keeps #1185's API cursor, but no longer runs the ping itself.
It passes DefaultDialect.do_ping() a small proxy of the pinged connection (_PingConnection) whose cursor() opens the API cursor (_internal_cursor(Cursor)) and whose other attributes are the connection's, and returns its result.
AthenaAioDialect inherits this; the async adapted connection's _internal_cursor() maps Cursor to AioCursor as before.

Before SQLAlchemy 2.0.5, DefaultDialect.do_ping() is where a failed ping is checked for a disconnect: it calls is_disconnect(err, connection, cursor) and the handle_error listeners and returns False, so the pool discards the connection and opens a new one.
#1185's override skipped that, so with SQLAlchemy 2.0.0 to 2.0.4 such a checkout raised instead of reconnecting.
Because is_disconnect() receives the proxy as its connection there, an override that reads the connection (for example its schema_name, or an adapter's driver_connection) sees the pinged connection's attributes.
From 2.0.5, SQLAlchemy handles the error around do_ping() (_do_ping_w_event()) with the original connection, and the behavior there is unchanged.

No release note: #1185 is unreleased.

WHY

Closes #1199, found by an independent review of the 3.x backport #1198, which needs this fix before the next 3.x release (SQLAlchemy 1.x handles disconnects inside do_ping() too).

TEST

Tested commit: 3573420 for just lint and the offline -k do_ping tests; the reproductions and live tests ran at 29d61f3, which differs only in the _PingConnection docstrings.

• just lint: passed.
• tests/pyathena/sqlalchemy/test_base.py -k do_ping (offline): 4 passed.
  The rewritten test_do_ping_keeps_default_dialect_ping fails the ping (execute() raises OperationalError) and replaces DefaultDialect.do_ping() with a stand-in that asks is_disconnect() as SQLAlchemy before 2.0.5 does.
  It asserts False, the API cursor class, the raised error, and the pinged connection's schema_name as seen by is_disconnect().
  It fails with master's do_ping() (the error propagates) and with this PR's first commit 3aae3c7 (AttributeError on the namespace), with no AWS request in either case.
  test_do_ping_uses_api_cursor_without_unload still checks the API cursor, SELECT 1, and the closed cursor through SQLAlchemy 2.0.46's real do_ping().
• The reproduction in SQLAlchemy ping no longer detects disconnects before SQLAlchemy 2.0.5 #1199 and a variant whose is_disconnect() override reads connection.schema_name (Python 3.11, uv run --isolated --with sqlalchemy==<version>): with this branch, both reconnect with SQLAlchemy 2.0.0, 2.0.4, 2.0.5, and 2.0.46.
  With fix: run ping with plain cursor #1185 (52ff4e00), the SQLAlchemy ping no longer detects disconnects before SQLAlchemy 2.0.5 #1199 reproduction raised OperationalError: ExpiredTokenException on 2.0.4; with 3aae3c7, the variant raised AttributeError: 'types.SimpleNamespace' object has no attribute 'schema_name' on 2.0.4.
• TestSQLAlchemyAthena::test_ping_with_unload and TestAsyncSQLAlchemyAthena::test_ping_with_unload (live AWS, Python 3.15 venv of this worktree): 2 passed.

Not run locally: the full PyAthena suite, the SQLAlchemy compliance suites, and the async engine with SQLAlchemy 2.0.0–2.0.4; the AWS CI of this PR runs the suites.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (implementation behavior): CLEAN.

Scope: base e78b8e3, head 3aae3c7; AthenaDialect.do_ping() (inherited by AthenaAioDialect) and both new and existing ping tests.

• SQLAlchemy 2.0.5+: DefaultDialect.do_ping() opens the cursor through the namespace (_internal_cursor(Cursor)), executes SELECT 1, closes it in finally, and returns True; an error propagates to _do_ping_w_event(), which is called with the real DBAPI connection, so its is_disconnect()/event handling sees the same arguments as before fix: run ping with plain cursor #1185.
• SQLAlchemy 2.0.0–2.0.4: the error, including one raised while opening the cursor, is caught by DefaultDialect.do_ping() (except self.loaded_dbapi.Error), which passes the namespace to is_disconnect(); this dialect does not override it and the handle_error path does not receive the connection. Measured with the SQLAlchemy ping no longer detects disconnects before SQLAlchemy 2.0.5 #1199 reproduction.
• Async: the adapted connection's _internal_cursor() maps Cursor to AioCursor, and its DBAPI module exposes pyathena.error.Error, so loaded_dbapi.Error matches the async cursor's errors.
• Callers that pass a pool-proxied connection (e.g. engine.dialect.do_ping(engine.raw_connection())) reach _internal_cursor through _ConnectionFairy.__getattr__, as with fix: run ping with plain cursor #1185.
• Tests: test_do_ping_keeps_default_dialect_ping fails on both cases with master's do_ping() and passes here; the real SQLAlchemy 2.0.46 path stays covered by test_do_ping_uses_api_cursor_without_unload.

Limitation: the async engine was not run with SQLAlchemy 2.0.0–2.0.4; only the synchronous reproduction was.

[laughingman7743]

Self-review round two (claims, callers, operations): CLEAN.

Scope: same base/head; the PR body, the commit message, the new code comment and test docstring, and #1199.

• "From 2.0.5, SQLAlchemy handles the error around do_ping()": _do_ping_w_event is absent from lib/sqlalchemy/engine/default.py at rel_2_0_1 through rel_2_0_4 and present at rel_2_0_5 and rel_2_0_6; 2.0.46's pool calls it (pool/base.py:1301).
• "Before 2.0.5 ... consults is_disconnect() and the handle_error listeners": SQLAlchemy 2.0.0 DefaultDialect.do_ping() (engine/default.py:672-) calls is_disconnect() and _handle_dbapi_exception_noconnection() when _has_events. SQLAlchemy 1.4's version consults only is_disconnect(), which matters for the 3.x port, not for master (sqlalchemy>=2.0.0).
• "No release note: fix: run ping with plain cursor #1185 is unreleased": git tag --contains 52ff4e00 is empty.
• The reproduction results in the PR body and SQLAlchemy ping no longer detects disconnects before SQLAlchemy 2.0.5 #1199 were produced with this branch at 3aae3c7 and with 52ff4e00/52ff4e00^1 (detached worktrees), Python 3.11, uv run --isolated --with sqlalchemy==<version>.
• Existing callers: no public signature changes; do_ping() still returns True on success with SQLAlchemy 2.0.46 (both live test_ping_with_unload tests passed on AWS at 3aae3c7).
• Operations: the ping still costs one SELECT 1 query through the API cursor; no retry or request count changes.

[laughingman7743]

Independent review (relayed): Codex CLI 0.162.1, model gpt-6-astra, reasoning effort max, codex exec -s read-only, session 01a14285-2ebe-7fe2-9bcc-2bb2904cf627. Static review only: no edits, builds, tests, GitHub, or network access. Snapshot: detached worktree at head 3aae3c7, base e78b8e3. The prompt gave the intended behavior without the PR number, description, commit message, or prior findings.

Coverage reported: the two-file diff, cursor selection and cleanup, async adaptation, pool_pre_ping, direct pool-proxy callers, typing, and the regression tests, against cached SQLAlchemy 2.0.0, 2.0.4, 2.0.5, and 2.0.46 (2.0.1–2.0.3 not inspected individually).

Result: FINDINGS (1).

P2 — Preserve the original connection for older disconnect checks — pyathena/sqlalchemy/base.py:550. Before 2.0.5, DefaultDialect.do_ping() calls is_disconnect(err, dbapi_connection, cursor), so it receives the cursor-only SimpleNamespace as the connection. On SQLAlchemy 2.0.4, an application's is_disconnect() override that inspects the connection (for example connection.client, or connection.driver_connection for async) raises AttributeError, and pool checkout fails instead of reconnecting; handle_error listeners are never reached. Introduced by this range, as an incomplete repair of the pre-existing bypass.
The new test checks delegation, the cursor type, and propagation of a fabricated False, but never exercises an actual DBAPI failure or the classifier's arguments; without the change it enters the unpatched execute path, which may attempt AWS I/O in the sync case.

Author verification: confirmed. With 3aae3c7 and SQLAlchemy 2.0.4 (Python 3.11), an is_disconnect() override that reads connection.schema_name makes checkout raise AttributeError: 'types.SimpleNamespace' object has no attribute 'schema_name'. Repair follows in this thread.

[laughingman7743]

Repair: 29d61f3 ("Show is_disconnect() the pinged connection's attributes").

do_ping() now passes DefaultDialect.do_ping() a _PingConnection proxy: cursor() opens _internal_cursor(Cursor), and every other attribute is the pinged connection's (__getattr__). The regression test fails the ping and uses a stand-in that asks is_disconnect() the way SQLAlchemy before 2.0.5 does. It asserts False, the API cursor class, the error, and the connection's schema_name as is_disconnect() sees it. It fails with master's do_ping() and with 3aae3c7 (AttributeError), and it makes no AWS request.

Validation at 29d61f3: just lint passed; -k do_ping 4 passed. The #1199 reproduction and a variant whose is_disconnect() reads connection.schema_name both reconnect with SQLAlchemy 2.0.0, 2.0.4, 2.0.5, and 2.0.46 (Python 3.11). Both live test_ping_with_unload tests passed (sync and async).

Repair self-review (range 3aae3c7..29d61f3, same base e78b8e3):

• Implementation behavior: CLEAN. Attribute lookups other than cursor and _connection reach the pinged connection, including the async adapter's driver_connection (AdaptedConnection.driver_connection). Before 2.0.5, _handle_dbapi_exception_noconnection() does not take the connection. From 2.0.5, _do_ping_w_event() still passes the original connection to is_disconnect(). The proxy is not the same object, so an override that compares the connection by identity still sees a different object (not addressed; such an override would also have to reach the pool's connection record, which do_ping() never had).
• Claims: CLEAN. The PR body now describes the proxy and the two reproductions with their versions and results, and records that the async engine was not run with 2.0.0–2.0.4. The commit message examples (schema_name, driver_connection) exist on Connection and AdaptedConnection.

An independent follow-up review of this repair is next.

[laughingman7743]

Independent follow-up review (relayed): Codex CLI 0.162.1, model gpt-6-astra, reasoning effort max, codex exec -s read-only, session 01a14295-77f9-7122-83ba-cba1ddbc3f6f. Static review only. Snapshot: detached worktree at 29d61f3; ranges 3aae3c72..29d61f39 and e78b8e3b..29d61f39. The snapshot and the PR worktree were unchanged afterwards.

Coverage reported: proxy delegation, recursion, cursor cleanup, sync/async adapters, pool-proxied callers, SQLAlchemy's version-dependent ping and handle_error paths (cached 2.0.0, 2.0.4, 2.0.5, 2.0.54, 2.1.1), typing, docstring rules, and the regression test.

Result: FINDINGS (1, documentation only).

P3 — Constructor documentation violates the repository rule — pyathena/sqlalchemy/base.py:114, introduced by 29d61f3. _PingConnection.__init__ has no docstring; its argument documentation is on the class, while docs/contributing.md requires __init__ to describe the constructor arguments in its Args: section, also for new private methods. No runtime failure.
The functional repair resolves the reported attribute-access defect; no actionable recursion, lifecycle, pool-proxy, async, or typing problem. By static inspection, the rewritten test fails against the previous implementation at connection.schema_name and asserts False, the original exception, the delegated schema value, and the API cursor type for both connection variants.

Repair: 3573420 moves the Args: section into an __init__ docstring (docs/contributing.md: "__init__ describes the constructor arguments in its Args: section"). just lint passed and -k do_ping 4 passed at 3573420. Repair self-review: implementation unchanged (docstring only); claims CLEAN, and the PR body names which results ran at 29d61f3 and which at 3573420. A follow-up review of 29d61f3..3573420 is next.

[laughingman7743]

Independent follow-up review of the docstring repair (relayed): Codex CLI 0.162.1, model gpt-6-astra, codex exec -s read-only, session 01a1429f-9cba-7802-8712-ec810df59308. Static review only. Snapshot: detached worktree at 3573420; range 29d61f39..35734209.

Result: CLEAN. __init__ now has a summary and an Args: section for connection, which resolves the previous finding; the revision only relocates the argument documentation and adds the constructor summary, with no executable or signature changes (git diff --check passed).

The independent review of this PR is complete at 3573420.