Repository navigation
polish(auth+transport): sign-executor race, Retry-After parity, MessageQueue, http2 test, benches - #289
Conversation
Pre-fix, sign_request_async and close() could race: thread A read _closed=False outside the lock, close() ran end-to-end (set _closed, shut down + nulled the executor), then thread A entered the locked double-checked init and constructed a brand-new ThreadPoolExecutor on a closed auth. The new pool was never shut down (close() already returned), defeating the documented terminality and leaking a 2-thread pool until interpreter shutdown. Move the _closed check inside the lock: keep a lock-free fast path for the already-constructed case, but on the slow path re-read _closed under the lock and raise without instantiating a new executor if close() has won the race. Regression test simulates the interleaving by wrapping the lock's __enter__ to call close() mid-acquire.
… parity (#267) Pre-fix, Retry-After: -5 returned retry_after=None (falls back to computed exponential backoff) while the semantically equivalent ``Retry-After: <past HTTP-date>`` returned retry_after=0.0 (retry immediately). Tests that pinned one shape silently disagreed with the other, and clock-skewed proxies emitting past timestamps got inconsistent behavior depending on which form they used. Pick the more permissive policy and clamp negative delta-seconds to 0.0 \u2014 matching the existing HTTP-date branch. Non-finite (NaN/inf) still falls back to None since those would survive the transport caller's min() cap and either crash time.sleep or schedule a near-infinite delay. Regression test asserts both forms produce identical retry_after.
Two small per-message hot-path savings: - MessageQueue._size: an O(1) int counter incremented/decremented on every put / popleft purely so qsize() could exclude the single terminal sentinel. Two extra int ops per message on the WS recv hot path with no functional gain \u2014 derive qsize from len(self._buffer) minus a sentinel adjustment when the queue is closed. The deque's maxlen=maxsize+1 was already the real memory ceiling. - import asyncio inside AsyncTransport.request: a sys.modules lookup on every async request. The module is already imported at top level by KalshiAuth.sign_request_async (and many others); move the import to the file header. Updated the deque-maxlen regression to assert the cap via sustained overflow rather than simulated counter drift (the counter no longer exists).
…#271) #271 items 3 + 4 \u2014 testing-coverage polish. tests/test_http2.py: verify KalshiConfig(http2=True) propagates to the underlying httpx.Client / httpx.AsyncClient's connection pool, so a regression that silently drops the kwarg surfaces here instead of landing without any failing test. Guarded by skipif when h2 isn't installed in the env (httpx rejects http2=True without it). scripts/bench_orderbook_iterator.py: drives _OrderbookIterator.__anext__ end-to-end through a synthetic stream that mimics the recv loop (apply_delta_inplace + yield). Existing bench_orderbook_delta only measures the in-place mutation; this one captures the mgr.get + Orderbook materialization that real async for book in ws.orderbook() consumers pay. scripts/bench_page_to_dataframe.py: builds a 1000-item Page[Market] and times to_dataframe() / to_polars(). Catches regressions in the column-oriented build path (#264) and confirms the DollarDecimal contract (#225/#190) doesn't quietly fall back to per-row model_dump. Closes #267 Closes #271
Code Review — PR #289Overall: Solid wave-3 polish PR. All three fixes are correct, the tests are well-targeted, and the
|
| State | _buffer contents |
qsize() |
|---|---|---|
| Open, 2 items | [i1, i2] |
2 |
| Closed, 2 items + sentinel | [i1, i2, sentinel] |
2 |
| Closed, sentinel only | [sentinel] |
0 |
| Closed, sentinel consumed | [] |
0 |
The invariant holds because put_sentinel / put_error set _closed = True and append the sentinel in the same coroutine with no await in between, so from asyncio's perspective these are atomic — qsize() can never observe _closed=True with the sentinel not yet in the buffer.
kalshi/_base_client.py — Retry-After clamping + asyncio import ✅
The behavior change from None → 0.0 for negative delta-seconds is correct and well-justified: a server sending Retry-After: -5 and one sending a past HTTP-date both mean "retry now", and the two branches now agree. time.sleep(0.0) is safe; the transport's min(..., retry_max_delay) cap still applies.
Moving import asyncio to module level is a clean improvement.
tests/test_http2.py — HTTP/2 wiring test ⚠️ minor
The test accesses httpx_client._transport._pool._http2 which is private httpx internals. The # type: ignore[attr-defined] comments acknowledge this, but if httpx restructures its pool (it has done so before between minor versions), these three tests will fail with AttributeError rather than giving a clear "h2 not wired through" message. Worth pinning the concern:
# httpx internals (valid as of httpx ~0.28.x). If this AttributeError-fails,
# check httpx._transports.default.HTTPTransport after a httpx upgrade.
pool = httpx_client._transport._pool # type: ignore[attr-defined]Not a blocker — the test is valuable — but a note in the assertion message would make the eventual breakage easier to triage.
scripts/bench_orderbook_iterator.py — private API use ℹ️
The benchmark calls mgr._apply_snapshot_inplace() and mgr._apply_delta_inplace() directly. This is fine for a scripts-only benchmark (not in CI), but worth a comment so it's not mistaken for a supported usage pattern:
# Access private methods directly to isolate the iterator hot path.
mgr._apply_snapshot_inplace(_build_snapshot(ticker, depth))Summary
| Area | Verdict |
|---|---|
| Auth race fix | ✅ Correct, well-tested |
_size removal |
✅ Simpler and equally correct |
| Retry-After clamping | ✅ Correct parity fix |
| HTTP/2 test | ✅ Valuable — minor fragility concern |
| Benchmarks | ✅ Clean, useful additions |
| Test coverage | ✅ New tests directly target the fixed invariants |
No blocking issues. The two minor nits (HTTP/2 test annotation, bench script comment) are optional.
Closes #267
Closes #271
Part of #273 (post-v2.4 audit), Wave 3.