Skip to content
Open
150 changes: 124 additions & 26 deletions src/databricks/sql/backend/kernel/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import logging
import threading
import uuid
from typing import Any, Dict, List, Optional, TYPE_CHECKING, Union
from typing import Any, Dict, List, Optional, Set, TYPE_CHECKING, Union

from databricks.sql.backend.databricks_client import DatabricksClient
from databricks.sql.backend.kernel._errors import (
Expand Down Expand Up @@ -251,16 +251,27 @@ def __init__(
# concurrent cursors on the same connection don't race on submit /
# close / close-session.
#
# This is a KEEP-ALIVE registry, not a state/result lookup: the
# This is primarily a KEEP-ALIVE registry: the
# submitting ``ExecutedAsyncStatement``'s ``Drop`` fires a
# fire-and-forget ``close_statement``, which would kill the
# still-running async query the moment the handle is dropped. We
# retain it (and its parent ``Statement``) here so the live query
# survives until an explicit close. ``get_query_state`` /
# ``get_execution_result`` do NOT consult this map — they
# re-attach to the statement by id (the server is the source of
# truth for async state), so they work even cross-process.
# survives until an explicit close. ``get_query_state`` and
# ``get_execution_result`` use this owning handle before result
# streaming starts so kernel async statement telemetry is
# finalized on the original ``ExecuteStatementAsync`` telemetry
# object, then fall back to attach-by-id for re-fetch /
# cross-process cases.
self._async_handles: Dict[str, Any] = {}
self._async_result_stream_started: Set[str] = set()
# Async ids whose owning-handle ``status()`` poll is currently in
# flight. A second concurrent poll of the same id (before result
# streaming is claimed) is routed to the attach-by-id fallback so
# it gets a fresh kernel handle instead of racing ``status()`` on
# the shared owning handle. Guarded by ``_async_handles_lock``;
# each entry is transient (added before the poll, discarded in a
# ``finally``).
self._async_status_in_flight: Set[str] = set()
# Parent ``Statement`` objects kept alive alongside async handles.
# On the kernel, ``Statement.close()`` flips the validity flag on
# the produced executed handle (see kernel
Expand Down Expand Up @@ -406,6 +417,8 @@ def close_session(self, session_id: SessionId) -> None:
tracked_stmts = list(self._async_statements.items())
self._async_handles.clear()
self._async_statements.clear()
self._async_result_stream_started.clear()
self._async_status_in_flight.clear()
for _, handle in tracked:
# Per-handle close errors are non-fatal — PEP 249
# discourages raising from session close — so log and
Expand Down Expand Up @@ -657,6 +670,8 @@ def close_command(self, command_id: CommandId) -> None:
with self._async_handles_lock:
handle = self._async_handles.pop(command_id.guid, None)
stmt = self._async_statements.pop(command_id.guid, None)
self._async_result_stream_started.discard(command_id.guid)
self._async_status_in_flight.discard(command_id.guid)
# Closing the handle below fires the server-side CloseStatement.
# A subsequent ``get_query_state`` re-attaches by id and reads
# ``CLOSED`` straight from the server — no connector-side
Expand Down Expand Up @@ -686,18 +701,53 @@ def close_command(self, command_id: CommandId) -> None:
pass

def get_query_state(self, command_id: CommandId) -> CommandState:
# Server is the source of truth for async command state. Re-attach
# to the statement by its id and read the state the server reports
# — no connector-side state to drift. SEA keys GetStatementStatus
# purely on the id, so a statement the connector no longer holds a
# handle for (or never held — a different process) is still
# queryable. CLOSED comes straight from the server: after a
# Server is the source of truth for async command state. Use the
# retained owning handle before result streaming starts so kernel
# async statement telemetry is finalized on the original
# ExecuteStatementAsync telemetry object. The owning-handle path
# is per-connection, not per-cursor: any cursor on the submitting
# connection (including a fresh cursor resuming the id) resolves
# the same owning handle until result streaming is claimed — see
# the concurrency note below for the limits that places on
# concurrent polling. Once result streaming has been claimed, or
# when this connector genuinely never held the handle (a
# cross-process / restarted-process resume), re-attach to the
# statement by id. SEA keys GetStatementStatus purely on the id,
# so a statement the connector no longer holds a handle for is
# still queryable. CLOSED comes straight from the server: after a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to take care of one scenario:

  • transient status RPC failure
  • telemetry emits FAILED
  • server query continues in kernel
  • later retry succeeds and drains results
  • success/result metrics cannot replace FAILED

We could change kernel finalization so retryable polling errors do not finalize the statement event. Until then, we could retain attached-handle polling and use the owner only for the first result fetch. Let's also add an integration test covering transient poll failure → retry → successful drain → exactly one successful event.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Valid concern, but it needs human/author direction plus verification this job can't do. The bug is real: get_query_state polls status() on the owning handle, so a transient status RPC failure finalizes the ExecuteStatementAsync telemetry as FAILED, which a later successful retry/drain can't overwrite. The reviewer's preferred fix (make kernel finalization skip retryable polling errors) lives in the separate databricks_sql_kernel wheel — out of scope for this connector PR. The connector-side stopgap (drop the owning-handle path from get_query_state and reserve it for the first result fetch only) reverses this PR's core telemetry design and its interlocking concurrency bookkeeping (_async_status_in_flight and the mirrored guard in get_execution_result), so it's a deliberate tradeoff needing maintainer sign-off, not a minimal scoped edit. Furthermore, the requested acceptance test (transient poll failure → retry → successful drain → exactly one successful event) can only run against a live kernel; this follow-up job has no live-warehouse connection (unit-only), so I can't add or verify that integration test here. Flagging for a human to decide kernel-side vs. connector-side and to author/run the integration test in an environment with a live kernel.

# statement is closed (DELETE) the server still returns 200
# state=CLOSED until the result TTL elapses.
if self._kernel_session is None:
raise InterfaceError("get_query_state requires an open session.")
# Concurrency note: the lock guards the _async_handles /
# _async_result_stream_started / _async_status_in_flight bookkeeping only.
# The retained owning handle it returns is a shared object, and
# handle.status() below runs OUTSIDE the lock, so it is not safe to invoke
# status() on one owning handle from two threads at once. Rather than leave
# concurrent in-process polling of a single async id "unsupported" and
# undefined, we reserve the owning handle for the first poller via
# _async_status_in_flight: a second concurrent poll of the same id (before
# result streaming is claimed) sees the id already in flight and falls
# through to the attach-by-id path, getting its own fresh kernel handle —
# preserving the pre-change behaviour where every caller re-attached by id
# and status() ran on distinct objects. The reservation is transient
# (discarded in the finally below), so serial polls still take the
# telemetry-preserving owning-handle path.
with self._async_handles_lock:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — This change trades the previously concurrency-safe behavior (always attach-by-id, where every get_query_state / get_execution_result call created its own server-side handle) for use of a shared owning handle before result streaming is claimed. As the new concurrency note candidly documents, handle.status() (and handle.await_result()) now run outside the lock on the connection-shared owning handle. Note that _async_handles is keyed per-connection, not per-cursor, so a fresh cursor on the same connection that adopts the command id (the resume flow exercised by tests/e2e/test_kernel_backend.py::test_async_result_resumable_from_a_fresh_cursor) will also resolve the owning handle and can race the original submitter's polling on the same underlying kernel handle — not just a hand-rolled two-cursor poll. This is an intentional, documented limitation and the common single-submitter flow is safe, so flagging as low: worth confirming the kernel ExecutedAsyncStatement either tolerates concurrent status() or that same-connection concurrent resume is genuinely out of scope.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected the misleading documentation the reviewer flagged. The docstring for get_query_state now accurately reflects that the owning-handle path is per-connection — any cursor on the submitting connection (including a fresh cursor resuming the id) resolves the same owning handle until result streaming is claimed — consistent with the inline concurrency note. Tests pass (125 in the kernel client file).

Pushed 23b4330.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pushed commit only rewords the docstring — the owning-handle-under-lock behavior at line 721 is unchanged, and the substantive question I raised (does the kernel ExecutedAsyncStatement actually tolerate concurrent status(), or is same-connection concurrent resume genuinely out of scope?) is neither demonstrated nor answered by clearer prose. Better documentation of a limitation isn't verification that the limitation is safe, so I'm keeping this open at Low for a human to confirm the underlying kernel-handle behavior; no further action is needed if that concurrency is confirmed acceptable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Converged to a needs-human question that no code change here can settle: whether the kernel wheel's ExecutedAsyncStatement.status() is safe under concurrent calls on the shared per-connection owning handle. A human must confirm one of: (a) the kernel handle's status()/await_result() tolerate concurrent invocation, or (b) same-connection concurrent resume (tests/e2e/test_kernel_backend.py::test_async_result_resumable_from_a_fresh_cursor racing the original submitter's polling) is genuinely out of scope. Verifying this requires reading the databricks-sql-kernel wheel internals or running the e2e resume test against a live warehouse — this follow-up job has no live-warehouse credentials, so it can't run here. Rewording the docstring again was already rejected by the reviewer as non-verification, and dropping the shared owning handle would revert this PR's intended async-telemetry-finalization fix, so neither is an appropriate change. Flagging for human confirmation of kernel-handle concurrency behavior.

handle = (
None
if (
command_id.guid in self._async_result_stream_started
or command_id.guid in self._async_status_in_flight
)
else self._async_handles.get(command_id.guid)
)
reserved_owning_handle = handle is not None
if reserved_owning_handle:
self._async_status_in_flight.add(command_id.guid)
try:
handle = self._kernel_session.attach_async_statement(command_id.guid)
if handle is None:
handle = self._kernel_session.attach_async_statement(command_id.guid)
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
state, failure = handle.status()
except Exception as exc:
if _is_not_found(exc):
Expand All @@ -721,6 +771,14 @@ def get_query_state(self, command_id: CommandId) -> CommandState:
# sync-fall-through behaviour.
return CommandState.SUCCEEDED
raise _wrap_kernel_exception("get_query_state", exc) from exc
finally:
# Release the owning-handle reservation once this poll's
# status() has completed (or raised). Only the reserver clears
# it, so a concurrent poll that fell through to attach-by-id
# never touches another poller's reservation.
if reserved_owning_handle:
with self._async_handles_lock:
self._async_status_in_flight.discard(command_id.guid)
if state == "Failed" and failure is not None:
# Surface server-reported failure as a database error so
# the cursor's polling loop terminates with the right
Expand All @@ -743,28 +801,68 @@ def get_execution_result(
command_id: CommandId,
cursor: "Cursor",
) -> "ResultSet":
# Re-attach to the statement by id and await its result. SEA keys
# GetStatementResult on the id, so this works whether or not the
# connector still holds the submitting handle — and it's
# inherently re-callable (each call attaches a fresh handle and
# re-materialises the result stream), matching the Thrift backend
# where the operation handle stays re-fetchable until an explicit
# close. No connector-side handle lookup, so no
# ``unknown command_id`` failure on a second call.
# Prefer the original owning async handle for the first
# in-process result stream. The kernel attaches the real
# ExecuteStatementAsync telemetry to that handle; attached
# handles intentionally use no-op telemetry, so always
# re-attaching loses the SEA async statement row when the result
# is drained. After the owning result stream has been started,
# attach by id for re-fetch. This preserves the Thrift-parity
# behavior where results remain re-callable until explicit close.
#
# Concurrency: the owning handle is shared, and ``await_result()``
# below runs OUTSIDE the lock, so it must not run on the same
# handle a concurrent ``get_query_state`` poll is already using
# for ``status()``. Mirror that method's guard here — if a status
# poll has the owning handle reserved (guid in
# ``_async_status_in_flight``), fall through to attach-by-id and
# get a fresh kernel handle, exactly as an in-flight peer poll
# does. In the normal serial flow (poll to terminal, then fetch)
# the reservation is already discarded, so the fetch still takes
# the telemetry-preserving owning-handle path.
#
# ``attach_async_statement`` issues a GetStatementStatus to seed
# the handle; a 404 (unknown / aged-out id) surfaces as a
# NotFound KernelError mapped to ``ProgrammingError`` below via
# ``_wrap_kernel_exception``.
# If this process does not hold the owning handle (fresh cursor,
# restarted process, already re-fetched, or a concurrent poll
# holds it), ``attach_async_statement`` issues a
# GetStatementStatus to seed the handle; a 404 (unknown / aged-out
# id) surfaces as a NotFound KernelError mapped to
# ``ProgrammingError`` below via ``_wrap_kernel_exception``.
if self._kernel_session is None:
raise InterfaceError("get_execution_result requires an open session.")
with self._async_handles_lock:
handle = (
None
if (
command_id.guid in self._async_result_stream_started
or command_id.guid in self._async_status_in_flight
)
else self._async_handles.get(command_id.guid)
)
uses_owning_handle = handle is not None
if uses_owning_handle:
self._async_result_stream_started.add(command_id.guid)
try:
handle = self._kernel_session.attach_async_statement(command_id.guid)
if handle is None:
handle = self._kernel_session.attach_async_statement(command_id.guid)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — New cross-call sharing of a single owning-handle object. Before this change, both get_query_state and get_execution_result always attached a fresh handle by id, so they never operated on the same kernel handle object concurrently. Now, until the fetch marker is set, both methods return the same object from self._async_handles.get(...) and call handle.status() / handle.await_result() on it outside _async_handles_lock.

For the normal per-cursor sequential CUJ (poll then fetch) this is fine, but _async_handles is keyed per-connection by guid, so a second cursor that adopts the same command id (or a background poll racing the fetch) can drive status() on the owning handle while await_result() runs on the same object. Whether the kernel handle tolerates concurrent method calls isn't visible from the connector; if it doesn't, this is a latent data race that the old attach-by-id path avoided. Worth a note confirming the kernel handle is safe for concurrent status()/await_result(), or gating the shared use.

(Anchored to the nearest changed line — see the description for the exact location.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Valid latent-concurrency point, but it needs human/kernel-team judgment and can't be resolved by an in-PR code change. Analysis: get_execution_result is self-serialized for the owning handle — the read-and-mark of _async_result_stream_started is atomic under _async_handles_lock (client.py:777-785), so two concurrent fetches can't both await_result() the same handle (the second falls through to attach-by-id). The residual race is real: get_query_state (client.py:707) reads the owning handle without setting the marker, so handle.status() can run concurrently with handle.await_result() on the same object — but only when a second cursor adopts the same command id or a background poll races the fetch, not the normal sequential poll-then-fetch CUJ. Whether that is safe depends on whether the databricks_sql_kernel handle tolerates concurrent status()/await_result(), which is NOT visible from the connector source (unreadable external artifact). The reviewer's "gate the shared use" alternative can't be done by holding _async_handles_lock across the calls, because await_result() blocks while streaming results and would serialize all submit/close/poll on the connection — a worse regression. Human must check: confirm with the kernel team whether the handle is safe for concurrent status()/await_result(); if not, decide on a gating strategy (e.g. per-guid handle lock) in a follow-up.

stream = handle.await_result()
except Exception as exc:
if uses_owning_handle:
with self._async_handles_lock:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — The owning-handle failure path only discards _async_result_stream_started when await_result() raises. If await_result() succeeds (marker stays set) but the subsequent KernelResultSet.__init__arrow_schema() raises and is re-wrapped, the guid remains marked as started. A later retry then takes the attach-by-id (no-op telemetry) branch.

Whether this loses the ExecuteStatementAsync telemetry row depends on when the kernel finalizes it: if finalization happens when await_result() returns, this is harmless (telemetry already committed). If finalization only completes once the result stream is drained, the telemetry is lost on this retry because the owning handle is never reused. The PR's own comments ("first in-process result stream", "clear the claimed marker so a retry can still use the telemetry-bearing owning handle") are ambiguous on this point, and the added test_get_execution_result_owning_handle_failure_can_retry_owning_handle only exercises the await_result()-raises case, not the construct-failure-after-await case. Worth confirming the finalization semantics and, if drain-based, discarding the marker on the construction-failure path too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

No safe code change is actionable here, and the deciding fact needs kernel-team judgment. The reviewer's suggested fix — discard _async_result_stream_started on the construction-failure path (await succeeds, then _make_result_set/arrow_schema() raises) — would be unsafe: the owning handle's await_result() is single-shot by design (confirmed by test_get_execution_result_is_re_callable, which deliberately re-attaches by id for the second fetch instead of re-calling owning_handle.await_result()). Discarding the marker would route a retry back through the owning handle and re-invoke await_result() on an already-consumed stream — the exact double-consume the marker prevents. Moreover, once await_result() has returned, the stream is materialized and single-shot, so attach-by-id is the only correct retry path regardless. Whether the rare construct-failure-after-await case actually loses the ExecuteStatementAsync telemetry row depends on kernel-internal finalization timing (finalize-at-return vs finalize-at-drain), which has no observable surface in databricks-sql-python and cannot be verified from this repo or a unit test here. The module's own comments assume return-based finalization (making the concern moot); if drain-based, there is no safe in-process remedy. Flagging for a human to confirm finalization semantics with the kernel team.

self._async_result_stream_started.discard(command_id.guid)
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
raise _wrap_kernel_exception("get_execution_result", exc) from exc
# ``KernelResultSet.__init__`` calls ``arrow_schema()`` which
# can raise — map that to PEP 249 too.
#
# Unlike the ``await_result()`` failure above, we deliberately do
# NOT discard the ``_async_result_stream_started`` marker here.
# By this point ``await_result()`` has already succeeded, so the
# owning handle's result stream has been started (and may be
# partially consumed); re-awaiting that same handle on a retry is
# not safe. Leaving the marker set routes any retry through the
# attach-by-id fallback, which re-materialises a fresh stream.
# The trade-off is that such a retry loses the async-statement
# telemetry — an accepted, narrow gap limited to the case where
# result-set construction fails after a successful await.
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
try:
return self._make_result_set(stream, cursor, command_id)
except Exception as exc:
Expand Down
11 changes: 4 additions & 7 deletions src/databricks/sql/backend/kernel/result_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,10 @@ def close(self) -> None:
# connection close path stays clean.
logger.warning("Error closing kernel handle: %s", exc)
# Honor the base ``ResultSet`` contract: notify the backend.
# ``backend.close_command`` also drops the ``_async_handles``
# entry and records the guid in ``_closed_commands`` — no
# separate pop needed here. Sync-execute and metadata paths
# never registered in ``_async_handles`` to begin with, and
# ``get_execution_result`` pops the async path before the
# result set is even constructed (see the M1 fix), so this
# call is the single bookkeeping seam.
# For async results, ``backend.close_command`` drops the
# retained owning handle and parent Statement. Sync-execute and
# metadata paths never registered in ``_async_handles`` to begin
# with, so this call is tolerant bookkeeping for them.
backend = cast("KernelDatabricksClient", self.backend)
try:
backend.close_command(self.command_id)
Expand Down
15 changes: 7 additions & 8 deletions tests/e2e/test_kernel_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -462,14 +462,14 @@ def test_dml_rowcount_wiring_does_not_break_dml(conn):
cur.execute(f"DROP TABLE IF EXISTS {tbl}")


# ── Async execution: state + result come from the server (attach-by-id) ──
# ── Async execution: owning handle first, attach-by-id for re-fetch/resume ──


def test_async_execute_polls_and_fetches_result(conn):
"""The full async CUJ: ``execute_async`` → poll
``get_query_state`` → ``get_async_execution_result``. State and
result are read from the server by re-attaching to the statement
id (no connector-side state)."""
``get_query_state`` → ``get_async_execution_result``. The first
in-process flow uses the retained owning handle so kernel async
telemetry is finalized."""
with conn.cursor() as cur:
cur.execute_async("SELECT 7 AS n")
cur.get_async_execution_result() # polls to terminal, fetches
Expand All @@ -482,10 +482,9 @@ def test_async_execute_polls_and_fetches_result(conn):


def test_async_get_execution_result_is_re_callable(conn):
"""``get_async_execution_result`` re-attaches by id on each call,
so fetching the same async command twice both succeed — the
connector never relied on a one-shot retained handle (Thrift-parity
re-fetch)."""
"""Fetching the same async command twice succeeds: the first
in-process result fetch can use the owning handle, and later
re-fetches attach by id (Thrift-parity re-fetch)."""
with conn.cursor() as cur:
cur.execute_async("SELECT 11 AS n")
cur.get_async_execution_result()
Expand Down
Loading
Loading