perf(ws): orderbook materialization via model_construct + apply_snapshot single-copy + zero-delta cache preservation

[TexasCoding]

Summary

Three correctness/perf fixes on the OrderbookManager hot path. None
change observable wire behavior: returned Orderbook snapshots have the
same shape, the defensive-copy contract on public apply_snapshot is
preserved, and zero-delta messages still round-trip through
apply_delta. What changes is the cost per call and — for #347 — the
identity-stability of the #244 cached view across no-op deltas.

Issues closed

• Closes _BookState.to_orderbook re-validates every level through Pydantic on each materialization #327 — _BookState.to_orderbook skips re-validation via model_construct
• Closes Public OrderbookManager.apply_snapshot copies yes/no dicts twice — once via inplace adopt, once via dict() overwrite #344 — public apply_snapshot defensively copies each side dict exactly once
• Closes _apply_delta_inplace invalidates the cached Orderbook view even when delta=0 (no-op mutation) #347 — zero-effect deltas preserve the #244 materialization cache

Behavioral changes

• OrderbookManager.get(ticker) and apply_delta(...) now return the
  same Orderbook instance after a delta that nets to no change
  (e.g. delta=0 at an existing level). Previously the cache was
  invalidated and a fresh equal-value instance was returned. Value
  semantics are unchanged.

Tests

• tests/ws/test_orderbook.py::TestWave2CorrectnessRegressions
  • test_issue_327_to_orderbook_no_revalidation — spies on the
    validating __init__ of OrderbookLevel and Orderbook; asserts
    zero calls across snapshot + delta materialization.
  • test_issue_344_apply_snapshot_public_single_copy — spies on
    _BookState.__setattr__ and asserts each of yes / no is
    assigned exactly once per public apply_snapshot.
  • test_issue_347_zero_delta_preserves_cache — populates the #244
    cache, applies delta=0 at an existing level, asserts cache
    identity is preserved across the no-op and that a subsequent
    non-zero delta still invalidates (control).
  • test_issue_347_zero_delta_at_missing_level_preserves_cache —
    codifies that delta<=0 at a non-existent level is a no-op.

Source

Round-3 independent audit closure plan, wave W2.

[claude]

Code Review

Overall: Well-scoped, well-documented set of hot-path fixes. The three issues are addressed surgically, tests are thorough, and the docstrings explain the why clearly. A few findings below.

---

#327 — model_construct bypass

The optimization is sound for the current data path: _BookState.yes/no always holds Decimal→Decimal dicts that entered via the validated snapshot/delta path, so re-running DollarDecimal/FixedPointCount coercions in to_orderbook() is pure overhead.

