test(retry): cover Retry-After cap, HTTP-date fallback, TimeoutException

[TexasCoding]

Summary

Wave 2 #97: regression tests for three retry-safety paths that landed in PRs #111 / #112 but had no explicit unit-test coverage.

+8 tests (4 sync in tests/test_client.py, 4 async in tests/test_async_client.py):

• F-Q-01 (HIGH) — test_429_retry_after_caps_at_retry_max_delay (sync + async). Mocks Retry-After: 9999, asserts the recorded sleep equals retry_max_delay=0.1, not 9999. Prevents a malicious or misconfigured server from making the client sleep arbitrarily long.
• F-Q-02 (MED) — test_429_retry_after_http_date_falls_back_to_backoff (sync + async). HTTP-date format header (not int seconds); transport still retries, recorded sleep lands in the Full Jitter window [0, base * 2**0].
• F-Q-03 (HIGH) — test_get_retries_on_timeout + test_post_not_retried_on_timeout (sync + async). GET retries on httpx.TimeoutException; POST raises wrapped KalshiError on first call (route.call_count == 1) since duplicate-POST risk overrides retry.

Pattern matches existing test_429_retry_after_zero_is_honored_end_to_end: sync patches kalshi._base_client.time.sleep, async patches asyncio.sleep (the module is imported inside the async transport method).

Closes #97

Test plan

• Suite: 1568 passed, 48 skipped (+8)
• uv run ruff check . clean
• uv run mypy kalshi/ clean (strict)

[claude]

Code Review — PR #133

Overall: LGTM with minor notes. This is a clean, well-scoped regression test PR. The tests target real safety properties, follow existing project patterns exactly, and all assertions are meaningful. Good candidate for merge after the minor items below.

---

What the PR does

Adds 8 tests (4 sync / 4 async) covering three retry-safety behaviors that were implemented but untested:

ID	Behavior	Risk if broken
F-Q-01	Retry-After: 9999 clamped to retry_max_delay	Server-induced indefinite sleep
F-Q-02	HTTP-date Retry-After falls back to computed backoff	Silent drop of retry or parse panic
F-Q-03	GET retries on TimeoutException; POST raises immediately	Duplicate orders on retry

All three map directly to real failure modes. Worth having.

---

Bugs / Correctness

None found. The async monkeypatch target asyncio.sleep is correct — _base_client.py calls asyncio.sleep(...) via the asyncio module reference, so patching the attribute on the module object intercepts it. The equivalent sync patch kalshi._base_client.time.sleep follows the same reasoning. Both are right.

---

Suggestions

1. F-Q-02 assertion window is tight but non-deterministic

# both sync and async
assert 0.0 <= sleeps[0] <= 0.01

_compute_backoff calls random.uniform(0, 0.01) (attempt=0, base=0.01). The upper bound 0.01 is exactly the computed cap, so the assertion is always correct — random.uniform cannot exceed it. No bug. But the comment says [0, 0.01], which is accurate; consider being explicit that 0.01 comes from retry_base_delay * 2**0:

# uniform(0, retry_base_delay * 2**0) = uniform(0, 0.01)
assert 0.0 <= sleeps[0] <= 0.01

This helps a future reader who changes retry_base_delay in the fixture understand why the bound is 0.01, not an arbitrary magic number. Minor, but these tests are specifically documenting retry math so the explicitness pays off here.

2. test_get_retries_on_timeout doesn't assert sleep was called

The test confirms call_count == 2 and status_code == 200, but doesn't verify that time.sleep / asyncio.sleep was called (i.e., that a backoff delay occurred between the two attempts). The monkeypatch is in place — consider capturing calls like the other tests do and asserting len(sleeps) == 1. Without this, the test would still pass even if someone accidentally removed the time.sleep(delay) call from the timeout retry path.

# async version example:
sleeps: list[float] = []

async def fake_sleep(d: float) -> None:
    sleeps.append(d)

monkeypatch.setattr("asyncio.sleep", fake_sleep)
# ... rest of test ...
assert route.call_count == 2
assert len(sleeps) == 1
assert sleeps[0] >= 0.0

3. Redundant return None in async no-op sleep (minor)

# test_get_retries_on_timeout (async)
async def fake_sleep(d: float) -> None:
    return None  # <- implicit

return None is implicit in a -> None function. Not wrong, just noise.

4. Block comment references issue numbers and F-Q codes

# --- Regression: issue #97 -------------------------------------------
# Async counterparts of F-Q-01 (cap), F-Q-02 (HTTP-date), and F-Q-03

Per CLAUDE.md: "Don't reference the current task, fix, or callers… those belong in the PR description and rot as the codebase evolves." The PR description already carries this context well. The F-Q codes aren't defined anywhere in the codebase itself, so they'll be opaque to future readers. The per-test docstrings capture the important WHY already. Consider dropping the block header and F-Q references from the inline comments.

---

Coverage assessment

