[TexasCoding]

Closes #301.

What

SyncTransport.close() and AsyncTransport.close() (async) were the only lifecycle methods in the SDK without an explicit _closed flag. httpx.Client.close() happens to be idempotent today, but the SDK pattern (KalshiAuth, KalshiClient) tracks _closed explicitly — #267 even added a re-check-under-lock guard to the sign executor. This PR brings the transports in line.

• New _closed: bool set in both __init__s.
• close() / aclose() early-return if already closed; otherwise flip the flag and delegate to httpx.
• Docstrings note the contract.

Tests

• TestSyncTransportLifecycle.test_sync_transport_close_is_idempotent
• TestAsyncTransportLifecycle.test_async_transport_close_is_idempotent

Both triple-close and assert _closed is True after the first call — mirrors tests/test_auth.py::test_close_is_idempotent.

Final PR of the post-v2.5 audit closure (Wave W2).

[claude]

Code Review

Overview

This PR makes SyncTransport.close() and AsyncTransport.close() explicitly idempotent by adding a _closed: bool guard, bringing them in line with the KalshiAuth pattern established in #267. The change is minimal, well-scoped, and the tests mirror the existing test_auth.py::test_close_is_idempotent pattern cleanly.

---

One Concern: Thread Safety Gap in SyncTransport.close()

KalshiAuth.close() sets _closed = True under _sign_executor_lock to prevent a TOCTOU race:

# auth.py — lock guards the flag flip
with self._sign_executor_lock:
    self._closed = True
    ...

SyncTransport.close() in this PR does not hold any lock:

# _base_client.py — flag flip is unguarded
if self._closed:
    return
self._closed = True          # ← race window between check and flip
self._client.close()

