Cursor.close() leaks server-side handle for async commands that were never fetched (#791)

[peco-engineer-bot]

Summary

Automated fix for #791 — Cursor.close() leaks server-side handle for async commands that were never fetched.

Cursor.close() now issues an explicit backend.close_command(active_command_id) for the async-submit-without-fetch case (where active_result_set is None), freeing the server-side handle instead of leaking it until session close. Verified the E2E test goes green and the full async suite still passes.

Root cause & plan

Root cause: Cursor.close() in src/databricks/sql/client.py (lines 1758-1763) sets self.active_command_id = None and then only frees the server-side statement handle through the active_result_set path (_close_and_clear_active_result_set -> backend.close_command). For a command submitted via execute_async whose result was never fetched (get_async_execution_result / get_execution_result never called), active_result_set is None, so backend.close_command(active_command_id) is never invoked. The server-side statement handle therefore leaks until the session closes. This affects all three backends (Thrift, SEA, kernel), since the leak is in the shared Cursor.close() path.
Files: src/databricks/sql/client.py, tests/e2e/test_driver.py
Planned coverage:

• E2E (Thrift): submit a query with cursor.execute_async(...), capture cursor.active_command_id, then call cursor.close() without ever fetching the result. Assert the server-side handle was freed by re-polling the saved command_id via cursor.backend.get_query_state(command_id) (or connection.session.backend) and expecting it to raise a server error (RequestError/ServerOperationError/DatabaseError) because the operation was closed. Pre-fix this poll succeeds (leak); post-fix it fails. Add to TestPySQLAsyncQueriesSuite following the existing execute_async patterns, on the default schema. (Async-submit-without-fetch: Cursor.close() must call backend.close_command() to free the leaked server-side handle.)

Files changed

• src/databricks/sql/client.py
• tests/e2e/test_driver.py

Test plan

• tests/e2e/test_driver.py::TestPySQLAsyncQueriesSuite::test_execute_async__close_without_fetch_frees_handle — fails (red) against the original code, passes (green) after the fix

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

[peco-review-bot]

Verdict: 1 Low

Looks good — the fix correctly closes the leaked server-side handle for async-submitted commands that were never fetched by issuing backend.close_command() in the new elif branch, guarded by try/except and reordering active_command_id = None to after the close. logger is defined in the module, all backends implement close_command(command_id), and there's no double-close risk since the branch only fires when active_result_set is None. One low-severity note on E2E coverage (Thrift-only) below.

[peco-review-bot]

✅ No issues identified by the review bot.

[eric-wang-1990]

We should never assign use_sea, there are only 2 active modes either default(thrift), or use_kernel which use kernel.

[peco-engineer-bot]

Already satisfied. The test at tests/e2e/test_driver.py:350 (test_execute_async__close_without_fetch_frees_handle) now parametrizes only extra_params=[{}] — the default (thrift) mode — with no use_sea assigned. The {"use_sea": True} case shown in the review diff hunk is no longer present in the current tree, so this async-handle-leak test does not exercise SEA. No code change needed here. (Note: CONTRIBUTING.md and .bot/prompts/engineer/system.md still document adding a {"use_sea": True} param case for backend-parametrized e2e tests generally; if the intent is to drop use_sea as a supported mode repo-wide, that's a broader change for a human to reconcile with those docs.)

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks good — the fix correctly frees the leaked server-side handle for the async-submit-without-fetch case, and the mutually-exclusive if active_result_set / elif active_command_id structure avoids double-closing sync/metadata commands. Two non-blocking notes: no fast/mocked unit test covers the new branch (only a Thrift-only E2E test), and the new close_command call lacks the connection.open guard that the established result-set close path uses.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Sound, well-tested fix — the new elif branch faithfully mirrors the existing ResultSet.close/session.py idiom (connection.open gate, RequestError/CursorAlreadyClosedError handling) and the three added unit tests cover the main branches. Two concerns: the E2E test's SEA parametrization may not raise as asserted since SEA get_query_state returns state without erroring on a closed handle (medium), and the copied e.args[1] access can IndexError for single-arg RequestErrors (low).

[peco-review-bot]

Verdict: 1 Low

Looks good — the async-submit-without-fetch handle leak is fixed correctly and the fix faithfully mirrors ResultSet.close() (connection.open gate, benign CursorAlreadyClosedError, and a more-robust len(e.args) > 1 guard). Lifecycle/reordering of active_command_id = None is safe (no reader depends on it during result-set close, no double-close). One low-severity note on the breadth of the exception catch; unit + e2e coverage is thorough. Nit: no unit test exercises the CursorAlreadyClosedError info-log branch (only the single-arg RequestError and generic-Exception paths are covered) — worth adding for completeness.

[peco-review-bot]

Verdict: 1 Low

Looks good — the fix correctly closes the leaked server-side handle for the async-submit-without-fetch case and faithfully follows the existing ResultSet.close/Session.close conventions (even hardening the e.args[1] access with a length guard). Ordering and no-double-close behavior are sound, and unit coverage is solid. One low concern: the E2E test's SEA re-poll assertion may be brittle depending on unverified SEA post-delete state behavior.

[peco-review-bot]

🔵 Low — The SEA branch of this test may be brittle. After close_command issues a DELETE on the statement, the subsequent get_query_state does a plain GET (_poll_query) and returns whatever status.state the server reports. The test only tolerates a raised error or state in (CommandState.CLOSED, CommandState.CANCELLED). If SEA responds to a GET on a just-deleted statement with any other terminal/transient state (e.g. it still echoes SUCCEEDED briefly, or a state not in that tuple), the else assert fails and the test flakes — even though the handle was correctly freed. Since get_query_state intentionally does not call _check_command_not_in_failed_or_closed_state, there's no guarantee of a CLOSED/CANCELLED signal. Consider confirming empirically what SEA returns post-delete, or broadening the accepted signal (e.g. also accept a 404-derived error) so the test isn't tied to an unverified state assumption.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

The comment targets the SEA branch of an E2E test whose correct accepted-state set depends on what a live SEA warehouse actually returns from get_query_state() after close_command issues the statement DELETE. This job has no live-warehouse connection and must not run/add e2e tests, so I cannot verify SEA's real post-delete signal here. The reviewer's two options both hinge on that empirical fact: (a) confirm empirically — impossible in this job; (b) broaden the accepted states — unsafe to do blind, because the pre-fix leak "returns a live/terminal-success state," so broadening (e.g. accepting SUCCEEDED) without knowing SEA's freed-handle behavior would let the test pass in the leak case and destroy the regression it guards. A human needs to run this against a live SEA warehouse to observe the actual post-DELETE state (or 404), then either narrow the assertion to that confirmed signal or, if SEA gives no reliable freed signal, restructure the SEA branch (e.g. skip the re-poll assertion for SEA). Flagging for human judgment rather than guessing at a broadening that could mask the leak.