Skip to content

Commit 244563d

Browse files
ai: apply changes for #873 (2 review threads)
Addresses: - #3635951484 at tests/e2e/test_driver.py:379 - #3635951489 at src/databricks/sql/client.py:1775 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent ee59c9c commit 244563d

3 files changed

Lines changed: 41 additions & 6 deletions

File tree

src/databricks/sql/client.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1772,7 +1772,7 @@ def close(self) -> None:
17721772
try:
17731773
self.backend.close_command(self.active_command_id)
17741774
except RequestError as e:
1775-
if isinstance(e.args[1], CursorAlreadyClosedError):
1775+
if len(e.args) > 1 and isinstance(e.args[1], CursorAlreadyClosedError):
17761776
# Already-closed handle (e.g. a prior cancel() or concurrent
17771777
# session teardown) is an expected, benign case — mirror
17781778
# ResultSet.close and log at info, not warning.

tests/e2e/test_driver.py

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -371,11 +371,20 @@ def test_execute_async__close_without_fetch_frees_handle(self, extra_params):
371371
# without ever calling get_async_execution_result().
372372
cursor.close()
373373

374-
# After close, the server-side handle must have been freed, so a
375-
# re-poll of the saved command id should raise a server error.
376-
# Pre-fix (leak): this poll succeeds. Post-fix: it raises.
377-
with pytest.raises((RequestError, OperationalError, DatabaseError)):
378-
backend.get_query_state(command_id)
374+
# After close, the server-side handle must have been freed. How a
375+
# re-poll of the saved command id surfaces that is backend-specific:
376+
# - Thrift raises a server error on the closed handle.
377+
# - SEA's get_query_state() does a plain GET and returns the
378+
# status.state, so a freed statement may come back as a terminal
379+
# CLOSED/CANCELLED state instead of raising.
380+
# Accept either signal as proof the handle was freed. Pre-fix (leak),
381+
# the poll instead returns a live/terminal-success state.
382+
try:
383+
state = backend.get_query_state(command_id)
384+
except (RequestError, OperationalError, DatabaseError):
385+
pass
386+
else:
387+
assert state in (CommandState.CLOSED, CommandState.CANCELLED)
379388

380389

381390
# Exclude Retry tests because they require specific setups, and LargeQueries too slow for core

tests/unit/test_client.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,32 @@ def test_cursor_close_does_not_double_close_when_result_set_present(self):
222222
mock_backend.close_command.assert_not_called()
223223
self.assertIsNone(cursor.active_command_id)
224224

225+
def test_cursor_close_tolerates_single_arg_request_error(self):
226+
"""A RequestError with only one positional arg must not escape close().
227+
228+
Some RequestErrors (e.g. from unified_http_client during shutdown) are
229+
built with a single message arg, so e.args[1] would IndexError. close()
230+
must guard the length check and swallow the error like any other
231+
close_command failure.
232+
"""
233+
from databricks.sql.exc import RequestError
234+
235+
mock_backend = Mock(spec=ThriftDatabricksClient)
236+
mock_backend.close_command.side_effect = RequestError(
237+
"HTTP client is closing or has been closed"
238+
)
239+
mock_connection = Mock()
240+
cursor = client.Cursor(connection=mock_connection, backend=mock_backend)
241+
242+
cursor.active_command_id = Mock(spec=CommandId)
243+
cursor.active_result_set = None
244+
245+
# Should not raise despite the single-arg RequestError.
246+
cursor.close()
247+
248+
mock_backend.close_command.assert_called_once()
249+
self.assertIsNone(cursor.active_command_id)
250+
225251
@patch("%s.session.ThriftDatabricksClient" % PACKAGE_NAME)
226252
def test_cant_open_cursor_on_closed_connection(self, mock_client_class):
227253
connection = databricks.sql.connect(**self.DUMMY_CONNECTION_ARGS)

0 commit comments

Comments
 (0)