polish(transport): async close cancellation-safe + _delete_with_body params + cycle detector + method.upper hoist

[TexasCoding]

Summary

Four small transport / resource polish fixes flagged by the Round-3 audit:
an async-close cancellation leak, a missing params kwarg on
_delete_with_body, a pagination cycle that the O(1) detector couldn't
catch, and a method.upper() call recomputed several times per attempt
inside the retry loop. None of these are public-API breaks.

Issues closed

• Closes AsyncTransport.close() sets _closed before aclose(); cancellation leaks the pool #333 — AsyncTransport.close() flips _closed=True before awaiting aclose(), leaking the pool on cancellation.
• Closes _delete_with_body lacks a params kwarg that _delete_with_body_json already accepts #340 — _delete_with_body lacked a params kwarg that the bytes-fast-path sibling already accepted.
• Closes method.upper() recomputed multiple times per attempt inside the request retry loop #342 — method.upper() was recomputed several times per retry attempt in both transports.
• Closes Pagination cycle detector only catches adjacent cursor repeats #352 — pagination cycle detector only caught adjacent cursor repeats; A → B → A → B → … looped forever.

Behavioral changes

• AsyncTransport.close() no longer silently no-ops after a cancelled
  first call: the second close() actually drains the httpx pool.
• _list_all now raises KalshiError on any non-adjacent cursor revisit
  (LB drift / cache rotation), not just immediate replay. Memory is
  bounded by _CURSOR_SEEN_CAP=1024; once exhausted the guard degrades
  to the legacy adjacent-repeat check, so legitimately very-long
  paginates still work.
• SyncResource._delete_with_body / AsyncResource._delete_with_body
  now accept params: dict[str, Any] | None = None for symmetry with
  _delete_with_body_json. Default is None; existing callers
  unaffected.

Tests

• tests/test_async_client.py::TestAsyncTransportContextManager::test_issue_333_async_close_cancellation_safe —
  injects a CancelledError inside the first aclose(), confirms
  _closed stays False, then re-invokes close() and asserts
  httpx.AsyncClient.is_closed.
• tests/test_async_client.py::TestAsyncTransportContextManager::test_issue_342_method_upper_hoisted —
  passes lowercase verb through AsyncTransport.request; asserts the
  wire request is sent as GET.
• tests/test_base_helpers.py::TestDeleteWithBodyAcceptsParams — sync
  • async wire assertions that the new params kwarg survives onto the
    request URL.
• tests/test_base_helpers.py::TestSyncListAllCursorLoopDetection::test_issue_352_pagination_cycle_detector_non_adjacent
  and the matching async sibling — A → B → A sequence now raises
  KalshiError; replaces the prior test that asserted the old O(1)
  behavior.

Source

Round-3 independent audit closure plan, wave W3.

[claude]

Code Review

Overview

Four focused transport/resource fixes from the Round-3 audit. Clean, minimal, well-motivated. Each has a corresponding test and a closed issue. No public API breaks. Overall this is a solid PR — the notes below are mostly minor polish rather than blockers.

---

Fix 1 — method.upper() hoist (#342)

✅ Correct and clean. Normalizing once before the retry loop is the right call: it eliminates repeated CPU work and removes the theoretical risk of the same logical string being compared inconsistently at different call sites mid-loop.

Minor nit: the inline comment # #342: normalize verb once. Used by auth-sign, the httpx call, log could drop the "Used by…" tail. Per project style, comments should explain the why, not the what, and the callers are self-evident from the code.

---

Fix 2 — AsyncTransport.close() cancellation safety (#333)

✅ Correct fix, well-reasoned docstring. Flipping _closed only after aclose() returns means a cancelled task leaves the flag False, so a follow-up close() can actually drain the pool.

One thing worth verifying: the removed comment claimed "Concurrent close() is safe under asyncio's cooperative scheduling." With the new ordering, two concurrent close() coroutines can both pass the if self._closed: return guard before either sets the flag — both will then call await self._client.aclose(). That's still safe because httpx's aclose is idempotent (and the docstring says so), but it's worth a brief note so a future reader doesn't reinstate the old order thinking they're "fixing" a double-close race.

The test is exemplary: it injects CancelledError mid-aclose, asserts _closed stays False, then re-invokes and checks both the flag and httpx.AsyncClient.is_closed. Good coverage of the pre-fix failure mode.

---

Fix 3 — _delete_with_body params kwarg (#340)

✅ Simple, correct, symmetric. Both sync and async variants get the same signature, the tests assert the query param survives to the wire, and the default None makes it backwards-compatible.

---

Fix 4 — Pagination cycle detector (#352)

✅ Logic is correct. The condition:

if page.cursor in seen_cursors or page.cursor == last_cursor:

covers two distinct cases:

• in seen_cursors catches any revisit while below the 1024-page cap.
• == last_cursor is the adjacent-repeat fallback once the cap is exhausted (because a cursor processed past the cap isn't added to the set, so the set check alone would miss it).

The fallback design is sound — a genuinely very-long paginate that exhausts the cap still gets the adjacent-repeat protection.

Minor issues:

1. No test for the cap-exhausted degradation path. The fallback to last_cursor-only detection after 1024 pages is load-bearing, but there's no test for it. A unit test using a monkeypatched _CURSOR_SEEN_CAP = 3 (or similar small value) would cover this cheaply without 1025 mock responses.

2. Test test_issue_352_pagination_cycle_detector_non_adjacent leaves the respx mock with only 3 staged responses and no catch-all. If the implementation somehow makes a 4th request, the test will fail with a respx ConnectionError rather than the expected KalshiError. That's a fine enough signal, but a side_effect=responses mock that runs dry raises StopIteration wrapped as a transport error — the match r"[Cc]ursor loop.*'A'" would fail. It works today because the guard fires on the 3rd response before any 4th request; worth a brief comment to that effect so the intent stays clear.

3. Renamed test behavior contract. The old test_list_all_cursor_loop_detection_uses_last_cursor_only_o1 test documented an intentional non-adjacent pass-through that some callers may have relied on. The new behavior is strictly safer, but it's worth confirming there are no call sites in the resource layer that pass cursors obtained from external sources (e.g. resuming a previously saved cursor that happens to collide with a cursor from an earlier page).

---

Minor Observations

• last_cursor is redundant below the cap (it's always in seen_cursors), but removing it would complicate the above-cap fallback, so keeping it is the right call.
• The async _list_all docstring (Raises ``KalshiError`` on repeated cursor; see sync docstring.) still works, but since the behavior changed significantly it could be more self-contained.
• The test_issue_342_method_upper_hoisted test only exercises a lowercase input ("get"). Adding a quick assertion that an already-uppercase verb ("GET") round-trips cleanly would complete the coverage.

---

Summary

Fix	Verdict
method.upper() hoist	✅ Merge as-is
AsyncTransport.close() cancellation	✅ Merge; optional docstring note about concurrent-close idempotency
_delete_with_body params	✅ Merge as-is
Pagination cycle detector	✅ Merge; consider adding a cap-degradation test as a follow-up issue

No blockers. The cap-degradation test gap is the only item I'd call out as worth tracking.