Skip to content

Commit 62c5164

Browse files
authored
fix(http): stop replaying POST on a gateway 5xx (#61)
* fix(http): stop replaying POST on a gateway 5xx HotdataClient passed its own `retries=` into Configuration, which replaces the generated SDK's retry policy wholesale. That policy listed POST in `allowed_methods` alongside a (502, 503, 504) forcelist, so an intermediary timing out a long request produced a silent, identical re-POST while the server was still working on the first one. For a non-idempotent call — a data load — the duplicate collides with the write lock the original holds and is refused, and the caller sees contention rather than the timeout that actually happened. The override is removed so the SDK's own default applies. It is the policy this wrapper was reaching for: hotdata._retry retries a pre-response connection reset on any method (the stale pooled socket case, where the server did no work) while leaving read timeouts and status retries idempotent-only. hotdata_framework/http.py existed only to build that policy and had no other callers, so it goes with it. * docs(retry): correct the test comment and the stale README bullet The test's ProtocolError with no cause is not a read timeout — those are ReadTimeoutError and never reach that branch. Say what the value actually is, and note that _is_connection_error is urllib3-private on an unpinned transitive dependency. README advertised configuring SDK retries; that is now the SDK's own default, which this package deliberately leaves alone.
1 parent 08bd929 commit 62c5164

5 files changed

Lines changed: 92 additions & 20 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- A `POST` is no longer replayed because of a response status. `HotdataClient`
13+
passed its own `retries=` into `Configuration`, which replaced the generated
14+
SDK's policy wholesale with one listing `POST` in `allowed_methods` alongside
15+
a `(502, 503, 504)` forcelist — so an intermediary timing out a long request
16+
produced a silent, identical re-`POST` while the server was still working on
17+
the first one. For a load that is not idempotent: the duplicate collides with
18+
the write lock the original holds and is refused.
19+
20+
The override is removed and the SDK's own default now applies. It is the
21+
policy this wrapper was reaching for — `hotdata._retry` retries a
22+
*pre-response* connection reset (the stale pooled socket case, where the
23+
server did no work) on any method, while leaving read timeouts and status
24+
retries idempotent-only.
25+
26+
### Removed
27+
28+
- `hotdata_framework.http` and `default_http_retries()`. The module existed only
29+
to build the retry policy above and had no other callers. It predates
30+
`hotdata._retry`, which supersedes it.
31+
1032

1133
## [0.11.0] - 2026-08-11
1234

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ Runtime boundary and guarantees are defined in `CONTRACT.md`.
1010

1111
- **Environment-driven client setup** — create clients from `HOTDATA_API_KEY`, optional `HOTDATA_API_URL`, and `HOTDATA_WORKSPACE`.
1212
- **Workspace resolution** — choose an explicit workspace from env, otherwise discover workspaces and select the active workspace or first available workspace.
13-
- **HTTP resilience** — configure SDK retries for transient connection failures and retry SQL execution on stale pooled sockets.
13+
- **HTTP resilience** — retry SQL execution on stale pooled sockets. Transport-level retries are the SDK's own default, which this package leaves in place so a non-idempotent request is never replayed on a response status.
1414
- **SQL execution helper** — run SQL through `POST /v1/query`, poll async query runs when needed, and return a `QueryResult`.
1515
- **Result utilities** — convert query results to records, pandas DataFrames, or metadata dictionaries for adapter display layers.
1616
- **History helpers** — list recent results and query run history with normalized dataclasses.

‎hotdata_framework/client.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@
5252
normalize_host,
5353
pick_workspace,
5454
)
55-
from hotdata_framework.http import default_http_retries
5655
from hotdata_framework.result import QueryResult
5756

