From 6ac2e67012418fa30b3a315f4b7a8df4b4bc1c19 Mon Sep 17 00:00:00 2001 From: Anoop Narang Date: Tue, 1 Sep 2026 17:57:49 +0530 Subject: [PATCH 1/3] fix: own the readiness poll instead of inheriting it `HotdataSourceClient` subclasses `ManagedDatabaseClient` and called `self._poll(...)` in both readiness waits without defining one. That worked only because the framework happened to provide it. It no longer does: its managed client now waits on the query run rather than polling the result body for a status, which left the shared helper with no second caller there. So both waits here would raise `AttributeError` on the next `hotdata-framework` release this package's pin accepts, and no test would have caught it -- the method resolved fine against the installed version. Reaching into a private method on a base class from another distribution is what made that possible, so the poll is defined here now and the tests assert ownership rather than mere reachability. Two status bugs came along with the old shared helper and are fixed in the local copy. `interrupted` was in neither branch, so an interrupted run satisfied nothing and waited out the full 300s bound before reporting a timeout that named the wrong problem. `cancelled` was treated as terminal and is not a status this API sends. An unrecognised status still keeps waiting -- calling it terminal would fail every read the moment a status is added upstream, where waiting costs one slow call -- and the timeout now names the status it last saw, which is the part that was actually missing. The two test files that faked `managed_client.ResultsApi` now fake `QueryRunsApi`, since that is where readiness comes from. Suite passes against both the pinned framework 0.12.0 and framework main, so this can land either side of that release. --- CHANGELOG.md | 12 ++++ src/hotdata_dlt_destination/source_client.py | 67 ++++++++++++++++++-- tests/test_client.py | 28 +++++--- tests/test_e2e_inmemory.py | 28 ++++++-- tests/test_source_client.py | 65 +++++++++++++++++++ 5 files changed, 180 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 877e101..efeaac0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,18 @@ Changelog style — terse "bullet + brief why": **Breaking:**. See the released entries below for the target density. --> +### Fixed + +- `HotdataSourceClient` now defines its own `_poll` rather than inheriting one from + `ManagedDatabaseClient`, which no longer provides it: the framework's managed + client waits on the query run instead of on the result body, and dropped the + shared helper with it. Both readiness waits here would otherwise raise + `AttributeError` on the next `hotdata-framework` release this pin accepts. The + local copy treats `interrupted` as terminal — it was absent, so an interrupted + run waited out the full 300s bound and then reported a timeout — and drops + `cancelled`, which this API does not send; an unrecognised status keeps waiting + and the timeout names it. + ## [0.15.0] - 2026-08-27 ### Changed diff --git a/src/hotdata_dlt_destination/source_client.py b/src/hotdata_dlt_destination/source_client.py index e77980b..6404772 100644 --- a/src/hotdata_dlt_destination/source_client.py +++ b/src/hotdata_dlt_destination/source_client.py @@ -44,13 +44,14 @@ trusted the persisted result is taken regardless of cost: slower and complete beats faster and silently short. The count is a second request rather than folded into the readiness poll because -the poll lives in the shared client, and duplicating its sync/async branching to -save ~12% of the latency would risk the two drifting. +the poll would then have to branch on sync/async the way the read above it does, +and duplicating that to save ~12% of the latency would risk the two drifting. """ from __future__ import annotations -from typing import TYPE_CHECKING, Any, TypeVar +import time +from typing import TYPE_CHECKING, Any, Protocol, TypeVar import pyarrow as pa import pyarrow.types as pat @@ -64,7 +65,7 @@ from hotdata_framework.managed_client import ManagedDatabaseClient if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Callable, Iterator # Rows per Arrow batch handed to dlt. Bounds peak memory for one batch; it does # not bound the query, whose size the caller controls with its own LIMIT. @@ -75,6 +76,16 @@ T = TypeVar("T", pa.Table, pa.RecordBatch) +class _HasStatus(Protocol): + """What `_poll` needs off a run or a result: a status, and a reason.""" + + status: str + error_message: str | None + + +_S = TypeVar("_S", bound=_HasStatus) + + class IncompleteReadError(RuntimeError): """A read returned fewer rows than the result holds. @@ -320,6 +331,49 @@ def _resolve(self, response: object, *, database_id: str) -> tuple[str, int] | N "so whether it produced rows cannot be established" ) + # Statuses that end a wait without satisfying it. `interrupted` is terminal + # and belongs here -- omitting it made an interrupted run wait out the whole + # 300s bound and then report a timeout, which names the wrong problem. + # `cancelled`, which this poll used to list, is not a status the API sends. + _POLL_FAILURES = frozenset({"failed", "interrupted"}) + + def _poll( + self, + fetch: Callable[[], _S], + *, + is_ready: Callable[[_S], bool], + describe: str, + ) -> _S: + """Poll ``fetch`` until ``is_ready`` holds, or raise on failure/timeout. + + Owned here rather than inherited. The base class used to provide this and + no longer does: it now waits on the query run instead, so the generic + helper had no second caller left there. This client still needs a wait of + its own shape -- retry per request, not per wait, and ``limit=0`` on the + readiness check -- so it keeps one, and a private method on a base class + in another package was never a safe thing to build on. + + An unrecognised status keeps polling rather than ending the wait. Calling + an unknown status terminal would be easier to diagnose and much worse to + live with: one status added upstream would fail every read at once, where + waiting costs a single slow call. So the timeout names the status it last + saw, which is what makes an omission findable. + """ + deadline = time.monotonic() + self._QUERY_TIMEOUT_SECONDS + last_status: str | None = None + while time.monotonic() < deadline: + obj = fetch() + last_status = obj.status + if is_ready(obj): + return obj + if obj.status in self._POLL_FAILURES: + raise RuntimeError(obj.error_message or f"{describe} {obj.status}") + time.sleep(self._POLL_INTERVAL_SECONDS) + raise TimeoutError( + f"{describe} timed out after {self._QUERY_TIMEOUT_SECONDS}s " + f"(last status: {last_status})" + ) + def _await_result(self, result_id: str, *, database_id: str) -> tuple[str, int]: """Wait for a result to be ready, and take its row count off the way out. @@ -338,9 +392,8 @@ def _await_result(self, result_id: str, *, database_id: str) -> tuple[str, int]: response because the total is a HEADER: reading it means the ``_with_http_info`` variant, whose wrapper puts the model one level down at ``.data``, and ``_poll`` inspects ``obj.status`` directly to spot a - failed or cancelled result. Passing the wrapper in breaks that check, so - the poll keeps the plain model and the header is fetched once, after - ready. + failed result. Passing the wrapper in breaks that check, so the poll + keeps the plain model and the header is fetched once, after ready. """ results = ResultsApi(self._runtime.api) self._poll( diff --git a/tests/test_client.py b/tests/test_client.py index 74e5890..e6678b5 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -292,7 +292,7 @@ def test_list_managed_tables_by_id(monkeypatch) -> None: def _patch_query_apis(monkeypatch, arrow_table, *, arrow_in_hotdata_client: bool) -> dict: from hotdata.models.query_response import QueryResponse as _QR - scopes: dict[str, list] = {"result": [], "arrow": []} + scopes: dict[str, list] = {"run": [], "arrow": []} class FakeQueryApi: def __init__(self, api): @@ -311,13 +311,25 @@ def query(self, request, *, x_database_id): execution_time_ms=1, ) - class FakeResultsApi: + class FakeQueryRunsApi: + """Readiness comes off the query run, not off the result body. + + The framework used to poll `GET /results/{id}` for it, which meant + downloading the whole result to read one status field. It now waits on + the run, so this is the read that has to carry the database scope. + """ + def __init__(self, api): pass - def get_result(self, result_id, *, x_database_id=None): - scopes["result"].append(x_database_id) - return SimpleNamespace(status="ready", result_id=result_id, error_message=None) + def get_query_run(self, query_run_id, *, x_database_id=None): + scopes["run"].append(x_database_id) + return SimpleNamespace( + status="succeeded", + result_id="r1", + error_message=None, + warning_message=None, + ) class FakeArrowResultsApi: def __init__(self, api): @@ -328,7 +340,7 @@ def get_result_arrow(self, result_id, *, x_database_id): return arrow_table monkeypatch.setattr("hotdata_framework.managed_client.QueryApi", FakeQueryApi) - monkeypatch.setattr("hotdata_framework.managed_client.ResultsApi", FakeResultsApi) + monkeypatch.setattr("hotdata_framework.managed_client.QueryRunsApi", FakeQueryRunsApi) monkeypatch.setattr("hotdata_framework.managed_client.ArrowResultsApi", FakeArrowResultsApi) if arrow_in_hotdata_client: monkeypatch.setattr( @@ -346,7 +358,7 @@ def test_execute_sql_carries_database_scope(monkeypatch) -> None: client = _client(_Runtime(), config=_cfg(database_id="db_99")) table = client.execute_sql('SELECT * FROM "default"."public"."spans"') assert table.num_rows == 2 - assert scopes["result"] == ["db_99"] + assert scopes["run"] == ["db_99"] assert scopes["arrow"] == ["db_99"] client.close() @@ -360,7 +372,7 @@ def test_fetch_table_carries_database_scope(monkeypatch) -> None: client = _client(rt, config=_cfg(database_id="db_42")) table = client.fetch_table(schema="public", table="orders") assert table is not None and table.num_rows == 1 - assert scopes["result"] == ["db_42"] + assert scopes["run"] == ["db_42"] assert scopes["arrow"] == ["db_42"] client.close() diff --git a/tests/test_e2e_inmemory.py b/tests/test_e2e_inmemory.py index a3e1f0b..86967f7 100644 --- a/tests/test_e2e_inmemory.py +++ b/tests/test_e2e_inmemory.py @@ -44,6 +44,9 @@ def __init__(self) -> None: # (db, schema, table) -> (partition_by, sorted_by) as declared at creation self.layouts: dict[tuple, tuple[list, list]] = {} self.tables: dict[tuple, pa.Table] = {} + # The id minted by the most recent query, so the query-run fake can name + # the result the run produced. + self.last_result_id: str | None = None self.uploads: dict[str, pa.Table] = {} self.results: dict[str, pa.Table] = {} self.load_counts: dict[tuple, int] = {} @@ -210,6 +213,10 @@ def query(self, request, *, x_database_id): be = _ACTIVE["backend"] database = be.id_to_name[x_database_id] result = be.execute_sql(database, request.sql) + # The run is what names the result now, so the id has to be reachable + # from the run fake as well as from this reply. + rid = be.store_result(result) + be.last_result_id = rid return QueryResponse( columns=[], rows=[], @@ -217,18 +224,29 @@ def query(self, request, *, x_database_id): preview_row_count=0, truncated=False, nullable=[], - result_id=be.store_result(result), + result_id=rid, query_run_id="qr", execution_time_ms=1, ) -class _FakeResultsApi: +class _FakeQueryRunsApi: + """Readiness, and the id of the result the run produced. + + The framework waits on the query run rather than polling the result body for + a status, so this replaces the results fake that used to answer `ready`. + """ + def __init__(self, api): pass - def get_result(self, result_id, *, x_database_id=None): - return SimpleNamespace(status="ready", result_id=result_id, error_message=None) + def get_query_run(self, query_run_id, *, x_database_id=None): + return SimpleNamespace( + status="succeeded", + result_id=_ACTIVE["backend"].last_result_id, + error_message=None, + warning_message=None, + ) class _FakeArrowResultsApi: @@ -265,7 +283,7 @@ def backend(monkeypatch): be = InMemoryBackend() _ACTIVE["backend"] = be monkeypatch.setattr(mc, "QueryApi", _FakeQueryApi) - monkeypatch.setattr(mc, "ResultsApi", _FakeResultsApi) + monkeypatch.setattr(mc, "QueryRunsApi", _FakeQueryRunsApi) monkeypatch.setattr(mc, "ArrowResultsApi", _FakeArrowResultsApi) # hotdata_client.py also references ArrowResultsApi (for execute_sql) and # DatabasesApi (for the id-first bind). diff --git a/tests/test_source_client.py b/tests/test_source_client.py index 8794f14..18afd67 100644 --- a/tests/test_source_client.py +++ b/tests/test_source_client.py @@ -1139,3 +1139,68 @@ def test_read_rows_returns_empty_for_an_unpersisted_but_verified_zero() -> None: client = FakeSourceClient(table=empty, truncated=False, result_id=None) assert client.read_rows("SELECT * FROM public.t", database_id=DB) == [] assert client.fetch_calls == 0 + + +def test_the_readiness_poll_is_owned_here_not_inherited() -> None: + """`_poll` must be defined on this class, not merely reachable through it. + + It used to live on `ManagedDatabaseClient`, and this client called it without + defining one. When that base stopped waiting on the result and started + waiting on the query run, the generic helper lost its second caller there and + was removed -- and both readiness waits here would have become + `AttributeError` on the next framework release that this package's pin + accepted. Nothing in this suite would have caught it, because the method + resolved fine against the version installed at the time. + + Reaching into a private method on a base class from another distribution is + the thing that made that possible, so the assertion is about ownership rather + than about the attribute merely existing. + """ + assert "_poll" in HotdataSourceClient.__dict__, ( + "_poll must be defined on HotdataSourceClient; inheriting it from " + "hotdata-framework couples this package to a private method it does not own" + ) + + +def test_poll_stops_on_a_run_that_was_interrupted() -> None: + """`interrupted` is terminal, and was in neither status this poll knew. + + An interrupted run therefore satisfied no branch, so the wait ran to its full + 300-second bound and then reported a timeout -- naming the wrong problem, and + charging five minutes for it. + """ + client = HotdataSourceClient.__new__(HotdataSourceClient) + calls: list[str] = [] + + def fetch() -> SimpleNamespace: + calls.append("poll") + return SimpleNamespace(status="interrupted", error_message="instance lost") + + with pytest.raises(RuntimeError, match="instance lost"): + client._poll(fetch, is_ready=lambda r: r.status == "ready", describe="Result r1") + + # One request, not a timeout's worth. + assert calls == ["poll"] + + +def test_poll_keeps_waiting_on_an_unknown_status_and_names_it_on_timeout() -> None: + """An unrecognised status waits rather than ending the wait. + + Treating an unknown status as terminal is the easier failure to diagnose and + much the worse one to suffer: a single status added upstream would fail every + read at once, where waiting costs one slow call. What makes the omission + findable is the timeout naming the status it last saw -- which is exactly + what was missing while `interrupted` went unrecognised. + """ + client = HotdataSourceClient.__new__(HotdataSourceClient) + + def fetch() -> SimpleNamespace: + return SimpleNamespace(status="something_new", error_message=None) + + with ( + pytest.MonkeyPatch.context() as mp, + pytest.raises(TimeoutError, match="something_new"), + ): + mp.setattr(HotdataSourceClient, "_QUERY_TIMEOUT_SECONDS", 0.05) + mp.setattr(HotdataSourceClient, "_POLL_INTERVAL_SECONDS", 0.0) + client._poll(fetch, is_ready=lambda r: r.status == "ready", describe="Result r1") From f8935bf0222fe526003b7be7fdac460082a9517f Mon Sep 17 00:00:00 2001 From: Anoop Narang Date: Tue, 1 Sep 2026 18:05:08 +0530 Subject: [PATCH 2/3] fix: fake both readiness endpoints, and own the wait's constants The test fakes bound the suite to one framework version. Readiness moved endpoint between releases -- 0.13.0 polls `ResultsApi.get_result` for a synchronous query reply, later versions wait on the query run -- and swapping the fake from one to the other traded a break against the newer framework for a break against the locked one, which is the version CI installs. Both are faked now, `ResultsApi` with `raising=False` since later frameworks do not import it, and the scope assertions accept whichever endpoint answered. Also owns the two constants the wait reads. `_poll` was defined here while still taking `_QUERY_TIMEOUT_SECONDS` and `_POLL_INTERVAL_SECONDS` off a base class in another distribution -- the same coupling that broke these waits once already, left one deletion away from doing it again. Same values, so no behaviour change, and the ownership test now covers all three names rather than only the method. --- CHANGELOG.md | 4 ++- src/hotdata_dlt_destination/source_client.py | 8 ++++++ tests/test_client.py | 30 +++++++++++++------- tests/test_e2e_inmemory.py | 18 ++++++++++-- tests/test_source_client.py | 11 ++++--- 5 files changed, 54 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efeaac0..415cca0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,9 @@ Changelog style — terse "bullet + brief why": local copy treats `interrupted` as terminal — it was absent, so an interrupted run waited out the full 300s bound and then reported a timeout — and drops `cancelled`, which this API does not send; an unrecognised status keeps waiting - and the timeout names it. + and the timeout names it. `_QUERY_TIMEOUT_SECONDS` and `_POLL_INTERVAL_SECONDS` + are defined here too, at the same values, so the wait rents nothing from the + base class. ## [0.15.0] - 2026-08-27 diff --git a/src/hotdata_dlt_destination/source_client.py b/src/hotdata_dlt_destination/source_client.py index 6404772..5aeb215 100644 --- a/src/hotdata_dlt_destination/source_client.py +++ b/src/hotdata_dlt_destination/source_client.py @@ -331,6 +331,14 @@ def _resolve(self, response: object, *, database_id: str) -> tuple[str, int] | N "so whether it produced rows cannot be established" ) + # The wait's own bounds, rather than the base class's. Same values it uses, + # so behaviour is unchanged -- but `_poll` reading two private attributes off + # a base class in another distribution is the coupling that broke these waits + # once already, and owning the method while renting its constants would leave + # the same failure one deletion away. + _QUERY_TIMEOUT_SECONDS = 300.0 + _POLL_INTERVAL_SECONDS = 0.4 + # Statuses that end a wait without satisfying it. `interrupted` is terminal # and belongs here -- omitting it made an interrupted run wait out the whole # 300s bound and then report a timeout, which names the wrong problem. diff --git a/tests/test_client.py b/tests/test_client.py index e6678b5..903a9fb 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -292,7 +292,7 @@ def test_list_managed_tables_by_id(monkeypatch) -> None: def _patch_query_apis(monkeypatch, arrow_table, *, arrow_in_hotdata_client: bool) -> dict: from hotdata.models.query_response import QueryResponse as _QR - scopes: dict[str, list] = {"run": [], "arrow": []} + scopes: dict[str, list] = {"run": [], "result": [], "arrow": []} class FakeQueryApi: def __init__(self, api): @@ -311,14 +311,11 @@ def query(self, request, *, x_database_id): execution_time_ms=1, ) + # Which endpoint answers "is it ready?" depends on the installed framework: + # up to 0.13 it polls `GET /results/{id}`, after that it waits on the query + # run. Both are faked so this suite does not silently bind to one of them -- + # the assertion below accepts whichever was used. class FakeQueryRunsApi: - """Readiness comes off the query run, not off the result body. - - The framework used to poll `GET /results/{id}` for it, which meant - downloading the whole result to read one status field. It now waits on - the run, so this is the read that has to carry the database scope. - """ - def __init__(self, api): pass @@ -331,6 +328,14 @@ def get_query_run(self, query_run_id, *, x_database_id=None): warning_message=None, ) + class FakeResultsApi: + def __init__(self, api): + pass + + def get_result(self, result_id, *, x_database_id=None, limit=None): + scopes["result"].append(x_database_id) + return SimpleNamespace(status="ready", result_id=result_id, error_message=None) + class FakeArrowResultsApi: def __init__(self, api): pass @@ -341,6 +346,11 @@ def get_result_arrow(self, result_id, *, x_database_id): monkeypatch.setattr("hotdata_framework.managed_client.QueryApi", FakeQueryApi) monkeypatch.setattr("hotdata_framework.managed_client.QueryRunsApi", FakeQueryRunsApi) + # `raising=False`: the symbol is gone from the module in later framework + # versions, which stop reading a result body to learn its status. + monkeypatch.setattr( + "hotdata_framework.managed_client.ResultsApi", FakeResultsApi, raising=False + ) monkeypatch.setattr("hotdata_framework.managed_client.ArrowResultsApi", FakeArrowResultsApi) if arrow_in_hotdata_client: monkeypatch.setattr( @@ -358,7 +368,7 @@ def test_execute_sql_carries_database_scope(monkeypatch) -> None: client = _client(_Runtime(), config=_cfg(database_id="db_99")) table = client.execute_sql('SELECT * FROM "default"."public"."spans"') assert table.num_rows == 2 - assert scopes["run"] == ["db_99"] + assert (scopes["run"] or scopes["result"]) == ["db_99"] assert scopes["arrow"] == ["db_99"] client.close() @@ -372,7 +382,7 @@ def test_fetch_table_carries_database_scope(monkeypatch) -> None: client = _client(rt, config=_cfg(database_id="db_42")) table = client.fetch_table(schema="public", table="orders") assert table is not None and table.num_rows == 1 - assert scopes["run"] == ["db_42"] + assert (scopes["run"] or scopes["result"]) == ["db_42"] assert scopes["arrow"] == ["db_42"] client.close() diff --git a/tests/test_e2e_inmemory.py b/tests/test_e2e_inmemory.py index 86967f7..e181640 100644 --- a/tests/test_e2e_inmemory.py +++ b/tests/test_e2e_inmemory.py @@ -230,11 +230,22 @@ def query(self, request, *, x_database_id): ) +class _FakeResultsApi: + """Readiness, as frameworks up to 0.13 ask for it.""" + + def __init__(self, api): + pass + + def get_result(self, result_id, *, x_database_id=None, limit=None): + return SimpleNamespace(status="ready", result_id=result_id, error_message=None) + + class _FakeQueryRunsApi: """Readiness, and the id of the result the run produced. - The framework waits on the query run rather than polling the result body for - a status, so this replaces the results fake that used to answer `ready`. + Later frameworks wait on the query run rather than polling the result body + for a status. Both fakes are installed so this suite works either side of + that change instead of binding to one framework version. """ def __init__(self, api): @@ -284,6 +295,9 @@ def backend(monkeypatch): _ACTIVE["backend"] = be monkeypatch.setattr(mc, "QueryApi", _FakeQueryApi) monkeypatch.setattr(mc, "QueryRunsApi", _FakeQueryRunsApi) + # `raising=False`: later frameworks drop this import, having stopped reading + # a result body to learn its status. + monkeypatch.setattr(mc, "ResultsApi", _FakeResultsApi, raising=False) monkeypatch.setattr(mc, "ArrowResultsApi", _FakeArrowResultsApi) # hotdata_client.py also references ArrowResultsApi (for execute_sql) and # DatabasesApi (for the id-first bind). diff --git a/tests/test_source_client.py b/tests/test_source_client.py index 18afd67..2db86d3 100644 --- a/tests/test_source_client.py +++ b/tests/test_source_client.py @@ -1156,10 +1156,11 @@ def test_the_readiness_poll_is_owned_here_not_inherited() -> None: the thing that made that possible, so the assertion is about ownership rather than about the attribute merely existing. """ - assert "_poll" in HotdataSourceClient.__dict__, ( - "_poll must be defined on HotdataSourceClient; inheriting it from " - "hotdata-framework couples this package to a private method it does not own" - ) + for name in ("_poll", "_QUERY_TIMEOUT_SECONDS", "_POLL_INTERVAL_SECONDS"): + assert name in HotdataSourceClient.__dict__, ( + f"{name} must be defined on HotdataSourceClient; inheriting it from " + "hotdata-framework couples this package to a private name it does not own" + ) def test_poll_stops_on_a_run_that_was_interrupted() -> None: @@ -1201,6 +1202,8 @@ def fetch() -> SimpleNamespace: pytest.MonkeyPatch.context() as mp, pytest.raises(TimeoutError, match="something_new"), ): + # Both constants belong to this class, so this patches nothing it + # does not own. mp.setattr(HotdataSourceClient, "_QUERY_TIMEOUT_SECONDS", 0.05) mp.setattr(HotdataSourceClient, "_POLL_INTERVAL_SECONDS", 0.0) client._poll(fetch, is_ready=lambda r: r.status == "ready", describe="Result r1") From 056b1f9db08779702b1baf40a24dcd0c8272f675 Mon Sep 17 00:00:00 2001 From: Anoop Narang Date: Tue, 1 Sep 2026 18:08:49 +0530 Subject: [PATCH 3/3] fix: name the wait's constants distinctly from the base class's Owning the values was right; reusing the base class's names for them was not. Matching `_QUERY_TIMEOUT_SECONDS` and `_POLL_INTERVAL_SECONDS` overrode them for every *inherited* method too, so a framework release that retuned either would have stopped reaching this client's inherited waits -- silently, which is worse than the coupling being fixed. `_POLL_TIMEOUT_SECONDS` and `_POLL_SLEEP_SECONDS` give the same ownership without shadowing anything. Verified: the base still supplies its own constants to inherited methods, and this class no longer defines either of their names. --- CHANGELOG.md | 7 ++++--- src/hotdata_dlt_destination/source_client.py | 21 ++++++++++---------- tests/test_source_client.py | 11 +++++----- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 415cca0..174cc4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,9 +29,10 @@ Changelog style — terse "bullet + brief why": local copy treats `interrupted` as terminal — it was absent, so an interrupted run waited out the full 300s bound and then reported a timeout — and drops `cancelled`, which this API does not send; an unrecognised status keeps waiting - and the timeout names it. `_QUERY_TIMEOUT_SECONDS` and `_POLL_INTERVAL_SECONDS` - are defined here too, at the same values, so the wait rents nothing from the - base class. + and the timeout names it. The wait reads its own `_POLL_TIMEOUT_SECONDS` / + `_POLL_SLEEP_SECONDS` rather than the base class's constants, so it rents + nothing -- and under distinct names, which avoids shadowing the base values for + inherited waits. ## [0.15.0] - 2026-08-27 diff --git a/src/hotdata_dlt_destination/source_client.py b/src/hotdata_dlt_destination/source_client.py index 5aeb215..35cbc5a 100644 --- a/src/hotdata_dlt_destination/source_client.py +++ b/src/hotdata_dlt_destination/source_client.py @@ -331,13 +331,14 @@ def _resolve(self, response: object, *, database_id: str) -> tuple[str, int] | N "so whether it produced rows cannot be established" ) - # The wait's own bounds, rather than the base class's. Same values it uses, - # so behaviour is unchanged -- but `_poll` reading two private attributes off - # a base class in another distribution is the coupling that broke these waits - # once already, and owning the method while renting its constants would leave - # the same failure one deletion away. - _QUERY_TIMEOUT_SECONDS = 300.0 - _POLL_INTERVAL_SECONDS = 0.4 + # This wait's own bounds. `_poll` reading two private attributes off a base + # class in another distribution is the coupling that broke these waits once + # already, so it reads its own -- but under names of their own rather than + # the base class's, which the first attempt reused. Matching those names + # would shadow them for every *inherited* method too, so a framework release + # that retuned either value would silently stop reaching this client. + _POLL_TIMEOUT_SECONDS = 300.0 + _POLL_SLEEP_SECONDS = 0.4 # Statuses that end a wait without satisfying it. `interrupted` is terminal # and belongs here -- omitting it made an interrupted run wait out the whole @@ -367,7 +368,7 @@ def _poll( waiting costs a single slow call. So the timeout names the status it last saw, which is what makes an omission findable. """ - deadline = time.monotonic() + self._QUERY_TIMEOUT_SECONDS + deadline = time.monotonic() + self._POLL_TIMEOUT_SECONDS last_status: str | None = None while time.monotonic() < deadline: obj = fetch() @@ -376,9 +377,9 @@ def _poll( return obj if obj.status in self._POLL_FAILURES: raise RuntimeError(obj.error_message or f"{describe} {obj.status}") - time.sleep(self._POLL_INTERVAL_SECONDS) + time.sleep(self._POLL_SLEEP_SECONDS) raise TimeoutError( - f"{describe} timed out after {self._QUERY_TIMEOUT_SECONDS}s " + f"{describe} timed out after {self._POLL_TIMEOUT_SECONDS}s " f"(last status: {last_status})" ) diff --git a/tests/test_source_client.py b/tests/test_source_client.py index 2db86d3..39a6026 100644 --- a/tests/test_source_client.py +++ b/tests/test_source_client.py @@ -1156,7 +1156,7 @@ def test_the_readiness_poll_is_owned_here_not_inherited() -> None: the thing that made that possible, so the assertion is about ownership rather than about the attribute merely existing. """ - for name in ("_poll", "_QUERY_TIMEOUT_SECONDS", "_POLL_INTERVAL_SECONDS"): + for name in ("_poll", "_POLL_TIMEOUT_SECONDS", "_POLL_SLEEP_SECONDS"): assert name in HotdataSourceClient.__dict__, ( f"{name} must be defined on HotdataSourceClient; inheriting it from " "hotdata-framework couples this package to a private name it does not own" @@ -1202,8 +1202,9 @@ def fetch() -> SimpleNamespace: pytest.MonkeyPatch.context() as mp, pytest.raises(TimeoutError, match="something_new"), ): - # Both constants belong to this class, so this patches nothing it - # does not own. - mp.setattr(HotdataSourceClient, "_QUERY_TIMEOUT_SECONDS", 0.05) - mp.setattr(HotdataSourceClient, "_POLL_INTERVAL_SECONDS", 0.0) + # Both constants belong to this class -- and are deliberately not named + # after the base class's, which these would otherwise shadow for every + # inherited wait as well. + mp.setattr(HotdataSourceClient, "_POLL_TIMEOUT_SECONDS", 0.05) + mp.setattr(HotdataSourceClient, "_POLL_SLEEP_SECONDS", 0.0) client._poll(fetch, is_ready=lambda r: r.status == "ready", describe="Result r1")