fix(transport): Retry-After honored for 503/504/408 + jitter applied to Retry-After + success-body size cap

[TexasCoding]

Summary

Three MEDIUM-severity correctness fixes around the HTTP retry/response path. Retry-After is now honored for every retryable status RFC 7231 §7.1.3 covers (408/503/504/other 5xx, not just 429); Full Jitter is applied on top of the server's Retry-After hint so a synchronized client fleet no longer re-stampedes on the same tick; and the success-path response body now carries the same 16 KB-style size guard _map_error already had on the error path — capped at 64 MiB to bound memory before response.json() materialises the body into a Python dict graph.

Issues closed

• Closes Retry-After path bypasses jitter — synchronized clients stampede the rate limit #321 — Retry-After path now adds Full Jitter on top of the server hint, capped at retry_max_delay.
• Closes Retry-After header honored only on 429; 503/504/408 silently ignored #322 — _parse_retry_after extracted from the 429 branch and applied to 408/503/504/other 5xx; retry_after lifted to the KalshiError base so the retry loop reads it generically.
• Closes Success-path response body has no size cap (16 KB cap is error-only) #323 — _enforce_response_body_cap (Content-Length pre-check + len(response.content) post-buffer check) added to both SyncResource._load_json and AsyncResource._load_json.

Behavioral changes

• KalshiRateLimitError.__init__ no longer overrides the base; retry_after is now a KalshiError attribute. Existing call sites (KalshiRateLimitError(message, status_code, retry_after=...)) still work — the keyword signature is preserved.
• The previous test_429_retry_after_zero_is_honored_end_to_end assertion sleeps == [0.0] is updated to [0, retry_base_delay] since the floor is now jittered. The "zero is honored" property (not falsy-dropped) is preserved.
• KalshiServerError and KalshiTimeoutError raised from _map_error carry the parsed retry_after when the response advertised one. Callers who already type-narrow to those classes can now read .retry_after to compute their own backoff.

Tests

• tests/test_base_helpers.py::TestIssue323SuccessBodyCap::test_issue_323_success_body_capped — Content-Length-advertised oversize, chunked actual-oversize, normal-size pass-through, end-to-end SyncResource._get rejection, and malformed Content-Length fall-through.
• tests/test_client.py::TestSyncTransportRetry::test_issue_321_retry_after_with_jitter — pinned random.uniform to verify floor + jitter math (0.03 + 0.005 = 0.035).
• tests/test_client.py::TestSyncTransportRetry::test_issue_321_retry_after_jitter_capped_at_retry_max_delay — min(floor + jitter, retry_max_delay) ceiling.
• tests/test_client.py::TestSyncTransportRetry::test_issue_322_retry_after_honored_on_5xx_and_408 — 408/503/504 each map to the right typed exception AND sleep the server-hinted floor end-to-end (zero jitter forced for the assertion).
• tests/test_client.py::TestSyncTransportRetry::test_issue_322_retry_after_attached_to_kalshi_error_base — retry_after is readable via base-class attribute access (no isinstance narrowing).

Source

Round-3 independent audit closure plan, wave W2.

[claude]

Code Review — PR #368

Overview