Two threads calling close() simultaneously can both pass the if self._closed check and both invoke self._client.close(). In practice this is benign today (httpx's close is itself idempotent), but it violates the "idempotent" contract at the SDK level and is inconsistent with KalshiAuth.

For AsyncTransport this is a non-issue since async coroutines are cooperative and you'd need explicit concurrent calls to trigger a race.

Suggestion: Either add a threading.Lock to SyncTransport mirroring KalshiAuth, or add a note in the docstring that idempotency is guaranteed for sequential callers but not for concurrent ones. The former is strictly correct; the latter is honest about scope.

---

Minor Observations

Tests don't assert _client.close() was called exactly once. The triple-close test verifies _closed is True after the second/third calls (correct), but doesn't verify the httpx delegate was only invoked once. A unittest.mock.patch or spy on transport._client.close would make this explicit. This is low priority — the flag guard is the real invariant — but worth considering if the goal is to prove "stacked cleanup paths don't double-close the httpx client."

Docstring verbosity. The CLAUDE.md guideline favors concise comments, and these docstrings are on the longer side. The issue reference (#301) and core contract ("safe to call multiple times") are worth keeping; the explanation of which callers stack cleanup paths reads more like PR description prose. Minor style nit.

---

Summary

The core logic is correct and the test structure is solid. The one substantive issue is the unguarded flag flip in SyncTransport.close() — worth a quick follow-up if thread-safe teardown is ever a requirement, though the practical risk is low given httpx's own idempotency. Everything else is polish-level.

Verdict: Approve with the threading note flagged for awareness.

[TexasCoding]

Round-1 review addressed in follow-up:

• Threading scope: docstrings now explicitly bound idempotence to sequential callers on SyncTransport.close() and note asyncio cooperative scheduling on AsyncTransport.close(). Skipping a threading.Lock because the KalshiAuth precedent (Polish bundle: auth + transport small items (sign-executor race, Retry-After negative) #267) needs one only because _get_sign_executor can recreate the executor under contention — no analogue here, and httpx.Client.close() is itself idempotent so the redundant-close worst case is benign.
• Test asserting close called exactly once: bot explicitly called this low-priority ("the flag guard is the real invariant"). Skipped to keep the diff small.
• Docstring verbosity: tightened on both methods.

[claude]

Code Review — PR #309: Idempotent close() for Sync/AsyncTransport

Overview

Clean, well-scoped PR. Adds _closed: bool guards to both transport close() methods to prevent redundant httpx client teardowns. Docstrings are honest about the threading tradeoffs and tests mirror the established test_auth.py::test_close_is_idempotent pattern.

---

Issues

1. Missing explicit type annotation (nit — breaks codebase consistency)

auth.py line 80 uses the explicit self._closed: bool = False form. Both new lines in this PR use the unannotated self._closed = False. Mypy strict will infer bool from the literal, so it won't fail CI, but it diverges from the established style for this class of flag. Should be self._closed: bool = False in both __init__s.

2. request() has no use-after-close guard (pre-existing gap, not introduced here)

KalshiAuth._get_sign_executor() raises a RuntimeError if _closed is True, giving the caller a clean SDK error. Neither SyncTransport.request() nor AsyncTransport.request() checks _closed, so a post-close() call will get an httpx-level error (e.g., RuntimeError: Cannot send a request, as the client has been closed) rather than a descriptive KalshiError. This pre-existed but is worth tracking — a follow-up issue to add a _closed guard at the top of request() / async request() would complete the lifecycle story.

3. Thread-safety asymmetry with KalshiAuth.close() (acknowledged, but worth a call-out)

The sync docstring honestly notes the TOCTOU race. KalshiAuth.close() avoids this with self._sign_executor_lock, making its _closed flip fully race-free. The transport's decision not to add a lock is fine given the worst-case is one redundant idempotent Client.close(), but it's a deliberate deviation from the auth pattern. If the docstring said "unlike KalshiAuth, no lock is needed because the worst-case is harmless" that would help future readers understand the asymmetry was intentional.

---

Tests

• Fixtures: transport fixture is correctly defined in both test_client.py (line 47) and test_async_client.py (line 44) — the new test classes resolve the fixture without any scoping issues.
• Triple-close assertion pattern mirrors test_auth.py::test_close_is_idempotent exactly. ✓
• assert transport._closed is True after first close, then re-asserted after triple-close — covers both the early-return path and the flag persistence. ✓
• Missing: no test for request() after close(). Not required to merge, but would pair naturally with a future use-after-close guard.

---

Summary

Blocking: None.

Suggested before merge:

• Change self._closed = False → self._closed: bool = False in both __init__s to stay consistent with auth.py.

Suggested as follow-up issues:

• Add _closed guard to SyncTransport.request() and AsyncTransport.request() (raises KalshiError) to give callers a clean SDK error instead of a raw httpx error.
• Clarify in the sync docstring that the lock-free approach is a deliberate divergence from KalshiAuth (not an oversight).

The core change is correct and the PR description/docstrings are unusually transparent about the design tradeoffs. Approve with the nit above.

[TexasCoding]

Round-2 nit applied (305e... incoming): explicit bool annotation on _closed to match auth.py:80 style. Deferring the two follow-ups (use-after-close guard, additional docstring clarification) — the docstring already explains the deliberate divergence, and the use-after-close concern is pre-existing and orthogonal.

Merging on green.

[claude]

Code Review — PR #309: Make Sync/AsyncTransport.close() idempotent

Summary: Clean, focused polish PR (53 additions, 0 deletions) that adds an explicit _closed guard to both transports, closing Issue #301.

---

✅ What's Good

• Minimal and correct. Every changed line traces directly to the goal. No scope creep.
• Honest documentation. The sync docstring accurately acknowledges the TOCTOU race and explains why it's acceptable (httpx's own idempotency as the backstop). That's the right level of honesty rather than papering over the limitation.
• Async rationale is sound. Cooperative scheduling under asyncio does make the check-then-set safe — no interleaving between the if self._closed: check and self._closed = True flip.
• Tests follow the established pattern. Mirroring test_auth.py::test_close_is_idempotent (triple-close + _closed is True assertion) is the right call.

---

🔍 Observations / Suggestions

1. Tests don't verify _client.close() is called exactly once (minor)

The tests confirm that subsequent calls don't raise, but they don't assert that httpx.Client.close() is skipped on the 2nd and 3rd calls. Given the early-return is the whole point, a mock.patch assertion would make the test more complete:

from unittest.mock import patch

def test_sync_transport_close_is_idempotent(self, transport: SyncTransport) -> None:
    with patch.object(transport._client, "close") as mock_close:
        transport.close()
        transport.close()
        transport.close()
    mock_close.assert_called_once()  # only the first close delegates
    assert transport._closed is True

This would survive any future change to httpx that drops its own idempotency guarantee. Not a blocker, but worth considering since the whole motivation is "httpx happens to be idempotent today."

2. KalshiClient.close() has no explicit guard (implicit reliance)

After this PR, KalshiClient.close() is effectively idempotent via its delegates (transport now guarded, auth already guarded), but it has no _closed flag of its own. The PR description includes KalshiClient in the "tracks _closed explicitly" category, but that's not currently true — see client.py:190-194:

def close(self) -> None:
    self._transport.close()          # now guarded ✅
    if self._auth is not None and self._auth_owned:
        self._auth.close()           # already guarded ✅

This works, but the safety is implicit. A reader of KalshiClient.close() can't tell it's safe to double-call without reading the delegates. Worth a follow-up issue or a quick _closed guard at the KalshiClient level. (Arguably the transport guard is sufficient and this is intentional scope — just flagging it.)

3. _closed initialization ordering (cosmetic)

In __init__, _closed = False is set after the httpx.Client(...) construction:

self._client = httpx.Client(**client_kwargs)
self._closed: bool = False   # ← after

If httpx.Client() raises, _closed is never set — harmless since close() would never be called on a failed transport. But setting it before _client would be the conventional "initialize all attributes at top of __init__" style:

self._closed: bool = False
self._client = httpx.Client(**client_kwargs)

Not a correctness issue.

---

🚫 No Issues Found With

• Correctness of the idempotency logic
• Test structure and fixtures
• Docstring accuracy (especially the async cooperative-scheduling note)
• Consistency with the KalshiAuth pattern

---

Verdict: Approve with the test-coverage suggestion (item 1) being the only thing worth acting on before merge. The mock.patch assertion is low-cost and would make the tests verify behavior rather than just absence-of-exception. Items 2 and 3 are fine as follow-ups or can be deferred entirely.

[TexasCoding]

Round-3 review: zero blocking items. All three observations explicitly non-blocking:

• mock.patch to assert exact-once delegate — captured as a future test-tightening if httpx ever changes its own idempotence; the existing test asserts the contract.
• KalshiClient.close() explicit _closed flag — the bot itself notes "Arguably the transport guard is sufficient and this is intentional scope". Defer.
• Init ordering — cosmetic.

Merging.