[TexasCoding]

Wave 1-C of the audit umbrella. 8 issues; the largest WS-correctness batch including the critical orderbook resync.

Changes

• WS: orderbook permanently desyncs after a sequence gap (no resnapshot) #189 [critical] _handle_seq_gap drives a real unsubscribe+resubscribe for orderbook_delta AND order_group_updates (was orderbook-only, was just clear-and-pray). All-markets subs supported via per-sid ticker tracking in OrderbookManager. Local book repopulates after a gap instead of staying permanently empty.
• WS: generic subscribe() silently drops unknown params keys #195 Subscription.to_subscribe_params validates param keys against a per-channel allow-set; raises KalshiSubscriptionError on unknown keys (typo-trap fix).
• WS: server-side seq reset silently dispatches subsequent messages (no gap fired) #196 SequenceTracker.track distinguishes seq==last (drop), seq<last (reset → gap-recovery), out-of-order (drop). Was silently dispatching reset messages.
• WS: reconnect should fast-fail on permanent close codes (1008, 4001…) #197 WS recv loop fast-fails on permanent close codes (1002/3/7-10, 4xxx) with KalshiConnectionError instead of burning the 10-retry budget on doomed reconnects.
• WS: order_group_updates sequence gap has no recovery path (consumer silently misses events) #205 order_group_updates gap routed through the same resubscribe helper as orderbook (was: no recovery at all).
• WS: unsubscribe leaks orderbook state (long-running sub/unsub cycles grow memory) #206 KalshiWebSocket.unsubscribe() tears down OrderbookManager state for the sub's tickers (was: long-running sub/unsub cycles leaked orderbook entries forever).
• WS: ERROR backpressure strategy silently terminates consumer iterator (no programmatic signal) #207 ERROR backpressure strategy now raises KalshiBackpressureError through the consumer iterator (was: silent StopAsyncIteration, indistinguishable from clean close).
• Errors: KalshiSequenceGapError + KalshiSubscriptionError lack channel / sid / seq context #213 Every KalshiSequenceGapError / KalshiSubscriptionError raise site in kalshi/ws/ populates the new channel / sid / last_seq / next_seq / op kwargs added in W0-B.

Tests

27 new tests. ruff + targeted pytest green.

Coordination note

Coordinated with W1-D (audit/w1d-ws-payloads) via IRC during execution. Clean merge — no orderbook.py conflict.

Closes #189, Closes #195, Closes #196, Closes #197, Closes #205, Closes #206, Closes #207, Closes #213
Refs #224

[claude]

Code Review — PR #231 (Wave 1-C WS Correctness)

This is a large, well-scoped correctness batch. The overall direction is right and the issues addressed are real. Notes below, roughly ordered by severity.

---

Correctness issues

_CHANNEL_PARAMS fallback silently accepts wrong keys on unmodeled channels

The fallback for unmodeled channels (_SUBSCRIBE_FORWARD_KEYS) is a union of all forward keys, including keys specific to individual channels (e.g., send_initial_snapshot is orderbook-only, shard_factor/shard_key are communications-only). A typo on root or a future channel will pass silently instead of failing loudly. Since the point of #195 is to catch typos, the fallback should probably be narrower, or at least documented clearly that the escape hatch is intentionally permissive.

resubscribe_one restores _stashing to prev_stashing in finally, even on success

prev_stashing = self._stashing
self._stashing = True
try:
    ...
finally:
    self._stashing = prev_stashing

If resubscribe_one is called while resubscribe_all has already set _stashing = True, the restore is correct. But if called from outside that context (gap handler calls it directly), prev_stashing is False and the finally correctly restores it to False. This is fine. However, if a future caller nests resubscribe_one calls while stashing is already active, the inner call will disable stashing on exit. Worth a comment explaining why the restore-to-prev_stashing pattern is safe here.

Duplicate _ErrorSentinel check in both get() and __anext__()

The same isinstance(item, _ErrorSentinel) guard appears in both get and __anext__. These share identical deque-pop logic. If the logic diverges (e.g., someone updates one but not the other), the error surface is asymmetric. Since consumers use async for (which goes through __anext__), the get path is only hit if someone calls it directly. Low risk, but worth a brief note about the duplication.

put_sentinel docstring says "Idempotent" but put_error could race it

Both put_sentinel and put_error check self._closed before setting it. There is no lock. In async code this is fine (single event loop, no true concurrency), but the comment is slightly misleading for anyone who reads it from a threading context. Minor.

---

Potential bugs

_handle_seq_gap: last_seq=gap.expected - 1 is slightly wrong after a reset

KalshiSequenceGapError(
    ...
    last_seq=gap.expected - 1,
    next_seq=gap.received,
)