5857
# Load modes the managed-table endpoint accepts: replace overwrites, append adds
@@ -154,11 +153,16 @@ def __init__(
154153
self._host = normalize_host(host) if host else default_host()
155154
self._api_key = api_key
156155
self._workspace_id = workspace_id
156+
# No `retries=`: the generated SDK's own default is the correct policy
157+
# and passing one here replaces it wholesale. `hotdata._retry` retries a
158+
# pre-response connection reset on any method — the stale pooled socket
159+
# case this wrapper was reaching for — while leaving read timeouts and
160+
# status retries idempotent-only, so a POST that may have reached the
161+
# server is never replayed.
157162
self._config = Configuration(
158163
host=self._host,
159164
api_key=api_key,
160165
workspace_id=workspace_id,
161-
retries=default_http_retries(),
162166
)
163167
self._api = ApiClient(self._config)
164168
if request_timeout is not None:

‎hotdata_framework/http.py‎

Lines changed: 0 additions & 17 deletions
This file was deleted.

‎tests/test_retry_policy.py‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
"""A POST must never be replayed because of a response status.
2+
3+
A load is not idempotent: re-sending one that the server is still working on
4+
collides with the write lock the first attempt holds, and the duplicate is
5+
refused. The generated SDK already draws this line — ``hotdata._retry`` retries
6+
a *pre-response* connection reset on any method (the stale pooled socket case,
7+
where the server did no work) while leaving read timeouts and status retries
8+
idempotent-only. This wrapper used to pass its own ``retries=`` into
9+
``Configuration``, which replaced that policy wholesale with one that listed
10+
POST alongside a 502/503/504 forcelist — so a gateway timing out a long request
11+
produced a silent duplicate of it.
12+
13+
These tests pin the resulting policy rather than the absence of an argument,
14+
so re-introducing an override that is unsafe for POST fails here.
15+
"""
16+
17+
from __future__ import annotations
18+
19+
from hotdata_framework.client import HotdataClient
20+
21+
WS = "work_test0000000000000000000000"
22+
23+
24+
def _retry_policy():
25+
client = HotdataClient(api_key="hd_test", workspace_id=WS, host="https://example.invalid")
26+
return client._config.retries
27+
28+
29+
def test_no_status_forcelist() -> None:
30+
"""A status code means the request reached the server, so retrying one is a
31+
decision about idempotency that the transport layer cannot make."""
32+
retry = _retry_policy()
33+
assert not retry.status_forcelist, retry.status_forcelist
34+
assert retry.status == 0
35+
36+
37+
def test_post_is_not_read_or_status_retried() -> None:
38+
"""``allowed_methods`` gates both read and status retries. POST outside it is
39+
what stops a request the server may already be processing from being sent
40+
twice."""
41+
retry = _retry_policy()
42+
assert "POST" not in retry.allowed_methods
43+
assert "GET" in retry.allowed_methods
44+
45+
46+
def test_pre_response_connection_reset_still_retries_any_method() -> None:
47+
"""The case the wrapper's own override was reaching for, kept: a reset before
48+
any response means the request never landed, so a POST retry is safe."""
49+
from urllib3.exceptions import ProtocolError
50+
51+
# `_is_connection_error` is urllib3-private, and urllib3 reaches this package
52+
# only as an unpinned transitive dependency — so an upstream rename breaks
53+
# this test for reasons unrelated to this repo. Accepted knowingly: it is the
54+
# only hook that separates a reset from any other ProtocolError without
55+
# standing up a live socket.
56+
retry = _retry_policy()
57+
cause = ConnectionResetError(54, "Connection reset by peer")
58+
reset = ProtocolError("Connection aborted.", cause)
59+
assert retry._is_connection_error(reset)
60+
# A ProtocolError with no connection-level cause is NOT reclassified: only a
61+
# reset proves the request never landed. Read timeouts never reach this branch
62+
# at all — urllib3 raises those as ReadTimeoutError, and they stay method-gated.
63+
assert not retry._is_connection_error(ProtocolError("Connection aborted.", None))

0 commit comments

Comments
 (0)