One maintenance assumption worth documenting: Orderbook.yes/no use NullableList[OrderbookLevel], which wraps a BeforeValidator(_none_to_empty_list). Since model_construct skips all validators, a None value for either side would pass through silently (no coercion to []). In to_orderbook() both sides are built from list comprehensions, so this is safe today — but if a future code path ever assigns None into _BookState.yes/no directly, the resulting Orderbook.yes would be None instead of [] and callers would get a confusing TypeError. A brief assertion guard (or a note in the docstring alongside the existing #327 explanation) would lock down the assumption:

# If this assertion fires, data entered _BookState via an unvalidated path.
assert isinstance(self.yes, dict) and isinstance(self.no, dict)

(Or just update the docstring to list the specific fields assumed to be canonical — either way locks in the invariant for future readers.)

---

#344 — _install_snapshot refactor

Clean. The return-value threading (_install_snapshot → _BookState, used by apply_snapshot, ignored by _apply_snapshot_inplace) is immediately clear from the call sites. No issues.

---

#347 — Zero-delta cache preservation

Logic is correct. The restructured if/elif correctly avoids invalidation in both delta==0 at an existing level and delta<=0 at a missing level. All four test cases cover the meaningful branches including the control (non-zero delta still invalidates).

---

Tests — monkeypatch fixture declared but unused

In test_issue_327_to_orderbook_no_revalidation and test_issue_344_apply_snapshot_public_single_copy, the pytest monkeypatch fixture is injected via the parameter signature but never touched — the tests create a standalone pytest.MonkeyPatch() instance and manage cleanup manually with try/finally. This works, but is confusing: a reader would assume the fixture is used.

The idiomatic fix is to just use the injected fixture (typed pytest.MonkeyPatch), which handles cleanup automatically via its own finalizer:

def test_issue_327_to_orderbook_no_revalidation(
    self, monkeypatch: pytest.MonkeyPatch
) -> None:
    from kalshi.models.markets import Orderbook, OrderbookLevel
    from kalshi.ws import orderbook as ob_mod

    calls = {"level": 0, "ob": 0}
    # ... spy setup using monkeypatch.setattr(...) directly ...
    # no try/finally needed — pytest unwinds the fixture

If the manual MonkeyPatch instance is intentional (e.g. to scope cleanup more tightly), the monkeypatch parameter should be removed from the signature to avoid the misleading declaration.

---

Minor: module-level imports in test functions

from kalshi.models.markets import Orderbook, OrderbookLevel and from kalshi.ws import orderbook as ob_mod appear inside the test function bodies. Module-level imports are cleaner and consistent with the rest of the file — the only reason to import inside a test is to test import-time behavior or to avoid slowing collection, neither of which applies here.

---

Summary

Area	Finding	Severity
model_construct assumption	NullableList validator bypassed — safe today, fragile under future changes	Low
Test monkeypatch usage	Fixture declared but unused; manual try/finally pattern is confusing	Low
Test imports	Module-level preferred over per-function	Nitpick
_install_snapshot refactor	Clean, no issues	✅
Zero-delta logic (#347)	Correct, well-tested	✅
Single-copy snapshot (#344)	Correct	✅

The core logic and test coverage are solid. The monkeypatch fixture issue is the only concrete thing I'd fix before merge; the rest are minor polish.

[TexasCoding]

Round-2 review addressed in d215125:

Addressed:

• Removed unused monkeypatch: object fixture parameter from test_issue_327_to_orderbook_no_revalidation; refactored to use the pytest.MonkeyPatch fixture properly (dropped the manual pytest.MonkeyPatch() + try/finally boilerplate). Applied the same cleanup to test_issue_344_apply_snapshot_public_single_copy for consistency.
• Hoisted per-function imports (pytest, kalshi.models.markets, kalshi.ws.orderbook) from the new W2 regression tests to module scope.

Deferred:

• NullableList validator-bypass note (Low severity, flagged as 'safe today, fragile under future changes'). Deferred to v2.8 if it becomes relevant.

Verification:

• uv run ruff check tests/ws/test_orderbook.py → clean
• uv run pytest tests/ws/test_orderbook.py -v → 32 passed
• uv run mypy → pre-existing error count unchanged (6, all from earlier W2 perf commit)

[claude]

Round-3 Review

Round-2 feedback correctly addressed. Both items are resolved cleanly:

• monkeypatch fixture is now used properly in test_issue_327 and test_issue_344 — no manual MonkeyPatch() or try/finally needed.
• Module-level imports (pytest, Orderbook, OrderbookLevel, ob_mod) correctly hoisted out of the test function bodies.

---

Production code — no new issues

_install_snapshot refactor (#344): The _BookState return value is threaded correctly. apply_snapshot uses it; _apply_snapshot_inplace discards it. Clean.

_apply_delta_inplace zero-delta logic (#347): The restructured if/elif is correct across all cases:

existing_qty	delta	new_qty	action
exists	0	== existing	no-op, cache preserved ✅
exists	negative, removes level	≤ 0	delete + invalidate ✅
exists	nonzero change	!= existing	update + invalidate ✅
absent	> 0	—	insert + invalidate ✅
absent	≤ 0	—	no-op, cache preserved ✅

model_construct bypass (#327): Safe for the current invariant — _BookState.yes/no are always Decimal→Decimal dicts that entered via validated paths. to_orderbook builds both sides via list comprehensions so None can never flow through even though NullableList's BeforeValidator is bypassed. The earlier recommendation to add a guard assertion was deferred (Low severity) — that call stands but isn't blocking.

---

One observation on the test_issue_327 spy design

The spy wraps __init__ with **kw-only signature:

def spy_level_init(self: object, **kw: object) -> None:
    calls["level"] += 1
    orig_level_init(self, **kw)

Pydantic v2's generated __init__ is keyword-only in practice, so this works. The only edge case would be if the spy itself were ever called with positional args — which can't happen since the test asserts zero calls. No issue; just noting it for anyone who copy-pastes the pattern for a spy that needs to handle call-through.

---

mypy status

Round-2 verification noted "6 pre-existing mypy errors from an earlier W2 perf commit." pyproject.toml runs strict = true and CLAUDE.md says mypy must pass before every commit. Suggest verifying these 6 errors are covered by an existing suppression (# type: ignore) or are false-positives that don't trigger CI — if they're live strict errors they'll block merge once CI runs against this branch.

---

Summary

Area	Status
Round-2 fixes (monkeypatch, imports)	✅ Addressed
_install_snapshot refactor (#344)	✅ Correct
Zero-delta cache preservation (#347)	✅ Correct, well-tested
model_construct bypass (#327)	✅ Correct; NullableList guard still deferred
mypy pre-existing errors	⚠️ Verify not blocking CI

No blocking issues with the code or test changes. Ready to merge pending mypy CI confirmation.