Skip to content

Commit 21730a3

Browse files
committed
fix(managed): enumerate in-flight statuses, not terminal ones
Follow-up from review on the query-run wait. Both polls listed the statuses that mean finished and treated everything else as "keep waiting". That arrangement is what let `interrupted` be polled for the full five-minute timeout: a status absent from the list was indistinguishable from one still running. Listing the in-flight side instead -- `running` for a query run, `pending`/`processing` for a result -- makes an unrecognised status raise on the first pass with its own name in the message, so the next status the API adds costs one round trip rather than a timeout. That also fixes both status bugs in `HotdataClient`, which still had `cancelled` in its terminal sets and lacked `interrupted` entirely, so `execute_sql` kept the behaviour this branch documents as a bug. Doing both keeps the changelog entry true for the whole package rather than for one of its two clients. The async-submit test asserted the model attribute, which would still pass if the `async` alias were missing or misspelled -- the server would then ignore the field, answer synchronously with a full preview, and silently restore what this branch removes. It now asserts the serialised wire key.
1 parent 6c959b0 commit 21730a3

5 files changed

Lines changed: 127 additions & 20 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,16 +35,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3535

3636
- fix(managed): recognise `interrupted`, and drop a run status the API never sends.
3737

38-
The query-run poll treated `failed` and `cancelled` as the terminal failures.
39-
`cancelled` is not a status this API returns. `interrupted` is — a run whose
40-
server was replaced before it finished — and it matched neither branch, so the
41-
poll ran to its five-minute timeout and raised `TimeoutError` instead of
42-
failing fast.
43-
44-
An interrupted run is safe to re-run, so it is now raised as transient and the
45-
surrounding retry re-submits the query. `classify_sdk_error` passes an
46-
already-classified error through unchanged, rather than demoting a
47-
caller-raised transient error to terminal.
38+
Both `ManagedDatabaseClient` and `HotdataClient` treated `failed` and
39+
`cancelled` as the terminal run failures. `cancelled` is not a status this API
40+
returns. `interrupted` is — a run whose server was replaced before it finished
41+
— and it matched neither, so an interrupted run was polled for the full
42+
five-minute timeout and then raised `TimeoutError`: a retryable condition
43+
hidden behind a long wait and an error naming the wrong problem.
44+
45+
On `ManagedDatabaseClient` an interrupted run is now raised as transient, so
46+
the surrounding retry re-submits the query. That needed `classify_sdk_error` to
47+
pass an already-classified error through unchanged rather than demoting a
48+
caller-raised transient error to terminal. `HotdataClient.execute_sql` now
49+
fails fast on it with the run's own message.
50+
51+
Both polls now enumerate the statuses that mean *still in flight* rather than
52+
the ones that mean *finished*, for query runs and for results alike. Listing
53+
the terminal side treats an unrecognised status as "keep waiting", which is
54+
precisely how `interrupted` came to be waited out; listing the in-flight side
55+
makes the next status the API adds fail on the first pass, naming itself.
4856

4957
## [0.13.0] - 2026-08-27
5058

‎hotdata_framework/client.py‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,15 @@
7777
_INDEX_TYPES = frozenset(get_args(IndexType))
7878
_VECTOR_METRICS = frozenset(get_args(VectorMetric))
7979

80-
_TERMINAL = frozenset({"succeeded", "failed", "cancelled"})
81-
_RESULT_FAILURE = frozenset({"failed", "cancelled"})
80+
# Enumerate the in-flight statuses, not the terminal ones. A poll that lists
81+
# what is terminal treats anything it does not recognise as "keep waiting", so a
82+
# status the API adds later -- or one that was simply missed -- costs the full
83+
# timeout before surfacing. Listing what is still in flight makes an unknown
84+
# status raise on the first pass, naming itself. `interrupted` was missed by
85+
# exactly the other arrangement, and `cancelled` was listed here without being a
86+
# status this API sends.
87+
_RUN_IN_FLIGHT = frozenset({"running"})
88+
_RESULT_IN_FLIGHT = frozenset({"pending", "processing"})
8289
# Jobs have no "cancelled" state; "partially_succeeded" carries an error_message.
8390
_JOB_TERMINAL = frozenset({"succeeded", "partially_succeeded", "failed"})
8491

