diff --git a/backend/pyproject.toml b/backend/pyproject.toml index e172cca2..40dcabf2 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -18,6 +18,7 @@ dev = [ "pytest-asyncio>=0.24.0", "pytest-cov>=5.0.0", "ruff>=0.7.0", + "httpx>=0.27.0", ] [build-system] diff --git a/backend/tests/market/test_cache.py b/backend/tests/market/test_cache.py index b5ab3d55..e7d21eac 100644 --- a/backend/tests/market/test_cache.py +++ b/backend/tests/market/test_cache.py @@ -1,5 +1,7 @@ """Tests for PriceCache.""" +import threading + from app.market.cache import PriceCache @@ -101,3 +103,29 @@ def test_price_rounding(self): cache = PriceCache() update = cache.update("AAPL", 190.12345) assert update.price == 190.12 + + def test_concurrent_updates_are_not_lost(self): + """Many threads hammering update() concurrently must not lose or + corrupt writes — the version counter should exactly equal the number + of update() calls, and every ticker should end up with a valid entry. + """ + cache = PriceCache() + tickers = [f"T{i}" for i in range(10)] + updates_per_thread = 200 + num_threads = 8 + + def worker(thread_id: int) -> None: + for i in range(updates_per_thread): + ticker = tickers[(thread_id + i) % len(tickers)] + cache.update(ticker, 100.0 + i) + + threads = [threading.Thread(target=worker, args=(t,)) for t in range(num_threads)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert cache.version == num_threads * updates_per_thread + assert len(cache) == len(tickers) + for ticker in tickers: + assert cache.get(ticker) is not None diff --git a/backend/tests/market/test_simulator.py b/backend/tests/market/test_simulator.py index 02f7f8a8..75d1fe87 100644 --- a/backend/tests/market/test_simulator.py +++ b/backend/tests/market/test_simulator.py @@ -129,3 +129,20 @@ def test_prices_rounded_to_two_decimals(self): if "." in price_str: decimal_part = price_str.split(".")[1] assert len(decimal_part) <= 2 + + def test_full_default_watchlist_builds_valid_cholesky(self): + """The full 10-ticker default watchlist's correlation matrix (mixing + tech, finance, and TSLA's special-cased correlation) must produce a + valid Cholesky decomposition and step cleanly, not just the 1-2 + ticker cases exercised elsewhere in this file.""" + tickers = list(SEED_PRICES.keys()) + sim = GBMSimulator(tickers=tickers) + + assert sim._cholesky is not None + assert sim._cholesky.shape == (len(tickers), len(tickers)) + + for _ in range(50): + result = sim.step() + assert set(result.keys()) == set(tickers) + for price in result.values(): + assert price > 0 diff --git a/backend/tests/market/test_stream.py b/backend/tests/market/test_stream.py new file mode 100644 index 00000000..105af9a8 --- /dev/null +++ b/backend/tests/market/test_stream.py @@ -0,0 +1,167 @@ +"""Integration tests for the SSE price streaming endpoint. + +`_generate_events` is a `while True` loop that only exits when the client +disconnects. Both `httpx.ASGITransport` and Starlette's `TestClient` fully +await an ASGI call to completion before handing back anything to consume — +neither delivers a disconnect mid-stream, so driving this endpoint through +either one deadlocks (confirmed empirically). Instead, these tests drive +`_generate_events` directly with a minimal fake `Request` whose +`is_disconnected()` we control, and separately exercise the routed endpoint +function (returned by `create_stream_router`) to cover response/header +wiring without consuming its unbounded body. +""" + +import asyncio +import json + +import pytest +from fastapi.responses import StreamingResponse + +from app.market.cache import PriceCache +from app.market.stream import _generate_events, create_stream_router + + +class _FakeClient: + host = "test-client" + + +class _FakeRequest: + """Minimal stand-in for fastapi.Request — only what _generate_events uses.""" + + def __init__(self) -> None: + self.client = _FakeClient() + self._disconnected = False + + async def is_disconnected(self) -> bool: + return self._disconnected + + def disconnect(self) -> None: + self._disconnected = True + + +def _parse_data_event(event: str) -> dict: + assert event.startswith("data: ") + assert event.endswith("\n\n") + return json.loads(event[len("data: ") : -2]) + + +class TestCreateStreamRouter: + """Tests for the router factory itself (not the streaming body).""" + + def test_builds_independent_routers(self): + """Each call must return its own APIRouter, not share module state. + + Regression test: create_stream_router() used to decorate onto a + shared module-level router, so calling it twice would register + /prices twice on the same object. + """ + cache = PriceCache() + router_a = create_stream_router(cache) + router_b = create_stream_router(cache) + + assert router_a is not router_b + assert len(router_a.routes) == 1 + assert len(router_b.routes) == 1 + + @pytest.mark.asyncio + async def test_endpoint_returns_streaming_response_with_sse_headers(self): + cache = PriceCache() + cache.update("AAPL", 190.00) + router = create_stream_router(cache) + endpoint = router.routes[0].endpoint + + response = await endpoint(_FakeRequest()) + + assert isinstance(response, StreamingResponse) + assert response.media_type == "text/event-stream" + assert response.headers["cache-control"] == "no-cache" + assert response.headers["connection"] == "keep-alive" + assert response.headers["x-accel-buffering"] == "no" + + +@pytest.mark.asyncio +class TestGenerateEvents: + """Tests for the _generate_events async generator directly.""" + + async def test_first_event_is_retry_directive(self): + cache = PriceCache() + request = _FakeRequest() + gen = _generate_events(cache, request, interval=0.01) + + first = await gen.__anext__() + assert first == "retry: 1000\n\n" + + request.disconnect() + with pytest.raises(StopAsyncIteration): + await gen.__anext__() + + async def test_emits_seeded_prices(self): + cache = PriceCache() + cache.update("AAPL", 190.50) + cache.update("GOOGL", 175.25) + request = _FakeRequest() + gen = _generate_events(cache, request, interval=0.01) + + await gen.__anext__() # retry directive + payload = _parse_data_event(await gen.__anext__()) + + assert payload["AAPL"]["price"] == 190.50 + assert payload["AAPL"]["direction"] == "flat" + assert payload["GOOGL"]["price"] == 175.25 + + request.disconnect() + with pytest.raises(StopAsyncIteration): + await gen.__anext__() + + async def test_reflects_subsequent_updates(self): + cache = PriceCache() + cache.update("AAPL", 190.00) + request = _FakeRequest() + gen = _generate_events(cache, request, interval=0.01) + + await gen.__anext__() # retry directive + first_payload = _parse_data_event(await gen.__anext__()) + assert first_payload["AAPL"]["price"] == 190.00 + + cache.update("AAPL", 191.00) + second_payload = _parse_data_event(await gen.__anext__()) + assert second_payload["AAPL"]["price"] == 191.00 + assert second_payload["AAPL"]["direction"] == "up" + + request.disconnect() + with pytest.raises(StopAsyncIteration): + await gen.__anext__() + + async def test_no_data_event_while_cache_stays_empty(self): + """With an empty cache, the version never changes, so no data event + should ever be produced — only the initial retry directive.""" + cache = PriceCache() + request = _FakeRequest() + gen = _generate_events(cache, request, interval=0.02) + + first = await gen.__anext__() + assert first == "retry: 1000\n\n" + + async def disconnect_after_several_ticks() -> None: + await asyncio.sleep(0.1) # ~5 ticks at a 0.02s interval + request.disconnect() + + asyncio.create_task(disconnect_after_several_ticks()) + + # If a data event had been produced, anext() would return it instead + # of the loop eventually hitting the disconnect and raising here. + with pytest.raises(StopAsyncIteration): + await gen.__anext__() + + async def test_stops_on_disconnect_mid_stream(self): + cache = PriceCache() + cache.update("AAPL", 190.00) + request = _FakeRequest() + gen = _generate_events(cache, request, interval=0.01) + + await gen.__anext__() # retry directive + await gen.__anext__() # initial data event + + request.disconnect() + with pytest.raises(StopAsyncIteration): + await gen.__anext__() diff --git a/backend/uv.lock b/backend/uv.lock index 67d471b2..fd497795 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -177,6 +177,7 @@ dependencies = [ [package.optional-dependencies] dev = [ + { name = "httpx" }, { name = "pytest" }, { name = "pytest-asyncio" }, { name = "pytest-cov" }, @@ -186,6 +187,7 @@ dev = [ [package.metadata] requires-dist = [ { name = "fastapi", specifier = ">=0.115.0" }, + { name = "httpx", marker = "extra == 'dev'", specifier = ">=0.27.0" }, { name = "massive", specifier = ">=1.0.0" }, { name = "numpy", specifier = ">=2.0.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.3.0" }, @@ -206,6 +208,19 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/04/4b/29cac41a4d98d144bf5f6d33995617b185d14b22401f75ca86f384e87ff1/h11-0.16.0-py3-none-any.whl", hash = "sha256:63cf8bbe7522de3bf65932fda1d9c2772064ffb3dae62d55932da54b31cb6c86", size = 37515, upload-time = "2025-04-24T03:35:24.344Z" }, ] +[[package]] +name = "httpcore" +version = "1.0.9" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "certifi" }, + { name = "h11" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/06/94/82699a10bca87a5556c9c59b5963f2d039dbd239f25bc2a63907a05a14cb/httpcore-1.0.9.tar.gz", hash = "sha256:6e34463af53fd2ab5d807f399a9b45ea31c3dfa2276f15a2c3f00afff6e176e8", size = 85484, upload-time = "2025-04-24T22:06:22.219Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/7e/f5/f66802a942d491edb555dd61e3a9961140fd64c90bce1eafd741609d334d/httpcore-1.0.9-py3-none-any.whl", hash = "sha256:2d400746a40668fc9dec9810239072b40b4484b640a8c38fd654a024c7a1bf55", size = 78784, upload-time = "2025-04-24T22:06:20.566Z" }, +] + [[package]] name = "httptools" version = "0.7.1" @@ -235,6 +250,21 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/53/cf/878f3b91e4e6e011eff6d1fa9ca39f7eb17d19c9d7971b04873734112f30/httptools-0.7.1-cp314-cp314-win_amd64.whl", hash = "sha256:cfabda2a5bb85aa2a904ce06d974a3f30fb36cc63d7feaddec05d2050acede96", size = 88205, upload-time = "2025-10-10T03:55:00.389Z" }, ] +[[package]] +name = "httpx" +version = "0.28.1" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "anyio" }, + { name = "certifi" }, + { name = "httpcore" }, + { name = "idna" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/b1/df/48c586a5fe32a0f01324ee087459e112ebb7224f646c0b5023f5e79e9956/httpx-0.28.1.tar.gz", hash = "sha256:75e98c5f16b0f35b567856f597f06ff2270a374470a5c2392242528e3e3e42fc", size = 141406, upload-time = "2024-12-06T15:37:23.222Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/2a/39/e50c7c3a983047577ee07d2a9e53faf5a69493943ec3f6a384bdc792deb2/httpx-0.28.1-py3-none-any.whl", hash = "sha256:d909fcccc110f8c7faf814ca82a9a4d816bc5a6dbfea25d6591d6985b8ba59ad", size = 73517, upload-time = "2024-12-06T15:37:21.509Z" }, +] + [[package]] name = "idna" version = "3.11" diff --git a/planning/MARKET_DATA_REVIEW.md b/planning/MARKET_DATA_REVIEW.md index 830db8ea..145c4ee4 100644 --- a/planning/MARKET_DATA_REVIEW.md +++ b/planning/MARKET_DATA_REVIEW.md @@ -2,90 +2,86 @@ **Date:** 2026-09-12 **Reviewer:** Claude -**Scope:** `backend/app/market/` (8 source modules) and `backend/tests/market/` (6 test files, 75 tests) +**Scope:** `backend/app/market/` (8 source modules) and `backend/tests/market/` (7 test files, 84 tests) -This is the second review pass. The first pass (findings below, in §1) surfaced six issues; all six have been fixed in this pass, along with two new regression tests locking in the two behavioral fixes. This document supersedes the review that was in this same file before those fixes; the original findings are preserved in `planning/archive/` for history. +This is the third pass on this document. The first pass found six issues (§1 below); the second pass fixed all six and added regression tests for the two behavioral bugs. This third pass closes out the remaining items that were previously left open on purpose (§2): an SSE integration test, a `PriceCache` concurrency test, and a full-10-ticker `GBMSimulator` test. The market data subsystem is now considered complete and ready for the rest of the backend to build on. --- -## 1. Fixes Applied This Pass +## 1. Issues Fixed (Second Pass) | # | Issue | Severity | Fix | |---|---|---|---| -| 1 | `MassiveDataSource.start()` didn't normalize ticker case, but `add_ticker`/`remove_ticker` did — a ticker passed to `start()` in lowercase could never be removed later (reproduced: `remove_ticker("aapl")` silently failed to remove `"aapl"` when `start()` had stored it un-normalized). | Medium | `start()` now does `[t.upper().strip() for t in tickers]`, matching `add_ticker`/`remove_ticker`. Added `test_start_normalizes_ticker_case` to lock this in. | -| 2 | `GBMSimulator`'s `dt` was hardcoded to assume a 500ms tick regardless of `SimulatorDataSource.update_interval` — changing the interval would silently scale the effective annualized volatility with no warning. | Medium | `SimulatorDataSource.start()` now derives `dt = self._interval / GBMSimulator.TRADING_SECONDS_PER_YEAR` and passes it explicitly. Added `test_dt_scales_with_update_interval` to lock this in. | -| 3 | `stream.py` built its `APIRouter` at module scope and decorated onto it inside `create_stream_router()` — calling the factory twice (e.g., across tests, or a future reload path) would double-register `/prices` on the same shared router. | Low | `router = APIRouter(...)` moved inside `create_stream_router()`, so each call gets its own router. | -| 4 | `PriceCache.version` was read without `self._lock`, unlike every other accessor. | Low | **Not changed** — re-assessed as not worth it; see §5. | -| 5 | `tests/conftest.py` defined an `event_loop_policy` fixture that just returned the default policy, contributing a `DeprecationWarning` on every async test run (`asyncio.DefaultEventLoopPolicy` is deprecated, slated for removal in Python 3.16). | Trivial | Fixture removed; `conftest.py` is now just the module docstring. | -| 6 | `ruff format --check` flagged 3 test files as not matching the formatter's line-wrapping. | Trivial | Ran `ruff format` on the affected files (and once more on `stream.py` after edit #3 left it missing a blank line). | - -Item 4 is intentionally left as-is — see §5 for reasoning. +| 1 | `MassiveDataSource.start()` didn't normalize ticker case, but `add_ticker`/`remove_ticker` did — a ticker passed to `start()` in lowercase could never be removed later. | Medium | `start()` now does `[t.upper().strip() for t in tickers]`. Locked in by `test_start_normalizes_ticker_case`. | +| 2 | `GBMSimulator`'s `dt` was hardcoded to assume a 500ms tick regardless of `SimulatorDataSource.update_interval`. | Medium | `SimulatorDataSource.start()` derives `dt = self._interval / GBMSimulator.TRADING_SECONDS_PER_YEAR`. Locked in by `test_dt_scales_with_update_interval`. | +| 3 | `stream.py` built its `APIRouter` at module scope; calling `create_stream_router()` twice would double-register `/prices`. | Low | `router = APIRouter(...)` now built inside `create_stream_router()`. Locked in by `test_builds_independent_routers`. | +| 4 | `PriceCache.version` read without `self._lock`. | Low | Left as-is — see §4. | +| 5 | `tests/conftest.py`'s `event_loop_policy` fixture was a deprecated no-op, producing a `DeprecationWarning` on every async test. | Trivial | Fixture removed. | +| 6 | Formatting drift flagged by `ruff format --check` in several test files. | Trivial | `ruff format` applied. | --- -## 2. Test Results (after fixes) - -**75 tests collected, 75 passed, 0 failed.** (`uv run pytest -q --cov=app --cov-report=term-missing`, with `massive==2.2.0` actually installed via `uv sync --extra dev`.) +## 2. Improvements Added (This Pass) -Two tests were added this pass (73 → 75): `test_start_normalizes_ticker_case` (massive) and `test_dt_scales_with_update_interval` (simulator source). No `DeprecationWarning`s remain in the run. +These were the items previously listed as "worth doing, not urgent" / "deliberately not fixed" because they needed infrastructure the subsystem didn't have yet at the time (an ASGI test harness) or were judged lower value. All three are now done: -| Module | Coverage | Notes | -|---|---|---| -| models.py | 100% | | -| cache.py | 100% | | -| interface.py | 100% | | -| seed_prices.py | 100% | | -| factory.py | 100% | | -| simulator.py | 98% | Uncovered: L149 duplicate-add guard, L273-274 exception path in `_run_loop` | -| massive_client.py | 94% | Uncovered: `_poll_loop`'s `while True` body (L85-87), real (unmocked) `_fetch_snapshots` body (L125) | -| stream.py | 31% | Still untested — needs an ASGI test client, not added this pass (see §5) | -| **Total** | **91%** | Unchanged from before — the new tests cover previously-untested *behavior*, not previously-uncovered *lines* | +### 2.1 SSE integration test for `stream.py` (was 31% coverage, no tests) -**Lint:** `ruff check app/ tests/` — clean. `ruff check --select F401` (unused imports) — clean. +Getting this right took a real detour worth recording: the natural first attempt — `httpx.ASGITransport` and, separately, FastAPI's `TestClient` — both **deadlock** against this endpoint. `_generate_events` is an unbounded `while True` loop that only exits when it observes `request.is_disconnected()`. Both of those test clients fully run the ASGI call to completion (buffering the entire response) *before* handing anything back to the caller to consume — there is no mechanism for the client to signal a disconnect mid-stream, so the server-side generator never sees one and the client never gets anything back. Confirmed this empirically with `faulthandler`-dumped stack traces showing both hung inside the initial `send()`/`handle_request()` call, before the streaming body was ever reached. -**Format:** `ruff format --check app/ tests/` — clean, all 19 files formatted (was 3 files + `stream.py` dirty before this pass). +The fix was to test `_generate_events` directly: a minimal fake `Request` (just `.client.host` and a controllable `is_disconnected()`) drives the async generator with `__anext__()`, so the test controls disconnection deterministically instead of depending on transport-level streaming semantics that don't exist in either test client. `create_stream_router()`'s route-building and the endpoint's `StreamingResponse`/headers are tested separately by calling the routed endpoint function directly (via `router.routes[0].endpoint`) without consuming its body. ---- +New file: `tests/market/test_stream.py`, 7 tests: +- Router factory builds independent routers per call (locks in fix #3 above) +- Endpoint returns a `StreamingResponse` with the right media type and headers +- First event is the `retry: 1000` directive +- Initial data event reflects whatever's already in the cache +- A cache update after the connection opens streams through as a new event +- No data event is ever produced while the cache stays empty across several ticks +- The generator stops (raises `StopAsyncIteration`) on disconnect -## 3. Architecture Assessment +`stream.py` coverage: **31% → 94%** (only the `asyncio.CancelledError` logging branch remains uncovered — that requires actually cancelling the task rather than a clean disconnect, which is a real server-shutdown path, not something worth engineering a test around). -Unchanged from the prior review: this remains a clean strategy-pattern implementation (`MarketDataSource` ABC → `SimulatorDataSource` / `MassiveDataSource`) writing into a single shared, thread-safe `PriceCache`, matching `planning/PLAN.md` §6. The fixes in this pass were surgical — no structural changes, no new modules, no altered public API shapes. `GBMSimulator.__init__` already accepted a `dt` parameter; the fix just wires the caller to pass the right value instead of relying on the default. +Added `httpx>=0.27.0` to `[project.optional-dependencies].dev` — it's what `fastapi.testclient.TestClient` needs even though it isn't used directly in the final tests; harmless to keep since it's dev-only and a natural fit for future API testing. ---- +### 2.2 `PriceCache` concurrent-writers test -## 4. Re-Verification of Everything From the Original (2026-02-10) Review +`tests/market/test_cache.py::test_concurrent_updates_are_not_lost` spins up 8 real OS threads (matching how the cache is actually used — `MassiveDataSource` calls into it via `asyncio.to_thread`), each performing 200 `update()` calls across 10 shared tickers, then asserts `cache.version` exactly equals `8 * 200 = 1600`. This is a meaningful assertion, not a smoke test: a broken lock (or a `+=` race) would show up here as a version count *less than* 1600 — a lost update — with high probability under real thread interleaving. Ran the full suite three times back-to-back to confirm no flakiness. -For completeness, since this file has now gone through two review passes beyond the original: +### 2.3 Full 10-ticker `GBMSimulator` test -| Original finding | Status | -|---|---| -| `pyproject.toml` missing wheel packaging config | Fixed (prior pass) | -| Massive tests fragile without `massive` installed | Fixed (prior pass) — confirmed again this pass, ran with `massive` installed | -| `_generate_events` return type annotation | Fixed (prior pass) | -| `PriceCache.version` not under lock | Still open — see §5, deliberately not fixed | -| `SimulatorDataSource.get_tickers` accessed private state | Fixed (prior pass) | -| Module-level `router` in `stream.py` | **Fixed this pass** | -| Unused imports in tests | Fixed (prior pass) | -| Missing SSE integration test | Still open — see §5 | -| No `PriceCache` concurrency test | Still open — see §5 | -| No full-10-ticker `GBMSimulator` test | Still open, but manually verified working (no numerical issue) | -| `DEFAULT_CORR` vs `CROSS_GROUP_CORR` naming | Fixed (prior pass) | +`tests/market/test_simulator.py::test_full_default_watchlist_builds_valid_cholesky` builds a `GBMSimulator` with all 10 tickers from `SEED_PRICES` (mixing the tech group, the finance group, and TSLA's special-cased correlation all at once — the case none of the existing 1-2 ticker tests exercised), asserts the Cholesky decomposition is a proper 10×10 matrix, and runs 50 steps confirming all tickers stay present and positive. This had already been manually verified working in the first review pass; it's now a permanent regression test. --- -## 5. Remaining Open Items (Deliberately Not Fixed) +## 3. Test Results (Final) -None of these are regressions or newly discovered problems — they're the same low-priority items from before, re-assessed and left open on purpose: +**84 tests collected, 84 passed, 0 failed.** (`uv run pytest -q --cov=app --cov-report=term-missing`, `massive` installed via `uv sync --extra dev`.) Verified stable across 3 consecutive runs. -- **`PriceCache.version` unlocked read.** A single `int` read is atomic under CPython's GIL, and this project targets standard CPython, not a no-GIL build. Adding a lock here would be defensive code against a scenario that doesn't apply; not worth the (tiny) overhead or the inconsistency of "sometimes we lock, sometimes the JIT/GIL makes it safe anyway." -- **No SSE integration test for `stream.py`.** Testing this properly needs an ASGI test client (`httpx` + `ASGITransport`, or FastAPI's `TestClient`), which isn't currently a dependency, and there's no FastAPI `app` yet to mount the router into — that's the next phase of backend work, not this one. Adding a dependency and a test harness for a router that isn't wired into a real app yet would be premature; revisit once `main.py`/the FastAPI app exists. -- **No `PriceCache` concurrent-writers test.** The lock usage is straightforward (one `Lock`, held for the full duration of every method) and inspection gives high confidence it's correct. A multi-threaded stress test would mostly be testing Python's `threading.Lock`, not this code's logic. -- **No test with the full 10-ticker default set.** Manually verified this pass (again) that `GBMSimulator(tickers=list(SEED_PRICES.keys()))` builds a valid 10×10 Cholesky decomposition and steps cleanly. Still a coverage gap worth closing eventually, but it's a "nice to have," not a bug. +| Module | Coverage | Notes | +|---|---|---| +| models.py | 100% | | +| cache.py | 100% | | +| interface.py | 100% | | +| seed_prices.py | 100% | | +| factory.py | 100% | | +| simulator.py | 98% | Uncovered: L149 duplicate-add guard, L273-274 exception path in `_run_loop` | +| massive_client.py | 94% | Uncovered: `_poll_loop`'s `while True` body, real (unmocked) `_fetch_snapshots` body | +| stream.py | 94% | Uncovered: `asyncio.CancelledError` logging branch (server-shutdown path) | +| **Total** | **97%** | Up from 91% at the start of this pass | + +**Lint:** `ruff check app/ tests/` — clean. +**Format:** `ruff format --check app/ tests/` — clean, all 20 files formatted. +**Dependency sanity:** `uv sync` (prod-only) and `uv sync --extra dev` both verified to install cleanly from a fresh lockfile resolution. --- -## 6. Verdict +## 4. Remaining Open Item + +- **`PriceCache.version` unlocked read.** Still deliberately left as-is. A single `int` read is atomic under CPython's GIL, this project targets standard CPython, and adding a lock here would be defensive code against a scenario (a no-GIL Python build) this project doesn't target. Re-affirmed in this pass; no plan to change unless the project's Python target changes. + +--- -All six issues identified in the first review pass are fixed, verified by a clean 75-test run, clean lint, and clean format check. The two behavioral fixes (ticker-case normalization, `dt`/`update_interval` coupling) now have dedicated regression tests so they can't silently regress. Nothing here blocks moving on to the rest of the backend (portfolio, watchlist, chat, and the FastAPI app that will actually mount `create_stream_router()`). +## 5. Verdict -No further action needed on the market data subsystem before that next phase begins. +The market data backend is complete, tested, and ready. All issues from both prior review passes are resolved except the one item in §4, which is a deliberate judgment call rather than an oversight. Coverage is 97% overall, with every module except the two background polling loops (whose bodies are `await asyncio.sleep()` + a call already tested directly) above 90%. Nothing here should block building the rest of the backend — portfolio, watchlist, chat, and the FastAPI `app` that will mount `create_stream_router()` for real.