For a forward gap, gap.expected is last + 1, so gap.expected - 1 == last, which is correct. For a kind="reset", gap.expected is still the value from SequenceTracker.track where expected = last + 1 even for the reset path. So this is consistent, but a consumer reading last_seq on a reset gap would get the value before the rewind (which is the watermark that triggered the reset). This matches the intent but could confuse callers. Consider documenting what last_seq means in reset vs. gap contexts.

apply_snapshot sid-migration path drops the old sid's bucket when it becomes empty, but remove does the same thing

old_bucket.discard(ticker)
if not old_bucket:
    self._sid_tickers.pop(prior.sid, None)

remove() has identical logic. This is correct and consistent. No bug, just noting the parallel.

resubscribe_one — unsubscribe failure is swallowed, but sub.server_sid = None is still set

except KalshiSubscriptionError:
    logger.debug(...)
self._sid_to_client.pop(old_sid, None)
sub.server_sid = None

The _sid_to_client.pop and sub.server_sid = None are outside the except block, so they run even if the unsubscribe failed. This is intentional (best-effort), but the logger.debug in the except loses the exc_info on Python ≥ 3.12 unless exc_info=True is passed. The code already passes exc_info=True here, so this is fine — just confirming it's correct.

---

Design observations

_PERMANENT_CLOSE_CODES uses getattr(e, "rcvd", None) to extract the close code

This is a duck-typed workaround for the websockets library's internal structure. It works for the current websockets version, but is fragile if websockets changes its exception attributes. A small comment linking to the websockets version this was tested against would help future maintainers. Alternatively, catch websockets.exceptions.ConnectionClosedError and access .rcvd directly via a typed import.

_handle_seq_gap docstring says step 2 "assigns a new sid" — but resubscribe_one returns None when the server doesn't echo a sid

The function handles this correctly (if isinstance(new_sid, int)), but the docstring should note the None case.

SequenceGap.kind is a bare str rather than a Literal["gap", "reset"]

Would make the type checker catch callers that misspell the string. Minor.

---

Test coverage

The test suite is thorough for the happy path and key error paths. A few gaps:

• resubscribe_one with a server that does NOT echo a sid — the None return path is untested. The new_sid could be absent in error responses.
• put_sentinel idempotency — there's no test that verifies a double put_sentinel does not evict a queued item. The comment in put_sentinel explains the risk clearly, but a regression test would lock it in.
• remove_by_sid with overlapping tickers (same ticker appearing under two sids simultaneously) — the apply_snapshot migration logic handles this, but there's no test exercising the sid-migration path directly.
• test_close_1006_reconnects: the test asserts reconnection by checking ConnectionState.STREAMING | CONNECTED, but doesn't verify that subscriptions were resubscribed (i.e., sub.server_sid changed). The reconnect could succeed at transport level while subscriptions are silently missing.

---

Style / conventions