@@ -918,7 +925,7 @@ def _poll_query_run(
918925
last = None
919926
while time.monotonic() < deadline:
920927
last = runs.get_query_run(query_run_id)
921-
if last.status in _TERMINAL:
928+
if last.status not in _RUN_IN_FLIGHT:
922929
return last
923930
time.sleep(interval_s)
924931
raise TimeoutError(
@@ -1027,7 +1034,7 @@ def _wait_result_ready(
10271034
last = results.get_result(result_id)
10281035
if last.status == "ready":
10291036
return last
1030-
if last.status in _RESULT_FAILURE:
1037+
if last.status not in _RESULT_IN_FLIGHT:
10311038
raise RuntimeError(last.error_message or f"Result {last.status}")
10321039
time.sleep(interval_s)
10331040
raise TimeoutError(

‎hotdata_framework/managed_client.py‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ class ManagedDatabaseClient:
4040
database lifecycle.
4141
"""
4242

43+
# The only status a query run reports while still in flight. Listed this way
44+
# round so an unknown status raises immediately rather than being polled to
45+
# the timeout; see `_await_query_run`.
46+
_RUN_IN_FLIGHT = frozenset({"running"})
47+
4348
_QUERY_TIMEOUT_SECONDS = 300.0
4449
_POLL_INTERVAL_SECONDS = 0.4
4550
_MAX_BACKOFF_SECONDS = 30.0
@@ -178,8 +183,12 @@ def _await_query_run(self, query_run_id: str, *, database_id: str) -> str | None
178183
raise HotdataTransientError(
179184
run.error_message or f"Query run {query_run_id} was interrupted"
180185
)
181-
if run.status == "failed":
182-
raise RuntimeError(run.error_message or f"Query run {query_run_id} failed")
186+
# Anything not still in flight is terminal, whether or not this
187+
# client has heard of it. Enumerating the terminal statuses instead
188+
# would poll an unrecognised one to the timeout -- which is exactly
189+
# how `interrupted` came to be waited out for five minutes.
190+
if run.status not in self._RUN_IN_FLIGHT:
191+
raise RuntimeError(run.error_message or f"Query run {query_run_id} {run.status}")
183192
time.sleep(self._POLL_INTERVAL_SECONDS)
184193
raise TimeoutError(
185194
f"Query run {query_run_id} did not finish within {self._QUERY_TIMEOUT_SECONDS}s"

‎tests/test_client.py‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,20 +245,52 @@ def test_list_qualified_table_names_passes_connection_id():
245245
assert it.call_args.kwargs["connection_id"] == "conn_a"
246246

247247

248-
def test_wait_result_ready_raises_on_cancelled():
248+
def test_wait_result_ready_raises_on_any_status_that_is_not_in_flight():
249+
"""A result neither `ready` nor still being saved is terminal.
250+
251+
The poll enumerates the in-flight statuses rather than the terminal ones, so
252+
a status this client has never heard of raises on the first pass instead of
253+
being waited out. `cancelled` -- which this poll listed as terminal, and
254+
which the API does not send -- is as good a stand-in as any.
255+
"""
249256
client = HotdataClient("k", "ws", host="https://api.hotdata.dev")
250257

251258
class FakeResultsApi:
252259
def get_result(self, result_id: str):
253-
return SimpleNamespace(status="cancelled", error_message=None)
260+
return SimpleNamespace(status="something_new", error_message=None)
254261

255262
with (
256263
patch.object(client, "_results_api", return_value=FakeResultsApi()),
257-
pytest.raises(RuntimeError, match="cancelled"),
264+
pytest.raises(RuntimeError, match="something_new"),
258265
):
259266
client._wait_result_ready("res_1", timeout_s=0.1, interval_s=0)
260267

261268

269+
def test_poll_query_run_returns_promptly_on_interrupted():
270+
"""`interrupted` is terminal, so the poll must stop on it.
271+
272+
It was absent from the terminal set, so an interrupted run was polled for the
273+
full timeout and then raised `TimeoutError` -- hiding a retryable condition
274+
behind a five-minute wait, behind an error naming the wrong problem.
275+
"""
276+
client = HotdataClient("k", "ws", host="https://api.hotdata.dev")
277+
calls: list[str] = []
278+
279+
class FakeQueryRunsApi:
280+
def get_query_run(self, query_run_id: str):
281+
calls.append(query_run_id)
282+
return SimpleNamespace(
283+
status="interrupted", error_message="instance lost", result_id=None
284+
)
285+
286+
with patch.object(client, "_query_runs_api", return_value=FakeQueryRunsApi()):
287+
run = client._poll_query_run("qrun_1", timeout_s=30.0, interval_s=0)
288+
289+
assert run.status == "interrupted"
290+
# One request, not a timeout's worth.
291+
assert len(calls) == 1
292+
293+
262294
def test_connection_id_by_name_raises_on_duplicate_names():
263295
client = HotdataClient("k", "ws", host="https://api.hotdata.dev")
264296
listing = SimpleNamespace(

‎tests/test_managed_client.py‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -673,7 +673,12 @@ def get_result_arrow(self, result_id: str, **kwargs: Any) -> pa.Table:
673673

674674
assert table is not None
675675
assert len(requests) == 1
676-
assert requests[0].var_async is True
676+
# The attribute alone is not the contract: the field only reaches the server
677+
# as `async`. Were the alias missing or misspelled, the attribute assertion
678+
# would still pass, the server would ignore the field and answer
679+
# synchronously with a full preview -- restoring the exact behaviour this
680+
# change removes, silently.
681+
assert requests[0].to_dict()["async"] is True
677682

678683

679684
def test_async_reply_is_followed_through_to_arrow(
@@ -742,3 +747,49 @@ def get_result_arrow(self, result_id: str, **kwargs: Any) -> pa.Table:
742747
"get_query_run:succeeded",
743748
"arrow:rslt-from-run",
744749
]
750+
751+
752+
def test_unknown_run_status_fails_fast_instead_of_polling_to_the_timeout(
753+
monkeypatch: pytest.MonkeyPatch,
754+
) -> None:
755+
"""Only `running` means "still in flight"; anything else is terminal.
756+
757+
Enumerating the terminal statuses is what let `interrupted` be polled for
758+
five minutes, so the test runs the other way round: a status this client has
759+
never seen raises on the first pass and names itself.
760+
"""
761+
calls: list[str] = []
762+
763+
class FakeQueryApi:
764+
def __init__(self, api: object) -> None:
765+
pass
766+
767+
def query(self, request: object, *, x_database_id: str) -> AsyncQueryResponse:
768+
return _async_query_response()
769+
770+
class FakeQueryRunsApi:
771+
def __init__(self, api: object) -> None:
772+
pass
773+
774+
def get_query_run(self, query_run_id: str, **kwargs: Any) -> Any:
775+
calls.append("get_query_run")
776+
return SimpleNamespace(status="evicted", result_id=None, error_message=None)
777+
778+
monkeypatch.setattr(mc, "QueryApi", FakeQueryApi)
779+
monkeypatch.setattr(mc, "QueryRunsApi", FakeQueryRunsApi)
780+
monkeypatch.setattr(mc.time, "sleep", lambda _seconds: None)
781+
782+
client = mc.ManagedDatabaseClient(
783+
api_key="k",
784+
workspace_id="w",
785+
api_base_url="https://example.test",
786+
max_retries=1,
787+
retry_backoff_seconds=0.0,
788+
)
789+
client._runtime = _fake_runtime()
790+
791+
with pytest.raises(HotdataTerminalError, match="evicted"):
792+
client.fetch_table(database="mydb", schema="public", table="orders")
793+
794+
# One poll, not a timeout's worth.
795+
assert len(calls) == 1

0 commit comments

Comments
 (0)