Three well-scoped correctness fixes on the retry/response path: Retry-After generalized to all retryable statuses (#322), Full Jitter applied on top of server hints to prevent thundering herd (#321), and a 64 MiB size cap mirrored onto the success path (#323). The logic is sound and the test coverage is intentional. A handful of issues worth addressing below.

---

Bugs / Correctness

1. len(response.content) called twice in _enforce_response_body_cap

if len(response.content) > MAX_RESPONSE_BODY_BYTES:
    raise KalshiError(
        f"Response body is {len(response.content)} bytes, ..."

response.content is a cached property but len() is still computed twice. Assign once:

body_len = len(response.content)
if body_len > MAX_RESPONSE_BODY_BYTES:
    raise KalshiError(f"Response body is {body_len} bytes, ...")

2. Redundant tuple element in _map_error

if status in (408, 429, 504) or status >= 500:

504 is already covered by >= 500; the tuple only needs (408, 429). The code works correctly, but the redundancy is confusing — it looks like 504 has special significance in the outer condition when it actually doesn't.

---

CLAUDE.md Convention Violations

3. Multi-paragraph docstrings and block comments

CLAUDE.md is explicit: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." This PR adds:

• A 6-line block comment for MAX_RESPONSE_BODY_BYTES
• An 8-line docstring for _parse_retry_after
• A 14-line docstring for _enforce_response_body_cap

All three should be collapsed to a single summary line. The detail belongs in the PR description (where it already is).

---

Test Coverage Gaps

4. No async counterparts for the three new sync tests

test_issue_321_retry_after_with_jitter, test_issue_321_retry_after_jitter_capped_at_retry_max_delay, and test_issue_322_retry_after_honored_on_5xx_and_408 are sync-only. The async transport has symmetric code for the error.retry_after is not None branch, but it's only covered by the updated zero-Retry-After test. A regression in AsyncTransport.request would go undetected by these three new scenarios.

5. Loop-based test is fragile

test_issue_322_retry_after_honored_on_5xx_and_408 iterates over (408, 503, 504) and calls respx.reset() between iterations inside a single @respx.mock-decorated test. If one iteration fails, pytest reports a single test failure with no indication of which status code caused it. Three separate parameterized cases (or three separate test methods) would be cleaner and give better failure attribution.

---

Behavioral Change Worth Flagging

6. Retry-After: 0 no longer means zero sleep

Pre-PR: Retry-After: 0 → sleep exactly 0.0 (immediate retry).
Post-PR: sleep is jitter(0, retry_base_delay) — up to 0.01 s by default.

The intent (avoiding synchronized re-stampede) is correct, and the tests are updated to match. But any caller relying on "the transport never sleeps on Retry-After: 0" will silently get a small delay. Worth documenting in CHANGELOG.md if not already there.

---

Minor

• Missing blank line before class TestIssue323SuccessBodyCap: in test_base_helpers.py (immediately follows the last assert of the previous test class without a separator).
• floor as a local variable in request() shadows the math.floor builtin — not imported here, so no runtime impact, but a future reader adding from math import floor would get a subtle collision. retry_floor or ra_floor would be clearer.
• MAX_RESPONSE_BODY_BYTES is not exposed via KalshiConfig. If a future endpoint legitimately exceeds 64 MiB, callers have no way to raise the cap without forking the library. Not blocking, but worth a tracking issue.

---

Summary

The core logic — extracting _parse_retry_after, lifting retry_after to the base class, adding the success-path size guard — is correct and well-motivated. Main actionable items before merge: fix the double len() call (#1), trim the docstrings to one-liners (#3), and add async test coverage for the three new retry scenarios (#4).

[TexasCoding]

Round-2 review addressed in 37ec046:

Addressed:

• Double len(response.content) in _enforce_response_body_cap collapsed to a single body_len local (kalshi/resources/_base.py).
• Multi-paragraph docstrings on _parse_retry_after and _enforce_response_body_cap trimmed to single-line summaries per CLAUDE.md.
• Async parity tests added: test_issue_321_retry_after_with_jitter_async and test_issue_322_retry_after_honored_on_5xx_and_408_async in tests/test_async_client.py; test_issue_323_success_body_capped_async in tests/test_base_helpers.py.

Verified locally: ruff + mypy clean; pytest -k 'issue_321 or issue_322 or issue_323' → 8/8 pass.

Deferred: none — all actionable items resolved.

[claude]

Code Review — PR #368

Overall: Three well-scoped correctness fixes. The design decisions are sound, tests are thorough, and the KalshiRateLimitError.__init__ simplification is a clean win. One blocking lint issue must be fixed before merge; a handful of minor points follow.

---

Blocking

tests/test_base_helpers.py:513 — missing blank line (E302)

        assert len(items) == total, f"Expected {total} items, got {len(items)} (cap leaked?)"

class TestIssue323SuccessBodyCap:   # ← only ONE blank line above

PEP 8 / ruff E302 requires two blank lines before a top-level class definition. CI will reject this.

---

Correctness

_map_error combined condition (_base_client.py:101)

if status in (408, 429, 504) or status >= 500:

504 appears in both the tuple and satisfies >= 500 — redundant but harmless. The inner routing handles it correctly before the KalshiServerError fallthrough, so no bug here. Just worth a note if someone ever reads the condition and wonders why 504 is listed explicitly.

Jitter math — verified against the test fixtures:

• Retry-After: 0.03, retry_max_delay=0.1, retry_base_delay=0.01, attempt 0:
  • floor = min(0.03, 0.1) = 0.03
  • capped = min(0.01 × 2⁰, 0.1) = 0.01; midpoint jitter → 0.005
  • delay = min(0.03 + 0.005, 0.1) = 0.035 ✓

Retry-After: 0 semantics — the PR preserves the "not falsy-dropped" property. floor = 0, jitter drawn from [0, retry_base_delay). RFC 7231 says "no sooner than 0 seconds"; adding small jitter on top doesn't violate the contract and is the right call for fleet desynchronisation. ✓

KalshiRateLimitError backward compatibility — removing the overriding __init__ is correct. Keyword argument retry_after= passes through to KalshiError.__init__, positional callers via (message, status_code, retry_after) still work. ✓

---

Minor Issues

_enforce_response_body_cap — "pre-buffer" framing is slightly inaccurate

The constant's docstring says a response could OOM "before the parse completes" and refers to a Content-Length "pre-check". In practice, httpx.Client.request() (non-streaming) fully buffers the response body before _load_json is ever called, so the Content-Length branch isn't truly pre-buffer — it's pre-parse. The real protection is preventing the 5–10× JSON-parse inflation of an already-buffered 64 MiB body. The function is still correct and valuable; just the comments overstate one aspect.

Suggestion: replace "before httpx finishes reading" with "before JSON parse materialises the object graph".

_enforce_response_body_cap raises KalshiError(status_code=200) on 2xx

raise KalshiError(
    f"Response body is {body_len} bytes, exceeds max_response_bytes=…",
    status_code=response.status_code,  # e.g. 200
)

A KalshiError with status_code=200 is semantically confusing — it looks like a success to anything that checks the code. Consider omitting status_code here (pass None) since this isn't a server-reported error status, or document why the code is included. Not a functional bug, but it will surprise anyone catching KalshiError and inspecting status_code.

Memory-intensive unit test

test_issue_323_success_body_capped allocates a b"x" * (MAX_RESPONSE_BODY_BYTES + 1) buffer (64 MiB + 1 byte) inline. This is necessary to exercise the post-buffer check, but it makes the test noticeably slow in low-memory CI environments. Consider gating it with @pytest.mark.slow or using the Content-Length-only path for the "chunked oversize" scenario (spoof the header without allocating real bytes), since the post-buffer check is already covered by the end-to-end _get test (case 4).

---

What's good

• Extracting _parse_retry_after as a named function is a clean improvement — it was a 40-line inline block, now it's 20 lines with a clear signature and no logic change.
• The error.retry_after base-class attribute access (removing the isinstance(error, KalshiRateLimitError) guard) is exactly the right design. Any retryable error type can now carry the hint.
• Sync/async parity is maintained throughout — both transport implementations get identical retry logic and tests.
• Test comments include the full arithmetic (floor, capped backoff, midpoint jitter, expected delay) which makes failures trivial to debug.
• KalshiServerError and KalshiTimeoutError now surface retry_after to callers who want to build their own backoff on top of the SDK.

---

Summary: Fix the missing blank line (E302) and optionally address the status_code=200 ambiguity. Everything else is solid.

[TexasCoding]

Round-2 review addressed in 28c0acb: E302 blank line; KalshiError status_code dropped on body-cap (client-side enforcement, not server-reported). Deferring: docstring framing nit ("pre-buffer" → "pre-parse", cosmetic); @pytest.mark.slow on 64 MiB test (would lose coverage).

[claude]

Code Review — PR #368

Three well-scoped correctness fixes around the retry/response path. The overall approach is sound and the PR description is thorough. Below is a mix of genuine issues, minor nits, and confirmations of what's done well.

---

Overview

• Issue Retry-After path bypasses jitter — synchronized clients stampede the rate limit #321 — Full Jitter applied on top of Retry-After to prevent thundering-herd re-stampede.
• Issue Retry-After header honored only on 429; 503/504/408 silently ignored #322 — _parse_retry_after extracted and applied to 408/503/504/5xx, not just 429; retry_after lifted to KalshiError base.
• Issue Success-path response body has no size cap (16 KB cap is error-only) #323 — 64 MiB cap on success-path response bodies via _enforce_response_body_cap.

---

Medium Issues

1. _enforce_response_body_cap post-buffer check doesn't prevent OOM for chunked responses

len(response.content) requires httpx to have already buffered the full body. For chunked responses without a Content-Length header, the entire body is in memory before the size check runs — the check only prevents JSON parsing on an already-OOM'd heap.

The Content-Length pre-check covers the most common attack surface (a malicious proxy lying about the body size before sending it), but the post-buffer check for chunked responses is theater for the actual OOM scenario. A comment acknowledging this limitation would set correct expectations:

# For chunked responses (no Content-Length), httpx has already buffered the body
# by the time we reach this check. This guards against JSON parse amplification
# (5-10x inflation) but not against the initial buffer allocation.
body_len = len(response.content)

Or, if true streaming protection is desired in the future, httpx supports client.stream() — worth a follow-up issue.

2. KalshiError raised from _enforce_response_body_cap has status_code=None

Every other KalshiError raised from the HTTP path carries a status_code. Callers catching KalshiError commonly branch on error.status_code, and None for what is effectively a 200 response is surprising:

# Current — status_code is None
raise KalshiError(
    f"Response body is {body_len} bytes, exceeds max_response_bytes={MAX_RESPONSE_BODY_BYTES}",
)

# Better — include the actual status code for consistency
raise KalshiError(
    f"Response body is {body_len} bytes, exceeds max_response_bytes={MAX_RESPONSE_BODY_BYTES}",
    status_code=response.status_code,
)

Same applies to the Content-Length pre-check branch.

---

Minor Issues

3. Redundant membership check: 504 is in the tuple and covered by >= 500

if status in (408, 429, 504) or status >= 500:

504 is caught by both arms. Not a bug, but noisy — either drop 504 from the tuple or reorder to make intent clearer:

if status == 429 or status in (408, 504) or status >= 500:

This mirrors the inner if-chain and reads more directly.

4. Retry-After: 0 semantics change deserves a CHANGELOG note

The PR correctly updates the test comment, but Retry-After: 0 previously produced a deterministic sleep(0.0). Now it produces a random sleep in [0, retry_base_delay). Any caller relying on Retry-After: 0 as a "no wait" signal (e.g., a mock-based integration harness that asserts no sleep occurred) will silently get a non-zero sleep. The CHANGELOG entry for this release should call this out explicitly as a behavioral change.

5. Jitter is effectively zero when floor == retry_max_delay

floor = min(error.retry_after, self._config.retry_max_delay)
delay = min(
    floor + _compute_backoff(attempt, self._config),
    self._config.retry_max_delay,
)

When error.retry_after >= retry_max_delay, floor == retry_max_delay and the outer min() always yields retry_max_delay regardless of the jitter draw. The anti-stampede benefit is lost precisely when the server's hint is longest and client synchronization risk is highest. This might be acceptable as a deliberate tradeoff (we can't wait longer than retry_max_delay), but it's worth a comment.

---

What's Done Well

• _parse_retry_after extraction is clean. The RFC-edge-case handling (NaN/inf, negative deltas, HTTP-date format, clock-skewed past dates) is preserved exactly and much easier to reason about in isolation.
• retry_after lifted to KalshiError base is the right design. Removing the bespoke KalshiRateLimitError.__init__ and keeping the retry_after kwarg signature identical is a clean backward-compatible change.
• _compute_backoff reuse for jitter is the right call — reusing the existing Full Jitter implementation rather than inlining a second random.uniform call keeps the randomness logic in one place.
• Test quality — pinning random.uniform to get exact pytest.approx assertions on the jitter math is the right pattern. The loop over (408, 503, 504) with respx.reset() is correct; testing both _map_error directly and the end-to-end transport path covers the right layers.
• KalshiServerError and KalshiTimeoutError now carry retry_after — this unblocks callers who want to implement their own backoff on top of the SDK without isinstance-narrowing to KalshiRateLimitError.

---

Summary

Two actionable items before merge:

1. Add status_code=response.status_code to both KalshiError raises in _enforce_response_body_cap for consistency.
2. Add a comment (or streaming follow-up issue) acknowledging that the post-buffer chunked-body check doesn't prevent the initial allocation.

The rest are low-priority nits. The core logic is correct and the test coverage is solid.