fix(ws): roll back seq on backpressure + reset all tickers on multi-ticker gap

[TexasCoding]

Summary

Wave 4 — two correctness fixes that build on the recv-loop hardening landed in #138. Two commits, one PR.

#78 — ERROR-overflow advances last_seq, silently desyncing orderbook

SequenceTracker.track(sid, seq, msg_type) advanced _last_seq[sid] = seq before the message was enqueued. With ERROR overflow strategy, put() raises KalshiBackpressureError — but the tracker had already moved the watermark. On reconnect / future gap check, the dropped sequence is treated as already-seen → silent orderbook desync.

Fix (option (b), two-phase provisional/rollback):

• kalshi/ws/sequence.py — added peek(sid) and rollback(sid, prev) to SequenceTracker.
• kalshi/ws/client.py — _process_frame captures the pre-track watermark via peek, wraps dispatch in try/except KalshiBackpressureError, calls rollback(sid, prev), then re-raises so the recv-loop's fix(ws): recv-loop overhaul — reconnect races, narrowed exceptions, log scrub, ConnectionManager API #138 sentinel-broadcast handler still fires.

Option (a) — "advance after dispatch" — would also require deferring OrderbookManager.apply_* (in-place internal state) past the queue.put; that's a larger structural change. Two-phase rollback is surgical and preserves the existing gap-skip early-return semantics.

#79 — Multi-ticker seq-gap: only tickers[0] was cleared

For a multi-ticker orderbook_delta subscription, the gap handler only reset _last_seq for tickers[0]. Other tickers' seq state lingered → false gaps or missed gaps on later deltas.

Fix (all-tickers reset): the gap envelope itself (sid + expected + received) carries no ticker — per-ticker resolution is impossible at the gap-detection layer. _handle_seq_gap now iterates the full subscription's market_tickers list and resets each.

Sibling-branch invariants (Wave 3) preserved