The three behaviors covered are the right ones. No gaps relative to the stated scope. DELETE/HEAD timeout retry is covered by the existing test_delete_not_retried / test_get_retries_on_502 tests; no new coverage needed there.

---

Summary

Items 2 (missing sleep assertion in timeout retry test) and 4 (comment style) are worth addressing before merge. Item 1 is a nice-to-have. Item 3 is a true nit.

[TexasCoding]

Addressed in latest commit:

• 🔴 test_get_retries_on_timeout didn't verify sleep was called — fixed for both sync + async. Captures sleeps via the existing monkeypatch and asserts len(sleeps) == 1. Now removing the time.sleep(delay) line in the timeout-retry path would fail the test.
• ✅ F-Q codes / issue # in inline comments — dropped the block-comment header. PR body keeps the context; CLAUDE.md §3 says inline references rot.

Skipped (justified):

• F-Q-02 explanation comment (bot called nice-to-have; the docstring already names the Full Jitter formula).
• "Redundant return None in async no-op sleep" — moot now that the body is sleeps.append(d).

[claude]

Code Review — PR #133: Retry regression tests

Summary: Wave 2 of issue #97 — adds 8 unit tests (4 sync + 4 async) covering three retry-safety paths that were shipped in PRs #111/#112 but had no explicit coverage. Test-only PR; no production code changes.

---

What the PR gets right

• Correct patch targets. Sync tests patch kalshi._base_client.time.sleep; async tests patch asyncio.sleep globally. Both are correct — time is a top-level module import, and asyncio is a local import inside AsyncTransport.request, so patching the live module object via asyncio.sleep is the right approach.
• Retry-After cap test (F-Q-01). sleeps == [0.1] is an exact assertion — it proves the cap applied, proves exactly one retry occurred (implicitly), and catches any off-by-one in the min() call. Good.
• POST-on-timeout no-retry test (F-Q-03b). route.call_count == 1 directly verifies the duplicate-order guard. Pattern is tight.
• Consistent style. Matches existing tests (e.g., test_429_retry_after_zero_is_honored_end_to_end), same respx.mock + monkeypatch setup, same class placement.

---

Issues

Medium — test_get_retries_on_timeout: lower-bound-only assertion is a tautology

Both sync and async versions assert:

assert sleeps[0] >= 0.0

random.uniform(0, x) always returns a non-negative value, so this check is trivially true regardless of what delay is actually computed. If someone removed the time.sleep(delay) / asyncio.sleep(delay) call from the timeout branch entirely, len(sleeps) == 1 would catch it — but if someone swapped _compute_backoff(...) for a fixed 9999, this assertion would still pass.

The HTTP-date fallback test already shows the correct pattern:

assert 0.0 <= sleeps[0] <= 0.01

With retry_base_delay=0.01 (the config fixture), _compute_backoff(attempt=0, config) yields uniform(0, min(0.01 * 2**0, 0.1)) = uniform(0, 0.01), so <= 0.01 is the correct upper bound. The same bound applies to the timeout-retry path. Suggest updating both sync and async versions:

assert 0.0 <= sleeps[0] <= 0.01, (
    f"Expected backoff sleep in [0, 0.01], got {sleeps[0]!r}"
)

Minor — test_429_retry_after_caps_at_retry_max_delay: route not stored; call count not verified

respx.get("https://test.kalshi.com/trade-api/v2/markets").mock(
    side_effect=[...]
)

The route is discarded, so there's no way to assert route.call_count == 2. The implicit proof is that resp.status_code == 200 (second response succeeded) and sleeps has one entry — so a retry definitely happened. This is acceptable, but the sibling http_date_falls_back_to_backoff test does capture the route and assert call_count == 2. Worth making it consistent:

route = respx.get("https://test.kalshi.com/trade-api/v2/markets").mock(side_effect=[...])
...
assert route.call_count == 2

Nit — asyncio patch target is non-obvious without the PR description

The reason monkeypatch.setattr("asyncio.sleep", fake_sleep) works (rather than kalshi._base_client.asyncio.sleep) is that asyncio is imported inside AsyncTransport.request, so asyncio.sleep is looked up on the live module object from sys.modules at call time. The PR description explains this clearly, but a one-line comment in the test would make future maintainers self-sufficient:

# asyncio is a local import inside AsyncTransport.request, so patching
# the module attribute works correctly here.
monkeypatch.setattr("asyncio.sleep", fake_sleep)

---

Test coverage assessment

Path	Sync	Async	Notes
Retry-After: 9999 clamped to retry_max_delay	✅	✅	HIGH security property
HTTP-date Retry-After falls back to backoff	✅	✅	MED; backoff window correct
GET retries on TimeoutException	✅	✅	Delay assertion is a tautology (see above)
POST does NOT retry on TimeoutException	✅	✅	Tight; call_count == 1 is good

---

Verdict

Approve with suggestions. The PR fills real gaps, is well-structured, and doesn't touch production code. The timeout retry assertion is the only substantive issue — it passes today but would survive a regression that introduced an unreasonably large backoff. The remaining items are minor consistency and readability improvements.