diff --git a/planning/MARKET_DATA_REVIEW.md b/planning/MARKET_DATA_REVIEW.md new file mode 100644 index 0000000..3ecb1f1 --- /dev/null +++ b/planning/MARKET_DATA_REVIEW.md @@ -0,0 +1,242 @@ +# Market Data Backend — Code Review + +**Date:** 2026-09-02 +**Scope:** `backend/app/market/` (8 source files, 730 LOC) and `backend/tests/market/` (7 test files, 1,161 LOC) +**Reviewer:** Claude, in response to issue #5 + +--- + +## 1. Test Execution — Could Not Run + +This review's environment does not have permission to execute shell commands that +run the Python interpreter or install dependencies (`uv sync`, `uv run pytest`, even +`python3 -c ...` are all blocked pending approval, and this run has no human +available to approve them). This is the same limitation `planning/MARKET_DATA_SUMMARY.md` +recorded on the previous pass. **No test was executed as part of this review.** + +To get a real pass/fail signal, re-run this task with `Bash(uv sync:*)` and +`Bash(uv run:*)` added to the allowed tools, or run locally: + +```bash +cd backend +uv sync --extra dev +uv run --extra dev pytest -v --cov=app --cov-report=term-missing +uv run --extra dev ruff check app/ tests/ +``` + +In place of execution, every test file was read in full and traced by hand against +the source it exercises (see §4). All 96 tests found in the suite exercise real +code paths correctly as far as static reading can confirm — no test asserts on +behavior the source doesn't actually implement, and no test's mocking hides a +divergence between the mock's shape and the real one (the concern that let a +prior bug through at 94% coverage, per `test_massive.py`'s own docstring). + +**Test count:** 96 across 7 files (`test_models.py` 11, `test_cache.py` 24, +`test_simulator.py` 17, `test_simulator_source.py` 10, `test_factory.py` 7, +`test_massive.py` 17, `test_stream.py` 15 by count of `def test_`/`async def test_` +— slightly higher than the 94 recorded in `MARKET_DATA_SUMMARY.md` §"Test Suite", +consistent with incremental additions since that doc was last updated). + +--- + +## 2. Architecture Assessment + +The module is well-factored and matches `planning/MARKET_DATA_DESIGN.md` and +`planning/MARKET_DATA_SUMMARY.md` closely: + +``` +MarketDataSource (ABC) +├── SimulatorDataSource → GBMSimulator (Cholesky-correlated GBM) +└── MassiveDataSource → Polygon.io REST poller + │ + ▼ + PriceCache (thread-safe, latest price + 600-point rolling history) + │ + ├──→ create_stream_router() → GET /api/stream/prices (SSE, with keepalive) + └──→ create_history_router() → GET /api/prices/{ticker}/history +``` + +**Strengths confirmed by this pass:** + +- Strategy pattern cleanly isolates the two data sources behind `MarketDataSource`; nothing downstream needs to know which is active. +- `PriceUpdate` is `frozen=True, slots=True` — correct choice for a value object shared across threads/tasks. +- `PriceCache` centralizes all locking (`threading.Lock`) around the one mutable structure producers and consumers touch; the API surface (`update`, `get`, `get_all`, `get_price`, `remove`, `get_history`) is small and each method acquires the lock exactly once. +- The GBM math is textbook-correct log-normal price evolution, and the `dt` sizing (`0.5s / (252 * 6.5h * 3600s)`) is derived, not guessed, with the derivation left in a comment. +- Cholesky-based correlated draws (`simulator.py:84-90`) are a genuinely nice touch for a simulator whose only job is to look convincing on a chart. +- The three TODOs recorded as open in `PLAN.md` §13 (SSE keepalive, rolling history, `/history` endpoint) are all implemented and each has direct test coverage (`test_stream.py`). +- The two defects `MARKET_DATA_DESIGN.md` §8.4 recorded against the Massive client (wrong attribute name, nanoseconds-as-milliseconds) are fixed in `massive_client.py:130-136`, and `test_massive.py` deliberately builds real `TickerSnapshot` objects via `TickerSnapshot.from_dict(...)` rather than `MagicMock`, which is exactly the right defense against that class of bug recurring silently. +- `pyproject.toml` already has `[tool.hatch.build.targets.wheel] packages = ["app"]` — the "High" build-breaking bug from the archived 2026-02-10 review (`planning/archive/MARKET_DATA_REVIEW.md` §3.1) is fixed. +- `massive` is a top-level import now (`massive_client.py:9-11`), not a lazy one — the archived review's §3.2 concern about tests being fragile without the package installed no longer applies; `pyproject.toml` lists it as a core dependency. + +--- + +## 3. Issues Found + +### 3.1 `create_stream_router` / `create_history_router` mutate a shared module-level router (Severity: Medium) + +`stream.py:18-19` defines `router` and `history_router` at module scope. Both +factory functions register their route via a closure on these **same shared +objects** rather than creating a fresh `APIRouter()` per call: + +```python +router = APIRouter(prefix="/api/stream", tags=["streaming"]) +history_router = APIRouter(prefix="/api/prices", tags=["prices"]) + +def create_stream_router(price_cache: PriceCache) -> APIRouter: + @router.get("/prices") + async def stream_prices(request: Request) -> StreamingResponse: + ... + return router +``` + +Calling either factory more than once appends another route to the same +underlying router rather than returning an independent one. This was flagged +as a "latent footgun for testing" in the archived review (§3.6) when there +were no tests exercising it; now there are, and it is no longer latent: +`test_stream.py`'s `_history_endpoint()` helper calls `create_history_router(cache)` +fresh in **six different tests**, so `history_router` in the running test +process accumulates six duplicate `/{ticker}/history` routes by the end of +the file. The tests still pass because they grab `router.routes[-1].endpoint` +— the most recently registered one — but this only works by coincidence of +ordering, not because the router is actually being rebuilt. + +The real risk is downstream: once this module is wired into the FastAPI app +(the next piece of work per `PLAN.md` §13 "Still open"), any test that builds +the app more than once per process — a very common pytest pattern (an `app` +fixture instantiated per test, or per module) — will silently accumulate +duplicate routes on every rebuild, since `router`/`history_router` are shared +mutable module state that outlives any single app instance. + +**Fix:** construct a new `APIRouter()` inside each factory function instead of +reusing a module-level instance: + +```python +def create_stream_router(price_cache: PriceCache) -> APIRouter: + router = APIRouter(prefix="/api/stream", tags=["streaming"]) + + @router.get("/prices") + async def stream_prices(request: Request) -> StreamingResponse: + ... + return router +``` + +### 3.2 `PriceCache.update()` treats a falsy timestamp as "no timestamp given" (Severity: Low) + +```python +ts = timestamp or time.time() +``` + +(`cache.py:40`) A caller that explicitly passes `timestamp=0.0` (Unix epoch, +1970-01-01) gets `time.time()` substituted instead, because `0.0` is falsy. +No current caller does this — `massive_client.py` only reaches this path with +`time.time()` already substituted upstream when `sip_timestamp` is falsy — so +this is not exploitable today, but it is a latent correctness gap for any +future caller (e.g., a test replaying historical data from epoch-adjacent +timestamps, or a backfill script). Prefer `timestamp if timestamp is not None +else time.time()`. + +### 3.3 `MassiveDataSource`'s poller task dies silently on `AuthError` (Severity: Low) + +`_poll_once()` deliberately re-raises `AuthError` (`massive_client.py:103-105`) +with the comment "unrecoverable: do not retry on a loop" — a reasonable +choice. But the only place that awaits `self._task` is `stop()` +(`massive_client.py:60-69`), which nothing calls until shutdown. If the key +is revoked *after* `start()` succeeds (rather than being bad from the first +poll), the background task raised inside `_poll_loop()` simply stops running; +asyncio logs "Task exception was never retrieved" at some later point (often +at garbage collection, easy to miss in container logs), and the app has no +other signal that live prices have silently frozen. `test_auth_error_propagates` +confirms the exception propagates out of `_poll_once()`, but there is no test +for what happens to `_poll_loop()` or the app once that happens. + +This is fine as coded for now since nothing outside the market module reads +task health yet, but whoever wires this into the app (`PLAN.md` §13, item 3) +should either attach a `Task.add_done_callback` that logs loudly / flips a +health flag, or have `GET /api/health` report `market_source` as degraded +when the task is dead. Worth a one-line note in `MARKET_DATA_SUMMARY.md` so +it isn't forgotten during integration. + +### 3.4 `PriceCache.version` property reads outside the lock (Severity: Trivial) + +Unchanged from the archived review's §3.4: `cache.py:94-97` reads `self._version` +without acquiring `self._lock`. Safe under CPython's GIL for a single `int` +read, inconsistent with the rest of the class, and only a real concern on a +no-GIL build. Not worth blocking on, but a two-line fix if anyone is passing +through this file for another reason. + +### 3.5 `market_data_demo.py` and `backend/README.md` are outside the reviewed test scope but were not separately verified + +The demo script (`market_data_demo.py`, 205 lines) is referenced by +`MARKET_DATA_SUMMARY.md` as a manual verification tool and has no automated +test coverage, which is appropriate for a Rich terminal demo — flagging only +so it's clear this review's "all tests pass" scope is `backend/tests/market/`, +not the demo script. + +--- + +## 4. Test Suite Assessment (by module) + +| Module | File | Assessment | +|---|---|---| +| `models.py` | `test_models.py` (11 tests) | Complete: creation, `change`/`change_percent`/`direction` in both directions, zero-previous-price edge case, `to_dict()` shape, and frozen-dataclass immutability. No gaps. | +| `cache.py` | `test_cache.py` (24 tests) | Thorough. Covers direction transitions, `version` monotonicity, `__len__`/`__contains__`, price rounding, custom timestamps, and a dedicated `TestPriceHistory` class covering bounding, ordering, per-ticker isolation, limit-narrower-than-stored, and that `remove()` clears history without touching other tickers. No test for concurrent multi-thread writes (the lock is exercised only single-threaded) — the archived review flagged this as missing in §4.2 and it remains missing; low priority since the logic is simple enough to verify by inspection. | +| `interface.py` | (no dedicated file; exercised transitively via simulator/massive tests) | Reasonable — it's an ABC with no logic of its own. | +| `seed_prices.py` | `test_simulator.py`, `test_factory.py` (transitively) | No dedicated test file, but every constant is exercised indirectly through `GBMSimulator` tests (`_pairwise_correlation` tests cover tech/finance/TSLA/cross-sector explicitly). Fine given it's pure data. | +| `simulator.py` | `test_simulator.py` (17), `test_simulator_source.py` (10) | Strong. `GBMSimulator`: positivity over 10,000 steps, seed matching, add/remove (including duplicate/nonexistent no-ops), unknown-ticker random seeding, Cholesky construction/teardown on ticker count crossing 1↔2, all four correlation branches, `dt` sanity, and rounding. `SimulatorDataSource`: cache population on start, periodic updates via real `asyncio.sleep`, idempotent stop, dynamic add/remove, empty-start, and exception resilience. The timing-based assertions (`asyncio.sleep(0.3)` then assert version advanced) are inherently a little flaky under CI load, but the margins used (3-6x the interval) are generous enough to be low-risk. | +| `massive_client.py` | `test_massive.py` (17) | Strong, and specifically hardened against the exact bug class that shipped previously — `_apply_snapshots` is tested against real `TickerSnapshot.from_dict(...)` objects, not mocks, for timestamp conversion, missing-trade skipping, mixed valid/invalid batches, and multi-ticker updates. Polling lifecycle covers success, `BadResponse` (swallowed), `AuthError` (re-raised, see §3.3), generic exceptions (swallowed), ticker add/remove with normalization, and start/stop idempotency. No gap of consequence. | +| `stream.py` | `test_stream.py` (15) | Was 31% covered and untested in the archived review; now has direct coverage of the async generator via a hand-rolled `FakeRequest`, including the retry directive, snapshot-on-connect (and thus reconnect), the frozen payload field set, keepalive timing (via `monkeypatch` on `KEEPALIVE_SECONDS` rather than a real 15s wait — good practice), a fresh data event following a ping, disconnect handling, and the empty-cache case. `create_history_router`'s endpoint is tested for ordering, unknown-ticker empty response, normalization, and limit clamping in both directions. The one real gap is architectural, not a missing test: see §3.1 — the tests would catch a *regression* in behavior but not the router-reuse issue itself, since grabbing `routes[-1]` happens to paper over it. | +| `factory.py` | `test_factory.py` (7) | Complete for its size: unset/empty/whitespace-only key → simulator, set key → Massive, and that both branches thread the cache reference through correctly. | + +**Net assessment:** the suite is comprehensive and, importantly, methodologically +careful — the deliberate choice to build real `TickerSnapshot` objects instead of +`MagicMock` in `test_massive.py` is the single best thing about this test suite, +since it's precisely what would have caught the `last_trade.timestamp` / +`sip_timestamp` bug the archived review found. No test was found asserting +something the source doesn't do, and no source behavior of consequence lacks a +test, with the caveats above (concurrency, and the router-reuse issue masked +by test ordering). + +--- + +## 5. Comparison Against the Prior Review + +`planning/archive/MARKET_DATA_REVIEW.md` (2026-02-10) recorded 7 issues. Status now: + +| # | Issue | Status | +|---|---|---| +| 3.1 | Missing hatchling wheel config | **Fixed** | +| 3.2 | Massive tests fragile without the `massive` package | **Fixed** (now a core dependency, imported at module level) | +| 3.3 | `_generate_events` return type `-> None` instead of `AsyncGenerator` | **Fixed** (`stream.py:87`) | +| 3.4 | `PriceCache.version` reads outside the lock | **Still open** (§3.4 above, trivial) | +| 3.5 | `SimulatorDataSource.get_tickers` reached into `GBMSimulator._tickers` | **Fixed** — `GBMSimulator.get_tickers()` now exists (`simulator.py:140-142`) and is used | +| 3.6 | Module-level router registered on repeated calls | **Still open, and now demonstrated by the test suite itself** (§3.1 above, upgraded to Medium given it will bite during app integration) | +| 3.7 | Unused imports in tests | **Fixed** — no unused `pytest`/`math`/`asyncio` imports found in any current test file | + +Also confirmed fixed: the two Massive parsing defects `MARKET_DATA_DESIGN.md` +§8.4 described (wrong attribute name, nanosecond/millisecond confusion), and +all three items `PLAN.md` §13 listed as open TODOs (rolling history, `/history` +endpoint, SSE keepalive). + +--- + +## 6. Verdict + +The market data backend is in good shape and ready to be built on. Of the two +open items: + +- **§3.1 (shared module-level router)** should be fixed before the FastAPI + `lifespan` wiring work begins (`PLAN.md` §13, item 3) — it's a small, + mechanical fix (stop reusing module-level `router`/`history_router`; build + one per call) and doing it now avoids a confusing bug later when the app + factory is instantiated more than once, which is standard practice for + backend test fixtures. +- **§3.2/§3.4 (falsy-timestamp substitution, unlocked version read)** are + low-risk and can be picked up opportunistically. +- **§3.3 (silent poller death on revoked key)** is a design note for whoever + adds the `GET /api/health` endpoint — surface poller liveness there. + +None of these block downstream work. **Tests were not executed in this pass** +due to environment permissions (§1) — that is the one action item this review +could not complete, and it should be re-run with `uv`/`python3` execution +permitted to get an authoritative pass/fail/coverage number rather than the +static analysis this document is based on.