• KalshiBackpressureError from dispatch still breaks the recv loop and broadcasts sentinels (WS recv loop swallows all exceptions with broad except Exception: continue #83 in fix(ws): recv-loop overhaul — reconnect races, narrowed exceptions, log scrub, ConnectionManager API #138). _process_frame re-raises after rollback.
• No changes to the recv-loop exception ladder, reconnect path, or dispatcher.
• Existing test_backpressure_error_breaks_loop_and_sentinels passes unchanged.

Closes #78
Closes #79

Test plan

• +8 tests: 5 in tests/ws/test_seq_correctness.py (new), 3 in tests/ws/test_multi_ticker_gap.py (new)
• Suite: 1654 passed, 48 skipped (was 1646)
• uv run ruff check . clean
• uv run mypy kalshi/ clean (strict)

[claude]

Code Review — PR #139

Overview

Two targeted correctness fixes for WebSocket sequence tracking. Both patches are surgical, well-scoped, and accompanied by dedicated regression tests. The design choices (two-phase rollback vs. deferred advance, all-tickers reset vs. per-ticker) are well-reasoned and match the constraints of the existing architecture.

---

Fix #78 — Seq rollback on backpressure

Approach is correct. Two-phase provisional/rollback is the right call here given that OrderbookManager.apply_* mutates in-place. The alternative (deferred advance) would require significant structural changes to the dispatch/orderbook pipeline.

One clarity nit — tracked = True placement:

ok = await self._seq_tracker.track(...)
tracked = True          # ← set before the gap-check early return
if not ok:
    await self._handle_seq_gap(...)
    return

tracked is only meaningful in the except KalshiBackpressureError block, which is unreachable when ok=False (we return first). It works correctly, but setting tracked = True before the gap branch is a subtle read-order surprise. Consider moving it to after the early-return check:

ok = await self._seq_tracker.track(...)
if not ok:
    await self._handle_seq_gap(...)
    return
tracked = True          # only reached when tracking succeeded and we'll dispatch

The deliberate non-rollback of orderbook state is correct and well-documented. On KalshiBackpressureError, the recv loop tears down via sentinel broadcast anyway, so an orphaned-but-never-desynced book is fine.

---

Fix #79 — Multi-ticker gap clear

Clean and obvious fix. Changing if tickers and self._orderbook_mgr: → if self._orderbook_mgr: for ticker in tickers: correctly handles both the empty-list case (loop is a no-op) and the multi-ticker case. No issues here.

---

Tests

test_seq_correctness.py — The unit tests for peek/rollback are well-structured and cover the first-message (prev=None) edge case.

test_dropped_delta_keeps_seq_watermark_unchanged — One concern: the test uses a fixed await asyncio.sleep(0.1) to ensure the snapshot is processed before the overflow delta is sent:

await fake_ws.send_to_all({ ...snapshot... })
await asyncio.sleep(0.1)   # ← timing-based
assert session._seq_tracker.peek(sid) == 1

This is a potential flakiness source under CI load. The test_multi_ticker_gap.py tests use the better event-based polling pattern:

async with asyncio.timeout(2.0):
    while not await all_seeded():
        await asyncio.sleep(0.01)

The backpressure test should use the same approach — poll until session._seq_tracker.peek(sid) == 1 before sending the overflow delta.

test_multi_ticker_gap.py — All three scenarios are well-chosen: multi-ticker (the regression), single-ticker (sanity), and no-tickers-param (edge case). Test isolation via _StubMgr for the no-tickers case is the right call.

---

Convention — issue numbers in source docstrings

CLAUDE.md: "Don't reference the current task, fix, or callers... those belong in the PR description and rot as the codebase evolves."

The docstrings and inline comments liberally reference #78 and #79:

def peek(self, sid: int) -> int | None:
    """...Used by ... to capture the pre-track watermark so it can be
    restored via :meth:`rollback` if the downstream dispatch fails
    (issue #78: ERROR-overflow backpressure must not silently advance ...)
    """

The invariant being documented is valuable; the issue number isn't. Replace with a description of the invariant itself, e.g.:

Capture the pre-track watermark before calling :meth:`track`. If the
downstream dispatch raises, pass it to :meth:`rollback` to prevent the
dropped message from being silently treated as already-seen.

Same for the rollback docstring, the _handle_seq_gap docstring, and the inline # #78:/# #79: comment markers in client.py. The test module docstrings ("""Regression tests for ... (#78)""") are fine — cross-referencing issues in test files is reasonable.

---

Summary

Correctness	✅ Both fixes are correct
Architecture fit	✅ Surgical; preserves existing exception ladder
Test coverage	✅ 8 new tests, all meaningful
Flakiness risk	⚠️ asyncio.sleep(0.1) in one test
Convention	⚠️ Issue numbers in production code docstrings/comments

The logic is solid. Two things to address before merge: swap the fixed sleep for event-based polling, and strip the #78/#79 references from production-code comments (keep them in tests and PR description).

[TexasCoding]

CI failure root-caused + bot nits addressed in latest commit:

🔴 CI failure — ast-serialize 0.5.0 (transitive of mypy 2.1.0) was published between Wave 3 landing and this PR opening. It dropped cp39-abi3 wheels and only ships cp314; CI's fresh resolution (uv.lock is gitignored) broke on Python 3.12 + 3.13. Pinned ast-serialize<0.5 in dev deps with a comment explaining the trigger to remove. Fresh local re-resolve picks 0.4.0 cleanly.

Bot review:

• 🟦 tracked = True placement — moved past the if not ok: return early-exit so the value is only set when it's actually meaningful.
• 🟦 asyncio.sleep(0.1) in test_dropped_delta_keeps_seq_watermark_unchanged — replaced with a poll on peek(sid) == 1 bounded by wait_for(2.0), matching the pattern in test_multi_ticker_gap.py.
• 🟦 #78 / #79 references in production-code docstrings / comments — stripped per CLAUDE.md §3. The invariants themselves are still documented, just without the issue-number prefix. Test-file docstrings retain the cross-references.

Verify: tests/ws/test_seq_correctness.py + test_multi_ticker_gap.py 8 passed. ruff + mypy --strict clean.

[claude]

Code Review

Overall: Solid, surgical correctness fix. Both bugs are real and the solutions are well-reasoned. The two-phase peek/rollback pattern is clean, and the multi-ticker fix is the minimal correct change. A few observations below — mostly minor.

---

kalshi/ws/sequence.py — peek / rollback

• Both methods are sync (no async) which is correct — they only access an in-memory dict and have no I/O.
• rollback using pop(sid, None) when prev is None is correct: it removes the entry entirely, so the next track() is treated as the first-ever message for that sid. The test test_rollback_to_none_removes_entry verifies this.
• Minor nit: the docstrings are longer than the methods themselves. Per project conventions (CLAUDE.md §2), comments explaining what the code does are discouraged; the body of each method is self-evident. The why (two-phase rollback for backpressure compensation) belongs in the caller's docstring, which it already has in _process_frame. Consider trimming to a single-line each.

---

kalshi/ws/client.py — _process_frame

Redundant guards in the except block:

if tracked and sid is not None and self._seq_tracker:
    self._seq_tracker.rollback(sid, prev_seq)

tracked = True is only reachable inside if sid is not None and seq is not None and self._seq_tracker:, so the two extra conditions can never be False when tracked is True. The guard can be simplified to just if tracked:. The current form is harmless but adds noise.

Acknowledged design gap (not a new bug):

The docstring correctly notes that OrderbookManager state is mutated before dispatch and is not rolled back on KalshiBackpressureError. The recv loop tears down immediately after, so the orphaned book is never consumed — this is a safe tradeoff. That said, the current code applies the orderbook mutation unconditionally before the try/except:

# snapshot/delta applied here (mutates OrderbookManager in place)
...
try:
    await self._dispatcher.dispatch(raw)
except KalshiBackpressureError:
    ...  # book already mutated; only seq is rolled back

If a future refactor ever changes the tear-down behavior (e.g., soft overflow instead of fatal), the inconsistency between seq rollback and orderbook state could silently resurface. Worth a TODO-comment on the orderbook apply block, or a GitHub issue to track. The PR description already explains it, but in-code visibility would help future readers.

---

kalshi/ws/client.py — _handle_seq_gap

The fix (iterate all tickers, not just tickers[0]) is correct. The if self._orderbook_mgr: guard (moved outside the if tickers check) is an improvement — it avoids silently skipping the seq reset when there are no tickers. No issues here.

---

pyproject.toml

"ast-serialize<0.5",

The comment explains the rationale clearly. A few thoughts:

• This is a transitive dep pin (brought in by mypy 2.1.x), which can cause solver conflicts if other packages later require >=0.5. Transitive pins are fine for CI stability but tend to rot.
• Is there a GitHub issue open for removing this once upstream publishes broader wheels? If not, worth filing so it doesn't get forgotten.

---

Tests

test_seq_correctness.py:

• TestSequenceTrackerRollback is a clean unit test of the new API surface. All four cases are meaningful.
• TestBackpressureDoesNotAdvanceSeq.test_dropped_delta_keeps_seq_watermark_unchanged is the critical regression path. Using wait_for(snapshot_landed(), timeout=2.0) to avoid fixed sleeps is the right pattern. Awaiting _recv_task directly instead of sleeping is clean.
• One observation: the test asserts len(collected) == 1 (only the snapshot landed). This is a good invariant. But if the test ever runs on a very slow CI box, the asyncio.timeout(2.0) on the snapshot-landed poll could be tight. Not a real concern given this is local-loopback, but consider bumping to 5.0 for consistency with config.timeout.

test_multi_ticker_gap.py:

• test_gap_clears_all_tickers_in_sub directly calls _handle_seq_gap which removes the race from forcing a real wire gap. Good test design.
• test_no_tickers_param_does_not_raise covers the empty-params edge case. The _StubMgr inline class is a reasonable approach for this isolated unit test. The # type: ignore[assignment] on ws._sub_mgr = _StubMgr() is expected since it bypasses the type system.
• The # type: ignore[no-untyped-def] annotations on fixture params are consistent with the rest of the test suite.

---

Summary

Area	Verdict
Correctness (fix #78)	✅ Correct; two-phase rollback is the right approach
Correctness (fix #79)	✅ Correct; all-tickers clear is the only safe option
Style / conventions	Minor: redundant guards in except block, verbose docstrings
Tests	✅ Comprehensive; 8 new tests, deterministic, no fixed sleeps
pyproject.toml pin	✅ Acceptable temporary fix; track for removal
mypy/ruff	Per PR description: both pass clean

Two action items worth considering before merge:

1. Simplify if tracked and sid is not None and self._seq_tracker: → if tracked: in the except block.
2. Add a TODO comment on the orderbook-apply block (or open a follow-up issue) noting that rollback only covers seq, not book state, so future changes to the tear-down path should re-evaluate.

Neither is a blocker — happy to approve once you've had a look.