• channels.py:214: op: str | None = None with # type: ignore[arg-type] on the call — the ignore suggests the type of op in KalshiSubscriptionError.__init__ doesn't accept str | None. This is worth cleaning up in the error class rather than suppressing the warning at the call site.
• The test_w1c_correctness.py module-level docstring references issue numbers (e.g., #189) — consistent with project style, fine.
• _wait_for helper in test_w1c_correctness.py is duplicated from other test files. Worth extracting to conftest.py or tests/ws/conftest.py to reduce drift.

---

Summary

The critical fix (#189 — real resubscribe instead of clear-and-pray) is well-implemented and the per-sid ticker tracking in OrderbookManager is the right data structure for all-markets subs. The error propagation work (#207) is a meaningful improvement for consumers. The permanent-close-code classification (#197) is pragmatic. The FixedPointCount / datetime type migrations across WS models are clean and well-tested.

Blocking items: none.
Suggested before merge: add a Literal["gap", "reset"] annotation to SequenceGap.kind, add the put_sentinel idempotency regression test, and extract _wait_for to a shared conftest.

[claude]

Code Review — Wave 1-C WS Correctness Sweep

This is a well-scoped, well-motivated batch fix. The critical issues (#189, #196, #207) were genuine correctness bugs — silent state corruption, silent error swallowing — and the solutions are sound. Overall: approve with minor notes.

---

What the PR does

Fixes 8 WebSocket correctness issues in one coordinated batch:

• WS: orderbook permanently desyncs after a sequence gap (no resnapshot) #189 [critical] — orderbook gap recovery now drives a real unsubscribe+resubscribe instead of clear-and-pray
• WS: server-side seq reset silently dispatches subsequent messages (no gap fired) #196 — sequence tracker now distinguishes gap / duplicate / reset (3-way instead of 2-way)
• WS: ERROR backpressure strategy silently terminates consumer iterator (no programmatic signal) #207 — ERROR backpressure strategy surfaces KalshiBackpressureError through the consumer iterator instead of an indistinguishable StopAsyncIteration
• WS: reconnect should fast-fail on permanent close codes (1008, 4001…) #197 — permanent close codes (4xxx, 1002/3/7-10) fast-fail with KalshiConnectionError instead of burning the retry budget
• WS: generic subscribe() silently drops unknown params keys #195 — to_subscribe_params validates against a per-channel allow-set; typos fail loudly
• WS: order_group_updates sequence gap has no recovery path (consumer silently misses events) #205 — order_group_updates gap routes through the same resubscribe path as orderbook
• WS: unsubscribe leaks orderbook state (long-running sub/unsub cycles grow memory) #206 — unsubscribe() now tears down OrderbookManager state for the sub's tickers
• Errors: KalshiSequenceGapError + KalshiSubscriptionError lack channel / sid / seq context #213 — every KalshiSequenceGapError / KalshiSubscriptionError raise site populated with channel / sid / last_seq / next_seq / op

---

Code Quality

Strengths:

• _ErrorSentinel + put_error is a clean, minimal mechanism. The frozen dataclass wrapping approach avoids any isinstance pollution on T, and idempotency on put_sentinel after put_error prevents the last-in-flight message eviction bug described in the docstring.
• remove_by_sid O(k) complexity is correct: the _sid_tickers index makes the gap path scale with tickers per sid, not total books.
• resubscribe_one correctly saves/restores _stashing instead of unconditionally setting it, and best-effort unsubscribe with continued fresh subscribe is the right policy for server-initiated resets.
• _CHANNEL_PARAMS fallback to frozenset(_SUBSCRIBE_FORWARD_KEYS) for unmodeled channels is a good escape hatch.

Minor issues:

1. _ErrorSentinel in get() and __anext__() — the sentinel check is duplicated across both methods (get at line 130 and __anext__ at line 150 in the new code). get() is already the backing implementation called from within the async iterator loop. If __anext__ delegates to get, the check only needs to live in one place. This is not a bug, but if they ever diverge it will be subtle.

2. apply_snapshot parameter sid — the new signature is apply_snapshot(self, msg, sid=None). The default None causes sid_val = sid if sid is not None else msg.sid to use msg.sid. But the call site in client.py already passes sid=snapshot.sid explicitly — so the default path is only reachable from external callers or tests that don't pass sid. This is fine, but it may be worth considering whether the parameter should be required (or removed) since msg.sid is always available.

3. kind: str = "gap" on SequenceGap — this is fine functionally, but a Literal["gap", "reset"] annotation would make the contract explicit without adding any runtime cost:

   from typing import Literal
   kind: Literal["gap", "reset"] = "gap"

4. _PERMANENT_CLOSE_CODES close code 1010 — RFC 6455 defines 1010 as the client's way of saying "mandatory extension was not negotiated." In practice this is sent by the client, not the server. Classifying it as permanent is defensible (if the server echoes it, reconnect will fail for the same reason), but worth a comment explaining the defensive inclusion.

5. resubscribe_one stash save/restore — when _stashing is False and the resubscribe completes, prev_stashing is restored to False in the finally. This means any frames that arrived during the unsubscribe→subscribe window are stashed but then permanently orphaned (the stash is only drained in _drain_resubscribe_stash, which is called from resubscribe_all). Is resubscribe_one expected to call _drain_resubscribe_stash after returning? If not, the comment about "frames... captured for later replay" in the docstring is misleading — they're captured but never replayed via resubscribe_one.

---

Tests

Test coverage is thorough and well-structured. A few observations:

• The _pause_recv_loop() pattern used in test_multi_ticker_gap.py and test_w1c_correctness.py is a good determinism fix. The comment explaining why it's needed (gap handler fires from inside the recv loop in production, so no concurrency, but direct-call tests must simulate quiescence) is helpful.
• test_close_1006_reconnects correctly validates the recoverable-code path by asserting state ends in STREAMING/CONNECTED — rather than just asserting no exception, which would still pass if the reconnect silently hung.
• asyncio.sleep(0.1) on line 1885 in test_error_strategy_overflow_raises_kalshi_backpressure_error — this is a timing assumption that could make the test flaky on a slow CI runner. A poll-based _wait_for on observable state (e.g. queue full) would be more robust.
• test_resubscribe_failure_broadcasts_error — the from kalshi.errors import KalshiSequenceGapError import inside the test body (line 1244) should be at the top of the file with the other imports.

---

Security / Correctness

• No new external inputs are introduced, no new deserialization paths. The data.get("msg", {}) pattern in resubscribe_one is consistent with the rest of the codebase and correctly handles a missing msg key.
• The getattr(e, "rcvd", None) / getattr(e, "sent", None) approach for extracting close codes from ConnectionClosed is correct — it avoids importing websockets internals directly and degrades gracefully if the library changes its exception shape.

---

Summary

The critical correctness fixes are solid. The main item worth a follow-up is the stash-drain question in resubscribe_one (#5 above) — if frames arriving during the resync window are stashed but never replayed, the docstring claim should be corrected. Everything else is minor polish.